Skip to content

Commit c38c873

Browse files
Clean review nits before the next cut (#465)
* Clean review nits before the next cut Single-source keepRecentTurns, honest copy flash (only after a successful clipboard write), scheduleUpgradeNotice coverage, and stale Ink/type-to-filter comment fixes from the multi-agent pass. * Clear selection before clipboard settles Leave the flash gated on write success/failure, but drop the highlight immediately so a slow clipboard helper cannot pin the selection. Cover writeClipboard settlement paths and latch upgrade-notice tests on the fetch promise instead of a fixed sleep.
1 parent f87f345 commit c38c873

9 files changed

Lines changed: 348 additions & 35 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@ Format loosely follows [Keep a Changelog](https://keepachangelog.com/). Versions
3131
drag selection in the transcript writes the selected text to the system
3232
clipboard on mouse-up and flashes a short status line. Alt+M still hands the
3333
mouse back for native terminal selection; Alt+C remains the keyboard copy
34-
path for whole messages, tool outputs, and diffs.
34+
path for whole messages, tool outputs, and diffs. Highlight clears
35+
immediately; status flash only after the clipboard write settles — success
36+
shows the preview, throw/reject shows `Copy failed` (same honesty on Alt+C
37+
structured copy).
3538
- **Install-aware upgrade notice.** When a newer GitHub release exists, a
3639
non-blocking startup notice names the running and latest versions and the
3740
right upgrade step for Homebrew, source/Bun, deb, release binary, or

src/session/compactor.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -206,19 +206,19 @@ export type CompactorConfig = {
206206
maxAnchorTurns: number;
207207
};
208208

209-
const DEFAULT_COMPACTOR_CONFIG: CompactorConfig = {
210-
keepRecentTurns: 6,
211-
summaryMaxChars: 2000,
212-
maxAnchorTurns: 8,
213-
};
214-
215209
// Recent turns kept verbatim by both real pruning-compactor registrations
216210
// (the main session and sub-agents). Exported so callers that need to know
217211
// in advance whether a compaction would do anything — the compaction
218212
// governor's arming floor — derive it from this value instead of carrying
219213
// an independent literal that can silently drift out of sync.
220214
export const COMPACTOR_KEEP_RECENT_TURNS = 6;
221215

216+
const DEFAULT_COMPACTOR_CONFIG: CompactorConfig = {
217+
keepRecentTurns: COMPACTOR_KEEP_RECENT_TURNS,
218+
summaryMaxChars: 2000,
219+
maxAnchorTurns: 8,
220+
};
221+
222222
// `apply` below no-ops at or below this turn count: keeping `keepRecentTurns`
223223
// turns plus at least one more is what makes pruning worth doing at all.
224224
export function compactorNoOpFloor(keepRecentTurns: number): number {

src/tui/copy-path.test.ts

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
formatCopyText,
88
pickCopyRow,
99
streamLogMarkdown,
10+
writeClipboard,
1011
} from "./copy-path"
1112
import type { StreamRow } from "./stream"
1213

@@ -30,6 +31,79 @@ describe("classifyCopy", () => {
3031
})
3132
})
3233

