Merged
Conversation
|
@Xaxxoo 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 a unified Redis infrastructure for the backend by creating a centralized Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Controller as Controller Handler
participant Interceptor as CacheInterceptor
participant CacheService as CacheService
participant Redis as Redis Client
participant Response as HTTP Response
Client->>Interceptor: GET /api/resource
Interceptor->>Interceptor: Check `@CacheResponse` metadata
alt Metadata Present & GET Request
Interceptor->>Interceptor: Build cache key from path + query
Interceptor->>CacheService: get(cacheKey)
CacheService->>Redis: GET cacheKey
alt Cache HIT
Redis-->>CacheService: Cached JSON data
CacheService-->>Interceptor: Parsed object
Interceptor->>Response: Set X-Cache: HIT header
Interceptor-->>Client: Return cached response
else Cache MISS
Redis-->>CacheService: null
CacheService-->>Interceptor: null
Interceptor->>Controller: next.handle()
Controller-->>Interceptor: Observable<response>
Interceptor->>CacheService: set(cacheKey, data, {ttl})
CacheService->>Redis: SET cacheKey data EX ttl
Redis-->>CacheService: OK
Interceptor->>Response: Set X-Cache: MISS header
Interceptor-->>Client: Return live response
else Redis Error
Redis-->>CacheService: Error
CacheService-->>Interceptor: null (degraded)
Interceptor->>Controller: next.handle()
Controller-->>Interceptor: Observable<response>
Interceptor-->>Client: Return live response
end
else No Metadata or Non-GET
Interceptor->>Controller: next.handle()
Controller-->>Interceptor: Observable<response>
Interceptor-->>Client: Return response (no caching)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 16
🧹 Nitpick comments (6)
backend/src/unify-redis/index.ts (1)
1-2: Incomplete barrel exports.The barrel file only exports Redis-related symbols but omits the other public modules in this directory (
CacheModule,CacheService,CacheInterceptor,HealthController). Consider adding exports for all public entities to provide a complete entry point.export * from './redis.module'; export * from './redis.constants'; +export * from './cache.module'; +export * from './cache.service'; +export * from './cache.interceptor'; +export * from './health.controller';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/unify-redis/index.ts` around lines 1 - 2, The barrel currently only exports Redis items; update it to re-export all public entities in this directory so consumers can import from the single entry point — add exports for CacheModule (cache.module), CacheService (cache.service), CacheInterceptor (cache.interceptor) and HealthController (health.controller) alongside the existing redis.module and redis.constants exports so the barrel exposes all public symbols.backend/src/unify-redis/health.controller.spec.ts (1)
43-45: Redundant provider override.
HealthCheckServiceis already provided withmockHealth()on line 27. The.overrideProvider()call on line 43-44 is unnecessary:}) - .overrideProvider(HealthCheckService) - .useValue(mockHealth()) .compile();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/unify-redis/health.controller.spec.ts` around lines 43 - 45, The test redundantly overrides HealthCheckService a second time; remove the .overrideProvider(HealthCheckService).useValue(mockHealth()) call in health.controller.spec.ts (the override block that precedes .compile()) and rely on the existing provider registration that uses mockHealth() (the provider added earlier). Ensure the Test.createTestingModule(...) chain still calls .compile() after removing the override lines.backend/src/unify-redis/cache.module.ts (1)
17-21: RedundantReflectorregistration.
Reflectoris already provided globally by@nestjs/coreand does not need to be registered as a provider. Removing it avoids confusion and keeps the provider list minimal.providers: [ CacheService, CacheInterceptor, - Reflector, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/unify-redis/cache.module.ts` around lines 17 - 21, Remove the redundant Reflector provider from the module providers array: delete the Reflector entry from the providers list (where CacheService and CacheInterceptor are registered) and also remove the Reflector import if present; leave CacheService and CacheInterceptor as the providers (no other code changes required since Reflector is already provided by `@nestjs/core` globally.backend/src/unify-redis/redis.module.ts (1)
59-64:forRootAsyncdoes not exportREDIS_OPTIONS, unlikeforRoot.
forRootexports bothREDIS_CLIENTandREDIS_OPTIONS, butforRootAsynconly exportsREDIS_CLIENT. Consumers expectingREDIS_OPTIONSfromforRootAsyncwill fail to inject it. If the options aren't needed externally, consider removingREDIS_OPTIONSfromforRoot's exports as well for consistency.Option A: Add REDIS_OPTIONS to forRootAsync
return { module: RedisModule, imports: [ConfigModule], - providers: [clientProvider], - exports: [REDIS_CLIENT], + providers: [clientProvider, optionsProvider], + exports: [REDIS_CLIENT, REDIS_OPTIONS], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/unify-redis/redis.module.ts` around lines 59 - 64, The forRootAsync export list is missing REDIS_OPTIONS causing injection failures; update the forRootAsync return object in RedisModule to include REDIS_OPTIONS alongside REDIS_CLIENT (i.e., exports: [REDIS_CLIENT, REDIS_OPTIONS]) so it matches forRoot, referencing the forRootAsync factory in RedisModule and the REDIS_OPTIONS/REDIS_CLIENT symbols; alternatively, if options are not intended for consumers, remove REDIS_OPTIONS from forRoot exports to keep behavior consistent.backend/src/unify-redis/cache.service.ts (1)
103-114:KEYScommand blocks Redis and scales poorly.
KEYS patternis O(N) and blocks the Redis server. In production with many keys, this can cause latency spikes. The context snippets show a similar pattern exists inbackend/src/cache/cache.service.ts:invalidatePattern, but consolidating on this approach perpetuates the risk.Consider using
SCANfor non-blocking iteration:Proposed fix using SCAN
async reset(pattern: string): Promise<void> { try { - const keys = await this.redis.keys(pattern); - if (keys.length > 0) { - await this.redis.del(...keys); - this.logger.log(`Evicted ${keys.length} key(s) matching "${pattern}"`); + let cursor = '0'; + let totalDeleted = 0; + do { + const [nextCursor, keys] = await this.redis.scan(cursor, 'MATCH', pattern, 'COUNT', 100); + cursor = nextCursor; + if (keys.length > 0) { + await this.redis.del(...keys); + totalDeleted += keys.length; + } + } while (cursor !== '0'); + if (totalDeleted > 0) { + this.logger.log(`Evicted ${totalDeleted} key(s) matching "${pattern}"`); } } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/unify-redis/cache.service.ts` around lines 103 - 114, The reset method uses this.redis.keys(pattern) which is blocking and O(N); replace it with a non-blocking SCAN-based iteration: implement a scan loop (or use a scanStream) in the reset function to yield matching keys in batches, collect each batch and delete them in safe-sized chunks (use UNLINK if available for non-blocking deletion) and log the total evicted; also update the similar invalidatePattern usage to the same SCAN + chunked del/UNLINK approach; ensure errors increment this._errors and keep the existing logging behavior.backend/src/unify-redis/cache.interceptor.ts (1)
20-26: Consider using NestJS'sSetMetadatahelper.The manual
Reflect.defineMetadataworks but NestJS providesSetMetadatawhich integrates better with theReflectorand follows framework conventions.Proposed refactor
+import { SetMetadata, applyDecorators } from '@nestjs/common'; + export function CacheResponse(ttl = 60, key?: string) { - return (target: object, propertyKey: string | symbol, descriptor: PropertyDescriptor) => { - Reflect.defineMetadata(CACHE_TTL_METADATA, ttl, descriptor.value); - if (key) Reflect.defineMetadata(CACHE_KEY_METADATA, key, descriptor.value); - return descriptor; - }; + const decorators = [SetMetadata(CACHE_TTL_METADATA, ttl)]; + if (key) decorators.push(SetMetadata(CACHE_KEY_METADATA, key)); + return applyDecorators(...decorators); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/unify-redis/cache.interceptor.ts` around lines 20 - 26, Replace the manual Reflect.defineMetadata calls in CacheResponse with Nest's SetMetadata decorator helper: import SetMetadata from '@nestjs/common' and invoke SetMetadata(CACHE_TTL_METADATA, ttl) and, when key is provided, SetMetadata(CACHE_KEY_METADATA, key) as decorators inside the returned decorator function (i.e., call the SetMetadata results with the same target, propertyKey, descriptor) so the metadata is registered via NestJS conventions; keep the function name CacheResponse and the metadata keys CACHE_TTL_METADATA and CACHE_KEY_METADATA unchanged and return the original descriptor.
🤖 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/unify-redis/app.module.ts`:
- Around line 4-6: Update the incorrect import paths in app.module.ts: replace
the folder-based imports for CacheModule, HealthController, and RedisModule
(currently imported from './cache/cache.module', './health/health.controller',
'./redis/redis.module') with same-directory imports that point to the actual
files (e.g., import CacheModule from './cache.module', HealthController from
'./health.controller', RedisModule from './redis.module' or the exact filenames
present) so the import specifiers reference files in the same directory as
app.module.ts.
- Around line 11-37: The unify-redis AppModule (export class AppModule) is not
wired into the running application so RedisModule.forRootAsync() and CacheModule
are never used; either move the RedisModule.forRootAsync() and CacheModule
imports from this unify-redis AppModule into the real application module that
bootstraps the app (the existing application AppModule) replacing the current
Redis setup there, or if this file is only an example, rename the class (e.g.,
UnifyRedisExampleModule) and add a clear comment/docstring explaining it is a
reference implementation and not part of the runtime bootstrap.
In `@backend/src/unify-redis/cache.interceptor.spec.ts`:
- Around line 183-196: The test is asserting cache.get before the interceptor's
returned Observable completes; change the test to await the Observable returned
by interceptor.intercept(ctx, next) (e.g., wrap it with RxJS lastValueFrom or
subscribe and await completion) so the interceptor has run and invoked cache.get
before assertions; ensure you import lastValueFrom if you use it and call await
lastValueFrom(interceptor.intercept(ctx, next)) prior to reading the mock calls
for cache.get.
In `@backend/src/unify-redis/cache.interceptor.ts`:
- Around line 65-74: The async callback passed to tap() can throw and its
returned Promise is not awaited, causing unhandled errors and missed header
setting; update the cache write in CacheInterceptor so the async operation is
properly chained or errors are caught: either replace the tap(...) usage with an
operator that supports async mapping (e.g., switchMap/mergeMap and of(data)) to
await this.cache.set(cacheKey, data, { ttl }) before returning the original
data, or keep tap(...) but wrap the await this.cache.set(...) call in a
try/catch and log any errors and still set the response header (use the same
cacheKey, ttl, ctx.switchToHttp().getResponse()). Ensure no fire-and-forget
awaits remain so errors are observable and X-Cache is always set.
- Around line 77-80: The buildKey method is mangling array query params by
casting req.query to Record<string,string>; update buildKey (private
buildKey(req: Request)) to serialize req.query properly by iterating
Object.entries(req.query) and for each key append each value (handling arrays
and singletons) to a URLSearchParams instance so repeated params (e.g.,
ids=1&ids=2) are preserved, then use params.toString() when building the final
key.
In `@backend/src/unify-redis/cache.module.ts`:
- Line 3: The import line "import { RedisModule } from '../redis/redis.module';"
is wrong and unused; either remove the import entirely if RedisModule is not
required, or correct the path to "./redis.module" and add RedisModule to the
`@Module` imports array in cache.module.ts so it is actually used (e.g., include
RedisModule in the imports property of the CacheModule definition).
- Around line 15-24: Delete the orphaned legacy CacheModule and its unused
services (the old CacheModule class plus CacheWarmingService and
CacheMetricsService) so only the unify-redis CacheModule remains; remove any
leftover imports of the old module and ensure app.module.ts imports the
unify-redis CacheModule only. Replace any usages of the string token
'REDIS_CLIENT' with the unified Symbol('REDIS_CLIENT') token across the codebase
to eliminate duplication/conflict. In the unify-redis/cache.module.ts file,
update or remove the incorrect JSDoc that claims "RedisModule is NOT imported
here" so it accurately reflects that RedisModule is imported. Finally, run a
project-wide search to confirm there are no remaining references to the old
module/classes or the string 'REDIS_CLIENT' after deletion.
In `@backend/src/unify-redis/cache.service.spec.ts`:
- Line 2: The import path for REDIS_CLIENT in cache.service.spec.ts is
incorrect; update the import from '../redis/redis.constants' to the correct
relative path that points to the file in the same directory (e.g.,
'./redis.constants') so the REDIS_CLIENT symbol is resolved properly when
running CI.
In `@backend/src/unify-redis/cache.service.ts`:
- Line 8: The import path for REDIS_CLIENT in cache.service.ts is incorrect
(causing TS2307); update the import in backend/src/unify-redis/cache.service.ts
to use a same-directory relative path so it correctly imports REDIS_CLIENT from
redis.constants (replace the '../redis/redis.constants' import with the correct
'./redis.constants' import), then run type-check/build to confirm the module
resolves.
In `@backend/src/unify-redis/health.controller.spec.ts`:
- Around line 28-39: The test is providing Terminus indicators with string
tokens which won't satisfy constructor injection; replace the string tokens with
the actual classes (DiskHealthIndicator, MemoryHealthIndicator,
HttpHealthIndicator) in the providers array and supply the same mock objects
(useValue with checkStorage, checkHeap, pingCheck) so Nest can resolve them, and
ensure you import DiskHealthIndicator, MemoryHealthIndicator,
HttpHealthIndicator and HealthCheckService from '@nestjs/terminus' for the test
file.
- Line 3: The test imports CacheService from the wrong relative path; update the
import in backend/src/unify-redis/health.controller.spec.ts to reference the
local file where CacheService is defined (replace ../cache/cache.service with
the same-directory path, e.g. ./cache.service) so the spec can resolve the
CacheService symbol correctly.
In `@backend/src/unify-redis/health.controller.ts`:
- Around line 13-19: The HttpHealthIndicator parameter is injected into the
HealthController constructor but never used; remove the unused dependency by
deleting the private readonly http: HttpHealthIndicator parameter from the
constructor signature (in the class where HealthCheckService,
DiskHealthIndicator, MemoryHealthIndicator, CacheService are declared) and also
remove the corresponding HttpHealthIndicator import from the top of the file so
only used symbols (HealthCheckService, DiskHealthIndicator,
MemoryHealthIndicator, CacheService) remain imported.
- Line 9: The import path for CacheService is incorrect; update the import in
health.controller.ts to import CacheService from the local module in the same
directory by changing the path string to './cache.service' (referencing the
CacheService symbol) so the controller imports the service from the correct
file.
- Around line 36-57: The redisHealthIndicator method is calling non-public
members on CacheService; add a public async ping() method and a public metrics
getter (or use the existing getMetrics()) on CacheService, then update
redisHealthIndicator to call await this.cache.ping() and use
this.cache.getMetrics() (or this.cache.metrics) and annotate the method return
type as Promise<HealthIndicatorResult> while using status literals as const
(e.g., status: 'up' as const / 'down' as const) to satisfy typing; locate
symbols redisHealthIndicator, CacheService, ping(), getMetrics(), metrics, and
HealthIndicatorResult to apply these changes.
In `@backend/src/unify-redis/redis.constants.ts`:
- Around line 1-2: The new Symbol tokens REDIS_CLIENT and REDIS_OPTIONS conflict
with existing string tokens; update all consumers (e.g., the current cache
service/module and the leaderboards Redis module) to import and use the exported
Symbol constants REDIS_CLIENT and REDIS_OPTIONS from the unify-redis module
instead of hard-coded string tokens ('REDIS_CLIENT' etc.), and make sure
providers register the same Symbol keys when binding the Redis client and
options so DI resolves the single shared instance across modules; if any module
still defines its own REDIS_CLIENT, remove that local string token and replace
usages with the shared Symbol import to ensure compatibility.
In `@backend/src/unify-redis/redis.module.ts`:
- Line 4: The project is mixing token types causing injection failures: change
the token usage in redis.constants.ts to export REDIS_CLIENT and REDIS_OPTIONS
as string literals (e.g., 'REDIS_CLIENT', 'REDIS_OPTIONS') or alternatively
update all consumers to import the Symbol exports; pick the consistent approach
(prefer keeping string tokens to match existing consumers like cache.module.ts
and health.controller.ts). Also ensure the module factory methods are
consistent: update the RedisModule.forRootAsync implementation to export both
REDIS_CLIENT and REDIS_OPTIONS (same export set as forRoot) so downstream
consumers can inject REDIS_OPTIONS when needed; update references to
RedisModuleOptions if necessary to align with the chosen token representation.
---
Nitpick comments:
In `@backend/src/unify-redis/cache.interceptor.ts`:
- Around line 20-26: Replace the manual Reflect.defineMetadata calls in
CacheResponse with Nest's SetMetadata decorator helper: import SetMetadata from
'@nestjs/common' and invoke SetMetadata(CACHE_TTL_METADATA, ttl) and, when key
is provided, SetMetadata(CACHE_KEY_METADATA, key) as decorators inside the
returned decorator function (i.e., call the SetMetadata results with the same
target, propertyKey, descriptor) so the metadata is registered via NestJS
conventions; keep the function name CacheResponse and the metadata keys
CACHE_TTL_METADATA and CACHE_KEY_METADATA unchanged and return the original
descriptor.
In `@backend/src/unify-redis/cache.module.ts`:
- Around line 17-21: Remove the redundant Reflector provider from the module
providers array: delete the Reflector entry from the providers list (where
CacheService and CacheInterceptor are registered) and also remove the Reflector
import if present; leave CacheService and CacheInterceptor as the providers (no
other code changes required since Reflector is already provided by `@nestjs/core`
globally.
In `@backend/src/unify-redis/cache.service.ts`:
- Around line 103-114: The reset method uses this.redis.keys(pattern) which is
blocking and O(N); replace it with a non-blocking SCAN-based iteration:
implement a scan loop (or use a scanStream) in the reset function to yield
matching keys in batches, collect each batch and delete them in safe-sized
chunks (use UNLINK if available for non-blocking deletion) and log the total
evicted; also update the similar invalidatePattern usage to the same SCAN +
chunked del/UNLINK approach; ensure errors increment this._errors and keep the
existing logging behavior.
In `@backend/src/unify-redis/health.controller.spec.ts`:
- Around line 43-45: The test redundantly overrides HealthCheckService a second
time; remove the .overrideProvider(HealthCheckService).useValue(mockHealth())
call in health.controller.spec.ts (the override block that precedes .compile())
and rely on the existing provider registration that uses mockHealth() (the
provider added earlier). Ensure the Test.createTestingModule(...) chain still
calls .compile() after removing the override lines.
In `@backend/src/unify-redis/index.ts`:
- Around line 1-2: The barrel currently only exports Redis items; update it to
re-export all public entities in this directory so consumers can import from the
single entry point — add exports for CacheModule (cache.module), CacheService
(cache.service), CacheInterceptor (cache.interceptor) and HealthController
(health.controller) alongside the existing redis.module and redis.constants
exports so the barrel exposes all public symbols.
In `@backend/src/unify-redis/redis.module.ts`:
- Around line 59-64: The forRootAsync export list is missing REDIS_OPTIONS
causing injection failures; update the forRootAsync return object in RedisModule
to include REDIS_OPTIONS alongside REDIS_CLIENT (i.e., exports: [REDIS_CLIENT,
REDIS_OPTIONS]) so it matches forRoot, referencing the forRootAsync factory in
RedisModule and the REDIS_OPTIONS/REDIS_CLIENT symbols; alternatively, if
options are not intended for consumers, remove REDIS_OPTIONS from forRoot
exports to keep behavior consistent.
🪄 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: ec8164a5-d94d-456f-b744-3936f65ddac9
📒 Files selected for processing (12)
backend/src/unify-redis/app.module.tsbackend/src/unify-redis/cache.interceptor.spec.tsbackend/src/unify-redis/cache.interceptor.tsbackend/src/unify-redis/cache.module.tsbackend/src/unify-redis/cache.service.spec.tsbackend/src/unify-redis/cache.service.tsbackend/src/unify-redis/health.controller.spec.tsbackend/src/unify-redis/health.controller.tsbackend/src/unify-redis/index.tsbackend/src/unify-redis/redis.constants.tsbackend/src/unify-redis/redis.module.spec.tsbackend/src/unify-redis/redis.module.ts
Comment on lines
+4
to
+6
| import { CacheModule } from './cache/cache.module'; | ||
| import { HealthController } from './health/health.controller'; | ||
| import { RedisModule } from './redis/redis.module'; |
There was a problem hiding this comment.
Incorrect import paths — files are in the same directory.
The imports reference subdirectories (./cache/, ./health/, ./redis/) that don't exist. All files are in backend/src/unify-redis/, so paths should be relative to the same directory:
-import { CacheModule } from './cache/cache.module';
-import { HealthController } from './health/health.controller';
-import { RedisModule } from './redis/redis.module';
+import { CacheModule } from './cache.module';
+import { HealthController } from './health.controller';
+import { RedisModule } from './redis.module';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/app.module.ts` around lines 4 - 6, Update the
incorrect import paths in app.module.ts: replace the folder-based imports for
CacheModule, HealthController, and RedisModule (currently imported from
'./cache/cache.module', './health/health.controller', './redis/redis.module')
with same-directory imports that point to the actual files (e.g., import
CacheModule from './cache.module', HealthController from './health.controller',
RedisModule from './redis.module' or the exact filenames present) so the import
specifiers reference files in the same directory as app.module.ts.
Comment on lines
+11
to
+37
| @Module({ | ||
| imports: [ | ||
| // ── Config ────────────────────────────────────────────────────────────── | ||
| ConfigModule.forRoot({ | ||
| isGlobal: true, | ||
| cache: true, | ||
| }), | ||
|
|
||
| // ── Redis — registered once, exported globally ─────────────────────────── | ||
| // All feature modules that previously imported their own redis.module.ts | ||
| // now rely on the REDIS_CLIENT token provided here. | ||
| RedisModule.forRootAsync(), | ||
|
|
||
| // ── Cache — platform-level service available everywhere ────────────────── | ||
| // CacheService and CacheInterceptor are now injectable application-wide | ||
| // without re-importing CacheModule in every feature module. | ||
| CacheModule, | ||
|
|
||
| // ── Health ─────────────────────────────────────────────────────────────── | ||
| TerminusModule, | ||
|
|
||
| // ── Feature modules ────────────────────────────────────────────────────── | ||
| // LeaderboardsModule, | ||
| ], | ||
| controllers: [HealthController], | ||
| }) | ||
| export class AppModule {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what AppModule main.ts actually bootstraps
echo "=== main.ts bootstrap ==="
cat backend/src/main.ts | head -30
echo ""
echo "=== Actual AppModule imports ==="
rg -A 50 '@Module' backend/src/app.module.ts | head -60Repository: OlufunbiIK/tip-tune
Length of output: 2925
🏁 Script executed:
# Check if the unify-redis module exists and examine its structure
echo "=== Check unify-redis directory ==="
ls -la backend/src/unify-redis/ 2>&1 || echo "Directory does not exist"
echo ""
echo "=== Check if unify-redis/app.module.ts exists ==="
cat backend/src/unify-redis/app.module.ts 2>&1 | head -50
echo ""
echo "=== Search for imports of unify-redis module ==="
rg 'unify-redis' backend/src/ --type ts
echo ""
echo "=== Search for RedisModule and CacheModule usage ==="
rg '(RedisModule|CacheModule)' backend/src/ --type ts | head -20Repository: OlufunbiIK/tip-tune
Length of output: 4497
Unintegrated unify-redis/app.module.ts — orphaned implementation not used by the running application.
The unify-redis/app.module.ts exists but is not imported or referenced anywhere in the codebase. The application bootstraps backend/src/app.module.ts instead, which does not import RedisModule or CacheModule from the unify-redis package. This means the unified Redis infrastructure remains unavailable to the running application.
To fulfill the PR objective of unifying Redis provisioning:
- Option A: Migrate
RedisModule.forRootAsync()andCacheModuleimports into the actualbackend/src/app.module.tsin place of existing Redis setup. - Option B: If this is a reference implementation, rename the module (e.g.,
UnifyRedisExampleModule) and document its purpose clearly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/app.module.ts` around lines 11 - 37, The unify-redis
AppModule (export class AppModule) is not wired into the running application so
RedisModule.forRootAsync() and CacheModule are never used; either move the
RedisModule.forRootAsync() and CacheModule imports from this unify-redis
AppModule into the real application module that bootstraps the app (the existing
application AppModule) replacing the current Redis setup there, or if this file
is only an example, rename the class (e.g., UnifyRedisExampleModule) and add a
clear comment/docstring explaining it is a reference implementation and not part
of the runtime bootstrap.
Comment on lines
+183
to
+196
| it('incorporates query-string parameters into the cache key', async () => { | ||
| cache.get.mockResolvedValue(null); | ||
| cache.set.mockResolvedValue(true); | ||
|
|
||
| const ctx = makeContext('GET', '/scores', { page: '2', limit: '20' }); | ||
| const next = { handle: makeHandler([]) }; | ||
|
|
||
| await interceptor.intercept(ctx, next); | ||
|
|
||
| const calledKey: string = (cache.get as jest.Mock).mock.calls[0][0]; | ||
| expect(calledKey).toContain('/scores'); | ||
| expect(calledKey).toContain('page=2'); | ||
| expect(calledKey).toContain('limit=20'); | ||
| }); |
There was a problem hiding this comment.
Potential test flakiness: missing await on observable subscription.
Line 190 calls interceptor.intercept() but doesn't await the result or subscribe to the observable before asserting on cache.get at Line 192. The assertion may execute before the interceptor has a chance to call cache.get.
Proposed fix
- await interceptor.intercept(ctx, next);
+ const result$ = await interceptor.intercept(ctx, next);
+ await new Promise<void>((res) => result$.subscribe({ complete: res }));
const calledKey: string = (cache.get as jest.Mock).mock.calls[0][0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/cache.interceptor.spec.ts` around lines 183 - 196,
The test is asserting cache.get before the interceptor's returned Observable
completes; change the test to await the Observable returned by
interceptor.intercept(ctx, next) (e.g., wrap it with RxJS lastValueFrom or
subscribe and await completion) so the interceptor has run and invoked cache.get
before assertions; ensure you import lastValueFrom if you use it and call await
lastValueFrom(interceptor.intercept(ctx, next)) prior to reading the mock calls
for cache.get.
Comment on lines
+65
to
+74
| return next.handle().pipe( | ||
| tap(async (data) => { | ||
| const stored = await this.cache.set(cacheKey, data, { ttl }); | ||
| if (stored) { | ||
| this.logger.debug(`Cache SET: ${cacheKey} (ttl=${ttl}s)`); | ||
| const response = ctx.switchToHttp().getResponse<Response>(); | ||
| response.setHeader('X-Cache', 'MISS'); | ||
| } | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Unhandled promise in tap() callback may silently fail.
The tap() operator doesn't await async callbacks—the returned promise is fire-and-forget. If cache.set() throws after the observable emits, the error is unhandled and X-Cache: MISS header won't be set for that response.
Consider using switchMap or mergeMap with of(data) to properly chain the async operation, or wrap the callback in a .catch() for observability:
Minimal fix: catch and log errors
return next.handle().pipe(
- tap(async (data) => {
- const stored = await this.cache.set(cacheKey, data, { ttl });
- if (stored) {
- this.logger.debug(`Cache SET: ${cacheKey} (ttl=${ttl}s)`);
- const response = ctx.switchToHttp().getResponse<Response>();
- response.setHeader('X-Cache', 'MISS');
- }
- }),
+ tap((data) => {
+ this.cache.set(cacheKey, data, { ttl }).then((stored) => {
+ if (stored) {
+ this.logger.debug(`Cache SET: ${cacheKey} (ttl=${ttl}s)`);
+ const response = ctx.switchToHttp().getResponse<Response>();
+ response.setHeader('X-Cache', 'MISS');
+ }
+ }).catch((err) => {
+ this.logger.warn(`Cache SET error: ${err.message}`);
+ });
+ }),
);📝 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.
Suggested change
| return next.handle().pipe( | |
| tap(async (data) => { | |
| const stored = await this.cache.set(cacheKey, data, { ttl }); | |
| if (stored) { | |
| this.logger.debug(`Cache SET: ${cacheKey} (ttl=${ttl}s)`); | |
| const response = ctx.switchToHttp().getResponse<Response>(); | |
| response.setHeader('X-Cache', 'MISS'); | |
| } | |
| }), | |
| ); | |
| return next.handle().pipe( | |
| tap((data) => { | |
| this.cache.set(cacheKey, data, { ttl }).then((stored) => { | |
| if (stored) { | |
| this.logger.debug(`Cache SET: ${cacheKey} (ttl=${ttl}s)`); | |
| const response = ctx.switchToHttp().getResponse<Response>(); | |
| response.setHeader('X-Cache', 'MISS'); | |
| } | |
| }).catch((err) => { | |
| this.logger.warn(`Cache SET error: ${err.message}`); | |
| }); | |
| }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/cache.interceptor.ts` around lines 65 - 74, The async
callback passed to tap() can throw and its returned Promise is not awaited,
causing unhandled errors and missed header setting; update the cache write in
CacheInterceptor so the async operation is properly chained or errors are
caught: either replace the tap(...) usage with an operator that supports async
mapping (e.g., switchMap/mergeMap and of(data)) to await
this.cache.set(cacheKey, data, { ttl }) before returning the original data, or
keep tap(...) but wrap the await this.cache.set(...) call in a try/catch and log
any errors and still set the response header (use the same cacheKey, ttl,
ctx.switchToHttp().getResponse()). Ensure no fire-and-forget awaits remain so
errors are observable and X-Cache is always set.
Comment on lines
+77
to
+80
| private buildKey(req: Request): string { | ||
| const qs = new URLSearchParams(req.query as Record<string, string>).toString(); | ||
| return `http:${req.path}${qs ? `?${qs}` : ''}`; | ||
| } |
There was a problem hiding this comment.
Query string handling may mangle array parameters.
URLSearchParams with a Record<string, string> cast will coerce array values (e.g., ?ids=1&ids=2) incorrectly. Express parses these as { ids: ['1', '2'] }, but the cast produces ids=1,2 instead of ids=1&ids=2.
For correct cache key generation with array params, serialize req.query directly:
Proposed fix
private buildKey(req: Request): string {
- const qs = new URLSearchParams(req.query as Record<string, string>).toString();
+ const qs = new URLSearchParams(
+ Object.entries(req.query).flatMap(([k, v]) =>
+ Array.isArray(v) ? v.map((val) => [k, String(val)]) : [[k, String(v ?? '')]]
+ )
+ ).toString();
return `http:${req.path}${qs ? `?${qs}` : ''}`;
}📝 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.
Suggested change
| private buildKey(req: Request): string { | |
| const qs = new URLSearchParams(req.query as Record<string, string>).toString(); | |
| return `http:${req.path}${qs ? `?${qs}` : ''}`; | |
| } | |
| private buildKey(req: Request): string { | |
| const qs = new URLSearchParams( | |
| Object.entries(req.query).flatMap(([k, v]) => | |
| Array.isArray(v) ? v.map((val) => [k, String(val)]) : [[k, String(v ?? '')]] | |
| ) | |
| ).toString(); | |
| return `http:${req.path}${qs ? `?${qs}` : ''}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/cache.interceptor.ts` around lines 77 - 80, The
buildKey method is mangling array query params by casting req.query to
Record<string,string>; update buildKey (private buildKey(req: Request)) to
serialize req.query properly by iterating Object.entries(req.query) and for each
key append each value (handling arrays and singletons) to a URLSearchParams
instance so repeated params (e.g., ids=1&ids=2) are preserved, then use
params.toString() when building the final key.
| HttpHealthIndicator, | ||
| MemoryHealthIndicator, | ||
| } from '@nestjs/terminus'; | ||
| import { CacheService } from '../cache/cache.service'; |
There was a problem hiding this comment.
Incorrect import path.
The import path ../cache/cache.service is invalid. CacheService is in the same directory:
-import { CacheService } from '../cache/cache.service';
+import { CacheService } from './cache.service';📝 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.
Suggested change
| import { CacheService } from '../cache/cache.service'; | |
| import { CacheService } from './cache.service'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/health.controller.ts` at line 9, The import path for
CacheService is incorrect; update the import in health.controller.ts to import
CacheService from the local module in the same directory by changing the path
string to './cache.service' (referencing the CacheService symbol) so the
controller imports the service from the correct file.
Comment on lines
+13
to
+19
| constructor( | ||
| private readonly health: HealthCheckService, | ||
| private readonly http: HttpHealthIndicator, | ||
| private readonly disk: DiskHealthIndicator, | ||
| private readonly memory: MemoryHealthIndicator, | ||
| private readonly cache: CacheService, | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
HttpHealthIndicator is injected but never used.
The http dependency is declared but not called anywhere in the controller. Remove it to avoid unnecessary dependency injection overhead:
constructor(
private readonly health: HealthCheckService,
- private readonly http: HttpHealthIndicator,
private readonly disk: DiskHealthIndicator,
private readonly memory: MemoryHealthIndicator,
private readonly cache: CacheService,
) {}Also update the imports:
import {
DiskHealthIndicator,
HealthCheck,
HealthCheckService,
- HttpHealthIndicator,
MemoryHealthIndicator,
} from '@nestjs/terminus';📝 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.
Suggested change
| constructor( | |
| private readonly health: HealthCheckService, | |
| private readonly http: HttpHealthIndicator, | |
| private readonly disk: DiskHealthIndicator, | |
| private readonly memory: MemoryHealthIndicator, | |
| private readonly cache: CacheService, | |
| ) {} | |
| constructor( | |
| private readonly health: HealthCheckService, | |
| private readonly disk: DiskHealthIndicator, | |
| private readonly memory: MemoryHealthIndicator, | |
| private readonly cache: CacheService, | |
| ) {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/health.controller.ts` around lines 13 - 19, The
HttpHealthIndicator parameter is injected into the HealthController constructor
but never used; remove the unused dependency by deleting the private readonly
http: HttpHealthIndicator parameter from the constructor signature (in the class
where HealthCheckService, DiskHealthIndicator, MemoryHealthIndicator,
CacheService are declared) and also remove the corresponding HttpHealthIndicator
import from the top of the file so only used symbols (HealthCheckService,
DiskHealthIndicator, MemoryHealthIndicator, CacheService) remain imported.
Comment on lines
+36
to
+57
| private async redisHealthIndicator() { | ||
| const key = 'redis'; | ||
| const alive = await this.cache.ping(); | ||
| const metrics = this.cache.metrics; | ||
|
|
||
| if (!alive) { | ||
| return { | ||
| [key]: { | ||
| status: 'down', | ||
| degraded: true, | ||
| metrics, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| [key]: { | ||
| status: 'up', | ||
| metrics, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check CacheService implementation for ping and metrics
echo "=== CacheService public API ==="
ast-grep --pattern $'class CacheService {
$$$
}'Repository: OlufunbiIK/tip-tune
Length of output: 94
🏁 Script executed:
fd -type f -name "*.ts" | grep -i cache | head -20Repository: OlufunbiIK/tip-tune
Length of output: 233
🏁 Script executed:
fd --type f -name "*cache*.ts" | head -20Repository: OlufunbiIK/tip-tune
Length of output: 293
🏁 Script executed:
fd -t f 'cache.*\.ts'Repository: OlufunbiIK/tip-tune
Length of output: 623
🏁 Script executed:
cat -n backend/src/unify-redis/cache.service.tsRepository: OlufunbiIK/tip-tune
Length of output: 5280
🏁 Script executed:
cat -n backend/src/unify-redis/health.controller.tsRepository: OlufunbiIK/tip-tune
Length of output: 1987
🏁 Script executed:
cat -n backend/src/cache/cache.service.tsRepository: OlufunbiIK/tip-tune
Length of output: 5513
🏁 Script executed:
rg "HealthIndicatorResult" --type ts -A 5Repository: OlufunbiIK/tip-tune
Length of output: 3648
🏁 Script executed:
cat -n backend/src/health/health.controller.ts | head -100Repository: OlufunbiIK/tip-tune
Length of output: 4021
🏁 Script executed:
rg "status.*as const" backend/src/health/ -A 2 -B 2Repository: OlufunbiIK/tip-tune
Length of output: 2677
Add ping() method and public metrics getter to CacheService, and properly type the health indicator method.
The CacheService imported at line 9 (from backend/src/cache/cache.service.ts) has no public ping() method and exposes metrics only through a private property. The code at lines 38-39 will fail at runtime:
this.cache.ping()— method does not existthis.cache.metrics— property is private; onlygetMetrics()is public
Additionally, add the return type annotation and use as const on status literals to match HealthIndicatorResult:
Suggested changes
+import { HealthIndicatorResult } from '@nestjs/terminus';
- private async redisHealthIndicator() {
+ private async redisHealthIndicator(): Promise<HealthIndicatorResult> {
const key = 'redis';
const alive = await this.cache.ping();
const metrics = this.cache.metrics;
if (!alive) {
return {
[key]: {
- status: 'down',
+ status: 'down' as const,
degraded: true,
metrics,
},
};
}
return {
[key]: {
- status: 'up',
+ status: 'up' as const,
metrics,
},
};
}Update CacheService to expose ping() as a public method and either add a public getter for metrics or adjust the code to use getMetrics() instead.
🧰 Tools
🪛 GitHub Actions: Backend CI
[error] 38-38: TS2339: Property 'ping' does not exist on type 'CacheService'.
[error] 39-39: TS2341: Property 'metrics' is private in 'CacheService' and cannot be accessed from 'health.controller.ts'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/health.controller.ts` around lines 36 - 57, The
redisHealthIndicator method is calling non-public members on CacheService; add a
public async ping() method and a public metrics getter (or use the existing
getMetrics()) on CacheService, then update redisHealthIndicator to call await
this.cache.ping() and use this.cache.getMetrics() (or this.cache.metrics) and
annotate the method return type as Promise<HealthIndicatorResult> while using
status literals as const (e.g., status: 'up' as const / 'down' as const) to
satisfy typing; locate symbols redisHealthIndicator, CacheService, ping(),
getMetrics(), metrics, and HealthIndicatorResult to apply these changes.
Comment on lines
+1
to
+2
| export const REDIS_CLIENT = Symbol('REDIS_CLIENT'); | ||
| export const REDIS_OPTIONS = Symbol('REDIS_OPTIONS'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of REDIS_CLIENT injection
echo "=== String-based REDIS_CLIENT injections ==="
rg -n "@Inject\(['\"]REDIS_CLIENT['\"]" backend/src/
echo ""
echo "=== Symbol-based REDIS_CLIENT injections ==="
rg -n "REDIS_CLIENT" backend/src/ --type ts | grep -v "\.spec\." | grep -v "constants"Repository: OlufunbiIK/tip-tune
Length of output: 1856
Symbol-based tokens are a good practice, but require migration across all consumers.
Using Symbol for injection tokens avoids accidental collisions with string-based tokens. However, the existing backend/src/cache/cache.service.ts and backend/src/cache/cache.module.ts use string-based injection with 'REDIS_CLIENT'. The new backend/src/unify-redis/ module's Symbol-based approach will not be compatible with these old implementations. Additionally, backend/src/leaderboards/redis.module.ts defines its own REDIS_CLIENT as a string token, creating multiple incompatible injection tokens across the codebase. Ensure all consumers in backend/src/cache/ and verify compatibility with the leaderboards module, or the implementations will receive different (or undefined) Redis clients.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/redis.constants.ts` around lines 1 - 2, The new
Symbol tokens REDIS_CLIENT and REDIS_OPTIONS conflict with existing string
tokens; update all consumers (e.g., the current cache service/module and the
leaderboards Redis module) to import and use the exported Symbol constants
REDIS_CLIENT and REDIS_OPTIONS from the unify-redis module instead of hard-coded
string tokens ('REDIS_CLIENT' etc.), and make sure providers register the same
Symbol keys when binding the Redis client and options so DI resolves the single
shared instance across modules; if any module still defines its own
REDIS_CLIENT, remove that local string token and replace usages with the shared
Symbol import to ensure compatibility.
| import { DynamicModule, Global, Logger, Module, Provider } from '@nestjs/common'; | ||
| import { ConfigModule, ConfigService } from '@nestjs/config'; | ||
| import Redis from 'ioredis'; | ||
| import { REDIS_CLIENT, REDIS_OPTIONS, RedisModuleOptions } from './redis.constants'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all injection sites for REDIS_CLIENT to assess migration scope
rg -n "REDIS_CLIENT" --type ts -C2Repository: OlufunbiIK/tip-tune
Length of output: 11968
🏁 Script executed:
cat -n backend/src/unify-redis/redis.module.tsRepository: OlufunbiIK/tip-tune
Length of output: 4254
🌐 Web query:
ioredis reconnecting event signature
💡 Result:
The reconnecting event in ioredis is emitted after the close event when a reconnection will be made. Its signature is a single argument: the time (in milliseconds) before reconnecting. Usage example: redis.on('reconnecting', function(ms) { console.log(Reconnecting in ${ms}ms); }); This applies to both standalone Redis and Cluster instances. There is an open issue (#1817) requesting to add the attempt number as a second argument, but as of 2026-02-13, it remains unchanged.
Citations:
- 1:
reconnectingevent arguments should include the attempt number redis/ioredis#1817 - 2: https://github.com/redis/ioredis
- 3: https://zhuangya.gitbooks.io/ioredis-doc/content/doc/connection-events.html
- 4: https://ioredis.readthedocs.io/en/stable/README/
Token mismatch will cause injection failures across the codebase.
The new redis.constants.ts exports REDIS_CLIENT as Symbol('REDIS_CLIENT'), but the existing codebase (e.g., backend/src/cache/cache.module.ts, backend/src/health/health.controller.ts) uses the string literal 'REDIS_CLIENT'. Injecting via symbol won't match providers registered with a string token, causing undefined injection errors.
Either migrate all existing consumers to import the symbol, or use a string token consistently:
Proposed fix in redis.constants.ts
-export const REDIS_CLIENT = Symbol('REDIS_CLIENT');
-export const REDIS_OPTIONS = Symbol('REDIS_OPTIONS');
+export const REDIS_CLIENT = 'REDIS_CLIENT';
+export const REDIS_OPTIONS = 'REDIS_OPTIONS';Additionally, forRootAsync() exports only REDIS_CLIENT (line 63) while forRoot() exports both REDIS_CLIENT and REDIS_OPTIONS (line 31). Ensure consistency if REDIS_OPTIONS is needed downstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/unify-redis/redis.module.ts` at line 4, The project is mixing
token types causing injection failures: change the token usage in
redis.constants.ts to export REDIS_CLIENT and REDIS_OPTIONS as string literals
(e.g., 'REDIS_CLIENT', 'REDIS_OPTIONS') or alternatively update all consumers to
import the Symbol exports; pick the consistent approach (prefer keeping string
tokens to match existing consumers like cache.module.ts and
health.controller.ts). Also ensure the module factory methods are consistent:
update the RedisModule.forRootAsync implementation to export both REDIS_CLIENT
and REDIS_OPTIONS (same export set as forRoot) so downstream consumers can
inject REDIS_OPTIONS when needed; update references to RedisModuleOptions if
necessary to align with the chosen token representation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #344
Summary by CodeRabbit
New Features
Tests