Repository navigation
Fix model menu - #2157
Fix model menu#2157
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe desktop model selector now renders all model families and their options in one submenu, removing family submenu state and positioning logic. Related tests were updated, and Tauri configuration arrays were reformatted without changing their values. ChangesDesktop model selector
Tauri configuration formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)wework/src-tauri/tauri.conf.jsonTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/components/chat/composer/ModelSelector.tsx (1)
315-330: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecompute model submenu layout when model content can change.
updateSubmenuLayoutonly re-runs whenactiveDesktopSubmenu,open, orupdateSubmenuLayoutchanges, so a models refresh while the submenu is already open keeps using the oldsubmenuHeight/submenuScrollHeight. Include the model-content values that affect the submenu size, or split the effect into one that watches that content separately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/chat/composer/ModelSelector.tsx` around lines 315 - 330, Update the useLayoutEffect that positions the model submenu so it also re-runs when model content affecting its dimensions changes, such as the model list or related loading/refresh state. Preserve the existing activeDesktopSubmenu, open, and updateSubmenuLayout handling while ensuring an open models submenu recalculates its height and scroll height after refreshes.
🧹 Nitpick comments (1)
wework/src/components/chat/composer/ModelSelector.test.tsx (1)
175-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of inline grouping; consider re-adding viewport-clamping coverage for the (now larger) unified submenu.
The removed test previously verified that the family submenu stays above the viewport bottom when content grows. Now that the "models" submenu inlines every family's model list (larger/variable content), that clamping behavior is exercised by the same
updateSubmenuLayoutcode path but is no longer covered by a test.Do you want me to draft a regression test that mocks
getBoundingClientRect/scrollHeightfor the multi-family submenu to confirm it stays clamped within the viewport?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/chat/composer/ModelSelector.test.tsx` around lines 175 - 196, Add regression coverage alongside the “shows models from every family in the second-level menu” test by mocking the unified submenu’s getBoundingClientRect and scrollHeight with content extending past the viewport. Assert the updateSubmenuLayout behavior clamps the submenu’s bottom position within the viewport, while preserving the existing multi-family model assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wework/src/components/chat/composer/ModelSelector.tsx`:
- Around line 315-330: Update the useLayoutEffect that positions the model
submenu so it also re-runs when model content affecting its dimensions changes,
such as the model list or related loading/refresh state. Preserve the existing
activeDesktopSubmenu, open, and updateSubmenuLayout handling while ensuring an
open models submenu recalculates its height and scroll height after refreshes.
---
Nitpick comments:
In `@wework/src/components/chat/composer/ModelSelector.test.tsx`:
- Around line 175-196: Add regression coverage alongside the “shows models from
every family in the second-level menu” test by mocking the unified submenu’s
getBoundingClientRect and scrollHeight with content extending past the viewport.
Assert the updateSubmenuLayout behavior clamps the submenu’s bottom position
within the viewport, while preserving the existing multi-family model
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8729f1ab-8500-426e-83ee-76d453452449
📒 Files selected for processing (4)
wework/src-tauri/tauri.conf.jsonwework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/composer/ModelSelector.test.tsxwework/src/components/chat/composer/ModelSelector.tsx
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores