⚠️ fix: Keep Session Module Server-only - #100
Merged
Merged
Conversation
The production build fails: TanStack Start's import-protection plugin rejects the client bundle because server/utils/refresh.ts imports getRequestHeader from @tanstack/react-start/server as a plain function call, and this file is reachable from a client-bundled route (login.tsx -> @/server -> scopes.ts -> utils/api.ts -> refresh.ts) even though every real caller is server-only. The plugin can't prove that from a bare import alone. Fixing that surfaces the same problem one level deeper: refresh.ts also imports session.ts, which calls useSession the same unwrapped way. The CI workflow that gates pull requests only runs lint, tsc, and vitest, never a full production build, so nothing before now caught this. Wraps both in createServerOnlyFn, the pattern the compiler's own error message points to. Verified with a clean install and a from-scratch production build (both client and SSR passes), plus tsc, eslint, and the full test suite.
createServerOnlyFn only replaces the wrapped function body in the client bundle; it doesn't touch the rest of the module's top-level statements. session.ts computed SESSION_SECRET validation and cookie settings, and threw, at module scope, so that code ran the instant the module was pulled into the client bundle transitively (even though the wrapped useAppSession call itself was correctly stubbed out). Bundlers don't tree-shake a module with side effects just because its exports go unused, so this threw unconditionally inside the browser's own JS runtime, where process.env is always empty, regardless of whether the server actually has SESSION_SECRET set. Moves the throwing validation and the cookie-secure/base-path env reads into the useAppSession closure, and turns SESSION_CONFIG (a plain top-level const) into a lazily-computed getSessionConfig(), both wrapped in createServerOnlyFn. Nothing in this module now executes merely by being imported.
dustinhealy
changed the title
dustinhealy
changed the title
danny-avila
approved these changes
Jul 30, 2026
Contributor
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The production build has been failing since #99 merged (see the docker-publish workflow run on main). TanStack Start's import-protection plugin rejects the client bundle:
refresh.tscallsgetRequestHeaderfrom@tanstack/react-start/serveras a plain function, and this file is reachable from a client-bundled route (login.tsx -> @/server -> scopes.ts -> utils/api.ts -> refresh.ts) even though every real caller is server-only. The plugin can't prove that from a bare import alone and rejects the build. Fixing that surfaces the same issue one level deeper:refresh.tsalso importssession.ts, which callsuseSessionthe same unwrapped way.Fix
First commit wraps both
readTenantHeaderanduseAppSessionincreateServerOnlyFn, the pattern the compiler's own error message points to.That fix alone still leaves a real bug:
createServerOnlyFnonly replaces the wrapped function's body in the client bundle, it doesn't touch the rest of the module's top-level statements.session.tscomputedSESSION_SECRETvalidation and cookie settings, and threw, at module scope, so that code still ran the instant the module was pulled into the client bundle transitively, including inside the browser's own JS runtime, even withuseAppSessionitself correctly stubbed out. Bundlers don't tree-shake a module with side effects just because its exports go unused, so this threw unconditionally in the browser, whereprocess.envis always empty, regardless of whether the server itself hasSESSION_SECRETset.Second commit moves the throwing validation and env reads into the
useAppSessionclosure, and turnsSESSION_CONFIG(a plain top-level const) into a lazily-computedgetSessionConfig(), both wrapped increateServerOnlyFn. Nothing in this module now executes merely by being imported.Testing
Fresh clone, fresh
bun install, from-scratchbun run build(both client and SSR passes succeed, including withSESSION_SECRETunset), confirmed the error string no longer appears anywhere in the client bundle output,tsc --noEmitclean,eslintclean, full test suite (799 tests) passing withSESSION_SECRETset, and a real browser (Playwright) hit against a locally built production server showing zero console errors.