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.0a7575b to
ec176e4
Compare
| // install, and getDisplayMedia requires transient user activation so capture | ||
| // cannot start without a click inside the recorder document. | ||
| export const capabilities = { | ||
| supportsTabCapture: TARGET === "chrome", |
There was a problem hiding this comment.
supportsTabCapture is defined as false for Firefox, but the service worker tab-recording path still calls chrome.tabCapture.getMediaStreamId without checking it. When a Firefox build starts a tab-mode recording, that path can still dereference an API Firefox does not provide, so the recording flow fails before capture can start. This needs a separate guard or tab-mode disablement at the call path that uses the capability.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/chrome-extension/src/platform/capabilities.ts
Line: 9
Comment:
**Tab Capture Unguarded**
`supportsTabCapture` is defined as false for Firefox, but the service worker tab-recording path still calls `chrome.tabCapture.getMediaStreamId` without checking it. When a Firefox build starts a tab-mode recording, that path can still dereference an API Firefox does not provide, so the recording flow fails before capture can start. This needs a separate guard or tab-mode disablement at the call path that uses the capability.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…ument TARGET define requirement
… in Chrome build, guard tabCapture
8d0d262 to
4e327b7
Compare
| export default defineConfig({ | ||
| plugins: [react()], | ||
| define: { | ||
| __TARGET__: JSON.stringify("chrome"), |
There was a problem hiding this comment.
This shared config now always compiles __TARGET__ as "chrome". A Firefox artifact built through this config will set supportsOffscreen and supportsTabCapture to true, so the worker can still enter the Chrome-only recorder host and tab capture paths instead of using the Firefox capability set. Please make the target value come from the build target, such as a Firefox-specific config or mode/env define.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/chrome-extension/vite.config.ts
Line: 8
Comment:
**Chrome Target Hardcoded**
This shared config now always compiles `__TARGET__` as `"chrome"`. A Firefox artifact built through this config will set `supportsOffscreen` and `supportsTabCapture` to true, so the worker can still enter the Chrome-only recorder host and tab capture paths instead of using the Firefox capability set. Please make the target value come from the build target, such as a Firefox-specific config or mode/env define.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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 prepares the extension recorder for target-specific Chrome and Firefox behavior. The main changes are:
recorder.html.src/offscreentosrc/recorder.Confidence Score: 4/5
This is close, but the target define should be fixed before merging.
Files Needing Attention: apps/chrome-extension/vite.config.ts
Important Files Changed
__TARGET__, but the shared config hardcodes it to"chrome".Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(extension): use string literal for C..." | Re-trigger Greptile
Context used: