fix: resolve 4 open bugs (#231, #232, #233, #234)

[bd73-com]

Summary

• Bug: Extension token origin validation skipped when sender has no tab URL #234 — Reject extension token messages when sender.tab?.url is undefined, closing the origin validation bypass
• Bug: Popup shows authenticated state with null userInfo after network error #233 — Cache userInfo in chrome.storage.local after successful verification; restore it on network error so tier/account display correctly offline; fall back to unauthenticated when no cache exists
• Bug: cancelCampaign returns premature pending count as cancelled count #232 — In cancelCampaign for active sends, atomically mark pending recipients as failed in the database instead of returning an optimistic stale pendingCount
• Bug: Missing lock_timeout in email.delivered webhook handler causes potential indefinite hang #231 — Add SET LOCAL lock_timeout = '5s' to the email.delivered webhook handler, consistent with email.opened, email.clicked, email.bounced, and email.complained

Test plan

• npm run check passes
• npm run test — all 1692 tests pass (updated 2 resendWebhook tests for new lock_timeout call)
• Manual: open extension popup offline after prior auth → should show cached account info or fall back to unauthenticated
• Manual: send FTC_EXTENSION_TOKEN from extension console without sender.tab → should get { ok: false, error: "no sender tab" }
• Manual: cancel an active campaign → response cancelled count should match DB rows actually marked as failed

Closes #231, closes #232, closes #233, closes #234

https://claude.ai/code/session_01Danxnk8GmCxffs8cm5saep

Summary by CodeRabbit

• Bug Fixes

  • Improved sender validation and error handling to fail gracefully on invalid or missing sender data.
  • Campaign cancellation now atomically updates real recipient state and returns actual cancellation counts.
  • Offline fallback for authentication using cached user info; cache cleared on disconnect.

• Performance & Reliability

  • Bounded lock behavior added to webhook processing to reduce locking contention.

• Tests

  • Added and updated tests covering campaign cancellation and webhook locking behavior.

• Chore

  • Extension version bumped.

[coderabbitai]

📝 Walkthrough

Walkthrough

Fixes four reliability/security issues: extension token origin validation bypass when sender tab is missing, popup showing authenticated state with null userInfo on network error, campaign cancellation returning stale pending counts, and missing transaction lock_timeout in webhook handlers to prevent indefinite DB locks.

Changes

Cohort / File(s)	Summary
Extension service worker extension/src/background/service-worker.ts	Rejects messages with missing sender tab (returns ok: false, error: "no sender tab"), wraps sender URL parsing in try/catch (responds invalid sender URL on failure), and ensures all error paths return false to avoid storing tokens without origin validation.
Extension popup extension/src/popup/popup.ts	Caches verified userInfo to local storage (cachedUserInfo) after successful verification. On network/verification errors, attempts to restore userInfo from cache to avoid null authenticated state; removes cachedUserInfo on disconnect.
Campaign cancellation (server) server/services/campaignEmail.ts, server/services/campaignEmail.test.ts	cancelCampaign now performs an atomic transactional UPDATE to mark remaining pending recipients failed, records failed_at/failure_reason, increments failed_count, and returns the actual number of rows affected (actualCancelled). Tests added/updated to cover active send cancellation and transactional behavior.
Resend webhook handlers & tests server/services/resendWebhook.ts, server/services/resendWebhook.test.ts	Adds SET LOCAL lock_timeout = '5s' at transaction start for webhook event handlers (delivered/opened/clicked/etc.) to bound lock waits. Tests updated to assert lock_timeout is executed and to reflect reduced tx calls for delivered events (lock only) versus other guarded events (lock + FOR UPDATE + counter update).
Extension manifest version bump extension/manifest.json, manifest.json	Manifest version increment from 1.0.0 → 1.0.1 (no functional changes beyond version).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• fix: resolve stale cancel count and webhook retry duplication #195 — Adjusts campaignEmail.ts cancel/update logic to return actual transactional cancelled counts; closely related to the cancelCampaign atomic update here.
• Fix campaign failed_count, dashboard rate cap, CTA text, and cleanup atomicity #184 — Introduces atomic UPDATE...RETURNING patterns in campaign recipient handling; relevant to new transactional cancel semantics and tests.
• fix: resolve TOCTOU race in Resend webhook handler #226 — Modifies resendWebhook.ts to add SET LOCAL lock_timeout behavior in webhook handlers; directly related to the lock_timeout additions and test changes here.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically identifies the PR's core purpose: fixing four open bugs numbered #231, #232, #233, #234, directly aligning with the changeset.
Linked Issues check	✅ Passed	All four linked issues are fully addressed: #234 adds sender tab validation guard [service-worker.ts]; #233 implements userInfo caching with offline fallback [popup.ts]; #232 atomically marks pending recipients as failed [campaignEmail.ts]; #231 adds lock_timeout to delivered webhook [resendWebhook.ts].
Out of Scope Changes check	✅ Passed	All changes are strictly scoped to the four linked issues; version bump in manifest.json is legitimate release maintenance accompanying extension fixes.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/review-github-issues-ZHzds

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@extension/src/popup/popup.ts`:
- Around line 96-98: The code assigns cached.cachedUserInfo directly to userInfo
after chrome.storage.local.get("cachedUserInfo"); add a defensive shape check
before assignment: implement a small type-guard (e.g., isUserInfo(obj)) that
verifies required properties of the expected UserInfo and only set userInfo =
cached.cachedUserInfo when isUserInfo returns true; otherwise ignore the cache
or clear it. Reference chrome.storage.local.get, cached.cachedUserInfo, and the
userInfo variable when locating where to add the check.

In `@server/services/resendWebhook.ts`:
- Around line 134-136: The transaction in resendWebhook sets a 5s lock_timeout
(see tx.execute(sql`SET LOCAL lock_timeout = '5s'`)) which can surface timeout
errors when cancelCampaign holds campaign_recipients locks; leave the timeout
as-is but add targeted handling: wrap the db.transaction call in resendWebhook
with a try/catch that detects Postgres lock timeout errors (e.g., SQLSTATE 55P03
or message "lock timeout") and increment a monitoring metric / emit a warning
log that includes context (webhook id, campaign id), so retries are observable
without changing cancelCampaign or the SET LOCAL lock_timeout behavior.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 64c0acc6-86ca-4dca-96c6-af2e5e844be6

📥 Commits

Reviewing files that changed from the base of the PR and between 814d309 and 706bbac.

📒 Files selected for processing (5)

• extension/src/background/service-worker.ts
• extension/src/popup/popup.ts
• server/services/campaignEmail.ts
• server/services/resendWebhook.test.ts
• server/services/resendWebhook.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Operational note: Lock contention with cancelCampaign is possible.

When cancelCampaign holds locks on campaign_recipients rows for longer than 5 seconds, this webhook handler will experience a lock timeout error. This is an acceptable tradeoff since:

1. The 5s timeout prevents indefinite hangs
2. Resend/Svix will retry failed webhook deliveries
3. The atomic guards ensure idempotent processing on retry

No action needed, but worth monitoring for elevated lock timeout errors during heavy cancellation activity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/services/resendWebhook.ts` around lines 134 - 136, The transaction in
resendWebhook sets a 5s lock_timeout (see tx.execute(sql`SET LOCAL lock_timeout
= '5s'`)) which can surface timeout errors when cancelCampaign holds
campaign_recipients locks; leave the timeout as-is but add targeted handling:
wrap the db.transaction call in resendWebhook with a try/catch that detects
Postgres lock timeout errors (e.g., SQLSTATE 55P03 or message "lock timeout")
and increment a monitoring metric / emit a warning log that includes context
(webhook id, campaign id), so retries are observable without changing
cancelCampaign or the SET LOCAL lock_timeout behavior.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

extension/src/popup/popup.ts (1)

207-214: 🧹 Nitpick | 🔵 Trivial

Consider atomicity of disconnect cleanup (minor).

The two await calls (clearToken() then chrome.storage.local.remove("cachedUserInfo")) are not atomic. If init() runs concurrently (e.g., from an FTC_AUTH_COMPLETE message event), it could briefly see token cleared but cachedUserInfo still present.

In practice, this is low risk because init() checks token validity first, and the fallback to cached userInfo only occurs on network errors — not on token absence. The worst case is a momentary UI flicker before landing on unauthenticated state.

🔧 Optional: Atomic cleanup

 document.getElementById("disconnect-btn")?.addEventListener("click", async () => {
-   await clearToken();
-   await chrome.storage.local.remove("cachedUserInfo");
+   await chrome.storage.local.remove(["ftc_extension_token", "ftc_token_expiry", "cachedUserInfo"]);
    userInfo = null;
    dropdownOpen = false;
    state = "unauthenticated";
    render();
 });

Note: This would require exporting the storage key constants from token.ts or duplicating them.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@extension/src/popup/popup.ts` around lines 207 - 214, The disconnect flow can
be non-atomic: clearToken() is awaited then
chrome.storage.local.remove("cachedUserInfo") is awaited separately, allowing
init() (e.g., triggered by FTC_AUTH_COMPLETE) to observe a cleared token but
still-present cachedUserInfo; make the cleanup atomic by removing both token and
cachedUserInfo in a single storage operation or by exporting and using the same
storage key constant from token.ts so you can call
chrome.storage.local.remove([...]) (or chrome.storage.local.set with both keys
cleared) in one awaited call; update the click handler that references
clearToken() and chrome.storage.local.remove("cachedUserInfo") to use the
combined removal approach and ensure userInfo = null, dropdownOpen = false,
state = "unauthenticated" and render() run after the atomic operation completes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@extension/src/popup/popup.ts`:
- Around line 207-214: The disconnect flow can be non-atomic: clearToken() is
awaited then chrome.storage.local.remove("cachedUserInfo") is awaited
separately, allowing init() (e.g., triggered by FTC_AUTH_COMPLETE) to observe a
cleared token but still-present cachedUserInfo; make the cleanup atomic by
removing both token and cachedUserInfo in a single storage operation or by
exporting and using the same storage key constant from token.ts so you can call
chrome.storage.local.remove([...]) (or chrome.storage.local.set with both keys
cleared) in one awaited call; update the click handler that references
clearToken() and chrome.storage.local.remove("cachedUserInfo") to use the
combined removal approach and ensure userInfo = null, dropdownOpen = false,
state = "unauthenticated" and render() run after the atomic operation completes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5146b126-0b3e-4f32-9876-6e85040a37aa

📥 Commits

Reviewing files that changed from the base of the PR and between 706bbac and 6e420b3.

⛔ Files ignored due to path filters (1)

• extension/fetchthechange-extension.zip is excluded by !**/*.zip

📒 Files selected for processing (5)

• extension/manifest.json
• extension/src/popup/popup.ts
• server/services/campaignEmail.test.ts
• server/services/campaignEmail.ts
• server/services/resendWebhook.test.ts

All findings addressed in commits aad7490-6e420b3.