Skip to content

feat(native): one door to app-local plugins, with the fallback made mandatory - #2978

Merged
innolope-dev merged 7 commits into
devfrom
chore/native-capability
Sep 4, 2026
Merged

feat(native): one door to app-local plugins, with the fallback made mandatory#2978
innolope-dev merged 7 commits into
devfrom
chore/native-capability

Conversation

@innolope-dev

Copy link
Copy Markdown
Collaborator

The runtime half of the OTA/native split. #2977 stops an incompatible bundle being published; this makes the JS degrade when it reaches a binary that doesn't have what it expects — which is the layer that works regardless of any Capgo channel setting.

Why

Every call to an app-local Capacitor plugin has to survive the same thing: the JS half ships over the air onto binaries built months earlier, where the plugin is not there. Capacitor answers a missing native method with a rejected promise, not a compile error — so a forgotten try/catch is a crash on a user's device that no type and no test sees.

The pattern to get this right already existed in all three call sites. Nothing enforced it, and nothing stopped a fourth from being written without it.

What nativeCapability() closes

Three hazards, in one place rather than at each call site:

  1. Missing plugin — older binary running OTA'd JS: the call rejects, the caller's fallback answers.
  2. Wrong platformregisterPlugin hands back a perfectly good proxy on web, where invoking it rejects. The platform gate means no call is attempted at all.
  3. The thenable trap — the proxy answers ANY property with a native-method wrapper, .then included, so resolving a promise with a plugin leaves it pending forever. That shipped twice (getPreferences in 1.0.44, the Crisp helper in 1.0.45–1.0.47) and already has its own eslint rule and test mock. Here the proxy never escapes the closure it's handed to, so it's unreachable by construction rather than by review.

The one deliberate bit of friction

The fallback is a function, never a bare value:

return ClipboardDetect.call(async (plugin) => (await plugin.hasStrings()).value, () => false)

Every call site therefore has to state what "this device can't do it" means — which is exactly the omission that turns a missing plugin into a crash. It receives the rejection, for callers that report it:

return PushProvisioning.call(
    (plugin) => plugin.addCard(args),
    (error) => ({ added: false, error: error instanceof Error ? error.message : 'unavailable' })
)

Scope

All three existing call sites migrated with no behaviour changeClipboardDetect (iOS), InstallReferrer (Android), PushProvisioning (both). One small improvement falls out: addCardToWallet previously had a catch but no platform gate, so on web it attempted the call and relied on the rejection; it's now gated like everything else.

An eslint rule bans registerPlugin outside the wrapper, so a new plugin cannot bypass it. Verified firing on a synthetic file and clean on the wrapper and all three migrated modules.

7 tests, using the house createPluginProxy mock rather than a plain object — a plain object has no .then, so it would let the trap through. One test guards the guard: if createPluginProxy ever stopped rejecting for absent methods, every "older binary" case above would pass vacuously.

Deliberately not included

Version-gated capabilities stay bespoke. canRestartInPlace() in capgo-updater.ts reads the native plugin's own version (getPluginVersion()) to decide whether an in-place restart deadlocks — "does the plugin exist" and "how does this build of it behave" are different questions, and folding the second into this wrapper would over-generalise it. That one is the reference for the version-gated shape.

