Manager API: Namespace localStorage keys by project ID#34352
Closed
cyphercodes wants to merge 1 commit into
Closed
Manager API: Namespace localStorage keys by project ID#34352cyphercodes wants to merge 1 commit into
cyphercodes wants to merge 1 commit into
Conversation
…t cross-instance state sharing
The Manager API store was using a constant STORAGE_KEY for all Storybook instances,
causing localStorage/sessionStorage to be shared across different projects. This meant
that tag filters and other persisted state from one Storybook would leak into another.
Changes:
- Add projectId parameter to Store class (derived from git remote URL + project path)
- Namespace storage keys as `@storybook/manager/store:${projectId}` when projectId is available
- Falls back to original key format for backward compatibility when projectId is undefined
Fixes storybookjs#34194
Contributor
📝 WalkthroughWalkthroughThe changes introduce project-scoped storage namespacing to the store system. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/manager-api/root.tsx`:
- Line 48: The code calls getAnonymousProjectId() (which uses process.cwd() and
executeCommandSync()) in the browser context causing undefined projectId and
un-namespaced Store keys; stop resolving projectId in the client—update
ManagerProvider to accept an optional projectId prop (e.g., add projectId?:
string to ManagerProvider props and use that when constructing the Store) and
remove the direct getAnonymousProjectId() invocation from root.tsx;
alternatively, if SSR-safe resolution is intended, wrap any
getAnonymousProjectId() usage with a runtime guard (typeof process !==
'undefined') or move the call to server/build-time and pass the resulting
projectId into ManagerProvider as a prop.
In `@code/core/src/manager-api/store.ts`:
- Around line 160-162: Tests currently don't cover namespaced storage paths
because they instantiate Store without a projectId; add unit tests that
construct the Store with a non-empty projectId (e.g., using the same constructor
used in production) and assert that update(storage, delta, this.projectId)
writes to the namespaced key. Specifically, create tests that call Store methods
which trigger the update call (referencing the update function and the
Store/instance property this.projectId), verifying both session and local
persistence branches (store.session and store.local) produce keys/values that
include the provided projectId namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11166d01-8656-486a-a2d9-f450978d1185
📒 Files selected for processing (2)
code/core/src/manager-api/root.tsxcode/core/src/manager-api/store.ts
|
|
||
| import { isEqual } from 'es-toolkit/predicate'; | ||
|
|
||
| import { getAnonymousProjectId } from '../telemetry/anonymous-id'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if getAnonymousProjectId has browser-compatible implementation or is build-time resolved
echo "=== Checking getAnonymousProjectId implementation ==="
cat code/core/src/telemetry/anonymous-id.ts 2>/dev/null || fd "anonymous-id" --exec cat {}
echo ""
echo "=== Check if value is injected via build/bundler config ==="
rg -n "anonymousProjectId|getAnonymousProjectId" --type ts --type js -g '!*.test.*' code/core/src/manager/ code/builders/ 2>/dev/null | head -30
echo ""
echo "=== Check for any globals or window injection ==="
rg -n "STORYBOOK.*PROJECT|projectId" code/core/src/manager-api/ --type ts -A 2 -B 2 | head -40Repository: storybookjs/storybook
Length of output: 5171
🏁 Script executed:
# Check the exact usage in root.tsx
echo "=== Usage in root.tsx ==="
cat -n code/core/src/manager-api/root.tsx | sed -n '40,160p'
echo ""
echo "=== Check if there's any webpack/build config handling this ==="
fd -e config.js -e webpack -e vite | xargs grep -l "getAnonymousProjectId\|anonymous-id" 2>/dev/null || echo "No matching build config found"
echo ""
echo "=== Check if the import has any side-effects disabled ==="
rg "getAnonymousProjectId" code/core/src/manager-api/root.tsx -B 5 -A 5Repository: storybookjs/storybook
Length of output: 5025
🏁 Script executed:
# Check for comments explaining this pattern in telemetry or store files
echo "=== Comments in anonymous-id.ts ==="
grep -n "// \|/\*" code/core/src/telemetry/anonymous-id.ts
echo ""
echo "=== Check if this is SSR or build-time executed ==="
rg "import.*anonymous-id\|getAnonymousProjectId" code/core/src/manager-api/ -B 3 -A 3
echo ""
echo "=== Check if there's server-side setup that would inject projectId ==="
rg "projectId" code/core/src/main/manager/manager-main.ts -A 2 -B 2 2>/dev/null | head -30Repository: storybookjs/storybook
Length of output: 606
Remove Node.js-dependent code from browser context, or defer projectId resolution.
getAnonymousProjectId() calls process.cwd() and executeCommandSync() which are not available in the browser. The function's try-catch silently fails, returning undefined, causing the Store to use an un-namespaced storage key and defeating the project isolation purpose of this PR.
Solution options:
- Move this call to a build-time step or server-side initialization that passes
projectIdas a prop - Accept
projectIdas an optional prop toManagerProvider(from server-side setup) - Wrap the Node.js code with proper
typeof process !== 'undefined'checks if SSR-compatible behavior is intended
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/manager-api/root.tsx` at line 48, The code calls
getAnonymousProjectId() (which uses process.cwd() and executeCommandSync()) in
the browser context causing undefined projectId and un-namespaced Store keys;
stop resolving projectId in the client—update ManagerProvider to accept an
optional projectId prop (e.g., add projectId?: string to ManagerProvider props
and use that when constructing the Store) and remove the direct
getAnonymousProjectId() invocation from root.tsx; alternatively, if SSR-safe
resolution is intended, wrap any getAnonymousProjectId() usage with a runtime
guard (typeof process !== 'undefined') or move the call to server/build-time and
pass the resulting projectId into ManagerProvider as a prop.
Comment on lines
160
to
162
| const storage = persistence === 'session' ? store.session : store.local; | ||
| await update(storage, delta); | ||
| await update(storage, delta, this.projectId); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify test coverage for projectId-namespaced storage paths
echo "=== Checking Store instantiations in tests ==="
rg -n "new Store\(" --type js --type ts -g '*test*' -A 5
echo ""
echo "=== Checking if any tests pass projectId ==="
rg -n "projectId" --type js --type ts -g '*test*' code/core/src/manager-api/Repository: storybookjs/storybook
Length of output: 5292
🏁 Script executed:
#!/bin/bash
# Check Store class definition and projectId usage
echo "=== Store class constructor signature ==="
head -50 code/core/src/manager-api/store.ts | cat -n
echo ""
echo "=== Lines around 160-162 in store.ts ==="
sed -n '150,170p' code/core/src/manager-api/store.ts | cat -n
echo ""
echo "=== Check if projectId is a constructor parameter ==="
rg -n "constructor\|projectId" code/core/src/manager-api/store.ts -A 2 -B 1Repository: storybookjs/storybook
Length of output: 2681
🏁 Script executed:
#!/bin/bash
# Find the Store class constructor and projectId assignment
echo "=== Full Store class with constructor ==="
sed -n '50,130p' code/core/src/manager-api/store.ts | cat -n
echo ""
echo "=== Check root.tsx for Store instantiation ==="
rg -n "new Store" code/core/src/root.tsx -B 2 -A 5Repository: storybookjs/storybook
Length of output: 3769
🏁 Script executed:
#!/bin/bash
# Find root.tsx
echo "=== Searching for root.tsx ==="
fd -name "root.tsx" code/
echo ""
echo "=== Searching for Store instantiation in production ==="
rg -n "new Store" code/core/src/ -t ts -t tsx --max-count 5Repository: storybookjs/storybook
Length of output: 428
🏁 Script executed:
#!/bin/bash
# Search for root.tsx and Store instantiation with correct syntax
echo "=== Searching for root.tsx in entire repo ==="
fd root.tsx
echo ""
echo "=== Searching for Store instantiation in manager-api ==="
rg "new Store" code/core/src/manager-api/ -A 3
echo ""
echo "=== Check how Store is instantiated in production ==="
rg "new Store" code/ -A 3 --type ts --type js | head -50Repository: storybookjs/storybook
Length of output: 6829
Add tests with projectId to cover namespaced storage paths.
The update call on line 162 correctly uses this.projectId to namespace the storage key. However, all existing tests instantiate the Store without a projectId, so they only test the fallback (non-namespaced) path. The production code in root.tsx always passes projectId: getAnonymousProjectId(), creating a coverage gap.
Consider adding test cases that pass a projectId to verify the namespaced storage behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/manager-api/store.ts` around lines 160 - 162, Tests currently
don't cover namespaced storage paths because they instantiate Store without a
projectId; add unit tests that construct the Store with a non-empty projectId
(e.g., using the same constructor used in production) and assert that
update(storage, delta, this.projectId) writes to the namespaced key.
Specifically, create tests that call Store methods which trigger the update call
(referencing the update function and the Store/instance property
this.projectId), verifying both session and local persistence branches
(store.session and store.local) produce keys/values that include the provided
projectId namespace.
Contributor
|
Superseded by #34195 |
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.
Problem
The Manager API store uses a constant
STORAGE_KEYfor all Storybook instances, causing localStorage/sessionStorage to be shared across different projects. This means that tag filters and other persisted state from one Storybook instance leak into another when users switch between projects.Fixes #34194
Solution
Namespace storage keys using the existing anonymous project ID (derived from git remote URL + project path):
projectIdparameter to the Store class@storybook/manager/store:${projectId}when a projectId is availableChanges
code/core/src/manager-api/store.ts: Updated to accept and use projectId for namespacing storage keyscode/core/src/manager-api/root.tsx: ImportgetAnonymousProjectIdand pass it to Store constructorTesting
This fix ensures that:
Summary by CodeRabbit
Release Notes