Repository navigation
chore: migrate mobile to NativeWind 5 + Tailwind 4 - #297
Merged
Merged
Conversation
Tailwind v4 is CSS-first: a theme is CSS custom properties in an `@theme` block, and there is no supported way to hand it a plain JS object (`@config` only loads a whole v3-shaped config file). So the tokens stay authored in TypeScript — still the single source of truth — and are *rendered* to a committed `theme.css` that both apps import. `theme-css.test.ts` is both the generator (`GENERATE_THEME_CSS=1`, via `pnpm --filter @whocards/tokens generate:theme`) and the drift guard: a token change that forgets to regenerate fails CI. The generated file is excluded from oxfmt so the formatter and the generator can't fight over it. Drops `tailwind-preset.ts` (a v3 `theme.extend` object) — nothing can consume it under Tailwind v4.
Dependabot's tailwindcss 3.4.19 -> 4.3.3 bump (#280) is green in CI but would have shipped an unstyled app: NativeWind 4.2.6 imports `tailwindcss/lib/cli`, `tailwindcss/lib/util/*`, `tailwindcss/loadConfig` and `tailwindcss/types/config` — all v3-only paths that v4 removed — and the Quality gate runs no Metro or native build, so nothing catches it. NativeWind v5 is the supported path onto Tailwind v4. - `tailwind.config.ts` is gone; `src/global.css` is the whole configuration (`@import`s, `@source`, and the generated `@whocards/tokens/theme.css`). - Metro: `withNativewind(config)` — no `input` option; global.css is imported as a normal module from `src/app/_layout.tsx`. The v5 resolver is what gives every RN primitive its `className` prop. - `nativewind`'s `colorScheme`/`useColorScheme` exports are gone in v5. The equivalent react-native-css API is wrapped in `src/lib/color-scheme.ts` so the seven components that read the resolved scheme in JS keep one import site. - `@expo/metro-config>lightningcss` is pinned to 1.30.1: react-native-css resolves lightningcss through @expo/metro-config, and >= 1.32 regressed the napi visitor bridge so any unresolved `var()` throws during `transform()` — which Tailwind v4's preflight and `@property` rules both emit. Without the pin every Metro build fails. Rationale is inline in pnpm-workspace.yaml. nativewind@5.0.0-preview.4 is a preview release; there is no stable v5 yet.
Under NativeWind v5 `className` never reaches the rendered element — the
component wrapper compiles it away into a `style` array — so the existing
`expect(node.props.className).toBe('text-white')` assertions could only ever
prove a string was passed in. They would keep passing with the whole CSS
pipeline dead. Snapshots would be no better: they assert "unchanged", not
"correct", and would happily bless an unstyled render.
Instead the assertions now read the resolved style back and compare it against
@whocards/tokens:
- `src/test-utils/resolved-style.ts` flattens a node's `style` prop, plus
`withOpacity()` (the 8-digit hex lightningcss serialises `/NN` to, f32 alpha
and all) and `rem()` (tokens compile to px at REM 14).
- `src/__tests__/tailwind-theme.test.tsx` walks the token → Tailwind → NativeWind
pipeline: every colour, spacing step, radius and font token renders its utility
class and must resolve to the token's value. Each table is checked for
completeness against the token object, so a new token fails the suite until it
is covered.
- question-text, secondary-button and theme-settings-page now assert token
values (colour, family, weight, size) where they asserted class strings.
- `jest.global-setup.js` compiles the real `src/global.css` through the same
PostCSS/Tailwind + react-native-css pipeline Metro uses, once per run, and
caches the stylesheet for jest.setup.ts to install.
Proven to fail closed, twice. Removing the `nativewind/babel` override from
babel.config.js (i.e. class resolution off): 4 suites red, 41 tests failed / 24
passed, with `Expected "#262432", Received undefined`. Changing `colors.white`
without regenerating theme.css (i.e. token binding broken): 6 tests red across 3
suites plus the tokens drift guard. Both restored, all green.
The website already had a Tailwind v4 `@theme` block that restated the design tokens by hand — a second copy of the same values, kept in sync by nobody. Now that packages/tokens renders those tokens to a `@theme` block of its own, the website imports it and keeps only what is genuinely website-only (the hero background image and the question grid-template row). Output is byte-identical: 172367 bytes before and after, with the same set of theme custom properties; only the order of the `:root` declarations moved. Closes the mobile half of Dependabot's tailwindcss 4.3.3 bump (#280), which is what this branch replaces.
… 30 / RNTL 14 Lands dependabot/#290 (minor-and-patch: react, react-native and friends) and dependabot/#279 (test tooling: jest, jest-expo, @testing-library/react-native, vitest) as one change — neither is green on its own. main already carries expo@~57.0.20 with every expo-* at ~57.x, but react-native@0.85.3 and jest-expo@~56.0.5. jest-expo 57's setup calls a non-virtual `jest.mock('@react-native/assets-registry/registry', ...)` without depending on that package itself, so under pnpm's strict node_modules only the react-native version bundled with SDK 57 exposes it. #279 alone (jest-expo 57 + RN 0.85) and #290 alone (RN bump + jest-expo 56) both fail the Quality gate with `Cannot find module '@react-native/assets-registry/registry'`. react-native lands on 0.86.3, not #290's 0.87.1: 0.86.3 is the version Expo SDK 57 pins in its bundledNativeModules.json (`npx expo install --check` no longer lists react-native at all), while 0.87 drops @react-native/assets-registry and is outside the SDK 57 matrix. reanimated 4.6.0 and worklets 0.12.1 both declare `react-native: "0.83 - 0.87"`, so the rest of #290 lands unchanged. RNTL 14 is a breaking release (render/renderHook/rerender/unmount/fireEvent/act are all async now, and the UNSAFE_* composite queries are gone), so the mobile test files move to the new API — see the PR body for the per-file rationale. nativewind, tailwindcss and react-native-css-interop are deliberately untouched (the NativeWind v5 / Tailwind v4 migration is its own branch). Supersedes #290 Supersedes #279
…ading stale calls Two fixes on top of the jest 30 / RNTL 14 migration. settings-modal.test.tsx outgrew jest's default 5s PER-TEST budget on GitHub's 4-vCPU runners: the file took 12.4s end-to-end there on main and 19.6s after the bump, enough for its first test (which also pays NativeWind's one-off style-registry init) to cross 5s and fail CI while running in 1.8s locally. Nothing hangs, it's wall-clock contention with the website's vitest run, so raise the budget with jest.setTimeout(20_000) in that one file rather than globally in jest.config.js -- a genuinely stuck test anywhere else should still fail fast on the 5s default. share-modal.test.tsx's expo-status-bar spy is module-level and nothing clears mocks between tests, so lastStatusBarStyle() read calls accumulated across the ten earlier <ShareModal> renders in the file: had the sheet stopped rendering a <StatusBar>, .at(-1) would have returned a stale call and both issue #173 tests would have gone green on the regression. Clear the spy per test and throw when it has no calls. Verified by deleting the <StatusBar> from share-modal.tsx: both tests now fail with '<StatusBar> was never rendered'.
✅ Deploy Preview for whocards-calmly canceled.
|
|
Warning Review limit reachedNext included review available in 30 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (50)
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 |
Review follow-ups on #297. Eight findings, no behaviour change beyond the line-height fix. - Line height: v5 parses a `leading-*` step as em, not rem. `text-sm leading-5` was 1.25rem/0.875rem = 1.4286em, which is already text-sm's own line height, so the class is dropped outright. `text-xl leading-8` was 2rem/1.25rem = 1.6em, which the integer-only `@utility leading-*` override cannot express — `leading-6` (1.5em = 26.25px) is the nearest step. Covered by new assertions on the resolved `lineHeight`, of which there were none anywhere before. - New `src/__tests__/color-scheme.test.ts`: the `?? 'dark'` fallback for a platform with no OS preference, and re-asserting a manual override after an OS appearance flip. - The per-suite `afterEach(() => setColorScheme('system'))` reset moves to `jest.setup.ts` beside the StyleCollection injection; five copies deleted. - Test-only class names no longer ship: `src/global.css` gains `@source not './__tests__/**'`, and jest compiles a `jest.global.css` wrapper that adds them back. Shipped stylesheet drops from 24,770 to 20,845 bytes (175 -> 147 selectors). - The lightningcss pin gets a removal condition, cross-referenced between `pnpm-workspace.yaml` and `apps/mobile/package.json`. - `theme-css.ts` records why `.oxfmtrc.json` ignores the generated `theme.css`. - `babel.config.js`'s account of the Metro resolver corrected: Metro rewrites broadly, Babel is the narrower tool. - `docs/design/163-light-mode/proposal.md` no longer points at `colorScheme.set()` or `tailwind.config.ts`, both deleted by this PR. Refs #297
Contributor
Author
|
Review follow-ups pushed in 7c7709c — all eight findings. Still a draft. Line height (the one behaviour change). v5 parses a
Three new assertions on the resolved The rest. New Left alone deliberately: the Gates: |
…0.86.3 Evidence that this upgrade did not change what a player sees, on the JS side. Two suites cover the three screens (Library, player, root layout) and the interactive surfaces (Settings sheet + its four pages, Share sheet, player bar): 19 snapshots in a pixel-only format — host component type, the FLATTENED resolved style one property per line, rendered text, and the accessibility props that have a presentational effect. Function props, refs, testIDs and every RN-internal prop are dropped, so a release that renames its internals doesn't churn the suite and train everyone to run `jest -u` without reading the diff. Both baselines were RECORDED ON origin/main (RN 0.85.3 / jest 29 / RNTL 13) in a scratch checkout and then replayed, untouched, here under RN 0.86.3 / jest 30 / RNTL 14. All 19 pass byte-for-byte; the only edit to either .snap is its first line, Jest's own file-format guide-link marker, which jest 30 refuses to read in jest 29's form. The test files avoid the RNTL 13/14 differences deliberately (awaited `render`/`fireEvent`, no `UNSAFE_*` queries) so the same file runs on both toolchains. What this cannot see: the native side. Yoga's layout maths, text shaping and shadow rasterisation are invisible to a JS-side render, so a clean run says the JS render is unchanged, not that the screen is. That still needs the Maestro pass on a device — hence `pnpm -F mobile ui-diff`, which captures the store screenshot flow on one ref and compares it frame-for-frame with another. Also here: `src/test-utils/ui-snapshot.ts` (the serializer, with `flattenStyle` as the single place a style gets resolved for tests) and `src/test-utils/reanimated-mock.ts` (a settled, synchronous reanimated, since the real package's Worklets native half can't initialize under jest-expo). Refs #295
The tagline's `leading-8` -> `leading-6` is the migration's one accepted
visual change, and nothing at the call site said so. Measured against
text-xl's 17.5px font size: v4's `leading-8` was 2rem = 28px; v5 gives
leading-6 = 26.25px, leading-7 = 30.63px, unmigrated leading-8 = 35px.
The exact equivalent is 1.6em = `leading-6.4`, unreachable two ways:
nativewind/theme.css's override takes `--value(integer)`, and an arbitrary
value does not help either. `leading-[...]` does fall through to Tailwind's
built-in utility and compiles — `.leading-\[1\.6\] { line-height: 1.6 }` is
in the CSS — but react-native-css accepts only a plain number for
lineHeight, so `leading-[1.6]`, `leading-[1.6em]`, `leading-[2rem]` and
`leading-[28px]` all resolve to no lineHeight at all. Verified by rendering
each one and reading back the resolved style.
`text-sm` at theme-settings-page.tsx is unaffected and still exactly 17.5px.
Also splits `renderClass` out of `lineHeightOf` so the default-line-height
case can assert font size and line height together — they are 17.5/24.5 at
text-xl, and asserting only the second invited the question of whether it
was measuring the first.
Refs #297
CodeRabbit raised four findings on the UI-snapshot work. One is a real foot-gun and is fixed; the other two (it reported the first twice) are declined, with the reasoning recorded where the next reader will meet it. ui-diff.sh interpolates a capture label into `rm -rf "$OUT_ROOT/$label"`, so a label like `../../x` deletes outside .ui-diff. No attacker is needed for that — a slip while pasting a ref name does it. Labels are now constrained to an opaque `[A-Za-z0-9][A-Za-z0-9._-]*` at both entry points, before any filesystem work. Valid labels behave exactly as before. Declined, with the reason now in the source: - Dropping `source` from the snapshot serializer is deliberate. A bundled source resolves to a numeric asset-registry id that depends on module registration order, so it churns on precisely the toolchain upgrades this suite exists to hold still. Asset identity is a separate question and git already answers it. - The reanimated mock runs completion callbacks before the caller commits the value, where real Reanimated commits first. Only a callback that reads the value it is being written into could tell, and none of the five call sites does — each only runOnJS's an unrelated setter. Deferring to a microtask would trade that for act()-flush timing. No snapshot was regenerated: the pre-upgrade baselines still replay 19/19 byte-for-byte. 32 suites / 237 tests, format and lint clean.
…interop, pin the Library container assertion Review follow-ups on #297: - play/[deck].tsx: the safe-area adapter's SafeAreaView ignores className, so the 'Deck not found.' container lost flex-1 + centering; use explicit styles like index.tsx. Its snapshot now records resolved styles instead of an inert className. - Remove react-native-css-interop (NativeWind 4's engine, nothing imports it). - ui-snapshot-screens: find the Library container by testID and assert the whole libraryContainerStyle object instead of the two values the finder searched for. - Drop the per-file colour-scheme afterEach now that jest.setup.ts resets globally. - proposal.md: the earlier useColorScheme() mention now points at useIsDark().
acharlop
marked this pull request as ready for review
September 11, 2026 21:13
Contributor
Author
Updated from main, two-axis review follow-ups in
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| /** Re-renders on every scheme change, whether from the OS or from Settings. */ | ||
| export const useColorScheme = () => useSyncExternalStore(subscribe, getColorScheme) |
There was a problem hiding this comment.
Expo web uses static output, whose server prerender reaches useColorScheme() through RootLayout. This call omits useSyncExternalStore’s required server snapshot getter, so React can reject the server render and fail the static web export. Pass an appropriate getter as the third argument.
Suggested change
| export const useColorScheme = () => useSyncExternalStore(subscribe, getColorScheme) | |
| export const useColorScheme = () => useSyncExternalStore(subscribe, getColorScheme, getColorScheme) |
Contributor
Author
There was a problem hiding this comment.
Valid: app.json declares web.output: "static" and the app ships react-native-web, so the static export would hit this. Fixed in e7fc96f by passing getColorScheme as the server snapshot (the store is synchronous and has a non-null value at module load, so the same getter is the right server value).
app.json declares web.output static; React rejects a server render of useSyncExternalStore without a getServerSnapshot. Greptile P1 on #297.
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
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.
Migrates mobile to NativeWind 5 preview.4 and Tailwind 4. Mobile and website consume the shared tokens through a generated, committed CSS theme; drift tests keep the CSS aligned with the TypeScript tokens.
Builds on #295 (merged): the previous React Native 0.85.3 / Expo 57 combination failed native compilation at ExpoModulesJSI
getStringData; #295 supplied the compatible React Native 0.86.3 toolchain. The branch has been updated from main, so the diff is now only the NativeWind 5 / Tailwind 4 migration. The tests preserve both RNTL 14 asynchronous behavior checks and NativeWind resolved-style assertions.Native screenshot comparison also exposed a collapsed home screen: the safe-area adapter reexports SafeAreaView without className support. Its layout now uses native StyleSheet values matching the previous spacing. The Jest mock preserves this limitation, with a regression assertion on the actual layout styles. Light, Dark, and System settings also have a native persistence flow.
The 19 render snapshots are now NativeWind 5 regression baselines with compiled CSS. They are not unchanged pre-upgrade snapshots; before/after visual evidence comes from native captures. The accepted home tagline line-height change remains 28px to 26.25px.
Validation on this Mac:
pnpm check: all 18 tasks passed.NativeWind remains a preview release; this PR does not claim Android build or visual verification.
The PR is not safe to merge until the new theme store supports Expo’s static web prerender path.
Summary
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR TS[TypeScript design tokens] --> GEN[theme-css renderer] GEN --> CSS[Committed theme.css] CSS --> WEB[Website Tailwind 4] CSS --> MOBILE[Mobile global.css] MOBILE --> POSTCSS[Tailwind 4 / PostCSS] POSTCSS --> NATIVE[NativeWind 5 / react-native-css] NATIVE --> UI[React Native component styles] APPEARANCE[OS Appearance] --> SCHEME[color-scheme external store] SETTINGS[Persisted theme setting] --> SCHEME SCHEME --> NATIVE SCHEME --> UIReviews (1) · Last reviewed commit: "fix(mobile): style the unknown-Deck safe..."