fix: dedupe concurrent getNodeAjaxOptions requests (#10155)#10159
fix: dedupe concurrent getNodeAjaxOptions requests (#10155)#10159hiteshjambhale wants to merge 2 commits 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>
WalkthroughConcurrent GET requests from ChangesAJAX request deduplication
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant getNodeAjaxOptions
participant getInflight
participant api.get
Caller->>getNodeAjaxOptions: Request node options
getNodeAjaxOptions->>getInflight: Request fullUrl with params
alt No matching pending request
getInflight->>api.get: Start GET request
api.get-->>getInflight: Return response
getInflight-->>getNodeAjaxOptions: Shared response promise
else Matching pending request
getInflight-->>getNodeAjaxOptions: Reuse shared promise
end
getNodeAjaxOptions-->>Caller: Resolve transformed options
🚥 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
| // 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 || {}); |
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
|
@hiteshjambhale fix is working, we only get one API call for column details instead of N(number of columns). |
Address review feedback on the getNodeAjaxOptions dedup:
- Move the in-flight request sharing out of node_ajax.js and into
api_instance.js as a reusable getInflight() helper, so any part of
the app can dedupe identical concurrent GETs, not just node options.
- Build the in-flight key with a stable, key-order-independent
stringify (sorted keys, recursive) so {a:1,b:2} and {b:2,a:1} and
nested params resolve to the same key.
- Run caching and transform per-caller in each caller's own .then on
the shared response, so concurrent callers with differing params
each cache correctly instead of inheriting the first caller's.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/static/js/api_instance.js`:
- Line 31: Update the in-flight request key generated by getInflight in
api_instance.js to include the effective Axios/API request context, not only the
URL and params. Ensure requests using different caller-specific headers,
authorization/CSRF values, or runtime options cannot share cached results, while
preserving deduplication for identical request contexts.
- Around line 56-59: Update getInflight() so each shared Axios GET request uses
a finite timeout or cancellation mechanism before being stored in
_inflightGetRequests. Ensure the map entry is removed when the request aborts,
rejects, or resolves, preventing stalled requests from retaining the key
indefinitely.
- Around line 35-44: Update stableStringify, used by getInflight, to produce
collision-free keys for supported parameter values and reject unsupported
config.params inputs, including undefined-containing arrays, Date, Map, and
cyclic structures. Preserve deterministic object-key ordering while ensuring
distinct JSON-like types cannot serialize to the same key; add coverage for
these collision and rejection cases.
🪄 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 Plus
Run ID: 28f67f76-5bdb-4492-8df2-e84e32878bcb
📒 Files selected for processing (2)
web/pgadmin/browser/static/js/node_ajax.jsweb/pgadmin/static/js/api_instance.js
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/browser/static/js/node_ajax.js
| * (e.g. every column row's Data Type dropdown mounting at once on a wide table) | ||
| * share a single HTTP request instead of each firing their own identical GET. | ||
| */ | ||
| const _inflightGetRequests = new Map(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
rg -n -C3 '\b(getInflight|getApiInstance)\s*\(' web/pgadmin --glob '*.js'
rg -n -C2 '\b(Authorization|headers|timeout|responseType|signal)\b' web/pgadmin --glob '*.js'Repository: pgadmin-org/pgadmin4
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== api_instance.js =="
cat -n web/pgadmin/static/js/api_instance.js
echo
echo "== node_ajax relevant section =="
cat -n web/pgadmin/browser/static/js/node_ajax.js | sed -n '70,135p'
echo
echo "== focused getInflight usages =="
rg -n -C2 'getInflight\(' web/pgadmin --glob '*.js'Repository: pgadmin-org/pgadmin4
Length of output: 8914
Include the effective API/request context in the in-flight key.
node_ajax.js only passes URL params to getInflight(), while api_instance.js ignores the api argument and only includes url + params in the cache key. Since getApiInstance(headers={}) creates a separate Axios instance for each call and can layer caller-specific headers, concurrent requests with different authorization/CSRF or runtime options can share the first settled response. Include the effective request context in the key, or use one shared default Axios client and merge only request-specific config.
🤖 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/static/js/api_instance.js` at line 31, Update the in-flight
request key generated by getInflight in api_instance.js to include the effective
Axios/API request context, not only the URL and params. Ensure requests using
different caller-specific headers, authorization/CSRF values, or runtime options
cannot share cached results, while preserving deduplication for identical
request contexts.
| function stableStringify(value) { | ||
| if (value === null || typeof value !== 'object') { | ||
| return JSON.stringify(value); | ||
| } | ||
| if (Array.isArray(value)) { | ||
| return '[' + value.map(stableStringify).join(',') + ']'; | ||
| } | ||
| return '{' + Object.keys(value).sort().map( | ||
| (key) => JSON.stringify(key) + ':' + stableStringify(value[key]) | ||
| ).join(',') + '}'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file locations =="
git ls-files | rg '(^|/)api_instance\.js$|pgadmin|static/js' || true
echo "== target excerpt =="
if [ -f web/pgadmin/static/js/api_instance.js ]; then
nl -ba web/pgadmin/static/js/api_instance.js | sed -n '1,90p'
fi
echo "== usages of stableStringify / config.params =="
rg -n "stableStringify|config\.params|params" web/pgadmin/static/js/api_instance.js web/pgadmin 2>/dev/null | head -200Repository: pgadmin-org/pgadmin4
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target excerpt =="
nl -ba web/pgadmin/static/js/api_instance.js | sed -n '1,90p'
echo "== stableStringify / config.params usages in api_instance.js only =="
rg -n "stableStringify|config\.params|params" web/pgadmin/static/js/api_instance.js || true
echo "== direct JS behavioral probe =="
node - <<'JS'
function stableStringify(value) {
if (value === null || typeof value !== 'object') {
return JSON.stringify(value);
}
if (Array.isArray(value)) {
return '[' + value.map(stableStringify).join(',') + ']';
}
return '{' + Object.keys(value).sort().map(
(key) => JSON.stringify(key) + ':' + stableStringify(value[key])
).join(',') + '}';
}
const cases = [
[undefined],
[],
[null],
new Date(),
new Map(),
{ b: 1, a: 2 },
];
for (const v of cases) {
try {
console.log(JSON.stringify([v]), '=>', stableStringify(v));
} catch (err) {
console.log(JSON.stringify(v), '=> throws', err.name + ': ' + err.message);
}
}
const cyclic = {};
cyclic.self = cyclic;
try {
console.log('cyclic:', stableStringify(cyclic));
} catch (err) {
console.log('cyclic: throws', err.name + ': ' + err.message);
}
JSRepository: pgadmin-org/pgadmin4
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target excerpt =="
awk '{printf "%6d\t%s\n", NR, $0}' web/pgadmin/static/js/api_instance.js | sed -n '1,90p' || true
echo "== stableStringify / config.params usages in api_instance.js only =="
grep -nE "stableStringify|config\.params|params" web/pgadmin/static/js/api_instance.js || true
echo "== direct JS behavioral probe =="
node - <<'JS'
function stableStringify(value) {
if (value === null || typeof value !== 'object') {
return JSON.stringify(value);
}
if (Array.isArray(value)) {
return '[' + value.map(stableStringify).join(',') + ']';
}
return '{' + Object.keys(value).sort().map(
(key) => JSON.stringify(key) + ':' + stableStringify(value[key])
).join(',') + '}';
}
const cases = [
[undefined],
[],
[null],
new Date(),
new Map(),
{ b: 1, a: 2 },
];
for (const v of cases) {
try {
console.log(JSON.stringify([v]), '=>', stableStringify(v));
} catch (err) {
console.log(JSON.stringify(v), '=> throws', err.name + ': ' + err.message);
}
}
const cyclic = {};
cyclic.self = cyclic;
try {
console.log('cyclic:', stableStringify(cyclic));
} catch (err) {
console.log('cyclic: throws', err.name + ': ' + err.message);
}
JSRepository: pgadmin-org/pgadmin4
Length of output: 5026
Avoid parameter serialization collisions in getInflight.
stableStringify([undefined]) produces undefined and stableStringify([]) produces ``, so both serialize to []; arrays with no enumerable keys, `Date`, and `Map` also collapse to `{}`. Keys with identical enumerable contents but different types therefore collide, and cyclic `config.params` throws before the request starts. Restrict params to documented JSON-like values and reject unsupported inputs, or add tagged handling plus collision coverage.
🤖 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/static/js/api_instance.js` around lines 35 - 44, Update
stableStringify, used by getInflight, to produce collision-free keys for
supported parameter values and reject unsupported config.params inputs,
including undefined-containing arrays, Date, Map, and cyclic structures.
Preserve deterministic object-key ordering while ensuring distinct JSON-like
types cannot serialize to the same key; add coverage for these collision and
rejection cases.
| request = api.get(url, config).finally(() => { | ||
| _inflightGetRequests.delete(key); | ||
| }); | ||
| _inflightGetRequests.set(key, request); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C3 '\b(timeout|AbortController|signal)\b' \
web/pgadmin/static/js/api_instance.js \
web/pgadmin/browser/static/js/node_ajax.jsRepository: pgadmin-org/pgadmin4
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate relevant files =="
git ls-files | fgrep -E '(^|/)api_instance\.js$|node_ajax\.js$|axios|api' | sed -n '1,200p'
echo
echo "== api_instance.js outline/size/contents =="
wc -l web/pgadmin/static/js/api_instance.js
sed -n '1,220p' web/pgadmin/static/js/api_instance.js
echo
echo "== node_ajax.js relevant symbols/size =="
wc -l web/pgadmin/browser/static/js/node_ajax.js
sed -n '1,220p' web/pgadmin/browser/static/js/node_ajax.js
echo
echo "== all getApiInstance calls =="
rg -n -C2 'getApiInstance|_inflightGetRequests|apiInstance|axios\.get' web/pgadmin/static/js web/pgadmin -g '*.js'Repository: pgadmin-org/pgadmin4
Length of output: 50376
🌐 Web query:
Axios defaults request timeout default 0 infinite
💡 Result:
The default value for the Axios request timeout is 0 [1][2]. In Axios, a timeout value of 0 means there is no timeout, effectively causing the request to wait indefinitely [1][3][4]. It is highly recommended to set an explicit timeout value in production environments [1]. Without a defined timeout, a stalled or hanging request can remain open indefinitely, which may lead to resource exhaustion or unexpected application behavior [1][3]. When a request exceeds the specified timeout duration, Axios aborts the request and throws an error with the code 'ECONNABORTED' [1][4]. Note that the standard timeout property primarily governs response timeouts; it may not always account for connection-level issues, for which alternative approaches like AbortSignal are recommended [5].
Citations:
- 1: https://github.com/axios/axios
- 2: https://stackoverflow.com/questions/57995863/what-is-axios-default-timeout
- 3: Clarify in README that default timeout is 0 (no timeout) axios/axios#1750
- 4: https://apidog.com/blog/handle-axios-timeout/
- 5: https://stackoverflow.com/questions/68158083/how-to-handle-axios-timeout-with-hanging-api-server
Enforce a timeout before registering shared GET requests.
getInflight() stores the Axios promise before it settles, and the axios instance has no timeout/abort configuration, so a stalled request can retain the key forever and make later callers await the same pending request. Add a finite timeout (and/or cancellation handling) inside the request wrapper and clean the map on both abort and resolution.
🤖 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/static/js/api_instance.js` around lines 56 - 59, Update
getInflight() so each shared Axios GET request uses a finite timeout or
cancellation mechanism before being stored in _inflightGetRequests. Ensure the
map entry is removed when the request aborts, rejects, or resolves, preventing
stalled requests from retaining the key indefinitely.
3 participants
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