Core: Scope localStorage per SB instance and purge stale tag filters#34195
Core: Scope localStorage per SB instance and purge stale tag filters#34195Copilot wants to merge 15 commits into
Conversation
All comments addressed in d2f4089:
|
|
Tested locally and the deduplication by ID works: I also added fake keys to the storage, reloaded Storybook, and those keys were correctly removed from the store state. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a helper function to purge stale tag filters after fetching the story index, removing filters that no longer match any tags unless they are built-in filters. Made storage keys instance-scoped by appending the anonymous project ID to prevent cross-project state collision. Added version tracking for persisted state by storing and checking a version identifier with stored data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager-api/typings.d.ts (1)
13-13: Consider adding JSDoc for consistency.The corresponding declaration in
code/core/src/manager/typings.d.tsincludes a JSDoc comment explaining the purpose and fallback behavior. Consider adding the same documentation here for consistency across typing files.📝 Suggested JSDoc addition
+/** + * A stable, anonymous project identifier derived from Git info (remote URL + working directory + * path). Used to scope localStorage keys per Storybook instance so that persistence is not shared + * across different projects. Falls back to 'anonymous' when Git info is unavailable. + */ declare var STORYBOOK_INSTANCE_ID: string | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/typings.d.ts` at line 13, Add a JSDoc comment above the STORYBOOK_INSTANCE_ID declaration to match the other typings: document that STORYBOOK_INSTANCE_ID may be undefined, explain its purpose as an instance identifier and the fallback behavior (e.g., unused when undefined), and mirror the wording used in the corresponding manager/typings.d.ts so both files stay consistent; update the declaration for STORYBOOK_INSTANCE_ID to keep the same type (string | undefined) without changing semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/manager-api/typings.d.ts`:
- Line 13: Add a JSDoc comment above the STORYBOOK_INSTANCE_ID declaration to
match the other typings: document that STORYBOOK_INSTANCE_ID may be undefined,
explain its purpose as an instance identifier and the fallback behavior (e.g.,
unused when undefined), and mirror the wording used in the corresponding
manager/typings.d.ts so both files stay consistent; update the declaration for
STORYBOOK_INSTANCE_ID to keep the same type (string | undefined) without
changing semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 537677c2-6e87-44df-b889-204f954fcfa1
📒 Files selected for processing (8)
code/core/src/builder-manager/utils/framework.tscode/core/src/manager-api/modules/stories.tscode/core/src/manager-api/store.tscode/core/src/manager-api/tests/store.test.jscode/core/src/manager-api/tests/stories.test.tscode/core/src/manager-api/typings.d.tscode/core/src/manager/typings.d.tscode/core/src/telemetry/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/store.ts`:
- Around line 91-93: The call to persistVersion(store.local) writes to
localStorage even when persistence is disabled; update the initialization in the
store setup to check the allowPersistence flag (the setting that disables
localStorage persistence) and only call persistVersion(store.local) when
allowPersistence is true (or equivalent getter) so no persistence occurs when
allowPersistence is false; locate the call to persistVersion and guard it with
the allowPersistence condition used elsewhere in this module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e411e8d1-3ed5-408c-9d99-b53f8859484f
📒 Files selected for processing (3)
code/core/src/builder-manager/utils/framework.tscode/core/src/manager-api/store.tscode/core/src/telemetry/anonymous-id.test.ts
✅ Files skipped from review due to trivial changes (2)
- code/core/src/telemetry/anonymous-id.test.ts
- code/core/src/builder-manager/utils/framework.ts
|
For context, this PR addresses a general bug in our storage management which also affects keyboard shortcuts, layout options, etc. We never thought to deduplicate it per instance. It also addresses an issue specifically with stored tags which would occur with both sessionStorage or localStorage tags. |
| function get(storage: StoreAPI) { | ||
| let data = storage.get(STORAGE_KEY); | ||
|
|
||
| // Migration: if no data exists under the instance-specific key, try the old shared key. | ||
| // This is a one-time migration that preserves existing settings when upgrading. | ||
| if (!data && STORAGE_KEY !== STORAGE_KEY_BASE) { | ||
| data = storage.get(STORAGE_KEY_BASE); | ||
| if (data) { | ||
| storage.set(STORAGE_KEY, data); | ||
| } | ||
| } |
There was a problem hiding this comment.
Re-added the migration logic in get() that copies data from the old shared key (@storybook/manager/store) to the instance-scoped key. Also added a unit test for it. See d5ae983.
Sidnioulz
requested review from
valentinpalkovic
and removed request for
huang-julien and
ndelangen
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
- Make sure instance deduplication only happens when running dev, not on built instances where we might not want to even share the anonymous project id publicly
- Restore the storage migration logic and, if possible, add a unit test for it
- Also add the deduplication logic to lastViewedStoryIds
Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com> Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/1172a9ba-085a-4149-916f-63bf6a11d4d3
Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com> Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/1172a9ba-085a-4149-916f-63bf6a11d4d3
All three items addressed in d5ae983:
|
Conflicts resolved in 469ca5c. Key decisions:
|
Sidnioulz
force-pushed
the
copilot/fix-localstorage-persistence-issue
branch
from
469ca5c to
2da328e
Compare
|
I don't have time to continue this. If this is affecting you, feel free to pick it up! It's probably best to remove anything merge-rebase related as those branch syncs messed the branch up. |
getAnonymousProjectId()anonymousfallback for non-Git projectsgetAnonymousProjectIdfromstorybook/internal/telemetrySTORYBOOK_ANONYMOUS_PROJECT_IDassignmentallExistingTags,isExistingTag) in purge logiclastViewedStoryIdsstorage per instancenextbranch (status filters, .ts extensions, URL-based tag persistence)💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.