chore(rethinkdb): SuggestedAction: Phase 3 #10043
Conversation
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
WalkthroughThe recent changes involve a significant shift from RethinkDB to PostgreSQL, enhancing various functionalities such as user bootstrap processes and suggested actions. Updates to type definitions, GraphQL queries, and resolver types improve consistency and type safety. Overall, these modifications streamline data access and manipulation across the application, leading to greater architectural efficiency and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant GraphQL
participant PostgreSQL
User->>Frontend: Request action (e.g., sign up)
Frontend->>GraphQL: Call signUpWithPassword
GraphQL->>PostgreSQL: Insert user data
PostgreSQL-->>GraphQL: Confirmation
GraphQL-->>Frontend: Success response
Frontend-->>User: Display confirmation message
sequenceDiagram
participant User
participant Frontend
participant GraphQL
participant PostgreSQL
User->>Frontend: Request email verification
Frontend->>GraphQL: Call verifyEmail
GraphQL->>PostgreSQL: Check verification token
PostgreSQL-->>GraphQL: Confirmation
GraphQL-->>Frontend: Verification success
Frontend-->>User: Notify verification success
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 as PR comments)
Additionally, you can add 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: 2
Outside diff range, codebase verification and nitpick comments (5)
packages/server/graphql/types/User.ts (1)
3-5: EmptyUsertype.The
Usertype has been reduced to an empty object. This change will likely break many existing queries and mutations that rely on user data. Please verify that this change is intentional and part of a broader refactor.
- Location of concern:
packages/server/graphql/types/User.tsAnalysis chain
Empty
Usertype.The
Usertype has been reduced to an empty object. Ensure that this change aligns with the overall refactor and does not break any existing queries or mutations relying on user data.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of fields from the `User` type does not break any existing queries or mutations. # Test: Search for the usage of `User` type in the codebase. Expect: No broken queries or mutations. rg --type graphql -A 5 $'User'Length of output: 61
Script:
#!/bin/bash # Description: Verify that the removal of fields from the `User` type does not break any existing queries or mutations. # Find all GraphQL files fd -e graphql -e gql -e graphqls > graphql_files.txt # Search for the usage of `User` type in the found GraphQL files if [ -s graphql_files.txt ]; then rg 'User' $(cat graphql_files.txt) else echo "No GraphQL files found." fiLength of output: 129703
packages/server/dataloader/rethinkPrimaryKeyLoaderMakers.ts (1)
Verify removal of
suggestedActions.The
suggestedActionsis still referenced in multiple places in the codebase. Its removal from the export inrethinkPrimaryKeyLoaderMakers.tsmight break dependent code. Please review the following files for potential issues:
packages/server/safeMutations/acceptTeamInvitation.tspackages/server/graphql/public/typeDefs/User.graphqlpackages/server/graphql/mutations/dismissSuggestedAction.tspackages/server/graphql/public/types/User.tspackages/server/dataloader/primaryKeyLoaderMakers.tspackages/server/dataloader/foreignKeyLoaderMakers.tspackages/client/mutations/handlers/handleRemoveSuggestedActions.tspackages/client/mutations/AcceptTeamInvitationMutation.tspackages/client/components/TimelineSuggestedAction.tsxAnalysis chain
Line range hint
1-1:
Verify removal ofsuggestedActions.Ensure that the removal of
suggestedActionsdoes not break any dependent code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `suggestedActions` is not referenced anywhere in the codebase. # Test: Search for references to `suggestedActions`. Expect: No occurrences found. rg --type python -A 5 $'suggestedActions'Length of output: 71
Script:
#!/bin/bash # Description: Verify that `suggestedActions` is not referenced anywhere in the codebase. # Test: Search for references to `suggestedActions`. Expect: No occurrences found. rg 'suggestedActions'Length of output: 1822
packages/server/dataloader/rethinkForeignKeyLoaderMakers.ts (1)
Issue with removal of
suggestedActionsByUserIdThe export
suggestedActionsByUserIdis still being referenced in multiple files, which will cause runtime errors if it is removed.
- File:
packages/server/safeMutations/acceptTeamInvitation.ts
- Line:
const suggestedActions = await dataLoader.get('suggestedActionsByUserId').load(userId)- File:
packages/server/dataloader/foreignKeyLoaderMakers.ts
- Line:
export const suggestedActionsByUserId = foreignKeyLoaderMaker(- File:
packages/server/graphql/public/types/User.ts
- Line:
const suggestedActions = await dataLoader.get('suggestedActionsByUserId').load(userId)Please ensure that all references to
suggestedActionsByUserIdare removed or replaced appropriately before removing the export.Analysis chain
Line range hint
1-1:
Verify the removal ofsuggestedActionsByUserId.Ensure that all references to
suggestedActionsByUserIdare removed from the codebase to prevent any potential issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `suggestedActionsByUserId` are removed from the codebase. # Test: Search for references to `suggestedActionsByUserId`. Expect: No occurrences. rg --type js --type ts 'suggestedActionsByUserId'Length of output: 444
Script:
#!/bin/bash # Extract relevant lines from the files where `suggestedActionsByUserId` is referenced. # Extract lines around the reference in acceptTeamInvitation.ts rg --type js --type ts -A 5 -B 5 'suggestedActionsByUserId' packages/server/safeMutations/acceptTeamInvitation.ts # Extract lines around the reference in foreignKeyLoaderMakers.ts rg --type js --type ts -A 5 -B 5 'suggestedActionsByUserId' packages/server/dataloader/foreignKeyLoaderMakers.ts # Extract lines around the reference in User.ts rg --type js --type ts -A 5 -B 5 'suggestedActionsByUserId' packages/server/graphql/public/types/User.tsLength of output: 1594
packages/server/graphql/public/typeDefs/User.graphql (1)
282-282: Issues found with changing theuserIdsfield's type definition.The stricter requirement for the
userIdsinput (changing from[ID]to[ID!]) will cause issues in the following parts of the codebase whereuserIdscan be null or where null checks are performed:
packages/server/graphql/public/types/User.tspackages/server/graphql/private/types/FlagOverLimitPayload.tspackages/server/database/migrations/20200202171128-notificationsV2.tspackages/client/utils/constructFilterQueryParamURL.tspackages/client/utils/useQueryParameterParser.tspackages/client/mutations/connections/getUserTasksConn.tspackages/client/mutations/connections/getArchivedTasksConn.tspackages/client/components/ArchiveTaskUserRoot.tsxpackages/client/components/ArchiveTaskRoot.tsxA thorough review and refactoring are necessary to handle the stricter type requirement for
userIds.Analysis chain
Verify the impact of changing the
userIdsfield's type definition.Ensure that the stricter requirement for the
userIdsinput (changing from[ID]to[ID!]) does not cause issues in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of changing the `userIds` field's type definition. # Test: Search for the usage of `userIds` in the codebase. Expect: No occurrences of null values in the array. rg --type graphql -A 5 $'userIds'Length of output: 64
Script:
#!/bin/bash # Description: Verify the impact of changing the `userIds` field's type definition. # Test: Search for the usage of `userIds` in the codebase. Expect: No occurrences of null values in the array. rg -A 5 'userIds'Length of output: 98860
Script:
#!/bin/bash # Description: Verify the impact of changing the `userIds` field's type definition. # Test: Search for potential null checks or handling of `userIds` being null in the codebase. rg -A 5 'userIds.*null'Length of output: 5775
packages/server/graphql/public/types/User.ts (1)
123-123: Consider using optional chaining.Change to an optional chain to simplify the code and handle potential
nullorundefinedvalues safely.- startCursor: firstEdge && firstEdge.cursor, + startCursor: firstEdge?.cursor,Tools
Biome
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| const suggestedActions = await dataLoader.get('suggestedActionsByUserId').load(userId) | ||
| const hasTryRetro = suggestedActions.some((sa) => sa.type === 'tryRetroMeeting') | ||
| if (hasTryRetro) return null | ||
| const actions = [ | ||
| { | ||
| id: generateUID(), | ||
| createdAt: now, | ||
| priority: 3, | ||
| removedAt: null, | ||
| teamId, | ||
| type: 'tryRetroMeeting' as const, | ||
| userId | ||
| }, | ||
| { | ||
| id: generateUID(), | ||
| createdAt: now, | ||
| priority: 4, | ||
| removedAt: null, | ||
| type: 'createNewTeam' as const, | ||
| userId | ||
| }, | ||
| { | ||
| id: generateUID(), | ||
| createdAt: now, | ||
| priority: 5, | ||
| removedAt: null, | ||
| teamId, | ||
| type: 'tryActionMeeting' as const, | ||
| userId | ||
| } | ||
| ] | ||
| await getKysely().insertInto('SuggestedAction').values(actions).execute() |
There was a problem hiding this comment.
Improved logic and efficiency.
The changes to use a data loader and construct an array of action objects for insertion improve the efficiency and maintainability of the function.
Consider extracting the action object construction into a separate helper function for better readability.
const createAction = (type: string, priority: number, teamId: string, userId: string, now: Date) => ({
id: generateUID(),
createdAt: now,
priority,
removedAt: null,
teamId,
type,
userId
});
const actions = [
createAction('tryRetroMeeting', 3, teamId, userId, now),
createAction('createNewTeam', 4, teamId, userId, now),
createAction('tryActionMeeting', 5, teamId, userId, now)
];| @@ -77,6 +77,7 @@ | |||
| "Comment": "../../database/types/Comment#default as CommentDB", | |||
| "Company": "./types/Company#CompanySource", | |||
| "CreateGcalEventInput": "./types/CreateGcalEventInput#default", | |||
| "CreateGcalEventInput": "./types/CreateGcalEventInput#default", | |||
There was a problem hiding this comment.
Remove duplicate entry for CreateGcalEventInput.
The entry for CreateGcalEventInput is duplicated, which can cause confusion and potential errors in the code generation process.
- "CreateGcalEventInput": "./types/CreateGcalEventInput#default",Committable suggestion was skipped due to low confidence.
Signed-off-by: Matt Krick <[email protected]>
…n-phase3 Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
packages/server/graphql/public/types/User.ts (2)
4-4: Consider removing unused import.The
MeetingMemberIdimport is not used in the file.-import MeetingMemberId from 'parabol-client/shared/gqlIds/MeetingMemberId'
46-46: Consider renaming the import for clarity.The
ReqResolversimport should be renamed for clarity, as it is used to define theUserresolver.-import {ReqResolvers} from './ReqResolvers' +import {ReqResolvers as UserResolvers} from './ReqResolvers'
| return { | ||
| edges, | ||
| pageInfo: { | ||
| startCursor: firstEdge && firstEdge.cursor, |
There was a problem hiding this comment.
Use optional chaining for safety.
Change to an optional chain to avoid potential runtime errors.
- startCursor: firstEdge && firstEdge.cursor,
+ startCursor: firstEdge?.cursor,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.
| startCursor: firstEdge && firstEdge.cursor, | |
| startCursor: firstEdge?.cursor, |
Tools
Biome
[error] 197-197: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| return { | ||
| edges, | ||
| pageInfo: { | ||
| startCursor: firstEdge && firstEdge.cursor, |
There was a problem hiding this comment.
Use optional chaining for safety.
Change to an optional chain to avoid potential runtime errors.
- endCursor: firstEdge && edges[edges.length - 1]!.cursor,
+ endCursor: firstEdge?.cursor,Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
packages/server/postgres/migrations/1722527208553_SuggestAction-phase2.ts (1)
28-34: Consider defining constants outside the function.Defining
MAX_PG_PARAMS,PG_COLS, andBATCH_SIZEoutside the function can improve readability and maintainability.- const MAX_PG_PARAMS = 65545 - const PG_COLS = ['id', 'createdAt', 'priority', 'removedAt', 'type', 'teamId', 'userId'] as const - const BATCH_SIZE = Math.trunc(MAX_PG_PARAMS / PG_COLS.length) + const MAX_PG_PARAMS = 65545; + const PG_COLS = ['id', 'createdAt', 'priority', 'removedAt', 'type', 'teamId', 'userId'] as const; + const BATCH_SIZE = Math.trunc(MAX_PG_PARAMS / PG_COLS.length);
| let curcreatedAt = r.minval | ||
| let curId = r.minval | ||
| for (let i = 0; i < 1e6; i++) { | ||
| console.log('inserting row', i * BATCH_SIZE, curcreatedAt, curId) | ||
| const rawRowsToInsert = (await r | ||
| .table('SuggestedAction') | ||
| .between([curcreatedAt, curId], [r.maxval, r.maxval], { | ||
| index: 'createdAtId', | ||
| leftBound: 'open', | ||
| rightBound: 'closed' | ||
| }) | ||
| .orderBy({index: 'createdAtId'}) | ||
| .limit(BATCH_SIZE) | ||
| .pluck(...PG_COLS) | ||
| .run()) as SuggestedAction[] | ||
|
|
||
| const rowsToInsert = rawRowsToInsert.map((row) => ({ | ||
| ...row | ||
| })) | ||
| if (rowsToInsert.length === 0) break | ||
| const lastRow = rowsToInsert[rowsToInsert.length - 1] | ||
| curcreatedAt = lastRow.createdAt | ||
| curId = lastRow.id | ||
| try { | ||
| await pg | ||
| .insertInto('SuggestedAction') | ||
| .values(rowsToInsert) | ||
| .onConflict((oc) => oc.doNothing()) | ||
| .execute() | ||
| } catch (e) { | ||
| await Promise.all( | ||
| rowsToInsert.map(async (row) => { | ||
| try { | ||
| await pg | ||
| .insertInto('SuggestedAction') | ||
| .values(row) | ||
| .onConflict((oc) => oc.doNothing()) | ||
| .execute() | ||
| } catch (e) { | ||
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | ||
| console.log(`Skipping ${row.id} because it has no user/team`) | ||
| return | ||
| } | ||
| console.log(e, row) | ||
| } | ||
| }) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Efficient data migration but consider adding progress tracking.
The data migration logic is efficient, but adding progress tracking (e.g., percentage completed) can be helpful for large datasets.
- for (let i = 0; i < 1e6; i++) {
+ const totalRows = await r.table('SuggestedAction').count().run();
+ for (let i = 0; i < Math.ceil(totalRows / BATCH_SIZE); i++) {
+ console.log(`Progress: ${((i * BATCH_SIZE) / totalRows) * 100}%`);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.
| let curcreatedAt = r.minval | |
| let curId = r.minval | |
| for (let i = 0; i < 1e6; i++) { | |
| console.log('inserting row', i * BATCH_SIZE, curcreatedAt, curId) | |
| const rawRowsToInsert = (await r | |
| .table('SuggestedAction') | |
| .between([curcreatedAt, curId], [r.maxval, r.maxval], { | |
| index: 'createdAtId', | |
| leftBound: 'open', | |
| rightBound: 'closed' | |
| }) | |
| .orderBy({index: 'createdAtId'}) | |
| .limit(BATCH_SIZE) | |
| .pluck(...PG_COLS) | |
| .run()) as SuggestedAction[] | |
| const rowsToInsert = rawRowsToInsert.map((row) => ({ | |
| ...row | |
| })) | |
| if (rowsToInsert.length === 0) break | |
| const lastRow = rowsToInsert[rowsToInsert.length - 1] | |
| curcreatedAt = lastRow.createdAt | |
| curId = lastRow.id | |
| try { | |
| await pg | |
| .insertInto('SuggestedAction') | |
| .values(rowsToInsert) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| await Promise.all( | |
| rowsToInsert.map(async (row) => { | |
| try { | |
| await pg | |
| .insertInto('SuggestedAction') | |
| .values(row) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | |
| console.log(`Skipping ${row.id} because it has no user/team`) | |
| return | |
| } | |
| console.log(e, row) | |
| } | |
| }) | |
| ) | |
| } | |
| } | |
| let curcreatedAt = r.minval | |
| let curId = r.minval | |
| const totalRows = await r.table('SuggestedAction').count().run(); | |
| for (let i = 0; i < Math.ceil(totalRows / BATCH_SIZE); i++) { | |
| console.log(`Progress: ${((i * BATCH_SIZE) / totalRows) * 100}%`); | |
| console.log('inserting row', i * BATCH_SIZE, curcreatedAt, curId) | |
| const rawRowsToInsert = (await r | |
| .table('SuggestedAction') | |
| .between([curcreatedAt, curId], [r.maxval, r.maxval], { | |
| index: 'createdAtId', | |
| leftBound: 'open', | |
| rightBound: 'closed' | |
| }) | |
| .orderBy({index: 'createdAtId'}) | |
| .limit(BATCH_SIZE) | |
| .pluck(...PG_COLS) | |
| .run()) as SuggestedAction[] | |
| const rowsToInsert = rawRowsToInsert.map((row) => ({ | |
| ...row | |
| })) | |
| if (rowsToInsert.length === 0) break | |
| const lastRow = rowsToInsert[rowsToInsert.length - 1] | |
| curcreatedAt = lastRow.createdAt | |
| curId = lastRow.id | |
| try { | |
| await pg | |
| .insertInto('SuggestedAction') | |
| .values(rowsToInsert) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| await Promise.all( | |
| rowsToInsert.map(async (row) => { | |
| try { | |
| await pg | |
| .insertInto('SuggestedAction') | |
| .values(row) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | |
| console.log(`Skipping ${row.id} because it has no user/team`) | |
| return | |
| } | |
| console.log(e, row) | |
| } | |
| }) | |
| ) | |
| } | |
| } |
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Description
This one got ugly because I had to get
SuggestedActionout of the graphql/User object, so I ended up just migrating User to the SDL pattern.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores