refactor(extension): extract target-neutral recorder host and platform layer#1998
Conversation
| contextTypes: [chrome.runtime.ContextType.OFFSCREEN_DOCUMENT], | ||
| documentUrls: [recorderUrl], | ||
| }, | ||
| (contexts) => resolve(contexts), |
There was a problem hiding this comment.
chrome.runtime.getContexts can invoke the callback with contexts undefined (or fail in older runtimes), which would make hasRecorderHost() throw on .length. Small hardening tweak:
| (contexts) => resolve(contexts), | |
| (contexts) => resolve(contexts ?? []), |
| return new Promise<Array<{ documentUrl?: string }>>((resolve) => { | ||
| chrome.runtime.getContexts( | ||
| { | ||
| contextTypes: [chrome.runtime.ContextType.OFFSCREEN_DOCUMENT], |
There was a problem hiding this comment.
Firefox Worker Imports Chrome APIs
recorder-host.ts is imported by the service worker at module load time, but this line reads chrome.runtime.ContextType.OFFSCREEN_DOCUMENT before any capability guard can run. In a Firefox build where that Chrome-only API is missing, the background worker can throw during startup instead of reaching the supportsOffscreen checks.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/chrome-extension/src/background/recorder-host.ts
Line: 14
Comment:
**Firefox Worker Imports Chrome APIs**
`recorder-host.ts` is imported by the service worker at module load time, but this line reads `chrome.runtime.ContextType.OFFSCREEN_DOCUMENT` before any capability guard can run. In a Firefox build where that Chrome-only API is missing, the background worker can throw during startup instead of reaching the `supportsOffscreen` checks.
How can I resolve this? If you propose a fix, please make it concise.| export type ExtensionTarget = "chrome" | "firefox"; | ||
|
|
||
| export const TARGET: ExtensionTarget = | ||
| typeof __TARGET__ === "undefined" ? "chrome" : __TARGET__; |
There was a problem hiding this comment.
TARGET defaults to "chrome" whenever __TARGET__ is absent, but the changed Vite config does not define __TARGET__. A Firefox build that uses this config without an injected define will enable Chrome-only capabilities like offscreen and tab capture, sending the Firefox worker into unsupported APIs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/chrome-extension/src/platform/target.ts
Line: 8
Comment:
**Missing Target Becomes Chrome**
`TARGET` defaults to `"chrome"` whenever `__TARGET__` is absent, but the changed Vite config does not define `__TARGET__`. A Firefox build that uses this config without an injected define will enable Chrome-only capabilities like offscreen and tab capture, sending the Firefox worker into unsupported APIs.
How can I resolve this? If you propose a fix, please make it concise.
Groundwork for a Firefox port, zero Chrome behavior change.
Renames offscreen.html→recorder.html and src/offscreen/→src/recorder/; extracts the offscreen lifecycle into src/background/recorder-host.ts; adds src/platform/ (compile-time TARGET, capability flags, EXTENSION_PROTOCOL).
Fixes three sender checks that hardcoded chrome-extension: (on Firefox they'd misclassify extension pages as web pages).
Pins the recorder to the streaming pipeline explicitly (selectRecordingPipelineFromSupport(..., { preferStreamingUpload: true })) since the UA heuristic in recorder-core returns false on Firefox; Chrome output unchanged.
Greptile Summary
This PR splits the extension recorder into target-neutral pieces. The main changes are:
recorder.html.src/recorder.recorder-host.ts.Confidence Score: 4/5
The Firefox-target path can still load Chrome-only recorder-host code and can default to Chrome capabilities.
recorder-host.tsbefore any offscreen capability guard can run.__TARGET__.apps/chrome-extension/src/background/recorder-host.ts, apps/chrome-extension/src/platform/target.ts, apps/chrome-extension/vite.config.ts
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor(extension): extract target-neut..." | Re-trigger Greptile
Context used: