ui: tweak vertical space for mobile #343
Conversation
WalkthroughAdds responsive behavior using a new isNarrow flag from Theme context. Header adjusts padding/height based on isNarrow. Models page introduces a narrow-screen menu (RiMenuFill) to access existing actions (toggle ID/Name, show/hide unlisted, unload all), manages menu state, and centralizes unload-all handling with temporary disabling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ModelsPage as Models Page
participant Menu as Narrow Menu
participant ModelsPanel as Models Panel
participant ModelSvc as Model Service
User->>ModelsPage: Navigate
ModelsPage->>ModelsPage: Detect isNarrow via Theme
alt Wide
ModelsPage->>ModelsPanel: Render inline controls
else Narrow
ModelsPage->>Menu: Render menu button (RiMenuFill)
User->>Menu: Toggle menu open/close
Menu->>ModelsPanel: Invoke actions (Toggle ID/Name, Show/Hide Unlisted)
Menu->>ModelsPage: Unload All clicked
end
par Unload All
ModelsPage->>ModelsPage: Set isUnloading=true (disable controls)
ModelsPage->>ModelSvc: unloadAllModels()
ModelSvc-->>ModelsPage: Resolve
ModelsPage->>ModelsPage: Brief delay, set isUnloading=false
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/pages/Models.tsx (2)
64-66: Fix the dependency array.The
toggleIdorNamecallback includesshowIdorNamein its dependency array, but the function doesn't actually read this value—it uses the updater form ofsetShowIdorName. This causes unnecessary re-creation of the callback on every state change.Apply this diff:
const toggleIdorName = useCallback(() => { setShowIdorName((prev) => (prev === "name" ? "id" : "name")); -}, [showIdorName]); +}, [setShowIdorName]);
51-62: Prevent potential memory leak from timeout.If the component unmounts while the timeout is pending, calling
setIsUnloading(false)will trigger a React warning about setting state on an unmounted component.Consider tracking the timeout and clearing it on unmount:
const handleUnloadAllModels = useCallback(async () => { setIsUnloading(true); try { await unloadAllModels(); } catch (e) { console.error(e); } finally { - setTimeout(() => { + const timeoutId = setTimeout(() => { setIsUnloading(false); }, 1000); + return () => clearTimeout(timeoutId); } }, [unloadAllModels]);Alternatively, use a ref to track mount status or leverage a cleanup effect in the component body.
🧹 Nitpick comments (2)
ui/src/pages/Models.tsx (2)
73-112: Consider adding click-outside behavior to close the menu.The menu currently only closes when an action is selected or the menu button is clicked again. Users might expect the menu to close when clicking outside of it, which is a common UX pattern for dropdown menus.
You can add a click-outside handler using
useEffectanduseRef, or consider using a library likereact-usethat provides auseClickAwayhook.
75-77: Add accessibility attributes to the menu button.The menu button lacks appropriate ARIA attributes to indicate its purpose and state to screen reader users.
Apply this diff:
-<button className="btn text-base flex items-center gap-2 py-1" onClick={() => setMenuOpen(!menuOpen)}> +<button + className="btn text-base flex items-center gap-2 py-1" + onClick={() => setMenuOpen(!menuOpen)} + aria-label="Open menu" + aria-expanded={menuOpen} + aria-haspopup="true" +> <RiMenuFill size="20" /> </button>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/src/components/Header.tsx(2 hunks)ui/src/pages/Models.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ui/src/components/Header.tsx (1)
ui/src/contexts/ThemeProvider.tsx (1)
useTheme(91-97)
ui/src/pages/Models.tsx (2)
ui/src/contexts/ThemeProvider.tsx (1)
useTheme(91-97)ui/src/hooks/usePersistentState.ts (1)
usePersistentState(3-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (1)
ui/src/components/Header.tsx (1)
8-8: LGTM! Clean responsive header implementation.The
isNarrowflag is correctly used to apply a more compact header layout on mobile devices, reducing vertical space as intended by the PR.Also applies to: 20-20
Tweak the UI to waste less vertical space on mobile.
Before and after:
Summary by CodeRabbit
New Features
Style