Conversation
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCreated a new internal shared UI package ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
🌏 Preview Deployments
Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Pull request overview
This PR extracts ShadCN UI components from the web application into a new shared package @ucdjs-internal/shared-ui to enable reuse across multiple UCD.js projects, specifically for the upcoming pipeline UI.
Changes:
- Created new
@ucdjs-internal/shared-uipackage with UI components, hooks, utilities, and styles - Updated web app to import UI components from the shared package instead of local files
- Migrated global CSS to the shared-ui package and updated web app to import from there
Reviewed changes
Copilot reviewed 66 out of 69 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/shared-ui/* |
New shared UI package containing extracted ShadCN components, utilities, hooks, and global styles |
apps/web/src/routes/**/*.tsx |
Updated imports to use @ucdjs-internal/shared-ui/components instead of local UI components |
apps/web/src/components/**/*.tsx |
Updated imports to use shared UI package for all UI components |
apps/web/tsconfig.json |
Simplified configuration by removing redundant compiler options now in base config |
apps/web/package.json |
Added dependency on @ucdjs-internal/shared-ui workspace package and subpath imports configuration |
apps/web/components.json |
Updated ShadCN configuration to point to shared-ui package aliases |
apps/web/src/globals.css |
Replaced all CSS with single import from shared-ui package |
tooling/tsconfig/base.json |
Added TypeScript path mappings for shared-ui package exports |
pnpm-lock.yaml |
Updated dependencies with new shared-ui package and React linting plugin versions |
apps/api/wrangler-types.d.ts |
Auto-generated types file updated with spelling corrections |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/routes/v/$version/u/$hex.tsx (1)
24-26: Return the promise from the loader to enable proper prefetching.The
ensureQueryDatacall returns a promise, but since it's not returned, TanStack Router won't wait for the data before completing navigation. WhileuseSuspenseQuerywill still work (Suspense handles the loading state), this defeats the purpose of prefetching in the loader.Suggested fix
loader: ({ context, params }) => { - context.queryClient.ensureQueryData(characterQueryOptions(params.hex, params.version)); + return context.queryClient.ensureQueryData(characterQueryOptions(params.hex, params.version)); },
🤖 Fix all issues with AI agents
In `@packages/shared-ui/LICENSE`:
- Around line 1-21: Update the copyright year in the LICENSE file: replace the
incorrect copyright header string "Copyright (c) 2026-PRESENT Lucas Nørgård"
with "Copyright (c) 2025-PRESENT Lucas Nørgård" so it matches the parent
repository and the other packages.
In `@packages/shared-ui/README.md`:
- Line 18: Unindent the "## 📄 License" heading so it starts at the beginning of
the line (remove any leading spaces/tabs) to satisfy markdownlint rule MD023 and
ensure it is recognized as a top-level heading; locate the line containing "##
📄 License" and remove indentation so the line begins with "##".
🧹 Nitpick comments (3)
packages/shared-ui/src/styles/globals.css (1)
1-133: Verify Tailwind@sourcecovers all class-bearing files in shared-ui.
If class names live outsidesrc/ui, they won’t be scanned and styles can be missing. Consider widening the glob if needed.🧩 Optional glob expansion
-@source "../ui/**/*.{ts,tsx}"; +@source "../**/*.{ts,tsx}";packages/shared-ui/src/hooks/use-mobile.ts (1)
1-19: Consider initializing frommatchMediato avoid first-render false/flicker.
This keeps SSR/client consistent and reduces layout shifts on mobile.♻️ Suggested refinement
- const [isMobile, setIsMobile] = React.useState<boolean | undefined>(undefined); + const [isMobile, setIsMobile] = React.useState<boolean | undefined>(() => { + if (typeof window === "undefined") return undefined; + return window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`).matches; + }); React.useEffect(() => { + if (typeof window === "undefined") return; const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); + setIsMobile(mql.matches); }; mql.addEventListener("change", onChange); - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); + setIsMobile(mql.matches); return () => mql.removeEventListener("change", onChange); }, []);apps/web/src/components/file-explorer/explorer-toolbar.tsx (1)
1-2: Consider using a path alias for the type import.Line 1 uses a relative path (
"../../routes/file-explorer/$") while thecnutility on line 2 uses the#lib/utilsalias. For consistency with the migration pattern used elsewhere in this PR, consider using a path alias if one is configured for route types.
1 participant
🔗 Linked issue
📚 Description
This PR will split the ShadCN UI from Web to be a shared package. This is because we wanna use it for the upcoming pipeline UI.
Summary by CodeRabbit
Improvements
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.