style: add page banner and refactor container to use common header across the app #548
Conversation
WalkthroughVersion bumped from 0.1.0-alpha.55 to 0.1.0-alpha.56. Containers page header refactored to use new Banner component. New Banner layout component introduced with customizable properties, loading skeleton support, and flexible styling via variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 2
🧹 Nitpick comments (2)
view/components/layout/page-banner.tsx (2)
106-106: Remove redundantpriorityprop.Setting
priority={false}is unnecessary sincefalseis the default value for Next.js Image component.Apply this diff:
- priority={false}
89-114: Consider simplifying the ImageSection nesting.The ImageSection contains multiple nested divs that could be reduced for better maintainability.
Consider this simplified structure:
const ImageSection = ( - <div className="flex-1"> - <div className="relative mx-auto max-w-xs"> - <div className="aspect-square"> - <div className="flex h-full items-center justify-center"> - <div className="relative w-full h-full text-center"> - {image ? ( - <div className="flex items-center justify-center w-full h-full"> - {image} - </div> - ) : ( - <Image - src={imageSrc} - alt={imageAlt} - className="object-contain" - fill - sizes="(min-width: 768px) 320px, 50vw" - priority={false} - /> - )} - </div> - </div> - </div> - </div> - </div> + <div className="flex-1 flex items-center justify-center"> + <div className="relative w-full max-w-xs aspect-square"> + {image ? ( + <div className="flex items-center justify-center w-full h-full"> + {image} + </div> + ) : ( + <Image + src={imageSrc} + alt={imageAlt} + className="object-contain" + fill + sizes="(min-width: 768px) 320px, 50vw" + /> + )} + </div> + </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)version.txt(1 hunks)view/app/containers/page.tsx(5 hunks)view/components/layout/page-banner.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
view/app/containers/page.tsx (3)
view/components/layout/page-banner.tsx (1)
Banner(62-158)view/components/rbac/PermissionGuard.tsx (1)
AnyPermissionGuard(42-58)view/components/ui/select-wrapper.tsx (1)
SelectWrapper(39-147)
view/components/layout/page-banner.tsx (3)
view/components/ui/typography.tsx (1)
TypographyH1(1-13)view/lib/utils.ts (1)
cn(4-6)view/components/ui/skeleton.tsx (1)
Skeleton(13-13)
🪛 LanguageTool
version.txt
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context: v0.1.0-alpha.56
(QB_NEW_DE)
🔇 Additional comments (8)
package.json (1)
3-3: LGTM!The version bump aligns with the new Banner component feature being introduced in this PR.
view/components/layout/page-banner.tsx (5)
1-11: LGTM!The imports and
'use client'directive are appropriate for this component's functionality with refs and potential client-side interactivity.
12-45: LGTM!The CVA variants for
bannerContainerandbannerLayoutare well-structured with sensible defaults.
47-60: LGTM!Type definitions are comprehensive and properly extend the necessary base types.
141-189: LGTM!The Banner render logic and BannerSkeleton component are well-implemented with proper className merging and loading state support.
74-74: No action required—file exists.The file
plugin.pngexists atview/public/plugin.pngand is correctly referenced by the path/plugin.pngin a Next.js application. The defaultimageSrcis valid.Likely an incorrect or invalid review comment.
view/app/containers/page.tsx (1)
93-166: LGTM!The layout restructure properly preserves all functionality while improving the visual hierarchy with the new Banner component. Permission guards and responsive behavior are maintained correctly.
version.txt (1)
1-1: LGTM!The version bump aligns with the package.json update.
|
|
||
| import React from 'react'; | ||
| import { RefreshCw, Trash2, Loader2, Scissors, Grid, List } from 'lucide-react'; | ||
| import { RefreshCw, Trash2, Loader2, Scissors, Grid, List, Puzzle, Container } from 'lucide-react'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the Puzzle icon is used.
The Puzzle icon is imported but doesn't appear to be used in the current file.
Run the following script to check if Puzzle is used elsewhere or can be removed:
🏁 Script executed:
#!/bin/bash
# Description: Search for Puzzle icon usage in the file
# Search for Puzzle usage in containers page
rg -n "Puzzle" view/app/containers/Length of output: 130
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Search for Puzzle icon usage across entire codebase
# Search for Puzzle usage (excluding the import line)
rg -n "Puzzle" --type=tsx --type=ts | grep -v "^view/app/containers/page.tsx:4:"
Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Search for Puzzle icon usage across entire codebase
# Search for Puzzle in all files and filter out just the import
rg -n "Puzzle" | grep -v "^view/app/containers/page.tsx:4:"
Length of output: 172
Remove unused Puzzle icon from imports.
The Puzzle icon is imported on line 4 of view/app/containers/page.tsx but is not used in that file. It is already properly imported in view/hooks/use-app-sidebar.ts where it's actually used.
🤖 Prompt for AI Agents
In view/app/containers/page.tsx around line 4, the imported Puzzle icon is
unused; remove Puzzle from the import list (i.e., delete "Puzzle" from the
lucide-react named imports) so the file only imports the icons that are actually
used, then run a quick lint/check to ensure no stray references remain.
| <div className="w-full mb-6"> | ||
| <Banner | ||
| isLoading={false} | ||
| badgeText="beta" | ||
| heading={t('containers.title')} | ||
| subheading={t('containers.description')} | ||
| description="Easily extend functionality with community-built add-ons." | ||
| image={<Container className='w-32 h-32 text-primary' />} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Consider using dynamic loading state and fix the description.
Two concerns:
isLoading={false}is hardcoded. Consider using the actual loading state fromuseContainerList(e.g.,isLoadingorisFetching) to show the skeleton while data loads.- The description "Easily extend functionality with community-built add-ons." seems like a placeholder or copy-pasted from a plugins/extensions page and doesn't accurately describe container functionality.
Apply this diff:
<div className="w-full mb-6">
<Banner
- isLoading={false}
+ isLoading={!initialized && isLoading}
badgeText="beta"
heading={t('containers.title')}
subheading={t('containers.description')}
- description="Easily extend functionality with community-built add-ons."
+ description={t('containers.bannerDescription') || "Manage and monitor your Docker containers efficiently."}
image={<Container className='w-32 h-32 text-primary' />}
/>
</div>Note: Add containers.bannerDescription to your i18n translations file for proper localization.
🤖 Prompt for AI Agents
In view/app/containers/page.tsx around lines 82 to 91, replace the hardcoded
isLoading={false} with the actual loading state from useContainerList (e.g.,
pass isLoading={isLoading || isFetching}) so the Banner shows a skeleton while
data loads, and replace the static description string with a localized value
t('containers.bannerDescription'); also add the new containers.bannerDescription
key to your i18n translations file with the correct container-specific copy.
Ensure you import/use the loading flags returned from useContainerList and
update any types/props if needed.
|
@occoder-dev : Can you share the screen shot of the changes? |
2 participants
Issue
Link to related issue(s):
Description
Short summary of what this PR changes or introduces.
Scope of Change
Select all applicable areas impacted by this PR:
Screenshot / Video / GIF (if applicable)
Attach or embed screenshots, screen recordings, or GIFs demonstrating the feature or fix.
Related PRs (if any)
Link any related or dependent PRs across repos.
Additional Notes for Reviewers (optional)
Anything reviewers should know before testing or merging (e.g., environment variables, setup steps).
Developer Checklist
To be completed by the developer who raised the PR.
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
New Features
Chores