do not yank chat scroll when the user has scrolled up - #3158
do not yank chat scroll when the user has scrolled up#3158thedhruvhegde wants to merge 1 commit into
Conversation
Greptile SummaryThis PR changes idle prompt submission so an existing scroll anchor is preserved rather than unconditionally pinning the optimistic prompt.
Confidence Score: 4/5This PR is not yet safe to merge because a subsequent idle submission can remain anchored to the previous prompt instead of pinning the newly sent prompt.
Files Needing Attention: apps/emdash-desktop/src/core/features/conversations/browser/acp/acp-chat-store.ts, apps/emdash-desktop/src/renderer/tests/browser/acp-chat-store-submission.test.ts
|
| Filename | Overview |
|---|---|
| apps/emdash-desktop/src/core/features/conversations/browser/acp/acp-chat-store.ts | Preserves anchor modes during idle submission, but conflates user-parked anchors with prior programmatic pin anchors. |
| apps/emdash-desktop/src/renderer/tests/browser/acp-chat-store-submission.test.ts | Adds a tail scroll getter to the fixture without testing either branch of the new scroll behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
S[Submit prompt while idle] --> R[Read persisted scroll mode]
R -->|tail| P[Pin new optimistic prompt]
R -->|anchor| K[Keep existing anchor]
A[Previous pin-on-send] -->|also produces anchor| R
K --> O[Viewport may remain pinned to previous prompt]
Prompt To Fix All With AI
### Issue 1
apps/emdash-desktop/src/core/features/conversations/browser/acp/acp-chat-store.ts:471
**Prior anchors block new pins**
The new guard treats every `anchor` as evidence that the user scrolled away, but `pinTopMode()` also creates an anchor when a prompt is sent. Response completion does not automatically restore `tail`, so a later idle submission can leave the viewport anchored to the previous prompt instead of pinning the new optimistic prompt. The code needs to distinguish a user-parked position from a prior programmatic pin.
### Issue 2
apps/emdash-desktop/src/renderer/tests/browser/acp-chat-store-submission.test.ts:376
**Scroll branches remain untested**
The updated fixture always returns `tail`, so it never exercises the new `anchor` branch or asserts either required result: preserving an existing anchor and pinning a tail submission. Add focused cases for both states so regressions in this scroll behavior do not pass unnoticed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "do not pin the chat to a new prompt when..." | Re-trigger Greptile
| this._view?.setScrollMode(pinMode); | ||
| this.chatState.scroll.set(pinMode); | ||
| const currentScroll = this.chatState.scroll.get?.() ?? { kind: 'tail' as const }; | ||
| if (currentScroll.kind !== 'anchor') { |
There was a problem hiding this comment.
The new guard treats every anchor as evidence that the user scrolled away, but pinTopMode() also creates an anchor when a prompt is sent. Response completion does not automatically restore tail, so a later idle submission can leave the viewport anchored to the previous prompt instead of pinning the new optimistic prompt. The code needs to distinguish a user-parked position from a prior programmatic pin.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/emdash-desktop/src/core/features/conversations/browser/acp/acp-chat-store.ts
Line: 471
Comment:
**Prior anchors block new pins**
The new guard treats every `anchor` as evidence that the user scrolled away, but `pinTopMode()` also creates an anchor when a prompt is sent. Response completion does not automatically restore `tail`, so a later idle submission can leave the viewport anchored to the previous prompt instead of pinning the new optimistic prompt. The code needs to distinguish a user-parked position from a prior programmatic pin.
**Knowledge Base Used:**
- [Chat state, streaming, and composition](https://app.greptile.com/emdash/-/custom-context/knowledge-base/generalaction/emdash/-/docs/chat-state-and-composition.md)
- [Agent chat experience](https://app.greptile.com/emdash/-/custom-context/knowledge-base/generalaction/emdash/-/docs/chat-experience.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| history: { seed: historySeed }, | ||
| }, | ||
| scroll: { set: vi.fn() }, | ||
| scroll: { get: () => ({ kind: 'tail' }), set: vi.fn() }, |
There was a problem hiding this comment.
Scroll branches remain untested
The updated fixture always returns tail, so it never exercises the new anchor branch or asserts either required result: preserving an existing anchor and pinning a tail submission. Add focused cases for both states so regressions in this scroll behavior do not pass unnoticed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/emdash-desktop/src/renderer/tests/browser/acp-chat-store-submission.test.ts
Line: 376
Comment:
**Scroll branches remain untested**
The updated fixture always returns `tail`, so it never exercises the new `anchor` branch or asserts either required result: preserving an existing anchor and pinning a tail submission. Add focused cases for both states so regressions in this scroll behavior do not pass unnoticed.
---
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!
summary
every non-busy submit forced
pinTopMode, which rewrote scroll intent even if the user had parked the transcript.pin-on-send now only runs when the current intent is not already an anchor (user scrolled away).
closes #2006
test plan