Uh oh!
There was an error while loading. Please reload this page.
build(extension): add TARGET-driven dual builds with Firefox manifest - #2000
build(extension): add TARGET-driven dual builds with Firefox manifest#2000ManthanNimodiya wants to merge 3 commits into
Conversation
| "scripts": { | ||
| "build": "rm -rf dist && vite build && vite build --config vite.content.config.ts && vite build --config vite.content-overlay.config.ts", | ||
| "dev": "rm -rf dist && (trap 'kill 0' INT TERM; vite build --watch --config vite.content.config.ts --mode development & vite build --watch --config vite.content-overlay.config.ts --mode development & vite build --watch --mode development & wait)", | ||
| "build": "pnpm build:chrome && pnpm build:firefox", |
There was a problem hiding this comment.
When the Chrome publish workflow runs the default build, this script now creates both dist/chrome and dist/firefox. The existing package step zips dist as the extension root, so the upload contains target subdirectories and Firefox artifacts instead of a valid Chrome extension layout.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/chrome-extension/package.json
Line: 7
Comment:
**Chrome Package Contains Both Targets**
When the Chrome publish workflow runs the default `build`, this script now creates both `dist/chrome` and `dist/firefox`. The existing package step zips `dist` as the extension root, so the upload contains target subdirectories and Firefox artifacts instead of a valid Chrome extension layout.
How can I resolve this? If you propose a fix, please make it concise.| const getRecorderContexts = async () => { | ||
| const recorderUrl = chrome.runtime.getURL(RECORDER_URL); | ||
| return new Promise<Array<{ documentUrl?: string }>>((resolve) => { | ||
| chrome.runtime.getContexts( | ||
| { | ||
| contextTypes: [chrome.runtime.ContextType.OFFSCREEN_DOCUMENT], | ||
| documentUrls: [recorderUrl], | ||
| }, |
There was a problem hiding this comment.
The shared background script imports this helper for both targets, but hasRecorderHost() and ensureRecorderHost() call Chrome-only offscreen APIs without checking the target. In the Firefox build, normal paths like starting a recording, probing the mic, enumerating devices, or connecting camera preview can reach chrome.runtime.getContexts or chrome.offscreen.createDocument, producing a runtime error instead of opening a Firefox recorder page.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/chrome-extension/src/background/recorder-host.ts
Line: 9-16
Comment:
**Firefox Reaches Offscreen APIs**
The shared background script imports this helper for both targets, but `hasRecorderHost()` and `ensureRecorderHost()` call Chrome-only offscreen APIs without checking the target. In the Firefox build, normal paths like starting a recording, probing the mic, enumerating devices, or connecting camera preview can reach `chrome.runtime.getContexts` or `chrome.offscreen.createDocument`, producing a runtime error instead of opening a Firefox recorder page.
How can I resolve this? If you propose a fix, please make it concise.| import { readFileSync } from "node:fs"; | ||
| import { resolve } from "node:path"; | ||
| import { describe, expect, it } from "vitest"; | ||
| // The two manifests are maintained by hand; these assertions keep the parts | ||
| // that must not drift (version, entry points, content scripts, resources) in | ||
| // lockstep and pin the deliberate per-browser differences. | ||
| type Manifest = { | ||
| manifest_version: number; | ||
| name: string; | ||
| short_name: string; | ||
| version: string; | ||
| homepage_url: string; | ||
| icons: Record<string, string>; | ||
| action: unknown; | ||
| background: { | ||
| service_worker?: string; | ||
| scripts?: string[]; | ||
| type?: string; | ||
| }; | ||
| permissions: string[]; | ||
| host_permissions: string[]; | ||
| content_scripts: unknown[]; | ||
| web_accessible_resources: Array<{ | ||
| resources: string[]; | ||
| matches: string[]; | ||
| use_dynamic_url?: boolean; | ||
| }>; | ||
| options_page?: string; | ||
| options_ui?: { page: string }; | ||
| browser_specific_settings?: { | ||
| gecko?: { | ||
| id?: string; | ||
| strict_min_version?: string; | ||
| }; | ||
| }; | ||
| }; | ||
| const loadManifest = (target: "chrome" | "firefox"): Manifest => | ||
| JSON.parse( | ||
| readFileSync( | ||
| resolve(__dirname, `../../manifests/manifest.${target}.json`), | ||
| "utf8", | ||
| ), | ||
| ); |
There was a problem hiding this comment.
apps/chrome-extension is type: module, so __dirname won’t exist if Vitest executes this test as ESM. Safer to derive it from import.meta.url.
| import{readFileSync}from"node:fs"; | |
| import{resolve}from"node:path"; | |
| import{describe,expect,it}from"vitest"; | |
| // The two manifests are maintained by hand; these assertions keep the parts | |
| // that must not drift (version, entry points, content scripts, resources) in | |
| // lockstep and pin the deliberate per-browser differences. | |
| typeManifest={ | |
| manifest_version: number; | |
| name: string; | |
| short_name: string; | |
| version: string; | |
| homepage_url: string; | |
| icons: Record<string,string>; | |
| action: unknown; | |
| background: { | |
| service_worker?: string; | |
| scripts?: string[]; | |
| type?: string; | |
| }; | |
| permissions: string[]; | |
| host_permissions: string[]; | |
| content_scripts: unknown[]; | |
| web_accessible_resources: Array<{ | |
| resources: string[]; | |
| matches: string[]; | |
| use_dynamic_url?: boolean; | |
| }>; | |
| options_page?: string; | |
| options_ui?: {page: string}; | |
| browser_specific_settings?: { | |
| gecko?: { | |
| id?: string; | |
| strict_min_version?: string; | |
| }; | |
| }; | |
| }; | |
| constloadManifest=(target: "chrome"|"firefox"): Manifest=> | |
| JSON.parse( | |
| readFileSync( | |
| resolve(__dirname,`../../manifests/manifest.${target}.json`), | |
| "utf8", | |
| ), | |
| ); | |
| import{readFileSync}from"node:fs"; | |
| import{dirname,resolve}from"node:path"; | |
| import{fileURLToPath}from"node:url"; | |
| import{describe,expect,it}from"vitest"; | |
| // The two manifests are maintained by hand; these assertions keep the parts | |
| // that must not drift (version, entry points, content scripts, resources) in | |
| // lockstep and pin the deliberate per-browser differences. | |
| const__dirname=dirname(fileURLToPath(import.meta.url)); | |
| typeManifest={ | |
| manifest_version: number; | |
| name: string; | |
| short_name: string; | |
| version: string; | |
| homepage_url: string; | |
| icons: Record<string,string>; | |
| action: unknown; | |
| background: { | |
| service_worker?: string; | |
| scripts?: string[]; | |
| type?: string; | |
| }; | |
| permissions: string[]; | |
| host_permissions: string[]; | |
| content_scripts: unknown[]; | |
| web_accessible_resources: Array<{ | |
| resources: string[]; | |
| matches: string[]; | |
| use_dynamic_url?: boolean; | |
| }>; | |
| options_page?: string; | |
| options_ui?: {page: string}; | |
| browser_specific_settings?: { | |
| gecko?: { | |
| id?: string; | |
| strict_min_version?: string; | |
| }; | |
| }; | |
| }; | |
| constloadManifest=(target: "chrome"|"firefox"): Manifest=> | |
| JSON.parse( | |
| readFileSync( | |
| resolve(__dirname,`../../manifests/manifest.${target}.json`), | |
| "utf8", | |
| ), | |
| ); |
| const getRecorderContexts = async () => { | ||
| const recorderUrl = chrome.runtime.getURL(RECORDER_URL); | ||
| return new Promise<Array<{ documentUrl?: string }>>((resolve) => { | ||
| chrome.runtime.getContexts( |
There was a problem hiding this comment.
This file assumes chrome.runtime.getContexts + chrome.offscreen.createDocument exist. Since this PR adds a Firefox build (and the manifest drops the offscreen permission), it might be worth adding a Firefox-specific implementation here (or at least a feature-detect + clearer error) so sendOffscreen(...) doesn’t end up throwing a TypeError at runtime.
92c255e to
718b032CompareReview the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Stacked on #1998, only the last commit(s) are new here, the earlier ones will disappear once the base PR merges.
TARGET=chrome|firefox vite builds → dist/chrome/dist/firefox.
Manifests move to manifests/manifest.{chrome,firefox}.json (vite copies the right one per target; a unit test keeps them in lockstep).
Firefox manifest: event-page background, options_ui, no offscreen/tabCapture, browser_specific_settings.gecko (id, min 128). New scripts: build:firefox, dev:firefox, run:firefox/lint:firefox (web-ext). e2e stays Chromium-only against dist/chrome. web-ext lint: 0 errors.
Greptile Summary
This PR adds target-specific extension builds for Chrome and Firefox. The main changes are:
distsubdirectories.manifests/with a new Firefox manifest.Confidence Score: 4/5
The Chrome release package and Firefox recorder startup paths can break with the new target layout.
distas one extension root.apps/chrome-extension/package.json, apps/chrome-extension/manifests/manifest.chrome.json, apps/chrome-extension/src/background/recorder-host.ts
Important Files Changed
Comments Outside Diff (1)
apps/chrome-extension/manifests/manifest.chrome.json, line 1 (link)Moving the Chrome manifest out of
public/manifest.jsonleaves the Chrome release workflow's version read pointing at a path that no longer exists. The package step fails before upload when it tries to read the removed public manifest.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "build(extension): add TARGET-driven dual..." | Re-trigger Greptile
Context used: