Re: [PR] fix(ui): stop the default-model picker spinning on every switch [maka]

["Astro-Han (via GitHub)" <gi...@apache.org>]

Astro-Han commented on code in PR #3828:
URL: https://github.com/apache/maka/pull/3828#discussion_r3927053419


##########
packages/ui/src/use-optimistic-selection.ts:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useCallback, useEffect, useRef, useState } from 'react';
+
+export interface OptimisticSelection {
+  /**
+   * The value to render: the pending pick while its write is unconfirmed,
+   * otherwise the authoritative value.
+   */
+  value: string;
+  /**
+   * Show a pick immediately and arm the read barrier at `floorGeneration` — the
+   * read generation as of the pick, i.e. before the write starts. The pick is
+   * dropped once `committedGeneration` advances *past* this floor: any read
+   * issued after the pick (including a concurrent external write's read that
+   * lands mid-write) clears it, while a read already in flight at pick time
+   * (generation ≤ floor, returning the pre-write value) does not.
+   */
+  begin(next: string, floorGeneration: number): void;
+  /** Drop the pending pick — e.g. the write threw — falling back to the authoritative value. */
+  cancel(): void;
+}
+
+/**
+ * Reflect a just-picked value instantly while its write is in flight, then
+ * defer to the authoritative value once a read issued after the pick lands —
+ * without spinning, stranding a stale value, or reverting prematurely.
+ *
+ * The clearing signal is a monotonic read GENERATION captured at the pick (the
+ * write's start), not a value compare and not a snapshot-reference change.
+ * Ordering the barrier at the write's start — rather than at "the latest
+ * generation observed after the write's Promise resolves" — is what makes it
+ * correct: a read issued after the pick that observed a newer authority (this
+ * write, or a concurrent external write) has generation > floor and clears the
+ * pick even if the caller's own explicit refresh never lands; and a read
+ * already in flight at pick time (generation ≤ floor) can only carry the
+ * pre-write value, so it never clears.
+ *
+ * Consequences, all correct: an in-flight pre-write read never clears the pick;
+ * the pick converges to whatever an after-the-pick read reports — this pick, a
+ * concurrent external write, or the prior value restored (A→B→A); a refresh
+ * that lands no after-the-pick read leaves `committedGeneration` at/under the
+ * floor, so the pick is kept (the write already persisted it). `cancel` covers
+ * a thrown write.
+ */
+export function useOptimisticSelection(
+  authoritative: string,
+  committedGeneration: number,
+): OptimisticSelection {
+  const [pending, setPending] = useState<string | null>(null);
+  // Read-generation floor captured at the pick; null means no pick is pending.
+  const floorRef = useRef<number | null>(null);
+
+  useEffect(() => {
+    if (
+      pending !== null &&
+      floorRef.current !== null &&
+      committedGeneration > floorRef.current

Review Comment:
   [P2 · category ②] Carrying this over unchanged, since the head is a pure rebase and the last round's version of this went unanswered.
   
   `committedGeneration > floorRef.current` proves the read was issued after the pick. It does not prove the read observed this write. `settings-surface.tsx` subscribes `connectionsBridge.subscribeEvents(() => reloadConnections(...))` for *any* connection event, not just a default-model change. So while `setDefaultModel(B)` is in flight, an unrelated connection event can issue a read, have it accepted at a generation past the floor, and commit the still-authoritative A. This clears pending B and the row falls back to A. If the write then succeeds but the explicit `onRefresh()` fails or is judged stale (`reloadConnections` swallows the failure, it does not throw), no further accepted read corrects A to B, and the row sits on A while durable authority is B until the next accepted connections read.
   
   Bounded and self-healing, and it corrupts nothing, which is why this is P2 and not higher.
   
   The reason I do not think this is an implementation slip: `use-optimistic-selection.test.tsx:155` pins "pick B, then read A at generation 6, show A" as the correct A→B→A behavior. That sequence and the failure sequence above are the same input to this hook. Moving the floor to the write's completion is the shape we already rejected upstream, and adding a value compare brings back the other half of it. Generation alone cannot separate the two.
   
   Two ways to close it, your call:
   
   1. Give the write its own authority. `connections.setDefaultModel` returns `Promise<void>` today (`apps/desktop/src/preload/bridge-contract.d.ts`). If it returned the post-write `defaultConnection` (or a Host revision), the row could compare against authority directly and this whole generation barrier could be deleted. That is the authoritative fix and it is a net subtraction.
   2. If (1) is out of scope for this PR: leave the behavior as is, but say so in the hook's doc comment and on the A→B→A case, so "a refresh that lands no after-the-pick read keeps the pick" does not read as covering every failure path when it does not cover this one.
   
   Please push back if every post-pick connections read is in fact serialized behind this mutation. I could not find that guarantee in the bridge or the request authority.



##########
apps/desktop/src/renderer/settings/general-settings-page.tsx:
##########
@@ -294,6 +298,7 @@ export function GeneralSettingsPage(props: {
       </SettingsSection>
       {showRuntimeHostDefaults ? (
         <GeneralDefaultsCard
+          key={props.runtimeHostGenerationKey}

Review Comment:
   [P3] The synchronous `targetRevision` fence below already covers the pre-unmount window, so this remount is no longer what keeps a superseded save from acting. Its remaining job is narrower: dropping a pending optimistic pick stranded in `@maka/ui` when a fenced save returns early and no read is ever accepted. That is worth keeping, but it is not written down, so the next reader will see two guards for one hazard and delete this one. One line of comment here naming what the remount still owns would prevent that.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org