Skip to content

Harden timeline windowing and default it on for phones via config omission semantics - #2391

Closed
vburojevic wants to merge 5 commits into
get-bb:mainfrom
vburojevic:bb/mobile-perf2/timeline-windowing-rollout
Closed

Harden timeline windowing and default it on for phones via config omission semantics#2391
vburojevic wants to merge 5 commits into
get-bb:mainfrom
vburojevic:bb/mobile-perf2/timeline-windowing-rollout

Conversation

@vburojevic

Copy link
Copy Markdown
Contributor

What was wrong

The thread timeline virtualizer (#2011, hardened by #2260/#2300) was sound but not shippable on phones. TimelineWindowedItems re-read scroll geometry in a dependency-less layout effect after every React commit — a forced layout per streaming delta. The loader's Suspense fallback mounted and measured every loaded row while the TanStack chunk downloaded. The thresholds were desktop numbers (compact minimum 40 rows, overscan 8), so a 39-row phone thread never windowed. And the feature shipped behind a default-off experiment, leaving production phones with 1,519–5,249 DOM nodes on a 402×874 viewport — the #1 SPA-owned cause of the 10–39s mobile hangs. Defaulting it on for phones was blocked by the config payload itself: /system/config materialized every experiment from defaultExperiments, so a client could not distinguish "user chose false" from "user never chose".

What changed

  • Geometry-revision scheme replaces the depless layout effect: expand/collapse paths bump a revision through TimelineWindowingGeometryInvalidateContext; windowed lists re-read geometry only when the revision, item count, or scroll-root usability changes — one read per trigger, 0.5px setScrollMargin guard unchanged.
  • The windowing chunk warms at boot beside the SplitWorkspaceRoute preload (still a dynamic import outside both closures); the captureMeasurements fallback mounts only the trailing 60 rows.
  • Thresholds: compact top-level minItemCount 40 → 16 (desktop 60, nested 20 unchanged); overscan 8 → 4 on compact viewports.
  • ThreadTimelineSurface: windowing resolves as experiment ?? isCompactViewport — compact windows whenever the experiment is unset, explicit false still disables everywhere (the kill switch), desktop keeps the served value.
  • Semantic omission at the config boundary (per the repo's optional-field rule — omission means "user never chose"): /system/config serves only persisted experiment choices (storedExperimentsSchema partial record, getStoredExperiments); setExperiments still persists every key on save, so the first explicit save wins everywhere after. Server-internal policy keeps concrete booleans via getExperiments. Consumers needing concrete values overlay defaultExperiments at their boundary (Settings on web and mobile, editMessages readers whose default is true, the CLI update); the connect plugin's mobileApp gate fails closed; the offline fallback omits timelineWindowing so an unreachable server still yields the compact default. The experiments PUT contract is unchanged. Nothing crosses the server↔host-daemon wire.
  • Docs in the same change: docs/configuration.md § Experiments, bb-cli skill surfaces.

How you verified

Fail-before/pass-after: geometry-trigger test (expected 4 to be 3 — a forced layout read per commit before); compact-default surface test (expected 'false' to be 'true' before); omission route test (expected { changelogPreview: false, …(4) } to deeply equal {} before), which also asserts getExperiments stays concrete. Full suites: @bb/app 3,352; @bb/server 2,002; @bb/db 406; @bb/mobile 846; connect plugin 90. check:bundle green with the windowed chunk outside both closures. Merged-tree WebKit QA on the perf fixture (1,408-event thread, iPhone viewport): 9 mounted rows / 1,362 DOM nodes with no experiment set (compact default active end-to-end), scroll up/back with no blanking, desktop stays unwindowed (107 rows), zero console errors.

AGENT GENERATED

vburojevic and others added 5 commits August 24, 2026 14:02
TimelineWindowedItems re-read its scroll geometry (two
getBoundingClientRect calls plus scrollTop) in a dependency-less layout
effect after every React commit, which forced a layout per streaming
delta. Replace it with a geometry-revision scheme: expand/collapse paths
(ExpandableTimelineRow's isExpanded transitions) bump a revision through
TimelineWindowingGeometryInvalidateContext, and the windowed list re-reads
only when the revision, its item count, or scroll-root usability changes.
A trigger-string ref leaves the mount read to the ResizeObserver effect so
each trigger costs one geometry read; the 0.5px setScrollMargin guard is
unchanged.

New tests prove commits without a geometry trigger no longer force a
layout read, that a revision bump re-reads and repositions the window
when the owning row's expansion moves a nested list, and that a real
row toggle inside ThreadTimelineRows still refreshes geometry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First open of a long thread on a cold connection paid a serialized boot
parse -> route chunk -> windowed chunk download while the Suspense
fallback mounted and measured every loaded row, then remounted the whole
list once TanStack landed. Warm the TimelineWindowedItems chunk from
App.tsx next to the SplitWorkspaceRoute preload (still a dynamic import,
so it stays out of the boot closure), and bound the captureMeasurements
fallback to the trailing 60 rows - the bottom-anchor region a thread
opens into.

Verified with the timeline suites and check-bundle-budget: boot payload
443.2 / 467.8 KiB brotli, and the TimelineWindowedItems chunk appears in
neither bootChunks nor the SplitWorkspaceRoute closure.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 40-row compact minimum and 8-row overscan were desktop numbers: 39
rows on a phone never windowed at all, and 16 retained rich rows of
overscan is most of a phone's DOM budget. Window top-level lists from 16
rows on compact viewports (desktop stays 60, nested stays 20) and halve
overscan to 4 there, using the same useIsCompactViewport signal that
already drives the row minimum.

Verified with new tests: a 20-row compact top-level timeline now windows,
and the compact render retains exactly one overscan side's worth fewer
wrappers than the desktop render of the same geometry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Phones are where the unwindowed timeline hangs (1,519-5,249 DOM nodes on
a 402x874 viewport in mobile hang traces), yet windowing shipped behind a
default-off experiment. Resolve the enablement at ThreadTimelineSurface
as `experiment ?? isCompactViewport`: compact viewports window whenever
the experiment is unset, an explicitly false experiment still disables
windowing everywhere (the kill switch), and desktop keeps the served
value. `defaultExperiments.timelineWindowing` stays false, so server
value semantics are unchanged.

New surface tests assert compact default-on (fails before this change),
the explicit-false opt-out, the unchanged desktop default, and the
explicit-true opt-in. docs/configuration.md and the bb-cli skill
surfaces now describe the compact default and kill-switch semantics.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The config payload materialized every experiment from defaultExperiments,
so a client could not distinguish "user chose false" from "user never
chose" - and the compact-viewport windowing default rests on exactly that
distinction: a fresh install's served false would defeat default-on
before the user ever touched Settings.

Omission now carries the meaning (AGENTS.md's optional-field rule: real
semantics, not a hidden default). The /system/config experiments payload
becomes a partial record (storedExperimentsSchema / getStoredExperiments)
serving persisted rows only; setExperiments still writes every key on a
save, so after the first explicit choice the stored values win everywhere.
Server-internal policy (runtime policy, edit-message gating) keeps
reading concrete booleans through getExperiments. Consumers that need a
concrete boolean overlay defaultExperiments at their boundary: the
Settings views (web and mobile), the editMessages readers (its default is
true, so `?? false` would have regressed fresh installs), and the CLI's
experiment update; the connect plugin's mobileApp gate fails closed. The
offline fallback config omits timelineWindowing so an unreachable server
still yields the compact windowing default. The experiments PUT contract
is unchanged, and nothing crosses the server/host-daemon wire, so
HOST_DAEMON_PROTOCOL_VERSION is untouched.

New tests: /system/config omits never-saved experiments while
getExperiments stays concrete (fails before: the payload carried all five
defaults); getStoredExperiments returns only persisted rows; a compact
surface windows when the payload omits the key.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@SawyerHood

Copy link
Copy Markdown
Collaborator

Closing this PR for rework after an adversarial review of the #2385#2393 set. Method: two independent multi-agent review passes over a worktree at this PR head; every finding went to three independent refuters (code trace, a throwaway experiment against the real code, and an impact judge), and only findings that survived at least two of three are listed. The branch stays as-is; please reopen this PR or open a new one when the blocking items are addressed.

Blocking

  1. Any experiments save persists timelineWindowing=false and kills the phone default this PR shipsapps/server/src/routes/system.ts:218. Every client (web Settings, mobile Experiments screen, CLI) overlays defaultExperiments into the full record that PUT requires; the route merges with getExperiments, and setExperiments upserts all keys. Enabling mobileApp (needed for pairing), flipping changelogPreview, or running bb settings experiment mobileApp true writes timelineWindowing=false, which ThreadTimelineSurface treats as the kill switch. Every install that saved any experiment since Add experiment-gated timeline row windowing #2011 already holds that row and never receives the default. Reproduced with the real @bb/db module on in-memory SQLite at this head.
  2. GET is now partial while PUT stays exhaustive; shipped mobile and CLI builds breakpackages/server-contract/src/api/system.ts:186. Mobile builds and globally installed CLIs ship separately from the server. An old useEditMessagesExperimentEnabled reads undefined ?? false (the edit affordance disappears while server policy says true), and an old Experiments screen PUTs a sparse object and gets 400 on every toggle. The body's "the experiments PUT contract is unchanged" hides that the round trip old clients depend on changed.
  3. The published plugin-SDK type changed with no version bump (same file). bb.sdk.system.config().experiments goes from a full record to a partial one; the npm version guard fails on main now that 0.4.17 is published, and third-party plugins typed against boolean break or silently disable features.
  4. The geometry revision reads at the start of the expand transition and never re-readsapps/app/src/components/thread/timeline/TimelineWindowedItems.tsx:290. The bump runs in the commit that starts the 200 ms height transition, so nested windowed lists below keep a scrollMargin wrong by the row's expanded height; scroll-driven rerenders no longer re-read (the removed depless layout effect did). On a tree with Let taps paint: transition-priority navigation, deferred expanders and sidebar realization #2386 merged this gets worse: the deferred body mounts after the single read, so the read sees no body at all (reproduced on the merged tree: mounted range stays 10..29 instead of 0..11 after the deferred commit, +200 ms, and scroll idle).

Also found

  • The trailing-60 Suspense fallback ignores alwaysMountedKeys, so a search deep-link into a windowed thread silently never scrolls (TimelineWindowedItemsLoader.tsx:84).
  • Settings shows "Timeline windowing" OFF on phones where it is active; the kill switch needs an on→off round trip (apps/app/src/views/SettingsView.tsx:1054).
  • bb settings show prints "experiments": {} on a fresh install instead of effective values (apps/cli/src/commands/settings.ts:139), and the guide template still documents the experiment as default-off (packages/templates/src/templates/bb-guide-customization.md:128).
  • Making all five keys optional pushes default resolution into six consumers, against the AGENTS.md rule to fill defaults once at the server boundary.

Suggested salvage

Split it. The windowing hardening and the phone default are sound: fix the geometry invalidation (re-read after the transition or from the deferred body mount / the shared observer write phase, plus a scroll-idle self-heal), render pinned rows in the fallback, and land behind the existing experiment. The default-on rollout needs a contract that survives old clients: keep experiments concrete and add a separate sparse storedExperiments/experimentChoices field with PUT-as-PATCH, or a tri-state on | off | auto. Bump the plugin SDK in that change.

AGENT GENERATED

@SawyerHood SawyerHood closed this Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants