Conversation
Greptile SummaryThis PR fixes keyboard navigation in the freebuff model picker: arrow keys now skip unavailable models, Enter falls back to the last valid selection when focus is on a closed row, and a generation counter prevents stale landing-restart fetches from overwriting newer queue state. The logic is cleanly extracted into a new Confidence Score: 5/5Safe to merge; all remaining findings are minor style suggestions with no impact on correctness. All bugs described in the PR are correctly fixed. The two inline comments are P2 style/documentation suggestions that do not affect runtime behaviour. cli/src/utils/freebuff-model-navigation.ts — minor simplification opportunity in resolveFreebuffModelCommitTarget.
|
| Filename | Overview |
|---|---|
| cli/src/utils/freebuff-model-navigation.ts | New utility module extracting navigation logic; minor simplification opportunity in resolveFreebuffModelCommitTarget (redundant null-return guard). |
| cli/src/components/freebuff-model-selector.tsx | Keyboard handler correctly delegates to the two new utility functions; dependency array correctly updated to include selectedModel. |
| cli/src/hooks/use-freebuff-session.ts | Generation counter cleanly prevents stale landing-restart fetches from overwriting newer queue state; nextMethod reset to GET also guards against left-over POST from a prior restart. |
| cli/src/components/waiting-room-screen.tsx | One-line fix keeps picker error message visible when session is in the 'none' (landing) state; straightforward and correct. |
| cli/src/utils/tests/freebuff-model-navigation.test.ts | Good coverage of forward/backward skip, wrap-around, and commit-target fallback; the case where both focusedId and selectedId are unavailable is not exercised but that is a minor gap. |
Prompt To Fix All With AI
This 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.
---
This is a comment left during a code review.
Path: cli/src/utils/freebuff-model-navigation.ts
Line: 14-19
Comment:
**Loop can return the current focused model**
When all peer models are unavailable the loop reaches `offset === modelIds.length`, which wraps back to `currentIdx` and returns the focused model itself. The tests confirm this is intentional (pressing an arrow key stays put), but it means `setFocusedId(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.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Fix freebuff model picker enter" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const targetId = isSelectable(focusedId) ? focusedId : selectedId | ||
|
|
||
| if (targetId === committedId) return null | ||
| if (!isSelectable(targetId)) return null | ||
| return targetId |
There was a problem hiding this comment.
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.
| const targetId = isSelectable(focusedId) ? focusedId : selectedId | |
| if (targetId === committedId) return null | |
| if (!isSelectable(targetId)) return null | |
| return targetId | |
| if (!isSelectable(targetId) || targetId === committedId) return null | |
| return targetId |
Prompt To Fix With AI
This 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!
| 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 | ||
| } |
There was a problem hiding this comment.
When all peer models are unavailable the loop reaches offset === modelIds.length, which wraps back to currentIdx and returns the focused model itself. The tests confirm this is intentional (pressing an arrow key stays put), but it means setFocusedId(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
This is a comment left during a code review.
Path: cli/src/utils/freebuff-model-navigation.ts
Line: 14-19
Comment:
**Loop can return the current focused model**
When all peer models are unavailable the loop reaches `offset === modelIds.length`, which wraps back to `currentIdx` and returns the focused model itself. The tests confirm this is intentional (pressing an arrow key stays put), but it means `setFocusedId(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.
How can I resolve this? If you propose a fix, please make it concise.2 participants
Fixes the freebuff model picker getting stuck when arrow keys move focus onto an unavailable model such as GLM outside deployment hours. Keyboard navigation now skips unavailable models, and Enter falls back to the selected available model if focus is stale or closed. The waiting-room screen also keeps picker errors visible, and landing restarts ignore stale queue-depth fetches so older checks cannot overwrite newer queue state. Validated with
bun test cli/src/utils/__tests__/freebuff-model-navigation.test.tsandbun test cli/src/utils/__tests__/keyboard-actions.test.ts.