Skip to content

Follow-ups from the conversation PR review #142

Open

Description

@TheGreatAxios

Background

This issue collects the non-blocking follow-ups from the review of the conversation stack (#113–#129). The three merge blockers were fixed before 0.4.0: the tool-group header focus handoff (#129), MessageAction ref forwarding (#121), and scrollToItem reporting arrival after its target disappeared (#114).

The items below are real edge cases or accessibility gaps, but none was judged important enough to hold the release. False alarms found during convergence are intentionally omitted.

Findings

1. MessageParts can pull focus back after a deferred decision (#129)

Problem: decided remains armed if a person chooses Approve or Deny, moves focus to the page, and the host records the updated parts on a later turn. That later update can focus the folded activity trigger even though the person already left.

Outcome:

  • Leaving MessageParts after a decision clears the deferred focus watch.
  • A later parts update never steals focus from the page.

2. MediaViewer uses the media-element allowlist for a navigation link (#125)

Problem: safeMediaSrc(..., "media") permits data:image/svg+xml because it is safe in an <img>. The viewer reuses that value as a target="_blank" download href, where it is a document navigation rather than a media-element load.

Outcome:

  • Download links only use URL schemes appropriate for navigation/download, such as http, https, or blob.
  • Script-capable data: documents are never assigned to an anchor href.

3. PDF previews are unsandboxed (#125)

Problem: A viewer item labelled as a PDF is rendered in an iframe without a sandbox. The frame source may be http, https, or blob, and the label does not prove the response bytes are a PDF.

Outcome:

  • PDF previews cannot run same-origin script or navigate the top page.
  • Native PDF viewing continues to work, or invalid/non-PDF responses are refused.

4. An out-of-range media index can reopen itself (#125)

Problem: When the open item falls off a shortened list, the rendered index becomes null, but the stored index is not cleared. If the list grows again, the modal reopens without a new open() call.

Outcome:

  • Falling outside the list commits the viewer to a closed state.
  • Controlled callers receive one onIndexChange(null) notification.
  • Growing the list does not reopen the viewer.

5. ConversationMap keyboard focus may scroll the page (#123)

Problem: Arrow, Home, and End move focus with focus() rather than focus({ preventScroll: true }). A clipped tick can make the browser scroll an ancestor or the page, even though rail movement is intended to stay local.

Outcome:

  • Keyboard movement keeps window.scrollY and non-rail ancestor scroll positions unchanged.
  • The rail itself still keeps the focused tick visible.

6. ConversationMap can drop focus when the rail hides (#123)

Problem: If the map shrinks to the single turn whose tick is focused, the rail unmounts but the recovery effect returns early because that id is still present. Focus falls to document.body.

Outcome:

  • When the focused rail disappears, focus moves to the conversation log whether or not the turn id remains.

7. ConversationEarlier can reuse a stale retry focus flag (#122)

Problem: If “Load earlier messages” causes the marker to unmount in the same commit, the retried flag survives because the layout effect sees a null marker. A later unrelated stall transition can focus the marker.

Outcome:

  • Unmounting the marker clears the retry focus state.
  • A later load or stall transition does not pull focus back unexpectedly.

8. ConversationEarlier can submit an ancestor form (#122)

Problem: The stalled “Load earlier messages” control has no button type, so HTML treats it as submit inside a host form.

Outcome:

  • The control is type="button".
  • Activating it only calls onReachStart and never submits an ancestor form.

9. Failed-message Retry can submit an ancestor form (#121)

Problem: The Retry control in SendStatusLine also has no type, so retrying a message can submit a surrounding composer form.

Outcome:

  • Retry is type="button".
  • Activating Retry calls onRetry without submitting the host form.

10. A live ToolGroup counts failures twice (#120)

Problem: The live “done” count is calculated as count - active - waiting, which includes errors. The header then appends the same errors again as “failed,” for example 3 done · 1 failed among three non-running calls.

Outcome:

  • Live counts partition calls into active, waiting, successful/done, and failed without overlap.

11. A streaming Activity can unmount the focused step (#120)

Problem: ActivitySteps mounts only the latest ten. When a new step arrives, the oldest visible step can unmount while focus is inside it, dropping focus to the page.

Outcome:

  • Sliding the latest-ten window keeps focus inside the activity run.
  • The destination is predictable and has an accessible name.

12. Closed model and mode pickers omit the selected value from their name (#118)

Problem: aria-label="Model" or aria-label="Mode" replaces the visible selected short name. A screen reader hears the field label but not the current choice until the list opens.

Outcome:

  • The closed combobox name includes both the field label and selected value.
  • Browser tests query the combined accessible name rather than visible text separately.

13. IME Escape can close a host-opened prompt popup (#118)

Problem: useClaimEscape does not ignore isComposing. Escape intended to dismiss an IME candidate can close a model, mode, or add popup and prevent the key from reaching the editor.

Outcome:

  • A composing Escape is left to the IME and does not close a prompt popup.

14. Prompt popups can reopen when options return (#118)

Problem: When items, models, or modes becomes empty, the popup returns null without clearing open. Restoring options makes the popup reappear without a new user action.

Outcome:

  • An empty option list closes the popup and updates controlled state once.
  • Restoring options leaves it closed.

15. Aborting a panel drag leaves an uncommitted width (#116)

Problem: Cancelling an in-flight drag tears down listeners but does not restore --resizable-width. The panel can display the preview width while React state still holds the committed width.

Outcome:

  • Every drag abort restores the committed CSS width, or explicitly commits the preview.
  • CSS and controlled state cannot diverge.

16. Switching keep-mounted panel panes can lose focus (#116)

Problem: The outgoing pane becomes inert without moving focus. Real browsers move focus out of the inert subtree, often to the page, while the incoming pane receives nothing.

Outcome:

  • Switching panes while focus is inside the outgoing pane leaves focus inside the panel, preferably in the incoming pane or on a named panel control.

17. An empty prompt token can freeze highlighting (#115)

Problem: splitPromptSegments advances by token.length. An empty token matches at a legal boundary and advances by zero, creating an infinite render-path loop. The normal token contract does not create empty tokens, but host input can.

Outcome:

  • Empty token strings are skipped or rejected before scanning.
  • splitPromptSegments always advances and returns for arbitrary host input.

18. The full-chat story Add menu does not open suggestions (#127)

Problem: The story inserts @ or / and focuses the textarea, but does not place the hook’s caret inside the token. Mention and command suggestions remain closed until another edit or selection.

Outcome:

  • Choosing “Mention an agent” or “Use a command” opens the matching suggestion list immediately.

19. ConversationThread renders an empty input tray for boolean children (#126)

Problem: The input slot is omitted only for null and undefined. The common {show && <PromptInput />} pattern passes false, so React renders no child but the padded wrapper remains.

Outcome:

  • Boolean children do not mount the input slot.
  • A real child still mounts the tray unchanged.

20. React 18 root refs need one library-wide decision

Problem: The package accepts React 18 and React 19, but several conversation components rely on React 19’s ref-as-a-prop behavior. Under React 18 those root refs stay null. This is not specific to ConversationThread; the stack uses the pattern across Conversation, ConversationEarlier, ConversationMap, ConversationContent, and ConversationItem.

Outcome:

  • Either forward refs consistently for all public components that promise root refs on React 18, with React 18 coverage, or drop React 18 from the peer range.
  • Do not patch only one component and leave the family inconsistent.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions