[shaohuzhang1]

feat: chat speak

[shaohuzhang1]

Here is an analysis of the provided code, highlighting potential issues, irregularities, and optimization suggestions.

1. Security & Privacy Concerns

• XSS via Dynamic Icon Names: icon_pause-round_outlined and icon_speaker_outlined are hardcoded, which is safe. However, ensure that if dynamic names were ever used, they are sanitized. In this specific snippet, name bindings are static or derived from simple booleans (speaking ? ... : ...`), so XSS risk from icon names is low.
• Content Injection in answerText: window.speechSynthesis.speak(utterance) uses answerText.value. If collectAnswerText returns raw HTML or unescaped content, it could lead to injection issues if the speech synthesis backend or underlying browser implementation mishandles the string. While SpeechSynthesisUtterance treats input as plain text, ensure collectAnswerText does not expose sensitive data unintentionally.

2. Performance & Reactivity Issues

• Redundant computed Evaluation: answerText is a computed property. It will re-evaluate every time any reactive dependency changes. If props.message.content is a large object, collectAnswerText might be expensive. Consider memoizing or optimizing collectAnswerText if it performs complex parsing.
• onBeforeUnmount Leak Risk: speechSupported is checked inside stopReading(), but onBeforeUnmount(stopReading) relies on window.speechSynthesis existing. If the component unmounts before window is properly defined (e.g., during initial SSR hydration or race conditions), stopReading will throw an error because window.speechSynthesis would be undefined.
  • Fix: Add a guard in stopReading:

    function stopReading() {
      if (!speechSupported) return; // Guard against SSR or unsupported env
      if (utterance) {
        // ... cancel logic
      }
      speaking.value = false;
    }

3. Bug Risks & Logic Flaws

• Race Condition in toggleReading:

  utterance.onend = utterance.onerror = () => {
    speaking.value = false;
    utterance = undefined;
  };
  speaking.value = true;
  window.speechSynthesis.speak(utterance);

  If speechSynthesis.speak() triggers onend/onerror asynchronously very quickly (e.g., if the text is empty or error occurs immediately), speaking.value is set to false, but utterance is also set to undefined. However, if a user clicks "Stop" (toggleReading) while speaking is true, stopReading() cancels synthesis. The subsequent onend callback might fire after stopReading already cleared utterance and set speaking to false. This is generally handled by checking if (!speechSupported || !answerText.value) return at the start, but edge cases with rapid toggling may cause state desync.

  • Optimization: Check if speaking.value is already changing in the callback?
  • Better Pattern: Use a unique ID for each utterance to ignore stale callbacks.

• regenerateAnswer State Conflict:
  You have two separate loading states: messageList.streamLoading.value and messageList.loading.value. Ensure these are mutually exclusive or that canRegenerate correctly accounts for all pending operations. If streamLoading is true, regenerate is disabled, which is good. But after regeneration starts, does loading become true? Ensure the UI reflects this transition smoothly.

• Missing Error Handling for Synthesis:
  If speechSynthesis fails mid-speak (e.g., system error), onerror is called, but no user feedback (toast/error message) is provided. The UI just stops speaking silently. Consider adding a toast notification if speech fails unexpectedly.

4. Code Style & Best Practices

• Vue Composition API Best Practices:
  • ✅ Good: Using computed for derived state.
  • ✅ Good: Cleanup via onBeforeUnmount.
  • ⚠️ Suggestion: Define utterance type more strictly. Currently let utterance: SpeechSynthesisUtterance | undefined. This is fine, but consider wrapping speech logic in a Composable (e.g., useSpeechSynthesis) for reusability across components.
• Template Optimization:
  • You moved <ButtonFeedback> up in the template. Ensure the visual layout remains consistent. Moving elements in DOM without adjusting CSS flex/grid order might cause unexpected shifts.
  • ✅ Good: Conditional rendering with v-if for showShare and canFeedback avoids unnecessary DOM nodes.

5. Specific Suggestions for Improvement

A. Refactor Speech Logic into a Composable

Encapsulate speaking, utterance, stopReading, and toggleReading into a reusable hook to avoid cluttering the main component.

// composable.ts
export function useSpeech(contentRef: Ref<string>) {
  const supported = typeof window !== 'undefined' && 'speechSynthesis' in window;
  const speaking = ref(false);
  let currentUtterance: SpeechSynthesisUtterance | null = null;

  const stop = () => {
    if (supported && currentUtterance) {
      currentUtterance.onend = null;
      currentUtterance.onerror = null;
      window.speechSynthesis.cancel();
      currentUtterance = null;
    }
    speaking.value = false;
  };

  const toggle = () => {
    if (speaking.value) return stop();
    if (!supported || !contentRef.value) return;
    
    window.speechSynthesis.cancel(); // Clear queue
    const utt = new SpeechSynthesisUtterance(contentRef.value);
    utt.onend = () => { speaking.value = false; currentUtterance = null; };
    utt.onerror = () => { 
      speaking.value = false; 
      currentUtterance = null; 
      // Optionally handle error logging here
    };
    currentUtterance = utt;
    speaking.value = true;
    window.speechSynthesis.speak(utt);
  };

  onBeforeUnmount(stop);

  return { supported, speaking, toggle };
}

B. Improve canRegenerate Logic

Ensure messageList is defined before accessing its properties to avoid runtime errors if props.message is passed without context. Although guarded by &&, TypeScript might complain.

C. Accessibility (a11y)

• Add aria-label to buttons if tooltips are not sufficient for screen readers.
• Ensure aria-pressed="true" on the speak button when speaking is true.

Final Verdict

The code is structurally sound and follows Vue 3 Composition API patterns. The main improvements lie in extracting speech logic into a composable, adding guards for SSR/environment checks in cleanup, and providing better error feedback for TTS failures.

[shaohuzhang1]

The patch is syntactically valid, but there are several correctness/data-loss risks and some cleanup suggestions.

Main issues

1. reChat can clear the wrong message

   const target = opts.newQuestion
     ? messages.value[messages.value.length - 1]
     : messages.value.find(...) || messages.value[messages.value.length - 1]

   For reChat, if the record cannot be found, this falls back to the last message. The first stream chunk then clears that message’s content. That is dangerous.

   • For opts.reChat, do not fall back to the last message; return if no matching target is found.

2. Target lookup may match a non-assistant message

   (!!opts.chatRecordId && message.recordId === opts.chatRecordId) ||
   message.id === `${opts.chatRecordId}_ASSISTANT`

   The recordId branch does not verify message.role === 'ASSISTANT'. If any non-assistant message has the same recordId, you could clear the wrong message.

   • Add message.role === 'ASSISTANT'.
   • Also guard the legacy-ID branch when opts.chatRecordId is missing, to avoid matching "undefined_ASSISTANT".

3. regenerate does not verify the target is an assistant message

   if (streamLoading.value || loading.value || !message.recordId) return
   const index = messages.value.indexOf(message)
   const question = messages.value[index - 1]
   if (index < 1 || question?.role !== 'USER' || !question.content[0]) return

   If regenerate is called on a USER message with a recordId, and the previous message is also USER, this can pass and send the wrong request.

   • Add if (message.role !== 'ASSISTANT') return.
   • Use question?.content?.[0] to avoid a runtime error if content is missing.

4. recordId may not be updated on reChat

   if (chunk?.chat_record_id && !target.recordId) target.recordId = chunk.chat_record_id

   During a reChat, target.recordId is usually already set, so a new chat_record_id from the backend will be ignored. If reChat can return a new record ID, this leaves a stale ID.

   • Decide explicitly whether reChat should update it, e.g.:

     if (chunk?.chat_record_id && (!target.recordId || opts.reChat)) {
       target.recordId = chunk.chat_record_id
     }

     — but only if that matches the backend semantics.

5. Old content is cleared on the first chunk, even if that chunk has no content

   if (opts.reChat && !replacementStarted) {
     target.content = []
     target.record = undefined
     target.write_ed = false
     replacementStarted = true
   }

   If the first chunk is only metadata, a ping, an error, or an empty delta, the old answer is cleared before any replacement content arrives.

   • Clear only when the first actual content-bearing chunk arrives.
   • Consider preserving the old content and restoring it if the stream fails before meaningful content is received.

6. State reset is incomplete
   You reset content, record, and write_ed, but other streaming/error state may remain stale, e.g. error, status, finished, references, etc. Audit the ChatMessage shape and reset all fields that can affect rendering.

7. regenerate ignores the result of sendMessage
   If sendMessage returns a Promise, callers of regenerate cannot await it or catch errors.

   • Return it:

     return sendMessage(...)

   • Or make regenerate async and await it.

8. indexOf(message) relies on object identity
   If the caller ever passes a cloned or reconstructed message object, indexOf returns -1. If that is possible, use an ID-based lookup, e.g. findIndex(m => m.id === message.id).

9. Potential retry/duplicate issue
   replacementStarted is local to one sendMessage call. If runStream internally retries after a partial response, the content will not be cleared again and chunks may be appended to a partial answer. Check whether runStream supports retries.

Minor suggestions

• Simplify and guard the target lookup:

  const assistantId = opts.chatRecordId
    ? `${opts.chatRecordId}_ASSISTANT`
    : undefined
  
  const matched = opts.chatRecordId
    ? messages.value.find(
        (m) =>
          m.role === 'ASSISTANT' &&
          (m.recordId === opts.chatRecordId || m.id === assistantId),
      )
    : undefined
  
  const target = opts.newQuestion
    ? messages.value[messages.value.length - 1]
    : matched ?? (!opts.reChat ? messages.value[messages.value.length - 1] : undefined)

• If recordId is not guaranteed unique, pass message.id or the index to sendMessage so the exact message can be targeted.
• Consider naming it regenerateMessage for clarity if the store exports many actions.

Overall: the main fixes should be to prevent reChat from falling back to the last message, ensure the target is an assistant message, and clear old content only when actual replacement content arrives.

[shaohuzhang1]

The diff is syntactically valid CSS, but there are a few things to verify:

1. --spacing must be defined and must resolve to a length

   • If --spacing is undefined, calc(800px + var(--spacing) * 8) becomes invalid at computed-value time.
   • In that case, max-width falls back to none, and because width: 100% is set, the element can expand to the full parent width. That is a likely layout regression.
   • Safer:

     max-width: calc(800px + (var(--spacing, 0px) * 8));

2. box-sizing: border-box changes the width calculation

   • Before, max-width: 800px likely behaved as content-box unless a global reset already set border-box.
   • With border-box, padding and borders are included in the 800px. Increasing the max-width may be compensating for that, but only if the horizontal padding/border is exactly var(--spacing) * 8.
   • If the goal is to preserve an 800px content area, it is clearer to account for actual padding/border:

     max-width: calc(800px + var(--padding-inline) * 2);

     or keep a fixed max-width and let border-box include padding.

3. Check whether --spacing is a length, not a unitless number

   • This is valid if --spacing is something like 4px or 0.25rem.
   • This is invalid if --spacing is 4 with no unit.
   • Also consider whether using a relative unit such as rem is intentional, since the max width will then change with font size.

4. Browser support for calc() multiplication

   • Modern browsers support calc(var(--spacing) * 8).
   • If you need to support very old browsers, precompute the value or avoid multiplication in calc().

5. Readability / line length

   • max-width controls line length. Increasing it from 800px to 800px + spacing * 8 may reduce readability if this element contains long-form conversation text.

Suggested safer version, assuming --spacing is a length:

.mk-conversation-content {
  box-sizing: border-box;
  margin-inline: auto;
  max-width: calc(800px + (var(--spacing, 0px) * 8));
  width: 100%;
}

If this width is reused elsewhere, consider storing it in a custom property:

.mk-conversation-content {
  --conversation-max-width: calc(800px + (var(--spacing, 0px) * 8));

  box-sizing: border-box;
  margin-inline: auto;
  max-width: var(--conversation-max-width);
  width: 100%;
}

Main action items: verify the --spacing token’s scope/type, confirm the border-box + increased max-width combination is intentional, and add a fallback to avoid an unintended full-width layout.