Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe dashboard page client updates its top-level content wrapper from a full-height flex container to a viewport-offset height using ChangesDashboard Layout Height
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR appears to address a dashboard detail-page layout issue in the dashboard app by changing how the main split view gets its height within the project sidebar layout.
Changes:
- Reordered imports in the dashboard detail page.
- Replaced the split-view container’s
h-fullsizing with explicit viewport-based heights in light and dark mode.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {/* Both panels are always in the DOM so the iframe never unmounts/reloads. | ||
| The chat panel animates its width; the dashboard panel adjusts via flex-1. */} | ||
| <div data-full-bleed className="flex h-full"> | ||
| <div data-full-bleed className="flex h-[calc(100vh-3.5rem)] dark:h-[calc(100vh-6rem)]"> |
Greptile SummaryThis PR fixes the dashboard detail page layout by replacing Confidence Score: 4/5Safe to merge — the fix is correct and consistent with the existing sidebar pattern; only a P2 style note about duplicated magic numbers. A single P2 finding (duplicated hardcoded rem values) that mirrors an already-established pattern in the codebase. No logic bugs, security concerns, or regressions introduced. No files require special attention beyond the noted style concern on line 394. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["h-screen flex-col overflow-y-auto\n(sidebar-layout outer container)"] --> B
B["Sticky Header\nLight: h-14 = 3.5rem\nDark: mt-3 + h-14 + mb-3 = 5rem"] --> C
C["flex flex-1 items-start\n(items-start breaks height propagation)"] --> D
D["main flex-1\n(no explicit height)"] --> E
E["Content wrapper\nhas-[data-full-bleed]:h-full\n(h-full fails — parent has no height)"] --> F
F["PageLayout noPadding\nflex flex-1 min-h-0 flex-col"] --> G
G["data-full-bleed div\nBEFORE: h-full ❌ collapses\nAFTER: h-[calc(100vh-3.5rem)] ✅\ndark:h-[calc(100vh-6rem)] ✅"]
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/dashboards/[dashboardId]/page-client.tsx:394
**Hardcoded header heights may drift out of sync**
The `calc(100vh-3.5rem)` / `dark:h-[calc(100vh-6rem)]` values match the sidebar (which already uses identical magic numbers in `sidebar-layout.tsx:738`), but they duplicate the same constants in two places. If the header height or dark-mode margins ever change, every site that uses `calc(100vh - <header>)` will need a coordinated update. Consider extracting these to a CSS custom property (e.g. `--header-height`) set on the root, so there is a single source of truth.
Reviews (1): Last reviewed commit: "Merge branch 'dev' into custom-dashboard..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| {/* Both panels are always in the DOM so the iframe never unmounts/reloads. | ||
| The chat panel animates its width; the dashboard panel adjusts via flex-1. */} | ||
| <div data-full-bleed className="flex h-full"> | ||
| <div data-full-bleed className="flex h-[calc(100vh-3.5rem)] dark:h-[calc(100vh-6rem)]"> |
There was a problem hiding this comment.
The calc(100vh-3.5rem) / dark:h-[calc(100vh-6rem)] values match the sidebar (which already uses identical magic numbers in sidebar-layout.tsx:738), but they duplicate the same constants in two places. If the header height or dark-mode margins ever change, every site that uses calc(100vh - <header>) will need a coordinated update. Consider extracting these to a CSS custom property (e.g. --header-height) set on the root, so there is a single source of truth.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/dashboards/[dashboardId]/page-client.tsx
Line: 394
Comment:
**Hardcoded header heights may drift out of sync**
The `calc(100vh-3.5rem)` / `dark:h-[calc(100vh-6rem)]` values match the sidebar (which already uses identical magic numbers in `sidebar-layout.tsx:738`), but they duplicate the same constants in two places. If the header height or dark-mode margins ever change, every site that uses `calc(100vh - <header>)` will need a coordinated update. Consider extracting these to a CSS custom property (e.g. `--header-height`) set on the root, so there is a single source of truth.
How can I resolve this? If you propose a fix, please make it concise.![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| {/* Both panels are always in the DOM so the iframe never unmounts/reloads. | ||
| The chat panel animates its width; the dashboard panel adjusts via flex-1. */} | ||
| <div data-full-bleed className="flex h-full"> | ||
| <div data-full-bleed className="flex h-[calc(100vh-3.5rem)] dark:h-[calc(100vh-6rem)]"> |
There was a problem hiding this comment.
Hardcoded magic numbers (3.5rem for light mode, 6rem for dark mode) for header heights are duplicated across 5 files, creating a maintenance burden where any header height changes require coordinated updates in multiple locations.
| {/* Both panels are always in the DOM so the iframe never unmounts/reloads. | ||
| The chat panel animates its width; the dashboard panel adjusts via flex-1. */} | ||
| <div data-full-bleed className="flex h-full"> | ||
| <div data-full-bleed className="flex h-[calc(100vh-3.5rem)] dark:h-[calc(100vh-6rem)]"> |
There was a problem hiding this comment.
| <div data-full-bleed className="flex h-[calc(100vh-3.5rem)] dark:h-[calc(100vh-6rem)]"> | |
| <div data-full-bleed className="flex h-[calc(100vh-4.5rem)] dark:h-[calc(100vh-5.75rem)]"> |
Dashboard container viewport-based heights don't account for main element padding, causing overflow of its parent container
This PR fixes a layout bug
Summary by CodeRabbit