fix(ui): stabilize virtual follow autoscroll#406
Conversation
Keep DOM and Virtua timing in VirtualFollowList while moving pure follow and hold transitions into a colocated behavior module with focused tests. The integration restores hidden session scroll snapshots safely, gates streaming magnetization on active streaming and hold state, and preserves explicit latest/submit bottom jumps without reintroducing fast-down heuristics. Use Virtua offsets for scroll snapshots and decide streaming rejoin from the item at the bottom of the viewport so long assistant messages can still magnetize near their tail without using stale top-of-viewport position. Remove the unused generic scroll cache hook now that message scroll restore is handled by the virtual follow integration. Validation: node --test packages/ui/src/components/virtual-follow-behavior.test.ts, npm run typecheck --workspace @codenomad/ui, npm run build --workspace @codenomad/tauri-app, and manual release-exe testing.
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
|
Understood request: gatekeeper review of the latest PR #406 state, focused on regressions, implementation quality, and whether this should be accepted as-is. Assumption: review only; I did not make code changes. Gatekeeper result: not ready as-is. Findings:
A safer implementation would pass the old session’s last-good snapshot into the cleanup, or store
This needs an integration-level test because the pure controller tests do not cover the
Validation run:
I would request changes before merging, specifically for findings 1 and 2. -- |
Keep last-good scroll snapshots keyed by session so cleanup during session switches cannot write another session's snapshot under the previous session id. Also allow streaming rejoin while the hold preference is enabled when no hold target is actively suspending bottom pinning. Validation: node --test packages/ui/src/components/virtual-follow-behavior.test.ts and npm run typecheck --workspace @codenomad/ui.
|
Addressed in
Validation rerun:
The file-size note is acknowledged; |
|
same review again |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/25557064452 Artifacts expire in 7 days. |
|
Understood request: repeat the same gatekeeper review on the latest PR state after Gatekeeper result: no blocking findings in this pass. Findings: None found that I would block merge on after the Prior findings check:
Residual risks / non-blocking notes:
Validation run:
Recommendation: acceptable to proceed if CI typecheck/build is green in the proper dependency environment and the manual scroll scenarios from the PR description were rerun after -- |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/25557064452 Artifacts expire in 7 days. |
1 similar comment
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/25557064452 Artifacts expire in 7 days. |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/25597432431 Artifacts expire in 7 days. |
# Conflicts: # packages/ui/src/components/session/session-view.tsx
|
@pascalandr - Are we good with this implementation? |
|
Yes |
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
|
Understood request: fresh gatekeeper review of the latest PR #406 state after the current Gatekeeper result: no blocking findings in this pass. Findings: None found that I would block merge on in the current diff. What I checked:
Validation run:
Non-blocking quality notes:
Recommendation: acceptable to proceed if CI typecheck/build passes in the proper dependency environment and the manual release scroll scenarios were rerun after the latest -- |
|
|
Understood request: answer the two focused review questions against the latest PR state. Assumption: analysis only; I did not make code changes.
Yes. The PR does add more work on content-render notifications, and in fast streaming that can translate into more per-frame work. The hot path is
This is not necessarily catastrophic because some upstream renderers already coalesce before calling The first improvement I would ask for if we want to harden performance is to coalesce
Yes. The current implementation is doing the right kind of state separation by moving pure follow behavior into Best simplification options, in order of impact:
My gatekeeper position: I would not block this PR solely on performance/simplification if manual fast-stream testing was good and CI is green, because it fixes real scroll correctness problems. But I would strongly prefer at least the -- |
|
How about onCreateEffect or any other effects? |
|
Understood request: assess the PR’s Solid effects specifically, including Short answer: most added effects are event/state-transition effects and are not frame-hot by themselves, but there are two places I would tighten. Effect review:
The one effect-adjacent concern I would call out is actually a memo, not a
I would not call that a blocker, but if the question is “where could effects/memos be simplified for fast streaming,” this is my top candidate after Simplification recommendations:
Gatekeeper position: I still would not block solely on these effects if manual fast-stream testing is good, but I would strongly prefer the -- |
|
Hey @pascalandr That's a lot of work for just fixing leaving autoscroll when trying to scroll up. |
|
@shantur Lol that's not the only point, check summary.
Do you want me to make the improvements recommended by CodenomadBot now ? |
If you think those are valid, please do. |
Fast streaming renders can notify VirtualFollowList multiple times in the same animation frame, repeating follow reconciliation, Virtua metric reads, and bottom-pin checks. Coalescing notifyContentRendered behind a single RAF keeps the existing follow and hold behavior while capping duplicate same-frame work during rapid updates. The change preserves the current notification path, falls back to immediate execution when requestAnimationFrame is unavailable, and cancels pending frames during cleanup so cached panes do not flush stale work after teardown. Validation:\n- node --test packages/ui/src/components/virtual-follow-behavior.test.ts\n- npm run typecheck --workspace @codenomad/ui
…s open (NeuralNomadsAI#459) ## Summary Fixes a mobile UX bug where, with the session list (left drawer) open on phone layout, the still-visible instance/project tab bar at the top was non-interactive. Tapping a tab did nothing — users had to close the drawer first and then switch projects/instances. After this change, tapping a tab in that bar switches the instance/project **and** the drawer auto-closes in a single gesture, matching user expectation. ## Root cause On phone (`max-width: 767px`), `InstanceShell` renders the left session sidebar via MUI's `Drawer variant="temporary"`. The drawer paper is offset down to `floatingTopPx()` so the instance tab bar remains visually visible above it. However, MUI's `Modal` Backdrop is `position: fixed; inset: 0` and covers the entire viewport — including the area over the tab bar. The backdrop is styled `backgroundColor: transparent` (so it's invisible) but it still captures pointer events at `z-index: 60`. Taps over the tab bar hit the transparent backdrop → MUI calls `onClose` (closing the drawer) but the tab's click handler never fires. ## Implementation 1. **`packages/ui/src/components/instance/instance-shell2.tsx`** — Constrain the MUI Drawer Backdrop via `sx` overrides on both the left (session list) and right drawers so the backdrop is bound to the drawer paper's vertical range (`top: floatingTopPx(); height: floatingHeight();`) instead of fullscreen. Taps over the tab bar now reach the tab buttons. 2. **`packages/ui/src/styles/panels/tabs.css`** — Lift `.tab-bar-instance` to `position: relative; z-index: 70` so it stacks deterministically above the drawer (z-index 60) across browsers as defense-in-depth. 3. **`packages/ui/src/components/instance/shell/useDrawerChrome.ts` + `instance-shell2.tsx`** — Expose `closeFloatingDrawersIfAny` from `useDrawerChrome` and call it from an `InstanceShell` effect that fires when `props.isActiveInstance` flips `true → false`. This closes any open floating drawer on the instance the user just switched away from, so its previously-open state doesn't bleed back when the user returns to that tab later. Tablet (>=768px) and desktop (>=1280px) layouts use pinned drawers (no temporary modal), so the backdrop constraint and z-index lift are inert there. The fix applies symmetrically to the right drawer on phone. ## Verification - `tsc --noEmit` clean. - `vite build` clean. - Manual mobile-emulation checklist in the task file (`tasks/done/058-mobile-session-list-blocks-tab-switch.md`) and SUMMARY in `evidences/058-mobile-session-list-blocks-tab-switch/`. ## Reviewer manual check (phone viewport <=767px) - Open session list drawer. Tap a different instance tab → it activates AND drawer closes. - Tap "+" while drawer open → folder picker opens, drawer closes. - Tap Settings / Notifications / Remote while drawer open → action fires, drawer closes. - Same checks against the right drawer. - Tap area below tab bar but outside drawer paper → drawer still closes (existing backdrop dismissal preserved). - Resize to tablet and desktop widths → pinned drawers unaffected. - No visual regression in light or dark theme. --------- Co-authored-by: Ubuntu <omer@Omer.dn3uxh3znnmu5eefnjnut0i1af.tlvx.internal.cloudapp.net> Co-authored-by: Shantur Rathore <i@shantur.com>
3 participants
Summary
VirtualFollowListand moving pure follow/hold behavior into a colocated tested module.Supersedes / fixes
Validation
node --test packages/ui/src/components/virtual-follow-behavior.test.tsnpm run typecheck --workspace @codenomad/uinpm run build --workspace @codenomad/tauri-app