refactor: extract cutoff date helper and remove unused discordId param#700
refactor: extract cutoff date helper and remove unused discordId param#700kamilwronka wants to merge 1 commit intodevelopfrom
Conversation
- Extract createCutoffDate() in ReservationsCleanupService to match the existing pattern in TimersCleanupService, removing duplication - Remove unused discordId parameter from getComments() in loot module (was passed through but never used in the query) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR removes the Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/reservations/reservations-cleanup.service.ts`:
- Around line 58-60: Add defensive validation of the retentionDays argument
inside createCutoffDate so callers like
cleanupExpiredReservationsManual(retentionDays) and the analogous
cleanupExpiredTimersManual validate inputs: ensure retentionDays is a finite
number and non-negative (e.g., Number.isFinite(retentionDays) && retentionDays
>= 0); if not, throw a clear RangeError (or similar) explaining the invalid
retentionDays value instead of silently computing a future cutoff. Apply the
same guard to the TimersCleanupService variant
(cleanupExpiredTimersManual/createCutoffDate there) so
exported/manual-invocation paths cannot expand the delete window with negative
or non-finite inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a6e3aa8-5f07-45a7-817d-e0edf494d113
📒 Files selected for processing (6)
apps/api/src/loots/loots.controller.spec.tsapps/api/src/loots/loots.controller.tsapps/api/src/loots/loots.service.spec.tsapps/api/src/loots/loots.service.tsapps/api/src/loots/services/loot-comment.service.tsapps/api/src/reservations/reservations-cleanup.service.ts
💤 Files with no reviewable changes (2)
- apps/api/src/loots/loots.service.spec.ts
- apps/api/src/loots/loots.controller.ts
| async cleanupExpiredReservationsManual( | ||
| retentionDays: number = this.retentionDays, | ||
| ): Promise<number> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether upstream validation already enforces non-negative integer retentionDays.
# 1) Find callers of cleanupExpiredReservationsManual
rg -nP --type=ts -C3 '\bcleanupExpiredReservationsManual\s*\('
# 2) Find DTOs/schemas that may validate retentionDays
rg -nP --type=ts -C3 '\bretentionDays\b|\bIsInt\b|\bMin\s*\('Repository: lootlog/monorepo
Length of output: 9059
🏁 Script executed:
# Find how environment variables are validated
rg -nP --type=ts 'RESERVATIONS_RETENTION_DAYS|TIMER_RETENTION_DAYS' -B5 -A5
# Find where these manual cleanup methods are called
rg -nP --type=ts '\bcleanupExpiredReservationsManual\b|\bcleanupExpiredTimersManual\b' -B3 -A3
# Search for any validation schemas or DTOs
fd -type f -name '*.ts' | xargs rg -l 'retentionDays' | head -20Repository: lootlog/monorepo
Length of output: 11086
🏁 Script executed:
# Read the complete env.ts to see full validation
cat -n apps/api/src/config/env.ts
# Search for controllers that might expose these cleanup methods
rg -nP --type=ts 'Controller|@Controller|@Post|@Get' -B2 -A8 | rg -i 'cleanup|retention' -B5 -A5
# Find any API routes or endpoints
fd -type f -name '*.controller.ts' | xargs rg -l 'cleanupExpired|retention'Repository: lootlog/monorepo
Length of output: 5727
🏁 Script executed:
# Read TimersController
cat -n apps/api/src/timers/timers.controller.ts
# Read ReservationsController
cat -n apps/api/src/reservations/reservations.controller.tsRepository: lootlog/monorepo
Length of output: 14988
🏁 Script executed:
# Search for any endpoints or handlers that call the cleanup methods
rg -nP 'cleanupExpiredTimersManual|cleanupExpiredReservationsManual' -B5 -A5
# Check if ReservationsCleanupService is injected anywhere else
rg -nP 'ReservationsCleanupService' -B2 -A2
# Check if TimersCleanupService is injected anywhere else
rg -nP 'TimersCleanupService' -B2 -A2Repository: lootlog/monorepo
Length of output: 10725
Add defensive validation for retentionDays in createCutoffDate().
While cleanupExpiredReservationsManual() and cleanupExpiredTimersManual() are not currently exposed via API, the methods are exported from their modules and accept optional caller-provided retentionDays values. Negative or non-finite values would move the cutoff date into the future, unintentionally expanding the delete scope. The environment validation (z.coerce.number()) has no bounds constraints. Add a guard to reject invalid values:
private createCutoffDate(retentionDays: number): Date {
+ if (!Number.isFinite(retentionDays) || retentionDays < 0 || !Number.isInteger(retentionDays)) {
+ throw new Error("retentionDays must be a non-negative integer");
+ }
const cutoffDate = new Date();
cutoffDate.setDate(cutoffDate.getDate() - retentionDays);
return cutoffDate;
}Also applies to: TimersCleanupService (lines 79-84)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/reservations/reservations-cleanup.service.ts` around lines 58 -
60, Add defensive validation of the retentionDays argument inside
createCutoffDate so callers like cleanupExpiredReservationsManual(retentionDays)
and the analogous cleanupExpiredTimersManual validate inputs: ensure
retentionDays is a finite number and non-negative (e.g.,
Number.isFinite(retentionDays) && retentionDays >= 0); if not, throw a clear
RangeError (or similar) explaining the invalid retentionDays value instead of
silently computing a future cutoff. Apply the same guard to the
TimersCleanupService variant (cleanupExpiredTimersManual/createCutoffDate there)
so exported/manual-invocation paths cannot expand the delete window with
negative or non-finite inputs.
1 participant
Summary
createCutoffDate()inReservationsCleanupService— the cutoff date calculation was duplicated inline in two methods. Now uses a private helper, matching the identical pattern already inTimersCleanupService.discordIdparameter fromgetComments()— the parameter was passed through the controller → service → comment service chain but never used in the actual database query. Removed from the type signature, call sites, and tests.Touched files
apps/api/src/reservations/reservations-cleanup.service.tsapps/api/src/loots/services/loot-comment.service.tsapps/api/src/loots/loots.service.tsapps/api/src/loots/loots.controller.tsapps/api/src/loots/loots.controller.spec.tsapps/api/src/loots/loots.service.spec.tsSafety
@DiscordId()decorator removal doesn't affect HTTP contract)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit