fix: call sidePanel.open() synchronously to fix context menu side panel#963
fix: call sidePanel.open() synchronously to fix context menu side panel#963octo-patch wants to merge 6 commits intoChatGPTBox-dev:masterfrom
Conversation
…Box-dev#935) Anthropic now issues session keys with the prefix sk-ant-sid02- in addition to the previous sk-ant-sid01- format. The validation was too strict, rejecting valid new-format keys with an error. Relaxed the check to match any sk-ant-sid prefix to be forward-compatible with future key format versions.
…el (fixes ChatGPTBox-dev#857) Chrome's sidePanel.open() must be called synchronously within a user gesture handler. Previously, the call was inside a Browser.tabs.query() Promise callback, which breaks the user gesture chain and causes: Error: sidePanel.open() may only be called in response to a user gesture. Fixed by extracting the itemId before the async query and handling the openSidePanel action synchronously at the top of the event handler, before any async operations.
Review Summary by QodoFix side panel context menu and Claude session key validation
WalkthroughsDescription• Fix sidePanel.open() call to execute synchronously within user gesture context • Relax Claude session key validation to accept sk-ant-sid02 format • Extract menuItemId before async operations to preserve gesture chain • Update error message to reflect both session key formats Diagramflowchart LR
A["User clicks context menu"] -->|synchronously| B["Check if openSidePanel action"]
B -->|yes| C["Call sidePanel.open immediately"]
C --> D["Return before async query"]
B -->|no| E["Extract itemId for async path"]
E --> F["Query active tab asynchronously"]
F --> G["Handle other menu actions"]
H["Claude session key validation"] -->|sk-ant-sid01 or 02| I["Accept key"]
H -->|other prefix| J["Reject key"]
File Changes1. src/background/menus.mjs
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBackground menu handler now computes Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant BG as Background
participant CS as ContentScript
participant SP as ChromeSidePanel
User->>BG: right-click → select "Open Side Panel" (user gesture)
BG->>BG: compute itemId from info.menuItemId
alt itemId == openSidePanel and menuConfig.openSidePanel.action exists
BG->>CS: call menuConfig.openSidePanel.action(true, tab)
CS->>SP: chrome.sidePanel.open(...)
SP-->>CS: success / error
CS-->>BG: promise resolved/rejected (if applicable)
else
BG->>BG: Browser.tabs.query(...).then(...) or sendMessage path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Code Review by Qodo
1. Claude key validation too loose
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the menu click handler to ensure sidePanel.open() is called synchronously within a user gesture and updates the Claude session key validation to support multiple versions. Feedback suggests adding error handling for the asynchronous menu action to prevent unhandled promise rejections and making the Claude session key error message more generic to match the updated validation logic.
| // Chrome's user gesture requirement and causes the error: | ||
| // "sidePanel.open() may only be called in response to a user gesture." | ||
| if (itemId === 'openSidePanel' && menuConfig.openSidePanel?.action) { | ||
| menuConfig.openSidePanel.action(true, tab) |
There was a problem hiding this comment.
The action function is asynchronous and returns a Promise. Since it is called here without await or a .catch() block, any errors occurring within the action (such as a failure in the chrome.sidePanel.open API call) will result in an unhandled promise rejection in the background script. It is recommended to add a .catch() block to handle potential errors gracefully.
| menuConfig.openSidePanel.action(true, tab) | |
| menuConfig.openSidePanel.action(true, tab).catch(console.error) |
| if (!sessionKey.startsWith('sk-ant-sid')) { | ||
| throw new Error('Session key invalid: Must be in the format sk-ant-sid01-***** or sk-ant-sid02-*****') |
There was a problem hiding this comment.
The validation check and the error message are inconsistent. The check now allows any session key starting with sk-ant-sid (including potential future versions like sid03), but the error message explicitly mentions only sid01 and sid02. To be more future-proof and consistent with the code logic, the error message should be updated to a more generic format.
| if (!sessionKey.startsWith('sk-ant-sid')) { | |
| throw new Error('Session key invalid: Must be in the format sk-ant-sid01-***** or sk-ant-sid02-*****') | |
| if (!sessionKey.startsWith('sk-ant-sid')) { | |
| throw new Error('Session key invalid: Must be in the format sk-ant-sid-*****') | |
| } |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if (!sessionKey.startsWith('sk-ant-sid')) { | ||
| throw new Error('Session key invalid: Must be in the format sk-ant-sid01-***** or sk-ant-sid02-*****') | ||
| } |
There was a problem hiding this comment.
1. Claude key validation too loose 🐞 Bug ≡ Correctness
The constructor now only checks sessionKey.startsWith('sk-ant-sid') while the error message claims
only sk-ant-sid01-***** or sk-ant-sid02-***** are valid, allowing malformed keys to pass
validation and fail later during authenticated requests.
Agent Prompt
### Issue description
Claude `sessionKey` validation was loosened to a generic `startsWith('sk-ant-sid')`, which no longer enforces the documented `sid01`/`sid02` formats and allows malformed keys through.
### Issue Context
The thrown error message still claims the key must be `sk-ant-sid01-***** or sk-ant-sid02-*****`, but the code does not enforce that constraint.
### Fix Focus Areas
- src/services/clients/claude/index.mjs[74-84]
### Suggested fix
- Replace the `startsWith('sk-ant-sid')` check with a stricter pattern that matches the message, e.g.:
- `/^sk-ant-sid0[12]-/`
- Alternatively, if you intended to allow any `sidXX`, update both validation and error message consistently (e.g., `/^sk-ant-sid\d{2}-/`).
- Ensure the error message and validation logic match exactly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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 `@src/background/menus.mjs`:
- Around line 14-16: The call to menuConfig.openSidePanel.action(true, tab) is
intentionally synchronous to preserve user-gesture requirements but currently
can yield unhandled promise rejections because the action (defined in
src/content-script/menu-tools/index.mjs) is async and its internal
chrome.sidePanel.open Promise isn't returned; fix by updating the content-script
helper at src/content-script/menu-tools/index.mjs (the function that calls
chrome.sidePanel.open around line 66) to return the Promise from
chrome.sidePanel.open, and in src/background/menus.mjs where you handle itemId
=== 'openSidePanel' call menuConfig.openSidePanel.action(true, tab) and, if it
returns a Promise, attach a .catch to handle/log failures (e.g., check for a
returned value and call .catch on it) so you keep the synchronous call site
while observing any async errors.
In `@src/services/clients/claude/index.mjs`:
- Around line 77-78: The current sessionKey check using
sessionKey.startsWith('sk-ant-sid') accepts any variant but the error message
only mentions sk-ant-sid01-***** and sk-ant-sid02-*****; update the validation
in the constructor where sessionKey is checked to either (A) enforce the
documented formats by replacing the startsWith check with a regex that matches
exactly sk-ant-sid01-... or sk-ant-sid02-... (e.g., /^sk-ant-sid0[12]-\S+$/) so
malformed keys are rejected early, or (B) if you intend to accept all
sk-ant-sid* variants, change the Error message to refer generically to
sk-ant-sid* formats; modify the check around the sessionKey variable accordingly
(the same location as the existing sessionKey.startsWith call).
🪄 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
Run ID: 9c525545-8a53-47ae-9f16-165c4f3f71be
📒 Files selected for processing (2)
src/background/menus.mjssrc/services/clients/claude/index.mjs
| if (itemId === 'openSidePanel' && menuConfig.openSidePanel?.action) { | ||
| menuConfig.openSidePanel.action(true, tab) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect side-panel action implementations and call sites.
# Expect: openSidePanel.action returns/awaits/catches chrome.sidePanel.open(...),
# and menus.mjs observes the action promise with .catch(...) or equivalent.
rg -n -C5 "openSidePanel|chrome\.sidePanel\.open|menuConfig\.openSidePanel\.action" --iglob "*.mjs"Repository: ChatGPTBox-dev/chatGPTBox
Length of output: 2114
Handle side-panel action failures without moving the API call out of the gesture.
Line 15 intentionally calls the action synchronously, which preserves the user-gesture requirement. However, src/content-script/menu-tools/index.mjs:62-70 defines this action as async but does not return the Promise from chrome.sidePanel.open(...), and menus.mjs does not observe the returned Promise. This allows unhandled promise rejections if sidePanel.open() fails. Keep the call synchronous, but handle failures by observing the returned Promise:
if (itemId === 'openSidePanel' && menuConfig.openSidePanel?.action) {
- menuConfig.openSidePanel.action(true, tab)
+ void Promise.resolve(menuConfig.openSidePanel.action(true, tab)).catch((error) => {
+ console.error('failed to open side panel', error)
+ })
return
}Also update src/content-script/menu-tools/index.mjs:66 to return the Promise:
- chrome.sidePanel.open({ windowId: tab.windowId, tabId: tab.id })
+ return chrome.sidePanel.open({ windowId: tab.windowId, tabId: tab.id })📝 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.
| if (itemId === 'openSidePanel' && menuConfig.openSidePanel?.action) { | |
| menuConfig.openSidePanel.action(true, tab) | |
| return | |
| if (itemId === 'openSidePanel' && menuConfig.openSidePanel?.action) { | |
| void Promise.resolve(menuConfig.openSidePanel.action(true, tab)).catch((error) => { | |
| console.error('failed to open side panel', error) | |
| }) | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/background/menus.mjs` around lines 14 - 16, The call to
menuConfig.openSidePanel.action(true, tab) is intentionally synchronous to
preserve user-gesture requirements but currently can yield unhandled promise
rejections because the action (defined in
src/content-script/menu-tools/index.mjs) is async and its internal
chrome.sidePanel.open Promise isn't returned; fix by updating the content-script
helper at src/content-script/menu-tools/index.mjs (the function that calls
chrome.sidePanel.open around line 66) to return the Promise from
chrome.sidePanel.open, and in src/background/menus.mjs where you handle itemId
=== 'openSidePanel' call menuConfig.openSidePanel.action(true, tab) and, if it
returns a Promise, attach a .catch to handle/log failures (e.g., check for a
returned value and call .catch on it) so you keep the synchronous call site
while observing any async errors.
| if (!sessionKey.startsWith('sk-ant-sid')) { | ||
| throw new Error('Session key invalid: Must be in the format sk-ant-sid01-***** or sk-ant-sid02-*****') |
There was a problem hiding this comment.
Align the validation with the stated accepted formats.
Line 77 accepts any sk-ant-sid* value, but Line 78 documents only sk-ant-sid01-***** and sk-ant-sid02-*****. This lets malformed keys pass constructor validation and fail later during requests.
Proposed fix
- if (!sessionKey.startsWith('sk-ant-sid')) {
+ const isValidSessionKey = ['sk-ant-sid01-', 'sk-ant-sid02-'].some((prefix) =>
+ sessionKey.startsWith(prefix),
+ )
+ if (!isValidSessionKey) {
throw new Error('Session key invalid: Must be in the format sk-ant-sid01-***** or sk-ant-sid02-*****')
}If accepting all future sk-ant-sid* variants is intentional, update the error message instead.
📝 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.
| if (!sessionKey.startsWith('sk-ant-sid')) { | |
| throw new Error('Session key invalid: Must be in the format sk-ant-sid01-***** or sk-ant-sid02-*****') | |
| const isValidSessionKey = ['sk-ant-sid01-', 'sk-ant-sid02-'].some((prefix) => | |
| sessionKey.startsWith(prefix), | |
| ) | |
| if (!isValidSessionKey) { | |
| throw new Error('Session key invalid: Must be in the format sk-ant-sid01-***** or sk-ant-sid02-*****') | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/clients/claude/index.mjs` around lines 77 - 78, The current
sessionKey check using sessionKey.startsWith('sk-ant-sid') accepts any variant
but the error message only mentions sk-ant-sid01-***** and sk-ant-sid02-*****;
update the validation in the constructor where sessionKey is checked to either
(A) enforce the documented formats by replacing the startsWith check with a
regex that matches exactly sk-ant-sid01-... or sk-ant-sid02-... (e.g.,
/^sk-ant-sid0[12]-\S+$/) so malformed keys are rejected early, or (B) if you
intend to accept all sk-ant-sid* variants, change the Error message to refer
generically to sk-ant-sid* formats; modify the check around the sessionKey
variable accordingly (the same location as the existing sessionKey.startsWith
call).
The synchronous menuConfig.openSidePanel.action(true, tab) call is intentional — chrome.sidePanel.open() must run inside the user-gesture callback or it throws 'sidePanel.open() may only be called in response to a user gesture'. We don't want to break that. But the action used to be async, so the chrome.sidePanel.open Promise was discarded and any rejection (e.g. an invalid tab/window combo) would surface as an unhandled rejection in the background script. Two small changes: - menu-tools/index.mjs: drop the async wrapper and return the chrome.sidePanel.open() promise directly so the caller can observe it. - background/menus.mjs: still call the action synchronously (gesture preserved) but, if it returned a thenable, attach a .catch that logs the failure instead of letting it become unhandled. Addresses review feedback from CodeRabbit and gemini-code-assist.
|
Thanks for the reviews! I've pushed a follow-up commit (327c967) that addresses the unhandled-rejection concern raised by CodeRabbit and gemini-code-assist on Changes:
The Claude session-key validation comment isn't in scope for this PR — it was the change introduced by #962 and is being tracked/addressed there. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Unhandled promise rejection from chrome.sidePanel.open() in keyboard command handler (src/background/commands.mjs:15)
The PR explicitly fixed the unhandled rejection from chrome.sidePanel.open() in src/background/menus.mjs:19-21 by attaching a .catch() to the returned Promise. However, the same openSidePanel action is also invoked from src/background/commands.mjs:15 (via keyboard shortcut), where the returned Promise is silently discarded. Since the action was changed from async (which swallowed the inner promise) to a plain function that directly returns the chrome.sidePanel.open() Promise (src/content-script/menu-tools/index.mjs:66), a failure (e.g., side panel API not available, or invalid tab/window) will surface as an unhandled promise rejection in the background service worker, which could cause it to be terminated by Chrome.
View 5 additional findings in Devin Review.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… against missing chrome.sidePanel Addresses two follow-up review findings on PR ChatGPTBox-dev#963: 1. devin-ai-integration[bot]: src/background/commands.mjs:15 had the same unhandled-rejection pattern as menus.mjs. The keyboard command path now mirrors the menus.mjs treatment: call the action synchronously to preserve user-gesture context, then attach a .catch() to the returned thenable so a rejected sidePanel.open() does not bubble up as an unhandled rejection. 2. coderabbitai[bot]: src/content-script/menu-tools/index.mjs:62-69 called chrome.sidePanel.open directly, which throws synchronously in browsers where chrome.sidePanel is not defined (e.g. Firefox). Guard the call with a typeof chrome / chrome.sidePanel check and return a rejected Promise so the caller's .catch() handles it uniformly with API rejections.
|
Thanks for the additional findings, @devin-ai-integration and @coderabbitai! I've pushed a follow-up commit (810e8d3) that addresses both: 1. The keyboard-command path had the same pattern as The fix mirrors the 2.
The Claude session-key validation comment is being addressed in #962 since that's where the validation was loosened. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4281
All three concerns from this thread (unhandled rejection in |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`Browser.commands.onCommand`'s `tab` parameter is documented as optional, so an action that dereferences `tab.*` (e.g. `openSidePanel` reading `tab.windowId` / `tab.id`) can throw synchronously before returning a Promise. The previous `.catch` only observed Promise rejections, so a synchronous throw would still surface as an uncaught error in the background script. Wrap the action invocation in try/catch and log+return on synchronous failure, mirroring the async rejection handling. Addresses CodeRabbit review on PR ChatGPTBox-dev#963.
|
Thanks for the additional finding, @coderabbitai! Pushed commit
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the current state of the fix. 🏁 Script executed: Length of output: 2518
The full error-handling chain across |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
contextMenus.onClicked.tab is optional per the Chrome API ('If the
click did not take place in a tab, this parameter will be missing').
The openSidePanel action dereferences tab.windowId/tab.id, so it can
throw synchronously, which the existing .catch() Promise handler
would not catch. Mirror the pattern already used in
src/background/commands.mjs by wrapping the call in try/catch.
Addresses devin-ai-integration[bot] review on src/background/menus.mjs:23.
1 participant
Fixes #857
Problem
Clicking "Open Side Panel" from the right-click context menu does nothing, with this error in the background script console:
Root cause: In
menus.mjs, theonClickMenuhandler immediately kicks off an asyncBrowser.tabs.query()call. ThesidePanel.open()is then called inside the.then()callback — but by that point, Chrome no longer considers it to be within the original user gesture context, so the API call is rejected.Solution
Extract the
itemIdbefore the async query, and handle theopenSidePanelaction synchronously at the top of the event handler — before any async operations — so thatsidePanel.open()is called within the original user gesture chain.All other menu actions that require the current tab information are unaffected and still go through the
Browser.tabs.query()path.Testing
Summary by CodeRabbit
New Features
Improvements
Bug Fixes