Skip to content

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the bug Something isn't working label Nov 30, 2025
@claude
Copy link

claude bot commented Nov 30, 2025

Code Review - PR #702

🟡 Code Quality & Best Practices

Inconsistent error handling in subscriber hooks

The subscriber methods (lines 27-37 in game-channel.subscriber.ts) use void to suppress the promise, but this silently swallows any errors from clearSearchCacheKey(). This differs from the existing LeaderboardEntrySubscriber pattern which calls deferClearResponseCache directly.

Current code:

afterCreate(args: EventArgs<GameChannel | GameChannelProp>) {
  void this.clearSearchCacheKey(args)
}

The clearSearchCacheKey method is async and could throw errors (e.g., if channel.game fails to populate). These errors would be silently ignored, potentially leaving stale cache entries.

Suggested fix: Either call deferClearResponseCache directly (matching the LeaderboardEntrySubscriber pattern), or ensure errors are logged:

afterCreate(args: EventArgs<GameChannel | GameChannelProp>) {
  void this.clearSearchCacheKey(args).catch((err) => {
    // Log error via Sentry or logger
    console.error('Failed to clear channel cache:', err)
  })
}

✅ Other Categories

Potential bugs: No issues found

Performance: No issues found - the subscriber pattern is efficient and the game population check is appropriate

Security: No issues found

Test coverage: Excellent - comprehensive tests cover create, update, delete, props changes, and cache isolation by game


Overall: The PR successfully refactors cache invalidation from manual service calls to automatic subscriber hooks. The implementation is clean and well-tested. The error handling concern is minor but worth addressing for production reliability.

@codecov
Copy link

codecov bot commented Nov 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.21%. Comparing base (5571071) to head (8a66b82).
⚠️ Report is 9 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #702   +/-   ##
========================================
  Coverage    99.21%   99.21%           
========================================
  Files          225      226    +1     
  Lines        16608    16635   +27     
  Branches      1538     1543    +5     
========================================
+ Hits         16477    16504   +27     
  Misses         128      128           
  Partials         3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tudddorrr tudddorrr force-pushed the game-channel-subscriber branch 4 times, most recently from fb5fc29 to 957c111 Compare December 1, 2025 01:09
@tudddorrr tudddorrr force-pushed the game-channel-subscriber branch 2 times, most recently from 8cd7081 to 7a23b21 Compare December 1, 2025 08:44
@tudddorrr tudddorrr force-pushed the game-channel-subscriber branch from 7a23b21 to 8a66b82 Compare December 1, 2025 17:16
@tudddorrr tudddorrr merged commit ef584ab into develop Dec 1, 2025
6 checks passed
@tudddorrr tudddorrr deleted the game-channel-subscriber branch December 1, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants