Skip to content

fix[notask]: extract nested node_modules packages in addons manifest …#1731

Merged
Victor-Rodzko merged 2 commits into
mainfrom
fix/cli-manifest-nested-addons
Apr 24, 2026
Merged

fix[notask]: extract nested node_modules packages in addons manifest …#1731
Victor-Rodzko merged 2 commits into
mainfrom
fix/cli-manifest-nested-addons

Conversation

@Victor-Rodzko

@Victor-Rodzko Victor-Rodzko commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Mobile apps crash at runtime with ADDON_NOT_FOUND for native addons (bare-abort, bare-os, bare-fs, etc.). The addons manifest always generates "addons": [], so bare-link never creates the required xcframeworks/shared libraries.

How does it solve it?

Bundle resolution keys contain nested paths like /node_modules/@qvac/sdk/node_modules/bare-abort/binding.js. The regex used .match() without /g, capturing only the first segment (@qvac/sdk) and missing all nested addon packages. Additionally, package.json lookup only checked root node_modules/, missing nested packages.

  • Use /g flag with matchAll() to capture all node_modules segments per resolution key
  • Look up package.json in both root and nested node_modules paths

Result: 34 native addons detected vs 0 before.

How was it tested?

  • 16 unit tests added covering regex extraction, package resolution, and extractPackedString/extractBarePackHeader helpers
  • Regression test explicitly verifies old regex misses nested packages while new regex captures them
  • End-to-end: ran manifest generation against a production bundle — confirmed 34 addons (vs 0 with old regex). Rebuilt iOS app, installed on iPhone 16e, app launches and initializes all native modules without ADDON_NOT_FOUND
Packages found Native addons
Before (old regex) 1 0
After (fixed regex) 150 34

Note: check (cli) e2e tests #40/#41 (embeddings) are a pre-existing failure — same tests fail on unrelated PRs (e.g. QVAC-12239).

@Victor-Rodzko Victor-Rodzko requested review from a team as code owners April 23, 2026 15:03
@Victor-Rodzko Victor-Rodzko force-pushed the fix/cli-manifest-nested-addons branch from e6c6b00 to 5f304c7 Compare April 23, 2026 15:39
@Victor-Rodzko Victor-Rodzko force-pushed the fix/cli-manifest-nested-addons branch from 5f304c7 to e1c87e4 Compare April 23, 2026 17:19

@simon-iribarren simon-iribarren 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.

Approving.

Clear root-cause + fix:

  • .match() without /g on /node_modules/([^/]+)/-style paths only captures the first segment, so nested packages (/node_modules/@qvac/sdk/node_modules/bare-abort/...) were silently dropped. Switch to matchAll() + /g is the right move.
  • package.json lookup extended to nested node_modules paths so the resolver can actually find the addon manifests it needs.
  • 0 → 34 addons detected is a strong regression-class signal; the explicit regression test that asserts the old regex misses nested packages is exactly the shape of test I want to see for bugs like this.

CI: confirmed check (cli) failures on tests #40/#41 (embeddings: single input returns vector / batch input returns multiple vectors) are pre-existing — same two tests fail on unrelated PRs (e.g. #1681 at tests #42/#43; difference is just numbering due to skip counts). Not a blocker for this PR.

@github-actions

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Tier-based Approval Status

**PR Tier:** TIER1

**Current Status:** ✅ APPROVED

**Requirements:**
- 1 Team Member approval ✅ (1/1)
- 1 Team Lead OR Management approval ✅ (1/1)



---
*This comment is automatically updated when reviews change.*

@Victor-Rodzko

Copy link
Copy Markdown
Contributor Author

/review

@Victor-Rodzko Victor-Rodzko merged commit 193be15 into main Apr 24, 2026
12 of 13 checks passed
@Victor-Rodzko Victor-Rodzko deleted the fix/cli-manifest-nested-addons branch April 24, 2026 16:10
Proletter pushed a commit that referenced this pull request May 24, 2026
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.

3 participants