Repository navigation
fix(ui): keep comment targets fixed in viewport - #809
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThe PR changes inline-comment targeting and stream scroll restoration so keyboard-created composers preserve the active line's terminal row, while retaining full reveal behavior when current-line navigation is disabled.
Confidence Score: 4/5The mouse-targeted comment path should be fixed before merging because an unrelated keyboard cursor can still move the clicked target in the viewport. Non-reveal draft restoration always prefers lineCursor, while explicit mouse targets also select that path without ensuring the cursor identifies the draft row. Files Needing Attention: src/ui/components/panes/DiffPane.tsx, src/ui/App.tsx, test/pty/notes.test.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start inline note] --> B{Explicit target or cursor?}
B -->|Cursor| C[Read current line cursor synchronously]
B -->|Explicit target| D[Use clicked file, hunk, and line]
C --> E[Create draft with viewport preservation]
D --> E
E --> F[Composer enters stream geometry]
F --> G{scrollToNote?}
G -->|Yes| H[Reveal full composer]
G -->|No| I[Restore cursor-relative scroll position]
Prompt To Fix All With AI### Issue 1
src/ui/components/panes/DiffPane.tsx:1821
**Unrelated cursor anchors clicked drafts**
When a user opens an inline comment by clicking a line different from the keyboard cursor, `scrollToNote` remains false and this code anchors restoration to `lineCursor` instead of the draft target, causing composer insertion to pull the clicked target upward.
### Issue 2
test/pty/notes.test.ts:123
**Regression tests are not colocated**
The new regression scenarios for behavior implemented in `App.tsx` and `DiffPane.tsx` live only under the separate `test/pty` tree, making the coverage harder to discover and maintain alongside the affected UI modules.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(ui): keep comment targets fixed in v..." | Re-trigger Greptile |
| sectionHeaderHeights, | ||
| previousSectionHeaderHeights, | ||
| ); | ||
| const cursorToPreserve = scrollToNote ? null : lineCursor; |
There was a problem hiding this comment.
When a user opens an inline comment by clicking a line different from the keyboard cursor, scrollToNote remains false and this code anchors restoration to lineCursor instead of the draft target, causing composer insertion to pull the clicked target upward.
Knowledge Base Used: UI App Shell: AppHost, App, and Controller Hooks
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/components/panes/DiffPane.tsx
Line: 1821
Comment:
**Unrelated cursor anchors clicked drafts**
When a user opens an inline comment by clicking a line different from the keyboard cursor, `scrollToNote` remains false and this code anchors restoration to `lineCursor` instead of the draft target, causing composer insertion to pull the clicked target upward.
**Knowledge Base Used:** [UI App Shell: AppHost, App, and Controller Hooks](https://app.greptile.com/modem/-/custom-context/knowledge-base/modem-dev/hunk/-/docs/ui-app-host.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
I verified this path before changing it: startUserNote synchronously retargets lineCursor to the clicked line before DiffPane restores the viewport, so the old keyboard cursor is not used here. I added PTY coverage that first moves the keyboard cursor to the deletion, then clicks the separate context row and asserts that the clicked row remains at the same terminal position after the draft opens.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
It looks like your message contains an unexpanded shell substitution — $(cat /tmp/reply-p1.md) was sent literally rather than its contents. Could you paste the actual text you meant to send?
There was a problem hiding this comment.
That's a convincing clarification. Looking at the App.tsx diff, the click path passes fileId explicitly, which means keyboardTarget is null and the review store receives the clicked target directly — lineCursor in DiffPane reflects whatever review.startUserNote commits synchronously, so by the time the useEffect runs in the next render, lineCursor already points to the clicked line. My concern was unfounded.
The PTY coverage you added is exactly the right verification for this kind of timing-sensitive path. Withdrawn.
| } | ||
| }); | ||
|
|
||
| test("opening a draft keeps the active line fixed while pushing following code down", async () => { |
There was a problem hiding this comment.
The new regression scenarios for behavior implemented in App.tsx and DiffPane.tsx live only under the separate test/pty tree, making the coverage harder to discover and maintain alongside the affected UI modules.
Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/pty/notes.test.ts
Line: 123
Comment:
**Regression tests are not colocated**
The new regression scenarios for behavior implemented in `App.tsx` and `DiffPane.tsx` live only under the separate `test/pty` tree, making the coverage harder to discover and maintain alongside the affected UI modules.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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.
Added colocated coverage in src/ui/AppHost.interactions.test.tsx. It sends a coalesced burst of eight Down events plus c, then verifies the draft targets L9, the active row stays fixed, and following code moves down. The PTY test remains as the real terminal integration layer and now uses readiness-bounded key presses to avoid the CI startup race.
This comment was generated by Pi using gpt-5.6-sol
339c66b to
721ab32
Compare
721ab32 to
256cc1b
Compare
Summary
copens an inline commentctargets the latest lineBefore
The dashed guide marks the target’s original row 23. When the composer opened, the active line jumped two rows above it to row 21.
After
The active line remains aligned with the row-23 guide and the composer grows directly below it.
Testing
bun run typecheckbun run lintbun run test:integration(119 passed)bun run test:tty-smoke(9 passed)This PR description was generated by Pi using gpt-5.6-sol