Skip to content

feat(providers): allow ordering accounts and selecting default models - #459

Open
lucas77778 wants to merge 3 commits into
masterfrom
chenyu/code-590
Open

feat(providers): allow ordering accounts and selecting default models#459
lucas77778 wants to merge 3 commits into
masterfrom
chenyu/code-590

Conversation

@lucas77778

Copy link
Copy Markdown
Member

Summary

  • add drag-and-drop priority ordering for provider accounts and persist the complete account order
  • update New Task defaults from the first compatible account's first model
  • allow users to promote a selected account model to the default position
  • add English/Chinese copy and focused tests

Linear: CODE-590

Verification

  • pnpm check:ci
  • pnpm test — 2966 passed, 1 skipped
  • exercised the mock Webview end-to-end and verified that dragging DeepSeek above Anthropic changes the New Task default from Claude Opus 5 to DeepSeek V4 Pro

Checklist

  • pnpm check:ci and pnpm test both pass
  • I ran the affected surface and observed the change working
  • No wire message changed
  • New code and assets are my own work
  • Docs and comments are updated where behavior changed

Copilot AI lite review requested due to automatic review settings August 19, 2026 04:29
@linear-code

linear-code Bot commented Aug 19, 2026

Copy link
Copy Markdown

CODE-590

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No blocking issues — a few rough edges around the persistence path and one coverage gap.

Reviewed changes — full diff of both commits (f82f5fda, b3aa8543) across all 7 files, plus the surrounding selection/persistence code the feature depends on.

  • Account drag orderingAccountList splits its row body into a new AccountRow that calls useSortable, and emits the full reordered id list through a new optional onReorder; canReorder gates on onReorder, reorderDisabled, and an empty search box.
  • Order persistencehandleReorder in providers-settings.tsx rehydrates accounts from ids, optimistically writes the SWR cache, then calls setAccounts, reverting on rejection.
  • Default modelModelSelection gains a per-model star that moves the picked model to index 0 of selected; rendered row order deliberately stays put, with the filled star as the only indicator.
  • Copy + tests — four new en/zh-cn keys, one new test file for AccountList, one new case for ModelSelection.

I verified the central claim the new copy makes, since the PR changes no selection logic: enabledAccountModels (packages/foundation/providers/src/enabled-models.ts:33-41) is a plain filter + flatMap over accounts then account.models with no sort anywhere, its docblock states "The first entry is the agent's default", and enabled-models.test.ts:22-32 asserts that reversing the pool changes the head. The daemon mirrors it in applyProviderDefaults. Both index-0 semantics this PR exposes are real, and no persisted preference overrides them — new-session-defaults-store.ts deliberately omits a model id. move(items, event), the sensor set (which does include KeyboardSensor), and the 1:1 order-preserving view model all check out.

ℹ️ The one path that can lose an ordering is the one without a test

