fix(transcript): keep the reading position when a disclosure is toggled - #437
Conversation
Expanding a tool, thinking or activity title re-bottomed a pinned transcript, so the clicked title jumped up by the height of the opened detail, and collapsing it moved back. The content ResizeObserver re-pins on every content resize and cannot tell a stream from a reader opening a row, and `overflow-anchor: none` on the scroller means the browser cannot do it either. A manual disclosure now hands the title element it was toggled from to the scroll container that owns it, synchronously, before the expansion state changes. That container leaves follow mode and restores the title's viewport offset from inside its own ResizeObserver, for every frame of a height transition, adopting the browser's own boundary clamping instead of fighting it. The transcript scroller and every nested follow scroller (D302) share one controller, and a nested owner passes its hold outward, because growing the dock grows the transcript's content. The hold is released by real scroll input, the jump-to-latest control, a new turn, a hidden pane and every navigation; there is no delayed grab-back and the observer is untouched. Scroll input is now attributed to the scroller that can consume it: a press on a row control or an editable field is an ordinary click, a keystroke in a text field belongs to that field, and input a nested scroller consumes is not the outer scroller's gesture. Follow records the scroll position the scroller actually reached rather than the one requested, and a sub-pixel tolerance applies only to scroll events no gesture produced, so a fractional device pixel ratio no longer reads as the reader scrolling up. Regression proof: `pnpm test:e2e:transcript-disclosure` mounts the real scroll hooks and a real tool row in a real 600 CSS px Electron viewport; without this change the same fixture reports the clicked title moving by 1270px, and with it the title's offset and the scroll offset are unchanged. Refs #324
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings remain in disclosure anchoring, scroll-input attribution, and regression coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes transcript and nested-panel disclosure toggles so the clicked title retains its viewport position during content resizing.
Changes:
- Added disclosure anchoring, scroll-owner attribution, and nested follow-scroll integration.
- Wired tool, activity, thinking, delegate, and error disclosures.
- Added unit, contract, Electron E2E coverage, and documentation updates.
File summaries
| File | Summary / review note |
|---|---|
scripts/e2e/transcript-disclosure-anchor.tsx |
E2E fixture. Moderate (2): add activity-group and side-panel coverage. Moderate (1): exercise outer-notifier propagation. Moderate (1): assert renderErrors. |
scripts/e2e-transcript-disclosure-anchor.mjs |
E2E launcher. |
package.json |
Registers the new E2E command. |
docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md |
Chinese E2E documentation. Nit (3): unrelated network-policy edits diverge from the English plan. |
docs/spec/08-meta/decisions-log.md |
Records D429. Nit (2): add the corresponding Chinese entry. |
docs/spec/06-delivery/04-e2e-test-plan.md |
Adds the disclosure scenario and traceability. |
docs/spec/04-ux/09-interaction-patterns.md |
Documents disclosure reading behavior. Nit (2): add the Chinese translation. |
apps/desktop/test/transcript-settle.test.mjs |
Updates observer contract coverage. |
apps/desktop/test/transcript-disclosure-reading.test.mjs |
Adds disclosure wiring coverage. |
apps/desktop/test/subagent-transcript.test.mjs |
Updates follow-scroll contracts. |
apps/desktop/test/interaction-performance.test.mjs |
Updates scroll-input contracts. |
apps/desktop/test/disclosure-anchor.test.mjs |
Tests anchor and gesture math. |
apps/desktop/src/lib/transcript-scroll.ts |
Adds gesture classification and overflow handling. Moderate (2): treat any positive overflow as scrollable. |
apps/desktop/src/lib/scroll-input.ts |
Attributes DOM input to scroll owners. Moderate (1): include unmarked consuming regions. Moderate (1): avoid treating ordinary detail-text presses as scroll gestures. |
apps/desktop/src/lib/disclosure-anchor.ts |
Implements anchor calculations. |
apps/desktop/src/lib/disclosure-anchor-context.ts |
Provides disclosure-owner context. |
apps/desktop/src/hooks/use-follow-scroll.ts |
Integrates nested follow scrolling. |
apps/desktop/src/hooks/use-disclosure-anchor.ts |
Controls held anchors. Moderate (1): propagate release to the outer owner. |
apps/desktop/src/features/chat/transcript/ToolRow.tsx |
Anchors tool and nested-run disclosures. |
apps/desktop/src/features/chat/transcript/SubagentDetail.tsx |
Anchors delegate brief disclosure. Moderate (1): anchor the error-detail toggle before changing state. |
apps/desktop/src/features/chat/transcript/shared.tsx |
Anchors shared disclosures. |
apps/desktop/src/features/chat/transcript/hooks/useTranscriptScroll.ts |
Integrates transcript anchoring. Nit (2): remove the unused owner-attribute import. |
apps/desktop/src/features/chat/transcript/ChatTranscript.tsx |
Marks the transcript scroll owner. |
apps/desktop/src/features/chat/transcript/ActivityGroup.tsx |
Anchors activity-group titles. |
apps/desktop/src/components/workpanel/SubagentPanel.tsx |
Integrates nested panel ownership. |
Review details
Suppressed comments (6)
apps/desktop/src/features/chat/transcript/SubagentDetail.tsx:203
SubagentDetailhas a separatemessage-error-togglethat still callssetOpendirectly (line 108) and is rendered inside this follow scroller. Collapsing that error detail can therefore still trigger the resize observer's bottom re-pin and move the reader; the notifier added here only covers the delegate brief. Add the same pre-state-change anchor notification to the error-detail toggle, with its own button ref.
const notifyDisclosureAnchor = useDisclosureAnchorNotifier();
apps/desktop/src/hooks/use-disclosure-anchor.ts:94
- A nested disclosure is handed to both this owner and
outerNotifier, butreleaseonly clears this owner'sheldRef. When the user scrolls the nested dock, its gesture handler releases the inner hold while the outer handler correctly rejects the event asnestedOwner, so the outer hold remains indefinitely; later outer resize callbacks can keep restoring the stale title and prevent the transcript from responding normally. The release path must propagate outward along with the hold.
const release = useCallback(() => {
heldRef.current = null;
}, []);
apps/desktop/src/lib/scroll-input.ts:46
- This lookup only recognizes descendants marked with
data-scroll-owner. The transcript also contains real verticaloverflow: autoregions such as.tool-row-content,.tool-fields, and.tool-match-listthat are not marked, so wheel input consumed inside them still timestamps the outer transcript as a gesture. A layout scroll within the 200 ms window can then release follow even though the transcript never moved; include all consuming nested regions in the attribution or mark them as owners.
const nearestOwner =
target instanceof Element
? target.closest(`[${SCROLL_OWNER_ATTRIBUTE}]`)
: null;
const context: ScrollInputContext = {
apps/desktop/src/lib/scroll-input.ts:65
- Because
insideis true for every descendant of the scroll owner and the selector only excludes controls, a pointerdown on ordinary detail text/Markdown setspointerOnScrollSurfaceto true. That press does not move the scroller, butmarkScrollGestureimmediately releases a held disclosure and makes the next layout scroll look user-driven, contrary to the documented ordinary-detail-click behavior. Restrict this path to the owner/scrollbar target or defer release until actual scroll/pointer movement.
context.pointerOnScrollSurface =
inside &&
!(target instanceof Element && target.closest(CONTROL_SELECTOR) !== null);
scripts/e2e/transcript-disclosure-anchor.tsx:238
- This dock is mounted through a second React root, so its provider has no outer notifier. Consequently the E2E never executes
outerNotifier?.(title)or verifies that growing a nested dock leaves the transcript behind it in place, even though that outward propagation is a key part of the fix.
const dockContainer = mount(<FollowFixture tool={toolRowMessage("dock")} />);
const dockScroller = dockContainer.querySelector<HTMLElement>(
".subagent-run-rows",
);
const dockTitle = dockContainer.querySelector<HTMLElement>(
scripts/e2e/transcript-disclosure-anchor.tsx:267
renderErrorsis populated by React'sonUncaughtErrorcallback but is never asserted. The probe can therefore returnok: trueafter an uncaught render error if the remaining DOM still satisfies these checks, allowing this regression test to pass with a broken fixture.
return {
ok: true,
transcript: { before, expanded, collapsed },
dock: { before: dockBefore, expanded: dockExpanded },
- Files reviewed: 25/25 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| scrollHeight: number; | ||
| clientHeight: number; | ||
| }): boolean { | ||
| return geometry.scrollHeight > geometry.clientHeight + 1; |
| > | ||
| <div ref={contentRef} className="thread-content"> | ||
| <div style={{ height: 700 }} /> | ||
| <ToolRow message={messages[1]} /> |
| import { | ||
| isRecentScrollGesture, | ||
| isScrollGestureInput, | ||
| reduceTranscriptScroll, | ||
| SCROLL_OWNER_ATTRIBUTE, | ||
| TRANSCRIPT_SCROLL_ROUNDING_TOLERANCE_PX, | ||
| type ScrollInputType, | ||
| } from "../../../../lib/transcript-scroll"; |
| - A manual disclosure — a tool, thinking or activity title, a delegate's brief | ||
| toggle, or an error-detail toggle — holds the reading position of the scroller | ||
| that owns it (issue #324). The title is handed to that scroller before the | ||
| expansion state changes, follow mode is left, and the scroller restores the | ||
| title's viewport offset from its own resize observer for every frame of the |
| ## 2026-09-16 — A manual disclosure keeps the reader's place (D429) | ||
|
|
||
| - **A manual disclosure hands its own title to the scroller that owns it, | ||
| synchronously, before the expansion state changes.** The content | ||
| `ResizeObserver` still re-pins a pinned transcript on every content resize |
| - **预期**:上述绕过形态全部拒绝;公共 CDN 放行。解析到私网地址或 redirect 到回环会抛出策略错误,且不会请求私网目标。策略失败不重试。每次拒绝都带上 `NETWORK_POLICY_BLOCKED`(spec 08 §3.1),使安装面板能给出原因并提供重试,而不是让安装按钮无解释地保持禁用;市场列表也能把被拒绝的源与单纯不可达的源区分开。 | ||
| - **链接规格**:`05-security/01-security.md`、ADR 0243、`03-runtime/01-ipc-protocol.md` §12b | ||
| - **验收**:Security、Quality | ||
| - **里程碑**:M6+ | ||
| - **状态**:已自动化(`pnpm test:e2e:skill-market`、`apps/desktop/test/public-https-fetch.test.mjs`、`packages/shared/src/public-network.test.ts`) | ||
| - **状态**:已自动化(`pnpm test:e2e:skill-market`、`apps/desktop/test/public-https-fetch.test.mjs`、`apps/desktop/test/skill-market-scan.test.mjs`、`apps/desktop/test/skill-market-failure.test.mjs`、`packages/shared/src/public-network.test.ts`) |
…ure-scroll-anchor # Conflicts: # docs/spec/08-meta/decisions-log.md
…ure-scroll-anchor # Conflicts: # docs/spec/08-meta/decisions-log.md # docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md # docs/zh-CN/spec/08-meta/decisions-log.md
Summary
Opening or closing a tool / thinking / activity-group title in a transcript that is pinned to the bottom dragged the whole conversation: the clicked title jumped by the height of the opened detail (~240px in the report's fixture) and jumped back on collapse. It happened with the turn already finished, so
isRunningwas false and this was not streaming growth.The content
ResizeObserverinuseTranscriptScroll.tsre-pins on every content resize while pinned, and it cannot tell a stream from a reader opening a row..thread-scrollalready setsoverflow-anchor: none, so the browser will not compensate either.Changes
A manual disclosure now hands the title element it was toggled from to the scroll container that owns it, synchronously, before the expansion state changes. That container leaves follow mode and restores the title's viewport offset from inside its own
ResizeObserver, so every frame of an animated height change is covered, and it adopts the browser's own boundary clamping instead of fighting it.lib/disclosure-anchor.ts,lib/disclosure-anchor-context.ts,lib/scroll-input.ts,hooks/use-disclosure-anchor.ts(new) — the anchor math, the per-owner notification context, DOM-event-to-scroll-owner attribution, and the shared hold controller.hooks/use-follow-scroll.tsandtranscript/hooks/useTranscriptScroll.ts— both scroll owners restore the anchor ahead of follow, record the scroll position the scroller actually reached, and release the hold on real scroll input, the jump-to-latest control, a new turn, a hidden pane and every navigation.transcript/shared.tsx—useAutomaticDisclosurereturns atitleRefand notifies beforesetOpen; the error-detail disclosure is wired the same way.transcript/ToolRow.tsx,ActivityGroup.tsx,SubagentDetail.tsx,components/workpanel/SubagentPanel.tsx,ChatTranscript.tsx— titles carry the ref, scrollers are marked as owners, and a nested owner passes its hold outward.lib/transcript-scroll.ts— gesture classification (isScrollGestureInput), a vertical-onlyconsumesVerticalScroll, and a sub-pixel tolerance that applies only to scroll events no gesture produced.There is no delayed grab-back compensation, and the
ResizeObserveris neither removed nor bypassed — two existing contract tests still pinnew ResizeObserver(followScrollNow)andassert.doesNotMatch(... scheduleFollowScroll).Beyond the report's model
The report describes notifying "the container that owns the title". That is not sufficient: growing a nested dock also grows the transcript's content, so the outer owner must hold too, or it re-bottoms and drags the same title. A nested owner therefore propagates its hold outward. The same defect class was also found and fixed in the delegate-brief collapse and the error-detail collapse, which reproduce in the same scroller.
Behaviour change to note
A manual disclosure now leaves follow mode and shows the jump-to-latest control. That is what the issue asks for ("do not additionally grab back the bottom"; a reader who scrolled up keeps their place), and it is recorded in
04-ux/09-interaction-patterns.md§9.1 and D429.Validation
On the integrated local
main(merge commit2201cb43, after merging this branch):pnpm build:jspnpm --filter @pi-desktop/desktop typecheckpnpm lint:biomepnpm -r --if-present testpnpm docs:checkpnpm test:e2e:transcript-disclosure(new)pnpm test:e2e:transcriptpnpm test:e2e:layoutpnpm test:e2e:bootpnpm test:e2e:subagentspnpm test:e2e:theme-surfacesfailures: []New scenario
E2E-CHAT-disclosure-toggle-keeps-reading-position. The fixture (pnpm test:e2e:transcript-disclosure) mounts the real scroll hooks and a real tool row in a real 600 CSS px Electron viewport and clicks the title with a real DOM click:Reverting
apps/desktop/srcto the previous commit while keeping the committed tests reportsexpanding the tool moved the clicked title by -1270px, so the regression is real.Three existing source-contract assertions were updated (one in
transcript-settle, four ininteraction-performance, one insubagent-transcript). Each kept its intent — the observer still re-pins synchronously inside its callback, the gesture window still gates follow release, DOM input is still classified for real — they no longer pin the exact shape of the refactored code.Documentation:
04-ux/09-interaction-patterns.md§9.1,06-delivery/04-e2e-test-plan.md(scenario + two traceability rows),08-meta/decisions-log.mdD429, and thezh-CNmirror of the E2E plan.Remaining risk
grid-template-rowsmulti-frame animation path is not measured end to end; it is covered by the implementation plus the unit and contract tests.841vs840.909observation was not reproduced at a real fractional DPR; a regression guard was written from the reported numbers.pointerdownto the scroll element itself; that is reasoned from the code, not tested.Fixes #324