fix(mobile,ci): restore the expo-modules-core pin the Android release guard enforces - #543
Merged
Merged
Conversation
… guard enforces PR #502 (React Doctor burn-down) dropped "expo-modules-core": "57.0.3" from apps/mobile/package.json as an unrelated drive-by. The release job installs the isolated mobile workspace with --package-lock=false, so a from-scratch resolve is free to pick any version satisfying expo 57.0.4's ~57.0.3 range; it took the newest, 57.0.5, and the pin guard failed the first release run since (29512207801). Restore the pin at 57.0.3, the version the root lockfile and the last green release (2026-07-09) both resolved. Also make the guard distinguish a deleted pin from a real drift. It read the expected version straight out of package.json, so a removed entry compared against undefined and reported "pinned to undefined" instead of naming the actual fault. A missing pin now fails as UNPINNED with a message that says the pin must be restored, not dropped from the guard. Verified by reproducing the release step locally: prepare-release-workspace plus the same lockfile-free install, then the guard extracted verbatim from the workflow YAML. All five modules resolve to the verified set and the step exits 0; deleting the pin again reproduces the UNPINNED failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sz19TStgQiJxZCZiE9tNsr
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #543
Scope: PR #543 in orbit-ui-mobile — fix(mobile,ci): restore the expo-modules-core pin the Android release guard enforces
Recommendation: APPROVE
Summary
This PR restores the expo-modules-core: 57.0.3 pin in apps/mobile/package.json (accidentally deleted as a drive-by change in PR #502), adds the matching package-lock.json entry, and hardens the Android release guard in .github/workflows/android-release.yml to fail loudly (UNPINNED) when a guarded module's pin is missing entirely, instead of silently misreporting it as a version MISMATCH against undefined. The root cause is correctly diagnosed and fixed at the source (the deleted pin), and the guard hardening is a genuine regression-proofing improvement, not a workaround.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
None (the new WHY-style comments in the embedded guard script match the file's existing comment idiom in this CI YAML — not a JS/TS source file the local/no-comments ESLint rule covers — so not worth flagging).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — confirmed via subagent: apps/web/package.json has no Expo/native-module equivalent, and the release guard is Android-release-only CI machinery; the PR's own note that no web mirror exists is correct (platform-adapter exemption). |
| i18n-syncer | N/A — no user-facing strings changed. |
| contract-aligner | N/A — no packages/shared/src/types/*, no orbit-api DTOs touched, single-repo diff. |
| security-reviewer | N/A — no orbit-api code changed. |
| design-reviewer | N/A — no apps/web, apps/mobile, or landing-page UI files changed. |
Validation
| Check | Result |
|---|---|
| Lint | N/A — this PR runs Build / Unit Tests / SonarCloud as separate required CI checks (skipped per workflow instructions) |
| Type check | N/A — same |
| Tests | N/A — same |
| Build (api) | N/A — orbit-api not touched, not checked out in this job |
Note: the PR author already reproduced the exact failing/passing guard behavior locally (documented in the PR body: prepare-release-workspace.js + npm install --package-lock=false --legacy-peer-deps + the guard extracted verbatim from the workflow YAML), showing expo-modules-core: resolved=57.0.3 pinned=57.0.3 passes and re-deleting the pin reproduces UNPINNED ... exit 1. That is credible, targeted verification for a CI-script-only change.
Deferred — N/A dimensions & files not verdicted
- SOLID/clean-arch, DESIGN.md/AI-slop, i18n, Contract drift, Security, Backend hard rules, FEATURES.md parity — all N/A, surface not touched by this diff (CI workflow + one dependency pin only).
- All three changed files (
android-release.yml,apps/mobile/package.json,package-lock.json) were reviewed in full and given a verdict — nothing deferred there. The lockfile'snode_modules/expo-modules-coreresolved-version entry (57.0.3) matches the restored pin, and no secondpackage-lock.jsonexists underapps/mobilethat would also need updating.
What's good
- Correct root-cause fix: restores the deleted pin rather than papering over the guard's blind spot.
- The guard hardening is a real improvement — it turns a future "deleted pin" mistake into an explicit, correctly-labeled
UNPINNEDCI failure instead of a confusingpinned=undefinedmismatch message, and it now reports both failure classes together instead of the first short-circuiting the second. - Verification was done by reproducing the actual failing CI step locally rather than assuming, and by re-deleting the pin to prove the new failure mode still has teeth.
- Correctly scoped: PR body explicitly and correctly notes the parity contract's platform-adapter exemption applies (no web mirror for a native Expo module pin), which the parity-checker subagent independently confirmed.
Recommendation
Merge as-is. No changes requested.
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 0 |
This was referenced Jul 16, 2026
thomasluizon
added a commit
that referenced
this pull request
Jul 16, 2026
… follow-up) (#545) Android Release 1.3.22 (81) failed R8 eleven minutes into the Gradle build: Missing class expo.modules.kotlin.types.ColorCompat (referenced from: expo.modules.ui.UtilsKt.colorToComposeColorOrNull) expo-router depends on @expo/ui at ^57.0.4 and never declares a dependency on expo-modules-core, so npm has no way to see that @expo/ui 57.0.5+ calls ColorCompat, which expo-modules-core only added in 57.0.4. The release job installs the isolated workspace with --package-lock=false, so that unpinned range resolved to the newest publish (57.0.6) against expo-modules-core pinned at 57.0.3 (#543) and produced a set that cannot link. Verified against the registry rather than inferred: @expo/ui 57.0.4 has no reference to ColorCompat, 57.0.5 and 57.0.6 do, and ColorCompat.kt first appears in expo-modules-core 57.0.4. @expo/ui is the symptom. Reproducing the release install locally showed 13 of 57 native modules resolving to versions the repo-root lockfile does not hold, plus 7 more unpinned and matching only by luck of the current registry state: the AAB was compiled from native code no local install or CI job ever built, and #543 was the same class of failure a day earlier. So pin every native module to the version the root lockfile resolves (exact versions in dependencies for direct deps, overrides for the transitive ones) rather than chase them one release at a time. Only declared ranges move; no resolved version, and no lockfile version or integrity hash, changes. Replace the hand-maintained five-module guard with verify-release-native-modules.js, which walks the installed release tree, selects every module carrying an android/ Gradle project, and asserts each matches the root lockfile. It subsumes the old guard (all five modules are native and still checked), needs no upkeep when a native dep is added, and names the offender in about a minute instead of failing R8 eleven minutes in. The lockfile stays the single source of truth, so a deliberate dependency update passes as soon as the root lockfile carries it. Verified by reproducing the release step: prepare-release-workspace plus the same lockfile-free install now resolves all 57 native modules to the lockfile set and the guard exits 0; run against the pre-fix tree it exits 1 and lists all 13 drifted modules, @expo/ui among them. Residual, deliberately not bundled: 87 JS-only packages still drift the same way (mostly build-time tooling such as @expo/cli and @expo/config-plugins). Closing that means installing the release workspace from a lockfile instead of resolving fresh, which is a change to the Play release path that deserves its own PR. Claude-Session: https://claude.ai/code/session_01DDueAWEMzgcryVDyG7yBq2 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thomasluizon
added a commit
that referenced
this pull request
Sep 18, 2026
…sRef prop The radio conversion replaced the browser's default focus ring on the web pickers, so a keyboard user lost the only cue that told them where they were. Each converted control now carries an explicit focus-visible outline in --primary. MotionPressable's focusRef prop is gone: the split focus plugin made it redundant, and its one caller now forwards an ordinary ref. The surface manifest is regenerated at this head. #543
thomasluizon
added a commit
that referenced
this pull request
Sep 18, 2026
* fix: add radio group keyboard navigation * fix(mobile): enable radio keyboard focus * fix(mobile): preserve React Native release flags * fix(mobile): preserve rendered radio order * fix(mobile): follow native radio focus * fix(mobile): type Android focus props * fix(mobile): remove invisible radio focus target * fix(mobile): preserve radio selection on focus entry * test(mobile): distinguish radio entry from movement * fix(web): avoid duplicate radio selection * fix(mobile): unify radio group navigation * fix(mobile): keep native radio entry focus * fix: align radio entry focus and move-parent parity * fix: preserve selection during radio entry redirect * fix: classify radio focus by provenance * test: assert time keyboard selection * Separate radio selection from the commit that closes a picker An arrow key called the destination row's registered handler, and for the four preference pickers that handler was selectAndClose, so the first directional key dismissed the sheet and committed the adjacent value. The roving tabIndex this pull request introduced had already removed the Tab route, so every option except the adjacent one became unreachable by keyboard. The timezone list was the worst case. The two actions are now separate at the group, not per call site. RadioGroup takes onCommit and the row hook returns onActivate. A directional move calls only the destination row's value handler and leaves the host open. onCommit runs on an explicit activation, which on web is the native button click that a pointer press, Enter and Space all raise, and on mobile is onPress. A row has no close-capable channel any more, so a fifth caller cannot reinherit the defect. Also in this change: - Delete fallbackFocusedHandleRef, a second focus tracker that the root layout's FocusProvenanceView already owns. - Scope the armed redirect to the focus event that armed it, so a redirect which never lands expires instead of swallowing a later key. - Delete apps/web/components/ui/radio-navigation.ts, a one line re-export with no production importer, and point its test at @orbit/shared/utils. - Delete apps/mobile/components/ui/selection-field.tsx, which had no production caller anywhere in apps/mobile. - Add apps/mobile/__tests__/support/focus-provenance.tsx so focus driving tests mount the real provider instead of the deleted fallback. Gates: - npx turbo run test --force passed 4 of 4 tasks: eslint-rules 392 tests, shared 2041, mobile 2674, web 2968. That run predates three later edits: nine unnecessary non null assertions removed from three mobile test files, one banned line comment removed from radio-row.tsx, and the test helper's tree parameter widened. - npx turbo run type-check --force passed 3 of 3 tasks on these exact bytes, exit 0. - The eight mobile radio suites passed 71 tests on these exact bytes, exit 0. - The pre-commit lint hook runs eslint over every changed file, so this commit exists only because that gate passed. The full npm run lint was not run separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Commit a preference only on a press, and leave D-pad traversal to Android Arrow keys inside the Timezone and Week-start pickers wrote one server mutation per keypress, because selection followed focus straight into onTimeZoneChange and onWeekStartChange. The pickers now hold a draft and persist it from onCommit, the shape time-field.tsx already used, so the value changes once and after the sheet has finished closing. Mobile radio groups no longer set nextFocusDown, nextFocusLeft, nextFocusRight or nextFocusUp. Those are Android focus overrides, so a per-group modulo wrap trapped every arrow key inside one time column and inside the segmented control. Every enabled option stays focusable and the platform's own focus search runs. Web keeps its roving tabIndex. A press on the row that focus already selected now commits without selecting again, so a D-pad centre press cannot write twice. The mobile-only index prop is gone: entry focus redirects to the checked option, and when none is checked it stays where the platform put it, so no caller has to count its own children. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Select an unchecked root row in the move-parent overlay test A press on a row that is already selected now commits without calling onSelect again, so the first MoveParentOverlay case could no longer observe onSelectOption. It rendered with selectedMoveParentId null, which makes the root row the checked one, and then clicked the root row. The case renders with the tree destination checked instead, so the root row is unchecked and the press that follows selects it. That is what the case is named for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cover the release template the config plugin actually patches The plugin test fed one fixture that already carried the legacy override block, so the pristine Expo template path, the one every CI and EAS build starts from because .gitignore:86 ignores apps/mobile/android/, was never executed. A second fixture now runs that path and asserts all five imports land after the anchor. Removing the Provider import reddens the new test and leaves the old one green, which is the gap it closes. The plugin used to drop the ReactNativeFeatureFlagsDefaults import unconditionally while removing its block only on an exact match, so a tree an older revision patched lost an import its surviving block still needed. The import now goes only once nothing constructs the class. Import detection matches a whole line, so the longer ReactNativeFeatureFlagsProvider no longer stands in for the shorter ReactNativeFeatureFlags. dangerouslyForceOverride returns the flags read before the call and its KDoc at ReactNativeFeatureFlags.kt:631-645 says to log it. The generated onCreate now keeps that value and warns when it is not null. The move-parent eyebrow and the mobile no-results text moved out of the radiogroup on both platforms. role=radiogroup owns radio children, so a text node inside it made TalkBack and NVDA count a fifth item. PickerOptions took its draft from a prop once and never resynced, so a cold start where the profile resolves after the first render left every row unchecked. An effect on the incoming value now moves the draft with it. The unused disabled prop is gone from both select-check RadioRows; no production caller passed it. The disabled-skipping coverage moved to the move-parent suites, where the list RadioRow carries a real disabled destination. The mobile focusable-target test now counts focusable hosts against options, and the dead onBlur call in the re-entry test is deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Give the relocated eyebrow the type role and gap a group label needs The design review of 398103a found two DESIGN.md breaks in the eyebrow this branch just moved above the move-parent group. DESIGN.md:720 defines the eyebrow role as Geist Mono 12/500 +0.08em uppercase on fg-3. The web copy used var(--font-sans), so the same string rendered in two families across platforms while mobile already used GeistMono_500Medium. It now uses var(--font-mono), which is what components/ui/section-label.tsx has always used for the same role. DESIGN.md:351 requires the gap between two groups to be at least twice the gap within one. The mobile eyebrow sat 4px above a group whose rows are 4px apart, so a label meant to head the whole list read as one more row. Its bottom margin is now 8. The web eyebrow also carried margin 2px 0 0 from its old position inside the group. The parent column already supplies an 8px gap, so the margin is gone and the gap is the only spacing mechanism. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Retire the selection-field surface and make the drift gate notice a crash The branch deletes apps/mobile/components/ui/selection-field.tsx, so the committed inventory still named a surface the tree no longer has and the new Surface Manifest Drift job went red on it. Regenerate the manifest, move m-overlay-ui-selection-field into the deleted section with the decision that removed it, and drop the MOBILE_OVERLAY_BASES entry that named the same file. With that entry gone the web and mobile base lists are the same value, so one OVERLAY_BASES serves both call sites instead of two aliases for one list. The determinism assertion the whole drift gate rests on passed on a dead process: the generator writes nothing on a non-zero exit, so a crashed second run left the first run's bytes on disk and the byte comparison agreed with itself. Both derivations now assert their exit code before the comparison. Also close what 1024's review left owed on these two files: de-duplicate trackedFiles, because git ls-files prints an unmerged path once per index stage; narrow the chat-order comment to the real condition, a character between 0x2F and 0x5C after the shared prefix; say that an untracked file is excluded for being untracked rather than for being ignored, because git ls-files reads the index; and record that the android build-tree exclusion now only guards a force-added artifact. The fixture wrapper moves to confirm-sheet. The order expected it to stay green, and it does not: with selection-field gone from the base list the fixture caller drops out of the inventory, and a fixture naming a file no tree has tests nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Give the time picker one widget contract and split the focus plugin The time picker announced itself two different ways. Web used listbox and option with aria-selected, while Android used radiogroup and radio. Web now uses the shared RadioGroup and useRadioGroupItem, so both platforms announce a radio group. The mobile option also set accessibilityState.selected next to checked; only checked makes a node checkable, so the extra key is gone. The absence guard on the mobile time column could not fail on the axis that broke. It asked whether a nextFocus handle belonged to the same column, so a handle pointing at the next column passed. It now asserts every nextFocus prop is undefined, on both columns. ReleaseLevel was missing from the config plugin import list, and only a hand-written fixture that supplied the import kept the generated Kotlin compiling. The plugin now adds the import itself, and a template without it proves that. withReactNativeImperativeFocus applies to debug builds too, so it moves out of the release-build plugin into plugins/with-react-native-imperative-focus.js with its own tests and registration. A mobile RadioGroup with no FocusProvenanceView ancestor now throws. Without the provider every focus reads a null previous target, which reads as entry, so the group would redirect focus on every move. The web language picker dropped a lookup that returned its own input. The prop is now SupportedLocale, narrowed once in usePreferenceControls, which is the shape the mobile twin already had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fall back to the standalone rule when a radio group has no focus provenance usePreviousFocusTarget() returns null outside FocusProvenanceView, and a null previous target reads the same as "focus arrived from outside the group". The group therefore armed a redirect to the checked row on every focus event, not only on entry. RadioGroup now publishes a null onFocus in that case, so useRadioGroupItem takes the standalone path and selects the focused option. A group under the provider keeps the entry redirect it needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Give every converted control a visible focus ring and retire the focusRef prop The radio conversion replaced the browser's default focus ring on the web pickers, so a keyboard user lost the only cue that told them where they were. Each converted control now carries an explicit focus-visible outline in --primary. MotionPressable's focusRef prop is gone: the split focus plugin made it redundant, and its one caller now forwards an ordinary ref. The surface manifest is regenerated at this head. #543 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
What broke
Android Release run 29512207801 failed at Guard pinned native module versions resolved correctly:
Root cause
pinned=undefinedwas the tell: the guard reads its expected versions out ofapps/mobile/package.json, and PR #502 (React Doctor burn-down) deleted"expo-modules-core": "57.0.3"fromdependenciesas an unrelated drive-by change.The release job installs the isolated mobile workspace with
--package-lock=false, so nothing constrains a from-scratch resolve except the ranges.expo@57.0.4requiresexpo-modules-core@~57.0.3, which permits 57.0.3 through 57.0.5, so npm took the newest (57.0.5). The deleted pin was the only thing holding the module at the verified 57.0.3.Timeline confirms it: last green release was 2026-07-09, the pin was deleted 2026-07-13, and this was the first release run since.
The fix
"expo-modules-core": "57.0.3"inapps/mobile/package.json(+ the matchingpackage-lock.jsonentry, sonpm cistays in sync). 57.0.3 is what the root lockfile resolves and what the last green release built against.undefinedand reportedpinned to undefined, which named the symptom rather than the fault. A missing pin now fails asUNPINNEDwith a message saying the pin must be restored, not dropped from the guard. Both failure classes are now reported together instead of the first one short-circuiting.Verification
Reproduced the release step locally rather than assuming:
prepare-release-workspace.js+ the identicalnpm install --package-lock=false --legacy-peer-deps, then ran the guard extracted verbatim from the workflow YAML:Re-deleting the pin reproduces the original failure with the new message (
UNPINNED expo-modules-core: resolved=57.0.3 pinned=<missing>, exit 1), confirming the guard still has teeth.Notes
npx expo install --checkstill reports the pinned SDK set as "outdated" (expo-router,expo-notifications, and others) — that is the pre-existing, intended state of a deliberately frozen SDK 57.0.4 set, unchanged by this PR, and the step is opt-in (default: false).🤖 Generated with Claude Code
https://claude.ai/code/session_01Sz19TStgQiJxZCZiE9tNsr