Repository navigation
fix(navigation): run the visit deadline only while a presenter is attached - #1814
Merged
Merged
Conversation
…ached The navigation controller armed a pending visit's 15 s deadline the moment the visit was created. The startup `home` visit is created inside `createServices()` before React renders, so whenever no presenter exists for 15 s — today, the key-setup screen left idle; soon, a first-run flow that keeps `ConnectedApp` hidden for minutes — the visit is already `failed`/`timeout` when the first presenter mounts, and the user's first frame is "This destination couldn’t open … kept for retry". The deadline exists to catch a presenter that never finishes; while no presenter exists its expiry carries no information. The controller's host object gains `attach()`, returning a detach function, and a pending visit's timer is armed only while at least one presenter is attached: `start()` arms at once when one is, `attach()` arms the active pending visit when the first presenter arrives, and the last detach clears it. `useAppNavigation` attaches in a mount-time layout effect, so the startup visit's deadline runs from the moment `ConnectedApp` mounts. `provideNavigation` already spreads the controller's host methods into `services.navigationHost`, so no FOUNDATION file changes and no constant is lengthened: every visit opened while the app is presenting still has exactly its 15 s. Preserved: a visit a mounted presenter never finishes still times out 15 s after that presenter attached (no retry, no loop); ingress/deep-link failures (`fail()`) and `resolve()`'s "keep the original deadline" are untouched; a StrictMode attach→detach→attach restarts the deadline at mount; `attach()` after `dispose()` arms nothing. Tests: `controller.test.ts` — `setup()` attaches a presenter by default, as the app does, so the existing timeout pins hold; a new case shows the startup visit outliving 60 s with no presenter, the deadline starting on attach, standing while a second presenter remains, pausing when the last leaves, and a later presenter measured from its own start. `service.test.ts` attaches in its request()-driven setup. The App-level proof through the real first-run gate (`src/app/first-run-navigation.test.tsx`) lands with the Welcome flow later in this stack. docs/plugin-architecture.md says the deadline runs only while a presenter is attached. Stack: PR 01 of the onboarding stack, based on main; nothing depends on it except 07 (Welcome), and it depends on nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
kalvinnchau
reviewed
Oct 28, 2026
kalvinnchau
left a comment
kalvinnchau
left a comment
Contributor
There was a problem hiding this comment.
🤖 Two small follow-ups: preserve the adjacent deadline regression test and name the identity gate precisely.
…and name the identity gate The ThreadPanel deadline regression test never attached a presenter, so the presenter guard left its deadline unarmed and the test no longer protected the 15-second contract. Attach one, as the service tests do. The presenter is held back by IdentitySetup, not the first-run CommunityLanding branch, so name that gate in the docs and comments. Co-authored-by: Jitter <2b2e6415e748c35180847a37aadae06a11e30dddc76b784e7fd9354c4eb42e7a@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
matt2e
force-pushed
the
onboarding/01-navigation-presenter-deadline
branch
from
October 28, 2026 14:21
5cc2c69 to
b9ea475
Compare
This was referenced Oct 28, 2026
Open
kalvinnchau
approved these changes
Oct 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Onboarding stack (merge in order, top to bottom):
Depends on: nothing. Only PR 07 (
Welcome) depends on it.Diff: production +38/−8; tests +41/−1; docs +3/−1
What this layer is
The navigation controller's 15 s visit deadline is armed only while at least one presenter is attached (
attach()on the host object, returning a detach function);useAppNavigationattaches in a mount-time layout effect. Onmaintoday the startuphomevisit is created insidecreateServices()and times out if the key-setup screen sits idle for 15 s, so this is a standalone defect fix with no first-run dependency. Preserved: a mounted presenter that never finishes still times out 15 s after attaching; ingress failures andresolve()are untouched.Review focus
controller.ts:arm()is the only place the timer starts;start(),attach()and the last detach are its three callers. Check thatresolve()andfail()are unchanged.src/app/navigation.ts: the layout effect attaches once per presenter mount and detaches on unmount; a StrictMode double mount restarts the deadline.controller.test.ts:setup()now attaches by default so the existing timeout pins still prove the 15 s contract; the new case is the no-presenter one. The App-level proof through the first-run gate (first-run-navigation.test.tsx) lands in PR 07.Checks run on this branch's tree
bin/pnpm tsc --noEmit✓ (exit 0)biome checkon the 4 changed TS files ✓bin/pnpm vitest run src/features/navigation src/app: 698 passed / 2 failed of 700 — both failures were 5 s timeouts under load in files this layer does not touch (App.profile.test.tsx,NotificationSettings.test.tsx); rerun alone: 2 files, 27/27 passed ✓Native evidence
None on this tree. The native first-run recordings and the hands-on checks on the branch were made on the full tree, which only the DO-NOT-MERGE PR 09 reproduces (it adds the isolated debug Keychain slot). Treat every layer below 09 as unit/integration-tested only.
🤖 Generated with Claude Code