Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(rethinkdb): TeamMember: Phase 1 #9979

Merged
merged 10 commits into from
Jul 19, 2024
Merged

Conversation

mattkrick
Copy link
Member

@mattkrick mattkrick commented Jul 15, 2024

Description

Writes to PG
Refactors reads to use a dataloader where possible

Summary by CodeRabbit

  • New Features

    • Added migration logic for creating and dropping tables related to team members in the database.
  • Bug Fixes

    • Updated meeting creation to handle cases where the creator is undefined.
  • Refactor

    • Refactored various functions to use data loader for efficient data fetching and processing.
    • Replaced direct database queries with PostgreSQL operations using a new library for improved performance.
  • Chores

    • Updated type definitions and altered various method signatures for improved type safety and code clarity.

Signed-off-by: Matt Krick <[email protected]>
Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Copy link
Contributor

coderabbitai bot commented Jul 15, 2024

Walkthrough

The changes involve a series of updates and refactors across the server and client codebases, focusing on data loading, database interactions, and mutator functions. Key changes include modifying data loader interfaces to handle arrays, updating database queries to be more efficient, introducing PostgreSQL functions, and enhancing type handling. The goal is to improve performance and maintainability of data interactions and related functionality.

Changes

File(s) Change Summary
Meeting.ts Changed createdBy property's type in Meeting class from string to string | null.
RootDataLoader.ts Updated clearAll method in GenericDataLoader to accept an array, refactored clearAll in RootDataLoader to handle arrays, and optimized rethinkForeignKeyLoader call.
rethinkForeignKeyLoader.ts Added parameters parent, dependsOn, and primaryKeyLoaderName to rethinkForeignKeyLoader function.
createTask.ts, deleteTask.ts, endCheckIn.ts, updateTask.ts, requestToJoinDomain.ts Removed direct TeamMember queries, replaced with data loader calls for fetching team members and related data.
createTeamAndLeader.ts, removeFromOrg.ts, safeEndRetrospective.ts, softDeleteUser.ts, promoteToTeamLead.ts Refactored to use data loaders and PostgreSQL for operations, removed RethinkDB dependencies.
setPokerSpectate.ts, updateEmail.ts, updateUserProfile.ts, backupOrganization.ts Introduced getKysely for PostgreSQL interactions, refactored related operations.
changeEmailDomain.ts, hardDeleteUser.ts Updated mutation resolvers to handle email domain changes and user deletions, including new logic for nullifying createdBy in NewMeeting records.
1720730818119_TeamMember-phase1.ts Added a new database migration script for creating and managing team member-related tables and triggers.
insertNewTeamMember.ts, acceptTeamInvitation.ts Updated to accept dataLoader parameter, refactored to use PostgreSQL for database updates, and manage team members more efficiently.
useSnacksForNewMeetings.ts Modified useSnacksForNewMeetings function to handle potential undefined preferredName.

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between eb30b1b and 5fa485f.

Files selected for processing (1)
  • packages/server/postgres/migrations/1720730818119_TeamMember-phase1.ts (1 hunks)
Additional comments not posted (3)
packages/server/postgres/migrations/1720730818119_TeamMember-phase1.ts (3)

16-37: Ensure column constraints and types are appropriate.

Review the constraints and types for each column to ensure they align with the application's requirements.

  • Verify if VARCHAR(100) is sufficient for id, preferredName, teamId, and userId.
  • Verify if VARCHAR(2056) is appropriate for picture.
  • Ensure citext extension is enabled for the email column.

38-39: Optimise index creation for performance.

The conditional index creation ensures that only active team members are indexed, which can improve query performance.


40-41: Ensure the trigger function set_updatedAt exists.

The trigger function set_updatedAt should be defined elsewhere in the codebase.

  • Verify the existence and correctness of the set_updatedAt function.

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 21

Outside diff range, codebase verification and nitpick comments (7)
packages/server/dataloader/rethinkForeignKeyLoader.ts (1)

7-22: Approved changes with a suggestion for improvement.

The refactoring of rethinkForeignKeyLoader to include additional parameters (parent, dependsOn, primaryKeyLoaderName) enhances modularity and reusability. The implementation correctly integrates these parameters into the loader's functionality.

