pageCard統一 #449
pageCard統一 #449
Conversation
…eld and enhance auto-save functionality to handle undefined title and page content values.
… field and improving auto-save functionality to set empty strings for undefined title and page content values.
…sWithInfo and update related components. Add textAndOccurrenceHash field to SeedText model for improved data handling.
…move unnecessary props and simplify structure by integrating PageCard directly, enhancing code readability and maintainability.
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
web/app/routes/$userName+/functions/queries.server.ts (1)
67-71: Consider optimizing repeated user data.While adding user information to each page object improves data accessibility, it introduces data duplication when multiple pages belong to the same user. Consider these optimizations:
- Use a normalized data structure to avoid duplication
- Implement caching for user data
- Consider moving user data to a separate response field if it's constant across pages
Example of a normalized structure:
interface NormalizedResponse { user: { userName: string; displayName: string; icon: string; }; pages: Array<{ ...pageFields; userId: string; // Reference to user }>; }web/app/components/PageCard.tsx (2)
16-23: Props interface looks good, but consider adding JSDoc commentsThe props interface is well-structured with clear types. Consider adding JSDoc comments to document the purpose of each prop, especially for callback functions.
Example documentation:
type PageCardProps = { /** The page data to display */ pageCard: PageCardType; /** URL for the page link */ pageLink: string; /** URL for the user profile link */ userLink: string; /** Whether to show owner-specific actions */ showOwnerActions?: boolean; /** Callback to toggle the page's public status */ onTogglePublicStatus?: (pageId: number) => void; /** Callback to archive the page */ onArchive?: (pageId: number) => void; };
35-44: Consider extracting action buttons to a separate componentThe conditional rendering of actions could be extracted to improve readability and maintainability.
-{showOwnerActions && onTogglePublicStatus && onArchive && ( - <div className="absolute top-2 right-2"> - <PageActionsDropdown - editPath={`${pageLink}/edit`} - onTogglePublic={() => onTogglePublicStatus(pageCard.id)} - onDelete={() => onArchive(pageCard.id)} - isPublished={pageCard.isPublished} - /> - </div> -)} +{showOwnerActions && onTogglePublicStatus && onArchive && ( + <PageCardActions + pageId={pageCard.id} + pageLink={pageLink} + isPublished={pageCard.isPublished} + onTogglePublicStatus={onTogglePublicStatus} + onArchive={onArchive} + /> +)}web/app/routes/$userName+/index.test.tsx (1)
Line range hint
144-149: Complete or remove the commented test codeThe commented test code suggests an incomplete implementation of the toggle publish functionality test. Either complete the test implementation or remove the commented code to maintain test clarity.
Would you like me to help implement the complete test case for toggle publish functionality?
web/app/routes/$userName+/index.tsx (2)
181-189: Add prop validation for PageCard componentThe PageCard component receives multiple critical props. Consider adding runtime prop validation using PropTypes or TypeScript interfaces to ensure type safety.
Here's a suggested TypeScript interface for the props:
interface PageCardProps { pageCard: { id: number; slug: string; // Add other required page fields }; pageLink: string; userLink: string; showOwnerActions: boolean; onTogglePublicStatus: (pageId: number) => void; onArchive: (pageId: number) => void; }
Line range hint
193-252: Consider extracting pagination logic into a separate componentThe pagination implementation, while functional, could be extracted into a reusable component to improve maintainability and reusability.
Consider creating a separate
PaginationComponentthat accepts:
- currentPage
- totalPages
- onPageChange
This would make the pagination logic reusable across different parts of the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (8)
web/app/components/PageCard.tsx(1 hunks)web/app/routes/$userName+/functions/queries.server.ts(1 hunks)web/app/routes/$userName+/index.test.tsx(2 hunks)web/app/routes/$userName+/index.tsx(2 hunks)web/app/routes/home/_home.tsx(3 hunks)web/app/routes/home/functions/queries.server.ts(3 hunks)web/app/routes/types.ts(1 hunks)web/prisma/seed.ts(6 hunks)
🔇 Additional comments (9)
web/app/routes/types.ts (1)
1-8: LGTM! Type definitions look clean and well-structured.
The type transformations are well-designed, particularly the explicit typing of createdAt as string. This ensures type safety when working with date fields in the UI layer.
Let's verify that all components using PageCardType handle the createdAt field correctly:
✅ Verification successful
Type transformation and usage verified successfully
The PageCard component correctly handles the createdAt field as a string in its display, and the type transformation from the original Prisma Date type (from queries.server.ts) to string is appropriate for the UI layer. The component simply renders the createdAt value within a CardDescription component without any date manipulation, which aligns with receiving it as a pre-formatted string.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using PageCardType and verify date handling
rg -t typescript -t tsx "PageCardType|createdAt.*string" --glob '!*.test.*' --glob '!*types.ts'
Length of output: 132
Script:
#!/bin/bash
# Search for components using PageCardType and verify date handling with correct file extensions
rg -t ts -t tsx "PageCardType|createdAt.*string" --glob '!*.test.*' --glob '!*types.ts'
Length of output: 117
Script:
#!/bin/bash
# Search for components using PageCardType and verify date handling without file type filters
rg "PageCardType|createdAt.*string" --type-not test --glob '!*types.ts'
Length of output: 102
Script:
#!/bin/bash
# Search for components using PageCardType and verify date handling
rg "PageCardType|createdAt.*string" --glob '!*.test.*' --glob '!*types.ts'
# Also search for any usage of the imported type
rg "FetchPaginatedPublicPagesReturn" --glob '!*.test.*'
Length of output: 571
Script:
#!/bin/bash
# Let's check the PageCard component to see how it handles the createdAt field
cat web/app/components/PageCard.tsx
# Also check the query implementation to understand the original type
cat web/app/routes/home/functions/queries.server.ts
Length of output: 3690
web/app/routes/home/functions/queries.server.ts (3)
28-29: Good addition of title and isPublished fields.
Including these fields in the select clause improves data completeness and reduces the need for additional queries.
71-74: Improved naming with pagesWithInfo.
The rename from pages to pagesWithInfo better reflects the enriched data structure.
77-79: Good practice: Exporting return type.
Exporting the return type improves type safety across the application.
web/app/routes/$userName+/functions/queries.server.ts (1)
Line range hint 74-80: Verify error handling for missing user data.
The current implementation assumes user data will always be available. Let's verify error handling for edge cases.
✅ Verification successful
Let me analyze the error handling patterns in the codebase, particularly around user data access. The previous command failed due to typescript file type recognition, so I've adjusted it and also included the specific file content to examine the context better.
The previous commands failed due to incorrect file paths. Let's first locate the file and then analyze its error handling patterns. I've also broadened the search to look for general error handling patterns in the routes directory.
Looking at the code and error handling patterns, I can now generate a final response regarding the error handling for missing user data.
Error handling for missing user data is properly implemented
The fetchSanitizedUserWithPages function in the code has appropriate error handling:
- It returns
nullwhen the user is not found - The
sanitizeUserfunction is only called after confirming user exists - The calling code handles the null case appropriately by throwing a 404 response
The error handling follows the pattern used throughout the codebase where:
- Null checks are performed before data access
- Appropriate HTTP responses are thrown for missing resources
- Type safety is maintained through the sanitization process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in related components
rg -t typescript -t tsx "user\.(userName|displayName|icon)" -C 2 --glob '!*.test.*' | grep -C 2 "if|try|catch|throw|Error"
Length of output: 159
Script:
#!/bin/bash
# Search for error handling patterns in related components with ripgrep
rg --type-add 'ts:*.{ts,tsx}' -t ts "user\.(userName|displayName|icon)" -C 2 --glob '!*.test.*' | grep -C 2 "if|try|catch|throw|Error"
# Also check the specific file for error handling
cat "web/app/routes/$userName+/functions/queries.server.ts"
Length of output: 264
Script:
#!/bin/bash
# Let's first find the correct path to the file
fd "queries.server.ts"
# Then examine the file content and surrounding context
fd "queries.server.ts" --exec cat {}
# Search for error handling patterns without filtering by user properties
rg --type-add 'ts:*.{ts,tsx}' -t ts "throw|Error|try|catch" -C 2 "web/app/routes"
Length of output: 32114
web/app/routes/home/_home.tsx (1)
56-61: LGTM! Clean integration of PageCard component
The PageCard integration looks good with proper prop passing and key usage.
web/prisma/seed.ts (1)
148-148: LGTM! Proper database schema update
The addition of textAndOccurrenceHash to the database schema is implemented correctly.
web/app/routes/$userName+/index.test.tsx (1)
86-86: LGTM: Test assertion updated to handle multiple text occurrences
The change from findByText to findAllByText correctly handles potential multiple occurrences of "Test User" text, which could appear in both the profile header and page cards.
Also applies to: 113-113
web/app/routes/$userName+/index.tsx (1)
9-11: Verify removal of Lock icon
The Lock icon import has been removed. Let's verify this doesn't affect any existing functionality.
✅ Verification successful
Lock icon removal is safe in this file
The Lock icon import was safely removed from $userName+/index.tsx as it's not used in this file. The Lock icon is still properly imported and used in other components where it's needed:
PageCard.tsx- shows lock for unpublished pagesEditHeader.tsx- uses lock in edit interfaceSourceTextAndTranslationSection.tsx- displays lock for unpublished content
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of Lock icon in the codebase
rg -l "Lock" --type tsx --type jsx
# Search specifically for lock-related UI elements
ast-grep --pattern 'Lock className="$_"'
Length of output: 207
Script:
#!/bin/bash
# Let's try a more comprehensive search for Lock icon usage
rg "Lock" --type-add 'tsx:*.tsx' --type-add 'jsx:*.jsx'
# Also search for specific imports of Lock
rg "import.*Lock.*from.*lucide" --type-add 'tsx:*.tsx' --type-add 'jsx:*.jsx'
# Let's check the history of this specific file to see how Lock was used before
git log -p -- web/app/routes/$userName+/index.tsx
Length of output: 1344
| <Link to={userLink} className="flex items-center"> | ||
| <Avatar className="w-6 h-6 mr-2"> | ||
| <AvatarImage | ||
| src={pageCard.user.icon} | ||
| alt={pageCard.user.displayName} | ||
| /> | ||
| <AvatarFallback> | ||
| {pageCard.user.displayName.charAt(0).toUpperCase()} | ||
| </AvatarFallback> | ||
| </Avatar> | ||
| <span className="text-sm text-gray-600"> | ||
| {pageCard.user.displayName} | ||
| </span> | ||
| </Link> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-label to user link for better accessibility
The user link should have an aria-label describing its purpose.
-<Link to={userLink} className="flex items-center">
+<Link
+ to={userLink}
+ className="flex items-center"
+ aria-label={`View ${pageCard.user.displayName}'s profile`}
+>📝 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.
| <Link to={userLink} className="flex items-center"> | |
| <Avatar className="w-6 h-6 mr-2"> | |
| <AvatarImage | |
| src={pageCard.user.icon} | |
| alt={pageCard.user.displayName} | |
| /> | |
| <AvatarFallback> | |
| {pageCard.user.displayName.charAt(0).toUpperCase()} | |
| </AvatarFallback> | |
| </Avatar> | |
| <span className="text-sm text-gray-600"> | |
| {pageCard.user.displayName} | |
| </span> | |
| </Link> | |
| <Link | |
| to={userLink} | |
| className="flex items-center" | |
| aria-label={`View ${pageCard.user.displayName}'s profile`} | |
| > | |
| <Avatar className="w-6 h-6 mr-2"> | |
| <AvatarImage | |
| src={pageCard.user.icon} | |
| alt={pageCard.user.displayName} | |
| /> | |
| <AvatarFallback> | |
| {pageCard.user.displayName.charAt(0).toUpperCase()} | |
| </AvatarFallback> | |
| </Avatar> | |
| <span className="text-sm text-gray-600"> | |
| {pageCard.user.displayName} | |
| </span> | |
| </Link> |
| <CardTitle className="flex items-center pr-3 break-all overflow-wrap-anywhere"> | ||
| {!pageCard.isPublished && <Lock className="h-4 w-4 mr-2" />} | ||
| {pageCard.title} | ||
| </CardTitle> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-label for better accessibility
The lock icon needs an aria-label for screen readers.
<CardTitle className="flex items-center pr-3 break-all overflow-wrap-anywhere">
- {!pageCard.isPublished && <Lock className="h-4 w-4 mr-2" />}
+ {!pageCard.isPublished && (
+ <Lock className="h-4 w-4 mr-2" aria-label="Private page" />
+ )}
{pageCard.title}
</CardTitle>📝 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.
| <CardTitle className="flex items-center pr-3 break-all overflow-wrap-anywhere"> | |
| {!pageCard.isPublished && <Lock className="h-4 w-4 mr-2" />} | |
| {pageCard.title} | |
| </CardTitle> | |
| <CardTitle className="flex items-center pr-3 break-all overflow-wrap-anywhere"> | |
| {!pageCard.isPublished && ( | |
| <Lock className="h-4 w-4 mr-2" aria-label="Private page" /> | |
| )} | |
| {pageCard.title} | |
| </CardTitle> |
| const { pagesWithInfo, totalPages, currentPage } = | ||
| await fetchPaginatedPublicPages(page, 9, currentUser?.id); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for fetchPaginatedPublicPages
The loader should handle potential errors from the database query.
- const { pagesWithInfo, totalPages, currentPage } =
- await fetchPaginatedPublicPages(page, 9, currentUser?.id);
+ try {
+ const { pagesWithInfo, totalPages, currentPage } =
+ await fetchPaginatedPublicPages(page, 9, currentUser?.id);
+ // ... rest of the code
+ } catch (error) {
+ console.error('Failed to fetch pages:', error);
+ throw new Response('Failed to load pages', { status: 500 });
+ }Committable suggestion skipped: line range outside the PR's diff.
| @@ -6,6 +6,7 @@ const prisma = new PrismaClient(); | |||
| interface SeedText { | |||
| text: string; | |||
| number: number; | |||
| textAndOccurrenceHash: string; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a hash function instead of hardcoded values
The textAndOccurrenceHash values are currently hardcoded. Consider using a hash function to generate these values consistently.
import { createHash } from 'crypto';
function generateTextHash(text: string): string {
return createHash('sha256')
.update(text)
.digest('hex')
.substring(0, 32); // Use first 32 chars for brevity
}
// Usage in seed data:
textAndOccurrenceHash: generateTextHash(text),Also applies to: 32-32, 44-45, 57-57, 69-70
Summary by CodeRabbit
New Features
PageCardcomponent for displaying page information, including title, creation date, and user details.HomeandUserPagecomponents to utilize the newPageCardfor rendering pages.Bug Fixes
UserProfiletests.Documentation
Chores