[STU-189] Add shadcn/ui Table component and migrate DataTable#42
Open
BAWES wants to merge 3 commits into
Open
Conversation
…[STU-163] Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ar login spec [STU-177] Replace raw <label> elements with the shadcn/ui Label component for consistent design token usage and accessibility (htmlFor/id pairing). Tests: none (component swap, verified with tsc + lint + build) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces raw HTML table elements in the shared DataTable component with shadcn/ui Table, TableHeader, TableBody, TableRow, TableHead, and TableCell. This unblocks page migration work in STU-185 by providing the structural table components needed for role pages. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR refactors the authentication and public-facing UI layers from class-based CSS and role-driven logic to a component-driven architecture. It removes role-based login routing, introduces new UI primitives for forms and tables, simplifies the login flow to error-driven presentation, and migrates both the landing and login pages to use reusable Tailwind-styled components with Lucide React icons. ChangesUI Component Migration and Login Flow Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/app/styles.cssParsing error: This experimental syntax requires enabling one of the following parser plugin(s): "decorators", "decorators-legacy". (1:0) 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/styles.css (1)
341-356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove dangling selector commas in the portal chooser rules
Those trailing commas merge selector lists into the following rule, so declarations intended for the hero container also apply to child elements (and even
.rowin the mobile block). Specifically:
- 341-356:
.portalChooserHero h1,and.portalChooserHero p:not(.eyebrow),get merged into.portalChooserHero { ... }.- 5989-5998:
.portalChooserHero h1,gets merged into.row { ... }.- 6223-6249:
.portalChooserPromise span,.portalChooserHero h1, and.portalChooserPromise/spanget merged into.portalChooserHero { padding-top: ... }.Proposed cleanup
-.portalChooserHero h1, -.portalChooserHero p:not(.eyebrow), .portalChooserHero { max-width: 820px; padding: 42px 0 12px; }-.portalChooserPromise span, -.portalChooserHero h1, -.portalChooserPromise, -.portalChooserPromise span, .portalChooserHero { max-width: 900px; padding-top: clamp(38px, 8vw, 92px); }- .portalChooserHero h1, - .row { grid-template-columns: 1fr; min-height: 0; }🤖 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 `@src/app/styles.css` around lines 341 - 356, There are dangling trailing commas after selector lines like ".portalChooserHero h1," and ".portalChooserHero p:not(.eyebrow)," which cause those selectors to merge into the next rule; remove the trailing commas so each selector list is complete (e.g., change ".portalChooserHero h1," to ".portalChooserHero h1" or remove the standalone selector lines entirely) and fix the same pattern for the other occurrences referenced (the later ".portalChooserHero h1," in the .row rule and the ".portalChooserPromise span", ".portalChooserHero h1", ".portalChooserPromise"/"span" group) so that declarations intended for ".portalChooserHero" or ".portalChooserPromise" only apply to those selectors and not downstream rules.
🧹 Nitpick comments (2)
src/components/ui/table.tsx (1)
7-105: ⚡ Quick winConsider forwarding refs for all table components.
None of the table components forward the
refprop. In React 19,refis a regular prop that can be destructured and passed through. Consumers might need programmatic access to table DOM nodes for scroll management, focus handling, intersection observers, or measurements.♻️ Example fix for Table component (apply pattern to all)
-function Table({ className, ...props }: React.ComponentProps<"table">) { +function Table({ className, ref, ...props }: React.ComponentProps<"table">) { return ( <div data-slot="table-container" className="relative w-full overflow-x-auto" > <table + ref={ref} data-slot="table" className={cn("w-full caption-bottom text-sm", className)} {...props} /> </div> ) }Apply the same pattern to
TableHeader,TableBody,TableFooter,TableRow,TableHead,TableCell, andTableCaptionby destructuringrefand passing it to the corresponding HTML element.🤖 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 `@src/components/ui/table.tsx` around lines 7 - 105, All table components (Table, TableHeader, TableBody, TableFooter, TableRow, TableHead, TableCell, TableCaption) currently ignore the ref prop; update each component to accept and forward the ref (destructure ref from props and pass it to the underlying HTML element) using the appropriate HTML element ref type (e.g., React.Ref<HTMLTableElement> for Table, HTMLTableSectionElement for thead/tbody/tfoot, HTMLTableRowElement for tr, HTMLTableCellElement for th/td, HTMLTableCaptionElement for caption) so consumers can access DOM nodes programmatically.src/components/ui/label.tsx (1)
4-14: ⚡ Quick winConsider forwarding refs for programmatic access.
The
Labelcomponent doesn't forward therefprop. In React 19,refis a regular prop that can be destructured and passed through. If consumers need to programmatically focus or measure the label element, they currently cannot access the DOM node.♻️ Proposed fix to forward refs
-function Label({ className, ...props }: React.LabelHTMLAttributes<HTMLLabelElement>) { +function Label({ className, ref, ...props }: React.LabelHTMLAttributes<HTMLLabelElement>) { return ( <label + ref={ref} className={cn( "text-[13px] font-bold text-muted-foreground leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70", className )} {...props} /> ); }🤖 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 `@src/components/ui/label.tsx` around lines 4 - 14, The Label component currently drops the ref so consumers can't access the DOM node; update the component to forward the ref by accepting a ref prop (e.g., ref?: React.Ref<HTMLLabelElement>) alongside the existing props or by using React.forwardRef, and pass that ref through to the <label> element so programmatic focus/measure works; modify the Label function signature and ensure the ref is forwarded to the rendered label element (reference symbol: Label).
🤖 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.
Inline comments:
In `@src/components/ui/table.tsx`:
- Line 1: Remove the top-level "use client" directive from this module so the
presentational table components (the Table-related components exported from this
file) can be used in Server Components; ensure there are no client-only features
(hooks, state, event handlers, or browser APIs) in any exported components in
this file and run the build/tests after removal to confirm no client-side
assumptions remain.
In `@src/modules/auth/LoginForm.tsx`:
- Line 5: Update the two relative internal imports in LoginForm.tsx to use the
project path alias (replace the "./" style imports); specifically change the
import source for chooseAccountAction and loginAction (currently imported from
"./actions") to use the "`@/`..." alias form, and likewise update the other
relative import referenced on line 9 to the equivalent "`@/`..." alias so all
internal TypeScript imports follow the project's alias convention.
In `@src/modules/workspace/DataTable.tsx`:
- Line 69: The TableCell rendering sets colSpan to columns.length + (rowHref ? 1
: 0) which can evaluate to 0 when columns is empty and rowHref is false; update
the colSpan calculation in the DataTable component (the TableCell at the shown
location) to ensure it is at least 1 (e.g., use Math.max(1, ... ) or conditional
fallback) so the TableCell never receives a zero span.
---
Outside diff comments:
In `@src/app/styles.css`:
- Around line 341-356: There are dangling trailing commas after selector lines
like ".portalChooserHero h1," and ".portalChooserHero p:not(.eyebrow)," which
cause those selectors to merge into the next rule; remove the trailing commas so
each selector list is complete (e.g., change ".portalChooserHero h1," to
".portalChooserHero h1" or remove the standalone selector lines entirely) and
fix the same pattern for the other occurrences referenced (the later
".portalChooserHero h1," in the .row rule and the ".portalChooserPromise span",
".portalChooserHero h1", ".portalChooserPromise"/"span" group) so that
declarations intended for ".portalChooserHero" or ".portalChooserPromise" only
apply to those selectors and not downstream rules.
---
Nitpick comments:
In `@src/components/ui/label.tsx`:
- Around line 4-14: The Label component currently drops the ref so consumers
can't access the DOM node; update the component to forward the ref by accepting
a ref prop (e.g., ref?: React.Ref<HTMLLabelElement>) alongside the existing
props or by using React.forwardRef, and pass that ref through to the <label>
element so programmatic focus/measure works; modify the Label function signature
and ensure the ref is forwarded to the rendered label element (reference symbol:
Label).
In `@src/components/ui/table.tsx`:
- Around line 7-105: All table components (Table, TableHeader, TableBody,
TableFooter, TableRow, TableHead, TableCell, TableCaption) currently ignore the
ref prop; update each component to accept and forward the ref (destructure ref
from props and pass it to the underlying HTML element) using the appropriate
HTML element ref type (e.g., React.Ref<HTMLTableElement> for Table,
HTMLTableSectionElement for thead/tbody/tfoot, HTMLTableRowElement for tr,
HTMLTableCellElement for th/td, HTMLTableCaptionElement for caption) so
consumers can access DOM nodes programmatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c674adf9-631c-435c-8205-420fbd67a426
📒 Files selected for processing (8)
src/app/login/[role]/page.tsxsrc/app/login/page.tsxsrc/app/page.tsxsrc/app/styles.csssrc/components/ui/label.tsxsrc/components/ui/table.tsxsrc/modules/auth/LoginForm.tsxsrc/modules/workspace/DataTable.tsx
💤 Files with no reviewable changes (1)
- src/app/login/[role]/page.tsx
| @@ -0,0 +1,116 @@ | |||
| "use client" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove unnecessary "use client" directive.
These table components are pure presentational wrappers with no client-side features (hooks, state, browser APIs, or event handlers). The "use client" directive forces all consumers to cross the Server/Client boundary, preventing these primitives from being used in Server Components and increasing the JavaScript bundle size unnecessarily.
♻️ Proposed fix
-"use client"
-
import * as React from "react"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "use client" | |
| import * as React from "react" |
🤖 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 `@src/components/ui/table.tsx` at line 1, Remove the top-level "use client"
directive from this module so the presentational table components (the
Table-related components exported from this file) can be used in Server
Components; ensure there are no client-only features (hooks, state, event
handlers, or browser APIs) in any exported components in this file and run the
build/tests after removal to confirm no client-side assumptions remain.
|
|
||
| import { useActionState } from "react"; | ||
| import { LogIn } from "lucide-react"; | ||
| import { chooseAccountAction, loginAction } from "./actions"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Switch relative internal imports to @/ aliases.
Lines 5 and 9 still use relative internal imports; this should be normalized to the project alias convention.
♻️ Proposed fix
-import { chooseAccountAction, loginAction } from "./actions";
+import { chooseAccountAction, loginAction } from "`@/modules/auth/actions`";
...
-import type { LoginAccountChoice } from "./types";
+import type { LoginAccountChoice } from "`@/modules/auth/types`";As per coding guidelines, "Use @/ path alias for all internal imports in TypeScript files".
Also applies to: 9-9
🤖 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 `@src/modules/auth/LoginForm.tsx` at line 5, Update the two relative internal
imports in LoginForm.tsx to use the project path alias (replace the "./" style
imports); specifically change the import source for chooseAccountAction and
loginAction (currently imported from "./actions") to use the "`@/`..." alias form,
and likewise update the other relative import referenced on line 9 to the
equivalent "`@/`..." alias so all internal TypeScript imports follow the project's
alias convention.
| <tr className="emptyTableRow"> | ||
| <td colSpan={columns.length + (rowHref ? 1 : 0)}> | ||
| <TableRow className="emptyTableRow"> | ||
| <TableCell colSpan={columns.length + (rowHref ? 1 : 0)}> |
There was a problem hiding this comment.
Guard colSpan against zero columns.
colSpan can become 0 when columns is empty and rowHref is not provided, which is invalid for a table cell span.
Proposed fix
- <TableCell colSpan={columns.length + (rowHref ? 1 : 0)}>
+ <TableCell colSpan={Math.max(1, columns.length + (rowHref ? 1 : 0))}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <TableCell colSpan={columns.length + (rowHref ? 1 : 0)}> | |
| <TableCell colSpan={Math.max(1, columns.length + (rowHref ? 1 : 0))}> |
🤖 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 `@src/modules/workspace/DataTable.tsx` at line 69, The TableCell rendering sets
colSpan to columns.length + (rowHref ? 1 : 0) which can evaluate to 0 when
columns is empty and rowHref is false; update the colSpan calculation in the
DataTable component (the TableCell at the shown location) to ensure it is at
least 1 (e.g., use Math.max(1, ... ) or conditional fallback) so the TableCell
never receives a zero span.
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
Tablecomponent (Table,TableHeader,TableBody,TableRow,TableHead,TableCell,TableFooter,TableCaption)DataTablecomponent to use shadcn/ui Table structural components instead of raw HTML elementsTest plan
npx tsc --noEmit)npm run lint)🤖 Generated with Claude Code
Co-Authored-By: Paperclip noreply@paperclip.ing
Summary by CodeRabbit
Removed Features
UI/UX Updates