Skip to content

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Dec 4, 2025
@claude
Copy link

claude bot commented Dec 4, 2025

Code Review - PR #728

🔴 Critical Issues

Potential N+1 query problem with eager loading
In src/entities/game-channel.ts:42, you changed the game relationship to eager loading:

@ManyToOne(() => Game, { eager: true })
game: Game

However, the subscriber at src/subscribers/game-channel.subscriber.ts:19 accesses channel.game without checking if it's loaded:

await deferClearResponseCache(GameChannel.getSearchCacheKey(channel.game, true))

Issue: When a GameChannelProp is being deleted with cascade, the prop's reference to gameChannel may exist but the channel's reference to game might not be eagerly loaded in all lifecycle hook contexts, especially during cascading deletes. This could cause the cache key to be generated with an uninitialized Game object or fail silently.

Fix: Either ensure the game is loaded before accessing it in the subscriber, or handle the case where channel.game might not be initialized:

async clearSearchCacheKey(entity: GameChannel | GameChannelProp) {
  const channel = entity instanceof GameChannel ? entity : entity.gameChannel
  
  if (\!channel || \!channel.game) {
    return
  }
  
  await deferClearResponseCache(GameChannel.getSearchCacheKey(channel.game, true))
}

🟡 Major Issues

Inconsistent void handling in subscriber
In src/subscribers/game-channel.subscriber.ts:22-31, you use void to fire-and-forget async calls:

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

But the method is declared as async and the pattern is inconsistent with similar code in leaderboard-entry.subscriber.ts:23 which doesn't use async in the method signature and just uses void.

Fix: Remove async keyword from clearSearchCacheKey for consistency:

clearSearchCacheKey(entity: GameChannel | GameChannelProp) {
  const channel = entity instanceof GameChannel ? entity : entity.gameChannel
  if (\!channel || \!channel.game) return
  void deferClearResponseCache(GameChannel.getSearchCacheKey(channel.game, true))
}

🔵 Minor Issues

Missing cache invalidation for member changes
The subscriber only tracks GameChannel and GameChannelProp changes, but the search query in game-channel.service.ts filters by member count (totalMessages) and potentially filters channels. When members are added/removed (which affects the query results), the cache isn't invalidated. This could lead to stale member counts in cached responses.

Consider whether member changes should also invalidate the cache, or document why they don't need to.


Test Coverage

No issues found - test coverage is comprehensive.

@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.20%. Comparing base (1897407) to head (c336772).
⚠️ Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #728   +/-   ##
========================================
  Coverage    99.20%   99.20%           
========================================
  Files          223      224    +1     
  Lines        16475    16514   +39     
  Branches      1507     1509    +2     
========================================
+ Hits         16344    16383   +39     
  Misses         129      129           
  Partials         2        2           

☔ 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 merged commit 4abe37b into develop Dec 4, 2025
8 of 9 checks passed
@tudddorrr tudddorrr deleted the bring-back-channel-index-cache branch December 4, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants