diff --git a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/prompt-recall.test.ts b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/prompt-recall.test.ts new file mode 100644 index 00000000..69bbad15 --- /dev/null +++ b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/prompt-recall.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it } from 'bun:test'; + +/* + The rule, as a pure function, because the interesting part is not the DOM. + + History must never clobber text the person typed, and must still be walkable once a recalled prompt is + sitting in the box. Those two pull in opposite directions and the second is what a naive "only when empty" + check breaks. +*/ +const mayRecall = (value: string, recalling: boolean): boolean => value.trim() === '' || recalling; + +describe('when history may take the arrow key', () => { + it('takes it on an empty composer', () => { + expect(mayRecall('', false)).toBe(true); + }); + + // The bug this rule exists for: the caret is on the first line of a one-line draft, so every position + // test passes, and Up used to replace what you were writing. + it('leaves a draft alone', () => { + expect(mayRecall('half a thought', false)).toBe(false); + }); + + // Without this, you could step back exactly once and then be refused, because the recalled prompt is + // itself text in the box. + it('keeps walking once it is in the history', () => { + expect(mayRecall('a prompt from earlier', true)).toBe(true); + }); + + it('treats whitespace as empty', () => { + expect(mayRecall(' ', false)).toBe(true); + expect(mayRecall('\n\t ', false)).toBe(true); + }); + + it('protects a draft that merely starts with whitespace', () => { + expect(mayRecall(' still typing', false)).toBe(false); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts index 012feec5..5420fc92 100644 --- a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts +++ b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts @@ -389,15 +389,26 @@ export function useEmbeddableChat(params: UseEmbeddableChatParams, onMessageComp /** * Recall a previous prompt, the way a shell does. * - * Up only reaches the history from the FIRST line and Down only from the last, so in a multi-line draft - * the arrows still move the caret — there is a line above to go to, and taking the key would strand the - * cursor. On the first line there is nowhere up to go, which is exactly when history is what you meant. - * `step` returning null (empty history, or already at the oldest) leaves the key alone too. + * **Only from an empty composer.** The caret-on-the-first-line test below is not enough on its own, and + * that was the bug: with a one-line draft you were typing, the caret IS on the first line, so Up replaced + * what you had written with something from last week. A draft is not recoverable from anywhere, and + * history is — so the tie goes to the draft, always. + * + * `isRecalling` is what keeps the rule from making the feature useless. Once a recalled prompt is in the + * box the box is not empty, so a plain emptiness test would allow one step back and then refuse; walking + * further is still allowed because that text came from the history rather than from you. + * + * Whitespace counts as empty. A composer holding two spaces is not a draft anybody would miss, and an + * invisible character silently blocking Up is worse than losing it. + * + * The line tests stay on top of that: in a recalled prompt spanning three lines, Up from the middle moves + * the caret, because there is a line above to go to and taking the key would strand the cursor. */ const recallPrompt = (ev: KeyboardEvent, direction: -1 | 1): void => { const field = ev.currentTarget; const { selectionStart, selectionEnd, value } = field; if (selectionStart !== selectionEnd) return; // a selection means the arrows are extending/collapsing it + if (value.trim() !== '' && !history.isRecalling()) return; const onEdge = direction === -1 ? !value.slice(0, selectionStart).includes('\n') : !value.slice(selectionEnd).includes('\n'); if (!onEdge) return; diff --git a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/usePromptHistory.ts b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/usePromptHistory.ts index 2daeeb55..2c0d1acc 100644 --- a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/usePromptHistory.ts +++ b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/usePromptHistory.ts @@ -25,6 +25,15 @@ export type PromptHistory = { * whatever it normally does. */ step: (direction: -1 | 1, current: string) => string | null; + /** + * Are we currently walking the history, rather than composing? + * + * The caller needs this to answer "may Up take this key" without also making the history unusable. The + * rule is that history must never clobber text the PERSON typed — but once a recalled prompt is in the + * box, the box is not empty any more, and a plain emptiness test would allow exactly one step back and + * then refuse. This is the difference between "there is text" and "there is text of yours". + */ + isRecalling: () => boolean; }; export function usePromptHistory(): PromptHistory { @@ -87,5 +96,7 @@ export function usePromptHistory(): PromptHistory { return list[at - 1] ?? null; }, []); - return { record, step }; + const isRecalling = useCallback(() => indexRef.current !== null, []); + + return { record, step, isRecalling }; }