However, adding comments explaining the purpose and expected types of the new parameters would improve code readability and maintainability.

7a8,10
+  // parent: RootDataLoader instance to access other loaders
+  // dependsOn: Function to register dependency on other loaders
+  // primaryKeyLoaderName: Key of the primary key loader to be used
packages/server/graphql/private/mutations/updateEmail.ts (1)

Line range hint 2-37: Approved changes with a suggestion for monitoring performance.

The addition of PostgreSQL queries alongside existing RethinkDB operations in the updateEmail mutation is a necessary step in the database migration process. The code correctly handles updates across both databases, ensuring data consistency during the transition.

However, maintaining dual-database operations can lead to increased complexity and potential performance bottlenecks. It would be advisable to monitor the performance of these operations and plan to phase out RethinkDB as soon as feasible.

packages/server/safeMutations/safeArchiveTeam.ts (1)

44-45: Optimize data loader cache clearing.

The method clearAll is called multiple times in a row, which could be consolidated into a single call for better performance and readability.

- dataLoader.clearAll('teamMembers').clearAll('users').clearAll('teams')
+ dataLoader.clearAll('teamMembers', 'users', 'teams')
packages/server/database/types/Meeting.ts (1)

30-30: Ensure consistent handling of nullable createdBy property.

The search results indicate that the createdBy property is not consistently handled for null values across the codebase. To prevent potential runtime errors or unexpected behaviour, please review and update the following locations to ensure proper null handling:

  • packages/server/utils/analytics/analytics.ts
  • packages/server/utils/AzureDevOpsServerManager.ts
  • packages/server/postgres/generatedMigrationHelpers.ts
  • packages/server/graphql/types/Poll.ts
  • packages/server/graphql/mutations/createTask.ts
  • packages/server/graphql/mutations/updateCommentContent.ts
  • packages/server/graphql/private/mutations/hardDeleteUser.ts
  • packages/server/graphql/types/Task.ts
  • packages/server/graphql/types/Threadable.ts
  • packages/server/graphql/types/Team.ts
  • packages/server/graphql/mutations/resetRetroMeetingToGroupStage.ts
  • packages/server/postgres/migrations/1616623815919_addTeamTable.ts
  • packages/server/postgres/migrations/1627394777775_addPollsTable.ts
  • packages/server/postgres/migrations/1680865284180_addDomainJoinRequestTable.ts
  • packages/server/postgres/migrations/1683902222006_domainJoinRequestAutoIncrementId.ts
  • packages/server/postgres/migrations/1631553388387_add-template-teams-users.ts
  • packages/server/graphql/mutations/pokerResetDimension.ts
  • packages/server/graphql/mutations/pokerRevealVotes.ts
  • packages/server/graphql/mutations/navigateMeeting.ts
  • packages/server/graphql/types/Comment.ts
  • packages/server/graphql/mutations/deleteComment.ts
  • packages/server/graphql/mutations/helpers/addSeedTasks.ts
  • packages/server/graphql/mutations/helpers/calculateEngagement.ts
  • packages/server/graphql/mutations/helpers/safeEndRetrospective.ts
  • packages/server/graphql/mutations/helpers/generateDiscussionSummary.ts
  • packages/server/graphql/mutations/helpers/generateWholeMeetingSummary.ts
  • packages/server/graphql/mutations/helpers/createTeamAndLeader.ts
  • packages/server/graphql/mutations/helpers/addAIGeneratedContentToThreads.ts
  • packages/server/graphql/mutations/createPoll.ts
  • packages/server/graphql/mutations/addComment.ts
  • packages/server/graphql/public/mutations/acceptRequestToJoinDomain.ts
  • packages/server/graphql/public/mutations/revealTeamHealthVotes.ts
  • packages/server/graphql/public/mutations/batchArchiveTasks.ts
  • packages/server/graphql/public/mutations/requestToJoinDomain.ts
  • packages/server/graphql/public/types/NewMeeting.ts
  • packages/server/graphql/public/types/DomainJoinRequest.ts
  • packages/server/dataloader/primaryKeyLoaderMakers.ts
  • packages/server/dataloader/customLoaderMakers.ts
  • packages/server/database/migrations/20200717141122-masterTemplates.ts
  • packages/server/database/migrations/20200207101941-createdByTask.ts
  • packages/server/database/migrations/20210324160118-swapTatorIdWithCreatedBy.ts
  • packages/server/database/migrations/20210629164700-task-hash.ts
  • packages/server/database/migrations/20210726140543-estimate-stage-tasks.ts
  • packages/server/database/migrations/20210721151034-integration-hash.ts
  • packages/server/database/migrations/20211025134235-estimateStageTaskIdBackfill.ts
  • packages/server/database/types/Comment.ts
  • packages/server/database/types/Task.ts
  • packages/server/database/types/Meeting.ts
  • packages/server/database/types/Team.ts
  • packages/server/database/types/ImportedTask.ts
  • packages/client/hooks/useSnacksForNewMeetings.ts
  • packages/embedder/indexing/retrospectiveDiscussionTopic.ts
  • packages/client/components/Poll/Poll.tsx
  • packages/client/components/NullableTask/NullableTask.tsx
  • packages/client/components/ReviewRequestToJoinOrgModal.tsx
  • packages/client/components/ThreadedTaskBase.tsx
  • packages/client/components/ThreadedCommentHeader.tsx
  • packages/client/components/ThreadedCommentBase.tsx
  • packages/client/components/TeamPrompt/TeamPromptRepliesAvatarList.tsx
  • packages/client/mutations/fragments/CompleteTaskFrag.ts
  • packages/client/mutations/UpdatePokerScopeMutation.ts
  • packages/client/mutations/CreateTaskMutation.ts
  • packages/client/mutations/AddCommentMutation.ts
  • packages/client/modules/email/components/SummaryEmail/ExportToCSV.tsx
  • packages/client/modules/demo/initDB.ts
  • packages/client/modules/demo/ClientGraphQLServer.ts
