Increase default stop arrival fetch count - #30
Conversation
📝 WalkthroughWalkthroughAdds a new constant Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/modules/stops/stops.service.ts (1)
184-188:⚠️ Potential issue | 🟠 MajorNormalize
limitbefore building the cache key.Line [187] keys by raw
limit, but Line [328] caps it. This can fragment cache entries and waste Redis memory for identical result sets.Proposed fix
async getArrivals( stopId: string, agencyKey: string, limit = DEFAULT_STOP_SEARCH_LIMIT, after?: string, ): Promise<{ data: ArrivalResponse[]; stopId: string; agencyKey: string; stopName: string }> { - const cacheKey = `cache:arrivals:v3:${agencyKey}:${stopId}:${limit}:${after ?? 'now'}`; + const cappedLimit = Math.min(limit, MAX_SEARCH_LIMIT); + const cacheKey = `cache:arrivals:v3:${agencyKey}:${stopId}:${cappedLimit}:${after ?? 'now'}`; @@ - Math.min(limit, MAX_SEARCH_LIMIT), + cappedLimit, ], );Also applies to: 328-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/modules/stops/stops.service.ts` around lines 184 - 188, Normalize the incoming limit before building cacheKey to avoid fragmented Redis entries: compute a normalizedLimit using the same capping logic applied later (e.g., normalizedLimit = Math.min(limit, DEFAULT_STOP_SEARCH_LIMIT) or call the existing cap used around lines 328-329), then use normalizedLimit (not raw limit) when creating cacheKey (`cache:arrivals:v3:${agencyKey}:${stopId}:${normalizedLimit}:${after ?? 'now'}`) so cache keys match the actual query size used by the rest of the function.
🧹 Nitpick comments (1)
backend/src/common/constants.ts (1)
36-36: Clarify constant intent to avoid misuse.
DEFAULT_STOP_SEARCH_LIMITis easy to confuse with the existing search-page default. Consider renaming it to an arrivals-specific name (or add a clear comment) so future call sites don’t apply it to the wrong endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/common/constants.ts` at line 36, The constant DEFAULT_STOP_SEARCH_LIMIT is ambiguous and may be misused; rename it to something arrivals-specific (e.g., ARRIVALS_DEFAULT_SEARCH_LIMIT) or add a clear inline comment above DEFAULT_STOP_SEARCH_LIMIT clarifying it's only for arrivals/stop-arrivals endpoint; update all references/usages (imports, tests, and any modules that consume DEFAULT_STOP_SEARCH_LIMIT) to the new name or rely on the clarified comment so callers for the search-page default are not accidentally using this value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/src/modules/stops/stops.service.ts`:
- Around line 184-188: Normalize the incoming limit before building cacheKey to
avoid fragmented Redis entries: compute a normalizedLimit using the same capping
logic applied later (e.g., normalizedLimit = Math.min(limit,
DEFAULT_STOP_SEARCH_LIMIT) or call the existing cap used around lines 328-329),
then use normalizedLimit (not raw limit) when creating cacheKey
(`cache:arrivals:v3:${agencyKey}:${stopId}:${normalizedLimit}:${after ??
'now'}`) so cache keys match the actual query size used by the rest of the
function.
---
Nitpick comments:
In `@backend/src/common/constants.ts`:
- Line 36: The constant DEFAULT_STOP_SEARCH_LIMIT is ambiguous and may be
misused; rename it to something arrivals-specific (e.g.,
ARRIVALS_DEFAULT_SEARCH_LIMIT) or add a clear inline comment above
DEFAULT_STOP_SEARCH_LIMIT clarifying it's only for arrivals/stop-arrivals
endpoint; update all references/usages (imports, tests, and any modules that
consume DEFAULT_STOP_SEARCH_LIMIT) to the new name or rely on the clarified
comment so callers for the search-page default are not accidentally using this
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd547dec-b9e4-44ab-ba3e-b401e1cc46b8
📒 Files selected for processing (2)
backend/src/common/constants.tsbackend/src/modules/stops/stops.service.ts
Summary
Technical details:
Why:
Testing:
Closes #30 if applicable.