-
Notifications
You must be signed in to change notification settings - Fork 113
feat: anon login claim opportunities #3463
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
base: main
Are you sure you want to change the base?
Conversation
|
🍹 The Update (preview) for dailydotdev/api/prod (at a2fdd84) was successful. Resource Changes Name Type Operation
~ vpc-native-check-analytics-report-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generic-referral-reminder-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-trending-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-tags-str-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-tag-recommendations-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generate-search-invites-cron kubernetes:batch/v1:CronJob update
~ vpc-native-hourly-notification-cron kubernetes:batch/v1:CronJob update
~ vpc-native-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-bg-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-daily-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-tag-view-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-stale-user-transactions-cron kubernetes:batch/v1:CronJob update
~ vpc-native-calculate-top-readers-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-images-cron kubernetes:batch/v1:CronJob update
~ vpc-native-private-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-sync-subscription-with-cio-cron kubernetes:batch/v1:CronJob update
~ vpc-native-temporal-deployment kubernetes:apps/v1:Deployment update
- vpc-native-api-clickhouse-migration-c719572f kubernetes:batch/v1:Job delete
- vpc-native-api-db-migration-c719572f kubernetes:batch/v1:Job delete
~ vpc-native-clean-zombie-user-companies-cron kubernetes:batch/v1:CronJob update
~ vpc-native-ws-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-post-analytics-history-day-clickhouse-cron kubernetes:batch/v1:CronJob update
+ vpc-native-api-db-migration-1136a091 kubernetes:batch/v1:Job create
~ vpc-native-clean-gifted-plus-cron kubernetes:batch/v1:CronJob update
+ vpc-native-api-clickhouse-migration-1136a091 kubernetes:batch/v1:Job create
~ vpc-native-personalized-digest-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-current-streak-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-highlighted-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-validate-active-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-opportunities-cron kubernetes:batch/v1:CronJob update
~ vpc-native-post-analytics-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-public-threshold-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-user-profile-updated-sync-cron kubernetes:batch/v1:CronJob update
|
|
@claude review |
|
Claude finished @capJavert's task in 2m 10s —— View job Code ReviewI've reviewed this PR that refactors the ClaimableItem entity from email-specific to a more generic identifier-based approach and adds the ability for logged-in users to claim anonymous opportunities. Overall, the implementation is solid with good test coverage. Here are my findings: Strengths1. Good Migration Design
2. Comprehensive Test Coverage
3. Transaction Safety
4. Proper Security
Issues & Recommendations1. Potential Bug in Cron - Empty IDs Check The cron job filters out falsy items but this check is insufficient: .where(`flags->>'opportunityId' IN (:...ids)`, {
ids: raw
.filter((item: { id: string }) => item)
.map((item: { id: string }) => item.id),
})Problem: If Fix: Add early return or check for empty array: if (raw.length === 0 || raw.every((item: { id: string }) => !item)) {
logger.info({ count: 0, claimables: 0 }, 'zombies opportunities cleaned! 🧟');
return;
}
const opportunityIds = raw
.filter((item: { id: string }) => item?.id)
.map((item: { id: string }) => item.id);
if (opportunityIds.length === 0) {
logger.info({ count: affected, claimables: 0 }, 'zombies opportunities cleaned! 🧟');
return;
}
const { affected: claimables } = await entityManager
.getRepository(ClaimableItem)
.createQueryBuilder()
.delete()
.where(`flags->>'opportunityId' IN (:...ids)`, { ids: opportunityIds })
.execute();2. Inconsistent Null Handling in .filter((item) => item.flags.opportunityId)This filter is good but the type system doesn't reflect that Recommendation: Add TypeScript type guard or use optional chaining in map: .filter((item): item is ClaimableItem & { flags: { opportunityId: string } } =>
!!item.flags.opportunityId
)
.map((item) => item.flags.opportunityId)3. Migration Feedback - Consider IF EXISTS/IF NOT EXISTS The migration uses Current: Works but could fail on rollback/replay 4. Missing Transaction Await con.transaction(async (entityManager) => {The transaction is not awaited! This means the cron handler returns before the transaction completes. Fix: await con.transaction(async (entityManager) => {5. Consider Adding Index on The entity has queries filtering by @Column({ nullable: true })
@Index('IDX_claimable_item_claimed_by_id')
claimedById?: string;This is optional but could improve query performance as the table scales. 6. Minor: Hardcoded Magic Number const timeThreshold = subDays(new Date(), 2);Per CLAUDE.md guidelines, prefer using constants from const timeThreshold = subDays(new Date(), 2 * ONE_DAY_IN_DAYS); // if constant existsThough for this case, the inline SummaryMust Fix (Blocking):
Should Consider:
Nice to Have:
The core functionality is well-implemented with solid test coverage. After addressing the two critical issues (#1 and #4), this PR will be good to merge. |
rebelchris
left a comment
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.
Looks solid to me
emailcolumn toidentifier, to make it more generic and reusable