Skip to content

feat(opportunity): add PreviewType support for opportunity previews#3543

Merged
capJavert merged 3 commits intomainfrom
feat/opportunity-preview-type
Feb 4, 2026
Merged

feat(opportunity): add PreviewType support for opportunity previews#3543
capJavert merged 3 commits intomainfrom
feat/opportunity-preview-type

Conversation

@capJavert
Copy link
Contributor

Save preview type from OpportunityPreviewResult to opportunity.flags.preview.type and pass PreviewType.ANALYSIS when triggering preview from private endpoint.

Save preview type from OpportunityPreviewResult to opportunity.flags.preview.type
and pass PreviewType.ANALYSIS when triggering preview from private endpoint.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@capJavert capJavert self-assigned this Feb 4, 2026
@capJavert capJavert requested a review from a team as a code owner February 4, 2026 12:04
@capJavert
Copy link
Contributor Author

@claude review

@claude
Copy link

claude bot commented Feb 4, 2026

Claude finished @capJavert's task in 1m 0s —— View job


PR Review

Reviewing changes for PreviewType support

Todo List

  • Read all changed files to understand implementation
  • Review code quality and adherence to guidelines
  • Check test coverage
  • Verify TypeORM patterns and best practices
  • Provide comprehensive feedback

Review Summary

This PR cleanly adds PreviewType support to opportunity previews. The implementation is well-structured with good test coverage. I found a few minor issues to address:

Issues Found

1. Potential N+1 Query Issue with Lazy Relation

Location: src/common/opportunity/preview.ts:22

const keywords = await opportunity.keywords;

According to CLAUDE.md best practices, awaiting lazy relations can cause N+1 queries if this function is called in a loop. While it's not necessarily called in a loop in this PR, it's a pattern to avoid.

Recommendation: Consider either:

  • Pass keywords as a parameter if the caller already has them loaded
  • Use eager loading when fetching the opportunity (query builder with leftJoinAndSelect)
  • Document that callers should load keywords beforehand

Fix this →

2. Test Assertion Style

Location: __tests__/workers/opportunity/opportunityPreviewResult.ts:170-175

expect(opportunity!.flags.preview).toEqual({
  userIds: ['1'],
  totalCount: 1,
  status: OpportunityPreviewStatus.READY,
  type: undefined,
});

Issue: Per CLAUDE.md guidelines, prefer avoiding assertions for undefined values. The test explicitly checks that type: undefined, but undefined properties are typically omitted from objects in JavaScript/TypeScript.

Recommendation: Consider restructuring this test to check that the type property is not set:

expect(opportunity!.flags.preview).toMatchObject({
  userIds: ['1'],
  totalCount: 1,
  status: OpportunityPreviewStatus.READY,
});
expect(opportunity!.flags.preview?.type).toBeUndefined();

Or simply omit the type field from the assertion entirely since it's optional.

Positive Observations

Excellent test coverage - Added 3 comprehensive test cases covering:

  • PreviewType.ANALYSIS
  • PreviewType.DEFAULT
  • No preview type provided

Clean TypeScript usage - Proper use of optional properties (type?: PreviewType)

Consistent pattern - The previewType parameter follows the existing optional parameter pattern

Schema update - Package version bump to @dailydotdev/schema@0.2.68 is included

Proper flag updates - Uses updateFlagsStatement utility as recommended in CLAUDE.md

Integration with existing flows - Correctly passes PreviewType.ANALYSIS when auto-triggering preview for machine-sourced opportunities (parseOpportunity.ts:125)

Code Quality Notes

  • Type safety is properly maintained throughout
  • No security issues detected
  • Follows repository conventions for worker implementations
  • JSONB flag pattern is correctly implemented

Minor Suggestions (Optional)

  1. Consider adding a comment explaining when each PreviewType should be used (ANALYSIS vs DEFAULT) in the type definition or the worker handler

  2. The lazy loading in preview.ts - While it works correctly in the current usage (parseOpportunity.ts:119 loads with relations: { keywords: true }), documenting this requirement would prevent future issues

Overall, this is a solid implementation. The primary concern is the potential N+1 query pattern, though it's not currently causing issues due to proper eager loading in the caller.


| Branch: feat/opportunity-preview-type

@pulumi
Copy link

pulumi bot commented Feb 4, 2026

🍹 The Update (preview) for dailydotdev/api/prod (at 71785db) was successful.

Resource Changes

    Name                                                       Type                           Operation
~   vpc-native-update-current-streak-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-images-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-generic-referral-reminder-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-expire-super-agent-trial-cron                   kubernetes:batch/v1:CronJob    update
~   vpc-native-update-source-tag-view-cron                     kubernetes:batch/v1:CronJob    update
~   vpc-native-bg-deployment                                   kubernetes:apps/v1:Deployment  update
~   vpc-native-sync-subscription-with-cio-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-private-deployment                              kubernetes:apps/v1:Deployment  update
~   vpc-native-user-posts-analytics-refresh-cron               kubernetes:batch/v1:CronJob    update
~   vpc-native-update-highlighted-views-cron                   kubernetes:batch/v1:CronJob    update
+   vpc-native-api-clickhouse-migration-483cf32c               kubernetes:batch/v1:Job        create
+   vpc-native-api-db-migration-483cf32c                       kubernetes:batch/v1:Job        create
~   vpc-native-clean-gifted-plus-cron                          kubernetes:batch/v1:CronJob    update
~   vpc-native-check-analytics-report-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-personalized-digest-deployment                  kubernetes:apps/v1:Deployment  update
~   vpc-native-clean-zombie-opportunities-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-history-day-clickhouse-cron      kubernetes:batch/v1:CronJob    update
~   vpc-native-ws-deployment                                   kubernetes:apps/v1:Deployment  update
~   vpc-native-temporal-deployment                             kubernetes:apps/v1:Deployment  update
-   vpc-native-api-clickhouse-migration-74d8ab5e               kubernetes:batch/v1:Job        delete
-   vpc-native-api-db-migration-74d8ab5e                       kubernetes:batch/v1:Job        delete
~   vpc-native-update-source-public-threshold-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-hourly-notification-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-calculate-top-readers-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-user-profile-updated-sync-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-generate-search-invites-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-user-profile-analytics-history-clickhouse-cron  kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-users-cron                         kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-stale-user-transactions-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-personalized-digest-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-daily-digest-cron                               kubernetes:batch/v1:CronJob    update
~   vpc-native-update-trending-cron                            kubernetes:batch/v1:CronJob    update
~   vpc-native-user-profile-analytics-clickhouse-cron          kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-user-companies-cron                kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-clickhouse-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-deployment                                      kubernetes:apps/v1:Deployment  update
~   vpc-native-validate-active-users-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-update-views-cron                               kubernetes:batch/v1:CronJob    update
... and 1 other changes

@capJavert capJavert merged commit d3469a7 into main Feb 4, 2026
9 checks passed
@capJavert capJavert deleted the feat/opportunity-preview-type branch February 4, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant