feat(SUP-23): implement session storage for pending capture state restoration#104
feat(SUP-23): implement session storage for pending capture state restoration#104arjun-veer wants to merge 5 commits intomainfrom
Conversation
Summary by CodeRabbit
WalkthroughAdds session-scoped persistence and restore for pending capture state to CaptureMemoryManager, centralizes rendering/state handling, ensures pending state is saved/cleared on show/hide/error, integrates restoration into content script startup, tightens cleanup logging, and makes captureEnabled default to false in settings. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ContentScript as Content Script
participant MemoryMgr as CaptureMemoryManager
participant Storage as sessionStorage
participant UI as Modal UI
ContentScript->>MemoryMgr: restoreIfNeeded(ctx)
MemoryMgr->>Storage: read(STORAGE_KEY)
alt stored valid state
MemoryMgr->>MemoryMgr: isValidStoredState(data)
MemoryMgr->>MemoryMgr: construct manager (cache title/domain)
MemoryMgr->>MemoryMgr: show() -> savePendingState()
MemoryMgr->>UI: render() with cached title/domain
MemoryMgr-->>ContentScript: return restored instance
else no/invalid state
MemoryMgr-->>ContentScript: return null
ContentScript->>MemoryMgr: create new instance (if needed)
end
User->>UI: interact (submit/hide/error)
UI->>MemoryMgr: hide()/error() -> clearPendingState()
MemoryMgr->>Storage: remove(STORAGE_KEY)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/entrypoints/content/index.ts (1)
147-165:⚠️ Potential issue | 🔴 CriticalRestored
captureMemoryManageris immediately overwritten on line 165.After restoration (lines 147–153), execution falls through because
fieldTracker,submissionMonitor, andcaptureServiceare stillnull— so the early-return guard at lines 138–145 doesn't trigger. Line 165 then unconditionally creates a newCaptureMemoryManager(), discarding the just-restored instance. The restored modal becomes orphaned (visible but unmanageable).Proposed fix — skip re-creation when restored manager exists
if (!captureMemoryManager) { const restored = await CaptureMemoryManager.restoreIfNeeded(ctx); if (restored) { captureMemoryManager = restored; logger.info("Restored pending capture modal from storage"); } } logger.info("Initializing memory capture for site:", { hostname, pathname, reason, }); try { fieldTracker = await getFieldDataTracker(); submissionMonitor = getFormSubmissionMonitor(); captureService = new CaptureService(); - captureMemoryManager = new CaptureMemoryManager(); + if (!captureMemoryManager) { + captureMemoryManager = new CaptureMemoryManager(); + } submissionMonitor.start();
🤖 Fix all issues with AI agents
In `@src/entrypoints/content/components/capture-memory-manager.tsx`:
- Around line 490-504: The isValidStoredState type guard currently only checks
capturedFields is a non-empty array; update isValidStoredState (and the
StoredCaptureState validation) to validate the shape of capturedFields entries
by checking that at least the first element (or all elements) is an object with
required properties (e.g., fieldOpid is a string/number as expected, question is
a string, answer is a string or null/undefined per model) and that their types
are correct before returning true; reference the isValidStoredState function and
the capturedFields array and ensure this stronger validation prevents passing
malformed data into saveCapturedMemories or render paths.
- Around line 473-478: restoreIfNeeded sets manager.siteTitle/siteDomain but
calling CaptureMemoryManager.show() then unconditionally overwrites them (and
calls savePendingState()) — change show() to avoid re-reading the DOM and
re-saving when we are restoring: either add an optional parameter (e.g.,
show({skipDomRestore: true}) or isRestore flag) or check existing values inside
show() before assigning from document.title/window.location.hostname, and skip
calling savePendingState() when that flag/condition indicates a restoration;
update callers (the restore path that sets manager.siteTitle/siteDomain) to call
show with the restore flag or rely on the guarded assignments so the original
captured title/domain are preserved.
There was a problem hiding this comment.
Pull request overview
Implements restoration of the “pending capture” modal after reload/navigation by persisting the modal state and attempting to restore it when the content script initializes auto-capture.
Changes:
- Add a restore hook in the content script startup flow to re-open a pending capture modal if persisted state exists.
- Persist/clear pending capture modal state from the capture UI manager and add restoration/validation logic (versioning + expiry).
- Refactor
CaptureMemoryManager.renderto rely on storedsiteTitle/siteDomainfields rather than passing them as parameters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/entrypoints/content/index.ts | Attempts to restore a pending capture modal before initializing auto-capture services. |
| src/entrypoints/content/components/capture-memory-manager.tsx | Adds pending modal state persistence + restore/validation logic, and refactors render to use stored site metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/entrypoints/content/index.ts (1)
419-421: 🧹 Nitpick | 🔵 Trivial
captureMemoryManager.hide()is not awaited inonInvalidated.While
hide()is declaredasync, its internals (clearPendingStateviasessionStorage.removeItem,ui.remove()) are synchronous, so this works in practice. However, ifhide()ever gains truly async work, this would silently skip cleanup. Consider addingvoidto signal intentional fire-and-forget:Proposed fix
if (captureMemoryManager) { - captureMemoryManager.hide(); + void captureMemoryManager.hide(); }
🤖 Fix all issues with AI agents
In `@src/entrypoints/content/components/capture-memory-manager.tsx`:
- Around line 398-416: In savePendingState(), stop using sessionStorage and
instead persist the same StoredCaptureState via WXT's storage API: call
storage.defineItem("session:captureState") (or the existing STORAGE_KEY prefixed
with "session:") and use its set method to store the JSON/state; update
savePendingState to be async (or return the Promise) and await the storage call,
and convert the existing try/catch to handle Promise rejections and log errors
via logger.error as before. Locate the savePendingState method in
CaptureMemoryManager (and adjust any corresponding loadPendingState logic to
read from storage.get or the defined item) so persistence is moved from
page-accessible sessionStorage to browser.storage.session using the session:
prefix.
…ge for pending state
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/entrypoints/content/components/capture-memory-manager.tsx`:
- Around line 213-225: The guard in show() uses falsy checks on this.siteTitle
and this.siteDomain which treats legitimate empty strings (e.g., document.title
=== "") as unset; change to use explicit sentinel/flag: initialize
this.siteTitle and this.siteDomain to undefined (or add boolean flags like
this.siteTitleSet) and in show() check against undefined (or check the flag)
before assigning document.title/window.location.hostname, and ensure restored
empty titles/domains are preserved; keep savePendingState() call as-is.
- Line 36: Remove the dead constant STORAGE_KEY which is no longer used; locate
the top-level const STORAGE_KEY = "superfill:pendingCaptureModal" in
capture-memory-manager.tsx and delete it (the code now uses
pendingCaptureStorage which contains its own key string), and run a quick grep
for STORAGE_KEY to ensure there are no remaining references before committing.
- Around line 418-424: Remove the dead QuotaExceededError branch and simplify
the catch in the save/persist logic so it matches browser.storage.session
failure modes: stop checking error instanceof Error && error.name ===
"QuotaExceededError" and instead treat all rejections as a single catch-all,
logging the error via logger.error with a clear message (e.g., "Failed to save
pending capture state") and the error object/string; reference the existing
logger.error call and the storage usage of browser.storage.session to locate and
replace the conditional branch in the catch block.
- Around line 483-488: The restore flow re-saves pending state with a fresh
timestamp because CaptureMemoryManager.show() always calls savePendingState(),
so restoreIfNeeded (which sets manager.siteTitle/siteDomain and calls
manager.show()) extends TTL; modify show() and/or savePendingState() to accept a
flag or optional originalTimestamp so that when restoreIfNeeded calls show() it
either skips re-saving or re-saves using pendingState.timestamp; update
restoreIfNeeded to pass that flag/original timestamp (reference:
restoreIfNeeded, CaptureMemoryManager.show, savePendingState,
pendingState.timestamp) so restored state keeps its original timestamp or avoids
writing a new one.
- Around line 406-417: Change the noisy success-path logs from info to debug for
internal bookkeeping in the capture memory manager: in savePendingState()
replace the logger.info("Saved pending capture modal state...") call with a
debug-level log, and likewise in the corresponding clearPendingState() method
replace its logger.info(...) call (the "Cleared pending capture modal state..."
message) with logger.debug(...); this keeps normal hide/save flows quiet while
preserving the messages for troubleshooting.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/entrypoints/content/index.ts (1)
410-441:⚠️ Potential issue | 🟡 MinorUnhandled promise from
captureMemoryManager.hide()during invalidation; inconsistent error handling.Two concerns:
Fire-and-forget
hide()(line 435):hide()is async and callsclearPendingState(), butonInvalidateddoesn't await it. During extension reload/update, theclearPendingStatestorage write may not complete, leaving orphaned state inbrowser.storage.session. For normal page navigation this is actually desirable (state should persist for restoration), but during invalidation the intent appears to be cleanup.Empty catch blocks (lines 413, 417, 422, 440) contrast with the improved
logger.debuglogging instopAutoCapture. Consider applying the samelogger.debugpattern here for consistency, since these are the same kinds of cleanup operations.Proposed improvement
ctx.onInvalidated(() => { try { formDetectionService.dispose(); - } catch {} + } catch (error) { + logger.debug("Error disposing form detection service:", error); + } try { unwatchCaptureSettings?.(); - } catch {} + } catch (error) { + logger.debug("Error unwatching capture settings:", error); + } unwatchCaptureSettings = null; try { unlistenSubmission?.(); - } catch {} + } catch (error) { + logger.debug("Error removing submission listener:", error); + } unlistenSubmission = null; if (fieldTracker) { fieldTracker.dispose(); } if (submissionMonitor) { submissionMonitor.dispose(); } if (captureService) { captureService.dispose(); } if (captureMemoryManager) { - captureMemoryManager.hide(); + void captureMemoryManager.hide(); } try { cleanupGuideListener(); - } catch {} + } catch (error) { + logger.debug("Error cleaning up guide listener:", error); + } });Using
voidon line 435 makes the intentional fire-and-forget explicit (suppresses the floating promise lint), and thelogger.debugadditions maintain consistency withstopAutoCapture.
🤖 Fix all issues with AI agents
In `@src/entrypoints/content/components/capture-memory-manager.tsx`:
- Around line 456-464: The code currently constructs new
URL(pendingState.siteUrl) which can throw for malformed siteUrl; update
validation in isValidStoredState to verify pendingState.siteUrl is a well-formed
URL (e.g., attempt new URL(...) inside that validator or use a safe URL-regex)
so invalid stored states are rejected earlier, or add a small try-catch around
new URL(pendingState.siteUrl) before comparing hostnames to convert malformed
values into the same stale-state-clearing path
(pendingCaptureStorage.setValue(null) and logger.info) without bubbling to the
outer error log; reference pendingState.siteUrl, isValidStoredState,
pendingCaptureStorage.setValue, and logger.info when making the change.
- Around line 482-490: The assignment to manager.currentFields is redundant
because CaptureMemoryManager.show() already sets currentFields from the passed
capturedFields; remove the line "manager.currentFields =
pendingState.capturedFields" and keep the instantiation and the call to
manager.show(ctx, pendingState.capturedFields, { skipPersist: true }); ensure
manager.siteTitle and manager.siteDomain assignments remain as they are.
| const currentUrl = window.location.href; | ||
| const pendingUrl = new URL(pendingState.siteUrl); | ||
| const currentUrlObj = new URL(currentUrl); | ||
|
|
||
| if (pendingUrl.hostname !== currentUrlObj.hostname) { | ||
| await pendingCaptureStorage.setValue(null); | ||
| logger.info("Cleared stale pending capture from different domain"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
new URL() on stored siteUrl can throw on malformed data — handled by outer catch, but consider validating earlier.
If a corrupted siteUrl passes the typeof === "string" check in isValidStoredState but isn't a valid URL, new URL(pendingState.siteUrl) on line 457 throws. The outer try-catch handles it gracefully (clears state and returns null), so this isn't a functional bug, but it means a recoverable validation failure takes the same heavy error-logging path as an unexpected crash.
Proposed enhancement — validate URL in isValidStoredState
private static isValidStoredState(data: unknown): data is StoredCaptureState {
if (typeof data !== "object" || data === null) return false;
const state = data as Record<string, unknown>;
+ // Validate siteUrl is a parseable URL
+ let parsedUrl: URL;
+ try {
+ parsedUrl = new URL(state.siteUrl as string);
+ } catch {
+ return false;
+ }
+
return (
Array.isArray(state.capturedFields) &&
typeof state.siteUrl === "string" &&
typeof state.siteTitle === "string" &&🤖 Prompt for AI Agents
In `@src/entrypoints/content/components/capture-memory-manager.tsx` around lines
456 - 464, The code currently constructs new URL(pendingState.siteUrl) which can
throw for malformed siteUrl; update validation in isValidStoredState to verify
pendingState.siteUrl is a well-formed URL (e.g., attempt new URL(...) inside
that validator or use a safe URL-regex) so invalid stored states are rejected
earlier, or add a small try-catch around new URL(pendingState.siteUrl) before
comparing hostnames to convert malformed values into the same
stale-state-clearing path (pendingCaptureStorage.setValue(null) and logger.info)
without bubbling to the outer error log; reference pendingState.siteUrl,
isValidStoredState, pendingCaptureStorage.setValue, and logger.info when making
the change.
| const manager = new CaptureMemoryManager(); | ||
| manager.currentFields = pendingState.capturedFields; | ||
| manager.siteTitle = pendingState.siteTitle; | ||
| manager.siteDomain = pendingState.siteDomain; | ||
|
|
||
| await manager.show(ctx, pendingState.capturedFields, { | ||
| skipPersist: true, | ||
| }); | ||
| return manager; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant assignment of currentFields before show().
Line 483 assigns manager.currentFields, but show() (line 217) immediately overwrites it with the same value. The assignment is harmless but unnecessary.
Proposed cleanup
const manager = new CaptureMemoryManager();
- manager.currentFields = pendingState.capturedFields;
manager.siteTitle = pendingState.siteTitle;
manager.siteDomain = pendingState.siteDomain;🤖 Prompt for AI Agents
In `@src/entrypoints/content/components/capture-memory-manager.tsx` around lines
482 - 490, The assignment to manager.currentFields is redundant because
CaptureMemoryManager.show() already sets currentFields from the passed
capturedFields; remove the line "manager.currentFields =
pendingState.capturedFields" and keep the instantiation and the call to
manager.show(ctx, pendingState.capturedFields, { skipPersist: true }); ensure
manager.siteTitle and manager.siteDomain assignments remain as they are.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| state.capturedFields.every( | ||
| (f: unknown) => | ||
| typeof f === "object" && | ||
| f !== null && | ||
| typeof (f as Record<string, unknown>).fieldOpid === "string" && | ||
| typeof (f as Record<string, unknown>).formOpid === "string" && | ||
| typeof (f as Record<string, unknown>).question === "string" && | ||
| typeof (f as Record<string, unknown>).answer === "string" && | ||
| typeof (f as Record<string, unknown>).timestamp === "number" && | ||
| typeof (f as Record<string, unknown>).wasAIFilled === "boolean" && | ||
| typeof (f as Record<string, unknown>).fieldMetadata === "object" && | ||
| (f as Record<string, unknown>).fieldMetadata !== null, | ||
| ) |
There was a problem hiding this comment.
The validation function doesn't check for optional fields in CapturedFieldData. According to the CapturedFieldData interface, fields like originalAIValue and aiConfidence are optional, and fieldMetadata has nested optional properties like placeholder. The validation should handle these optional fields gracefully rather than rejecting stored data that lacks them. Consider checking that these fields are either undefined or have the correct type when present.
| state.capturedFields.every( | |
| (f: unknown) => | |
| typeof f === "object" && | |
| f !== null && | |
| typeof (f as Record<string, unknown>).fieldOpid === "string" && | |
| typeof (f as Record<string, unknown>).formOpid === "string" && | |
| typeof (f as Record<string, unknown>).question === "string" && | |
| typeof (f as Record<string, unknown>).answer === "string" && | |
| typeof (f as Record<string, unknown>).timestamp === "number" && | |
| typeof (f as Record<string, unknown>).wasAIFilled === "boolean" && | |
| typeof (f as Record<string, unknown>).fieldMetadata === "object" && | |
| (f as Record<string, unknown>).fieldMetadata !== null, | |
| ) | |
| state.capturedFields.every((f: unknown) => { | |
| if (typeof f !== "object" || f === null) { | |
| return false; | |
| } | |
| const field = f as Record<string, unknown>; | |
| if ( | |
| typeof field.fieldOpid !== "string" || | |
| typeof field.formOpid !== "string" || | |
| typeof field.question !== "string" || | |
| typeof field.answer !== "string" || | |
| typeof field.timestamp !== "number" || | |
| typeof field.wasAIFilled !== "boolean" | |
| ) { | |
| return false; | |
| } | |
| if ( | |
| typeof field.fieldMetadata !== "object" || | |
| field.fieldMetadata === null | |
| ) { | |
| return false; | |
| } | |
| const fieldMetadata = field.fieldMetadata as Record<string, unknown>; | |
| // Optional top-level field: originalAIValue (string when present) | |
| if ( | |
| "originalAIValue" in field && | |
| field.originalAIValue !== undefined && | |
| typeof field.originalAIValue !== "string" | |
| ) { | |
| return false; | |
| } | |
| // Optional top-level field: aiConfidence (number when present) | |
| if ( | |
| "aiConfidence" in field && | |
| field.aiConfidence !== undefined && | |
| typeof field.aiConfidence !== "number" | |
| ) { | |
| return false; | |
| } | |
| // Optional nested field: fieldMetadata.placeholder (string when present) | |
| if ( | |
| "placeholder" in fieldMetadata && | |
| fieldMetadata.placeholder !== undefined && | |
| typeof fieldMetadata.placeholder !== "string" | |
| ) { | |
| return false; | |
| } | |
| return true; | |
| }) |
| state.capturedFields.every( | ||
| (f: unknown) => | ||
| typeof f === "object" && | ||
| f !== null && | ||
| typeof (f as Record<string, unknown>).fieldOpid === "string" && | ||
| typeof (f as Record<string, unknown>).formOpid === "string" && | ||
| typeof (f as Record<string, unknown>).question === "string" && | ||
| typeof (f as Record<string, unknown>).answer === "string" && | ||
| typeof (f as Record<string, unknown>).timestamp === "number" && | ||
| typeof (f as Record<string, unknown>).wasAIFilled === "boolean" && | ||
| typeof (f as Record<string, unknown>).fieldMetadata === "object" && | ||
| (f as Record<string, unknown>).fieldMetadata !== null, | ||
| ) |
There was a problem hiding this comment.
The validation function doesn't validate the nested fieldMetadata properties according to the CapturedFieldData interface. The interface specifies that fieldMetadata should have properties like type, purpose, labels (array), required (boolean), and optional placeholder. Consider adding validation for these specific properties to ensure the stored state matches the expected structure.
| state.capturedFields.every( | |
| (f: unknown) => | |
| typeof f === "object" && | |
| f !== null && | |
| typeof (f as Record<string, unknown>).fieldOpid === "string" && | |
| typeof (f as Record<string, unknown>).formOpid === "string" && | |
| typeof (f as Record<string, unknown>).question === "string" && | |
| typeof (f as Record<string, unknown>).answer === "string" && | |
| typeof (f as Record<string, unknown>).timestamp === "number" && | |
| typeof (f as Record<string, unknown>).wasAIFilled === "boolean" && | |
| typeof (f as Record<string, unknown>).fieldMetadata === "object" && | |
| (f as Record<string, unknown>).fieldMetadata !== null, | |
| ) | |
| state.capturedFields.every((f: unknown) => { | |
| if (typeof f !== "object" || f === null) { | |
| return false; | |
| } | |
| const field = f as Record<string, unknown>; | |
| if ( | |
| typeof field.fieldOpid !== "string" || | |
| typeof field.formOpid !== "string" || | |
| typeof field.question !== "string" || | |
| typeof field.answer !== "string" || | |
| typeof field.timestamp !== "number" || | |
| typeof field.wasAIFilled !== "boolean" | |
| ) { | |
| return false; | |
| } | |
| const meta = field.fieldMetadata as Record<string, unknown> | null | undefined; | |
| if (typeof meta !== "object" || meta === null) { | |
| return false; | |
| } | |
| if ( | |
| typeof meta.type !== "string" || | |
| typeof meta.purpose !== "string" || | |
| !Array.isArray(meta.labels) || | |
| !meta.labels.every((label: unknown) => typeof label === "string") || | |
| typeof meta.required !== "boolean" | |
| ) { | |
| return false; | |
| } | |
| if ( | |
| "placeholder" in meta && | |
| meta.placeholder !== undefined && | |
| typeof meta.placeholder !== "string" | |
| ) { | |
| return false; | |
| } | |
| return true; | |
| }) |
| if (!captureMemoryManager) { | ||
| const restored = await CaptureMemoryManager.restoreIfNeeded(ctx); | ||
| if (restored) { | ||
| captureMemoryManager = restored; | ||
| logger.info("Restored pending capture modal from storage"); | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a potential race condition when restoration is attempted before other services are initialized. If restoreIfNeeded() successfully restores the manager and calls show(), but then the subsequent initialization of fieldTracker, submissionMonitor, or captureService fails (line 186-189), the restored UI will remain visible but the capture functionality won't be fully operational. Consider handling this case by hiding the restored manager if initialization fails.
|
|
||
| await manager.show(ctx, pendingState.capturedFields, { | ||
| skipPersist: true, | ||
| }); |
There was a problem hiding this comment.
If manager.show() fails (line 487), the error is caught internally by show() which doesn't re-throw, and the manager is still returned on line 490. This means restoreIfNeeded() returns a manager instance that failed to display the UI. The caller in index.ts assigns this manager to captureMemoryManager, but it's not in a usable state. Consider checking manager.isVisible after calling show() and returning null if the UI failed to display, or have show() re-throw errors during restoration.
| }); | |
| }); | |
| if (!manager.isVisible) { | |
| logger.error( | |
| "CaptureMemoryManager.show did not display the UI during restoration; clearing pending state", | |
| ); | |
| try { | |
| await pendingCaptureStorage.setValue(null); | |
| } catch (cleanupError) { | |
| logger.debug("Error clearing corrupted state:", cleanupError); | |
| } | |
| return null; | |
| } |
| } | ||
|
|
||
| const currentUrl = window.location.href; | ||
| const pendingUrl = new URL(pendingState.siteUrl); |
There was a problem hiding this comment.
The URL construction on lines 457-458 can throw an error if pendingState.siteUrl is not a valid URL. While this is caught by the outer try-catch, it would be better to validate the URL before attempting to parse it, or handle the specific error case with a more informative log message. This would help distinguish between corrupted URL data versus other types of errors.
| const pendingUrl = new URL(pendingState.siteUrl); | |
| let pendingUrl: URL; | |
| try { | |
| pendingUrl = new URL(pendingState.siteUrl); | |
| } catch (parseError) { | |
| logger.warn("Invalid pending capture siteUrl, clearing", { | |
| siteUrl: pendingState.siteUrl, | |
| error: | |
| parseError instanceof Error ? parseError.message : String(parseError), | |
| }); | |
| await pendingCaptureStorage.setValue(null); | |
| return null; | |
| } |
| if (pendingUrl.hostname !== currentUrlObj.hostname) { | ||
| await pendingCaptureStorage.setValue(null); | ||
| logger.info("Cleared stale pending capture from different domain"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When restoring state, the code only checks if the hostname matches (line 460) but doesn't validate that the current page is suitable for showing the capture modal. If the user navigates from one page to another on the same domain where capture is disabled (e.g., due to settings changes or messaging sites), the modal will still be restored. Consider adding a check using shouldAutoCaptureForCurrentPage() before restoring the modal.
2 participants
No description provided.