fix: prevent popup crash by ensuring DOM is loaded before accessing elements#81
fix: prevent popup crash by ensuring DOM is loaded before accessing elements#81Khushi5623 wants to merge 6 commits into
Conversation
…d making it more reliable
|
@Khushi5623 Contains conflict, also the audit is failing |
📝 WalkthroughWalkthroughThe popup script now initializes only after DOM content loads, implements disabled-state management for the scan submit button across all result paths, hardens the string escape helper to safely handle null/undefined values, and refactors the scan function from permission-checking to direct extension management queries with improved error reporting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
area: extension
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension/src/popup.js`:
- Around line 191-193: The handler handleScanUrlSubmit is not guarded against
re-entry so Enter-key calls can start overlapping scans; add a shared lock
(e.g., a module-level boolean isScanning) or an early-return guard at the top of
handleScanUrlSubmit that returns immediately when scanning is true, set
isScanning = true when a scan begins and set it back to false in all
completion/error paths, ensure the existing scanUrlSubmit.disabled = true/false
code is kept in sync with isScanning so both button clicks and Enter-key
invocations use the same lock.
- Around line 676-706: The initial scan should be gated by the optional
"management" permission: inside scan(force) before calling
chrome.management.getAll, call chrome.permissions.contains({permissions:
['management']}, callback) and if the permission is not granted hideStatus() and
render the permission-request state (reuse the same UI/handler used by the
existing permission button flow that requests management) instead of calling
chrome.management.getAll and showing "Cannot access extensions"; if
permissions.contains reports granted, proceed to call chrome.management.getAll
and then runScanWithExtensions(exts, force); keep the existing try/catch for
unexpected errors.
🪄 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: dfbdabae-7768-4cb3-aaab-933836159ab1
📒 Files selected for processing (1)
packages/extension/src/popup.js
| if (scanUrlSubmit) { | ||
| scanUrlSubmit.disabled = true; | ||
| } |
There was a problem hiding this comment.
Guard handleScanUrlSubmit() itself against re-entry.
Disabling the button only blocks pointer clicks. The Enter-key handler still calls handleScanUrlSubmit() directly at Lines 315-318, so users can start overlapping scans while the first one is still in flight. Add an early return at the top of the handler (or a dedicated isScanning flag) so every entry path shares the same lock.
Suggested fix
function handleScanUrlSubmit() {
+ if (scanUrlSubmit && scanUrlSubmit.disabled) return;
var raw = scanUrlInput && scanUrlInput.value ? scanUrlInput.value.trim() : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/extension/src/popup.js` around lines 191 - 193, The handler
handleScanUrlSubmit is not guarded against re-entry so Enter-key calls can start
overlapping scans; add a shared lock (e.g., a module-level boolean isScanning)
or an early-return guard at the top of handleScanUrlSubmit that returns
immediately when scanning is true, set isScanning = true when a scan begins and
set it back to false in all completion/error paths, ensure the existing
scanUrlSubmit.disabled = true/false code is kept in sync with isScanning so both
button clicks and Enter-key invocations use the same lock.
| function scan(force) { | ||
| showStatus('Getting extensions…'); | ||
|
|
||
| try { | ||
| chrome.management.getAll(function (all) { | ||
| if (chrome.runtime.lastError || !all) { | ||
| hideStatus(); | ||
| showError('Cannot access extensions'); | ||
| return; | ||
| } | ||
|
|
||
| showStatus('Getting extensions…'); | ||
| var selfId = chrome.runtime.id; | ||
| var exts = []; | ||
|
|
||
| chrome.runtime.sendMessage({ action: 'getAllExtensions' }, function (all) { | ||
| if (chrome.runtime.lastError || !all) { | ||
| chrome.management.getAll(function (fallbackAll) { | ||
| if (chrome.runtime.lastError) { | ||
| hideStatus(); | ||
| showError('Cannot access extensions: ' + (chrome.runtime.lastError.message || 'unknown')); | ||
| return; | ||
| } | ||
| var selfId = chrome.runtime.id; | ||
| var filtered = []; | ||
| for (var j = 0; j < fallbackAll.length; j++) { | ||
| if (!fallbackAll[j].permissions) fallbackAll[j].permissions = []; | ||
| if (fallbackAll[j].type === 'extension' && fallbackAll[j].id !== selfId && fallbackAll[j].enabled) filtered.push(fallbackAll[j]); | ||
| } | ||
| runScanWithExtensions(filtered, force); | ||
| }); | ||
| return; | ||
| for (var i = 0; i < all.length; i++) { | ||
| if ( | ||
| all[i].type === 'extension' && | ||
| all[i].id !== selfId && | ||
| all[i].enabled | ||
| ) { | ||
| exts.push(all[i]); | ||
| } | ||
| } | ||
|
|
||
| var exts = []; | ||
| for (var i = 0; i < all.length; i++) { | ||
| if (!all[i].permissions) all[i].permissions = []; | ||
| if (all[i].enabled) exts.push(all[i]); | ||
| } | ||
| runScanWithExtensions(exts, force); | ||
| }); | ||
| runScanWithExtensions(exts, force); | ||
| }); | ||
| } catch (e) { | ||
| hideStatus(); | ||
| showError('Unexpected error while scanning'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== manifest permission model =="
sed -n '1,40p' packages/extension/src/manifest.json
echo
echo "== popup startup + management API usage =="
rg -n -C3 'scan\(false\)|chrome\.management\.getAll|chrome\.permissions\.(contains|request)' \
packages/extension/src/popup.js \
packages/extension/src/background.jsRepository: Stanzin7/ExtensionShield
Length of output: 3415
🏁 Script executed:
sed -n '65,80p' packages/extension/src/popup.jsRepository: Stanzin7/ExtensionShield
Length of output: 573
Gate the initial scan on the optional management permission.
scan(false) runs unconditionally on popup load (line 792), but packages/extension/src/manifest.json declares "management" under optional_permissions, not required permissions. On a fresh install, chrome.management.getAll() at line 680 will fail and the popup opens directly to "Cannot access extensions" instead of prompting for permission. The button handler at lines 70–76 correctly checks chrome.permissions.contains() before requesting the permission, but the startup scan bypasses this entirely. Apply the same permission-gating pattern to the initial scan(false) call and render a permission-request state instead of treating missing permission as a scan error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/extension/src/popup.js` around lines 676 - 706, The initial scan
should be gated by the optional "management" permission: inside scan(force)
before calling chrome.management.getAll, call
chrome.permissions.contains({permissions: ['management']}, callback) and if the
permission is not granted hideStatus() and render the permission-request state
(reuse the same UI/handler used by the existing permission button flow that
requests management) instead of calling chrome.management.getAll and showing
"Cannot access extensions"; if permissions.contains reports granted, proceed to
call chrome.management.getAll and then runScanWithExtensions(exts, force); keep
the existing try/catch for unexpected errors.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
2 participants
🛠 Fix: Prevent Popup Crash by Ensuring DOM Readiness
🚨 Issue
The popup script was accessing DOM elements before the document was fully loaded, which could lead to null references and potential crashes.
✅ Changes Made
DOMContentLoadedevent listener🎯 Impact
This fix aligns with best practices for Chrome Extension development and improves overall reliability.
Summary by CodeRabbit
New Features
Bug Fixes