Skip to content

fix(tui): show the copy-on-select confirmation as a transient notice - #366

Merged
LivXue merged 4 commits into
EverMind-AI:mainfrom
LivXue:fix/tui_copy_notice_toast
Aug 25, 2026
Merged

fix(tui): show the copy-on-select confirmation as a transient notice#366
LivXue merged 4 commits into
EverMind-AI:mainfrom
LivXue:fix/tui_copy_notice_toast

Conversation

@LivXue

@LivXue LivXue commented Aug 25, 2026

Copy link
Copy Markdown
Member

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 the
transcript and the repeats to the notice. A new copyNoticeStore holds the
single notice; showCopyNotice keeps one dismiss timer, clearing any pending
one before arming the next, so an earlier deadline can never clear a newer
notice. /copy is deliberately untouched: an explicit command's answer still
belongs in the transcript, and it cannot stack on its own.

Type

  • Fix

Verification

Run from ui-tui/ at branch head, in a fresh worktree of github/main.
Dependencies came from npm ci in ui-tui/ and in the hermes package; the
hermes package's own node_modules holds only esbuild, so the ink render
tests run against one react copy (two copies break every hook call).

npx vitest run --no-file-parallelism
  Test Files  93 passed (93)
  Tests  1084 passed | 13 skipped (1097)

npm run type-check
  clean, exit 0

npm run lint
  22 problems (0 errors, 22 warnings)

npx prettier --check src/app/copyNoticeStore.ts src/__tests__/copyNotice.test.tsx
  src/__tests__/clipboard.test.ts src/app/useMainApp.ts
  src/components/appLayout.tsx src/lib/clipboard.ts
  All matched files use Prettier code style!

python3 -m scripts.check_commit_messages github/main..HEAD
  exit 0

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-parallelism deliberately: the ink render tests fail under default
worker 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 cases
updated to the new return shape).

New tests were proved load-bearing rather than assumed:

  • Removing the notice render line in appLayout.tsx makes the render test
    fail (content and placement above the input box).

  • Removing the pending-timer clear in showCopyNotice makes the
    re-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.md has 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 branch
restores the #360 behaviour; no data or on-disk format is involved.

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Related Issues

N/A

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>
@LivXue
LivXue requested a review from gloryfromca August 25, 2026 04:19
@LivXue LivXue self-assigned this Aug 25, 2026

@gloryfromca gloryfromca 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.

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 full ComposerPane, and the die()/entry.tsx exit path (the pending setTimeout cannot delay quit -- die() calls process.exit(0)).
  • Callers: createCopyOnSelectReporter has one production caller, and dropping sys from the effect's dependency array is right because the callback no longer closes over it; sys itself is still used by maybeWarn and 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-by trailer; the new files have module docstrings and add no narrating comments. The CONTEXT.md question raised in the PR description holds up -- notice is already the vocabulary of copyResultNotice / copyOnSelectNotice, so nothing new is coined.
  • Whether tests were weakened to look green: no. The three reporter cases in clipboard.test.ts moved from toBe on a string to toEqual on 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 from appLayout.tsx fails 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.

Comment thread ui-tui/src/app/useMainApp.ts Outdated
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)

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.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

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.

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', () => {

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.

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()
  }
})

Copy link
Copy Markdown
Member Author

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.

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.

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[] }) => (

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.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Dropped in cd04fa0: the CompletionItem import, the /comp branches and the parameter threading through makeComposer, makeProps and App are gone.

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.

Fixed in cd04fa0 -- the harness is down to what it actually renders, and tsc --noEmit is clean with the CompletionItem import gone. Thanks.

gloryfromca
gloryfromca previously approved these changes Aug 25, 2026
LivXue and others added 3 commits August 25, 2026 06:11
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 gloryfromca 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.

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..HEAD in full, and re-read clipboard.ts / copyNoticeStore.ts / the ComposerPane insertion against it.
  • sys returning to the dependency array: it is stable (useCallback over appendMessage, itself []-deps), so the bus subscription is not rebuilt per render. The old TDZ note about keeping the effect below const sys was dropped in b6e2414 and not restored, but I am not asking for it back -- tsc rejects 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) {

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.

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. */

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.

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.

@LivXue
LivXue merged commit 274ab1f into EverMind-AI:main Aug 25, 2026
28 checks passed
@LivXue
LivXue deleted the fix/tui_copy_notice_toast branch August 25, 2026 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants