Skip to content

Conversation

@capJavert
Copy link
Contributor

@capJavert capJavert commented Jan 20, 2026

Changes

Enable claiming opportunities from anon flow after login

API dailydotdev/daily-api#3463

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Preview domain

https://anon-login-claim-opportunity.preview.app.daily.dev

@capJavert capJavert self-assigned this Jan 20, 2026
@capJavert capJavert requested a review from a team as a code owner January 20, 2026 23:01
@vercel
Copy link

vercel bot commented Jan 20, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
daily-webapp Ready Ready Preview Jan 22, 2026 1:16pm
1 Skipped Deployment
Project Deployment Review Updated (UTC)
storybook Ignored Ignored Jan 22, 2026 1:16pm

Request Review

@capJavert
Copy link
Contributor Author

@claude review

@claude
Copy link
Contributor

claude bot commented Jan 22, 2026

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


Code Review

  • Read changed files
  • Analyze implementation
  • Provide review feedback

Summary

This PR implements anonymous user flow for claiming opportunities after login. The implementation is solid overall with good error handling and proper integration with the existing auth flow. I found a few minor issues and areas for improvement.

Key Changes

  1. RecruiterSignInModal.tsx: Added logic to claim opportunities after successful login using the saved trackingId
  2. OpportunityPreviewContext.tsx: No functional changes (just file read for context)
  3. graphql.ts: Added CLAIM_OPPORTUNITIES_MUTATION GraphQL mutation
  4. mutations.ts: Added claimOpportunitiesMutationOptions mutation helper
  5. types.ts: Added OpportunitiesClaim type definition

Issues & Suggestions

1. Missing Error Feedback to User

Severity: Medium

In RecruiterSignInModal.tsx:54-66, the handleSuccessfulLogin function silently swallows errors from the claimOpportunities mutation:

try {
  if (trackingIdState) {
    await claimOpportunities({ identifier: trackingIdState });
  }
} catch {
  // if we can't claim at this time we move on
}

While it's good that the flow continues even if claiming fails, the user has no visibility into whether their opportunities were successfully claimed. Consider:

  • Logging the error for debugging purposes
  • Optionally showing a toast notification if claiming fails (though this depends on your UX requirements)

Recommendation: At minimum, add logging:

} catch (error) {
  // Log for debugging but don't block the flow
  console.error('Failed to claim opportunities:', error);
}

2. Race Condition Risk

Severity: Low

The trackingId is captured on mount via useState(trackingId) (line 30), but there's a potential race condition if the auth context updates trackingId between component mount and login completion. This is unlikely in practice but worth noting.

Current code:

const [trackingIdState] = useState(trackingId); // save initial trackingId before login/registration

This approach is correct for preserving the anonymous tracking ID, but ensure that:

  1. The trackingId from useAuthContext is indeed the anonymous ID before authentication
  2. After login, a different tracking ID is used (or the same one is not overwritten)

If this assumption is violated, you might claim opportunities for the wrong user.

3. Type Safety

Severity: Low

In types.ts:327-329, the OpportunitiesClaim type is well-defined:

export type OpportunitiesClaim = {
  ids: string[];
};

However, the mutation in RecruiterSignInModal.tsx doesn't use the returned ids array. If you plan to use this data in the future (e.g., to show which opportunities were claimed), consider storing it in state or passing it to the onSuccess callback.

4. GraphQL Mutation Naming Consistency

Severity: Low / Informational

The mutation is named CLAIM_OPPORTUNITIES_MUTATION (line 783 in graphql.ts), which is consistent with other mutations in the file. The mutation returns ids: string[], but it's unclear from the mutation definition what these IDs represent:

  • IDs of opportunities that were successfully claimed?
  • IDs of opportunities that already existed?
  • All opportunity IDs associated with the identifier?

Consider adding a JSDoc comment to clarify the return value's meaning.

Positive Observations

  1. Good error handling pattern: The silent catch with comment explaining the rationale is appropriate for this use case
  2. Proper state preservation: Using useState to capture the initial trackingId is the right approach
  3. Separation of concerns: Registration and login have separate success handlers, making the code clear
  4. Type safety: Proper TypeScript types throughout
  5. Consistent patterns: Follows existing mutation patterns in the codebase

Testing Recommendations

Since there's no test file included, here are key scenarios to manually test:

  1. Happy path: Anon user views opportunity → logs in → opportunities are claimed
  2. No tracking ID: User logs in without a trackingId (should skip claiming)
  3. Claim fails: API returns error (should continue flow normally)
  4. Network timeout: Request times out (should not hang the modal)
  5. New user registration: Ensure claiming only happens on login, not registration (correct per current implementation)

Minor Observations

  • Line 30: The comment is helpful - consider also adding a comment explaining why we only claim on login, not registration (since registration already associates the user with their anonymous actions implicitly)

Conclusion

The implementation is clean and follows good practices. The main recommendation is to add error logging for the claim operation to aid debugging. The rest are minor suggestions that can be addressed in future iterations if needed.


Branch: anon-login-claim-opportunity

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.

3 participants