34+
describe("writeClipboard", () => {
35+
test("sync success runs onSuccess", () => {
36+
const events: string[] = []
37+
writeClipboard(
38+
{
39+
writeText: (text) => {
40+
events.push(`write:${text}`)
41+
},
42+
},
43+
"hi",
44+
{
45+
onSuccess: () => events.push("ok"),
46+
onFailure: () => events.push("fail"),
47+
},
48+
)
49+
expect(events).toEqual(["write:hi", "ok"])
50+
})
51+
52+
test("sync throw runs onFailure", () => {
53+
const events: string[] = []
54+
writeClipboard(
55+
{
56+
writeText: () => {
57+
throw new Error("nope")
58+
},
59+
},
60+
"hi",
61+
{
62+
onSuccess: () => events.push("ok"),
63+
onFailure: () => events.push("fail"),
64+
},
65+
)
66+
expect(events).toEqual(["fail"])
67+
})
68+
69+
test("async resolve defers onSuccess", async () => {
70+
let resolveWrite!: () => void
71+
const writeP = new Promise<void>((r) => {
72+
resolveWrite = r
73+
})
74+
const events: string[] = []
75+
writeClipboard(
76+
{ writeText: () => writeP },
77+
"hi",
78+
{
79+
onSuccess: () => events.push("ok"),
80+
onFailure: () => events.push("fail"),
81+
},
82+
)
83+
expect(events).toEqual([])
84+
resolveWrite()
85+
await writeP
86+
await Promise.resolve()
87+
expect(events).toEqual(["ok"])
88+
})
89+
90+
test("async reject runs onFailure", async () => {
91+
const events: string[] = []
92+
writeClipboard(
93+
{ writeText: () => Promise.reject(new Error("nope")) },
94+
"hi",
95+
{
96+
onSuccess: () => events.push("ok"),
97+
onFailure: () => events.push("fail"),
98+
},
99+
)
100+
expect(events).toEqual([])
101+
await Promise.resolve()
102+
await Promise.resolve()
103+
expect(events).toEqual(["fail"])
104+
})
105+
})
106+
33107
describe("formatCopyText / copyStreamRow", () => {
34108
test("writes plain text and summary", () => {
35109
const port = createRecordingClipboard()

src/tui/copy-path.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,37 @@ export type ClipboardPort = {
2828
readonly writeText: (text: string) => void | Promise<void>
2929
}
3030

31+
/**
32+
* Write to the clipboard, then run success/failure handlers.
33+
* Never throws: sync throws and promise rejections both hit onFailure.
34+
* Flash "Copied …" only from onSuccess so a failed write never lies.
35+
*/
36+
export function writeClipboard(
37+
port: ClipboardPort,
38+
text: string,
39+
handlers: {
40+
readonly onSuccess: () => void
41+
readonly onFailure?: () => void
42+
},
43+
): void {
44+
const fail = () => {
45+
handlers.onFailure?.()
46+
}
47+
try {
48+
const result = port.writeText(text)
49+
if (
50+
result != null
51+
&& typeof (result as PromiseLike<void>).then === "function"
52+
) {
53+
void Promise.resolve(result).then(handlers.onSuccess, fail)
54+
return
55+
}
56+
handlers.onSuccess()
57+
} catch {
58+
fail()
59+
}
60+
}
61+
3162
/** Recording port for headless tests. */
3263
export function createRecordingClipboard(): ClipboardPort & {
3364
readonly writes: string[]
@@ -134,14 +165,17 @@ export function streamLogMarkdown(targets: readonly CopyTarget[]): string {
134165
/**
135166
* Copy the active (or last) stream row via the clipboard port.
136167
* Returns the payload, or null when there is nothing to copy.
168+
* Write errors are swallowed (no flash here — callers own chrome).
137169
*/
138170
export function copyStreamRow(
139171
row: StreamRow | undefined | null,
140172
port: ClipboardPort,
141173
): CopyPayload | null {
142174
if (!row) return null
143175
const payload = formatCopyText(row)
144-
void port.writeText(payload.text)
176+
writeClipboard(port, payload.text, {
177+
onSuccess: () => {},
178+
})
145179
return payload
146180
}
147181

src/tui/demo.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
/**
22
* Interactive OpenTUI product-skin demo (real TTY only).
3-
* Run: bun src/tui/demo.ts
3+
* Run: `bun src/tui/demo.ts`
44
*
5-
* Wave 7: residual surfaces + observe on shared kit.
6-
* Not production CLI. Ink remains production.
5+
* Not the production CLI (`src/index.ts` → OpenTUI shell). Playground only.
76
*
87
* Keys:
98
* Enter=queue · Alt+Enter=steer · Ctrl+C=stop

src/tui/selection-copy.test.ts

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,5 +88,95 @@ describe("copyFinishedSelection", () => {
8888
expect(h.flashes[0]).toContain("ok go")
8989
expect(h.flashes[0]).not.toContain("\n")
9090
})
91+
92+
test("clears highlight immediately while write is still pending", async () => {
93+
let resolveWrite!: () => void
94+
const writeP = new Promise<void>((r) => {
95+
resolveWrite = r
96+
})
97+
const flashes: string[] = []
98+
let cleared = 0
99+
const ok = copyFinishedSelection(
100+
{
101+
clipboard: {
102+
writeText: () => writeP,
103+
},
104+
flash: (text: string) => {
105+
flashes.push(text)
106+
},
107+
clearSelection: () => {
108+
cleared += 1
109+
},
110+
},
111+
{
112+
isDragging: false,
113+
getSelectedText: () => "pending",
114+
},
115+
)
116+
expect(ok).toBe(true)
117+
expect(cleared).toBe(1)
118+
expect(flashes).toEqual([])
119+
resolveWrite()
120+
await writeP
121+
await Promise.resolve()
122+
expect(flashes[0]).toContain("Copied 7 chars")
123+
expect(cleared).toBe(1)
124+
})
125+
126+
test("flashes Copy failed after clear when write rejects", async () => {
127+
const flashes: string[] = []
128+
let cleared = 0
129+
const ok = copyFinishedSelection(
130+
{
131+
clipboard: {
132+
writeText: () => Promise.reject(new Error("no clipboard")),
133+
},
134+
flash: (text: string) => {
135+
flashes.push(text)
136+
},
137+
clearSelection: () => {
138+
cleared += 1
139+
},
140+
},
141+
{
142+
isDragging: false,
143+
getSelectedText: () => "secret",
144+
},
145+
)
146+
expect(ok).toBe(true)
147+
expect(cleared).toBe(1)
148+
expect(flashes).toEqual([])
149+
await Promise.resolve()
150+
await Promise.resolve()
151+
expect(flashes).toEqual(["Copy failed"])
152+
expect(cleared).toBe(1)
153+
})
154+
155+
test("flashes Copy failed when write throws synchronously", () => {
156+
const flashes: string[] = []
157+
let cleared = 0
158+
const ok = copyFinishedSelection(
159+
{
160+
clipboard: {
161+
writeText: () => {
162+
throw new Error("no clipboard")
163+
},
164+
},
165+
flash: (text: string) => {
166+
flashes.push(text)
167+
},
168+
clearSelection: () => {
169+
cleared += 1
170+
},
171+
},
172+
{
173+
isDragging: false,
174+
getSelectedText: () => "secret",
175+
},
176+
)
177+
expect(ok).toBe(true)
178+
expect(cleared).toBe(1)
179+
expect(flashes).toEqual(["Copy failed"])
180+
})
91181
})
92182

src/tui/selection-copy.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import type { Selection } from "@opentui/core"
10-
import type { ClipboardPort } from "./copy-path.js"
10+
import { writeClipboard, type ClipboardPort } from "./copy-path.js"
1111

1212
/** Minimal deps so unit tests do not need a full AppShell. */
1313
export type SelectionCopyHost = {
@@ -23,8 +23,12 @@ export type FinishedSelection = {
2323
}
2424

2525
/**
26-
* Copy a finished (non-dragging) selection. Returns true when text was
27-
* written. Empty selections and still-dragging states are no-ops.
26+
* Copy a finished (non-dragging) selection. Returns true when a write was
27+
* attempted. Empty selections and still-dragging states are no-ops.
28+
*
29+
* Clears the highlight immediately so a slow or hung clipboard helper cannot
30+
* leave the selection stuck. Status flash waits for write settlement:
31+
* `Copied …` on success, `Copy failed` on throw/reject.
2832
*/
2933
export function copyFinishedSelection(
3034
host: SelectionCopyHost,
@@ -34,13 +38,21 @@ export function copyFinishedSelection(
3438
const text = selection.getSelectedText()
3539
if (text.length === 0) return false
3640

37-
void host.clipboard.writeText(text)
3841
// Notice row is one line; always collapse whitespace so multi-line
3942
// drag-selects do not inject raw newlines into chrome.
4043
const oneLine = text.replace(/\s+/g, " ").trim()
4144
const preview =
4245
oneLine.length > 48 ? `${oneLine.slice(0, 45)}…` : oneLine
43-
host.flash(`Copied ${text.length} chars: ${preview}`)
46+
47+
// Clear before the write settles — honesty only gates the flash message.
4448
host.clearSelection()
49+
writeClipboard(host.clipboard, text, {
50+
onSuccess: () => {
51+
host.flash(`Copied ${text.length} chars: ${preview}`)
52+
},
53+
onFailure: () => {
54+
host.flash("Copy failed")
55+
},
56+
})
4557
return true
4658
}

0 commit comments

Comments
 (0)