refactor: replace cross-tab chrome.storage sync with sinking#160
refactor: replace cross-tab chrome.storage sync with sinking#160
Conversation
…DB + SharedWorker) Core: Add `persistToolbarState` option (default true) to guard all localStorage calls. When false, localStorage is skipped and in-memory state is used as fallback. This allows external consumers (e.g. web extension) to take over persistence. Web extension: Replace chrome.storage-based toolbar state sync with sinking library for cross-tab IndexedDB sync via SharedWorker. Chrome.storage is kept only for the extension icon enable/disable toggle. On startup, the extension disables core's localStorage persistence and uses sinking for all toolbar state read/write/subscribe operations. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@react-grab/cli
grab
@react-grab/ami
@react-grab/amp
@react-grab/claude-code
@react-grab/codex
@react-grab/cursor
@react-grab/droid
@react-grab/gemini
@react-grab/opencode
react-grab
@react-grab/relay
@react-grab/utils
commit: |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found several critical issues: race condition in startup sequence, inconsistent persistence flag reading, missing error handling, and wrong worker file extension in manifest. Must fix before merge.
| const savedToolbarState = loadToolbarState( | ||
| initialOptions.persistToolbarState !== false, | ||
| ); |
There was a problem hiding this comment.
Critical inconsistency: Initial load uses initialOptions.persistToolbarState but runtime checks use pluginRegistry.store.options.persistToolbarState. If setOptions({ persistToolbarState: false }) is called (which the extension does at line 93 of react-grab.ts), shouldPersistToolbarState() will correctly return false, but this initial load already happened with the wrong value. The extension explicitly passes persistToolbarState: false in initial options, so this specific code path works, but the inconsistent pattern is fragile—if initialization order changes or if someone calls setOptions before the toolbar mounts, this breaks.
| if (workerUrl) { | ||
| initSinkingClient(workerUrl); | ||
| } |
There was a problem hiding this comment.
Race condition: Sinking client initialization is not awaited. The constructor for Sinking likely triggers async IndexedDB operations or SharedWorker connection setup. Immediately after this non-blocking call, line 231 calls loadToolbarStateFromSinking() which will return null if the client hasn't finished initializing. This means the persisted toolbar state will be silently lost on first load.
Fix: Change to await initSinkingClient(workerUrl); and add proper error handling for initialization failures.
| if (sinkingUnsubscribe) { | ||
| sinkingUnsubscribe(); | ||
| } | ||
| sinkingUnsubscribe = subscribeToToolbarState(handleSinkingChange); |
There was a problem hiding this comment.
Potential race condition: Subscription happens synchronously during subscribeToStateChanges, but if the sinking client isn't fully initialized yet (see issue at line 224-226), this subscription may fail silently or not trigger when expected. The handler will be registered but might miss early state changes if another tab updates the state during the initialization window.
| export const loadToolbarStateFromSinking = | ||
| async (): Promise<ToolbarState | null> => { | ||
| if (!sinkingClient) return null; | ||
| const record = await sinkingClient.get<ToolbarStateRecord>( | ||
| TOOLBAR_STATE_STORE, | ||
| TOOLBAR_STATE_KEY, | ||
| ); | ||
| if (!record) return null; | ||
| return { | ||
| edge: record.edge, | ||
| ratio: record.ratio, | ||
| collapsed: record.collapsed, | ||
| enabled: record.enabled, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
No error handling: All sinking operations are wrapped in zero try-catch blocks. If IndexedDB fails (quota exceeded, browser privacy mode, corrupted DB), SharedWorker fails to initialize, or any sinking API throws, the extension will crash or silently fail. Recommendation: wrap all sinking operations in try-catch and gracefully degrade to in-memory state when persistence fails.
| export const saveToolbarStateToSinking = async ( | ||
| state: ToolbarState, | ||
| ): Promise<void> => { | ||
| if (!sinkingClient) return; | ||
| const record: ToolbarStateRecord = { ...state, id: TOOLBAR_STATE_KEY }; | ||
| await sinkingClient.put(TOOLBAR_STATE_STORE, TOOLBAR_STATE_KEY, record); | ||
| }; |
There was a problem hiding this comment.
No error handling: Same issue—saveToolbarStateToSinking has no error handling. If IndexedDB write fails, the user will lose their toolbar state without any indication. The void in line 61 of react-grab.ts (calling this function) suppresses TypeScript's unhandled promise warning, making this worse.
| ], | ||
| "web_accessible_resources": [ | ||
| { | ||
| "resources": ["src/worker.ts"], |
There was a problem hiding this comment.
Wrong file extension: The worker file is TypeScript (worker.ts) but web_accessible_resources should reference the compiled JavaScript output. Extensions can't execute TypeScript directly. This will cause a runtime error when the content script tries to load the worker URL. Need to check what the build output path is (likely dist/src/worker.js or similar) and reference that instead, or ensure the build process handles this correctly.
| disableCorePersistence(delayedApi); | ||
| subscribeToStateChanges(delayedApi); |
There was a problem hiding this comment.
Redundant call to disableCorePersistence: This is already done inside createExtensionApi at line 111. When localhost detects an existing API and waits for it, calling disableCorePersistence again is correct, but this code path shows a subtle issue: if the page loads the library itself on localhost, the extension correctly disables persistence—but the page's API was initialized with default persistToolbarState: true, meaning localStorage was already written to during the page's initialization (line 252-254 of core/index.tsx). The extension then disables it, but stale localStorage data remains and will be read on next page load if the extension isn't active.
| if (event.data?.type === "__REACT_GRAB_TOOLBAR_STATE_SAVE__") { | ||
| chrome.storage.local.set({ react_grab_toolbar_state: event.data.state }); | ||
| if (event.data?.type === "__REACT_GRAB_GET_WORKER_URL__") { | ||
| const workerUrl = chrome.runtime.getURL("src/worker.ts"); |
There was a problem hiding this comment.
Incorrect path: Similar to manifest.json issue—this references the source TypeScript file instead of the compiled JavaScript. chrome.runtime.getURL('src/worker.ts') will return a URL to the .ts file which won't exist in the built extension. Should be 'src/worker.js' or wherever the built worker ends up.
- Remove duplicate ToolbarState interfaces, import from react-grab - Remove unused getSinkingClient export - Extract toToolbarState and getToolbarStateQuery helpers in storage client - Extract resolveToolbarState helper in core to deduplicate pattern - Simplify subscribeToStateChanges with optional chaining and direct callback Co-authored-by: Cursor <cursoragent@cursor.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/web-extension/src/storage/client.ts">
<violation number="1" location="packages/web-extension/src/storage/client.ts:29">
P0: The implementation attempts to create a `SharedWorker` using a `chrome-extension://` URL (passed as `workerUrl`) from a content script running in the page's MAIN world. This violates the Same-Origin Policy for SharedWorkers, which requires the worker script to be served from the same origin as the document spawning it.
This will cause `new Sinking(...)` to throw a `SecurityError`, causing the `startup` sequence in `react-grab.ts` to crash and the extension to fail completely.
To fix this, you must use an iframe bridge pattern: inject a hidden iframe (served from the extension) into the page, spawn the SharedWorker from within that iframe, and communicate via `postMessage`.</violation>
<violation number="2" location="packages/web-extension/src/storage/client.ts:38">
P1: Storage operations like `sinkingClient.get` can fail (e.g., IndexedDB errors, corruption, quota limits). Since `loadToolbarStateFromSinking` is awaited during the critical `startup` phase in `react-grab.ts`, an unhandled error here will cause the entire initialization to abort, preventing the toolbar from appearing.
Wrap the operation in a `try/catch` block to ensure graceful degradation (falling back to default state) if storage is inaccessible.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| export const initSinkingClient = (workerUrl: string | URL): Sinking => { | ||
| if (sinkingClient) return sinkingClient; | ||
| sinkingClient = new Sinking({ workerUrl, schema }); |
There was a problem hiding this comment.
P0: The implementation attempts to create a SharedWorker using a chrome-extension:// URL (passed as workerUrl) from a content script running in the page's MAIN world. This violates the Same-Origin Policy for SharedWorkers, which requires the worker script to be served from the same origin as the document spawning it.
This will cause new Sinking(...) to throw a SecurityError, causing the startup sequence in react-grab.ts to crash and the extension to fail completely.
To fix this, you must use an iframe bridge pattern: inject a hidden iframe (served from the extension) into the page, spawn the SharedWorker from within that iframe, and communicate via postMessage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/web-extension/src/storage/client.ts, line 29:
<comment>The implementation attempts to create a `SharedWorker` using a `chrome-extension://` URL (passed as `workerUrl`) from a content script running in the page's MAIN world. This violates the Same-Origin Policy for SharedWorkers, which requires the worker script to be served from the same origin as the document spawning it.
This will cause `new Sinking(...)` to throw a `SecurityError`, causing the `startup` sequence in `react-grab.ts` to crash and the extension to fail completely.
To fix this, you must use an iframe bridge pattern: inject a hidden iframe (served from the extension) into the page, spawn the SharedWorker from within that iframe, and communicate via `postMessage`.</comment>
<file context>
@@ -0,0 +1,82 @@
+
+export const initSinkingClient = (workerUrl: string | URL): Sinking => {
+ if (sinkingClient) return sinkingClient;
+ sinkingClient = new Sinking({ workerUrl, schema });
+ return sinkingClient;
+};
</file context>
| export const loadToolbarStateFromSinking = | ||
| async (): Promise<ToolbarState | null> => { | ||
| if (!sinkingClient) return null; | ||
| const record = await sinkingClient.get<ToolbarStateRecord>( |
There was a problem hiding this comment.
P1: Storage operations like sinkingClient.get can fail (e.g., IndexedDB errors, corruption, quota limits). Since loadToolbarStateFromSinking is awaited during the critical startup phase in react-grab.ts, an unhandled error here will cause the entire initialization to abort, preventing the toolbar from appearing.
Wrap the operation in a try/catch block to ensure graceful degradation (falling back to default state) if storage is inaccessible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/web-extension/src/storage/client.ts, line 38:
<comment>Storage operations like `sinkingClient.get` can fail (e.g., IndexedDB errors, corruption, quota limits). Since `loadToolbarStateFromSinking` is awaited during the critical `startup` phase in `react-grab.ts`, an unhandled error here will cause the entire initialization to abort, preventing the toolbar from appearing.
Wrap the operation in a `try/catch` block to ensure graceful degradation (falling back to default state) if storage is inaccessible.</comment>
<file context>
@@ -0,0 +1,82 @@
+export const loadToolbarStateFromSinking =
+ async (): Promise<ToolbarState | null> => {
+ if (!sinkingClient) return null;
+ const record = await sinkingClient.get<ToolbarStateRecord>(
+ TOOLBAR_STATE_STORE,
+ TOOLBAR_STATE_KEY,
</file context>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| isApplyingExternalState = false; | ||
| } else if (!initialState.enabled) { | ||
| } | ||
|
|
There was a problem hiding this comment.
Race condition: initSinkingClient called without awaiting async SharedWorker initialization, potentially allowing subsequent operations to proceed before the Sinking client is fully ready
| const record = await getToolbarStateQuery(); | ||
| if (!record) return null; | ||
| return toToolbarState(record); | ||
| }; |
There was a problem hiding this comment.
Missing error handling in toolbar state storage operations causes silent failures when IndexedDB or Sinking operations fail
| export const subscribeToToolbarState = (listener: () => void): (() => void) => { | ||
| if (!sinkingClient) return () => {}; | ||
| return sinkingClient.subscribe(getToolbarStateQuery().description, listener); | ||
| }; |
There was a problem hiding this comment.
subscribeToToolbarState and getCachedToolbarState incorrectly call sinkingClient.get() without awaiting and try to access .description property on a Promise object
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Re-subscribe to sinking toolbar state after initializing sinkingClient if an API was already initialized via the react-grab:init event. This ensures the cross-tab sync subscription is properly established even when the event fires before startup() runs on non-localhost sites.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/web-extension/src/content/react-grab.ts">
<violation number="1" location="packages/web-extension/src/content/react-grab.ts:214">
P2: The manual subscription to sinking state here is incomplete for existing APIs. If the `react-grab:init` event was missed (e.g., due to late injection), this code fails to disable core persistence and attach the API-to-sinking listener (`onToolbarStateChange`). This can lead to double persistence (localStorage + Sinking) and one-way synchronization. Use the existing helper functions `disableCorePersistence` and `subscribeToStateChanges` to ensure full initialization.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const existingApi = getActiveApi(); | ||
| if (existingApi) { | ||
| sinkingUnsubscribe?.(); | ||
| sinkingUnsubscribe = subscribeToToolbarState(handleSinkingChange); | ||
| } |
There was a problem hiding this comment.
P2: The manual subscription to sinking state here is incomplete for existing APIs. If the react-grab:init event was missed (e.g., due to late injection), this code fails to disable core persistence and attach the API-to-sinking listener (onToolbarStateChange). This can lead to double persistence (localStorage + Sinking) and one-way synchronization. Use the existing helper functions disableCorePersistence and subscribeToStateChanges to ensure full initialization.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/web-extension/src/content/react-grab.ts, line 214:
<comment>The manual subscription to sinking state here is incomplete for existing APIs. If the `react-grab:init` event was missed (e.g., due to late injection), this code fails to disable core persistence and attach the API-to-sinking listener (`onToolbarStateChange`). This can lead to double persistence (localStorage + Sinking) and one-way synchronization. Use the existing helper functions `disableCorePersistence` and `subscribeToStateChanges` to ensure full initialization.</comment>
<file context>
@@ -210,6 +210,12 @@ const startup = async (): Promise<void> => {
if (workerUrl) {
initSinkingClient(workerUrl);
+
+ const existingApi = getActiveApi();
+ if (existingApi) {
+ sinkingUnsubscribe?.();
</file context>
| const existingApi = getActiveApi(); | |
| if (existingApi) { | |
| sinkingUnsubscribe?.(); | |
| sinkingUnsubscribe = subscribeToToolbarState(handleSinkingChange); | |
| } | |
| const existingApi = getActiveApi(); | |
| if (existingApi) { | |
| disableCorePersistence(existingApi); | |
| subscribeToStateChanges(existingApi); | |
| } |
2 participants
Summary
persistToolbarStateoption to core (defaulttrue) so localStorage persistence can be disabled by external consumerschrome.storage-based toolbar state sync with sinking (IndexedDB + SharedWorker) for cross-tab syncchrome.storageonly for the extension icon enable/disable toggle (cross-origin)Changes
Core (
react-grab):types.ts: AddpersistToolbarState?: booleantoOptions,SettableOptions,ReactGrabRendererPropsplugin-registry.ts: AddpersistToolbarStatetoOptionsState(defaulttrue)toolbar/state.ts: GuardloadToolbarState/saveToolbarStatebehind the flagtoolbar/index.tsx: Pass flag to load/save callsrenderer.tsx: PasspersistToolbarStateprop to Toolbarcore/index.tsx: Track flag via plugin registry, guard all localStorage calls, fall back to in-memory state when disabledWeb extension:
sinkingdependencysrc/worker.ts(SharedWorker entry)src/storage/client.ts(sinking client with typed helpers)manifest.json: Addweb_accessible_resourcesfor workerbridge.ts: Remove toolbar state chrome.storage handling, add worker URL relayreact-grab.ts: Disable core localStorage, init sinking client, load/save/subscribe via sinkingBehavior
Test plan
pnpm typecheckpassespnpm lintpassespnpm formatpassespnpm testpasses (429 tests)Made with Cursor
Summary by cubic
Replaced the web extension’s cross‑tab toolbar state sync from chrome.storage to sinking (IndexedDB + SharedWorker) for reliable, real‑time sync. Added a persistToolbarState option to core so external consumers can disable localStorage persistence (default stays true).
Refactors
Migration
Written for commit 7fe1b47. Summary will update on new commits.
Note
Medium Risk
Touches persistence and synchronization paths for toolbar state and changes extension storage backends, which could cause state loss/desync across reloads/tabs if misconfigured or if the worker/client init fails.
Overview
Decouples toolbar persistence from core
localStorageby introducingpersistToolbarState(defaulttrue) and threading it through renderer/toolbar/core state helpers; when disabled, core state reads fall back to in-memorycurrentToolbarStateand writes are skipped.Refactors the web extension to stop syncing toolbar state via
chrome.storagemessages and instead persist/sync it across tabs usingsinking(IndexedDB + SharedWorker). This adds asinkingclient wrapper plus a web-accessible worker entry, updates the content-script bridge to only handle enabled/disabled state and to provide the worker URL, and ensures the extension disables core persistence and hydrates/applies toolbar state fromsinkingon startup.Written by Cursor Bugbot for commit 7fe1b47. This will update automatically on new commits. Configure here.