-
Notifications
You must be signed in to change notification settings - Fork 336
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
Conversation
Signed-off-by: Matt Krick <[email protected]>
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. |
WalkthroughThe 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
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (3)
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 (
|
There was a problem hiding this 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 usedpackages/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 nullablecreatedBy
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 SafetyThe method
clearAll
in theRootDataLoader
class can be improved for better type safety and consistency.- clearAll(pkLoaderName: TPrimaryLoaderNames): this + clearAll(pkLoaderName: TPrimaryLoaderNames): RootDataLoaderAdjust 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.
const teamMembers = await dataLoader.get('teamMembersByUserId').load(userIdToDelete) | ||
const teamIds = teamMembers.map(({id}) => TeamMemberId.split(id).teamId) |
There was a problem hiding this comment.
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.
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) |
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))] |
There was a problem hiding this comment.
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.
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 |
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(), |
There was a problem hiding this comment.
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.
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(), |
const teamMemberIds = teamIds.map((teamId) => TeamMemberId.join(teamId, viewerId)) | ||
const leadTeamMembers = (await dataLoader.get('teamMembers').loadMany(teamMemberIds)) | ||
.filter(isValid) | ||
.filter(({isLead}) => isLead) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
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.
const teamMembers = await dataLoader.get('teamMembersByTeamId').load(teamId) | ||
const teamLeader = teamMembers.find(({isLead}) => isLead)! | ||
const {userId: teamLeadUserId} = teamLeader |
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
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.
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), |
There was a problem hiding this comment.
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.
await pg | ||
.updateTable('TeamMember') | ||
.set({isNotRemoved: true}) | ||
.where('id', '=', teamMemberId) | ||
.execute() | ||
await r.table('TeamMember').get(teamMemberId).replace(existingTeamMember).run() | ||
dataLoader.clearAll('teamMembers') |
There was a problem hiding this comment.
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.
Signed-off-by: Matt Krick <[email protected]>
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]>
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. |
There was a problem hiding this 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
.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' | ||
}) | ||
) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
;(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() |
There was a problem hiding this comment.
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]>
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]>
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]>
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]>
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
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. |
There was a problem hiding this 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 initialisedismissedMeetingIds
but it's not updated within theuseEffect
. Consider using auseReducer
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 includesmeetings.length
, which might lead to missed updates if other dependencies change. Review and ensure all necessary dependencies are included.
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]>
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. |
There was a problem hiding this 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() |
There was a problem hiding this comment.
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.
await client.end() | |
try { | |
await client.end() | |
} catch (err) { | |
console.error('Failed to disconnect from the database:', err) | |
throw err | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
export async function down() { | ||
const client = new Client(getPgConfig()) | ||
await client.connect() |
There was a problem hiding this comment.
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.
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 | |
} |
await client.query(` | ||
DROP TABLE "TeamMember"; | ||
DROP TYPE "TeamDrawerEnum"; |
There was a problem hiding this comment.
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.
export async function up() { | ||
const client = new Client(getPgConfig()) | ||
await client.connect() |
There was a problem hiding this comment.
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.
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() |
There was a problem hiding this comment.
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.
await client.end() | |
try { | |
await client.end() | |
} catch (err) { | |
console.error('Failed to disconnect from the database:', err) | |
throw err | |
} |
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; |
There was a problem hiding this comment.
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.
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 | |
} |
Description
Writes to PG
Refactors reads to use a dataloader where possible
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores