fix(plugins): restore ChatGPT formula copy in Edge - #911
Conversation
Fixes Nagi-ovo#909 Co-authored-by: Codex <codex@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR fixes ChatGPT formula copying without MathML annotations and adds explicit plugin content-script synchronization after host permission changes. It adds routing, permission-flow, and regression-test coverage. ChangesChatGPT Formula Copy
Plugin Content-Script Synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PluginManager
participant runtime.sendMessage
participant BackgroundService
participant syncPluginContentScripts
PluginManager->>runtime.sendMessage: Send gv.plugins.syncContentScripts after permission approval
runtime.sendMessage->>BackgroundService: Route synchronization request
BackgroundService->>syncPluginContentScripts: Await serialized registration reconciliation
syncPluginContentScripts-->>BackgroundService: Complete synchronization
BackgroundService-->>runtime.sendMessage: Return { ok: true }
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/popup/components/PluginManager.tsx`:
- Around line 37-43: The helper requestPluginContentScriptSync must report
whether browser.runtime.sendMessage succeeded instead of suppressing failures.
Update handleGrantRequiredAccess to remove plugin.id from missingPermissionIds
only when synchronization succeeds, preserving the repair action otherwise, and
add a test covering a rejected runtime.sendMessage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da2f19d1-d0da-45a7-a342-5359fbc72974
📒 Files selected for processing (9)
.github/docs/REGRESSION_NOTES.mdsrc/features/formulaCopy/FormulaCopyService.test.tssrc/features/formulaCopy/FormulaCopyService.tssrc/features/plugins/runtime/messages.tssrc/pages/background/__tests__/runtimeMessageRouting.test.tssrc/pages/background/index.tssrc/pages/background/runtimeMessageRouting.tssrc/pages/popup/components/PluginManager.tsxsrc/pages/popup/components/__tests__/PluginManager.test.tsx
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Keep the required-access repair action visible until the background confirms content-script reconciliation. Co-authored-by: Codex <codex@users.noreply.github.com>
Nagi-ovo
left a comment
Nagi-ovo
left a comment
There was a problem hiding this comment.
LGTM — verified the extraction priority (Gemini data-math → ChatGPT data-math-source → AI Studio/Claude annotations), the .katex-display block detection stays correct for Claude, and the gv.plugins.syncContentScripts type is properly added to the background routing allow-list with tests. Resolved the REGRESSION_NOTES append conflict against main on the branch. Thanks for the clean replacement PR!
![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: 16c1d04253
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| if (message?.type === PLUGIN_CONTENT_SCRIPT_SYNC_MESSAGE) { | ||
| await syncPluginContentScripts(); | ||
| sendResponse({ ok: true }); |
There was a problem hiding this comment.
When chrome.scripting.registerContentScripts rejects during doSyncPluginContentScripts, that function catches/logs and resolves, so this new branch still sends { ok: true }. In the Edge permission-repair flow the popup removes the pluginGrantRequiredAccess retry as soon as it sees ok, leaving a granted-but-unregistered plugin (Formula Copy still inert) with no retry path; have the sync return a real success/failure before acknowledging.
AGENTS.md reference: AGENTS.md:L84-L84
Useful? React with 👍 / 👎.
| // permissions.onAdded event that normally performs registration. | ||
| // Reconcile explicitly while retaining onAdded as Chrome's | ||
| // popup-close fallback. | ||
| await requestPluginContentScriptSync(); |
There was a problem hiding this comment.
In the new-host enable path, requestPluginContentScriptSync() is explicitly the Edge fallback, but this result is ignored. If sendMessage fails or the background returns { ok: false } after the user grants the host, the plugin remains enabled from the pre-prompt write, and because the permission is now present the missing-access repair button will not appear; surface or roll back the failed sync instead of returning.
AGENTS.md reference: AGENTS.md:L84-L84
Useful? React with 👍 / 👎.
2 participants
Fixes #909
Description / 描述
Formula Copy had two independent failures on ChatGPT/Edge:
permissions.onAddedas the Chrome popup-close fallback.data-math-sourcevalue and recognizes.katex-displayas block math, while preserving the legacy annotation paths used by Claude, older ChatGPT markup, Gemini, and AI Studio.The PR adds realistic ChatGPT inline/block DOM regression coverage, permission-registration routing tests, and a regression note. The follow-up review fix keeps the required-access repair action available when background reconciliation fails and adds a rejected-message test.
Related Issue / 相关 Issue
Fixes #909
community-only, I was assigned after maintainer approval before starting. / 如果 Issue 带有community-only标签，我已在开始前获得维护者确认并被分配。（N/A: [Bug] Formula Copy does not work on ChatGPT in Microsoft Edge #909 is labeledcontribution, notcommunity-only.)Visual Proof / 可视化证据
The contributor manually verified the rebuilt
dist_chrome_devpackage on the real ChatGPT formula from #909: hover styling appeared and clicking the formula copied the expected LaTeX successfully.visual-prove.mp4
Browser Testing / 浏览器测试
Current tested head / 当前验证提交:
a32b97ea2b379bf11c32d26b35dfb16c36339dc7Full-suite and cross-browser build baseline / 全量测试及跨浏览器构建基线:
e591cab1c4385fbfbae362da774ebed3b822f6760788f16181e33ca77346eef5787c0d204a4f8278, loaded the freshly rebuiltdist_chrome_dev, refreshed the real ChatGPT conversation, and clicked currentdata-math-sourceKaTeX. Copy succeeded with the configured LaTeX delimiters.Missing checks and owner / 缺失检查与负责人:
a32b97eaChrome live reload; owner: @shallowaria. The linked live recording predates the clean branch and follow-up review fix.Commands run / 已运行命令:
bun run lint— 0 errors; 209 existing warnings; no resulting editsbun run typecheckbun run i18n:check— 10 locales, 693 keysa32b97ea— 3 files, 30 testsbunx vitest run --exclude scripts/__tests__/verify-release-privacy.test.ts --testTimeout 20000 --hookTimeout 30000 --silent— 277 files, 2597 testsbun run build:all— Chrome, Firefox, and Safari builds passed; Safari resource wiring passedbun run build:edge— Edge production code build and manifest preparation passed;dist_edgewas producedbun run docs:builda32b97ea: targeted Prettier;bun run typecheck;bun run lint(0 errors, 209 existing warnings); focused suite (3 files, 30 tests);bun run build:chromegit diff --check origin/main...HEADCommands not completed and reason / 未完成命令及原因:
bun run verify:prwas not completed as one command on this Windows checkout because the repository symlink-only privacy test is incompatible here. Its applicable formatting, lint, typecheck, test, browser-build, i18n, and diff gates were run separately.scripts/__tests__/verify-release-privacy.test.tsrequires repository symlinks unavailable in this Windows checkout; the full test run excluded only that file.bun run build:edgereached the final packaging step, then failed because the externalzipexecutable is unavailable on Windows. CI's Edge build remains required.Cannot access 'default' before initialization; both commits were created with--no-verifyonly after the targeted Prettier checks and the verification suite above passed.Checklist / 检查清单
Summary by CodeRabbit
Bug Fixes
Plugin Improvements
Tests