feat: Chrome MV3 extension with Clerk auth sync (#7)#93
Open
Maurya-H wants to merge 14 commits into
Open
Conversation
|
@Maurya-H is attempting to deploy a commit to the venkat-kolasani's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd Chrome MV3 extension to save opportunities with Clerk-authenticated API calls
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
Context used 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
extensions/manifest.json (2)
12-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDev-only host permissions shipped alongside production origin.
http://localhost:3000/*andhttp://localhost:3001/*are included next to the productionfuturetracker.onlineorigin. Consider gating these via a build-time manifest transform (e.g., invite.config.js) so only the production host ships in release builds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/manifest.json` around lines 12 - 17, The manifest currently ships dev-only localhost host_permissions alongside the production origin, so update the manifest generation to strip those entries from release builds. Adjust the build-time manifest transform in vite.config.js (or the manifest pipeline that produces extensions/manifest.json) so host_permissions includes only the production futuretracker.online and Clerk domains for production, while preserving localhost entries only in development.
11-25: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftContent script runs on every page despite
scriptingpermission being available.
content_scriptsmatches<all_urls>and injects unconditionally ondocument_idlefor every page the user visits, even thoughactiveTab+scriptingare already declared. Given the popup only needs metadata when the user actively opens it (percontent.js'sGET_PAGE_METADATAhandler), consider injecting the script on-demand withchrome.scripting.executeScriptfrom the popup/background usingactiveTab, and drop the staticcontent_scriptsentry. This reduces the Chrome install-time permission warning ("Read and change your data on all websites") and limits code execution to user-initiated actions only, which also improves Chrome Web Store review odds.🔒 Suggested direction
- "permissions": ["activeTab", "storage", "scripting","cookies"], + "permissions": ["activeTab", "storage", "scripting", "cookies"], "host_permissions": [ "https://futuretracker.online/*", "http://localhost:3000/*", "http://localhost:3001/*", "https://*.clerk.accounts.dev/*" ], "background": { "service_worker": "src/background.js", "type": "module" }, - "content_scripts": [ - { - "matches": ["<all_urls>"], - "js": ["src/content.js"], - "run_at": "document_idle" - } - ] + // Inject src/content.js on demand via chrome.scripting.executeScript + // when the popup opens, using the activeTab permission instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/manifest.json` around lines 11 - 25, The manifest is still registering a static content script that runs on every page, which defeats the purpose of using activeTab and scripting. Remove the unconditional content_scripts entry from the manifest and load src/content.js on demand from the popup/background flow using chrome.scripting.executeScript when the user opens the popup or triggers metadata collection. Keep the existing GET_PAGE_METADATA path in content.js working with the new injection approach, and rely on activeTab for the temporary page access.extensions/src/popup/popup.jsx (1)
54-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLabels not associated with their inputs.
<label>elements have nohtmlFor, so screen readers can't programmatically associate them with theTitle/Descriptionfields.♿ Suggested fix
- <label>Title</label> + <label htmlFor="opp-title">Title</label> <input + id="opp-title" value={data.title} ... /> - <label>Description</label> + <label htmlFor="opp-description">Description</label> <textarea + id="opp-description" value={data.description} ... />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/popup/popup.jsx` around lines 54 - 71, The Title and Description labels in popup.jsx are not programmatically associated with their form controls, so update the input/textarea markup in the popup component to connect each <label> to its corresponding field using matching identifiers in the popup rendering logic around the Title and Description inputs. Use the existing data.title and data.description controls in the popup component to add the necessary label-to-control linkage so screen readers can correctly identify each field.extensions/src/content.js (1)
8-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOnly return
truewhen actually responding asynchronously.The listener returns
trueunconditionally, even for message types it doesn't handle, which needlessly keeps the response channel open and can trigger Chrome's "message channel closed" warnings for unhandled message types.🔧 Proposed fix
chrome.runtime.onMessage.addListener((msg, sender, sendResponse) => { if (msg.type === 'GET_PAGE_METADATA') { sendResponse({ title: getMeta('og:title') || document.title, description: getMeta('og:description') || '', url: window.location.href, }); + return true; } - return true; + return false; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/content.js` around lines 8 - 17, Update the chrome.runtime.onMessage.addListener handler in content.js so it only returns true when GET_PAGE_METADATA is actually handled and sendResponse is used, and otherwise returns false/undefined for unhandled msg.type values. Keep the response channel open only for the GET_PAGE_METADATA path by tying the return value to that branch, using the existing listener and sendResponse logic.extensions/.env.example (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
VITE_SYNC_HOSTin env example.
extensions/src/lib/clerk.jsreadsimport.meta.env.VITE_SYNC_HOST(falling back tohttp://localhost:3000), but it's undocumented here. Anyone following this file for prod setup will silently ship with the wrong sync host.📝 Proposed addition
# Clerk Authentication (Extension) VITE_CLERK_PUBLISHABLE_KEY=pk_test_your_key_here +VITE_SYNC_HOST=https://futuretracker.online # Backend API URL VITE_API_BASE=http://localhost:3001🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/.env.example` around lines 1 - 5, Add the missing VITE_SYNC_HOST entry to the extension env example so it matches what extensions/src/lib/clerk.js reads from import.meta.env.VITE_SYNC_HOST. Update the .env.example alongside VITE_CLERK_PUBLISHABLE_KEY and VITE_API_BASE to document the sync host used by the Clerk integration, so production setup doesn’t rely on the fallback localhost value.extensions/src/background.js (1)
1-12: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUpdate the Clerk background worker pattern
@clerk/chrome-extension/backgroundis deprecated in@clerk/chrome-extension@^3.1.47; switch to@clerk/chrome-extension/clientwithbackground: true.- If Clerk is needed in the service worker, initialize it in the listener that actually uses it rather than
onInstalled, since that hook runs only once per install/update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/background.js` around lines 1 - 12, The background worker setup is using the deprecated Clerk background import and initializing Clerk only in onInstalled, which runs just once. Update the background entrypoint to use createClerkClient from `@clerk/chrome-extension/client` with background: true, and move the clerkClient initialization/load into the service worker listener or handler that actually needs Clerk rather than onInstalled. Keep the existing CLERK_PUBLISHABLE_KEY and SYNC_HOST wiring, and adjust the current clerkClient usage in the background script accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/.env.example`:
- Around line 10-13: The example environment file has a duplicated CORS_ORIGIN
setting, which makes the allowlist ambiguous. Update the backend/.env.example
entry to keep only one CORS_ORIGIN line with the full comma-separated origin
list, and remove the stale duplicate near EXTENSION_ID so the example is
unambiguous.
In `@extensions/src/lib/api.js`:
- Around line 1-14: The saveOpportunity API call needs a timeout and an early
check for a missing auth token. Update saveOpportunity in api.js to
short-circuit when token is falsy instead of sending a Bearer null header, and
add timeout handling around the fetch request so a stalled request fails cleanly
rather than leaving the popup stuck. Use the existing API_BASE and
saveOpportunity symbols to locate the request logic, and make sure the timeout
error is surfaced through the same rejection path as other request failures.
In `@extensions/src/popup/main.jsx`:
- Around line 1-4: The import in main.jsx uses a filename casing that does not
match the actual popup component file, so update the reference in the main
entrypoint to match the exact on-disk casing used by Popup.jsx/popup.jsx. Check
the import of Popup in main.jsx and either rename the file or adjust the import
path so the symbol resolves correctly on case-sensitive filesystems.
In `@extensions/src/popup/popup.jsx`:
- Around line 39-44: The save payload in popup.jsx is hardcoding the listing
metadata, which mislabels non-internship items and assumes every listing is
already applied. Update the save flow around the saveOpportunity call to use the
actual listing type from the current data instead of forcing category:
'internship', and only set status to 'applied' when that reflects the user’s
real action. Make sure the fields passed from the popup’s submit/save handler
preserve the listing’s true category and application state.
- Around line 10-16: The popup’s metadata fetch in useEffect is missing error
handling and a null guard: tab.id is used directly and chrome.runtime.lastError
is ignored when chrome.tabs.sendMessage fails. Update the
chrome.tabs.query/sendMessage flow to verify the active tab exists before
sending, and handle chrome.runtime.lastError in the sendMessage callback so
failures are surfaced instead of leaving the UI blank. Use the existing
useEffect callback and the GET_PAGE_METADATA message path to locate the fix.
- Line 6: The popup auth state is reading isSignedIn before Clerk finishes
initializing, which can briefly send authenticated users down the signed-out
path. Update the popup logic around useAuth() so the signed-in/signed-out branch
is gated by isLoaded as well as isSignedIn, and only render auth-dependent UI
once Clerk has finished loading; use the existing useAuth hook and the popup
component as the place to apply this guard.
---
Nitpick comments:
In `@extensions/.env.example`:
- Around line 1-5: Add the missing VITE_SYNC_HOST entry to the extension env
example so it matches what extensions/src/lib/clerk.js reads from
import.meta.env.VITE_SYNC_HOST. Update the .env.example alongside
VITE_CLERK_PUBLISHABLE_KEY and VITE_API_BASE to document the sync host used by
the Clerk integration, so production setup doesn’t rely on the fallback
localhost value.
In `@extensions/manifest.json`:
- Around line 12-17: The manifest currently ships dev-only localhost
host_permissions alongside the production origin, so update the manifest
generation to strip those entries from release builds. Adjust the build-time
manifest transform in vite.config.js (or the manifest pipeline that produces
extensions/manifest.json) so host_permissions includes only the production
futuretracker.online and Clerk domains for production, while preserving
localhost entries only in development.
- Around line 11-25: The manifest is still registering a static content script
that runs on every page, which defeats the purpose of using activeTab and
scripting. Remove the unconditional content_scripts entry from the manifest and
load src/content.js on demand from the popup/background flow using
chrome.scripting.executeScript when the user opens the popup or triggers
metadata collection. Keep the existing GET_PAGE_METADATA path in content.js
working with the new injection approach, and rely on activeTab for the temporary
page access.
In `@extensions/src/background.js`:
- Around line 1-12: The background worker setup is using the deprecated Clerk
background import and initializing Clerk only in onInstalled, which runs just
once. Update the background entrypoint to use createClerkClient from
`@clerk/chrome-extension/client` with background: true, and move the clerkClient
initialization/load into the service worker listener or handler that actually
needs Clerk rather than onInstalled. Keep the existing CLERK_PUBLISHABLE_KEY and
SYNC_HOST wiring, and adjust the current clerkClient usage in the background
script accordingly.
In `@extensions/src/content.js`:
- Around line 8-17: Update the chrome.runtime.onMessage.addListener handler in
content.js so it only returns true when GET_PAGE_METADATA is actually handled
and sendResponse is used, and otherwise returns false/undefined for unhandled
msg.type values. Keep the response channel open only for the GET_PAGE_METADATA
path by tying the return value to that branch, using the existing listener and
sendResponse logic.
In `@extensions/src/popup/popup.jsx`:
- Around line 54-71: The Title and Description labels in popup.jsx are not
programmatically associated with their form controls, so update the
input/textarea markup in the popup component to connect each <label> to its
corresponding field using matching identifiers in the popup rendering logic
around the Title and Description inputs. Use the existing data.title and
data.description controls in the popup component to add the necessary
label-to-control linkage so screen readers can correctly identify each field.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2899598b-a8e1-4160-8c51-6be474a8eaed
⛔ Files ignored due to path filters (4)
extensions/package-lock.jsonis excluded by!**/package-lock.jsonextensions/public/icons/128.pngis excluded by!**/*.pngextensions/public/icons/16.pngis excluded by!**/*.pngextensions/public/icons/48.pngis excluded by!**/*.png
📒 Files selected for processing (15)
.gitignorebackend/.env.exampleextensions/.env.exampleextensions/manifest.jsonextensions/package.jsonextensions/postcss.config.jsextensions/readme.mdextensions/src/background.jsextensions/src/content.jsextensions/src/lib/api.jsextensions/src/lib/clerk.jsextensions/src/popup/index.htmlextensions/src/popup/main.jsxextensions/src/popup/popup.jsxextensions/vite.config.js
Comment on lines
+1
to
+14
| const API_BASE = import.meta.env.VITE_API_BASE || 'http://localhost:3001'; | ||
|
|
||
| export async function saveOpportunity(token, payload) { | ||
| const res = await fetch(`${API_BASE}/api/opportunities`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| body: JSON.stringify(payload), | ||
| }); | ||
| if (!res.ok) throw new Error(`Save failed: ${res.status}`); | ||
| return res.json(); | ||
| } No newline at end of file |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a request timeout and guard against a missing token.
fetch has no timeout, so a stalled connection leaves the popup's "Saving..." state hanging indefinitely with no way for the user to recover short of closing the popup. Also consider short-circuiting when token is falsy rather than sending Authorization: Bearer null.
🛠️ Suggested fix
const API_BASE = import.meta.env.VITE_API_BASE || 'http://localhost:3001';
export async function saveOpportunity(token, payload) {
- const res = await fetch(`${API_BASE}/api/opportunities`, {
- method: 'POST',
- headers: {
- 'Content-Type': 'application/json',
- Authorization: `Bearer ${token}`,
- },
- body: JSON.stringify(payload),
- });
- if (!res.ok) throw new Error(`Save failed: ${res.status}`);
- return res.json();
+ if (!token) throw new Error('Missing auth token');
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10000);
+ try {
+ const res = await fetch(`${API_BASE}/api/opportunities`, {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ Authorization: `Bearer ${token}`,
+ },
+ body: JSON.stringify(payload),
+ signal: controller.signal,
+ });
+ if (!res.ok) throw new Error(`Save failed: ${res.status}`);
+ return res.json();
+ } finally {
+ clearTimeout(timeoutId);
+ }
}📝 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
| const API_BASE = import.meta.env.VITE_API_BASE || 'http://localhost:3001'; | |
| export async function saveOpportunity(token, payload) { | |
| const res = await fetch(`${API_BASE}/api/opportunities`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${token}`, | |
| }, | |
| body: JSON.stringify(payload), | |
| }); | |
| if (!res.ok) throw new Error(`Save failed: ${res.status}`); | |
| return res.json(); | |
| } | |
| const API_BASE = import.meta.env.VITE_API_BASE || 'http://localhost:3001'; | |
| export async function saveOpportunity(token, payload) { | |
| if (!token) throw new Error('Missing auth token'); | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 10000); | |
| try { | |
| const res = await fetch(`${API_BASE}/api/opportunities`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${token}`, | |
| }, | |
| body: JSON.stringify(payload), | |
| signal: controller.signal, | |
| }); | |
| if (!res.ok) throw new Error(`Save failed: ${res.status}`); | |
| return res.json(); | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extensions/src/lib/api.js` around lines 1 - 14, The saveOpportunity API call
needs a timeout and an early check for a missing auth token. Update
saveOpportunity in api.js to short-circuit when token is falsy instead of
sending a Bearer null header, and add timeout handling around the fetch request
so a stalled request fails cleanly rather than leaving the popup stuck. Use the
existing API_BASE and saveOpportunity symbols to locate the request logic, and
make sure the timeout error is surfaced through the same rejection path as other
request failures.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extensions/src/popup/popup.jsx (1)
55-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winForm labels not programmatically associated with inputs.
<label>elements have nohtmlFor/idpairing with theTitle/Description/URLinputs, so screen readers can't associate them.♿ Suggested fix
- <label>Title</label> + <label htmlFor="opp-title">Title</label> <input + id="opp-title" value={data.title}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/popup/popup.jsx` around lines 55 - 67, The form fields in popup.jsx are using plain <label> elements without programmatic association, so update the Title, Description, and URL label/input pairs in the popup form to be linked via matching label and control identifiers. Use the existing input/textarea elements in the popup component and add stable ids to the fields, then point each corresponding label’s htmlFor at the matching id so assistive technologies can correctly identify the controls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@extensions/src/popup/popup.jsx`:
- Around line 55-67: The form fields in popup.jsx are using plain <label>
elements without programmatic association, so update the Title, Description, and
URL label/input pairs in the popup form to be linked via matching label and
control identifiers. Use the existing input/textarea elements in the popup
component and add stable ids to the fields, then point each corresponding
label’s htmlFor at the matching id so assistive technologies can correctly
identify the controls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4db29ba7-5592-4545-902e-8926e7a0c146
📒 Files selected for processing (3)
extensions/src/content.jsextensions/src/lib/clerk.jsextensions/src/popup/popup.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/src/content.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
extensions/src/popup/popup.jsx (1)
45-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
getToken()runs outsidetryand errors are swallowed silently.
await getToken()on Line 47 executes beforesetStatus('saving')and outside thetryblock, so if it rejects,handleSaverejects withstatusstuck atidleand no user feedback. Separately, thecatch (e)on Line 59 discardse, hiding the save failure cause. Move the token fetch insidetryand log the error.🛡️ Suggested fix
async function handleSave() { if (!data.title) return alert('Please enter a title!'); - const token = await getToken(); - if (!token) return alert('Not signed in!'); setStatus('saving'); try { + const token = await getToken(); + if (!token) { + setStatus('idle'); + return alert('Not signed in!'); + } await saveOpportunity(token, { title: data.title, description: data.description, link: data.link, category: data.category, status: data.status, }); setStatus('saved'); } catch (e) { + console.error('FutureTracker: save failed', e); setStatus('error'); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/popup/popup.jsx` around lines 45 - 62, Move the token fetch in handleSave into the same try block as saveOpportunity so getToken() failures are caught and status is updated consistently, and update the catch in popup.jsx to use the caught error instead of discarding it so the failure is logged or surfaced with context. Keep the existing handleSave flow and setStatus calls, but ensure any rejection from getToken or saveOpportunity sets the error state and does not fail silently.extensions/src/background.js (1)
9-14: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle
clerkClient.load()rejection.
load()can reject on network or misconfiguration; the top-levelinitClerk()call has no.catch/tryso failures surface only as an unhandled rejection with no diagnostic. Wrap it so auth-init failures are logged.🛡️ Suggested fix
async function initClerk() { - await clerkClient.load(); - console.log('FutureTracker: Clerk loaded'); + try { + await clerkClient.load(); + console.log('FutureTracker: Clerk loaded'); + } catch (err) { + console.error('FutureTracker: Clerk failed to load', err); + } } initClerk();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/background.js` around lines 9 - 14, `initClerk()` in background.js does not handle failures from `clerkClient.load()`, so auth initialization can reject without any logged context. Wrap the await in `initClerk` with try/catch (or add a `.catch` to the `initClerk()` call) and log the error with a clear message before returning/failing gracefully; use the `initClerk` and `clerkClient.load()` symbols to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@extensions/src/background.js`:
- Around line 9-14: `initClerk()` in background.js does not handle failures from
`clerkClient.load()`, so auth initialization can reject without any logged
context. Wrap the await in `initClerk` with try/catch (or add a `.catch` to the
`initClerk()` call) and log the error with a clear message before
returning/failing gracefully; use the `initClerk` and `clerkClient.load()`
symbols to locate the fix.
In `@extensions/src/popup/popup.jsx`:
- Around line 45-62: Move the token fetch in handleSave into the same try block
as saveOpportunity so getToken() failures are caught and status is updated
consistently, and update the catch in popup.jsx to use the caught error instead
of discarding it so the failure is logged or surfaced with context. Keep the
existing handleSave flow and setStatus calls, but ensure any rejection from
getToken or saveOpportunity sets the error state and does not fail silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cfa2d54-e195-4282-a0c6-8b04b42d78d4
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
backend/.env.exampleextensions/readme.mdextensions/src/background.jsextensions/src/popup/popup.jsx
✅ Files skipped from review due to trivial changes (2)
- extensions/readme.md
- backend/.env.example
There was a problem hiding this comment.
🧹 Nitpick comments (2)
extensions/src/popup/popup.jsx (2)
40-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
alert()with the existing inline status UI.
alert('Please enter a title!')(Line 41) andalert('Not signed in!')(Line 47) use native blocking dialogs inside an extension popup. Chromium extension popups are known to close on blur, andalert()/confirm()dialogs are dismissed when their context loses focus, so these messages can vanish before the user reads them, or close the popup outright, silently discarding entered data. The component already has an inline status pattern (status === 'error'at Line 114) that would be more reliable and consistent here.♻️ Suggested fix
async function handleSave() { - if (!data.title) return alert('Please enter a title!'); + if (!data.title) { + setStatus('missing-title'); + return; + } setStatus('saving'); try { const token = await getToken(); if (!token) { - setStatus('idle'); - return alert('Not signed in!'); + setStatus('error'); + return; }Then render a corresponding message alongside the existing
saved/errorfeedback block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/popup/popup.jsx` around lines 40 - 61, The `handleSave` flow in `popup.jsx` uses blocking `alert()` calls for validation and auth errors, which should be replaced with the existing inline `status` UI. Update `handleSave` to set an appropriate error status/message instead of calling `alert('Please enter a title!')` and `alert('Not signed in!')`, and then render those messages in the same feedback area that already handles `status === 'saved'` and `status === 'error'` so the popup stays open and the user can see the message reliably.
66-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssociate
<label>elements with their controls.Labels for Title, Description, URL, Category, and Status are plain text with no
htmlFor/idpairing, so screen readers can't associate them with their inputs.♻️ Example fix (repeat pattern for each field)
- <label>Title</label> + <label htmlFor="ft-title">Title</label> <input + id="ft-title" value={data.title}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/popup/popup.jsx` around lines 66 - 105, The Title, Description, URL, Category, and Status labels in popup.jsx are not associated with their form controls, so update the form in the popup component to pair each label with its corresponding input/select using matching htmlFor and id values. Use the existing form fields inside the popup render to add unique ids for the controls and connect them to their labels so assistive technologies can correctly identify each field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@extensions/src/popup/popup.jsx`:
- Around line 40-61: The `handleSave` flow in `popup.jsx` uses blocking
`alert()` calls for validation and auth errors, which should be replaced with
the existing inline `status` UI. Update `handleSave` to set an appropriate error
status/message instead of calling `alert('Please enter a title!')` and
`alert('Not signed in!')`, and then render those messages in the same feedback
area that already handles `status === 'saved'` and `status === 'error'` so the
popup stays open and the user can see the message reliably.
- Around line 66-105: The Title, Description, URL, Category, and Status labels
in popup.jsx are not associated with their form controls, so update the form in
the popup component to pair each label with its corresponding input/select using
matching htmlFor and id values. Use the existing form fields inside the popup
render to add unique ids for the controls and connect them to their labels so
assistive technologies can correctly identify each field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 383033ef-2816-44e7-9eee-d5463525e663
📒 Files selected for processing (2)
extensions/src/background.jsextensions/src/popup/popup.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/src/background.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
extensions/src/popup/popup.jsx (1)
10-18: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a URL fallback in the popup.
data.linkonly comes fromGET_PAGE_METADATA, so achrome.tabs.sendMessagefailure leaves the read-only URL field empty and can save the opportunity without its source link. Usetab.urlas a fallback when the active tab is queried.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/popup/popup.jsx` around lines 10 - 18, The popup metadata load in useEffect only fills data.link from GET_PAGE_METADATA, so if chrome.tabs.sendMessage fails the URL field stays empty. Update the active-tab query handler to use tab.url as a fallback when populating state, and keep the existing GET_PAGE_METADATA merge in the popup.jsx flow so the read-only link is always available even when message delivery fails.
🧹 Nitpick comments (2)
extensions/src/popup/popup.jsx (2)
70-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark title input as required for consistency with validation.
handleSaveenforces a required title (Line 41), but the input itself has norequired/aria-requiredattribute.♿ Proposed fix
<input id="ft-title" value={data.title} onChange={(e) => setData({ ...data, title: e.target.value })} + required + aria-required="true" style={{ width: '100%', marginBottom: '10px', padding: '6px', borderRadius: '4px', border: 'none', background: '`#1e293b`', color: '`#f1f5f9`' }} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/popup/popup.jsx` around lines 70 - 76, The Title field in the popup form is validated as required by handleSave, but the input itself is missing the matching accessibility/HTML requirement. Update the ft-title input in popup to include required and aria-required so the form semantics and validation stay consistent; keep the change localized to the input element used by the popup form.
128-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an
aria-liveregion for save status feedback.Status messages (saved/error/missing-title/auth-error) toggle in and out via conditional rendering with no live region, so screen readers won't announce the outcome of a save action.
♿ Proposed fix
+ <div aria-live="polite"> {status === 'saved' && <p style={{ color: '`#22c55e`', textAlign: 'center' }}>Saved successfully ✓</p>} {status === 'error' && <p style={{ color: '`#ef4444`', textAlign: 'center' }}>Failed to save. Try again.</p>} {status === 'missing-title' && <p style={{ color: '`#ef4444`', textAlign: 'center' }}>Please enter a title!</p>} {status === 'auth-error' && <p style={{ color: '`#ef4444`', textAlign: 'center' }}>Not signed in!</p>} + </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/src/popup/popup.jsx` around lines 128 - 132, The status feedback in popup.jsx is conditionally rendered without a live region, so screen readers may not announce save outcomes. Update the status message area around the existing saved/error/missing-title/auth-error conditions to use an aria-live region (and keep the visible message text) so assistive tech announces changes when the status state updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@extensions/src/popup/popup.jsx`:
- Around line 10-18: The popup metadata load in useEffect only fills data.link
from GET_PAGE_METADATA, so if chrome.tabs.sendMessage fails the URL field stays
empty. Update the active-tab query handler to use tab.url as a fallback when
populating state, and keep the existing GET_PAGE_METADATA merge in the popup.jsx
flow so the read-only link is always available even when message delivery fails.
---
Nitpick comments:
In `@extensions/src/popup/popup.jsx`:
- Around line 70-76: The Title field in the popup form is validated as required
by handleSave, but the input itself is missing the matching accessibility/HTML
requirement. Update the ft-title input in popup to include required and
aria-required so the form semantics and validation stay consistent; keep the
change localized to the input element used by the popup form.
- Around line 128-132: The status feedback in popup.jsx is conditionally
rendered without a live region, so screen readers may not announce save
outcomes. Update the status message area around the existing
saved/error/missing-title/auth-error conditions to use an aria-live region (and
keep the visible message text) so assistive tech announces changes when the
status state updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 506f89b2-9ae8-4ed8-a92e-5f9fc74b4eaa
📒 Files selected for processing (1)
extensions/src/popup/popup.jsx
Venkat-Kolasani
requested changes
Jul 10, 2026
Venkat-Kolasani
left a comment
Venkat-Kolasani
left a comment
Owner
There was a problem hiding this comment.
Hi @Maurya-H -- thanks for the work on this extension PR. I reviewed it locally and the extension build succeeds, but I need a few changes before I can merge.
Please reduce extension permissions:
Remove the unused cookies permission.
Avoid always injecting the content script on <all_urls> if possible; prefer on-demand access via activeTab/chrome.scripting, or restrict the match patterns.
Production setup:
Add the actual production backend domain to host_permissions; the documented Render API URL is not currently permitted.
Add a stable extension ID approach (manifest.json key) so Clerk and backend CORS can reliably allow chrome-extension://.
Fix/update documentation:
The backend reads CORS_ORIGIN, not EXTENSION_ID. Update extensions/readme.md accordingly.
Document VITE_SYNC_HOST=https://futuretracker.online.
Correct the folder-structure filename from Popup.jsx to popup.jsx.
Include clear local and production API setup instructions, including the exact CORS value.
Reliability and tests:
Add a timeout/abort handling to the save request so the popup cannot remain stuck on “Saving…” if the API stalls.
Add a test script and at least basic tests for the extension’s metadata/save logic.
Please clean up the trailing whitespace reported by git diff --check.
Once these are addressed, please run and share results for:
npm run test:ci
npm run build
npm run check:architecture
cd backend && npm test
cd extensions && npm run build
Also, the Vercel check needs authorization from the project team; I’ll handle that separately after the PR is otherwise ready. Also share the pictures/screenshot of the chrome extension working after testing end-to-end, Share Proof.
Thank you again for your intrest into futuretracker..
Author
HI @Venkat-Kolasani ! |
Author
|
✅ npm run test:ci → 28/28 passed |
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
2 participants
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.
Fixes #7
What this PR does
extensions/folder with Chrome MV3 extension@clerk/chrome-extensionwithsyncHost: https://futuretracker.onlineManual Test Plan
Out of Scope (per issue #7)
Known Issue
Dashboard crashes when opportunity has null deadline — pre-existing bug in
src/utils/dateHelpers.js, not introduced by this PR.parseLocalDateneeds a null check.Checklist
extensions/folder — Chrome MV3 only@clerk/chrome-extensionwith syncHostSummary by CodeRabbit