fix: align mobile to Expo SDK 57, repair the rotting override pins, and de-flake the suite - #715
Merged
Merged
Conversation
Dependabot has five open high alerts, all npm transitives reached only through package-lock.json. Three have a published patch and are fixed here with the overrides block the repo already uses for exactly this (brace-expansion@1, and js-yaml@4 which this raises rather than adds). js-yaml@4 4.3.0 -> 4.3.1 GHSA-5p4m-2wfm-xmqj (eslintrc, xcpretty, orval) js-yaml@3 3.15.0 -> 3.15.1 GHSA-5p4m-2wfm-xmqj (@lhci/utils, dev) nanoid@3 3.3.16 -> 3.3.17 GHSA-2v37-7h3g-55p8 (expo-router, postcss) Every dependent's declared range already admits the patched version, so no dependent needed relaxing. npm reported the installed copies "invalid" against the new overrides but would not re-resolve them from the existing lock, so the lockfile was regenerated with `npm update js-yaml nanoid`. The remaining two alerts are image-size (GHSA-w3rx-r6r6-pgpr, GHSA-5p2g-fcmc-qvqq) and are NOT fixed, because no fixed release exists: the advisory range is <= 2.0.2 and 2.0.2 is the latest published version. npm's only proposed remedy is a major downgrade to expo 53 / react-native 0.72, which is a worse outcome than the bug. It is unreachable in production regardless: image-size is pulled by metro and runs in the bundler at build time, never shipped into the app. Validation: type-check 3/3, and 670 test files pass across all four workspaces. The full suite must be run with --concurrency=1; at turbo's default concurrency the shared barrel-import test at types.test.ts:1943 exceeds its 15s timeout purely from machine contention, unrelated to this change (shared imports neither package). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Monthly non-security freshness sweep. /dep-sweep and the reminder workflow that files these tickets were both deleted by #676, so this follows the contract stated in the ticket body instead: bump web and shared minor/patch, defer mobile to expo install --check, report the orbit-api NuGet drift without changing it. package-lock.json only. Every bump was already inside an existing semver range, so no package.json edit was needed. 263 packages moved, including next 16.2.11 -> 16.3.0, @sentry/* 10.68.0 -> 10.70.0, @supabase/supabase-js 2.110.8 -> 2.112.3, react-hook-form 7.82 -> 7.85, next-intl 4.13.4 -> 4.13.6, axios 1.18.1 -> 1.19.0, and 23 @radix-ui patch bumps. #599 recorded that regenerating this lockfile refloats transitives, so the pins that had to hold were checked against the base commit rather than by eye. Every exactly-pinned root override is identical: the Expo SDK 57 native module set did not move, orval and @orval/zod held at 8.20.0 so the committed Zod snapshot stays comparable, and react, react-dom and react-test-renderer held. The only override-pinned package that moved is postcss 8.5.23 -> 8.5.26, whose override is the caret range ^8.5.18, so that is the pin working as designed. Two things npm did that the invocation did not ask for. A hoisted lockfile ignores --workspace scoping, so Expo build tooling moved with the sweep (@expo/cli 57.0.10 -> 57.0.14, @react-native/* 0.86.0 -> 0.86.2); all in-range, all still SDK 57, none in the pinned native set. And brace-expansion moved 1.1.18 -> 1.1.16, a downgrade, because the lockfile had drifted above its own exact security pin and re-resolution pulled it back. Validation: lint 3/3 with 0 errors, type-check 3/3, and 670 test files pass across all four workspaces. The suite needs --concurrency=1; at turbo's default the shared barrel-import test at types.test.ts:1943 exceeds its 15s timeout from machine contention alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the residual risk flagged on the August sweep, plus two override defects found while fixing it. expo install --check now reports "Dependencies are up to date". It previously wanted 26 packages moved, including react-native 0.86.0 -> 0.86.2 while six of its own tooling packages had already floated to 0.86.2, leaving core and tooling on different versions across the Android build. The 30 exact Expo pins in the root overrides block are deleted rather than refreshed. They existed to stop transitives refloating, but they are hand-maintained and silently rot, which is what produced the skew above. Removing them and measuring instead shows the tree dedupes on its own: duplicate expo/react-native module copies went from 1 to 0. The single remaining duplicate was react-native-device-info, hoisted 10.3.0 against react-native-siren's nested 8.7.1. Two copies of one native module cannot both autolink, so siren was running 8.x JS against 10.x native. It is now the one Expo-adjacent override that survives, because it is the only one carrying its weight. overrides: 45 entries -> 16 brace-expansion@1 was pinned to exactly 1.1.16, which is no longer a patched version. Two advisories now cover it, GHSA-mh99-v99m-4gvg (<1.1.17) and GHSA-rgw5-rvv9-x895 (<1.1.18, which bypasses the first mitigation). The tree had drifted to the safe 1.1.18 and the exact pin was dragging it back into both. Every security override is now a caret range so a pin cannot rot into the vulnerability it was added to fix. apps/mobile/package.json carried a 21-entry overrides block that npm never applied, since overrides are honoured only from the root workspace. #599 hoisted 7 of these; the block is now deleted outright. Not taken, each verified rather than assumed: eslint 10 eslint-config-next advertises eslint >=9.0.0, but its transitive eslint-plugin-react is at its last release (7.37.5), peers capped at ^9.7, and it throws "contextOrFilename.getFilename is not a function" on eslint 10. The brace-expansion override is the real fix regardless; eslint 10 was only npm's route to it. typescript 7 typescript-eslint@8.67.0 peers typescript <6.1.0. react 19.2.8 expo install --check rejects it and wants 19.2.3. Validation: lint 3/3 with 0 errors, type-check 3/3, 670 test files pass, and npm ci accepts the lockfile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…once
`turbo run test` runs the four workspace suites concurrently and each
vitest sizes its worker pool to the whole machine, so they oversubscribe
the CPU. Tests that re-import a module graph then blow their timeout for
no reason connected to the code under test.
Reproduced before fixing: five consecutive default-concurrency runs, red
every time, and the failure moved between workspaces rather than sitting
on one test.
shared types.test.ts barrel import, 15087ms against a 15s limit while
its sibling barrels finish in 224ms to 490ms
mobile use-push-notifications-state beforeEach, "Hook timed out in
10000ms", on a vi.resetModules() that rebuilds the graph per test
web i18n/request and lib/server-fetch, the latter being the
intermittent failure already filed as orbit-tickets #287
All four workspaces pass standalone, which is what identifies contention
rather than a product defect as the cause.
testTimeout and hookTimeout go to 30s in all three vitest configs. The
inline per-test timeouts are deleted, because that is what made the first
attempt at this fix fail: `it(..., 10000)` silently overrides the config,
so raising the config alone left web red. Those magic numbers were an
earlier pass at this same flake and each one resets the same clock as the
suite grows.
Verified with two further full default-concurrency runs, 4/4 tasks and
670 test files green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8df0183517
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses the Codex review on #715, which is right about the cause: 30s timeouts left four machine-sized worker pools oversubscribing the CPU and merely waited longer, while tripling how long a genuine hang takes to surface. maxWorkers is now '25%' in all three configs, so four concurrent workspace suites add up to about one machine's worth of workers instead of four. Confirmed against the installed vitest types rather than assumed: maxWorkers?: number | string, "Maximum number or percentage of workers". The cap alone is not sufficient, which is why the timeouts do not go all the way back to the defaults. Measured at default turbo concurrency: no fix 5 of 5 runs red 30s timeouts, no cap 2 of 2 green cap, default 5s timeouts 1 of 3 red, at 5000ms cap plus 15s margin 4 of 4 green 15s rather than the previous 30s, so a real hang reports in half the time while the residual cold-import cost still fits. One failure in the cap-only run was an AssertionError, not a timeout: server-fetch expecting forceRefresh: true on the second call. That is the pre-existing intermittent failure filed as orbit-tickets #287. It appears to be mock state cascading from the timed-out test above it in the same file, and it has not recurred in the four green runs, but this change does not claim to fix #287 because the cascade is unproven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79da6e29b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses the second Codex review on #715. The cap landed on mobile, web and shared but not eslint-rules, which kept vitest's default machine-sized pool and added workers on top of the three capped ones. That also made the comment wrong where it claimed four suites were capped. Its two test files make the practical pool small, but the config is what sets the ceiling, and on a 4-core CI runner the uncapped fourth is exactly where the aggregate goes back over one machine's worth. Verified with two more full default-concurrency runs, 4/4 tasks and 670 test files green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Resolves the squash-merge artifact from #713 and #714 landing on main. package.json: the overrides block conflicted because #713 was squashed, so git could not see that this branch already contains its exact pins. This branch's version is kept: it is #713's pins converted to caret ranges, which is the point of this PR, plus the react-native-device-info dedupe. No new resolution was invented. package-lock.json: regenerated with npm install rather than hand-merged. Verified after regeneration: js-yaml 4.3.1/3.15.1, nanoid 3.3.18, brace-expansion 1.1.18 everywhere, react-native-device-info a single copy at 10.3.0, and zero duplicate expo/react-native module copies. 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.
Closes the three follow-ups flagged on #714, and fixes two override defects found while doing it.
1. Mobile is aligned to Expo SDK 57
npx expo install --checknow reports "Dependencies are up to date". It previously wanted 26 packages moved.The specific risk called out on #714 is gone:
react-nativewas pinned at 0.86.0 while six of its own tooling packages (@react-native/codegen,babel-plugin-codegen,debugger-frontend,debugger-shell,dev-middleware,normalize-colors) had already floated to 0.86.2, so core and tooling disagreed across the Android build. Both sides are now 0.86.2, which is what Expo expects.2. The 30 Expo override pins are deleted, not refreshed
They existed to stop transitives refloating, but they are hand-maintained and rot silently, which is exactly what produced the skew above. Rather than re-pin 30 versions that will drift again, they are removed and the invariant is measured instead:
Duplicate
expo/react-nativemodule copies: 1 before, 0 after.The single real duplicate was
react-native-device-info, hoisted at 10.3.0 againstreact-native-siren's nested 8.7.1. Two copies of one native module cannot both autolink, so siren was running 8.x JS against 10.x native. It is now the one Expo-adjacent override that survives, because it is the only one carrying weight.3.
brace-expansionwas pinned into two live CVEsThis is the item #714 recorded as "cost of exact pinning". It is worse than that: the pin was reintroducing vulnerabilities.
"brace-expansion@1": "1.1.16"is no longer a patched version. Two advisories now cover it:<1.1.17<1.1.18(bypasses the first mitigation)The tree had drifted up to the safe 1.1.18 and the exact pin was dragging it back down into both. Now
^1.1.18.Every security override is now a caret range (
js-yaml@3,js-yaml@4,nanoid@3too) so a pin cannot rot into the vulnerability it was added to fix.nanoidimmediately picked up 3.3.18 as a result.npm auditroot advisories:brace-expansion+image-sizebefore,image-sizealone after (the dismissed, unfixable one).4. The dead
apps/mobileoverrides block is deleted21 entries npm never applied, since overrides are honoured only from the root workspace. #599 hoisted 7; the rest are gone, with the one load-bearing entry promoted to root as described above.
5. The suite no longer flakes at default concurrency
turbo run testruns four workspace suites concurrently and each vitest sizes its worker pool to the whole machine, so they oversubscribe the CPU and tests that re-import a module graph blow their timeouts.Reproduced first: five consecutive default-concurrency runs, red every time, with the failure moving between workspaces rather than sitting on one test.
types.test.tsbarrel import, 15087ms against a 15s limit, while sibling barrels finish in 224-490msuse-push-notifications-statebeforeEach, "Hook timed out in 10000ms", on avi.resetModules()that rebuilds the graph per testi18n/requestandlib/server-fetch, the latter already filed as orbit-tickets #287All four pass standalone, which is what identifies contention rather than a product defect.
testTimeoutandhookTimeoutgo to 30s in all three configs, and the inline per-test timeouts are deleted. That second half matters:it(..., 10000)silently overrides the config, so my first attempt at this fix left web red. Those magic numbers were an earlier pass at this same flake, and each one just resets the same clock as the suite grows.Verified with two further full default-concurrency runs: 4/4 tasks, 670 test files green.
Majors considered and rejected, each verified rather than assumed
eslint-config-nextadvertiseseslint: ">=9.0.0", but its transitiveeslint-plugin-reactis at its final release (7.37.5) with peers capped at^9.7and throwscontextOrFilename.getFilename is not a functionon eslint 10. Tried it, it broke web's lint, reverted. Costs nothing: thebrace-expansionoverride is the real fix, eslint 10 was only npm's route to it.typescript-eslint@8.67.0peerstypescript >=4.8.4 <6.1.0.expo install --checkrejects it and wants exactly 19.2.3.Validation
npm run lint3/3, 0 errorsnpm run type-check3/3npm run test4/4, 670 test files, twice at default concurrencynpm ciaccepts the lockfileStill unexercised: the Android native build. That is the one thing here a green JS suite cannot speak for, and it is worth a build before merge given the
react-nativeandreact-native-device-infomoves.🤖 Generated with Claude Code