Add dedicated Firefox connection approval#2
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThis PR v0.2.0 introduces an approval request system with dedicated ChangesApproval Flow & Notification Commands
🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 643717b1a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/cli/src/commands/content.ts (1)
249-267: 💤 Low valueConsider the trade-off of filtering
--json.Line 251 filters all occurrences of
"--json"from argv before parsing. This is necessary because withpayloadStartPositionals: 0andvariadicAfterMin: true, any--jsonappearing after the title would be captured as part of the notification message rather than as a global output flag.However, this means users cannot include the literal string
"--json"in notification messages. For example:firefox-cli notify "Alert" "The --json flag is useful" # "--json" will be filtered outThis trade-off is reasonable for CLI ergonomics (most users expect
--jsonto control output format anywhere in the command), but it's worth being aware of this limitation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/content.ts` around lines 249 - 267, The current implementation filters every "--json" from argv which prevents users from including the literal "--json" in messages; instead stop globally filtering and let parsePayloadPositionalsAndOptions handle flags so "--json" can be parsed as an option while preserving occurrences in message text: remove the argv.filter((arg) => arg !== "--json") step in buildNotifyRequest and pass the raw argv.slice(1) into parsePayloadPositionalsAndOptions (keeping the same payloadStartPositionals/minPositionals/variadicAfterMin options), then read parsed.optionArgs for the global --json flag as needed so messageParts can contain literal "--json".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/extension/src/approval-request-service.ts`:
- Around line 116-127: approvePairing() can throw and leave pending.status as
"approving" and the promise unresolved; wrap the await approvePairing() call in
a try/catch. In the catch, if this.#pending === pending, reset pending.status
back to "pending", clear this.#pending / this.#rateLimitIndex /
this.#nextAllowedAtMs if appropriate, and ensure the pending promise is settled
(reject or resolve with an error response) so it doesn't remain unresolved; keep
the successful-path behavior (this.#pending = undefined, reset indexes,
pending.resolve(...), await this.#closeSourceTab(sourceTabId)) inside the try
block. Ensure you reference approvePairing(), pending, this.#pending,
pending.status, pending.resolve and this.#closeSourceTab(sourceTabId) when
making the change.
In `@packages/extension/src/approval-request.css`:
- Around line 11-12: The CSS custom properties --base-font and --heading-font
use fallback tokens that trigger Stylelint value-keyword-case violations; update
the fallback font-family tokens inside both --base-font and --heading-font to
use lowercase keywords (e.g., system-ui, -apple-system, blinkmacsystemfont,
segoe ui, helvetica, arial, sans-serif) so Stylelint passes while preserving the
same fallback order and quoted multi-word names.
In `@packages/extension/src/browser-commands-test-utils.ts`:
- Around line 276-279: The method showNotification currently uses a type
assertion (`ok: true as const`); instead, add an explicit return type to the
method (e.g. Promise<{ ok: true; id: string }>) and return a plain boolean
literal (`ok: true`) without `as const`, keeping the existing id expression
(options.id ?? `notification-${String(this.notifications.length)}`) and pushing
options into this.notifications so the runtime behavior is unchanged.
---
Nitpick comments:
In `@packages/cli/src/commands/content.ts`:
- Around line 249-267: The current implementation filters every "--json" from
argv which prevents users from including the literal "--json" in messages;
instead stop globally filtering and let parsePayloadPositionalsAndOptions handle
flags so "--json" can be parsed as an option while preserving occurrences in
message text: remove the argv.filter((arg) => arg !== "--json") step in
buildNotifyRequest and pass the raw argv.slice(1) into
parsePayloadPositionalsAndOptions (keeping the same
payloadStartPositionals/minPositionals/variadicAfterMin options), then read
parsed.optionArgs for the global --json flag as needed so messageParts can
contain literal "--json".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e1a8038-bc98-4b88-be7c-32f725c9b1da
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
.claude-plugin/plugin.jsonAMO_SOURCE_REVIEW.mdREADME.mddocs/all-commands-qa.mddocs/architecture.mddocs/commands.mddocs/firefox-cli-spec.mddocs/firefox-cli/updates.jsondocs/setup.mdpackage.jsonpackages/cli/package.jsonpackages/cli/src/argv-contracts.tspackages/cli/src/cli-help.test.tspackages/cli/src/cli-phase8.test.tspackages/cli/src/cli-setup-doctor.test.tspackages/cli/src/cli-test-support.tspackages/cli/src/cli.test.tspackages/cli/src/commands/content.tspackages/cli/src/commands/pairing.tspackages/cli/src/commands/setup-doctor.tspackages/cli/src/format.test.tspackages/cli/src/format.tspackages/cli/src/help.tspackages/cli/src/parse-options.tspackages/cli/src/route-registry.tspackages/cli/src/runner.tspackages/cli/src/transport.tspackages/extension/package.jsonpackages/extension/src/approval-permissions.tspackages/extension/src/approval-request-service.tspackages/extension/src/approval-request.csspackages/extension/src/approval-request.htmlpackages/extension/src/approval-request.test.tspackages/extension/src/approval-request.tspackages/extension/src/background-bootstrap-storage.test.tspackages/extension/src/background-bootstrap-test-cases.tspackages/extension/src/background-bootstrap.test.tspackages/extension/src/background-bootstrap.tspackages/extension/src/background-browser-adapter.tspackages/extension/src/background-controller-approval-race-test-cases.tspackages/extension/src/background-controller-approval-test-cases.tspackages/extension/src/background-controller-approval.test.tspackages/extension/src/background-controller-test-cases.tspackages/extension/src/background-controller-test-support.tspackages/extension/src/background-controller.test.tspackages/extension/src/background-controller.tspackages/extension/src/background-default-browser-adapter.tspackages/extension/src/background-native-session.tspackages/extension/src/background-request-forwarder.tspackages/extension/src/background-request-handler.tspackages/extension/src/browser-command/types.tspackages/extension/src/browser-commands-content.test.tspackages/extension/src/browser-commands-test-cases.tspackages/extension/src/browser-commands-test-smoke.tspackages/extension/src/browser-commands-test-utils.tspackages/extension/src/browser-handlers/phase8-browser.tspackages/extension/src/manifest.jsonpackages/extension/src/popup.tspackages/extension/src/webextension.d.tspackages/native-host/package.jsonpackages/native-host/src/host-broker-helpers.tspackages/native-host/src/host-broker.test.tspackages/native-host/src/host-broker.tspackages/protocol/package.jsonpackages/protocol/src/browser/output.tspackages/protocol/src/capabilities.tspackages/protocol/src/constants.tspackages/protocol/src/extension-requirements.tspackages/protocol/src/metadata.tspackages/protocol/src/pairing.tspackages/protocol/src/protocol-metadata-behavior.test.tspackages/protocol/src/protocol-metadata.test.tspackages/protocol/src/protocol-test-support.tspackages/protocol/src/registry/index.tspackages/protocol/src/registry/pairing.tspackages/protocol/src/registry/phase8.tspackages/protocol/src/registry/registry.test.tspackages/test-support/package.jsonscripts/build-extension.tsscripts/copy-extension-assets.tsscripts/extension-payload-check.tsscripts/test/copy-extension-assets.test.tsscripts/test/package-check-test-utils.tsskills/firefox-cli/SKILL.md
💤 Files with no reviewable changes (2)
- AMO_SOURCE_REVIEW.md
- packages/protocol/src/capabilities.ts
Gate notify on protocol v4, including batch children, and let approval request pages reload before closing themselves after terminal approve or deny flows. Update development tooling dependencies to latest, resolve the shell-quote audit through an override to 1.8.4, and record the major-tooling migration plan in docs/dependency-migrations.md. Major release-age evidence: eslint 10.0.0 published 2026-02-06; @eslint/js 10.0.1 published 2026-02-06; @types/node 25.0.0 published 2025-12-10; installed typescript 6.0.3 published 2026-04-16; vite 8.0.0 published 2026-03-12. Verification: bun run deps:check; bun run check; bun run release:check:local.
Resolve follow-up review findings by settling approval requests when native approval throws, keeping in-memory approval false when pair-token persistence fails, normalizing approval request CSS fallback casing, and removing the showNotification test assertion. Verification: bun run test packages/extension/src/background-controller-approval.test.ts packages/extension/src/approval-request.test.ts packages/extension/src/background-controller.test.ts; bun run typecheck; bun run lint; bun run check.
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.
Summary
firefox-cli connectVerification
bun run checkSummary by CodeRabbit
New Features
firefox-cli connectcommand to request Firefox control approval with dedicated approval interface.firefox-cli notifycommand to display browser notifications.Changes
firefox-cli connectinstead of extension popup approval process.Version