fix(web): guard terminal persistence when storage is unavailable#1021
fix(web): guard terminal persistence when storage is unavailable#1021kfiramar wants to merge 1 commit intopingdotgg:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
|
|
||
| const TERMINAL_STATE_STORAGE_KEY = "t3code:terminal-state:v1"; | ||
|
|
||
| const noopStorage: Storage = { |
There was a problem hiding this comment.
feels like we have a lot of these mocks now in a lot of different test files. should unify it to one central MemoryStorage that we can use as runtmie fallback and also in all tests
you up for doing this? i know it will increase the scope of the PR a bit but for maintainability would be nice to do.
There was a problem hiding this comment.
Sure :)
Will implement it later (if you want that could be even a different PR)
Summary
localStoragewhen the runtime exposes the full storage API and otherwise falls back to a no-op storage backendterminalStateStoretests with an in-memory storage double so the suite exercises terminal behavior instead of runner-specific storage quirksProblem
On my local repro, the repo mostly validated cleanly after a reinstall, but
apps/web/src/terminalStateStore.test.tsconsistently failed under the current Bun/Vitest environment.All 8 failures collapsed to the same persistence crash:
The issue was not the terminal state logic itself. The problem was that the persisted Zustand store assumed
localStoragewas a complete Storage implementation whenever it existed. In this runtime,localStoragewas present enough to be detected, but not complete enough for Zustand persistence to safely callsetItem.Fix Details
getTerminalStateStorage()inapps/web/src/terminalStateStore.tsto validate the runtime storage shape before passing it intocreateJSONStorageapps/web/src/terminalStateStore.test.tsThis keeps the fix intentionally narrow:
Why this approach
I considered fixing this only in the test, but the crash originates inside the persisted store configuration itself. Making the store resilient is the more robust fix because it protects both tests and any non-browser or partially browser-like runtime edge cases.
I also avoided a broader dependency update pass here. The repo is already on a modern stack, and the failure was reproducible as a runtime compatibility issue rather than an outdated-package issue.
Verification
I ran the focused suite first and then the full required repo checks:
bun x vitest run apps/web/src/terminalStateStore.test.ts bun run fmt:check bun lint bun run typecheck bun run testAll of the commands above pass with this change.
Notes
mockServiceWorkerdiff so this PR only contains the targeted fix..mise.tomlrequests Bun1.3.9and Node24.13.1), which made it even more valuable to keep the fix defensive.Note
Guard terminal state persistence when localStorage is unavailable
The
useTerminalStateStorein terminalStateStore.ts previously calledlocalStoragedirectly in the persist middleware, causing errors when storage is absent or incomplete. AgetTerminalStateStorageutility now validates thatlocalStorageexposes the expected methods at runtime, falling back to a no-opStorageimplementation if not. In valid environments, behavior is unchanged.Macroscope summarized aa7dbd7.