enhancement: Move Report Issue to sidebar (bottom) & bug: invalid date preview on container listing screen #422
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Sidebar as AppSidebar / NavUser
participant Handler as handleReportIssue
Note over Sidebar: Sidebar is a full-height flex column (new `.sidebar`)
User->>Sidebar: Click "Report Issue" (bottom-anchored button)
Sidebar->>Handler: invoke handleReportIssue()
Handler-->>Sidebar: complete (open modal/route)
Sidebar-->>User: UI response (existing feedback)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
Pre-merge checks✅ Passed checks (3 passed)
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 (1)
view/app/globals.css (1)
526-533: Add mobile‑safe viewport units; don’t rename .sidebar without updating call sitesview/app/globals.css declares .sidebar (view/app/globals.css:527–528) and it’s referenced as className="sidebar" in view/components/layout/app-sidebar.tsx:154 — renaming the CSS to .app-sidebar will break that usage.
- Action: update the existing .sidebar rule to include mobile‑safe fallbacks (keep height:100vh fallback, add height:100dvh and min-height:100svh).
- If you want a namespaced class, either update all call sites (at least view/components/layout/app-sidebar.tsx:154) or add an alias selector so both work (.sidebar, .app-sidebar { … }) and migrate gradually.
🧹 Nitpick comments (2)
view/components/layout/app-sidebar.tsx (1)
154-175: Align className with the namespaced CSS and avoid generic selectorsIf you adopt the CSS change, update the component to use the new class to prevent global selector collisions.
- <Sidebar className="sidebar" collapsible="icon" {...props}> + <Sidebar className="app-sidebar" collapsible="icon" {...props}>view/components/layout/nav-user.tsx (1)
231-241: Use SidebarMenuItem/SidebarMenuButton for consistency, collapse/tooltip behavior, and focus statesRaw may not integrate with the sidebar’s collapsed mode and a11y. Wrap it in SidebarMenuItem/SidebarMenuButton and add focus-visible styles.
- {/* Bottom-anchored "Report Issue" action */} - <div className="mt-auto p-4"> - <button - onClick={handleReportIssue} - className="flex items-center gap-2 text-sm text-sidebar-foreground hover:text-sidebar-accent" - > - <AlertCircle /> - {t('user.menu.reportIssue')} - </button> - </div> + {/* Bottom-anchored "Report Issue" action */} + <div className="mt-auto p-4"> + <SidebarMenuItem> + <SidebarMenuButton asChild> + <button + onClick={handleReportIssue} + className="flex items-center gap-2 text-sm text-sidebar-foreground hover:text-sidebar-accent focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-sidebar-ring focus-visible:ring-offset-2" + aria-label={t('user.menu.reportIssue')} + title={t('user.menu.reportIssue')} + > + <AlertCircle /> + <span className="truncate">{t('user.menu.reportIssue')}</span> + </button> + </SidebarMenuButton> + </SidebarMenuItem> + </div>Please verify in:
- collapsed “icon” mode: icon shows and tooltip/title is available
- mobile: button remains reachable and anchored at bottom
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
view/app/globals.css(1 hunks)view/components/layout/app-sidebar.tsx(1 hunks)view/components/layout/nav-user.tsx(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
view/app/containers/components/table.tsx (1)
86-89: Locale hardcoding, potential invalid timestamp, and per-row formatter creation
- Don’t hardcode 'en-US' (breaks i18n); use default locale or app locale.
- parseInt without radix is brittle; also guard against invalid/empty timestamps to avoid RangeError.
- New Intl.DateTimeFormat is created per row; memoize once.
Option A (minimal change; within current lines):
- ? new Intl.DateTimeFormat('en-US', { month: 'short', day: '2-digit', year: 'numeric' }).format( - new Date(parseInt(container.created) * 1000) - ) + ? new Intl.DateTimeFormat(undefined, { month: 'short', day: '2-digit', year: 'numeric' }).format( + new Date(parseInt(container.created as string, 10) * 1000) + )Option B (more robust; replaces Lines 85–89):
- const formattedDate = container.created - ? new Intl.DateTimeFormat('en-US', { month: 'short', day: '2-digit', year: 'numeric' }).format( - new Date(parseInt(container.created) * 1000) - ) - : '-'; + const tsSec = Number(container.created); + const formattedDate = + Number.isFinite(tsSec) + ? dateFormatter.format(new Date(tsSec * 1000)) + : '-';Add once near Line 41:
const dateFormatter = React.useMemo( () => new Intl.DateTimeFormat(undefined, { month: 'short', day: '2-digit', year: 'numeric' }), [] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
view/app/containers/components/table.tsx(1 hunks)
🔇 Additional comments (1)
view/app/containers/components/table.tsx (1)
86-89: Scope creep: date-format change seems unrelated to “Move Report Issue to sidebar”Confirm this change belongs in this PR; otherwise split to a separate PR to keep diffs focused and revert here.
rupanshi01
changed the title
|
@rupanshi01: Can you comment on the issues so that I can assign these issues to you? Could you please add screenshot for reference as well? Stay around, you will have more issues where you can get involved and contribute! Join our Discord community for discussions and updates. Happy learning! 💚 |
There was a problem hiding this comment.
Hey @rupanshi01, your PR needs some tweaks before we can merge it. Also, try to focus on one thing per PR, okay? Your title mentions two things, so maybe split it into two separate PRs?
view/app/globals.css
Outdated
| @@ -523,3 +523,10 @@ | |||
| scrollbar-width: none; | |||
| } | |||
| } | |||
|
|
|||
| /* Ensure the sidebar supports bottom anchoring */ | |||
There was a problem hiding this comment.
This is not required, ideally we shouldn't be modifying the global.css file, you can modify the behaviour in sidebar itself
view/components/layout/nav-user.tsx
Outdated
| @@ -232,6 +228,17 @@ Add any other context about the problem here.`; | |||
| </DropdownMenuContent> | |||
| </DropdownMenu> | |||
| </SidebarMenuItem> | |||
|
|
|||
| {/* Bottom-anchored "Report Issue" action */} | |||
| <div className="mt-auto p-4"> | |||
There was a problem hiding this comment.
Div is not required here, sidebar section remains same but resuse the components to make it bottom aligned and render
changed 3 files global.css , app-sidebar.tsx , nav-user-tsx .
Summary by CodeRabbit
New Features
Style
UX Improvements
Display