fix(tui): show the copy-on-select confirmation as a transient notice - #366
Conversation
Each settled drag wrote a permanent transcript line, so a reading session piled up "copied N characters" rows under the conversation. The confirmation is now a single notice above the input box that replaces itself and clears: 3s for the terse line, 5s for the first copy's path caveat. Co-authored-by: Claude (deepseek-v4-pro) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned. The store is correct (the timer-clear ordering is right, and the re-shown-notice test is genuinely load-bearing), the layout insertion follows the precedent of the bgTasks line right above it, and the reporter's shape change has exactly one caller. What is left is one design point about the OSC 52 caveat and two test-quality nits, all inline, none of them merge-blocking.
What I covered
- The diff, plus
copyOnSelect.ts,clipboard.ts, the fullComposerPane, and thedie()/entry.tsxexit path (the pendingsetTimeoutcannot delay quit --die()callsprocess.exit(0)). - Callers:
createCopyOnSelectReporterhas one production caller, and droppingsysfrom the effect's dependency array is right because the callback no longer closes over it;sysitself is still used bymaybeWarnand others. - React reconciliation:
{copyNotice && ...}keeps its slot in the static children array, so the input box is not remounted when the notice appears or clears -- no lost composer state. - Project rules: commit header is 68 chars, message is ASCII-only and carries the
Co-authored-bytrailer; the new files have module docstrings and add no narrating comments. TheCONTEXT.mdquestion raised in the PR description holds up --noticeis already the vocabulary ofcopyResultNotice/copyOnSelectNotice, so nothing new is coined. - Whether tests were weakened to look green: no. The three reporter cases in
clipboard.test.tsmoved fromtoBeon a string totoEqualon the object and still assert both fields, and the five store cases use fake timers with real before/after assertions on the boundary tick. I mutation-tested the render assertion myself: deleting the notice line fromappLayout.tsxfails 1 of the 7 new cases (see the second inline note for which one does not fail).
Test run, from ui-tui/ at b6e24142:
vitest run src/__tests__/copyNotice.test.tsx src/__tests__/clipboard.test.ts src/__tests__/copyOnSelect.test.ts
Test Files 3 passed (3)
Tests 49 passed (49) # 0 skipped -- the two render cases really ran
tsc --noEmit -p tsconfig.json -> exit 0
eslint src/ -> 22 warnings, 0 errors, none on a line this branch touches
Full suite here: 92 passed | 1 failed (93), the failure being 2 cases in src/__tests__/textInputTypingBurst.test.tsx. Not attributable to this branch -- it fails identically on a fresh worktree of github/main in the same environment, with or without --no-file-parallelism. My environment differs from the one in the PR description (I reused an existing node_modules rather than a clean npm ci), which is the likely reason I see 0 skipped where the description reports 13.
| const report = reportCopyOnSelect.current(graphemeCount(text), path, getUiState().sid ?? 'draft') | ||
|
|
||
| // The first copy's caveat is longer than the terse line, so it stays up longer. | ||
| showCopyNotice(report.text, report.firstOfSession ? 5000 : 3000) |
There was a problem hiding this comment.
Non-blocking, but worth a thought before this lands: the OSC 52 caveat is now shown only during the window in which the user has no reason to read it.
The caveat exists to explain an empty paste -- clipboard.ts says so in as many words ("the first copy is the only moment a user is looking for the reason a paste came up empty"). But the user does not find out the paste is empty inside Raven. They drag a selection, switch to their editor, hit paste, and get nothing -- and that is comfortably more than 5 seconds later. On main the answer was still sitting in the transcript when they came back. After this change there is no record of it anywhere, so the diagnostic PR #360 deliberately spent the first copy on is unreachable exactly when it is needed.
Concrete: terminal without OSC 52 clipboard access allowed, user drags a span, tabs to another app ~10s later, pastes nothing, and has no way to learn that the terminal refused it.
The two goals are not actually in tension, because the caveat is the one line that cannot stack -- it is once per session by construction. Keeping the first-of-session line in the transcript via sys() and making only the terse repeats transient would stop the row pile-up (that is the repeats, which are unbounded) while leaving the diagnostic recoverable. report.firstOfSession already tells the callback which branch it is in, so it is a two-line change here rather than a redesign. Your call -- if you would rather keep one uniform presentation, say so and I will drop it.
There was a problem hiding this comment.
Kept the caveat in the transcript as suggested: 64ed001 routes the first-of-session line through sys(), and only the unbounded terse repeats become the transient notice. The reporter's per-session tally is unchanged, so the caveat still fires exactly once per session.
There was a problem hiding this comment.
Verified, and this is the shape I was hoping for -- routing on report.firstOfSession rather than tuning a duration, so the caveat is recoverable minutes later and the unbounded repeats are the only thing that goes transient. The reworked block comment above the effect earns its place too: it now says why the split falls where it does.
One thing came out of checking it, which I have raised inline rather than here: the routing branch itself has no test. I inverted it (caveat to the notice, repeats to sys) and ran the full suite -- 92 passed | 1 failed (93), the one failure being the two pre-existing textInputTypingBurst cases that are equally red on github/main here. Zero new failures. So the decision this commit exists to make is invisible to the suite.
| expect(frame.indexOf('copied 7 characters')).toBeLessThan(frame.indexOf('~/repo')) | ||
| }) | ||
|
|
||
| it('renders nothing for the notice once it has been dismissed', () => { |
There was a problem hiding this comment.
This case cannot fail for the reason its name gives. dismissCopyNotice() runs against a store that is already null (the afterEach above 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 from appLayout.tsx gives 1 failed | 6 passed in 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:
const frame = renderFrame({
setup: () => {
showCopyNotice('copied 7 characters', 3000)
dismissCopyNotice()
}
})There was a problem hiding this comment.
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.
Confirmed, and I re-ran your probe rather than taking it on trust: deleting the $copyNotice.set(null) from dismissCopyNotice turns 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.
| } | ||
| }) | ||
|
|
||
| const App = ({ completions = [] }: { completions?: CompletionItem[] }) => ( |
There was a problem hiding this comment.
Minor: the completions plumbing is inert. renderFrame renders <App /> with no props, so completions is always [] -- which makes the CompletionItem import, the /comp branches in makeComposer for input / inputBuf, and the parameter threading through makeProps and App dead scaffolding. A later reader sees it and assumes the completion overlay is covered here. Either drop it or pass it from renderFrame if a completions frame is the follow-up you had in mind.
There was a problem hiding this comment.
Dropped in cd04fa0: the CompletionItem import, the /comp branches and the parameter threading through makeComposer, makeProps and App are gone.
There was a problem hiding this comment.
Fixed in cd04fa0 -- the harness is down to what it actually renders, and tsc --noEmit is clean with the CompletionItem import gone. Thanks.
The caveat explains an empty paste, but the user discovers the empty paste after switching to another app, long after a transient notice is gone. The caveat is once per session by construction, so keeping it in the transcript cannot pile up rows; only the unbounded terse repeats become transient notices. Co-authored-by: Claude (deepseek-v4-pro) <noreply@anthropic.com>
…e harness The completions plumbing was copied from the status bar harness and is never driven here: renderFrame always renders App without props, so the parameter threading and the /comp branches are dead. Co-authored-by: Claude (deepseek-v4-pro) <noreply@anthropic.com>
The case dismissed a store that was already null, so it could not fail for the reason its name gives. Show a notice first so the case pins the show-then-dismiss transition. Co-authored-by: Claude (deepseek-v4-pro) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned. Re-reviewing at the new head: the three commits since b6e2414 do what they say, and 64ed001 is a better answer than the one I suggested was needed -- routing on report.firstOfSession instead of tuning a duration. Two non-blocking notes below, both fallout from the reroute rather than anything the earlier revision had.
What I covered on this revision
- The delta
b6e24142..HEADin full, and re-readclipboard.ts/copyNoticeStore.ts/ theComposerPaneinsertion against it. sysreturning to the dependency array: it is stable (useCallbackoverappendMessage, itself[]-deps), so the bus subscription is not rebuilt per render. The old TDZ note about keeping the effect belowconst syswas dropped in b6e2414 and not restored, but I am not asking for it back --tscrejects the use-before-declaration outright, so the constraint is enforced by the compiler rather than by the comment.- The store's single-timer logic is still load-bearing now that production only ever passes 3000: copy at t=0 and again at t=2000 still needs the
clearTimeout, or the first deadline wipes the second notice at t=3000. - Both of the author's load-bearing claims, re-run rather than trusted -- see my replies on those two threads. Deleting
dismissCopyNotice's atom clear turns the reworked render case red (2 failed | 5 passed), which it did not do before. - Project rules: all three new commit headers are Conventional-Commits, ASCII, under 100 chars, and carry the trailer. The PR description was updated to match the reroute, which matters here because the squash body is taken from it -- it no longer claims the 5-second caveat that 64ed001 removed.
Test run at 3b0564f7, from ui-tui/:
vitest run src/__tests__/copyNotice.test.tsx src/__tests__/clipboard.test.ts src/__tests__/copyOnSelect.test.ts
Test Files 3 passed (3)
Tests 49 passed (49) # 0 skipped
tsc --noEmit -p tsconfig.json -> exit 0
Full suite: 92 passed | 1 failed (93), 1095 passed | 2 failed (1097). The two failures are textInputTypingBurst.test.tsx and are not yours -- they fail identically on a fresh worktree of github/main in this environment. As before I reused an existing node_modules instead of a clean npm ci, which is the likely reason I see 0 skipped where your description reports 13.
| sys(reportCopyOnSelect.current(graphemeCount(text), path, getUiState().sid ?? 'draft')) | ||
| const report = reportCopyOnSelect.current(graphemeCount(text), path, getUiState().sid ?? 'draft') | ||
|
|
||
| if (report.firstOfSession) { |
There was a problem hiding this comment.
This branch is the whole point of 64ed001, and nothing in the suite holds it in place. I inverted it -- caveat to showCopyNotice, terse repeats to sys -- and ran the full suite: 92 passed | 1 failed (93), the single failure being the two pre-existing textInputTypingBurst cases that are equally red on github/main here. Not one new failure. So the regression this PR exists to prevent (unbounded rows stacking under the conversation) could be reintroduced by swapping two lines and CI would wave it through.
The pieces either side are well covered -- clipboard.test.ts pins which report carries the caveat, copyNotice.test.tsx pins the store and the render -- and the gap is exactly the seam between them, because useMainApp is never rendered by any test.
There is precedent in this very file for the cheap fix: modelSelectCommand and buildChatStreamHandle are exported from useMainApp.ts specifically so tests can reach them without a live hook, and modelSelectCommand.test.ts / sessionManagement.test.ts import them directly. A deliverCopyReport(report, { showNotice, sys }) in the same style would make the seam a three-line test and needs no new harness.
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.
| } | ||
|
|
||
| /** The notice for one copy-on-select write, plus whether it carried the path | ||
| * caveat -- the caveat takes longer to read, so the caller shows it longer. */ |
There was a problem hiding this comment.
Stale as of 64ed001: the caller no longer "shows it longer". The caveat now goes to the transcript and the repeats to the notice, so the flag is a routing signal, not a duration hint.
Worth a word because of where it could lead. If durations ever unify (they already have -- production passes 3000 on the only path that reaches showCopyNotice), a reader who believes this comment concludes firstOfSession no longer earns its keep and drops it from the return -- which silently sends the caveat back to the transient notice. Something like "...plus whether it carried the path caveat -- the caveat is the answer to a paste that came up empty, so the caller keeps it in the transcript and only the repeats go transient" would say what the flag is now for.
Summary
PR #360 reports every copy-on-select write as a permanent transcript line, so a
reading session that drags through the transcript piles up "copied N
characters" rows under the conversation. The confirmation splits by what the
line is for: the unbounded terse repeats become a transient notice rendered
above the input box -- one line that replaces itself on every new copy and
clears after 3 seconds -- while the first copy of a session keeps its path
caveat in the transcript. The caveat 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 reporter still owns the per-session caveat tally and now returns
{ text, firstOfSession }so the caller can route the caveat to thetranscript and the repeats to the notice. A new
copyNoticeStoreholds thesingle notice;
showCopyNoticekeeps one dismiss timer, clearing any pendingone before arming the next, so an earlier deadline can never clear a newer
notice.
/copyis deliberately untouched: an explicit command's answer stillbelongs in the transcript, and it cannot stack on its own.
Type
Verification
Run from
ui-tui/at branch head, in a fresh worktree ofgithub/main.Dependencies came from
npm ciinui-tui/and in the hermes package; thehermes package's own
node_modulesholds only esbuild, so the ink rendertests run against one react copy (two copies break every hook call).
The 22 lint warnings are the baseline, not new: 22 before the branch and 22
after, none in a file this branch touches. The suite is run with
--no-file-parallelismdeliberately: the ink render tests fail under defaultworker parallelism at this suite size, and no CI job runs them.
Suites that exercise the changed code directly:
src/__tests__/copyNotice.test.tsx(7 cases),
src/__tests__/clipboard.test.ts(30 cases, the 3 reporter casesupdated to the new return shape).
New tests were proved load-bearing rather than assumed:
Removing the notice render line in
appLayout.tsxmakes the render testfail (content and placement above the input box).
Removing the pending-timer clear in
showCopyNoticemakes there-shown-notice test fail: the earlier 3s deadline then clears the newer
notice.
The dismissed-notice case now shows a notice first, and removing
dismissCopyNotice's atom clear turns it red (review fix).Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
No doc change.
ui-tui/CONTEXT.mdhas no clipboard or selection entry, and"copy notice" continues the existing "notice" vocabulary of
copyResultNotice/
copyOnSelectNotice, so nothing new is coined.Risk
Behaviour change: the per-drag copy confirmation is now transient and above
the input box instead of a permanent transcript line, and the first copy of a
session lands one transcript line, exactly as on
main. Reverting the branchrestores the #360 behaviour; no data or on-disk format is involved.
Related Issues
N/A