Fix/UI theme bugs 40 41#43
Conversation
…base/application Groups results by server when viewing all instances. Each server section is collapsible. Keeps existing database/application filter dropdowns for filtering within a selected server. Closes #32
…etter details - Drive selector dropdown to filter by specific drive letter/name - Instance filter dropdown in estate view (no instance param) - Active filter badges with clear button - Used/Free/Total three-column detail grid per drive card - Critical (>85%) and Warning (>70%) counts in header - Percentage shown on each drive card - Drive label shown when available Closes #31 (partial — latency requires backend changes)
…base/application Groups results by server when viewing all instances. Each server section is collapsible. Keeps existing database/application filter dropdowns for filtering within a selected server. Closes #32
Show an empty placeholder instead of loading all instances at once. The page loads instantly with a 'Select an instance' prompt, and only fetches data once a server is chosen. Addresses feedback from @edwillis777 (#28): - Memory: was taking 34 seconds to load - Blocking: was taking 1m 30s to load Closes #29 (partial — Performance Summary needs separate attention)
feat(drives): add drive selector, instance filter, and detailed stats
…ver-drilldown feat(slow-queries): add server grouping with drill-down
…d-heavy-pages perf: require instance selection on Memory and Blocking pages
TabNav was using bg-white/5 (transparent white) which renders as a dark semi-transparent overlay in light mode, making tabs hard to see. Replaced with theme-aware CSS classes: - Dark mode: keeps the original subtle dark background - Light mode: uses a visible slate background with readable text colors Closes #41
There was a problem hiding this comment.
Pull request overview
Updates several frontend pages to improve dark/light theme consistency and refine “performance” UX, including better filtering/grouping and clearer empty states.
Changes:
- Slow Queries: adds per-server grouping in “All Instances” view and refactors row rendering/expansion logic.
- Memory + Blocking: changes initial loading behavior and adds a “Select an instance” empty state.
- Drives + TabNav: adds drive/instance filtering UI, fixes em-dash rendering, and introduces TabNav theme overrides via CSS classes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/SlowQueriesPage.tsx | Adds server-grouped “All Instances” rendering and refactors expandable rows. |
| frontend/src/pages/MemoryPage.tsx | Changes loading defaults and adds “select instance” empty state gating. |
| frontend/src/pages/DrivesPage.tsx | Adds filtering UI, updates drive card layout, and fixes dash rendering. |
| frontend/src/pages/BlockingPage.tsx | Changes loading defaults and adds “select instance” empty state gating. |
| frontend/src/index.css | Adds TabNav dark/light-mode styling overrides. |
| frontend/src/components/TabNav.tsx | Switches TabNav styling to use new CSS classes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!selectedInstance) { setLoading(false); return; } |
There was a problem hiding this comment.
Same issue as MemoryPage: if (!selectedInstance) { ... return; } makes the empty dropdown value (labeled “All Instances”) show a “Select an instance” empty state rather than loading data. Either remove/rename that option to indicate a required selection, or support the all-instances behavior when selectedInstance is undefined.
| if (!selectedInstance) { setLoading(false); return; } |
| function formatBytes(bytes: number): string { | ||
| if (!bytes) return '—'; | ||
| function formatBytes(bytes: number | null | undefined): string { | ||
| if (!bytes) return '—'; |
There was a problem hiding this comment.
formatBytes treats 0 as “no value” because it uses if (!bytes). This will render an em dash for legitimate 0-byte values (e.g., 0 free/used), which is misleading. Consider changing the check to bytes == null (or typeof bytes !== 'number') so 0 formats as 0.0 MB (or similar).
| if (!bytes) return '—'; | |
| if (bytes == null) return '—'; |
| (!selectedInstance || d.InstanceDisplayName === selectedInstance) | ||
| ); | ||
| }, [drives, selectedDrive, selectedInstance]); |
There was a problem hiding this comment.
selectedInstance is still applied in filtered even when id is present (instance detail route). Since the instance filter UI is hidden in that mode, a previously-selected instance filter can silently make the instance-drives view appear empty after navigation. Fix by resetting selectedInstance when id changes, or by ignoring selectedInstance in the filter predicate when id is truthy.
| (!selectedInstance || d.InstanceDisplayName === selectedInstance) | |
| ); | |
| }, [drives, selectedDrive, selectedInstance]); | |
| (id || !selectedInstance || d.InstanceDisplayName === selectedInstance) | |
| ); | |
| }, [drives, selectedDrive, selectedInstance, id]); |
| <button onClick={() => setSelectedInstance('')} className="ml-1 hover:text-white">×</button> | ||
| </span> | ||
| )} | ||
| {selectedDrive && ( | ||
| <span className="px-2 py-0.5 rounded-full bg-purple-400/10 text-purple-400"> | ||
| {selectedDrive} | ||
| <button onClick={() => setSelectedDrive('')} className="ml-1 hover:text-white">×</button> |
There was a problem hiding this comment.
The filter “clear” button uses only the “×” glyph and has no accessible label. Add an aria-label (e.g., “Clear instance filter”) so screen readers announce its purpose.
| <button onClick={() => setSelectedInstance('')} className="ml-1 hover:text-white">×</button> | |
| </span> | |
| )} | |
| {selectedDrive && ( | |
| <span className="px-2 py-0.5 rounded-full bg-purple-400/10 text-purple-400"> | |
| {selectedDrive} | |
| <button onClick={() => setSelectedDrive('')} className="ml-1 hover:text-white">×</button> | |
| <button aria-label="Clear instance filter" onClick={() => setSelectedInstance('')} className="ml-1 hover:text-white">×</button> | |
| </span> | |
| )} | |
| {selectedDrive && ( | |
| <span className="px-2 py-0.5 rounded-full bg-purple-400/10 text-purple-400"> | |
| {selectedDrive} | |
| <button aria-label="Clear drive filter" onClick={() => setSelectedDrive('')} className="ml-1 hover:text-white">×</button> |
| <button onClick={() => setSelectedInstance('')} className="ml-1 hover:text-white">×</button> | ||
| </span> | ||
| )} | ||
| {selectedDrive && ( | ||
| <span className="px-2 py-0.5 rounded-full bg-purple-400/10 text-purple-400"> | ||
| {selectedDrive} | ||
| <button onClick={() => setSelectedDrive('')} className="ml-1 hover:text-white">×</button> |
There was a problem hiding this comment.
This “×” clear button should be type="button" and should have an accessible label (e.g., aria-label="Clear drive filter"). Without an explicit type, buttons can default to submit if this component is ever placed inside a form, and the glyph-only label is not screen-reader friendly.
| <button onClick={() => setSelectedInstance('')} className="ml-1 hover:text-white">×</button> | |
| </span> | |
| )} | |
| {selectedDrive && ( | |
| <span className="px-2 py-0.5 rounded-full bg-purple-400/10 text-purple-400"> | |
| {selectedDrive} | |
| <button onClick={() => setSelectedDrive('')} className="ml-1 hover:text-white">×</button> | |
| <button | |
| type="button" | |
| aria-label="Clear instance filter" | |
| onClick={() => setSelectedInstance('')} | |
| className="ml-1 hover:text-white" | |
| > | |
| × | |
| </button> | |
| </span> | |
| )} | |
| {selectedDrive && ( | |
| <span className="px-2 py-0.5 rounded-full bg-purple-400/10 text-purple-400"> | |
| {selectedDrive} | |
| <button | |
| type="button" | |
| aria-label="Clear drive filter" | |
| onClick={() => setSelectedDrive('')} | |
| className="ml-1 hover:text-white" | |
| > | |
| × | |
| </button> |
| useEffect(() => { | ||
| if (!selectedInstance) { setLoading(false); return; } | ||
| setLoading(true); |
There was a problem hiding this comment.
The new guard if (!selectedInstance) { ... return; } prevents loading any data when the dropdown is set to its empty value (currently labeled “All Instances”). That makes the “All Instances” option behave like an error/empty state and breaks the previous ability to call the endpoint without an instanceId. Either remove/rename the “All Instances” option and make the UI clearly require a selection, or allow the undefined instance case and render a meaningful all-instances view.
Description
Type of Change
Checklist
npm run buildpasses (frontend)dotnet buildpasses (backend)@param(no string concat)Screenshots