Repository navigation
refactor(comments): slim verbose comments in mobile - #9547
Conversation
Collapse multi-line explanatory comment blocks into single-line "why" statements
per AGENTS.md ("Document the Why, Briefly"): drop restatements of the code and
mechanism narration; keep the non-obvious reason, external refs, and directives.
Comments-only — verified no code changed via a Babel/esbuild comment-strip
token-equality gate against origin/main; typecheck and oxlint clean.
Area: mobile. 11 files changed, 339 insertions(+), 1137 deletions(-).
Co-authored-by: Orca <help@stably.ai>
📝 WalkthroughWalkthroughThe pull request updates mobile home, host, session, transport, notification, and source-control code. Behavioral changes include cache hydration and preservation, client replacement tracking, foreground notification handling, worktree limits, host removal retry state, usage rendering, bottom inset handling, connection logging, and action labels. Most remaining changes revise inline documentation for lifecycle, synchronization, keyboard, terminal, notification, persistence, and PR-loading behavior. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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/app/index.tsx (1)
327-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove ref mutation into an effect to preserve render purity.
Mutating
allClientsRef.currentdirectly during render violates React's purity rules and can lead to inconsistent state in Concurrent Mode (and multiple invocations in Strict Mode).Move this assignment into a
useEffectblock, matching the pattern used forclientRefin the host screen.♻️ Proposed fix
- allClientsRef.current = allClients.map((entry) => ({ - hostId: entry.hostId, - client: entry.client - })) + useEffect(() => { + allClientsRef.current = allClients.map((entry) => ({ + hostId: entry.hostId, + client: entry.client + })) + }, [allClients])
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dc005ffe-8956-40a5-b3b6-5e963eb4aa9f
📒 Files selected for processing (11)
mobile/app/h/[hostId]/index.tsxmobile/app/h/[hostId]/session/[worktreeId].tsxmobile/app/index.tsxmobile/src/notifications/mobile-notifications.tsmobile/src/session/use-mobile-pr-sidebar-controller.tsmobile/src/source-control/MobileSourceControlPanel.tsxmobile/src/terminal/terminal-viewport-refit.tsmobile/src/terminal/terminal-webview-html.tsmobile/src/transport/client-context.tsxmobile/src/transport/host-store.tsmobile/src/transport/rpc-client.ts
| // Why: ConfirmModal closes on confirm; re-open for retry so the failure isn't silent. | ||
| setConfirmRemove(hostToRemove) | ||
| Alert.alert('Could not remove host', 'Please try again.') | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear state to prevent the confirm modal from getting stuck.
In both files, the host removal catch block attempts to update the confirm state using the exact same value it already holds (hostToRemove or true). Because React uses Object.is for state comparisons, it will bail out of the update and skip re-rendering.
Since ConfirmModal closes internally on confirm, the parent state is left out of sync. This prevents the user from opening the modal again on subsequent removal attempts because setting the state to the identical value will continually be ignored.
Clear the state instead so it matches the closed modal, relying on the Alert to inform the user of the failure:
mobile/app/index.tsx#L667-L670: changesetConfirmRemove(hostToRemove)tosetConfirmRemove(null).mobile/app/h/[hostId]/index.tsx#L665-L668: changesetConfirmRemoveHost(true)tosetConfirmRemoveHost(false).
📍 Affects 2 files
mobile/app/index.tsx#L667-L670(this comment)mobile/app/h/[hostId]/index.tsx#L665-L668
Collapse multi-line explanatory comment blocks into single-line "why" statements
per AGENTS.md ("Document the Why, Briefly"): drop restatements of the code and
mechanism narration; keep the non-obvious reason, external refs, and directives.
Comments-only — verified no code changed via a Babel/esbuild comment-strip
token-equality gate against origin/main; typecheck and oxlint clean.
Area: mobile. 11 files changed, 339 insertions(+), 1137 deletions(-).
Co-authored-by: Orca <help@stably.ai>
Slim verbose code comments — mobile
Part of a codebase-wide sweep to slim over-verbose comments per
AGENTS.md→ Document the "Why", Briefly. Comments should state the non-obvious why, not narrate the what/how when the code is already clear; multi-line blocks collapse to a single line.This PR — mobile: 11 files changed, 339 insertions(+), 1137 deletions(-).
What changed
Why:paragraphs → one-line why-statements.eslint-disable,@ts-expect-error,oxlint-disable, …),TODO/FIXME, JSDoc tags, and every genuine non-obvious reason / external reference (issue numbers, platform quirks, race & ordering constraints).useEffect/ regex / bitwise / concurrency: kept a ≤1-line summary where it aids reading.{/* … */}comment text shortened; comments inside template-literal strings left untouched.Safety
Every file in this PR is comments-only — enforced deterministically by stripping all comments (incl. JSX
{/* */}) from bothorigin/mainand the change and confirming the remaining code tokens are identical.pnpm typecheck✅ ·oxlint✅ (no new findings vsmain).Part of a set of 6 area PRs from one sweep.
🤖 Generated with Claude Code
Made with Orca 🐋