build(extension): add TARGET-driven dual builds with Firefox manifest#2000
Open
ManthanNimodiya wants to merge 2 commits into
Open
build(extension): add TARGET-driven dual builds with Firefox manifest#2000ManthanNimodiya wants to merge 2 commits into
ManthanNimodiya wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "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", |
Contributor
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.
Comment on lines
+9
to
+16
| 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], | ||
| }, |
Contributor
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.![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Comment on lines
+1
to
+46
| 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.
Suggested change
| 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", | |
| ), | |
| ); | |
| 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)); | |
| 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", | |
| ), | |
| ); |
| 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: