Fix: Address potential bugs and improve error handling#863
Fix: Address potential bugs and improve error handling#863PeterDaveHello merged 8 commits intomasterfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdded centralized redaction and reconnection in the background service; hardened messaging, webRequest/tab listeners, and side-panel handling. Content script gains idempotent mounting, multi-source token management (Claude/Kimi/ChatGPT), selection/UI tooling, and new run/manage utilities with extensive logging and error guards. Changes
Sequence Diagram(s)sequenceDiagram
participant Port as Background Port
participant Manager as Reconnect Manager
participant Handler as Message Handler
participant Redactor as Redaction Layer
participant API as API Executor
Port->>Manager: open connection
Manager->>Handler: forward incoming message
Handler->>Redactor: redact sensitive fields
Redactor-->>Handler: redacted payload
Handler->>API: executeApi(request)
API-->>Handler: response / error
Handler->>Port: send response
alt Connection lost
Port--xManager: disconnect
Manager->>Manager: schedule reconnect (backoff)
Manager->>Port: attempt reconnect
end
sequenceDiagram
participant Runtime as Browser Runtime
participant Script as Content Script
participant TokenMgr as Token Manager
participant Claude as Claude Service
participant UI as UI Components
Runtime->>Script: inject / start
Script->>Script: run()
Script->>TokenMgr: detect / overwrite tokens
TokenMgr->>Claude: request sessionKey
Claude-->>TokenMgr: sessionKey or null
Script->>Script: mountComponent (retry)
Script->>UI: init selection tools / static card
alt Jump-back required
Script->>TokenMgr: poll token readiness
TokenMgr-->>Script: token ready
Script->>UI: render WebJumpBackNotification
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 3fbed97
Previous suggestions✅ Suggestions up to commit 0b5e7a4
✅ Suggestions up to commit a986d2e
✅ Suggestions up to commit 789d8fe
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/background/index.mjs (1)
433-439: Returning{}on failure drops request headersIf an exception occurs we currently return an empty object, which strips all headers and can break the request:
- return {} // Return empty object or original headers on error? + return { requestHeaders: details.requestHeaders }src/content-script/index.jsx (2)
622-627: Dead code triggers “defined but never used” lint warning
prepareForForegroundRequestsis kept as a no-op placeholder but still defined.
Either delete it or export it for tests to silence the linter.🧰 Tools
🪛 GitHub Check: tests
[failure] 622-622:
'prepareForForegroundRequests' is defined but never used
878-916: Risk of multipleregisterPortListenerregistrations
registerPortListeneris invoked every page load; if the script is injected twice (e.g. SPA navigation) duplicate listeners will accumulate.Guard with a module-level flag:
+if (!window.__chatgptboxPortListenerRegistered) { registerPortListener(…) + window.__chatgptboxPortListenerRegistered = true +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/background/index.mjs(2 hunks)src/content-script/index.jsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/background/index.mjs
[error] 380-380: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 459-459: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/content-script/index.jsx
[error] 265-265: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 410-410: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 530-530: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 581-581: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 610-610: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: tests
src/background/index.mjs
[failure] 460-460:
'chrome' is not defined
src/content-script/index.jsx
[failure] 622-622:
'prepareForForegroundRequests' is defined but never used
🪛 GitHub Actions: pr-tests
src/background/index.mjs
[error] 460-460: ESLint: 'chrome' is not defined. (no-undef)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/background/index.mjs (4)
72-88:⚠️ Potential issueProxy reconnection still risks infinite recursion & listener leaks
proxyOnDisconnectinvokessetPortProxyunconditionally and immediately, without:
- Removing the listeners attached to the previous proxy port.
- Capping the retry count / adding back-off delay.
The exact same concerns were raised in the previous review but remain unaddressed.
Leaving this as-is can snow-ball into dozens of lingering listeners and a tight retry loop on a flapping tab.
See the earlier suggestion for a bounded retry counter + exponential back-off.
103-104:⚠️ Potential issueSensitive configuration is still dumped to the console
console.debug('[background] Full config details:', config)will happily print API keys, custom URLs and tokens.
The prior review already flagged this leakage – please remove or redact before shipping.- console.debug('[background] Full config details:', config) + // Avoid leaking secrets – only log non-sensitive fields + console.debug('[background] Config loaded. modelName:', config.modelName)
432-439: 🛠️ Refactor suggestionReturning
{}on error strips all request headersIf an exception bubbles up we return an empty object, which drops every header and can break the request completely.
Safer fall-back: return the original headers so the request proceeds unchanged.- return {} // Return empty object or original headers on error? + return { requestHeaders: details.requestHeaders }
458-459:⚠️ Potential issue
chromeglobal is still unguarded – breaks Firefox & eslintAccessing
chrome.sidePanelwithout atypeof chrome !== 'undefined'guard triggersno-undefand fails on Firefox wherechromeis absent.- if (chrome && chrome.sidePanel) { + if (typeof chrome !== 'undefined' && chrome?.sidePanel) {🧰 Tools
🪛 Biome (1.9.4)
[error] 458-458: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.eslintrc.json(1 hunks)package.json(1 hunks)src/background/index.mjs(2 hunks)src/content-script/index.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .eslintrc.json
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/background/index.mjs
[error] 380-380: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 458-458: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/content-script/index.jsx
[error] 265-265: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 410-410: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 530-530: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 581-581: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 610-610: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/background/index.mjs (1)
545-556:⚠️ Potential issueChrome global usage still present despite past review feedback.
This code still uses the unguarded
chromeglobal that was flagged in previous reviews as causing ESLint errors and Firefox compatibility issues.The pipeline error persists due to unguarded
chromeusage. Apply the fix from the past review:-// eslint-disable-next-line no-undef -if (chrome && chrome.sidePanel) { - // eslint-disable-next-line no-undef - await chrome.sidePanel.setOptions({ +if (typeof chrome !== 'undefined' && chrome?.sidePanel) { + await chrome.sidePanel.setOptions({ tabId, path: 'IndependentPanel.html', enabled: true, }) - console.debug(`[background] Side panel options set for tab ${tabId} using chrome.sidePanel`) + console.debug(`[background] Side panel options set for tab ${tabId} using chrome.sidePanel`) } else { - console.debug('[background] chrome.sidePanel API also not available.') + console.debug('[background] chrome.sidePanel API also not available.') }🧰 Tools
🪛 Biome (1.9.4)
[error] 546-546: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/content-script/index.jsx (1)
719-758: 🛠️ Refactor suggestionKimi token polling has the same race condition issue.
The same timeout/polling race condition exists in the Kimi refresh token logic.
Apply the same cleanup pattern as suggested for the Claude session key polling logic above.
🧹 Nitpick comments (5)
src/background/index.mjs (3)
64-69: Previous listener cleanup logic has improved but could be more robust.The cleanup logic for old listeners is better than before, but there's still a potential issue: if
port._proxyOnMessageorport._proxyOnDisconnectare undefined, the removeListener calls will silently fail rather than throwing the expected error that the try-catch handles.Consider checking for listener existence before removal:
if (port.proxy) { try { - if (port._proxyOnMessage) port.proxy.onMessage.removeListener(port._proxyOnMessage); - if (port._proxyOnDisconnect) port.proxy.onDisconnect.removeListener(port._proxyOnDisconnect); + if (port._proxyOnMessage && port.proxy.onMessage.hasListener?.(port._proxyOnMessage)) { + port.proxy.onMessage.removeListener(port._proxyOnMessage); + } + if (port._proxyOnDisconnect && port.proxy.onDisconnect.hasListener?.(port._proxyOnDisconnect)) { + port.proxy.onDisconnect.removeListener(port._proxyOnDisconnect); + } } catch(e) {
93-124: Exponential backoff implementation is well-designed but could benefit from configuration.The reconnection logic with exponential backoff and retry limits addresses the infinite loop concerns from past reviews effectively. The implementation is solid with proper cleanup and error handling.
Consider making the retry configuration more visible:
+const RECONNECT_CONFIG = { + MAX_ATTEMPTS: 5, + BASE_DELAY_MS: 1000, + BACKOFF_MULTIPLIER: 2 +}; + -const MAX_RECONNECT_ATTEMPTS = 5; +const MAX_RECONNECT_ATTEMPTS = RECONNECT_CONFIG.MAX_ATTEMPTS; ... -const delay = Math.pow(2, port._reconnectAttempts - 1) * 1000; +const delay = Math.pow(RECONNECT_CONFIG.BACKOFF_MULTIPLIER, port._reconnectAttempts - 1) * RECONNECT_CONFIG.BASE_DELAY_MS;
488-503: Header modification logic is correct but could use optional chaining.The header modification logic is functionally correct, but Biome suggests using optional chaining for cleaner code.
Apply the static analysis suggestion:
-for (let i = 0; i < headers.length; i++) { - if (headers[i].name.toLowerCase() === 'origin') { +for (let i = 0; i < headers?.length; i++) { + if (headers[i]?.name?.toLowerCase() === 'origin') { headers[i].value = 'https://www.bing.com' modified = true - } else if (headers[i].name.toLowerCase() === 'referer') { + } else if (headers[i]?.name?.toLowerCase() === 'referer') { headers[i].value = 'https://www.bing.com/search?q=Bing+AI&showconv=1&FORM=hpcodx' modified = true } }src/content-script/index.jsx (2)
264-276: Selection element containment check could use optional chaining.The logic is correct but could benefit from the optional chaining suggested by static analysis tools.
-if (toolbarContainer && toolbarContainer.contains(e.target)) { +if (toolbarContainer?.contains(e.target)) { console.debug('[content] Mouseup inside toolbar, ignoring.') return } const selectionElement = window.getSelection()?.rangeCount > 0 && window.getSelection()?.getRangeAt(0).endContainer.parentElement -if (toolbarContainer && selectionElement && toolbarContainer.contains(selectionElement)) { +if (toolbarContainer?.contains(selectionElement)) { console.debug('[content] Mouseup selection is inside toolbar, ignoring.') return }🧰 Tools
🪛 Biome (1.9.4)
[error] 265-265: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
810-822: Storage change listener could be more specific.The storage change listener correctly handles config changes but could be more efficient by checking specific config keys that affect ChatGPT tab state.
-if (areaName === 'local' && (changes.userConfig || changes.config)) { +if (areaName === 'local' && + (changes.userConfig?.newValue?.modelName !== changes.userConfig?.oldValue?.modelName || + changes.userConfig?.newValue?.apiMode !== changes.userConfig?.oldValue?.apiMode || + changes.config)) { console.log( '[content] User config changed in storage, re-evaluating ChatGPT tab state.', ) await manageChatGptTabState() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/background/index.mjs(2 hunks)src/content-script/index.jsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/background/index.mjs
[error] 169-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 452-452: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 546-546: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/content-script/index.jsx
[error] 265-265: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 410-410: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 530-530: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 581-581: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 610-610: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
src/background/index.mjs (2)
450-461: Request body handling uses optional chaining correctly.The requestBody handling logic properly checks for existence before accessing properties, addressing potential null pointer issues.
🧰 Tools
🪛 Biome (1.9.4)
[error] 452-452: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
566-598: Try-catch blocks for initialization provide good error isolation.The wrapping of initialization functions (
registerPortListener,registerCommands,refreshMenu) in separate try-catch blocks is a good practice for preventing one failure from affecting others.src/content-script/index.jsx (4)
44-78: Retry logic for element detection is well-implemented.The retry mechanism with URL change detection prevents infinite loops and provides good user feedback through logging. The logic properly handles the case where elements aren't found after all retries.
862-872: Textarea value vs textContent issue has been properly fixed.The code now correctly uses
input.valueinstead ofinput.textContentfor the textarea element, which addresses the focus hack issue identified in past reviews.
896-947: Port listener registration logic is well-designed.The global flag to prevent duplicate port listener registration is a good pattern, and the error handling around the registration is comprehensive.
838-892: ChatGPT tab state management function is well-structured.The
manageChatGptTabStatefunction properly handles the ChatGPT-specific logic with appropriate guards and error handling. The textarea manipulation usingvalueinstead oftextContentis correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/background/index.mjs (3)
497-497: Apply optional chaining as suggested by static analysis.The code can be simplified using optional chaining to safely access nested properties.
- if (details.requestBody && details.requestBody.formData) { + if (details.requestBody?.formData) {🧰 Tools
🪛 Biome (1.9.4)
[error] 497-497: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
216-223: Consider reducing log verbosity for production environments.The extensive debug logging, while helpful for development, may impact performance and expose sensitive information in production environments. Consider implementing log level controls or environment-based logging.
+ const isDebugMode = process.env.NODE_ENV === 'development' || userConfig?.debugMode; + - console.debug('[background] Full session details (redacted):', redactSensitiveFields(session)) - console.debug('[background] Full config details (redacted):', redactSensitiveFields(config)) - if (session.apiMode) { - console.debug('[background] Session apiMode details (redacted):', redactSensitiveFields(session.apiMode)) - } + if (isDebugMode) { + console.debug('[background] Full session details (redacted):', redactSensitiveFields(session)) + console.debug('[background] Full config details (redacted):', redactSensitiveFields(config)) + if (session.apiMode) { + console.debug('[background] Session apiMode details (redacted):', redactSensitiveFields(session.apiMode)) + } + }
54-58: Document reconnection configuration constants.The reconnection constants lack documentation about their purpose and impact on user experience.
+/** + * Configuration for exponential backoff reconnection strategy + * @constant {Object} RECONNECT_CONFIG + * @property {number} MAX_ATTEMPTS - Maximum reconnection attempts before giving up + * @property {number} BASE_DELAY_MS - Initial delay between reconnection attempts + * @property {number} BACKOFF_MULTIPLIER - Multiplier for exponential backoff calculation + */ const RECONNECT_CONFIG = { MAX_ATTEMPTS: 5, - BASE_DELAY_MS: 1000, // Base delay in milliseconds - BACKOFF_MULTIPLIER: 2, // Multiplier for exponential backoff + BASE_DELAY_MS: 1000, + BACKOFF_MULTIPLIER: 2, };src/content-script/index.jsx (4)
529-529: Apply optional chaining as suggested by static analysis.The nested property access can be simplified using optional chaining.
- if (siteAdapterAction && siteAdapterAction.init) { + if (siteAdapterAction?.init) {🧰 Tools
🪛 Biome (1.9.4)
[error] 529-529: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
580-580: Apply optional chaining as suggested by static analysis.The property access can be simplified using optional chaining.
- if (preElement && preElement.textContent) { + if (preElement?.textContent) {🧰 Tools
🪛 Biome (1.9.4)
[error] 580-580: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
609-609: Apply optional chaining as suggested by static analysis.The nested property access can be simplified using optional chaining.
- if (data && data.accessToken) { + if (data?.accessToken) {🧰 Tools
🪛 Biome (1.9.4)
[error] 609-609: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
38-41: Consider reducing debug log verbosity in production.Similar to the background script, the extensive debug logging may impact performance in production environments. Consider implementing environment-based logging controls.
+ const isDebugMode = process.env.NODE_ENV === 'development'; + - console.debug('[content] mountComponent called with siteConfig:', siteConfig) + if (isDebugMode) { + console.debug('[content] mountComponent called with siteConfig:', siteConfig) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/background/index.mjs(2 hunks)src/content-script/index.jsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/background/index.mjs
[error] 497-497: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/content-script/index.jsx
[error] 529-529: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 580-580: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 609-609: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/background/index.mjs (1)
108-213: Excellent error handling and reconnection logic implementation.The
setPortProxyfunction demonstrates comprehensive error handling with proper listener cleanup, exponential backoff, and detailed logging. The implementation correctly addresses the infinite reconnection loop issues identified in previous reviews.src/content-script/index.jsx (4)
44-79: Robust retry logic with proper URL change detection.The retry mechanism for element detection is well-implemented with proper checks for URL changes during retries, preventing unnecessary work when navigation occurs.
655-697: Excellent promise timeout cleanup implementation.The Claude session key polling logic properly addresses the memory leak issues identified in previous reviews with proper cleanup of both timers and settled promise protection.
861-873: Correct textarea value manipulation.The code correctly uses the
valueproperty instead oftextContentfor textarea elements, addressing the issue identified in previous reviews.
895-946: Well-designed global port listener registration.The port listener registration includes proper guards to prevent duplicate registration and comprehensive error handling for the ChatGPT Web model processing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/background/index.mjs (1)
658-673: Chrome global usage still needs proper guarding despite ESLint disable.While the ESLint disable comment addresses the linting error, the code still has the underlying compatibility issue with Firefox. The chrome global should be properly guarded.
The existing review comment about this issue is still valid. Replace the ESLint disable approach with proper runtime checking:
- if (!sidePanelSet) { - // eslint-disable-next-line no-undef - if (typeof chrome !== 'undefined' && chrome.sidePanel && typeof chrome.sidePanel.setOptions === 'function') { + if (!sidePanelSet && typeof chrome !== 'undefined' && chrome?.sidePanel?.setOptions) {
🧹 Nitpick comments (3)
src/background/index.mjs (1)
76-133: Review the redactSensitiveFields function for potential edge cases and optimization.The function has good protection against circular references and deep recursion, but there are a few potential issues:
- The
seenWeakSet is passed by reference through recursive calls, which could cause issues if the same object appears in different branches of the recursion tree.- String value checking in arrays (lines 96-104) iterates through all sensitive keywords for every string, which could be inefficient for large arrays.
Consider these optimizations:
function redactSensitiveFields(obj, recursionDepth = 0, maxDepth = 5, seen = new WeakSet()) { if (recursionDepth > maxDepth) { return 'REDACTED_TOO_DEEP'; } if (obj === null || typeof obj !== 'object') { return obj; } if (seen.has(obj)) { return 'REDACTED_CIRCULAR_REFERENCE'; } seen.add(obj); + // Pre-compile sensitive keyword regex for better performance + const sensitiveRegex = new RegExp(SENSITIVE_KEYWORDS.join('|'), 'i'); + + const checkSensitive = (str) => sensitiveRegex.test(str.toLowerCase()); if (Array.isArray(obj)) { const redactedArray = []; for (let i = 0; i < obj.length; i++) { const item = obj[i]; if (item !== null && typeof item === 'object') { redactedArray[i] = redactSensitiveFields(item, recursionDepth + 1, maxDepth, seen); } else if (typeof item === 'string') { - let isItemSensitive = false; - const lowerItem = item.toLowerCase(); - for (const keyword of SENSITIVE_KEYWORDS) { - if (lowerItem.includes(keyword)) { - isItemSensitive = true; - break; - } - } - redactedArray[i] = isItemSensitive ? 'REDACTED' : item; + redactedArray[i] = checkSensitive(item) ? 'REDACTED' : item; } else { redactedArray[i] = item; } } + seen.delete(obj); // Clean up after processing return redactedArray; } else { const redactedObj = {}; for (const key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) { const lowerKey = key.toLowerCase(); - let isKeySensitive = false; - for (const keyword of SENSITIVE_KEYWORDS) { - if (lowerKey.includes(keyword)) { - isKeySensitive = true; - break; - } - } - if (isKeySensitive) { + if (checkSensitive(key)) { redactedObj[key] = 'REDACTED'; } else if (obj[key] !== null && typeof obj[key] === 'object') { redactedObj[key] = redactSensitiveFields(obj[key], recursionDepth + 1, maxDepth, seen); } else { redactedObj[key] = obj[key]; } } } + seen.delete(obj); // Clean up after processing return redactedObj; } }src/content-script/index.jsx (2)
37-79: Retry mechanism in mountComponent could be optimized.The retry loop is well-implemented but could benefit from early termination on certain conditions and better performance:
async function mountComponent(siteConfig) { console.debug('[content] mountComponent called with siteConfig:', siteConfig) try { const userConfig = await getUserConfig() console.debug('[content] User config in mountComponent:', userConfig) if (!userConfig.alwaysFloatingSidebar) { const retry = 10 let oldUrl = location.href let elementFound = false + const selectors = [ + ...(siteConfig ? [siteConfig.sidebarContainerQuery, siteConfig.appendContainerQuery, siteConfig.resultsContainerQuery].filter(Boolean) : []), + userConfig.prependQuery, + userConfig.appendQuery + ].filter(Boolean); + for (let i = 1; i <= retry; i++) { console.debug(`[content] mountComponent retry ${i}/${retry} for element detection.`) if (location.href !== oldUrl) { console.log('[content] URL changed during retry, stopping mountComponent.') return } - const e = - (siteConfig && - (getPossibleElementByQuerySelector(siteConfig.sidebarContainerQuery) || - getPossibleElementByQuerySelector(siteConfig.appendContainerQuery) || - getPossibleElementByQuerySelector(siteConfig.resultsContainerQuery))) || - getPossibleElementByQuerySelector([userConfig.prependQuery]) || - getPossibleElementByQuerySelector([userConfig.appendQuery]) + const e = getPossibleElementByQuerySelector(selectors); if (e) { console.log('[content] Element found for mounting component:', e) elementFound = true break } else { console.debug(`[content] Element not found on retry ${i}.`) if (i === retry) { console.warn('[content] Element not found after all retries for mountComponent.') return } - await new Promise((r) => setTimeout(r, 500)) + // Progressive delay: 500ms, 750ms, 1000ms, etc. + await new Promise((r) => setTimeout(r, 500 + (i - 1) * 250)) } }
721-770: Duplicate polling logic for Kimi token - consider extracting to utility function.The Kimi refresh token polling logic is nearly identical to the Claude session key polling, leading to code duplication.
Consider extracting the polling logic into a reusable utility function:
async function pollForCondition(conditionFn, options = {}) { const { interval = 500, timeout = 30000, errorHandler = null, onSuccess = null } = options; let promiseSettled = false; let timerId = null; let timeoutId = null; const cleanup = () => { if (timerId) clearInterval(timerId); if (timeoutId) clearTimeout(timeoutId); }; return new Promise((resolve, reject) => { timerId = setInterval(async () => { if (promiseSettled) { cleanup(); return; } try { const result = await conditionFn(); if (result) { if (!promiseSettled) { promiseSettled = true; cleanup(); if (onSuccess) await onSuccess(result); resolve(result); } } } catch (err) { if (errorHandler && await errorHandler(err) && !promiseSettled) { promiseSettled = true; cleanup(); reject(err); } } }, interval); timeoutId = setTimeout(() => { if (!promiseSettled) { promiseSettled = true; cleanup(); reject(new Error('Polling timeout exceeded')); } }, timeout); }).catch((err) => { cleanup(); throw err; }); }Then use it for both Claude and Kimi polling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.eslintrc.json(1 hunks)src/background/index.mjs(2 hunks)src/content-script/index.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .eslintrc.json
🔇 Additional comments (5)
src/background/index.mjs (2)
135-259: LGTM! The setPortProxy function addresses previous infinite loop concerns.The extensive rewrite successfully addresses the past review comments about infinite reconnect loops and listener leaks by:
- Implementing proper retry limits with exponential backoff
- Cleaning up old listeners before adding new ones
- Using named listener functions stored on the port object for easier removal
- Comprehensive error handling with try-catch blocks around cleanup operations
The reconnection logic is now robust and prevents resource leaks.
271-391: Enhanced executeApi function improves observability and error handling.The additions provide excellent debugging capabilities with redacted logging and comprehensive model support. The error handling ensures that failures are properly communicated back to the caller.
src/content-script/index.jsx (3)
868-887: Textarea manipulation logic addresses previous textContent issue.The fix correctly uses the
valueproperty instead oftextContentfor textarea elements, which resolves the previous issue where the focus hack wasn't working properly.
903-954: Global port listener registration is well-structured.The approach of moving the port listener registration outside the
runfunction and using a global flag to prevent duplicate registration is a good design pattern. The comprehensive error handling ensures robustness.
651-698: 🛠️ Refactor suggestionPotential cleanup issue in Claude session key polling.
The promise polling logic has improved cleanup but there's still a potential race condition where the cleanup function might not be called if an exception occurs during the interval callback.
await new Promise((resolve, reject) => { timerId = setInterval(async () => { if (promiseSettled) { console.warn('[content] Promise already settled but Claude interval still running. This indicates a potential cleanup issue.'); cleanup() return } try { claudeSession = await getClaudeSessionKey() if (claudeSession) { if (!promiseSettled) { promiseSettled = true cleanup() console.log('[content] Claude session key found after waiting.') resolve() } } } catch (err) { console.error('[content] Error polling for Claude session key:', err) const errMsg = err.message.toLowerCase(); if ((errMsg.includes('network') || errMsg.includes('permission')) && !promiseSettled) { promiseSettled = true; cleanup(); reject(new Error(`Failed to get Claude session key due to: ${err.message}`)); + } else if (!promiseSettled) { + // For non-critical errors, continue polling but log the issue + console.debug('[content] Non-critical error in Claude polling, continuing...'); } } }, 500) timeoutId = setTimeout(() => { if (!promiseSettled) { promiseSettled = true cleanup() console.warn('[content] Timed out waiting for Claude session key.') reject(new Error('Timed out waiting for Claude session key.')) } }, 30000) - }).catch((err) => { + }) + .catch((err) => { + // Ensure cleanup happens even if promise chain fails + cleanup(); console.error('[content] Failed to get Claude session key for jump back notification:', err) return })Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the extension’s stability and robustness by improving error handling, logging, and development tooling.
- Updated ESLint commands in package.json to use npx, ensuring the local version of ESLint is used.
- Modified ESLint configuration by adding webextensions support and Node-specific overrides for build and script files.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| package.json | Updated lint commands to use npx for consistent, local dependency usage. |
| .eslintrc.json | Added the webextensions environment and Node overrides to better target files. |
Comments suppressed due to low confidence (2)
package.json:8
- Using 'npx' ensures that the project's local ESLint version is used. Consider updating the development documentation to note that npx is required to run these lint commands.
"lint": "npx eslint --ext .js,.mjs,.jsx .",
.eslintrc.json:8
- [nitpick] Review the Node-specific override configuration to ensure it aligns with the intended setup for build and script files. It might be helpful to document how these overrides interact with the base config.
"overrides": [
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (e) { | ||
| console.warn( | ||
| '[background] Error removing old listeners from previous port.proxy instance:', | ||
| e, | ||
| ) |
There was a problem hiding this comment.
When switching/recreating port.proxy, the old proxy port is only detached from listeners but never disconnected. If setPortProxy is called multiple times (e.g., when chatgptTabId changes or reconnect logic runs), this can leave orphaned Port connections alive in the target tab and waste resources. Consider explicitly calling port.proxy.disconnect() (in a try/catch) before overwriting it, and then nulling out the reference once disconnected.
| } catch (e) { | |
| console.warn( | |
| '[background] Error removing old listeners from previous port.proxy instance:', | |
| e, | |
| ) | |
| if (typeof port.proxy.disconnect === 'function') { | |
| port.proxy.disconnect() | |
| } | |
| } catch (e) { | |
| console.warn( | |
| '[background] Error cleaning up previous port.proxy instance:', | |
| e, | |
| ) | |
| } finally { | |
| port.proxy = null |
| console.log( | ||
| `[background] executeApi called for model: ${session.modelName}, apiMode: ${session.apiMode}`, | ||
| ) | ||
| const redactedSession = redactSensitiveFields(session) | ||
| const redactedConfig = redactSensitiveFields(config) | ||
| console.debug('[background] Full session details (redacted):', redactedSession) | ||
| console.debug('[background] Full config details (redacted):', redactedConfig) |
There was a problem hiding this comment.
redactSensitiveFields does not treat user prompt fields (e.g., question, prompt, selectionText) as sensitive, so the redacted session/config logs can still include the full user query/prompt text. This can inadvertently expose user content in extension logs. Consider expanding the sensitive-keyword list (or doing field-specific redaction) so prompts/questions are either omitted or replaced with length/summary metadata.
| const preferredLanguage = await getPreferredLanguage() | ||
| const replyPromptBelow = `Reply in ${preferredLanguage}. Regardless of the language of content I provide below. !!This is very important!!` | ||
| const replyPromptAbove = `Reply in ${preferredLanguage}. Regardless of the language of content I provide above. !!This is very important!!` | ||
| const result = `${replyPromptBelow}\n\n${input}\n\n${replyPromptAbove}` | ||
| console.debug('[content] getInput from function result:', result) | ||
| return result |
There was a problem hiding this comment.
This debug log prints the full generated prompt (including the user’s input/query text) to the console. That can leak sensitive user content into extension logs and can also be extremely large. Consider logging only non-sensitive metadata (e.g., whether input was found + character count) or gating the full prompt behind an explicit dev/debug flag.
| .trim() | ||
| .replace(/^-+|-+$/g, '') | ||
| if (selection) { | ||
| console.debug('[content] Text selected:', selection) |
There was a problem hiding this comment.
This debug log prints the full selected text from the page into the console. Selections often contain sensitive data (emails, keys, private text). Consider logging only that a selection occurred and its length, or removing the raw selection from logs to avoid accidental disclosure.
| console.debug('[content] Text selected:', selection) | |
| const selectionLength = selection.length | |
| console.debug('[content] Text selected. Length:', selectionLength) |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PeterDaveHello
force-pushed
the
bugfix/general-fixes
branch
from
98c5f41 to
0cfcb28
Compare
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const createSelectionTools = async (toolbarContainerElement, selection) => { | ||
| console.debug( | ||
| '[content] createSelectionTools called with selection:', | ||
| selection, | ||
| 'and container:', | ||
| toolbarContainerElement, | ||
| ) |
There was a problem hiding this comment.
createSelectionTools logs the full selected text to the console. Selected text can be sensitive (passwords, emails, private content), so logging it verbatim is a privacy risk. Prefer logging only selection length or a redacted/truncated preview behind a debug flag.
| const selection = window | ||
| .getSelection() | ||
| ?.toString() | ||
| .trim() | ||
| .replace(/^-+|-+$/g, '') | ||
| if (selection) { | ||
| console.debug('[content] Text selected via touch:', selection) | ||
| const touch = e.changedTouches[0] | ||
| toolbarContainer = createElementAtPosition(touch.pageX + 20, touch.pageY + 20) | ||
| await createSelectionTools(toolbarContainer, selection) | ||
| } else { |
There was a problem hiding this comment.
This debug statement logs the full touch-selected text (selection) to the console. Since this can contain private user content, avoid logging it verbatim; log only length/metadata or redact/truncate behind an explicit debug setting.
| console.log('[content] Received CREATE_CHAT message:', message) | ||
| try { | ||
| const data = message.data | ||
| let prompt = '' | ||
| if (data.itemId in toolsConfig) { | ||
| console.debug('[content] Generating prompt from toolsConfig for item:', data.itemId) | ||
| prompt = await toolsConfig[data.itemId].genPrompt(data.selectionText) | ||
| } else if (data.itemId in menuConfig) { | ||
| console.debug('[content] Generating prompt from menuConfig for item:', data.itemId) | ||
| const menuItem = menuConfig[data.itemId] | ||
| if (!menuItem.genPrompt) { | ||
| console.warn('[content] No genPrompt for menu item:', data.itemId) | ||
| return | ||
| } | ||
| prompt = await menuItem.genPrompt() | ||
| if (prompt) { | ||
| const preferredLanguage = await getPreferredLanguage() | ||
| prompt = await cropText(`Reply in ${preferredLanguage}.\n` + prompt) | ||
| } | ||
| } else { | ||
| console.warn('[content] Unknown itemId for CREATE_CHAT:', data.itemId) | ||
| return | ||
| } | ||
| console.debug('[content] Generated prompt:', prompt) |
There was a problem hiding this comment.
The CREATE_CHAT handler logs the full incoming message and the generated prompt. These can include user selection text and other sensitive data. Avoid logging raw prompt/selection content; log only itemId/lengths or redact/truncate, ideally behind a debug flag.
| port._proxyOnMessage = (msg) => { | ||
| const redactedMsg = redactSensitiveFields(msg) | ||
| console.debug('[background] Message from proxy tab (redacted):', redactedMsg) | ||
| if (port._reconnectAttempts) { |
There was a problem hiding this comment.
redactSensitiveFields is used before logging proxy messages, but it does not treat response content keys like answer (and potentially body, text, etc.) as sensitive. As a result, console.debug can still print full model outputs / user content to the background console. Consider expanding redaction to include common payload keys (e.g. answer, body, text) or truncating long strings before logging.
| console.debug('[content] Getting input from siteConfig.inputQuery') | ||
| question = await getInput(siteConfig.inputQuery) | ||
| } | ||
| console.debug('[content] Question for component:', question) |
There was a problem hiding this comment.
The debug log prints the full derived question prompt content to the console. This can include sensitive user queries/page content and can leak PII in logs. Consider logging only metadata (e.g., presence/length) or gating prompt/selection logging behind an explicit debug flag/config toggle.
| console.debug('[content] Question for component:', question) | |
| console.debug( | |
| '[content] Question for component:', | |
| question ? `{present, length=${question.length}}` : 'none', | |
| ) |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PeterDaveHello
force-pushed
the
bugfix/general-fixes
branch
from
0cfcb28 to
0c6a109
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c6a1098a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| port._reconnectAttempts = (port._reconnectAttempts || 0) + 1 | ||
| if (port._reconnectAttempts >= RECONNECT_CONFIG.MAX_ATTEMPTS) { |
There was a problem hiding this comment.
setPortProxy increments port._reconnectAttempts on every proxy disconnect, but it only resets the counter when a message is later received from the proxy. If the ChatGPT tab disconnects/reloads while idle, reconnects can succeed without any incoming message, so the counter keeps growing across otherwise successful reconnects until MAX_ATTEMPTS is hit, after which forwarding is disabled for that still-open conversation port.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!sidePanelSet) { | ||
| console.debug('[background] Attempting chrome.sidePanel.setOptions as fallback.') | ||
| const chromeApi = globalThis.chrome | ||
| if (chromeApi?.sidePanel && typeof chromeApi.sidePanel.setOptions === 'function') { | ||
| try { | ||
| await chromeApi.sidePanel.setOptions({ | ||
| tabId, | ||
| path: 'IndependentPanel.html', | ||
| enabled: true, | ||
| }) | ||
| console.debug( | ||
| `[background] Side panel options set for tab ${tabId} using chrome.sidePanel`, | ||
| ) | ||
| sidePanelSet = true | ||
| } catch (chromeError) { | ||
| console.error( | ||
| '[background] chrome.sidePanel.setOptions also failed:', | ||
| chromeError.message, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The Browser.sidePanel and chrome.sidePanel fallback logic attempts to handle cross-browser compatibility, but accessing globalThis.chrome directly without checking if it's defined could throw errors in non-Chrome browsers. While the try-catch will handle it, this approach is fragile. Consider checking for API availability before attempting to use it, or use feature detection consistently.
| function ensureChatGptPortListenerRegistered() { | ||
| if (chatGPTBoxPortListenerRegistered) { | ||
| console.log('[content] Port listener already registered, skipping.') | ||
| return | ||
| } | ||
|
|
||
| if (location.hostname !== 'chatgpt.com' || location.pathname === '/auth/login') { | ||
| console.debug( | ||
| '[content] Not on chatgpt.com or on login page, skipping port listener registration.', | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| const url = new URL(window.location.href) | ||
| if (url.searchParams.has('chatgptbox_notification')) { | ||
| if (location.hostname === 'claude.ai' && !(await getClaudeSessionKey())) { | ||
| console.log('claude not logged in') | ||
|
|
||
| await new Promise((resolve) => { | ||
| const timer = setInterval(async () => { | ||
| const token = await getClaudeSessionKey() | ||
| if (token) { | ||
| clearInterval(timer) | ||
| resolve() | ||
| try { | ||
| console.log('[content] Attempting to register port listener for chatgpt.com.') | ||
| registerPortListener(async (session, port) => { | ||
| console.debug( | ||
| `[content] Port listener callback triggered. Session model: ${session?.modelName}, Port: ${port.name}`, | ||
| ) | ||
| try { | ||
| if (isUsingChatgptWebModel(session)) { | ||
| console.log( | ||
| '[content] Session is for ChatGPT Web Model, processing request for question:', | ||
| session.question, | ||
| ) | ||
| const accessToken = await getChatGptAccessToken() | ||
| if (!accessToken) { | ||
| console.warn('[content] No ChatGPT access token available for web API call.') | ||
| port.postMessage({ error: 'Missing ChatGPT access token.' }) | ||
| return | ||
| } | ||
| }, 500) | ||
| await generateAnswersWithChatgptWebApi(port, session.question, session, accessToken) | ||
| console.log('[content] generateAnswersWithChatgptWebApi call completed.') | ||
| } else { | ||
| console.debug( | ||
| '[content] Session is not for ChatGPT Web Model, skipping processing in this listener.', | ||
| ) | ||
| } | ||
| } catch (e) { | ||
| console.error('[content] Error in port listener callback:', e, 'Session:', session) | ||
| try { | ||
| port.postMessage({ | ||
| error: e.message || 'An unexpected error occurred in content script port listener.', | ||
| }) | ||
| } catch (postError) { | ||
| console.error('[content] Error sending error message back via port:', postError) | ||
| } | ||
| } | ||
| }) | ||
| console.log('[content] Generic port listener registered successfully for chatgpt.com pages.') | ||
| chatGPTBoxPortListenerRegistered = true | ||
| } catch (error) { | ||
| console.error('[content] Error registering global port listener:', error) | ||
| } | ||
| } | ||
|
|
||
| async function run() { | ||
| console.log('[content] Script run started.') | ||
| try { | ||
| ensureChatGptPortListenerRegistered() |
There was a problem hiding this comment.
The ensureChatGptPortListenerRegistered function is called in the run() function, but it doesn't guarantee that the port listener is registered before manageChatGptTabState sends the SET_CHATGPT_TAB message. If there's any delay or error in registering the port listener, the background script might set the chatgptTabId but no listener would be ready to handle messages from that tab. Consider awaiting the port listener registration or handling this race condition explicitly.
| if (toolbarContainer && toolbarContainer.className === 'chatgptbox-toolbar-container') { | ||
| console.debug('[content] Deleting toolbar:', toolbarContainer) | ||
| toolbarContainer.remove() | ||
| toolbarContainer = null |
There was a problem hiding this comment.
Line 232 sets toolbarContainer = null after removal, but the deleteToolbar function is called from multiple places including event listeners that may have already captured the toolbarContainer reference. This could lead to cases where the local reference in an event handler still points to the removed element while the module-level variable is null, causing inconsistent state. Consider whether this nullification is necessary or if it should be handled differently.
| toolbarContainer = null |
| async function manageChatGptTabState() { | ||
| if (manageChatGptTabStatePromise) { | ||
| console.debug('[content] manageChatGptTabState already running, waiting for in-flight call.') | ||
| return manageChatGptTabStatePromise | ||
| } | ||
|
|
||
| await overwriteAccessToken() | ||
| await prepareForForegroundRequests() | ||
| manageChatGptTabStatePromise = (async () => { | ||
| console.debug('[content] manageChatGptTabState called. Current location:', location.href) | ||
| try { | ||
| if (location.hostname !== 'chatgpt.com' || location.pathname === '/auth/login') { | ||
| console.debug( | ||
| '[content] Not on main chatgpt.com page, skipping manageChatGptTabState logic.', | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| const userConfig = await getUserConfig() | ||
| const isThisTabDesignatedForChatGptWeb = chatgptWebModelKeys.some((model) => | ||
| getApiModesStringArrayFromConfig(userConfig, true).includes(model), | ||
| ) | ||
| console.debug( | ||
| '[content] Is this tab designated for ChatGPT Web:', | ||
| isThisTabDesignatedForChatGptWeb, | ||
| ) | ||
|
|
||
| if (isThisTabDesignatedForChatGptWeb) { | ||
| if (location.pathname === '/') { | ||
| console.debug('[content] On chatgpt.com root path.') | ||
| const input = document.querySelector('#prompt-textarea') | ||
| if (!chatGptPromptTextareaPoked && input && input.value === '') { | ||
| console.log('[content] Manipulating #prompt-textarea for focus.') | ||
| if (document.activeElement === input) { | ||
| console.debug('[content] #prompt-textarea already focused; skipping injection.') | ||
| } else { | ||
| const injectedValue = ' ' | ||
| chatGptPromptTextareaPoked = true | ||
| input.value = injectedValue | ||
| input.dispatchEvent(new Event('input', { bubbles: true })) | ||
| setTimeout(() => { | ||
| const currentInput = document.querySelector('#prompt-textarea') | ||
| if (!currentInput?.isConnected) { | ||
| console.warn( | ||
| '[content] #prompt-textarea no longer available in setTimeout callback.', | ||
| ) | ||
| return | ||
| } | ||
| if (document.activeElement === currentInput) { | ||
| console.debug('[content] #prompt-textarea focused; skipping injection cleanup.') | ||
| return | ||
| } | ||
| if (currentInput.value === injectedValue) { | ||
| currentInput.value = '' | ||
| currentInput.dispatchEvent(new Event('input', { bubbles: true })) | ||
| console.debug('[content] #prompt-textarea manipulation complete.') | ||
| } else if (currentInput.value.startsWith(injectedValue)) { | ||
| currentInput.value = currentInput.value.slice(injectedValue.length) | ||
| currentInput.dispatchEvent(new Event('input', { bubbles: true })) | ||
| console.debug('[content] Removed injected leading space from #prompt-textarea.') | ||
| } | ||
| }, 300) | ||
| } | ||
| } else { | ||
| console.debug( | ||
| '[content] #prompt-textarea not found, not empty (value: "' + | ||
| input?.value + | ||
| '"), or not on root path for manipulation.', | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| console.log('[content] Sending SET_CHATGPT_TAB message.') | ||
| await Browser.runtime.sendMessage({ | ||
| type: 'SET_CHATGPT_TAB', | ||
| data: {}, | ||
| }) | ||
| console.log('[content] SET_CHATGPT_TAB message sent successfully.') | ||
| } else { | ||
| console.log('[content] This tab is NOT configured for ChatGPT Web model processing.') | ||
| } | ||
| } catch (error) { | ||
| console.error('[content] Error in manageChatGptTabState:', error) | ||
| } | ||
| })() | ||
|
|
||
| prepareForSelectionTools() | ||
| prepareForSelectionToolsTouch() | ||
| prepareForStaticCard() | ||
| prepareForRightClickMenu() | ||
| prepareForJumpBackNotification() | ||
| try { | ||
| await manageChatGptTabStatePromise | ||
| } finally { | ||
| manageChatGptTabStatePromise = null | ||
| } |
There was a problem hiding this comment.
The manageChatGptTabState function uses a promise singleton pattern to prevent concurrent execution, but the promise is stored in a module-level variable. If manageChatGptTabState is called while a previous call is still pending, the second call will await the first call's promise. However, if the first call encounters an error and throws, the second caller will also receive that error even though it might succeed if retried. Consider whether each caller should get independent execution or if error handling should be improved.
| try { | ||
| const response = await fetch(validatedUrl, message.data?.init) |
There was a problem hiding this comment.
The FETCH handler validates that the URL uses http or https protocols, but it doesn't validate or sanitize the init parameter (message.data.init) that's passed directly to fetch(). An attacker could potentially inject malicious headers, credentials, or other fetch options. Consider validating or sanitizing the init parameter, especially the headers, method, and credentials fields.
| } | ||
|
|
||
| port.proxy.onMessage.addListener(port._proxyOnMessage) | ||
| port.onMessage.addListener(port._portOnMessage) | ||
| port.proxy.onDisconnect.addListener(port._proxyOnDisconnect) | ||
| port.onDisconnect.addListener(port._portOnDisconnect) |
There was a problem hiding this comment.
The setPortProxy function cleans up and re-establishes listeners every time it's called, but it doesn't check if the listeners were already added. This could lead to duplicate listener registration if setPortProxy is called multiple times with the same proxy connection due to an error scenario. Consider tracking listener registration state to prevent duplicates.
| } | |
| port.proxy.onMessage.addListener(port._proxyOnMessage) | |
| port.onMessage.addListener(port._portOnMessage) | |
| port.proxy.onDisconnect.addListener(port._proxyOnDisconnect) | |
| port.onDisconnect.addListener(port._portOnDisconnect) | |
| // Mark proxy listeners as no longer registered after cleanup. | |
| port._proxyListenersRegistered = false | |
| } | |
| // Avoid registering duplicate listeners for the same proxy connection. | |
| if (port._proxyListenersRegistered && port.proxy === proxyRef) { | |
| console.debug( | |
| '[background] Proxy listeners already registered for this connection; skipping re-registration.', | |
| ) | |
| } else { | |
| port.proxy.onMessage.addListener(port._proxyOnMessage) | |
| port.onMessage.addListener(port._portOnMessage) | |
| port.proxy.onDisconnect.addListener(port._proxyOnDisconnect) | |
| port.onDisconnect.addListener(port._portOnDisconnect) | |
| port._proxyListenersRegistered = true | |
| } |
PeterDaveHello
force-pushed
the
bugfix/general-fixes
branch
from
0c6a109 to
d4fe6c0
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| port._reconnectAttempts = (port._reconnectAttempts || 0) + 1 | ||
| if (port._reconnectAttempts >= RECONNECT_CONFIG.MAX_ATTEMPTS) { | ||
| console.error( | ||
| `[background] Max reconnect attempts (${RECONNECT_CONFIG.MAX_ATTEMPTS}) reached for tab ${proxyTabId}. Giving up.`, | ||
| ) |
There was a problem hiding this comment.
port._reconnectAttempts is never reset when starting a fresh setPortProxy() connection, and it also isn’t reset after hitting the max-attempts “giving up” path. That means a port that previously exhausted retries can immediately hit the max-attempts branch on the very next disconnect, preventing recovery unless the main port is closed/reopened. Consider resetting _reconnectAttempts when initiating a new explicit connection (e.g. at the top of setPortProxy when proxyTabId changes) and/or when max attempts are reached (so later user-initiated requests can retry).
User description
It's a large patch, and I'm unsure if we can get it merged. It's generated by Google's Jules Agent, which serves as a trial run and test lab. We can also review the feedback from other agents.
Jule's commit message:
GitHub Copilot's summary:
PR Type
Enhancement, Bug fix
Description
Major refactor to improve error handling and logging throughout background and content scripts.
Enhanced communication and state management between background and content scripts.
Improved web request interception and header modification logic.
Refined UI mounting and selection tool logic in the content script.
Changes walkthrough 📝
index.mjs
Robust error handling and logging in background scriptsrc/background/index.mjs
functions and listeners.
ChatGPT tab.
use of 'blocking' for header modification.
responses.
and menu refresh.
index.jsx
Refactor content script for reliability and diagnosticssrc/content-script/index.jsx
manageChatGptTabState with robust error handling.
and event-driven logic.
better DOM checks and error diagnostics.
configuration updates.
Summary by CodeRabbit
New Features
Bug Fixes & Reliability
Chores