refactor(web): convert viewer-store to Zustand with direct named actions#31209
Conversation
Closes LUM-1641 - Replace viewerReducer + 21 action type interfaces with Zustand store using create() + createSelectors and direct named actions - Remove dispatchViewer parameter threading from use-app-viewer-actions; consumers now call useViewerStore.getState().actionName() directly - Rename showTokenDialog state field to isTokenDialogOpen to avoid collision with the showTokenDialog() action function - Add early-return optimizations for idempotent state updates - Rewrite all 38 tests to use Zustand store API Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cd4ad79fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| useViewerStore.getState().handleAppUnpinned(appId); | ||
| const state = useViewerStore.getState(); | ||
| if ( | ||
| viewerStateRef.current.activeAppId === appId && | ||
| (viewerStateRef.current.mainView === "app" || viewerStateRef.current.mainView === "app-editing") | ||
| state.activeAppId === appId && | ||
| (state.mainView === "app" || state.mainView === "app-editing") |
There was a problem hiding this comment.
handleActiveAppUnpinned now calls handleAppUnpinned(appId) before checking whether the unpinned app is currently active in app/app-editing view. Because handleAppUnpinned clears activeAppId and switches mainView to chat when it matches, the subsequent condition is always false in the matched case, so SET_EDITING_KEY is never dispatched. This leaves the conversation list editing key stale when the active pinned app is removed during an edit session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this is a real bug. Already fixed: snapshot state before mutation so the condition evaluates against pre-mutation values. Will be in the next push.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| (appId: string) => { | ||
| dispatchViewer({ type: "ACTIVE_APP_UNPINNED", appId }); | ||
| useViewerStore.getState().handleAppUnpinned(appId); | ||
| const state = useViewerStore.getState(); | ||
| if ( | ||
| viewerStateRef.current.activeAppId === appId && | ||
| (viewerStateRef.current.mainView === "app" || viewerStateRef.current.mainView === "app-editing") | ||
| state.activeAppId === appId && | ||
| (state.mainView === "app" || state.mainView === "app-editing") | ||
| ) { | ||
| dispatchConversationList({ type: "SET_EDITING_KEY", key: null }); | ||
| } |
There was a problem hiding this comment.
🔴 handleActiveAppUnpinned reads post-mutation state, so SET_EDITING_KEY dispatch never fires
In the old reducer-based code, dispatchViewer (React's useReducer dispatch) was asynchronous — viewerStateRef.current still held the pre-dispatch state when the condition was evaluated, so the SET_EDITING_KEY clearing logic worked correctly. In the new Zustand-based code, handleAppUnpinned(appId) synchronously mutates the store (setting activeAppId to null and mainView to "chat" at apps/web/src/stores/viewer-store.ts:201-205). The immediately-following useViewerStore.getState() returns the post-mutation state, so state.activeAppId === appId evaluates to null === appId (always false), and dispatchConversationList({ type: "SET_EDITING_KEY", key: null }) is never called. This means when a user unpins an app that was being actively edited, the editing key remains stale in the conversation list store.
| (appId: string) => { | |
| dispatchViewer({ type: "ACTIVE_APP_UNPINNED", appId }); | |
| useViewerStore.getState().handleAppUnpinned(appId); | |
| const state = useViewerStore.getState(); | |
| if ( | |
| viewerStateRef.current.activeAppId === appId && | |
| (viewerStateRef.current.mainView === "app" || viewerStateRef.current.mainView === "app-editing") | |
| state.activeAppId === appId && | |
| (state.mainView === "app" || state.mainView === "app-editing") | |
| ) { | |
| dispatchConversationList({ type: "SET_EDITING_KEY", key: null }); | |
| } | |
| (appId: string) => { | |
| const state = useViewerStore.getState(); | |
| const shouldClearEditingKey = | |
| state.activeAppId === appId && | |
| (state.mainView === "app" || state.mainView === "app-editing"); | |
| useViewerStore.getState().handleAppUnpinned(appId); | |
| if (shouldClearEditingKey) { | |
| dispatchConversationList({ type: "SET_EDITING_KEY", key: null }); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed — same issue as the Codex comment. Already fixed by reading state before calling handleAppUnpinned. Will be in the next push.
The previous code called handleAppUnpinned(appId) — which synchronously clears activeAppId and sets mainView to 'chat' — then read getState() to check the pre-mutation values. The condition was always false in the matched case, so SET_EDITING_KEY was never dispatched. Snapshot state before calling the action so the condition evaluates against pre-mutation values. Also adds '.use.* vs .getState()' guidance to CONVENTIONS.md with the read-before-mutate rule, correct/incorrect examples, and a reference table for when to use each API. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
- Add JSDoc on createSelectors explaining when to use each API and that Zustand mutations are synchronous - Add concise .use.* vs .getState() section in CONVENTIONS.md with reference table and short examples - Trim store creation example to use createSelectors (matches actual codebase pattern) instead of useShallow convenience hooks - Cross-link CONTRIBUTING.md to CONVENTIONS.md and STYLE_GUIDE.md for apps/web/ contributors Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
Value: Eliminates 200+ lines of discriminated-union boilerplate from the viewer state machine, making viewer state readable and writable from any callback without prop-drilling dispatchViewer or threading a viewerStateRef.
What this does: Converts viewer-store.ts from useReducer + 21 action-type interfaces to a Zustand store with direct named actions, wrapped with createSelectors for per-field hooks. Consumer hook (use-app-viewer-actions.ts) drops dispatchViewer and viewerStateRef params entirely; 38 tests rewritten to the new API.
Pattern check — all green:
createSelectorswrapping +.use.field()/.getState()split — canonical ✅useViewerStore.getState().action()in callbacks (not render) — correct ✅- State and Actions as separate interfaces — per CONVENTIONS ✅
reset: () => set({ ...INITIAL_STATE })— correct spread pattern ✅
handleActiveAppUnpinned bug fix (commit 5d69b157):
Bots flagged the read-after-mutate at 0cd4ad79 — pre-mutation state was lost after handleAppUnpinned ran, so SET_EDITING_KEY dispatch never fired. Fix is correct: snapshot { activeAppId, mainView } before calling handleAppUnpinned, then evaluate the condition against the snapshot. Verified at HEAD 65fee42b. ✅
showTokenDialog rename → isTokenDialogOpen:
The state field was renamed to avoid a name collision with the action showTokenDialog(pendingAppId). Smart call — the old name would have put a boolean and a (string) => void at the same key in ViewerStore. TypeScript would catch misuse and CI is green.
INITIAL_VIEWER_STATE de-exported:
Renamed to const INITIAL_STATE (non-exported). Any outside import would have been a TS error — CI confirms it's clean. All 38 tests rewritten, so no lingering reducer-style references.
finishDeploying(clearPendingAppId?) behavior:
Old DEPLOYING_DONE kept pendingDeployAppId when flag was falsy; new impl matches: ...(clearPendingAppId ? { pendingDeployAppId: null } : {}). Correct.
Minor / non-blocking:
closeApp()doesn't setmainView: "chat"— the oldCLOSE_APPreducer presumably did. The consumer (handleCloseApp) covers this viaswitchConversationor explicitsetMainView("chat"), so behavior is preserved. Worth a quick sanity-run on the "close app, no last conversation" path if you haven't already.- Bots (Codex + Devin) reviewed at stale commit
0cd4ad79; their bug report was accurate but is already resolved. No further bot re-review needed given the fix is straightforward and CI is clean.
CI: 3/3 green (Socket × 2, Lint/Type/Build). Ready to merge.
Vellum Constitution — Trust-seeking: replacing opaque dispatch strings with named typed actions makes every state transition legible and auditable at a glance.
|
Thanks for the thorough review. Re: the case "CLOSE_APP":
return {
...state,
activeAppId: null,
openedAppState: null,
isAppMinimized: false,
};So the new |
1 participant
Prompt / plan
Convert
viewer-store.tsfrom theuseReducer/dispatch pattern to a Zustand store with direct named actions per Zustand's flux-inspired practice guide and the project's CONVENTIONS.md.Closes LUM-1641
What changed
Store conversion (
stores/viewer-store.ts):viewerReducer(200+ line switch-case) + 21 action type interfaces +ViewerActiondiscriminated union with direct named actions on a Zustand storecreateSelectorsfor auto-generated.use.field()hooksViewerStateandViewerActionsinterfaces per conventionConsumer updates (
use-app-viewer-actions.ts):dispatchViewer({ type: "..." })calls withuseViewerStore.getState().actionName()dispatchViewerparameter andviewerStateRef— no longer needed with Zustand's.getState()handleActiveAppUnpinned: state is now snapshot before callinghandleAppUnpinned()so the condition evaluates against pre-mutation valuesTest updates (
viewer-store.test.ts):viewerReducer(state, action)pattern touseViewerStore.getState().actionName()APIDocumentation:
.use.*vs.getState()guidance tocreateSelectorsJSDoc — explains when to use each API and that Zustand mutations are synchronouscreateSelectors(matches actual codebase pattern)apps/web/contributorsWhy this is safe
handleActiveAppUnpinnedfix corrects a regression that would have caused stale editing keys when unpinning the active appReferences
Test plan
bun test src/stores/viewer-store.test.ts)bunx tsc --noEmitpassesbun run lintpassesLink to Devin session: https://app.devin.ai/sessions/565d827296144ac9bf12bd108169e5ef
Requested by: @ashleeradka