Skip to content

Prevent chat restore into reserved blank space - #143

Closed
hamzamerzic wants to merge 4 commits into
mobius-os:mainfrom
hamzamerzic:fix/prevent-blank-scroll-restore-20260722
Closed

Prevent chat restore into reserved blank space#143
hamzamerzic wants to merge 4 commits into
mobius-os:mainfrom
hamzamerzic:fix/prevent-blank-scroll-restore-20260722

Conversation

@hamzamerzic

Copy link
Copy Markdown
Collaborator

Summary

  • Treat the dynamic reply spacer as layout reservation rather than readable conversation content when deriving and restoring chat scroll anchors.
  • Self-heal stale saved anchors that put the whole viewport below the last real message.
  • Reuse the same content-intersection invariant for persistence, restoration, foreground return, spacer sizing, and attention-card navigation.

Why

A chat could be left while scrolled inside the blank space reserved for pinning a reply. The old controller persisted the last message with a very large negative offset because it always fell back to the final row even when no row intersected the viewport. Reopening then recreated the blank screen indefinitely.

Review coverage

Two local review passes traced mount restoration, chat exit, foreground return, nudge navigation, pin/follow transitions, spacer growth, viewport resize, and malformed/stale saved modes. The implementation fixes the existing scroll-mode ownership path rather than adding a parallel correction.

Validation

  • 68 focused scroll-mode tests pass
  • full frontend suites: 1,773 library + 47 hook tests pass
  • production Vite/PWA build passes
  • pre-push privacy/syntax/frontend gate passes
  • git diff --check passes

@hamzamerzic hamzamerzic added area: ui User interface and interaction design bug Something isn't working labels Jul 22, 2026
@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Two thorough review rounds complete on exact head 1401b1d7. Pass 1 traced the failure from topmost-visible-row selection through chat-exit persistence and mount restoration: a viewport wholly inside the dynamic reply spacer no longer manufactures a last-row anchor with a huge negative offset. Pass 2 checked foreground return, attention-card navigation, saved PIN/FOLLOW normalization, viewport resize, anchor spacer sizing, partial-row boundary conditions, and legacy invalid-state self-healing. The fix stays inside the existing scroll-mode ownership and persistence funnel. Validation: 68 focused tests; 1,773 library + 47 hook tests; production build and pre-push gate all pass. No remaining finding; hosted CI is the final gate.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Current-main integration pass complete on exact head 0d129c66. The sole conflict was adjacent test coverage from the newly landed automatic-continuation marker work; the resolution preserves both invariants, and _validateSavedMode still excludes synthetic continuation rows via isOwnerUserMessage. Focused scroll suite is now 69/69 and production build/pre-push frontend gate pass. The PR is mergeable again and hosted CI has been triggered.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Temporary merge-order hold: do not merge this exact head after #144 without updating it first. 0d129c66 merged current main only to preserve #139’s adjacent automatic-continuation owner-message regression. Once #144 restores safe main, I will merge/rebase that reverted main here and rerun the 69 scroll tests so #143 cannot reintroduce any #139 files through ancestry.

@hamzamerzic
hamzamerzic force-pushed the fix/prevent-blank-scroll-restore-20260722 branch from 0d129c6 to 1736476 Compare July 22, 2026 18:34
@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Post-revert reconciliation pushed as exact head 17364766b1ad451440d129c85e6cec68b23db654. I rebuilt the branch from restored main 38f2e158 and replayed only the reviewed scroll repair. The resulting PR is one commit / two files and its tree is byte-identical to the original reviewed production repair 1401b1d7; the #139 isOwnerUserMessage import, automatic-continuation saved-pin behavior, and associated test are absent. Merge-tree is clean. Independent local verification on this exact tree: 68/68 focused scroll-state tests, production Vite/PWA build, and git diff --check all pass. The exact diff still covers capture, persistence, restore, foreground/chat exit, spacer sizing, attention nudge, valid partial-row preservation, and invalid blank-reservation self-healing. Awaiting the fresh hosted checks before final approval verdict.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Safety hold cleared on final exact head 17364766b1ad451440d129c85e6cec68b23db654. The branch was cleanly rebased onto reverted main after #144 merged; GitHub now shows exactly the intended two scroll files and one feature commit, with no #139 residue. I independently built the same post-revert tree, reran all 68 focused scroll tests and production build, and verified byte-identical source against this rebased head. No remaining finding; fresh hosted CI is the final gate.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

