Skip to content

refactor: use nullish coalescing and simplify checks in events#709

Open
kamilwronka wants to merge 1 commit intodevelopfrom
refactor/nullish-coalescing-events-20260408
Open

refactor: use nullish coalescing and simplify checks in events#709
kamilwronka wants to merge 1 commit intodevelopfrom
refactor/nullish-coalescing-events-20260408

Conversation

@kamilwronka
Copy link
Copy Markdown
Contributor

@kamilwronka kamilwronka commented Apr 8, 2026

Summary

  • Replace || with ?? for nullish coalescing in event-kill.service.ts and event-wrapped.service.ts — prevents incorrect fallback when values are 0 or empty string
  • Simplify redundant === null || === undefined checks in event-points.service.tsNumber.isFinite() already returns false for null and undefined

Why each change is safe

  • ||??: Map.get() and optional chaining return undefined (not other falsy values) on miss, so ?? is semantically correct. The key fix is countMapStats() in event-wrapped.service.ts — it can return 0 (valid), which || would incorrectly treat as falsy.
  • Simplified Number.isFinite guards: Number.isFinite(null) and Number.isFinite(undefined) both return false, making the explicit null/undefined checks redundant.

Files touched

  • apps/api/src/events/services/event-kill.service.ts — 6 ||?? replacements
  • apps/api/src/events/services/event-points.service.ts — 2 simplified conditions
  • apps/api/src/events/services/event-wrapped.service.ts — 1 ||?? replacement

Residual risks

  • None. Changes are behavior-preserving for non-zero/non-empty values, and fix a subtle correctness issue for zero values.

Test plan

  • All pre-commit hooks passed (lint + format + tests)
  • 52 test files, 599 tests passed

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved accuracy of event tracking data by correcting how zero values and empty strings are handled in statistics calculations and event metadata serialization.

Replace || with ?? for nullish coalescing in event-kill and event-wrapped
services to prevent incorrect fallback when values are 0 or empty string.
Simplify redundant null/undefined checks in event-points where
Number.isFinite already handles null and undefined.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

📝 Walkthrough

Walkthrough

Three event service files updated to use nullish-coalescing operators (??) instead of logical OR (||) for better handling of optional values, and one file refactored to consolidate null/undefined validation logic using Number.isFinite().

Changes

Cohort / File(s) Summary
Nullish-coalescing refactoring
apps/api/src/events/services/event-kill.service.ts, apps/api/src/events/services/event-wrapped.service.ts
Replace logical OR with nullish-coalescing operators for mapName, unassignedAt serialization, and time-based field defaults to correctly preserve falsy values like 0 and empty strings.
Validation logic consolidation
apps/api/src/events/services/event-points.service.ts
Simplify multi-branch null/undefined checks in getTrackingDurationSecondsForRanking() and calculateTrackingDurationPercentage() using single Number.isFinite() guard.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

Nullish coalescing hops through the code,
Where zero and empty now safely show,
The rabbit refactors with finesse and grace,
Replacing old logic with its proper place! 🐰
Valid values preserved, falsy no more.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: replacing logical OR with nullish coalescing and simplifying conditional checks across event service files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/nullish-coalescing-events-20260408

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
apps/api/src/events/services/event-wrapped.service.ts (1)

701-703: Remove unreachable fallback in mapCount expression.

countMapStats() already guarantees a numeric result, so the fallback chain after it never executes. Simplifying this improves readability.

♻️ Suggested simplification
-      const mapCount =
-        countMapStats(summary.mapStats) ?? heroCoverage?.mapCount ?? 0;
+      const mapCount = countMapStats(summary.mapStats);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/events/services/event-wrapped.service.ts` around lines 701 -
703, The assigned mapCount uses an unreachable fallback because
countMapStats(summary.mapStats) always returns a number; replace the
ternary/fallback chain with a direct assignment. Change the mapCount declaration
to call countMapStats(summary.mapStats) only (remove the ??
heroCoverage?.mapCount ?? 0 parts) so the code reads: const mapCount =
countMapStats(summary.mapStats); and remove any now-unnecessary references to
heroCoverage?.mapCount in that expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/api/src/events/services/event-wrapped.service.ts`:
- Around line 701-703: The assigned mapCount uses an unreachable fallback
because countMapStats(summary.mapStats) always returns a number; replace the
ternary/fallback chain with a direct assignment. Change the mapCount declaration
to call countMapStats(summary.mapStats) only (remove the ??
heroCoverage?.mapCount ?? 0 parts) so the code reads: const mapCount =
countMapStats(summary.mapStats); and remove any now-unnecessary references to
heroCoverage?.mapCount in that expression.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a6095f9a-41cf-479c-a5d5-70375eddabd5

📥 Commits

Reviewing files that changed from the base of the PR and between f99033e and 94f9277.

📒 Files selected for processing (3)
  • apps/api/src/events/services/event-kill.service.ts
  • apps/api/src/events/services/event-points.service.ts
  • apps/api/src/events/services/event-wrapped.service.ts

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