feat: pr cards #46
feat: pr cards #46
Conversation
WalkthroughThe changes update the logic for calculating the time to first approval in pull request tracking by using additional dates and a helper function. Additionally, the GraphQL queries and types now include a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 1
🧹 Nitpick comments (11)
apps/web/src/components/card-pull-request/card-pull-request.tsx (2)
34-35: Hook usage seems clean.
LeveragingusePrCardto encapsulate pull request–specific logic (e.g.,getTimeLabel) is a solid approach. Consider supplementing with unit tests to ensure stable behavior.
41-148: Consider lazy-loading theTimelinePullRequest.
Wrapping the timeline in aHoverCardis a great UX improvement. For performance-sensitive apps, you might load timeline details on demand (e.g., upon hover) to reduce initial bundle size.apps/web/src/components/card-pull-request/icon-pull-request-state.tsx (1)
17-17: Favor theme-based colors over hard-coded hex.
Usingvariant="transparent"is a good move for a cleaner look, but consider referencing theme constants instead of hex codes to ensure consistent updates across the app.Also applies to: 25-25, 33-33, 41-41
apps/web/src/components/card-pull-request/badge-status.tsx (2)
10-23: Consider aligning colors with Mantine’s theme system.
Hard-coding color variables is flexible, but leveraging the Mantine theme or a shared constants file may reduce styling discrepancies and make global updates simpler.
25-38: Implementation is straightforward and effective.
The extractedBadgeStatuscomponent is cohesive. For a lighter footprint, you could also explore Mantine’s built-in<Badge>component, though your approach gives you more control.apps/web/src/components/card-pull-request/use-pr-card.tsx (1)
16-26: Consider making comment thresholds configurable.The hardcoded thresholds (30, 20) for comment count visualization could be made configurable through theme variables or constants.
+const COMMENT_THRESHOLDS = { + HIGH: 30, + MEDIUM: 20, +} as const; + const getCommentFlameProps = (comments: number) => { - if (comments >= 30) { + if (comments >= COMMENT_THRESHOLDS.HIGH) { return { color: theme.colors.red[8], fill: theme.colors.red[8] }; } - if (comments >= 20) { + if (comments >= COMMENT_THRESHOLDS.MEDIUM) { return { color: theme.colors.red[6] }; } return { color: theme.colors.red[4] }; };apps/web/src/components/card-pull-request/use-badges.tsx (3)
26-52: Consider making time thresholds configurable.The hook uses hardcoded time thresholds (24h, 48h) for determining badge states. Consider making these configurable to allow for different team workflows and requirements.
+interface BadgeThresholds { + review: { + warning: number; + error: number; + success: number; + }; + approval: { + warning: number; + error: number; + success: number; + }; +} + +const defaultThresholds: BadgeThresholds = { + review: { + warning: 24, + error: 48, + success: 2, + }, + approval: { + warning: 24, + error: 48, + success: 2, + }, +}; + -export const useBadges = (pullRequest: PullRequest) => { +export const useBadges = ( + pullRequest: PullRequest, + thresholds: BadgeThresholds = defaultThresholds, +) => {
38-46: Consider centralizing color values in a theme configuration.The color values are hardcoded in the component. Consider moving them to a centralized theme configuration for better maintainability and consistency.
+const badgeColors = { + error: { + start: 'var(--mantine-color-red-5)', + end: 'var(--mantine-color-pink-5)', + }, +}; + if (hasBadgeOf("error")) { - return { - start: "var(--mantine-color-red-5)", - end: "var(--mantine-color-pink-5)", - }; + return badgeColors.error; }
1-179: Add unit tests for badge generation logic.The badge generation logic is complex and would benefit from comprehensive unit tests to ensure reliability.
Would you like me to generate unit test cases for the badge generation logic? The tests would cover various scenarios such as:
- Badge states for different time thresholds
- Color determination based on badge states
- Edge cases for merged/closed pull requests
apps/web/src/components/card-pull-request/timeline-pull-request.tsx (2)
28-30: Consider centralizing color values in a theme configuration.The color values are hardcoded in the component. Consider moving them to a centralized theme configuration for better maintainability and consistency.
+const timelineColors = { + success: 'var(--mantine-color-green-4)', + error: 'var(--mantine-color-red-4)', + warning: 'var(--mantine-color-yellow-4)', +}; + -const successColor = "var(--mantine-color-green-4)"; -const errorColor = "var(--mantine-color-red-4)"; -const warningColor = "var(--mantine-color-yellow-4)";
114-119: Consider using a localization system for text strings.Text strings are hardcoded in the component. Consider moving them to a localization system for better maintainability and internationalization support.
+const timelineMessages = { + draft: 'Drafted', + opened: 'Opened', + firstReviewed: 'First reviewed', + firstReview: 'First review', + approved: 'Approved', + approval: 'Approval', + closed: 'Closed', + merged: 'Merged', + merge: 'Merge', + in: 'in', + awaitingFor: 'awaiting for', + skipped: 'Skipped', +}; + -title={isDraft ? "Drafted" : "Opened"} +title={isDraft ? timelineMessages.draft : timelineMessages.opened}Also applies to: 129-141, 145-166, 169-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts(1 hunks)apps/web/src/api/pull-request.api.ts(1 hunks)apps/web/src/app/home/components/my-open-pull-requests/my-open-pull-requests.tsx(1 hunks)apps/web/src/app/home/components/team-open-pull-requests/team-open-pull-requests.tsx(1 hunks)apps/web/src/app/people/[handle]/pull-requests/page.tsx(1 hunks)apps/web/src/app/teams/[id]/pull-requests/page.tsx(1 hunks)apps/web/src/components/card-pull-request/badge-code-review-time.tsx(0 hunks)apps/web/src/components/card-pull-request/badge-merge-time.tsx(0 hunks)apps/web/src/components/card-pull-request/badge-status.tsx(1 hunks)apps/web/src/components/card-pull-request/card-pull-request.module.css(1 hunks)apps/web/src/components/card-pull-request/card-pull-request.tsx(1 hunks)apps/web/src/components/card-pull-request/icon-pull-request-state.tsx(1 hunks)apps/web/src/components/card-pull-request/timeline-pull-request.tsx(1 hunks)apps/web/src/components/card-pull-request/use-badges.tsx(1 hunks)apps/web/src/components/card-pull-request/use-pr-card.tsx(1 hunks)packages/graphql-types/frontend/gql.ts(2 hunks)packages/graphql-types/frontend/graphql.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/web/src/components/card-pull-request/badge-merge-time.tsx
- apps/web/src/components/card-pull-request/badge-code-review-time.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/components/card-pull-request/card-pull-request.module.css
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/src/components/card-pull-request/timeline-pull-request.tsx
[error] 139-139: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 164-164: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (12)
apps/web/src/components/card-pull-request/card-pull-request.tsx (3)
16-24: Imports look well-organized.
No issues found with the new and updated imports; breaking out functionality into specialized modules (e.g.,usePrCard,useBadges) helps keep the component thin and focused.
27-30: Good prop consolidation into a singlepullRequestobject.
This design choice simplifies prop management and makes the component easier to maintain. Keep up the modular approach.
36-38: Badge logic is well abstracted.
Extracting badge logic intouseBadgesand rendering them viaBadgeStatusis a neat approach. It promotes reusability and keeps code organized.Also applies to: 86-99
apps/web/src/components/card-pull-request/use-pr-card.tsx (1)
9-44: LGTM! Well-structured hook implementation.The hook effectively encapsulates pull request display logic, making it reusable across components.
apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts (1)
37-41: LGTM! Improved time to first approval calculation.The calculation now correctly considers multiple date fields in order of priority:
firstApprovalAt,firstReadyAt, orprCreatedAt.apps/web/src/api/pull-request.api.ts (1)
66-66: LGTM! Added firstApprovalAt field.The field addition aligns with the tracking transformer changes and enhances pull request data.
apps/web/src/app/home/components/my-open-pull-requests/my-open-pull-requests.tsx (1)
83-85: LGTM! Simplified pull request rendering.The refactor reduces prop drilling by passing the entire pull request object, making the code more maintainable and aligned with the single responsibility principle.
apps/web/src/app/home/components/team-open-pull-requests/team-open-pull-requests.tsx (1)
93-95: LGTM! Clean refactor to use a single pullRequest prop.The changes simplify the component's interface by passing the entire pull request object instead of individual props, making the code more maintainable and reducing prop drilling.
apps/web/src/app/people/[handle]/pull-requests/page.tsx (1)
105-105: LGTM! Consistent refactor to use a single pullRequest prop.The changes maintain consistency with other components by using the consolidated pull request object.
apps/web/src/app/teams/[id]/pull-requests/page.tsx (1)
186-186: LGTM! Improved prop passing pattern.The change to pass a single
pullRequestobject instead of individual props is a good improvement that reduces prop drilling and makes the component more maintainable.packages/graphql-types/frontend/gql.ts (1)
40-40: LGTM! GraphQL query properly updated.The PullRequests query has been correctly updated to include the new
firstApprovalAtfield in the tracking object, maintaining type safety and schema consistency.Also applies to: 1044-1044
packages/graphql-types/frontend/graphql.ts (1)
923-923: LGTM! Type definitions properly updated.The PullRequestsQuery type has been correctly updated to include the new
firstApprovalAtfield with proper typing, ensuring type safety throughout the application.
| {hasReviews && timeToFirstReview && ( | ||
| <>in {humanizeDuration(timeToFirstReview)}</> | ||
| )} | ||
| {!hasReviews && timeToFirstReview && ( | ||
| <>awaiting for {humanizeDuration(timeToFirstReview)}</> | ||
| )} | ||
| {!hasReviews && isDone && <>Skipped</>} | ||
| </Text> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary Fragment usage.
The Fragment wrapper is unnecessary when there's only one child element.
{hasReviews && timeToFirstReview && (
- <>in {humanizeDuration(timeToFirstReview)}</>
+ `in ${humanizeDuration(timeToFirstReview)}`
)}
{!hasReviews && timeToFirstReview && (
- <>awaiting for {humanizeDuration(timeToFirstReview)}</>
+ `awaiting for ${humanizeDuration(timeToFirstReview)}`
)}
{!hasReviews && isDone && (
- <>Skipped</>
+ "Skipped"
)}📝 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.
| {hasReviews && timeToFirstReview && ( | |
| <>in {humanizeDuration(timeToFirstReview)}</> | |
| )} | |
| {!hasReviews && timeToFirstReview && ( | |
| <>awaiting for {humanizeDuration(timeToFirstReview)}</> | |
| )} | |
| {!hasReviews && isDone && <>Skipped</>} | |
| </Text> | |
| {hasReviews && timeToFirstReview && ( | |
| `in ${humanizeDuration(timeToFirstReview)}` | |
| )} | |
| {!hasReviews && timeToFirstReview && ( | |
| `awaiting for ${humanizeDuration(timeToFirstReview)}` | |
| )} | |
| {!hasReviews && isDone && "Skipped"} | |
| </Text> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
Summary by CodeRabbit
New Features
Refactor
Style