Conversation
|
@ddoemonn is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
ddoemonn
changed the title
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/02/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/02/25)1 label was added to this PR based on Keith Williams's automation. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic found 2 issues across 3 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
Devanshusharma2005
left a comment
Devanshusharma2005
left a comment
There was a problem hiding this comment.
Please address the cubic and yarn issues. Marking it draft for now. Feel free to rfr.
|
This PR is being marked as stale due to inactivity. |
WalkthroughThe changes introduce two new custom hooks, Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. All modifications are directly related to the objectives of centralizing and refactoring state management in the ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
♻️ Duplicate comments (2)
apps/web/components/booking/hooks/useBookingFormState.ts (2)
3-9: LGTM! Proper TypeScript interfaces implemented.The
BookingAttendeeinterface provides proper type safety with well-defined fields. This successfully addresses the previous concern about usingany[]types.
49-58: LGTM! Actions object properly memoized.The actions object is now correctly wrapped in
useMemowith appropriate dependencies, ensuring a stable reference and preventing unnecessary re-renders. This addresses the previous performance concern.
🧹 Nitpick comments (1)
apps/web/components/booking/hooks/useDialogState.ts (1)
15-17: Improve type safety in DialogState interface.The current
DialogStateinterface uses generic string indexing, which reduces type safety. Consider using theDialogTypeunion for stronger typing:-interface DialogState { - [key: string]: boolean; -} +type DialogState = { + [K in DialogType]: boolean; +};This ensures only valid dialog types can be used as keys and provides better IntelliSense support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/components/booking/BookingListItem.tsx(19 hunks)apps/web/components/booking/hooks/useBookingFormState.ts(1 hunks)apps/web/components/booking/hooks/useDialogState.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/components/booking/BookingListItem.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/components/booking/BookingListItem.tsxapps/web/components/booking/hooks/useDialogState.tsapps/web/components/booking/hooks/useBookingFormState.ts
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/components/booking/hooks/useDialogState.tsapps/web/components/booking/hooks/useBookingFormState.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/components/booking/BookingListItem.tsx (1)
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/components/booking/hooks/useDialogState.ts (1)
Learnt from: alishaz-polymath
PR: #22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the currentLink.maxUsageCount ?? 1 fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
apps/web/components/booking/hooks/useBookingFormState.ts (3)
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{ts,tsx} : Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops
🔇 Additional comments (9)
apps/web/components/booking/hooks/useDialogState.ts (3)
27-38: LGTM! Well-structured state initialization.The explicit initialization of all dialog states prevents undefined values and ensures consistent behavior. The TypeScript typing is properly applied.
40-67: LGTM! Proper memoization and functional updates.The action functions are well-implemented with:
- Correct use of functional state updates to prevent stale closures
- Proper memoization with appropriate dependency arrays
- Clear and explicit state management
69-81: LGTM! Excellent backward compatibility design.The
getDialogSetterfunction provides seamless compatibility with React's standard state setter pattern, handling both direct boolean values and updater functions correctly. This design allows easy migration from individualuseStatehooks.apps/web/components/booking/hooks/useBookingFormState.ts (1)
33-47: LGTM! Well-implemented callback functions.All state-modifying functions use proper functional updates and are correctly memoized with
useCallback. The dependency arrays are appropriate for these implementations.apps/web/components/booking/BookingListItem.tsx (5)
57-59: LGTM! Clean hook integration and centralized state management.The import and initialization of both custom hooks successfully centralizes what were previously scattered
useStatecalls throughout the component. The clear commenting helps document the architectural change.Also applies to: 122-124
151-151: LGTM! Consistent dialog action usage.All dialog actions follow a consistent pattern using the centralized
dialogActions.openDialog()anddialogActions.closeDialog()methods. The dialog type strings match theDialogTypeunion from the hook.Also applies to: 223-223, 264-264, 274-274, 284-284, 295-295, 307-307, 329-329, 360-360
199-199: LGTM! Clean form state integration.The form state usage consistently leverages the centralized
formStatefor reading values andformActionsfor updates, successfully replacing scattered local state management.Also applies to: 551-552
484-485: LGTM! Flexible dialog component integration.The dialog components successfully integrate with the centralized state using both
getDialogSetter()for direct compatibility and inline functions for explicit control. Both patterns work well with the new state management architecture.Also applies to: 488-491, 500-501, 505-506, 511-512, 521-522, 530-534, 537-540, 752-754
112-760: Excellent refactoring that successfully centralizes state management.This refactoring successfully achieves the PR objectives by:
- Consolidating 15+ scattered
useStatehooks into two centralized custom hooks- Improving separation of concerns between UI and state logic
- Maintaining full backward compatibility with existing functionality
- Providing consistent APIs for dialog and form state management
- Enhancing testability through isolated state logic
The implementation follows React best practices and significantly improves code maintainability without introducing any breaking changes.
|
This PR is being marked as stale due to inactivity. |
dhairyashiil
left a comment
dhairyashiil
left a comment
There was a problem hiding this comment.
Can you resolve the merge conflicts? making it draft until then
4 participants
What does this PR do?
Refactors the BookingListItem component to extract state management into centralized custom hooks, addressing technical debt and improving maintainability. This change consolidates 15+ scattered
useStatehooks into two focused custom hooks:useDialogStatefor dialog management anduseBookingFormStatefor form inputs.Key Changes:
useDialogStatehook - Centralizes all dialog visibility state managementuseBookingFormStatehook - Manages form inputs like rejection reasonsuseStatecalls - Consolidated 15+ individual state declarationsMandatory Tasks (DO NOT REMOVE)
How should this be tested?
Environment Setup:
Test Scenarios:
Dialog Functionality:
Form State Management:
Booking Actions:
Expected Behavior:
Test Data:
Checklist
Additional Notes:
This refactor maintains 100% backward compatibility - no user-facing changes. The improvement is purely internal code quality, making the component more maintainable and testable for future development.
Summary by cubic
Refactored BookingListItem to use two custom hooks for dialog and form state, replacing over 15 scattered useState calls and making the code easier to maintain.