Studio: stop a settings module reading a chat store key before it exists - #8979
Conversation
|
Merged #8980 in. Not scope creep: |
|
Correction to my earlier comment. I merged #8980 in to get past a red |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
I went looking for before/after UI evidence on this and could not get the white screen to happen at the merge base, so posting what I measured instead. Two Studio installs, merge base vs head
I also applied the reordering the description names as the reproduction, moving the Which import shapes actually throwMeasured with a vite entry, no backend and no auth, on the merge base and the head. Run: https://github.com/mahiatlinux/unsloth/actions/runs/32098608444 (harness on branch
One row moves. What that meansThe TDZ needs The third row is what this PR does buy: once the store itself gains an import that puts it in the cycle, the base breaks and the head does not. That is exactly what Two things that might be worth adjusting:
Nothing here contradicts the change itself. The keys module and its test are a reasonable guard, the storage key value is unchanged, and I saw no behaviour difference in the running app. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Studio boots today by accident.
general-tab.tsxbuilds a top-levelconstarray of preference keys, and one of its entries isSIDEBAR_ORGANIZATION_STORAGE_KEY. That key was defined insidebar-organization-store.ts, which imports zustand and sits inside an import cycle running through the@/features/chatbarrel. Enter that cycle from the settings side and the binding is still in its temporal dead zone when the array is built:Nothing catches it. There is no error boundary above the router, so the whole tree unmounts: a white screen on launch, with the message only in the console.
The app avoids it because
app-sidebar.tsximports the theme toggler about 115 lines above its own chat import, so the store happens to be evaluated first. Swapping those two import lines reproduces the white screen. That is not a property anyone can be expected to preserve by hand while editing an import block.It is not hypothetical elsewhere either: a new vite smoke entry that imports the chat renderer hit exactly this error and rendered nothing at all until the import order was worked around by hand.
Change
SIDEBAR_ORGANIZATION_STORAGE_KEYmoves tosidebar-organization-keys.ts, a module that imports nothing. A module with no imports of its own is always fully evaluated before the module that imports it, cycle or not, so the value cannot be read early.sidebar-organization-store.tsre-exports it, so every existing importer, including the chat barrel, is unchanged.general-tab.tsximports it from the keys module. It already used a direct path rather than the barrel, with a comment explaining the cycle; the comment now says why the store was not far enough.No behaviour changes and the storage key's value is identical, so existing installs read and write the same key.
Coverage
tests/module-scope-cycle-safety.test.tswalks the source with the TypeScript compiler and pins the three things that make this safe:general-tab.tsxgets the key from the keys module, not from the store and not from the barrelThe first test fails if anyone adds an import to the keys module, which is the way this regresses.
Testing
npm test: 2,840 passed, 1 failed. The failure istests/delete-chat-files-preference.test.ts, which fails identically onmain(ERR_MODULE_NOT_FOUNDfor an extensionless./pasted-textimport inchat-preferences-store.ts) and is unrelated to this change. Fixed separately.npm run typechecknpx eslinton all three changed files. The one error isno-restricted-importson the deep@/features/chat/stores/...path, whichgeneral-tab.tsxalready had onmainfor the same import, pointing at the store instead of the keys module.