The two new tests both target presentation: account-master-list.test.tsx mocks @dnd-kit/* and asserts that a drag end emits the full id list and that a filtered list disables the grips; model-selection.test.tsx asserts makeDefault moves the picked model to the head. Both can genuinely fail, and the first correctly pins that the unfiltered accounts array is what gets passed to move — good.

What is untested is handleReorder, which is where an ordering can actually be lost: the optimistic cache write, the setAccounts round-trip, and the rollback on rejection. Given that account order now determines the default model for every new thread, the rollback branch in particular is worth pinning down.

Technical details
# Add coverage for the reorder persistence path

## Affected sites
- `packages/client/workbench/src/settings/providers/providers-settings.tsx:81-94``handleReorder`; no test exercises it.
- `packages/client/workbench/src/settings/providers/__tests__/` — has `view.test.ts`, `add-flow.test.tsx`, `model-options.test.ts`, `model-selection.test.tsx`, but nothing covering the settings panel's mutation handlers.

## Required outcome
- A test that proves a successful drag persists the new order through `setAccounts` with the accounts in the emitted id order.
- A test that proves a rejected `setAccounts` restores the previously rendered order rather than leaving the optimistic one in place.

## Suggested approach (optional)
- The existing `add-flow.test.tsx` already establishes how this package mocks `../../runtime/tayori`; the same `useData` / `useMutation` mock shape works here, with `trigger` set to `vi.fn().mockRejectedValue(...)` for the rollback case.
- Alternatively, extract the id-list → `Accounts` rehydration into a pure helper next to `view.ts` and unit-test it directly, leaving only the SWR wiring in the component.

ℹ️ Nitpicks

  • packages/presentation/ui/src/shell/providers/account-master-list.tsx:15SIDEBAR_SORTABLE_SENSORS is now imported from ../sidebar into a providers surface, so the constant is shared by two unrelated lists while its name still describes only its first consumer. AGENTS.md asks that names describe responsibility rather than incidental origin; a neutral home (or name) would age better now that there is a second caller.
  • packages/presentation/ui/src/shell/__tests__/account-master-list.test.tsx:12-18 — the useTranslations mock returns the key and drops the values argument, so t('reorderAccount', { label: account.label }) asserts identically whether or not label is passed. The new {label} / {model} interpolations added to en.ts and zh-cn.ts are therefore unverified by these tests.
  • packages/client/workbench/src/settings/providers/model-selection.tsx:169disabled:opacity-100 exists to keep the filled default star at full strength, but it applies to every star, so while the form is busy a non-default star renders disabled yet fully opaque and still looks clickable. Scoping the override to isDefault would keep the busy-state affordance.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/presentation/ui/src/shell/providers/account-master-list.tsx Outdated
@lucas77778
lucas77778 requested a review from PeronGH August 19, 2026 05:19
Copilot AI review requested due to automatic review settings August 19, 2026 06:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the delta since my review of b3aa8543, i.e. commit a1840e5e, which addresses every point from that review.

  • Reorder persistence now reconciles and reportshandleReorder awaits a bare mutateAccounts() after a successful setAccounts, and the catch raises an error toast (settings.providers.reorderFailed + extractErrorMessage) alongside the existing revert.
  • orderHint gated on canReorder — the "drag to set priority" copy no longer shows while a non-empty search box has disabled every grip.
  • Added providers-settings.test.tsx — covers the two handleReorder branches: a successful drag persists the emitted order and revalidates, and a rejected save restores the previous order and toasts.
  • Scoped the default-star opacity overridedisabled:opacity-100 moved behind cn(..., isDefault && …) so only the always-disabled default star stays opaque while the form is busy, with a test pinning both halves.
  • Renamed and relocated the shared sensorsshell/sidebar/sortable-sensors.tsshell/sortable-sensors.ts, SIDEBAR_SORTABLE_SENSORSSORTABLE_SENSORS, dropped from the sidebar barrel; threads-view.tsx and account-master-list.tsx both import it by relative path.
  • Test translate mocks now interpolatepassthrough/translateKey join the values argument, so the new {label} / {model} placeholders are actually asserted (reorderAccount:Account A, models.makeDefault:Model B).

I checked the one thing the new revalidation depends on, since adding it was my own suggestion and a mis-ordered read would have shown up as rows visibly snapping back: read-after-write is ordered. The engine's config.set handler (packages/host/engine/src/agent/request-handler.ts:118-137) runs providers.update(...) and only then sendSuccess, and apps/daemon/src/provider-store.ts:24-29 mutates the in-memory pool synchronously alongside a synchronous saveProviderConfiguration — so the getAccounts refetch issued after the ack cannot observe the pre-write order.

The sensor move is clean rather than a partial rename: a repo-wide grep for SIDEBAR_SORTABLE_SENSORS and sortable-sensors returns only the two new relative imports, the old file is gone, the barrel export was removed, and the constant was never part of the @linkcode/ui public root barrel. tsc --build --noEmit over packages/presentation/ui and packages/client/workbench exits 0, and all 11 tests across the three touched test files pass. Both new handleReorder tests are genuinely falsifiable — the success case asserts mutateAccounts call 2 is the argument-less revalidate (not the pool-argument revert), so dropping await mutateAccounts() fails it, and the rollback case asserts the exact toast payload including description: 'disk full'.

One note for the record, not a request: the shortened SORTABLE_SENSORS doc comment drops the detail about PointerSensor's default preventActivation versus button-built handles, but that trap is already written down in .claude/rules/frontend.md, and AGENTS.md caps inline comments at 1–2 lines — so the one-liner is the compliant choice here.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@PeronGH PeronGH left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the ordering semantics against the contract rather than the diff alone: packages/foundation/providers/src/enabled-models.ts already defines the default as "each enabled account in pool order, contributing its picked set in its own order — the first entry is the agent's default", and the daemon reads the same thing (provider-config.ts, enabledAccounts(...)[0]). Both halves of this PR edit exactly those two orderings instead of introducing a stored default, which is the right shape.

Persistence path looks correct — optimistic mutateAccounts(..., { revalidate: false })trigger → bare mutateAccounts() reconcile, with revert + toast on failure. The pool captured in the catch is the pre-drag order, so the revert restores the right thing. dnd-kit usage avoids the known traps: useSortable results are destructured into plain identifiers before reaching ref=, and a flat single list correctly needs no onDragOver + preventDefault.

Non-blocking notes:

  • providers-settings.tsxif (reordered.length !== pool.length) return; is unreachable. providerAccountListViewModel is a 1:1 .map with no filtering and accountsById is built from the same pool, so the lengths can't diverge. A silent return there would also read to the user as the same unexplained snap-back that the error path now avoids.
  • account-master-list.tsx — the no-op guard (reordered.some((id, index) => id !== current[index])) is what stops a drag that lands where it started from writing to the daemon, and it's untested. With move already mocked, it's one mockReturnValueOnce. The event.canceled early return is uncovered too.
  • model-selection.test.tsx — "keeps only the disabled default marker fully opaque" asserts .className contains the literal disabled:opacity-100. It covers the isDefault && branch, but by restating the implementation's own string, and jsdom computes no Tailwind so the opacity itself isn't verified. The sibling test (default moves to head) is the one carrying the weight.
  • The default-model star is a disabled button used as a state indicator. Native disabled drops it from the tab order, so a keyboard user infers the default from the absence of a "Make X default" button rather than being told. aria-pressed on an enabled no-op would state it directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants