Repository navigation
Conversation
This was referenced Jul 24, 2026
Contributor
|
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: 📥 CommitsReviewing files that changed from the base of the PR and between 112bf74eb57ff171dc12e6ffd11588b4f0c94456 and f0b19fc. 📒 Files selected for processing (26)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (22)
📝 WalkthroughWalkthroughThe change centralizes built-in terminal theme palettes in shared modules and updates renderer theme resolution to use the shared catalog. Mobile adds persisted theme-slot preferences with AsyncStorage hydration, partial saves, subscriptions, and tests. A theme-selection helper and external-store hook resolve device themes against host themes, while terminal panes and WebViews consume the resolved theme and apply its background. Tests cover catalog integrity, persistence behavior, theme resolution, and frame styling. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ffaf282c-409e-4069-869d-fae584f079c8
📥 Commits
Reviewing files that changed from the base of the PR and between 17959a2 and 4f953e42c8c2aa2433807241aa7d52dddf16ddbf.
📒 Files selected for processing (25)
mobile/src/session/TerminalPaneView.test.tsmobile/src/session/TerminalPaneView.tsxmobile/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.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
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ffaf282c-409e-4069-869d-fae584f079c8
📥 Commits
Reviewing files that changed from the base of the PR and between 17959a2 and 4f953e42c8c2aa2433807241aa7d52dddf16ddbf.
📒 Files selected for processing (25)
mobile/src/session/TerminalPaneView.test.tsmobile/src/session/TerminalPaneView.tsxmobile/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.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
🛑 Comments failed to post (1)
src/shared/terminal-themes/catalog-merge.ts (1)
6-13: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject inherited prototype keys during catalog merge and lookup.
Names such as
__proto__,constructor, andtoStringare valid strings underTerminalThemeMap, but ordinary-object semantics can omit or resolve them as inherited values. This violates the lookup contract and lets mobile persistence validation accept invalid theme IDs.
src/shared/terminal-themes/catalog-merge.ts#L6-L13: buildmergedwithObject.create(null)(or define keys safely).src/shared/terminal-themes/index.ts#L24-L25: require an own-property match before returning a palette.src/shared/terminal-themes/terminal-theme-catalog.test.ts#L54-L58: add regression assertions for__proto__,constructor, andtoStringreturningnull.Proposed fix
- const merged: TerminalThemeMap = {} + const merged = Object.create(null) as TerminalThemeMap - return TERMINAL_THEME_CATALOG[name] ?? null + return Object.prototype.hasOwnProperty.call(TERMINAL_THEME_CATALOG, name) + ? TERMINAL_THEME_CATALOG[name] + : null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const merged = Object.create(null) as TerminalThemeMap for (const catalog of catalogs) { for (const [name, theme] of Object.entries(catalog)) { if (Object.prototype.hasOwnProperty.call(merged, name)) { throw new Error(`Duplicate terminal theme name: ${name}`) } merged[name] = theme📍 Affects 3 files
src/shared/terminal-themes/catalog-merge.ts#L6-L13(this comment)src/shared/terminal-themes/index.ts#L24-L25src/shared/terminal-themes/terminal-theme-catalog.test.ts#L54-L58
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ffaf282c-409e-4069-869d-fae584f079c8
📥 Commits
Reviewing files that changed from the base of the PR and between 17959a2 and 4f953e42c8c2aa2433807241aa7d52dddf16ddbf.
📒 Files selected for processing (25)
mobile/src/session/TerminalPaneView.test.tsmobile/src/session/TerminalPaneView.tsxmobile/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.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
🛑 Comments failed to post (1)
src/shared/terminal-themes/catalog-merge.ts (1)
6-13: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject inherited prototype keys during catalog merge and lookup.
Names such as
__proto__,constructor, andtoStringare valid strings underTerminalThemeMap, but ordinary-object semantics can omit or resolve them as inherited values. This violates the lookup contract and lets mobile persistence validation accept invalid theme IDs.
src/shared/terminal-themes/catalog-merge.ts#L6-L13: buildmergedwithObject.create(null)(or define keys safely).src/shared/terminal-themes/index.ts#L24-L25: require an own-property match before returning a palette.src/shared/terminal-themes/terminal-theme-catalog.test.ts#L54-L58: add regression assertions for__proto__,constructor, andtoStringreturningnull.Proposed fix
- const merged: TerminalThemeMap = {} + const merged = Object.create(null) as TerminalThemeMap - return TERMINAL_THEME_CATALOG[name] ?? null + return Object.prototype.hasOwnProperty.call(TERMINAL_THEME_CATALOG, name) + ? TERMINAL_THEME_CATALOG[name] + : null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const merged = Object.create(null) as TerminalThemeMap for (const catalog of catalogs) { for (const [name, theme] of Object.entries(catalog)) { if (Object.prototype.hasOwnProperty.call(merged, name)) { throw new Error(`Duplicate terminal theme name: ${name}`) } merged[name] = theme📍 Affects 3 files
src/shared/terminal-themes/catalog-merge.ts#L6-L13(this comment)src/shared/terminal-themes/index.ts#L24-L25src/shared/terminal-themes/terminal-theme-catalog.test.ts#L54-L58
bbingz
force-pushed
the
feat/mobile-terminal-theme-selection
branch
from
July 25, 2026 04:26
4f953e4 to
46ac49d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mobile/src/theme/mobile-theme.ts (1)
67-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the palette commentary.
Several comments narrate derivations and desktop implementation details. Keep only concise token rationale near the values.
Suggested direction
- // INVENTED: desktop light has only two text tiers. Lightest neutral that still - // clears 3:1 on bgBase/bgPanel/bgRaised (3.69/3.38/3.07). Must stay 6-digit hex — - // MobileAgentIcon.tsx:77 concatenates an alpha suffix onto it. + // Must remain 6-digit hex; MobileAgentIcon appends an alpha suffix.As per coding guidelines, “Comments must be concise, limited to non-obvious information, and preferably one line.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10963250-6494-45d6-9ba7-f2f15753ca13
📥 Commits
Reviewing files that changed from the base of the PR and between 4f953e42c8c2aa2433807241aa7d52dddf16ddbf and 46ac49d98a8a5122944d71e3d977dfffd8859472.
📒 Files selected for processing (30)
mobile/src/components/NewWorkspaceFab.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/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-data.ts
- src/renderer/src/lib/terminal-themes/types.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- mobile/src/terminal/terminal-theme-slot-selection.ts
- src/shared/terminal-themes/popular-dark.ts
- src/shared/terminal-themes/popular-light.ts
- mobile/src/terminal/terminal-webview-html.ts
- src/shared/terminal-themes/defaults.ts
- mobile/src/terminal/TerminalWebView.tsx
- src/shared/terminal-themes/classic.ts
- src/renderer/src/lib/terminal-contrast-correction.test.ts
- src/shared/terminal-themes/catalog-merge.ts
- src/shared/terminal-themes/popular-dark-core.ts
- mobile/src/session/TerminalPaneView.tsx
- mobile/src/storage/terminal-theme-preference.ts
- mobile/src/session/TerminalPaneView.test.ts
- mobile/src/terminal/terminal-theme-slot-selection.test.ts
- mobile/src/terminal/TerminalWebView.test.ts
- mobile/src/terminal/use-mobile-terminal-theme.ts
- src/shared/terminal-themes/popular-dark-extended.ts
- src/renderer/src/lib/terminal-theme.ts
- mobile/src/storage/terminal-theme-preference.test.ts
- mobile/src/terminal/use-mobile-terminal-theme.test.ts
bbingz
force-pushed
the
feat/mobile-terminal-theme-selection
branch
2 times, most recently
from
July 25, 2026 09:15
e9e2a4c to
112bf74
Compare
Contributor
Author
|
Rebased onto current
CR note about trimming |
bbingz
force-pushed
the
feat/mobile-terminal-theme-selection
branch
from
July 25, 2026 09:43
112bf74 to
1819158
Compare
Contributor
Author
The built-in terminal theme catalog lived under src/renderer/src/lib/, which Metro cannot see: mobile/metro.config.js adds only <repo>/src/shared to watchFolders. Moving it to src/shared/ lets the phone render the exact same 30 palettes the desktop ships instead of a hardcoded copy that silently drifts. Two content changes ride along because they are what makes the destination correct rather than merely reachable: - types.ts drops `@xterm/xterm`'s ITheme for the shared TerminalColorOverrides. It is the same 22 keys plus an optional `bold`, and it is already the mobile wire shape (RuntimeMobileTerminalTheme['theme']). A renderer-only xterm type does not belong in a module Node and Metro both consume. - terminal-themes/shared.ts becomes catalog-merge.ts. "shared.ts" inside src/shared/ names nothing; AGENTS.md bans the vague form outright. index.ts absorbs the two one-line functions from terminal-themes-data.ts under domain-concrete names, so src/shared never exports a bare `getTheme`, and that file is deleted rather than left as a shim: only three call sites referenced the old paths repo-wide. The catalog had no test of its own. The new one pins the count at 30 (a naive grep for quoted keys reports 22 because eight themes use unquoted keys), pins the two shipped default names — the display name IS the persisted settings id — and asserts every palette defines exactly the 22 keys mobile's WebView copies, so no theme can render half-default on the phone. Behaviour is unchanged: terminal-theme.ts's export surface is identical, and terminal-contrast-correction.test.ts passes with only its import path edited, which is the proof no palette value moved.
getBuiltinTerminalThemePalette must only return own catalog entries so persisted or typed ids like constructor/toString never resolve through Object.prototype. Merge into a null-prototype map and pin the reject list.
On `orca serve` / SSH hosts the phone has no say in its terminal palette:
it renders whatever the host pushes, or a hardcoded Tokyo Night when the
host pushes nothing. Give the device its own choice, mirroring desktop's
model exactly — a dark slot, a light slot, and `useSeparateLightTheme`
(default true, `src/shared/constants.ts`) — so the two products describe
the same concept with the same words and the same asymmetry (light mode
without a separate light theme uses the dark slot, still reporting light).
A `null` slot is stored as an absent key, so a fresh install and an
explicit "follow desktop" choice are one state: no migration, and no magic
string that a future built-in name could collide with. Nothing is
user-visible yet — with nothing stored the resolver is pure passthrough,
so today's host-follows behaviour is unchanged until the picker lands.
The store lives in its own module rather than `preferences.ts` (which the
app-theme preference will need room in), validates persisted names against
the shared catalog at load so a renamed built-in degrades to "follow
desktop" instead of a blank terminal, and publishes through
`useSyncExternalStore` so a write repaints mounted panes with no
navigation event — which also keeps the override out of the session
screen and out of the record layer's JSON change-detection entirely.
`TerminalPaneView.test.ts` pins the one seam the feature hangs on:
reverting the single `terminalTheme={resolvedTerminalTheme}` line passes
every other test in this change.
…RN frame The frame around the terminal WebView paints the app palette's `terminalBg`. That was invisible while the palette and the terminal were both dark, but device-local theme selection lets a user pick a light terminal inside dark app chrome, which would render the WebView inside a dark halo. The divergence is created by that change, so the fix ships with it; `colors.terminalBg` stays the fallback until a palette arrives.
…r theme slots A save issued before the store had read AsyncStorage merged the patch onto the in-memory default and then wrote all three keys, destroying the two slots it never touched; the `hydrated` flag it set also made the in-flight load discard the stored values for good. The save now hydrates first when the read has not landed yet, still publishing optimistically so a tap repaints live panes without waiting for storage, and the load publishes unconditionally so the patch merges onto a real snapshot. Also: a failed read no longer stays memoised for the whole session, the cold-start hydration effect is pinned by a test (removing it previously passed every test), and the frame background keeps the spec's `theme?.background` guard so a version-mismatched or third-party host that pushes a palette-less payload falls back instead of crashing the pane.
…hed slots A rejected read leaves the merge base at the in-memory default while writes may still succeed, so writing all three keys erased the slots the patch never named. Only patched slots are written in that state. Also pins the WebView normalizer's whitelist against the shared catalog on the side that owns it, so a catalog slot mobile does not know can never silently render half-default.
bbingz
force-pushed
the
feat/mobile-terminal-theme-selection
branch
from
July 25, 2026 10:13
1819158 to
f17e226
Compare
Contributor
Author
|
Catch-up rebase onto latest |
Contributor
Author
|
Consolidated into #10421 — this was the storage half of the picker with no observable effect on its own. Closing. |
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.
Summary
Gives the phone its own terminal palette choice, mirroring desktop's model exactly: a dark slot, a light slot, and a match-dark-mode flag — the same shape as
terminalThemeDark/terminalThemeLight/terminalUseSeparateLightTheme, resolved the same way.Default behaviour is unchanged. With nothing stored, every slot is
null, which means "follow the desktop-pushed palette" — exactly what happens today. There is no UI in this PR; the picker is #10420.Deliberately not a single-slot model: the desktop does not pin the terminal dark in light mode, it keeps a separate light terminal theme (
terminalUseSeparateLightThemeshipstrue). Mirroring that now avoids a storage migration when the app gains light mode.Where the override wires in
mobile/src/session/TerminalPaneView.tsx, not the session screen. Two reasons: the session screen sits at 4994 effective lines against a 5015 cap, andTerminalPaneViewis the actual forwarding boundary, so the override lands where the prop already flows. TheJSON.stringifychange-detection paths inmobile-terminal-records.tsare untouched.TerminalPaneView.test.tsexists specifically because reverting that one wiring line otherwise passes every other test in this PR.The storage guard is the part worth your attention
saveMobileTerminalThemeSelectionwrites conditionally, which is non-obvious from the diff. The truth table:hydratedtruefalsefalseThat last row is the point. A rejected read leaves the merge base at the in-memory default while
setItemmay still succeed, so writing all three keys would silently erase whichever slots the patch never named. Mutation-checked: defeating the guard makes the test fail.A stale or hand-edited theme name degrades to "follow desktop" rather than a blank terminal.
Known minor quirk, not data loss: on the hydrate-succeeds-mid-call path a subscriber can observe two publishes in the same microtask chain (real disk value, then the merged value). Accepted.
Tests
mobile/src/suites for the preference store, slot selection, the hook, and the pane seam. Full mobile suite green; typecheck, oxlint, oxfmt and the max-lines ratchet clean.ELI5
Gives the phone its own terminal theme choice like desktop: separate dark and light slots plus a match-dark-mode flag. With nothing saved, it still follows the desktop/host theme, so default behavior stays the same.