chore: refactor navigation#1919
Conversation
Greptile SummaryThis PR refactors the navigation system away from React context and component-local state toward a centralized MobX store. Navigation logic (focus tracking, telemetry, modal close,
Confidence Score: 4/5Safe to merge with awareness that navigation renders are now synchronous where they were previously deferred via startTransition. The refactor is well-structured and addresses all three concerns from previous review threads. The isNavigating external reset and the removal of startTransition are the two items worth a second look before merging. src/renderer/lib/layout/provider.tsx — the external runInAction reset of isNavigating and the absence of a transition wrapper on navigation are worth a second look.
|
| Filename | Overview |
|---|---|
| src/renderer/lib/stores/navigation-store.ts | Navigation logic moved into the MobX store; navigate() handles focus tracking, telemetry, params, and modal close; viewEvents map de-duplicated here; isNavigating flag added and reset externally from provider. |
| src/renderer/lib/layout/provider.tsx | WorkspaceViewProvider converted to an observer; React context and useState replaced with direct MobX reads; startTransition removed; isNavigating reset via external runInAction in a useEffect. |
| src/renderer/lib/layout/navigation-provider.tsx | Context-based hook implementations replaced with direct appState.navigation reads via useObserver; useNavigate now returns a stable useCallback wrapper; five context objects removed. |
| src/renderer/lib/modal/modal-provider.tsx | ModalProvider component and ModalContext removed; useModalContext converted to a plain hook; useShowModal/useTransitionModal now use useCallback with stable id dependency. |
| src/renderer/App.tsx | ModalProvider wrapper removed from the component tree; indentation level of child providers reduced accordingly. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Component calls useNavigate()"] --> B["useCallback wrapper"]
B --> C["appState.navigation.navigate(viewId, params)"]
C --> D{"viewId !== currentViewId?"}
D -- Yes --> E["focusTracker.transition()"]
E --> F["captureTelemetry(viewEvents[viewId])"]
F --> G["currentViewId = viewId\nisNavigating = true"]
D -- No --> H["skip transition & telemetry"]
G --> I{"params !== undefined?"}
H --> I
I -- Yes --> J["viewParamsStore = {...store, [viewId]: params}"]
I -- No --> K["modalStore.closeModal()"]
J --> K
K --> L["MobX notifies observers"]
L --> M["WorkspaceViewProvider re-renders (observer)"]
M --> N["useEffect: runInAction isNavigating = false"]
L --> O["reaction: syncTelemetryScope"]
L --> P["useWorkspaceSlots / useParams re-compute via useObserver"]
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/layout/provider.tsx:58-62
**`isNavigating` reset bypasses store encapsulation with no guard**
`runInAction` directly writes `isNavigating = false` from outside the store. While `makeAutoObservable` allows external `runInAction` writes, the corresponding `navigate()` setter lives inside `NavigationStore` — if `isNavigating` ever needs extra logic on reset (guard check, audit log, etc.) there is now a second write site that won't pick it up. More critically, `WorkspaceViewProvider` observes `currentViewId` and re-renders when it changes, so the reset fires after the commit of the new view — but because `observer` re-renders are synchronous, any component that reads `isNavigating` between the commit and the effect callback will still see `true`. A dedicated `resetNavigating()` action on the store would make the intent explicit and keep all mutation paths in one place.
### Issue 2 of 2
src/renderer/lib/layout/provider.tsx:46-56
**`startTransition` removed — navigation renders are now blocking**
The previous implementation wrapped `setCurrentViewId` / `setViewParamsStore` in `startTransition`, letting React defer the expensive destination-view render as a low-priority transition and keep the current frame responsive. The new `navigate()` MobX action is fully synchronous: it commits `currentViewId` and `isNavigating` in one batch, and `observer`-wrapped components re-render synchronously on the same microtask. If the destination view tree is heavy, frames can be dropped. Wrapping the `navigate()` call site in `startTransition` would restore the previous responsiveness guarantee.
Reviews (2): Last reviewed commit: "fix: nav refactor fixes" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| this.viewParamsStore = { ...this.viewParamsStore, [viewId]: params }; | ||
| } | ||
| modalStore.closeModal(); | ||
| this.isNavigating = true; |
There was a problem hiding this comment.
isNavigating cannot reset for same-view navigation
navigate() sets isNavigating = true unconditionally, but the reset lives in a useEffect keyed on [currentViewId] in WorkspaceViewProvider. When navigate() is called with the same viewId that is already active (e.g. to update params on the current task view), MobX will not re-fire the observer because currentViewId doesn't change, so the useEffect never runs and isNavigating is permanently stuck at true.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/lib/stores/navigation-store.ts
Line: 59
Comment:
**`isNavigating` cannot reset for same-view navigation**
`navigate()` sets `isNavigating = true` unconditionally, but the reset lives in a `useEffect` keyed on `[currentViewId]` in `WorkspaceViewProvider`. When `navigate()` is called with the same `viewId` that is already active (e.g. to update params on the current task view), MobX will not re-fire the observer because `currentViewId` doesn't change, so the `useEffect` never runs and `isNavigating` is permanently stuck at `true`.
How can I resolve this? If you propose a fix, please make it concise.| const viewEvents: Record< | ||
| ViewId, | ||
| | 'home_viewed' | ||
| | 'project_viewed' | ||
| | 'task_viewed' | ||
| | 'settings_viewed' | ||
| | 'skills_viewed' | ||
| | 'mcp_viewed' | ||
| > = { | ||
| home: 'home_viewed', | ||
| project: 'project_viewed', | ||
| task: 'task_viewed', | ||
| settings: 'settings_viewed', | ||
| skills: 'skills_viewed', | ||
| mcp: 'mcp_viewed', | ||
| }; |
There was a problem hiding this comment.
viewEvents map
An identical viewEvents record is defined in both this file and src/renderer/lib/layout/provider.tsx (lines 31–46). The two copies can drift if a new ViewId is added — a new entry would have to be added in both places, and a missed update in one would go unnoticed at compile time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/lib/stores/navigation-store.ts
Line: 11-26
Comment:
**Duplicate `viewEvents` map**
An identical `viewEvents` record is defined in both this file and `src/renderer/lib/layout/provider.tsx` (lines 31–46). The two copies can drift if a new `ViewId` is added — a new entry would have to be added in both places, and a missed update in one would go unnoticed at compile time.
How can I resolve this? If you propose a fix, please make it concise.| export function useShowModal<MId extends ModalId>(id: MId) { | ||
| const { showModal } = useModalContext(); | ||
| return (args: UserArgs<MId>) => showModal(id, args); | ||
| return (args: UserArgs<MId>) => { | ||
| modalStore.setModal(id, wrapArgs(args)); | ||
| window.dispatchEvent(new CustomEvent('emdash:overlay:changed', { detail: { open: true } })); | ||
| }; | ||
| } | ||
|
|
||
| export function useTransitionModal<MId extends ModalId>(id: MId) { | ||
| const { transitionModal } = useModalContext(); | ||
| return (args: UserArgs<MId>) => transitionModal(id, args); | ||
| return (args: UserArgs<MId>) => { | ||
| modalStore.setModal(id, wrapArgs(args)); | ||
| }; | ||
| } |
There was a problem hiding this comment.
useShowModal / useTransitionModal return new function references every render
These helpers no longer call useCallback, so they produce a fresh closure on every render. The previous implementation delegated to useModalContext() where showModal and transitionModal were wrapped in useCallback. With 30+ call sites (many inside observer-wrapped components), any consumer that passes the returned handler as a prop to a memoized child or uses it as an effect dependency will see unnecessary re-renders or spurious effect runs.
| export function useShowModal<MId extends ModalId>(id: MId) { | |
| const { showModal } = useModalContext(); | |
| return (args: UserArgs<MId>) => showModal(id, args); | |
| return (args: UserArgs<MId>) => { | |
| modalStore.setModal(id, wrapArgs(args)); | |
| window.dispatchEvent(new CustomEvent('emdash:overlay:changed', { detail: { open: true } })); | |
| }; | |
| } | |
| export function useTransitionModal<MId extends ModalId>(id: MId) { | |
| const { transitionModal } = useModalContext(); | |
| return (args: UserArgs<MId>) => transitionModal(id, args); | |
| return (args: UserArgs<MId>) => { | |
| modalStore.setModal(id, wrapArgs(args)); | |
| }; | |
| } | |
| export function useShowModal<MId extends ModalId>(id: MId) { | |
| return useCallback( | |
| (args: UserArgs<MId>) => { | |
| modalStore.setModal(id, wrapArgs(args)); | |
| window.dispatchEvent(new CustomEvent('emdash:overlay:changed', { detail: { open: true } })); | |
| }, | |
| [id] | |
| ); | |
| } | |
| export function useTransitionModal<MId extends ModalId>(id: MId) { | |
| return useCallback( | |
| (args: UserArgs<MId>) => { | |
| modalStore.setModal(id, wrapArgs(args)); | |
| }, | |
| [id] | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/lib/modal/modal-provider.tsx
Line: 67-78
Comment:
**`useShowModal` / `useTransitionModal` return new function references every render**
These helpers no longer call `useCallback`, so they produce a fresh closure on every render. The previous implementation delegated to `useModalContext()` where `showModal` and `transitionModal` were wrapped in `useCallback`. With 30+ call sites (many inside observer-wrapped components), any consumer that passes the returned handler as a prop to a memoized child or uses it as an effect dependency will see unnecessary re-renders or spurious effect runs.
```suggestion
export function useShowModal<MId extends ModalId>(id: MId) {
return useCallback(
(args: UserArgs<MId>) => {
modalStore.setModal(id, wrapArgs(args));
window.dispatchEvent(new CustomEvent('emdash:overlay:changed', { detail: { open: true } }));
},
[id]
);
}
export function useTransitionModal<MId extends ModalId>(id: MId) {
return useCallback(
(args: UserArgs<MId>) => {
modalStore.setModal(id, wrapArgs(args));
},
[id]
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
| useEffect(() => { | ||
| runInAction(() => { | ||
| appState.navigation.isNavigating = false; | ||
| }); | ||
| }, [currentViewId]); |
There was a problem hiding this comment.
isNavigating reset bypasses store encapsulation with no guard
runInAction directly writes isNavigating = false from outside the store. While makeAutoObservable allows external runInAction writes, the corresponding navigate() setter lives inside NavigationStore — if isNavigating ever needs extra logic on reset (guard check, audit log, etc.) there is now a second write site that won't pick it up. More critically, WorkspaceViewProvider observes currentViewId and re-renders when it changes, so the reset fires after the commit of the new view — but because observer re-renders are synchronous, any component that reads isNavigating between the commit and the effect callback will still see true. A dedicated resetNavigating() action on the store would make the intent explicit and keep all mutation paths in one place.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/lib/layout/provider.tsx
Line: 58-62
Comment:
**`isNavigating` reset bypasses store encapsulation with no guard**
`runInAction` directly writes `isNavigating = false` from outside the store. While `makeAutoObservable` allows external `runInAction` writes, the corresponding `navigate()` setter lives inside `NavigationStore` — if `isNavigating` ever needs extra logic on reset (guard check, audit log, etc.) there is now a second write site that won't pick it up. More critically, `WorkspaceViewProvider` observes `currentViewId` and re-renders when it changes, so the reset fires after the commit of the new view — but because `observer` re-renders are synchronous, any component that reads `isNavigating` between the commit and the effect callback will still see `true`. A dedicated `resetNavigating()` action on the store would make the intent explicit and keep all mutation paths in one place.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
No description provided.