feat(desktop): preserve transcript continuity across bounded ranges - #4560
feat(desktop): preserve transcript continuity across bounded ranges#4560Sun-GLiang wants to merge 5 commits into
Conversation
fc447c8 to
fd473f0
Compare
fd473f0 to
c69e995
Compare
|
Thanks for the work, and I owe you an apology: I left you hanging after asking you to pause #4258 and #4295 on Aug 31, so you had to build against the outdated issue text. That is entirely on me. I have corrected #4123. Most of this PR's 6.7k lines trace back to two incorrect assumptions in the original issue:
The position-index chain ( Two scroll-layer issues to note regardless of scope:
Landing order: #4793 (pin owns scroll anchoring) then #4259 (containment CSS) must land first. Both touch the same scroller, and #4259 already conflicts with this branch. Next steps: A rebase will not fix this, as the shape follows issue text I got wrong. The corrected #4123 is now specific enough to build against directly: generic rows, a gap row, and existing pagers — no new persistence, no epoch bump. If you would rather not redo it after two false starts, let me know and I will take it over. Either way, I will review follow-ups within a day, not a week. |
c69e995 to
6b2c88a
Compare
6b2c88a to
d856588
Compare
d856588 to
9868f5f
Compare
Generated-by: Codex
9868f5f to
7fdc015
Compare
|
Thanks for substantially simplifying this! My earlier comments about the timer/extra scroll writer and waiting for #4793 no longer apply; #4793 is closed, and I don't see a reason to require #4259 first. One remaining P2 at AI-assisted review with Codex. |
|
@Astro-Han Fixed in b93cefee6. I reproduced the behavior in the Electron E2E: after explicit newer navigation, the newer range appeared briefly, then The fix stays at the existing The existing bounded-history E2E now waits across rendered frames after clicking the newer gap and verifies that both boundaries remain present, so the page stays advanced (regression). The added assertion fails before the fix and passes after it. Local verification passed: Desktop build, typecheck, 2163 Desktop tests, 101 architecture checks, and the targeted Electron E2E. |
|
Thanks for reproducing and fixing this! At One final integration check: #4831 has since changed the scroll authority on main. Please rebase onto that baseline and verify older → newer → latest again, including focus and visible position. No further redesign is requested. AI-assisted source review with Codex; I haven't independently rerun Electron. |
Keep focus on the activating boundary-gap button when its shared loading state temporarily disables it, without reclaiming focus after the reader moves elsewhere. Use preventScroll so focus recovery cannot disturb the preserved viewport anchor. Cover focus ownership in the UI harness and assert focus plus bounded visible-position continuity in the Electron older-to-newer-to-latest path. Generated-by: Codex
|
@Astro-Han Integrated the #4831 baseline in The added integration assertions exposed one focus issue: before the fix, the newer-gap button lost focus 10/10 times when the shared pending state temporarily rendered it disabled. Verification after the fix:
|
3ef3da3 to
0e1d09e
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 0e1d09e9cd0da48f86208fa7f20c0c5c4778a1cd (OPEN). No P0–P1; one P2 and two P3s below. This is a user-facing feature; merging is a human decision regardless.
What it does, honestly this time
Previously, scrolling into a history range with unloaded messages showed a banner at the top of the session ("you are viewing history, jump to latest") — but the missing range is below, so the banner's position had nothing to do with the fact it described. Now gaps render where they actually are: "earlier messages above" and "newer messages below" gap rows inside the transcript list, via a pure projection (transcript-row-projection.ts:47-53). Notably the "newer" gap goes before the currently active/streaming turn (falling back to append only when activeTurnId is missing), so the live reply stays at the bottom instead of looking like history. The old "jump to latest" ability moved onto the existing scroll-to-bottom button (app-shell.tsx:2859) — a more intuitive home.
P2 — "scroll to bottom" is wired via click interception, not a prop
packages/ui/src/chat-surface-layout.tsx:87-95 wraps in a display: contents div with onClickCapture invoking the callback, instead of giving TranscriptScrollButton an onActivate-style prop. Please add the prop and drop the wrapper div.
P3 — both gap rows share one pending flag and spin together
In chat-view.tsx, both directions read isPending={props.historyLoadPending === true} — pressing "load earlier" also spins the untouched "load newer" button. Focus is not stolen (verified in component logic), so this is purely visual; split pending by direction to fix.
P3 — scroll-to-bottom still runs a pending state when already at the bottom
Pressing it while already at the latest position still goes through pending. Cosmetic; consider short-circuiting.
What I could not judge
Whether the design-system Button disables while isLoading was inferred from a11y-test behavior, not read from source (package not installed locally) — affects only the P3 severity above.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Opus-Qronos-AstroHan 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移。当前 head 是 0e1d09e,未关闭。把位置不对的提示改成了缺口行,P2 是滚动到底部用点击拦截接的,建议改 prop,另两条 P3 是视觉问题。功能类合并由人类拍板。
Summary
Rebuilds transcript partial-history handling on current
mainaround the existing bounded-range seams.This intentionally does not add a position index, storage table, cursor protocol, compatibility-epoch change, alternate pager, or second scroll authority.
Fixes #4123
Current status
Ready for maintainer review. The rebuild and local verification are complete, and CI is green on the current head.
Verification
Current head:
0e1d09e9cd0da48f86208fa7f20c0c5c4778a1cdnpm --workspace @maka/desktop run build:with-depsnpm --workspace @maka/desktop run check:architecture— 101 checks passed plus architecture-ledger validation.npm run typecheck,npm run lint,npm run format:check, andgit diff --checkpassed.test— passed in 12m13s on the updated head.Review focus
loadBefore,loadAround,loadLatest, reading-anchor, and scroll-authority seamspreventScrolland no focus stealingAI use
Codex rebuilt the implementation from current
main, added tests, and reviewed the diff against the corrected issue and repository standards.Checklist
Does this PR entail a change in behavior?