-
Notifications
You must be signed in to change notification settings - Fork 457
fix(ui): stop the default-model picker spinning on every switch #3828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
liuxiaocs7
wants to merge
1
commit into
apache:main
Choose a base branch
from
liuxiaocs7:fix/default-model-picker-spinner
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+230
−31
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
141 changes: 141 additions & 0 deletions
141
packages/ui/src/__tests__/use-pending-selection.test.tsx
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| /* | ||
| * 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 assert from 'node:assert/strict'; | ||
| import test from 'node:test'; | ||
| import { act, createElement } from 'react'; | ||
| import { createRoot } from 'react-dom/client'; | ||
| import { parseHTML } from 'linkedom'; | ||
| import { usePendingSelection } from '../use-pending-selection.js'; | ||
|
|
||
| interface Deferred { | ||
| resolve(): void; | ||
| reject(): void; | ||
| } | ||
|
|
||
| interface Harness { | ||
| value(): string; | ||
| pick(next: string): Promise<void>; | ||
| render(authoritative: string): Promise<void>; | ||
| settle(index?: number): Promise<void>; | ||
| reject(index?: number): Promise<void>; | ||
| } | ||
|
|
||
| const flush = () => new Promise((r) => setTimeout(r, 0)); | ||
|
|
||
| async function mount(initial: string): Promise<Harness> { | ||
| const { document, window } = parseHTML('<div id="root"></div>'); | ||
| Object.assign(globalThis, { | ||
| document, | ||
| window, | ||
| IS_REACT_ACT_ENVIRONMENT: true, | ||
| }); | ||
| const container = document.querySelector('#root'); | ||
| assert.ok(container); | ||
|
|
||
| const writes: Deferred[] = []; | ||
| let handle: { value: string; onChange: (n: string) => void } | null = null; | ||
| const onValueChange = (_next: string) => | ||
| new Promise<void>((resolve, reject) => { | ||
| writes.push({ resolve, reject }); | ||
| }); | ||
|
|
||
| function Host({ authoritative }: { authoritative: string }) { | ||
| handle = usePendingSelection(authoritative, onValueChange); | ||
| return null; | ||
| } | ||
|
|
||
| const root = createRoot(container as unknown as Element); | ||
| await act(() => { | ||
| root.render(createElement(Host, { authoritative: initial })); | ||
| }); | ||
|
|
||
| return { | ||
| value: () => handle!.value, | ||
| pick: async (next) => { | ||
| await act(async () => { | ||
| handle!.onChange(next); | ||
| await flush(); | ||
| }); | ||
| }, | ||
| render: async (authoritative) => { | ||
| await act(() => { | ||
| root.render(createElement(Host, { authoritative })); | ||
| }); | ||
| }, | ||
| settle: async (index = writes.length - 1) => { | ||
| await act(async () => { | ||
| writes[index]!.resolve(); | ||
| await flush(); | ||
| }); | ||
| }, | ||
| reject: async (index = writes.length - 1) => { | ||
| await act(async () => { | ||
| writes[index]!.reject(); | ||
| await flush(); | ||
| }); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| test('a pick shows immediately, before the write settles', async () => { | ||
| const h = await mount('A'); | ||
| assert.equal(h.value(), 'A'); | ||
| await h.pick('B'); | ||
| assert.equal(h.value(), 'B'); | ||
| }); | ||
|
|
||
| test('the pick clears to authority once the write resolves and value catches up', async () => { | ||
| const h = await mount('A'); | ||
| await h.pick('B'); | ||
| await h.render('B'); // caller's refresh lands the new authority | ||
| await h.settle(); | ||
| assert.equal(h.value(), 'B'); | ||
| }); | ||
|
|
||
| test('a rejected write rolls back to the authoritative value', async () => { | ||
| const h = await mount('A'); | ||
| await h.pick('B'); | ||
| assert.equal(h.value(), 'B'); | ||
| await h.reject(); | ||
| assert.equal(h.value(), 'A'); | ||
| }); | ||
|
|
||
| test('the pick holds across an unrelated authority change while the write is in flight', async () => { | ||
| const h = await mount('A'); | ||
| await h.pick('B'); | ||
| // An unrelated event pushes a different authoritative value mid-write; the | ||
| // user's pick still shows until their own write settles. | ||
| await h.render('C'); | ||
| assert.equal(h.value(), 'B'); | ||
| await h.settle(); | ||
| assert.equal(h.value(), 'C'); | ||
| }); | ||
|
|
||
| test('latest pick wins: a slower earlier write settling does not wipe a newer pick', async () => { | ||
| const h = await mount('A'); | ||
| await h.pick('B'); // write #0 | ||
| await h.pick('C'); // write #1 (newer) | ||
| assert.equal(h.value(), 'C'); | ||
| await h.settle(0); // the older B write resolves late | ||
| assert.equal(h.value(), 'C'); // still C, not cleared | ||
| await h.render('C'); | ||
| await h.settle(1); | ||
| assert.equal(h.value(), 'C'); | ||
| }); |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| /* | ||
| * 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, useRef, useState } from 'react'; | ||
|
|
||
| export interface PendingSelection { | ||
| /** | ||
| * The value to render: the just-picked value while its write is unsettled, | ||
| * otherwise the authoritative value. | ||
| */ | ||
| value: string; | ||
| /** | ||
| * Show `next` at once and fire the write; the pick clears when that write | ||
| * settles — by when the authoritative `value` has caught up on success, or | ||
| * falling back to it on failure. | ||
| */ | ||
| onChange(next: string): void; | ||
| } | ||
|
|
||
| /** | ||
| * Reflect a just-picked value immediately and hold it until the caller's write | ||
| * settles, then defer to the authoritative `value`. No spinner and no lag: the | ||
| * pick shows from the click and clears the moment `onValueChange` resolves (by | ||
| * when `value` has caught up) or rejects (rolling back to `value`). | ||
| * | ||
| * A monotonic token makes the latest pick win, so a slower earlier write's | ||
| * settle cannot wipe a newer pick. The state is deliberately local and | ||
| * write-scoped: it carries no cross-read generation and no state that outlives | ||
| * the component, so reopening the surface always starts clean. | ||
| * | ||
| * Limitation: the pick clears when the write settles, not when `authoritative` | ||
| * is confirmed to carry it — so if the write resolves but the caller never | ||
| * lands the new value into `authoritative` (e.g. its refresh is silently | ||
| * dropped), the trigger falls back to the prior `authoritative` until the | ||
| * caller next updates it, self-healing on that next update and never wrong | ||
| * durably. | ||
| */ | ||
| export function usePendingSelection( | ||
| authoritative: string, | ||
| onValueChange: (next: string) => void | Promise<void>, | ||
| ): PendingSelection { | ||
| const [pending, setPending] = useState<string | null>(null); | ||
| const tokenRef = useRef(0); | ||
| const onChange = useCallback( | ||
| (next: string) => { | ||
| const token = (tokenRef.current += 1); | ||
| setPending(next); | ||
| // Clear on either outcome — success (authoritative caught up) or failure | ||
| // (roll back to authoritative) — and only if this is still the latest | ||
| // pick. Two-arg `then` (not `finally`) so a rejected write is consumed | ||
| // here rather than surfacing as an unhandled rejection. | ||
| const settle = () => { | ||
| if (tokenRef.current === token) setPending(null); | ||
| }; | ||
| Promise.resolve(onValueChange(next)).then(settle, settle); | ||
| }, | ||
| [onValueChange], | ||
| ); | ||
| return { value: pending ?? authoritative, onChange }; | ||
| } |
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
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.
Uh oh!
There was an error while loading. Please reload this page.