Skip to content

fix(ui): guide legacy session account recovery - #4102

Open
me2seeks wants to merge 6 commits into
apache:mainfrom
me2seeks:fix/legacy-session-account-guidance
Open

fix(ui): guide legacy session account recovery#4102
me2seeks wants to merge 6 commits into
apache:mainfrom
me2seeks:fix/legacy-session-account-guidance

Conversation

@me2seeks

Copy link
Copy Markdown
Contributor

Part of #3852.

Summary

  • make Desktop legacy/deleted/mismatched account notices open the existing exact account-and-model picker
  • reload the account catalog instead of opening an empty picker, disable recovery while model switching is locked, and point credential failures at Settings · Models
  • tell TUI users to run /model, with an explicit no-choice fallback (/setup only for API-key connections)

The selected path remains the existing { connectionId, connectionSlug, model } mutation. This does not auto-bind legacy Sessions and does not add a Runtime Host operation or compatibility-epoch change.

Validation

  • npm run typecheck --workspace @maka/ui
  • npm run typecheck --workspace @maka/desktop
  • npm run typecheck --workspace maka-agent
  • npm run test:dist --workspace @maka/ui (248 passed)
  • Desktop focused tests (18 passed)
  • TUI legacy/deleted-account focused tests (2 passed)
  • Biome lint, git diff --check, ASF header audit

Review

Two independent pre-PR reviews passed after checking snapshot loading/failure, mid-turn locking, credential recovery copy, stale picker rows, and exact Connection identity selection.

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 28, 2026
@Astro-Han

Copy link
Copy Markdown
Contributor

This is a synthesis of the independent blind review by @Sol-404ARE at exact head f15e7e4dbe8dc9df6a6e8d7bbaa7ced7ec6d6a86 (base 1975adb35caaa1dc8cd33b0bad75e34dcc90f592, 14 files +421/-40). I verified the diff and the exact-head CI myself; the file:line findings below are from Sol's sealed review.

What I checked myself:

  • Read gh pr diff 4102 (fix: guide legacy session account recovery, 14 files) and confirmed the recovery routing through use-shell-chat-model.ts:231-255, session-health-notice.ts:125-143, and the picker freshness at use-shell-connections.ts:92-129 / chat-model-switcher.tsx:245-249,282-303.
  • Checked exact-head CI: test SUCCESS, label SUCCESS, OPEN/MERGEABLE/BLOCKED/REVIEW_REQUIRED.

Findings from Sol's review (file:line anchored):

Standards — NO-GO — 3×P2 + 1×P3 (worst P2)

  1. P2 — Required PR template was replacedCONTRIBUTING.md:81-87 requires the template's AI-selection/tool-scope (:25-36), checklist (:38-41), and behavior selector (:43-46); the body omits all three. This is counted as one systemic hygiene finding.

  2. P2 — Required UI evidence is absent — visible CTA/picker behavior changes at chat-message-surface.tsx:290-305, app-shell.tsx:760-763,2944-2949, and composer.tsx:1968-1981 have no before/after screenshot or recording, violating CONTRIBUTING.md:87.

  3. P2 — No composed oracle for notice CTA → hook routing → AppShell ref adapter → controlled Composer picker — renderer/UI standards at renderer/README.md:78-83 and ui/README.md:58-64 require a focused behavioral contract. Existing tests separately check pure target derivation and direct imperative picker opening, so deleting the Banner click binding, the hook routing, or the AppShell adapter can leave them green. Add a composed CTA → picker/reload/settings contract plus the live-turn-disabled case.

  4. P3 — Judgment-only Duplicated Code / authority driftapp-shell.tsx:890-896 reconstructs switch safety while composer.tsx:1359-1378,1977-1978,2014-2016 separately derives lock/reason and pending state; prefer a single availability witness/reason contract. Other Fowler concerns were withdrawn; entropy rises slightly from the duplicated switch-lock authority.

No other Standards finding; EN/zh and architecture boundaries pass.

Spec — NO-GO — 1×P2 (worst P2)

  • P2 — A failed catalog refresh can leave the deleted account as a normal checked recovery choice whose selection is a no-opuse-shell-connections.ts:92-129,140-145 retains the prior snapshot after getSnapshot fails and still reports hasSnapshot=true. use-shell-chat-model.ts:231-255 then treats the choice list as settled/non-empty, and session-health-notice.ts:125-143 routes straight to the picker. In chat-model-switcher.tsx:245-249,282-303 the stale exact choice A remains checked; selecting it returns early at :293-297 and the Host performs no validation/mutation. Repro: load Session A with a ready choice, delete A, make the connection_list_changed refresh fail, then observe connection_missing; clicking the CTA and selecting the still-checked A is a no-op and the Session stays blocked. The existing test at composer-model-picker-recovery.test.tsx:64-111 only uses a fresh replacement plus a legacy Session without id, so this stale-exact-A + failing-refresh case is missed. Invalidate or visibly disable stale choices and force a refresh/revalidation before accepting the current exact choice.

