test: cover mouse-driven diff selection and sidebar resize#449
Merged
Conversation
DiffPane and App had their largest uncovered clusters in mouse-drag interactions that no test exercised: diff-pane text selection (drag-extend, double/triple-click word/line expansion, OSC52 clipboard copy and its unsupported-terminal fallback) and sidebar drag-resize plus the edit-selected-file action. Drive these through the @OpenTui testRender mouse harness so the behavior users rely on is locked down. DiffPane.tsx rises from 91% to 97% and App.tsx from 90% to 97% line coverage; no production code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR adds 15 render-level unit tests across two new files to cover the previously untested mouse-drag paths in
Confidence Score: 4/5Test-only change with no production code touched; safe to merge. The renderer created inside src/ui/AppHost.selection.test.tsx — specifically the renderSelectionApp helper and waitForFrame utility. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant T as Test
participant H as testRender harness
participant R as Renderer (spy)
participant DP as DiffPane
T->>H: "testRender(<AppHost />)"
H-->>T: setup (renderer, mockMouse, mockInput)
T->>R: monkey-patch isOsc52Supported / copyToClipboardOSC52
T->>H: flush() — settle initial renders
Note over T,DP: Selection path
T->>H: mockMouse.drag(startX, startY, endX, endY, LEFT)
H->>DP: beginCopySelection → updateCopySelection → endCopySelection
DP->>R: copyToClipboardOSC52(selectedText)
R-->>T: captured in copied[]
T->>H: waitForFrame("Copied selection to clipboard")
H-->>T: frame with notice
Note over T,DP: Sidebar resize path
T->>H: mockMouse.pressDown(dividerX, row)
H->>DP: "beginSidebarResize (sets isResizingSidebar=true)"
T->>H: flush()
T->>H: mockMouse.moveTo(newX, row)
H->>DP: updateSidebarResize (updates width)
T->>H: flush()
T->>H: mockMouse.release(newX, row)
H->>DP: endSidebarResize
T->>H: dividerColumn() — assert new position
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant T as Test
participant H as testRender harness
participant R as Renderer (spy)
participant DP as DiffPane
T->>H: "testRender(<AppHost />)"
H-->>T: setup (renderer, mockMouse, mockInput)
T->>R: monkey-patch isOsc52Supported / copyToClipboardOSC52
T->>H: flush() — settle initial renders
Note over T,DP: Selection path
T->>H: mockMouse.drag(startX, startY, endX, endY, LEFT)
H->>DP: beginCopySelection → updateCopySelection → endCopySelection
DP->>R: copyToClipboardOSC52(selectedText)
R-->>T: captured in copied[]
T->>H: waitForFrame("Copied selection to clipboard")
H-->>T: frame with notice
Note over T,DP: Sidebar resize path
T->>H: mockMouse.pressDown(dividerX, row)
H->>DP: "beginSidebarResize (sets isResizingSidebar=true)"
T->>H: flush()
T->>H: mockMouse.moveTo(newX, row)
H->>DP: updateSidebarResize (updates width)
T->>H: flush()
T->>H: mockMouse.release(newX, row)
H->>DP: endSidebarResize
T->>H: dividerColumn() — assert new position
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/ui/AppHost.selection.test.tsx:57-70
**`waitForFrame` silently exhausts retries without surfacing a timeout**
When the predicate is never satisfied in 10 attempts, the function returns the last captured frame without any indication that the condition was not met. The follow-up `expect(noticeFrame).toContain(...)` then fails with a generic "string does not contain X" message rather than something diagnostic like "notice never appeared after 200 ms". This makes future flaky-test investigations harder — consider adding a `console.warn` or a distinct assertion at the point where the loop exits without matching.
### Issue 2 of 2
src/ui/AppHost.selection.test.tsx:90-110
**Renderer leak if `flush` inside `renderSelectionApp` throws**
`testRender` creates the renderer on line 99, but `flush` is called at line 108 — still inside `renderSelectionApp`, before the `try/finally` in the caller. If `flush` throws (e.g., a render-phase exception during the initial layout pass), the renderer is never destroyed. The `AppHost.sidebar-resize.test.tsx` file avoids this by moving cleanup to `afterEach`; the same lifecycle approach applied here would also protect against this gap.
Reviews (1): Last reviewed commit: "test: cover mouse-driven diff selection ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+57
to
+70
| async function waitForFrame(setup: Harness, predicate: (frame: string) => boolean, attempts = 10) { | ||
| let frame = setup.captureCharFrame(); | ||
| for (let attempt = 0; attempt < attempts; attempt += 1) { | ||
| if (predicate(frame)) { | ||
| return frame; | ||
| } | ||
| await act(async () => { | ||
| await Bun.sleep(20); | ||
| await setup.renderOnce(); | ||
| }); | ||
| frame = setup.captureCharFrame(); | ||
| } | ||
| return frame; | ||
| } |
Contributor
There was a problem hiding this comment.
waitForFrame silently exhausts retries without surfacing a timeout
When the predicate is never satisfied in 10 attempts, the function returns the last captured frame without any indication that the condition was not met. The follow-up expect(noticeFrame).toContain(...) then fails with a generic "string does not contain X" message rather than something diagnostic like "notice never appeared after 200 ms". This makes future flaky-test investigations harder — consider adding a console.warn or a distinct assertion at the point where the loop exits without matching.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/AppHost.selection.test.tsx
Line: 57-70
Comment:
**`waitForFrame` silently exhausts retries without surfacing a timeout**
When the predicate is never satisfied in 10 attempts, the function returns the last captured frame without any indication that the condition was not met. The follow-up `expect(noticeFrame).toContain(...)` then fails with a generic "string does not contain X" message rather than something diagnostic like "notice never appeared after 200 ms". This makes future flaky-test investigations harder — consider adding a `console.warn` or a distinct assertion at the point where the loop exits without matching.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Member
Author
There was a problem hiding this comment.
Good catch — fixed in 82df4d7: waitForFrame now logs a warning naming the unmet condition when it exhausts its retries, so a follow-up assertion failure is diagnosable instead of surfacing only a generic substring miss.
Comment on lines
+90
to
+110
| async function renderSelectionApp( | ||
| bootstrap: AppBootstrap, | ||
| { | ||
| width = 110, | ||
| height = 26, | ||
| osc52 = true, | ||
| }: { width?: number; height?: number; osc52?: boolean } = {}, | ||
| ) { | ||
| const { AppHost } = await import("./AppHost"); | ||
| const setup = await testRender(<AppHost bootstrap={bootstrap} />, { width, height }); | ||
|
|
||
| const copied: string[] = []; | ||
| setup.renderer.isOsc52Supported = () => osc52; | ||
| setup.renderer.copyToClipboardOSC52 = (text: string) => { | ||
| copied.push(text); | ||
| return true; | ||
| }; | ||
|
|
||
| await flush(setup); | ||
| return { setup, copied }; | ||
| } |
Contributor
There was a problem hiding this comment.
flush inside renderSelectionApp throws
testRender creates the renderer on line 99, but flush is called at line 108 — still inside renderSelectionApp, before the try/finally in the caller. If flush throws (e.g., a render-phase exception during the initial layout pass), the renderer is never destroyed. The AppHost.sidebar-resize.test.tsx file avoids this by moving cleanup to afterEach; the same lifecycle approach applied here would also protect against this gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/AppHost.selection.test.tsx
Line: 90-110
Comment:
**Renderer leak if `flush` inside `renderSelectionApp` throws**
`testRender` creates the renderer on line 99, but `flush` is called at line 108 — still inside `renderSelectionApp`, before the `try/finally` in the caller. If `flush` throws (e.g., a render-phase exception during the initial layout pass), the renderer is never destroyed. The `AppHost.sidebar-resize.test.tsx` file avoids this by moving cleanup to `afterEach`; the same lifecycle approach applied here would also protect against this gap.
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
There was a problem hiding this comment.
Fixed in 82df4d7: renderSelectionApp now wraps the initial flush in try/catch and destroys the renderer if it throws, before returning to the caller's try/finally.
Address review feedback on the new copy-selection tests: surface a warning when waitForFrame exhausts its retries so a follow-up assertion failure points at the unmet condition instead of a generic substring miss, and destroy the renderer if the initial settle throws before the caller's try/finally can take over. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
DiffPane and App had their largest uncovered clusters in mouse-drag interactions that no test exercised. This adds render-level coverage through the
@opentuitestRendermouse harness.DiffPane.tsxApp.tsx15 new tests across two new files:
AppHost.selection.test.tsx(9) — diff-pane text selection: drag-extend, double/triple-click word & line expansion, OSC52 clipboard copy + unsupported-terminal fallback, no-move-no-copy, out-of-viewport clear, right-button bail, pinned-header & split-side resolution.AppHost.sidebar-resize.test.tsx(6) — sidebar drag-resize (widen / narrow-clamped-at-min), release-with-no-drag no-op, non-left-button guard, and the edit-selected-file action (ewith no$EDITOR→ notice, no spawn).Remaining gaps in those files (watch-mode fs polling, the real editor-spawn success branch, scroll/geometry paths owned by other interaction tests) are deliberately left to integration coverage.
Verification
bun run typecheck✅bun run lint✅ (0 warnings)bun run format:check✅Test-only; no production code changed (empty changeset included).
🤖 Generated with Claude Code