fix: Attempted fix for dashboard not loading sometimes#1109
Open
fix: Attempted fix for dashboard not loading sometimes#1109
Conversation
Contributor
📝 WalkthroughWalkthroughModified in-flight method call tracking in the Redis module from a per-user Set of method names to a per-user map of method name → Promise. Duplicate concurrent calls now return the existing Promise instead of throwing; types and cleanup logic adjusted accordingly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
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 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@redis/index.ts`:
- Around line 97-100: The code double-awaits and miscasts the resolved value:
change the return to cast the stored promise before awaiting so you await the
Promise<T>, not assert the resolved value as a promise; locate the pendingCalls
lookup (this.pendingCalls[userId][methodName]) and replace `return await (await
existingCall as Promise<T>)` with `return await (existingCall as Promise<T>)`
(or simply `return await existingCall` with the appropriate generic typing) to
ensure the cast applies to the Promise, not its resolved value.
- Around line 108-113: The code sets this.pendingCalls[userId][methodName] =
undefined which leaves the property present so Object.keys(...) never becomes
zero; change the removal to delete this.pendingCalls[userId][methodName] (in the
block that checks this.pendingCalls[userId]?.[methodName] === pendingCall) so
the property is actually removed and the subsequent cleanup check for
Object.keys(this.pendingCalls[userId]).length === 0 in the pendingCalls cleanup
logic will work as intended.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
redis/index.ts (1)
108-113:⚠️ Potential issue | 🟠 MajorCleanup logic still keeps stale keys and prevents bucket removal.
Line 109 assigns
undefinedinstead of removing the property, soObject.keys(...)at Line 111 still sees the key. Also, Line 112 setting{}keeps an empty user bucket around.🐛 Proposed cleanup fix
if (this.pendingCalls[userId]?.[methodName] === pendingCall) { - this.pendingCalls[userId][methodName] = undefined + delete this.pendingCalls[userId][methodName] } if (this.pendingCalls[userId] !== undefined && Object.keys(this.pendingCalls[userId]).length === 0) { - this.pendingCalls[userId] = {} + delete this.pendingCalls[userId] }#!/bin/bash # Read-only verification: confirm cleanup uses assignment-to-undefined and empty-object retention. rg -nP 'pendingCalls\[userId\]\[methodName\]\s*=\s*undefined|delete\s+this\.pendingCalls\[userId\]\[methodName\]|Object\.keys\(this\.pendingCalls\[userId\]\)\.length\s*===\s*0|this\.pendingCalls\[userId\]\s*=\s*\{\}' redis/index.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@redis/index.ts` around lines 108 - 113, The cleanup leaves stale keys because code sets this.pendingCalls[userId][methodName] = undefined and later assigns an empty object; change the logic to delete the method entry and remove the user bucket when empty: replace the assignment-to-undefined with delete this.pendingCalls[userId][methodName] (use the same userId, methodName, pendingCall check) and when Object.keys(this.pendingCalls[userId]).length === 0 call delete this.pendingCalls[userId] instead of assigning {} so no empty user bucket remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@redis/index.ts`:
- Around line 97-103: The dedupe currently keys in-flight requests only by
this.pendingCalls[userId][methodName], which causes different-argument calls
(e.g., dashboardData with different timezone/buttonIds) to incorrectly share a
Promise; change the keying to include an argument-aware dedupe key: add or
compute a dedupeKey parameter (e.g.,
`${userId}:${timezone}:${buttonIds?.join(',')}` or `${userId}:${timezone}`) and
use this.pendingCalls[userId][dedupeKey] (or a nested Map keyed by methodName
then dedupeKey) when reading/setting existingCall and pendingCall; update the
call sites that invoke executeCall to pass the appropriate dedupeKey for
dashboardData/paymentsCount so concurrent calls with different args do not
collide.
---
Duplicate comments:
In `@redis/index.ts`:
- Around line 108-113: The cleanup leaves stale keys because code sets
this.pendingCalls[userId][methodName] = undefined and later assigns an empty
object; change the logic to delete the method entry and remove the user bucket
when empty: replace the assignment-to-undefined with delete
this.pendingCalls[userId][methodName] (use the same userId, methodName,
pendingCall check) and when Object.keys(this.pendingCalls[userId]).length === 0
call delete this.pendingCalls[userId] instead of assigning {} so no empty user
bucket remains.
Comment on lines
+97
to
+103
| const existingCall = this.pendingCalls[userId][methodName] | ||
| if (existingCall !== undefined) { | ||
| return await (existingCall as Promise<T>) | ||
| } | ||
|
|
||
| this.pendingCalls[userId].add(methodName) | ||
| const pendingCall = fn() | ||
| this.pendingCalls[userId][methodName] = pendingCall as Promise<unknown> |
Contributor
There was a problem hiding this comment.
Deduplication key is not argument-safe for dashboardData/paymentsCount.
At Line 97, in-flight reuse is keyed only by [userId][methodName]. Concurrent requests with different timezone/buttonIds can incorrectly share one Promise and return the wrong payload.
💡 Suggested fix (argument-aware keying)
- private static async executeCall<T>(
- userId: string,
- methodName: MethodName,
- fn: () => Promise<T>
- ): Promise<T> {
+ private static async executeCall<T>(
+ scopeId: string,
+ methodName: MethodName,
+ fn: () => Promise<T>
+ ): Promise<T> {
- if (this.pendingCalls[userId] === undefined) {
- this.pendingCalls[userId] = {}
+ if (this.pendingCalls[scopeId] === undefined) {
+ this.pendingCalls[scopeId] = {}
}
- const existingCall = this.pendingCalls[userId][methodName]
+ const existingCall = this.pendingCalls[scopeId][methodName]
if (existingCall !== undefined) {
return await (existingCall as Promise<T>)
}
const pendingCall = fn()
- this.pendingCalls[userId][methodName] = pendingCall as Promise<unknown>
+ this.pendingCalls[scopeId][methodName] = pendingCall as Promise<unknown>// call-site examples
return await this.executeCall(`${userId}:${timezone}:${buttonIds?.join(',') ?? ''}`, 'dashboardData', async () => ...)
return await this.executeCall(`${userId}:${timezone}`, 'paymentsCount', async () => ...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@redis/index.ts` around lines 97 - 103, The dedupe currently keys in-flight
requests only by this.pendingCalls[userId][methodName], which causes
different-argument calls (e.g., dashboardData with different timezone/buttonIds)
to incorrectly share a Promise; change the keying to include an argument-aware
dedupe key: add or compute a dedupeKey parameter (e.g.,
`${userId}:${timezone}:${buttonIds?.join(',')}` or `${userId}:${timezone}`) and
use this.pendingCalls[userId][dedupeKey] (or a nested Map keyed by methodName
then dedupeKey) when reading/setting existingCall and pendingCall; update the
call sites that invoke executeCall to pass the appropriate dedupeKey for
dashboardData/paymentsCount so concurrent calls with different args do not
collide.
Collaborator
|
Not sure about the fix. The root cause seems that either the dashboard cache request is emitted concurrently, which is dubious, or that a call is missing an await. |
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.
Related to #1098
Description
Sometimes (especially after a re-deploy) the dashboard doesn't load without doing an extra refresh.
Test plan
I'm not 100% how to reproduce it without trying this for real in prod but perhaps there is a good way? We can also give it a quick deploy in prod together.
Summary by CodeRabbit
Bug Fixes
Refactor