Spec exclusions that were verified as green: TUI correctly limits /setup to API-key-only; /model persists exact {model,slug,id}; Desktop Host mutation is exact; switch locks/fresh picker and duplicate-name disambiguation pass; no stale missing-llmConnectionId memo bug.

Verification: git diff --check PASS, targeted Biome 14/14 PASS, ASF headers 14/14 PASS, worktree clean, head unchanged, test/label SUCCESS.

What I did not judge: a true picker refresh-failure → stale-choice → no-op E2E with a real Host/catalog was not executed beyond race-inspection — verification was by code inspection and the tests noted above.

Gate: Standards 3×P2 and Spec 1×P2 remain; despite test/label green, head f15e7e4 is not merge-ready as “guide legacy session account recovery” until the stale-choice invalidation and the UI evidence/template are closed. Seal: notes/pr-4102-provisional.md.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@Astro-Han

Copy link
Copy Markdown
Contributor

This is a synthesis of the feature-gate audit by @feature杀手 at exact head 775f39b03e3dfd80c87dead8ef2542b3e03f6bb2 (increment f15e7e4 → 775f39b). I verified the drift and the increment myself; the file:line findings below are from the auditor's sealed report.

What I checked myself:

  • Compared f15e7e4 → 775f39b (delta for the new repair, ~+166 lines plus 775f39b guard) and confirmed the hidden-Composer picker path at app-shell.tsx:760-763,890-896,2871-2873 / composer.tsx:1572-1575 and the side-conversation ID check at use-quote-companion.ts:475-480,536-543 / session-manager.ts:4981-4989.
  • Checked head state: 775f39b03e3d80c87dead8ef2542b3e03f6bb2 is now the remote head (drift from f15e7e4), test pending at last check, OPEN/MERGEABLE/BLOCKED/REVIEW_REQUIRED.

Findings from the feature audit (file:line anchored, notes/pr-4102-feature-audit.md):

Overall — Necessity GO, existing Host authority design GO; CODE / GLOBAL-DESIGN NO-GO — not merge-ready.

  • Blocker 1 — The recovery button is clickable while the Composer that owns the picker is hidden. app-shell.tsx:890-896 computes modelPickerDisabled without localInteractionAvailable, but :2871-2873 hides the Composer when localInteractionAvailable=false. Clicking still reaches :760-763 and sets the menu state to open, while the menu lives inside the native hidden form at composer.tsx:1572-1575. The exact-head probe shows actionDisabled=false / formHidden=true / menuOpenedInsideHiddenForm=true — the user sees “Select connection and model,” clicks, and nothing visible happens; the pre-opened menu can later pop out when the boundary restores. The picker availability should be a single shared fact that gates both the banner CTA and the Composer — hide means disabled/rejected, without adding a new recovery state.

  • Blocker 2 — The new side-conversation defense only looks at llmConnectionId presence. use-quote-companion.ts:475-480,536-543 only blocks legacy null-ID; a deleted Connection or an ID/slug mismatch still carries a non-empty ID and will immediately fork. The Runtime then copies the old ID/slug/model at session-manager.ts:4981-4989, so a child forked from a stale source is not repaired when the source later recovers, and the side conversation has no independent picker. The probe proves deleted-connection-id is forked once and retained. New tests only cover undefined-ID. The fork/initial-send should reuse the existing Host/catalog exact source-configuration readiness check and cover at least legacy_connection_identity, connection_missing, and connection_identity_mismatch before forking, without building a second side-chat identity state machine.

Globally it is correct not to split the PR and not to add a new Runtime Host operation/wire/epoch/durable migration: the existing exact {connectionId, connectionSlug, model} mutation is the single correct write authority, and the new ChatRecoveryNotice does converge three previously duplicated Banner layouts. The right simplification is to converge the two partial readiness predicates above. After code closure the PR body should also be updated to the true 3 commits / 20 files (currently missing the side-conversation defer and notice convergence, and “two pre-PR reviews” does not describe the new head).

Verification: Desktop 1669/1669, UI 248/248, TUI focused 2/2, three-workspace typecheck, 20-file Biome and diff-check all PASS; two adversarial probes stably reproduce the gaps. The sole CLI failure #4011 was isolated and passed on retry. Remote test was pending at last recheck. No GitHub writes were made prose.

Gate: two P-level blockers remain on the increment f15e7e4 → 775f39b; despite the prior head f15e7e4 having been CODE GO, the current head is not merge-ready until the hidden-picker and side-conversation readiness predicates are converged. Prior head comment 5455485290 applies to f15e7e4 only and must not be extrapolated.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

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

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants