feat(plugins): add ChatGPT temporary chat handoff - #921
Conversation
Add opt-in Markdown, JSON, PDF, selection, and temporary-chat export flows. Co-authored-by: Codex <codex@users.noreply.github.com>
|
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:
📝 WalkthroughWalkthroughThe PR adds a ChatGPT export plugin for Markdown, JSON, and PDF. It collects virtualized conversations, supports message selection and temporary-chat handoff, adds localized UI, and introduces ChatGPT-specific PDF rendering. ChangesChatGPT export
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
src/features/export/services/__tests__/PDFPrintService.test.ts (1)
95-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the title assertion discriminating, and reduce CSS string coupling.
Two points:
- Line 96 and Line 105 use the same value,
Export test. The ChatGPT branch atgetPrintDialogTitleprefersmetadata.titleover the page title. With identical values the assertion at Line 116 passes for either precedence order. Use distinct values to lock the intended behavior.- Lines 117-123 assert exact CSS declaration text, including spacing. A reformat of
buildChatGptPrintStylesbreaks these tests without any behavior change. Assert on the scoped selector plus a small number of load-bearing declarations, or assert on computed markup instead.💚 Proposed change for point 1
- document.title = 'Export test'; + document.title = 'Stale page title'; window.print = vi.fn();🤖 Prompt for 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. In `@src/features/export/services/__tests__/PDFPrintService.test.ts` around lines 95 - 124, Update the ChatGPT layout test around PDFPrintService.export to use distinct page and metadata titles, then assert the resulting document.title reflects the metadata-title precedence. Replace exact CSS string/spacing assertions with less brittle checks that verify the scoped selector and only essential load-bearing declarations or computed markup behavior.
🤖 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/features/export/services/PDFPrintService.ts`:
- Around line 534-543: Update isMeaningfulConversationTitle to reject the
literal title “ChatGPT” alongside the existing Gemini exclusions, so
getConversationTitle cannot use document.title as a meaningful fallback and
empty metadata titles resolve to the intended neutral fallback.
- Around line 1262-1283: Update the generated code-block styles in the PDF print
template around the `.gv-print-turn-text pre` rule to preserve readable contrast
when print background graphics are disabled. Keep the existing dark styling by
default, and add a `forced-colors` or no-background safeguard that switches the
code surface and text to a light, bordered presentation without relying on the
dark background.
In `@src/features/plugins/builtin/chatgptExport/conversation.ts`:
- Around line 256-275: Update the collection loop around the conversation
collector to merge mounted messages after the final scroll, including when
maxSteps is reached. If collection remains above the bottom once the step limit
is exhausted, return an explicit incomplete-collection error instead of
exporting collected messages; preserve normal completion behavior at the bottom.
Add a regression test using a small maxSteps value that verifies the error and
final-scroll merge.
- Around line 118-132: Update readStableId so fallback identities never use the
scan-local order, which is unstable across virtualized windows. Prefer a stable
ChatGPT identifier for every supported schema; when none exists, return a
non-deduplicating identity or otherwise ensure the message is retained rather
than merged in collected. Add coverage for window swaps containing repeated
role/text messages.
In `@src/features/plugins/builtin/chatgptExport/exporter.test.ts`:
- Around line 18-89: Add a JSON export test to the ChatGPT export formats suite
using exportChatGptConversation with format 'json'; stub the Blob/download path,
parse the generated payload, and assert the schema preserves selected state,
message IDs, roles, and content. Reuse collectMountedChatGptMessages and the
existing metadata setup, while keeping the current Markdown and PDF coverage
unchanged.
In `@src/features/plugins/builtin/chatgptExport/i18n.ts`:
- Line 93: Update the tempRegret and tempTitle translation strings in the chat
export i18n definitions to describe saving and transferring the temporary chat
to a normal chat, replacing the regret wording. Keep both labels aligned and
readable across themes.
In `@src/features/plugins/builtin/chatgptExport/index.ts`:
- Around line 67-70: The whole-conversation icon path in the paths mapping
should use coordinates that keep the downward arrow within the SVG viewport.
Update the whole entry’s arrow segment while preserving its existing vertical
stem and bottom bar; leave the selected and temporary paths unchanged.
In `@src/features/plugins/builtin/chatgptExport/styles.ts`:
- Around line 284-315: Complete the dark-theme selector coverage in the styles
for .gv-chatgpt-export-menu-item:hover,
.gv-chatgpt-export-menu-item:focus-visible, .gv-chatgpt-export-btn:hover,
.gv-chatgpt-export-format-option:has(input:checked), and
.gv-chatgpt-export-button--floating by adding the missing body.dark variants.
Preserve the existing light-theme rules and dark colors for html.dark,
body.dark, and [data-theme='dark'].
In `@src/features/plugins/builtin/chatgptExport/tempHandoff.test.ts`:
- Around line 19-53: Extend the temporary chat handoff tests to cover
handoffTemporaryChat and resumePendingHandoff lifecycle behavior, pending-entry
expiry, and attachment-delivery failure. Verify failed attachment delivery
preserves the existing composer draft instead of clearing it through
input.replaceChildren(), while retaining the current route, transcript, and
attachment assertions. Run bun run test, bun run lint, and bun run typecheck.
In `@src/features/plugins/builtin/chatgptExport/tempHandoff.ts`:
- Around line 182-187: Update the attachment fallback in tempHandoff around
dispatchPaste so a false result never calls input.replaceChildren or overwrites
the existing composer draft. Instead, return a delivery failure and display the
appropriate error while preserving the user's current content; keep the
successful paste path unchanged.
- Around line 106-110: Update writePending and resumePendingHandoff to persist a
stable active ChatGPT account identity in PendingHandoff and compare it before
delivery; clear the pending handoff and skip delivery when identities differ.
Also adjust the attachment-paste fallback in deliver so it preserves existing
composer content or requires confirmation before replacing it, rather than
clearing the draft automatically.
In `@src/features/plugins/builtin/chatgptExport/ui.ts`:
- Around line 45-83: Update the dialog creation and lifecycle around the
existing overlay/dialog helpers to add an accessible name via the title element,
then reuse the established gv-pm-* modal precedent for Tab focus trapping,
Escape/outside dismissal, focus restoration to the trigger, teardown, and theme
behavior. Add keyboard coverage verifying Tab cycling within the modal and
restoring focus after close.
---
Nitpick comments:
In `@src/features/export/services/__tests__/PDFPrintService.test.ts`:
- Around line 95-124: Update the ChatGPT layout test around
PDFPrintService.export to use distinct page and metadata titles, then assert the
resulting document.title reflects the metadata-title precedence. Replace exact
CSS string/spacing assertions with less brittle checks that verify the scoped
selector and only essential load-bearing declarations or computed markup
behavior.
🪄 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: b7469713-a40e-4214-a43b-f3ca24e62781
📒 Files selected for processing (19)
src/features/export/services/PDFPrintService.tssrc/features/export/services/__tests__/PDFPrintService.test.tssrc/features/plugins/builtin/builtin.test.tssrc/features/plugins/builtin/chatgptExport/conversation.test.tssrc/features/plugins/builtin/chatgptExport/conversation.tssrc/features/plugins/builtin/chatgptExport/exporter.test.tssrc/features/plugins/builtin/chatgptExport/exporter.tssrc/features/plugins/builtin/chatgptExport/i18n.tssrc/features/plugins/builtin/chatgptExport/index.test.tssrc/features/plugins/builtin/chatgptExport/index.tssrc/features/plugins/builtin/chatgptExport/selectionMode.test.tssrc/features/plugins/builtin/chatgptExport/selectionMode.tssrc/features/plugins/builtin/chatgptExport/styles.tssrc/features/plugins/builtin/chatgptExport/tempHandoff.test.tssrc/features/plugins/builtin/chatgptExport/tempHandoff.tssrc/features/plugins/builtin/chatgptExport/ui.test.tssrc/features/plugins/builtin/chatgptExport/ui.tssrc/features/plugins/builtin/index.tssrc/pages/content/pluginNativeRegistration.ts
![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: 2bb4b8e4ef
ℹ️ 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".
Co-authored-by: Codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 655ea91a99
ℹ️ 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".
Co-authored-by: Codex <codex@users.noreply.github.com>
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/features/plugins/builtin/chatgptExport/tempHandoff.test.ts`:
- Around line 122-139: Add a test for the immediate account-change path in
handoffTemporaryChat, covering navigation from /u/0/ to /u/1/. Assert it returns
account-mismatch, preserves the existing composer draft, and removes the pending
state; then run bun run test, bun run lint, and bun run typecheck.
🪄 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: b363876c-3abf-4109-ab80-962d65fdc8de
📒 Files selected for processing (14)
src/features/export/services/PDFPrintService.tssrc/features/export/services/__tests__/PDFPrintService.test.tssrc/features/plugins/builtin/chatgptExport/conversation.test.tssrc/features/plugins/builtin/chatgptExport/conversation.tssrc/features/plugins/builtin/chatgptExport/exporter.test.tssrc/features/plugins/builtin/chatgptExport/exporter.tssrc/features/plugins/builtin/chatgptExport/i18n.tssrc/features/plugins/builtin/chatgptExport/index.test.tssrc/features/plugins/builtin/chatgptExport/index.tssrc/features/plugins/builtin/chatgptExport/styles.tssrc/features/plugins/builtin/chatgptExport/tempHandoff.test.tssrc/features/plugins/builtin/chatgptExport/tempHandoff.tssrc/features/plugins/builtin/chatgptExport/ui.test.tssrc/features/plugins/builtin/chatgptExport/ui.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/features/plugins/builtin/chatgptExport/i18n.ts
- src/features/plugins/builtin/chatgptExport/index.test.ts
- src/features/export/services/tests/PDFPrintService.test.ts
- src/features/plugins/builtin/chatgptExport/ui.test.ts
- src/features/plugins/builtin/chatgptExport/ui.ts
- src/features/plugins/builtin/chatgptExport/conversation.ts
- src/features/export/services/PDFPrintService.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f0e159199
ℹ️ 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".
Co-authored-by: Codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 096a5f8c69
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/features/export/services/DOMContentExtractor.ts`:
- Around line 82-87: Update extractAssistantContent and its processNodes flow to
include direct text nodes alongside child elements, preserving their DOM order
when combining ordinary user-content such as text surrounding links. Ensure
mixed content retains both surrounding text and extracted element output, and
add a regression case covering text before and after an anchor.
- Around line 82-87: Update the empty-content fallback in DOMContentExtractor so
standard ChatGPT file-pill metadata is extracted and merged into
ExtractedContent.attachments instead of relying only on
extractAssistantContent(element). Preserve the existing text and HTML fallback
behavior, and add a regression test covering standard file pills in the JSON
export.
- Around line 610-624: Update both anchor-handling branches, including the
standalone-link block and the corresponding branch around the other reported
location, to serialize anchor children through the existing inline extractor
instead of using link.textContent. Wrap the extracted HTML and Markdown in the
link destination while preserving rich child elements such as images, bold text,
and emphasis; retain the current URL fallback only when the extracted content is
empty.
- Around line 610-624: Validate link URLs in both anchor-export paths, including
the standalone-link branch around the visible `tagName === 'a'` logic and the
other anchor branch near the corresponding export code, before adding them to
`htmlParts` or `textParts`. Allow only intended schemes such as `http` and
`https`; skip or safely handle disallowed schemes like `javascript:` while
preserving existing label and escaping behavior for valid links.
- Around line 610-624: Update the standalone-link handling in the extraction
method to resolve relative href values against link.ownerDocument.baseURI before
generating either HTML or Markdown output, using the resolved destination
consistently for both paths. Apply the same normalization in the corresponding
link serialization logic around the later link-handling block.
🪄 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: 7255be61-b37c-4558-87ea-2d2521087360
📒 Files selected for processing (5)
src/features/export/services/DOMContentExtractor.tssrc/features/export/services/__tests__/DOMContentExtractor.test.tssrc/features/plugins/builtin/chatgptExport/exporter.test.tssrc/features/plugins/builtin/chatgptExport/tempHandoff.test.tssrc/features/plugins/builtin/chatgptExport/tempHandoff.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/plugins/builtin/chatgptExport/exporter.test.ts
- src/features/plugins/builtin/chatgptExport/tempHandoff.ts
Co-authored-by: Codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37bb145ada
ℹ️ 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".
Co-authored-by: Codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daab5703cb
ℹ️ 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".
Co-authored-by: Codex <codex@users.noreply.github.com>
|
The 80% docstring warning is not adopted. That threshold is not part of this repository's checks, and boilerplate docstrings on local plugin internals would add noise without improving a public API. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5c6960b1a
ℹ️ 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".
Co-authored-by: Codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/features/export/services/DOMContentExtractor.ts (1)
486-496: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the formula marker test before
FormulaCopyService.extractLatexSource. Both extraction paths callextractLatexSourceon every element node, and that helper performsclosestplus up to two subtreequerySelectorscans. The cheap class/attribute test that gates the branch runs afterwards, so non-math subtrees are scanned repeatedly during export.
src/features/export/services/DOMContentExtractor.ts#L486-L496: compute themath-block/katex-display/data-math/data-math-sourcetest first inprocessNodes, then callextractLatexSourceonly when it passes.src/features/export/services/DOMContentExtractor.ts#L888-L897: apply the same ordering inprocessInlineContent, where the recursion repeats the scan at each nesting level.🤖 Prompt for 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. In `@src/features/export/services/DOMContentExtractor.ts` around lines 486 - 496, In src/features/export/services/DOMContentExtractor.ts lines 486-496 within processNodes, evaluate the math-block/katex-display/data-math/data-math-source marker before calling FormulaCopyService.extractLatexSource, and only extract when the marker passes. Apply the same ordering in processInlineContent at lines 888-897 so both recursive paths avoid scanning non-math elements.src/features/plugins/builtin/chatgptExport/tempHandoff.test.ts (1)
139-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDispose each
PluginScopefrom a teardown hook.Each test calls
await scope.dispose()as the last statement. If an assertion fails earlier, the scope stays live. Its listeners, timers, and observers then leak into the following tests and can produce cascading failures that hide the original one. Register the scope in a shared list and dispose it inafterEach.♻️ Proposed refactor
+const scopes: PluginScope[] = []; + +function createScope(): PluginScope { + const scope = new PluginScope(); + scopes.push(scope); + return scope; +} + +afterEach(async () => { + for (const scope of scopes.splice(0)) await scope.dispose(); +});Then replace
new PluginScope()withcreateScope()and remove the trailingawait scope.dispose()calls.Also applies to: 178-193
🤖 Prompt for 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. In `@src/features/plugins/builtin/chatgptExport/tempHandoff.test.ts` around lines 139 - 149, Ensure every PluginScope created in the tests is disposed from an afterEach teardown hook, including scopes created in the test blocks around addComposer and handoffTemporaryChat. Register scopes through a shared createScope helper backed by a list, replace direct new PluginScope() calls with createScope(), and remove trailing await scope.dispose() statements.src/features/formulaCopy/FormulaCopyService.ts (1)
186-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
extractLatexSourceto a standalone helper module.Import the helper directly from
DOMContentExtractor, and re-export it fromFormulaCopyService.tsto preserve existing callers. This avoids loadingtemml,webextension-polyfill, logger, and i18n dependencies for the two extractor call sites.🤖 Prompt for 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. In `@src/features/formulaCopy/FormulaCopyService.ts` at line 186, Move extractLatexSource out of the FormulaCopyService class into a standalone helper module, then import that helper directly in DOMContentExtractor. Re-export the helper from FormulaCopyService.ts so existing callers remain compatible, without retaining the heavy service dependencies in the extractor paths.
🤖 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/features/export/services/DOMContentExtractor.ts`:
- Around line 393-406: Update DOMContentExtractor.extractLinkLabel to normalize
and collapse whitespace in the resolved link text before constructing the
returned html and text values, while preserving the existing inline-content and
fallback behavior.
---
Nitpick comments:
In `@src/features/export/services/DOMContentExtractor.ts`:
- Around line 486-496: In src/features/export/services/DOMContentExtractor.ts
lines 486-496 within processNodes, evaluate the
math-block/katex-display/data-math/data-math-source marker before calling
FormulaCopyService.extractLatexSource, and only extract when the marker passes.
Apply the same ordering in processInlineContent at lines 888-897 so both
recursive paths avoid scanning non-math elements.
In `@src/features/formulaCopy/FormulaCopyService.ts`:
- Line 186: Move extractLatexSource out of the FormulaCopyService class into a
standalone helper module, then import that helper directly in
DOMContentExtractor. Re-export the helper from FormulaCopyService.ts so existing
callers remain compatible, without retaining the heavy service dependencies in
the extractor paths.
In `@src/features/plugins/builtin/chatgptExport/tempHandoff.test.ts`:
- Around line 139-149: Ensure every PluginScope created in the tests is disposed
from an afterEach teardown hook, including scopes created in the test blocks
around addComposer and handoffTemporaryChat. Register scopes through a shared
createScope helper backed by a list, replace direct new PluginScope() calls with
createScope(), and remove trailing await scope.dispose() statements.
🪄 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: 8f25a327-82a3-46fe-8d10-bfad9bb4201f
📒 Files selected for processing (7)
src/features/export/services/DOMContentExtractor.tssrc/features/export/services/__tests__/DOMContentExtractor.test.tssrc/features/formulaCopy/FormulaCopyService.tssrc/features/plugins/builtin/chatgptExport/conversation.test.tssrc/features/plugins/builtin/chatgptExport/conversation.tssrc/features/plugins/builtin/chatgptExport/tempHandoff.test.tssrc/features/plugins/builtin/chatgptExport/tempHandoff.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/plugins/builtin/chatgptExport/tempHandoff.ts
- src/features/plugins/builtin/chatgptExport/conversation.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9ba16e526
ℹ️ 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".
Co-authored-by: Codex <codex@users.noreply.github.com>
Co-authored-by: Codex <codex@users.noreply.github.com>
f414867 to
72ad036
Compare
|
Addressed the three folded review nits in \72ad036: formula extraction now runs only for marked math nodes, test scopes are disposed from \�fterEach, and the pure LaTeX-source helper is split out so DOM export no longer imports the heavier formula-copy service. The existing static API remains compatible. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72ad036f03
ℹ️ 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".
|
@codex review Please review the current HEAD ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa6165d832
ℹ️ 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".
|
Review boundary: the Codex reviewer responded to the explicitly requested comprehensive one-pass review with the two findings above; both are now addressed in 7003e4d with regression coverage. Treat that pass as complete. Do not restart the serial drip-feed loop by posting additional incremental findings against commits whose sole purpose is addressing this batch. Any later observations belong to a separate maintainer-requested review cycle and will be treated as out of scope here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7003e4dd93
ℹ️ 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".
|
Review boundary: these two findings were on code that predates 7003e4d and therefore arrived after the prior one-pass review was explicitly declared complete. Both are nevertheless fixed in e00eb0a with regression coverage, and the threads are resolved. This review cycle is closed; please do not continue with serial incremental findings unless a maintainer explicitly requests a new review round. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e00eb0a401
ℹ️ 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".
Rework Nagi-ovo#921 to preserve its unique temporary-chat handoff without duplicating the conversation exporter added by Nagi-ovo#920. Co-authored-by: Tan Chuping <81346625+DaltonWritingCenter@users.noreply.github.com> Co-authored-by: Codex <codex@users.noreply.github.com>
|
Hi @TanChuping, thanks for your interest in contributing! Voyager automatically admits focused bug fixes when the PR title uses This PR does not use a Feature PRs require prior maintainer alignment: please open a GitHub Discussion or issue describing the change first. Contributors listed in If this gate classified the pull request incorrectly, reply and tag a maintainer listed in Patch size: 94 changed files, 9465 changed lines. See https://github.com/Nagi-ovo/voyager/blob/main/.github/CONTRIBUTING.md for the contribution policy. |
Nagi-ovo
changed the title
|
Thanks so much for the original handoff work. I narrowed the PR after #920 and pushed the tested implementation successfully. One GitHub-only cleanup remains: because the fast-forward update placed current main on the first-parent side, GitHub is still displaying the old 94-file merge-base even though the actual head tree differs from main in only 11 files. I prepared a lineage-only merge commit that changes no source tree and restores the focused diff, but “Allow edits by maintainers” is now disabled, so the fork rejected it. Could you please re-enable maintainer edits briefly? I will push only that ancestry cleanup and then leave the branch alone. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64955bd012
ℹ️ 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".
| return ( | ||
| insertTextIntoChatInput(existing ? `\n\n${text}` : text, input) && | ||
| readComposerText(input).includes(text) | ||
| ); |
There was a problem hiding this comment.
When ChatGPT uses its normal contenteditable composer, every planned inline handoff and attachment directive contains newlines, but the insertion helper represents those breaks as paragraphs or <br> elements while textContent concatenates their text without newline characters. Consequently this includes(text) check reports delivery-failed even after visibly inserting the handoff, retains the pending payload, and can duplicate it on a later retry. Verify normalized editor text/DOM state rather than comparing the original multiline string directly.
Useful? React with 👍 / 👎.
Nagi-ovo
left a comment
Nagi-ovo
left a comment
There was a problem hiding this comment.
Thanks for sticking with this. The plugin boundary and reuse of the shared #920 collector now look right, and CI is green. I found four blockers in the handoff/recovery path; the inline comments include deterministic reproductions and the regression cases we need before merge.
|
|
||
| function currentComposer(excluded?: HTMLElement): HTMLElement | null { | ||
| return ( | ||
| Array.from(document.querySelectorAll<HTMLElement>(COMPOSER_SELECTOR)).find( |
There was a problem hiding this comment.
This combined selector returns the first match in DOM order, not #prompt-textarea first. I reproduced a page with another contenteditable[role=textbox] before the real prompt: the handoff was inserted into that editor, the real prompt stayed empty, and pending state was cleared as ready. Please resolve strong composer selectors sequentially and constrain any generic fallback to the actual ChatGPT composer; add a competing-editor regression test.
| toggle.click(); | ||
| for (let attempt = 0; attempt < 16; attempt += 1) { | ||
| if (!isTemporaryChat()) { | ||
| const replacement = currentComposer(temporaryComposer); |
There was a problem hiding this comment.
This assumes leaving temporary mode replaces the composer DOM node, but ChatGPT can reuse the same #prompt-textarea during SPA reconciliation. The exact production path then waited about 4.7s, clicked New Chat once, and returned null even though a usable normal composer was present. Please determine readiness from temporary-mode/route state instead of node identity, and add a same-node-reuse test.
| } | ||
|
|
||
| function writePending(delivery: HandoffDelivery, accountScope: string): void { | ||
| sessionStorage.setItem( |
There was a problem hiding this comment.
This persists the full temporary-chat transcript in ChatGPT page-owned sessionStorage. That storage is not extension-private, and failure paths can leave the payload there until a later read or tab close. Please follow the existing fork handoff precedent and keep pending content in extension-owned storage with TTL and removal on success/cancel/expiry; also handle storage-write failure before any full-page navigation.
| return 'account-mismatch'; | ||
| } | ||
| if (isTemporaryChat()) return null; | ||
| const input = await findComposer(scope, 6_000); |
There was a problem hiding this comment.
Abort during resumePendingHandoff does not clear pending state. I reproduced disabling the plugin while this wait was active, then re-enabling it within the TTL: the old transcript was inserted automatically. Since the plugin promises clean cancellation, please clear or otherwise invalidate the pending operation on plugin disposal and add a disable-mid-resume regression test.
Co-authored-by: Codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a234a3b1d
ℹ️ 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".
Co-authored-by: Codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6c80bb827
ℹ️ 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".
Three-round SOL/MAX cold-read review and verificationCommit Priority summary
Disposition: 17 fully fixed, 2 proportionally addressed with an explicit retained boundary, and 1 not adopted after trigger/impact/cost analysis. Findings and disposition
Verification
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 188c9442fb
ℹ️ 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".
3 participants
Description
Scope after #920
#920 provides the shared ChatGPT conversation exporter. This PR is therefore limited to the unique temporary-chat handoff feature instead of maintaining a second Markdown/JSON/PDF exporter. It reuses #920's collector and ships under a separate plugin ID, so export and handoff can be enabled together.
Thanks so much to @TanChuping for the original implementation and for identifying this useful workflow.
Three-round SOL/MAX cold-read review
Tested tree:
188c944Three independent SOL/MAX cold-read reviews were run sequentially. Each round reviewed the implementation after the prior round's fixes; the third round read the complete temporary-chat handoff feature rather than only the diff. A separate three-pass final maintainer review followed before push.
Disposition: 17 fully fixed, 2 proportionally addressed with an explicit retained boundary, and 1 not adopted after trigger/impact/cost analysis.
pagehide/pageshow, and the abort reason is captured synchronously./u/Nroute changes are already rejected. Covering a deliberate unscoped-account switch would require account APIs or another cross-page confirmation state and is disproportionate to a non-normal flow.beforeunloadsequence could leave sticky navigation state.pagehide/pageshowlifecycle events control unload state.<br>elements, and block boundaries./g/<gpt>and/u/N/g/<gpt>contexts are preserved.aria-describedby.临时对话反悔to临时对话续接.临时对话反悔is restored, Traditional Chinese uses暫時對話反悔, and both are locked by tests.Verification
verify-release-privacy.test.tsfor signed-appembedded.provisionprofilefilesconsolewarnings in the background entrypointgit diff --checkpassedChecklist
Follow-up to #920. Preserves the unique handoff portion of the original #921 contribution.