feat(accounts): Add external reading API integration and account storage change notification functionality #761
feat(accounts): Add external reading API integration and account storage change notification functionality #761chdown wants to merge 2 commits into
Conversation
…age change notification functionality - Add the notification function for storage changes in the account storage service - Implement the constant definition and message handler for the external read API - Add the interface component for the external read API settings - Integrate the external read API into the background script - Add relevant unit tests and integration tests - Implement the function of bridging notifications to the page context in the content script
📝 WalkthroughWalkthroughThis pull request introduces an External Read API integration that enables external applications to access a user's persisted data (accounts and API credential profiles) via secure tokens. It adds runtime action routing, context methods for preference management, content/background message handlers, a settings UI component, localization strings, storage change notifications, and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
actor External Client
participant Background
participant Storage
participant User Prefs
External Client->>Background: External message (getCapabilities/getSnapshot + token)
activate Background
Background->>User Prefs: Validate token exists & enabled
alt Token invalid/disabled
Background->>External Client: Error response
else Token valid
Background->>Storage: Export accounts & profiles
activate Storage
Storage-->>Background: Data snapshot
deactivate Storage
Background->>External Client: Snapshot/capabilities response
end
deactivate Background
sequenceDiagram
participant Storage
participant Background
participant ExternalReadApi
participant Content Script
participant All Tabs
Storage->>Background: Storage change event
activate Background
Background->>ExternalReadApi: notifyExternalReadApiAccountStorageChanged()
activate ExternalReadApi
ExternalReadApi->>ExternalReadApi: Compute diff (created/updated/deleted)
ExternalReadApi->>ExternalReadApi: Dedup via signature check
alt Duplicate
ExternalReadApi-->>Background: Suppress notification
else New change
ExternalReadApi->>Content Script: Runtime message (ExternalReadApiNotifyContent)
activate Content Script
Content Script->>All Tabs: Broadcast via window.postMessage()
deactivate Content Script
end
deactivate ExternalReadApi
deactivate Background
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
dosubot
Bot
added
the
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/apiCredentialProfiles/apiCredentialProfilesStorage.ts (1)
632-649:⚠️ Potential issue | 🟡 Minor
mergeConfignotifies with a coercedlocalrather than the on-disk previous.Unlike
importConfig(line 616) which clones the raw read result,mergeConfigpasses the coercedlocal(line 635-637) asoldValue. This means external subscribers see the post-coercion shape onoldValueand the merged shape onnewValue. If coercion was a no-op for the current data, the diff equals the actual change; otherwise the first event after a coercion-affecting upgrade may include synthetic differences.For consistency with
importConfig, consider snapshotting the rawreadConfig()result before coercion:♻️ Proposed adjustment
async mergeConfig(raw: unknown): Promise<ApiCredentialProfilesConfig> { return this.withStorageWriteLock(async () => { const now = Date.now() - const local = coerceApiCredentialProfilesConfig(await this.readConfig(), { - now, - }) + const previous = cloneConfig(await this.readConfig()) + const local = coerceApiCredentialProfilesConfig(previous, { now }) const incoming = coerceApiCredentialProfilesConfig(raw, { now }) const merged = mergeApiCredentialProfilesConfigs({ local, incoming, now, }) await this.saveConfig(merged) - await notifyExternalReadApiProfilesStorageChanged(local, merged) + await notifyExternalReadApiProfilesStorageChanged(previous, merged) return merged }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apiCredentialProfiles/apiCredentialProfilesStorage.ts` around lines 632 - 649, mergeConfig currently coerces the read config into `local` and then calls notifyExternalReadApiProfilesStorageChanged(local, merged), causing subscribers to see the coerced state as the "old" value; instead, capture the raw on-disk result returned by readConfig() (like importConfig does) before calling coerceApiCredentialProfilesConfig, and pass that raw snapshot as the `oldValue` to notifyExternalReadApiProfilesStorageChanged while keeping `merged` as the new value so subscribers get the true pre-merge on-disk state; update mergeConfig to read and store the rawRead (from readConfig()), use coerceApiCredentialProfilesConfig(rawRead, { now }) to produce `local`, and call notifyExternalReadApiProfilesStorageChanged(rawRead, merged).
🧹 Nitpick comments (9)
src/entrypoints/content/messageHandlers/handlers/externalReadApi.ts (2)
14-20: Type thesendResponseparameter precisely instead ofany.The handler signature uses
(res: any) => void, which loses type safety on the response shape and is inconsistent with the typedsendResponsecallbacks used acrosssrc/entrypoints/background/runtimeMessages.ts. Consider a small typed alias.♻️ Proposed change
+type ExternalReadApiNotifyResponse = + | { success: true } + | { success: false; error: string } + export function handleExternalReadApiNotify( request: { payload?: ExternalReadApiNotifyPayload }, - sendResponse: (res: any) => void, + sendResponse: (response: ExternalReadApiNotifyResponse) => void, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/content/messageHandlers/handlers/externalReadApi.ts` around lines 14 - 20, The sendResponse parameter of handleExternalReadApiNotify is currently typed as (res: any) => void which loses type safety; replace this with a precise callback type (e.g., a shared SendResponse<T> alias used in runtimeMessages.ts) and import/use that alias so the response shape is enforced — update the function signature in handleExternalReadApiNotify to use the typed SendResponse<ResponsePayloadType> (or the existing alias from src/entrypoints/background/runtimeMessages.ts) and adjust any call sites to satisfy the new type.
50-50:return trueis unnecessary here —sendResponseis invoked synchronously.
return truefrom aruntime.onMessagelistener is only required to keep the message channel open for an asynchronous reply. SincesendResponseis already called in both branches (success and catch) beforereturn, returningtruehas no effect. Returningundefinedis fine; if the call site relies on a truthy return value to signal "handled", consider returningfalseor a more meaningful flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/content/messageHandlers/handlers/externalReadApi.ts` at line 50, The listener currently ends with an unnecessary "return true" in the runtime.onMessage handler even though sendResponse is invoked synchronously in both success and catch branches; remove the "return true" (or replace with an explicit "return false" if you need a falsy handled flag) from the handler to avoid implying an async response, and keep the existing synchronous sendResponse calls in the same function (the runtime.onMessage listener and sendResponse references will help you locate the code).src/contexts/UserPreferencesContext.tsx (1)
1219-1245: Consider aligning fallback pattern with sibling update handlers.
prev.externalReadApi ?? DEFAULT_PREFERENCES.externalReadApi!relies on a non-null assertion, while sibling handlers likeupdateRedemptionAssist(lines 1123-1138) andupdateWebAiApiCheck(lines 1179-1193) use a?? ?? ({...} satisfies ...)chain that survives aDEFAULT_PREFERENCESshape change without crashing at runtime. Worth using the same defensive pattern for consistency.♻️ Suggested adjustment
- const merged = deepOverride( - prev.externalReadApi ?? DEFAULT_PREFERENCES.externalReadApi!, - updates, - ) + const base = + prev.externalReadApi ?? + DEFAULT_PREFERENCES.externalReadApi ?? + ({ + enabled: false, + notificationsEnabled: true, + tokens: [], + } satisfies ExternalReadApiPreferences) + const merged = deepOverride(base, updates)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/UserPreferencesContext.tsx` around lines 1219 - 1245, The updateExternalReadApi handler uses a non-null assertion on DEFAULT_PREFERENCES which can crash if the default shape changes; change the fallback to the same defensive pattern used by siblings by replacing prev.externalReadApi ?? DEFAULT_PREFERENCES.externalReadApi! with a chained fallback like prev.externalReadApi ?? DEFAULT_PREFERENCES.externalReadApi ?? ({ /* default ExternalReadApiPreferences fields */ } satisfies ExternalReadApiPreferences) so deepOverride always receives a concrete object; update references in updateExternalReadApi (and keep deepOverride, ExternalReadApiPreferences, userPreferences.savePreferences, and setPreferences usage intact).tests/entrypoints/content/messageHandlers/handlers/externalReadApi.test.ts (1)
13-71: Consider adding a negative-path test for sanitization/error branches.This single test only covers the happy path. Per the AI summary,
handleExternalReadApiNotify"sanitizes/coerces the incoming payload" and has an error fallback that responds with a non-success result. A targeted test for at least one malformed payload (e.g., missing/invalidtopics, throwingpostMessage) would strengthen coverage of those branches.As per coding guidelines: "For new executable files, functions, branches, listeners/controllers, or error fallback paths, include at least one targeted test covering the added behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/entrypoints/content/messageHandlers/handlers/externalReadApi.test.ts` around lines 13 - 71, Add a negative-path unit test for handleExternalReadApiNotify to cover sanitization and error fallback: create a test that calls handleExternalReadApiNotify with a malformed payload (e.g., missing or invalid topics) and assert it returns the non-success response via the sendResponse mock, does not dispatch the EXTERNAL_READ_API_EVENT_NAME event, and handles postMessage failures (spy on window.postMessage to throw and assert sendResponse receives the error path). Use the existing test's patterns (sendResponse mock, eventListener registration, postMessage spy) to keep setup/teardown consistent and verify both sanitization rejection and the postMessage error branch.src/features/BasicSettings/components/tabs/General/ExternalReadApiSettings.tsx (1)
241-241: Token value rendered in plain text.The token
InputisreadOnlybut always visible, so anyone glancing at the screen during a screen-share/screenshot can read every active token. Given the privacy warning explicitly notes these tokens grant access toaccess_token,sessionCookie,refreshToken, andapiKeydata, consider matching the New API/Done Hub patterns in this same settings screen which usehideToken/showTokentoggles for sensitive credentials.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/BasicSettings/components/tabs/General/ExternalReadApiSettings.tsx` at line 241, The token is currently rendered plain in ExternalReadApiSettings by <Input value={entry.token} readOnly disabled={isSaving} />, exposing sensitive credentials; change this to support a show/hide toggle like the New API/Done Hub patterns: add local state (e.g., showToken) within ExternalReadApiSettings, render the Input as masked when showToken is false (use input type="password" or a masked display) and reveal the token only when toggled, and add a small toggle button/icon next to the Input that flips showToken while respecting isSaving and readOnly semantics so tokens remain hidden by default.src/services/integrations/externalReadApi/index.ts (3)
81-103: Drop the empty JSDoc placeholders.There are ~30 empty
/**\n *\n */blocks scattered through this file (e.g., lines 81–83, 92–94, 105–107, 116–118, 148–150, 158–160, 168–170, 182–184, …, 744–746). They add noise without explaining intent. Either delete them, or replace the ones above non-obvious helpers (e.g.,buildTrackedChangeSignature,filterTrackedStorageChanges,handleTrackedStorageChange) with a one-liner that documents the invariant. As per coding guidelines: "Add brief inline comments or short code-block comments when non-obvious intent, invariants, edge cases, or protocol/browser constraints need clarification; do not narrate obvious code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/integrations/externalReadApi/index.ts` around lines 81 - 103, Remove the empty JSDoc comment blocks that add noise (e.g., the blank /** */ above createErrorResponse and createSuccessResponse) by deleting them; for helper functions with non-obvious behavior (e.g., buildTrackedChangeSignature, filterTrackedStorageChanges, handleTrackedStorageChange) replace the blank JSDoc with a concise one-line comment describing the invariant or intent (not a full narrative). Ensure any retained comment is a short inline description immediately above the function declaration and remove all other purely empty JSDoc blocks throughout the file.
372-372:JSON.stringifyis unreliable for deep equality.Comparing entries with
JSON.stringify(previous) !== JSON.stringify(account)is sensitive to key ordering and (silently) dropsundefinedvalues, functions, andNaN. In practice this means two semantically equal account objects produced by different write paths can stringify to different strings (false positives → spuriousaccount.updatedevents broadcast to all tabs), and conversely a change that only replacesvalue: 1withvalue: undefinedis invisible (false negative). It's also O(size) per entity on a hot path.Prefer a small structural-equality helper (e.g.,
dequalif a dep is acceptable, or a hand-rolled deep-equal restricted to the known account/bookmark/profile shapes). Same applies todiffBookmarkChanges(line 406) anddiffProfileChanges(line 440).Also applies to: 406-406, 440-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/integrations/externalReadApi/index.ts` at line 372, The current equality check uses JSON.stringify(previous) !== JSON.stringify(account), which is unreliable and slow; replace this brittle comparison in the account change detection with a structural deep-equality helper that handles key ordering, undefined/NaN, and only the known shapes used by your domain (accounts, bookmarks, profiles). Implement or import a small deterministic deepEqual function and use it where the code currently compares previous vs account, and similarly replace JSON.stringify checks inside diffBookmarkChanges and diffProfileChanges with the same deepEqual (or a shape-restricted comparator) to avoid false positives/negatives and reduce overhead; reference the variables/functions previous, account, diffBookmarkChanges, and diffProfileChanges when making the changes.
302-338: Diff events drop entity identifiers — consumers can't tell which entity changed.
buildAccountEvent/buildBookmarkEvent/buildProfileEventemit{ type, topic }with noid(or other identifier) of the affected entity. As a result, when N accounts are created in a single batch, the broadcast contains N identical{ type: "account.created", topic: "accounts" }entries — and external consumers must always re-pullgetSnapshotto find out what changed. Two consequences worth confirming:
- The intent appears to be "coarse" notifications (the test name "broadcasts coarse events" suggests this is by design). If so, the duplicate entries are pure overhead — a
Set-like collapse to one event per(type, topic)would be cheaper and equally informative.- If finer granularity is intended, the per-entity
idshould be included in the event detail (and theaccount.updatedbranch at line 372 would also benefit fromchangedKeysso consumers know which fields changed).Could you clarify which model is intended, and update the events / collapse duplicates accordingly?
Also applies to: 355-452
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/integrations/externalReadApi/index.ts` around lines 302 - 338, The change events currently emitted by buildAccountEvent, buildBookmarkEvent, buildProfileEvent (and buildAccountConfigEvent) lack entity identifiers which makes per-entity consumers impossible to use; decide between two fixes: (A) for fine-grained events, update these functions to accept an entity identifier (e.g., id: string) and include id in the returned ExternalReadApiChangeDetail (and ensure account.updated builders also include changedKeys when available), then update callers that create batches to pass each entity's id; or (B) for coarse notifications, collapse duplicate events before broadcasting by deduplicating the out-going list by (type, topic) — e.g., produce one event per unique (type, topic) via a Set/map — and keep the existing payload shape; pick one model and apply the corresponding change across buildAccountEvent, buildBookmarkEvent, buildProfileEvent, buildAccountConfigEvent and the code path that assembles/broadcasts the array so all consumers receive consistent event granularity.tests/services/integrations/externalReadApi.test.ts (1)
198-447: Add coverage for the remaining branches inexternalReadApi/index.ts.The suite exercises the happy paths plus disabled-feature and bad-token, but several added branches still have no targeted test:
- Invalid
namespace→createErrorResponse("Invalid namespace.")(line 640–642).- Unsupported
method→createErrorResponse("Unsupported method.")(line 651–653).notificationsEnabled: false→notifyAllTabsearly-returns andsendTabMessageWithRetryis not called (line 500–502). Currently nothing pins this contract.- The exported runtime-message helper
handleExternalReadApiStorageChangedMessage(line 668) — both the unsupported-storage-key branch and the success path.- The dedup path in
filterTrackedStorageChanges/handleTrackedStorageChange(line 537–611): firing the same storage change twice with the samelast_updatedshould only broadcast once.As per coding guidelines: "For new executable files, functions, branches, listeners/controllers, or error fallback paths, include at least one targeted test covering the added behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/integrations/externalReadApi.test.ts` around lines 198 - 447, Add unit tests to cover the remaining branches in externalReadApi/index.ts: (1) callExternal with an invalid namespace and assert it resolves to createErrorResponse("Invalid namespace."); (2) callExternal with a valid namespace but unsupported method and assert createErrorResponse("Unsupported method."); (3) set preferences.externalReadApi.notificationsEnabled = false, trigger the storageChangedListener path that would normally call notifyAllTabs and assert sendTabMessageWithRetry is NOT called; (4) import and call handleExternalReadApiStorageChangedMessage directly to exercise the unsupported-storage-key branch (pass a payload with an unknown key) and the success path (pass a valid ACCOUNT_STORAGE_KEYS.ACCOUNTS change) asserting expected return values and side effects; (5) test the deduplication by invoking storageChangedListener/handleTrackedStorageChange twice with the same last_updated and assert sendTabMessageWithRetry only broadcast once (covers filterTrackedStorageChanges dedupe). Use existing helpers (callExternal, setupIntegration, storageChangedListener, sendTabMessageWithRetryMock, ACCOUNT_STORAGE_KEYS, EXTERNAL_READ_API_NAMESPACE) to implement each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/entrypoints/content/messageHandlers/handlers/externalReadApi.ts`:
- Around line 32-40: The current broadcasting via window.dispatchEvent and
window.postMessage (using eventName and detail) exposes sensitive change events
to any page script; fix by gating broadcasts: add an allowlist check (e.g.,
allowedOrigins array) and only call window.postMessage when
window.location.origin or the target origin is in allowedOrigins, and include a
per-session token field in the posted message (e.g., add token) so receivers
must perform a handshake/validation before acting; also add a clear inline
comment in externalReadApi.ts explaining the security model, why dispatchEvent
is limited/unsafe for cross-origin listeners, and reference the handshake
validator function (e.g., validateHandshake) that listeners must call.
In
`@src/features/BasicSettings/components/tabs/General/ExternalReadApiSettings.tsx`:
- Around line 136-140: The handleDeleteToken function currently deletes a token
immediately; change it to first show a confirmation dialog (using the app's
existing modal/confirm utility used elsewhere) and only call saveSettings({
tokens: config.tokens.filter(entry => entry.id !== tokenId) }) if the user
confirms; ensure cancellation leaves state unchanged, disable the delete control
while awaiting the confirmation/save to avoid double clicks, and surface any
save errors to the user.
In `@src/locales/ja/settings.json`:
- Around line 147-164: The Japanese locale contains untranslated strings in the
externalReadApi block; update the keys copyToken, deleteToken, generateToken,
regenerateToken, and tokenManagement to Japanese (e.g., "copyToken" → "コピー",
"deleteToken" → "削除", "generateToken" → "Token を生成", "regenerateToken" → "Token
を再生成", "tokenManagement" → "アクセストークン") so they match the rest of the block and
zh-CN; also confirm whether "title" ("External Read API / Hook") should be
localized or intentionally kept as a brand and apply the chosen approach
consistently.
In `@src/services/accounts/accountStorage.ts`:
- Around line 138-148: The notification currently sends an oldValue (`previous`)
produced by getStorageConfig() (which uses the read normalizer) and a newValue
(`next`) produced by normalizeAccountStorageConfigForWrite(current), causing
mismatched shapes; fix by normalizing `previous` with the same write normalizer
before calling notifyExternalReadApiAccountStorageChanged so both `oldValue` and
`newValue` share the write-normalized shape (use
normalizeAccountStorageConfigForWrite(previous) or otherwise ensure both use the
same normalizer) in the mutation block where `previous`, `current`, `next`, and
`result` are computed.
- Around line 60-79: notifyExternalReadApiAccountStorageChanged currently awaits
sendMessageWithRetry and can block writers; change it to avoid blocking
background-side storage writes by making the notification fire-and-forget or
reduce retry attempts: either (A) call sendMessageWithRetry without awaiting
(void sendMessageWithRetry(...)) when in background context so
browser.storage.onChanged (already wired) provides delivery, and only await when
called from non-background contexts (detect via your existing context check or
add an isBackgroundContext helper), or (B) set sendMessageWithRetry options to
maxAttempts: 1 for background calls to avoid retries; keep the same payload
(RuntimeActionIds.ExternalReadApiStorageChanged, ACCOUNT_STORAGE_KEYS.ACCOUNTS,
oldValue, newValue) and retain the catch/log path for awaited calls.
In `@src/services/apiCredentialProfiles/apiCredentialProfilesStorage.ts`:
- Around line 44-63: The notifyExternalReadApiProfilesStorageChanged function
currently awaits sendMessageWithRetry inside write-locked code paths (called by
importConfig, mergeConfig, createProfile, updateProfile,
updateTelemetrySnapshot, removeTagIdFromAllProfiles, deleteProfile) which
serializes profile writes when no receiver is present; change it to be
non-blocking: do not await retries inside the lock—either call
sendMessageWithRetry in a fire-and-forget fashion (spawn async task and
swallow/log errors) or use a single-attempt notify API variant and log failures
immediately, or move the notification call out of the write lock to run after
the lock is released; update notifyExternalReadApiProfilesStorageChanged (and/or
its callers) to implement one of these approaches while preserving error logging
via getErrorMessage.
In `@tests/entrypoints/options/ExternalReadApiSettings.test.tsx`:
- Around line 177-189: Add an afterEach that undoes the global stubs to avoid
cross-test pollution: call vi.unstubAllGlobals() to revert
vi.stubGlobal("crypto") and restore navigator.clipboard to its prior value (save
the original clipboard in the surrounding scope before you stub it in
beforeEach, then use Object.defineProperty(globalThis.navigator, "clipboard", {
value: originalClipboard, configurable: true }) in afterEach). Ensure the
afterEach runs in the same test scope where vi.stubGlobal("crypto") and
Object.defineProperty(... "clipboard") are set so crypto and navigator.clipboard
are restored between tests.
In `@tests/services/integrations/externalReadApi.test.ts`:
- Around line 145-160: The test currently replaces globalThis.browser with an
ad-hoc stub (setting messageExternalListener/storageChangedListener) which
breaks the project-wide fake WebExtension behavior; instead import or use the
shared fake browser from wxt/testing/fake-browser (the one wired in
tests/setup.ts) and only add or augment runtime.onMessageExternal if missing—do
not overwrite globalThis.browser; locate the stub in this file referencing
globalThis.browser, messageExternalListener, storageChangedListener and replace
the assignment with code that obtains the fake from wxt/testing/fake-browser and
attaches an addListener wrapper for runtime.onMessageExternal and
storage.onChanged (or augment the existing fake with those properties) so
listener arrays, async response semantics, and sender plumbing remain consistent
with other tests.
In `@wxt.config.ts`:
- Around line 56-58: The externally_connectable block currently allows matches
["http://*/*", "https://*/*"], which exposes the extension API to every visited
origin; either narrow the match patterns to only the known sites/userscripts you
intend to support (update the externally_connectable.matches array) or, if the
wide scope is intentional (e.g., to support user-installed userscripts), add a
clear inline comment next to externally_connectable explaining that this broad
allowlist is deliberate and reference the service-level guard
ensureExternalAccessAllowed in
src/services/integrations/externalReadApi/index.ts so future maintainers
understand the rationale.
- Around line 56-58: The manifest's externally_connectable entry is
Firefox-unsupported and should be included only for non-Firefox builds; update
the manifest generation in wxt.config.ts to conditionally add the
externally_connectable object (the matches: ["http://*/*","https://*/*"] block)
only when env.browser (or the existing browser build variable used in that file)
is not 'firefox', so Firefox manifests omit the key and avoid validation
warnings while other browsers keep the entry.
---
Outside diff comments:
In `@src/services/apiCredentialProfiles/apiCredentialProfilesStorage.ts`:
- Around line 632-649: mergeConfig currently coerces the read config into
`local` and then calls notifyExternalReadApiProfilesStorageChanged(local,
merged), causing subscribers to see the coerced state as the "old" value;
instead, capture the raw on-disk result returned by readConfig() (like
importConfig does) before calling coerceApiCredentialProfilesConfig, and pass
that raw snapshot as the `oldValue` to
notifyExternalReadApiProfilesStorageChanged while keeping `merged` as the new
value so subscribers get the true pre-merge on-disk state; update mergeConfig to
read and store the rawRead (from readConfig()), use
coerceApiCredentialProfilesConfig(rawRead, { now }) to produce `local`, and call
notifyExternalReadApiProfilesStorageChanged(rawRead, merged).
---
Nitpick comments:
In `@src/contexts/UserPreferencesContext.tsx`:
- Around line 1219-1245: The updateExternalReadApi handler uses a non-null
assertion on DEFAULT_PREFERENCES which can crash if the default shape changes;
change the fallback to the same defensive pattern used by siblings by replacing
prev.externalReadApi ?? DEFAULT_PREFERENCES.externalReadApi! with a chained
fallback like prev.externalReadApi ?? DEFAULT_PREFERENCES.externalReadApi ?? ({
/* default ExternalReadApiPreferences fields */ } satisfies
ExternalReadApiPreferences) so deepOverride always receives a concrete object;
update references in updateExternalReadApi (and keep deepOverride,
ExternalReadApiPreferences, userPreferences.savePreferences, and setPreferences
usage intact).
In `@src/entrypoints/content/messageHandlers/handlers/externalReadApi.ts`:
- Around line 14-20: The sendResponse parameter of handleExternalReadApiNotify
is currently typed as (res: any) => void which loses type safety; replace this
with a precise callback type (e.g., a shared SendResponse<T> alias used in
runtimeMessages.ts) and import/use that alias so the response shape is enforced
— update the function signature in handleExternalReadApiNotify to use the typed
SendResponse<ResponsePayloadType> (or the existing alias from
src/entrypoints/background/runtimeMessages.ts) and adjust any call sites to
satisfy the new type.
- Line 50: The listener currently ends with an unnecessary "return true" in the
runtime.onMessage handler even though sendResponse is invoked synchronously in
both success and catch branches; remove the "return true" (or replace with an
explicit "return false" if you need a falsy handled flag) from the handler to
avoid implying an async response, and keep the existing synchronous sendResponse
calls in the same function (the runtime.onMessage listener and sendResponse
references will help you locate the code).
In
`@src/features/BasicSettings/components/tabs/General/ExternalReadApiSettings.tsx`:
- Line 241: The token is currently rendered plain in ExternalReadApiSettings by
<Input value={entry.token} readOnly disabled={isSaving} />, exposing sensitive
credentials; change this to support a show/hide toggle like the New API/Done Hub
patterns: add local state (e.g., showToken) within ExternalReadApiSettings,
render the Input as masked when showToken is false (use input type="password" or
a masked display) and reveal the token only when toggled, and add a small toggle
button/icon next to the Input that flips showToken while respecting isSaving and
readOnly semantics so tokens remain hidden by default.
In `@src/services/integrations/externalReadApi/index.ts`:
- Around line 81-103: Remove the empty JSDoc comment blocks that add noise
(e.g., the blank /** */ above createErrorResponse and createSuccessResponse) by
deleting them; for helper functions with non-obvious behavior (e.g.,
buildTrackedChangeSignature, filterTrackedStorageChanges,
handleTrackedStorageChange) replace the blank JSDoc with a concise one-line
comment describing the invariant or intent (not a full narrative). Ensure any
retained comment is a short inline description immediately above the function
declaration and remove all other purely empty JSDoc blocks throughout the file.
- Line 372: The current equality check uses JSON.stringify(previous) !==
JSON.stringify(account), which is unreliable and slow; replace this brittle
comparison in the account change detection with a structural deep-equality
helper that handles key ordering, undefined/NaN, and only the known shapes used
by your domain (accounts, bookmarks, profiles). Implement or import a small
deterministic deepEqual function and use it where the code currently compares
previous vs account, and similarly replace JSON.stringify checks inside
diffBookmarkChanges and diffProfileChanges with the same deepEqual (or a
shape-restricted comparator) to avoid false positives/negatives and reduce
overhead; reference the variables/functions previous, account,
diffBookmarkChanges, and diffProfileChanges when making the changes.
- Around line 302-338: The change events currently emitted by buildAccountEvent,
buildBookmarkEvent, buildProfileEvent (and buildAccountConfigEvent) lack entity
identifiers which makes per-entity consumers impossible to use; decide between
two fixes: (A) for fine-grained events, update these functions to accept an
entity identifier (e.g., id: string) and include id in the returned
ExternalReadApiChangeDetail (and ensure account.updated builders also include
changedKeys when available), then update callers that create batches to pass
each entity's id; or (B) for coarse notifications, collapse duplicate events
before broadcasting by deduplicating the out-going list by (type, topic) — e.g.,
produce one event per unique (type, topic) via a Set/map — and keep the existing
payload shape; pick one model and apply the corresponding change across
buildAccountEvent, buildBookmarkEvent, buildProfileEvent,
buildAccountConfigEvent and the code path that assembles/broadcasts the array so
all consumers receive consistent event granularity.
In `@tests/entrypoints/content/messageHandlers/handlers/externalReadApi.test.ts`:
- Around line 13-71: Add a negative-path unit test for
handleExternalReadApiNotify to cover sanitization and error fallback: create a
test that calls handleExternalReadApiNotify with a malformed payload (e.g.,
missing or invalid topics) and assert it returns the non-success response via
the sendResponse mock, does not dispatch the EXTERNAL_READ_API_EVENT_NAME event,
and handles postMessage failures (spy on window.postMessage to throw and assert
sendResponse receives the error path). Use the existing test's patterns
(sendResponse mock, eventListener registration, postMessage spy) to keep
setup/teardown consistent and verify both sanitization rejection and the
postMessage error branch.
In `@tests/services/integrations/externalReadApi.test.ts`:
- Around line 198-447: Add unit tests to cover the remaining branches in
externalReadApi/index.ts: (1) callExternal with an invalid namespace and assert
it resolves to createErrorResponse("Invalid namespace."); (2) callExternal with
a valid namespace but unsupported method and assert
createErrorResponse("Unsupported method."); (3) set
preferences.externalReadApi.notificationsEnabled = false, trigger the
storageChangedListener path that would normally call notifyAllTabs and assert
sendTabMessageWithRetry is NOT called; (4) import and call
handleExternalReadApiStorageChangedMessage directly to exercise the
unsupported-storage-key branch (pass a payload with an unknown key) and the
success path (pass a valid ACCOUNT_STORAGE_KEYS.ACCOUNTS change) asserting
expected return values and side effects; (5) test the deduplication by invoking
storageChangedListener/handleTrackedStorageChange twice with the same
last_updated and assert sendTabMessageWithRetry only broadcast once (covers
filterTrackedStorageChanges dedupe). Use existing helpers (callExternal,
setupIntegration, storageChangedListener, sendTabMessageWithRetryMock,
ACCOUNT_STORAGE_KEYS, EXTERNAL_READ_API_NAMESPACE) to implement each test.
🪄 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: c451b535-8500-4416-8ca7-6e78ed7514f9
📒 Files selected for processing (27)
src/constants/runtimeActions.tssrc/contexts/UserPreferencesContext.tsxsrc/entrypoints/background/index.tssrc/entrypoints/background/runtimeMessages.tssrc/entrypoints/content/messageHandlers/handlers/externalReadApi.tssrc/entrypoints/content/messageHandlers/handlers/index.tssrc/entrypoints/content/messageHandlers/index.tssrc/features/BasicSettings/components/tabs/General/ExternalReadApiSettings.tsxsrc/features/BasicSettings/components/tabs/General/GeneralTab.tsxsrc/locales/en/settings.jsonsrc/locales/ja/settings.jsonsrc/locales/zh-CN/settings.jsonsrc/locales/zh-TW/settings.jsonsrc/services/accounts/accountStorage.tssrc/services/apiCredentialProfiles/apiCredentialProfilesStorage.tssrc/services/integrations/externalReadApi/constants.tssrc/services/integrations/externalReadApi/index.tssrc/services/preferences/userPreferences.tstests/entrypoints/background/backgroundSuspendCleanup.test.tstests/entrypoints/background/runtimeMessages.more.test.tstests/entrypoints/content/messageHandlers/handlers/externalReadApi.test.tstests/entrypoints/options/ExternalReadApiSettings.test.tsxtests/entrypoints/options/pages/UsageAnalytics/UsageAnalyticsCharts.test.tsxtests/features/BasicSettings/PermissionOnboardingDialog.languageSelection.test.tsxtests/services/integrations/externalReadApi.test.tstests/services/userPreferences.test.tswxt.config.ts
| window.dispatchEvent(new CustomEvent(eventName, { detail })) | ||
| window.postMessage( | ||
| { | ||
| source: "all-api-hub-extension", | ||
| type: eventName, | ||
| detail, | ||
| }, | ||
| window.location.origin, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify what data ends up in the broadcast `events` payload and whether origin filtering exists.
fd -e ts -e tsx | xargs rg -nP -C3 '\b(diffAccountChanges|diffProfileChanges|diffBookmarkChanges|diffAccountConfigChanges|notifyAllTabs)\b'
fd -e ts externalReadApi --type f | xargs rg -nP -C3 'notifyAllTabs|ExternalReadApiNotifyContent'Repository: qixing-jk/all-api-hub
Length of output: 4571
🏁 Script executed:
#!/bin/bash
# Get the full context of externalReadApi.ts to understand the data flow
wc -l src/services/integrations/externalReadApi/index.ts
# Read the complete diff functions to see what's being exposed
sed -n '355,385p' src/services/integrations/externalReadApi/index.ts
sed -n '389,419p' src/services/integrations/externalReadApi/index.ts
sed -n '423,461p' src/services/integrations/externalReadApi/index.ts
sed -n '465,496p' src/services/integrations/externalReadApi/index.tsRepository: qixing-jk/all-api-hub
Length of output: 3880
🏁 Script executed:
#!/bin/bash
# Get the notifyAllTabs function and event building logic
sed -n '500,612p' src/services/integrations/externalReadApi/index.tsRepository: qixing-jk/all-api-hub
Length of output: 3045
🏁 Script executed:
#!/bin/bash
# Check the content handler file for any filtering/authorization logic
cat -n src/entrypoints/content/messageHandlers/handlers/externalReadApi.tsRepository: qixing-jk/all-api-hub
Length of output: 1857
🏁 Script executed:
#!/bin/bash
# Search for any comments about security or origin filtering in externalReadApi files
rg -nP '(security|authorization|token|allowlist|filter|origin)' src/services/integrations/externalReadApi/ src/entrypoints/content/messageHandlers/handlers/externalReadApi.tsRepository: qixing-jk/all-api-hub
Length of output: 1963
Broadcast of sensitive change events to all page scripts needs authorization gating.
The events payload contains diffed account/profile/bookmark changes (IDs and change types: created/updated/deleted), broadcast via window.dispatchEvent and window.postMessage to all scripts in the page context. Once the feature is enabled globally, any 3rd-party script (analytics, ads, embeds) on every page the user visits can passively listen and collect these notifications without ever presenting or validating a token.
Recommended mitigations:
- Gate dispatch on a per-origin allowlist managed alongside the tokens, or
- Include a token/handshake challenge in the message routing so the handler only fires after validation.
- At minimum, add a comment documenting the security model and why any script can read these events once enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/entrypoints/content/messageHandlers/handlers/externalReadApi.ts` around
lines 32 - 40, The current broadcasting via window.dispatchEvent and
window.postMessage (using eventName and detail) exposes sensitive change events
to any page script; fix by gating broadcasts: add an allowlist check (e.g.,
allowedOrigins array) and only call window.postMessage when
window.location.origin or the target origin is in allowedOrigins, and include a
per-session token field in the posted message (e.g., add token) so receivers
must perform a handshake/validation before acting; also add a clear inline
comment in externalReadApi.ts explaining the security model, why dispatchEvent
is limited/unsafe for cross-origin listeners, and reference the handshake
validator function (e.g., validateHandshake) that listeners must call.
| const handleDeleteToken = async (tokenId: string) => { | ||
| await saveSettings({ | ||
| tokens: config.tokens.filter((entry) => entry.id !== tokenId), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing confirmation for irreversible token deletion.
Deleting a token immediately invalidates external access for any client carrying that token (per the privacyWarning copy: "Deleting or disabling a token immediately cuts off that external access."). A single misclick on the trash icon will silently drop the token with no confirmation prompt and no undo.
Consider gating deletion behind a confirmation dialog (e.g., the existing modal/confirm utility used elsewhere in the app), or at least a two-step click-to-confirm pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/features/BasicSettings/components/tabs/General/ExternalReadApiSettings.tsx`
around lines 136 - 140, The handleDeleteToken function currently deletes a token
immediately; change it to first show a confirmation dialog (using the app's
existing modal/confirm utility used elsewhere) and only call saveSettings({
tokens: config.tokens.filter(entry => entry.id !== tokenId) }) if the user
confirms; ensure cancellation leaves state unchanged, disable the delete control
while awaiting the confirmation/save to avoid double clicks, and surface any
save errors to the user.
| "copyToken": "Copy", | ||
| "deleteToken": "Delete", | ||
| "description": "有効な access token を持つページやユーザースクリプトに、拡張機能の永続化データの読み取りを明示的に許可し、必要に応じて開いているページコンテキストへ軽量な変更通知を配信します。", | ||
| "emptyTokens": "利用可能な access token はまだありません。", | ||
| "enable": "外部の読み取り専用アクセスを有効化", | ||
| "enableDesc": "有効にすると、有効な token を持つ呼び出し元が All API Hub の永続化された主データ全体を問い合わせできます。", | ||
| "generateToken": "Generate Token", | ||
| "notifications": "変更通知を有効化", | ||
| "notificationsDesc": "有効にすると、開いているページが追跡対象データ変更後の fire-and-forget イベントを受け取れます。下流の配信保証や再試行は対象外です。", | ||
| "privacyWarning": "警告: 有効な token を持つ呼び出し元は、access_token、sessionCookie、refreshToken、apiKey などの機密フィールドを含む完全な永続化主データを読み取れます。token を削除または無効化すると、その外部アクセスは直ちに遮断されます。", | ||
| "regenerateToken": "Regenerate Token", | ||
| "title": "External Read API / Hook", | ||
| "tokenCopied": "Access token をコピーしました", | ||
| "tokenCopyFailed": "Access token のコピーに失敗しました", | ||
| "tokenCreatedAt": "作成日時: {{createdAt}}", | ||
| "tokenManagement": "Access tokens", | ||
| "tokenManagementDesc": "名前付き token 資格情報を作成します。下流はデータ読み取り時に各リクエストで token を渡すだけでよく、変更通知は上のトグルで別途制御します。", | ||
| "tokenNamePlaceholder": "例: metapi-sync" |
There was a problem hiding this comment.
Untranslated English strings in the Japanese locale.
Several values in the new externalReadApi block are bare English while the rest of the block is properly translated, and the zh-CN counterpart is fully localized. Please translate to Japanese for consistency:
copyToken: "Copy" → e.g. "コピー"deleteToken: "Delete" → e.g. "削除"generateToken: "Generate Token" → e.g. "Token を生成"regenerateToken: "Regenerate Token" → e.g. "Token を再生成"tokenManagement: "Access tokens" → e.g. "アクセストークン"
title ("External Read API / Hook") could reasonably stay as a feature brand, but please confirm intent — the zh-CN file translates the prose part. Based on coding guidelines, "Keep locale key shapes stable across languages; do not let one language drift into a pluralized or structurally different key family unless intentionally introduced for every locale" — the spirit applies here to translation completeness as well.
🌐 Suggested diff
"externalReadApi": {
- "copyToken": "Copy",
- "deleteToken": "Delete",
+ "copyToken": "コピー",
+ "deleteToken": "削除",
"description": "有効な access token を持つページやユーザースクリプトに、拡張機能の永続化データの読み取りを明示的に許可し、必要に応じて開いているページコンテキストへ軽量な変更通知を配信します。",
"emptyTokens": "利用可能な access token はまだありません。",
"enable": "外部の読み取り専用アクセスを有効化",
"enableDesc": "有効にすると、有効な token を持つ呼び出し元が All API Hub の永続化された主データ全体を問い合わせできます。",
- "generateToken": "Generate Token",
+ "generateToken": "Token を生成",
"notifications": "変更通知を有効化",
"notificationsDesc": "有効にすると、開いているページが追跡対象データ変更後の fire-and-forget イベントを受け取れます。下流の配信保証や再試行は対象外です。",
"privacyWarning": "警告: 有効な token を持つ呼び出し元は、access_token、sessionCookie、refreshToken、apiKey などの機密フィールドを含む完全な永続化主データを読み取れます。token を削除または無効化すると、その外部アクセスは直ちに遮断されます。",
- "regenerateToken": "Regenerate Token",
+ "regenerateToken": "Token を再生成",
"title": "External Read API / Hook",
"tokenCopied": "Access token をコピーしました",
"tokenCopyFailed": "Access token のコピーに失敗しました",
"tokenCreatedAt": "作成日時: {{createdAt}}",
- "tokenManagement": "Access tokens",
+ "tokenManagement": "アクセストークン",
"tokenManagementDesc": "名前付き token 資格情報を作成します。下流はデータ読み取り時に各リクエストで token を渡すだけでよく、変更通知は上のトグルで別途制御します。",
"tokenNamePlaceholder": "例: metapi-sync"
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/locales/ja/settings.json` around lines 147 - 164, The Japanese locale
contains untranslated strings in the externalReadApi block; update the keys
copyToken, deleteToken, generateToken, regenerateToken, and tokenManagement to
Japanese (e.g., "copyToken" → "コピー", "deleteToken" → "削除", "generateToken" →
"Token を生成", "regenerateToken" → "Token を再生成", "tokenManagement" → "アクセストークン")
so they match the rest of the block and zh-CN; also confirm whether "title"
("External Read API / Hook") should be localized or intentionally kept as a
brand and apply the chosen approach consistently.
| async function notifyExternalReadApiAccountStorageChanged( | ||
| oldValue: AccountStorageConfig, | ||
| newValue: AccountStorageConfig, | ||
| ) { | ||
| try { | ||
| await sendMessageWithRetry({ | ||
| action: RuntimeActionIds.ExternalReadApiStorageChanged, | ||
| storageKey: ACCOUNT_STORAGE_KEYS.ACCOUNTS, | ||
| oldValue, | ||
| newValue, | ||
| }) | ||
| } catch (error) { | ||
| logger.warn( | ||
| "Failed to notify external read API about account storage change", | ||
| { | ||
| error: getErrorMessage(error), | ||
| }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether handleExternalReadApiStorageChangedMessage is registered on background, and whether the integration also listens to browser.storage.onChanged for the same keys.
rg -nP -C3 'handleExternalReadApiStorageChangedMessage|ExternalReadApiStorageChanged'
rg -nP -C3 'storage\.onChanged' src/services/integrations/externalReadApiRepository: qixing-jk/all-api-hub
Length of output: 5111
Review the throughput impact of await-ing sendMessageWithRetry during storage writes.
notifyExternalReadApiAccountStorageChanged awaits sendMessageWithRetry, which retries up to 3 times with exponential backoff on "Receiving end does not exist" errors. When the background context holds the write lock and no popup/options are open, this message fails recoverably and burns through retries (~500ms + 1000ms delay, plus message timeouts) before logging a warning—serializing other writers behind that locked await.
While browser.storage.onChanged (already wired at src/services/integrations/externalReadApi/index.ts:726) provides redundant delivery on the background side, the blocking await still incurs the latency cost:
- For background-side writes: storage.onChanged will fire regardless, so the await adds unnecessary latency during retry failures.
- For popup/options-side writes: the await is useful to confirm the message reached the background.
Consider:
- Removing the
await(fire-and-forget) since storage changes are delivered via the listener anyway. - Or, if you need to confirm delivery from non-background contexts, split the logic:
voidthe notify on background (where storage.onChanged suffices), andawaitit elsewhere. - Or, set
maxAttempts: 1to avoid blocking on retries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/accounts/accountStorage.ts` around lines 60 - 79,
notifyExternalReadApiAccountStorageChanged currently awaits sendMessageWithRetry
and can block writers; change it to avoid blocking background-side storage
writes by making the notification fire-and-forget or reduce retry attempts:
either (A) call sendMessageWithRetry without awaiting (void
sendMessageWithRetry(...)) when in background context so
browser.storage.onChanged (already wired) provides delivery, and only await when
called from non-background contexts (detect via your existing context check or
add an isBackgroundContext helper), or (B) set sendMessageWithRetry options to
maxAttempts: 1 for background calls to avoid retries; keep the same payload
(RuntimeActionIds.ExternalReadApiStorageChanged, ACCOUNT_STORAGE_KEYS.ACCOUNTS,
oldValue, newValue) and retain the catch/log path for awaited calls.
| const previous = this.cloneConfig(await this.getStorageConfig()) | ||
| const current = this.cloneConfig(previous) | ||
| const { result, changed } = mutation(current) | ||
| if (!changed) { | ||
| return result | ||
| } | ||
|
|
||
| const next = normalizeAccountStorageConfigForWrite(current) | ||
| await this.storage.set(ACCOUNT_STORAGE_KEYS.ACCOUNTS, next) | ||
| await notifyExternalReadApiAccountStorageChanged(previous, next) | ||
| return result |
There was a problem hiding this comment.
Notification payload mixes read- and write-normalized shapes.
previous is derived from getStorageConfig() (which applies normalizeAccountStorageConfigForRead), while next is normalizeAccountStorageConfigForWrite(current). External readers receiving oldValue/newValue in the same message will see asymmetric shapes for fields that differ between the read- and write-normalized forms (e.g., legacy fields filled in on read, dropped on write).
If subscribers diff oldValue against newValue, they will see spurious changes on the very first notification after a legacy read. Consider normalizing previous with the write normalizer (or both with the same normalizer) before sending.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/accounts/accountStorage.ts` around lines 138 - 148, The
notification currently sends an oldValue (`previous`) produced by
getStorageConfig() (which uses the read normalizer) and a newValue (`next`)
produced by normalizeAccountStorageConfigForWrite(current), causing mismatched
shapes; fix by normalizing `previous` with the same write normalizer before
calling notifyExternalReadApiAccountStorageChanged so both `oldValue` and
`newValue` share the write-normalized shape (use
normalizeAccountStorageConfigForWrite(previous) or otherwise ensure both use the
same normalizer) in the mutation block where `previous`, `current`, `next`, and
`result` are computed.
| async function notifyExternalReadApiProfilesStorageChanged( | ||
| oldValue: ApiCredentialProfilesConfig, | ||
| newValue: ApiCredentialProfilesConfig, | ||
| ) { | ||
| try { | ||
| await sendMessageWithRetry({ | ||
| action: RuntimeActionIds.ExternalReadApiStorageChanged, | ||
| storageKey: API_CREDENTIAL_PROFILES_STORAGE_KEYS.API_CREDENTIAL_PROFILES, | ||
| oldValue, | ||
| newValue, | ||
| }) | ||
| } catch (error) { | ||
| logger.warn( | ||
| "Failed to notify external read API about profile storage change", | ||
| { | ||
| error: getErrorMessage(error), | ||
| }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same notification cost-on-write concern as accountStorage.ts.
This helper has the same characteristic as notifyExternalReadApiAccountStorageChanged in src/services/accounts/accountStorage.ts: each call is await-ed inside a write-locked path, and if there is no live message receiver (e.g., only background is running), sendMessageWithRetry will retry up to 3× with exponential backoff before the warning is finally logged. Across importConfig, mergeConfig, createProfile, updateProfile, updateTelemetrySnapshot, removeTagIdFromAllProfiles, and deleteProfile, this serializes all profile writes behind that retry budget when no popup/options is open.
See the corresponding comment on src/services/accounts/accountStorage.ts for proposed mitigations (fire-and-forget, single-attempt config, or rely on browser.storage.onChanged when in-process).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/apiCredentialProfiles/apiCredentialProfilesStorage.ts` around
lines 44 - 63, The notifyExternalReadApiProfilesStorageChanged function
currently awaits sendMessageWithRetry inside write-locked code paths (called by
importConfig, mergeConfig, createProfile, updateProfile,
updateTelemetrySnapshot, removeTagIdFromAllProfiles, deleteProfile) which
serializes profile writes when no receiver is present; change it to be
non-blocking: do not await retries inside the lock—either call
sendMessageWithRetry in a fire-and-forget fashion (spawn async task and
swallow/log errors) or use a single-attempt notify API variant and log failures
immediately, or move the notification call out of the write lock to run after
the lock is released; update notifyExternalReadApiProfilesStorageChanged (and/or
its callers) to implement one of these approaches while preserving error logging
via getErrorMessage.
| vi.stubGlobal("crypto", { | ||
| randomUUID: vi.fn(() => "uuid-2"), | ||
| getRandomValues: vi.fn((values: Uint8Array) => { | ||
| values.fill(1) | ||
| return values | ||
| }), | ||
| }) | ||
| Object.defineProperty(globalThis.navigator, "clipboard", { | ||
| value: { | ||
| writeText: vi.fn().mockResolvedValue(undefined), | ||
| }, | ||
| configurable: true, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if vitest is configured with auto-unstub.
fd -t f 'vitest.config' --exec catRepository: qixing-jk/all-api-hub
Length of output: 2704
🏁 Script executed:
cat -n tests/entrypoints/options/ExternalReadApiSettings.test.tsx | head -300Repository: qixing-jk/all-api-hub
Length of output: 8615
Add afterEach to unstub globals and restore navigator.clipboard to prevent cross-test pollution.
vi.stubGlobal("crypto", ...) at line 177 and Object.defineProperty(globalThis.navigator, "clipboard", ...) at line 184 are set in beforeEach but never cleaned up. vi.clearAllMocks() does not revert stubs — only vi.unstubAllGlobals() does. Without explicit unstubGlobals: true in vitest config, these stubs persist across tests and may leak to other test files in the same worker.
Add afterEach cleanup
beforeEach(() => {
vi.clearAllMocks()
mockUpdateExternalReadApi.mockResolvedValue(true)
mockResetExternalReadApiConfig.mockResolvedValue(true)
mockedUseUserPreferencesContext.mockReturnValue(createContextValue())
vi.stubGlobal("crypto", {
randomUUID: vi.fn(() => "uuid-2"),
getRandomValues: vi.fn((values: Uint8Array) => {
values.fill(1)
return values
}),
})
Object.defineProperty(globalThis.navigator, "clipboard", {
value: {
writeText: vi.fn().mockResolvedValue(undefined),
},
configurable: true,
})
})
+ afterEach(() => {
+ vi.unstubAllGlobals()
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.stubGlobal("crypto", { | |
| randomUUID: vi.fn(() => "uuid-2"), | |
| getRandomValues: vi.fn((values: Uint8Array) => { | |
| values.fill(1) | |
| return values | |
| }), | |
| }) | |
| Object.defineProperty(globalThis.navigator, "clipboard", { | |
| value: { | |
| writeText: vi.fn().mockResolvedValue(undefined), | |
| }, | |
| configurable: true, | |
| }) | |
| beforeEach(() => { | |
| vi.clearAllMocks() | |
| mockUpdateExternalReadApi.mockResolvedValue(true) | |
| mockResetExternalReadApiConfig.mockResolvedValue(true) | |
| mockedUseUserPreferencesContext.mockReturnValue(createContextValue()) | |
| vi.stubGlobal("crypto", { | |
| randomUUID: vi.fn(() => "uuid-2"), | |
| getRandomValues: vi.fn((values: Uint8Array) => { | |
| values.fill(1) | |
| return values | |
| }), | |
| }) | |
| Object.defineProperty(globalThis.navigator, "clipboard", { | |
| value: { | |
| writeText: vi.fn().mockResolvedValue(undefined), | |
| }, | |
| configurable: true, | |
| }) | |
| }) | |
| afterEach(() => { | |
| vi.unstubAllGlobals() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/entrypoints/options/ExternalReadApiSettings.test.tsx` around lines 177
- 189, Add an afterEach that undoes the global stubs to avoid cross-test
pollution: call vi.unstubAllGlobals() to revert vi.stubGlobal("crypto") and
restore navigator.clipboard to its prior value (save the original clipboard in
the surrounding scope before you stub it in beforeEach, then use
Object.defineProperty(globalThis.navigator, "clipboard", { value:
originalClipboard, configurable: true }) in afterEach). Ensure the afterEach
runs in the same test scope where vi.stubGlobal("crypto") and
Object.defineProperty(... "clipboard") are set so crypto and navigator.clipboard
are restored between tests.
| ;(globalThis as any).browser = { | ||
| runtime: { | ||
| onMessageExternal: { | ||
| addListener: vi.fn((listener: RuntimeMessageExternalListener) => { | ||
| messageExternalListener = listener | ||
| }), | ||
| }, | ||
| }, | ||
| storage: { | ||
| onChanged: { | ||
| addListener: vi.fn((listener: StorageChangedListener) => { | ||
| storageChangedListener = listener | ||
| }), | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use wxt/testing/fake-browser instead of hand-rolling the browser global.
The test installs an ad-hoc globalThis.browser with stub addListener capture. The repo's testing convention is to mock WebExtension APIs through wxt/testing/fake-browser (set up in tests/setup.ts); doing it differently here means listener wiring, storage events, and runtime.onMessageExternal won't behave like the real shim used by every other test (e.g., listener arrays, async response semantics, sender plumbing).
If onMessageExternal is missing from fake-browser, augment it locally on top of the fake instance rather than replacing the whole browser object. As per coding guidelines: "WebExtension APIs should be mocked with wxt/testing/fake-browser".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/services/integrations/externalReadApi.test.ts` around lines 145 - 160,
The test currently replaces globalThis.browser with an ad-hoc stub (setting
messageExternalListener/storageChangedListener) which breaks the project-wide
fake WebExtension behavior; instead import or use the shared fake browser from
wxt/testing/fake-browser (the one wired in tests/setup.ts) and only add or
augment runtime.onMessageExternal if missing—do not overwrite
globalThis.browser; locate the stub in this file referencing globalThis.browser,
messageExternalListener, storageChangedListener and replace the assignment with
code that obtains the fake from wxt/testing/fake-browser and attaches an
addListener wrapper for runtime.onMessageExternal and storage.onChanged (or
augment the existing fake with those properties) so listener arrays, async
response semantics, and sender plumbing remain consistent with other tests.
| externally_connectable: { | ||
| matches: ["http://*/*", "https://*/*"], | ||
| }, |
There was a problem hiding this comment.
Broad http://*/* + https://*/* matches expose the external API surface to every site the user visits.
While the service layer enforces per-token authentication (see ensureExternalAccessAllowed in src/services/integrations/externalReadApi/index.ts), this match pattern still lets any origin attempt to connect and probe the API (including timing/error-channel side observation). If the intent is to interoperate with specific known sites/userscripts, consider narrowing the matches list, or at least adding an inline comment explaining why the broadest possible scope is intentional ("user-installed userscripts run on arbitrary origins") so future maintainers know it's deliberate, not an oversight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wxt.config.ts` around lines 56 - 58, The externally_connectable block
currently allows matches ["http://*/*", "https://*/*"], which exposes the
extension API to every visited origin; either narrow the match patterns to only
the known sites/userscripts you intend to support (update the
externally_connectable.matches array) or, if the wide scope is intentional
(e.g., to support user-installed userscripts), add a clear inline comment next
to externally_connectable explaining that this broad allowlist is deliberate and
reference the service-level guard ensureExternalAccessAllowed in
src/services/integrations/externalReadApi/index.ts so future maintainers
understand the rationale.
🧩 Analysis chain
🌐 Web query:
Does Firefox WebExtension manifest support externally_connectable in 2026?
💡 Result:
No, Firefox WebExtensions manifest does not support "externally_connectable" for web page communication in 2026. The key is documented in MDN, but explicitly states that web page communication via runtime.connect/sendMessage from web pages is not supported in Firefox (see bug 1319168). Extension-to-extension communication works without it (equivalent to {"ids": ["*"]}), and the related runtime.onMessageExternal/onConnectExternal APIs are supported since Firefox 54. However, the full feature, especially "matches" for websites, remains unimplemented as of the latest docs (updated 2025) and open bug status. No 2026 changes indicate implementation. Use content scripts or postMessage for web page to extension communication instead.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/externally_connectable
- 2: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onMessageExternal
- 3: https://bugzilla.mozilla.org/show_bug.cgi?id=1319168
- 4: https://bugzil.la/1319168
- 5: https://bugzilla.mozilla.org/show_bug.cgi?id=1258360
- 6: https://blog.mozilla.org/addons/2026/04/23/webextensions-api-changes-firefox-149-152/
- 7: https://extensionworkshop.com/documentation/develop/manifest-v3-migration-guide
Gate externally_connectable on non-Firefox browsers — this manifest key is not supported in Firefox (MDN docs, bug 1319168) and will produce validation warnings in Firefox builds. Since runtime.onMessageExternal also doesn't fire from web pages on Firefox, declaring this key is both ineffective and noisy. The surrounding manifest already branches on env.browser, so scope this declaration to non-Firefox to keep Firefox manifests clean.
Suggested fix
// ensure can get site cookies, please refer to https://stackoverflow.com/a/70070106/22460724
host_permissions: ["<all_urls>"],
- externally_connectable: {
- matches: ["http://*/*", "https://*/*"],
- },
+ ...(env.browser === "firefox"
+ ? {}
+ : {
+ externally_connectable: {
+ matches: ["http://*/*", "https://*/*"],
+ },
+ }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wxt.config.ts` around lines 56 - 58, The manifest's externally_connectable
entry is Firefox-unsupported and should be included only for non-Firefox builds;
update the manifest generation in wxt.config.ts to conditionally add the
externally_connectable object (the matches: ["http://*/*","https://*/*"] block)
only when env.browser (or the existing browser build variable used in that file)
is not 'firefox', so Firefox manifests omit the key and avoid validation
warnings while other browsers keep the entry.
|
Preview build ready for commit Artifacts:
This build is for review only and is not released. |
|
这个PR和那个issue之间是什么关系？没有太明白 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
我的理解是外部三方平台太多了，如果都在插件内容去写后续很难解耦，所以直接提供一套对外访问的接口和消息组件，然后再通过中间层去桥接实现，比如我会通过 tampermonkey 脚本实现 通过消息自动或者手动更新新数据到 metapi |
|
外部API确实是一个很有创意的想法，少有的浏览器扩展暴露API，感谢创意和PR。等我处理完其他部分再来处理这个API的功能 |
3 participants
feat(accounts): Add external reading API integration and account storage change notification functionality
Summary by CodeRabbit
Release Notes