feat(export): add ChatGPT export - #920
Conversation
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds opt-in ChatGPT conversation export through platform adapters, virtualized turn collection, snapshot-aware rendering, bounded image processing, cancellation, localized progress UI, and native plugin lifecycle management. Existing Gemini export paths use the shared adapter contract. ChangesChatGPT export integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ChatGPT
participant PluginHost
participant ChatGPTAdapter
participant ExportService
participant ImageFetcher
ChatGPT->>PluginHost: match chatgpt.com and start plugin
PluginHost->>ChatGPTAdapter: collect and materialize selected turns
ChatGPTAdapter->>ExportService: provide extracted turn snapshots
ExportService->>ImageFetcher: fetch images with limits and cancellation
ExportService-->>ChatGPT: download the selected export
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 4fdb78fe7b
ℹ️ 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".
| // ChatGPT retains these top-level virtual-list items even when it unloads | ||
| // their inner message DOM. Their DOM order and data-turn-id-container value | ||
| // are consequently the only reliable source for selection identity/order. | ||
| return turnContainers.slice(1).map((turn) => ({ |
There was a problem hiding this comment.
On ChatGPT pages where the first [data-turn-id-container] is an actual message, this unconditional slice(1) removes it before selection mode is built, so no checkbox is attached to the first prompt and Select all/role filters can never include it. A single-turn conversation can therefore export only the assistant response; skip only a verified sentinel/unknown container instead of always discarding index 0.
Useful? React with 👍 / 👎.
| if (pendingUser) { | ||
| pendingUser.assistant = assistantContent.text; | ||
| pendingUser.assistantContent = assistantContent; | ||
| turns.push(pendingUser); |
There was a problem hiding this comment.
When a user manually selects a user turn and a later assistant turn while leaving intervening messages unselected, selectedContainers has already filtered those intervening turns out, so this pendingUser branch merges unrelated prompt and answer into one exported turn. That corrupts partial exports; preserve original adjacency or flush the pending user when any unselected turn sits between the selected user and assistant.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/export/services/DOMContentExtractor.ts (1)
383-388: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore Shadow DOM traversal in
processNodes.processNodesexcludes Shadow DOM children.searchAllrecovers only matching code selectors, so other exportable Shadow DOM content remains unprocessed. Add regression coverage.🤖 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 383 - 388, Update processNodes to traverse and process all children of container.shadowRoot, rather than relying solely on searchAll’s matching selectors; preserve normal light-DOM processing and add regression coverage verifying non-code exportable Shadow DOM content is included.
🧹 Nitpick comments (10)
src/features/export/services/ConversationExportService.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type-only import for
ExportPlatformAdapter.
ExportPlatformAdapteris used only in the signature ofsetExportAdapter. A value import keepsplatformAdapters.tsin the module graph of every consumer ofConversationExportService. That module runsSiteRegistry.createDefault()at module scope, so the side effect and its dependencies are pulled into the service layer and into unrelated bundles.♻️ Proposed fix
-import { ExportPlatformAdapter } from '`@pages/content/export/adapter/platformAdapters`'; +import type { ExportPlatformAdapter } from '`@pages/content/export/adapter/platformAdapters`';🤖 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/ConversationExportService.ts` at line 6, Change the ExportPlatformAdapter import used by ConversationExportService to a type-only import, preserving its use in the setExportAdapter signature and preventing platformAdapters runtime initialization from entering the service module graph.src/pages/content/export/adapter/platformAdapters.ts (2)
494-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
chatgptResolveConversationRootduplicates the default branch ofresolveConversationRootForPlatform.This function ignores
userSelectorsand walksCHATGPT_ROOT_CANDIDATES, which is exactly whatresolveConversationRootForPlatformdoes in itsdefaultbranch for non-Gemini sites. Consider keeping one implementation and letting the other delegate.🤖 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/pages/content/export/adapter/platformAdapters.ts` around lines 494 - 503, Update chatgptResolveConversationRoot to delegate to resolveConversationRootForPlatform instead of independently ignoring userSelectors and iterating CHATGPT_ROOT_CANDIDATES. Reuse the existing default-branch implementation while preserving the current document input and HTMLElement return behavior, and remove the duplicated root-resolution logic.
205-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the exported root-candidate list instead of redefining it.
src/pages/content/export/conversationDom.tsnow exportsCONVERSATION_ROOT_CANDIDATESwith the same four selectors.GEMINI_ROOT_CANDIDATESduplicates that list. The two lists can drift.♻️ Proposed refactor
-import { resolveConversationRoot } from '../conversationDom'; +import { CONVERSATION_ROOT_CANDIDATES, resolveConversationRoot } from '../conversationDom'; @@ -const GEMINI_ROOT_CANDIDATES = [ - '`#chat-history`', - 'infinite-scroller.chat-history', - 'chat-window-content', - 'main', -]; +const GEMINI_ROOT_CANDIDATES = CONVERSATION_ROOT_CANDIDATES;🤖 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/pages/content/export/adapter/platformAdapters.ts` around lines 205 - 210, Remove the duplicate GEMINI_ROOT_CANDIDATES definition and import and reuse the exported CONVERSATION_ROOT_CANDIDATES from conversationDom.ts wherever the Gemini root selectors are needed.src/pages/content/export/index.ts (2)
2539-2555: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe Gemini fallback path returns a no-op cleanup while a listener stays registered.
Line 2540 registers
onStorageChangeonchrome.storage.onChanged, and line 2551 returns() => {}. The listener and the toolbar are then only released onbeforeunload. The new return type ofstartExportButtonis a cleanup contract, so callers that stop the feature leave this listener and toolbar active.♻️ Proposed fix
- return () => {}; + return () => { + toolbarHandle?.remove(); + toolbarHandle = null; + try { + chrome.storage?.onChanged?.removeListener(onStorageChange); + } catch {} + };🤖 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/pages/content/export/index.ts` around lines 2539 - 2555, Update the Gemini fallback branch in startExportButton so its returned cleanup function removes the registered chrome.storage.onChanged listener and releases the injected toolbar/button resources, rather than returning a no-op. Preserve the beforeunload cleanup while making cleanup safe to invoke when the feature is stopped explicitly.
94-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTwo modules write the same global export adapter at import time.
ConversationExportService.setExportAdaptersets a single static field onDOMContentExtractor. Two content-script modules call it at module scope, so import order decides the effective adapter. Both resolve from the same URL today, so behavior is correct now, but the contract is implicit and breaks if either module ever loads on a different host.
src/pages/content/export/index.ts#L94-L96: move the registration into an explicit startup function, for example the beginning ofstartExportButton, instead of module scope.src/pages/content/deepResearch/menuButton.ts#L33-L35: move the registration intoinjectDownloadButtonor a shared startup path, so the Deep Research surface does not race the export entry point.🤖 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/pages/content/export/index.ts` around lines 94 - 96, Move adapter registration out of module scope in src/pages/content/export/index.ts lines 94-96 and perform it at the start of startExportButton. Likewise move the registration from src/pages/content/deepResearch/menuButton.ts lines 33-35 into injectDownloadButton or a shared startup path, so each surface explicitly initializes its resolved adapter without import-order races.src/features/export/services/__tests__/DOMContentExtractor.test.ts (1)
64-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe ChatGPT tile test exercises a hand-written adapter, not the production one.
getUserAttachmentCandidateshere re-implements the Gemini and ChatGPT branches fromplatformAdapters.ts. The new test at lines 117-141 therefore validates this local copy. A regression inchatgptGetUserAttachmentCandidateswould not fail the test, and the two copies can drift.Consider exporting the ChatGPT adapter builder and registering the real ChatGPT adapter for that one test case.
🤖 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__/DOMContentExtractor.test.ts` around lines 64 - 87, Replace the hand-written getUserAttachmentCandidates implementation in the test adapter with the production ChatGPT adapter from platformAdapters.ts. Export the ChatGPT adapter builder, register the real adapter for the ChatGPT tile test case, and keep the existing Gemini adapter setup unchanged.src/pages/content/export/platformConversationDom.ts (1)
16-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
resolveConversationRootForPlatformhelper. No callers exist; export flow usesexportAdapter.resolveConversationRoot(...).🤖 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/pages/content/export/platformConversationDom.ts` around lines 16 - 35, Remove the unused exported resolveConversationRootForPlatform helper and its associated implementation, leaving platform root resolution through exportAdapter.resolveConversationRoot(...) unchanged.src/features/export/services/DOMContentExtractor.ts (1)
93-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead assignment and move the Gemini selector into the adapter.
Two points:
- Line 100 assigns
result.text, but Line 131 reassigns it unconditionally. Nothing reads it in between. Delete Line 100.- Line 96 queries
.query-text-line, a Gemini-specific selector, inside the platform-neutral extractor, and then passes the result to every adapter. The ChatGPT adapter must ignore this argument. TheextractUserTextsignature would be cleaner as(element: HTMLElement, textParts: string[]), with each adapter owning its own query.♻️ Proposed change for point 1
const textLines = element.querySelectorAll<HTMLElement>('.query-text-line'); const textParts: string[] = []; this.exportAdapter.extractUserText(textLines, textParts, element); - result.text = textParts.join('\n'); - // Build HTML representation🤖 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 93 - 100, Remove the intermediate result.text assignment and update extractUserText to accept only (element: HTMLElement, textParts: string[]). Move the .query-text-line lookup into the Gemini adapter, and update DOMContentExtractor plus all adapter implementations and call sites to use the revised signature while preserving platform-specific extraction behavior.src/features/export/services/MarkdownFormatter.ts (1)
245-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
formatPromptHeadingignoresturn.userContent.
formatTurnnow prefersturn.userContentin all four of its user branches.formatPromptHeadingstill reads onlyturn.userElementandturn.user. For a ChatGPT turn there is nouserElement, sodomContentisnullandhasMediafalls back to a regex over the text.The practical effect is limited, because
DOMContentExtractor.extractUserContentembedsmarkdown intotext, so the regex usually matches. It does not match an attachment-only turn, wheretextcontains📎 nameand no image markdown. In that caseomitUserSectionbecomestrueat Line 140 and the attachment list disappears from the export.Read
turn.userContentfirst for consistency with the rest offormatTurn.♻️ Proposed change
private static formatPromptHeading(turn: ChatTurn): { hasMedia: boolean; text: string } { const fallback = this.formatContent(turn.user); - const domContent = turn.userElement - ? DOMContentExtractor.extractUserContent(turn.userElement) - : null; + const domContent = + turn.userContent ?? + (turn.userElement ? DOMContentExtractor.extractUserContent(turn.userElement) : null); const extracted = domContent?.text || fallback;🤖 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/MarkdownFormatter.ts` around lines 245 - 263, Update formatPromptHeading to read turn.userContent first, matching the user-content precedence used by formatTurn, while retaining turn.userElement/turn.user as fallbacks. Ensure the selected content preserves attachment-only media detection so omitUserSection does not remove attachment lists.src/pages/content/export/adapter/chatgpt.ts (1)
71-99: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a fast path for already-mounted turns.
materializeChatGptTurnContaineralways scrolls and always waits at leastminWaitMsplusidleMs.buildChatGptTurnsForSelectioncalls it sequentially for every selected container. A selection of 100 turns therefore costs at least ~20 seconds, and up to 300 seconds when containers keep timing out. Turns that are already mounted need no scroll and no wait.Check for mounted content first and return immediately when the role is already resolved.
♻️ Proposed fast path
export async function materializeChatGptTurnContainer( turn: ChatGptTurnContainer, ): Promise<ChatGptTurnContainer> { + const contentSelectors = [USER_MESSAGE_SELECTOR, ASSISTANT_MESSAGE_SELECTOR, IMAGEGEN_SELECTOR]; + + // Already mounted: no scroll and no stabilization wait are needed. + const mountedRole = resolveTurnRole(turn.container); + if (mountedRole !== 'unknown') { + return { ...turn, role: mountedRole }; + } + turn.container.scrollIntoView({ block: 'center', behavior: 'auto', }); - // 等待角色确定 - const contentSelectors = [USER_MESSAGE_SELECTOR, ASSISTANT_MESSAGE_SELECTOR, IMAGEGEN_SELECTOR]; - const before = computeConversationFingerprint(turn.container, contentSelectors, 10);🤖 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/pages/content/export/adapter/chatgpt.ts` around lines 71 - 99, Update materializeChatGptTurnContainer to resolve the current role and detect already-mounted content before scrolling or waiting; when the role is resolved, return the turn immediately without calling scrollIntoView or waitForConversationFingerprintChangeOrTimeout. Preserve the existing stabilization flow for unresolved roles, re-resolving the role after waiting.
🤖 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 `@manifest.json`:
- Line 44: Add https://chat.openai.com/* to the optional_host_permissions list
in manifest.json and mirror the same permission in manifest.dev.json. Update the
opt-in assertions in src/core/utils/__tests__/manifestPermissions.test.ts to
verify both ChatGPT hosts and their corresponding content-script exclusions.
- Line 44: Remove <all_urls> from optional_host_permissions in both
manifest.json (line 44) and manifest.dev.json (line 58) if
ensureGeneratedUiCapturePermission() supports narrower access; otherwise
document the requirement and obtain explicit approval for retaining it in both
manifests.
In `@src/core/utils/__tests__/manifestPermissions.test.ts`:
- Around line 77-79: Update the optional_host_permissions assertion in the
manifest permissions test to compare against the complete approved allowlist,
rather than using expect.arrayContaining with only <all_urls>. Include every
expected host permission and ensure unexpected entries cause the test to fail.
In `@src/features/export/services/__tests__/ConversationExportService.test.ts`:
- Line 20: Move setExportAdapter below the JSDOM global setup in
ConversationExportService tests so resolveExportAdapter() runs only after the
test window has been assigned. Preserve the existing adapter initialization
while ensuring it reads the configured JSDOM window.
In `@src/features/export/services/DOMContentExtractor.ts`:
- Around line 70-73: Remove or gate the unconditional debug log in the
user-content extraction flow to match the class’s existing this.DEBUG logging
behavior, and update the JSDoc above extractUserContent to document only its
actual parameters, removing the stale imageSelectors description.
- Around line 467-479: Assistant-image deduplication is inert because the
tracking set is read-only, never updated, and omitted during recursion. In
src/features/export/services/DOMContentExtractor.ts#L467-L479, change
processNodes and extractAssistantImage to accept Set<string>, add each emitted
src to processedImageSrcs, and pass the set through recursive calls at Line 447.
In src/features/export/services/__tests__/ImageExportService.test.ts#L18-L40,
update the test adapter to add emitted sources and assert duplicate sources are
emitted only once.
- Around line 517-532: Update processNodes recursion for generic containers so
mixed-content elements preserve their direct text nodes as well as descendant
elements. Before or during recursive processing of child, extract and append its
direct text-node content while retaining the existing recursive handling for
element children; keep leaf extraction unchanged for elements without child
elements.
In `@src/features/export/services/PDFPrintService.ts`:
- Line 58: Replace the `platform: 'web'` value in the PDF export metadata with
the proper user-facing display name `Gemini`, preserving the existing
`ConversationMetadata` contract and ensuring `extractTitleFromURL` and
`getPrintDialogTitle` continue producing display titles.
- Around line 1120-1136: Update the normalizeConversationTitle calls in
renderHeader to pass the current platform, matching getPrintDialogTitle. Ensure
the PDF cover-page title strips the platform suffix, such as “ - ChatGPT,”
consistently with the print dialog while preserving existing title
normalization.
In `@src/pages/content/export/adapter/chatgpt.ts`:
- Around line 133-150: Update the selected-turn processing loop around
materializeChatGptTurnContainer to emit console.warn messages containing the
container ID when a materialized turn has role "unknown" or when the user-role
branch has no userElement. Preserve the existing skip behavior while ensuring
both omission paths are diagnosable.
In `@src/pages/content/export/adapter/platformAdapters.ts`:
- Around line 505-508: Update chatgptExtractUserImage to use a selector that
targets only actual inline user-content images, excluding file-tile icons and
other decorative images. Verify the selector against a ChatGPT user turn
containing both a file attachment and an inline image so only the inline image
is returned.
In `@src/pages/content/export/index.ts`:
- Around line 683-697: Update resolveSelectionMessages to stop unconditionally
removing the first collected turn container; map all turnContainers so the real
user message with sequence 0 remains included. Only exclude an explicitly
identified non-message container if such identification already exists.
---
Outside diff comments:
In `@src/features/export/services/DOMContentExtractor.ts`:
- Around line 383-388: Update processNodes to traverse and process all children
of container.shadowRoot, rather than relying solely on searchAll’s matching
selectors; preserve normal light-DOM processing and add regression coverage
verifying non-code exportable Shadow DOM content is included.
---
Nitpick comments:
In `@src/features/export/services/__tests__/DOMContentExtractor.test.ts`:
- Around line 64-87: Replace the hand-written getUserAttachmentCandidates
implementation in the test adapter with the production ChatGPT adapter from
platformAdapters.ts. Export the ChatGPT adapter builder, register the real
adapter for the ChatGPT tile test case, and keep the existing Gemini adapter
setup unchanged.
In `@src/features/export/services/ConversationExportService.ts`:
- Line 6: Change the ExportPlatformAdapter import used by
ConversationExportService to a type-only import, preserving its use in the
setExportAdapter signature and preventing platformAdapters runtime
initialization from entering the service module graph.
In `@src/features/export/services/DOMContentExtractor.ts`:
- Around line 93-100: Remove the intermediate result.text assignment and update
extractUserText to accept only (element: HTMLElement, textParts: string[]). Move
the .query-text-line lookup into the Gemini adapter, and update
DOMContentExtractor plus all adapter implementations and call sites to use the
revised signature while preserving platform-specific extraction behavior.
In `@src/features/export/services/MarkdownFormatter.ts`:
- Around line 245-263: Update formatPromptHeading to read turn.userContent
first, matching the user-content precedence used by formatTurn, while retaining
turn.userElement/turn.user as fallbacks. Ensure the selected content preserves
attachment-only media detection so omitUserSection does not remove attachment
lists.
In `@src/pages/content/export/adapter/chatgpt.ts`:
- Around line 71-99: Update materializeChatGptTurnContainer to resolve the
current role and detect already-mounted content before scrolling or waiting;
when the role is resolved, return the turn immediately without calling
scrollIntoView or waitForConversationFingerprintChangeOrTimeout. Preserve the
existing stabilization flow for unresolved roles, re-resolving the role after
waiting.
In `@src/pages/content/export/adapter/platformAdapters.ts`:
- Around line 494-503: Update chatgptResolveConversationRoot to delegate to
resolveConversationRootForPlatform instead of independently ignoring
userSelectors and iterating CHATGPT_ROOT_CANDIDATES. Reuse the existing
default-branch implementation while preserving the current document input and
HTMLElement return behavior, and remove the duplicated root-resolution logic.
- Around line 205-210: Remove the duplicate GEMINI_ROOT_CANDIDATES definition
and import and reuse the exported CONVERSATION_ROOT_CANDIDATES from
conversationDom.ts wherever the Gemini root selectors are needed.
In `@src/pages/content/export/index.ts`:
- Around line 2539-2555: Update the Gemini fallback branch in startExportButton
so its returned cleanup function removes the registered chrome.storage.onChanged
listener and releases the injected toolbar/button resources, rather than
returning a no-op. Preserve the beforeunload cleanup while making cleanup safe
to invoke when the feature is stopped explicitly.
- Around line 94-96: Move adapter registration out of module scope in
src/pages/content/export/index.ts lines 94-96 and perform it at the start of
startExportButton. Likewise move the registration from
src/pages/content/deepResearch/menuButton.ts lines 33-35 into
injectDownloadButton or a shared startup path, so each surface explicitly
initializes its resolved adapter without import-order races.
In `@src/pages/content/export/platformConversationDom.ts`:
- Around line 16-35: Remove the unused exported
resolveConversationRootForPlatform helper and its associated implementation,
leaving platform root resolution through
exportAdapter.resolveConversationRoot(...) unchanged.
🪄 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: d2a5cc3e-1b46-4521-a42e-f16be384a30e
📒 Files selected for processing (47)
manifest.dev.jsonmanifest.jsonpublic/contentStyle.csssrc/core/utils/__tests__/manifestPermissions.test.tssrc/features/export/services/ConversationExportService.tssrc/features/export/services/DOMContentExtractor.tssrc/features/export/services/ImageExportService.tssrc/features/export/services/MarkdownFormatter.tssrc/features/export/services/PDFPrintService.tssrc/features/export/services/__tests__/ConversationExportService.test.tssrc/features/export/services/__tests__/DOMContentExtractor.test.tssrc/features/export/services/__tests__/ImageExportService.test.tssrc/features/export/services/__tests__/MarkdownFormatter.test.tssrc/features/export/services/__tests__/PDFPrintService.safari.test.tssrc/features/export/services/__tests__/PDFPrintService.test.tssrc/features/export/types/export.tssrc/features/plugins/builtin/builtin.test.tssrc/features/plugins/builtin/chatgptExport/index.tssrc/features/plugins/builtin/chatgptExport/runtime.test.tssrc/features/plugins/builtin/chatgptExport/runtime.tssrc/features/plugins/builtin/index.tssrc/features/plugins/runtime/PluginHost.test.tssrc/features/plugins/sites/adapters/chatgpt.tssrc/locales/ar/messages.jsonsrc/locales/en/messages.jsonsrc/locales/es/messages.jsonsrc/locales/fr/messages.jsonsrc/locales/ja/messages.jsonsrc/locales/ko/messages.jsonsrc/locales/pt/messages.jsonsrc/locales/ru/messages.jsonsrc/locales/zh/messages.jsonsrc/locales/zh_TW/messages.jsonsrc/pages/content/deepResearch/menuButton.tssrc/pages/content/export/__tests__/exportCollectingBanner.test.tssrc/pages/content/export/adapter/__tests__/chatgpt.test.tssrc/pages/content/export/adapter/chatgpt.tssrc/pages/content/export/adapter/platformAdapters.tssrc/pages/content/export/adapter/type.tssrc/pages/content/export/conversationDom.tssrc/pages/content/export/exportCollectingBanner.tssrc/pages/content/export/index.tssrc/pages/content/export/platformConversationDom.tssrc/pages/content/fork/__tests__/chatPairs.test.tssrc/pages/content/fork/__tests__/index.test.tssrc/pages/content/index.tsxsrc/pages/content/pluginNativeRegistration.ts
| "https://*.ggpht.com/*" | ||
| ], | ||
| "optional_host_permissions": ["<all_urls>"], | ||
| "optional_host_permissions": ["https://chatgpt.com/*", "<all_urls>"], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add https://chat.openai.com/* to the optional ChatGPT contract.
The PR objective includes both ChatGPT domains, but the manifests and regression test cover only https://chatgpt.com/*. Add the second host to both manifests and assert both hosts in the opt-in test.
manifest.json#L44-L44: addhttps://chat.openai.com/*.manifest.dev.json#L58-L58: mirror the production permission list.src/core/utils/__tests__/manifestPermissions.test.ts#L82-L91: assert both hosts and both content-script exclusions.
📍 Affects 3 files
manifest.json#L44-L44(this comment)manifest.dev.json#L58-L58src/core/utils/__tests__/manifestPermissions.test.ts#L82-L91
🤖 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 `@manifest.json` at line 44, Add https://chat.openai.com/* to the
optional_host_permissions list in manifest.json and mirror the same permission
in manifest.dev.json. Update the opt-in assertions in
src/core/utils/__tests__/manifestPermissions.test.ts to verify both ChatGPT
hosts and their corresponding content-script exclusions.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- manifest files ---'
git ls-files '*manifest*.json' '.github/docs/REGRESSION_NOTES.md' | sed -n '1,120p'
printf '%s\n' '--- relevant manifest sections ---'
for f in manifest.json manifest.dev.json; do
if [ -f "$f" ]; then
echo "### $f"
nl -ba "$f" | sed -n '1,100p'
fi
done
printf '%s\n' '--- all_urls and permission-related references ---'
rg -n --hidden -S '<all_urls>|optional_host_permissions|host_permissions|permissions|contains.*Permission|permission' \
-g '!node_modules' -g '!dist' -g '!build' . | sed -n '1,300p'
printf '%s\n' '--- ChatGPT/export references ---'
rg -n --hidden -i -S 'chatgpt|export' \
-g '!node_modules' -g '!dist' -g '!build' . | sed -n '1,300p'Repository: Nagi-ovo/voyager
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- manifest excerpts ---'
for f in manifest.json manifest.dev.json; do
echo "### $f"
awk 'NR >= 20 && NR <= 95 { printf "%5d %s\n", NR, $0 }' "$f"
done
printf '%s\n' '--- current diff for manifests and permission code ---'
git diff -- manifest.json manifest.dev.json src/pages/background/index.ts vite.config.base.ts vite.config.chrome.ts vite.config.firefox.ts vite.config.safari.ts | sed -n '1,260p'
printf '%s\n' '--- generated UI capture implementation ---'
awk 'NR >= 80 && NR <= 120 { printf "%5d %s\n", NR, $0 }
NR >= 1580 && NR <= 1660 { printf "%5d %s\n", NR, $0 }' src/pages/background/index.ts
printf '%s\n' '--- export call sites ---'
awk 'NR >= 160 && NR <= 220 { printf "%5d %s\n", NR, $0 }' src/pages/content/export/index.ts
rg -n -S 'requestGeneratedUiCapturePermission|GENERATED_UI_CAPTURE_PERMISSION_ORIGINS|capture.*generated|generated.*capture' src/pages src/core | sed -n '1,160p'
printf '%s\n' '--- permission guidance and regression notes ---'
awk 'NR >= 50 && NR <= 68 { printf "%5d %s\n", NR, $0 }' .agents/skills/voyager-contribute/references/browser-testing.md
awk 'NR >= 1 && NR <= 55 { printf "%5d %s\n", NR, $0 }' docs/en/privacy.md
awk 'NR >= 1 && NR <= 30 { printf "%5d %s\n", NR, $0 }' src/pages/content/changelog/notes/1.5.3.mdRepository: Nagi-ovo/voyager
Length of output: 21755
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- generated UI selector and export flow ---'
rg -n -S -C 4 \
'GENERATED_UI_FRAME_SELECTOR|ensureGeneratedUiScreenshotPermission|captureGeneratedUiScreenshots|GENERATED_UI_CAPTURE_PERMISSION_MESSAGE_TYPE|captureVisibleTab' \
src/pages/content/export/index.ts src/pages/background/index.ts src/pages/background/runtimeMessageRouting.ts | sed -n '1,300p'
printf '%s\n' '--- background message handler ---'
awk 'NR >= 1740 && NR <= 1810 { printf "%5d %s\n", NR, $0 }' src/pages/background/index.ts
printf '%s\n' '--- export entry points that request the permission ---'
rg -n -S -C 8 \
'ensureGeneratedUiScreenshotPermission|captureGeneratedUiScreenshots' \
src/pages/content/export/index.ts | sed -n '1,260p'
printf '%s\n' '--- supported host and activeTab declarations ---'
python3 - <<'PY'
import json
for path in ("manifest.json", "manifest.dev.json"):
with open(path, encoding="utf-8") as f:
m = json.load(f)
print(path)
print("permissions:", m.get("permissions"))
print("host_permissions:", m.get("host_permissions"))
print("optional_host_permissions:", m.get("optional_host_permissions"))
print("content_script_matches:", [
match
for script in m.get("content_scripts", [])
for match in script.get("matches", [])
])
print("web_accessible_matches:", [
item.get("matches", [])
for item in m.get("web_accessible_resources", [])
])
PYRepository: Nagi-ovo/voyager
Length of output: 17467
🌐 Web query:
site:developer.chrome.com/docs/extensions/reference/api/tabs captureVisibleTab activeTab <all_urls> permission
💡 Result:
To use the chrome.tabs.captureVisibleTab method in a Chrome extension, you must possess either the activeTab permission or the <all_urls> host permission [1]. The activeTab permission is often preferred because it grants temporary access to the current tab only in response to a direct user action (such as clicking an extension icon), and it does not trigger the broad permission warnings associated with <all_urls> [1]. Additionally, activeTab allows the capture of sensitive pages—such as chrome:-scheme pages, other extension pages, and data: URLs—which cannot be captured using only <all_urls> [1]. If your extension requires the ability to capture tabs without a direct user interaction, <all_urls> or specific host permissions are required, though these will not allow access to the aforementioned sensitive restricted pages [1]. Note that file URLs can only be captured if the user has explicitly granted the extension file access [1].
Citations:
🌐 Web query:
site:developer.chrome.com/docs/extensions/develop/concepts/activeTab activeTab user gesture content script click captureVisibleTab
💡 Result:
The activeTab permission grants an extension temporary, elevated access to the currently active tab only after an explicit user gesture, such as clicking an extension action, using a context menu item, triggering a keyboard shortcut, or accepting an omnibox suggestion [1]. Regarding your specific query points: Content Scripts: Content scripts cannot directly call chrome.tabs APIs, including captureVisibleTab. Content scripts run in the context of the web page and must communicate with a background service worker or popup via messaging to trigger API calls that require extension-level privileges [1]. CaptureVisibleTab and Permissions: The chrome.tabs.captureVisibleTab API requires the <all_urls> host permission or the activeTab permission. If your extension uses activeTab, you must trigger the capture logic from your background script or popup in response to a user gesture that activates the tab permission [1]. User Gesture Requirement: While the activeTab permission itself is strictly gated by a user gesture, the captureVisibleTab API does not have an additional, separate user gesture requirement beyond the need for the extension to have valid host or activeTab permissions to perform the capture [1]. In summary, to use captureVisibleTab with activeTab, you must ensure the user has initiated an action (like a click) that grants your extension access to the active tab [1]. From there, you can execute the capture command in your background service worker or popup.
Citations:
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- manifest and code provenance ---'
git status --short
git diff --stat
git log -1 --oneline -- manifest.json manifest.dev.json src/pages/background/index.ts src/pages/content/export/index.ts
printf '%s\n' '--- dynamic content-script registration and export scope ---'
rg -n -S -C 5 \
'CUSTOM_CONTENT_SCRIPT_ID|registerContentScripts|src/pages/content/index|export' \
src/pages/background/index.ts src/pages/content/index.tsx src/pages/content/export/index.ts | sed -n '1,320p'Repository: Nagi-ovo/voyager
Length of output: 22999
Obtain explicit approval before retaining <all_urls>. ensureGeneratedUiCapturePermission() requests it during Gemini generated-UI export, not ChatGPT export. Remove it if the capture flow can use narrower access; otherwise document the requirement and obtain approval in both manifests.
📍 Affects 2 files
manifest.json#L44-L44(this comment)manifest.dev.json#L58-L58
🤖 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 `@manifest.json` at line 44, Remove <all_urls> from optional_host_permissions
in both manifest.json (line 44) and manifest.dev.json (line 58) if
ensureGeneratedUiCapturePermission() supports narrower access; otherwise
document the requirement and obtain explicit approval for retaining it in both
manifests.
Source: Coding guidelines
| expect(manifestChrome.optional_host_permissions).toEqual( | ||
| expect.arrayContaining(['<all_urls>']), | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert the complete optional-host allowlist.
expect.arrayContaining(['<all_urls>']) verifies only one entry and permits unexpected host permissions. This weakens the security regression test. Assert the complete approved set so accidental permission broadening fails the test.
🤖 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/core/utils/__tests__/manifestPermissions.test.ts` around lines 77 - 79,
Update the optional_host_permissions assertion in the manifest permissions test
to compare against the complete approved allowlist, rather than using
expect.arrayContaining with only <all_urls>. Include every expected host
permission and ensure unexpected entries cause the test to fail.
| import { MarkdownFormatter } from '../MarkdownFormatter'; | ||
| import { PDFPrintService } from '../PDFPrintService'; | ||
|
|
||
| DOMContentExtractor.setExportAdapter(resolveExportAdapter()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare JSDOM global setup order against setExportAdapter in export tests.
set -euo pipefail
fd -t f -e ts . src/features/export/services/__tests__ | while IFS= read -r file; do
if rg -q 'setExportAdapter' "$file"; then
echo "===== $file ====="
rg -n 'new JSDOM|globalThis\.(window|document|navigator)|setExportAdapter|resolveExportAdapter' "$file"
echo
fi
doneRepository: Nagi-ovo/voyager
Length of output: 2034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '===== ConversationExportService.test.ts ====='
sed -n '1,55p' src/features/export/services/__tests__/ConversationExportService.test.ts
printf '%s\n' '===== platformAdapters references and implementation ====='
rg -n -A12 -B8 'function resolveExportAdapter|const resolveExportAdapter|export .*resolveExportAdapter|window\.location|location\.href|setExportAdapter' src/pages/content/export src/features/exportRepository: Nagi-ovo/voyager
Length of output: 46613
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '===== Vitest configuration and package scripts ====='
fd -t f -a 'vitest*.{ts,js,mts,mjs}' 'vite.config.*' 'package.json' . | while IFS= read -r file; do
echo "===== $file ====="
case "$file" in
*package.json) rg -n '"(test|typecheck|lint)"|vitest|environment' "$file" ;;
*) cat -n "$file" ;;
esac
done
printf '%s\n' '===== adapter construction and URL-dependent methods ====='
sed -n '1,90p' src/pages/content/export/adapter/platformAdapters.ts
sed -n '640,690p' src/pages/content/export/adapter/platformAdapters.ts
rg -n -A6 -B3 'extractConversationIdFromUrl|resolveConversationRoot|getUserSelectors|getAssistantSelectors' src/pages/content/export/adapter/platformAdapters.tsRepository: Nagi-ovo/voyager
Length of output: 645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '===== candidate configuration files ====='
git ls-files | rg '(^|/)(package\.json|vitest[^/]*\.(ts|js|mts|mjs)|vite\.config\.[^/]+)$' | while IFS= read -r file; do
echo "$file"
done
printf '%s\n' '===== package scripts and Vitest settings ====='
for file in $(git ls-files | rg '(^|/)package\.json$|(^|/)vitest[^/]*\.(ts|js|mts|mjs)$|(^|/)vite\.config\.[^/]+$'); do
echo "===== $file ====="
if [[ "$file" == */package.json || "$file" == package.json ]]; then
rg -n '"(test|typecheck|lint)"|vitest|environment' "$file" || true
else
cat -n "$file"
fi
done
printf '%s\n' '===== adapter implementation ====='
sed -n '1,90p' src/pages/content/export/adapter/platformAdapters.ts
sed -n '640,690p' src/pages/content/export/adapter/platformAdapters.ts
rg -n -A8 -B3 'extractConversationIdFromUrl|resolveConversationRoot|getUserSelectors|getAssistantSelectors' src/pages/content/export/adapter/platformAdapters.tsRepository: Nagi-ovo/voyager
Length of output: 30290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '===== test setup ====='
sed -n '1,220p' src/tests/setup.ts
printf '%s\n' '===== SiteRegistry URL resolution ====='
ast-grep outline src/features/plugins/sites/registry.ts
sed -n '1,260p' src/features/plugins/sites/registry.ts
printf '%s\n' '===== site definitions and URL patterns ====='
rg -n -A12 -B5 'id:|matches:|host|url|pattern|gemini|chatgpt' src/features/plugins/sites src/features/plugins | head -240Repository: Nagi-ovo/voyager
Length of output: 25582
Move setExportAdapter below the JSDOM global setup. This test assigns the JSDOM window after resolveExportAdapter() reads the existing Vitest window.
🤖 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__/ConversationExportService.test.ts` at
line 20, Move setExportAdapter below the JSDOM global setup in
ConversationExportService tests so resolveExportAdapter() runs only after the
test window has been assigned. Preserve the existing adapter initialization
while ensuring it reads the configured JSDOM window.
| /** | ||
| * Extract user query content | ||
| * Extract user query content. | ||
| * @param imageSelectors - Platform-specific selectors for finding images. | ||
| * Empty/omitted = use Gemini's built-in selectors only. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unconditional debug log and fix the stale JSDoc.
Two separate problems in this block:
- Line 87 logs on every user-content extraction, without checking
this.DEBUG. Every other log in this class is gated bythis.DEBUG. A full-conversation export produces one log line per turn. Gate it or delete it. - The JSDoc at Lines 70-73 documents an
imageSelectorsparameter.extractUserContent(element: HTMLElement)has no such parameter. The image selection now comes from the adapter.
♻️ Proposed fix
/**
- * Extract user query content.
- * `@param` imageSelectors - Platform-specific selectors for finding images.
- * Empty/omitted = use Gemini's built-in selectors only.
+ * Extract user query content.
+ * Image and text extraction strategies come from the configured
+ * platform adapter.
*/ const images = this.exportAdapter.extractUserImage(element) ?? [];
- console.log('[DOMContentExtractor] images:', images);
+ if (this.DEBUG) console.log('[DOMContentExtractor] images:', images);
result.hasImages = images.length > 0;Also applies to: 87-87
🤖 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 70 - 73,
Remove or gate the unconditional debug log in the user-content extraction flow
to match the class’s existing this.DEBUG logging behavior, and update the JSDoc
above extractUserContent to document only its actual parameters, removing the
stale imageSelectors description.
| // Extract assistant image | ||
| if ( | ||
| child.querySelector( | ||
| '.attachment-container.youtube img.thumbnail, youtube-block img.thumbnail, single-video img.thumbnail', | ||
| this.exportAdapter.extractAssistantImage( | ||
| child, | ||
| htmlParts, | ||
| textParts, | ||
| flags, | ||
| tagName, | ||
| this.DEBUG, | ||
| processedImageSrcs, | ||
| ) | ||
| ) { | ||
| if (this.processYouTubeCovers(child, htmlParts, textParts, flags)) { | ||
| continue; | ||
| } | ||
| continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
processedImageSrcs is a read-only set that nothing ever populates, so assistant-image deduplication is inert and its test cannot detect that. The ExportPlatformAdapter.extractAssistantImage contract declares the parameter as ReadonlySet<string>, and DOMContentExtractor creates it empty at Line 200 without ever adding to it. Every has(src) check returns false.
src/features/export/services/DOMContentExtractor.ts#L467-L479: widen theprocessNodesparameter at Line 373 fromReadonlySet<string>toSet<string>, widen the matchingextractAssistantImageparameter insrc/pages/content/export/adapter/platformAdapters.ts, and record each emittedsrcin the set. Also forwardprocessedImageSrcsin the recursion at Line 447, which currently omits it.src/features/export/services/__tests__/ImageExportService.test.ts#L18-L40: addprocessedImageSrcs.add(src)in the test adapter after it pushes the image, and add an assertion that a repeatedsrcis emitted only once.
📍 Affects 2 files
src/features/export/services/DOMContentExtractor.ts#L467-L479(this comment)src/features/export/services/__tests__/ImageExportService.test.ts#L18-L40
🤖 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 467 - 479,
Assistant-image deduplication is inert because the tracking set is read-only,
never updated, and omitted during recursion. In
src/features/export/services/DOMContentExtractor.ts#L467-L479, change
processNodes and extractAssistantImage to accept Set<string>, add each emitted
src to processedImageSrcs, and pass the set through recursive calls at Line 447.
In src/features/export/services/__tests__/ImageExportService.test.ts#L18-L40,
update the test adapter to add emitted sources and assert duplicate sources are
emitted only once.
| exportedAt: content.exportedAt, | ||
| count: 1, | ||
| title: content.title, | ||
| platform: 'web', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
platform: 'web' leaks a lowercase, non-display token into user-facing titles.
ConversationMetadata.platform is documented in src/features/export/types/export.ts Line 52 as a display name, with the examples "ChatGPT", "Claude", "Gemini". The value 'web' is a lowercase internal token.
It reaches two user-visible strings:
extractTitleFromURLat Line 1094 and Line 1096 producesweb Conversationon the PDF cover page link.getPrintDialogTitleat Line 1150 and Line 1155 produces<title> - webin the print dialog and the printed header.
Before this change both read Gemini. Use a proper display name.
🐛 Proposed fix
const metadata: ConversationMetadata = {
url: content.url,
exportedAt: content.exportedAt,
count: 1,
title: content.title,
- platform: 'web',
+ platform: 'Gemini',
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| platform: 'web', | |
| platform: 'Gemini', |
🤖 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/PDFPrintService.ts` at line 58, Replace the
`platform: 'web'` value in the PDF export metadata with the proper user-facing
display name `Gemini`, preserving the existing `ConversationMetadata` contract
and ensuring `extractTitleFromURL` and `getPrintDialogTitle` continue producing
display titles.
| private static normalizeConversationTitle( | ||
| rawTitle: string | undefined, | ||
| platform?: string, | ||
| ): string { | ||
| if (!rawTitle) return ''; | ||
| const normalized = rawTitle | ||
| let normalized = rawTitle | ||
| .trim() | ||
| .replace(/\s+-\s+Gemini$/i, '') | ||
| .replace(/\s+-\s+Google Gemini$/i, '') | ||
| .replace(/\s+/g, ' ') | ||
| .trim(); | ||
| if (platform && platform !== 'Gemini') { | ||
| const escaped = platform.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| normalized = normalized.replace(new RegExp(`\\s+-\\s+${escaped}$`, 'i'), '').trim(); | ||
| } | ||
| return this.isMeaningfulConversationTitle(normalized) ? normalized : ''; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
renderHeader does not pass the platform to normalizeConversationTitle.
normalizeConversationTitle gained a platform parameter that strips a trailing - <platform> suffix. getPrintDialogTitle passes it at Lines 1143 and 1146. renderHeader at Lines 516 and 517 still calls the function with one argument.
For a ChatGPT export, the page title is typically <conversation> - ChatGPT. The print dialog title strips the suffix. The PDF cover page H1 at Line 530 keeps it. The two titles disagree in the same document.
🐛 Proposed fix
private static renderHeader(
metadata: ConversationMetadata,
preferMetadataTitle: boolean,
): string {
- const metadataTitle = this.normalizeConversationTitle(metadata.title);
- const pageConversationTitle = this.normalizeConversationTitle(this.getConversationTitle());
+ const platform = metadata.platform || 'Gemini';
+ const metadataTitle = this.normalizeConversationTitle(metadata.title, platform);
+ const pageConversationTitle = this.normalizeConversationTitle(
+ this.getConversationTitle(),
+ platform,
+ );🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 1132-1132: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s+-\\s+${escaped}$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 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/PDFPrintService.ts` around lines 1120 - 1136,
Update the normalizeConversationTitle calls in renderHeader to pass the current
platform, matching getPrintDialogTitle. Ensure the PDF cover-page title strips
the platform suffix, such as “ - ChatGPT,” consistently with the print dialog
while preserving existing title normalization.
| for (const turn of selectedContainers) { | ||
| const materialized = await materializeChatGptTurnContainer(turn); | ||
| const { container, role } = materialized; | ||
|
|
||
| if (role === 'user') { | ||
| // A second user message closes an earlier selected user-only turn. | ||
| if (pendingUser) { | ||
| turns.push(pendingUser); | ||
| } | ||
|
|
||
| const userElement = container.querySelector<HTMLElement>(USER_MESSAGE_SELECTOR); | ||
| if (!userElement) { | ||
| // The virtualized node did not finish mounting before the timeout. Do | ||
| // not invent content or bind it to another message; keep processing | ||
| // the remaining selected IDs. | ||
| pendingUser = null; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Log skipped turns instead of dropping them silently.
Two paths drop a selected message without any signal:
role === 'unknown'after materialization. The loop matches neither branch and continues.role === 'user'butuserElementis null at Line 144.
Both cases occur when ChatGPT does not mount the content within the 3000 ms timeout. The user selected the message, and the export omits it with no warning. Add a console.warn with the container ID so the omission is diagnosable.
🐛 Proposed logging
for (const turn of selectedContainers) {
const materialized = await materializeChatGptTurnContainer(turn);
const { container, role } = materialized;
+ if (role === 'unknown') {
+ console.warn('[Voyager] ChatGPT turn did not mount before timeout, skipping:', turn.id);
+ continue;
+ }
+
if (role === 'user') { const userElement = container.querySelector<HTMLElement>(USER_MESSAGE_SELECTOR);
if (!userElement) {
+ console.warn('[Voyager] ChatGPT user message not mounted, skipping:', turn.id);
// The virtualized node did not finish mounting before the timeout. Do
// not invent content or bind it to another message; keep processing
// the remaining selected IDs.
pendingUser = null;
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const turn of selectedContainers) { | |
| const materialized = await materializeChatGptTurnContainer(turn); | |
| const { container, role } = materialized; | |
| if (role === 'user') { | |
| // A second user message closes an earlier selected user-only turn. | |
| if (pendingUser) { | |
| turns.push(pendingUser); | |
| } | |
| const userElement = container.querySelector<HTMLElement>(USER_MESSAGE_SELECTOR); | |
| if (!userElement) { | |
| // The virtualized node did not finish mounting before the timeout. Do | |
| // not invent content or bind it to another message; keep processing | |
| // the remaining selected IDs. | |
| pendingUser = null; | |
| continue; | |
| } | |
| for (const turn of selectedContainers) { | |
| const materialized = await materializeChatGptTurnContainer(turn); | |
| const { container, role } = materialized; | |
| if (role === 'unknown') { | |
| console.warn('[Voyager] ChatGPT turn did not mount before timeout, skipping:', turn.id); | |
| continue; | |
| } | |
| if (role === 'user') { | |
| // A second user message closes an earlier selected user-only turn. | |
| if (pendingUser) { | |
| turns.push(pendingUser); | |
| } | |
| const userElement = container.querySelector<HTMLElement>(USER_MESSAGE_SELECTOR); | |
| if (!userElement) { | |
| console.warn('[Voyager] ChatGPT user message not mounted, skipping:', turn.id); | |
| // The virtualized node did not finish mounting before the timeout. Do | |
| // not invent content or bind it to another message; keep processing | |
| // the remaining selected IDs. | |
| pendingUser = null; | |
| continue; | |
| } |
🤖 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/pages/content/export/adapter/chatgpt.ts` around lines 133 - 150, Update
the selected-turn processing loop around materializeChatGptTurnContainer to emit
console.warn messages containing the container ID when a materialized turn has
role "unknown" or when the user-role branch has no userElement. Preserve the
existing skip behavior while ensuring both omission paths are diagnosable.
| function chatgptExtractUserImage(element: HTMLElement): NodeListOf<HTMLImageElement> { | ||
| const ImageSelector = 'img'; | ||
| return element.querySelectorAll(ImageSelector); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
img is a very broad selector for ChatGPT user turns.
chatgptExtractUserImage returns every <img> inside the user element. ChatGPT renders file-tile icons and other decorative images inside user turns. Those images can appear in exports as content images. Verify against a real user turn that contains a file attachment plus an inline image.
🤖 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/pages/content/export/adapter/platformAdapters.ts` around lines 505 - 508,
Update chatgptExtractUserImage to use a selector that targets only actual inline
user-content images, excluding file-tile icons and other decorative images.
Verify the selector against a ChatGPT user turn containing both a file
attachment and an inline image so only the inline image is returned.
Prevent partial and mispaired virtualized exports, cancel stale sessions, escape exported attributes, and bound image fetching. Refs Nagi-ovo#841 Co-authored-by: Codex <codex@users.noreply.github.com>
|
Maintainer follow-up pushed in Local verification passed:
Live cross-browser interaction QA remains the final manual gate. @codex review |
Use materialized user content when generating Markdown prompt headings so uploaded media remains in the exported turn. Refs Nagi-ovo#841 Co-authored-by: Codex <codex@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/features/export/services/ImageExportService.ts (1)
148-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
??and||disagree between the image and PDF renderers.This code uses
??, so an empty-stringturn.userContent.htmlsuppresses the DOM fallback and renders<em>No content</em>.PDFPrintService.renderTurnat Lines 578-589 uses||for the same decision and falls back to the live DOM. Pick one semantic for empty snapshot HTML across both renderers.🐛 Proposed fix
const userHtml = - turn.userContent?.html ?? + turn.userContent?.html || (turn.userElement ? DOMContentExtractor.extractUserContent(turn.userElement).html : this.formatPlainTextAsHtml(turn.user)); const assistantHtml = - turn.assistantContent?.html ?? + turn.assistantContent?.html || (turn.assistantElement ? DOMContentExtractor.extractAssistantContent(turn.assistantElement).html : this.formatPlainTextAsHtml(turn.assistant));🤖 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/ImageExportService.ts` around lines 148 - 159, Align the empty snapshot HTML fallback semantics between the image export flow and PDFPrintService.renderTurn. Update the userHtml and assistantHtml selection in the image renderer to use the same empty-string behavior as the PDF renderer, while preserving the existing preference for captured content and fallback order.src/features/export/services/DOMContentExtractor.ts (2)
451-456: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecursion at Line 454 drops the dedupe set.
processNodes(child, htmlParts, textParts, flags)omitsprocessedImageSrcs, so the default parameter creates a new emptySet. Images inside a container that owns export code blocks are then deduplicated against an empty set and can be emitted twice, once here and once in a sibling branch that uses the outer set. All other recursive calls in this method forward the set.🐛 Proposed fix
if (tagName !== 'ul' && tagName !== 'ol' && exportCodeBlocks.length > 0) { - this.processNodes(child, htmlParts, textParts, flags); + this.processNodes(child, htmlParts, textParts, flags, processedImageSrcs); continue; }🤖 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 451 - 456, Pass the existing processedImageSrcs set through the recursive processNodes call in the exportCodeBlocks container branch. Update the call within processNodes so image deduplication remains shared with sibling recursion and other branches.
524-543: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecursion still drops direct text nodes of mixed-content containers, and no test covers it.
processNodesiteratescontainer.children, which holds element nodes only. The new inline branch rescues direct text only when every child element is inline, so a container that mixes direct text with a block child still loses its bare text.
src/features/export/services/DOMContentExtractor.ts#L524-L543: emit the direct text nodes ofchildbefore recursing, or walkchild.childNodesin document order instead ofchild.children.src/features/export/services/__tests__/DOMContentExtractor.test.ts#L173-L186: add a case with a container that mixes direct text and a block child, and assert both the direct text and the block text appear inextracted.text.🤖 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 524 - 543, The recursion in DOMContentExtractor’s processNodes path drops direct text from containers that mix text with block elements. Update the handling around the generic-container branch in src/features/export/services/DOMContentExtractor.ts:524-543 to emit direct text nodes in document order while still recursing into child elements, preserving both inline and block content. Add a test in src/features/export/services/__tests__/DOMContentExtractor.test.ts:173-186 using mixed direct text and a block child, asserting both texts appear in extracted.text.src/pages/content/export/adapter/platformAdapters.ts (1)
523-532: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMulti-line ChatGPT prompts collapse into a single line.
DOMContentExtractor.normalizeTextreplaces every whitespace run, including newlines, with one space. ChatGPT renders a multi-line user prompt as several block elements inside the user message. The export therefore joins all lines into one paragraph, and any indentation or blank-line structure in the prompt is lost.Push one entry per block child instead of one flattened string.
🐛 Proposed fix
function chatgptExtractUserText( _textLines: NodeListOf<HTMLElement>, textParts: string[], element: HTMLElement, ) { const contentOnly = element.cloneNode(true) as HTMLElement; chatgptGetUserAttachmentCandidates(contentOnly)?.forEach((candidate) => candidate.remove()); + const blocks = Array.from(contentOnly.querySelectorAll<HTMLElement>(':scope > p, :scope > div')); + if (blocks.length > 0) { + let pushed = false; + for (const block of blocks) { + const line = DOMContentExtractor.normalizeText(block.textContent || ''); + if (line) { + textParts.push(line); + pushed = true; + } + } + if (pushed) return; + } const fallback = DOMContentExtractor.normalizeText(contentOnly.textContent || ''); if (fallback) textParts.push(fallback); }🤖 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/pages/content/export/adapter/platformAdapters.ts` around lines 523 - 532, Update chatgptExtractUserText to preserve multi-line prompt structure by iterating over the cloned contentOnly block children after removing attachment candidates, normalizing and pushing each non-empty child’s text separately. Do not normalize the entire contentOnly container into one fallback string.
🧹 Nitpick comments (13)
src/features/export/services/ConversationExportService.ts (1)
485-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant signal ternary.
fetchImageForMarkdownPackagingdeclaressignal?: AbortSignal. Passingundefinedis equivalent to omitting the argument. The ternary adds no behavior.♻️ Proposed simplification
- const fetched = signal - ? await this.fetchImageForMarkdownPackaging(fetchUrl, budget, signal) - : await this.fetchImageForMarkdownPackaging(fetchUrl, budget); + const fetched = await this.fetchImageForMarkdownPackaging(fetchUrl, budget, signal);🤖 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/ConversationExportService.ts` around lines 485 - 487, Remove the redundant signal-based ternary in the export flow and make the call to fetchImageForMarkdownPackaging pass signal directly as its optional third argument, preserving the existing fetchUrl and budget values.src/features/export/services/ImageExportService.ts (2)
507-513: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
budgetbeforetoDataUrl.
toDataUrlcloses overbudget, butbudgetis declared after it at Line 512. The call site runs later, so no temporal dead zone error occurs today. Any future direct call before Line 512 would throw aReferenceError. Move the declaration above the closure.♻️ Proposed reorder
+ const budget = { remainingBytes: MAX_EXPORT_IMAGE_TOTAL_BYTES }; const toDataUrl = async (url: string): Promise<string | null> => { const fetched = await fetchBoundedExportImage(url, budget, signal); return fetched ? await blobToDataUrl(fetched.blob) : null; }; - const budget = { remainingBytes: MAX_EXPORT_IMAGE_TOTAL_BYTES }; await mapWithConcurrency(imgs, EXPORT_IMAGE_FETCH_CONCURRENCY, async (img) => {🤖 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/ImageExportService.ts` around lines 507 - 513, Move the budget declaration above the toDataUrl closure so the closure captures an already-initialized value, while preserving the existing fetch and concurrency behavior.
536-543: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSkip decode for removed images.
Line 532 removes images whose fetch failed. This loop still calls
decode()on those detached elements.PDFPrintService.inlineImagesfilters withimg.isConnectedand also bounds decode with a timeout. Align this path with that behavior to avoid wasted work and an unbounded decode wait.♻️ Proposed change
await Promise.all( - imgs.map((img) => + imgs + .filter((img) => img.isConnected) + .map((img) => (img as HTMLImageElement & { decode?: () => Promise<void> }).decode?.().catch(() => { /* ignore */ }), ), );🤖 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/ImageExportService.ts` around lines 536 - 543, Update the image decode loop in the export method to process only connected images, matching the filtering used by PDFPrintService.inlineImages, and bound each optional decode operation with the established timeout behavior. Preserve the existing abort assertion and ignored decode failures.src/features/export/services/__tests__/boundedImageFetch.test.ts (1)
9-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the cancellation path.
Cancellation is a primary objective of this change.
fetchBoundedExportImagethrowsDOMException('Export cancelled', 'AbortError')at Line 138 and Line 173 ofsrc/features/export/services/boundedImageFetch.ts. No test asserts that behavior. Thedata:and trusted-runtime-fallback branches are also untested.💚 Suggested test
it('throws AbortError when the signal is already aborted', async () => { const controller = new AbortController(); controller.abort(); await expect( fetchBoundedExportImage('https://example.com/image.png', { remainingBytes: 1024 }, controller.signal), ).rejects.toMatchObject({ name: 'AbortError' }); });🤖 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__/boundedImageFetch.test.ts` around lines 9 - 63, Add tests around fetchBoundedExportImage for an already-aborted signal, asserting it rejects with an error whose name is AbortError. Also add coverage for the data: URL path and trusted-runtime fallback branch, verifying their expected successful behavior while preserving existing response-validation tests.src/pages/content/export/index.ts (2)
1186-1219: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
scrollToTopAndRendercannot be cancelled.The caller checks cancellation at Line 1424, after this function resolves. The hard cap at Line 1215 is 3000 ms. If the user presses Escape during the wait, the export continues for up to three seconds before it observes the abort. Accept an optional
AbortSignaland resolve early onabort.🤖 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/pages/content/export/index.ts` around lines 1186 - 1219, Update scrollToTopAndRender to accept an optional AbortSignal and resolve its MutationObserver wait immediately when the signal aborts. Register an abort listener that calls the existing done cleanup, remove the listener during cleanup, and preserve the current mutation-idle and 3000 ms timeout behavior for non-aborted calls. Update the caller to pass the export cancellation signal.
2826-2834: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGemini cleanup omits export cancellation and dialog teardown.
The non-Gemini cleanup at Lines 2558-2566 calls
cancelActiveExportOperation()and hidesactiveExportDialog. This cleanup does neither. The doc comment at Lines 2518-2520 states Gemini's caller may ignore the returned cleanup, so the impact is limited today. Align the two paths so any future caller that does invoke this cleanup tears down a running export.♻️ Proposed change
return () => { + cancelActiveExportOperation(); if (reinjectTimer !== null) clearTimeout(reinjectTimer); window.removeEventListener('resize', reinjectExportButtonIfNeeded); window.removeEventListener('gv-print-cleanup', reinjectExportButtonIfNeeded); window.removeEventListener('afterprint', reinjectExportButtonIfNeeded); try { chrome.storage?.onChanged?.removeListener(storageChangeHandler); } catch {} + activeExportDialog?.hide(); + activeExportDialog = null; };🤖 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/pages/content/export/index.ts` around lines 2826 - 2834, Update the Gemini cleanup callback to call cancelActiveExportOperation() and hide activeExportDialog, matching the non-Gemini cleanup behavior. Keep the existing timer, event-listener, and storage-listener cleanup intact so invoking the returned callback fully tears down an active export and its dialog.src/pages/content/export/adapter/__tests__/chatgpt.test.ts (2)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the describe block by the function under test.
The block is named
chatgptCollectTurnContainers, but Lines 91-180 testbuildChatGptTurnsForSelectionandresolveChatGptSelectionRoles. Move those cases into their owndescribeblocks so failures name the correct unit.Also applies to: 91-180
🤖 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/pages/content/export/adapter/__tests__/chatgpt.test.ts` at line 52, Split the tests currently grouped under chatgptCollectTurnContainers into separate describe blocks for buildChatGptTurnsForSelection and resolveChatGptSelectionRoles, keeping only the relevant chatgptCollectTurnContainers cases in its existing block so failure names identify the correct unit.
173-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the route-change guard and the completeness guard.
Cancellation is covered. Two other new failure paths in
src/pages/content/export/adapter/chatgpt.tsare not:
assertSelectionActivethrowschatgpt_export_conversation_changedwhenexpectedUrlno longer matches the current URL.buildChatGptTurnsForSelectionthrowschatgpt_export_incomplete_selectionwhenextractedIds.size !== selectedContainerIds.size.Both guard against exporting the wrong or partial conversation, so they warrant tests.
As per coding guidelines, "new features and fixes must include tests".
🤖 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/pages/content/export/adapter/__tests__/chatgpt.test.ts` around lines 173 - 180, Add tests in chatgpt.test.ts covering both guards in buildChatGptTurnsForSelection: verify a mismatched expectedUrl rejects with chatgpt_export_conversation_changed, and verify differing extractedIds and selectedContainerIds sizes rejects with chatgpt_export_incomplete_selection. Keep the existing cancellation test unchanged.Source: Coding guidelines
src/pages/content/export/adapter/chatgpt.ts (1)
117-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe polling loop rescans the whole document several times per tick.
findTurnContainercallschatgptCollectTurnContainers(), which runsdocument.querySelectorAll('[data-turn-id-container]')over the entire page and builds aMapof every turn. Each poll iteration calls it once at Line 170 and again insideisGeneratingTurnat Line 142, pluscomputeConversationFingerprint. With a poll interval of 80 ms and a 3000 ms budget, one slow turn costs up to about 74 full-document scans, and the loop repeats for every selected turn.Cache the container list per poll iteration and pass it to
isGeneratingTurn, or look the container up directly by attribute selector.♻️ Proposed fix
-function findTurnContainer(id: string): ChatGptTurnContainer | null { - return chatgptCollectTurnContainers().find((turn) => turn.id === id) ?? null; -} +function findTurnContainer( + id: string, + ordered: ChatGptTurnContainer[] = chatgptCollectTurnContainers(), +): ChatGptTurnContainer | null { + return ordered.find((turn) => turn.id === id) ?? null; +}Then reuse one
orderedarray per loop iteration for bothfindTurnContainerandisGeneratingTurn.Also applies to: 168-188
🤖 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/pages/content/export/adapter/chatgpt.ts` around lines 117 - 119, Update the polling loop around findTurnContainer and isGeneratingTurn to collect the turn containers once per iteration and reuse the resulting ordered array for both lookups, rather than calling chatgptCollectTurnContainers repeatedly. Pass the cached collection into the relevant helper(s) while preserving the existing turn-selection and generation-check behavior.src/features/export/services/__tests__/DOMContentExtractor.test.ts (1)
173-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for text mixed with block children.
This test covers direct text with inline-only children. It does not cover a container that mixes direct text with a block child, which is the case that still takes the
processNodespath and drops the direct text. See the related comment onsrc/features/export/services/DOMContentExtractor.tsLines 524-543.💚 Suggested extra case
+ it('preserves direct text next to block children', () => { + const assistant = document.createElement('div'); + assistant.innerHTML = ` + <message-content> + <div class="markdown"><div>Intro text<p>Body</p></div></div> + </message-content> + `; + + const extracted = DOMContentExtractor.extractAssistantContent(assistant); + + expect(extracted.text).toContain('Intro text'); + expect(extracted.text).toContain('Body'); + });🤖 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__/DOMContentExtractor.test.ts` around lines 173 - 186, Add a test alongside “preserves direct text around nested inline elements” that places direct text before and after a block-level child within the markdown container, then assert DOMContentExtractor.extractAssistantContent preserves both text segments in the extracted output. Ensure the case exercises the processNodes path and verifies the direct text is not dropped.src/pages/content/export/adapter/__tests__/platformAdapters.test.ts (1)
1-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage to the remaining new ChatGPT extractors.
This file covers formula escaping only.
chatgptExtractAssistantImageandchatgptExtractCodeBlockare new and are not exported, so they have no direct coverage here. The image path carries theprocessedImageSrcsdedupe contract and thearia-hiddenskip; the code path marksprocessedByGVso the fallback scan skips the element. Both are worth a direct test.Export those two functions and add cases, or cover them through
DOMContentExtractorwith the ChatGPT adapter installed.As per coding guidelines, "new features and fixes must include tests".
🤖 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/pages/content/export/adapter/__tests__/platformAdapters.test.ts` around lines 1 - 49, Extend tests to cover the new chatgptExtractAssistantImage and chatgptExtractCodeBlock behavior. Prefer exporting both functions and add direct cases verifying image deduplication via processedImageSrcs, skipping aria-hidden images, and marking code blocks with processedByGV so fallback scanning skips them; alternatively cover the same contracts through DOMContentExtractor with the ChatGPT adapter installed.Source: Coding guidelines
src/pages/content/export/adapter/platformAdapters.ts (2)
688-697: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe non-null assertion in the fallback can produce an adapter with no
site.If
SiteRegistry.createDefault()ever stops registering a Gemini site,registry.resolveByUrl('https://gemini.google.com/')!returnsundefinedat runtime.buildGeminiAdapterthen builds an adapter whosesitefield isundefined, and consumers that readadapter.site.selectorsfail far from this line. Throw a clear error instead.♻️ Proposed fix
default: { - return buildGeminiAdapter(site ?? registry.resolveByUrl('https://gemini.google.com/')!); + const gemini = site ?? registry.resolveByUrl('https://gemini.google.com/'); + if (!gemini) throw new Error('[export] Gemini site adapter is not registered'); + return buildGeminiAdapter(gemini); }🤖 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/pages/content/export/adapter/platformAdapters.ts` around lines 688 - 697, Update resolveExportAdapter’s Gemini fallback to validate that registry.resolveByUrl('https://gemini.google.com/') returns a site before calling buildGeminiAdapter. If no site is found, throw a clear error immediately; otherwise pass the validated site without using a non-null assertion.
53-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the contract docs and type names with the platform-neutral interface.
Two points in the shared contract:
- The comment at Lines 53-57 describes "CSS selectors" and an "empty array", but
extractUserImagereturns aNodeListOf<HTMLImageElement>. The comment no longer matches the member.collectTurnContainers,buildTurnsForSelection, andresolveSelectionRolesare declared on the neutralExportPlatformAdapter, but their types areChatGptTurnContainerandChatGptTurnRole. A second virtualized platform would have to reuse ChatGPT-named types. Consider neutral aliases, for exampleExportTurnContainerandExportTurnRole, in./type.Also applies to: 101-120
🤖 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/pages/content/export/adapter/platformAdapters.ts` around lines 53 - 58, Update the extractUserImage contract comment to describe returning image nodes rather than CSS selectors or empty-array fallback behavior. In the ExportPlatformAdapter members collectTurnContainers, buildTurnsForSelection, and resolveSelectionRoles, replace ChatGptTurnContainer and ChatGptTurnRole with platform-neutral aliases defined in ./type, such as ExportTurnContainer and ExportTurnRole, and update their related declarations and usages consistently.
🤖 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/boundedImageFetch.ts`:
- Around line 121-126: Update the non-base64 data URL handling in
boundedImageFetch so the decoded string is encoded as UTF-8 bytes before
creating the Blob. Replace the charCodeAt-based byte construction in the try
block with a UTF-8 encoder, while preserving the existing size checks, budget
reservation, and base64 path behavior.
In `@src/pages/content/export/adapter/chatgpt.ts`:
- Around line 338-347: Update the turn pairing logic around
materializeChatGptTurnContainer to use stable positions captured from the
initial full container snapshot rather than comparing independently recollected
sequence values. Preserve adjacency based on those original positions so DOM
insertions before or between a user and assistant cannot split valid pairs, and
add a regression test covering DOM changes during export.
In `@src/pages/content/export/index.ts`:
- Around line 1951-1954: Localize the export validation errors before they reach
the generic alert: update the throws in the export flow around exportRouteKey
and turnsForExport to use the existing translation function, or extend
resolveExportErrorMessage to translate export_conversation_changed and
export_empty_selection. Ensure neither token is displayed verbatim.
---
Outside diff comments:
In `@src/features/export/services/DOMContentExtractor.ts`:
- Around line 451-456: Pass the existing processedImageSrcs set through the
recursive processNodes call in the exportCodeBlocks container branch. Update the
call within processNodes so image deduplication remains shared with sibling
recursion and other branches.
- Around line 524-543: The recursion in DOMContentExtractor’s processNodes path
drops direct text from containers that mix text with block elements. Update the
handling around the generic-container branch in
src/features/export/services/DOMContentExtractor.ts:524-543 to emit direct text
nodes in document order while still recursing into child elements, preserving
both inline and block content. Add a test in
src/features/export/services/__tests__/DOMContentExtractor.test.ts:173-186 using
mixed direct text and a block child, asserting both texts appear in
extracted.text.
In `@src/features/export/services/ImageExportService.ts`:
- Around line 148-159: Align the empty snapshot HTML fallback semantics between
the image export flow and PDFPrintService.renderTurn. Update the userHtml and
assistantHtml selection in the image renderer to use the same empty-string
behavior as the PDF renderer, while preserving the existing preference for
captured content and fallback order.
In `@src/pages/content/export/adapter/platformAdapters.ts`:
- Around line 523-532: Update chatgptExtractUserText to preserve multi-line
prompt structure by iterating over the cloned contentOnly block children after
removing attachment candidates, normalizing and pushing each non-empty child’s
text separately. Do not normalize the entire contentOnly container into one
fallback string.
---
Nitpick comments:
In `@src/features/export/services/__tests__/boundedImageFetch.test.ts`:
- Around line 9-63: Add tests around fetchBoundedExportImage for an
already-aborted signal, asserting it rejects with an error whose name is
AbortError. Also add coverage for the data: URL path and trusted-runtime
fallback branch, verifying their expected successful behavior while preserving
existing response-validation tests.
In `@src/features/export/services/__tests__/DOMContentExtractor.test.ts`:
- Around line 173-186: Add a test alongside “preserves direct text around nested
inline elements” that places direct text before and after a block-level child
within the markdown container, then assert
DOMContentExtractor.extractAssistantContent preserves both text segments in the
extracted output. Ensure the case exercises the processNodes path and verifies
the direct text is not dropped.
In `@src/features/export/services/ConversationExportService.ts`:
- Around line 485-487: Remove the redundant signal-based ternary in the export
flow and make the call to fetchImageForMarkdownPackaging pass signal directly as
its optional third argument, preserving the existing fetchUrl and budget values.
In `@src/features/export/services/ImageExportService.ts`:
- Around line 507-513: Move the budget declaration above the toDataUrl closure
so the closure captures an already-initialized value, while preserving the
existing fetch and concurrency behavior.
- Around line 536-543: Update the image decode loop in the export method to
process only connected images, matching the filtering used by
PDFPrintService.inlineImages, and bound each optional decode operation with the
established timeout behavior. Preserve the existing abort assertion and ignored
decode failures.
In `@src/pages/content/export/adapter/__tests__/chatgpt.test.ts`:
- Line 52: Split the tests currently grouped under chatgptCollectTurnContainers
into separate describe blocks for buildChatGptTurnsForSelection and
resolveChatGptSelectionRoles, keeping only the relevant
chatgptCollectTurnContainers cases in its existing block so failure names
identify the correct unit.
- Around line 173-180: Add tests in chatgpt.test.ts covering both guards in
buildChatGptTurnsForSelection: verify a mismatched expectedUrl rejects with
chatgpt_export_conversation_changed, and verify differing extractedIds and
selectedContainerIds sizes rejects with chatgpt_export_incomplete_selection.
Keep the existing cancellation test unchanged.
In `@src/pages/content/export/adapter/__tests__/platformAdapters.test.ts`:
- Around line 1-49: Extend tests to cover the new chatgptExtractAssistantImage
and chatgptExtractCodeBlock behavior. Prefer exporting both functions and add
direct cases verifying image deduplication via processedImageSrcs, skipping
aria-hidden images, and marking code blocks with processedByGV so fallback
scanning skips them; alternatively cover the same contracts through
DOMContentExtractor with the ChatGPT adapter installed.
In `@src/pages/content/export/adapter/chatgpt.ts`:
- Around line 117-119: Update the polling loop around findTurnContainer and
isGeneratingTurn to collect the turn containers once per iteration and reuse the
resulting ordered array for both lookups, rather than calling
chatgptCollectTurnContainers repeatedly. Pass the cached collection into the
relevant helper(s) while preserving the existing turn-selection and
generation-check behavior.
In `@src/pages/content/export/adapter/platformAdapters.ts`:
- Around line 688-697: Update resolveExportAdapter’s Gemini fallback to validate
that registry.resolveByUrl('https://gemini.google.com/') returns a site before
calling buildGeminiAdapter. If no site is found, throw a clear error
immediately; otherwise pass the validated site without using a non-null
assertion.
- Around line 53-58: Update the extractUserImage contract comment to describe
returning image nodes rather than CSS selectors or empty-array fallback
behavior. In the ExportPlatformAdapter members collectTurnContainers,
buildTurnsForSelection, and resolveSelectionRoles, replace ChatGptTurnContainer
and ChatGptTurnRole with platform-neutral aliases defined in ./type, such as
ExportTurnContainer and ExportTurnRole, and update their related declarations
and usages consistently.
In `@src/pages/content/export/index.ts`:
- Around line 1186-1219: Update scrollToTopAndRender to accept an optional
AbortSignal and resolve its MutationObserver wait immediately when the signal
aborts. Register an abort listener that calls the existing done cleanup, remove
the listener during cleanup, and preserve the current mutation-idle and 3000 ms
timeout behavior for non-aborted calls. Update the caller to pass the export
cancellation signal.
- Around line 2826-2834: Update the Gemini cleanup callback to call
cancelActiveExportOperation() and hide activeExportDialog, matching the
non-Gemini cleanup behavior. Keep the existing timer, event-listener, and
storage-listener cleanup intact so invoking the returned callback fully tears
down an active export and its dialog.
🪄 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: e3923a27-8a85-42cf-9c50-37d0fc6921d6
📒 Files selected for processing (19)
src/features/export/services/ConversationExportService.tssrc/features/export/services/DOMContentExtractor.tssrc/features/export/services/ImageExportService.tssrc/features/export/services/MarkdownFormatter.tssrc/features/export/services/PDFPrintService.tssrc/features/export/services/__tests__/ConversationExportService.test.tssrc/features/export/services/__tests__/DOMContentExtractor.test.tssrc/features/export/services/__tests__/MarkdownFormatter.test.tssrc/features/export/services/__tests__/PDFPrintService.test.tssrc/features/export/services/__tests__/boundedImageFetch.test.tssrc/features/export/services/boundedImageFetch.tssrc/features/export/types/export.tssrc/pages/content/export/adapter/__tests__/chatgpt.test.tssrc/pages/content/export/adapter/__tests__/platformAdapters.test.tssrc/pages/content/export/adapter/chatgpt.tssrc/pages/content/export/adapter/platformAdapters.tssrc/pages/content/export/adapter/type.tssrc/pages/content/export/index.tssrc/pages/content/export/pendingExportState.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/features/export/services/tests/MarkdownFormatter.test.ts
- src/features/export/services/tests/PDFPrintService.test.ts
- src/features/export/types/export.ts
- src/features/export/services/MarkdownFormatter.ts
| try { | ||
| const raw = /;base64(?:;|$)/i.test(match[2]) ? atob(match[3]) : decodeURIComponent(match[3]); | ||
| if (raw.length > MAX_EXPORT_IMAGE_BYTES || raw.length > budget.remainingBytes) return null; | ||
| const bytes = new Uint8Array(raw.length); | ||
| for (let index = 0; index < raw.length; index++) bytes[index] = raw.charCodeAt(index); | ||
| return reserveBudget(new Blob([bytes], { type: contentType }), contentType, budget); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Percent-encoded data URLs corrupt multi-byte characters.
decodeURIComponent returns a JavaScript string with code points above 255 for non-ASCII input. Line 125 uses charCodeAt, which truncates those code points to one byte. A non-base64 data:image/svg+xml,... URL that contains non-ASCII text produces a corrupted blob. Encode the decoded string as UTF-8 instead.
🐛 Proposed fix
try {
- const raw = /;base64(?:;|$)/i.test(match[2]) ? atob(match[3]) : decodeURIComponent(match[3]);
- if (raw.length > MAX_EXPORT_IMAGE_BYTES || raw.length > budget.remainingBytes) return null;
- const bytes = new Uint8Array(raw.length);
- for (let index = 0; index < raw.length; index++) bytes[index] = raw.charCodeAt(index);
+ let bytes: Uint8Array;
+ if (/;base64(?:;|$)/i.test(match[2])) {
+ const raw = atob(match[3]);
+ bytes = new Uint8Array(raw.length);
+ for (let index = 0; index < raw.length; index++) bytes[index] = raw.charCodeAt(index);
+ } else {
+ bytes = new TextEncoder().encode(decodeURIComponent(match[3]));
+ }
+ if (bytes.length > MAX_EXPORT_IMAGE_BYTES || bytes.length > budget.remainingBytes) return null;
return reserveBudget(new Blob([bytes], { type: contentType }), contentType, budget);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const raw = /;base64(?:;|$)/i.test(match[2]) ? atob(match[3]) : decodeURIComponent(match[3]); | |
| if (raw.length > MAX_EXPORT_IMAGE_BYTES || raw.length > budget.remainingBytes) return null; | |
| const bytes = new Uint8Array(raw.length); | |
| for (let index = 0; index < raw.length; index++) bytes[index] = raw.charCodeAt(index); | |
| return reserveBudget(new Blob([bytes], { type: contentType }), contentType, budget); | |
| try { | |
| let bytes: Uint8Array; | |
| if (/;base64(?:;|$)/i.test(match[2])) { | |
| const raw = atob(match[3]); | |
| bytes = new Uint8Array(raw.length); | |
| for (let index = 0; index < raw.length; index++) bytes[index] = raw.charCodeAt(index); | |
| } else { | |
| bytes = new TextEncoder().encode(decodeURIComponent(match[3])); | |
| } | |
| if (bytes.length > MAX_EXPORT_IMAGE_BYTES || bytes.length > budget.remainingBytes) return null; | |
| return reserveBudget(new Blob([bytes], { type: contentType }), contentType, budget); |
🤖 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/boundedImageFetch.ts` around lines 121 - 126,
Update the non-base64 data URL handling in boundedImageFetch so the decoded
string is encoded as UTF-8 bytes before creating the Blob. Replace the
charCodeAt-based byte construction in the try block with a UTF-8 encoder, while
preserving the existing size checks, budget reservation, and base64 path
behavior.
| if (pendingUser?.sequence === sequence - 1) { | ||
| pendingUser.turn.assistant = assistantContent.text; | ||
| pendingUser.turn.assistantContent = assistantContent; | ||
| turns.push(pendingUser.turn); | ||
| pendingUser = null; | ||
| } else { | ||
| if (pendingUser) { | ||
| turns.push(pendingUser.turn); | ||
| pendingUser = null; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether sequence is used anywhere else and whether callers rely on a single snapshot.
rg -nP --type=ts -C3 '\bsequence\b' src/pages/content/exportRepository: Nagi-ovo/voyager
Length of output: 7105
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- outline ---'
ast-grep outline src/pages/content/export/adapter/chatgpt.ts
printf '%s\n' '--- collector/materializer definitions and calls ---'
rg -n -C8 'function (chatgptCollectTurnContainers|materializeChatGptTurnContainer)|const (chatgptCollectTurnContainers|materializeChatGptTurnContainer)|chatgptCollectTurnContainers|materializeChatGptTurnContainer' src/pages/content/export/adapter/chatgpt.ts
printf '%s\n' '--- export loop ---'
sed -n '1,120p' src/pages/content/export/adapter/chatgpt.ts
sed -n '260,370p' src/pages/content/export/adapter/chatgpt.ts
printf '%s\n' '--- tests ---'
sed -n '1,240p' src/pages/content/export/adapter/__tests__/chatgpt.test.tsRepository: Nagi-ovo/voyager
Length of output: 21636
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- outline ---'
ast-grep outline src/pages/content/export/adapter/chatgpt.ts
printf '%s\n' '--- collector/materializer definitions and calls ---'
rg -n -C8 'function (chatgptCollectTurnContainers|materializeChatGptTurnContainer)|const (chatgptCollectTurnContainers|materializeChatGptTurnContainer)|chatgptCollectTurnContainers|materializeChatGptTurnContainer' src/pages/content/export/adapter/chatgpt.ts
printf '%s\n' '--- relevant source ---'
sed -n '1,120p' src/pages/content/export/adapter/chatgpt.ts
sed -n '260,370p' src/pages/content/export/adapter/chatgpt.ts
printf '%s\n' '--- tests ---'
sed -n '1,240p' src/pages/content/export/adapter/__tests__/chatgpt.test.tsRepository: Nagi-ovo/voyager
Length of output: 21640
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- materialization path ---'
sed -n '117,194p' src/pages/content/export/adapter/chatgpt.ts
printf '%s\n' '--- type definition ---'
cat -n src/pages/content/export/adapter/type.ts
printf '%s\n' '--- deterministic sequence-drift verifier ---'
python3 - <<'PY'
def sequence_map(ids):
return {turn_id: index for index, turn_id in enumerate(dict.fromkeys(ids))}
def pairs_as_implemented(before_user_ids, after_assistant_ids, user_id, assistant_id):
pending = sequence_map(before_user_ids)[user_id]
assistant_sequence = sequence_map(after_assistant_ids)[assistant_id]
return pending == assistant_sequence - 1, pending, assistant_sequence
cases = {
"append_new_assistant": (["u1", "a1"], ["u1", "a1", "a2"]),
"insert_before_pending_user": (["u1", "a1"], ["a2", "u1", "a1"]),
"insert_before_existing_assistant": (["u1", "a1"], ["u1", "a2", "a1"]),
"remove_before_pending_user": (["a0", "u1", "a1"], ["u1", "a1"]),
}
for name, (before, after) in cases.items():
paired, pending, assistant = pairs_as_implemented(before, after, "u1", "a1")
print(f"{name}: pending={pending}, assistant={assistant}, paired={paired}")
PYRepository: Nagi-ovo/voyager
Length of output: 4257
Use stable adjacency from the initial container snapshot
materializeChatGptTurnContainer recollects containers by ID, so sequence can come from a different DOM snapshot for each turn. A container inserted before the pending user or between the user and assistant can make pendingUser.sequence === sequence - 1 split a valid pair. Preserve each turn’s position from the initial full container snapshot, and add a regression test for DOM changes during export.
🤖 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/pages/content/export/adapter/chatgpt.ts` around lines 338 - 347, Update
the turn pairing logic around materializeChatGptTurnContainer to use stable
positions captured from the initial full container snapshot rather than
comparing independently recollected sequence values. Preserve adjacency based on
those original positions so DOM insertions before or between a user and
assistant cannot split valid pairs, and add a regression test covering DOM
changes during export.
| if (exportRouteKey(location.href) !== exportRouteKey(selectionUrl)) { | ||
| throw new Error('export_conversation_changed'); | ||
| } | ||
| if (turnsForExport.length === 0) throw new Error('export_empty_selection'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Untranslated error tokens reach the user.
These two Error messages are translation keys. resolveExportErrorMessage in src/features/export/ui/ExportErrorMessage.ts only special-cases IMAGE_RENDER_EVENT_ERROR_CODE. Every other value is inserted verbatim into the generic template. The alert at Line 1996 therefore shows export_conversation_changed or export_empty_selection as raw text.
Either add these codes to resolveExportErrorMessage, or throw new Error(t('export_conversation_changed')) with a localized message.
🤖 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/pages/content/export/index.ts` around lines 1951 - 1954, Localize the
export validation errors before they reach the generic alert: update the throws
in the export flow around exportRouteKey and turnsForExport to use the existing
translation function, or extend resolveExportErrorMessage to translate
export_conversation_changed and export_empty_selection. Ensure neither token is
displayed verbatim.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 357a122843
ℹ️ 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".
| { | ||
| const searchImageContainers = child.querySelectorAll( | ||
| '.attachment-container.search-images .image-container[data-full-size-image-uri]', |
There was a problem hiding this comment.
When a Gemini response contains a standalone <img>—notably the .gv-generated-ui-screenshot-section inserted by captureGeneratedUiScreenshots()—this adapter handles only search, generated-image, and YouTube wrappers and never handles tagName === 'img'. Since DOMContentExtractor now delegates all assistant-image processing here, the leaf image contributes no text or HTML and is silently omitted from Markdown, JSON, PDF, and image exports; retain the previous generic image fallback for Gemini.
AGENTS.md reference: AGENTS.md:L69-L69
Useful? React with 👍 / 👎.
| const raw = /;base64(?:;|$)/i.test(match[2]) ? atob(match[3]) : decodeURIComponent(match[3]); | ||
| if (raw.length > MAX_EXPORT_IMAGE_BYTES || raw.length > budget.remainingBytes) return null; | ||
| const bytes = new Uint8Array(raw.length); | ||
| for (let index = 0; index < raw.length; index++) bytes[index] = raw.charCodeAt(index); |
There was a problem hiding this comment.
For a non-base64 data image containing percent-encoded non-ASCII text, such as an SVG with CJK characters, decodeURIComponent() produces Unicode characters but copying charCodeAt() values into a Uint8Array truncates them to single bytes. The packaged or inlined image is therefore corrupted in Markdown ZIP, PDF, and PNG exports; preserve the base64 path but encode the decoded non-base64 payload with TextEncoder, as the previous implementation did.
AGENTS.md reference: AGENTS.md:L69-L69
Useful? React with 👍 / 👎.
| const codeBlock = child.querySelector('code-block'); | ||
| if (tagName === 'code-block' || child.classList.contains('code-block') || codeBlock) { | ||
| if (DEBUG) console.log('[DOMContentExtractor] Found code block!'); | ||
| const elementToExtract = (codeBlock || child) as HTMLElement; | ||
| const codeContent = DOMContentExtractor.extractCodeBlock(elementToExtract); |
There was a problem hiding this comment.
When a Gemini response wrapper contains prose plus a nested ordinary code-block, this descendant lookup claims the entire wrapper and returns after exporting only the first code block. DOMContentExtractor.processNodes() therefore never reaches its subsequent recursion branch, even though that branch explicitly preserves surrounding prose, additional blocks, and DOM order; restrict this adapter hook to a direct code-block/.code-block, leaving wrappers to recursive traversal as before.
AGENTS.md reference: AGENTS.md:L69-L69
Useful? React with 👍 / 👎.
| const source = child.closest('[data-math-source]') ?? child.closest('[role="math"]'); | ||
| const latex = ( | ||
| source?.getAttribute('data-math-source') || | ||
| source?.getAttribute('aria-label') || | ||
| '' | ||
| ).trim(); |
There was a problem hiding this comment.
On the supported legacy/older ChatGPT KaTeX shape, the original TeX is stored in annotation[encoding="application/x-tex"] rather than on a data-math-source or role="math" ancestor. Both export formula helpers therefore fail to recognize these formulas and traverse the rendered MathML as ordinary text instead of emitting $...$ or $$...$$; reuse the annotation fallback already implemented by FormulaCopyService.extractLatexSource().
Useful? React with 👍 / 👎.
| const data = await toDataUrl(src); | ||
| if (data) img.src = data; | ||
| else img.remove(); |
There was a problem hiding this comment.
When an image is already rendered on the page but cannot be refetched—for example, a third-party image without CORS whose host is excluded from the new runtime-fetch allowlist—toDataUrl() returns null and this branch removes the node before printing. Native printing could still use the browser's already-loaded image, so a best-effort inlining failure now guarantees that content disappears from the PDF; leave the original src intact on failure, as before. The same remove-on-failure behavior in ImageExportService.inlineImages() should be corrected consistently.
AGENTS.md reference: AGENTS.md:L69-L69
Useful? React with 👍 / 👎.
ChatGPT exposes client-created-root with the same container attribute as real turns. Filter that explicit sentinel in the collector instead of dropping the first position, preserving the first real message. Refs Nagi-ovo#841 Co-authored-by: Codex <codex@users.noreply.github.com>
|
Follow-up Verified against the current ChatGPT DOM and with a red/green regression test; full local suite passes (288 files / 2656 tests), typecheck passes, and @codex review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/content/export/adapter/__tests__/chatgpt.test.ts (1)
108-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert completion without a materialization timer.
The
scrollIntoViewassertion does not prove the zero-wait path. A regression can retainwait(80)and still pass. Start the promise under fake timers, assertvi.getTimerCount()is0after a microtask, then await the promise.🤖 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/pages/content/export/adapter/__tests__/chatgpt.test.ts` around lines 108 - 123, Update the zero-wait test around buildChatGptTurnsForSelection to use fake timers, start the promise without immediately awaiting it, flush a microtask, and assert vi.getTimerCount() is 0 before awaiting the promise. Retain the existing turn-content and scrollIntoView assertions.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/pages/content/export/adapter/__tests__/chatgpt.test.ts`:
- Around line 108-123: Update the zero-wait test around
buildChatGptTurnsForSelection to use fake timers, start the promise without
immediately awaiting it, flush a microtask, and assert vi.getTimerCount() is 0
before awaiting the promise. Retain the existing turn-content and scrollIntoView
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61f7f4a2-517d-414e-929a-e0b169585bab
📒 Files selected for processing (2)
src/pages/content/export/adapter/__tests__/chatgpt.test.tssrc/pages/content/export/adapter/chatgpt.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/content/export/adapter/chatgpt.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c28085bbdf
ℹ️ 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".
| // ChatGPT exposes a conventional assistant root. | ||
| const assistantElement = | ||
| container.querySelector<HTMLElement>(ASSISTANT_MESSAGE_SELECTOR) ?? container; | ||
| const assistantContent = DOMContentExtractor.extractAssistantContent(assistantElement); |
There was a problem hiding this comment.
When a ChatGPT response contains a Markdown link or citation such as <a href="https://example.com">source</a>, this snapshot delegates to DOMContentExtractor, whose inline traversal recurses through anchors without emitting their href. The captured text and HTML therefore retain only “source,” so Markdown and JSON exports lose the URL and PDF/image exports lose the clickable destination; add explicit anchor handling when building the snapshot.
Useful? React with 👍 / 👎.
| const assistantHtml = | ||
| turn.assistantContent?.html ?? | ||
| (turn.assistantElement | ||
| ? DOMContentExtractor.extractAssistantContent(turn.assistantElement).html | ||
| : this.formatPlainTextAsHtml(turn.assistant)); |
There was a problem hiding this comment.
When structured traversal captures no HTML but the extractor's plain-text fallback still produces assistant text, assistantContent.html is the empty string. The nullish-coalescing expression treats that as a complete snapshot, so image export renders “No content” instead of the captured reply; fall back to formatted turn.assistant whenever snapshot HTML is empty, as the PDF path already does.
Useful? React with 👍 / 👎.
| const sorted = computeSortedMessages(pairsInput); | ||
| allMessageIds = sorted.map((m) => m.messageId); | ||
| const selectionMessages = resolveSelectionMessages(pairsInput); | ||
| allMessageIds = selectionMessages.map((m) => m.messageId); |
There was a problem hiding this comment.
When ChatGPT switches a response branch, regenerates a reply, or edits a prompt without changing the /c/... URL, the refreshed container list can drop a previously selected message ID. This assignment replaces allMessageIds but leaves that stale ID in selectedIds, so the count and Export button remain enabled while collection fails with chatgpt_export_messages_missing; because its checkbox is gone, the user cannot deselect it individually. Remove selections absent from the refreshed registry or cancel the selection session when IDs disappear.
Useful? React with 👍 / 👎.
ChatGPT mounts image-card controls before the generated img node. Treating those labels as content let exports complete with an empty image response, so require a usable image URL and verify the snapshot retained image content.\n\nCo-authored-by: Codex <codex@users.noreply.github.com>
|
补了一个 ChatGPT 生成图导出修复（ 本地：typecheck、288 个测试文件 / 2658 tests、Chrome production/dev build 均通过。全仓 lint 仅被本地未跟踪的 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef7765642f
ℹ️ 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".
| const blob = await response.blob(); | ||
| return reserveBudget(blob, normalizeImageType(blob.type) ?? contentType, budget); |
There was a problem hiding this comment.
When a CORS-accessible image response omits Content-Length or declares an inaccurate value, response.blob() buffers the entire body before reserveBudget() applies the 8 MB per-image and 40 MB total limits. A very large or unbounded image referenced by a conversation can therefore consume excessive memory or hang/crash the export despite the advertised bounds; read the response stream incrementally and abort once the applicable limit is exceeded.
Useful? React with 👍 / 👎.
| function hasMountedContent(turn: ChatGptTurnContainer): boolean { | ||
| if (turn.container.querySelector(IMAGEGEN_SELECTOR)) { | ||
| // Image-generation cards mount their Edit/Share controls before the image. | ||
| // Those labels are not exportable response content, so wait for a usable | ||
| // image URL instead of snapshotting the placeholder card. | ||
| return hasUsableGeneratedImage(turn.container); |
There was a problem hiding this comment.
When a completed ChatGPT assistant turn contains an image-generation card whose image failed to load, was blocked, or was removed while explanatory/error text remains, this branch ignores all mounted text and requires a usable <img> solely because the card exists. Materialization consequently times out and aborts the entire conversation export; only require the image for image-only placeholder cards, or fall back to the conventional assistant text after the wait.
Useful? React with 👍 / 👎.
| `<pre><code class="language-${language}">${DOMContentExtractor.escapeHtml(code)}</code></pre>`, | ||
| ); | ||
| textParts.push(`\n\`\`\`${language}\n${code}\n\`\`\`\n`); |
There was a problem hiding this comment.
When a ChatGPT response contains code with a triple-backtick sequence, such as an example of Markdown syntax, this fixed three-backtick delimiter closes at the embedded sequence and exports the remainder as prose or malformed Markdown. Choose a backtick fence longer than the longest run in code (or a safe tilde fence) so Markdown and Markdown-formatted JSON snapshots preserve the complete code block.
Useful? React with 👍 / 👎.
| const contentOnly = element.cloneNode(true) as HTMLElement; | ||
| chatgptGetUserAttachmentCandidates(contentOnly)?.forEach((candidate) => candidate.remove()); | ||
| const fallback = DOMContentExtractor.normalizeText(contentOnly.textContent || ''); | ||
| if (fallback) textParts.push(fallback); |
There was a problem hiding this comment.
When a ChatGPT prompt contains multiple lines or indented pasted code, normalizeText() replaces every whitespace run—including newlines and indentation—with one space. The resulting snapshot is reused by Markdown, JSON, PDF, and image exports, so the prompt's structure is irretrievably flattened in every format; preserve line boundaries and normalize only incidental horizontal whitespace.
Useful? React with 👍 / 👎.
2 participants
AI-Assisted PR Policy / AI 辅助 PR 政策
voyager-contributeskill bundled in this repo — it walks the agent through this exact checklist plus the repository-specific pitfalls that cost past PRs the most review rounds. / 如果你使用 AI Agent（Claude Code、Codex 等），请让它使用仓库自带的voyager-contributeskill——它会带着 Agent 走完本清单，并覆盖历史 PR 中最耗评审轮次的仓库特有陷阱。Description / 描述
本 PR 添加了基本的 ChatGPT 的对话导出功能，并设计了对话导出功能的平台适配器
Related Issue / 相关 Issue
Closes #841
community-only, I was assigned after maintainer approval before starting. / 如果 Issue 带有community-only标签，我已在开始前获得维护者确认并被分配。Visual Proof / 可视化证据
Browser Testing / 浏览器测试
Tested commit / 测试提交:
Missing checks and owner, or N/A reason / 缺失检查与负责人，或不适用理由:
Commands not run and reason / 未运行命令及原因:
Checklist / 检查清单
bun run format,bun run lint, then the standard localbun run verify:pr, or listed every omitted command and reason above. / 我已依次运行格式化、自动修复及标准本地bun run verify:pr验证，或在上方逐项说明未运行命令及原因。Summary by CodeRabbit