fix: Attempted fix for dashboard not loading sometimes#1109
fix: Attempted fix for dashboard not loading sometimes#1109
Conversation
📝 WalkthroughWalkthroughModified in-flight method call tracking in Redis module from Set-based approach with error-throwing to Promise-mapping with deduplication. Concurrent calls now return the existing promise instead of throwing, enabling call deduplication. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 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.
| const existingCall = this.pendingCalls[userId][methodName] | ||
| if (existingCall !== undefined) { | ||
| return await (await existingCall as Promise<T>) | ||
| } |
There was a problem hiding this comment.
Double-await produces an incorrect type assertion on the resolved value, not the Promise.
return await (await existingCall as Promise<T>)The inner await existingCall already resolves Promise<unknown> to the plain T value. The subsequent as Promise<T> then incorrectly asserts that plain T value (e.g. a DashboardData object) as a Promise<T>, and the outer await re-awaits it. This only works by coincidence because await nonThenable returns the value unchanged; if T were itself a PromiseLike, the second await would unwrap it one extra level, producing Awaited<T> instead of T.
The cast should be applied to existingCall (the promise) before awaiting:
🐛 Proposed fix
- return await (await existingCall as Promise<T>)
+ return await (existingCall as Promise<T>)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@redis/index.ts` around lines 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.
| if (this.pendingCalls[userId]?.[methodName] === pendingCall) { | ||
| this.pendingCalls[userId][methodName] = undefined | ||
| } | ||
| if (this.pendingCalls[userId] !== undefined && Object.keys(this.pendingCalls[userId]).length === 0) { | ||
| this.pendingCalls[userId] = {} | ||
| } |
There was a problem hiding this comment.
= undefined leaves the key in the object, making the size-zero check permanently dead code.
this.pendingCalls[userId][methodName] = undefined does not remove the key — Object.keys(obj) still returns it. After any completed call, Object.keys(this.pendingCalls[userId]).length is always ≥ 1, so the second if branch (lines 111-113) can never fire.
Use delete to actually remove the key so that the cleanup branch works as intended:
🐛 Proposed 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] = {}
}🤖 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 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.
1 participant
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