Durable review guidance for Firefox's built-in IP Protection (VPN) module, covering panel UI, proxy/channel filtering, authentication, telemetry, and localization.
73
58%
Does it follow best practices?
Impact
97%
2.20xAverage score across 3 eval scenarios
Passed
No known issues
Optimize this skill with Tessl
npx tessl skill review --optimize ./plugins/ipprotection/skills/ipprotection-review/SKILL.mdbrowser/components/ipprotection/**/*, toolkit/components/ipprotection/**/*IPProtectionPanel.setState({...}) rather than having child components mutate panel state directly; keep IPProtectionPanel as the single source of truth for panel state, and prefer batched setState calls over property-by-property assignment.CustomEvent dispatched from the shared root and consumed by the panel/manager, rather than sharing state between sibling files. Use Services.obs observers (e.g. perm-changed) instead of calling into internal callbacks when the platform already emits the signal.addObserver/removeObserver or addEventListener pairs, bind the handler once in the constructor (or use handleEvent) — do not pass .bind(this) at registration time, since the two bindings won't match on removal.ipprotection-constants.mjs or the relevant *Helpers.sys.mjs, and import them everywhere they're referenced. Literals like 50, 150, 0.75, 0.9 in component code or Fluent strings are review-blocking.$variables; never hardcode units or amounts inside .ftl messages. Units (MB, GB) must be hardcoded into the message text, not passed as variables, since localizers translate them differently.browser/components/ipprotection/docs/Preferences.rst (or the toolkit equivalent) with correct type.python/l10n/fluent_migrations/ so translations aren't lost. Dropping an old ID is a separate, deliberate step once no callers remain.Intl.DisplayNames / Services.intl.DisplayNames and a single parameterized string.browser/locales/en-US/browser/ipProtection.ftl and must be wired into browser.xhtml outside the "Untranslated FTL" block.--space-*, --border-color-card, --icon-color, --font-weight-*, --dimension-*) instead of raw pixel values or hardcoded colors; SVG assets that need theming must use context-fill / context-stroke and let CSS set the color.padding-block-*, padding-inline-*, margin-block-*) so layouts work in RTL. Any directional glyph (arrow-right, etc.) or hand-rolled class like left/right is a red flag.toolkit/themes/shared/icons/ before adding new SVGs; optimize any new SVG (e.g. via SVGOMG) and place module illustrations under browser/components/ipprotection/assets/ (not browser/base/content/logos/).moz-button / moz-card and existing panel conventions (subviewbutton-nav, overflow attributes) over re-implementing styles; avoid overriding --arrowpanel-* variables when a local margin/padding change will do.overflows="true" to detect overflow — check overflowedItem="true" or cui-areatype="panel" (or set subviewbutton-nav at creation time).SpecialPowers.pushPrefEnv; it auto-reverts on teardown, so don't add manual clearUserPref cleanup for prefs it set.TestUtils.waitForCondition to await things that can be observed deterministically; resolve a promise from inside the stub/handler you care about. This is a 100 ms-per-check polling cost that compounds across CI.browser/xpcshell test. Use the project tag testing-approved, or one of the documented exceptions with a one-line justification, on every revision.browser_ipprotection_* tests (e.g. sandbox.stub(IPPProxyManager, "state").value(...)) and setupVpnPrefs / cleanupStatusCardTest helpers rather than reinventing setup.metrics.yaml must carry a #data-classification-* tag with a one-line justification, and the data_sensitivity property must match. Add vpn-telemetry@mozilla.com to notification_emails for new IPP metrics.counter/event metrics over bespoke aggregation; record state transitions (e.g. "was active before pause") rather than current state when the event already implies it.defaultProxyInfo to onProxyFilterResult — never null — so the user's system proxy is honored.nsIIOService::hostnameIsLocalIPAddress (and related platform APIs) over regex for IP/host classification.cancelChannelFilter, stop, abort reason) in every exit path, including error and paused transitions.toolkit/: New non-UI logic (state machine, proxy manager, guardian client, auth provider) should land in toolkit/components/ipprotection/ and avoid depending on desktop-only singletons (CustomizableUI, EveryWindow, etc.); platform-specific glue lives under fxa/ or android/ subdirs. Context: likely to fade once the Android/Fenix integration is fully wired up and the browser/ → toolkit/ migration is complete.IPPAuthProvider rather than reaching into GuardianClient singletons; avoid introducing cycles between IPPService and auth helpers. Context: likely to fade once the provider refactor lands and stabilizes.bandwidth-usage, ipprotection-content, IPProtectionInfobarManager); new callers should factor through a shared helper rather than re-implementing Math.floor / toFixed logic. Context: likely to fade once a single rounding utility is extracted.Math.floor for remaining bandwidth where UX expects one-decimal precision (notably at the 75% bucket), or inverting the progress bar by binding it to remaining instead of used.data-l10n-id accessibility attributes, or duplicating an aria-label that's already provided by tooltiptext.targeting decide.cfr group, without previousSessionEnd, or without !hasActiveEnterprisePolicies && !activeNotifications. Omitting dismiss: true on CTA button actions.lifetime frequency cap and without considering a Nimbus rollout for safe kill-switching.toolkit/themes/shared/icons/; placing state illustrations in browser/base/content/logos/.mach lint failures (fluent-lint, eslint, stylelint, file-whitespace) through to review; these should be clean before requesting review.mots.yaml without running mots clean.browser/components/ipprotection/content/*.mjs — Components must read from this.state populated by IPProtectionPanel; they should dispatch CustomEvents upward rather than mutating panel state. Use logical CSS properties and design tokens.browser/components/ipprotection/IPProtection*.sys.mjs — Panel/manager/alert code owns state mutations and pref observers. Prefer setState batching; document new prefs in docs/Preferences.rst.toolkit/components/ipprotection/** — Keep cross-platform-safe (no CustomizableUI, EveryWindow, browser/-only imports). Platform-specific glue goes in fxa/ or android/ subdirs (campaign).browser/components/ipprotection/IPPChannelFilter.sys.mjs — Always pass defaultProxyInfo through to excluded channels; store it alongside pending channels so reprocessing preserves it.browser/components/ipprotection/content/*.css + browser/themes/shared/** — Use design tokens and logical properties; prefer .toolbarbutton-icon over raw image; reuse subviewbutton-nav for subview arrows.browser/locales/en-US/browser/ipProtection.ftl — New/changed IDs require a migration. Pass numbers as $variables; hardcode units. Comments attach only to the next message.browser/components/asrouter/modules/FeatureCalloutMessages.sys.mjs — New callouts belong in the cfr group with previousSessionEnd, !hasActiveEnterprisePolicies && !activeNotifications, and dismiss: true on CTA buttons.**/metrics.yaml — New metrics need data-classification tag, matching data_sensitivity, and vpn-telemetry@mozilla.com in notifications.browser/components/ipprotection/tests/** — Prefer SpecialPowers.pushPrefEnv and promise-resolving stubs over waitForCondition; reuse existing head.js helpers.browser/components/ipprotection/docs/** — Keep StateMachine.rst, Preferences.rst, Constants.rst, Components.rst in sync with code changes in the same patch.mach lint --outgoing clean (eslint, stylelint, fluent-lint, file-whitespace, rejected-words).setState; no cross-file direct state writes.$variables.docs/Preferences.rst and use matching constants in code.context-fill/context-stroke; tokens (not pixel literals) drive spacing and color; logical properties used.pushPrefEnv and promise-based stubs; no waitForCondition unless truly necessary; testing-policy tag applied.metrics.yaml changes carry a data-classification tag and correct data_sensitivity; notification emails include vpn-telemetry@mozilla.com.StateMachine.rst, Components.rst, etc.) updated in the same patch when behavior changes.mots.yaml run through mots clean if touched.f73d56b
If you maintain this skill, you can claim it as your own. Once claimed, you can manage eval scenarios, bundle related skills, attach documentation or rules, and ensure cross-agent compatibility.