Skip to content

Studio: stop a settings module reading a chat store key before it exists - #8979

Merged
danielhanchen merged 9 commits into
mainfrom
fix-settings-cycle
Aug 18, 2026
Merged

Studio: stop a settings module reading a chat store key before it exists#8979
danielhanchen merged 9 commits into
mainfrom
fix-settings-cycle

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

Studio boots today by accident.

general-tab.tsx builds a top-level const array of preference keys, and one of its entries is SIDEBAR_ORGANIZATION_STORAGE_KEY. That key was defined in sidebar-organization-store.ts, which imports zustand and sits inside an import cycle running through the @/features/chat barrel. Enter that cycle from the settings side and the binding is still in its temporal dead zone when the array is built:

Cannot access 'SIDEBAR_ORGANIZATION_STORAGE_KEY' before initialization

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.tsx imports 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_KEY moves to sidebar-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.ts re-exports it, so every existing importer, including the chat barrel, is unchanged.
  • general-tab.tsx imports 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.ts walks the source with the TypeScript compiler and pins the three things that make this safe:

  • the keys module imports nothing, which is the entire reason a module-scope read of it is safe
  • general-tab.tsx gets the key from the keys module, not from the store and not from the barrel
  • the key is still read at module scope, so if that ever stops being true the other two guards are removed rather than left passing for no reason

The 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 is tests/delete-chat-files-preference.test.ts, which fails identically on main (ERR_MODULE_NOT_FOUND for an extensionless ./pasted-text import in chat-preferences-store.ts) and is unrelated to this change. Fixed separately.
  • npm run typecheck
  • npx eslint on all three changed files. The one error is no-restricted-imports on the deep @/features/chat/stores/... path, which general-tab.tsx already had on main for the same import, pointing at the store instead of the keys module.

@danielhanchen

Copy link
Copy Markdown
Member Author

Merged #8980 in. Not scope creep: main currently fails npm test on an extensionless import in chat-preferences-store.ts, and Unit tests runs before everything else, so this branch's CI stopped there and never reached the steps it exists to exercise. #8980 is green on ubuntu, windows and macos, so this drops out cleanly once it lands.

@danielhanchen

Copy link
Copy Markdown
Member Author

Correction to my earlier comment. I merged #8980 in to get past a red Unit tests on main, but #8981 landed that same fix independently at d43892ea7, so the reason is gone. I have merged main instead and dropped the #8980 content, so this branch's diff is its own again. #8980 is now Windows-only and unrelated to this.

@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 5b2b81e19a

ℹ️ 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".

@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 159a235f16

ℹ️ 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".

@mahiatlinux

Copy link
Copy Markdown
Collaborator

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

install.sh --local at the merge base and at the head, driven through login to the chat shell. Both render the full app: root_html_len 95133 vs 95119, composer present on both, zero page errors on both, no TDZ error on either. The small length delta is the randomised greeting, not this change. Settings then General opens normally on the base build.

I also applied the reordering the description names as the reproduction, moving the useAnimatedThemeToggle import in app-sidebar.tsx below the @/features/chat import block. Both sides still boot, in the production install and under the vite dev server.

Which import shapes actually throw

Measured 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 pr-8979-module-cycle-repro-ci-1 of my fork, base e46319458, head resolved live).

entry and shape merge base head
shipped imports, entered at the chat barrel renders renders
general-tab reads the key from @/features/chat Cannot access 'SIDEBAR_ORGANIZATION_STORAGE_KEY' before initialization same error
sidebar-organization-store given an import that puts it in the cycle same error renders

One row moves.

What that means

The TDZ needs general-tab.tsx to read the key through the barrel while the barrel is mid-evaluation. sidebar-organization-store.ts imports only zustand, so it is a leaf, and importing it directly always finishes evaluating it before the read. That barrel form is what #8932 introduced, and #8956 replaced it with the direct path, which is already in this PR's merge base. So the reachable white screen is gone before this branch starts.

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 module-scope-cycle-safety.test.ts pins with the "imports nothing" assertion.

Two things that might be worth adjusting:

  • The description reads as though main currently boots by accident. On the merge base it does not: the direct import in general-tab.tsx is load bearing and Fix CI on main: stale test doubles, a stale router stub, and two source defects #8956 put it there deliberately, with a comment saying why.
  • The second row shows the change does not protect the barrel shape. Anyone who switches that import back to @/features/chat gets the white screen on this branch too. The no-restricted-imports error on the deep path is the thing nudging people toward the barrel, so it may be worth an eslint exception on that line rather than leaving a lint error standing.

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.

@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 28b2efb338

ℹ️ 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".

danielhanchen added a commit to danielhanchen/unsloth-staging-2 that referenced this pull request Aug 18, 2026
@danielhanchen
danielhanchen merged commit e35b8a8 into main Aug 18, 2026
36 of 39 checks passed
@danielhanchen
danielhanchen deleted the fix-settings-cycle branch August 18, 2026 07:10
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.

2 participants