Skip to content

fix(conversation): keep map and marker focus, skip empty thread input - #145

Open
pratikbuilds wants to merge 1 commit into
corbitsdev:mainfrom
pratikbuilds:claude/i142-conversation-focus-85c233
Open

pratikbuilds wants to merge 1 commit into
corbitsdev:mainfrom
pratikbuilds:claude/i142-conversation-focus-85c233

Conversation

@pratikbuilds

Copy link
Copy Markdown
Collaborator

Summary

Focus fixes for the conversation map and the earlier-messages marker, plus a thread slot fix (findings 5, 6, 7 and 19).

  • 5, ConversationMap keys: Arrow, Home and End scroll only the rail to the next tick and focus it with preventScroll, so a clipped tick no longer scrolls the page or another ancestor.
  • 6, map hiding: when the map hides while a tick has focus, focus moves to the rail's tab stop or the conversation log instead of falling to the body.
  • 7, ConversationEarlier: the retry focus flag clears when the marker unmounts (a retry that ends the history), so a later stall or load cannot pull focus back to it.
  • 19, ConversationThread: boolean children, as {show && <PromptInput />} passes, no longer render an empty input slot.

Verification

  • bun run check
  • bun run test:e2e, with the new rail-scroll and hide-with-focus cases in e2e/conversation-map.spec.ts
  • The new conversation-earlier test keeps stalled true while the history ends and returns, and fails without hasEarlier in the effect's deps.

Closes #142

Addresses findings 5, 6, 7 and 19 of corbitsdev#142.

- 5: ConversationMap's Arrow/Home/End scroll only the rail to the next
  tick and focus it with preventScroll, so a clipped tick no longer
  scrolls the page or another ancestor.
- 6: when the map hides with the focused tick's turn still present, focus
  now moves to the conversation log instead of falling to the body.
- 7: ConversationEarlier clears its retry focus flag when the marker
  unmounts, so a later stall or load cannot pull focus to it.
- 19: ConversationThread leaves out the input slot for boolean children,
  as `{show && <PromptInput />}` passes.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review · Request changes

Summary

ConversationMap, ConversationEarlier, and ConversationThread fixes for #142 (items 5, 6, 7, and 19) pass bun run check, bun run dep-guard, and Playwright e2e/conversation-map.spec.ts. One should-fix in ConversationMap where hiding the map while a tick is previewed leaves preview state set and reopens a stale tooltip when the map returns.

Blockers

None.

Should-fix

  • src/ui/conversation-map.tsx:163 (reviewer, designer) — When hidden (turns.length < 2) becomes true while a tick is focused (or pointer-previewed), <nav> unmounts without firing onBlur or onPointerLeave. The layout effect calls setFocusedId(null) and moves focus to scrollContainer(), but never clears preview. As a result, previewing (preview !== null at line 178) stays true (keeping the document Escape listener attached while hidden), and when a second turn arrives (turns.length >= 2) while focus remains on the conversation log, previewed still matches the retained turn ID and immediately renders <div role="tooltip" data-slot="conversation-map-preview">. Call setPreview(null) before (stop ?? scrollContainer())?.focus({ preventScroll: true }), and clear preview when hidden or !turnIds.includes(preview.id).

Informational

  • src/ui/conversation-thread.tsx:120 (designer) — children === undefined || children === null || typeof children === "boolean" catches {show && <PromptInput />} when show is false, but multiple conditional siblings ({a && <X />}{b && <Y />} -> [false, false]) or an empty fragment still render <div data-slot="conversation-thread-input">.
  • src/ui/conversation-map.tsx:163, src/ui/conversation-earlier.tsx:98 (designer) — Focus recovery runs on prop transitions (turns.length < 2 and hasEarlier === false); if a consumer conditionally unmounts the component directly while focused, only effect cleanups run (compare ScrollToLatest in src/ui/conversation.tsx:1031).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-ups from the conversation PR review

2 participants