feat(chrome-ext): content script fixes, developer docs, and user guide - #821
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a complete PMOVES.AI Chrome Manifest V3 extension: background service worker with health polling and extensive message routing, YouTube content script with floating UI and actions, centralized API client and constants, popup and options UIs, styles, documentation, and a local mock server for testing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Content as YouTube<br/>Content Script
participant BG as Background<br/>Service Worker
participant API as PMOVES<br/>Backend Services
participant Storage as chrome.storage/local
User->>Content: Click "Process Video"
Content->>BG: sendMessage { action: "processVideo", videoUrl }
BG->>BG: extractVideoId, enqueue status
BG->>Storage: store processingStatus
BG->>API: POST /ingest (pmovesYt)
alt Ingest queued
API-->>BG: 202 Queued
BG->>Storage: record history entry
BG-->>Content: response { status: "queued" }
BG->>User: chrome.notifications create "Video queued"
else Error
API-->>BG: error
BG-->>Content: response { error }
BG->>User: chrome.notifications create "Error"
end
sequenceDiagram
participant BG as Background<br/>Service Worker
participant Health as Service<br/>Health Endpoints
participant Action as Extension<br/>Action Badge
participant UI as Popup/Content UI
BG->>BG: set alarm for health polling
loop every configured interval
BG->>Health: parallel health checks (tensorZero, gpu, hirag, etc.)
Health-->>BG: status/latency or error
end
BG->>BG: aggregate statuses -> overall state
BG->>Action: update badge color/text
UI->>BG: sendMessage refreshHealth
BG-->>UI: return aggregated health + latencies
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+238
to
+245
| shapesDiv.innerHTML = r.shapes.map(s => { | ||
| const svgUrl = `${$('#url-gateway')?.value || 'http://localhost:8085'}/viz/shape/${encodeURIComponent(s.shape_id)}.svg`; | ||
| return `<div style="margin:4px 0;font-size:13px;"> | ||
| <strong>${escapeHtml(s.label || s.shape_id)}</strong> | ||
| <span style="color:#888;margin-left:8px;">${s.created_at || ''}</span> | ||
| <a href="${svgUrl}" target="_blank" style="margin-left:8px;color:#667eea;">View SVG</a> | ||
| </div>`; | ||
| }).join(''); |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
In general, the fix is to ensure that any data originating from DOM text (or other untrusted sources) is not directly inserted into the DOM using innerHTML without appropriate escaping. Instead, either (a) use textContent / createElement / attribute setters, or (b) HTML-encode the content before concatenating it into HTML strings.
For this specific case, the only problematic part is the href="${svgUrl}" interpolation, where svgUrl incorporates $('#url-gateway')?.value. The rest of the interpolated content already uses escapeHtml for s.label/s.shape_id, and s.created_at is injected into a text context where the risk is much lower but still ideally should be encoded. The safest minimal fix, without changing functionality, is:
- Introduce an
escapeHtmlhelper near the top of the file (if not already present in unseen parts) to escape&,<,>,", and'. - Use
escapeHtmlwhen interpolatingsvgUrlinto thehrefattribute:href="${escapeHtml(svgUrl)}". - Optionally, also escape
s.created_atbefore insertion, but CodeQL’s reported flow only requires fixing thesvgUrluse.
We do not change how URLs are built or how shapes are rendered; we only add HTML-encoding at the point where tainted strings become HTML.
Suggested changeset
1
pmoves/chrome-extension/options/options.js
| @@ -2,6 +2,15 @@ | ||
|
|
||
| const $ = (sel) => document.querySelector(sel); | ||
|
|
||
| function escapeHtml(str) { | ||
| return String(str) | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/'/g, '''); | ||
| } | ||
|
|
||
| const SERVICES = [ | ||
| 'tensorzero', 'gpuOrchestrator', 'hirag', | ||
| 'pmovesYt', 'agentZero', 'fluteGateway', 'prometheus', 'gateway', | ||
| @@ -239,8 +248,8 @@ | ||
| const svgUrl = `${$('#url-gateway')?.value || 'http://localhost:8085'}/viz/shape/${encodeURIComponent(s.shape_id)}.svg`; | ||
| return `<div style="margin:4px 0;font-size:13px;"> | ||
| <strong>${escapeHtml(s.label || s.shape_id)}</strong> | ||
| <span style="color:#888;margin-left:8px;">${s.created_at || ''}</span> | ||
| <a href="${svgUrl}" target="_blank" style="margin-left:8px;color:#667eea;">View SVG</a> | ||
| <span style="color:#888;margin-left:8px;">${escapeHtml(s.created_at || '')}</span> | ||
| <a href="${escapeHtml(svgUrl)}" target="_blank" style="margin-left:8px;color:#667eea;">View SVG</a> | ||
| </div>`; | ||
| }).join(''); | ||
| pre.textContent = JSON.stringify(r, null, 2); |
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
| req.on('end', () => { | ||
| let parsed = {}; | ||
| try { parsed = JSON.parse(body); } catch {} | ||
| const result = handler(parsed); |
Check failure
Code scanning / CodeQL
Unvalidated dynamic method call High test
Copilot Autofix
AI 7 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
pmoves/chrome-extension/README.md (2)
1-86: Consider adding API documentation for programmatic access.The README covers features, installation, and usage well. However, per coding guidelines, README documentation should include API documentation for programmatic access (JavaScript, Python, cURL examples). Consider adding a section showing how to interact with the extension's message API or the backend services it connects to.
As per coding guidelines: "ALWAYS write documentation in README.md at project root containing: what the app does, how to use it, and API documentation for programmatic access (Javascript, Python, Curl)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/chrome-extension/README.md` around lines 1 - 86, Add an "API Documentation" section to README.md showing programmatic access examples: document the extension message API (background.js service worker message routing and content.js injection APIs), and include concrete usage examples for lib/pmoves-api.js (JavaScript), a Python snippet calling the exposed backend endpoints (TensorZero 3030, PMOVES.YT 8077, Hi-RAG 8086), and cURL examples for key operations (ingest video, query Hi-RAG, chat via TensorZero). For each example, show the request shape, required headers/auth (Agent Zero token, Flute API key), and the corresponding response shape so developers can integrate programmatically. Ensure the section references background.js, content.js, and lib/pmoves-api.js so readers can locate the implementation.
77-84: Add language specifier to fenced code block.The architecture diagram code block should specify a language (e.g.,
textorplaintext) for better rendering and linting compliance.📝 Proposed fix
-``` +```text background.js Service worker: config, health polling, message routing content.js YouTube page injection: buttons, menus, overlays lib/pmoves-api.js Shared API client for all 7 PMOVES.AI services lib/constants.js Default config, service URLs, health endpoints popup/ Dashboard: health grid, GPU panel, quick actions options/ Settings: endpoints, auth, features, diagnostics</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@pmoves/chrome-extension/README.mdaround lines 77 - 84, Update the fenced
code block that lists background.js, content.js, lib/pmoves-api.js,
lib/constants.js, popup/, and options/ in README.md by adding a language
specifier (e.g., ```text) to the opening fence so the block is rendered and
linted correctly; locate the code block containing the "background.js
Service worker: config, health polling, message routing" entry and change its
opening fence to include the language token.</details> </blockquote></details> <details> <summary>pmoves/chrome-extension/popup/popup.html (1)</summary><blockquote> `137-140`: **Consider populating version dynamically from `constants.js`.** The version is hardcoded as `v1.0.0`. Since `constants.js` exports `VERSION = '1.0.0'`, consider having `popup.js` set this value dynamically to avoid version drift. The popup.js would set: ```javascript document.getElementById('version-label').textContent = `v${VERSION}`; ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@pmoves/chrome-extension/popup/popup.html` around lines 137 - 140, The footer version is hardcoded; update popup.js to read the exported VERSION from constants.js and set the element with id "version-label" accordingly (e.g., assign `v${VERSION}` to its textContent), ensuring constants.js is loaded before popup.js so VERSION is available; target the DOM element by id "version-label" in popup.js and remove the hardcoded string in popup.html. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@pmoves/chrome-extension/background.js:
- Around line 145-156: The storeInHistory function performs a racy
read-modify-write against chrome.storage.local; serialize updates by introducing
a single updater (e.g., a promise queue or mutex) or an in-memory cache to
coalesce writes: keep a module-level processingHistoryCache array that
storeInHistory updates synchronously (unshift/truncate) and then flushes to
chrome.storage.local via a single pendingWritePromise to ensure only one
chrome.storage.local.set runs at a time; ensure processAllVideos calls that
enqueue updates go through this same updater so overlapping calls cannot
overwrite each other.- Around line 14-24: The code currently reads/writes the entire pmovesConfig via
chrome.storage.sync (in loadConfig and saveConfig), which will sync secrets like
auth.agentZeroToken and auth.fluteApiKey; change the logic to separate sensitive
auth from non-sensitive settings: when loading (loadConfig) read both
chrome.storage.sync for non-sensitive config and chrome.storage.local (or
chrome.storage.session) for the auth object, then merge them into the in-memory
config and call setServiceUrls(config.services); when saving (saveConfig) write
non-sensitive fields to chrome.storage.sync and write only the auth sub-object
(auth.agentZeroToken, auth.fluteApiKey) to chrome.storage.local/session so
tokens are not propagated via Chrome Sync. Ensure you reference pmovesConfig and
the auth.* fields when splitting/merging.- Around line 120-137: processingStatus is being left at 'processing' after
yt.ingest(), causing future requests to always see already_processing; modify
the flow around yt.ingest(url) so the entry keyed by (videoId || url) is removed
when the enqueue returns (success or failure). Specifically, after awaiting
yt.ingest(url) (and after storeInHistory/notify logic) call
processingStatus.delete(videoId || url), and also ensure you delete the key in a
catch or finally block so getStatus and retries can progress; touch the
processingStatus map, the yt.ingest call, and getStatus handling to make sure
the sentinel is cleaned up.In
@pmoves/chrome-extension/content.js:
- Around line 245-246: The branch that handles errors in content.js currently
injects response?.error directly into resultsDiv.innerHTML (the element
referenced as resultsDiv and the markup using class "pmoves-history-item
pmoves-status-failed"), which enables DOM XSS; instead create the error node
using text-safe APIs: build the container element (or set an element's
textContent) and assign sanitized text (or run response.error through an
escapeHtml helper) rather than interpolating into innerHTML; update the error
path that checks chrome.runtime.lastError || response?.error to construct the
DOM safely using resultsDiv.appendChild or element.textContent.- Around line 37-39: The FAB recreation is starting a new timer each time which
causes duplicate getGpuMetrics calls; declare a module-scoped variable (e.g.,
gpuBadgeIntervalId) and use it as the single interval id for updateGpuBadge,
then in createFloatingButton only call setInterval(updateGpuBadge, 30000) if
gpuBadgeIntervalId is not already set (or clear the previous interval with
clearInterval(gpuBadgeIntervalId) before re-creating and reassigning); update
any corresponding teardown logic to clearInterval(gpuBadgeIntervalId) and reset
it to null when the FAB is removed so the timer is created only once and not
duplicated.In
@pmoves/chrome-extension/lib/pmoves-api.js:
- Around line 214-230: synthesizeAudio currently calls fetch directly (bypassing
the shared request helper) so it lacks the client's timeout/abort behavior;
update synthesizeAudio to either call the existing request(...) helper with the
same endpoint and JSON body and return the arrayBuffer result, or add an
AbortController (and accept/forward an opts.signal or set a timeout) and pass
its signal into fetch, ensuring the request is aborted on timeout or when the
caller cancels; reference the synthesizeAudio function, the request(...) helper,
and use AbortController if you choose the fetch route.In
@pmoves/chrome-extension/options/options.css:
- Around line 208-221: The .diag-result CSS rule uses deprecated word-break and
unnecessarily quoted font names; replace word-break: break-word with
overflow-wrap: break-word and update the font stack to remove quotes around
Consolas and Monaco (e.g., font-family: Consolas, Monaco, monospace) while
preserving other properties like max-height and white-space.In
@pmoves/chrome-extension/options/options.js:
- Around line 80-85: The save callback for chrome.runtime.sendMessage currently
always sets "#save-status" to "Saved!"; update the callback in the sendMessage
call so it first checks chrome.runtime.lastError and the response payload (e.g.,
response.error) before updating the DOM: if lastError or response.error exists,
set "#save-status" to an error message (and color red) and do not show "Saved!",
otherwise set "Saved!" (green) and clear it after timeout; reference the
existing chrome.runtime.sendMessage call and the '#save-status' element when
making this change.- Around line 238-244: The code currently builds HTML via shapesDiv.innerHTML
using interpolated values (svgUrl from $('#url-gateway'), s.shape_id, s.label,
s.created_at), creating an XSS risk; replace the innerHTML construction in the
block that iterates r.shapes with DOM creation: for each s create a container
DIV via document.createElement('div'), create and set textContent on a STRONG
node for escape-safe label (use s.label || s.shape_id), create a span and set
its textContent to s.created_at || '', create an A element and set its href
property to the computed URL (built from $('#url-gateway').value but do not
inject into HTML), set link target and textContent, append children to the
container and append to shapesDiv; ensure no user/server data is concatenated
into innerHTML or used as HTML.In
@pmoves/chrome-extension/popup/popup.css:
- Around line 190-202: Replace the deprecated word-break: break-word in the
.action-result rule with overflow-wrap: break-word to achieve the same behavior;
locate the .action-result CSS block and remove or replace the word-break
property, adding overflow-wrap: break-word (and optionally keep word-break:
break-all only if truly needed for legacy behavior) so the rule uses modern,
supported syntax.In
@pmoves/chrome-extension/popup/popup.js:
- Around line 44-52: The click handler for '#gpu-optimize-btn' currently treats
any resolved value as success; update the async callback that calls msg({
action: 'gpuOptimize' }) to explicitly check result?.error and, if present, call
showResult(result.error || 'Optimization failed') and avoid calling refreshGpu
or showing success text; only on no-error call showResult(result?.message ||
'Optimization complete') and refreshGpu(); ensure the btn.disabled state and
textContent are correctly reset in both the error and success branches (or a
finally-like cleanup) so the button is re-enabled after handling either outcome.In
@pmoves/chrome-extension/test/mock-server.js:
- Around line 209-218: The partial-match fallback is using
req.url.startsWith(...) which matches query-string requests like
"/viz/recent?limit=5" against a broad "GET /" entry; change it to parse the
request URL (new URL(req.url, 'http://localhost')) and compare against
URL.pathname so queries don't hijack "/" routes, and only allow prefix matching
when the registered route path is explicitly dynamic (e.g., contains a dynamic
marker like ":" or "*" or endsWith "/"). Update the matching logic in the block
that computes match (the code referencing handler, routes, match, p, m and
respond) to use pathname equality for static routes and startsWith (or prefix
compare) only for routes with dynamic segments.
Nitpick comments:
In@pmoves/chrome-extension/popup/popup.html:
- Around line 137-140: The footer version is hardcoded; update popup.js to read
the exported VERSION from constants.js and set the element with id
"version-label" accordingly (e.g., assignv${VERSION}to its textContent),
ensuring constants.js is loaded before popup.js so VERSION is available; target
the DOM element by id "version-label" in popup.js and remove the hardcoded
string in popup.html.In
@pmoves/chrome-extension/README.md:
- Around line 1-86: Add an "API Documentation" section to README.md showing
programmatic access examples: document the extension message API (background.js
service worker message routing and content.js injection APIs), and include
concrete usage examples for lib/pmoves-api.js (JavaScript), a Python snippet
calling the exposed backend endpoints (TensorZero 3030, PMOVES.YT 8077, Hi-RAG
8086), and cURL examples for key operations (ingest video, query Hi-RAG, chat
via TensorZero). For each example, show the request shape, required headers/auth
(Agent Zero token, Flute API key), and the corresponding response shape so
developers can integrate programmatically. Ensure the section references
background.js, content.js, and lib/pmoves-api.js so readers can locate the
implementation.- Around line 77-84: Update the fenced code block that lists background.js,
content.js, lib/pmoves-api.js, lib/constants.js, popup/, and options/ in
README.md by adding a language specifier (e.g., ```text) to the opening fence so
the block is rendered and linted correctly; locate the code block containing the
"background.js Service worker: config, health polling, message routing"
entry and change its opening fence to include the language token.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `f5fc1d72-560e-47e8-9d6f-c20ad5dc0989` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between da3eb0da4db9650ea667c748157be8789dd6c2ad and 6210b1a0ac597157da3fe2ca8a9b62e91e4d4d34. </details> <details> <summary>⛔ Files ignored due to path filters (4)</summary> * `pmoves/chrome-extension/icons/icon128.png` is excluded by `!**/*.png` * `pmoves/chrome-extension/icons/icon16.png` is excluded by `!**/*.png` * `pmoves/chrome-extension/icons/icon32.png` is excluded by `!**/*.png` * `pmoves/chrome-extension/icons/icon48.png` is excluded by `!**/*.png` </details> <details> <summary>📒 Files selected for processing (14)</summary> * `pmoves/chrome-extension/README.md` * `pmoves/chrome-extension/background.js` * `pmoves/chrome-extension/content.js` * `pmoves/chrome-extension/lib/constants.js` * `pmoves/chrome-extension/lib/pmoves-api.js` * `pmoves/chrome-extension/manifest.json` * `pmoves/chrome-extension/options/options.css` * `pmoves/chrome-extension/options/options.html` * `pmoves/chrome-extension/options/options.js` * `pmoves/chrome-extension/popup/popup.css` * `pmoves/chrome-extension/popup/popup.html` * `pmoves/chrome-extension/popup/popup.js` * `pmoves/chrome-extension/styles.css` * `pmoves/chrome-extension/test/mock-server.js` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+14
to
+24
| async function loadConfig() { | ||
| const result = await chrome.storage.sync.get(['pmovesConfig']); | ||
| if (result.pmovesConfig) { | ||
| config = { ...config, ...result.pmovesConfig }; | ||
| } | ||
| setServiceUrls(config.services); | ||
| } | ||
|
|
||
| async function saveConfig() { | ||
| await chrome.storage.sync.set({ pmovesConfig: config }); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep auth tokens out of chrome.storage.sync.
pmovesConfig includes auth.agentZeroToken and auth.fluteApiKey, so these lines sync secrets through Chrome Sync to every signed-in browser. Store auth in chrome.storage.local or chrome.storage.session, and sync only non-sensitive settings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/background.js` around lines 14 - 24, The code
currently reads/writes the entire pmovesConfig via chrome.storage.sync (in
loadConfig and saveConfig), which will sync secrets like auth.agentZeroToken and
auth.fluteApiKey; change the logic to separate sensitive auth from non-sensitive
settings: when loading (loadConfig) read both chrome.storage.sync for
non-sensitive config and chrome.storage.local (or chrome.storage.session) for
the auth object, then merge them into the in-memory config and call
setServiceUrls(config.services); when saving (saveConfig) write non-sensitive
fields to chrome.storage.sync and write only the auth sub-object
(auth.agentZeroToken, auth.fluteApiKey) to chrome.storage.local/session so
tokens are not propagated via Chrome Sync. Ensure you reference pmovesConfig and
the auth.* fields when splitting/merging.
Comment on lines
+14
to
+20
| async function loadConfig() { | ||
| const result = await chrome.storage.sync.get(['pmovesConfig']); | ||
| if (result.pmovesConfig) { | ||
| config = { ...config, ...result.pmovesConfig }; | ||
| } | ||
| setServiceUrls(config.services); | ||
| } |
Contributor
There was a problem hiding this comment.
Wait for stored config and merge nested sections deeply.
loadConfig() is fire-and-forget, and both merges replace services/auth/features wholesale. On a cold worker, early getConfig/updateConfig calls can still see defaults, and any partial stored config can drop defaults like showFloatingButton or healthPollInterval. Keep a configReady promise and merge each nested section against DEFAULT_CONFIG.
Also applies to: 26-26, 202-212
Comment on lines
+120
to
+137
| if (processingStatus.has(videoId || url)) { | ||
| notify('Already Processing', 'This video is already being processed.'); | ||
| return { status: 'already_processing' }; | ||
| } | ||
|
|
||
| processingStatus.set(videoId || url, 'queued'); | ||
|
|
||
| try { | ||
| const result = await yt.ingest(url); | ||
| processingStatus.set(videoId || url, 'processing'); | ||
| notify('Video Queued', 'Video sent to PMOVES.YT for ingestion.'); | ||
| storeInHistory(url, videoId); | ||
|
|
||
| chrome.action.setBadgeText({ text: '1' }); | ||
| chrome.action.setBadgeBackgroundColor({ color: BADGE_COLORS.healthy }); | ||
| setTimeout(() => chrome.action.setBadgeText({ text: '' }), 5000); | ||
|
|
||
| return { status: 'queued', result }; |
Contributor
There was a problem hiding this comment.
Don't leave entries stuck in processingStatus forever.
After yt.ingest() succeeds, the key is left at 'processing' indefinitely. Every later retry of the same video or channel URL will return already_processing, and getStatus can never advance past that local sentinel. If there is no async tracker yet, treat this map as “request in flight” and delete the entry once the enqueue call returns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/background.js` around lines 120 - 137,
processingStatus is being left at 'processing' after yt.ingest(), causing future
requests to always see already_processing; modify the flow around yt.ingest(url)
so the entry keyed by (videoId || url) is removed when the enqueue returns
(success or failure). Specifically, after awaiting yt.ingest(url) (and after
storeInHistory/notify logic) call processingStatus.delete(videoId || url), and
also ensure you delete the key in a catch or finally block so getStatus and
retries can progress; touch the processingStatus map, the yt.ingest call, and
getStatus handling to make sure the sentinel is cleaned up.
Comment on lines
+145
to
+156
| function storeInHistory(url, videoId) { | ||
| chrome.storage.local.get(['processingHistory'], (result) => { | ||
| const history = result.processingHistory || []; | ||
| history.unshift({ | ||
| url, | ||
| videoId, | ||
| timestamp: new Date().toISOString(), | ||
| status: 'processing', | ||
| }); | ||
| if (history.length > 100) history.length = 100; | ||
| chrome.storage.local.set({ processingHistory: history }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Serialize processingHistory writes.
This read-modify-write on chrome.storage.local is racy. processAllVideos() can enqueue many requests back-to-back, and overlapping callbacks can all read the same old array and overwrite each other's entries. Queue these updates through a single promise/mutex or update an in-memory cache before flushing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/background.js` around lines 145 - 156, The
storeInHistory function performs a racy read-modify-write against
chrome.storage.local; serialize updates by introducing a single updater (e.g., a
promise queue or mutex) or an in-memory cache to coalesce writes: keep a
module-level processingHistoryCache array that storeInHistory updates
synchronously (unshift/truncate) and then flushes to chrome.storage.local via a
single pendingWritePromise to ensure only one chrome.storage.local.set runs at a
time; ensure processAllVideos calls that enqueue updates go through this same
updater so overlapping calls cannot overwrite each other.
Comment on lines
+80
to
+85
| chrome.runtime.sendMessage({ action: 'updateConfig', config }, () => { | ||
| const status = $('#save-status'); | ||
| status.textContent = 'Saved!'; | ||
| status.style.color = '#4CAF50'; | ||
| setTimeout(() => { status.textContent = ''; }, 2000); | ||
| }); |
Contributor
There was a problem hiding this comment.
Only show “Saved!” after the background acknowledges success.
This callback ignores chrome.runtime.lastError and any { error } payload from updateConfig, so failed saves are reported as successful. Check the response before updating #save-status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/options/options.js` around lines 80 - 85, The save
callback for chrome.runtime.sendMessage currently always sets "#save-status" to
"Saved!"; update the callback in the sendMessage call so it first checks
chrome.runtime.lastError and the response payload (e.g., response.error) before
updating the DOM: if lastError or response.error exists, set "#save-status" to
an error message (and color red) and do not show "Saved!", otherwise set
"Saved!" (green) and clear it after timeout; reference the existing
chrome.runtime.sendMessage call and the '#save-status' element when making this
change.
Comment on lines
+238
to
+244
| shapesDiv.innerHTML = r.shapes.map(s => { | ||
| const svgUrl = `${$('#url-gateway')?.value || 'http://localhost:8085'}/viz/shape/${encodeURIComponent(s.shape_id)}.svg`; | ||
| return `<div style="margin:4px 0;font-size:13px;"> | ||
| <strong>${escapeHtml(s.label || s.shape_id)}</strong> | ||
| <span style="color:#888;margin-left:8px;">${s.created_at || ''}</span> | ||
| <a href="${svgUrl}" target="_blank" style="margin-left:8px;color:#667eea;">View SVG</a> | ||
| </div>`; |
Contributor
There was a problem hiding this comment.
Don't interpolate user/server data into innerHTML on the options page.
Line 239 builds svgUrl from the editable gateway URL, and Line 242 inserts s.created_at from the backend, but both are written raw into innerHTML. In a privileged extension page that's an XSS sink. Build these rows with createElement(), use textContent for text nodes, and assign a.href directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/options/options.js` around lines 238 - 244, The code
currently builds HTML via shapesDiv.innerHTML using interpolated values (svgUrl
from $('#url-gateway'), s.shape_id, s.label, s.created_at), creating an XSS
risk; replace the innerHTML construction in the block that iterates r.shapes
with DOM creation: for each s create a container DIV via
document.createElement('div'), create and set textContent on a STRONG node for
escape-safe label (use s.label || s.shape_id), create a span and set its
textContent to s.created_at || '', create an A element and set its href property
to the computed URL (built from $('#url-gateway').value but do not inject into
HTML), set link target and textContent, append children to the container and
append to shapesDiv; ensure no user/server data is concatenated into innerHTML
or used as HTML.
Comment on lines
+190
to
+202
| .action-result { | ||
| margin-top: 8px; | ||
| padding: 8px 10px; | ||
| background: var(--surface); | ||
| border-radius: 6px; | ||
| font-size: 11px; | ||
| line-height: 1.5; | ||
| white-space: pre-wrap; | ||
| word-break: break-word; | ||
| max-height: 120px; | ||
| overflow-y: auto; | ||
| color: var(--text-dim); | ||
| } |
Contributor
There was a problem hiding this comment.
Replace deprecated word-break: break-word with overflow-wrap: break-word.
The break-word keyword for word-break is deprecated. Use overflow-wrap: break-word instead for the same behavior with better browser support.
🔧 Proposed fix
.action-result {
margin-top: 8px;
padding: 8px 10px;
background: var(--surface);
border-radius: 6px;
font-size: 11px;
line-height: 1.5;
white-space: pre-wrap;
- word-break: break-word;
+ overflow-wrap: break-word;
max-height: 120px;
overflow-y: auto;
color: var(--text-dim);
}📝 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
| .action-result { | |
| margin-top: 8px; | |
| padding: 8px 10px; | |
| background: var(--surface); | |
| border-radius: 6px; | |
| font-size: 11px; | |
| line-height: 1.5; | |
| white-space: pre-wrap; | |
| word-break: break-word; | |
| max-height: 120px; | |
| overflow-y: auto; | |
| color: var(--text-dim); | |
| } | |
| .action-result { | |
| margin-top: 8px; | |
| padding: 8px 10px; | |
| background: var(--surface); | |
| border-radius: 6px; | |
| font-size: 11px; | |
| line-height: 1.5; | |
| white-space: pre-wrap; | |
| overflow-wrap: break-word; | |
| max-height: 120px; | |
| overflow-y: auto; | |
| color: var(--text-dim); | |
| } |
🧰 Tools
🪛 Stylelint (17.4.0)
[error] 198-198: Unexpected deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/popup/popup.css` around lines 190 - 202, Replace the
deprecated word-break: break-word in the .action-result rule with overflow-wrap:
break-word to achieve the same behavior; locate the .action-result CSS block and
remove or replace the word-break property, adding overflow-wrap: break-word (and
optionally keep word-break: break-all only if truly needed for legacy behavior)
so the rule uses modern, supported syntax.
Comment on lines
+44
to
+52
| $('#gpu-optimize-btn').addEventListener('click', async () => { | ||
| const btn = $('#gpu-optimize-btn'); | ||
| btn.disabled = true; | ||
| btn.textContent = 'Optimizing...'; | ||
| try { | ||
| const result = await msg({ action: 'gpuOptimize' }); | ||
| showResult(result?.message || 'Optimization complete'); | ||
| refreshGpu(); | ||
| } catch (e) { |
Contributor
There was a problem hiding this comment.
Check result.error before reporting GPU optimization success.
msg() resolves background/runtime failures as { error }; it does not reject. This handler currently falls through to “Optimization complete” and refreshes the panel even when gpuOptimize failed. Branch on result?.error before the success path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/popup/popup.js` around lines 44 - 52, The click
handler for '#gpu-optimize-btn' currently treats any resolved value as success;
update the async callback that calls msg({ action: 'gpuOptimize' }) to
explicitly check result?.error and, if present, call showResult(result.error ||
'Optimization failed') and avoid calling refreshGpu or showing success text;
only on no-error call showResult(result?.message || 'Optimization complete') and
refreshGpu(); ensure the btn.disabled state and textContent are correctly reset
in both the error and success branches (or a finally-like cleanup) so the button
is re-enabled after handling either outcome.
Comment on lines
+209
to
+218
| if (!handler) { | ||
| // Try partial match (for paths with dynamic segments) | ||
| const match = Object.keys(routes).find(k => { | ||
| const [m, p] = k.split(' '); | ||
| return m === req.method && req.url.startsWith(p); | ||
| }); | ||
| if (match) { | ||
| respond(res, routes[match], req); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't let GET / shadow the gateway's query-string routes.
The partial fallback on Line 213 uses startsWith(), so on port 8085 requests like /viz/recent?limit=5, /events/recent?limit=20, or /viz/shape/...svg can match the first GET / entry and return the gateway homepage HTML. That makes the mock server hide missing route coverage instead of validating the popup/options flows. Match on URL.pathname and only use prefix matching for routes that are explicitly dynamic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/test/mock-server.js` around lines 209 - 218, The
partial-match fallback is using req.url.startsWith(...) which matches
query-string requests like "/viz/recent?limit=5" against a broad "GET /" entry;
change it to parse the request URL (new URL(req.url, 'http://localhost')) and
compare against URL.pathname so queries don't hijack "/" routes, and only allow
prefix matching when the registered route path is explicitly dynamic (e.g.,
contains a dynamic marker like ":" or "*" or endsWith "/"). Update the matching
logic in the block that computes match (the code referencing handler, routes,
match, p, m and respond) to use pathname equality for static routes and
startsWith (or prefix compare) only for routes with dynamic segments.
POWERFULMOVES
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
.claude/context/chrome-extension.md (1)
22-36: Consider adding a language identifier to the fenced code block.The ASCII architecture diagram lacks a language specifier, triggering markdownlint MD040. While this is acceptable for ASCII art, you could use an empty language or
textto silence the warning.📝 Proposed fix
-``` +```text YouTube Page Extension PMOVES.AI Services🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/context/chrome-extension.md around lines 22 - 36, Add a language identifier to the fenced code block containing the ASCII architecture diagram (in .claude/context/chrome-extension.md) to silence markdownlint MD040; change the opening triple backticks to include either an explicit "text" (recommended) or an empty language tag so the block becomes ```text (or ```), leaving the ASCII art content unchanged.pmoves/chrome-extension/help/help.css (1)
177-183: Remove unnecessary quotes around font family names.Stylelint flags that
"Consolas"and"Monaco"don't require quotes since they contain no spaces or special characters. Quotes are only required for font names with spaces (e.g.,"Segoe UI").🔧 Proposed fix
code { padding: 1px 5px; border-radius: 4px; background: var(--surface2); font-size: 12px; - font-family: 'Consolas', 'Monaco', monospace; + font-family: Consolas, Monaco, monospace; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/chrome-extension/help/help.css` around lines 177 - 183, In the CSS rule for the code selector, remove the unnecessary single quotes around the font family names so Stylelint won't flag them; update the font-family declaration in the code { ... } block (the rule containing padding, border-radius, background, font-size, font-family) to use unquoted Consolas and Monaco followed by monospace (e.g., font-family: Consolas, Monaco, monospace;).pmoves/chrome-extension/README.md (2)
77-84: Add language identifier to fenced code block.The architecture code block lacks a language specifier, triggering markdownlint MD040. Use
textfor plain text diagrams.📝 Proposed fix
-``` +```text background.js Service worker: config, health polling, message routing content.js YouTube page injection: buttons, menus, overlays lib/pmoves-api.js Shared API client for all 8 PMOVES.AI services lib/constants.js Default config, service URLs, health endpoints popup/ Dashboard: health grid, GPU panel, quick actions options/ Settings: endpoints, auth, features, diagnostics</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@pmoves/chrome-extension/README.mdaround lines 77 - 84, The fenced code
block showing the architecture list (lines containing "background.js",
"content.js", "lib/pmoves-api.js", etc.) in README.md lacks a language
identifier and triggers markdownlint MD040; update the opening fence to include
a language specifier (use "text") so the block becomes a fenced "text" code
block, e.g., replace the triple backticks that start the block with ```text
while leaving the content and closing backticks unchanged.</details> --- `88-96`: **Consider adding programmatic API examples.** The README references the full API documentation but doesn't include inline examples. As per coding guidelines for README files: include "API documentation for programmatic access (Javascript, Python, Curl)". Since this is a Chrome extension (not a standalone API), this guideline may be partially satisfied by the reference to `chrome-extension.md`. However, you could add a brief example of the message protocol for developers extending the extension. Example addition: ```javascript // Send message to background service worker chrome.runtime.sendMessage({ action: 'processVideo', url: 'https://youtube.com/watch?v=...' }); ``` As per coding guidelines: `**/README.md`: "ALWAYS write documentation in README.md... API documentation for programmatic access (Javascript, Python, Curl)" <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@pmoves/chrome-extension/README.md` around lines 88 - 96, Add brief programmatic examples to the README showing how to call the extension's background API (e.g., using chrome.runtime.sendMessage) and reference key actions handled by lib/pmoves-api.js (such as processVideo, queryKnowledge, startTTS, manageGPU); include one-line JavaScript, one-line cURL-like pseudo-example, and a short Python snippet illustrating the message payload shape (action + params) and expected response shape so developers know how to invoke and extend functions like processVideo and queryKnowledge programmatically. ``` </details> </blockquote></details> <details> <summary>pmoves/chrome-extension/help/help.html (1)</summary><blockquote> `144-147`: **Markdown file link may not render properly in browser.** The link to `../docs/USER_GUIDE.md` will open the raw markdown file when clicked (due to `target="_blank"`), which won't render formatted in most browsers. Consider either converting the user guide to HTML or noting this behavior. This is acceptable if users are expected to read the markdown in a code editor or if GitHub/IDE rendering is intended. Otherwise, you could add a note that the file is best viewed in a markdown-capable viewer. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@pmoves/chrome-extension/help/help.html` around lines 144 - 147, The anchor in the .full-guide div points to ../docs/USER_GUIDE.md and will open raw Markdown in the browser (target="_blank"); update the link or UI so users get a readable guide—either (a) point the href to an HTML-rendered version (or the repository's rendered URL) instead of the raw .md, or (b) keep the .md but add clarifying text next to the link (in the .full-guide paragraph) that the file is raw Markdown and should be viewed in a Markdown-capable viewer; modify the <a> element and surrounding text in help.html accordingly. ``` </details> </blockquote></details> <details> <summary>pmoves/chrome-extension/popup/popup.html (1)</summary><blockquote> `89-103`: **Consider adding accessible labels for form inputs.** The input fields have `placeholder` attributes but lack associated `<label>` elements or `aria-label` attributes. Screen readers may not properly announce the purpose of these fields. Consider adding visually-hidden labels or `aria-label` attributes. ```diff <div class="action-row"> - <input type="text" id="ingest-url" placeholder="YouTube URL to ingest..."> + <input type="text" id="ingest-url" placeholder="YouTube URL to ingest..." aria-label="YouTube URL to ingest"> <button id="ingest-btn" class="action-btn">Ingest</button> </div> ``` This applies to all input fields in the popup. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@pmoves/chrome-extension/popup/popup.html` around lines 89 - 103, Add accessible labels for each input (ingest-url, rag-query, chat-input, tts-input) by either adding a <label for="..."> element (visually hidden via a utility class) associated with each input, or by adding descriptive aria-label attributes (e.g., aria-label="YouTube URL to ingest"). Ensure the label text clearly matches the placeholder intent and keep labels unique per input so screen readers announce each field correctly. ``` </details> </blockquote></details> <details> <summary>pmoves/chrome-extension/content.js (1)</summary><blockquote> `389-394`: **Remove unused conditional block.** This block does nothing useful. The `if` branch has a comment but no code, and the `else` branch adds an empty `DOMContentLoaded` listener. Initialization is already handled via the `sendMessage` callback at line 14, so this code can be removed. <details> <summary>♻️ Proposed fix</summary> ```diff - // If config loaded synchronously (cached), init immediately - if (document.readyState !== 'loading') { - // Config will init via callback above - } else { - document.addEventListener('DOMContentLoaded', () => {}); - } })(); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@pmoves/chrome-extension/content.js` around lines 389 - 394, The conditional checking document.readyState and the empty DOMContentLoaded listener are dead code; remove the entire if/else block that references document.readyState and the empty document.addEventListener('DOMContentLoaded', ...) so initialization relies solely on the existing sendMessage callback (the config init flow referenced near the sendMessage callback). Ensure no other logic depends on that listener and delete both the comment-only if branch and the empty listener in content.js to keep the code clean. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@pmoves/chrome-extension/content.js:
- Around line 344-349: enableEscClose currently registers a keydown handler
(onKey) that is only removed when Escape is pressed, leaking the listener if the
modal is closed by other means; change enableEscClose(modalEl) to return a
cleanup function that removes the same onKey listener from document (and make
cleanup idempotent if you want), then update modal close button handlers to call
that returned cleanup() before calling modalEl.remove() so the keydown listener
is always detached regardless of how the modal is closed.In
@pmoves/chrome-extension/popup/popup.js:
- Around line 219-225: The interpolation of item.status directly into the class
attribute in the list.innerHTML assignment can allow attribute injection; change
it to a safe value by escaping or sanitizing item.status before embedding (e.g.,
use the existing escapeHtml(item.status) or a sanitizeClassName helper that
replaces non-alphanumeric characters with safe hyphens), or build the DOM nodes
with document.createElement and set classList.add(safeStatus) instead of using
innerHTML; update the code that sets class="activity-status ${item.status}" to
use the sanitized value (reference: item.status, the list.innerHTML mapping, and
the activity-status class).- Around line 284-292: The timeAgo function doesn't handle
null/undefined/invalid timestamps: new Date(ts).getTime() can be NaN and produce
"NaNd ago". Inside timeAgo, validate ts by creating a Date object (e.g., const d
= new Date(ts)) and check isFinite(d.getTime()) (or isNaN) before computing
diff; if invalid, return a safe string like 'unknown' (or 'just now') instead of
proceeding. Keep existing variables (diff, mins, hrs) and only compute them
after the validity check so invalid dates are handled gracefully.
Nitpick comments:
In @.claude/context/chrome-extension.md:
- Around line 22-36: Add a language identifier to the fenced code block
containing the ASCII architecture diagram (in
.claude/context/chrome-extension.md) to silence markdownlint MD040; change the
opening triple backticks to include either an explicit "text" (recommended) or
an empty language tag so the block becomestext (or), leaving the ASCII
art content unchanged.In
@pmoves/chrome-extension/content.js:
- Around line 389-394: The conditional checking document.readyState and the
empty DOMContentLoaded listener are dead code; remove the entire if/else block
that references document.readyState and the empty
document.addEventListener('DOMContentLoaded', ...) so initialization relies
solely on the existing sendMessage callback (the config init flow referenced
near the sendMessage callback). Ensure no other logic depends on that listener
and delete both the comment-only if branch and the empty listener in content.js
to keep the code clean.In
@pmoves/chrome-extension/help/help.css:
- Around line 177-183: In the CSS rule for the code selector, remove the
unnecessary single quotes around the font family names so Stylelint won't flag
them; update the font-family declaration in the code { ... } block (the rule
containing padding, border-radius, background, font-size, font-family) to use
unquoted Consolas and Monaco followed by monospace (e.g., font-family: Consolas,
Monaco, monospace;).In
@pmoves/chrome-extension/help/help.html:
- Around line 144-147: The anchor in the .full-guide div points to
../docs/USER_GUIDE.md and will open raw Markdown in the browser
(target="_blank"); update the link or UI so users get a readable guide—either
(a) point the href to an HTML-rendered version (or the repository's rendered
URL) instead of the raw .md, or (b) keep the .md but add clarifying text next to
the link (in the .full-guide paragraph) that the file is raw Markdown and should
be viewed in a Markdown-capable viewer; modify the element and surrounding
text in help.html accordingly.In
@pmoves/chrome-extension/popup/popup.html:
- Around line 89-103: Add accessible labels for each input (ingest-url,
rag-query, chat-input, tts-input) by either adding a element
(visually hidden via a utility class) associated with each input, or by adding
descriptive aria-label attributes (e.g., aria-label="YouTube URL to ingest").
Ensure the label text clearly matches the placeholder intent and keep labels
unique per input so screen readers announce each field correctly.In
@pmoves/chrome-extension/README.md:
- Around line 77-84: The fenced code block showing the architecture list (lines
containing "background.js", "content.js", "lib/pmoves-api.js", etc.) in
README.md lacks a language identifier and triggers markdownlint MD040; update
the opening fence to include a language specifier (use "text") so the block
becomes a fenced "text" code block, e.g., replace the triple backticks that
start the block with ```text while leaving the content and closing backticks
unchanged.- Around line 88-96: Add brief programmatic examples to the README showing how
to call the extension's background API (e.g., using chrome.runtime.sendMessage)
and reference key actions handled by lib/pmoves-api.js (such as processVideo,
queryKnowledge, startTTS, manageGPU); include one-line JavaScript, one-line
cURL-like pseudo-example, and a short Python snippet illustrating the message
payload shape (action + params) and expected response shape so developers know
how to invoke and extend functions like processVideo and queryKnowledge
programmatically.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `89310eec-7de9-4cbd-a98b-335a25b00f04` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6210b1a0ac597157da3fe2ca8a9b62e91e4d4d34 and 0c1593c08b4af0f0108dd2dd7734ab92437fdd4d. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `.claude/CLAUDE.md` * `.claude/context/chrome-extension.md` * `.claude/context/documentation-index.md` * `pmoves/chrome-extension/README.md` * `pmoves/chrome-extension/content.js` * `pmoves/chrome-extension/docs/USER_GUIDE.md` * `pmoves/chrome-extension/help/help.css` * `pmoves/chrome-extension/help/help.html` * `pmoves/chrome-extension/options/options.html` * `pmoves/chrome-extension/popup/popup.html` * `pmoves/chrome-extension/popup/popup.js` * `pmoves/chrome-extension/styles.css` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * pmoves/chrome-extension/styles.css </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+344
to
+349
| function enableEscClose(modalEl) { | ||
| function onKey(e) { | ||
| if (e.key === 'Escape') { modalEl.remove(); document.removeEventListener('keydown', onKey); } | ||
| } | ||
| document.addEventListener('keydown', onKey); | ||
| } |
Contributor
There was a problem hiding this comment.
Keydown listener leaks when modal is closed via button.
enableEscClose adds a keydown listener that's only removed when Escape is pressed. If the modal is closed via the Close button, the listener remains attached to document, causing a memory leak over multiple modal opens.
🐛 Proposed fix - return cleanup function
function enableEscClose(modalEl) {
function onKey(e) {
if (e.key === 'Escape') { modalEl.remove(); document.removeEventListener('keydown', onKey); }
}
document.addEventListener('keydown', onKey);
+ return () => document.removeEventListener('keydown', onKey);
}Then in each modal close button handler:
const cleanup = enableEscClose(modal);
modal.querySelector('.pmoves-close-button').addEventListener('click', () => {
cleanup();
modal.remove();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/content.js` around lines 344 - 349, enableEscClose
currently registers a keydown handler (onKey) that is only removed when Escape
is pressed, leaking the listener if the modal is closed by other means; change
enableEscClose(modalEl) to return a cleanup function that removes the same onKey
listener from document (and make cleanup idempotent if you want), then update
modal close button handlers to call that returned cleanup() before calling
modalEl.remove() so the keydown listener is always detached regardless of how
the modal is closed.
Comment on lines
+219
to
+225
| list.innerHTML = history.slice(0, 5).map(item => ` | ||
| <div class="activity-item"> | ||
| <span class="activity-id">${escapeHtml(item.videoId || 'URL')}</span> | ||
| <span class="activity-status ${item.status}">${item.status}</span> | ||
| <span class="activity-time">${timeAgo(item.timestamp)}</span> | ||
| </div> | ||
| `).join(''); |
Contributor
There was a problem hiding this comment.
Escape item.status in class attribute to prevent attribute injection.
While item.status likely comes from controlled backend values, it's interpolated directly into a class attribute without escaping. If status ever contains quotes or special characters, it could break the HTML or enable attribute injection.
🛡️ Proposed fix
list.innerHTML = history.slice(0, 5).map(item => `
<div class="activity-item">
<span class="activity-id">${escapeHtml(item.videoId || 'URL')}</span>
- <span class="activity-status ${item.status}">${item.status}</span>
+ <span class="activity-status ${escapeHtml(item.status)}">${escapeHtml(item.status)}</span>
<span class="activity-time">${timeAgo(item.timestamp)}</span>
</div>
`).join('');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/popup/popup.js` around lines 219 - 225, The
interpolation of item.status directly into the class attribute in the
list.innerHTML assignment can allow attribute injection; change it to a safe
value by escaping or sanitizing item.status before embedding (e.g., use the
existing escapeHtml(item.status) or a sanitizeClassName helper that replaces
non-alphanumeric characters with safe hyphens), or build the DOM nodes with
document.createElement and set classList.add(safeStatus) instead of using
innerHTML; update the code that sets class="activity-status ${item.status}" to
use the sanitized value (reference: item.status, the list.innerHTML mapping, and
the activity-status class).
Comment on lines
+284
to
+292
| function timeAgo(ts) { | ||
| const diff = Date.now() - new Date(ts).getTime(); | ||
| const mins = Math.floor(diff / 60000); | ||
| if (mins < 1) return 'just now'; | ||
| if (mins < 60) return `${mins}m ago`; | ||
| const hrs = Math.floor(mins / 60); | ||
| if (hrs < 24) return `${hrs}h ago`; | ||
| return `${Math.floor(hrs / 24)}d ago`; | ||
| } |
Contributor
There was a problem hiding this comment.
Handle invalid timestamps gracefully.
If ts is null, undefined, or an invalid date string, new Date(ts).getTime() returns NaN, causing all comparisons to fail and returning NaNd ago.
🛡️ Proposed fix
function timeAgo(ts) {
+ if (!ts) return '--';
const diff = Date.now() - new Date(ts).getTime();
+ if (isNaN(diff)) return '--';
const mins = Math.floor(diff / 60000);📝 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
| function timeAgo(ts) { | |
| const diff = Date.now() - new Date(ts).getTime(); | |
| const mins = Math.floor(diff / 60000); | |
| if (mins < 1) return 'just now'; | |
| if (mins < 60) return `${mins}m ago`; | |
| const hrs = Math.floor(mins / 60); | |
| if (hrs < 24) return `${hrs}h ago`; | |
| return `${Math.floor(hrs / 24)}d ago`; | |
| } | |
| function timeAgo(ts) { | |
| if (!ts) return '--'; | |
| const diff = Date.now() - new Date(ts).getTime(); | |
| if (isNaN(diff)) return '--'; | |
| const mins = Math.floor(diff / 60000); | |
| if (mins < 1) return 'just now'; | |
| if (mins < 60) return `${mins}m ago`; | |
| const hrs = Math.floor(mins / 60); | |
| if (hrs < 24) return `${hrs}h ago`; | |
| return `${Math.floor(hrs / 24)}d ago`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/chrome-extension/popup/popup.js` around lines 284 - 292, The timeAgo
function doesn't handle null/undefined/invalid timestamps: new
Date(ts).getTime() can be NaN and produce "NaNd ago". Inside timeAgo, validate
ts by creating a Date object (e.g., const d = new Date(ts)) and check
isFinite(d.getTime()) (or isNaN) before computing diff; if invalid, return a
safe string like 'unknown' (or 'just now') instead of proceeding. Keep existing
variables (diff, mins, hrs) and only compute them after the validity check so
invalid dates are handled gracefully.
MV3 Chrome extension foundation for PMOVES.AI service integration. API client covers 8 services with Agent Zero methods aligned to actual main.py endpoints (/healthz, /mcp/commands, /mcp/execute, /tasks, /jobs, /sessions, /events/publish). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Service worker handles config, health-poll alarm, and message routing. No task-polling alarms — Agent Zero /tasks is synchronous. Content script injects floating action button and thumbnail buttons on YouTube pages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Popup: health grid, GPU panel, quick actions (ingest, RAG, chat, TTS), synchronous agent task submission, CHIT pipeline with shape viz. Options: service endpoints, auth tokens, feature toggles, diagnostics (Agent Zero Health + MCP Commands buttons). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8-service mock server with Agent Zero routes matching actual API. README documents features, installation, service endpoints, and architecture. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix GPU badge interval stacking, extract attachThumbnailButton helper, add sidebar renderer hover selectors, enableEscClose utility, XSS fix in search results, and toast stacking limit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add chrome-extension.md with full API, message protocol, auth, and service integration reference (327 lines). Update CLAUDE.md and documentation index with cross-references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add standalone USER_GUIDE.md, in-extension help page (help.html/css), help button in popup and options page, and expand README with troubleshooting, FAQ, and integration overview sections. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move auth credentials from chrome.storage.sync to session (Fix 1) - Replace innerHTML with createElement in options shapes display (Fix 2) - Add method allowlist and pathname parsing in mock-server routes (Fix 3+4) - Add AbortController timeout to synthesizeAudio fetch (Fix 5) - Clean up processingStatus after 5min to prevent stuck state (Fix 6) - Add configReady promise to prevent stale config reads (Fix 7) - Serialize history storage writes with promise queue (Fix 8) - Add CSP to manifest, check error in GPU optimize result (Fix 9) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
POWERFULMOVES
force-pushed
the
feat/chrome-extension-agent-zero-api
branch
from
March 7, 2026 17:19
0c1593c to
520a352
Compare
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
3 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.
Summary
attachThumbnailButtonextraction, sidebar renderer hover selectors,enableEscCloseutility, XSS fix in search results, toast stacking limitchrome-extension.mdwith full API, message protocol, auth, and service mapping)USER_GUIDE.md), in-extension help page, help button in popup/options, README expansion with troubleshooting & FAQCommits
fix(chrome-ext): improve content script reliability— content.js + styles.cssdocs(chrome-ext): add developer integration reference— .claude/context/ docsdocs(chrome-ext): add user guide, help page, and README expansion— user-facing docs + popup/options changesTest plan
USER_GUIDE.mdrenders correctly on GitHubchrome-extension.mdcross-references are valid in CLAUDE.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests