Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThe mobile app now supports persisted 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e59fdcad-5aee-4f2e-9a7f-11e9c08cee62
📥 Commits
Reviewing files that changed from the base of the PR and between 17959a2 and ad6dcecba768a1b8e1ba15ec371416acfb9f56d3.
📒 Files selected for processing (37)
mobile/app/_layout.tsxmobile/app/h/_layout.tsxmobile/src/components/NewWorkspaceFab.tsxmobile/src/session/TerminalPaneView.test.tsmobile/src/session/TerminalPaneView.tsxmobile/src/storage/preferences.test.tsmobile/src/storage/preferences.tsmobile/src/storage/terminal-theme-preference.test.tsmobile/src/storage/terminal-theme-preference.tsmobile/src/terminal/TerminalWebView.test.tsmobile/src/terminal/TerminalWebView.tsxmobile/src/terminal/terminal-theme-slot-selection.test.tsmobile/src/terminal/terminal-theme-slot-selection.tsmobile/src/terminal/terminal-webview-html.tsmobile/src/terminal/use-mobile-terminal-theme.test.tsmobile/src/terminal/use-mobile-terminal-theme.tsmobile/src/theme/app-shell-theme.test.tsmobile/src/theme/mobile-theme-palette.test.tsmobile/src/theme/mobile-theme.tsmobile/src/theme/theme-context.test.tsmobile/src/theme/theme-context.tsxmobile/src/theme/themed-style-factories.test.tsmobile/src/theme/unthemed-color-imports.test.tssrc/renderer/src/lib/terminal-contrast-correction.test.tssrc/renderer/src/lib/terminal-theme.tssrc/renderer/src/lib/terminal-themes-data.tssrc/renderer/src/lib/terminal-themes/types.tssrc/shared/terminal-themes/catalog-merge.tssrc/shared/terminal-themes/classic.tssrc/shared/terminal-themes/defaults.tssrc/shared/terminal-themes/index.tssrc/shared/terminal-themes/popular-dark-core.tssrc/shared/terminal-themes/popular-dark-extended.tssrc/shared/terminal-themes/popular-dark.tssrc/shared/terminal-themes/popular-light.tssrc/shared/terminal-themes/terminal-theme-catalog.test.tssrc/shared/terminal-themes/types.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/lib/terminal-themes/types.ts
- src/renderer/src/lib/terminal-themes-data.ts
ad6dcec to
b595052
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mobile/src/theme/mobile-theme-palette.test.ts (1)
15-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared WCAG contrast utilities.
The mobile and renderer terminal contrast logic repeat the same sRGB linearization/relative-luminance and contrast-ratio formula. If these continue to be test utility helpers, a small shared contrast module can keep the implementations in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f37993d-9e82-4158-adcd-b7559f9b49a9
📥 Commits
Reviewing files that changed from the base of the PR and between ad6dcecba768a1b8e1ba15ec371416acfb9f56d3 and b595052d41b63af2243fe06dc3db96d7c352277d.
📒 Files selected for processing (37)
mobile/app/_layout.tsxmobile/app/h/_layout.tsxmobile/src/components/NewWorkspaceFab.tsxmobile/src/session/TerminalPaneView.test.tsmobile/src/session/TerminalPaneView.tsxmobile/src/storage/preferences.test.tsmobile/src/storage/preferences.tsmobile/src/storage/terminal-theme-preference.test.tsmobile/src/storage/terminal-theme-preference.tsmobile/src/terminal/TerminalWebView.test.tsmobile/src/terminal/TerminalWebView.tsxmobile/src/terminal/terminal-theme-slot-selection.test.tsmobile/src/terminal/terminal-theme-slot-selection.tsmobile/src/terminal/terminal-webview-html.tsmobile/src/terminal/use-mobile-terminal-theme.test.tsmobile/src/terminal/use-mobile-terminal-theme.tsmobile/src/theme/app-shell-theme.test.tsmobile/src/theme/mobile-theme-palette.test.tsmobile/src/theme/mobile-theme.tsmobile/src/theme/theme-context.test.tsmobile/src/theme/theme-context.tsxmobile/src/theme/themed-style-factories.test.tsmobile/src/theme/unthemed-color-imports.test.tssrc/renderer/src/lib/terminal-contrast-correction.test.tssrc/renderer/src/lib/terminal-theme.tssrc/renderer/src/lib/terminal-themes-data.tssrc/renderer/src/lib/terminal-themes/types.tssrc/shared/terminal-themes/catalog-merge.tssrc/shared/terminal-themes/classic.tssrc/shared/terminal-themes/defaults.tssrc/shared/terminal-themes/index.tssrc/shared/terminal-themes/popular-dark-core.tssrc/shared/terminal-themes/popular-dark-extended.tssrc/shared/terminal-themes/popular-dark.tssrc/shared/terminal-themes/popular-light.tssrc/shared/terminal-themes/terminal-theme-catalog.test.tssrc/shared/terminal-themes/types.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/lib/terminal-themes-data.ts
- src/renderer/src/lib/terminal-themes/types.ts
🚧 Files skipped from review as they are similar to previous changes (30)
- src/shared/terminal-themes/types.ts
- src/shared/terminal-themes/classic.ts
- mobile/src/terminal/terminal-webview-html.ts
- mobile/src/components/NewWorkspaceFab.tsx
- src/shared/terminal-themes/defaults.ts
- src/shared/terminal-themes/catalog-merge.ts
- src/shared/terminal-themes/popular-dark.ts
- mobile/src/theme/theme-context.test.ts
- mobile/src/terminal/use-mobile-terminal-theme.ts
- mobile/src/terminal/terminal-theme-slot-selection.ts
- src/shared/terminal-themes/popular-dark-core.ts
- src/shared/terminal-themes/popular-dark-extended.ts
- mobile/src/storage/preferences.test.ts
- mobile/src/session/TerminalPaneView.tsx
- src/shared/terminal-themes/popular-light.ts
- src/shared/terminal-themes/index.ts
- mobile/src/storage/preferences.ts
- mobile/src/theme/themed-style-factories.test.ts
- src/shared/terminal-themes/terminal-theme-catalog.test.ts
- mobile/src/terminal/TerminalWebView.tsx
- mobile/src/storage/terminal-theme-preference.test.ts
- mobile/src/terminal/terminal-theme-slot-selection.test.ts
- mobile/src/terminal/TerminalWebView.test.ts
- src/renderer/src/lib/terminal-contrast-correction.test.ts
- mobile/app/_layout.tsx
- mobile/src/terminal/use-mobile-terminal-theme.test.ts
- src/renderer/src/lib/terminal-theme.ts
- mobile/src/theme/theme-context.tsx
- mobile/src/storage/terminal-theme-preference.ts
- mobile/app/h/_layout.tsx
b595052 to
9e35852
Compare
|
Rebuilt onto current Stack base now includes:
Reviewable surface remains the theme runtime commits (provider / shells / defer mode-mismatch). CodeRabbit:
Verified: theme-context/app-shell/palette/factories/unthemed + preferences/slots/use-theme (80) + catalog (5); oxlint clean. |
9e35852 to
2e58516
Compare
2e58516 to
076c65e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mobile/src/theme/theme-context.test.ts (1)
23-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated console-suppression helper into a shared test utility. Both files define an identical
suppressReactTestRendererDeprecationWarningfunction; as morereact-test-renderer-based mobile tests are added in this stack, this duplication will keep growing.
mobile/src/theme/theme-context.test.ts#L23-L33: move this implementation into a new module (e.g.mobile/src/test-support/suppress-react-test-renderer-deprecation-warning.ts) and import it here.mobile/src/session/TerminalPaneView.test.ts#L29-L39: remove the duplicate and import the same shared implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fbfedc29-c6b4-4331-9d61-50aeba6e5cab
📥 Commits
Reviewing files that changed from the base of the PR and between b595052d41b63af2243fe06dc3db96d7c352277d and 076c65e.
📒 Files selected for processing (38)
mobile/app/_layout.tsxmobile/app/h/_layout.tsxmobile/src/components/NewWorkspaceFab.tsxmobile/src/session/TerminalPaneView.test.tsmobile/src/session/TerminalPaneView.tsxmobile/src/storage/preferences.test.tsmobile/src/storage/preferences.tsmobile/src/storage/terminal-theme-preference.test.tsmobile/src/storage/terminal-theme-preference.tsmobile/src/terminal/TerminalWebView.test.tsmobile/src/terminal/TerminalWebView.tsxmobile/src/terminal/terminal-theme-slot-selection.test.tsmobile/src/terminal/terminal-theme-slot-selection.tsmobile/src/terminal/terminal-webview-html.tsmobile/src/terminal/use-mobile-terminal-theme.test.tsmobile/src/terminal/use-mobile-terminal-theme.tsmobile/src/theme/app-shell-theme.test.tsmobile/src/theme/mobile-theme-palette.test.tsmobile/src/theme/mobile-theme.tsmobile/src/theme/theme-context.test.tsmobile/src/theme/theme-context.tsxmobile/src/theme/themed-style-factories.test.tsmobile/src/theme/unthemed-color-imports.test.tssrc/renderer/src/lib/terminal-contrast-correction.test.tssrc/renderer/src/lib/terminal-theme.tssrc/renderer/src/lib/terminal-themes-data.tssrc/renderer/src/lib/terminal-themes/index.tssrc/renderer/src/lib/terminal-themes/types.tssrc/shared/terminal-themes/catalog-merge.tssrc/shared/terminal-themes/classic.tssrc/shared/terminal-themes/defaults.tssrc/shared/terminal-themes/index.tssrc/shared/terminal-themes/popular-dark-core.tssrc/shared/terminal-themes/popular-dark-extended.tssrc/shared/terminal-themes/popular-dark.tssrc/shared/terminal-themes/popular-light.tssrc/shared/terminal-themes/terminal-theme-catalog.test.tssrc/shared/terminal-themes/types.ts
💤 Files with no reviewable changes (3)
- src/renderer/src/lib/terminal-themes-data.ts
- src/renderer/src/lib/terminal-themes/index.ts
- src/renderer/src/lib/terminal-themes/types.ts
🚧 Files skipped from review as they are similar to previous changes (30)
- mobile/src/components/NewWorkspaceFab.tsx
- src/shared/terminal-themes/popular-dark.ts
- src/shared/terminal-themes/catalog-merge.ts
- src/shared/terminal-themes/defaults.ts
- src/shared/terminal-themes/classic.ts
- mobile/src/theme/themed-style-factories.test.ts
- src/shared/terminal-themes/types.ts
- mobile/src/session/TerminalPaneView.tsx
- mobile/src/storage/preferences.test.ts
- src/shared/terminal-themes/popular-dark-core.ts
- src/shared/terminal-themes/popular-dark-extended.ts
- mobile/src/terminal/TerminalWebView.tsx
- mobile/src/storage/preferences.ts
- src/shared/terminal-themes/terminal-theme-catalog.test.ts
- mobile/src/theme/mobile-theme.ts
- src/renderer/src/lib/terminal-contrast-correction.test.ts
- mobile/src/terminal/terminal-webview-html.ts
- src/shared/terminal-themes/index.ts
- mobile/app/h/_layout.tsx
- src/shared/terminal-themes/popular-light.ts
- mobile/src/theme/theme-context.tsx
- mobile/src/theme/mobile-theme-palette.test.ts
- mobile/src/terminal/use-mobile-terminal-theme.test.ts
- mobile/src/terminal/terminal-theme-slot-selection.ts
- mobile/src/terminal/terminal-theme-slot-selection.test.ts
- mobile/src/terminal/TerminalWebView.test.ts
- src/renderer/src/lib/terminal-theme.ts
- mobile/app/_layout.tsx
- mobile/src/storage/terminal-theme-preference.ts
- mobile/src/storage/terminal-theme-preference.test.ts
Rename the existing tokens to darkColors, add lightColors (31 keys) derived from the desktop light tokens, and keep `export const colors = darkColors` so unconverted modules keep compiling and rendering dark. Pin the NewWorkspaceFab pressed state to surfaceBrightPressed (byte- identical in dark). Seed the unthemed-import ratchet and an empty themed-style-factories suite for the migration batches.
Catch unparenthesized useThemedStyles arrow factories in the ratchet, and assert the themed-style factory registry starts empty so later conversion batches must register deliberately rather than leaving a vacuous suite.
Add ThemeProvider / useTheme / useThemedStyles with an explicit resolved mode, persist orca:appTheme defaulting to dark (no visible change), and wire the root + host shells through the runtime. Terminal slot selection falls back to the desktop default for the app mode when the host palette mode mismatches. Drop the two converted layout paths from the unthemed import ratchet.
setAppTheme already updates preference optimistically and fire-and-forgets saveAppTheme; a rejected AsyncStorage write must not become an unhandled rejection (loadAppTheme already falls back on read errors).
Regenerate unthemed-color-imports from listBareColorsImporters on the consolidated tree (161 bare importers). Raise dark textMuted/secondary contrast floors to match stablyai#11651's readable dark tokens.
bbingz
changed the title
076c65e to
7d578d9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Queue hygiene (contributor): converting this PR to draft. It is not superseded by If product wants this stack prioritized, say so and we will mark ready again / rebase. |
|
Closing: the ratchet this PR depends on can no longer hold. Its gate pins 161 bare- By its own description the PR ships no visible change on its own, and the follow-on stack that would give it a payoff doesn't exist. Light mode (#9959) is still worth doing; it needs a plan that doesn't depend on a tree-wide pinned list. Closing to keep my open-PR queue honest. If this turns out to be wrong I'll reopen — please just say so, or reopen it yourself, within the next 7 days. |
Summary
Part of #9959.
Light/dark theme runtime foundation for mobile: a dual palette (
darkColors/lightColors),ThemeProvider/useTheme/useThemedStyles, persistedorca:appThemedefaulting to dark, and root/host shells wired through the runtime. No visible change by itself — the app still ships dark until a later Appearance/picker PR reveals light mode.export const colors = darkColorskeeps unconverted modules compiling and painting dark. The unthemed-import ratchet lists every barecolorsimporter and may only shrink (currently 161 paths, regenerated fromlistBareColorsImporters()on this branch).Dark palette + #11651
darkColorspreserves #11651's secondary/muted tokens (textSecondary: '#a1a1a1',textMuted: '#8c8c8c'). Upstreammobile-theme-contrast.test.ts(viaexport const colors = darkColors) stays green: muted ≥ 4.5:1 on every standard dark surface; secondary more prominent than muted.Contrast invariant (what the tests assert)
mobile-theme-palette.test.tsholds a dual-palette contrast ratchet (CONTRAST_PAIRS). After #11651, darktextMuted/textSecondaryfloors are ≥ 4.5:1 onbgBase/bgPanel/bgRaised. Light mode still has intentional sub-4.5 floors where documented (e.g. lighttextSecondaryonbgPanel≥ 4.34, onbgRaised≥ 3.93; lighttextMuted≥ 3.0 on those surfaces for UI chrome). Diff washes stay in the [1.09, 1.25] band oneditorSurfacein both modes.Screenshots
No visual change.
Testing
pnpm typecheck/ full mobile suite / desktop build — not runlistBareColorsImporters()(161 entries, exact equality)ELI5
This adds the plumbing for light mode on the phone (two color palettes and a theme provider) but still starts in dark mode so nothing looks different yet. A later PR will let you turn light mode on.
AI Review Report
Palette + runtime only; terminal-theme selection/picker files from the #10418–#10421 stack were not included (they land via #10421). Cross-platform: React Native theme tokens only; no Electron shortcuts/paths/shells. #11651 dark contrast preserved.
Security Audit
AsyncStorage app-theme preference only; persist failures swallowed (optimistic UI). No new IPC, secrets, or network.
Notes
Consolidates #10422 (light palette) into this branch. Skip of historical “defer terminal mode-mismatch” commit: it only touched terminal-theme slot files that belong to the #10421 stack, not this foundation PR.