Skip to content

fix(renderer): harden restored navigation state#2134

Merged
jschwxrz merged 2 commits into
mainfrom
jona/eng-1349-opening-emdash-canary-fails-until-reload
May 20, 2026
Merged

fix(renderer): harden restored navigation state#2134
jschwxrz merged 2 commits into
mainfrom
jona/eng-1349-opening-emdash-canary-fails-until-reload

Conversation

@jschwxrz
Copy link
Copy Markdown
Collaborator

  • harden restored renderer navigation state against stale data from older builds.
  • register valid view IDs during startup and ignore persisted currentViewId values that are no longer supported.
  • fall back to home if the workspace slot resolver encounters an unknown view ID
  • make project/task navigation guards validate unknown persisted params before use.
  • redirect stale or missing task routes back to the project view instead of crashing.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 20, 2026

Greptile Summary

This PR hardens navigation state restoration against stale data persisted by older builds by introducing a view registry that gates snapshot ingestion, adding runtime param validation to all canActivate guards, and gracefully falling back to home when unknown view IDs or params are encountered.

  • NavigationStore gains a _registeredViewIds set populated by setupNavigationGuards at startup; restoreSnapshot now rejects unknown currentViewId values and strips stale keys from viewParamsStore, then runs the guard post-restore to redirect if the stored params are no longer valid.
  • Task and project canActivate guards widened from typed params to unknown with explicit runtime shape validation; the task guard additionally checks task existence against the live task manager and redirects to the project view (rather than home) when only the task is missing.
  • task-titlebar.tsx removes unsafe non-null assertions on getTaskStore/getRegisteredTaskData and adds a null guard in ActiveTaskTitlebar before any store access.

Confidence Score: 4/5

Safe to merge — all crash paths from stale snapshots are now handled and covered by tests.

The core changes are correct and well-tested. The only noteworthy gap is that restoreSnapshot applies guard redirects in a single hop while _applyNavigation chases the full chain recursively; in the current guard set this never causes a problem because every redirect target is a terminal that passes its own guard, but the asymmetry could quietly misbehave if a chained redirect is added later.

src/renderer/lib/stores/navigation-store.ts — the single-hop redirect logic in restoreSnapshot deserves a look if new guard chains are introduced.

Important Files Changed

Filename Overview
src/renderer/lib/stores/navigation-store.ts Adds _registeredViewIds set, registerView, and isRegisteredViewId; hardens restoreSnapshot to reject unknown view IDs and strip stale params keys. Guard redirect in restore is single-hop only (see comment).
src/renderer/lib/stores/navigation-store.test.ts New test file with 5 cases covering stale view fallback, stale params stripping, settings preservation, and guard redirect on restore.
src/renderer/features/tasks/view.tsx Task canActivate guard widened to unknown params with runtime validation; adds task existence check and redirects to project view when the project is valid but the task is gone.
src/renderer/features/tasks/task-titlebar.tsx Removes unsafe non-null assertions; adds a null guard in ActiveTaskTitlebar before any store access.
src/renderer/features/projects/view.tsx Project canActivate guard widened to unknown params with runtime type validation; behaviour otherwise unchanged.
src/renderer/lib/layout/navigation-provider.tsx Adds fallback to registry.home for unknown currentViewId; minor double lookup of registry[viewId] (see suggestion).
src/renderer/app/view-registry.ts Calls registerView for every view before registering guards; cleans up canActivate signature to consistently accept unknown.

Sequence Diagram

sequenceDiagram
    participant Main as main.tsx (bootstrap)
    participant Registry as view-registry.ts
    participant NavStore as NavigationStore
    participant Guard as canActivate (task/project)

    Main->>Registry: setupNavigationGuards()
    loop each view
        Registry->>NavStore: registerView(viewId)
        Registry->>NavStore: registerGuard(viewId, canActivate)
    end

    Main->>NavStore: restoreSnapshot(persisted)
    NavStore->>NavStore: isRegisteredViewId(currentViewId)?
    alt unknown view ID
        NavStore-->>NavStore: "keep currentViewId = home"
    else registered
        NavStore->>NavStore: set currentViewId, lastNonSettingsView
        NavStore->>NavStore: filter viewParams to registered keys only
        NavStore->>Guard: _runGuard(currentViewId, params)
        Guard->>Guard: validate params shape
        alt params invalid or entity missing
            Guard-->>NavStore: ok false, redirect
            NavStore->>NavStore: update currentViewId to redirect target
        else valid
            Guard-->>NavStore: ok true
        end
    end

    Main->>Main: render App
Loading

Comments Outside Diff (1)

  1. src/renderer/lib/stores/navigation-store.ts, line 141-151 (link)

    P2 Single-hop guard redirect on restore

    restoreSnapshot applies only one level of guard redirect, while _applyNavigation recursively chases the full chain. In current guards this is safe — task→project redirect only fires when the project is valid, so the project guard always passes — but the asymmetry is a silent footgun. If a future guard redirects to another view that also has a failing guard (e.g. task → project → home), restoreSnapshot would leave currentViewId at the intermediate step ('project') with a guard that would immediately fail on the next revalidate().

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/renderer/lib/stores/navigation-store.ts
    Line: 141-151
    
    Comment:
    **Single-hop guard redirect on restore**
    
    `restoreSnapshot` applies only one level of guard redirect, while `_applyNavigation` recursively chases the full chain. In current guards this is safe — task→project redirect only fires when the project is valid, so the project guard always passes — but the asymmetry is a silent footgun. If a future guard redirects to another view that also has a failing guard (e.g. task → project → home), `restoreSnapshot` would leave `currentViewId` at the intermediate step (`'project'`) with a guard that would immediately fail on the next `revalidate()`.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
src/renderer/lib/stores/navigation-store.ts:141-151
**Single-hop guard redirect on restore**

`restoreSnapshot` applies only one level of guard redirect, while `_applyNavigation` recursively chases the full chain. In current guards this is safe — task→project redirect only fires when the project is valid, so the project guard always passes — but the asymmetry is a silent footgun. If a future guard redirects to another view that also has a failing guard (e.g. task → project → home), `restoreSnapshot` would leave `currentViewId` at the intermediate step (`'project'`) with a guard that would immediately fail on the next `revalidate()`.

### Issue 2 of 2
src/renderer/lib/layout/navigation-provider.tsx:58-60
**Double lookup of `registry[viewId]`**

`registry[viewId]` is evaluated twice — once for the fallback and again for the resolved ID. A single lookup removes the duplication.

```suggestion
    const registry = views as unknown as Record<string, ViewDefinition<Record<string, unknown>>>;
    const viewDef = registry[viewId];
    const def = viewDef ?? registry.home;
    const resolvedViewId = viewDef ? viewId : 'home';
```

Reviews (1): Last reviewed commit: "fix(renderer): harden restored navigatio..." | Re-trigger Greptile

Comment thread src/renderer/lib/layout/navigation-provider.tsx Outdated
@jschwxrz jschwxrz merged commit 987e6f4 into main May 20, 2026
1 check passed
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.

1 participant