test(pty): cover add-note context rows#351
Conversation
Greptile SummaryThis PR adds a single PTY integration test verifying that context/unchanged rows in a deletion-only diff expose the
Confidence Score: 4/5Safe to merge — change is test-only and introduces no production code paths. The test correctly covers the intended context-row scenario and follows established patterns throughout the file. The one concern is that revealAddNoteNear's row-fallback logic could allow the test to pass even if the context-row affordance is broken, since the adjacent deletion row would satisfy the [+] check. This doesn't affect the production build, but leaves the new coverage weaker than it appears. test/pty/ui-integration.test.ts — specifically the hover-fallback logic in the new test block. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test
participant S as PTY Session (hunk)
participant H as Harness
T->>H: createDeletionOnlyFilePair()
T->>H: launchHunk(diff, split, 120x16)
H-->>T: session
T->>S: "waitForText(/keep = true/, 15s)"
S-->>T: initial snapshot
T->>T: "lineIndexOf(initial, "keep = true") → targetRow"
loop revealAddNoteNear(targetRow)
T->>S: moveMouse(x, y) near targetRow
S-->>T: waitForText(/[+]/, 200ms)
end
T->>S: click(/[+]/)
S-->>T: waitForText(/Draft note/)
T->>S: type("Save this context draft.")
T->>S: press(ctrl+s)
S-->>T: waitForText(/Your note/)
T->>T: expect(saved).toContain("Save this context draft.")
T->>S: session.close()
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
test/pty/ui-integration.test.ts:742-768
**Hover fallback may mask a broken context-row `[+]`**
`revealAddNoteNear` probes the exact `targetRow` first, but if no `[+]` appears there it silently falls through to `targetRow ± 1` — which in this fixture is the `removeMe` deletion row, a row that is already known to surface `[+]` from the previous test. If the context-row affordance were broken (i.e., hovering `keep = true` never shows `[+]`), the helper would succeed against the adjacent deletion row, the test would still pass, and the regression would go undetected. Consider either (a) asserting that the `[+]` was found without falling back, or (b) calling `moveMouse` directly at the `targetRow` and asserting `[+]` appears before proceeding to `session.click`.
Reviews (1): Last reviewed commit: "test(pty): cover add-note context rows" | Re-trigger Greptile |
| test("clicking context-row add-note affordances can save draft notes", async () => { | ||
| const fixture = harness.createDeletionOnlyFilePair(); | ||
| const session = await harness.launchHunk({ | ||
| args: ["diff", fixture.before, fixture.after, "--mode", "split"], | ||
| cols: 120, | ||
| rows: 16, | ||
| }); | ||
|
|
||
| try { | ||
| const initial = await session.waitForText(/keep = true/, { | ||
| timeout: 15_000, | ||
| }); | ||
| const targetRow = lineIndexOf(initial, "keep = true"); | ||
| expect(targetRow).toBeGreaterThan(0); | ||
|
|
||
| await revealAddNoteNear(session, targetRow); | ||
| await session.click(/\[\+\]/); | ||
| await session.waitForText(/Draft note/, { timeout: 5_000 }); | ||
| await session.type("Save this context draft."); | ||
| await session.press(["ctrl", "s"]); | ||
| const saved = await session.waitForText(/Your note/, { timeout: 5_000 }); | ||
|
|
||
| expect(saved).toContain("Save this context draft."); | ||
| } finally { | ||
| session.close(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Hover fallback may mask a broken context-row
[+]
revealAddNoteNear probes the exact targetRow first, but if no [+] appears there it silently falls through to targetRow ± 1 — which in this fixture is the removeMe deletion row, a row that is already known to surface [+] from the previous test. If the context-row affordance were broken (i.e., hovering keep = true never shows [+]), the helper would succeed against the adjacent deletion row, the test would still pass, and the regression would go undetected. Consider either (a) asserting that the [+] was found without falling back, or (b) calling moveMouse directly at the targetRow and asserting [+] appears before proceeding to session.click.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/pty/ui-integration.test.ts
Line: 742-768
Comment:
**Hover fallback may mask a broken context-row `[+]`**
`revealAddNoteNear` probes the exact `targetRow` first, but if no `[+]` appears there it silently falls through to `targetRow ± 1` — which in this fixture is the `removeMe` deletion row, a row that is already known to surface `[+]` from the previous test. If the context-row affordance were broken (i.e., hovering `keep = true` never shows `[+]`), the helper would succeed against the adjacent deletion row, the test would still pass, and the regression would go undetected. Consider either (a) asserting that the `[+]` was found without falling back, or (b) calling `moveMouse` directly at the `targetRow` and asserting `[+]` appears before proceeding to `session.click`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch. I added a same-row-only helper for this context-row case, so the test can try nearby columns but no longer falls back to adjacent rows and cannot pass by hitting the deletion row. Re-ran the context test 5x plus the add-note PTY subset, typecheck, and lint.
Responded by pi using GPT-5.
0f95d07 to
fcf5eca
Compare
Summary
[+]Tests