Analysis chain

Updated createdBy property to allow null values.

This change enhances type safety and flexibility in handling meetings without a creator. Ensure that all related functions and database queries correctly handle this nullable type.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify handling of the nullable `createdBy` property in all related functions and queries.

# Test: Search for usage of `createdBy`. Expect: Proper handling of null values.
rg --type python -A 5 $'createdBy'

Length of output: 64


Script:

#!/bin/bash
# Description: Verify handling of the nullable `createdBy` property in all related functions and queries.

# Test: Search for usage of `createdBy` in TypeScript files. Expect: Proper handling of null values.
rg --type ts -A 5 $'createdBy'

Length of output: 76831

packages/server/graphql/mutations/promoteToTeamLead.ts (1)

Line range hint 5-63: Ensure proper error handling and optimise database queries.

The introduction of PostgreSQL and the complex logic for promoting a team lead require careful attention to error handling and query optimisation.

-    await pg
-      .updateTable('TeamMember')
-      .set(({not}) => ({isLead: not('isLead')}))
-      .where('id', 'in', [oldLeadTeamMemberId, promoteeOnTeam.id])
-      .execute()
+    try {
+      await pg.transaction(async trx => {
+        await trx('TeamMember')
+          .update({ isLead: false })
+          .where('id', '=', oldLeadTeamMemberId)
+
+        await trx('TeamMember')
+          .update({ isLead: true })
+          .where('id', '=', promoteeOnTeam.id)
+      })
+    } catch (error) {
+      return standardError(error, {userId: viewerId})
+    }

This refactor ensures that the promotion of team leads is handled in a single transaction, improving data consistency and handling potential errors more gracefully.

packages/server/dataloader/RootDataLoader.ts (1)

Line range hint 51-93: Ensure Method Consistency and Improve Type Safety

The method clearAll in the RootDataLoader class can be improved for better type safety and consistency.

- clearAll(pkLoaderName: TPrimaryLoaderNames): this
+ clearAll(pkLoaderName: TPrimaryLoaderNames): RootDataLoader

Adjust the return type to be more explicit, enhancing type safety and consistency across the codebase.

packages/server/graphql/private/typeDefs/_legacy.graphql (1)

Line range hint 1-1000: Consider strategies for managing legacy code.

This file is marked as legacy and contains a complex structure of GraphQL type definitions. Consider implementing strategies such as documentation, gradual refactoring, or even splitting the file to manage complexity and ensure maintainability.

