fix misattributed activities based on username + memberId #2715
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several new functionalities related to managing activities and member identities within the script executor worker. Key additions include four asynchronous functions for finding and updating activities, a new interface for activity arguments, and a workflow for fixing misattributed activities. Additionally, a new repository class for activity data management is created, along with enhancements to the member repository to find member identities. These changes expand the public API and improve the overall structure for handling activities and member data. Changes
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
services/apps/script_executor_worker/src/activities.ts(2 hunks)services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts(1 hunks)services/apps/script_executor_worker/src/types.ts(1 hunks)services/apps/script_executor_worker/src/workflows.ts(1 hunks)services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/member.repo.ts(2 hunks)
🔇 Additional comments (2)
services/apps/script_executor_worker/src/workflows.ts (1)
4-4: Import and export statements are correctly added
The import and export of fixMisattributedActivities are properly incorporated, ensuring the workflow is accessible where needed.
Also applies to: 12-12
services/apps/script_executor_worker/src/activities.ts (1)
13-17: Import and export new activity functions
The addition of findActivitiesWithWrongMembers, findMemberIdentity, and updateActivities to both imports and exports is correctly implemented, integrating the new functionalities into the module.
Also applies to: 46-48
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts
Show resolved
Hide resolved
services/libs/data-access-layer/src/old/apps/script_executor_worker/member.repo.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts (2)
11-18: Add input validation and consider configuration externalizationThe function lacks input validation and uses hardcoded limits. Consider:
- Validating input arguments
- Moving limits to configuration
Apply this diff:
+const DEFAULT_LIMITS = { + TEST_MODE: 10, + PRODUCTION: 100, +} as const; + export async function fixMisattributedActivities( args: IFixMisattributedActivitiesArgs, ): Promise<void> { + if (args === undefined) { + throw new Error('Arguments are required'); + } + - const PROCESS_ACTIVITIES_PER_RUN = args.testRun ? 10 : 100 + const PROCESS_ACTIVITIES_PER_RUN = args.testRun + ? DEFAULT_LIMITS.TEST_MODE + : DEFAULT_LIMITS.PRODUCTION;
11-43: Consider adding monitoring and metricsFor better operational visibility, consider:
- Adding metrics for tracking progress (e.g., activities processed, success rate)
- Implementing structured logging for monitoring
- Adding checkpoints for recovery
Example metrics implementation:
interface IWorkflowMetrics { activitiesProcessed: number; successCount: number; failureCount: number; lastProcessedTimestamp: number; } // Add to workflow args interface IFixMisattributedActivitiesArgs { testRun: boolean; metrics?: IWorkflowMetrics; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts(1 hunks)
🔇 Additional comments (3)
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts (3)
6-9: 🛠️ Refactor suggestion
Adjust activity configuration for better resilience
The current timeout and retry configuration could be optimized for database operations.
29-36:
Implement proper error handling and logging strategy
The current implementation has several issues:
- Commented-out update logic
- Console logs in production code
- No error handling
38-42:
Implement safeguards against infinite workflow execution
The workflow continuation lacks progress tracking and could potentially run indefinitely.
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts (1)
17-17: Use a structured logger instead ofconsole.logUsing
console.logandconsole.warnis not recommended in production code. Consider using a structured logger provided by your logging framework to ensure consistent logging practices.Apply this diff to replace
console.logwith the logger:- console.log(`Running in test mode with limit 10!`) + svc.log.info(`Running in test mode with limit 10!`)Similarly, update other logging statements in the code.
Also applies to: 34-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts(1 hunks)services/apps/script_executor_worker/src/types.ts(1 hunks)services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/member.repo.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/apps/script_executor_worker/src/types.ts
🔇 Additional comments (9)
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts (3)
22-25:
Add error handling for 'findActivitiesWithWrongMembers' call
The call to findActivitiesWithWrongMembers lacks error handling, which could lead to workflow failures if an exception occurs during the activity execution.
Apply this diff to add error handling:
+ let activitiesWithWrongMember = []
+ try {
const activitiesWithWrongMember = await activity.findActivitiesWithWrongMembers(
tenantId,
PROCESS_ACTIVITIES_PER_RUN,
)
+ } catch (err) {
+ console.error('Error fetching activities with wrong members:', err)
+ throw err
+ }41-46:
Add safeguards against infinite workflow loops
The workflow could potentially run indefinitely if misattributed activities continue to exist. Consider adding a stopping condition or a maximum retry count to prevent infinite loops.
Apply this diff to add a safeguard:
if (!args.testRun) {
+ // Stop the workflow after a maximum number of attempts
+ const maxAttempts = args.maxAttempts || 5
+ const currentAttempt = args.currentAttempt || 1
+ if (currentAttempt >= maxAttempts) {
+ console.log(`Reached maximum attempts (${maxAttempts}), stopping workflow.`)
+ return
+ }
+
await continueAsNew<typeof fixMisattributedActivities>({
testRun: args.testRun,
tenantId: args.tenantId,
+ maxAttempts,
+ currentAttempt: currentAttempt + 1,
})
}You'll need to update the IFixMisattributedActivitiesArgs interface to include the maxAttempts and currentAttempt fields.
33-39:
Add error handling for 'findMemberIdentity' and 'updateActivities' calls
The calls to findMemberIdentity and updateActivities lack error handling. Exceptions during these activities could cause the workflow to fail.
Apply this diff to add error handling and uncomment the update logic:
for (const a of activitiesWithWrongMember) {
- const memberIdentity = await activity.findMemberIdentity(a.username, a.platform, tenantId)
- console.log('activity with wrong member', a)
- console.log('memberIdentity found for username and platform', memberIdentity)
- // if (memberIdentity) {
- // await activity.updateActivities(a.username, a.platform, memberIdentity.memberId)
- // }
+ try {
+ const memberIdentity = await activity.findMemberIdentity(a.username, a.platform, tenantId)
+ console.log('Activity with wrong member:', a)
+ console.log('Member identity found:', memberIdentity)
+ if (memberIdentity) {
+ await activity.updateActivities(a.username, a.platform, memberIdentity.memberId, tenantId)
+ console.log(`Updated activities for ${a.username} on ${a.platform}`)
+ } else {
+ console.warn(`No member identity found for ${a.username} on ${a.platform}`)
+ }
+ } catch (err) {
+ console.error(`Error processing activity with ID ${a.id}:`, err)
+ }
}Likely invalid or redundant comment.
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts (3)
17-17:
Improve error handling to preserve original error details
In the catch blocks, you're creating a new Error instance with err as the message. This can lead to the loss of the original error's stack trace and message content. Instead, rethrow the original error or wrap it properly while maintaining the original error information.
Apply this diff to fix the error handling in all functions:
} catch (err) {
- throw new Error(err)
+ throw err
}Also applies to: 30-30, 46-46
10-21:
Ensure proper database connection cleanup
The database connections are not being closed properly, which can lead to connection leaks. Make sure to disconnect the database connections in a finally block.
Apply this diff to all functions to include connection cleanup:
For findActivitiesWithWrongMembers:
export async function findActivitiesWithWrongMembers(tenantId: string, limit: number) {
- let activitiesWithWrongMember: IActivityWithWrongMember[] = []
+ let activitiesWithWrongMember: IActivityWithWrongMember[] = []
+ const client = svc.postgres.reader.connection()
try {
- const activityRepo = new ActivityRepository(svc.postgres.reader.connection(), svc.log)
+ const activityRepo = new ActivityRepository(client, svc.log)
activitiesWithWrongMember = await activityRepo.getActivitiesWithWrongMembers(tenantId, limit)
} catch (err) {
throw err
+ } finally {
+ await client.disconnect()
}
return activitiesWithWrongMember
}Apply similar changes to findMemberIdentity and updateActivities functions.
Also applies to: 23-34, 36-48
36-48:
Add transaction handling to 'updateActivities' function
The update operation should be wrapped in a transaction to ensure data consistency, especially if multiple updates are involved.
Apply this diff to add transaction handling:
export async function updateActivities(
username: string,
platform: string,
correctMemberId: string,
tenantId: string,
) {
+ const client = svc.postgres.writer.connection()
try {
+ await client.tx(async (tx) => {
+ const activityRepo = new ActivityRepository(tx, svc.log)
+ await activityRepo.updateActivities(username, platform, correctMemberId, tenantId)
+ })
- const activityRepo = new ActivityRepository(svc.postgres.writer.connection(), svc.log)
- await activityRepo.updateActivities(username, platform, correctMemberId, tenantId)
} catch (err) {
throw err
+ } finally {
+ await client.disconnect()
}
}services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (1)
44-46:
Preserve original error details in catch blocks
In the catch blocks, you're creating a new Error instance with err as the message, which can cause the loss of the original error's stack trace and message. Re-throw the original error to maintain all error details.
Apply this diff to both methods:
For getActivitiesWithWrongMembers:
} catch (err) {
this.log.error('Error while finding activities!', err)
- throw new Error(err)
+ throw err
}For updateActivities:
} catch (err) {
this.log.error('Error while updating activities!', err)
- throw new Error(err)
+ throw err
}Also applies to: 73-75
services/libs/data-access-layer/src/old/apps/script_executor_worker/member.repo.ts (2)
173-175:
Add case-insensitive comparison and tenant isolation
The value comparison in the findMemberIdentity method is case-sensitive, which could miss matches with different casing. Additionally, ensure that tenant isolation is enforced to prevent cross-tenant data exposure.
Apply this diff to fix both issues:
select * from "memberIdentities"
- where value = $(username) and platform = $(platform)
+ where lower(value) = lower($(username)) and platform = $(platform)
and type = $(type) and verified = true and "tenantId" = $(tenantId)185-187:
Preserve original error details in catch blocks
You're creating a new Error instance with err as the message, which can lose the original error details. Re-throw the original error instead.
Apply this diff:
} catch (err) {
this.log.error('Error while finding member identity!', err)
- throw new Error(err)
+ throw err
}
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (2)
16-48: Consider adding query documentationThe SQL query is well-structured but complex. Consider adding a comment block explaining the join conditions and their purpose for better maintainability.
Add documentation above the query:
async getActivitiesWithWrongMembers( tenantId: string, limit = 100, ): Promise<IActivityWithWrongMember[]> { try { + /** + * Finds activities where the memberId doesn't match the verified identity: + * - Joins with memberIdentities on username and platform + * - Ensures identity is verified and of type 'username' + * - Groups by username and platform to get activity counts + * - Orders by count to prioritize frequently occurring mismatches + */ return await this.connection.query(
50-77: Add transaction warning for bulk updatesThe update method could affect multiple rows. Consider adding a warning in the documentation about using transactions for bulk updates to maintain data consistency.
Add a warning in the method documentation:
+ /** + * Updates activities with the correct memberId. + * @warning This method can affect multiple activities. Consider wrapping in a transaction + * when updating large datasets to maintain consistency. + */ async updateActivities( username: string, platform: string, correctMemberId: string, tenantId: string, ): Promise<void> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts(1 hunks)
🔇 Additional comments (4)
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (4)
1-8: LGTM! Clean interface definition with appropriate types.
The interface and imports are well-structured and provide a clear contract for activity-member mismatches.
10-14: LGTM! Well-structured constructor with proper dependency injection.
The class follows good practices with dependency injection and proper encapsulation.
45-47: Fix error handling to preserve stack traces
The error handling can lose important stack trace information.
74-76: Fix error handling to preserve stack traces
The error handling can lose important stack trace information.
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (2)
11-41: Optimize query performance and add index hintsThe query could benefit from performance optimizations and explicit index hints.
async getActivitiesWithWrongMembers( tenantId: string, limit = 100, ): Promise<IActivityCreateData[]> { try { return await this.connection.query( ` SELECT a.id, a.username, a.platform - FROM activities a + FROM activities a USE INDEX (idx_activities_tenant) JOIN "memberIdentities" mi ON a.username = mi.value AND a.platform = mi.platform AND mi.type = 'username' AND mi."verified" = true AND a."tenantId" = mi."tenantId" WHERE a."memberId" <> mi."memberId" AND a."tenantId" = $(tenantId) + ORDER BY a.id /* Ensure consistent ordering for pagination */ LIMIT $(limit) `,
43-72: Add batch update capability for better performanceConsider implementing batch updates to improve performance when processing multiple activities.
+ async updateActivitiesWithWrongMemberBatch( + updates: Array<{ + activityId: string, + username: string, + platform: string, + correctMemberId: string, + tenantId: string + }>, + ): Promise<void> { + try { + const values = updates.map(u => + `(${this.connection.escape(u.activityId)}, ${this.connection.escape(u.correctMemberId)})` + ).join(','); + + await this.connection.none(` + UPDATE activities a + SET "memberId" = v.correct_member_id + FROM (VALUES ${values}) AS v(activity_id, correct_member_id) + WHERE a.id = v.activity_id + `); + } catch (err) { + this.log.error('Error while batch updating activities!', err) + throw err instanceof Error ? err : new Error(String(err)) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
services/apps/script_executor_worker/src/activities.ts(2 hunks)services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts(1 hunks)services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/apps/script_executor_worker/src/activities.ts
🔇 Additional comments (6)
services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts (3)
45-50: 🛠️ Refactor suggestion
Add progress tracking between workflow runs
The workflow could run indefinitely if activities can't be fixed. Add progress tracking.
if (!args.testRun) {
+ // Track progress between runs
+ const processedCount = activitiesWithWrongMember.length
+ if (processedCount === 0) {
+ console.log('No more activities to process')
+ return
+ }
await continueAsNew<typeof fixMisattributedActivities>({
testRun: args.testRun,
tenantId: args.tenantId,
+ processedInLastRun: processedCount,
})
}Likely invalid or redundant comment.
6-9: 🛠️ Refactor suggestion
Adjust activity configuration timeout
The 10-minute timeout seems excessive for database operations. Consider reducing it and implementing a more sophisticated retry strategy.
const activity = proxyActivities<typeof activities>({
- startToCloseTimeout: '10 minute',
- retry: { maximumAttempts: 3 },
+ startToCloseTimeout: '1 minute',
+ retry: {
+ maximumAttempts: 3,
+ initialInterval: '1 second',
+ backoffCoefficient: 2,
+ maximumInterval: '10 seconds'
+ },
})Likely invalid or redundant comment.
32-43: 🛠️ Refactor suggestion
Add error handling and logging in the activity execution loop
The loop processing activities lacks error handling and progress logging.
for (const a of activitiesWithWrongMember) {
+ try {
const memberIdentity = await activity.findMemberIdentity(a.username, a.platform, tenantId)
if (memberIdentity) {
await activity.updateActivityWithWrongMember(
a.id,
a.username,
a.platform,
memberIdentity.memberId,
tenantId,
)
+ console.log(`Successfully updated activity ${a.id} for member ${memberIdentity.memberId}`)
}
+ } catch (error) {
+ console.error(`Failed to process activity ${a.id}:`, error)
+ // Continue with next activity instead of failing the entire batch
+ }
}Likely invalid or redundant comment.
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts (2)
7-18:
Improve database connection handling and error propagation
The function needs proper connection cleanup and better error handling.
export async function findActivitiesWithWrongMembers(tenantId: string, limit: number) {
let activitiesWithWrongMember: IActivityCreateData[] = []
+ const connection = svc.postgres.reader.connection()
try {
- const activityRepo = new ActivityRepository(svc.postgres.reader.connection(), svc.log)
+ const activityRepo = new ActivityRepository(connection, svc.log)
activitiesWithWrongMember = await activityRepo.getActivitiesWithWrongMembers(tenantId, limit)
} catch (err) {
- throw new Error(err)
+ throw err instanceof Error ? err : new Error(String(err))
+ } finally {
+ await connection.disconnect()
}
return activitiesWithWrongMember
}Likely invalid or redundant comment.
33-52:
Add transaction handling for activity updates
Updates should be wrapped in a transaction to ensure data consistency.
export async function updateActivityWithWrongMember(
activityId: string,
username: string,
platform: string,
correctMemberId: string,
tenantId: string,
) {
+ const connection = svc.postgres.writer.connection()
try {
- const activityRepo = new ActivityRepository(svc.postgres.writer.connection(), svc.log)
+ await connection.tx(async (tx) => {
+ const activityRepo = new ActivityRepository(tx, svc.log)
+ await activityRepo.updateActivityWithWrongMember(
+ activityId,
+ username,
+ platform,
+ correctMemberId,
+ tenantId,
+ )
+ })
} catch (err) {
- throw new Error(err)
+ throw err instanceof Error ? err : new Error(String(err))
+ } finally {
+ await connection.disconnect()
}
}Likely invalid or redundant comment.
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (1)
23-30: Verify index coverage for the JOIN conditions
The query joins on multiple conditions. Ensure proper indexes exist for optimal performance.
✅ Verification successful
Let's analyze the query conditions and the existing indexes. The JOIN conditions are:
a.username = mi.valuea.platform = mi.platformmi.type = 'username'mi.verified = truea.tenantId = mi.tenantId
And WHERE conditions:
a.memberId <> mi.memberIda.tenantId = $(tenantId)
Looking at the indexes found:
- For
memberIdentities: There's an indexix_memberIdentities_tenantId_platform_value_typecovering (tenantId,platform,value,type) - For
activities: Multiple indexes exist including composite indexes withtenantId,platform, and other fields
Index coverage is adequate for the query performance
The existing indexes provide good coverage for this query:
- The
memberIdentitiesindex covers most JOIN conditions (tenantId, platform, value, type) - Multiple
activitiesindexes exist that can help with the tenantId and platform filtering
The query optimizer can use these indexes effectively for the JOIN operation. No additional indexes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing indexes on the tables
rg -g '*.sql' -A 5 'CREATE.*INDEX.*(activities|memberIdentities)'
# Look for any existing index creation migrations
fd -e sql | xargs grep -l 'CREATE.*INDEX.*(activities|memberIdentities)'
Length of output: 7214
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (1)
23-27: Optimize JOIN conditions for better performanceThe JOIN conditions can be simplified. Since we're already matching on
tenantIdin the WHERE clause, we don't need it in the JOIN conditions.JOIN "memberIdentities" mi ON a.username = mi.value AND a.platform = mi.platform AND mi.type = 'username' AND mi."verified" = true - AND a."tenantId" = mi."tenantId"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts(1 hunks)services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts
- services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts
🔇 Additional comments (5)
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (5)
1-10: LGTM! Well-structured class initialization
The class follows good practices with proper dependency injection and type definitions.
37-40: Improve error handling to preserve original error details
The error handling needs improvement to preserve the original error stack trace.
} catch (err) {
this.log.error('Error while finding activities!', err)
- throw new Error(err)
+ throw err
}56-59: Improve error handling to preserve original error details
The error handling needs improvement to preserve the original error stack trace.
} catch (err) {
this.log.error('Error while updating activities!', err)
- throw new Error(err)
+ throw err
}43-55: 🛠️ Refactor suggestion
Add input validation and transaction handling documentation
Consider adding:
- Validation to ensure correctMemberId exists before updating
- Documentation about transaction handling, especially since the class accepts both DbConnection and DbTransaction
+ /**
+ * Updates the memberId of an activity with the correct member ID
+ * @param activityId The ID of the activity to update
+ * @param correctMemberId The correct member ID to set
+ * @throws Error if the activity or member doesn't exist
+ * @remarks This method should be called within a transaction when updating multiple activities
+ */
async updateActivityWithWrongMember(activityId: string, correctMemberId: string): Promise<void> {
try {
+ // Verify the member exists
+ const memberExists = await this.connection.oneOrNone(
+ 'SELECT id FROM members WHERE id = $(correctMemberId)',
+ { correctMemberId }
+ )
+ if (!memberExists) {
+ throw new Error(`Member with ID ${correctMemberId} does not exist`)
+ }
await this.connection.none(Likely invalid or redundant comment.
11-41: Verify complete coverage of misattribution scenarios
Let's verify that this implementation covers all possible misattribution scenarios mentioned in the PR objectives.
✅ Verification successful
Implementation correctly handles activity-member misattribution
The implementation comprehensively addresses the misattribution scenario by:
- Correctly identifying activities where username and platform match a verified member identity but have incorrect memberId
- Using verified member identities only (
mi."verified" = true) - Providing both detection (
getActivitiesWithWrongMembers) and correction (updateActivityWithWrongMember) methods - Ensuring tenant isolation through proper tenantId checks
The query implementation covers the core case where an activity is assigned to the wrong member while a verified identity exists for the correct member, which aligns with the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential edge cases in member-activity relationships
# Look for other activity-member relationship handling
rg -g '*.{ts,js}' 'JOIN.*activities.*member' -A 5
# Check for member identity verification patterns
rg -g '*.{ts,js}' 'memberIdentities.*username.*verified' -A 5
# Look for any special handling of platform-specific usernames
rg -g '*.{ts,js}' 'platform.*username.*activities' -A 5
Length of output: 173
Script:
#!/bin/bash
# Let's try a different approach to find related code
# Search for activity-member relationship queries
rg -g '*.{ts,js}' '"memberId".*activities' -A 5
# Look for similar SQL queries involving activities and members
rg -g '*.{ts,js}' 'SELECT.*FROM activities.*WHERE' -A 10
# Check for any other methods in the same repository file
fd -e ts "activities.repo.ts" --exec cat {}
Length of output: 5923
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (1)
21-34: Consider optimizing the SQL queryA few suggestions to improve the query:
- Consider case-insensitive username comparison
- The tenantId check is redundant in WHERE clause
- The return type suggests IActivityCreateData but only 3 fields are selected
Consider this optimization:
SELECT a.id, a.username, - a.platform + a.platform, + a.* -- Select all fields to match IActivityCreateData FROM activities a JOIN "memberIdentities" mi ON - a.username = mi.value + LOWER(a.username) = LOWER(mi.value) AND a.platform = mi.platform AND mi.type = 'username' AND mi."verified" = true AND a."tenantId" = mi."tenantId" - WHERE a."memberId" <> mi."memberId" - AND a."tenantId" = $(tenantId) + WHERE a."memberId" <> mi."memberId" LIMIT $(limit)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts(1 hunks)services/apps/script_executor_worker/src/main.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts
🔇 Additional comments (6)
services/apps/script_executor_worker/src/main.ts (2)
13-13: Verify QuestDB setup and dependencies
Enabling QuestDB suggests it's now a required dependency for the activity management features.
Consider documenting:
- QuestDB connection details
- Data migration plan if any
- Backup strategy
✅ Verification successful
Let me gather more information about the QuestDB setup by checking the implementation details.
QuestDB is properly configured and integrated
The verification shows that QuestDB is well-integrated with proper configuration and dependencies:
- QuestDB library is properly set up with both ILP and SQL clients
- Required environment variables are defined (
CROWD_QUESTDB_*) - Dependencies are correctly specified in package.json (
@questdb/nodejs-clientandpg-promise) - Connection handling, error management, and telemetry are implemented
- SSL is enabled for production environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if QuestDB related configurations and dependencies are properly set up
# Check for QuestDB connection configuration
rg -l "questdb.*connection|questdb.*url|questdb.*host"
# Check for QuestDB related environment variables
rg -l "QUESTDB_" .env* 2>/dev/null || echo "No QuestDB environment variables found"
# Look for QuestDB related dependencies in package files
find . -name "package.json" -exec jq -r '.dependencies | select(has("questdb"))' {} \;
Length of output: 324
Script:
#!/bin/bash
# Check QuestDB implementation details
echo "=== QuestDB Library Implementation ==="
cat services/libs/questdb/src/ilp.ts services/libs/questdb/src/sql.ts
echo -e "\n=== QuestDB Configuration in Script Executor ==="
cat services/apps/script_executor_worker/src/main.ts
echo -e "\n=== Package Dependencies ==="
cat services/libs/questdb/package.json
Length of output: 4904
13-13: Ensure configuration consistency across environments
The QuestDB configuration change might need to be reflected in other environments.
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (4)
1-13: LGTM! Well-structured class initialization
The class follows good practices with proper dependency injection and clear constructor parameter types.
41-43: Improve error handling to preserve error details
The error handling can lose the original error's stack trace and details.
71-73: Improve error handling to preserve error details
The error handling can lose the original error's stack trace and details.
14-74: Verify complete coverage of misattribution scenarios
Let's verify that this implementation covers all possible misattribution scenarios mentioned in the PR objectives.
✅ Verification successful
Based on the search results, I can now generate a final response:
Implementation correctly handles member-activity misattribution
The implementation is complete and correctly handles the misattribution scenario. The search results confirm that:
- This is the only place where member-activity relationships are corrected based on verified identities
- Other activity-member updates in the codebase handle different scenarios (conversations, organizations, etc.)
- The query correctly joins activities with verified member identities to detect mismatches
The implementation will effectively identify and fix cases where activities are associated with incorrect member IDs by comparing against verified username identities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential member-activity relationship handling in the codebase
# to ensure we haven't missed any edge cases
# Look for similar activity-member joins
rg -g '*.{ts,js}' 'JOIN.*activities.*member' -A 5
# Look for other activity updates
rg -g '*.{ts,js}' 'UPDATE.*activities.*SET.*member' -A 5
# Look for related member identity verification logic
ast-grep --pattern 'memberIdentities.+verified.+username'
Length of output: 168
Script:
#!/bin/bash
# Let's try a different approach to find related code patterns
# Look for activity-member relationship handling
rg -g '*.{ts,js}' '"memberId".*activities' -A 5
# Search for member identity verification patterns
rg -g '*.{ts,js}' '"memberIdentities".*"verified"' -A 5
# Look for similar activity update patterns
rg -g '*.{ts,js}' 'UPDATE.*activities' -A 5
# Search for activity-member relationship in repository
ast-grep --pattern 'activities.*memberId.*WHERE'
Length of output: 7096
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts (1)
8-8: Specify explicit type foractivitiesWithWrongMemberIt's recommended to explicitly declare the type of
activitiesWithWrongMemberfor better type safety and code clarity.Apply this diff:
- let activitiesWithWrongMember = [] + let activitiesWithWrongMember: any[] = []Replace
any[]with the appropriate type based on the expected structure of the activities.services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (1)
23-32: Consider indexing columns used in JOIN and WHERE clausesTo improve query performance, consider adding indexes on columns used in JOIN and WHERE clauses, such as
a.username,a.platform, andmi.value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
services/apps/script_executor_worker/src/activities.ts(2 hunks)services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts(1 hunks)services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts(1 hunks)services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/apps/script_executor_worker/src/activities.ts
- services/apps/script_executor_worker/src/workflows/fixMisattributedActivities.ts
🔇 Additional comments (3)
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts (1)
17-19: Improve error handling to preserve original error details
In your catch blocks, you're wrapping the original error by creating a new Error instance with err as the message. This practice can lead to the loss of the original error's stack trace and additional information. Instead, you should rethrow the original error to maintain the stack trace.
Apply this diff to fix the error handling:
} catch (err) {
- throw new Error(err)
+ throw err
}Also applies to: 31-33, 46-48, 62-64
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts (2)
40-42: Improve error handling to preserve original error details
In your catch blocks, you're wrapping the original error by creating a new Error instance with err as the message. This can lead to the loss of the original error's stack trace and message content. Instead, rethrow the original error to maintain its stack trace.
Apply this diff:
} catch (err) {
this.log.error('Error while finding activities!', err)
- throw new Error(err)
+ throw err
}Repeat similar adjustments for the other catch blocks.
Also applies to: 71-73, 103-105
17-38: Use parameterized queries to prevent SQL injection
While you're using query parameters, ensure that all variables are properly parameterized to prevent SQL injection attacks. Review the query to confirm that all dynamic values are safely injected.
[security_issue]
Verify that all variables in the SQL query are parameterized and not concatenated directly into the string.
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts
Outdated
Show resolved
Hide resolved
services/apps/script_executor_worker/src/activities/fix-misattributed-activities/index.ts
Show resolved
Hide resolved
services/libs/data-access-layer/src/old/apps/script_executor_worker/activities.repo.ts
Show resolved
Hide resolved
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
New Features
Bug Fixes