Sync model picker state across ADE surfaces#493
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR migrates model picker favorites/recents storage from process-wide in-memory + ChangesModel Picker DB-Backed Storage & TUI Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Owner
Author
|
@copilot review but do not make fixes |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ade-cli/src/services/sync/syncRemoteCommandService.ts (1)
2315-2355:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
modelPicker.*project-scoped.These handlers now read/write per-project state, but they’re still registered as
scope: "runtime". InsyncHostService.handleCommand, only project-scoped commands require/validateprojectId, so a missing or stale project id here will silently mutate the host’s current DB instead of the intended project after a switch.Proposed fix
- register("modelPicker.getFavorites", { viewerAllowed: true }, async () => ({ + register("modelPicker.getFavorites", { viewerAllowed: true }, async () => ({ favorites: requireModelPickerStore().getFavorites(), - }), "runtime"); + }), "project"); register("modelPicker.setFavorites", { viewerAllowed: true }, async (payload) => { const rawFavorites = (payload as { favorites?: unknown }).favorites; const favoritesInput = Array.isArray(rawFavorites) ? rawFavorites.filter((entry): entry is string => typeof entry === "string") : []; return { favorites: requireModelPickerStore().setFavorites(favoritesInput) }; - }, "runtime"); + }, "project"); register("modelPicker.toggleFavorite", { viewerAllowed: true }, async (payload) => { const modelId = typeof (payload as { modelId?: unknown }).modelId === "string" ? ((payload as { modelId?: string }).modelId as string) : ""; return requireModelPickerStore().toggleFavorite(modelId); - }, "runtime"); + }, "project"); register("modelPicker.getRecents", { viewerAllowed: true }, async () => ({ recents: requireModelPickerStore().getRecents(), - }), "runtime"); + }), "project"); register("modelPicker.pushRecent", { viewerAllowed: true }, async (payload) => { const modelId = typeof (payload as { modelId?: unknown }).modelId === "string" ? ((payload as { modelId?: string }).modelId as string) : ""; return { recents: requireModelPickerStore().pushRecent(modelId) }; - }, "runtime"); + }, "project");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts` around lines 2315 - 2355, The modelPicker handlers in registerModelPickerRemoteCommands are currently registered with scope "runtime" so they bypass projectId validation and can mutate the wrong DB; update each register call for "modelPicker.getFavorites", "modelPicker.setFavorites", "modelPicker.toggleFavorite", "modelPicker.getRecents", and "modelPicker.pushRecent" to use scope "project" (replace the final "runtime" argument with "project") so syncHostService.handleCommand will require/validate projectId and the handlers operate on the correct per-project store.apps/ade-cli/src/tuiClient/types.ts (1)
118-119:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInconsistent export/import for
ChatInfoPlanStep.Line 118 exports
ChatInfoPlanStepbut line 119 no longer imports it, which will cause a TypeScript error. RemoveChatInfoPlanStepfrom the export statement on line 118 to match the import.🐛 Proposed fix
-export type { SubagentSnapshot, ChatInfoPlan, ChatInfoPlanStep } from "../../../desktop/src/shared/chatSubagents"; +export type { SubagentSnapshot, ChatInfoPlan } from "../../../desktop/src/shared/chatSubagents"; import type { SubagentSnapshot, ChatInfoPlan } from "../../../desktop/src/shared/chatSubagents";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/types.ts` around lines 118 - 119, The export currently lists ChatInfoPlanStep but the file only imports SubagentSnapshot and ChatInfoPlan; remove ChatInfoPlanStep from the export statement so the exported symbols match the imported ones (i.e., export only SubagentSnapshot and ChatInfoPlan) to eliminate the TypeScript error; update the export line that currently reads "export type { SubagentSnapshot, ChatInfoPlan, ChatInfoPlanStep }" to omit ChatInfoPlanStep.
🧹 Nitpick comments (3)
apps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsx (1)
552-559: 💤 Low valueEffects run on every render; consider dependency arrays.
Both
useEffecthooks lack dependency arrays, so they execute after every render. For measurement (onMeasureOrigin) this ensures responsiveness to layout changes, butdrawInlineLogoperforms stdout writes on each render. If performance becomes a concern in terminals supporting inline images, consider memoizing or adding dependencies.Also applies to: 578-601
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsx` around lines 552 - 559, Both useEffect hooks run after every render because they lack dependency arrays; update the effect that measures the pane to include the dependencies it uses (e.g., onMeasureOrigin, rootRef.current and measurePaneOrigin) so it only re-runs when measurement inputs change, and update the effect that calls drawInlineLogo to include its inputs (e.g., drawInlineLogo, logo data, terminal capability flag) or guard/memoize the drawInlineLogo call so it does not perform stdout writes on every render; locate the hooks in ModelPickerPane (the useEffect that references onMeasureOrigin/rootRef/measurePaneOrigin and the other useEffect that calls drawInlineLogo) and add appropriate dependency arrays or a memo/guard to prevent unnecessary re-runs and terminal writes.apps/ade-cli/src/services/modelPickerStore.ts (2)
170-189: ⚡ Quick winWrap setFavorites reconciliation in a transaction for atomicity.
The current implementation performs multiple separate operations (deletes followed by inserts/updates) without an explicit transaction. While SQLite provides some default transactional behavior, concurrent calls could observe partial state during the reconciliation.
Consider wrapping the reconciliation logic in a transaction:
db.run("BEGIN"); try { for (const id of existing) { if (!desiredSet.has(id)) { db.run("delete from model_picker_favorites where model_id = ?", [id]); } } // ... rest of reconciliation db.run("COMMIT"); } catch (err) { db.run("ROLLBACK"); throw err; }This ensures all-or-nothing semantics and prevents intermediate states from being visible to concurrent readers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/modelPickerStore.ts` around lines 170 - 189, The setFavorites reconciliation (in setFavorites, which uses sanitizeIdList, readFavoritesFromDb, nowIso and multiple db.run calls) must be wrapped in an explicit transaction so deletes and inserts/updates are atomic; modify setFavorites to issue "BEGIN" before performing the existing deletion loop and insert/update loop, "COMMIT" after successful completion, and "ROLLBACK" in a catch/finally path on error (rethrow the error), ensuring all db.run operations use the same db handle so the caller never observes partial state.
123-131: 💤 Low valueConsider using sequential integers instead of timestamp staggering for ordering.
The current approach uses synthesized timestamps like
2026-05-31T23:30:15.000Z#0000to preserve ordering. While this works (string comparison sorts correctly), it's unconventional and might confuse future maintainers. Consider instead:
- Add an
order_indexinteger column to both tables- Maintain explicit ordering with simple incrementing integers
- This would be more self-documenting and avoid the non-standard
#delimiterThe current implementation is correct, just less intuitive than sequential integers.
Also applies to: 135-140, 181-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/modelPickerStore.ts` around lines 123 - 131, Replace the synthesized timestamp-with-hash ordering with an explicit integer order column: add an order_index column to the model_picker_favorites (and the other affected table referenced at lines 135-140 and 181-187) and when importing legacy.favorites use a simple incremental counter (e.g., start at 0 and increment per item) instead of building createdAt via nowIso() and String(index).padStart(...); update the INSERT in db.run that targets "model_picker_favorites (model_id, created_at)" to instead insert into "model_picker_favorites (model_id, order_index)" (and similarly for the other table), ensure any queries that previously ordered by created_at now order by order_index, and remove or stop synthesizing the createdAt string in the import logic (the functions/variables to change are nowIso(), legacy.favorites, the createdAt construction, and the INSERTs referencing model_picker_favorites).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/tuiClient/components/ModelPicker/modelPickerLayout.ts`:
- Around line 190-191: The builders produce inconsistent availability flags:
entriesFromCatalog sets isAvailable using model.isAvailable && authStatus !==
"unavailable" while entryFromDescriptor and entryFromModelInfo use only
authStatus !== "unavailable"; unify them by applying the same rule across all
entry builders—either include the intrinsic model.isAvailable check (use
model.isAvailable when present alongside authStatus !== "unavailable") in
entryFromDescriptor and entryFromModelInfo, or remove the model.isAvailable
check from entriesFromCatalog so all three functions rely solely on authStatus
!== "unavailable"; update the availability assignment in entriesFromCatalog,
entryFromDescriptor, and entryFromModelInfo accordingly and ensure any
null/undefined model.isAvailable is treated as true if you choose the
authStatus-only semantics.
---
Outside diff comments:
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 2315-2355: The modelPicker handlers in
registerModelPickerRemoteCommands are currently registered with scope "runtime"
so they bypass projectId validation and can mutate the wrong DB; update each
register call for "modelPicker.getFavorites", "modelPicker.setFavorites",
"modelPicker.toggleFavorite", "modelPicker.getRecents", and
"modelPicker.pushRecent" to use scope "project" (replace the final "runtime"
argument with "project") so syncHostService.handleCommand will require/validate
projectId and the handlers operate on the correct per-project store.
In `@apps/ade-cli/src/tuiClient/types.ts`:
- Around line 118-119: The export currently lists ChatInfoPlanStep but the file
only imports SubagentSnapshot and ChatInfoPlan; remove ChatInfoPlanStep from the
export statement so the exported symbols match the imported ones (i.e., export
only SubagentSnapshot and ChatInfoPlan) to eliminate the TypeScript error;
update the export line that currently reads "export type { SubagentSnapshot,
ChatInfoPlan, ChatInfoPlanStep }" to omit ChatInfoPlanStep.
---
Nitpick comments:
In `@apps/ade-cli/src/services/modelPickerStore.ts`:
- Around line 170-189: The setFavorites reconciliation (in setFavorites, which
uses sanitizeIdList, readFavoritesFromDb, nowIso and multiple db.run calls) must
be wrapped in an explicit transaction so deletes and inserts/updates are atomic;
modify setFavorites to issue "BEGIN" before performing the existing deletion
loop and insert/update loop, "COMMIT" after successful completion, and
"ROLLBACK" in a catch/finally path on error (rethrow the error), ensuring all
db.run operations use the same db handle so the caller never observes partial
state.
- Around line 123-131: Replace the synthesized timestamp-with-hash ordering with
an explicit integer order column: add an order_index column to the
model_picker_favorites (and the other affected table referenced at lines 135-140
and 181-187) and when importing legacy.favorites use a simple incremental
counter (e.g., start at 0 and increment per item) instead of building createdAt
via nowIso() and String(index).padStart(...); update the INSERT in db.run that
targets "model_picker_favorites (model_id, created_at)" to instead insert into
"model_picker_favorites (model_id, order_index)" (and similarly for the other
table), ensure any queries that previously ordered by created_at now order by
order_index, and remove or stop synthesizing the createdAt string in the import
logic (the functions/variables to change are nowIso(), legacy.favorites, the
createdAt construction, and the INSERTs referencing model_picker_favorites).
In `@apps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsx`:
- Around line 552-559: Both useEffect hooks run after every render because they
lack dependency arrays; update the effect that measures the pane to include the
dependencies it uses (e.g., onMeasureOrigin, rootRef.current and
measurePaneOrigin) so it only re-runs when measurement inputs change, and update
the effect that calls drawInlineLogo to include its inputs (e.g.,
drawInlineLogo, logo data, terminal capability flag) or guard/memoize the
drawInlineLogo call so it does not perform stdout writes on every render; locate
the hooks in ModelPickerPane (the useEffect that references
onMeasureOrigin/rootRef/measurePaneOrigin and the other useEffect that calls
drawInlineLogo) and add appropriate dependency arrays or a memo/guard to prevent
unnecessary re-runs and terminal writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a91bfdd-379c-49ea-93f1-2d9068173d83
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**
📒 Files selected for processing (23)
apps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/services/modelPickerStore.test.tsapps/ade-cli/src/services/modelPickerStore.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsxapps/ade-cli/src/tuiClient/components/ModelPicker/modelPickerGeometry.test.tsapps/ade-cli/src/tuiClient/components/ModelPicker/modelPickerGeometry.tsapps/ade-cli/src/tuiClient/components/ModelPicker/modelPickerLayout.test.tsapps/ade-cli/src/tuiClient/components/ModelPicker/modelPickerLayout.tsapps/ade-cli/src/tuiClient/components/ModelPicker/types.tsapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/types.tsapps/desktop/src/main/services/state/kvDb.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/SyncService.swift
💤 Files with no reviewable changes (2)
- apps/ade-cli/src/tuiClient/components/ModelPicker/types.ts
- apps/ade-cli/src/tuiClient/tests/RightPane.test.tsx
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 31, 2026 23:57
arul28
force-pushed
the
ship/tui-model-picker-nav-sync
branch
from
e49039b to
102da52
Compare
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 1, 2026 00:17
arul28
force-pushed
the
ship/tui-model-picker-nav-sync
branch
from
102da52 to
f66b95d
Compare
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 1, 2026 00:39
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
1 participant
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
Validation
Risks
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
This PR replaces the file-based (
~/.ade/modelPicker.json) model picker persistence with a per-project cr-sqlite DB, enabling CRR replication so desktop, TUI, and iOS converge on the same favorites/recents for a project. It also tightens TUI model picker behavior and geometry.model_picker_favorites/model_picker_recentstables tokvDb.tsandDatabaseBootstrap.sql, with a one-time best-effort import of the legacy JSON file wrapped in a proper SQLite transaction; a durable kv marker prevents stale re-import after rows are cleared.createModelPickerStorenow operates synchronously against the DB, with stamped-index ordering for favorites and a module-level sequence counter (sequencedNowStamp) to keep recents ordered within the same millisecond.normalizeCatalogProvider), wider right pane for the model picker, a two-column rail/list nav model withrailFocused, andmergeNewChatModelPickerContextto preserve in-progress picker state across lane refreshes.Confidence Score: 5/5
Safe to merge; the store rewrite is correct, the migration is atomic, and all three previous blocking concerns have been resolved.
The DB migration is wrapped in a proper SQLite transaction, the millisecond timestamp collision is fixed with the stamped-sequence counter, and the legacy file migration marker is durable. The TUI geometry and layout changes are well-unit-tested. No new correctness issues were found.
No files require special attention — the core store and sync wiring are straightforward and covered by the new test suite.
Important Files Changed
Sequence Diagram
sequenceDiagram participant iOS participant SyncHost as Sync Host (ade-cli) participant DB as cr-sqlite DB participant TUI participant Desktop Note over DB: model_picker_favorites<br/>model_picker_recents<br/>(CRR tables) TUI->>DB: pushRecent / toggleFavorite (sync write) DB-->>TUI: OK iOS->>SyncHost: modelPicker.setFavorites (WS sync cmd) SyncHost->>DB: requireModelPickerStore().setFavorites() DB-->>SyncHost: OK SyncHost-->>iOS: "{ favorites: [...] }" Desktop->>DB: CRR replication pull DB-->>Desktop: merged state Note over DB,iOS: CRR replication pushes<br/>changes back to iOSPrompt To Fix All With AI
Reviews (4): Last reviewed commit: "fix: address model picker review finding..." | Re-trigger Greptile