Comment on lines 22 to 23
const teamMembers = await dataLoader.get('teamMembersByUserId').load(userIdToDelete)
const teamIds = teamMembers.map(({id}) => TeamMemberId.split(id).teamId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved changes with a suggestion for error handling.

The updates to the softDeleteUser function to use the teamMembersByUserId loader for fetching team member data before performing cleanup operations are well-implemented. This change should improve performance by reducing the number of database hits.

Consider adding error handling around the data loader calls to manage potential failures gracefully, ensuring that the function can recover or fail cleanly if data loading issues occur.

22a23,25
+  try {
+    const teamMembers = await dataLoader.get('teamMembersByUserId').load(userIdToDelete)
+  } catch (error) {
+    console.error('Failed to load team members:', error)
+    throw new Error('Error loading team members')
+  }
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.

Suggested change
const teamMembers = await dataLoader.get('teamMembersByUserId').load(userIdToDelete)
const teamIds = teamMembers.map(({id}) => TeamMemberId.split(id).teamId)
try {
const teamMembers = await dataLoader.get('teamMembersByUserId').load(userIdToDelete)
} catch (error) {
console.error('Failed to load team members:', error)
throw new Error('Error loading team members')
}
const teamIds = teamMembers.map(({id}) => TeamMemberId.split(id).teamId)

Comment on lines +51 to +57
const orgTeams = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds)).filter(isValid).flat()
const teamIds = orgTeams.map(({id}) => id)
const teamMembers = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds))
.filter(isValid)
.flat()
const leadTeamMembers = teamMembers.filter(({isLead}) => isLead)
const leadUserIds = [...new Set(leadTeamMembers.map(({userId}) => userId))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimise data loading and filtering.

The current implementation loads and filters team data in separate steps, which can be optimised by combining these operations to reduce the number of iterations over the data.

-  const orgTeams = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds)).filter(isValid).flat()
-  const teamIds = orgTeams.map(({id}) => id)
-  const teamMembers = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds))
-    .filter(isValid)
-    .flat()
-  const leadTeamMembers = teamMembers.filter(({isLead}) => isLead)
-  const leadUserIds = [...new Set(leadTeamMembers.map(({userId}) => userId))]
+  const teamIds = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds))
+    .filter(isValid)
+    .flatMap(orgTeam => orgTeam.map(({id}) => id))
+  const leadUserIds = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds))
+    .filter(isValid)
+    .flatMap(teamMember => teamMember.filter(({isLead}) => isLead))
+    .map(({userId}) => userId)
+    .filter((v, i, a) => a.indexOf(v) === i) // Ensure uniqueness
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.

Suggested change
const orgTeams = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds)).filter(isValid).flat()
const teamIds = orgTeams.map(({id}) => id)
const teamMembers = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds))
.filter(isValid)
.flat()
const leadTeamMembers = teamMembers.filter(({isLead}) => isLead)
const leadUserIds = [...new Set(leadTeamMembers.map(({userId}) => userId))]
const teamIds = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds))
.filter(isValid)
.flatMap(orgTeam => orgTeam.map(({id}) => id))
const leadUserIds = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds))
.filter(isValid)
.flatMap(teamMember => teamMember.filter(({isLead}) => isLead))
.map(({userId}) => userId)
.filter((v, i, a) => a.indexOf(v) === i) // Ensure uniqueness

