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:
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 52.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 15 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Description check | The description covers the required sections in detail, but it conflicts with the supplied change summary on material implementation details. It says there are no new dependencies and that `orca:termi… | Update the description to match the pull request’s actual changes. Confirm whether i18next and the localization files are present, and whether orca:terminalThemeMode is included in PAGE_STORAGE_EXACT_KEYS; then correct the correspondi… |
✅ Passed checks (3 passed)
-
Check name Status Explanation Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request. Title check ✅ Passed The title clearly summarizes the main change: mobile terminals follow the phone’s appearance and support additional theme modes.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 52.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 15 files. (1 skipped: 1 unsupported.)
Full details: Description check
-
Explanation
The description covers the required sections in detail, but it conflicts with the supplied change summary on material implementation details. It says there are no new dependencies and that
orca:terminalThemeModeis not allowlisted, while the change summary reports addingi18nextand adding that key to the page-storage allowlist.Resolution
Update the description to match the pull request’s actual changes. Confirm whether
i18nextand the localization files are present, and whetherorca:terminalThemeModeis included inPAGE_STORAGE_EXACT_KEYS; then correct the corresponding claims and page behavior description.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
- Device-local theme preference store (
mobile/src/storage/terminal-theme-preference.ts) — an AsyncStorage-backedsystem/dark/light/desktopmode with a serialized write queue, deduplicated hydration, and change-only publishing. - Theme resolution hook (
mobile/src/terminal/use-mobile-terminal-theme.ts) — resolves the phone appearance or explicit mode against the shared default palettes, passing the host theme through only indesktopmode. - Session menu (
MobileSessionHeaderMoreActionsSheet.tsx) — a⋯ → Terminal appearanceaction sheet + picker, localized via i18next into 7 languages; the header more-button is now always shown. - Terminal pane (
TerminalPaneView.tsx) — applies the resolved theme and a matching background color to mounted panes, including hidden ones. - Palette sharing —
DEFAULT_TERMINAL_THEMESmoved fromterminal-themes/defaults.tstosrc/shared/terminal-default-themes.ts(values unchanged, type narrowed toTerminalColorOverrides) so desktop and mobile share one source.
ℹ️ System-following default changes the appearance of existing terminals on upgrade
The default flip is well-motivated, and Match desktop faithfully preserves the pre-feature behavior (host palette, including a published minimumContrastRatio override, is passed through unchanged), so nothing is lost. The one consequence to be aware of is upgrade-time: users with no stored preference (none existed before) will see their already-open terminals switch from the desktop palette to the phone-system appearance on first launch, with no migration or notification. This is expected for a new v1 display-only preference — no session/reconnection impact — but it is a visible behavior change worth a release-note line rather than a code fix.
Technical details
# Default flip to phone-following
## Affected sites
- mobile/src/terminal/use-mobile-terminal-theme.ts:34 — `mode === 'system'` resolves to the phone appearance.
- mobile/src/storage/terminal-theme-preference.ts:7 — module default is `'system'`.
## Required outcome
- Confirm the intended upgrade behavior; document in release notes. No code change needed.
## Open questions for the human (optional)
- Should existing sessions show a one-time hint when the appearance first flips, or is a silent flip acceptable?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
@coderabbitai Addressed the documentation warning in 9f0f790b6d with concise JSDoc for the device preference lifecycle, storage ordering/error contract, theme precedence, and session/pane integration. Also removed two personal-fork-only callback fields from the upstream test fixture. Focused appearance regressions pass: 3 files, 15 tests; changed-file lint and formatting pass. Please recheck the updated head. |
|
@pullfrog Please re-review the changes since 84dc7e29e4 on the current head f795819982. The startup-theme flash is fixed by waiting for saved appearance before dismissing the native splash, with deferred-read and read-failure regression tests. Both original threads have replies and are resolved. The intended upgrade default is now explicit in the PR release note. @coderabbitai review Current validation: 520 mobile test files, 4,224 passed / 3 skipped; mobile typecheck and changed-code quality passed. The preceding feature build passed Android release and desktop bundling, with native phone-theme/manual-override/persistence checks documented above. |
There was a problem hiding this comment.
ℹ️ No new issues in this delta — documentation and a small test-fixture cleanup only.
Reviewed changes
terminal-theme-preference.ts,use-mobile-terminal-theme.ts, and the touch components — added JSDoc comments describing the synchronous snapshot, hydration precedence, and serialized-write semantics. All are accurate against the implementation.use-mobile-terminal-theme.test.ts— removedonTerminalPlainTapandonTerminalPlainTapCancelledfrom thepanePropsfixture. Neither prop exists onTerminalPaneViewProps, so this drops two excess-property errors from the typed fixture.
The two minor suggestions from the prior review (cold-start hydration flash, zh/zh-Hant locale granularity) are unaffected by this commit and still stand if you want to take them on.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== HEAD ==='
git rev-parse HEAD
git show -s --format='%H%n%P%n%s%n%b' 9f0f790b6d
echo '=== CHANGED PATHS ==='
git show --name-status --format='' 9f0f790b6d
echo '=== PATCH ==='
git show --format=fuller --find-renames --find-copies 9f0f790b6d -- \
mobile/src/storage/terminal-theme-preference.ts \
mobile/src/terminal/use-mobile-terminal-theme.ts \
mobile/src/session/TerminalPaneView.tsx \
mobile/src/session/MobileSessionHeaderMoreActionsSheet.tsx \
mobile/src/storage/terminal-theme-preference.test.ts \
mobile/src/terminal/use-mobile-terminal-theme.test.ts \
mobile/src/session/MobileSessionHeaderMoreActionsSheet.test.tsx
echo '=== OUTLINES ==='
for file in \
mobile/src/storage/terminal-theme-preference.ts \
mobile/src/terminal/use-mobile-terminal-theme.ts \
mobile/src/session/TerminalPaneView.tsx \
mobile/src/session/MobileSessionHeaderMoreActionsSheet.tsx \
mobile/src/storage/terminal-theme-preference.test.ts \
mobile/src/terminal/use-mobile-terminal-theme.test.ts \
mobile/src/session/MobileSessionHeaderMoreActionsSheet.test.tsx
do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all || true
fi
done
echo '=== CALLBACK-FIELD REFERENCES IN MOBILE TESTS ==='
rg -n -C 3 --glob '*.{test,spec}.{ts,tsx}' \
'(on[A-Z][A-Za-z0-9]*(?:Callback|Change|Press|Select|Save|Load)|callback)' mobile/src || trueLength of output: 109155 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== COMMITS AFTER DOCUMENTATION FIX ==='
git log --oneline 9f0f790b6d..HEAD
echo '=== CURRENT-HEAD DIFF AFTER DOCUMENTATION FIX ==='
git diff --stat 9f0f790b6d..HEAD
git diff --find-renames 9f0f790b6d..HEAD -- \
mobile/src/storage/terminal-theme-preference.ts \
mobile/src/terminal/use-mobile-terminal-theme.ts \
mobile/src/session/TerminalPaneView.tsx \
mobile/src/session/MobileSessionHeaderMoreActionsSheet.tsx \
mobile/src/terminal/use-mobile-terminal-theme.test.ts
echo '=== CURRENT IMPLEMENTATIONS ==='
for file in \
mobile/src/storage/terminal-theme-preference.ts \
mobile/src/terminal/use-mobile-terminal-theme.ts \
mobile/src/session/TerminalPaneView.tsx \
mobile/src/session/MobileSessionHeaderMoreActionsSheet.tsx \
mobile/src/terminal/use-mobile-terminal-theme.test.ts
do
echo "--- $file ---"
cat -n "$file"
done
echo '=== CURRENT FIXTURE CONTRACT ==='
rg -n -C 4 \
'onTerminalPlainTap|onTerminalPlainTapCancelled|type TerminalPaneViewProps|interface TerminalPaneViewProps' \
mobile/src/session/TerminalPaneView.tsx \
mobile/src/terminal/use-mobile-terminal-theme.test.ts || trueLength of output: 23938 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== STARTUP IMPLEMENTATION OUTLINE ==='
ast-grep outline mobile/app/_layout.tsx --items all
echo '=== STARTUP IMPLEMENTATION ==='
cat -n mobile/app/_layout.tsx
echo '=== STARTUP REGRESSION TEST OUTLINE ==='
ast-grep outline mobile/src/storage/terminal-theme-startup.test.ts --items all
echo '=== STARTUP REGRESSION TEST ==='
cat -n mobile/src/storage/terminal-theme-startup.test.ts
echo '=== STARTUP IMPORT/CALL SITES ==='
rg -n -C 4 \
'\b(loadMobileTerminalThemeMode|SplashScreen\.(preventAutoHideAsync|hideAsync)|onNavigatorLayout)\b' \
mobile/app/_layout.tsx mobile/src/storage/terminal-theme-startup.test.tsLength of output: 19983
The JSDoc matches the implemented preference lifecycle, storage ordering and error behavior, theme precedence, and pane integration. The test fixture now matches The later splash-screen change also waits for preference hydration before No new issue found. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Startup flash fix —
mobile/app/_layout.tsxnowawaitsloadMobileTerminalThemeMode()(error-swallowed via.catch) beforeSplashScreen.hideAsync(), so the native splash covers the first frame until the persisted appearance is published. - Splash-ordering regression tests — new
mobile/src/storage/terminal-theme-startup.test.tsextracts the realonNavigatorLayoutcallback through the TypeScript API and executes it to asserthideAsyncis deferred until hydration resolves, and that a rejected read still warns and dismisses the splash.
The prior cold-start hydration flash is correctly addressed. loadMobileTerminalThemeMode() publishes the hydrated mode synchronously inside its .then before the promise resolves, so by the time hideAsync runs, useSyncExternalStore subscribers have already repainted with the stored choice — and a pane mounted after hydration reads the same snapshot, so there is no ordering gap. The new tests can genuinely fail: deleting the await load... line changes the callback so hideAsync fires immediately, tripping the not.toHaveBeenCalled() assertion rather than passing as theatre.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ Startup hydration fix looks right — one minor maintainability note inline.
Reviewed changes
mobile/app/_layout.tsx— the rootonNavigatorLayoutnow awaitsloadMobileTerminalThemeMode()(with acatchthat logs toconsole.warn) beforeSplashScreen.hideAsync(), so the persisted appearance is published before the first visible frame. This resolves the cold-start flash from my earlier review.mobile/src/storage/terminal-theme-startup.test.ts— new test asserting the splash stays up until the load settles, and still dismisses (after warning) on a storage failure.
Both splash assertions are real: they would fail if hideAsync were moved ahead of the load, or if the failure path skipped dismissal.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
|
|
Tested this PR on an iOS simulator (iPhone 11, iOS 27.0):
Since the initial PR description noted that iOS native testing hadn't been run yet, confirming here that iOS runtime behavior and preference hydration work cleanly! |
f795819 to
e9f8b97
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the two commits that landed since the prior pullfrog review — a rebase onto current main plus a test-adaptation commit. The feature code is unchanged from the previously reviewed revision; the delta is test-only.
- Rebased the branch onto current
main(27b823f) — squashed into one feature commit (949bc801) plus one test commit (e9f8b974); the conflicts were confined tomobile/package.jsonandmobile/pnpm-lock.yaml, and the terminal-appearance behavior is unchanged. - Adapted the mobile tests to
main's test typechecking — tests now query by mocked component bindings (ActionSheetModal,PickerModal,TerminalWebView) instead of string element types, and the type assertions are gone. - Pinned the menu test to English (
terminalAppearanceI18n.changeLanguage('en')inbeforeAll) so its label assertions no longer depend on the machine locale. - Added
terminal-theme-preference-page.test.ts— runs the preference against the session page's real storage bridge and asserts the un-allowlistedorca:terminalThemeModewrite is dropped withstorage-write-droppedand the page still starts in Automatic (phone).
The adapted tests remain meaningful: the menu tests fail if the picker options or the save path change, and the new page test fails if the key were added to the page allowlist or if a refused write rejected instead of resolving. Prior review threads are all resolved; no new concerns.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: bb9e75d4-548c-4246-a5bf-1a0265207a8f
📥 Commits
Reviewing files that changed from the base of the PR and between f795819982b7da78b94585562ec049f294dba27b and e9f8b9742248855068fea47f16957d59b4774ee8.
⛔ Files ignored due to path filters (1)
mobile/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
mobile/README.mdmobile/app/_layout.tsxmobile/package.jsonmobile/src/session/MobileSessionHeader.tsxmobile/src/session/MobileSessionHeaderMoreActionsSheet.test.tsxmobile/src/storage/terminal-theme-preference-page.test.tsmobile/src/storage/terminal-theme-startup.test.tsmobile/src/terminal/use-mobile-terminal-theme.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mobile/README.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
Mobile terminals used the palette published by the connected desktop, so switching the phone to light mode could leave the console dark. Add a device-local terminal appearance preference (Automatic (phone), Dark, Light, Match desktop) exposed from the session "More" menu. Automatic follows the phone's color scheme; Match desktop keeps the previous host-provided palette. Mounted and hidden panes update in place, and the saved preference is hydrated before the native splash is dismissed so the first frame uses the right palette. The default dark/light palettes move to src/shared so mobile and desktop reuse the same values; desktop catalog names and colors are unchanged. Menu labels are translated with i18next.
Upstream now gates mobile tests with a tsc ratchet and rejects type assertions in changed code. The ratchet refused the string element types these tests passed to findByType, so query by the mocked component bindings instead: they match the same host elements at runtime and type as components. The test-state and AST casts become a typed hoisted factory, a ts.isCallExpression guard and a declared return type. The menu labels resolve from the device locale, so the English assertions failed on a non-English machine; pin the i18n instance to English for the test. Main can also serve the session route as a hybrid-shell page, whose storage holds only allowlisted keys. Cover the preference there through the page's real AsyncStorage: the page is not handed the saved choice, so it follows the phone, and a choice made in the page applies until the page closes while its write is dropped without rejecting.
e9f8b97 to
631eae4
Compare
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one stale documentation reference inline.
Reviewed changes
This run reviewed the two commits that landed since the prior pullfrog review — a second rebase onto current main (fceca5c) plus the same test-adaptation commit. The feature code is otherwise unchanged from the previously reviewed revision; the material delta is where the shared palettes now resolve.
- Rebased onto current
main(fceca5c) — squashed into one feature commit (84110e9a56) plus one test commit (631eae4415). - Dropped the PR's own palette move — the previously added
src/shared/terminal-default-themes.tsis gone, anduse-mobile-terminal-theme.ts(plus its test) now importsDEFAULT_TERMINAL_THEMESfromsrc/shared/terminal-themes/defaults.ts, whichmainalready hosts. Same two palettes (Ghostty Default Style Dark,Builtin Tango Light), values unchanged; the PR is now mobile-only plus the i18n catalog.
All prior review threads are resolved, and the adapted tests remain meaningful.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
리뷰 반영 세 건. 페이지 allowlist에 orca:terminalThemeMode가 빠져 hybrid-shell 페이지에서 선택이 저장되지 않던 것을 allowlist 추가로 해소하고, 페이지 테스트를 새 계약(전달된 모드 적용, 브리지를 통한 저장)으로 갱신했다. 콜드 스타트에서 저장소 읽기가 거절 대신 멈추면 스플래시가 영구 유지되므로 2초 상한을 뒀다. README의 팔레트 경로를 현재 위치로 고쳤다. Constraint: 페이지 storage는 allowlist와 init 전달 값으로만 동작한다 Tested: vitest storage/page-bridge 6개 파일 54개, 품질 게이트 통과 Not-tested: 실기기 cold start에서의 스플래시 타이밍
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
45871bfd-204c-4a9c-a887-805ae0d1af4b
📒 Files selected for processing (4)
mobile/README.mdmobile/app/_layout.tsxmobile/src/mobile-web-shell/page-storage-keys.tsmobile/src/storage/terminal-theme-preference-page.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mobile/README.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
Caution
Adding orca:terminalThemeMode to the page allowlist makes the shell send it in init.storage, and a page bundle built before this key refuses the whole init frame — so the hybrid-shell session page fails to open on a newer phone against an older desktop. The PR body already documents this exact hazard as the reason the key was withheld, and its "older desktop page bundles are unaffected" claim no longer holds. Details inline.
Reviewed changes
This run reviewed the single commit that landed since the prior pullfrog review (631eae4 → d7f6184): the hybrid-shell allowlist change, a bound on the cold-start appearance read, and a README path fix.
- Granted
orca:terminalThemeModeto the hybrid-shell page (mobile/src/mobile-web-shell/page-storage-keys.ts) — the session page can now read the saved mode frominitand persist a selection through the bridge; the page test was rewritten to assert both crossing and persistence, and reworked its mock to survivevi.resetModules. - Bounded the cold-start appearance read (
mobile/app/_layout.tsx) — a 2sPromise.racenow caps how long a stalled storage read can hold the native splash beforehideAsync. - Corrected the README's shared-palette path —
mobile/README.mdnow points atsrc/shared/terminal-themes/defaults.ts, matching whatuse-mobile-terminal-theme.tsimports.
ℹ️ Nitpicks
mobile/app/_layout.tsx:196-199— the new 2s splash cap has no test;terminal-theme-startup.test.tsstill exercises only the awaited path, so removing or breaking the bound would pass CI.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
직전 커밋에서 orca:terminalThemeMode를 PAGE_STORAGE_EXACT_KEYS에 넣었지만, 구버전 데스크톱이 서빙한 페이지 번들은 BridgeInitStorageSchema의 refine(isPageStorageKey)로 init.storage를 검사해 모르는 키가 하나라도 있으면 init 프레임 전체를 거부한다. 그래서 새 앱과 구버전 데스크톱을 함께 쓰면서 테마를 저장한 사용자는 세션 페이지가 열리지 않게 되었다. PR 본문에 적어 둔 원래 설계(페이지에서는 Automatic으로 시작하고 쓰기는 드롭)로 되돌렸고, 페이지 테스트의 holder 하네스는 그대로 유지했다. Constraint: 구버전 페이지 번들은 init.storage의 모르는 키 하나 때문에 프레임 전체를 거부한다 Rejected: init에서만 키를 빼고 쓰기는 허용 | isPageStorageKeyForRoute와 pageStorageKeysForRoute의 계약이 갈라지는 메인테이너 결정 영역 Directive: 페이지 allowlist에 키를 추가하려면 구버전 번들과의 호환 경로(셸 필터링이나 스키마의 unknown key drop)가 먼저 있어야 한다 Tested: vitest 모바일 전체 903개 파일 통과 Related: d7f6184
terminal-theme-startup.test.ts는 _layout.tsx의 onNavigatorLayout 콜백 소스를 new Function으로 실행하는데, 직전 커밋의 new Promise<null>이 JS에서는 비교식으로 파싱되어 두 테스트가 깨졌다. 콜백을 순수 JS 문법으로 바꾸고 그 이유를 주석으로 남겼으며, 저장소 읽기가 멈춰도 2초 뒤 스플래시가 내려가는지 확인하는 테스트를 추가했다. upstream이 TerminalPaneView에 필수 prop onCellBoxChange를 추가해 use-mobile-terminal-theme.test.ts가 tests-typecheck 래칫에서 실패하던 것도 고쳤다. Tested: vitest 모바일 전체 903개 파일, check:tests-typecheck, tsc --noEmit, oxlint, oxfmt Not-tested: 실기기 콜드 스타트의 스플래시 타이밍
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the two commits that landed since the prior pullfrog review (d7f6184 → 1a4076bd). Together they revert the one caution from that review and close its nitpick.
- Withdrew
orca:terminalThemeModefrom the hybrid-shell page allowlist (012a2d32d8) —page-storage-keys.tsno longer grants the key, so the shell never puts it ininit.storageand a page bundle built before the key keeps accepting the frame. This restores the mixed-version safety the previous review flagged as[!CAUTION], and the PR body's withhold-the-key description is accurate again. - Restored the page test to the withdraw behavior —
terminal-theme-preference-page.test.tsnow seeds the key and asserts the page still reportssystem, and that a selection applies for the page lifetime while the write is dropped withstorage-write-dropped/refusal: 'not-allowed'. Both assertions would fail if the key were re-allowlisted or a refused write rejected. - Added a fake-timer case for the 2s splash bound (
1a4076bd4b) —terminal-theme-startup.test.tsnow advances 2000 ms against a never-settling read and assertshideAsyncruns, covering the previously untested timeout branch. - Fixed mobile test typecheck errors — dropped the
new Promise<null>generic from the splash callback (it is plain JS on purpose, since the startup test evaluates its source vianew Function) and added theonCellBoxChangenoop thatTerminalPaneViewPropsrequires.
The two open questions the caution left with the author were resolved by choosing the conservative path: the key stays withheld, and the hybrid-shell persistence limitation remains documented as a maintainer follow-up rather than shipped. The prior caution thread was replied to and resolved.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
모바일의 다른 화면은 모두 영어 문자열을 쓰는데, 이 메뉴만 i18next와 7개 언어 카탈로그를 새로 들여와 리뷰 쟁점과 번들 크기를 늘리고 있었다. 메뉴 문구를 다른 화면처럼 영어 상수로 옮겼고, 함께 번역되던 Agent History와 Checks 항목은 main의 문구로 되돌렸다. 그 결과 package.json과 lockfile은 main과 같아졌다. Rejected: i18next 유지 | 모바일 첫 i18n 도입을 기능 PR 안에서 하면 새 런타임 의존성과 카탈로그가 리뷰 범위를 키움 Tested: mobile tsc --noEmit, 시트·테마 관련 vitest 9개 파일 87개 통과, 변경 파일 oxlint·oxfmt --check Not-tested: 기기 실행
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the one commit that landed since the prior pullfrog review (1a4076bd → d22e61f765): removal of the i18next setup that the feature commit had introduced.
- Removed the i18next dependency and locale catalogs (
mobile/package.json,mobile/pnpm-lock.yaml, deletedmobile/src/i18n/terminal-appearance.tsand 7*.jsoncatalogs) — both dependency files are byte-identical tomainagain, so the PR is mobile-only. - Replaced the translated menu with English constants (
MobileSessionHeaderMoreActionsSheet.tsx) —APPEARANCE_TITLE,MODE_LABELS, andMODE_HINTSnow carry the same strings the catalog held, and the Agent History / Checks entries return tomain's wording. - Dropped the locale pin from the menu test (
MobileSessionHeaderMoreActionsSheet.test.tsx) — thebeforeAllchangeLanguage('en')call and the i18n import are gone; assertions compare against the literal constants.
This matches the repo convention that mobile has no localized UI, so the removal is the right call rather than a regression. No remaining reference to i18next, terminalAppearanceText, or the deleted catalog exists under mobile/. The menu tests still fail if the picker options or the save path change. All prior Pullfrog threads are resolved.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Any update on this it's looking good |
터미널 테마 선택을 현재 세션 메뉴에 통합하고, 저장된 테마를 앱 외관 및 언어와 함께 초기화했다. 분할 터미널과 숨겨진 탭의 현재 생명주기를 보존하면서 같은 WebView의 테마를 갱신하도록 맞췄다. Constraint: 현재 세션 메뉴, 분할 화면, 입력 활성 상태와 탭 지연 마운트를 유지했다. Tested: 핵심 테스트 36개와 연관 테스트 44개, 모바일 타입 및 테스트 타입 검사, 변경 파일 oxlint 검사를 통과했다. Not-tested: 실제 Android 및 iOS 화면은 이번 변경에서 확인하지 않았다.
ELI5
Your phone's terminal now follows your phone's light/dark setting by default. You can override it from the session's ⋯ → Terminal appearance menu without reconnecting.
What Changed
src/shared/terminal-themes/defaults.ts).mobile/package.jsonand the lockfile are unchanged.Inside the hybrid-shell session page
Since #21977, the session route can also run as a page inside the hybrid shell. This applies to OTA and dev builds; release builds default to the native shell. That page's storage only holds keys listed in
PAGE_STORAGE_EXACT_KEYS, and this PR does not addorca:terminalThemeModeto the list. Inside the page:initframe as on main.storage-write-droppedlog; no error is shown and nothing is thrown. The next page open starts in Automatic again.Alertdoes nothing, and a refused write resolves instead of rejecting.QuickCommandsSheethas the same limitation.Why the key is not granted here: a page bundle served by an older desktop checks
init.storageagainst its own allowlist. When it finds a key it does not know, it refuses the whole frame, so the session page would never open for a phone with a saved appearance. Mixed versions are the normal case. How new keys should reach older page bundles (the shell filtering to what the served bundle knows, or the schema dropping unknown keys instead of refusing) looks like a maintainer decision, so I left it for a follow-up.Rebased onto current main
This branch is two commits plus review follow-ups on
fceca5cece:feat(mobile): follow phone appearance and add terminal theme modes: the previously reviewed change, squashed into one commit.test(mobile): adapt terminal appearance tests to current main:terminal-theme-preference-page.test.ts, runs the preference against the page's real storage module. It checks the page behavior described above.d7f6184780: the native splash waits at most 2 seconds for the saved appearance, so a storage read that stalls cannot hold the splash. The README points atsrc/shared/terminal-themes/defaults.ts. This commit also grantedorca:terminalThemeModeto the page, which012a2d32d8reverts for the reason above.1a4076bd4b: tests only. The splash callback is written in plain JavaScript becauseterminal-theme-startup.test.tsevaluates its source, a new test covers the 2-second bound, and the theme test passes theonCellBoxChangeprop thatTerminalPaneViewon main now requires.d22e61f765: removes the i18next dependency and the 7 locale catalogs that commit 1 added. Mobile has no localized UI, so a first i18n setup does not belong in a feature PR. The menu labels are now English constants, the Agent History and Checks entries are back to main's strings, andmobile/package.jsonand the lockfile match main again.Why
Mobile used the terminal palette provided by the desktop, so changing the phone's appearance could leave the console in the opposite mode. A device-local preference lets the phone choose its own appearance and still keeps an explicit option to follow the desktop.
Related implementation: #10421. This PR uses the same local-store and shared-palette approach, but makes the phone's system appearance the default and adds a four-mode session menu.
Linked Issue
Fixes #25499
Related: the terminal portion of #9959, which #22184 and #20382 also request for e-ink phones. The app chrome does not change, so those broader requests for a light mode on mobile stay open.
Visual Proof
Android emulator with the same mock host and session. These screenshots come from builds made before the rebase:
5f129d158c). It includes an unrelated keyboard toolbar button.84dc7e29e4, based onbba68b1bdd).The rebase does not change the native UI. The hybrid-shell page is not shown.
After switching that same open terminal to phone dark mode:
The appearance picker:
Testing
Prebuilt APK for trying this on a phone: https://github.com/kargnas/orca/releases/tag/preview-mobile-20261005-f1d7719. It contains this PR and my other open mobile PRs on top of
main40f9e7a3fc, and installs next to the regular app as "Orca Preview". On an Android 13 emulator it launched, paired withmobile/scripts/mock-server.tsand painted the terminal.On the current head (
d22e61f765), merged withmain40f9e7a3fc:tsc --noEmit,check:tests-typecheck,oxlintandformat:checkpassed. The fullvitest run: 930 files passed, 2 skipped; 10,025 tests passed, 6 skipped. The only failures are two tests insrc/tasks/mobile-agent-launch-architecture.test.ts, which fail the same way on plainmainon my machine.mobile-web-app-session-terminal-closure,mobile-web-app-page-closure-families,mobile-web-terminal-engine-closure,mobile-web-app-push-subsystem-closure) passed.On the earlier rebase onto
27b823f934, not repeated since:pnpm tcpassed. The desktop terminal theme, contrast, and settings tests that use the moved palette: 6 files, 59 passed.build:mobile-web(build + verify) passed. In the fullconfig/scripts/mobile*.test.mjsrun, 52 of 57 files pass. The other 5 are WebKit render suites that fail because Playwright WebKit is not installed here. They fail the same way on cleanmainlocally; CI covers them.pnpm run check:code-quality:changedpassed (zero new findings, including the casting gate and React Doctor).From the pre-rebase revision (not repeated after the rebase):
assembleReleasepassed after the startup fix. On a headless Android 13 emulator I checked these cases: an existing terminal switching when the phone went from light to dark, manual Light overriding phone Dark, Match desktop, returning to Automatic, and the choice persisting after a restart.Not run:
useColorSchemeinside the page's WebView follows the phone.There are no RPC, bridge, or
initframe changes. The phone sends the session page exactly what it sends on main, so older desktop page bundles are unaffected. The session page that a new desktop serves carries the new menu, and it works against older phone shells because it asks the shell for nothing new. Host-provided palettes remain available unchanged in Match desktop.AI Disclosure
Implemented and reviewed with OpenAI Codex (GPT-6), including delegated focused code review and merge-fixture validation. The rebase onto current main, the test follow-up and the i18n removal were developed with Claude Code (Claude Opus 5.5). The contributor requested the behavior and tested the earlier mobile behavior on their phone.
Review
Please review:
orca:terminalThemeModeshould reach the hybrid-shell page, and how, given the older page bundles described above.The PR contains only the terminal appearance feature. It has no personal keyboard or gesture patches and no release-version changes.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Release note: Mobile terminals now follow the phone's light/dark appearance by default, including existing installations with no saved preference. Choose ⋯ → Terminal appearance → Match desktop to use the connected desktop's palette. Existing sessions stay connected.
The preference stays on the device. Desktop settings, paired-host capabilities, terminal transport, and SSH execution ownership do not change. There are no new environment variables.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred):pnpm tc, the full mobile suite,build:mobile-web, and the changed-code gates passed locally; full desktop lint, test, and build are left to CI