fix: isolate expo-task-manager/expo-background-task to /wallet/expo/background#487
fix: isolate expo-task-manager/expo-background-task to /wallet/expo/background#487pietro909 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThis PR moves Expo background helpers to ChangesExpo Background Task Module Split
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant ExpoWallet
participant BackgroundTask as expo-background-task
participant TaskManager as expo-task-manager
Consumer->>ExpoWallet: call setup({ background: { taskQueue, foregroundIntervalMs } })
ExpoWallet->>TaskManager: defineTask(taskName, executor) [module scope]
Consumer->>BackgroundTask: registerTaskAsync(taskName, { minimumInterval })
BackgroundTask->>TaskManager: trigger task executor event
TaskManager->>ExpoWallet: executor invokes taskRunner/processors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: #487 — isolate expo-task-manager/expo-background-task
Verdict: APPROVE ✅
Clean, well-motivated fix. The architectural split is sound and the implementation is tight.
What this PR does
Fixes #486 by moving expo-task-manager / expo-background-task from lazy require() (invisible to Metro's static dep collector) into a dedicated @arkade-os/sdk/wallet/expo/background subpath with static imports. The /wallet/expo entrypoint no longer touches those packages at all, so react-native-web and Node consumers aren't affected.
Breaking change: taskName and minimumBackgroundInterval removed from ExpoBackgroundConfig; OS-level registration is now the consumer's explicit responsibility.
Reviewed files
| File | Assessment |
|---|---|
package.json |
✅ New subpath export correctly wired (types, ESM, CJS, default) |
src/wallet/expo/background.ts |
✅ Static imports replace lazy require(); inline type declarations removed in favor of ambient .d.ts |
src/wallet/expo/expo-modules.d.ts |
✅ Ambient declarations cover the subset of APIs used; sits inside src/ so tsconfig.json "include": ["src"] picks it up |
src/wallet/expo/index.ts |
✅ Background re-exports removed — consumers must import from the new subpath |
src/wallet/expo/wallet.ts |
✅ import type from ./background — erased at compile time, no runtime dep chain. warnOnRemovedBackgroundFields catches JS callers still passing stale fields |
src/worker/expo/README.md |
✅ Migration table, updated examples, teardown example added |
test/wallet/expo/background.test.ts |
✅ vi.hoisted + vi.mock replaces brittle Module.prototype.require patching — correct for static imports |
test/wallet/expo/wallet.test.ts |
✅ Background mock removed (no longer imported by wallet.ts), warnOnRemovedBackgroundFields thoroughly tested including edge cases |
Key architectural check
The critical invariant — /wallet/expo must not transitively import expo-task-manager or expo-background-task at runtime — is preserved:
wallet.ts:34usesimport type { PersistedBackgroundConfig } from "./background"→ erased by TypeScript.index.tsre-exports only from./wallet, not from./background.- The PR description confirms a post-build grep of
dist/validates this.
Minor observations (non-blocking)
-
DefineBackgroundTaskOptions/PersistedBackgroundConfigno longer re-exported from/wallet/expo— they were previously inindex.ts. They're now only accessible via@arkade-os/sdk/wallet/expo/background. This is correct behavior (consumers of background types should import from the background subpath), but worth noting in changelog. -
registerExpoBackgroundTaskdefault interval math (background.ts:162):(options?.minimumInterval ?? 15) * 60converts minutes to seconds. Theexpo-background-taskAPI expects seconds, so this is correct. The JSDoc correctly documents the param as minutes. -
No cross-repo breakage: Searched all repos under
/root/arkana/repos/— zero consumers currently import from@arkade-os/sdk/wallet/expo. The primary downstream consumer (Trixie Wallet) is referenced in the PR description as already validated against the equivalent boltz-swap#146 change.
Protocol safety
This PR does not touch VTXO handling, transaction signing, forfeit paths, round lifecycle, connector trees, or exit paths. The background task handler reconstructs providers and calls runTasks + extendVtxo — but that logic is unchanged. The only changes are import/export topology and the consumer-facing registration API.
No protocol-critical review needed.
🤖 Reviewed by Arkana (opus)
pietro909
force-pushed
the
fix/expo-background-static-imports
branch
from
a5b2a90 to
81f7976
Compare
…wallet/expo/background Lazy require() in /wallet/expo/background.ts was invisible to Metro's static dependency collector, so expo-task-manager / expo-background-task never entered the bundle graph. Split helpers into a new @arkade-os/sdk/wallet/expo/background subpath using static imports; OS-task registration is now the consumer's responsibility. Fixes #486.
pietro909
force-pushed
the
fix/expo-background-static-imports
branch
from
81f7976 to
3a6d95b
Compare
1 participant
require()ofexpo-task-manager/expo-background-taskis invisible to Metro #486:defineExpoBackgroundTask(andregisterExpoBackgroundTask/unregisterExpoBackgroundTask) failed at JS startup because lazyrequire()in/wallet/expowas invisible to Metro's static dependency collector, soexpo-task-manager/expo-background-tasknever entered the bundle graph.@arkade-os/sdk/wallet/expo/backgroundsubpath that uses static imports./wallet/expoitself no longer references the optional Expo packages — react-native-web and Node consumers are unaffected.ExpoWallet.setup()/.dispose(). Consumers callregisterExpoBackgroundTask/unregisterExpoBackgroundTaskexplicitly.Testing
Direct port of arkade-os/boltz-swap#146, whose validated build pattern is in production in Trixie Wallet. Test changes:
test/wallet/expo/background.test.tsrewritten to mockexpo-task-manager/expo-background-taskviavi.hoisted+vi.mock— the previousModule.prototype.requirepatching does not intercept static imports.test/wallet/expo/wallet.test.tsdrops thetaskName/minimumBackgroundIntervalmocks (those fields are gone) and adds coverage for the newwarnOnRemovedBackgroundFieldsJS-caller guard.pnpm build: grep'ingdist/{esm,cjs}/wallet/expo/{index,wallet}.jsshows zero runtime imports ofexpo-task-manager/expo-background-task— only/wallet/expo/background.{js,cjs}pulls them in.Summary by CodeRabbit
New Features
Documentation
Breaking / API Changes
Tests