Plus-Addressing Mode#4
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds plus‑addressing and catch‑all email modes, provider-domain detection/parsing utilities, history persistence, a popup UI, redesigned options under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Popup as Popup UI
participant Background as Background Service
participant Sync as chrome.storage.sync
participant Local as chrome.storage.local
participant Tab as Active Tab
User->>Popup: open popup
Popup->>Background: sendMessage { action: "generateAndFill" }
Background->>Sync: get { emailMode, baseEmail, emailDomain }
Background->>Tab: read active tab URL/title
Background->>Background: derive siteDomain
alt emailMode == "plusAddressing"
Background->>Background: extractLocalPart(baseEmail) → format local+site@domain
else
Background->>Background: format site@userDomain
end
Background->>Local: addEntry({ email, domain, pageUrl, pageTitle, mode })
Background-->>Popup: response GenerateAndFillResponse (success|needsConfig|error, email?, message?)
Popup->>User: render email / show config prompt / show error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds email-provider-aware configuration to the extension, including Chrome profile email detection and a new plus-addressing mode, with an updated options UI and docs.
Changes:
- Introduces provider domain detection (
plus-supported/plus-unsupported/custom) and plus-addressing mode alongside catch-all mode. - Overhauls the options page UI to support mode selection, provider feedback, and live example previews; adds Chrome profile email import via
chrome.identity. - Updates build/test tooling and bumps extension version to 2.0.0 (manifest + package).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| toolkit/scripts/build.js | Ensures new providers module is required/packaged in the dist verification list. |
| toolkit/bun/bunfig.toml | Centralizes Bun test preload config under toolkit. |
| src/types/index.ts | Adds shared EmailMode type for catch-all vs plus-addressing. |
| src/providers.ts | Adds provider domain lists + helpers (status + email parsing + domain regex). |
| src/options.ts | Implements new settings flow, Chrome profile email detection/import, mode availability logic, and previews. |
| src/options.test.ts | Adds tests for provider helpers and plus-addressing generation + storage keys. |
| src/options.html | Rebuilds the options UI (email input, mode table, provider messaging, examples). |
| src/background.ts | Generates emails based on selected mode/settings; updates notification copy. |
| src/background.test.ts | Expands tests to cover plus-addressing mode behavior. |
| package.json | Bumps version and updates test scripts to use Bun config file. |
| manifest.json | Bumps version and adds identity / identity.email permissions. |
| docs/README.md | Documents provider compatibility + updated test commands and structure. |
| docs/Email-Provider.md | New detailed provider/mode compatibility documentation. |
| bunfig.toml | Removes root-level Bun config in favor of toolkit config location. |
| .claude/CLAUDE.md | Updates contributor/testing instructions to match new test command/config location. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+109
to
+134
| @@ -118,11 +125,16 @@ async function generateEmailForTab(tab: chrome.tabs.Tab): Promise<string | null> | |||
|
|
|||
| try { | |||
| const url = new URL(tab.url); | |||
| // Extract only the main domain (without subdomains) | |||
| const domain = extractMainDomain(url.hostname); | |||
| const siteDomain = extractMainDomain(url.hostname); | |||
|
|
|||
| // Generate email | |||
| return `${domain}@${userDomain}`; | |||
| if (mode === 'plusAddressing') { | |||
| const atIndex = (baseEmail as string).lastIndexOf('@'); | |||
| const localPart = (baseEmail as string).substring(0, atIndex); | |||
| const emailDomain = (baseEmail as string).substring(atIndex + 1); | |||
| return `${localPart}+${siteDomain}@${emailDomain}`; | |||
There was a problem hiding this comment.
In plus-addressing mode, baseEmail.includes('@') is not sufficient validation: values like @domain.com or name@ pass the check but produce invalid generated emails (empty local part or domain), which then fail later in the content script. Consider trimming baseEmail and validating that the last @ is not at index 0 or the end (or reuse extractLocalPart/extractDomainFromEmail), returning null if invalid.
| function updateModeAvailability(): void { | ||
| const value = input.value.trim(); | ||
| const domain = extractDomainFromEmail(value); | ||
| const isFullEmail = value.includes('@') && domain != null; |
There was a problem hiding this comment.
updateModeAvailability() treats any input containing @ as a “full email”, even if the extracted domain fails domainRegex (e.g., user@localhost). This can enable Plus Addressing and show previews that can never be saved (since saveSettings() later rejects invalid domains). Consider incorporating domainRegex into isFullEmail / mode availability so the UI state matches what can be persisted.
Suggested change
| const isFullEmail = value.includes('@') && domain != null; | |
| const domainValidationRegex = new RegExp(domainRegex.source, domainRegex.flags.replace(/[gy]/g, '')); | |
| const hasValidEmailDomain = domain != null && domainValidationRegex.test(domain); | |
| const isFullEmail = value.includes('@') && hasValidEmailDomain; |
Comment on lines
+416
to
+448
| <div class="mode-column" id="colPlusAddressing" data-mode="plusAddressing"> | ||
| <div class="mode-header">Plus Addressing</div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Format</div> | ||
| <div class="row-value"><code id="plusFormat">name+site@gmail.com</code></div> | ||
| </div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Requires</div> | ||
| <div class="row-value">Email provider with plus-address support</div> | ||
| </div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Catch-All</div> | ||
| <div class="row-value">Not needed</div> | ||
| </div> | ||
| </div> | ||
| <div class="example-item"> | ||
| When you visit <strong>github.com</strong>, the extension will generate: | ||
| <span class="example-email" id="exampleEmail2">github.com@yourdomain.com</span> | ||
| <div class="mode-column" id="colCatchAll" data-mode="catchAll"> | ||
| <div class="mode-header">Catch-All Prefix</div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Format</div> | ||
| <div class="row-value"><code id="catchAllFormat">site@yourdomain.com</code></div> | ||
| </div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Requires</div> | ||
| <div class="row-value">Own domain with email hosting</div> | ||
| </div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Catch-All</div> | ||
| <div class="row-value">Required</div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <input type="radio" name="emailMode" value="plusAddressing" id="modePlusAddressing" class="sr-only"> | ||
| <input type="radio" name="emailMode" value="catchAll" id="modeCatchAll" class="sr-only"> |
There was a problem hiding this comment.
The mode selector columns are clickable <div>s with no keyboard focus/activation support (no tabindex, role, aria-checked, or key handlers), and the actual radio inputs are visually hidden without associated labels. This makes mode selection largely inaccessible to keyboard/screen-reader users. Consider using <label> elements bound to the radios (or buttons with proper ARIA roles) and ensuring the selected/disabled state is conveyed via ARIA.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/options.html (1)
447-448: Hidden radio inputs lack associated labels for screen readers.The visually-hidden radio inputs would benefit from proper
<label>associations for assistive technology users.♿ Optional: Add labels for screen readers
- <input type="radio" name="emailMode" value="plusAddressing" id="modePlusAddressing" class="sr-only"> - <input type="radio" name="emailMode" value="catchAll" id="modeCatchAll" class="sr-only"> + <input type="radio" name="emailMode" value="plusAddressing" id="modePlusAddressing" class="sr-only"> + <label for="modePlusAddressing" class="sr-only">Plus Addressing mode</label> + <input type="radio" name="emailMode" value="catchAll" id="modeCatchAll" class="sr-only"> + <label for="modeCatchAll" class="sr-only">Catch-All Prefix mode</label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/options.html` around lines 447 - 448, The two hidden radio inputs (ids modePlusAddressing and modeCatchAll) lack accessible labels; add associated <label> elements (using for="modePlusAddressing" and for="modeCatchAll") that convey the option text and use the same visually-hidden/sr-only styling so screen readers can announce them, or alternatively add clear aria-label attributes matching those option names to the inputs if you prefer not to add label elements; ensure the label text reflects the option (e.g., "Plus addressing" and "Catch-all") so assistive tech can identify each choice.src/background.ts (1)
130-135: Consider extracting validatedbaseEmailto avoid repeated assertions.The type assertions are safe due to the guard at line 111, but the repetition could be cleaner.
♻️ Optional: Extract to local variable
if (mode === 'plusAddressing') { - const atIndex = (baseEmail as string).lastIndexOf('@'); - const localPart = (baseEmail as string).substring(0, atIndex); - const emailDomain = (baseEmail as string).substring(atIndex + 1); + const validatedEmail = baseEmail!; // Safe: validated at line 111 + const atIndex = validatedEmail.lastIndexOf('@'); + const localPart = validatedEmail.substring(0, atIndex); + const emailDomain = validatedEmail.substring(atIndex + 1); return `${localPart}+${siteDomain}@${emailDomain}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/background.ts` around lines 130 - 135, The code repeatedly asserts baseEmail as string inside the plusAddressing branch; extract a validated local variable (e.g., const validatedBaseEmail = baseEmail as string) before using it so you stop repeating casts and clearly indicate the value is already type-guarded; update uses in the plusAddressing logic (atIndex, localPart, emailDomain, and the returned template) to reference validatedBaseEmail instead of repeatedly asserting baseEmail.src/options.ts (1)
250-271: Stale storage keys persist when switching modes.When saving plus-addressing mode, only
emailModeandbaseEmailare set. When saving catch-all mode, onlyemailModeandemailDomainare set. The other mode's key remains in storage from previous saves.While this doesn't affect functionality (background.ts only reads the active mode's key), it leaves stale data in storage.
♻️ Clear inactive mode's key when saving
try { - await chrome.storage.sync.set({ emailMode: 'plusAddressing', baseEmail: value }); + await chrome.storage.sync.set({ emailMode: 'plusAddressing', baseEmail: value, emailDomain: undefined }); showStatus('Settings saved successfully!', 'success');Or use
removeafterset:try { await chrome.storage.sync.set({ emailMode: 'plusAddressing', baseEmail: value }); + await chrome.storage.sync.remove(['emailDomain']); showStatus('Settings saved successfully!', 'success');Apply similar change for the catchAll path to remove
baseEmail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/options.ts` around lines 250 - 271, When saving settings in src/options.ts, ensure stale storage keys are removed: after setting { emailMode: 'plusAddressing', baseEmail: value } remove the catch-all key 'emailDomain'; likewise after setting { emailMode: 'catchAll', emailDomain: cleanDomain } remove the plus-addressing key 'baseEmail'. Update the save handlers that call chrome.storage.sync.set (the branches that currently use emailMode/baseEmail and emailMode/emailDomain) to call chrome.storage.sync.remove for the inactive key (or perform a single chrome.storage.sync.set that omits the inactive key and then chrome.storage.sync.remove) and keep existing validation via domainRegex and extractDomainFromEmail and status reporting via showStatus.
🤖 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/options.html`:
- Around line 415-449: Make the clickable mode columns (elements with ids
colPlusAddressing and colCatchAll) keyboard-focusable and operable: add
tabindex="0" (and appropriate ARIA attributes if desired) to those divs in the
HTML so they can receive focus, then in src/options.ts attach keydown handlers
to the corresponding DOM nodes that detect Enter or Space, call
setMode('plusAddressing') or setMode('catchAll') respectively, and call
e.preventDefault() to avoid duplicate activation; ensure the handlers reference
the radio inputs (modePlusAddressing, modeCatchAll) or update their checked
state if needed so the UI stays in sync.
---
Nitpick comments:
In `@src/background.ts`:
- Around line 130-135: The code repeatedly asserts baseEmail as string inside
the plusAddressing branch; extract a validated local variable (e.g., const
validatedBaseEmail = baseEmail as string) before using it so you stop repeating
casts and clearly indicate the value is already type-guarded; update uses in the
plusAddressing logic (atIndex, localPart, emailDomain, and the returned
template) to reference validatedBaseEmail instead of repeatedly asserting
baseEmail.
In `@src/options.html`:
- Around line 447-448: The two hidden radio inputs (ids modePlusAddressing and
modeCatchAll) lack accessible labels; add associated <label> elements (using
for="modePlusAddressing" and for="modeCatchAll") that convey the option text and
use the same visually-hidden/sr-only styling so screen readers can announce
them, or alternatively add clear aria-label attributes matching those option
names to the inputs if you prefer not to add label elements; ensure the label
text reflects the option (e.g., "Plus addressing" and "Catch-all") so assistive
tech can identify each choice.
In `@src/options.ts`:
- Around line 250-271: When saving settings in src/options.ts, ensure stale
storage keys are removed: after setting { emailMode: 'plusAddressing',
baseEmail: value } remove the catch-all key 'emailDomain'; likewise after
setting { emailMode: 'catchAll', emailDomain: cleanDomain } remove the
plus-addressing key 'baseEmail'. Update the save handlers that call
chrome.storage.sync.set (the branches that currently use emailMode/baseEmail and
emailMode/emailDomain) to call chrome.storage.sync.remove for the inactive key
(or perform a single chrome.storage.sync.set that omits the inactive key and
then chrome.storage.sync.remove) and keep existing validation via domainRegex
and extractDomainFromEmail and status reporting via showStatus.
🪄 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: cee91896-788b-4198-ae3e-187d8e62551f
📒 Files selected for processing (15)
.claude/CLAUDE.mdbunfig.tomldocs/Email-Provider.mddocs/README.mdmanifest.jsonpackage.jsonsrc/background.test.tssrc/background.tssrc/options.htmlsrc/options.test.tssrc/options.tssrc/providers.tssrc/types/index.tstoolkit/bun/bunfig.tomltoolkit/scripts/build.js
💤 Files with no reviewable changes (1)
- bunfig.toml
Comment on lines
+415
to
+449
| <div class="mode-table"> | ||
| <div class="mode-column" id="colPlusAddressing" data-mode="plusAddressing"> | ||
| <div class="mode-header">Plus Addressing</div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Format</div> | ||
| <div class="row-value"><code id="plusFormat">name+site@gmail.com</code></div> | ||
| </div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Requires</div> | ||
| <div class="row-value">Email provider with plus-address support</div> | ||
| </div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Catch-All</div> | ||
| <div class="row-value">Not needed</div> | ||
| </div> | ||
| </div> | ||
| <div class="example-item"> | ||
| When you visit <strong>github.com</strong>, the extension will generate: | ||
| <span class="example-email" id="exampleEmail2">github.com@yourdomain.com</span> | ||
| <div class="mode-column" id="colCatchAll" data-mode="catchAll"> | ||
| <div class="mode-header">Catch-All Prefix</div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Format</div> | ||
| <div class="row-value"><code id="catchAllFormat">site@yourdomain.com</code></div> | ||
| </div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Requires</div> | ||
| <div class="row-value">Own domain with email hosting</div> | ||
| </div> | ||
| <div class="mode-row"> | ||
| <div class="row-label">Catch-All</div> | ||
| <div class="row-value">Required</div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <input type="radio" name="emailMode" value="plusAddressing" id="modePlusAddressing" class="sr-only"> | ||
| <input type="radio" name="emailMode" value="catchAll" id="modeCatchAll" class="sr-only"> | ||
| </section> |
There was a problem hiding this comment.
Mode selection columns lack keyboard accessibility.
The mode columns are clickable <div> elements, but they're not keyboard-focusable or operable. Users relying on keyboard navigation cannot select modes without clicking.
♿ Proposed fix: Add keyboard support to mode columns
- <div class="mode-column" id="colPlusAddressing" data-mode="plusAddressing">
+ <div class="mode-column" id="colPlusAddressing" data-mode="plusAddressing" tabindex="0" role="button" aria-pressed="false">
<div class="mode-header">Plus Addressing</div>
...
</div>
- <div class="mode-column" id="colCatchAll" data-mode="catchAll">
+ <div class="mode-column" id="colCatchAll" data-mode="catchAll" tabindex="0" role="button" aria-pressed="false">
<div class="mode-header">Catch-All Prefix</div>
...
</div>Then in src/options.ts, add keydown handlers:
colPlus.addEventListener('keydown', (e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
setMode('plusAddressing');
}
});
colCatch.addEventListener('keydown', (e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
setMode('catchAll');
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/options.html` around lines 415 - 449, Make the clickable mode columns
(elements with ids colPlusAddressing and colCatchAll) keyboard-focusable and
operable: add tabindex="0" (and appropriate ARIA attributes if desired) to those
divs in the HTML so they can receive focus, then in src/options.ts attach
keydown handlers to the corresponding DOM nodes that detect Enter or Space, call
setMode('plusAddressing') or setMode('catchAll') respectively, and call
e.preventDefault() to avoid duplicate activation; ensure the handlers reference
the radio inputs (modePlusAddressing, modeCatchAll) or update their checked
state if needed so the UI stays in sync.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/providers.ts (1)
14-28: Consider reusing email parsing helpers from providers.ts in background.ts to avoid duplication.The background.ts file (lines 131-134) contains duplicate logic for extracting the local part and domain from an email:
const atIndex = (baseEmail as string).lastIndexOf('@'); const localPart = (baseEmail as string).substring(0, atIndex); const emailDomain = (baseEmail as string).substring(atIndex + 1);This duplication lacks the null/boundary checks present in the well-implemented helpers
extractLocalPart()andextractDomainFromEmail()from providers.ts. Refactoring background.ts to import and use these helpers would ensure consistent validation across the codebase. Similar duplication also exists in options.test.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers.ts` around lines 14 - 28, Background.ts and options.test.ts duplicate ad-hoc email parsing that misses the null/boundary checks implemented in providers.ts; replace that manual parsing with imports and calls to extractLocalPart(email) and extractDomainFromEmail(email) so you reuse the validators (extractLocalPart and extractDomainFromEmail) and handle null returns where appropriate, updating any callers to check for null or bail out/throw as the original helpers would.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/Email-Provider.md`:
- Around line 107-114: The docs incorrectly state that the domain lists are
defined in providers.ts; update the text to say the sets PLUS_SUPPORTED_DOMAINS
and PLUS_UNSUPPORTED_DOMAINS are defined in the provider-domains module
(providers.ts only re-exports them), so change the reference from providers.ts
to provider-domains and/or mention that providers.ts re-exports those sets for
clarity.
In `@src/providers.ts`:
- Around line 7-12: The runtime currently ignores provider support when creating
plus-addressed emails; update generateEmailForTab to call
getProviderStatus(domain) (using the existing PLUS_SUPPORTED_DOMAINS /
PLUS_UNSUPPORTED_DOMAINS logic) and refuse or fail gracefully when status ===
'plus-unsupported' (e.g., throw/return a clear error message or fallback to a
supported mode) so that background generation matches the UI warnings and forced
UI behavior; ensure the error message identifies the domain and that
generateEmailForTab documents/propagates the failure to callers.
---
Nitpick comments:
In `@src/providers.ts`:
- Around line 14-28: Background.ts and options.test.ts duplicate ad-hoc email
parsing that misses the null/boundary checks implemented in providers.ts;
replace that manual parsing with imports and calls to extractLocalPart(email)
and extractDomainFromEmail(email) so you reuse the validators (extractLocalPart
and extractDomainFromEmail) and handle null returns where appropriate, updating
any callers to check for null or bail out/throw as the original helpers would.
🪄 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: 3ae60815-3264-49f8-86e5-a9ba5a9deeef
📒 Files selected for processing (6)
docs/Email-Provider.mddocs/README.mdsrc/options.test.tssrc/provider-domains.tssrc/providers.tstoolkit/scripts/build.js
✅ Files skipped from review due to trivial changes (2)
- src/options.test.ts
- docs/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- toolkit/scripts/build.js
Comment on lines
+7
to
+12
| export function getProviderStatus(domain: string): ProviderStatus { | ||
| const lower = domain.toLowerCase(); | ||
| if (PLUS_SUPPORTED_DOMAINS.has(lower)) return 'plus-supported'; | ||
| if (PLUS_UNSUPPORTED_DOMAINS.has(lower)) return 'plus-unsupported'; | ||
| return 'custom'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if background.ts uses getProviderStatus
rg -n "getProviderStatus" --type tsRepository: ZAAI-com/Clean-Autofill
Length of output: 1094
🏁 Script executed:
#!/bin/bash
# Find background.ts and related email generation files
fd -e ts -e js | grep -E "(background|content|email|generate)" | head -20Repository: ZAAI-com/Clean-Autofill
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Look for generateEmailForTab function
rg -n "generateEmailForTab" --type tsRepository: ZAAI-com/Clean-Autofill
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Search for plus-addressing or email generation patterns
rg -n "plus-addressed|plus-address|\+\+" --type ts | head -30Repository: ZAAI-com/Clean-Autofill
Length of output: 798
🏁 Script executed:
#!/bin/bash
# Read the generateEmailForTab function implementation
sed -n '94,150p' src/background.tsRepository: ZAAI-com/Clean-Autofill
Length of output: 2100
🏁 Script executed:
#!/bin/bash
# Check the full background.ts to understand the flow
wc -l src/background.ts && head -50 src/background.tsRepository: ZAAI-com/Clean-Autofill
Length of output: 1571
🏁 Script executed:
#!/bin/bash
# Look at the test cases for generateEmailForTab
rg -B 5 -A 15 "generates plus-addressed email for simple domain" src/background.test.tsRepository: ZAAI-com/Clean-Autofill
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Read the options.ts usage of getProviderStatus
sed -n '120,160p' src/options.tsRepository: ZAAI-com/Clean-Autofill
Length of output: 1408
🏁 Script executed:
#!/bin/bash
# Look for mode selection and validation in options.ts
rg -B 10 -A 10 "getProviderStatus\(domain" src/options.tsRepository: ZAAI-com/Clean-Autofill
Length of output: 741
🏁 Script executed:
#!/bin/bash
# Check if there's any UI logic that blocks plus-addressing for unsupported providers
rg -n "plus-unsupported|plus-supported" src/options.tsRepository: ZAAI-com/Clean-Autofill
Length of output: 237
Runtime email generation doesn't validate provider support despite UI warnings.
The getProviderStatus() function shows warnings in the options UI for unsupported providers (Yahoo, iCloud, etc.) and even forces plus-addressing mode selection for known providers. However, background.ts::generateEmailForTab() generates plus-addressed emails without consulting provider support status. This creates a gap where:
- UI shows a warning and forces plus-addressing for unsupported providers
- Runtime still generates emails knowing they won't work
Consider either:
- Blocking plus-addressing mode for unsupported providers in the UI, or
- Having
generateEmailForTab()check provider status and fail gracefully with a clear error message
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/providers.ts` around lines 7 - 12, The runtime currently ignores provider
support when creating plus-addressed emails; update generateEmailForTab to call
getProviderStatus(domain) (using the existing PLUS_SUPPORTED_DOMAINS /
PLUS_UNSUPPORTED_DOMAINS logic) and refuse or fail gracefully when status ===
'plus-unsupported' (e.g., throw/return a clear error message or fallback to a
supported mode) so that background generation matches the UI warnings and forced
UI behavior; ensure the error message identifies the domain and that
generateEmailForTab documents/propagates the failure to callers.
manuelgruber
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/ui/options.test.ts`:
- Around line 11-14: Tests currently import providers.js and reimplement helper
logic instead of exercising the real options page logic in src/ui/options.ts
(lines ~197-370); update the tests to import src/ui/options.ts so
loadSettings/saveSettings, provider-mode gating, and DOM event wiring are
executed. Concretely: replace the mirrored helper usage with an import of the
options module, set up the same DOM fixtures the module expects, then drive
behavior by simulating user interactions (e.g., filling inputs, changing
provider selection, submitting the form) and assert outcomes; additionally
spy/mocking provider functions from providers.js to verify provider-dependent
gating and persistence instead of duplicating their logic in the test.
- Around line 452-458: The test 'handles API error gracefully' is missing
await/return for the rejection matcher, so the test can finish before the
assertion runs; update the test that stubs
mockChrome.identity.getProfileUserInfo to either await the assertion or return
the Promise from expect(mockChrome.identity.getProfileUserInfo({ accountStatus:
'ANY' })).rejects.toThrow('API unavailable') so the rejects.toThrow check is
executed (reference mockChrome.identity.getProfileUserInfo and the test name to
locate the code).
In `@src/ui/options.ts`:
- Around line 120-130: When a user enters a bare domain (isFullEmail false) we
must reject known mailbox providers (e.g., gmail.com, yahoo.com, outlook.com,
hotmail.com, icloud.com, protonmail.com) so Catch-All isn't offered; add a check
against a small set/array (knownMailboxProviders) of known public mailbox
domains in the same branches that handle domain-only input (the block using
isFullEmail, setColumnState, colCatch, catchAllFeedbackEl, getMode, setMode and
the other similar branch around lines 259-268). If the domain matches the
blacklist, call setColumnState(colCatch, catchAllFeedbackEl, 'disabled',
'Catch-All not available for public mailbox providers') and if getMode() ===
'catchAll' switch mode away (e.g., setMode('plusAddressing') or another safe
default); otherwise keep the existing behavior enabling Catch-All. Ensure both
domain-only code paths use this new guard.
- Around line 359-366: The click handlers currently call
setMode('plusAddressing') and setMode('catchAll') but the radio inputs can
change directly and won’t trigger those, causing UI/preview drift; add listeners
to the mode radio group (e.g., select the radio inputs by name or IDs) and on
their 'change' events call setMode with the newly selected value so selected
styling and preview text stay in sync with the actual radio state (use the
existing setMode function and the colPlus/colCatch handlers as the authoritative
mode values).
In `@src/ui/popup.test.ts`:
- Around line 79-214: The tests never import or run src/ui/popup.ts, so
initialize the real module after seeding the test DOM and then drive events
through its actual listeners: set document.body.innerHTML to the popup HTML used
by getElements(), then import/require 'src/ui/popup.ts' so its top-level
initialization (the sendMessage call, copyButton and configLink event wiring)
runs; ensure mockChrome.runtime.sendMessage is implemented to invoke the
provided callback with a test GenerateAndFillResponse (so the module's
sendMessage callback executes), and simulate user actions by calling
document.querySelector for the copy button and config link and then dispatching
click events to verify clipboard.writeText and chrome.runtime.openOptionsPage
are invoked via the real popup listeners (refer to the module's top-level init
code, copyButton and configLink DOM IDs/selectors).
🪄 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: c73dbc99-8c81-4b03-88da-e2e764ad8493
📒 Files selected for processing (12)
.github/workflows/build-and-test.ymlmanifest.jsonsrc/background.tssrc/types/index.tssrc/ui/options.htmlsrc/ui/options.test.tssrc/ui/options.tssrc/ui/popup.htmlsrc/ui/popup.test.tssrc/ui/popup.tstoolkit/scripts/build.jstoolkit/scripts/validate.js
✅ Files skipped from review due to trivial changes (3)
- src/ui/popup.html
- src/ui/options.html
- src/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- toolkit/scripts/build.js
- manifest.json
Comment on lines
+359
to
+366
| // Event listeners | ||
| formEl.addEventListener('submit', saveSettings); | ||
| clearBtn.addEventListener('click', clearSettings); | ||
| importBtn.addEventListener('click', importFromChrome); | ||
| input.addEventListener('input', debouncedUpdate); | ||
|
|
||
| colPlus.addEventListener('click', () => setMode('plusAddressing')); | ||
| colCatch.addEventListener('click', () => setMode('catchAll')); |
There was a problem hiding this comment.
Listen to the radio change events as well.
Only the column clicks call setMode(). If the radio group changes without going through those container clicks, the selected styling and preview text drift out of sync.
🔧 Minimal fix
formEl.addEventListener('submit', saveSettings);
clearBtn.addEventListener('click', clearSettings);
importBtn.addEventListener('click', importFromChrome);
input.addEventListener('input', debouncedUpdate);
+ radioPlus.addEventListener('change', () => setMode('plusAddressing'));
+ radioCatch.addEventListener('change', () => setMode('catchAll'));
colPlus.addEventListener('click', () => setMode('plusAddressing'));
colCatch.addEventListener('click', () => setMode('catchAll'));📝 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.
Suggested change
| // Event listeners | |
| formEl.addEventListener('submit', saveSettings); | |
| clearBtn.addEventListener('click', clearSettings); | |
| importBtn.addEventListener('click', importFromChrome); | |
| input.addEventListener('input', debouncedUpdate); | |
| colPlus.addEventListener('click', () => setMode('plusAddressing')); | |
| colCatch.addEventListener('click', () => setMode('catchAll')); | |
| // Event listeners | |
| formEl.addEventListener('submit', saveSettings); | |
| clearBtn.addEventListener('click', clearSettings); | |
| importBtn.addEventListener('click', importFromChrome); | |
| input.addEventListener('input', debouncedUpdate); | |
| radioPlus.addEventListener('change', () => setMode('plusAddressing')); | |
| radioCatch.addEventListener('change', () => setMode('catchAll')); | |
| colPlus.addEventListener('click', () => setMode('plusAddressing')); | |
| colCatch.addEventListener('click', () => setMode('catchAll')); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/options.ts` around lines 359 - 366, The click handlers currently call
setMode('plusAddressing') and setMode('catchAll') but the radio inputs can
change directly and won’t trigger those, causing UI/preview drift; add listeners
to the mode radio group (e.g., select the radio inputs by name or IDs) and on
their 'change' events call setMode with the newly selected value so selected
styling and preview text stay in sync with the actual radio state (use the
existing setMode function and the colPlus/colCatch handlers as the authoritative
mode values).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/ui/options.test.ts (1)
452-459:⚠️ Potential issue | 🟡 MinorAwait the rejection matcher to enforce the assertion.
The test is synchronous but
expect(...).rejects.toThrow()returns a Promise. Withoutawait, the test completes before the assertion runs, making it a false positive.Suggested fix
- test('handles API error gracefully', () => { + test('handles API error gracefully', async () => { mockChrome.identity.getProfileUserInfo = mock(async () => { throw new Error('API unavailable'); }); - expect(mockChrome.identity.getProfileUserInfo({ accountStatus: 'ANY' })).rejects.toThrow( + await expect(mockChrome.identity.getProfileUserInfo({ accountStatus: 'ANY' })).rejects.toThrow( 'API unavailable', ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/options.test.ts` around lines 452 - 459, The test 'handles API error gracefully' currently calls expect(...).rejects.toThrow() without awaiting it, so the test may pass before the rejection assertion runs; update the test to either await the rejection assertion (await expect(mockChrome.identity.getProfileUserInfo({ accountStatus: 'ANY' })).rejects.toThrow('API unavailable')) or return that promise from the test so the test runner waits for the assertion, referencing the mocked function mockChrome.identity.getProfileUserInfo and the test name to locate the change.src/ui/options.ts (3)
385-392:⚠️ Potential issue | 🟡 MinorAdd
changeevent listeners to the radio inputs.The radio buttons can be changed via keyboard navigation without triggering the column click handlers. This causes the selected styling and preview text to drift out of sync with the actual radio state.
Suggested fix
// Settings event listeners formEl.addEventListener('submit', saveSettings); clearBtn.addEventListener('click', clearSettings); importBtn.addEventListener('click', importFromChrome); input.addEventListener('input', debouncedUpdate); + radioPlus.addEventListener('change', () => setMode('plusAddressing')); + radioCatch.addEventListener('change', () => setMode('catchAll')); colPlus.addEventListener('click', () => setMode('plusAddressing')); colCatch.addEventListener('click', () => setMode('catchAll'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/options.ts` around lines 385 - 392, The radio inputs for column mode aren't listened to for 'change' so keyboard selection can desync the UI; add 'change' event listeners on the actual radio input elements (e.g., the plus/catch-all radio inputs used by the colPlus/colCatch buttons) to call setMode('plusAddressing') or setMode('catchAll') and any UI/preview update logic so the selected styling and preview text always reflect the real radio state; locate where colPlus/colCatch and setMode are defined and hook the corresponding radio input elements' 'change' events to the same handler.
285-306:⚠️ Potential issue | 🟠 MajorCatch-All save path should reject known mailbox providers.
Similar to the UI availability check, the save handler should prevent persisting known provider domains (e.g.,
gmail.com,yahoo.com) as catch-all domains since they won't work.Suggested fix
if (!domainRegex.test(cleanDomain)) { showStatus('Please enter a valid domain (e.g., yourdomain.com)', 'error'); return; } + if (getProviderStatus(cleanDomain) !== 'custom') { + showStatus('Catch-All only works with a custom domain you control', 'error'); + return; + } + try { await chrome.storage.sync.set({ emailMode: 'catchAll', emailDomain: cleanDomain });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/options.ts` around lines 285 - 306, When saving a catch-all domain in the else branch, add a check to reject known mailbox providers (e.g., gmail.com, yahoo.com) before validating with domainRegex: use the same provider detection logic the UI availability check uses (or add a small knownProviders list / isKnownProvider(cleanDomain) helper) and if it matches call showStatus('Please enter a non-provider domain (no gmail/yahoo/etc.)', 'error') and return; only proceed to chrome.storage.sync.set, set input.value and show success if the domain is not a known provider and passes domainRegex.
146-157:⚠️ Potential issue | 🟠 MajorBare domains still enable Catch-All for known mailbox providers.
When the user enters just a domain (e.g.,
gmail.com) without the@, the code enables Catch-All mode (line 154) without checking if it's a known provider. This allows savinggmail.comas a catch-all domain, which will generate unusable addresses likesite@gmail.com.Suggested fix
if (!isFullEmail) { + // Check if bare domain is a known provider (catch-all won't work) + const cleanDomain = value.replace(/^@/, '').toLowerCase(); + if (domainRegex.test(cleanDomain) && getProviderStatus(cleanDomain) !== 'custom') { + setColumnState( + colPlus, + plusFeedbackEl, + 'disabled', + 'Enter your full email to use Plus Addressing', + ); + setColumnState(colCatch, catchAllFeedbackEl, 'disabled', `Not available for ${cleanDomain}`); + return; + } + // Just a domain entered — only catch-all works setColumnState(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/options.ts` around lines 146 - 157, The current branch that handles !isFullEmail enables Catch-All unconditionally; update the logic in the block that checks isFullEmail to first detect known mailbox providers (use or add a helper like isKnownMailboxProvider(domain) or consult the existing providers list) and if the domain is a known provider (e.g., gmail.com) disable the Catch-All column via setColumnState(colCatch, catchAllFeedbackEl, 'disabled', '<brief message>') and do not call setMode('catchAll'); only enable Catch-All (setColumnState(..., 'available', '')) and switch mode to catchAll when the domain is not a known provider. Ensure you reference isFullEmail, setColumnState, getMode, and setMode when making the change.
🧹 Nitpick comments (2)
src/ui/options.ts (2)
439-441:escapeAttris incomplete for full HTML attribute escaping.The function only escapes
&and", but<,>, and'can also cause issues in certain attribute contexts. While the data originates from extension storage (low XSS risk), consider using a more complete escaping function for defense in depth.More complete implementation
function escapeAttr(str: string): string { - return str.replace(/&/g, '&').replace(/"/g, '"'); + return str + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(/'/g, ''') + .replace(/</g, '<') + .replace(/>/g, '>'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/options.ts` around lines 439 - 441, escapeAttr currently only replaces & and " which leaves <, > and ' unescaped; update the escapeAttr function to also replace '<' -> '<', '>' -> '>?' and '\'' -> ''' (in addition to '&' -> '&' and '"' -> '"') using a single global replacement (or a small map + regex) so all five characters are properly escaped before inserting into HTML attributes; keep the function name escapeAttr and ensure the replacement logic returns the escaped string.
443-456: Consider usinggetHistoryfromsrc/history.tsinstead of duplicating filter logic.The
loadHistoryfunction reimplements the search filtering already available ingetHistory({ search }). This duplication could lead to inconsistencies if the filtering logic changes.Suggested refactor
+import { getHistory } from '../history.js'; + async function loadHistory(): Promise<void> { - const { emailHistory = [] } = await chrome.storage.local.get('emailHistory'); - let entries = emailHistory as EmailHistoryEntry[]; - const searchTerm = historySearch.value.trim().toLowerCase(); - if (searchTerm) { - entries = entries.filter( - (e) => - e.domain.toLowerCase().includes(searchTerm) || e.email.toLowerCase().includes(searchTerm), - ); - } - + const entries = await getHistory(searchTerm ? { search: searchTerm } : undefined); renderHistory(entries); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/options.ts` around lines 443 - 456, The loadHistory function duplicates filtering logic; replace its direct chrome.storage access and manual filter with a call to getHistory({ search }) using historySearch.value.trim() (or empty string) to fetch the filtered entries, then pass the result to renderHistory; update references to use getHistory (from src/history.ts) instead of emailHistory cast, while preserving the call to renderHistory(entries).
🤖 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/ui/options.test.ts`:
- Around line 535-544: The test expects 'mail.ru', 'inbox.ru', and 'bk.ru' to be
plus-unsupported but they are currently present in PLUS_SUPPORTED_DOMAINS;
remove those three domains from the PLUS_SUPPORTED_DOMAINS constant in
provider-domains.ts so getProviderStatus(domain) returns 'plus-unsupported' for
them (alternatively, if they truly support plus addressing, update the
unsupported array in the test to the supported array instead—use the symbols
PLUS_SUPPORTED_DOMAINS and getProviderStatus to locate the related logic and
adjust the domain membership accordingly).
---
Duplicate comments:
In `@src/ui/options.test.ts`:
- Around line 452-459: The test 'handles API error gracefully' currently calls
expect(...).rejects.toThrow() without awaiting it, so the test may pass before
the rejection assertion runs; update the test to either await the rejection
assertion (await expect(mockChrome.identity.getProfileUserInfo({ accountStatus:
'ANY' })).rejects.toThrow('API unavailable')) or return that promise from the
test so the test runner waits for the assertion, referencing the mocked function
mockChrome.identity.getProfileUserInfo and the test name to locate the change.
In `@src/ui/options.ts`:
- Around line 385-392: The radio inputs for column mode aren't listened to for
'change' so keyboard selection can desync the UI; add 'change' event listeners
on the actual radio input elements (e.g., the plus/catch-all radio inputs used
by the colPlus/colCatch buttons) to call setMode('plusAddressing') or
setMode('catchAll') and any UI/preview update logic so the selected styling and
preview text always reflect the real radio state; locate where colPlus/colCatch
and setMode are defined and hook the corresponding radio input elements'
'change' events to the same handler.
- Around line 285-306: When saving a catch-all domain in the else branch, add a
check to reject known mailbox providers (e.g., gmail.com, yahoo.com) before
validating with domainRegex: use the same provider detection logic the UI
availability check uses (or add a small knownProviders list /
isKnownProvider(cleanDomain) helper) and if it matches call showStatus('Please
enter a non-provider domain (no gmail/yahoo/etc.)', 'error') and return; only
proceed to chrome.storage.sync.set, set input.value and show success if the
domain is not a known provider and passes domainRegex.
- Around line 146-157: The current branch that handles !isFullEmail enables
Catch-All unconditionally; update the logic in the block that checks isFullEmail
to first detect known mailbox providers (use or add a helper like
isKnownMailboxProvider(domain) or consult the existing providers list) and if
the domain is a known provider (e.g., gmail.com) disable the Catch-All column
via setColumnState(colCatch, catchAllFeedbackEl, 'disabled', '<brief message>')
and do not call setMode('catchAll'); only enable Catch-All (setColumnState(...,
'available', '')) and switch mode to catchAll when the domain is not a known
provider. Ensure you reference isFullEmail, setColumnState, getMode, and setMode
when making the change.
---
Nitpick comments:
In `@src/ui/options.ts`:
- Around line 439-441: escapeAttr currently only replaces & and " which leaves
<, > and ' unescaped; update the escapeAttr function to also replace '<' ->
'<', '>' -> '>?' and '\'' -> ''' (in addition to '&' -> '&' and '"'
-> '"') using a single global replacement (or a small map + regex) so all
five characters are properly escaped before inserting into HTML attributes; keep
the function name escapeAttr and ensure the replacement logic returns the
escaped string.
- Around line 443-456: The loadHistory function duplicates filtering logic;
replace its direct chrome.storage access and manual filter with a call to
getHistory({ search }) using historySearch.value.trim() (or empty string) to
fetch the filtered entries, then pass the result to renderHistory; update
references to use getHistory (from src/history.ts) instead of emailHistory cast,
while preserving the call to renderHistory(entries).
🪄 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: 72c3d8f7-7766-4c69-bfac-9935f548e614
📒 Files selected for processing (8)
.claude/CLAUDE.mdsrc/background.tssrc/history.test.tssrc/history.tssrc/types/index.tssrc/ui/options.htmlsrc/ui/options.test.tssrc/ui/options.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .claude/CLAUDE.md
- src/types/index.ts
- src/ui/options.html
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
toolkit/scripts/build.js (1)
86-104:⚠️ Potential issue | 🟡 MinorInconsistency:
ui/popup.jsmissing from classic script mode array.Line 86 (ES module mode) includes
ui/popup.js, but line 104 (classic script mode) does not. If the build ever runs in classic script mode,ui/popup.jswould retain itsexport {}statement, potentially causing runtime issues.Proposed fix
- const jsFiles = ['background.js', 'content.js', 'utils.js', 'ui/options.js']; + const jsFiles = ['background.js', 'content.js', 'utils.js', 'ui/options.js', 'ui/popup.js'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolkit/scripts/build.js` around lines 86 - 104, The classic-script branch misses ui/popup.js in the jsFiles array, so in toolkit/scripts/build.js make the arrays consistent by adding 'ui/popup.js' to the jsFiles list (mirror the contentScriptFiles used earlier); update the jsFiles declaration near the classic script branch so background.js, content.js, utils.js, ui/options.js, and ui/popup.js are included and thereby ensure exports are stripped for the popup file as well.
♻️ Duplicate comments (1)
docs/Email-Provider.md (1)
109-109:⚠️ Potential issue | 🟡 MinorIncorrect file reference for domain lists.
Line 109 states the domain lists are in
src/providers.ts, but based on the codebase, the actualPLUS_SUPPORTED_DOMAINSandPLUS_UNSUPPORTED_DOMAINSsets are defined insrc/provider-domains.ts. Theproviders.tsfile only re-exports them.📝 Suggested fix
-The extension maintains two lists of known email provider domains in `src/providers.ts`. When you enter your email address: +The extension maintains two lists of known email provider domains in `src/provider-domains.ts` (re-exported by `src/providers.ts`). When you enter your email address:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/Email-Provider.md` at line 109, Update the docs to reference the correct source file for the domain lists: change the mention of src/providers.ts to src/provider-domains.ts and note that PLUS_SUPPORTED_DOMAINS and PLUS_UNSUPPORTED_DOMAINS are defined there (src/providers.ts only re-exports them); modify the sentence around the domain lists so it names the exact symbols PLUS_SUPPORTED_DOMAINS and PLUS_UNSUPPORTED_DOMAINS and clarifies that providers.ts merely re-exports those sets.
🧹 Nitpick comments (1)
toolkit/scripts/build.js (1)
146-163: Compiled file verification includes non-manifest modules — intentional but worth documenting.
history.js,provider-domains.js, andproviders.jsare verified as required outputs but aren't directly referenced inmanifest.json. Based on context,providers.jsis imported byui/options.tsat runtime. Consider adding a brief comment clarifying which files are manifest entry points vs. imported dependencies, for maintainability.Suggested comment addition
// Verify compiled output console.log('\n📋 Verifying compiled files:'); +// Note: Includes both manifest entry points and their ES module dependencies const requiredCompiledFiles = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolkit/scripts/build.js` around lines 146 - 163, The requiredCompiledFiles array currently lists both manifest entry points and imported dependencies (e.g., 'history.js', 'provider-domains.js', 'providers.js' which are not in manifest.json but are runtime imports like providers.js imported by ui/options.ts); add a brief clarifying comment above the requiredCompiledFiles declaration stating which entries are direct manifest entry points versus which are internal/imported dependencies (mention manifest.json and ui/options.ts as examples) so future maintainers understand why non-manifest files are included.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/README.md:
- Around line 11-13: The fenced code block containing "W3 → W1 (CI gate) → W2
(build + package) → release job" must include a language specifier to satisfy
markdownlint MD040; update the triple-backtick fence surrounding that line to
use "text" (i.e., ```text) so the block is explicitly marked as plain text and
the linter no longer flags it.
In `@docs/Email-Provider.md`:
- Line 77: The decision table row containing `name@hey.com` currently shows a ✅
and the text "Hey supports `+`" but conflicts with the provider table entry
"Hey" which is listed under "Does NOT Support Plus Addressing" with the note
"Aliases only via HEY for Domains"; update the decision-table row for
`name@hey.com` to use a ⚠️ (warning) instead of ✅ to reflect limited/conditional
support, or remove the `name@hey.com` row entirely so it no longer contradicts
the Hey provider entry.
- Line 11: Update the sentence that currently reads "Uses [sub-addressing (RFC
5233)](...)" to either (A) replace the RFC5233 citation with a clarification
that RFC 5233 only specifies Sieve filtering behavior for systems that already
support sub-addressing and does not define the '+' convention itself, noting
that plus-addressing is implementation/site-specific and not standardized by a
single RFC; or (B) swap RFC5233 for a more appropriate reference if you have
one, and explicitly state that support for the '+' separator is
provider-dependent. Locate the exact text "Uses [sub-addressing (RFC 5233)]" in
Email-Provider.md and update the wording accordingly.
---
Outside diff comments:
In `@toolkit/scripts/build.js`:
- Around line 86-104: The classic-script branch misses ui/popup.js in the
jsFiles array, so in toolkit/scripts/build.js make the arrays consistent by
adding 'ui/popup.js' to the jsFiles list (mirror the contentScriptFiles used
earlier); update the jsFiles declaration near the classic script branch so
background.js, content.js, utils.js, ui/options.js, and ui/popup.js are included
and thereby ensure exports are stripped for the popup file as well.
---
Duplicate comments:
In `@docs/Email-Provider.md`:
- Line 109: Update the docs to reference the correct source file for the domain
lists: change the mention of src/providers.ts to src/provider-domains.ts and
note that PLUS_SUPPORTED_DOMAINS and PLUS_UNSUPPORTED_DOMAINS are defined there
(src/providers.ts only re-exports them); modify the sentence around the domain
lists so it names the exact symbols PLUS_SUPPORTED_DOMAINS and
PLUS_UNSUPPORTED_DOMAINS and clarifies that providers.ts merely re-exports those
sets.
---
Nitpick comments:
In `@toolkit/scripts/build.js`:
- Around line 146-163: The requiredCompiledFiles array currently lists both
manifest entry points and imported dependencies (e.g., 'history.js',
'provider-domains.js', 'providers.js' which are not in manifest.json but are
runtime imports like providers.js imported by ui/options.ts); add a brief
clarifying comment above the requiredCompiledFiles declaration stating which
entries are direct manifest entry points versus which are internal/imported
dependencies (mention manifest.json and ui/options.ts as examples) so future
maintainers understand why non-manifest files are included.
🪄 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: 25bd6deb-3e79-46b3-af1f-49dc79d36798
📒 Files selected for processing (10)
.claude/CLAUDE.md.github/workflows/README.md.github/workflows/W1-Test.yml.github/workflows/W2-Build.yml.github/workflows/W3-Release-Chrome-Web-Store.yml.github/workflows/build-and-test.ymldocs/Email-Provider.mddocs/README.mdsrc/provider-domains.tstoolkit/scripts/build.js
💤 Files with no reviewable changes (1)
- .github/workflows/build-and-test.yml
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/W1-Test.yml
- docs/README.md
- .github/workflows/W2-Build.yml
- src/provider-domains.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/CLAUDE.md
manuelgruber
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
user+site@domain.comformat alongside catch-all modesrc/ui/folder for cleaner organizationTest plan
dist/in Chrome and verify popup opens on icon clickchrome://pages shows appropriate error in popupSummary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
Tests
Chores