Skip to content

fix: call sidePanel.open() synchronously to fix context menu side panel #963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
octo-patch wants to merge 6 commits into
base: master
Choose a base branch
from
+60 −8
Open

fix: call sidePanel.open() synchronously to fix context menu side panel #963

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
46acd1c
fix: accept Claude session keys in sk-ant-sid02 format (fixes #935)
Apr 20, 2026
792b6bc
fix: call sidePanel.open() synchronously to fix context menu side pan…
Apr 20, 2026
327c967
fix(menus): observe sidePanel.open promise to avoid unhandled rejections
octo-patch Apr 20, 2026
810e8d3
fix(menus): observe sidePanel.open promise in command handler & guard…
Apr 20, 2026
0f82ecd
fix(commands): also catch synchronous throws in command action
Apr 20, 2026
025cbb8
fix: also wrap menus.mjs openSidePanel call in try/catch
octo-patch Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/background/menus.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,21 @@ import { config as menuConfig } from '../content-script/menu-tools/index.mjs'

const menuId = 'ChatGPTBox-Menu'
const onClickMenu = (info, tab) => {
const itemId = info.menuItemId.replace(menuId, '')

// sidePanel.open() must be called synchronously within the user gesture handler.
// Calling it inside a Promise callback (e.g. Browser.tabs.query().then()) breaks
// 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)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
menuConfig.openSidePanel.action(true, tab)
menuConfig.openSidePanel.action(true, tab).catch(console.error)

return
Comment on lines +14 to +34
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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.

Suggested change
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.

}

Browser.tabs.query({ active: true, currentWindow: true }).then((tabs) => {
const currentTab = tabs[0]
const message = {
itemId: info.menuItemId.replace(menuId, ''),
itemId,
selectionText: info.selectionText,
useMenuPosition: tab.id === currentTab.id,
}
Expand Down
4 changes: 2 additions & 2 deletions src/services/clients/claude/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ export class Claude {
if (!sessionKey) {
throw new Error('Session key required')
}
if (!sessionKey.startsWith('sk-ant-sid01')) {
throw new Error('Session key invalid: Must be in the format sk-ant-sid01-*****')
if (!sessionKey.startsWith('sk-ant-sid')) {
throw new Error('Session key invalid: Must be in the format sk-ant-sid01-***** or sk-ant-sid02-*****')
Comment on lines +77 to +78
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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-*****')
}

Comment on lines +77 to +78
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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).

}
Comment on lines +77 to 79
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

if (fetch) {
this.fetch = fetch
Expand Down