Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a shared catalog of 30 built-in terminal themes and migrates renderer lookups to it. Adds AsyncStorage-backed mobile theme preferences, theme selection resolution, settings rows, picker drawers, accessibility metadata, and terminal pane integration. Applies resolved terminal background colors to the WebView frame and adds tests covering catalog behavior, persistence, selection, settings interactions, pane resolution, and WebView styling. 🚥 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: 1
🧹 Nitpick comments (1)
mobile/src/terminal/TerminalWebView.test.ts (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigrate new coverage away from deprecated
react-test-renderer.React 19 deprecates this renderer, and this test now suppresses its warning. Prefer React Native Testing Library if available.
mobile/src/terminal/TerminalWebView.test.ts#L1-L2: replace the renderer import and host-node assertions.mobile/src/terminal/TerminalWebView.test.ts#L35-L47: remove warning suppression after migration.mobile/src/components/PickerListDrawer.test.ts#L1-L2: replace the renderer import and row assertions.// verify-test-library.mjs // Run with: node verify-test-library.mjs import { readFile } from 'node:fs/promises' for (const file of ['package.json', 'mobile/package.json']) { try { const pkg = JSON.parse(await readFile(file, 'utf8')) const deps = { ...pkg.dependencies, ...pkg.devDependencies } console.log(file, { reactTestRenderer: deps['react-test-renderer'] ?? null, reactNativeTestingLibrary: deps['`@testing-library/react-native`'] ?? null }) } catch {} }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fa77db7-bca3-4ed7-8f01-cc863a120c49
📥 Commits
Reviewing files that changed from the base of the PR and between 17959a2 and 43f9bac1dade7e5be3ea5af66b9050cd7b4ece89.
📒 Files selected for processing (30)
mobile/app/terminal-settings.tsxmobile/src/components/PickerListDrawer.test.tsmobile/src/components/PickerListDrawer.tsxmobile/src/session/TerminalPaneView.test.tsmobile/src/session/TerminalPaneView.tsxmobile/src/storage/terminal-theme-preference.test.tsmobile/src/storage/terminal-theme-preference.tsmobile/src/terminal/TerminalThemeSettings.test.tsmobile/src/terminal/TerminalThemeSettings.tsxmobile/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.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
43f9bac to
3f7bde8
Compare
|
Rebased onto current |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mobile/src/theme/mobile-theme.ts (1)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the migration/history comments.
These comments explain implementation history rather than the current non-obvious contract. Keep a short statement of intent instead.
[diff]
-// Pressed state for the surfaceBright fill — bg-primary/90 composited over bgBase
-// (src/renderer/src/components/ui/button.tsx:12). Dark keeps the value
-// NewWorkspaceFab already rendered, so this is a rename, not a repaint.
+// Pressed state for the primary-action fill.
...
-// Why: unconverted modules keep compiling and keep rendering dark until the
-// themed-styles migration reaches them (see theme/unthemed-color-imports.test.ts).
+// Legacy dark-only alias while the themed-style migration is in progress.
[/diff]As per coding guidelines, comments must be concise, limited to non-obvious information, and preferably one line.
Also applies to: 115-116
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 594a8649-3412-4a42-98dc-49d2d1b888e8
📥 Commits
Reviewing files that changed from the base of the PR and between 43f9bac1dade7e5be3ea5af66b9050cd7b4ece89 and 3f7bde86608f68097a3eb683d04131c3c4c228bd.
📒 Files selected for processing (35)
mobile/app/terminal-settings.tsxmobile/src/components/NewWorkspaceFab.tsxmobile/src/components/PickerListDrawer.test.tsmobile/src/components/PickerListDrawer.tsxmobile/src/session/TerminalPaneView.test.tsmobile/src/session/TerminalPaneView.tsxmobile/src/storage/terminal-theme-preference.test.tsmobile/src/storage/terminal-theme-preference.tsmobile/src/terminal/TerminalThemeSettings.test.tsmobile/src/terminal/TerminalThemeSettings.tsxmobile/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/mobile-theme-palette.test.tsmobile/src/theme/mobile-theme.tsmobile/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
🚧 Files skipped from review as they are similar to previous changes (27)
- src/shared/terminal-themes/popular-dark.ts
- mobile/src/components/PickerListDrawer.test.ts
- src/shared/terminal-themes/types.ts
- src/shared/terminal-themes/classic.ts
- src/shared/terminal-themes/popular-dark-core.ts
- src/renderer/src/lib/terminal-contrast-correction.test.ts
- src/shared/terminal-themes/catalog-merge.ts
- mobile/src/terminal/terminal-theme-slot-selection.ts
- mobile/src/session/TerminalPaneView.test.ts
- src/shared/terminal-themes/popular-dark-extended.ts
- src/shared/terminal-themes/defaults.ts
- mobile/src/terminal/use-mobile-terminal-theme.ts
- mobile/src/terminal/terminal-webview-html.ts
- mobile/src/session/TerminalPaneView.tsx
- mobile/src/terminal/TerminalThemeSettings.test.ts
- mobile/src/components/PickerListDrawer.tsx
- src/shared/terminal-themes/popular-light.ts
- mobile/src/terminal/TerminalThemeSettings.tsx
- mobile/src/terminal/TerminalWebView.tsx
- src/shared/terminal-themes/terminal-theme-catalog.test.ts
- mobile/src/storage/terminal-theme-preference.test.ts
- mobile/src/terminal/TerminalWebView.test.ts
- mobile/src/terminal/use-mobile-terminal-theme.test.ts
- src/shared/terminal-themes/index.ts
- mobile/app/terminal-settings.tsx
- mobile/src/terminal/terminal-theme-slot-selection.test.ts
- mobile/src/storage/terminal-theme-preference.ts
3f7bde8 to
0316250
Compare
|
Correction to my previous comment: the ratchet entry is no longer needed and has been dropped — this branch is back to its original single commit. The earlier rebase had linearised a merge in the stack below, which pulled the light palette (and its ratchet test) into this branch's history; redone with |
0316250 to
003fe0c
Compare
|
Rebased onto current
CodeRabbit notes:
|
003fe0c to
59dc943
Compare
|
Catch-up rebase onto latest |
59dc943 to
04ec3c5
Compare
04ec3c5 to
61f45c9
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. |
bbingz
changed the title
|
Resolving the save-before-hydration thread with the adjudication that was previously only in the body: on the hydrate-succeeds-mid-call path a subscriber can observe two publishes in the same microtask chain (disk value, then the merged value) — an accepted, bounded degradation, not data loss. The truth table in the description covers the cases. |
|
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. |
|
Relationship note — this is the base of a two-PR stack, not a duplicate of #10419. All 31 files here are contained in #10419. The six files that are only in #10419 are the headless half:
Intended order: this PR first (relocate the theme catalog to Reviewing them in that order avoids reading the same 31-file relocation twice. If the relocation itself is the objection, say so here and I'll close both rather than have you review either. |
휴대폰 테마 선택에서 기존 Orca 기본 색상을 재사용하도록 기본 팔레트를 shared로 옮겼다. 데스크톱 카탈로그의 색상과 이름은 유지했다. Related: stablyai/orca#10421 Tested: 터미널 테마와 대비 테스트 23개; pnpm tc
61f45c9 to
a1dcff2
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
a1dcff2 to
93d7450
Compare
Summary
Add a device-local terminal theme picker under Settings → Terminal. A phone can select a built-in palette or Follow desktop, and mounted terminal panes update without navigation. The WebView frame follows the chosen background. Dark/light preferences and Match dark mode are saved independently; current mobile chrome is dark-only, so the dark slot is active and the light slot is stored for a future light-mode integration.
The built-in catalog moves into shared code for desktop/mobile reuse, preserving the 30 theme names and palettes. The picker drawer lives at screen root to avoid clipping and exposes selected/disabled states to accessibility tools. Missing or unknown choices follow the host palette.
This remains a draft and is the stack base for #10419, which supplies host-side theme publication. This PR does not change the remote theme contract or add a stream opcode.
Testing and review
At
a1dcff2cab9a16c6511c25179d3f5a376c6df67c, independent verification passed 46 mobile tests across 7 files using officialreact-test-renderer19.2.8 with mocked React Native primitives, plus 35 desktop/shared tests across 3 files. Checks cover persistence, failed-read retry, choices during hydration, slot resolution, live hook updates, picker selection/accessibility, pane propagation and WebView background.CLI, Node and Web TypeScript checks (
--composite false), changed-file lint/format and diff whitespace passed. These desktop typechecks do not cover the mobile app. Independent Cursor review passed for this exact SHA (spec-compliance PASS; quality APPROVED).Mobile app typecheck, native iOS/Android rendering, complete Expo bundling and live paired-host/SSH smoke were not run: the complete Expo/React Native dependency environment was not installed. No full repository suite was repeated. Manual Settings → Terminal palette selection / Follow desktop / restart validation remains unchecked; this is not a ready-for-merge claim.
AI disclosure
Cursor ported the change. Codex independently checked the original scope and source and reran the affected tests with the real React test renderer in isolated dependencies.