Direct @capacitor/* imports are not banned. Those are official plugins bundled with the shell and mostly reached through dynamic imports with a .catch already; banning them would be a much wider change for much less benefit. The app-local plugins are the ones that genuinely don't exist on older binaries.

…andatory

Every call to an app-local Capacitor plugin has to survive the same thing:
the JS half ships over the air onto binaries built months earlier, where
the plugin is not there. Capacitor answers a missing native method with a
rejected promise, not a compile error, so a forgotten try/catch is a crash
on a user's device that no type and no test sees.

nativeCapability() closes three hazards in one place instead of at each
call site: a missing plugin (older binary), a platform with no native half
(registerPlugin hands back a working proxy on web, where invoking it
rejects), and the thenable trap — the proxy answers ANY property with a
native-method wrapper, .then included, so resolving a promise WITH a
plugin leaves it pending forever. That one shipped twice, in 1.0.44 and
1.0.45-1.0.47; here the proxy cannot escape the closure it is handed to,
so it is unreachable by construction rather than by review.

The fallback is a function, never a bare value, so every call site must
state what 'this device can't do it' means — the omission that turns a
missing plugin into a crash. It receives the rejection for callers that
report it.

Migrates all three existing call sites (ClipboardDetect, InstallReferrer,
PushProvisioning) with no behaviour change, and adds an eslint rule so a
new one cannot bypass the wrapper. addCardToWallet gains a platform gate
it was missing.

Version-gated capabilities stay bespoke: canRestartInPlace() in
capgo-updater.ts reads the native plugin's own version, which is a
different question from whether the plugin exists.
@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
peanut-wallet Ready Ready Preview Sep 4, 2026 4:04pm UTC

Request Review

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 6af486c7-210d-4757-900f-543a785e51d9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Code-analysis diff

Painscore total: 7149.37 → 7152.83 (+3.46)
Findings: 0 net (+4 new, -4 resolved)

🆕 New findings (4)

  • critical complexity — src/utils/deferred-link.ts — CC 61, MI 58.08, SLOC 211
  • medium high-mdd — src/utils/deferred-link.ts:231 — doRestore: MDD 28.6 (uses across many lines from declarations)
  • medium method-complexity — src/utils/deferred-link.ts:231 — doRestore CC 16 SLOC 68
  • medium complexity — src/utils/native-capability.ts — CC 8, MI 61.48, SLOC 25

✅ Resolved (4)

  • src/utils/deferred-link.ts — CC 60, MI 57.51, SLOC 210
  • src/utils/deferred-link.ts:232 — doRestore: MDD 28.6 (uses across many lines from declarations)
  • src/utils/deferred-link.ts:232 — doRestore CC 16 SLOC 68
  • src/utils/push-provisioning.ts — CC 6, MI 63.15, SLOC 26

📈 Painscore deltas (top movers)

File Before After Δ
src/utils/native-capability.ts 0.0 5.9 +5.9
src/utils/push-provisioning.ts 4.1 2.9 -1.2
src/utils/clipboard-detect.ts 4.0 2.7 -1.3

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🧪 UI test report — ✅ all green

Suites

  • unit: 5796 ran, 0 failed, 0 skipped, 1.8m

📊 Coverage (unit)

metric %
statements 74.5%
branches 59.8%
functions 68.1%
lines 75.5%
⏱ 10 slowest test cases
time test
🐢 9.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › Network failure keeps loading while retries remain, then shows the generic error
4.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › MANTECA_SOURCE_OVER_MONTHLY_CAP fails fast with copy that names the real cause
4.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › MANTECA_MERCHANT_RECENT_REFUND fails fast with copy that names the real cause
4.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › MANTECA_USER_NOT_PROVISIONED fails fast with copy that names the real cause
4.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › User KYC not approved fails fast with copy that names the real cause
4.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › MANTECA_MERCHANT_VOLUME_NEAR_CAP fails fast with copy that names the real cause
4.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › routes the KYC rejection on its wire code, and does not retry it
4.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › a refused idempotency key tells the user to scan again, not to contact support
3.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › Scan that recovers on the retry lands on the payment screen, not an error
3.0s src/app/(mobile-ui)/qr-pay/__tests__/qr-pay-states.test.tsx › Going offline blames the connection, and reconnecting clears it for the recovered scan
📍 Inline annotations are in the **Unit test report** check above. Coverage artifact: `coverage-unit`. Generated by `.github/workflows/tests.yml`.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🖼 Visual diff — 6 screens moved

9 of 66 shots changed · 57 identical · baseline 6bee368 → head 8e5462b

worst % screen widths
3.51% avatar-picker 320, 430
3.19% profile 320
0.07% unverified 320, 430
0.07% withdraw 320, 430
0.03% badges 430
0.01% kyc-action-required 320

job summary · before/after/diff images — artifact

Fixture screenshots, no backend. Advisory — this check never blocks a merge. Posted from the default branch by ds-shots-comment.yml; the report it renders is untrusted data.

@chip-peanut-bot chip-peanut-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chip review — no blocking findings — this is not an approval

The three migrated native-plugin callers preserve their fallbacks, but the new abstraction does not fully enforce its thenable-safety guarantee and one migrated path is covered only by order-dependent tests.

Findings

  • MINOR · src/utils/native-capability.ts:67 · Keep the plugin proxy out of callback results
    call accepts any Promise<R> from its caller and then awaits it. On a supported platform, capability.call(async (plugin) => plugin, fallback) is type-correct; the async callback assimilates the Capacitor proxy's .then, which never invokes either resolver, so call hangs without reaching this catch or the fallback. That is the exact failure this abstraction says is impossible. Shape the API so callers select/invoke a method without receiving a returnable proxy (or otherwise make returning T impossible), and add a proxy-return regression test.

  • MINOR · src/utils/push-provisioning.ts:71 · Make the add-card tests platform-independent
    This call is now platform-gated, but the addCardToWallet tests never enable iOS or Android. They pass only because the preceding availability test leaves mockIsAndroidNative(true), and jest.clearAllMocks() does not reset mock return values; running either add-card test alone takes the unsupported-platform fallback and never invokes addCard. Reset both platform mocks in that describe and explicitly enable a supported platform for the native success and error cases.

  • MINOR · src/utils/tests/push-provisioning.test.ts:64 · [claude-opus] New platform gate on addCardToWallet is untested; its existing tests now pass only via mock leakage
    addCardToWallet() provisions a card credential into Apple/Google Wallet — a mutation of external state that CONTRIBUTING.md requires test coverage for. Before this PR it had no platform gate: it called PushProvisioning.addCard() and try/caught. This PR routes it through nativeCapability.call(), which adds a gate that, off-native, returns { added: false, error: 'PushProvisioning is not available on this platform' } WITHOUT invoking the plugin. That new behaviour has no test.

Worse, the two existing addCardToWallet tests now pass only by accident. Their beforeEach (src/utils/tests/push-provisioning.test.ts:64-66) calls jest.clearAllMocks() only, which per the file's own comment on line 28 keeps implementations. The preceding describe's last test sets mockIsAndroidNative.mockReturnValue(true) (line 54), and there is no resetMocks/clearMocks in the package.json jest block and no global beforeEach in jest.setup.ts — so android stays true and the gate happens to open. Run that describe with .only, or reorder the file, and 'passes through the native result' and 'never throws' both fail, because the result becomes { added: false, error: 'PushProvisioning is not available on this platform' } instead of the native answer.

Fix: in the addCardToWallet describe's beforeEach, set the platform mocks explicitly the way the first describe does (e.g. mockIsIOSNative.mockReturnValue(true)), and add one case asserting that off-native addCardToWallet returns added:false and never calls addCard.

Checked clean

  • Verified the detached worktree HEAD, trusted author, supplied base ref and SHA, and exact merge base.
  • Traced all three migrated app-local plugins through iOS, Android, web, older-binary, native-rejection, and fallback paths.
  • Inspected the native plugin registrations and confirmed no production registerPlugin call remains outside the wrapper.
  • Reviewed card-secret handling, native error propagation, injection boundaries, and privilege changes; no new security exposure was found.
  • Exact-head aggregate, unit, native-export, typecheck, eslint, format, analyze, design-system, provenance, and preview checks passed; ds-shots was still in progress at the final CI read.
  • git diff --check passed; no product or sibling-API contract changed.

Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.

Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.

Exact head: bde2960c8a67 · Context: repo, ci · Took 9m

Comment thread src/utils/native-capability.ts Outdated
Comment thread src/utils/push-provisioning.ts Outdated
Chip was right that the callback signature did not actually close the
thenable trap it claimed to. call(async (plugin) => plugin, fallback) is
type-correct, and the async function assimilates the proxy's .then while
resolving — so it hangs before reaching the catch or the fallback. There is
no runtime guard available: the hang happens before any of our code runs
again, so the invariant has to hold at the type level.

call now takes a method name, its options and the fallback. The plugin is
never handed out, so returning it is not something a caller can express.
The call sites read better for it, and two @ts-expect-error assertions
guard the invariant — typecheck is a CI job, so if either stops erroring
the hole is back.

Also fixes the add-card tests, which passed only because the availability
suite above them left mockIsAndroidNative true and clearAllMocks does not
reset return values. Run alone they took the unsupported-platform fallback
and never invoked addCard at all. Both platform answers are now set
explicitly, the native cases assert addCard was actually called, and a
third case covers the web fallback the platform gate added.

@chip-peanut-bot chip-peanut-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chip review — no blocking findings — this is not an approval

P1–P3 are fixed, but the exact head has one CI-breaking test error and two gaps in the new lint guard.

Findings

  • MINOR · src/utils/tests/native-capability.test.ts:115 · Make the negative type assertion compile
    The exact-head typecheck fails here: this directive applies to line 116, while TypeScript emits Expected 3 arguments, but got 2 on line 117, so it also reports the directive as unused. The same error aborts the design-system build and deploy preview. Keep the negative assertion, but put the invalid call on the line immediately after @ts-expect-error and pass all three parameters so the intended method-name type error is the one being suppressed.

  • MINOR · eslint.config.js:235 · Keep the plugin ban in the DS 10 override
    This restriction is added only to the base no-restricted-imports list. The later DS 10 allowlist block replaces that rule with RESTRICTED_IMPORT_PATHS, so a direct registerPlugin import in any listed production file (for example src/hooks/useLogin.tsx) passes ESLint and bypasses the single-door invariant. Carry REGISTER_PLUGIN_IMPORT_RESTRICTION into that override, or restructure the exemption so it removes only the useSearchParams restriction.

  • MINOR · eslint.config.js:51 · Point lint users to the current call signature
    The remediation still tells developers to use .call(invoke, onUnavailable), but this head replaced that callback API with .call(method, options, onUnavailable). Following the lint message now produces code that does not typecheck. Update the message to show the method-name form introduced by this commit.

Checked clean

  • Verified the detached worktree HEAD, merge base, trusted author, base ref, and base SHA against the supplied values.
  • P1 is fixed: callers select a typed method name and never receive the plugin proxy.
  • P2 is fixed: add-card tests reset both platform mocks and explicitly enable Android for native cases.
  • P3 is fixed: the web gate has direct coverage and asserts that the plugin method is not called.
  • Reviewed ClipboardDetect, InstallReferrer, and PushProvisioning fallback paths; the exact-head unit, ESLint, format, native-export, and analysis checks succeeded.
  • The typecheck, design-system build, deploy preview, and aggregate CI failures all trace to the same malformed negative type assertion reported above.
  • Checked the generic native dispatch boundary for caller-controlled method selection and new data exposure; current call sites use fixed method names and preserve their existing fallback behavior.

Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.

Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.

Exact head: b2f286ad8aeb · Context: repo · Took 11m

Comment thread src/utils/__tests__/native-capability.test.ts Outdated
Comment thread eslint.config.js Outdated
Comment thread eslint.config.js Outdated
Two mistakes in the guard I added for Chip's finding, both caught by CI.

@ts-expect-error binds to the next LINE, and prettier wraps a long argument
list — so the directive landed on `expect(() =>` while the error sat two
lines below, and typecheck reported the directive itself as unused.

Fixing that exposed the second: the assertion was executing. A two-argument
call leaves onUnavailable undefined, so the unsupported-platform branch
threw an unhandled rejection and took Node down with it. Nothing here
should run — the whole assertion is that it does not compile.

Both calls now sit in a function that is declared and never invoked, with
each directive directly above its call. TypeScript still checks the body,
so the invariant is still enforced by the typecheck job.

@chip-peanut-bot chip-peanut-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chip review — no blocking findings — this is not an approval

P1, P2, P3, and P6 are fixed. Three minor guard and guidance gaps remain: DS 10 files bypass the plugin ban, the lint message shows the old API, and platform support can be true without a live native bridge.

Findings

  • MINOR · eslint.config.js:235 · Keep the plugin ban in the DS 10 override
    The new restriction is present only in the base no-restricted-imports list. The later DS 10 allowlist replaces that rule with RESTRICTED_IMPORT_PATHS, so a direct registerPlugin import in any listed production file, such as src/hooks/useLogin.tsx, still passes ESLint and bypasses the single-door invariant. Carry REGISTER_PLUGIN_IMPORT_RESTRICTION into that override, or narrow the override to remove only the intended useSearchParams restriction.

  • MINOR · eslint.config.js:51 · Point lint users to the current call signature
    The remediation still tells developers to use .call(invoke, onUnavailable), but this head replaced that callback API with .call(method, options, onUnavailable). Following the lint message produces code that does not typecheck. Update the message to show the method-name form.

  • MINOR · src/utils/native-capability.ts:81 · Require a live bridge for platform support
    isIOSNative() and isAndroidNative() inherit getPlatform()'s build-flag and user-agent fallback, which deliberately classifies mobile Vercel previews as native even though no bridge exists. Opening such a preview on Android therefore makes isSupportedPlatform() return true and call() touch the web plugin proxy, contrary to this wrapper's off-native gate; a future UI consumer would also expose a native-only control there. Gate on isNativeBridge() plus the actual Capacitor platform (or bridge-specific helpers), and cover a native-flavoured preview with no bridge.

Checked clean

  • Verified the detached worktree HEAD, trusted author, supplied base ref and SHA, and exact merge base.
  • P1 is fixed: callers select a typed method name and never receive the Capacitor plugin proxy.
  • P2 and P6 are fixed: add-card tests reset both platform mocks, explicitly enable Android for native cases, assert the plugin is called, and cover the web fallback without a plugin call.
  • P3 is fixed: both negative type assertions are compile-only, each directive is directly above its invalid call, and exact-head typecheck succeeds.
  • Traced ClipboardDetect, InstallReferrer, and PushProvisioning through supported-platform, unsupported-platform, missing-plugin, and native-rejection paths; their result and fallback shapes are preserved.
  • Inspected the matching iOS and Android plugin implementations and confirmed the wrapper forwards the expected option objects and preserves the proxy receiver.
  • Exact-head aggregate CI, unit, native-export, typecheck, ESLint, format, analysis, design-system lint, and provenance checks succeeded; screenshot and deploy-preview checks were still in progress at the final read.
  • git diff --check passed, and no API or product contract changed.

Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.

Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.

Exact head: b4474d8e105b · Context: repo, ci · Took 8m

Comment thread eslint.config.js Outdated
Comment thread eslint.config.js Outdated
Comment thread src/utils/native-capability.ts
…ssage

Three findings, one of which meant the rule did not do what the PR claimed.

The DS 10 allowlist block REPLACES no-restricted-imports rather than
extending it, so the plugin ban only ever applied to files outside that
list. Every allowlisted production file — src/hooks/useLogin.tsx among them
— could import registerPlugin directly and walk straight around the
single-door invariant. Carried the restriction into that override and
verified by probing: the ban now fires in useLogin.tsx.

isIOSNative/isAndroidNative fall back to a user-agent sniff that
deliberately reports a capacitor-flavoured web build as native, so a Vercel
preview opened on an Android phone passed isSupportedPlatform() and reached
the web plugin proxy — the exact off-native call the gate exists to stop.
Gate on isNativeBridge() as well, which is what "will a native API really
work" already means elsewhere in capacitor.ts.

The lint message still told developers to write .call(invoke,
onUnavailable), which this branch replaced. Following it produced code that
does not typecheck.

Test mocks gain isNativeBridge, and a new case covers the native-looking
platform with no bridge.

@chip-peanut-bot chip-peanut-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chip review — no blocking findings — this is not an approval

P1-P8 are fixed at this head. The wrapper and migrated calls preserve their fallback behavior, but one existing ESLint override still leaves a narrow bypass around the new single-door rule.

Findings

  • MINOR · eslint.config.js:272 · Keep the plugin ban in the tailwind override
    The comment says native-capability is the only legal registerPlugin caller, but the preceding override still sets no-restricted-imports to off for src/utils/tw.ts and its test. A direct registerPlugin import in tw.ts therefore passes ESLint and bypasses the new single-door ratchet. Narrow that override to exempt only tailwind-merge while retaining REGISTER_PLUGIN_IMPORT_RESTRICTION, or enforce registerPlugin with a separate rule that the tailwind exception does not disable.

Checked clean

  • Verified the detached worktree HEAD, trusted author, supplied base ref and SHA, and exact merge base.
  • P1 is fixed: the method-name API never hands the Capacitor proxy to callers, so the callback return trap is no longer expressible.
  • P2 is fixed: add-card tests reset both platform mocks, enable Android explicitly for native cases, assert addCard is called, and cover the web fallback.
  • P3 is fixed: both invalid calls are compile-only with each @ts-expect-error directly above its call, and exact-head typecheck succeeds.
  • P4 and P6 are fixed at their cited DS 10 override: REGISTER_PLUGIN_IMPORT_RESTRICTION is carried into the replacement rule.
  • P5 and P7 are fixed: the lint guidance now shows .call(method, options, onUnavailable).
  • P8 is fixed: support requires isNativeBridge() as well as the declared platform, with a no-bridge regression test.
  • Traced ClipboardDetect, InstallReferrer, and PushProvisioning through supported-platform, unsupported-platform, missing-plugin, and native-rejection paths; their result and fallback shapes are preserved.
  • git diff --check passed. Exact-head aggregate, unit, native-export, typecheck, ESLint, format, analysis, design-system lint, provenance, and preview checks succeeded; ds-shots was still running at the final read.
  • Focused local lint execution was unavailable because the detached worktree has no installed ESLint binary; the override behavior is established directly by the later flat-config rule that sets no-restricted-imports to off.

Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.

Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.

Exact head: 0ec97817592f · Context: repo, ci · Took 7m

Comment thread eslint.config.js
… off

Chip found a second override disabling the plugin ban, after the DS 10 one:
the tailwind exemption sets no-restricted-imports to off for src/utils/tw.ts,
which switches off every restriction there, not just tailwind-merge. My own
native-capability.ts exemption had the same shape. Fixing them one at a time
would leave the next exemption free to reopen it silently.

Collect the restrictions in BASE_IMPORT_RESTRICTIONS and add
restrictedImportsExcept(...), so an override names the single restriction it
lifts and keeps the rest. A blanket 'off' no longer appears anywhere in the
config, and the barrel-import ban now applies in tw.ts and native-capability.ts
too, where it had been silently dropped.

Verified by probing a registerPlugin import into each previously-exempt path —
tw.ts, useLogin.tsx (DS 10 allowlist) and haptics.ts all reject it now, while
native-capability.ts still accepts it and tw.ts still imports tailwind-merge.

@chip-peanut-bot chip-peanut-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chip review — no blocking findings — this is not an approval

Two minor lint-enforcement holes remain. The runtime capability wrapper is sound at this head, all nine supplied prior findings no longer reproduce, and exact-head CI is green.

Findings

  • MINOR · eslint.config.js:49 · Cover the other registerPlugin entry points
    The restriction only bans the named registerPlugin import. Capacitor 8.2.0 also exposes the same registrar as Capacitor.registerPlugin, and no-restricted-imports does not inspect dynamic imports, so a new app-local plugin can use either route, pass lint, and omit the platform/fallback handling this PR is intended to make mandatory. Extend the guard to cover those supported access paths, ideally with a focused AST rule rather than relying only on an imported-name restriction.

Checked and not raised again

  • MINOR · eslint.config.js:235 · Keep the tailwind-merge ban in the DS 10 override — this review checked it and does not believe it. No task filed.

Checked clean

  • Confirmed the detached worktree HEAD and merge base exactly match the supplied head and base SHAs.
  • Checked nativeCapability platform gating, live-bridge gating, missing-plugin rejection, native rejection fallback, proxy receiver binding, and the method-name type invariant.
  • Checked ClipboardDetect, InstallReferrer, and PushProvisioning migrations against their native method signatures and prior fallback behavior.
  • Checked the exact-head registerPlugin census: production TypeScript now reaches the named export only from native-capability.ts.
  • Checked import-rule override composition and the current raw tailwind-merge importer census.
  • Exact-head aggregate CI, unit tests, typecheck, eslint, format, design-system checks, native export, analysis build, and preview deployment completed successfully.
  • Focused local execution was unavailable because the detached worktree has no installed dependencies; exact-head CI supplied the executable evidence.

Security review: did not run — this change has no security, privacy or money surface, so it was not asked. This review is one reviewer short.

Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.

Exact head: f4c259dfc16a · Context: repo · Took 14m

Comment thread eslint.config.js
no-restricted-imports only sees the named import. Capacitor 8 exposes the
same registrar as Capacitor.registerPlugin, and the rule cannot inspect a
dynamic import at all — so either route reached a raw plugin proxy with
none of the platform, bridge or fallback handling the wrapper exists to
make mandatory.

Added a no-restricted-syntax selector on the CALL, however the function was
obtained. Probed all three routes into an allowlisted file: named import,
Capacitor.registerPlugin and await import('@capacitor/core') are each
rejected now.

That rule had the same blanket-off problem the import rule did — three
overrides switched it off wholesale for one selector each — so
restrictedSyntaxExcept mirrors restrictedImportsExcept. No blanket 'off'
remains for either rule anywhere in the config, and useSafeBack, haptics
and useNativePlugins keep exactly the exemption each was written for.

@chip-peanut-bot chip-peanut-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chip review — no blocking findings — this is not an approval

The native capability wrapper and its three migrations are sound at this head, and prior findings P1-P10 are fixed. P11 remains: the DS 10 import override still lifts the tailwind-merge restriction.

Findings

  • MINOR · eslint.config.js:410 · Keep the tailwind-merge ban in the DS 10 override
    The DS 10 allowlist only needs to lift the useSearchParams restriction, but this override also removes TAILWIND_MERGE_IMPORT_RESTRICTION. As a result, every allowlisted file can import stock tailwind-merge without a lint error; combining DS token classes there can silently delete classes that the repository's tw wrapper understands. Remove TAILWIND_MERGE_IMPORT_RESTRICTION from the lifted list so this override exempts only useSearchParams.

Checked clean

  • Confirmed the detached worktree is exactly head 1d184ca and its merge base is the supplied dev base c5bbc4f.
  • Checked nativeCapability's platform and live-bridge gate, missing-plugin fallback, receiver binding, and method-name API that keeps the thenable proxy out of callbacks.
  • Checked ClipboardDetect, InstallReferrer, and PushProvisioning migrations against their existing fallback behavior and native platform implementations.
  • Checked the import and syntax restrictions across the base config and every override, including named, Capacitor.registerPlugin, and direct dynamic-import call shapes.
  • Exact-head unit, typecheck, eslint, format, native-export, analyze, report, and aggregate CI checks passed; ds-shots and the deploy preview were still running when checked.
  • A local synthetic ESLint probe was unavailable because the detached worktree has no installed dependencies; the surviving P11 exemption is directly established by restrictedImportsExcept filtering out both arguments at lines 408-410.

Security review: did not run — this change has no security, privacy or money surface, so it was not asked. This review is one reviewer short.

Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.

Exact head: 1d184ca6c92e · Context: repo · Took 6m

Comment thread eslint.config.js Outdated
The DS 10 allowlist needs useSearchParams lifted and nothing else. Its
pre-existing list was a bare RESTRICTED_IMPORT_PATHS, which also dropped the
tailwind-merge restriction by omission — invisible until the lifts became
explicit, and I preserved it verbatim rather than noticing it. Stock
tailwind-merge does not know the DS token groups and silently deletes DS
classes, so that exemption was never intended.

Verified free to tighten: none of the 45 allowlisted files imports
tailwind-merge directly, and both tw.ts and useLogin.tsx still lint clean.

@chip-peanut-bot chip-peanut-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chip review — no blocking findings — this is not an approval

The native-capability wrapper preserves the three migrated call paths while adding live-bridge, platform, missing-plugin, and rejection fallbacks. The supplied prior findings are fixed at this head, and no new actionable defect was found.

Checked clean

  • Confirmed the detached worktree HEAD and merge base match the supplied exact head and dev base SHAs.
  • Reviewed the full nine-file diff for correctness, failure paths, security, adversarial bypasses, and maintainability.
  • Verified ClipboardDetect, InstallReferrer, and PushProvisioning declarations match their native platform implementations and preserve their existing fallback contracts.
  • Verified nativeCapability keeps the plugin proxy private, requires a live bridge and declared platform, preserves the proxy receiver, and catches missing-plugin and native-call rejections.
  • Verified the registerPlugin import/call bans and scoped ESLint overrides retain the barrel, useSearchParams, tailwind-merge, and unrelated syntax restrictions.
  • Rechecked prior findings P1-P11 against the exact head; each is fixed by the current code.
  • Exact-head aggregate CI, unit tests, typecheck, ESLint, formatting, native export, design-system lint, analysis, and authorship checks completed successfully; visual snapshots and preview deployment were still in progress when checked.

Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.

Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.

Exact head: 8e5462b92907 · Context: repo · Took 9m

@innolope-dev
innolope-dev merged commit f7f9f82 into dev Sep 4, 2026
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant