Feature/firefox-extension - #17
Conversation
WalkthroughAdds multi-browser extension build and packaging (Chrome + Firefox), manifest templates and runtime generation, new npm scripts and web-ext devDependency, packaging script enhancements, documentation (AGENTS.md, CLAUDE.md), README Firefox instructions, changelog entry, CI workflow for Firefox releases, and origin guards in two provider files. Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant npm as "npm scripts"
participant build as "script/build.js"
participant esbuild as "esbuild"
participant fs as "File System (src/ → dist/)"
participant package as "script/package.sh"
Developer->>npm: npm run build:firefox (--prod)
npm->>build: invoke with flags
build->>esbuild: run bundling
esbuild->>fs: emit bundles to dist/
esbuild-->>build: build success
build->>fs: read src/manifest/base.json
build->>fs: read src/manifest/firefox.json
build->>build: merge manifests
build->>fs: write dist/manifest.json
build-->>npm: return success
Developer->>npm: npm run package:firefox
npm->>package: call with BROWSER=firefox
package->>fs: read dist/manifest.json (name, version)
package->>package: create .xpi via web-ext
package-->>Developer: produced .xpi / instructions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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. 🔧 ast-grep (0.40.0)src/providers/bitcoin-provider.ts[ ... [truncated 6047 characters] ... "message": "Detected usage of wildcard '' as origin in postMessage. Always specify an exact target origin instead of using '' to prevent sensitive data from being sent to malicious websites and avoid spoofing attacks.", src/providers/nostr-provider.ts[ ... [truncated 6043 characters] ... "message": "Detected usage of wildcard '' as origin in postMessage. Always specify an exact target origin instead of using '' to prevent sensitive data from being sent to malicious websites and avoid spoofing attacks.", Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
script/package.sh (4)
28-28: Make error message browser-aware.The error message suggests running
npm run build:firefoxregardless of which browser is being packaged. For Chrome builds, this could be misleading.Apply this diff:
- echo "Run 'npm run build' or 'npm run build:firefox' first" + echo "Run 'npm run build:$BROWSER' first"
48-48: Consider using pushd/popd or absolute paths.Changing directories with
cdcan be fragile if the script is interrupted or if assumptions about the directory structure change. The./*glob also excludes hidden files (dotfiles), which might be intentional but should be verified.Consider using
pushd/popdfor safer directory management:- cd "$DIST_DIR" && zip -r "../$OUTPUT_FILE" ./* && cd .. + pushd "$DIST_DIR" > /dev/null && zip -r "../$OUTPUT_FILE" ./* && popd > /dev/nullOr use an absolute path approach:
- cd "$DIST_DIR" && zip -r "../$OUTPUT_FILE" ./* && cd .. + (cd "$DIST_DIR" && zip -r "$(pwd)/../$OUTPUT_FILE" ./*)
71-71: Clarify the full path in instructions.The instruction references
$OUTPUT_FILE, but users might not know it's relative to the build directory.- echo " 4. Select $OUTPUT_FILE or dist/manifest.json" + echo " 4. Select $OUTPUT_FILE or $DIST_DIR/manifest.json"
117-120: Consider exit status when CRX creation is skipped.Exiting with status 0 when no browser is found indicates success, but the CRX file wasn't created. Depending on CI/CD expectations, this might hide issues.
Consider whether a non-zero exit would be more appropriate for automation scenarios, or add a flag to control this behavior.
script/build.js (1)
34-34: Shallow merge overwrites nested objects and arrays—consider using deep merge.Line 34 uses the spread operator which performs only a shallow merge. In this codebase, this causes issues: Firefox's
web_accessible_resourcesarray completely replaces the base manifest's array, losing development-specific patterns (localhost, 127.0.0.1, etc.). Similarly, if browser-specific manifests definepermissionsoroptional_permissions, those arrays would fully override the base values rather than merging. For proper manifest composition, consider using a deep merge utility like Lodash's _.merge() function, which deeply merges properties from source objects.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/manifest.jsonis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
AGENTS.md(1 hunks)CLAUDE.md(6 hunks)package.json(2 hunks)script/build.js(2 hunks)script/package.sh(3 hunks)src/manifest/base.json(1 hunks)src/manifest/chrome.json(1 hunks)src/manifest/firefox.json(1 hunks)
🔇 Additional comments (10)
CLAUDE.md (3)
7-7: LGTM!Clear and accurate project description update.
36-42: LGTM!Architecture section properly documents the build system entry points and includes the nostr-provider.
187-199: LGTM!Helpful additions documenting the message flow and global state management, which will assist developers working with the extension.
src/manifest/chrome.json (1)
1-5: LGTM!Minimal and correct Chrome-specific manifest override. Chrome MV3 service workers don't require the
type: "module"field (unlike Firefox), so this correctly provides only the necessary configuration.src/manifest/firefox.json (1)
12-21: Broader HTTP match pattern differs from base manifest.The Firefox manifest declares
web_accessible_resourcesmatchinghttp://*/*(all HTTP origins), whilebase.jsonrestricts HTTP access to localhost patterns only (http://localhost:*/*,http://127.0.0.1:*/*, etc.).If intentional for Firefox compatibility, this is fine. Otherwise, consider aligning the patterns or documenting why Firefox needs broader access.
src/manifest/base.json (1)
1-45: LGTM!Well-structured base manifest with appropriate MV3 configuration:
- Permissions are appropriately scoped (
storage,activeTab,windows)all_frames: truecorrectly enables NIP-07 in iframesweb_accessible_resourcessensibly restricts HTTP access to localhost patterns while allowing all HTTPS originsscript/build.js (2)
11-16: Argument parsing is functional but basic.The current implementation works for the defined flags but doesn't handle unknown flags or provide help text. This is acceptable for an internal build script.
Consider adding a
--helpflag and validation for unknown arguments in future iterations for better developer experience.
76-83: LGTM!The build completion and error handling logic is correct. The script properly generates the manifest after a successful build and exits with an appropriate error code on failure.
package.json (2)
8-11: LGTM!The build script naming is clear and consistent. Having both
buildandbuild:chromeas identical commands provides good developer experience with a sensible default.
14-18: LGTM!The packaging and Firefox-specific scripts are well-organized. The
firefox:runscript conveniently chains the build and run commands for a smooth developer workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
script/build.js (1)
58-59: Consider deep merge for nested manifest properties.The manifest merge uses shallow spread (
{...baseManifest, ...browserManifest}), which completely replaces any nested objects that appear in both manifests. For example, if bothbase.jsonandfirefox.jsondefine apermissionsarray orbackgroundobject, the browser-specific version will entirely replace the base version rather than merging them.If your manifest structure contains nested objects that should be merged (not replaced), consider using a deep merge utility. However, if complete replacement of top-level keys is the intended behavior, the current implementation is correct.
Example: deep merge if needed
// Merge manifests (browser-specific overrides base) - const finalManifest = { ...baseManifest, ...browserManifest } + const finalManifest = deepMerge(baseManifest, browserManifest)You would need to add a deep merge helper or use a library like
lodash.merge.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/manifest.jsonis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
AGENTS.md(1 hunks)CHANGELOG.md(1 hunks)README.md(1 hunks)package.json(2 hunks)script/build.js(2 hunks)script/package.sh(3 hunks)src/manifest/base.json(1 hunks)src/manifest/firefox.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- script/package.sh
- src/manifest/base.json
- package.json
🔇 Additional comments (5)
src/manifest/firefox.json (1)
4-5: Firefox version requirement updated appropriately.The
strict_min_versionis now set to 140.0, addressing the previous concern about Firefox 128 being outdated. This aligns with current Firefox releases and provides better feature compatibility.AGENTS.md (1)
1-36: LGTM! Comprehensive contributor documentation.The documentation is well-structured and covers all essential aspects for contributors:
- Project structure with correct paths (including
src/manifest/)- Accurate build commands for Chrome and Firefox variants
- Clear coding style and naming conventions
- Practical testing guidance
- Commit and PR best practices
- Important security reminders
The previous concerns about incorrect manifest path and trailing artifacts have been addressed.
script/build.js (2)
18-45: LGTM! Robust error handling for manifest operations.The
readManifestFileandwriteManifestFilehelper functions properly address the previous concern about missing error handling. They include:
- File existence validation
- Try-catch blocks for I/O and JSON operations
- Clear, actionable error messages
- Appropriate process termination on failure
98-105: LGTM! Build flow properly integrates manifest generation.The build script correctly:
- Generates the browser-specific manifest after a successful esbuild run
- Logs clear success/failure messages
- Exits with appropriate status codes
README.md (1)
45-49: LGTM! Clear Firefox installation instructions.The Firefox installation section provides accurate, concise instructions for manual installation:
- Correct build command reference
- Proper Firefox debugging URL
- Accurate minimum version requirement (140.0) matching the manifest
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/firefox-release.yml (1)
58-62: Consider adding existence check for signed artifacts.The artifact upload step assumes that
web-ext signproduces at least one XPI file inweb-ext-artifacts/. While this should normally be the case, you might want to add an explicit check before uploading to provide a clearer error message if no artifacts are produced.Optional: Add explicit artifact existence check
+ - name: Verify signed artifacts + run: | + if ! ls web-ext-artifacts/*.xpi 1> /dev/null 2>&1; then + echo "No signed XPI files found in web-ext-artifacts/" + exit 1 + fi + echo "Found signed artifacts:" + ls -lh web-ext-artifacts/*.xpi + - name: Upload signed XPI uses: actions/upload-artifact@v4 with: name: firefox-signed-xpi path: web-ext-artifacts/*.xpi
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.github/workflows/firefox-release.yml(1 hunks)src/providers/bitcoin-provider.ts(1 hunks)src/providers/nostr-provider.ts(1 hunks)
🔇 Additional comments (7)
src/providers/bitcoin-provider.ts (1)
83-86: LGTM! Origin guard enhances security.The origin check correctly protects against cross-origin message injection by validating that incoming messages originate from the same origin before processing. This is well-placed before data validation and complements the existing
ext: 'frost2x'identifier check.src/providers/nostr-provider.ts (1)
99-102: LGTM! Consistent origin guard implementation.The origin validation mirrors the security enhancement in
bitcoin-provider.ts, ensuring consistent protection across both provider implementations. This prevents cross-origin message attacks while allowing legitimate same-origin communication..github/workflows/firefox-release.yml (5)
1-12: LGTM! Workflow trigger configuration is well-designed.The manual trigger via
workflow_dispatchis appropriate for release workflows, and thechannelinput uses a choice type with predefined options, which prevents invalid values.
14-18: LGTM! Job configuration follows best practices.The job uses appropriate permissions (read-only contents) and runs on a standard Ubuntu runner.
20-31: No Node.js version consistency issues found. The workflow uses Node.js 22, and this is the only CI workflow in the project. There are no conflicting versions in other workflows, and while Node.js is not explicitly documented in package.json engines or via .nvmrc, this does not create an alignment problem.Likely an incorrect or invalid review comment.
33-34: Verify that npm run build:firefox:prod outputs to dist directoryConfirm the build script exists in package.json and produces the Firefox extension bundle in the dist directory for the subsequent signing step.
36-56: Well-implemented signing step with proper credential validation and web-ext installed.The --channel flag is required for web-ext sign, which is correctly provided via inputs. The credential validation (lines 42-45) provides a clear error message when required secrets are missing, and the optional extension ID handling (lines 47-50) is correctly implemented using conditional bash arguments. web-ext can be installed as one of the devDependencies of a project, and verification confirms it is installed at version ^9.1.0 in package.json.
To complete the setup, ensure the following repository secrets are configured on GitHub:
AMO_JWT_ISSUER(required)AMO_JWT_SECRET(required)AMO_EXTENSION_ID(optional, but recommended for updates)
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.