♻️ Keep guest ids in separate column #1468
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces comprehensive changes to support guest user functionality across the application. The modifications span multiple files and involve adding a new Changes
Possibly Related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (4)
apps/web/src/contexts/permissions.tsx (1)
53-56: Consolidate repeated checks intoownsObjectif possible
The additional check for context user IDs essentially duplicatesownsObject. Consider delegating all checks toownsObjectfor more concise code.apps/web/src/components/poll/desktop-poll.tsx (1)
311-311: Avoid confusion betweenuserIdandguestId.Passing both
userIdandguestIdtoParticipantRowis flexible but can be confusing. Consider clear naming or a single field that differentiates guests vs. registered users, if that simplifies usage.apps/web/src/components/poll/desktop-poll/participant-row.tsx (1)
112-112: Simplify boolean expressionThe ternary operator is unnecessary here since
ownsObject()already returns a boolean.- const isYou = ownsObject(participant) ? true : false; + const isYou = ownsObject(participant);🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
apps/web/src/components/discussion/discussion.tsx (1)
163-163: Consider paginating comments queryThe comments query fetches all comments at once. For discussions with many comments, this could impact performance.
Consider implementing pagination or infinite scroll to improve performance for large discussions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/web/src/app/[locale]/(admin)/polls/user-polls.tsx(1 hunks)apps/web/src/components/discussion/discussion.tsx(2 hunks)apps/web/src/components/poll/desktop-poll.tsx(1 hunks)apps/web/src/components/poll/desktop-poll/participant-row.tsx(2 hunks)apps/web/src/components/user-provider.tsx(3 hunks)apps/web/src/contexts/permissions.tsx(2 hunks)apps/web/src/trpc/routers/polls.ts(3 hunks)apps/web/src/trpc/routers/polls/comments.ts(1 hunks)apps/web/src/trpc/routers/polls/participants.ts(1 hunks)apps/web/src/utils/permissions.ts(1 hunks)packages/database/prisma/migrations/20241224103150_guest_id_column/migration.sql(1 hunks)packages/database/prisma/schema.prisma(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/src/components/poll/desktop-poll/participant-row.tsx
[error] 112-112: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (29)
apps/web/src/trpc/routers/polls.ts (3)
76-78: Consistent guest/user ID assignment [Approved]
This conditional spread operator correctly handles guest vs. registered user IDs.
99-105: Check null scenario for user field when guestId is present
When guestId is populated, the user field might be null. Ensure the frontend gracefully handles the possibility of poll.user being null.
175-177: Matches the guest/user logic in poll creation [Approved]
The same conditional ID assignment is applied here, maintaining consistency.
apps/web/src/utils/permissions.ts (1)
1-10: Ownership logic is straightforward and clear [Approved]
This method robustly distinguishes users from guests, ensuring correct ownership checks.
apps/web/src/contexts/permissions.tsx (2)
29-29: Destructuring ownsObject from context
Pulling in ownsObject is a clean way to centralize ownership checks across the app.
48-50: Improved null check for participants
Returning false when participant is not found prevents potential runtime issues.
apps/web/src/components/user-provider.tsx (3)
12-12: Importing isOwner
Importing isOwner ensures a single authority for ownership checks, improving maintainability.
32-35: Expanding signature to include guestId
Accepting an optional guestId standardizes ownership checks for both guests and registered users.
108-109: Delegation to isOwner
Delegating ownership logic cleanly unifies the user context with the permission utilities.
apps/web/src/trpc/routers/polls/participants.ts (1)
98-98: Conditional ID assignment on participant creation [Approved]
Properly assigns either guestId or userId based on the user’s status, staying consistent with the rest of the PR.
apps/web/src/app/[locale]/(admin)/polls/user-polls.tsx (1)
185-189: Consider clarifying how the user vs. guest fields are handled.
Allowing user to be null alongside an optional guestId provides flexibility for multiple user states. Ensure downstream usage consistently checks both fields so that logic remains clear when either is null.
packages/database/prisma/schema.prisma (6)
127-128: Ensure all references to userId handle nullable fields correctly.
Changing to String? allows polls without a registered user. Confirm that logic (in application code) properly covers cases where userId is now null, especially for existing references or constraints.
151-151: Check usage of the guest ID index.
The new guest ID index on Poll boosts query performance for guest users. Ensure queries that rely on the guest ID actually utilize this index.
189-189: Confirm participant creation for guests.
The optional guestId field in Participant allows storing guest data. Verify that participant creation logic properly sets guestId when dealing with guests, and ensure no collisions with userId.
200-200: Validate index usage on guestId in Participant.
This aligns with the Poll table’s approach. Double-check whether usage in TRPC resolvers or direct queries benefits from the new index.
248-248: Support guest-based comments in the application logic.
Adding guestId to Comment enables guest commenting. Ensure that conditional logic in comment creation/resolution consistently populates this field.
253-253: Leverage the new index on comment guestId.
This index should help performance when listing or filtering comments from guests. Confirm that relevant queries are in place.
packages/database/prisma/migrations/20241224103150_guest_id_column/migration.sql (9)
2-2: Added guest_id to comments.
This supports guest functionality but ensure references to this field in code are aligned with the new column.
5-5: Added guest_id to participants.
Allows distinguishing guests in participant records. Confirm your queries properly differentiate userId and guestId.
8-9: Polls table updated for guest usage.
Dropping NOT NULL from user_id and adding guest_id is a pivotal design change. Validate that existing poll logic gracefully handles possibly null user_id.
12-12: Index on comments_guest_id_idx.
Indexing guest_id on the comments table can enhance queries but verify the code patterns are indeed using this for filtering.
15-15: Index on participants_guest_id_idx.
Facilitates guest-based participant lookups. Confirm usage in TRPC or relevant controllers.
18-18: Index on polls_guest_id_idx.
Check that the actual queries filtering by guest_id in polls are present and optimized.
21-31: Poll migration logic for unknown user_id.
The migration sets guest_id to user_id if no matching user exists. Confirm that this fallback does not cause collisions or data integrity issues for existing user records.
33-45: Participants migration logic for unknown user_id.
Similar fallback approach ensures participants with invalid userId become guests. Confirm that subsequent code properly resolves them as guests and not partial users.
46-57: Comments migration logic and data consistency.
For comments referencing missing users, guest_id replaces user_id. Double-check that older references and user relationships are seamlessly replaced.
apps/web/src/trpc/routers/polls/comments.ts (1)
44-46: Ensure mutual exclusivity of userId and guestId
The conditional assignment looks correct but could be more explicit about the mutual exclusivity of these fields.
Let's verify the database schema and existing data:
✅ Verification successful
Mutual exclusivity of userId and guestId is properly enforced
Based on the Prisma schema and implementation:
- The Comment model correctly defines
userIdandguestIdas optional String fields with proper indexes - The comment creation logic in the router uses a spread operator to ensure only one of these fields is set based on
ctx.user.isGuest - The schema doesn't enforce mutual exclusivity at the database level, but the application logic correctly handles this by using conditional assignment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any comments have both userId and guestId set
ast-grep --pattern 'model Comment {
$$$
userId String?
guestId String?
$$$
}'
# Check for potential data integrity issues
rg -A 5 'Comment.*create'
Length of output: 540
Script:
#!/bin/bash
# Let's check the Prisma schema for Comment model constraints
fd 'schema.prisma' --exec cat {}
# Let's also check for any other Comment creation logic
rg -B 3 -A 3 'Comment.*create|createMany'
Length of output: 10902
apps/web/src/components/poll/desktop-poll/participant-row.tsx (1)
26-26: LGTM: Interface update for guest support
The addition of the optional guestId field to the participant interface correctly supports the guest user functionality.
apps/web/src/components/discussion/discussion.tsx (1)
203-203: LGTM: Proper permission handling
The permission check correctly handles both admin roles and object ownership, supporting the new guest user functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/auth/merge-user.ts (2)
14-14: Consider using transactions if partial failures are a concern.
SettingguestIdtonulland assigninguserIdin multiple separateupdateManycalls can lead to partial merges if any of these calls fail. Consider using a single transaction (prisma.$transaction) so that either all or none of the updates are committed.
26-26: Optional: Parallelize updates for performance.
All threeupdateManycalls are awaited sequentially. If these changes do not need to be applied in a strict order, consider running them in parallel usingawait Promise.all(...)to reduce the overall execution time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/auth/merge-user.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/src/auth/merge-user.ts (1)
Learnt from: lukevella
PR: lukevella/rallly#1189
File: apps/web/src/utils/auth.ts:202-202
Timestamp: 2024-11-12T09:08:14.221Z
Learning: The `mergeGuestsIntoUser()` function in `apps/web/src/utils/auth.ts` is correctly awaited.
🔇 Additional comments (3)
apps/web/src/auth/merge-user.ts (3)
9-11: Ensure guestIds list is properly validated.
Although this query filters rows by guestId, consider validating that guestIds is non-empty and contains valid entries before making the database call. This can help prevent unexpected updates or silent errors if the list is empty.
21-23: Maintain consistent logic for participants.
Merging participants by updating the guestId to null and assigning userId ensures a consistent approach across Poll, Participant, and Comment tables. It's good to see that the same pattern is followed here.
33-35: Confirm referencing correctness.
Double-check that guestIds aligns with how guest IDs are tracked in the comment table. If there's a mismatch in naming or foreign key references, this could potentially update unintended rows.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/auth/merge-user.ts (2)
7-44: Good use of transactions for data consistency!The implementation correctly uses a transaction to ensure atomic updates across multiple tables, with efficient parallel processing via
Promise.all.Consider adding error logging to track failed merges, as they could leave guest data in an inconsistent state:
export const mergeGuestsIntoUser = async ( userId: string, guestIds: string[], ) => { + console.log(`Merging guest IDs ${guestIds.join(', ')} into user ${userId}`); return await prisma.$transaction(async (tx) => { await Promise.all([ // ... existing code ... ]); + console.log('Successfully merged guest accounts'); }); };
8-44: Consider adding retry logic for resilience.While transactions provide atomicity, they might fail due to temporary database issues. For a more robust implementation, consider wrapping the operation in a retry mechanism.
Here's a suggested implementation using exponential backoff:
import { exponentialBackoff } from '@rallly/backend/utils'; export const mergeGuestsIntoUser = async ( userId: string, guestIds: string[], ) => { return await exponentialBackoff( async () => { return await prisma.$transaction(async (tx) => { await Promise.all([ // ... existing code ... ]); }); }, { maxAttempts: 3, initialDelay: 100, } ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/src/app/[locale]/invite/[urlId]/invite-page.tsx(1 hunks)apps/web/src/auth/merge-user.ts(1 hunks)apps/web/src/contexts/permissions.tsx(2 hunks)apps/web/src/trpc/routers/polls.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/contexts/permissions.tsx
- apps/web/src/trpc/routers/polls.ts
🧰 Additional context used
📓 Learnings (1)
apps/web/src/auth/merge-user.ts (1)
Learnt from: lukevella
PR: lukevella/rallly#1189
File: apps/web/src/utils/auth.ts:202-202
Timestamp: 2024-11-12T09:08:14.221Z
Learning: The `mergeGuestsIntoUser()` function in `apps/web/src/utils/auth.ts` is correctly awaited.
🔇 Additional comments (2)
apps/web/src/auth/merge-user.ts (1)
9-19: Verify the completeness of the merge operation.
The code updates three tables (polls, participants, comments), but there might be other tables or relations that need to be considered.
Let's check for other potential tables that might have guest references:
Also applies to: 21-31, 33-43
✅ Verification successful
Let me try a better search pattern to find table definitions.
The merge operation appears to be complete
Based on the schema.prisma file, the code correctly handles all tables that have a guestId column:
pollstableparticipantstablecommentstable
These are the only tables in the schema that contain guest references, and the merge operation in the code updates all of them. The other tables either don't have guest-related fields or are not relevant to the guest-to-user conversion process (like accounts, verification tokens, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for table definitions that might contain guest_id
fd -e prisma$ | xargs rg -A 5 'model.*{' | rg -B 5 -A 10 'guest.*Id'
# Search for any other potential guest-related fields in the codebase
rg -g '!*.{log,lock}' -i 'guest.*id'
Length of output: 8233
Script:
#!/bin/bash
# Search for table definitions in schema.prisma
cat packages/database/prisma/schema.prisma
Length of output: 7995
apps/web/src/app/[locale]/invite/[urlId]/invite-page.tsx (1)
19-20: LGTM! Good refactoring of ownership check.
The change to use ownsObject centralizes the ownership logic in the useUser hook, making it more maintainable and consistent with the guest user support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/src/trpc/routers/dashboard.ts (1)
17-18: Consider addressing the TODO comment about status enum.The TODO comment suggests moving
deletedstatus into an enum. This would indeed provide better type safety and make the status handling more maintainable.Would you like me to help implement this improvement by:
- Creating an appropriate status enum that includes deleted/cancelled states
- Updating the schema and related code to use this enum
- Opening a GitHub issue to track this enhancement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/trpc/routers/dashboard.ts(1 hunks)
🔇 Additional comments (1)
apps/web/src/trpc/routers/dashboard.ts (1)
9-15: LGTM! Clean implementation of guest user filtering.
The conditional spread operator usage provides a clean way to switch between guestId and userId based on the user type. The implementation maintains mutual exclusivity between guest and registered users.
Summary by CodeRabbit
New Features
guestIdproperty to participant and comment data models.Bug Fixes
Documentation
guestIdfields and adjustments to user associations.Chores
guest_idcolumns and improve query performance with new indexes.