Comment on lines 50 to 65
pg
.with('TeamMembers', (qc) =>
qc
.updateTable('TeamMember')
.set({
email: sql`CONCAT(LEFT(email, POSITION('@' in email)), ${normalizedNewDomain}::VARCHAR)`
})
.where('userId', 'in', userIdsToUpdate)
)
.updateTable('User')
.set({
email: sql`CONCAT(LEFT(email, POSITION('@' in email)), ${normalizedNewDomain}::VARCHAR)`
})
.where('id', 'in', userIdsToUpdate)
.returning('id')
.execute(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential SQL Injection Vulnerability and Inefficient Query Handling

The SQL query construction using concatenation might expose the system to SQL injection attacks. Additionally, the query updates multiple tables in separate statements within a single transaction, which can be optimised.

- email: sql`CONCAT(LEFT(email, POSITION('@' in email)), ${normalizedNewDomain}::VARCHAR)`
+ email: sql`CONCAT(LEFT(email, POSITION('@' in email)), ?::VARCHAR)`, [normalizedNewDomain]

Consider using parameterised queries to mitigate SQL injection risks. Also, look into combining updates to reduce transaction overhead.

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.

Suggested change
pg
.with('TeamMembers', (qc) =>
qc
.updateTable('TeamMember')
.set({
email: sql`CONCAT(LEFT(email, POSITION('@' in email)), ${normalizedNewDomain}::VARCHAR)`
})
.where('userId', 'in', userIdsToUpdate)
)
.updateTable('User')
.set({
email: sql`CONCAT(LEFT(email, POSITION('@' in email)), ${normalizedNewDomain}::VARCHAR)`
})
.where('id', 'in', userIdsToUpdate)
.returning('id')
.execute(),
pg
.with('TeamMembers', (qc) =>
qc
.updateTable('TeamMember')
.set({
email: sql`CONCAT(LEFT(email, POSITION('@' in email)), ?::VARCHAR)`, [normalizedNewDomain]
})
.where('userId', 'in', userIdsToUpdate)
)
.updateTable('User')
.set({
email: sql`CONCAT(LEFT(email, POSITION('@' in email)), ?::VARCHAR)`, [normalizedNewDomain]
})
.where('id', 'in', userIdsToUpdate)
.returning('id')
.execute(),

Comment on lines +25 to +28
const teamMemberIds = teamIds.map((teamId) => TeamMemberId.join(teamId, viewerId))
const leadTeamMembers = (await dataLoader.get('teamMembers').loadMany(teamMemberIds))
.filter(isValid)
.filter(({isLead}) => isLead)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved changes with a suggestion for improving clarity.

The updates to the DomainJoinRequest type to use the TeamMemberId utility and filter for team leads are well-implemented, enhancing the functionality and efficiency of the system.

Consider refactoring the filtering logic to make it more readable and maintainable, perhaps by breaking it into smaller, more descriptive functions.

26a27,29
+  // Filter valid team members who are leads
+  const leadTeamMembers = teamMembers.filter(isValid).filter(({isLead}) => isLead)

Committable suggestion was skipped due to low confidence.

.run()
await removeUserTms(teamId, userIds)
const users = await Promise.all(userIds.map((userId) => dataLoader.get('users').load(userId)))
const teamMembers = await dataLoader.get('teamMembersByTeamId').load(teamId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for data loader operations.

The data loader is used to fetch team members, but there's no error handling if the data loader fails or returns an unexpected result.

Comment on lines +231 to +234
const teamMembers = await dataLoader.get('teamMembersByTeamId').load(teamId)
await r({
task: r.table('Task').insert(task),
history: r.table('TaskHistory').insert(history),
teamMembers: r
.table('TeamMember')
.getAll(teamId, {index: 'teamId'})
.filter({
isNotRemoved: true
})
.coerceTo('array') as unknown as TeamMember[]
history: r.table('TaskHistory').insert(history)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for database operations.

The database operations are performed without proper error handling. Consider adding try-catch blocks around these operations to handle potential errors gracefully.


Refactor to improve code modularity.

The database interaction code could be refactored into a separate function to improve modularity and maintainability.

Comment on lines +251 to +253
const teamMembers = await dataLoader.get('teamMembersByTeamId').load(teamId)
const teamLeader = teamMembers.find(({isLead}) => isLead)!
const {userId: teamLeadUserId} = teamLeader
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refining the handling of potential null values.

The code assumes that teamLeader will always be found, which may not be safe. Consider adding error handling for cases where teamLeader is undefined.


Refactor to enhance readability and maintainability.

The nested operations within the conditional block could be refactored into separate functions to improve readability and maintain the single responsibility principle.

Comment on lines +99 to +110
const dependsOn: RegisterDependsOn = (inPrimaryLoaders) => {
const primaryLoaders = Array.isArray(inPrimaryLoaders) ? inPrimaryLoaders : [inPrimaryLoaders]
primaryLoaders.forEach((primaryLoader) => {
;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName)
})
}
if (loaderMaker instanceof RethinkPrimaryKeyLoaderMaker) {
const {table} = loaderMaker
loader = rethinkPrimaryKeyLoader(this.dataLoaderOptions, table)
} else if (loaderMaker instanceof RethinkForeignKeyLoaderMaker) {
const {fetch, field, pk} = loaderMaker
const basePkLoader = this.get(pk) as any
loader = rethinkForeignKeyLoader(basePkLoader, this.dataLoaderOptions, field, fetch)
loader = rethinkForeignKeyLoader(this, dependsOn, pk, field, fetch)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address Static Analysis Concern

The static analysis tool flagged a potential issue with the assignment within an expression, which can lead to confusion and bugs.

- ;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName)
+ this.dependentLoaders[primaryLoader] = this.dependentLoaders[primaryLoader] ?? []
+ this.dependentLoaders[primaryLoader].push(loaderName)

Refactor the code to separate the assignment from the expression to improve clarity and reduce the risk of unintended side effects.

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.

Suggested change
const dependsOn: RegisterDependsOn = (inPrimaryLoaders) => {
const primaryLoaders = Array.isArray(inPrimaryLoaders) ? inPrimaryLoaders : [inPrimaryLoaders]
primaryLoaders.forEach((primaryLoader) => {
;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName)
})
}
if (loaderMaker instanceof RethinkPrimaryKeyLoaderMaker) {
const {table} = loaderMaker
loader = rethinkPrimaryKeyLoader(this.dataLoaderOptions, table)
} else if (loaderMaker instanceof RethinkForeignKeyLoaderMaker) {
const {fetch, field, pk} = loaderMaker
const basePkLoader = this.get(pk) as any
loader = rethinkForeignKeyLoader(basePkLoader, this.dataLoaderOptions, field, fetch)
loader = rethinkForeignKeyLoader(this, dependsOn, pk, field, fetch)
const dependsOn: RegisterDependsOn = (inPrimaryLoaders) => {
const primaryLoaders = Array.isArray(inPrimaryLoaders) ? inPrimaryLoaders : [inPrimaryLoaders]
primaryLoaders.forEach((primaryLoader) => {
this.dependentLoaders[primaryLoader] = this.dependentLoaders[primaryLoader] ?? []
this.dependentLoaders[primaryLoader].push(loaderName)
})
}
if (loaderMaker instanceof RethinkPrimaryKeyLoaderMaker) {
const {table} = loaderMaker
loader = rethinkPrimaryKeyLoader(this.dataLoaderOptions, table)
} else if (loaderMaker instanceof RethinkForeignKeyLoaderMaker) {
const {fetch, field, pk} = loaderMaker
loader = rethinkForeignKeyLoader(this, dependsOn, pk, field, fetch)
Tools
Biome

[error] 102-102: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

@@ -51,7 +51,7 @@ export default async function createTeamAndLeader(
// add meeting settings
r.table('MeetingSettings').insert(meetingSettings).run(),
// denormalize common fields to team member
insertNewTeamMember(user, teamId),
insertNewTeamMember(user, teamId, dataLoader),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure data consistency during team member insertion.

The function insertNewTeamMember is called without handling potential errors. Consider wrapping this and related operations in a transaction to ensure data consistency.

Comment on lines 25 to 31
await pg
.updateTable('TeamMember')
.set({isNotRemoved: true})
.where('id', '=', teamMemberId)
.execute()
await r.table('TeamMember').get(teamMemberId).replace(existingTeamMember).run()
dataLoader.clearAll('teamMembers')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize database operations and error handling.

The update operations for an existing team member are split between PostgreSQL and RethinkDB. Consider consolidating these into a single transaction and ensure proper error handling.

- await pg
-   .updateTable('TeamMember')
-   .set({isNotRemoved: true})
-   .where('id', '=', teamMemberId)
-   .execute()
- await r.table('TeamMember').get(teamMemberId).replace(existingTeamTeamMember).run()
+ pg.transaction(async trx => {
+   await trx
+     .updateTable('TeamMember')
+     .set({isNotRemoved: true})
+     .where('id', '=', teamMemberId)
+     .execute()
+   // Consider removing or refactoring the RethinkDB operation if possible
+ })

Committable suggestion was skipped due to low confidence.

Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Comment on lines 47 to 65
.with('TeamInsert', (qc) => qc.insertInto('Team').values(verifiedTeam))
.with('UserUpdate', (qc) =>
qc
.updateTable('User')
.set({tms: sql`arr_append_uniq("tms", ${teamId})`})
.where('id', '=', userId)
)
.with('TeamMemberUpsert', (qc) =>
qc.insertInto('TeamMember').values({
id: TeamMemberId.join(teamId, userId),
teamId,
userId,
picture,
preferredName,
email,
isLead: true,
openDrawer: 'manageTeam'
})
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Complex PostgreSQL transaction

The use of PostgreSQL transactions to handle multiple related database operations is a good practice. However, ensure that error handling is robust to manage potential failures in any part of the transaction.

Consider adding try-catch blocks around the transaction to handle and log potential errors effectively.

const dependsOn: RegisterDependsOn = (inPrimaryLoaders) => {
const primaryLoaders = Array.isArray(inPrimaryLoaders) ? inPrimaryLoaders : [inPrimaryLoaders]
primaryLoaders.forEach((primaryLoader) => {
;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static analysis concern: Assignment within an expression

The assignment within the expression can lead to confusion and potential bugs. Refactoring this to separate the assignment and the expression would improve clarity.

- ;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName)
+ this.dependentLoaders[primaryLoader] = this.dependentLoaders[primaryLoader] ?? []
+ this.dependentLoaders[primaryLoader].push(loaderName)
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.

Suggested change
;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName)
this.dependentLoaders[primaryLoader] = this.dependentLoaders[primaryLoader] ?? []
this.dependentLoaders[primaryLoader].push(loaderName)
Tools
Biome

[error] 102-102: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

@@ -27,10 +27,11 @@ const removeTeamMember = async (
) => {
const {evictorUserId} = options
const r = await getRethink()
const pg = getKysely()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialization of PostgreSQL instance

Initializing a PostgreSQL instance within the function is necessary for the subsequent database operations. Ensure that this instance is properly managed and closed if needed to avoid resource leaks.

Consider managing the PostgreSQL connection lifecycle to prevent potential resource leaks.

Signed-off-by: Matt Krick <[email protected]>
Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Signed-off-by: Matt Krick <[email protected]>
Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

1 similar comment
Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
packages/client/hooks/useSnacksForNewMeetings.ts (1)

Line range hint 1-52: General Review: Optimise the use of hooks and improve code readability.

The hook useSnacksForNewMeetings is well-structured, but there are a few areas that could be optimised:

  • The useState hook is used to initialise dismissedMeetingIds but it's not updated within the useEffect. Consider using a useReducer or another state management strategy if state updates become more complex in the future.
  • The sorting and filtering logic within useEffect could be abstracted into a separate function to improve readability and reusability.
  • Dependency array of useEffect only includes meetings.length, which might lead to missed updates if other dependencies change. Review and ensure all necessary dependencies are included.

Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Signed-off-by: Matt Krick <[email protected]>
Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

DROP TABLE "TeamMember";
DROP TYPE "TeamDrawerEnum";
` /* Do undo magic */)
await client.end()
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for database disconnection.

It's important to handle errors that might occur during the disconnection from the database.

  try {
    await client.end()
  } catch (err) {
    console.error('Failed to disconnect from the database:', err)
    throw err
  }
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.

Suggested change
await client.end()
try {
await client.end()
} catch (err) {
console.error('Failed to disconnect from the database:', err)
throw err
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never want to catch an error just to log and rethrow that error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dschoordsch, understood. Instead of catching the error just to log and rethrow it, we can let the error propagate naturally. This way, it will be handled by the calling function or global error handler, ensuring that the error is not silently ignored.

Here's the revised code without the catch block:

await client.end()

This approach ensures that any disconnection errors are not silently ignored and are handled appropriately by the calling context.


Learnings added
Learnt from: Dschoordsch
PR: ParabolInc/parabol#9979
File: packages/server/postgres/migrations/1720730818119_TeamMember-phase1.ts:54-54
Timestamp: 2024-07-18T08:33:50.991Z
Learning: When handling database disconnections, avoid catching errors just to log and rethrow them. Let the errors propagate naturally to be handled by the calling function or global error handler.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment on lines +47 to +49
export async function down() {
const client = new Client(getPgConfig())
await client.connect()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for database connection.

It's important to handle errors that might occur during the connection to the database to avoid unhandled promise rejections.

  const client = new Client(getPgConfig())
  try {
    await client.connect()
  } catch (err) {
    console.error('Failed to connect to the database:', err)
    throw err
  }
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.

Suggested change
export async function down() {
const client = new Client(getPgConfig())
await client.connect()
const client = new Client(getPgConfig())
try {
await client.connect()
} catch (err) {
console.error('Failed to connect to the database:', err)
throw err
}

Comment on lines +50 to +52
await client.query(`
DROP TABLE "TeamMember";
DROP TYPE "TeamDrawerEnum";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider wrapping the query execution in a transaction.

Wrapping the query execution in a transaction ensures atomicity, meaning that either all changes are applied, or none are.

  await client.query('BEGIN')
  try {
    await client.query(`
      DROP TABLE "TeamMember";
      DROP TYPE "TeamDrawerEnum";
    ` /* Do undo magic */)
    await client.query('COMMIT')
  } catch (err) {
    await client.query('ROLLBACK')
    throw err
  }

Committable suggestion was skipped due to low confidence.

Comment on lines +4 to +6
export async function up() {
const client = new Client(getPgConfig())
await client.connect()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for database connection.

It's important to handle errors that might occur during the connection to the database to avoid unhandled promise rejections.

  const client = new Client(getPgConfig())
  try {
    await client.connect()
  } catch (err) {
    console.error('Failed to connect to the database:', err)
    throw err
  }
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.

Suggested change
export async function up() {
const client = new Client(getPgConfig())
await client.connect()
export async function up() {
const client = new Client(getPgConfig())
try {
await client.connect()
} catch (err) {
console.error('Failed to connect to the database:', err)
throw err
}

CREATE TRIGGER "update_TeamMember_updatedAt" BEFORE UPDATE ON "TeamMember" FOR EACH ROW EXECUTE PROCEDURE "set_updatedAt"();
END $$;
`)
await client.end()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for database disconnection.

It's important to handle errors that might occur during the disconnection from the database.

  try {
    await client.end()
  } catch (err) {
    console.error('Failed to disconnect from the database:', err)
    throw err
  }
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.

Suggested change
await client.end()
try {
await client.end()
} catch (err) {
console.error('Failed to disconnect from the database:', err)
throw err
}

Comment on lines +7 to +15
await client.query(`
DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'TeamDrawerEnum') THEN
CREATE TYPE "TeamDrawerEnum" AS ENUM (
'agenda',
'manageTeam'
);
END IF;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider wrapping the query execution in a transaction.

Wrapping the query execution in a transaction ensures atomicity, meaning that either all changes are applied, or none are.

  await client.query('BEGIN')
  try {
    await client.query(`
    DO $$
    BEGIN
      IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'TeamDrawerEnum') THEN
        CREATE TYPE "TeamDrawerEnum" AS ENUM (
          'agenda',
          'manageTeam'
        );
      END IF;
      ...
    END $$;
  `)
    await client.query('COMMIT')
  } catch (err) {
    await client.query('ROLLBACK')
    throw err
  }
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.

Suggested change
await client.query(`
DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'TeamDrawerEnum') THEN
CREATE TYPE "TeamDrawerEnum" AS ENUM (
'agenda',
'manageTeam'
);
END IF;
await client.query('BEGIN')
try {
await client.query(`
DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'TeamDrawerEnum') THEN
CREATE TYPE "TeamDrawerEnum" AS ENUM (
'agenda',
'manageTeam'
);
END IF;
...
END $$;
`)
await client.query('COMMIT')
} catch (err) {
await client.query('ROLLBACK')
throw err
}

@mattkrick mattkrick merged commit b0c2cf2 into master Jul 19, 2024
8 checks passed
@mattkrick mattkrick deleted the chore/teamMember-phase1 branch July 19, 2024 00:08
@github-actions github-actions bot mentioned this pull request Jul 19, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants