feat: scheduled releases, recommendations cache, reports audit trail, and pagination - #532
Conversation
…At enforcement Implement exponential backoff retry strategy as a dedicated policy object. Update the cron query to respect nextRetryAt windows, preventing retries before their scheduled backoff period elapses. This ensures failed releases honor the configured retry delay before reattemption. Refs Tip-tune-org#499
Add RecommendationCacheService to cache user recommendation snapshots with 1-hour TTL. Invalidate cache on new tips or feedback to prevent stale recommendations. Cache hits reduce query overhead; cold-start users fall back to live calculation. Refs Tip-tune-org#496
Separate enforcement actions from report resolution via ReportEnforcementService. Create audit logs for all user bans and content removals, tracking previous and new state. Support enforcement reversal through defined service methods. Makes all actions auditable and traceable back to originating reports. Refs Tip-tune-org#498
Define ReportQueryDto with class-validator validation for typed filter fields, pagination metadata, and strict query semantics. Controller validates and transforms raw queries. Service returns paginated results with total count. Reject invalid query parameters explicitly. Refs Tip-tune-org#497
Fix missing imports in stellar.service (Injectable, Logger), incorrect property name in tips.service (fromUser not fromUserId), deprecated enum usage in licensing services (LicenseRequestStatus to LicensingLifecycle), and unsupported withDeleted() method in track services. These pre-existing errors were blocking CI and preventing the build from succeeding.
|
@buinntalen is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR introduces three new services for caching recommendations, enforcing report-driven actions with audit trails, and implementing configurable retry policies for scheduled releases. It adds pagination and validation to report queries, updates several existing service implementations to integrate the new services, and makes minor fixes to licensing, tips, and track handling logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/scheduled-releases/scheduled-releases.service.ts (1)
251-304:⚠️ Potential issue | 🟡 Minor
retryCountis not persisted on the terminal failure.When the final failure occurs,
handleReleaseFailurecomputesretryCount = release.retryCount + 1but jumps straight tomarkAsPermanentlyFailed(release)without persisting that increment. As a result:
- The
scheduled_releases.retryCountcolumn ends one short of actual attempts.- The DLQ entry at line 294 records the stale
release.retryCount, while thelastErrorstring at line 293/284 reportsretryPolicy.maxRetriesattempts. These two fields will disagree on permanent-failure rows that took the full retry budget.Pass the up-to-date count through, or persist it before the terminal transition:
♻️ Proposed fix
private async handleReleaseFailure( release: ScheduledRelease, error: Error, ): Promise<void> { const retryCount = release.retryCount + 1; const nextRetryTime = this.retryPolicy.getNextRetryTime(retryCount); if (retryCount < this.retryPolicy.maxRetries) { // Schedule for retry await this.scheduledReleaseRepository.update(release.id, { retryCount, lastError: error.message, lastAttemptAt: new Date(), status: ReleaseStatus.PENDING, nextRetryAt: nextRetryTime, }); this.logger.log( `Release ${release.id} scheduled for retry #${retryCount} at ${nextRetryTime.toISOString()}`, ); } else { - await this.markAsPermanentlyFailed(release); + release.retryCount = retryCount; + release.lastError = error.message; + release.lastAttemptAt = new Date(); + await this.markAsPermanentlyFailed(release); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/scheduled-releases/scheduled-releases.service.ts` around lines 251 - 304, The terminal-failure path in handleReleaseFailure computes retryCount = release.retryCount + 1 but never persists or passes that increment into markAsPermanentlyFailed, so DB and DLQ keep the stale count; fix by either updating the record with retryCount before calling markAsPermanentlyFailed or changing markAsPermanentlyFailed to accept a retryCount param and use it when calling scheduledReleaseRepository.update and dlqService.createEntry (ensure lastError/failure message still references retryPolicy.maxRetries but retryCount field uses the incremented value). Target symbols: handleReleaseFailure, markAsPermanentlyFailed, scheduledReleaseRepository.update, dlqService.createEntry, retryCount, retryPolicy.maxRetries.backend/src/reports/reports.service.ts (1)
100-114:⚠️ Potential issue | 🟠 MajorEnforcement and report-save are not atomic.
applyEnforcementperforms persistent writes (user/track update + audit log) and then the report itself is saved on line 114. IfreportsRepository.save(report)fails (validation, conn drop, optimistic-lock), the user is already banned / track already hidden and an audit row exists, but the originatingReportkeeps its oldstatus/action— leaving operators an inconsistent state to clean up by hand.Wrap the enforcement + report save in a single TypeORM transaction (
DataSource.transactionor aQueryRunner), or move enforcement to fire after a successful report save and accept that the audit log is what proves the side-effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/reports/reports.service.ts` around lines 100 - 114, The enforcement call (enforcementService.applyEnforcement) and the subsequent reportsRepository.save(report) must be executed atomically to avoid inconsistent state; wrap the resolvedAt assignment, reportsRepository.save(report) and enforcementService.applyEnforcement(...) inside a single TypeORM transaction (use this.dataSource.transaction or a QueryRunner) so both the report update and the enforcement writes/audit commit or roll back together. Update enforcementService.applyEnforcement to accept an optional EntityManager/QueryRunner (or use repositories from the transaction manager) and call it with that manager inside the transaction block; alternatively obtain repositories from the transaction manager to save the report (instead of this.reportsRepository) so all DB writes use the same transactional context. Ensure you still set report.resolvedAt when updateDto.status is RESOLVED/DISMISSED and preserve checks for updateDto.action !== ReportAction.NONE inside the transaction.
🧹 Nitpick comments (8)
backend/src/track-listening-right-management/licensing-mail.service.ts (1)
40-56: Consider matchingREJECTEDexplicitly instead of treating it as the default branch.The ternaries on lines 42 and 48 now collapse every non-
APPROVEDstatus into a "rejected" label/subject. With the lifecycle enum,request.statuscan also beWITHDRAWN,EXPIRED,REOPENED, orPENDING. Today the "response" mail is only enqueued fromrespondToRequestafter a transition toAPPROVED/REJECTED, but the mail handler reloads the request by id when it dequeues, so a request that was subsequently withdrawn/expired/reopened before the worker runs will be emailed as "Rejected ❌". MatchingLicensingLifecycle.REJECTEDexplicitly (and skipping/erroring on other states) would make the notification accurate and forward-compatible with the broader lifecycle.♻️ Suggested change
async notifyRequesterOfResponse(request: LicenseRequest): Promise<void> { - const statusLabel = - request.status === LicensingLifecycle.APPROVED - ? "approved ✅" - : "rejected ❌"; + let statusLabel: string; + let subjectStatus: string; + if (request.status === LicensingLifecycle.APPROVED) { + statusLabel = "approved ✅"; + subjectStatus = "Approved"; + } else if (request.status === LicensingLifecycle.REJECTED) { + statusLabel = "rejected ❌"; + subjectStatus = "Rejected"; + } else { + this.logger.warn( + `Skipping response email for request ${request.id} in non-response state '${request.status}'`, + ); + return; + } await this.sendMail({ to: `user+${request.requesterId}@platform.local`, - subject: `Your License Request Has Been ${request.status === LicensingLifecycle.APPROVED ? "Approved" : "Rejected"}`, + subject: `Your License Request Has Been ${subjectStatus}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/track-listening-right-management/licensing-mail.service.ts` around lines 40 - 56, The current notifyRequesterOfResponse function treats any non-APPROVED status as "rejected", which mislabels states like WITHDRAWN/EXPIRED/PENDING; update notifyRequesterOfResponse to check request.status explicitly against LicensingLifecycle.APPROVED and LicensingLifecycle.REJECTED (use those two branches to set statusLabel and subject) and handle any other states by either skipping sending the email or throwing/logging an error so sendMail is not invoked for unexpected lifecycle values; make the change in the notifyRequesterOfResponse method and ensure any returned/early-exit behavior is clear to the caller.backend/src/track-listening-right-management/licensing.service.ts (1)
11-29: LGTM — lifecycle enum migration is consistent with the entity.Switching the duplicate-pending lookup (line 107) and the new request's initial
status(line 129) toLicensingLifecycle.PENDINGmatchesLicenseRequest.statusbeing typed/columned asLicensingLifecyclewith defaultPENDING(perlicense-request.entity.ts), so this is correct.One small style nit: the
export { LicenseRequestStatus } …re-export on line 19 is sandwiched between twoimportgroups. Most lint configs and readers prefer all imports first, then re-exports. Moving it below the import block would keep the top of the file uniform without changing behavior.♻️ Optional reorganization
import { LicensingLifecycle, WITHDRAWABLE_STATES, REOPENABLE_STATES, RESPONDABLE_STATES, } from "./licensing-lifecycle.enum"; - -/** `@deprecated` kept for callers that still reference the old enum */ -export { LicenseRequestStatus } from "./license-request.entity"; import { CreateTrackLicenseDto, CreateLicenseRequestDto, RespondToLicenseRequestDto, } from "./licensing.dto"; import { LicensingMailService } from "./licensing-mail.service"; import { LicensingDeliveryQueue } from "./licensing-delivery.queue"; import { NotificationsService } from "@/notifications/notifications.service"; import { Track } from "@/tracks/entities/track.entity"; import { NotificationType } from "@/notifications/notification.entity"; + +/** `@deprecated` kept for callers that still reference the old enum */ +export { LicenseRequestStatus } from "./license-request.entity";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/track-listening-right-management/licensing.service.ts` around lines 11 - 29, Move the re-export "export { LicenseRequestStatus } from \"./license-request.entity\";" out of the middle of the import group and place it after the import block (i.e., after the last import such as NotificationType) so all imports remain grouped together at the top and the lone re-export follows them; update licensing.service.ts by relocating the LicenseRequestStatus re-export to just below the import statements.backend/src/scheduled-releases/scheduled-release-retry.policy.ts (2)
12-20: DefaultbackoffStrategyshould be'exponential'.Issue
#499calls for replacing fixed-delay semantics with exponential backoff. A'fixed'default is the opposite of the policy's stated purpose; any future consumer that relies on the default will silently get fixed delay. The service inscheduled-releases.service.ts(line 25) does explicitly pass'exponential', but the default itself should be the safe, intent-aligned choice.♻️ Proposed fix
constructor( maxRetries: number = 3, baseDelayMs: number = 5000, - backoffStrategy: 'exponential' | 'fixed' = 'fixed', + backoffStrategy: 'exponential' | 'fixed' = 'exponential', ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/scheduled-releases/scheduled-release-retry.policy.ts` around lines 12 - 20, Change the default backoffStrategy in the constructor to 'exponential' so the class aligns with the intended exponential-backoff policy; update the constructor signature that currently sets backoffStrategy: 'exponential' | 'fixed' = 'fixed' to use = 'exponential', leaving maxRetries and baseDelayMs defaults unchanged and ensuring any consumers (e.g., scheduled-releases.service.ts) that rely on the default now get exponential behavior.
22-45: DisambiguateattemptNumbersemantics across methods.
shouldRetry(attemptNumber, ...)treats the argument as "past-attempt count" (so0= first try, blocks when>= maxRetries), whilegetDelayMs(attemptNumber)/getNextRetryTime(attemptNumber)treat it as a 1-indexed attempt number (formulabaseDelayMs * 2^(attemptNumber - 1)). The caller inscheduled-releases.service.tsalready compensates by passingrelease.retryCountto one andrelease.retryCount + 1to the other (lines 214 and 256) — but the shared parameter name guarantees future drift. Also,getDelayMs(0)would silently returnbaseDelayMs / 2for exponential.Consider renaming/aligning so the contract is unambiguous, and clamp the exponent to be defensive:
♻️ Proposed fix
export interface RetryPolicy { maxRetries: number; - getDelayMs(attemptNumber: number): number; - shouldRetry(attemptNumber: number, nextRetryAt: Date | null): boolean; + /** Delay before the next attempt, given how many attempts have already been made. */ + getDelayMs(pastAttempts: number): number; + /** Whether another attempt is allowed, given how many attempts have already been made. */ + shouldRetry(pastAttempts: number, nextRetryAt: Date | null): boolean; } @@ - getDelayMs(attemptNumber: number): number { - if (this.backoffStrategy === 'exponential') { - return this.baseDelayMs * Math.pow(2, attemptNumber - 1); - } - return this.baseDelayMs; - } + getDelayMs(pastAttempts: number): number { + const safeExponent = Math.max(0, pastAttempts); + if (this.backoffStrategy === 'exponential') { + return this.baseDelayMs * Math.pow(2, safeExponent); + } + return this.baseDelayMs; + } @@ - shouldRetry(attemptNumber: number, nextRetryAt: Date | null): boolean { - if (attemptNumber >= this.maxRetries) { + shouldRetry(pastAttempts: number, nextRetryAt: Date | null): boolean { + if (pastAttempts >= this.maxRetries) { return false; } @@ - getNextRetryTime(attemptNumber: number): Date { - const delayMs = this.getDelayMs(attemptNumber); + getNextRetryTime(pastAttempts: number): Date { + const delayMs = this.getDelayMs(pastAttempts); return new Date(Date.now() + delayMs); }Caller would then consistently pass
release.retryCount(the count of past failures) to all three methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/scheduled-releases/scheduled-release-retry.policy.ts` around lines 22 - 45, The three methods have inconsistent semantics for "attemptNumber"; rename the parameter to pastAttempts on getDelayMs, shouldRetry, and getNextRetryTime so all expect the count of prior failures (0 = first try), update callers to pass release.retryCount everywhere, and change the exponential formula to use Math.pow(2, Math.max(0, pastAttempts)) (i.e., delay = baseDelayMs * 2^pastAttempts) to avoid fractional delays and defensively clamp negative inputs. Ensure shouldRetry still compares pastAttempts against maxRetries (e.g., if pastAttempts >= maxRetries return false) and getNextRetryTime uses the new getDelayMs(pastAttempts) result.backend/src/scheduled-releases/scheduled-releases.service.ts (2)
25-25: Inject the retry policy instead of hardcoding it.Hardcoding
new ScheduledReleaseRetryPolicy(3, 5000, 'exponential')makes retry behavior non-configurable per environment and harder to mock in unit tests (issue#499explicitly calls out testable retry behavior). Provide it via the Nest DI container — using a token + factory pulling fromConfigService— so production, staging, and tests can each tunemaxRetries/baseDelayMs/backoffStrategywithout code changes.♻️ Sketch
- private readonly retryPolicy = new ScheduledReleaseRetryPolicy(3, 5000, 'exponential'); + // injected via constructor below @@ constructor( `@InjectRepository`(ScheduledRelease) private scheduledReleaseRepository: Repository<ScheduledRelease>, ... private readonly dlqService: DlqService, + `@Inject`(SCHEDULED_RELEASE_RETRY_POLICY) + private readonly retryPolicy: RetryPolicy, ) {}In the module:
{ provide: SCHEDULED_RELEASE_RETRY_POLICY, inject: [ConfigService], useFactory: (cfg: ConfigService) => new ScheduledReleaseRetryPolicy( cfg.get<number>('SCHEDULED_RELEASE_MAX_RETRIES', 3), cfg.get<number>('SCHEDULED_RELEASE_BASE_DELAY_MS', 5000), cfg.get<'exponential' | 'fixed'>('SCHEDULED_RELEASE_BACKOFF', 'exponential'), ), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/scheduled-releases/scheduled-releases.service.ts` at line 25, Replace the hardcoded instantiation of ScheduledReleaseRetryPolicy in ScheduledReleasesService with a DI-injected instance: remove the "new ScheduledReleaseRetryPolicy(3, 5000, 'exponential')" assignment and add a constructor parameter that injects the token SCHEDULED_RELEASE_RETRY_POLICY (e.g., `@Inject`(SCHEDULED_RELEASE_RETRY_POLICY) private readonly retryPolicy: ScheduledReleaseRetryPolicy) so the service uses the injected policy; also add a provider in the module that supplies SCHEDULED_RELEASE_RETRY_POLICY via a factory using ConfigService to read SCHEDULED_RELEASE_MAX_RETRIES, SCHEDULED_RELEASE_BASE_DELAY_MS, and SCHEDULED_RELEASE_BACKOFF and construct the ScheduledReleaseRetryPolicy, enabling environment-driven and testable retry configuration.
161-176:nextRetryAtgating LGTM; flag latent stuck-state and concurrency risk.The new filter
(sr.nextRetryAt IS NULL OR sr.nextRetryAt <= :now)correctly satisfies#499's gating requirement — new releases (nullable column, default NULL) match on the first run, and failed releases are excluded until their backoff window elapses.Two pre-existing concerns surface, both worth tracking even if out-of-scope here:
- Stuck
PUBLISHINGrecords: the cron only excludesFAILED_PERMANENTLY. If a worker crashes mid-publish, the row staysstatus=PUBLISHINGand the idempotency check at lines 204-209 will silently skip it forever. Consider either (a) excludingPUBLISHINGhere AND providing a sweeper that resets stalePUBLISHINGrows past a TTL, or (b) routing all picks through the existingScheduledReleaseLockServicewhoseclaimReleasealready encodes lease expiry.- Multi-instance duplicate processing:
processReleaseWithRetryreads then mutates without an atomic claim, so two pods running this cron will race. The repo already hasScheduledReleaseLockService.claimRelease(...)(atomic conditional UPDATE) — wiring it in here would make this scheduler exactly-once safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/scheduled-releases/scheduled-releases.service.ts` around lines 161 - 176, The current query correctly gates on nextRetryAt but can leave rows stuck in status=PUBLISHING and allows duplicate processing across instances; update the selection and processing flow: (1) add an exclusion for ReleaseStatus.PUBLISHING in the createQueryBuilder filter (in addition to FAILED_PERMANENTLY) to avoid permanently skipping crashed publishes, and (2) before calling processReleaseWithRetry, atomically claim each candidate using ScheduledReleaseLockService.claimRelease(...) (which does a conditional UPDATE/lease) and only process releases that were successfully claimed; alternatively implement a TTL-based sweeper to reset stale PUBLISHING rows if you cannot claim atomically. Ensure references to nextRetryAt, ReleaseStatus.PUBLISHING, ScheduledReleaseLockService.claimRelease, and processReleaseWithRetry are used so the changes are applied in the right locations.backend/src/reports/reports.controller.ts (1)
28-35: Surface validation errors and preferValidationPipe.Two related concerns:
- The generic
BadRequestException('Invalid query parameters')discards every per-field error fromclass-validator. Callers can't tell which parameter is wrong (e.g.,limit=200vs.status=foo), which hurts both API consumers and debugging.- NestJS already provides
ValidationPipeto doplainToInstance+validatewithtransform: true/whitelist: true, returning a structured 400 with constraint messages. Re-implementing it manually here is non-idiomatic and easy to drift from the global pipe configuration (if any).♻️ Proposed refactor
-import { Controller, Get, Post, Body, Patch, Param, UseGuards, Query, BadRequestException } from '@nestjs/common'; +import { Controller, Get, Post, Body, Patch, Param, UseGuards, Query, ValidationPipe } from '@nestjs/common'; import { ReportsService } from './reports.service'; import { CreateReportDto } from './dto/create-report.dto'; import { UpdateReportStatusDto } from './dto/update-report-status.dto'; import { AssignReportDto } from './dto/assign-report.dto'; import { ReportQueryDto } from './dto/report-query.dto'; ... -import { plainToInstance } from 'class-transformer'; -import { validate } from 'class-validator'; @@ `@Get`() `@UseGuards`(RolesGuard) `@Roles`(UserRole.ADMIN) - async findAll(`@Query`() query: any) { - const dto = plainToInstance(ReportQueryDto, query); - const errors = await validate(dto); - if (errors.length > 0) { - throw new BadRequestException('Invalid query parameters'); - } - return this.reportsService.findAll(dto); + findAll( + `@Query`(new ValidationPipe({ transform: true, whitelist: true, forbidNonWhitelisted: true })) + query: ReportQueryDto, + ) { + return this.reportsService.findAll(query); }If a global
ValidationPipewithtransform: trueis already enabled inmain.ts, simply typing@Query() query: ReportQueryDtois enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/reports/reports.controller.ts` around lines 28 - 35, The findAll method is swallowing per-field validation errors by manually calling plainToInstance + validate and throwing a generic BadRequestException; remove the manual plainToInstance/validate logic in findAll (and the generic BadRequestException) and instead accept a typed DTO parameter so Nest's ValidationPipe can run (change signature to `@Query`() query: ReportQueryDto) or, if you don't have a global pipe, annotate the handler with `@UsePipes`(new ValidationPipe({ transform: true, whitelist: true })) to ensure class-validator errors are returned verbatim; keep the call to this.reportsService.findAll(dto) but pass the validated/transformed ReportQueryDto.backend/src/reports/reports.service.ts (1)
26-27: UnusedauditLogRepositoryinjection.
auditLogRepositoryis injected but never referenced in this service — allAdminAuditLogwrites happen insideReportEnforcementService. Drop the injection (and the corresponding import on line 11) to keep the DI graph honest.♻️ Proposed cleanup
-import { AdminAuditLog } from '../admin/entities/admin-audit-log.entity'; @@ `@InjectRepository`(Track) private tracksRepository: Repository<Track>, - `@InjectRepository`(AdminAuditLog) - private auditLogRepository: Repository<AdminAuditLog>, private enforcementService: ReportEnforcementService,Note: if you drop this, you can also remove
AdminAuditLogfromTypeOrmModule.forFeature(...)inreports.module.tsonly ifReportEnforcementService's registration of it is sufficient (it is, since they share the same module).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/reports/reports.service.ts` around lines 26 - 27, Remove the unused injection of AdminAuditLog: delete the private auditLogRepository: Repository<AdminAuditLog> field and its `@InjectRepository`(AdminAuditLog) decorator in reports.service.ts and remove the AdminAuditLog import at the top of that file; then, if ReportEnforcementService already supplies AdminAuditLog persistence in the same module, also remove AdminAuditLog from TypeOrmModule.forFeature(...) in reports.module.ts to avoid registering an unused repository. Ensure no other references to auditLogRepository remain before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/recommendations/recommendation-cache.service.ts`:
- Around line 11-15: The in-memory Map in RecommendationCacheService is unsafe
for multi-instance deployments and unbounded; replace the private cache: Map
with a shared cache-manager (inject CACHE_MANAGER as cacheManager: Cache) backed
by Redis (cache-manager-redis-yet) and use cacheManager.get/set/del with TTLs so
recordFeedback, invalidateOnTipEvent, and invalidateOnFeedbackEvent call
cacheManager.del for the user keys (ensuring cluster-wide invalidation); if you
must keep in-memory for single-instance, swap the Map for an LRU implementation
(e.g., lru-cache) and enforce a max size and TTL plus a comment documenting
single-instance limitations.
- Around line 109-117: The method invalidateOnTipEvent in
recommendation-cache.service.ts is never invoked because it lacks an
`@OnEvent`('tip.verified') decorator and RecommendationsModule does not import
EventEmitterModule; remove the unused invalidateOnFeedbackEvent (since
recordFeedback already calls invalidateUserCache) and implement tip verification
invalidation by either (A) adding `@OnEvent`('tip.verified') to
invalidateOnTipEvent and importing EventEmitterModule into RecommendationsModule
so the event is handled, or (B) removing invalidateOnTipEvent and calling
recommendationCacheService.invalidateUserCache(userId) directly from the tip
verification path in tips.service.ts where tip.verified is emitted; update
unit/integration tests accordingly.
- Around line 91-107: The recordFeedback flow (recordFeedback in
recommendation-cache.service.ts using feedbackRepo and RecommendationFeedback
entity) is racy because it does findOne → create → save without a DB uniqueness
constraint on (userId, trackId); add a unique index/constraint on the
RecommendationFeedback entity for (userId, trackId) and create a migration to
apply it, then replace the findOne/create/save sequence with an atomic upsert
(use feedbackRepo.upsert or repository save with ON CONFLICT/ON DUPLICATE KEY
semantics) to insert-or-update feedback atomically, keeping the
invalidateUserCache(userId) call after the upsert so cache invalidation still
occurs.
In `@backend/src/recommendations/recommendations.service.ts`:
- Around line 34-54: Cache entries must be keyed/structured so varying caller
limits don't produce undersized hits; implement the "cache max and slice on
read" approach: when reading in getTrackRecommendations(userId, limit) compute
boundedLimit = Math.max(1, Math.min(limit, 50)), call
cacheService.getTrackRecommendations(userId) but treat it as a raw cached array
and only treat it as a hit if cached && cached.length >= boundedLimit (return
cached.slice(0, boundedLimit)); when computing recommendations, always generate
and cache up to the maximum bucket (use 50) by calling
cacheService.setTrackRecommendations(userId, recommendations.slice(0, 50)) so
future reads can safely slice down; update cacheService.getTrackRecommendations
to return the raw stored array (no slicing) and ensure setTrackRecommendations
stores up to 50 items.
In `@backend/src/reports/dto/report-query.dto.ts`:
- Around line 18-19: The assignedToId field currently only has `@IsOptional`() so
any value (including arrays) passes; update the ReportQueryDto's assignedToId
declaration to enforce a UUID string by adding the class-validator decorator
`@IsUUID`() (keep `@IsOptional`()), and ensure the TypeScript type remains string
(assignedToId?: string) so arrays are rejected by validation and only a proper
User.id-shaped UUID is accepted.
- Around line 21-22: The escalated query param is arriving as a string so update
the ReportQueryDto's escalated property to coerce the string to a real boolean
using class-transformer's `@Transform` (e.g. `@Transform`(({ value }) => value ===
'true')), keep `@IsOptional`(), ensure the property type remains boolean
(escalated?: boolean) and import Transform from 'class-transformer' so
where.escalated receives a proper boolean instead of the string.
In `@backend/src/reports/report-enforcement.service.ts`:
- Around line 48-92: The users/tracks update and the subsequent createAuditLog
must be executed in a single DB transaction so both succeed or both rollback:
change the enforcement flows in applyEnforcement (the block that calls
usersRepository.update or tracksRepository.update and then this.createAuditLog)
to run inside DataSource.transaction (or create a QueryRunner) and use the
transactional EntityManager to perform the update and to save the audit entry
(pass the manager to/create a transactional variant of createAuditLog), and
apply the same pattern in reverseEnforcement so the reversal update and its
audit log use the same transaction/QueryRunner.
- Around line 95-150: The reverseEnforcement method currently doesn't record
which enforcement or report it's reversing and uses a hardcoded reason; update
the reverseEnforcement(enforcement: EnforcementAction, admin: User, ipAddress:
string) signature to accept a reason: string and an optional originalReportId?:
string (or originalAuditLogId?: string), propagate these into the createAuditLog
call (add fields originalReportId/originalAuditLogId and use the provided reason
instead of the hardcoded string), and ensure the created audit log action still
uses `ENFORCEMENT_REVERSED_${enforcement.type}` but now includes the original
identifier in the audit payload so queries can link reversal→original (update
calls in both USER_BANNED and CONTENT_REMOVED branches and the surrounding
logging to include the reason and original id).
- Around line 101-118: In reverseEnforcement, don’t rely on the stale
enforcement.targetUser snapshot to compute previousState/newStatus; re-fetch the
current user from the DB (via usersRepository.findOne/findById or the repository
method used elsewhere) using enforcement.entityId, set previousState based on
that fresh record’s status, compute newStatus from that current status (toggle
BANNED → ACTIVE otherwise keep), then call usersRepository.update and
createAuditLog with previousState and newState; alternatively, if a
previousState was captured at apply-time, use that stored previousState instead
of enforcement.targetUser.
- Around line 49-68: The audit log is being created even when no enforcement
occurred (e.g., mismatched action/entity or missing row). Modify the enforcement
branches (the ReportAction.USER_BANNED + ReportEntityType.USER path that uses
usersRepository.findOne/update and the ReportAction.CONTENT_REMOVED +
ReportEntityType.TRACK path that uses tracksRepository.findOne/update) to set a
boolean flag (e.g., enforcementApplied = true) when an update actually happens
(after usersRepository.update or tracksRepository.update and after setting
previousState/newState), and then before calling createAuditLog(...) return
early or skip creating the audit entry if enforcementApplied is false (or
previousState/newState are still unset). Ensure createAuditLog is only invoked
when an actual change was applied.
In `@backend/src/reports/reports.service.ts`:
- Around line 104-112: The report update currently passes a hardcoded ipAddress
('0.0.0.0') into enforcementService.applyEnforcement which breaks auditing;
change the wiring so the real client IP is threaded from the controller into
ReportsService.updateStatus (or the method handling the update) and then
forwarded into enforcementService.applyEnforcement. Concretely: accept an ip
(e.g., requestIp: string) in the controller (use `@Ip`() or request.ip), add an ip
parameter to ReportsService.updateStatus (or the specific update method) and to
EnforcementService.applyEnforcement, update all call sites to pass the real IP
through, and ensure AdminAuditLog.ipAddress is populated with that value instead
of the hardcoded '0.0.0.0'.
In `@backend/src/tracks/track-file-reaper.service.ts`:
- Around line 91-93: The current query uses .where('id = :id', { id: track.id
}).orWhere('id = :id AND "deletedAt" IS NOT NULL', { id: track.id }) which
collapses to id = :id; change the second clause to an and condition so the
update only affects soft-deleted rows. Replace the orWhere call with an andWhere
(e.g. .andWhere('"deletedAt" IS NOT NULL') or .andWhere('id = :id AND
"deletedAt" IS NOT NULL', { id: track.id })) in the TrackFileReaperService query
so the update is actually constrained to rows where "deletedAt" IS NOT NULL and
avoid duplicate/conflicting parameter bindings.
In `@backend/src/tracks/tracks.service.ts`:
- Around line 240-242: The current QueryBuilder uses .where('id = :id', { id
}).orWhere('id = :id AND "deletedAt" IS NOT NULL', { id }) which collapses to
just id = :id; change this to enforce the soft-delete guard by combining the
predicates (e.g., replace the chain with a single .where('id = :id AND
"deletedAt" IS NOT NULL', { id }) or use .where('id = :id', { id
}).andWhere('"deletedAt" IS NOT NULL') so the update in TracksService targets
only soft-deleted rows.
---
Outside diff comments:
In `@backend/src/reports/reports.service.ts`:
- Around line 100-114: The enforcement call
(enforcementService.applyEnforcement) and the subsequent
reportsRepository.save(report) must be executed atomically to avoid inconsistent
state; wrap the resolvedAt assignment, reportsRepository.save(report) and
enforcementService.applyEnforcement(...) inside a single TypeORM transaction
(use this.dataSource.transaction or a QueryRunner) so both the report update and
the enforcement writes/audit commit or roll back together. Update
enforcementService.applyEnforcement to accept an optional
EntityManager/QueryRunner (or use repositories from the transaction manager) and
call it with that manager inside the transaction block; alternatively obtain
repositories from the transaction manager to save the report (instead of
this.reportsRepository) so all DB writes use the same transactional context.
Ensure you still set report.resolvedAt when updateDto.status is
RESOLVED/DISMISSED and preserve checks for updateDto.action !==
ReportAction.NONE inside the transaction.
In `@backend/src/scheduled-releases/scheduled-releases.service.ts`:
- Around line 251-304: The terminal-failure path in handleReleaseFailure
computes retryCount = release.retryCount + 1 but never persists or passes that
increment into markAsPermanentlyFailed, so DB and DLQ keep the stale count; fix
by either updating the record with retryCount before calling
markAsPermanentlyFailed or changing markAsPermanentlyFailed to accept a
retryCount param and use it when calling scheduledReleaseRepository.update and
dlqService.createEntry (ensure lastError/failure message still references
retryPolicy.maxRetries but retryCount field uses the incremented value). Target
symbols: handleReleaseFailure, markAsPermanentlyFailed,
scheduledReleaseRepository.update, dlqService.createEntry, retryCount,
retryPolicy.maxRetries.
---
Nitpick comments:
In `@backend/src/reports/reports.controller.ts`:
- Around line 28-35: The findAll method is swallowing per-field validation
errors by manually calling plainToInstance + validate and throwing a generic
BadRequestException; remove the manual plainToInstance/validate logic in findAll
(and the generic BadRequestException) and instead accept a typed DTO parameter
so Nest's ValidationPipe can run (change signature to `@Query`() query:
ReportQueryDto) or, if you don't have a global pipe, annotate the handler with
`@UsePipes`(new ValidationPipe({ transform: true, whitelist: true })) to ensure
class-validator errors are returned verbatim; keep the call to
this.reportsService.findAll(dto) but pass the validated/transformed
ReportQueryDto.
In `@backend/src/reports/reports.service.ts`:
- Around line 26-27: Remove the unused injection of AdminAuditLog: delete the
private auditLogRepository: Repository<AdminAuditLog> field and its
`@InjectRepository`(AdminAuditLog) decorator in reports.service.ts and remove the
AdminAuditLog import at the top of that file; then, if ReportEnforcementService
already supplies AdminAuditLog persistence in the same module, also remove
AdminAuditLog from TypeOrmModule.forFeature(...) in reports.module.ts to avoid
registering an unused repository. Ensure no other references to
auditLogRepository remain before committing.
In `@backend/src/scheduled-releases/scheduled-release-retry.policy.ts`:
- Around line 12-20: Change the default backoffStrategy in the constructor to
'exponential' so the class aligns with the intended exponential-backoff policy;
update the constructor signature that currently sets backoffStrategy:
'exponential' | 'fixed' = 'fixed' to use = 'exponential', leaving maxRetries and
baseDelayMs defaults unchanged and ensuring any consumers (e.g.,
scheduled-releases.service.ts) that rely on the default now get exponential
behavior.
- Around line 22-45: The three methods have inconsistent semantics for
"attemptNumber"; rename the parameter to pastAttempts on getDelayMs,
shouldRetry, and getNextRetryTime so all expect the count of prior failures (0 =
first try), update callers to pass release.retryCount everywhere, and change the
exponential formula to use Math.pow(2, Math.max(0, pastAttempts)) (i.e., delay =
baseDelayMs * 2^pastAttempts) to avoid fractional delays and defensively clamp
negative inputs. Ensure shouldRetry still compares pastAttempts against
maxRetries (e.g., if pastAttempts >= maxRetries return false) and
getNextRetryTime uses the new getDelayMs(pastAttempts) result.
In `@backend/src/scheduled-releases/scheduled-releases.service.ts`:
- Line 25: Replace the hardcoded instantiation of ScheduledReleaseRetryPolicy in
ScheduledReleasesService with a DI-injected instance: remove the "new
ScheduledReleaseRetryPolicy(3, 5000, 'exponential')" assignment and add a
constructor parameter that injects the token SCHEDULED_RELEASE_RETRY_POLICY
(e.g., `@Inject`(SCHEDULED_RELEASE_RETRY_POLICY) private readonly retryPolicy:
ScheduledReleaseRetryPolicy) so the service uses the injected policy; also add a
provider in the module that supplies SCHEDULED_RELEASE_RETRY_POLICY via a
factory using ConfigService to read SCHEDULED_RELEASE_MAX_RETRIES,
SCHEDULED_RELEASE_BASE_DELAY_MS, and SCHEDULED_RELEASE_BACKOFF and construct the
ScheduledReleaseRetryPolicy, enabling environment-driven and testable retry
configuration.
- Around line 161-176: The current query correctly gates on nextRetryAt but can
leave rows stuck in status=PUBLISHING and allows duplicate processing across
instances; update the selection and processing flow: (1) add an exclusion for
ReleaseStatus.PUBLISHING in the createQueryBuilder filter (in addition to
FAILED_PERMANENTLY) to avoid permanently skipping crashed publishes, and (2)
before calling processReleaseWithRetry, atomically claim each candidate using
ScheduledReleaseLockService.claimRelease(...) (which does a conditional
UPDATE/lease) and only process releases that were successfully claimed;
alternatively implement a TTL-based sweeper to reset stale PUBLISHING rows if
you cannot claim atomically. Ensure references to nextRetryAt,
ReleaseStatus.PUBLISHING, ScheduledReleaseLockService.claimRelease, and
processReleaseWithRetry are used so the changes are applied in the right
locations.
In `@backend/src/track-listening-right-management/licensing-mail.service.ts`:
- Around line 40-56: The current notifyRequesterOfResponse function treats any
non-APPROVED status as "rejected", which mislabels states like
WITHDRAWN/EXPIRED/PENDING; update notifyRequesterOfResponse to check
request.status explicitly against LicensingLifecycle.APPROVED and
LicensingLifecycle.REJECTED (use those two branches to set statusLabel and
subject) and handle any other states by either skipping sending the email or
throwing/logging an error so sendMail is not invoked for unexpected lifecycle
values; make the change in the notifyRequesterOfResponse method and ensure any
returned/early-exit behavior is clear to the caller.
In `@backend/src/track-listening-right-management/licensing.service.ts`:
- Around line 11-29: Move the re-export "export { LicenseRequestStatus } from
\"./license-request.entity\";" out of the middle of the import group and place
it after the import block (i.e., after the last import such as NotificationType)
so all imports remain grouped together at the top and the lone re-export follows
them; update licensing.service.ts by relocating the LicenseRequestStatus
re-export to just below the import statements.
🪄 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: df1b6ed3-d205-4071-a84a-536c387738d3
📒 Files selected for processing (16)
backend/src/recommendations/recommendation-cache.service.tsbackend/src/recommendations/recommendations.module.tsbackend/src/recommendations/recommendations.service.tsbackend/src/reports/dto/report-query.dto.tsbackend/src/reports/report-enforcement.service.tsbackend/src/reports/reports.controller.tsbackend/src/reports/reports.module.tsbackend/src/reports/reports.service.tsbackend/src/scheduled-releases/scheduled-release-retry.policy.tsbackend/src/scheduled-releases/scheduled-releases.service.tsbackend/src/stellar/stellar.service.tsbackend/src/tips/tips.service.tsbackend/src/track-listening-right-management/licensing-mail.service.tsbackend/src/track-listening-right-management/licensing.service.tsbackend/src/tracks/track-file-reaper.service.tsbackend/src/tracks/tracks.service.ts
| @Injectable() | ||
| export class RecommendationCacheService { | ||
| private readonly logger = new Logger(RecommendationCacheService.name); | ||
| private readonly cacheTtlMs = 3600000; | ||
| private cache = new Map<string, CacheEntry>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the recommended NestJS 10 approach (2025) for a per-user TTL recommendation cache that needs to be consistent across multiple instances behind a load balancer?
💡 Result:
The recommended NestJS 10 approach (as of 2025) for a per-user TTL recommendation cache consistent across multiple instances behind a load balancer is to use the official @nestjs/cache-manager module configured with a Redis store. 1. Install dependencies: npm install @nestjs/cache-manager cache-manager cache-manager-redis-yet 2. Register CacheModule globally with Redis in AppModule (or a dedicated CacheModule): import { Module } from '@nestjs/common'; import { CacheModule } from '@nestjs/cache-manager'; import { redisStore } from 'cache-manager-redis-yet'; import { ConfigService } from '@nestjs/config'; @Module({ imports: [ CacheModule.registerAsync({ isGlobal: true, useFactory: async (configService: ConfigService) => { const store = await redisStore({ socket: { host: configService.get('REDIS_HOST', 'localhost'), port: configService.get('REDIS_PORT', 6379), }, ttl: 0, // No default TTL; set per-operation }); return { store }; }, inject: [ConfigService], }), ], }) export class AppModule {} This uses Redis as the shared store, ensuring consistency across instances. In-memory won't work for multi-instance setups. 3. Inject CACHE_MANAGER and use per-user keys with dynamic TTL: import { Injectable, Inject } from '@nestjs/common'; import { CACHE_MANAGER } from '@nestjs/cache-manager'; import { Cache } from 'cache-manager'; @Injectable export class RecommendationService { constructor(@Inject(CACHE_MANAGER) private cacheManager: Cache) {} async getRecommendations(userId: string, ttlMs: number) { const key = recommendations:user:${userId}; let recs = await this.cacheManager.get(key); if (!recs) { recs = await this.computeRecommendations(userId); // Expensive computation // Set with dynamic TTL (ms); use { ttl: ttlMs / 1000 } for seconds if needed based on store version await this.cacheManager.set(key, recs, ttlMs); } return recs; } } Key prefixing with user:${userId} ensures per-user isolation. TTL is set dynamically per set call. For TypeScript issues with { ttl }, cast as any or use number directly (check cache-manager version compatibility). For advanced features (L1/L2 cache, stampede protection, tags), consider NestJS RedisX (@nestjs-redisx/cache), but official cache-manager + Redis is the standard baseline per docs.
Citations:
- 1: https://docs.nestjs.com/techniques/caching
- 2: https://medium.com/@prashanta0234/nest-js-redis-ttl-issue-redis-in-nest-js-0d8ca73926b8
- 3: https://oneuptime.com/blog/post/2026-02-02-nestjs-caching/view
- 4: https://oneuptime.com/blog/post/2026-03-31-redis-nestjs-cache-module/view
- 5: https://www.codingeasypeasy.com/blog/implementing-caching-in-nestjs-with-nestjscache-manager-and-redis-a-comprehensive-guide
- 6: https://oneuptime.com/blog/post/2026-02-02-nestjs-redis/view
- 7: https://medium.com/@npavfan2facts/caching-in-nestjs-where-it-stops-being-truth-a0bb32642e1c
- 8: https://stackoverflow.com/questions/74573177/in-nestjs-i-added-redis-as-cache-manager-when-i-specify-ttl-0-it-throws-a
- 9: https://stackoverflow.com/questions/79109438/unable-to-set-dynamic-ttl-for-redis-cache-in-nestjs
- 10: https://nestjs-redisx.dev/en/
- 11: https://github.com/nestjs-redisx/nestjs-redisx
- 12: https://nestjs-redisx.dev/en/reference/cache/service-api
🏁 Script executed:
cat backend/src/recommendations/recommendation-cache.service.tsRepository: OlufunbiIK/tip-tune
Length of output: 3604
In-memory Map cache won't survive multi-instance deployments and is unbounded.
Two operational concerns with the current implementation:
- Per-instance state. Behind a load balancer with N replicas, each instance has its own cache. Calls to
recordFeedback(line 92) only invalidate on the instance that handled the request, so other replicas continue serving stale recommendations until TTL expiry. The same applies to event-based invalidation (invalidateOnTipEvent,invalidateOnFeedbackEvent) unless events are broadcast cluster-wide. - Unbounded growth. Entries are only evicted lazily when that user is read again after expiry (lines 39–40, 67–68). Users who request once and never come back leave entries in
cacheforever. With no max-size or periodic sweep, this becomes a slow memory leak proportional to unique-user count.
Recommended approach:
- Back the cache with a shared store using Redis via
@nestjs/cache-managerconfigured withcache-manager-redis-yet. This is the standard NestJS 10 (2025) pattern for multi-instance consistent caching and handles TTL, invalidation, and unbounded growth automatically. - If staying in-memory for single-instance scenarios, cap entries with an LRU eviction policy (e.g.,
lru-cache) and document the single-instance requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/recommendations/recommendation-cache.service.ts` around lines 11
- 15, The in-memory Map in RecommendationCacheService is unsafe for
multi-instance deployments and unbounded; replace the private cache: Map with a
shared cache-manager (inject CACHE_MANAGER as cacheManager: Cache) backed by
Redis (cache-manager-redis-yet) and use cacheManager.get/set/del with TTLs so
recordFeedback, invalidateOnTipEvent, and invalidateOnFeedbackEvent call
cacheManager.del for the user keys (ensuring cluster-wide invalidation); if you
must keep in-memory for single-instance, swap the Map for an LRU implementation
(e.g., lru-cache) and enforce a max size and TTL plus a comment documenting
single-instance limitations.
| async recordFeedback( | ||
| userId: string, | ||
| trackId: string, | ||
| feedback: 'up' | 'down', | ||
| ): Promise<RecommendationFeedback> { | ||
| const existing = await this.feedbackRepo.findOne({ | ||
| where: { userId, trackId }, | ||
| }); | ||
|
|
||
| const entry = existing || this.feedbackRepo.create({ userId, trackId }); | ||
| entry.feedback = feedback; | ||
| const saved = await this.feedbackRepo.save(entry); | ||
|
|
||
| await this.invalidateUserCache(userId); | ||
|
|
||
| return saved; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there is no existing unique constraint / unique index on (userId, trackId)
# in the entity or migrations that would already prevent duplicates.
fd -t f 'recommendation-feedback*' | xargs -I{} sh -c 'echo "===== {} ====="; cat {}'
echo "----- migrations referencing recommendation_feedback -----"
rg -nP --type=ts -C2 'recommendation_feedback|RecommendationFeedback' -g '!**/node_modules/**'Repository: OlufunbiIK/tip-tune
Length of output: 7280
Race condition in recordFeedback allows duplicate feedback rows due to missing unique constraint.
The findOne → conditional create → save pattern is vulnerable to concurrent execution. When two requests simultaneously check for existing feedback on the same (userId, trackId) and both observe null, each will insert a new row. The entity has no unique constraint on (userId, trackId), so duplicates persist. This breaks non-determinism in downstream filtering (e.g., WHERE feedback = 'down' in collaborativeFilter and contentBasedFilter).
Fix by enforcing uniqueness at the database level and using upsert:
🛡️ Proposed fix using TypeORM upsert + unique constraint
In the entity:
-@Entity('recommendation_feedback')
+@Entity('recommendation_feedback')
+@Unique(['userId', 'trackId'])
export class RecommendationFeedback {In recommendation-cache.service.ts:
- const existing = await this.feedbackRepo.findOne({
- where: { userId, trackId },
- });
-
- const entry = existing || this.feedbackRepo.create({ userId, trackId });
- entry.feedback = feedback;
- const saved = await this.feedbackRepo.save(entry);
-
- await this.invalidateUserCache(userId);
-
- return saved;
+ await this.feedbackRepo.upsert(
+ { userId, trackId, feedback },
+ { conflictPaths: ['userId', 'trackId'] },
+ );
+ await this.invalidateUserCache(userId);
+ const saved = await this.feedbackRepo.findOneOrFail({
+ where: { userId, trackId },
+ });
+ return saved;A migration adding the unique index is also needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/recommendations/recommendation-cache.service.ts` around lines 91
- 107, The recordFeedback flow (recordFeedback in
recommendation-cache.service.ts using feedbackRepo and RecommendationFeedback
entity) is racy because it does findOne → create → save without a DB uniqueness
constraint on (userId, trackId); add a unique index/constraint on the
RecommendationFeedback entity for (userId, trackId) and create a migration to
apply it, then replace the findOne/create/save sequence with an atomic upsert
(use feedbackRepo.upsert or repository save with ON CONFLICT/ON DUPLICATE KEY
semantics) to insert-or-update feedback atomically, keeping the
invalidateUserCache(userId) call after the upsert so cache invalidation still
occurs.
| async invalidateOnTipEvent(userId: string): Promise<void> { | ||
| await this.invalidateUserCache(userId); | ||
| this.logger.debug(`Invalidated cache due to tip event for user ${userId}`); | ||
| } | ||
|
|
||
| async invalidateOnFeedbackEvent(userId: string): Promise<void> { | ||
| await this.invalidateUserCache(userId); | ||
| this.logger.debug(`Invalidated cache due to feedback event for user ${userId}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Are there any callers of these invalidation hooks anywhere?
rg -nP --type=ts -C2 '\binvalidateOn(Tip|Feedback)Event\s*\('
# 2) Is EventEmitterModule wired in app/recommendations modules?
rg -nP --type=ts -C2 'EventEmitterModule|@OnEvent'
# 3) Does tip verification emit any event we could subscribe to?
rg -nP --type=ts -C3 '\beventEmitter\.emit\b|\bEventEmitter2\b' -g '!**/node_modules/**'Repository: OlufunbiIK/tip-tune
Length of output: 43268
🏁 Script executed:
# Check if RecommendationsModule imports EventEmitterModule
cat -n backend/src/recommendations/recommendations.module.ts
# Search for feedback-related event handling
rg -nP --type=ts 'recordFeedback|feedback\..*event|recommendation\.feedback' -C2
# Confirm no callers of the invalidation methods
rg -nP --type=ts 'invalidateOnTipEvent|invalidateOnFeedbackEvent' | grep -v 'async invalidateOn'Repository: OlufunbiIK/tip-tune
Length of output: 3073
🏁 Script executed:
# Read the recordFeedback method to see if it calls invalidateOnFeedbackEvent
sed -n '91,117p' backend/src/recommendations/recommendation-cache.service.ts
# Search for any feedback event emission
rg -nP --type=ts 'emit.*feedback|eventEmitter.*feedback' -C2Repository: OlufunbiIK/tip-tune
Length of output: 917
Event-driven cache invalidation for tip verification is not implemented — PR #496 requirement not met.
invalidateOnTipEvent is a dead method with no callers. While tip.verified events are emitted from tips.service.ts (line 204–207), RecommendationsModule does not import EventEmitterModule, and the method lacks an @OnEvent('tip.verified') decorator, so tip verification never invalidates the recommendations cache.
The feedback case is already handled directly: recordFeedback calls invalidateUserCache synchronously (line 104), so invalidateOnFeedbackEvent can be removed. However, tip verification invalidation must be added—either:
- Decorate
invalidateOnTipEventwith@OnEvent('tip.verified')and importEventEmitterModuleintoRecommendationsModule, or - Remove the event-based method and call
cacheService.invalidateOnTipEvent(userId)directly from the tip verification path intips.service.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/recommendations/recommendation-cache.service.ts` around lines 109
- 117, The method invalidateOnTipEvent in recommendation-cache.service.ts is
never invoked because it lacks an `@OnEvent`('tip.verified') decorator and
RecommendationsModule does not import EventEmitterModule; remove the unused
invalidateOnFeedbackEvent (since recordFeedback already calls
invalidateUserCache) and implement tip verification invalidation by either (A)
adding `@OnEvent`('tip.verified') to invalidateOnTipEvent and importing
EventEmitterModule into RecommendationsModule so the event is handled, or (B)
removing invalidateOnTipEvent and calling
recommendationCacheService.invalidateUserCache(userId) directly from the tip
verification path in tips.service.ts where tip.verified is emitted; update
unit/integration tests accordingly.
| const boundedLimit = Math.max(1, Math.min(limit, 50)); | ||
|
|
||
| const cached = await this.cacheService.getTrackRecommendations(userId, boundedLimit); | ||
| if (cached) { | ||
| return cached; | ||
| } | ||
|
|
||
| const tipCount = await this.getUserTipCount(userId); | ||
|
|
||
| let recommendations: any[]; | ||
| if (tipCount < 3) { | ||
| return this.getPopularTracks(boundedLimit); | ||
| recommendations = await this.getPopularTracks(boundedLimit); | ||
| } else { | ||
| const collaborative = await this.collaborativeFilter(userId, boundedLimit); | ||
| const contentBased = await this.contentBasedFilter(userId, boundedLimit); | ||
| recommendations = this.mergeRecommendations(collaborative, contentBased, boundedLimit); | ||
| } | ||
|
|
||
| const collaborative = await this.collaborativeFilter(userId, boundedLimit); | ||
| const contentBased = await this.contentBasedFilter(userId, boundedLimit); | ||
|
|
||
| return this.mergeRecommendations(collaborative, contentBased, boundedLimit); | ||
| await this.cacheService.setTrackRecommendations(userId, recommendations); | ||
| return recommendations; | ||
| } |
There was a problem hiding this comment.
Cache can return fewer items than requested when limit varies between calls.
setTrackRecommendations(userId, recommendations) stores exactly boundedLimit items keyed only on userId, and getTrackRecommendations retrieves that array and slice(0, limit)s it. Sequence:
- Caller A requests
limit=20→ cache stores 20 items. - Caller B requests
limit=50(within the 1h TTL) → cache hit returns only 20 items — silently fewer than asked.
Downstream impact: getArtistRecommendations calls getTrackRecommendations(userId, 30) (Line 62), so any prior cached run with limit < 30 will degrade artist recommendations too.
Two ways to fix — pick one:
- Include
limit(or a small set of canonical buckets) in the cache key so different limits don't collide. - Always compute and cache the max bound (50), then slice on read; only treat entries with
length >= limitas a hit.
♻️ Sketch of the second option (cache max, slice on read)
In recommendations.service.ts:
- const cached = await this.cacheService.getTrackRecommendations(userId, boundedLimit);
- if (cached) {
- return cached;
+ const cached = await this.cacheService.getTrackRecommendations(userId, boundedLimit);
+ if (cached && cached.length >= boundedLimit) {
+ return cached.slice(0, boundedLimit);
}
@@
- let recommendations: any[];
+ const MAX_LIMIT = 50;
+ let recommendations: any[];
if (tipCount < 3) {
- recommendations = await this.getPopularTracks(boundedLimit);
+ recommendations = await this.getPopularTracks(MAX_LIMIT);
} else {
- const collaborative = await this.collaborativeFilter(userId, boundedLimit);
- const contentBased = await this.contentBasedFilter(userId, boundedLimit);
- recommendations = this.mergeRecommendations(collaborative, contentBased, boundedLimit);
+ const collaborative = await this.collaborativeFilter(userId, MAX_LIMIT);
+ const contentBased = await this.contentBasedFilter(userId, MAX_LIMIT);
+ recommendations = this.mergeRecommendations(collaborative, contentBased, MAX_LIMIT);
}
await this.cacheService.setTrackRecommendations(userId, recommendations);
- return recommendations;
+ return recommendations.slice(0, boundedLimit);And update the cache getTrackRecommendations to no longer slice (returning the raw cached array).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/recommendations/recommendations.service.ts` around lines 34 - 54,
Cache entries must be keyed/structured so varying caller limits don't produce
undersized hits; implement the "cache max and slice on read" approach: when
reading in getTrackRecommendations(userId, limit) compute boundedLimit =
Math.max(1, Math.min(limit, 50)), call
cacheService.getTrackRecommendations(userId) but treat it as a raw cached array
and only treat it as a hit if cached && cached.length >= boundedLimit (return
cached.slice(0, boundedLimit)); when computing recommendations, always generate
and cache up to the maximum bucket (use 50) by calling
cacheService.setTrackRecommendations(userId, recommendations.slice(0, 50)) so
future reads can safely slice down; update cacheService.getTrackRecommendations
to return the raw stored array (no slicing) and ensure setTrackRecommendations
stores up to 50 items.
| @IsOptional() | ||
| assignedToId?: string; |
There was a problem hiding this comment.
assignedToId has no actual validation.
@IsOptional() alone runs no constraint when the value is present, so ?assignedToId=... accepts anything — including arrays (?assignedToId=a&assignedToId=b), which Express parses into string[] and which TypeORM will silently turn into an IN filter. Constrain it to a UUID string to match the User.id shape.
🛡️ Proposed fix
`@IsOptional`()
+ `@IsUUID`()
assignedToId?: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @IsOptional() | |
| assignedToId?: string; | |
| `@IsOptional`() | |
| `@IsUUID`() | |
| assignedToId?: string; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/reports/dto/report-query.dto.ts` around lines 18 - 19, The
assignedToId field currently only has `@IsOptional`() so any value (including
arrays) passes; update the ReportQueryDto's assignedToId declaration to enforce
a UUID string by adding the class-validator decorator `@IsUUID`() (keep
`@IsOptional`()), and ensure the TypeScript type remains string (assignedToId?:
string) so arrays are rejected by validation and only a proper User.id-shaped
UUID is accepted.
| async reverseEnforcement( | ||
| enforcement: EnforcementAction, | ||
| admin: User, | ||
| ipAddress: string, | ||
| ): Promise<void> { | ||
| try { | ||
| if (enforcement.type === ReportAction.USER_BANNED && enforcement.targetUser) { | ||
| const previousState = { status: enforcement.targetUser.status }; | ||
| const newStatus = enforcement.targetUser.status === UserStatus.BANNED ? UserStatus.ACTIVE : enforcement.targetUser.status; | ||
| await this.usersRepository.update(enforcement.entityId, { | ||
| status: newStatus, | ||
| }); | ||
|
|
||
| await this.createAuditLog({ | ||
| adminId: admin.id, | ||
| admin, | ||
| action: `ENFORCEMENT_REVERSED_${enforcement.type}`, | ||
| entityType: enforcement.entityType, | ||
| entityId: enforcement.entityId, | ||
| previousState, | ||
| newState: { status: newStatus }, | ||
| reason: 'Enforcement reversal requested', | ||
| ipAddress, | ||
| }); | ||
| } else if ( | ||
| enforcement.type === ReportAction.CONTENT_REMOVED && | ||
| enforcement.targetTrack | ||
| ) { | ||
| const previousState = { isPublic: enforcement.targetTrack.isPublic }; | ||
| await this.tracksRepository.update(enforcement.entityId, { | ||
| isPublic: true, | ||
| }); | ||
|
|
||
| await this.createAuditLog({ | ||
| adminId: admin.id, | ||
| admin, | ||
| action: `ENFORCEMENT_REVERSED_${enforcement.type}`, | ||
| entityType: enforcement.entityType, | ||
| entityId: enforcement.entityId, | ||
| previousState, | ||
| newState: { isPublic: true }, | ||
| reason: 'Enforcement reversal requested', | ||
| ipAddress, | ||
| }); | ||
| } | ||
|
|
||
| this.logger.log( | ||
| `Reversed enforcement action ${enforcement.type} for entity ${enforcement.entityId}`, | ||
| ); | ||
| } catch (error) { | ||
| this.logger.error( | ||
| `Failed to reverse enforcement: ${error.message}`, | ||
| ); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reversal isn't linked to the original enforcement and has a hardcoded reason.
Two related gaps that weaken the audit trail required by #498:
- The reversal audit row encodes
ENFORCEMENT_REVERSED_<type>inactionbut stores no reference to the originating audit log id or the originatingReport. Querying "which reversal undid which enforcement?" requires fuzzy matching onentityId+ timestamps. reasonis hardcoded to'Enforcement reversal requested'— operators can't record why the reversal was performed.
Consider accepting a reason: string parameter (and ideally an originalReportId / originalAuditLogId) and persisting them in the audit log.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/reports/report-enforcement.service.ts` around lines 95 - 150, The
reverseEnforcement method currently doesn't record which enforcement or report
it's reversing and uses a hardcoded reason; update the
reverseEnforcement(enforcement: EnforcementAction, admin: User, ipAddress:
string) signature to accept a reason: string and an optional originalReportId?:
string (or originalAuditLogId?: string), propagate these into the createAuditLog
call (add fields originalReportId/originalAuditLogId and use the provided reason
instead of the hardcoded string), and ensure the created audit log action still
uses `ENFORCEMENT_REVERSED_${enforcement.type}` but now includes the original
identifier in the audit payload so queries can link reversal→original (update
calls in both USER_BANNED and CONTENT_REMOVED branches and the surrounding
logging to include the reason and original id).
| if (enforcement.type === ReportAction.USER_BANNED && enforcement.targetUser) { | ||
| const previousState = { status: enforcement.targetUser.status }; | ||
| const newStatus = enforcement.targetUser.status === UserStatus.BANNED ? UserStatus.ACTIVE : enforcement.targetUser.status; | ||
| await this.usersRepository.update(enforcement.entityId, { | ||
| status: newStatus, | ||
| }); | ||
|
|
||
| await this.createAuditLog({ | ||
| adminId: admin.id, | ||
| admin, | ||
| action: `ENFORCEMENT_REVERSED_${enforcement.type}`, | ||
| entityType: enforcement.entityType, | ||
| entityId: enforcement.entityId, | ||
| previousState, | ||
| newState: { status: newStatus }, | ||
| reason: 'Enforcement reversal requested', | ||
| ipAddress, | ||
| }); |
There was a problem hiding this comment.
reverseEnforcement reasons over a stale targetUser snapshot.
enforcement.targetUser is captured before the ban update (line 54), so its .status reflects the pre-ban state. The check enforcement.targetUser.status === UserStatus.BANNED ? UserStatus.ACTIVE : enforcement.targetUser.status is therefore effectively dead in the BANNED branch, and the "previousState" written to the reversal audit log records the pre-ban status, not the current DB state. If anything has changed in between (re-ban, manual admin intervention), reversal silently drifts.
Re-fetch the user from the DB and decide based on its current state, or restore from a stored previousState captured at apply-time.
🛡️ Suggested fix
- if (enforcement.type === ReportAction.USER_BANNED && enforcement.targetUser) {
- const previousState = { status: enforcement.targetUser.status };
- const newStatus = enforcement.targetUser.status === UserStatus.BANNED ? UserStatus.ACTIVE : enforcement.targetUser.status;
- await this.usersRepository.update(enforcement.entityId, {
- status: newStatus,
- });
+ if (enforcement.type === ReportAction.USER_BANNED && enforcement.targetUser) {
+ const current = await this.usersRepository.findOne({ where: { id: enforcement.entityId } });
+ if (!current) return;
+ const previousState = { status: current.status };
+ const newStatus =
+ current.status === UserStatus.BANNED ? UserStatus.ACTIVE : current.status;
+ await this.usersRepository.update(enforcement.entityId, { status: newStatus });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (enforcement.type === ReportAction.USER_BANNED && enforcement.targetUser) { | |
| const previousState = { status: enforcement.targetUser.status }; | |
| const newStatus = enforcement.targetUser.status === UserStatus.BANNED ? UserStatus.ACTIVE : enforcement.targetUser.status; | |
| await this.usersRepository.update(enforcement.entityId, { | |
| status: newStatus, | |
| }); | |
| await this.createAuditLog({ | |
| adminId: admin.id, | |
| admin, | |
| action: `ENFORCEMENT_REVERSED_${enforcement.type}`, | |
| entityType: enforcement.entityType, | |
| entityId: enforcement.entityId, | |
| previousState, | |
| newState: { status: newStatus }, | |
| reason: 'Enforcement reversal requested', | |
| ipAddress, | |
| }); | |
| if (enforcement.type === ReportAction.USER_BANNED && enforcement.targetUser) { | |
| const current = await this.usersRepository.findOne({ where: { id: enforcement.entityId } }); | |
| if (!current) return; | |
| const previousState = { status: current.status }; | |
| const newStatus = | |
| current.status === UserStatus.BANNED ? UserStatus.ACTIVE : current.status; | |
| await this.usersRepository.update(enforcement.entityId, { status: newStatus }); | |
| await this.createAuditLog({ | |
| adminId: admin.id, | |
| admin, | |
| action: `ENFORCEMENT_REVERSED_${enforcement.type}`, | |
| entityType: enforcement.entityType, | |
| entityId: enforcement.entityId, | |
| previousState, | |
| newState: { status: newStatus }, | |
| reason: 'Enforcement reversal requested', | |
| ipAddress, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/reports/report-enforcement.service.ts` around lines 101 - 118, In
reverseEnforcement, don’t rely on the stale enforcement.targetUser snapshot to
compute previousState/newStatus; re-fetch the current user from the DB (via
usersRepository.findOne/findById or the repository method used elsewhere) using
enforcement.entityId, set previousState based on that fresh record’s status,
compute newStatus from that current status (toggle BANNED → ACTIVE otherwise
keep), then call usersRepository.update and createAuditLog with previousState
and newState; alternatively, if a previousState was captured at apply-time, use
that stored previousState instead of enforcement.targetUser.
| if (updateDto.action && updateDto.action !== ReportAction.NONE) { | ||
| const ipAddress = '0.0.0.0'; | ||
| await this.enforcementService.applyEnforcement( | ||
| report, | ||
| updateDto.action, | ||
| admin, | ||
| ipAddress, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Hardcoded ipAddress = '0.0.0.0' defeats the audit trail's purpose.
Issue #498's acceptance criterion is that every enforcement action is auditable. With a hardcoded '0.0.0.0', the AdminAuditLog.ipAddress column captures no actionable forensic information — every reversal/audit query will see the same value. The controller currently doesn't propagate the request IP at all.
Thread the real client IP from the controller (e.g., via @Ip() or request.ip) down to updateStatus and into applyEnforcement.
🛡️ Suggested wiring
// reports.controller.ts
`@Patch`(':id/status')
`@UseGuards`(RolesGuard)
`@Roles`(UserRole.ADMIN)
updateStatus(
`@Param`('id') id: string,
`@Body`() updateDto: UpdateReportStatusDto,
`@CurrentUser`() admin: User,
+ `@Ip`() ip: string,
) {
- return this.reportsService.updateStatus(id, updateDto, admin);
+ return this.reportsService.updateStatus(id, updateDto, admin, ip);
}// reports.service.ts
- async updateStatus(id: string, updateDto: UpdateReportStatusDto, admin: User): Promise<Report> {
+ async updateStatus(id: string, updateDto: UpdateReportStatusDto, admin: User, ipAddress: string): Promise<Report> {
@@
- if (updateDto.action && updateDto.action !== ReportAction.NONE) {
- const ipAddress = '0.0.0.0';
- await this.enforcementService.applyEnforcement(
- report,
- updateDto.action,
- admin,
- ipAddress,
- );
- }
+ if (updateDto.action && updateDto.action !== ReportAction.NONE) {
+ await this.enforcementService.applyEnforcement(report, updateDto.action, admin, ipAddress);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/reports/reports.service.ts` around lines 104 - 112, The report
update currently passes a hardcoded ipAddress ('0.0.0.0') into
enforcementService.applyEnforcement which breaks auditing; change the wiring so
the real client IP is threaded from the controller into
ReportsService.updateStatus (or the method handling the update) and then
forwarded into enforcementService.applyEnforcement. Concretely: accept an ip
(e.g., requestIp: string) in the controller (use `@Ip`() or request.ip), add an ip
parameter to ReportsService.updateStatus (or the specific update method) and to
EnforcementService.applyEnforcement, update all call sites to pass the real IP
through, and ensure AdminAuditLog.ipAddress is populated with that value instead
of the hardcoded '0.0.0.0'.
| .where('id = :id', { id: track.id }) | ||
| .withDeleted() | ||
| .orWhere('id = :id AND "deletedAt" IS NOT NULL', { id: track.id }) | ||
| .execute(); |
There was a problem hiding this comment.
Same orWhere logic flaw — predicate is redundant.
(id = :id) OR (id = :id AND "deletedAt" IS NOT NULL) reduces to id = :id, so this update is not actually constrained to soft-deleted rows. It only works correctly today because the surrounding SELECT (lines 62-69) already filtered by track.deletedAt IS NOT NULL. If that upstream guarantee ever changes, the reaper would clear filename on live rows. Use andWhere so the predicate is real.
🛠 Proposed fix
await this.trackRepo
.createQueryBuilder()
.update(Track)
.set({ filename: null as unknown as string })
- .where('id = :id', { id: track.id })
- .orWhere('id = :id AND "deletedAt" IS NOT NULL', { id: track.id })
+ .where('id = :id', { id: track.id })
+ .andWhere('"deletedAt" IS NOT NULL')
.execute();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .where('id = :id', { id: track.id }) | |
| .withDeleted() | |
| .orWhere('id = :id AND "deletedAt" IS NOT NULL', { id: track.id }) | |
| .execute(); | |
| await this.trackRepo | |
| .createQueryBuilder() | |
| .update(Track) | |
| .set({ filename: null as unknown as string }) | |
| .where('id = :id', { id: track.id }) | |
| .andWhere('"deletedAt" IS NOT NULL') | |
| .execute(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/tracks/track-file-reaper.service.ts` around lines 91 - 93, The
current query uses .where('id = :id', { id: track.id }).orWhere('id = :id AND
"deletedAt" IS NOT NULL', { id: track.id }) which collapses to id = :id; change
the second clause to an and condition so the update only affects soft-deleted
rows. Replace the orWhere call with an andWhere (e.g. .andWhere('"deletedAt" IS
NOT NULL') or .andWhere('id = :id AND "deletedAt" IS NOT NULL', { id: track.id
})) in the TrackFileReaperService query so the update is actually constrained to
rows where "deletedAt" IS NOT NULL and avoid duplicate/conflicting parameter
bindings.
| .where('id = :id', { id }) | ||
| .withDeleted() | ||
| .orWhere('id = :id AND "deletedAt" IS NOT NULL', { id }) | ||
| .execute(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does TypeORM 0.3.x QueryBuilder update() automatically exclude soft-deleted rows or require withDeleted()?
💡 Result:
No, TypeORM 0.3.x QueryBuilder update does not automatically exclude soft-deleted rows. It requires calling withDeleted to include soft-deleted rows in the update target if needed, but by default excludes them like SELECT queries. Soft delete filtering (excluding rows where deletedAt IS NOT NULL) is automatically applied in QueryBuilder for SELECT, find* methods, and repository methods when the entity has @DeleteDateColumn. This is implemented in the query builder's where conditions for read operations. For update via UpdateQueryBuilder, the documentation does not mention automatic soft-delete filtering, and source code snippets for UpdateQueryBuilder do not show automatic addition of deletedAt IS NULL. Users must manually add .where("deletedAt IS NULL") if they want to exclude soft-deleted rows. withDeleted disables the soft-delete filter, allowing updates to include soft-deleted rows. This behavior is consistent in 0.3.x as seen in recent releases (e.g., 0.3.23 in 2025) and source code. To exclude soft-deleted rows in update, add explicit where condition. To include them, use .withDeleted.
Citations:
- 1: https://typeorm.biunav.com/en/select-query-builder.html
- 2: https://orkhan.gitbook.io/typeorm/docs/docs/query-builder/1-select-query-builder.md
- 3: https://typeorm.io/docs/query-builder/update-query-builder/
- 4: https://typeorm.io/docs/query-builder/delete-query-builder/
- 5: https://github.com/typeorm/typeorm/blob/master/src/query-builder/SoftDeleteQueryBuilder.ts
- 6: https://github.com/typeorm/typeorm/blob/master/src/query-builder/UpdateQueryBuilder.ts
- 7: Feature: Soft Delete typeorm/typeorm#534
🏁 Script executed:
# Find and examine the tracks.service.ts file
fd -type f -name "tracks.service.ts" | head -5Repository: OlufunbiIK/tip-tune
Length of output: 234
🏁 Script executed:
fd "tracks.service.ts"Repository: OlufunbiIK/tip-tune
Length of output: 100
🏁 Script executed:
# Read the file to see the context around lines 240-242
cat -n backend/src/tracks/tracks.service.ts | sed -n '230,255p'Repository: OlufunbiIK/tip-tune
Length of output: 1253
🏁 Script executed:
# See the full remove() method, especially Phase 1
cat -n backend/src/tracks/tracks.service.ts | sed -n '200,250p'Repository: OlufunbiIK/tip-tune
Length of output: 2136
🏁 Script executed:
# Check the findOne() method to understand soft-delete filtering
cat -n backend/src/tracks/tracks.service.ts | sed -n '190,210p'Repository: OlufunbiIK/tip-tune
Length of output: 688
orWhere makes the soft-delete predicate a no-op.
The resulting SQL is WHERE (id = :id) OR (id = :id AND "deletedAt" IS NOT NULL), which simplifies to just id = :id — so the "only update soft-deleted rows" guarantee is not enforced. This currently happens to work only because Phase 1 just soft-deleted it; any future reordering or re-use of this pattern will silently update live rows. Use andWhere (or merge into the initial where) to make the predicate meaningful.
🛠 Proposed fix
await this.tracksRepository
.createQueryBuilder()
.update(Track)
.set({ filename: null as unknown as string })
- .where('id = :id', { id })
- .orWhere('id = :id AND "deletedAt" IS NOT NULL', { id })
+ .where('id = :id', { id })
+ .andWhere('"deletedAt" IS NOT NULL')
.execute();Note: TypeORM's update() QueryBuilder does not auto-filter by deletedAt (that filter applies to SELECTs unless .withDeleted() is used), so this predicate is the only thing gating the update to soft-deleted rows.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .where('id = :id', { id }) | |
| .withDeleted() | |
| .orWhere('id = :id AND "deletedAt" IS NOT NULL', { id }) | |
| .execute(); | |
| .where('id = :id', { id }) | |
| .andWhere('"deletedAt" IS NOT NULL') | |
| .execute(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/tracks/tracks.service.ts` around lines 240 - 242, The current
QueryBuilder uses .where('id = :id', { id }).orWhere('id = :id AND "deletedAt"
IS NOT NULL', { id }) which collapses to just id = :id; change this to enforce
the soft-delete guard by combining the predicates (e.g., replace the chain with
a single .where('id = :id AND "deletedAt" IS NOT NULL', { id }) or use
.where('id = :id', { id }).andWhere('"deletedAt" IS NOT NULL') so the update in
TracksService targets only soft-deleted rows.
2 participants
Summary
Implements four high-complexity backend features in the Stellar Wave program:
Also fixes pre-existing TypeScript compilation errors blocking the build.
Changes
ScheduledReleaseRetryPolicywith exponential backoff configurationRecommendationCacheServicewith in-memory snapshot caching (1h TTL)ReportEnforcementServicefor auditable user bans and content removalsReportQueryDtowith class-validator typed filters and paginationTesting
Fixes
Closes #499
Closes #498
Closes #497
Closes #496
Summary by CodeRabbit
New Features
Bug Fixes