Repository navigation
Conversation
Reuse the existing desktop presentation policy in shared code, keep host failures and source notices separate from skipped transcript files, and retain their scanner-authored messages and actionable skip reasons. Validate the presentation fields while preserving newer host agent, kind, and host spellings. Adapted from the remaining mobile presentation in original PR #15671. Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe scan issue schema now validates required banner fields and optional issue metadata. Shared helpers classify blocking issues, notices, skipped-transcript counts, and transcript reasons. The mobile session history panel now renders banners based on those classifications. The panel also passes its empty state and header to the session list. Tests cover parsing, banner behavior, and long notice lists. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The mobile history presentation is ready for normal merge checks. Localization of this banner can follow the planned mobile localization migration. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is confined to interpreting and displaying existing history responses. No new privileged action or host-selection behavior was identified. Malformed scan notices now produce an error rather than being accepted; compatibility with all deployed host versions remains unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 3ae70a2a-5eb5-4303-b837-37c74cc1bc36
📒 Files selected for processing (11)
mobile/src/agent-history/MobileAgentSessionHistoryPanel.tsxmobile/src/agent-history/agent-history-reply-schema.test.tsmobile/src/agent-history/agent-history-reply-schema.tsmobile/src/agent-history/agent-history-styles.tsmobile/src/agent-history/mobile-agent-history-scan-banners.test.tsxmobile/src/agent-history/mobile-agent-history-scan-banners.tsxmobile/src/agent-history/use-mobile-agent-history-state.tssrc/renderer/src/components/right-sidebar/AiVaultScanIssueBanners.tsxsrc/renderer/src/components/right-sidebar/ai-vault-scan-issue-state.tssrc/shared/ai-vault-scan-issue-state.test.tssrc/shared/ai-vault-scan-issue-state.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/right-sidebar/ai-vault-scan-issue-state.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new issues array validation fails the entire aiVault.listSessions reply when any single issue row is malformed. Because the operation uses require-result-or-throw-message, that failure routes the whole mobile Agent History screen to its error state and discards every readable session. A per-row salvage keeps the sessions and still keeps malformed rows out of the renderer.
Reviewed changes
- Shared scan-issue policy — Desktop's
src/renderer/.../ai-vault-scan-issue-state.tsmoved intosrc/shared/ai-vault-scan-issue-state.tsand was retyped to a structural{ sessions, issues }view; the desktop banner now imports it from there and the policy test moved renderer → shared. - Mobile scan banners — New
MobileAgentHistoryScanBannersreuses that policy to render the host-failure row, kinded notices, and the accurate skipped-transcript count plus capped reasons, instead of counting every issue as a skipped transcript. - Reply-schema validation —
agentHistorySessionScanSchema.issueschanged from rows ofz.unknown()to alooseObjectrequiringagent/path/messagewithkind/executionHostIdsalvaged. - Scroll viewport fix —
MobileAgentSessionHistoryListgainedheader/emptyStateslots; the panel now renders banners, the resume message and the empty state inside theSectionList, so notices scroll with the sessions and cannot shrink the session viewport to zero. - Tests — Shared-policy coverage moved and extended; new banner tests and a 500-notice viewport regression; reply-schema tests for a newer host's issue kind and for refused rows.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
| export const agentHistorySessionScanSchema = z.looseObject({ | ||
| sessions: z.array(z.unknown()), | ||
| issues: z.array(z.unknown()) | ||
| issues: z.array( |
There was a problem hiding this comment.
issues is parsed with a plain z.array(...), so a single row missing agent/path or carrying a non-string message fails the whole agentHistorySessionScanSchema. agentHistorySessionScan uses require-result-or-throw-message, so that failure throws and use-mobile-agent-history-state.ts:138-144 replaces the entire screen with { kind: 'error' }, discarding every readable session because one issue row was malformed.
Session rows deliberately stay z.unknown() for exactly this forward-compat reason, so the issue array should salvage per-row instead. This requires adding salvagingArray to the ../../../src/shared/zod-salvage import.
Technical details
# One malformed issue row fails the entire scan reply
## Affected sites
- `mobile/src/agent-history/agent-history-reply-schema.ts:43-51` — plain `z.array(z.looseObject(...))` propagates any row failure to the whole parse.
- `mobile/src/agent-history/agent-history-reply-schema.test.ts:55-62` — pins the whole-reply failure (`safeParse(...).success === false`) using only `sessions: []`, so it never demonstrates that valid sessions survive the failure.
- `mobile/src/agent-history/use-mobile-agent-history-state.ts:127-144` — the interpret throw becomes `screenState.kind === 'error'`, replacing the valid `sessions`.
## Required outcome
- A malformed issue row must not remove readable sessions; the reply should still publish its valid `sessions` (and any valid issues).
## Suggested approach
- Wrap the row schema in `salvagingArray` so invalid rows drop individually; ~20 mobile reply schemas already use it (`source-control/git-status-reply-schema.ts`, `tasks/task-list-reply-schema.ts`, `session/session-read-reply-schema.ts`, ...). Update the `refuses unreadable banner rows` test to assert the bad row is dropped while valid rows/sessions survive. Unlike a top-level array, here the array is one member of a larger reply, so there is no equivalent JSON-array wire constraint to preserve.
## Open questions for the human
- If refusing the whole reply is intentional (treating any malformed issue row as host incompatibility), that should be stated explicitly — the current docstring only explains why *session* rows stay unknown.| issues: z.array( | |
| issues: salvagingArray( |
There was a problem hiding this comment.
Important
This is a repeat pass on be3095d. The prior review's blocker on the issues array is still open and unaddressed, and I found nothing new that changes that disposition. Recommend the one-line salvage fix before merge; everything else here is mergeable.
Reviewed changes
- Shared scan-issue policy — desktop's presentation policy moved to
src/shared/ai-vault-scan-issue-state.tsand was retyped to a structural{ sessions, issues } | null; the renderer banner now imports the shared module and the policy test moved with it (logic unchanged). - Mobile scan banners — new
MobileAgentHistoryScanBannersrenders the blocking host failure, kinded source/notice rows, the accurate skipped-transcript count and up to three distinct reasons, replacing the old "count every issue as a skipped transcript" behavior. - Reply-schema validation —
agentHistorySessionScanSchema.issuesnarrowed from opaque rows to alooseObjectrequiringagent/path/message, withkind/executionHostIdsalvaged. - Scroll viewport fix — banners, resume message and empty state now live in the
SectionList'sheader/emptyState; title/scope/search stay fixed and the list ownsflex: 1, so a long notice list scrolls instead of collapsing the sessions to zero height. - Tests — shared-policy coverage extended, new banner tests, a 500-notice viewport regression, and reply-schema tests for a newer host's kind and for refused rows.
⚠️ The issues array is still all-or-nothing, and its test locks that in
Same root cause as the open thread on mobile/src/agent-history/agent-history-reply-schema.ts:43, so I am not re-anchoring a duplicate inline comment. issues is a plain z.array(...), so a single row that fails agent/path/message fails the whole scan reply; under require-result-or-throw-message that surfaces as { kind: 'error' } in use-mobile-agent-history-state.ts:138-144 and discards every readable session. The house pattern for wire arrays is salvagingArray, which drops the bad row and keeps valid siblings.
Technical details
# One malformed issue row fails the entire scan reply
## Affected sites
- `mobile/src/agent-history/agent-history-reply-schema.ts:43-51` — plain `z.array(z.looseObject(...))` propagates any row failure to the whole parse.
- `mobile/src/agent-history/agent-history-reply-schema.test.ts:55-62` — asserts `success === false` with `sessions: []`, so it never demonstrates that valid sessions should survive; it needs to change with the fix.
- `mobile/src/agent-history/use-mobile-agent-history-state.ts:127-144` — the interpret throw becomes `screenState.kind === 'error'`.
## Context (why this is a forward-compat guard, not a likely malformed-row bug)
- `src/main/ai-vault/session-list-result-validation.ts:86-98` already validates each issue on the serving host (`agent.min(1)`, `path`, `message`) and replaces rejects with a notice, so a current host does not emit malformed rows.
- The exposure is a host whose issue shape diverges (e.g. dropping `path` for a source-wide notice). The client read is the last line, and the failure mode is total, so the house salvage pattern is still the right call.
## Required outcome
- A malformed issue row must not remove readable sessions; publish valid `sessions` and any valid issues.
## Suggested approach
- Wrap the row schema in `salvagingArray` and add it to the `../../../src/shared/zod-salvage` import; update the `refuses unreadable banner rows` test to assert the bad row drops while valid rows and sessions survive.ℹ️ Nitpicks
config/scripts/ci-shard-timings.json:6018still keys the deletedsrc/renderer/src/components/right-sidebar/ai-vault-scan-issue-state.test.tsand has no entry for the movedsrc/shared/ai-vault-scan-issue-state.test.ts. Shard balancing just falls back to a default weight, so this is cleanup only.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
When an agent's history database is busy, mobile History should explain that and say how to retry. It should count only individual unreadable transcripts as skipped. Notices also scroll with the history, so a long notice list cannot hide sessions or the empty state.
What Changed
Mobile now uses the same scan-issue presentation rules as desktop History. It shows the host's source and host messages, displays an empty-scan host failure once, and keeps the actual skipped-transcript count and distinct failure reasons. Desktop behavior stays the same through an import of the existing policy from shared code.
The existing SectionList now owns scan banners, resume messages, sessions, and its empty state in one scroll area. Title/Refresh, scope, and search stay fixed. Its viewport fills the remaining height, including when there are no sessions.
The mobile reply reader checks the fields needed to render each message while preserving future agent names, optional issue kinds, host identifiers, and other host metadata. It uses existing theme styles and adds no wire fields.
Why
A database-wide lock previously appeared as “2 transcripts skipped,” which hid the useful retry explanation and overstated individual file failures. Reusing desktop's existing policy keeps both screens consistent without another classification rule or scanner implementation. Using the list's existing header and empty slots also preserves every notice while keeping history reachable; no second scroll area or notice truncation is needed.
Linked Issue
References the remaining mobile presentation from original PR #15671, with Brennan Benson credited in the commit. This does not close the separate Windows proof in #14133; original issue and PR closure is handled after their full remaining scope merges.
Visual Proof
Additional local evidence:
mobile-before-locked.png— a genuine source lock displayed “2 transcripts skipped.”mobile-locked.png— scanner-authored OpenCode source notices, with zero skipped files.counts-mobile-mixed.png— a real unreadable transcript plus the locked source displays both notices and exactly “1 transcript skipped,” including the file failure reason.mobile-recovered.png— release and Refresh restore the CLI-created session.REPORT.md.Additional viewport reproduction at 390 × 812 renderer content:
Before: 500 explicitly synthetic copies of a genuine source notice reduce the session viewport to zero. After: the viewport is 677 pixels high; scrolling reaches the genuine CLI-created session and the empty state. Refresh stays fixed. These are hidden production mobile History routes over actual encrypted pairing, not a physical phone. The copied real session and current scanner are identical in both runs; current v2 lock classification remains a separate opener dependency.
Testing
Shared presentation tests, mobile banner/reply-reader tests, and mobile refresh-retry tests pass. The viewport correction passed 14 focused mobile tests. Both new regressions fail against the original source; the final copy extraction and formatting pass those two regressions again. Commit hooks enforce the existing 400-line panel limit without a suppression. Node, web, and mobile typechecks, the mobile test typecheck ratchet, the changed-code quality/design-system gate, and desktop/mobile production builds pass.
Manual proof used installed OpenCode 2.0.16 to create a real session in a private HOME/all four XDG roots. A consistent task-owned SQLite backup was held under a real exclusive lock, and the hidden desktop renderer and complete packaged mobile History route used an actual paired encrypted RPC connection. The proof combines this focused presentation commit with native core #24647 and the separate canonical opener commit
b746ce1ad9a; those changes are not part of this PR's diff. Each version's source wait is bounded at 1500 ms; the scan of both versions took about 3260 ms. A 300 ms lock recovered with no notice, and release plus both Refresh controls restored the real session.Actual testing was on macOS. The mobile route ran in a hidden Electron renderer; no physical iOS/Android, Windows, Linux desktop, SSH, or WSL rendered validation is claimed. No user data, global mobile settings, or real keychain service was changed.
Review
The shared policy and mobile classification were independently reviewed. The focused diff contains only the presentation and viewport changes. Independent review and rendered before/after evidence clear the viewport finding.
Agent skill upstream boundary
Notes
Existing optional issue fields and host messages carry the classification, so older/newer paired hosts remain compatible. Folder workspaces and execution-host identifiers are covered. Scanner/cache/worker changes and Windows-specific proof remain separate. The skipped-transcript sentence was moved unchanged from the existing panel; mobile has no existing locale runtime, so this focused change preserves its current language behavior.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Focused local checks passed as listed above; full repository lint/test/build coverage remains CI's gate. The before/after screenshots are attached above.