fix(dashboard): production polish — mobile nav, shared nav config, virtualized logs#223
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR centralizes dashboard navigation configuration into a dedicated module, updates the mobile menu and sidebar to consume it with improved sheet-closing behavior, and independently improves component UI and performance: error boundary messaging, job logs virtualization for large datasets, and table skeleton loading states. ChangesNavigation Configuration Centralization
Component UI and Performance Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dashboard/src/components/layout/nav-config.ts (1)
37-37: 💤 Low valueConsider distinct icons for Jobs and Tasks.
Both "Jobs" (line 37) and "Tasks" (line 61) currently use the
ListTreeicon, which may reduce visual distinction in the navigation menu.🎨 Suggested alternative icons
If Tasks represents definitions/templates while Jobs represents running instances, consider using
ClipboardListorFileTextfor Tasks:- { to: "/tasks", label: "Tasks", icon: ListTree }, + { to: "/tasks", label: "Tasks", icon: ClipboardList },Also applies to: 61-61
🤖 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 `@dashboard/src/components/layout/nav-config.ts` at line 37, The Jobs and Tasks entries in nav-config.ts both use the ListTree icon which reduces visual distinction; update the nav item for the route with to: "/tasks" (the object whose label is "Tasks") to use a different icon (for example ClipboardList or FileText) while keeping the Jobs entry (to: "/jobs", label: "Jobs") as ListTree (or choose a complementary pair), and ensure you import the chosen icon at the top and replace the icon reference in the Tasks nav object.dashboard/src/features/jobs/components/job-logs-tab.tsx (1)
54-69: 💤 Low valuePrefer the virtual item
keyfor row identityThe dynamic-measurement layout (absolute positioning +
data-index+measureElement+translateY(item.start)) is wired correctly. Small alignment: useitem.key(TanStack Virtual’s recommended stable identity for virtual rows) for the Reactkeyinstead of a composite that includesitem.index, which helps avoid remounting churn if ordering changes.♻️ Use
item.keyfor the row key<div - key={`${log.logged_at}-${log.level}-${item.index}`} + key={item.key} ref={virtualizer.measureElement} data-index={item.index} style={{ position: "absolute", top: 0, left: 0, width: "100%", transform: `translateY(${item.start}px)`, }}🤖 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 `@dashboard/src/features/jobs/components/job-logs-tab.tsx` around lines 54 - 69, The React key for virtual rows currently uses a composite including log.logged_at/log.level/item.index which can cause unnecessary remounts; change the key to use the stable virtual id by replacing the composite key with item.key where the virtual items are rendered (the map over virtualizer.getVirtualItems() in job-logs-tab.tsx) while keeping the existing ref, data-index, style, and use of logs[item.index] unchanged so identity is driven by item.key.
🤖 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.
Nitpick comments:
In `@dashboard/src/components/layout/nav-config.ts`:
- Line 37: The Jobs and Tasks entries in nav-config.ts both use the ListTree
icon which reduces visual distinction; update the nav item for the route with
to: "/tasks" (the object whose label is "Tasks") to use a different icon (for
example ClipboardList or FileText) while keeping the Jobs entry (to: "/jobs",
label: "Jobs") as ListTree (or choose a complementary pair), and ensure you
import the chosen icon at the top and replace the icon reference in the Tasks
nav object.
In `@dashboard/src/features/jobs/components/job-logs-tab.tsx`:
- Around line 54-69: The React key for virtual rows currently uses a composite
including log.logged_at/log.level/item.index which can cause unnecessary
remounts; change the key to use the stable virtual id by replacing the composite
key with item.key where the virtual items are rendered (the map over
virtualizer.getVirtualItems() in job-logs-tab.tsx) while keeping the existing
ref, data-index, style, and use of logs[item.index] unchanged so identity is
driven by item.key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 389c65c9-a774-4cf0-99d7-304c5e8b7aee
📒 Files selected for processing (6)
dashboard/src/components/layout/mobile-menu.tsxdashboard/src/components/layout/nav-config.tsdashboard/src/components/layout/route-error-boundary.tsxdashboard/src/components/layout/sidebar.tsxdashboard/src/features/jobs/components/job-logs-tab.tsxdashboard/src/features/resources/components/resources-table.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Production-polish pass on the dashboard, fixing every item surfaced by a senior-engineer review of the UI. Five focused, sequential commits — no behavior changes outside the dashboard.
Changes
onClickclose on each nav/external link (closes reliably, even when re-navigating to the same route).NAVand had drifted: the mobile menu was silently missing Tasks and Webhooks. Extracted a single source of truth inlayout/nav-config.ts; adding a route is now a one-line edit.ErrorStatein the route error boundary —RouteErrorFallbackreimplemented the error card's markup; it now renders the sharedErrorStateso error UI is consistent everywhere.SkeletonforTableSkeletonwith column widths matching the rendered table, consistent with every other table.JobLogsTabrendered all log lines unvirtualized, degrading the job detail page for high-volume jobs. Now uses@tanstack/react-virtualwith dynamic row measurement, preserving the full wrapped-message layout.Verification
pnpm typecheck— passpnpm lint(Biome, 207 files) — cleanpnpm test(Vitest) — 106/106pnpm build— pass;logs.lazychunk dropped from 19.35 kB → 3.41 kB gz (shares the already-loaded virtualizer)Summary by CodeRabbit
Release Notes
New Features
Style
Refactor