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:
📝 WalkthroughWalkthroughCentralizes built-in terminal theme catalogs and shared appearance resolution in shared modules. Adds custom theme selection, fallback handling, color normalization, opacity conversion, and catalog validation. Updates renderer theme consumers and mobile synchronization to use the shared resolver. Extends headless mobile runtime terminal tabs with host-derived themes across hydrated, persisted, live PTY-backed, and phone-created terminals, with tests covering system preferences, overrides, opacity, missing settings, and prototype-safe catalog lookup. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed scope, implementation context, testing results, limitations, and AI disclosure. However, it does not follow the required template and omits required sections such as Visual Proof or an exact N/A explanation, the required Linked Issue format, and the checklist sections. Resolution Update the description to use the repository template. Add ELI5, What Changed, Why, Linked Issue with a valid issue reference, Visual Proof with attachments or exactly N/A plus a reason, Testing checkboxes, Review, Agent skill upstream boundary, Notes, and the full Checklist.
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 |
60d19d2 to
5bde200
Compare
![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: 08dc2af8-461a-4d96-90c1-4163957b991f
📥 Commits
Reviewing files that changed from the base of the PR and between 60d19d26409d85953522a7d67c40345acf7b847f and 5bde2002215c08bd035007d32b779e1293b7842d.
📒 Files selected for processing (25)
mobile/src/components/NewWorkspaceFab.tsxmobile/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/main/runtime/headless-mobile-terminal-theme.test.tssrc/main/runtime/orca-runtime.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/renderer/src/runtime/sync-runtime-graph.tssrc/shared/runtime-mobile-terminal-theme.test.tssrc/shared/runtime-mobile-terminal-theme.tssrc/shared/terminal-theme-resolution.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 (18)
- src/shared/terminal-themes/terminal-theme-catalog.test.ts
- src/shared/terminal-themes/catalog-merge.ts
- src/shared/terminal-themes/types.ts
- src/shared/terminal-themes/popular-dark-core.ts
- src/shared/terminal-themes/popular-dark.ts
- src/shared/runtime-mobile-terminal-theme.test.ts
- src/shared/terminal-themes/classic.ts
- src/shared/terminal-themes/defaults.ts
- src/shared/terminal-themes/popular-dark-extended.ts
- src/renderer/src/lib/terminal-contrast-correction.test.ts
- src/main/runtime/orca-runtime.ts
- src/shared/terminal-themes/index.ts
- src/shared/runtime-mobile-terminal-theme.ts
- src/shared/terminal-theme-resolution.ts
- src/shared/terminal-themes/popular-light.ts
- src/main/runtime/headless-mobile-terminal-theme.test.ts
- src/renderer/src/runtime/sync-runtime-graph.ts
- src/renderer/src/lib/terminal-theme.ts
3e42457 to
0ec4d3b
Compare
|
Rebased onto current
CodeRabbit notes about |
0ec4d3b to
a269a90
Compare
|
Catch-up rebase onto refreshed #10418 tip on latest |
a269a90 to
f21603b
Compare
|
Catch-up rebase onto latest |
7992064 to
786d75c
Compare
Greptile SummaryThis PR publishes the host's terminal theme to all three headless mobile tab builders in the main process, closing a gap where paired phones on
Confidence Score: 5/5Safe to merge. The change is strictly additive on the wire, and all three tab builders are correctly stamped with no behavioural regressions for existing clients. The core logic (theme resolution, opacity conversion, string-only wire filtering) is a clean extraction of already-shipped renderer code into src/shared/, with no algorithmic changes. All three tab builders are covered, the snapshot-merge first-tab-wins invariant is respected, and the null-prototype catalog map plus Object.hasOwn guards prevent prototype-key lookup. The hardcoded dark bias for systemPrefersDark in main is intentional and well-documented. Sixteen new targeted tests and the existing 62-test suite all pass. No protocol changes, no capability negotiation, no new RPCs. Files Needing Attention: No files require special attention. The most critical path — the three builders in orca-runtime.ts — is straightforward and well-covered by tests.
|
| Filename | Overview |
|---|---|
| src/shared/runtime-mobile-terminal-theme.ts | New shared projection function. Correctly handles null/undefined settings, applies color overrides and opacity in the right order, strips non-string values before sending over the wire, and defaults terminalUseSeparateLightTheme absent to true to match the shipped GlobalSettings default. |
| src/shared/terminal-theme-resolution.ts | Logic extracted verbatim from the renderer's terminal-theme.ts into shared. systemPrefersDark parameter is now required (no DOM default), keeping the module import-safe for main and Metro. No behavioural changes to the resolver itself. |
| src/main/runtime/orca-runtime.ts | Adds resolveHostMobileTerminalTheme() private helper (hardcoded dark bias, well-commented) and stamps terminalTheme on all three RuntimeMobileSessionTerminalTab construction sites. Optional chaining on this.store?.getSettings?.() correctly keeps store-less instantiation working. |
| src/shared/terminal-themes/index.ts | Catalog relocated to shared. getBuiltinTerminalThemePalette uses Object.hasOwn on a null-prototype map, making prototype-key lookup impossible. BUILTIN_TERMINAL_THEME_NAMES is computed once at module load from the same map. |
| src/shared/terminal-themes/catalog-merge.ts | Null-prototype accumulator (Object.create(null)) replaces plain {}, preventing __proto__/constructor keys from shadowing Object.prototype. Object.hasOwn replaces Object.prototype.hasOwnProperty.call for consistency. |
| src/renderer/src/lib/terminal-theme.ts | Renderer module now re-exports the shared resolver and constants; removed duplicated logic. getBuiltinTheme is a thin wrapper over getBuiltinTerminalThemePalette. Return type changed from ITheme to TerminalColorOverrides — PR reports typechecks pass. |
| src/renderer/src/runtime/sync-runtime-graph.ts | Replaces local resolveMobileTerminalTheme(state, prefersDark) with the shared resolveRuntimeMobileTerminalTheme(state.settings, prefersDark). The null-settings guard in the shared function is equivalent to the removed early-return in the old local helper. |
| src/main/runtime/headless-mobile-terminal-theme.test.ts | New test file covering all three builders via the internal type alias. Exercises dark/light/system themes, opacity conversion, store-less instantiation, and palette stamping for each builder path. |
| src/shared/runtime-mobile-terminal-theme.test.ts | Unit tests for the shared projection. Covers null/undefined settings, system-mode resolution, absent terminalUseSeparateLightTheme defaulting, 3-digit hex expansion, non-string value stripping, and unknown-theme fallback. |
| src/shared/terminal-themes/terminal-theme-catalog.test.ts | Validates catalog size (30 entries), sort order, default name stability, own-property-only lookup, and that every palette has exactly the 22 keys the mobile WebView whitelists. |
Sequence Diagram
sequenceDiagram
participant Phone as Mobile Client
participant Main as Main Process (orca-runtime.ts)
participant Shared as src/shared/runtime-mobile-terminal-theme.ts
participant Catalog as src/shared/terminal-themes/index.ts
participant Store as RuntimeStore (GlobalSettings)
Phone->>Main: connect (headless / SSH)
Note over Main: Three tab-builder paths
Main->>Main: resolveHostMobileTerminalTheme()
Main->>Shared: "resolveRuntimeMobileTerminalTheme(settings, systemPrefersDark=true)"
Shared->>Store: getSettings()
Shared->>Shared: resolveEffectiveTerminalAppearance(settings, dark)
Shared->>Catalog: getBuiltinTerminalThemePalette(themeName)
Catalog-->>Shared: "TerminalColorOverrides | null"
Shared->>Shared: apply terminalColorOverrides + opacity
Shared-->>Main: "RuntimeMobileTerminalTheme { mode, theme }"
Main->>Main: "build RuntimeMobileSessionTerminalTab { terminalTheme }"
Main->>Phone: publish tab snapshot (with terminalTheme stamped)
Phone->>Phone: render terminal with host palette
Reviews (2): Last reviewed commit: "docs(runtime): correct the mobile termin..." | Re-trigger Greptile
2e4fba5 to
7eefa59
Compare
|
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 PR sits on top of #10421. Its 31 files are a strict subset of the 37 here; the six that are genuinely mine are Intended order: land #10421 first, then I'll rebase this down to those six files so it reviews as a small headless-only change. Flagging it so neither of you reviews the shared relocation twice. |
7eefa59 to
4a33ca2
Compare
bbingz
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
A phone paired to orca serve or SSH never sees the renderer, so main now resolves the host palette (dark-biased, no matchMedia) and stamps it on the three tab builders. The renderer sync path delegates to the same shared resolver so attached and headless hosts cannot diverge. Co-authored-by: Cursor <cursoragent@cursor.com>
4a33ca2 to
7e0a040
Compare
Summary
A phone paired to
orca serveor an SSH host can receive the host's configured terminal palette even when no desktop renderer is running. Main stamps the existing optionalterminalThemefield on all three terminal-tab builders: persisted-session hydration, PTY-backed rescue/split publication, and phone-initiated terminal creation. A runtime without a settings store still omits the field.Main and renderer share the palette resolver, including custom themes, color overrides, background/cursor opacity and the current minimum-contrast setting. The main-process resolver treats a system theme as dark because it has no
matchMedia; an explicit light setting stays light. Consequently, a main-generated tab can differ from a desktop whose OS currently prefers light. There is no new RPC, stream opcode, or required client field; older hosts retain the client's existing fallback.This remains a draft stacked on #10421 at
a1dcff2cab9a16c6511c25179d3f5a376c6df67c. Review #10421 first; this commit adds the headless publication and shared resolution. Its unique delta now spans 12 files because the runtime and graph-sync modules have since been split. The inherited mobile picker remains dark-mode-only, with the light slot stored for later integration.Testing and review
At
4a33ca211c0011ad43b77616b2dd09fe2ad8b8aa, independent verification passed 1,445 tests across 22 files, with 1 existing skipped test. This includes the three host builders, no-store omission, settings-based palette resolution, overrides/opacity, existing renderer contrast tests, the runtime service suite, and terminal snapshot preservation/reconciliation. CLI, Node and Web TypeScript checks (--composite false), changed-file lint/format and diff whitespace passed. No existing test assertion was removed or weakened.Independent Cursor review passed for this exact SHA (spec-compliance PASS; quality APPROVED). Native iOS/Android rendering, complete Expo bundling/mobile app typecheck, live paired-host/SSH and desktop-system-light smoke, packaging and the full repository suite were not run. These unit tests do not establish live settings propagation to an already-published phone session or native rendering. Manual integration remains unchecked; this is not a ready-for-merge claim.
AI disclosure
Cursor ported the original stack to the current runtime modules. Codex independently checked the original seven-file stack delta, source and affected tests. The shared catalog and picker validation belong to the separately reviewed parent #10421.