Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRemoves static hidden-item env vars and config parsing, adds Redis keys, updates control page to load hidden lists, introduces hide-toggle endpoints and routes, and changes background jobs to read hidden categories/budgets from Redis at runtime. ChangesHidden items migration to Redis
Sequence Diagram(s)sequenceDiagram
participant Client
participant TokenMiddleware
participant HideToggle as HideToggleHandler
participant Redis
Client->>TokenMiddleware: GET /hide-toggle/{type}/{name}?api_token=...
TokenMiddleware->>HideToggle: invoke handler
HideToggle->>Redis: lrange(sparkleft:hidden:{categories|budgets}, 0, -1)
Redis-->>HideToggle: current list
alt name in list
HideToggle->>Redis: lrem(sparkleft:hidden:{...}, 1, name)
HideToggle-->>Client: 202 {}
else name not in list
HideToggle->>Redis: rpush(sparkleft:hidden:{...}, name)
HideToggle-->>Client: 201 {}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/endpoints/hideCategory.ts (1)
11-19: ⚖️ Poor tradeoffConsider using Redis transactions or Lua script for atomicity.
The read-check-write pattern (lines 11-19) is not atomic. If two concurrent requests toggle the same category, race conditions can cause:
- Duplicate entries in the list (if both see "not hidden" and both rpush)
- Lost updates (if operations interleave incorrectly)
While this may be unlikely in practice for this use case, using a Lua script or Redis transaction would eliminate the race window.
🔒 Proposed Lua script approach
const toggleScript = ` local key = KEYS[1] local value = ARGV[1] local hidden = redis.call('LRANGE', key, 0, -1) for i, v in ipairs(hidden) do if v == value then redis.call('LREM', key, 0, value) return 0 end end redis.call('RPUSH', key, value) return 1 ` // In the handler: const wasAdded = await connection.eval(toggleScript, 1, hiddenCategoriesKey, categoryName) const isCategoryHidden = wasAdded === 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/endpoints/hideCategory.ts` around lines 11 - 19, The current read-check-write using connection.lrange, hiddenCategoriesKey, and then connection.lrem/connection.rpush is not atomic and can race; replace that logic with a single atomic Redis operation (preferably a Lua script run via connection.eval that accepts hiddenCategoriesKey and categoryName, checks LRANGE for the value, LREM if present, otherwise RPUSH, and returns a flag) and then use the script's return value to set isCategoryHidden and the logger messages instead of the separate lrange/lrem/rpush calls; alternatively you may use a MULTI/EXEC transaction with WATCH on hiddenCategoriesKey, but ensure the code no longer performs the separate lrange followed by lrem/rpush in the handler.src/endpoints/hideBudget.ts (2)
11-19: ⚖️ Poor tradeoffConsider using Redis transactions or Lua script for atomicity.
The read-check-write pattern (lines 11-19) is not atomic. If two concurrent requests toggle the same budget, race conditions can cause:
- Duplicate entries in the list (if both see "not hidden" and both rpush)
- Lost updates (if operations interleave incorrectly)
While this may be unlikely in practice for this use case, using a Lua script or Redis transaction would eliminate the race window.
🔒 Proposed Lua script approach
const toggleScript = ` local key = KEYS[1] local value = ARGV[1] local hidden = redis.call('LRANGE', key, 0, -1) for i, v in ipairs(hidden) do if v == value then redis.call('LREM', key, 0, value) return 0 end end redis.call('RPUSH', key, value) return 1 ` // In the handler: const wasAdded = await connection.eval(toggleScript, 1, hiddenBudgetsKey, budgetName) const isBudgetHidden = wasAdded === 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/endpoints/hideBudget.ts` around lines 11 - 19, The read-check-write sequence using connection.lrange + includes + (connection.lrem / connection.rpush) is not atomic and can race; replace it with a single atomic Redis Eval (Lua) or MULTI/EXEC approach: implement a Lua script (e.g. toggleScript) that LRANGE the list, checks for budgetName, LREM if present or RPUSH if absent, and returns a flag, then call connection.eval(toggleScript, 1, hiddenBudgetsKey, budgetName) and use the returned value to set isBudgetHidden and log accordingly; update the handler to remove the existing lrange/includes/rpush/lrem calls and use the eval-based result instead.
9-9: 💤 Low valueSimplify redundant destructuring.
The destructuring
{ budgetName: budgetName }is redundant and can be simplified to{ budgetName }.✨ Simplification
- const { budgetName: budgetName } = req.params + const { budgetName } = req.params🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/endpoints/hideBudget.ts` at line 9, The destructuring line uses a redundant alias; replace "const { budgetName: budgetName } = req.params" with the shorter "const { budgetName } = req.params" to simplify the code and remove the unnecessary duplicate identifier (refer to the destructuring of req.params where budgetName is extracted).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/endpoints/hideBudget.ts`:
- Around line 8-22: The hideBudget handler lacks error handling around Redis
calls; wrap the Redis interactions (connection.lrange(hiddenBudgetsKey, ...),
connection.lrem(...), connection.rpush(...)) inside a try/catch in the
hideBudget function, log the error with logger.error including budgetName and
which operation failed, and call next(err) from the catch block so Express error
middleware handles the failure instead of proceeding to next() normally; keep
the existing success-path logs and next() only on successful completion.
In `@src/endpoints/hideCategory.ts`:
- Around line 8-22: Route params in src/server.ts (lines 56-57) are :categoryId
and :budgetId while the handlers expect categoryName/budgetName and also never
send a response; fix by updating the handlers rather than routes: in
hideCategory (src/endpoints/hideCategory.ts lines 8-22) read the id from
req.params as const { categoryId } = req.params (instead of categoryName),
map/alias it to the local variable used for storage (e.g., const categoryName =
categoryId or use categoryId consistently with
connection.lrange/connection.rpush/connection.lrem), keep the existing
hiddenCategoriesKey logic, and replace next() with res.status(200).json({
hidden: isCategoryHidden ? false : true, categoryId }); in hideBudget
(src/endpoints/hideBudget.ts lines 8-22) do the analogous change: read {
budgetId } from req.params, alias or use it in the existing hide logic, and
return res.status(200).json({ hidden: isBudgetHidden ? false : true, budgetId
}); for src/server.ts (lines 56-57) no code change required because the handlers
will now use the :categoryId and :budgetId params.
- Around line 8-22: The hideCategory function currently calls Redis methods
(connection.lrange, connection.lrem, connection.rpush) without error handling;
wrap the Redis sequence in a try/catch inside hideCategory and on error log the
error (including categoryName and hiddenCategoriesKey) and propagate the error
by calling next(err) (or pass a converted HTTP error) so the request is handled
properly instead of crashing; ensure the successful path still calls next() and
preserve existing logging messages; reference the symbols hideCategory,
connection.lrange, connection.lrem, connection.rpush, hiddenCategoriesKey.
In `@src/queues/jobs/budgetSumUp.ts`:
- Around line 61-62: The Redis call redis.lrange(hiddenBudgetsKey, 0, -1) can
throw and crash the job; wrap it in a try/catch around the call that assigns
hiddenBudgets, log the error (use the file's existing logger or console.error),
and fall back to an empty array (const hiddenBudgets = []) or implement a short
retry before falling back; ensure subsequent code that computes insights
(allInsights.filter(({ name }) => !hiddenBudgets.includes(name))) uses that safe
hiddenBudgets variable so the job continues when Redis is unavailable.
In `@src/queues/jobs/uncategorizedTransactions.ts`:
- Line 68: Wrap the Redis call that builds hiddenCategoriesSet (the expression
new Set(await redis.lrange(hiddenCategoriesKey, 0, -1))) in a try/catch so Redis
errors don't crash the uncategorized transactions job: catch exceptions from
redis.lrange, log the error, and fall back to using an empty array/set for
hiddenCategoriesSet (or implement a retry/backoff before falling back) so
processing continues when Redis is unavailable. Ensure you reference the same
symbols (redis.lrange, hiddenCategoriesKey, hiddenCategoriesSet) when making the
change.
In `@src/server.ts`:
- Around line 56-57: Add rate limiting middleware to the new hide-toggle routes:
create a limiter (e.g., hideToggleLimiter) using express-rate-limit with
sensible defaults (windowMs and max, and a throttled message) and register it on
both routes so the order is TokenMiddleware, hideToggleLimiter, then the
handler; update the two route registrations that reference TokenMiddleware,
hideCategory, and hideBudget to include hideToggleLimiter between the token
middleware and the handler.
---
Nitpick comments:
In `@src/endpoints/hideBudget.ts`:
- Around line 11-19: The read-check-write sequence using connection.lrange +
includes + (connection.lrem / connection.rpush) is not atomic and can race;
replace it with a single atomic Redis Eval (Lua) or MULTI/EXEC approach:
implement a Lua script (e.g. toggleScript) that LRANGE the list, checks for
budgetName, LREM if present or RPUSH if absent, and returns a flag, then call
connection.eval(toggleScript, 1, hiddenBudgetsKey, budgetName) and use the
returned value to set isBudgetHidden and log accordingly; update the handler to
remove the existing lrange/includes/rpush/lrem calls and use the eval-based
result instead.
- Line 9: The destructuring line uses a redundant alias; replace "const {
budgetName: budgetName } = req.params" with the shorter "const { budgetName } =
req.params" to simplify the code and remove the unnecessary duplicate identifier
(refer to the destructuring of req.params where budgetName is extracted).
In `@src/endpoints/hideCategory.ts`:
- Around line 11-19: The current read-check-write using connection.lrange,
hiddenCategoriesKey, and then connection.lrem/connection.rpush is not atomic and
can race; replace that logic with a single atomic Redis operation (preferably a
Lua script run via connection.eval that accepts hiddenCategoriesKey and
categoryName, checks LRANGE for the value, LREM if present, otherwise RPUSH, and
returns a flag) and then use the script's return value to set isCategoryHidden
and the logger messages instead of the separate lrange/lrem/rpush calls;
alternatively you may use a MULTI/EXEC transaction with WATCH on
hiddenCategoriesKey, but ensure the code no longer performs the separate lrange
followed by lrem/rpush in the handler.
🪄 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: a5afa584-56fd-419a-994d-2bc7ad81879f
📒 Files selected for processing (8)
.env.defaultsrc/config.tssrc/endpoints/hideBudget.tssrc/endpoints/hideCategory.tssrc/queues/jobs/budgetSumUp.tssrc/queues/jobs/uncategorizedTransactions.tssrc/redis.tssrc/server.ts
💤 Files with no reviewable changes (2)
- .env.default
- src/config.ts
| export async function hideBudget(req: Request<{ budgetName: string }>, _res: Response, next: NextFunction) { | ||
| const { budgetName: budgetName } = req.params | ||
| logger.info("=================================== Hiding toggle budget ===================================") | ||
| const hiddenBudgets = await connection.lrange(hiddenBudgetsKey, 0, -1) | ||
| const isBudgetHidden = hiddenBudgets.includes(budgetName) | ||
|
|
||
| if (isBudgetHidden) { | ||
| logger.info("Budget with name %s is already hidden, removing from hidden budgets", budgetName) | ||
| await connection.lrem(hiddenBudgetsKey, 0, budgetName) | ||
| } else { | ||
| logger.info("Budget with name %s is not hidden, adding to hidden budgets", budgetName) | ||
| await connection.rpush(hiddenBudgetsKey, budgetName) | ||
| } | ||
|
|
||
| next() |
There was a problem hiding this comment.
Add error handling for Redis operations.
Redis operations (lrange, lrem, rpush) can fail due to network issues, Redis being down, or connection timeouts. Without error handling, failures will crash the endpoint and return 500 errors with no useful context.
🛡️ Proposed fix
export async function hideBudget(req: Request<{ budgetName: string }>, _res: Response, next: NextFunction) {
const { budgetName: budgetName } = req.params
logger.info("=================================== Hiding toggle budget ===================================")
- const hiddenBudgets = await connection.lrange(hiddenBudgetsKey, 0, -1)
- const isBudgetHidden = hiddenBudgets.includes(budgetName)
-
- if (isBudgetHidden) {
- logger.info("Budget with name %s is already hidden, removing from hidden budgets", budgetName)
- await connection.lrem(hiddenBudgetsKey, 0, budgetName)
- } else {
- logger.info("Budget with name %s is not hidden, adding to hidden budgets", budgetName)
- await connection.rpush(hiddenBudgetsKey, budgetName)
+ try {
+ const hiddenBudgets = await connection.lrange(hiddenBudgetsKey, 0, -1)
+ const isBudgetHidden = hiddenBudgets.includes(budgetName)
+
+ if (isBudgetHidden) {
+ logger.info("Budget with name %s is already hidden, removing from hidden budgets", budgetName)
+ await connection.lrem(hiddenBudgetsKey, 0, budgetName)
+ } else {
+ logger.info("Budget with name %s is not hidden, adding to hidden budgets", budgetName)
+ await connection.rpush(hiddenBudgetsKey, budgetName)
+ }
+ } catch (err) {
+ logger.error({ err }, "Failed to toggle budget visibility for %s", budgetName)
+ throw err
}
next()📝 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.
| export async function hideBudget(req: Request<{ budgetName: string }>, _res: Response, next: NextFunction) { | |
| const { budgetName: budgetName } = req.params | |
| logger.info("=================================== Hiding toggle budget ===================================") | |
| const hiddenBudgets = await connection.lrange(hiddenBudgetsKey, 0, -1) | |
| const isBudgetHidden = hiddenBudgets.includes(budgetName) | |
| if (isBudgetHidden) { | |
| logger.info("Budget with name %s is already hidden, removing from hidden budgets", budgetName) | |
| await connection.lrem(hiddenBudgetsKey, 0, budgetName) | |
| } else { | |
| logger.info("Budget with name %s is not hidden, adding to hidden budgets", budgetName) | |
| await connection.rpush(hiddenBudgetsKey, budgetName) | |
| } | |
| next() | |
| export async function hideBudget(req: Request<{ budgetName: string }>, _res: Response, next: NextFunction) { | |
| const { budgetName: budgetName } = req.params | |
| logger.info("=================================== Hiding toggle budget ===================================") | |
| try { | |
| const hiddenBudgets = await connection.lrange(hiddenBudgetsKey, 0, -1) | |
| const isBudgetHidden = hiddenBudgets.includes(budgetName) | |
| if (isBudgetHidden) { | |
| logger.info("Budget with name %s is already hidden, removing from hidden budgets", budgetName) | |
| await connection.lrem(hiddenBudgetsKey, 0, budgetName) | |
| } else { | |
| logger.info("Budget with name %s is not hidden, adding to hidden budgets", budgetName) | |
| await connection.rpush(hiddenBudgetsKey, budgetName) | |
| } | |
| } catch (err) { | |
| logger.error({ err }, "Failed to toggle budget visibility for %s", budgetName) | |
| throw err | |
| } | |
| next() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/endpoints/hideBudget.ts` around lines 8 - 22, The hideBudget handler
lacks error handling around Redis calls; wrap the Redis interactions
(connection.lrange(hiddenBudgetsKey, ...), connection.lrem(...),
connection.rpush(...)) inside a try/catch in the hideBudget function, log the
error with logger.error including budgetName and which operation failed, and
call next(err) from the catch block so Express error middleware handles the
failure instead of proceeding to next() normally; keep the existing success-path
logs and next() only on successful completion.
| const hiddenBudgets = await redis.lrange(hiddenBudgetsKey, 0, -1) | ||
| const insights = allInsights.filter(({ name }) => !hiddenBudgets.includes(name)) |
There was a problem hiding this comment.
Add error handling for Redis operations in the job.
If Redis is unavailable or times out, the lrange operation will throw an exception and crash the entire budget sum-up job. This prevents legitimate budget notifications from being sent.
Consider wrapping the Redis call in error handling with a fallback strategy (e.g., proceed with no hidden budgets, or retry).
🛡️ Proposed fix with fallback
- const hiddenBudgets = await redis.lrange(hiddenBudgetsKey, 0, -1)
- const insights = allInsights.filter(({ name }) => !hiddenBudgets.includes(name))
+ let hiddenBudgets: string[] = []
+ try {
+ hiddenBudgets = await redis.lrange(hiddenBudgetsKey, 0, -1)
+ } catch (err) {
+ this.logger.error({ err }, "Failed to fetch hidden budgets from Redis, proceeding with no filtering")
+ }
+ const insights = allInsights.filter(({ name }) => !hiddenBudgets.includes(name))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/queues/jobs/budgetSumUp.ts` around lines 61 - 62, The Redis call
redis.lrange(hiddenBudgetsKey, 0, -1) can throw and crash the job; wrap it in a
try/catch around the call that assigns hiddenBudgets, log the error (use the
file's existing logger or console.error), and fall back to an empty array (const
hiddenBudgets = []) or implement a short retry before falling back; ensure
subsequent code that computes insights (allInsights.filter(({ name }) =>
!hiddenBudgets.includes(name))) uses that safe hiddenBudgets variable so the job
continues when Redis is unavailable.
| const billsBudgetName = await getBudgetName(env.billsBudgetId) | ||
| const { data: allCategories } = await CategoriesService.listCategory({ client, query: { page: 1, limit: 50 } }) | ||
| const hiddenCategoriesSet = new Set(env.hiddenCategories) | ||
| const hiddenCategoriesSet = new Set(await redis.lrange(hiddenCategoriesKey, 0, -1)) |
There was a problem hiding this comment.
Add error handling for Redis operations in the job.
If Redis is unavailable or times out, the lrange operation will throw an exception and crash the uncategorized transactions job. This prevents notification messages from being created for uncategorized transactions.
Consider wrapping the Redis call in error handling with a fallback strategy (e.g., proceed with no hidden categories, or retry).
🛡️ Proposed fix with fallback
- const hiddenCategoriesSet = new Set(await redis.lrange(hiddenCategoriesKey, 0, -1))
+ let hiddenCategoriesSet: Set<string>
+ try {
+ hiddenCategoriesSet = new Set(await redis.lrange(hiddenCategoriesKey, 0, -1))
+ } catch (err) {
+ logger.error({ err }, "Failed to fetch hidden categories from Redis, proceeding with no filtering")
+ hiddenCategoriesSet = new Set()
+ }
const categories = allCategories.filter(({ attributes: { name } }) => name !== billsBudgetName && !hiddenCategoriesSet.has(name))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/queues/jobs/uncategorizedTransactions.ts` at line 68, Wrap the Redis call
that builds hiddenCategoriesSet (the expression new Set(await
redis.lrange(hiddenCategoriesKey, 0, -1))) in a try/catch so Redis errors don't
crash the uncategorized transactions job: catch exceptions from redis.lrange,
log the error, and fall back to using an empty array/set for hiddenCategoriesSet
(or implement a retry/backoff before falling back) so processing continues when
Redis is unavailable. Ensure you reference the same symbols (redis.lrange,
hiddenCategoriesKey, hiddenCategoriesSet) when making the change.
| app.get("/hide-toggle/category/:categoryId", TokenMiddleware, hideCategory) | ||
| app.get("/hide-toggle/budget/:budgetId", TokenMiddleware, hideBudget) |
There was a problem hiding this comment.
Add rate limiting to the new hide-toggle endpoints.
CodeQL correctly identifies that these routes perform database (Redis) operations without rate limiting. An attacker with a valid API token could flood these endpoints, causing Redis connection exhaustion or performance degradation.
Consider adding rate limiting middleware before these routes, similar to how other mutation endpoints might be protected.
🛡️ Example rate limiting approach
Using a package like express-rate-limit:
import rateLimit from 'express-rate-limit'
const hideToggleLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // limit each IP to 100 requests per windowMs
message: 'Too many hide/unhide requests, please try again later'
})
app.get("/hide-toggle/category/:categoryId", TokenMiddleware, hideToggleLimiter, hideCategory)
app.get("/hide-toggle/budget/:budgetId", TokenMiddleware, hideToggleLimiter, hideBudget)🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 57-57: Avoid using unsanitized user input with sendFile
Context: app.post("/webhook", verifyWebhookMiddleware, webhook)
Note: Security best practice.
(external-filename-upload-typescript)
[warning] 56-56: Avoid allowing access to unintended directories or files
Context: app.get("/hide-toggle/budget/:budgetId", TokenMiddleware, hideBudget)
Note: Security best practice.
(path-traversal-typescript)
🪛 GitHub Check: CodeQL
[failure] 56-56: Missing rate limiting
This route handler performs a database access, but is not rate-limited.
This route handler performs a database access, but is not rate-limited.
This route handler performs a database access, but is not rate-limited.
[failure] 57-57: Missing rate limiting
This route handler performs a database access, but is not rate-limited.
This route handler performs a database access, but is not rate-limited.
This route handler performs a database access, but is not rate-limited.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.ts` around lines 56 - 57, Add rate limiting middleware to the new
hide-toggle routes: create a limiter (e.g., hideToggleLimiter) using
express-rate-limit with sensible defaults (windowMs and max, and a throttled
message) and register it on both routes so the order is TokenMiddleware,
hideToggleLimiter, then the handler; update the two route registrations that
reference TokenMiddleware, hideCategory, and hideBudget to include
hideToggleLimiter between the token middleware and the handler.
Source: Linters/SAST tools
| settingCategoryForTransaction, | ||
| TransactionResultMiddleware, | ||
| ) | ||
| app.get("/hide-toggle/category/:categoryName", TokenMiddleware, hideCategory) |
| TransactionResultMiddleware, | ||
| ) | ||
| app.get("/hide-toggle/category/:categoryName", TokenMiddleware, hideCategory) | ||
| app.get("/hide-toggle/budget/:budgetName", TokenMiddleware, hideBudget) |
Summary by CodeRabbit
New Features
Style