fix(web): keep $ skill picker usable inside question answers - #8359
fix(web): keep $ skill picker usable inside question answers#8359NeilTheFisher wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| setComposerTrigger( | ||
| cursorAdjacentToMention ? null : detectComposerTrigger(nextPrompt, expandedCursor), | ||
| ); | ||
| setComposerTrigger(detectComposerTrigger(nextPrompt, expandedCursor)); |
There was a problem hiding this comment.
🟡 Medium chat/ChatComposer.tsx:1757
Normal ArrowLeft/ArrowRight navigation immediately after an inserted $skill chip reopens the skill trigger menu, even though the user only moved across the immutable token. onPromptChange receives cursorAdjacentToMention for this case, so keep the guard and suppress trigger detection when that flag is set.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/ChatComposer.tsx around line 1757:
Normal ArrowLeft/ArrowRight navigation immediately after an inserted `$skill` chip reopens the skill trigger menu, even though the user only moved across the immutable token. `onPromptChange` receives `cursorAdjacentToMention` for this case, so keep the guard and suppress trigger detection when that flag is set.
| // showing old value: the controlled effect (activePendingProgress.customAnswer | ||
| // -> composer) will rewrite editor to new value and place cursor. Calling | ||
| // focusAt now would move selection in old content (clamped) and fire | ||
| // handleEditorChange with old value, overwriting the just-inserted skill. |
There was a problem hiding this comment.
🟡 Medium components/ChatView.tsx:6149
The isPlainInsert guard only recognises appended text (value.startsWith(snapshot.value)). When a skill picker replaces the typed query (e.g. $br → $Browser ), isPlainInsert is false, so focusAt(nextCursor) fires against the editor's stale $br snapshot. That synchronously re-emits onChange with the old value, overwriting the pending-answer state update that was just queued — the selected skill never appears.
The comment above the guard already states the real invariant: skip focusAt whenever the editor shows a different value than the one being written, because the controlled effect will rewrite the editor. Broaden the check to snapshot.value !== value instead of the append-only heuristic.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ChatView.tsx around line 6149:
The `isPlainInsert` guard only recognises appended text (`value.startsWith(snapshot.value)`). When a skill picker **replaces** the typed query (e.g. `$br` → `$Browser `), `isPlainInsert` is `false`, so `focusAt(nextCursor)` fires against the editor's stale `$br` snapshot. That synchronously re-emits `onChange` with the old value, overwriting the pending-answer state update that was just queued — the selected skill never appears.
The comment above the guard already states the real invariant: skip `focusAt` whenever the editor shows a different value than the one being written, because the controlled effect will rewrite the editor. Broaden the check to `snapshot.value !== value` instead of the append-only heuristic.
There was a problem hiding this comment.
UI Consistency: composer answer-mode review
Four findings, all in changed lines. The main one is that the new plainText mode added to the shared ComposerPromptEditor primitive is never enabled by any call site, while several behavioral guards around it were relaxed in the live (rich) path.
apps/web/src/components/chat/ChatComposer.tsx:3495—plainText={false}makes the entire new editor mode unreachable, and the state comment at 1028-1031 now describes behavior that does not happen.apps/web/src/components/chat/ChatComposer.tsx:1757— dropping thecursorAdjacentToMentionsuppression only in the pending-answer branch makes chip-adjacent caret behavior diverge from the draft branch in the same rich editor.apps/web/src/components/chat/ChatComposer.tsx:1527-1533— capture-phasewindowEscape handler withstopPropagation()swallows Escape app-wide whenever a composer trigger is active.apps/web/src/components/ChatView.tsx:6150-6154— theisPlainInsertheuristic only recognizes appends, so mid-text insertion still hits the ping-pong it is meant to prevent.
Posted via Macroscope — UI Consistency
| : [] | ||
| } | ||
| skills={selectedProviderStatus?.skills ?? []} | ||
| plainText={false} // chip even in custom-answer (user wants chip not $text) |
There was a problem hiding this comment.
plainText is hardcoded to false here, and the only other consumer (SettingsFontPreviews) takes the default false — so the whole new mode in ComposerPromptEditor (the plain/rich LexicalComposer key, the token-plugin gating, the three cursor-mapping swaps, restoreFocusOnRemountRef and the render-phase initial*Ref mutations) is unreachable in the shipped app. That leaves a fairly large unexercised branch on a shared primitive, plus behavior that now runs unconditionally in the rich path (the remount focus-restore layout effect, refs written during render) with no consumer that needs it.
Suggest either wiring a real consumer for plainText or dropping the mode and keeping only the parts this fix actually needs (plainAnswerMode draft parking). Related: the state comment at lines 1028-1031 ("the editor renders raw text (no inline tokens) and the trigger menus stay closed") contradicts this call site — chips render and the trigger menus are explicitly kept working — so it should be corrected either way.
Posted via Macroscope — UI Consistency
| const onKeyDown = (event: KeyboardEvent) => { | ||
| if (event.key !== "Escape") return; | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| setComposerTrigger(null); | ||
| setComposerHighlightedItemId(null); | ||
| }; |
There was a problem hiding this comment.
This Escape handler is a capture-phase window listener that unconditionally preventDefault() + stopPropagation() while composerMenuOpen. Since the trigger is derived from composer text, it can stay open while focus moves elsewhere (model picker Select, a popover, a dialog), and in that state the composer eats Escape before any Base UI popup or global handler sees it — a keyboard-dismissal regression outside the composer.
Suggest scoping the handler to the composer (and its floating layer) so other surfaces keep their Escape:
const onKeyDown = (event: KeyboardEvent) => {
if (event.key !== "Escape") return;
+ const target = event.target as Element | null;
+ if (
+ target &&
+ !target.closest('[data-chat-composer-form="true"]') &&
+ !isInsideComposerFloatingLayer(target)
+ ) {
+ return;
+ }
event.preventDefault();Worth noting ComposerStashMenu already owns its own dismissal via an onClose prop; owning dismissal in the menu component would keep this behavior with the popup rather than in a global listener.
Posted via Macroscope — UI Consistency
| const isPlainInsert = | ||
| snapshot !== undefined && | ||
| typeof snapshot.value === "string" && | ||
| value.length > snapshot.value.length && | ||
| value.startsWith(snapshot.value); |
There was a problem hiding this comment.
isPlainInsert only matches insertions appended to the end (value.startsWith(snapshot.value)). A skill inserted with the caret mid-answer produces a value that is not a prefix-extension, so focusAt still runs against the stale editor content and the ping-pong this guard describes can still overwrite the insertion. (typeof snapshot.value === "string" is also dead — value is typed string on the snapshot.)
Suggest keying off an explicit signal that the change came from a programmatic replacement (e.g. a flag threaded from applyPromptReplacement, or having the editor handle acknowledged controlled writes) rather than inferring it from string shape, so mid-text inserts are covered too.
Posted via Macroscope — UI Consistency
| setComposerTrigger( | ||
| cursorAdjacentToMention ? null : detectComposerTrigger(nextPrompt, expandedCursor), | ||
| ); | ||
| setComposerTrigger(detectComposerTrigger(nextPrompt, expandedCursor)); |
There was a problem hiding this comment.
Because the editor still renders chips in answer mode (plainText={false}), cursorAdjacentToMention is still meaningful here. Removing the suppression means parking the caret directly beside an already-committed $skill/@file chip in the custom-answer field re-opens the picker with the committed token as the query, while the draft branch below (line 1776) still suppresses it — the same editor now behaves differently depending on whether a question is open.
Suggest keeping the guard in both branches:
| setComposerTrigger(detectComposerTrigger(nextPrompt, expandedCursor)); | |
| setComposerTrigger( | |
| cursorAdjacentToMention ? null : detectComposerTrigger(nextPrompt, expandedCursor), | |
| ); |
Posted via Macroscope — UI Consistency
2 participants
Alternative to #7818 which hides $/slash pickers in question answers. Fixes #8128 by keeping the picker usable.
Verified via Chrome MCP in pending and normal threads, no stack overflow.
Note
Keep
$skill picker usable inside question answers by adding plainText editor modependingAnswerChangecalledfocusAtduring plain text inserts, causing onChange to overwrite just-inserted text from the skill picker.plainTextmode to ComposerPromptEditor.tsx that disables token plugins, uses simple cursor clamping instead of token-aware mapping, and remounts the editor on mode flips while restoring focus.draftPromptRefwhile a pending question is active, preventing trait updates and stash restores from clobbering the visible answer or its caret. On question resolution, the draft is restored with caret and trigger reset.plainTextprop toggles cause a full editor remount viaLexicalComposerkey change ('-plain'vs'-rich'); verify focus restoration works across remounts inComposerPromptEditorInnerviarestoreFocusOnRemountRef.📊 Macroscope summarized efd8141. 3 files reviewed, 4 issues evaluated, 2 issues filtered, 2 comments posted
🗂️ Filtered Issues
apps/web/src/components/ChatView.tsx — 1 comment posted, 2 evaluated, 1 filtered
applyPromptReplacementoverwritesdraftPromptRef.currentwith the pending question answer whenplainAnswerModeis active. Selecting a$skill (or another picker item) while answering therefore replaces the parked pre-question composer draft; when the question resolves, the handoff restores that answer as the normal draft instead of the user's original unsent message. [ Skipped comment generation ]apps/web/src/components/chat/ChatComposer.tsx — 1 comment posted, 2 evaluated, 1 filtered
applyPromptReplacementoverwritesdraftPromptRef.currentwith the pending answer wheneverplainAnswerModeis active. Selecting a$skill in a question answer follows theactivePendingQuestionbranch and updates only the answer, but this assignment replaces the parked pre-question draft; when the question ends, the handoff at lines 1565-1568 restores the answer text as the composer draft and loses the user's original unsent draft. [ Cross-file consolidated ]