chore(rethinkdb): SuggestedAction: Phase 2 #10042
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]>
|
Warning Rate limit exceeded@mattkrick has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 1 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes primarily involve transitioning from RethinkDB to PostgreSQL across various modules of the server application. This migration enhances the database management strategy, allowing for improved query structuring and data handling. Additionally, several new features have been introduced to manage suggested actions effectively, along with updates to the email verification process to include expiration handling. Overall, these modifications improve code maintainability, performance, and functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant Database
User->>Server: Sign up with email
Server->>Database: Insert email verification data
Database-->>Server: Confirmation of insertion
Server->>User: Send verification email
User->>Server: Verify email with token
Server->>Database: Validate token and set expiration
Database-->>Server: Return validation result
Server->>User: Confirmation of email verification
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: 11
Outside diff range, codebase verification and nitpick comments (5)
packages/server/email/createEmailVerficationForExistingUser.ts (1)
37-48: Mismatch in function parametersThe function
createEmailVerficationForExistingUseris called with incorrect parameters inpackages/server/graphql/public/mutations/helpers/getIsUserIdApprovedByOrg.ts. The new implementation expects different parameters.
- File:
packages/server/graphql/public/mutations/helpers/getIsUserIdApprovedByOrg.ts- Code:
const emailError = await createEmailVerficationForExistingUser( userId, invitationToken, dataLoader )The function should be updated to match the new implementation.
Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
createEmailVerficationForExistingUsermatch the new implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `createEmailVerficationForExistingUser` match the new implementation. # Test: Search for the function usage. Expect: Only occurrences of the new implementation. rg --type ts 'createEmailVerficationForExistingUser'Length of output: 615
Script:
#!/bin/bash # Extracting relevant lines around the function call to inspect parameters passed to createEmailVerficationForExistingUser rg --type ts -A 10 'createEmailVerficationForExistingUser' packages/server/graphql/public/mutations/helpers/getIsUserIdApprovedByOrg.tsLength of output: 937
packages/server/postgres/migrations/1722013208553_SuggestAction-phase2.ts (3)
28-28: Remove redundant log message.The log message "Adding index complete" is redundant because the previous log message already indicates the completion of the index creation process.
- console.log('Adding index complete')
36-38: Clarify loop termination condition.The loop termination condition
i < 1e6is arbitrary. Consider clarifying the condition or making it configurable.- for (let i = 0; i < 1e6; i++) { + const MAX_ITERATIONS = 1e6; // or make it configurable + for (let i = 0; i < MAX_ITERATIONS; i++) {
52-54: Remove unnecessary spread operator.The spread operator in
rowsToInsertis unnecessary since no additional properties are being added.- const rowsToInsert = rawRowsToInsert.map((row) => ({ - ...row - })) + const rowsToInsert = rawRowsToInsertpackages/server/__tests__/autoJoin.test.ts (1)
60-62: Update test assertion forauthToken.The test assertion for
authTokenshould check for a non-empty string to ensure the token is valid.- authToken: expect.any(String), + authToken: expect.stringMatching(/^.+$/),
| await getKysely() | ||
| .updateTable('SuggestedAction') | ||
| .set({removedAt: now}) | ||
| .where('id', '=', suggestedActionId) | ||
| .execute() |
There was a problem hiding this comment.
Ensure proper error handling for Kysely operations.
The Kysely operations should have proper error handling to catch and log any potential issues.
+ try {
await getKysely()
.updateTable('SuggestedAction')
.set({removedAt: now})
.where('id', '=', suggestedActionId)
.execute()
+ } catch (error) {
+ console.error('Error updating SuggestedAction with Kysely:', error)
+ }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.
| await getKysely() | |
| .updateTable('SuggestedAction') | |
| .set({removedAt: now}) | |
| .where('id', '=', suggestedActionId) | |
| .execute() | |
| try { | |
| await getKysely() | |
| .updateTable('SuggestedAction') | |
| .set({removedAt: now}) | |
| .where('id', '=', suggestedActionId) | |
| .execute() | |
| } catch (error) { | |
| console.error('Error updating SuggestedAction with Kysely:', error) | |
| } |
| const inviteYourTeam = { | ||
| id: generateUID(), | ||
| userId, | ||
| teamId, | ||
| type: 'inviteYourTeam' as const, | ||
| priority: 2 | ||
| } |
There was a problem hiding this comment.
Avoid duplication of action creation.
The creation of the inviteYourTeam action is duplicated. Consider refactoring to avoid redundancy.
- const inviteYourTeam = {
- id: generateUID(),
- userId,
- teamId,
- type: 'inviteYourTeam' as const,
- priority: 2
- }
+ const inviteYourTeam = createInviteYourTeamAction(userId, teamId)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.
| const inviteYourTeam = { | |
| id: generateUID(), | |
| userId, | |
| teamId, | |
| type: 'inviteYourTeam' as const, | |
| priority: 2 | |
| } | |
| const inviteYourTeam = createInviteYourTeamAction(userId, teamId) |
| const inviteYourTeam = { | ||
| id: generateUID(), | ||
| userId, | ||
| teamId, | ||
| type: 'inviteYourTeam' as const, | ||
| priority: 2 | ||
| } |
There was a problem hiding this comment.
Avoid duplication of action creation.
The creation of the inviteYourTeam action is duplicated. Consider refactoring to avoid redundancy.
- const inviteYourTeam = {
- id: generateUID(),
- userId,
- teamId,
- type: 'inviteYourTeam' as const,
- priority: 2
- }
+ const createInviteYourTeamAction = (userId, teamId) => ({
+ id: generateUID(),
+ userId,
+ teamId,
+ type: 'inviteYourTeam' as const,
+ priority: 2
+ })
+ const inviteYourTeam = createInviteYourTeamAction(userId, teamId)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.
| const inviteYourTeam = { | |
| id: generateUID(), | |
| userId, | |
| teamId, | |
| type: 'inviteYourTeam' as const, | |
| priority: 2 | |
| } | |
| const createInviteYourTeamAction = (userId, teamId) => ({ | |
| id: generateUID(), | |
| userId, | |
| teamId, | |
| type: 'inviteYourTeam' as const, | |
| priority: 2 | |
| }) | |
| const inviteYourTeam = createInviteYourTeamAction(userId, teamId) |
| export async function down() { | ||
| const client = new Client(getPgConfig()) | ||
| await client.connect() | ||
| await client.query(` | ||
| DROP TABLE "SuggestedAction"; | ||
| DROP TYPE "SuggestedActionTypeEnum"; | ||
| ` /* Do undo magic */) | ||
| await client.end() | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling for the down migration.
The down migration should have proper error handling to catch and log any potential issues.
+ try {
await client.query(`
DROP TABLE "SuggestedAction";
DROP TYPE "SuggestedActionTypeEnum";
`)
+ } catch (error) {
+ console.error('Error dropping SuggestedAction table and type:', error)
+ }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.
| export async function down() { | |
| const client = new Client(getPgConfig()) | |
| await client.connect() | |
| await client.query(` | |
| DROP TABLE "SuggestedAction"; | |
| DROP TYPE "SuggestedActionTypeEnum"; | |
| ` /* Do undo magic */) | |
| await client.end() | |
| } | |
| export async function down() { | |
| const client = new Client(getPgConfig()) | |
| await client.connect() | |
| try { | |
| await client.query(` | |
| DROP TABLE "SuggestedAction"; | |
| DROP TYPE "SuggestedActionTypeEnum"; | |
| `) | |
| } catch (error) { | |
| console.error('Error dropping SuggestedAction table and type:', error) | |
| } | |
| await client.end() | |
| } |
| const toCreditCard = (creditCard: any) => { | ||
| if (!creditCard) return null | ||
| return sql<string>`(select json_populate_record(null::"CreditCard", ${JSON.stringify(creditCard)}))` | ||
| } |
There was a problem hiding this comment.
Improve type safety for toCreditCard function.
Avoid using any type for creditCard parameter. Define a proper type for creditCard to ensure type safety.
type CreditCard = {
// Define the properties of CreditCard here
// Example:
cardNumber: string;
expiryDate: string;
// Add other properties as needed
}
const toCreditCard = (creditCard: CreditCard | null) => {
if (!creditCard) return null
return sql<string>`(select json_populate_record(null::"CreditCard", ${JSON.stringify(creditCard)}))`
}| export async function down() { | ||
| await connectRethinkDB() | ||
| try { | ||
| await r.table('SuggestedAction').indexDrop('createdAtId').run() | ||
| } catch { | ||
| // index already dropped | ||
| } |
There was a problem hiding this comment.
Enhance error handling for index drop.
The catch block for index drop should log the error to provide more context.
- } catch {
+ } catch (error) {
+ console.error('Error dropping index:', error)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.
| export async function down() { | |
| await connectRethinkDB() | |
| try { | |
| await r.table('SuggestedAction').indexDrop('createdAtId').run() | |
| } catch { | |
| // index already dropped | |
| } | |
| export async function down() { | |
| await connectRethinkDB() | |
| try { | |
| await r.table('SuggestedAction').indexDrop('createdAtId').run() | |
| } catch (error) { | |
| console.error('Error dropping index:', error) | |
| } |
| 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.
Optimize error handling for individual row inserts.
The error handling for individual row inserts can be optimized by logging errors once and skipping rows with missing user/team in a single catch block.
- 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)
- }
- })
- )
+ for (const row of rowsToInsert) {
+ 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`)
+ } else {
+ console.log(e, row)
+ }
+ }
+ }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.
| 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) | |
| } | |
| for (const row of rowsToInsert) { | |
| 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`) | |
| } else { | |
| console.log(e, row) | |
| } | |
| } | |
| } |
| export async function down() { | ||
| const client = new Client(getPgConfig()) | ||
| await client.connect() | ||
| await client.query(` | ||
| DROP TABLE IF EXISTS "EmailVerification"; | ||
| `) | ||
| await client.end() |
There was a problem hiding this comment.
Ensure proper error handling for the down migration.
The down migration should have proper error handling to catch and log any potential issues.
+ try {
await client.query(`
DROP TABLE IF EXISTS "EmailVerification";
`)
+ } catch (error) {
+ console.error('Error dropping EmailVerification table:', error)
+ }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.
| export async function down() { | |
| const client = new Client(getPgConfig()) | |
| await client.connect() | |
| await client.query(` | |
| DROP TABLE IF EXISTS "EmailVerification"; | |
| `) | |
| await client.end() | |
| export async function down() { | |
| const client = new Client(getPgConfig()) | |
| await client.connect() | |
| try { | |
| await client.query(` | |
| DROP TABLE IF EXISTS "EmailVerification"; | |
| `) | |
| } catch (error) { | |
| console.error('Error dropping EmailVerification table:', error) | |
| } | |
| await client.end() | |
| } |
| const pg = getKysely() | ||
| const verificationToken = ( | ||
| await pg | ||
| .selectFrom('EmailVerification') | ||
| .select('token') | ||
| .where('email', '=', email) | ||
| .executeTakeFirstOrThrow(() => new Error(`No verification token found for ${email}`)) | ||
| ).token |
There was a problem hiding this comment.
Add error handling for database operations.
The function should handle potential errors when retrieving the verification token from PostgreSQL.
- const verificationToken = (
- await pg
- .selectFrom('EmailVerification')
- .select('token')
- .where('email', '=', email)
- .executeTakeFirstOrThrow(() => new Error(`No verification token found for ${email}`))
- ).token
+ let verificationToken;
+ try {
+ verificationToken = (
+ await pg
+ .selectFrom('EmailVerification')
+ .select('token')
+ .where('email', '=', email)
+ .executeTakeFirstOrThrow(() => new Error(`No verification token found for ${email}`))
+ ).token;
+ } catch (error) {
+ console.error('Error retrieving verification token:', error);
+ throw error;
+ }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.
| const pg = getKysely() | |
| const verificationToken = ( | |
| await pg | |
| .selectFrom('EmailVerification') | |
| .select('token') | |
| .where('email', '=', email) | |
| .executeTakeFirstOrThrow(() => new Error(`No verification token found for ${email}`)) | |
| ).token | |
| let verificationToken; | |
| try { | |
| verificationToken = ( | |
| await pg | |
| .selectFrom('EmailVerification') | |
| .select('token') | |
| .where('email', '=', email) | |
| .executeTakeFirstOrThrow(() => new Error(`No verification token found for ${email}`)) | |
| ).token; | |
| } catch (error) { | |
| console.error('Error retrieving verification token:', error); | |
| throw error; | |
| } |
| export const _suggestedActionsByUserId = foreignKeyLoaderMaker( | ||
| '_suggestedActions', | ||
| 'userId', | ||
| async (userIds) => { | ||
| return selectSuggestedAction().where('userId', 'in', userIds).execute() | ||
| } | ||
| ) |
There was a problem hiding this comment.
Add error handling for database operations.
The function should handle potential errors when querying the database for suggested actions.
- async (userIds) => {
- return selectSuggestedAction().where('userId', 'in', userIds).execute()
- }
+ async (userIds) => {
+ try {
+ return await selectSuggestedAction().where('userId', 'in', userIds).execute();
+ } catch (error) {
+ console.error('Error retrieving suggested actions:', error);
+ throw error;
+ }
+ }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.
| export const _suggestedActionsByUserId = foreignKeyLoaderMaker( | |
| '_suggestedActions', | |
| 'userId', | |
| async (userIds) => { | |
| return selectSuggestedAction().where('userId', 'in', userIds).execute() | |
| } | |
| ) | |
| export const _suggestedActionsByUserId = foreignKeyLoaderMaker( | |
| '_suggestedActions', | |
| 'userId', | |
| async (userIds) => { | |
| try { | |
| return await selectSuggestedAction().where('userId', 'in', userIds).execute(); | |
| } catch (error) { | |
| console.error('Error retrieving suggested actions:', error); | |
| throw error; | |
| } | |
| } | |
| ) |
Signed-off-by: Matt Krick <[email protected]>
migrate existing rows
Summary by CodeRabbit
New Features
Bug Fixes
Chores