-
Notifications
You must be signed in to change notification settings - Fork 70
fix(tui): show the copy-on-select confirmation as a transient notice #366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b6e2414
64ed001
cd04fa0
3b0564f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,249 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| // Copyright (c) 2026 EverMind. | ||
| // See NOTICES.md. | ||
| // | ||
| // The copy-on-select confirmation: a transient line above the composer that | ||
| // replaces itself on every new copy and clears after its duration, instead of | ||
| // stacking permanent transcript lines. | ||
|
|
||
| import { renderSync } from '@hermes/ink' | ||
| import React from 'react' | ||
| import { PassThrough } from 'stream' | ||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import type { | ||
| AppLayoutActions, | ||
| AppLayoutComposerProps, | ||
| AppLayoutProps, | ||
| AppLayoutStatusProps, | ||
| GatewayServices | ||
| } from '../app/interfaces.js' | ||
| import type { Msg } from '../types.js' | ||
|
|
||
| import { $copyNotice, dismissCopyNotice, showCopyNotice } from '../app/copyNoticeStore.js' | ||
| import { GatewayProvider } from '../app/gatewayContext.js' | ||
| import { patchUiState, resetUiState } from '../app/uiStore.js' | ||
| import { AppLayout } from '../components/appLayout.js' | ||
| import { DEFAULT_VOICE_RECORD_KEY } from '../lib/platform.js' | ||
| import { stripAnsi } from '../lib/text.js' | ||
|
|
||
| const HISTORY: Msg[] = Array.from({ length: 12 }, (_, i) => ({ | ||
| role: i % 2 === 0 ? 'user' : 'assistant', | ||
| text: `transcript line ${i} lorem ipsum` | ||
| })) | ||
|
|
||
| const actions: AppLayoutActions = { | ||
| answerApproval: () => {}, | ||
| answerClarify: () => {}, | ||
| answerConfirm: () => {}, | ||
| answerSecret: () => {}, | ||
| answerSudo: () => {}, | ||
| clearSelection: () => {}, | ||
| deleteSessionWithFallback: async () => false, | ||
| onModelSelect: () => {}, | ||
| resumeById: () => {}, | ||
| setStickyPrompt: () => {} | ||
| } | ||
|
|
||
| const status: AppLayoutStatusProps = { | ||
| cwdLabel: '~/repo', | ||
| goodVibesTick: 0, | ||
| sessionStartedAt: null, | ||
| showStickyPrompt: false, | ||
| statusColor: 'green', | ||
| stickyPrompt: '', | ||
| turnStartedAt: null, | ||
| voiceLabel: '' | ||
| } | ||
|
|
||
| const makeComposer = (): AppLayoutComposerProps => ({ | ||
| cols: 80, | ||
| compIdx: 0, | ||
| completions: [], | ||
| empty: true, | ||
| handleTextPaste: async () => null, | ||
| input: '', | ||
| inputBuf: [], | ||
| pagerPageSize: 10, | ||
| queueEditIdx: null, | ||
| queuedDisplay: [], | ||
| submit: () => {}, | ||
| updateInput: () => {}, | ||
| voiceRecordKey: DEFAULT_VOICE_RECORD_KEY | ||
| }) | ||
|
|
||
| const gwServices = { gw: {}, rpc: async () => null } as unknown as GatewayServices | ||
|
|
||
| const makeProps = (): AppLayoutProps => ({ | ||
| actions, | ||
| composer: makeComposer(), | ||
| mouseTracking: false, | ||
| progress: { showProgressArea: false }, | ||
| status, | ||
| transcript: { | ||
| historyItems: HISTORY, | ||
| scrollRef: { current: null }, | ||
| virtualHistory: { | ||
| bottomSpacer: 0, | ||
| end: HISTORY.length, | ||
| measureRef: () => () => {}, | ||
| offsets: HISTORY.map((_, i) => i), | ||
| start: 0, | ||
| topSpacer: 0 | ||
| }, | ||
| virtualRows: HISTORY.map((msg, index) => ({ index, key: `r${index}`, msg })) | ||
| } | ||
| }) | ||
|
|
||
| const App = () => ( | ||
| <GatewayProvider value={gwServices}> | ||
| <AppLayout {...makeProps()} /> | ||
| </GatewayProvider> | ||
| ) | ||
|
|
||
| // The renderer skips blank cells by moving the cursor with CSI sequences | ||
| // instead of writing spaces, so 'copied 7 characters' arrives as | ||
| // 'copied<ESC>[1C7<ESC>[1Ccharacters'. Turn those into spaces before | ||
| // stripping the rest of the ANSI, so plain-text assertions see real gaps. | ||
| const cursorForward = new RegExp(`${String.fromCharCode(27)}\\[(\\d+)?C`, 'g') | ||
|
|
||
| const toPlainFrame = (raw: string): string => | ||
| stripAnsi(raw.replace(cursorForward, (_, n) => ' '.repeat(n ? parseInt(n, 10) : 1))) | ||
|
|
||
| const renderFrame = ({ setup }: { setup?: () => void } = {}): string => { | ||
| resetUiState() | ||
| patchUiState({ statusBar: 'bottom' }) | ||
| setup?.() | ||
|
|
||
| const stdout = new PassThrough() | ||
| const stdin = new PassThrough() | ||
| const stderr = new PassThrough() | ||
| let output = '' | ||
|
|
||
| Object.assign(stdout, { columns: 80, isTTY: true, rows: 24 }) | ||
| Object.assign(stdin, { isTTY: true, ref: () => {}, setRawMode: () => {}, unref: () => {} }) | ||
| Object.assign(stderr, { isTTY: true }) | ||
| stdout.on('data', chunk => { | ||
| output += chunk.toString() | ||
| }) | ||
|
|
||
| const instance = renderSync(<App />, { | ||
| patchConsole: false, | ||
| stderr: stderr as NodeJS.WriteStream, | ||
| stdin: stdin as NodeJS.ReadStream, | ||
| stdout: stdout as NodeJS.WriteStream | ||
| }) | ||
|
|
||
| instance.unmount() | ||
| instance.cleanup() | ||
|
|
||
| return toPlainFrame(output) | ||
| } | ||
|
|
||
| describe('copy notice store', () => { | ||
| afterEach(() => { | ||
| vi.useRealTimers() | ||
| dismissCopyNotice() | ||
| }) | ||
|
|
||
| it('clears itself after its duration', () => { | ||
| vi.useFakeTimers() | ||
|
|
||
| showCopyNotice('copied 7 characters', 3000) | ||
|
|
||
| expect($copyNotice.get()).toBe('copied 7 characters') | ||
|
|
||
| vi.advanceTimersByTime(2999) | ||
|
|
||
| expect($copyNotice.get()).toBe('copied 7 characters') | ||
|
|
||
| vi.advanceTimersByTime(1) | ||
|
|
||
| expect($copyNotice.get()).toBeNull() | ||
| }) | ||
|
|
||
| it('replaces the previous notice instead of stacking', () => { | ||
| vi.useFakeTimers() | ||
|
|
||
| showCopyNotice('copied 7 characters', 3000) | ||
| showCopyNotice('sent 42 characters', 3000) | ||
|
|
||
| expect($copyNotice.get()).toBe('sent 42 characters') | ||
| }) | ||
|
|
||
| it("lets a re-shown notice outlive the previous one's deadline", () => { | ||
| vi.useFakeTimers() | ||
|
|
||
| showCopyNotice('copied 7 characters', 3000) | ||
| vi.advanceTimersByTime(2900) | ||
| showCopyNotice('sent 42 characters', 5000) | ||
|
|
||
| // The first notice's deadline passes; the newer one must survive it. | ||
| vi.advanceTimersByTime(100) | ||
|
|
||
| expect($copyNotice.get()).toBe('sent 42 characters') | ||
|
|
||
| vi.advanceTimersByTime(4900) | ||
|
|
||
| expect($copyNotice.get()).toBeNull() | ||
| }) | ||
|
|
||
| it('honours a per-notice duration', () => { | ||
| vi.useFakeTimers() | ||
|
|
||
| showCopyNotice('sent 42 characters', 5000) | ||
|
|
||
| vi.advanceTimersByTime(3000) | ||
|
|
||
| expect($copyNotice.get()).toBe('sent 42 characters') | ||
|
|
||
| vi.advanceTimersByTime(2000) | ||
|
|
||
| expect($copyNotice.get()).toBeNull() | ||
| }) | ||
|
|
||
| it('dismissing clears the notice and leaves a later one to its own timer', () => { | ||
| vi.useFakeTimers() | ||
|
|
||
| showCopyNotice('copied 7 characters', 3000) | ||
| dismissCopyNotice() | ||
|
|
||
| expect($copyNotice.get()).toBeNull() | ||
|
|
||
| showCopyNotice('sent 42 characters', 3000) | ||
| vi.advanceTimersByTime(2999) | ||
|
|
||
| expect($copyNotice.get()).toBe('sent 42 characters') | ||
|
|
||
| vi.advanceTimersByTime(1) | ||
|
|
||
| expect($copyNotice.get()).toBeNull() | ||
| }) | ||
| }) | ||
|
|
||
| describe('copy notice rendering', () => { | ||
| afterEach(() => { | ||
| dismissCopyNotice() | ||
| }) | ||
|
|
||
| it('shows the notice above the composer while one is set', () => { | ||
| const frame = renderFrame({ setup: () => showCopyNotice('copied 7 characters', 3000) }) | ||
|
|
||
| expect(frame).toContain('copied 7 characters') | ||
|
|
||
| // At statusBar='bottom' the cwd label sits below the input box, so the | ||
| // notice must render somewhere above it. | ||
| expect(frame.indexOf('copied 7 characters')).toBeLessThan(frame.indexOf('~/repo')) | ||
| }) | ||
|
|
||
| it('renders nothing for the notice once it has been dismissed', () => { | ||
| const frame = renderFrame({ | ||
| setup: () => { | ||
| showCopyNotice('copied 7 characters', 3000) | ||
| dismissCopyNotice() | ||
| } | ||
| }) | ||
|
|
||
| expect(frame).not.toContain('copied 7 characters') | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| /** | ||
| * One transient notice above the composer. | ||
| * | ||
| * Copy-on-select writes the clipboard on every settled drag; reporting each | ||
| * one as a transcript line stacks permanent rows. This holds a single notice | ||
| * that replaces itself and clears after its duration, so the confirmation is | ||
| * visible for a moment and then gone. | ||
| */ | ||
|
|
||
| import { atom } from 'nanostores' | ||
|
|
||
| export const $copyNotice = atom<null | string>(null) | ||
|
|
||
| let dismissTimer: null | ReturnType<typeof setTimeout> = null | ||
|
|
||
| export function showCopyNotice(text: string, ms: number): void { | ||
| if (dismissTimer !== null) { | ||
| clearTimeout(dismissTimer) | ||
| } | ||
|
|
||
| $copyNotice.set(text) | ||
| dismissTimer = setTimeout(() => { | ||
| dismissTimer = null | ||
| $copyNotice.set(null) | ||
| }, ms) | ||
| } | ||
|
|
||
| export function dismissCopyNotice(): void { | ||
| if (dismissTimer !== null) { | ||
| clearTimeout(dismissTimer) | ||
| dismissTimer = null | ||
| } | ||
|
|
||
| $copyNotice.set(null) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |
| import { buildToolTrailLine, sameToolTrailGroup, toolTrailLabel } from '../lib/text.js' | ||
| import { estimatedMsgHeight, messageHeightKey } from '../lib/virtualHeights.js' | ||
| import { createChatStream, type ChatStreamHandle, type ChatStreamRpcClient } from './chatStream.js' | ||
| import { showCopyNotice } from './copyNoticeStore.js' | ||
| import { createGatewayEventHandler } from './createGatewayEventHandler.js' | ||
| import { createSlashHandler } from './createSlashHandler.js' | ||
| import { getInputSelection } from './inputSelectionStore.js' | ||
|
|
@@ -326,10 +327,13 @@ | |
| // mouse tracking, so copy-on-select is what makes a transcript selection | ||
| // copyable at all. That holds on every platform, not just macOS. | ||
| // | ||
| // Nothing on screen changes when a drag ends, so the transcript line is the | ||
| // only confirmation the clipboard was written. Lives below `sys` because the | ||
| // dependency array is evaluated during render, while `sys` is still in its | ||
| // temporal dead zone further up. | ||
| // Nothing on screen changes when a drag ends, so the report is the only | ||
| // confirmation the clipboard was written. The first copy of a session | ||
| // carries the path caveat and stays in the transcript: it is the answer a | ||
| // user comes looking for after a paste comes up empty minutes later, and it | ||
| // cannot pile up because it is once per session by construction. The terse | ||
| // repeats, which are unbounded, show as a transient notice above the | ||
| // composer instead of stacking rows. | ||
| // | ||
| // The path caveat is per session while this hook outlives any one session: | ||
| // `newSession()` and `resumeById()` replace `ui.sid` without remounting it, | ||
|
|
@@ -342,7 +346,13 @@ | |
| useEffect( | ||
| () => | ||
| subscribeCopyOnSelect(selection, (text, path) => { | ||
| sys(reportCopyOnSelect.current(graphemeCount(text), path, getUiState().sid ?? 'draft')) | ||
| const report = reportCopyOnSelect.current(graphemeCount(text), path, getUiState().sid ?? 'draft') | ||
|
|
||
| if (report.firstOfSession) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This branch is the whole point of 64ed001, and nothing in the suite holds it in place. I inverted it -- caveat to The pieces either side are well covered -- There is precedent in this very file for the cheap fix: Non-blocking -- the behaviour is right today, and this is about keeping it right. Reasonable to take as a follow-up if you would rather not grow the file's exported surface in this PR. |
||
| sys(report.text) | ||
| } else { | ||
| showCopyNotice(report.text, 3000) | ||
| } | ||
| }), | ||
| [selection, sys] | ||
| ) | ||
|
|
@@ -513,7 +523,7 @@ | |
|
|
||
| const label = toolTrailLabel('clarify') | ||
|
|
||
| turnController.turnTools = turnController.turnTools.filter(line => !sameToolTrailGroup(label, line)) | ||
| patchTurnState({ turnTrail: turnController.turnTools }) | ||
|
|
||
| rpc<ClarifyRespondResponse>('clarify.respond', { answer, request_id: clarify.requestId }).then(r => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case cannot fail for the reason its name gives.
dismissCopyNotice()runs against a store that is alreadynull(theafterEachabove dismissed, and nothing in this test ever showed a notice), so the assertion is "a frame that was never given a notice does not contain one" -- true no matter what the component does with$copyNotice.I checked rather than guessed: deleting the
{copyNotice && <Text ...>}line fromappLayout.tsxgives1 failed | 6 passedin this file. The one that fails is the case above it; this one passes with the render line gone.Showing a notice first would make it load-bearing and cover the transition the name promises:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3b0564f: the case now shows a notice and dismisses it, so it pins the show-then-dismiss transition. Probed the other way: removing dismissCopyNotice's atom clear turns this case red.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, and I re-ran your probe rather than taking it on trust: deleting the
$copyNotice.set(null)fromdismissCopyNoticeturns this case red along with the store-level one --2 failed | 5 passed (7). On the previous head only the store case failed, so the render case now genuinely pins the transition its name describes. Nothing further from me on this one.