-
Notifications
You must be signed in to change notification settings - Fork 519
[codex] Fix freebuff model picker enter #545
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import { describe, expect, test } from 'bun:test' | ||
|
|
||
| import { | ||
| nextSelectableFreebuffModelId, | ||
| resolveFreebuffModelCommitTarget, | ||
| } from '../freebuff-model-navigation' | ||
|
|
||
| describe('nextSelectableFreebuffModelId', () => { | ||
| test('skips unavailable models when moving forward', () => { | ||
| const modelIds = ['glm', 'minimax'] | ||
|
|
||
| expect( | ||
| nextSelectableFreebuffModelId({ | ||
| modelIds, | ||
| focusedId: 'minimax', | ||
| direction: 'forward', | ||
| isSelectable: (id) => id !== 'glm', | ||
| }), | ||
| ).toBe('minimax') | ||
| }) | ||
|
|
||
| test('skips unavailable models when moving backward', () => { | ||
| const modelIds = ['glm', 'minimax'] | ||
|
|
||
| expect( | ||
| nextSelectableFreebuffModelId({ | ||
| modelIds, | ||
| focusedId: 'minimax', | ||
| direction: 'backward', | ||
| isSelectable: (id) => id !== 'glm', | ||
| }), | ||
| ).toBe('minimax') | ||
| }) | ||
|
|
||
| test('moves to the next available model when more than one is selectable', () => { | ||
| const modelIds = ['glm', 'minimax', 'other'] | ||
|
|
||
| expect( | ||
| nextSelectableFreebuffModelId({ | ||
| modelIds, | ||
| focusedId: 'minimax', | ||
| direction: 'forward', | ||
| isSelectable: (id) => id !== 'glm', | ||
| }), | ||
| ).toBe('other') | ||
| }) | ||
|
|
||
| test('returns null when no selectable model exists', () => { | ||
| expect( | ||
| nextSelectableFreebuffModelId({ | ||
| modelIds: ['glm'], | ||
| focusedId: 'glm', | ||
| direction: 'forward', | ||
| isSelectable: () => false, | ||
| }), | ||
| ).toBeNull() | ||
| }) | ||
| }) | ||
|
|
||
| describe('resolveFreebuffModelCommitTarget', () => { | ||
| test('falls back to the selected model when focus is on a closed model', () => { | ||
| expect( | ||
| resolveFreebuffModelCommitTarget({ | ||
| focusedId: 'glm', | ||
| selectedId: 'minimax', | ||
| committedId: null, | ||
| isSelectable: (id) => id !== 'glm', | ||
| }), | ||
| ).toBe('minimax') | ||
| }) | ||
|
|
||
| test('commits the focused model when it is selectable', () => { | ||
| expect( | ||
| resolveFreebuffModelCommitTarget({ | ||
| focusedId: 'minimax', | ||
| selectedId: 'glm', | ||
| committedId: null, | ||
| isSelectable: (id) => id === 'minimax', | ||
| }), | ||
| ).toBe('minimax') | ||
| }) | ||
|
|
||
| test('returns null when the target is already committed', () => { | ||
| expect( | ||
| resolveFreebuffModelCommitTarget({ | ||
| focusedId: 'minimax', | ||
| selectedId: 'minimax', | ||
| committedId: 'minimax', | ||
| isSelectable: () => true, | ||
| }), | ||
| ).toBeNull() | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||||||||||||
| export function nextSelectableFreebuffModelId(params: { | ||||||||||||||||
| modelIds: readonly string[] | ||||||||||||||||
| focusedId: string | ||||||||||||||||
| direction: 'forward' | 'backward' | ||||||||||||||||
| isSelectable: (modelId: string) => boolean | ||||||||||||||||
| }): string | null { | ||||||||||||||||
| const { modelIds, focusedId, direction, isSelectable } = params | ||||||||||||||||
| if (modelIds.length === 0) return null | ||||||||||||||||
|
|
||||||||||||||||
| const currentIdx = modelIds.indexOf(focusedId) | ||||||||||||||||
| if (currentIdx === -1) return null | ||||||||||||||||
|
|
||||||||||||||||
| const step = direction === 'forward' ? 1 : -1 | ||||||||||||||||
| // Include a full wrap back to the current item so arrows stay on the same | ||||||||||||||||
| // selectable model when every peer is unavailable. | ||||||||||||||||
| for (let offset = 1; offset <= modelIds.length; offset++) { | ||||||||||||||||
| const idx = | ||||||||||||||||
| (currentIdx + step * offset + modelIds.length) % modelIds.length | ||||||||||||||||
| const candidate = modelIds[idx] | ||||||||||||||||
| if (isSelectable(candidate)) return candidate | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return null | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| export function resolveFreebuffModelCommitTarget(params: { | ||||||||||||||||
| focusedId: string | ||||||||||||||||
| selectedId: string | ||||||||||||||||
| committedId: string | null | ||||||||||||||||
| isSelectable: (modelId: string) => boolean | ||||||||||||||||
| }): string | null { | ||||||||||||||||
| const { focusedId, selectedId, committedId, isSelectable } = params | ||||||||||||||||
| const targetId = isSelectable(focusedId) ? focusedId : selectedId | ||||||||||||||||
|
|
||||||||||||||||
| if (!isSelectable(targetId) || targetId === committedId) return null | ||||||||||||||||
| return targetId | ||||||||||||||||
|
Comment on lines
+33
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The two early-return conditions can be collapsed —
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: cli/src/utils/freebuff-model-navigation.ts
Line: 31-35
Comment:
**Merge two null-return guards into one**
The two early-return conditions can be collapsed — `!isSelectable(targetId)` only ever fires when `focusedId` is unavailable *and* `selectedId` is also unavailable (because if `isSelectable(focusedId)` were true, `targetId` would be `focusedId` and already known-selectable). Combining them makes the intent clearer with fewer lines.
```suggestion
if (!isSelectable(targetId) || targetId === committedId) return null
return targetId
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||
| } | ||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When all peer models are unavailable the loop reaches
offset === modelIds.length, which wraps back tocurrentIdxand returns the focused model itself. The tests confirm this is intentional (pressing an arrow key stays put), but it meanssetFocusedId(targetId)is called with the same value that is already focused — a harmless no-op React state update. Consider documenting this wrap-around behaviour in a comment so future readers don't assume the function always advances focus.Prompt To Fix With AI