The hosted E2E run exposed five regressions in live scroll ownership: restoring the lifecycle invariant globally made the live reader path lose its fallback anchor while the viewport was inside the dynamic reservation. That broke follow/hold transitions and pin/spacer clamping.

I rebased the repair onto the newer eaa2476d update rather than overwriting it. The combined result now keeps live ANCHOR_AT ownership and its exact reservation, while contentHoldModeFromScroll and the persistence/restore gates accept only anchors that intersect real transcript content and otherwise settle at the real tail.

Local verification on the combined head dcc45aef:

  • 1,775 frontend library tests passed
  • 47 hook tests passed
  • production build passed
  • pre-push gate passed

The exact browser E2E suite is not locally available here, so the new hosted E2E run is the remaining proof for the five previously failing cases.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Thorough re-review — round 1/2 (exact dcc45aef)

I traced each of the five failed browser cases back through the live mode owner, spacer calculation, lifecycle persistence, and mount validation. The corrected split is now explicit:

  • live scrolling may anchor the last real row while the viewport traverses reserved room, preserving reader ownership during layout/stream growth
  • live spacer sizing keeps that exact target reachable
  • foreground/chat-exit capture accepts an exact anchor only when it intersects real conversation content; otherwise it settles at the real tail
  • saved ANCHOR_AT geometry is validated before every write and again on restore, while useful live FOLLOW_BOTTOM/PIN_USER_MSG state is not prematurely erased

This preserves the original blank-restore fix without applying its content-intersection rule to the live path that needs reservation geometry. The combined unit suite and production build pass. No further code issue found in round 1; hosted E2E remains the required device-browser proof.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Thorough re-review — round 2/2 (persistence and race audit)

The independent pass checked every persistence call site and the mode transitions around gestures, disclosure/layout changes, foreground return, chat exit, pagehide, and unmount. There is one durable write owner; invalid ANCHOR_AT geometry cannot bypass _modeForPersistence, and mount still revalidates older state. The live-only fallback is not used by lifecycle capture because contentHoldModeFromScroll applies the content-intersection gate first.

I also checked the combined newer-author fix for premature FOLLOW/PIN collapse: it composes correctly with the live spacer repair, and moving away from physical bottom now retires follow even when no real row intersects. git diff --check is clean; 1,775 library tests, 47 hook tests, the production build, and the pre-push gate all pass on this head.

No further change requested in round 2. Do not call this merge-ready until the exact-head hosted E2E run confirms the five previously failing scroll scenarios.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Current exact review head is 230332e028fe9a3bbca9a0915626f1ece412fc94 (superseding dcc/eaa).

This final delta restores the owner-authoritative ARCHITECTURE R5a/R4a behavior for explicit question/paused attention nudges: the live one-shot action reaches the true physical tail and remains a settled ANCHOR_AT, never FOLLOW_BOTTOM. It retains the new live-vs-durable split: a live off-content physical anchor may hold its reservation, but _modeForPersistence and mount restore normalize it to the spacer-excluding real-content tail, so reload cannot recreate the blank viewport.

The regression now asserts both halves directly (live physical target and safe persisted target). Exact local evidence: 71/71 focused scroll tests, full frontend library + 47/47 hook tests, production Vite/PWA build, and git diff --check pass. The diff remains confined to the two scroll-controller files and contains no #139 semantics. Hosted run 29949521522 is the final browser/backend gate. The PR is behind current main (#129), so this head should not be merged directly; replay only the cleared scroll diff into the protected current-base integration.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Final exact-head verdict for 230332e028fe9a3bbca9a0915626f1ece412fc94: approve / no blocker. Hosted run 29949521522 is green across all seven jobs, including backend and the full E2E suite; the five deterministic scroll failures from 17364766 are resolved. This source branch remains behind current main, so the cleared artifact is its reviewed patch (already replayed patch-identically into #147), not a direct merge of the stale ancestry.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Landed on protected main through reviewed integration #147 (merge 226106c). The integrated patch matches this PR's reviewed head; closing the superseded source PR to keep the queue truthful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ui User interface and interaction design bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant