fix: dedupe concurrent getNodeAjaxOptions requests (#10155)#10159
Open
hiteshjambhale wants to merge 1 commit into
Open
fix: dedupe concurrent getNodeAjaxOptions requests (#10155)#10159hiteshjambhale wants to merge 1 commit into
hiteshjambhale wants to merge 1 commit into
Conversation
getNodeAjaxOptions cached responses only after they resolved, so concurrent callers for the same URL (e.g. every column row's Data Type dropdown mounting at once) all saw an empty cache and each fired its own identical HTTP GET. Track in-flight requests in a module-level Map keyed by the resolved URL and query params. Concurrent callers now await the same underlying request; the first to resolve populates the cache as before, and the entry is cleaned up on settle so the map does not leak. Fixes pgadmin-org#10155 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughChangesAJAX request deduplication
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant getNodeAjaxOptions
participant InflightMap
participant api.get
Caller->>getNodeAjaxOptions: Request node options
getNodeAjaxOptions->>InflightMap: Check URL and params key
alt No pending request
getNodeAjaxOptions->>api.get: Fetch fullUrl with params
api.get-->>getNodeAjaxOptions: Return response
getNodeAjaxOptions->>InflightMap: Remove completed request
else Pending request exists
InflightMap-->>getNodeAjaxOptions: Return shared Promise
end
getNodeAjaxOptions-->>Caller: Resolve transformed options
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🤖 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 `@web/pgadmin/browser/static/js/node_ajax.js`:
- Around line 137-145: Move the cacheNode.cache invocation out of the shared
api.get promise callback and into the per-caller request.then block before
transform(resData). Use each caller’s own otherParams, cacheNode, nodeObj, url,
treeNodeInfo, and cacheLevel values so concurrent requests apply independent
cache logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27d542f2-1ebc-4cbb-881c-b8a2f93d5bc4
📒 Files selected for processing (1)
web/pgadmin/browser/static/js/node_ajax.js
Comment on lines
+137
to
+145
| otherParams.useCache && cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData); | ||
| return resData; | ||
| }).finally(()=>{ | ||
| _inflightAjaxRequests.delete(inflightKey); | ||
| }); | ||
| _inflightAjaxRequests.set(inflightKey, request); | ||
| } | ||
| request.then((resData)=>{ | ||
| resolve(transform(resData)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move cache population outside the shared promise to avoid closure capture of the first caller's arguments.
The shared api.get(...).then(...) block captures the closure variables (otherParams, cacheNode, nodeObj, url, treeNodeInfo, cacheLevel) of the first caller. If concurrent callers request the same URL but differ in parameters (e.g., different nodeObj.type or useCache settings), the cache will only be populated for the first caller's key.
Moving the cacheNode.cache call into the individual request.then block ensures that every concurrent caller correctly executes its own caching logic and populates its respective cache entry before applying its transformation.
🐛 Proposed fix
- otherParams.useCache && cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData);
return resData;
}).finally(()=>{
_inflightAjaxRequests.delete(inflightKey);
});
_inflightAjaxRequests.set(inflightKey, request);
}
request.then((resData)=>{
+ if (otherParams.useCache) {
+ cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData);
+ }
resolve(transform(resData));📝 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
| otherParams.useCache && cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData); | |
| return resData; | |
| }).finally(()=>{ | |
| _inflightAjaxRequests.delete(inflightKey); | |
| }); | |
| _inflightAjaxRequests.set(inflightKey, request); | |
| } | |
| request.then((resData)=>{ | |
| resolve(transform(resData)); | |
| return resData; | |
| }).finally(()=>{ | |
| _inflightAjaxRequests.delete(inflightKey); | |
| }); | |
| _inflightAjaxRequests.set(inflightKey, request); | |
| } | |
| request.then((resData)=>{ | |
| if (otherParams.useCache) { | |
| cacheNode.cache(nodeObj.type + '#' + url, treeNodeInfo, cacheLevel, resData); | |
| } | |
| resolve(transform(resData)); |
🤖 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 `@web/pgadmin/browser/static/js/node_ajax.js` around lines 137 - 145, Move the
cacheNode.cache invocation out of the shared api.get promise callback and into
the per-caller request.then block before transform(resData). Use each caller’s
own otherParams, cacheNode, nodeObj, url, treeNodeInfo, and cacheLevel values so
concurrent requests apply independent cache logic.
mzabuawala
reviewed
Jul 17, 2026
| // Share a single in-flight request among all concurrent callers asking | ||
| // for the same URL + params, so we don't fire duplicate GETs before the | ||
| // first response lands and populates the cache. | ||
| let inflightKey = fullUrl + '#' + JSON.stringify(otherParams.urlParams || {}); |
Contributor
There was a problem hiding this comment.
Some observations -
- Not the best way to create a unique key
{
a:1,
b:2
}
vs
{
b:2,
a:1
}
And a nested object will make it worse.
- A better place to put such logic is inside the Axios wrapper, so that it will be used by the entire app
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.
Summary
Fixes #10155.
getNodeAjaxOptions()inweb/pgadmin/browser/static/js/node_ajax.js— the shared helper behind every AJAX-backed dropdown — cached responses only after they resolved. So when many components requested the same URL before the first response landed (e.g. every column row's Data Type dropdown mounting at once on a wide table's Columns tab), each one saw an empty cache and fired its own identicalGET. A 150-column table produced ~150 concurrent duplicateget_typesrequests.Fix
Added in-flight request sharing:
Maptracks requests currently in progress, keyed by the resolved full URL + query params..finally()), success or failure, so it doesn't leak.Result: one shared
get_typesrequest for all concurrent callers instead of one per row. Cached behavior and response-shape handling are unchanged.Notes
Could not run the JS linter locally — the repo's
.eslintrc.js(legacy format) is incompatible with the installed ESLint v9 (flat-config only), a pre-existing toolchain issue unrelated to this change. The file was verified to parse as valid ES module syntax.🤖 Generated with Claude Code
Summary by CodeRabbit