Add PTY mouse menu interaction coverage#145
Conversation
Greptile SummaryThis PR adds one integration test ( Key observations:
Confidence Score: 5/5Safe to merge — the new test is additive, well-structured, and all remaining findings are minor style suggestions. Both findings are P2 style suggestions (weaker assertion boundary and missing comment). Neither affects correctness, data integrity, or the test's ability to catch regressions under normal conditions. No production code is changed. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test
participant S as PTY Session
participant H as Harness
T->>S: launchHunk(split, 220x12)
S-->>T: session
T->>S: waitForText(menu bar)
S-->>T: initial snapshot
Note over T: assert line01 visible, line10 not visible
T->>S: scrollDown(2)
T->>H: waitForSnapshot(line08 visible)
H-->>T: primed snapshot
Note over T: assert line08 visible
T->>S: clickAt(scrollbarX=218, row=9) [below thumb - page down]
T->>H: waitForSnapshot(line12 visible, line08 absent)
H-->>T: pagedDown snapshot
Note over T: assert line12 visible, line08 not visible
T->>S: clickAt(scrollbarX=218, row=3) [above thumb - page up]
T->>H: waitForSnapshot(line01 visible, line12 absent)
H-->>T: pagedUp snapshot
Note over T: assert line01 visible, line12 not visible
T->>S: session.close() [finally]
Reviews (1): Last reviewed commit: "Add PTY scrollbar track click coverage" | Re-trigger Greptile |
| }); | ||
|
|
||
| expect(initial).toContain("line01 = 101"); | ||
| expect(initial).not.toContain("line10 = 110"); |
There was a problem hiding this comment.
Weaker initial-state assertion than the sibling test
The adjacent "mouse wheel scrolling" test uses the same fixture and terminal dimensions (220×12) and guards its initial state with not.toContain("line08 = 108"). This test uses not.toContain("line10 = 110"), which is a weaker bound—it would still pass even if the panel showed nine content lines instead of the expected seven.
Because the very next action (scrollDown(2)) relies on line08 not being visible yet, the initial negative assertion should mirror that boundary to prevent a silent false-positive if layout ever renders more content rows:
| expect(initial).not.toContain("line10 = 110"); | |
| expect(initial).toContain("line01 = 101"); | |
| expect(initial).not.toContain("line08 = 108"); |
There was a problem hiding this comment.
This one no longer applies — I reworked the PR away from the scrollbar-track test and into a mouse menu interaction test because the scrollbar path was too flaky on CI. The current test no longer uses that initial scroll boundary.
This comment was generated by Pi using OpenAI o4-mini
| test("scrollbar track clicks page the review pane up and down", async () => { | ||
| const fixture = harness.createScrollableFilePair(); | ||
| const terminalCols = 220; | ||
| const scrollbarX = terminalCols - 2; |
There was a problem hiding this comment.
Magic
- 2 offset should be commented
scrollbarX = terminalCols - 2 produces column index 218 in a 220-column terminal. The offset is not obvious—readers will wonder whether it accounts for a right-side border cell, a 1-indexed/0-indexed conversion, or the gutter width. A brief comment would make the intent clear:
| const scrollbarX = terminalCols - 2; | |
| const scrollbarX = terminalCols - 2; // scrollbar track sits one column left of the right edge |
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!
There was a problem hiding this comment.
Also obsolete after the test rewrite: the current PR no longer computes a scrollbar column offset, since it now exercises mouse-driven menu interaction instead of scrollbar clicks.
This comment was generated by Pi using OpenAI o4-mini
247c9c2 to
27c4ad4
Compare
27c4ad4 to
55b927a
Compare
Summary
Viewmenu in a real terminal opens the dropdownStacked viewswitches the live diff layout from split to stackTesting
This PR description was generated by Pi using OpenAI o4-mini