Generalize abortable fetching for race conditions#1089
Open
thangqp wants to merge 2 commits into
Open
Conversation
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces a reusable Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/parameters/common/contingency-table/contingency-table.tsx`:
- Around line 48-79: hasNoContingencies is used to skip the fetch which leaves
simulatedContingencyCount null (showing '...') and the flatMap can inject
undefined IDs; fix by explicitly preserving the "no contingencies" state and
removing undefined IDs: when deciding to skip because hasNoContingencies,
setSimulatedContingencyCount(0) (or the existing no-contingency sentinel) so the
noContingency alert shows, and change the ID builder in the fetcher (the
contingencyListsInfos.filter(...).flatMap(...)) to strip falsy/undefined IDs
(e.g., add .filter(Boolean) after mapping) so fetchContingencyCount never
receives undefined entries; keep other onSuccess/onError/cleanup behavior
unchanged.
In `@src/components/parameters/sensi/use-sensitivity-analysis-parameters.ts`:
- Around line 210-220: The fetch for factor counts can run with a null
studyUuid; update the useAbortableFetch call (deps and skipFetch) so it includes
studyUuid in deps and adds studyUuid to the skipFetch condition (i.e., skip when
!studyUuid || !factorCountParams || !currentNodeUuid || !currentRootNetworkUuid)
to prevent requests with an invalid study id; likewise, adjust
updateFactorCount() to early-return when studyUuid is not present before
recomputing factorCountParams so factorCountParams is not derived while the
study context (UseSensitivityAnalysisParametersFormProps) is incomplete.
In `@src/hooks/use-abortable-fetch.ts`:
- Around line 59-91: The then-handler currently calls onSuccessEvent for every
completion, allowing stale responses to overwrite newer state; update the
success path in use-abortable-fetch so it ignores late completions when the
associated signal is already aborted (similar to the catch check), i.e., before
calling onSuccessEvent, check signal.aborted and signal.reason?.message ===
IGNORE_SIGNAL (or a general signal.aborted) and return early to drop the result;
ensure setLoading and timeout handling also only run for non-aborted completions
so fetcherWithSignal, signal, IGNORE_SIGNAL, onSuccessEvent, setLoading, and
timeoutRef behave consistently with the existing abort 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37e88136-8100-44d2-8175-cc51a1f625d1
📒 Files selected for processing (4)
src/components/parameters/common/contingency-table/contingency-table.tsxsrc/components/parameters/sensi/use-sensitivity-analysis-parameters.tssrc/hooks/index.tssrc/hooks/use-abortable-fetch.ts
Comment on lines
+48
to
+79
| const hasNoContingencies = | ||
| !contingencyListsInfos || | ||
| (contingencyListsInfos.length ?? 0) === 0 || | ||
| contingencyListsInfos.every((c) => !c[ACTIVATED] || (c[CONTINGENCY_LISTS]?.length ?? 0) === 0); | ||
|
|
||
| const controller = new AbortController(); | ||
| // build a signal which allows us to cancel the fetch by calling controller.abort() or the timeout fires | ||
| const abortSignal = AbortSignal.any([controller.signal, AbortSignal.timeout(DEFAULT_TIMEOUT_MS)]); | ||
| setIsLoading(true); | ||
| const { loading: isLoading } = useAbortableFetch({ | ||
| deps: [snackError, contingencyListsInfos, fetchContingencyCount, showContingencyCount, isBuiltCurrentNode], | ||
| skipFetch: !showContingencyCount || !isBuiltCurrentNode || hasNoContingencies, | ||
| timeoutMs: DEFAULT_TIMEOUT_MS, | ||
| fetcher: (signal) => | ||
| fetchContingencyCount?.( | ||
| contingencyListsInfos | ||
| .filter((l) => l[ACTIVATED]) | ||
| .flatMap((l) => l[CONTINGENCY_LISTS]?.map((c) => c[ID])), | ||
| signal | ||
| ), | ||
|
|
||
| fetchContingencyCount?.( | ||
| contingencyListsInfos | ||
| .filter((lists) => lists[ACTIVATED]) | ||
| .flatMap((lists) => lists[CONTINGENCY_LISTS]?.map((contingencyList) => contingencyList[ID])), | ||
| abortSignal | ||
| ) | ||
| .then((contingencyCount) => { | ||
| setSimulatedContingencyCount(contingencyCount); | ||
| loadingTimeoutId = setTimeout(() => { | ||
| setIsLoading(false); | ||
| }, 500); | ||
| }) | ||
| .catch((error) => { | ||
| setSimulatedContingencyCount(null); | ||
| onSuccess: (count) => { | ||
| setSimulatedContingencyCount(count); | ||
| }, | ||
|
|
||
| if (abortSignal.aborted && abortSignal.reason?.message === IGNORE_SIGNAL) { | ||
| return; | ||
| } | ||
| setIsLoading(false); | ||
| snackWithFallback(snackError, error, { headerId: 'getSecurityAnalysisContingenciesCountError' }); | ||
| onError: (error) => { | ||
| setSimulatedContingencyCount(null); | ||
| snackWithFallback(snackError, error, { | ||
| headerId: 'getSecurityAnalysisContingenciesCountError', | ||
| }); | ||
| }, | ||
|
|
||
| return () => { | ||
| controller?.abort(new Error(IGNORE_SIGNAL)); | ||
| clearTimeout(loadingTimeoutId); | ||
| }; | ||
| }, [snackError, contingencyListsInfos, fetchContingencyCount, showContingencyCount, isBuiltCurrentNode]); | ||
| cleanup: () => { | ||
| setSimulatedContingencyCount(null); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Preserve the empty-selection state and strip undefined IDs.
With hasNoContingencies folded into skipFetch, this path now leaves simulatedContingencyCount at null, so the component falls back to the '...' placeholder instead of the existing noContingency alert. Also, flatMap((l) => l[CONTINGENCY_LISTS]?.map(...)) inserts undefined entries for activated rows whose list is missing.
Possible fix
+ const emptyCount: ContingencyCount = { contingencies: 0, notFoundElements: 0 };
+
const { loading: isLoading } = useAbortableFetch({
deps: [snackError, contingencyListsInfos, fetchContingencyCount, showContingencyCount, isBuiltCurrentNode],
- skipFetch: !showContingencyCount || !isBuiltCurrentNode || hasNoContingencies,
+ skipFetch: !showContingencyCount || !isBuiltCurrentNode,
timeoutMs: DEFAULT_TIMEOUT_MS,
fetcher: (signal) =>
- fetchContingencyCount?.(
- contingencyListsInfos
- .filter((l) => l[ACTIVATED])
- .flatMap((l) => l[CONTINGENCY_LISTS]?.map((c) => c[ID])),
- signal
- ),
+ hasNoContingencies
+ ? Promise.resolve(emptyCount)
+ : fetchContingencyCount?.(
+ contingencyListsInfos
+ .filter((l) => l[ACTIVATED])
+ .flatMap((l) => (l[CONTINGENCY_LISTS] ?? []).map((c) => c[ID])),
+ signal
+ ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/parameters/common/contingency-table/contingency-table.tsx`
around lines 48 - 79, hasNoContingencies is used to skip the fetch which leaves
simulatedContingencyCount null (showing '...') and the flatMap can inject
undefined IDs; fix by explicitly preserving the "no contingencies" state and
removing undefined IDs: when deciding to skip because hasNoContingencies,
setSimulatedContingencyCount(0) (or the existing no-contingency sentinel) so the
noContingency alert shows, and change the ID builder in the fetcher (the
contingencyListsInfos.filter(...).flatMap(...)) to strip falsy/undefined IDs
(e.g., add .filter(Boolean) after mapping) so fetchContingencyCount never
receives undefined entries; keep other onSuccess/onError/cleanup behavior
unchanged.
Comment on lines
+210
to
+220
| const { loading: isLoading } = useAbortableFetch({ | ||
| deps: [snackError, studyUuid, currentRootNetworkUuid, currentNodeUuid, factorCountParams], | ||
| skipFetch: !factorCountParams || !currentNodeUuid || !currentRootNetworkUuid, | ||
| timeoutMs: DEFAULT_TIMEOUT_MS, | ||
| fetcher: (signal) => | ||
| getSensitivityAnalysisFactorsCount( | ||
| studyUuid, | ||
| currentNodeUuid!, | ||
| currentRootNetworkUuid!, | ||
| factorCountParams!, | ||
| signal |
There was a problem hiding this comment.
Gate the factors-count request on studyUuid too.
studyUuid is still nullable here, but this fetch only skips on the node/root IDs. In the partially initialized state allowed by UseSensitivityAnalysisParametersFormProps, that can still fire the request with an invalid study id.
Possible fix
const { loading: isLoading } = useAbortableFetch({
deps: [snackError, studyUuid, currentRootNetworkUuid, currentNodeUuid, factorCountParams],
- skipFetch: !factorCountParams || !currentNodeUuid || !currentRootNetworkUuid,
+ skipFetch: !studyUuid || !factorCountParams || !currentNodeUuid || !currentRootNetworkUuid,
timeoutMs: DEFAULT_TIMEOUT_MS,
fetcher: (signal) =>
getSensitivityAnalysisFactorsCount(
studyUuid,Also align the early return in updateFactorCount() so factorCountParams is not recomputed while the study context is still incomplete.
📝 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
| const { loading: isLoading } = useAbortableFetch({ | |
| deps: [snackError, studyUuid, currentRootNetworkUuid, currentNodeUuid, factorCountParams], | |
| skipFetch: !factorCountParams || !currentNodeUuid || !currentRootNetworkUuid, | |
| timeoutMs: DEFAULT_TIMEOUT_MS, | |
| fetcher: (signal) => | |
| getSensitivityAnalysisFactorsCount( | |
| studyUuid, | |
| currentNodeUuid!, | |
| currentRootNetworkUuid!, | |
| factorCountParams!, | |
| signal | |
| const { loading: isLoading } = useAbortableFetch({ | |
| deps: [snackError, studyUuid, currentRootNetworkUuid, currentNodeUuid, factorCountParams], | |
| skipFetch: !studyUuid || !factorCountParams || !currentNodeUuid || !currentRootNetworkUuid, | |
| timeoutMs: DEFAULT_TIMEOUT_MS, | |
| fetcher: (signal) => | |
| getSensitivityAnalysisFactorsCount( | |
| studyUuid, | |
| currentNodeUuid!, | |
| currentRootNetworkUuid!, | |
| factorCountParams!, | |
| signal |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/parameters/sensi/use-sensitivity-analysis-parameters.ts`
around lines 210 - 220, The fetch for factor counts can run with a null
studyUuid; update the useAbortableFetch call (deps and skipFetch) so it includes
studyUuid in deps and adds studyUuid to the skipFetch condition (i.e., skip when
!studyUuid || !factorCountParams || !currentNodeUuid || !currentRootNetworkUuid)
to prevent requests with an invalid study id; likewise, adjust
updateFactorCount() to early-return when studyUuid is not present before
recomputing factorCountParams so factorCountParams is not derived while the
study context (UseSensitivityAnalysisParametersFormProps) is incomplete.
Comment on lines
+59
to
+91
| fetcherWithSignal | ||
| .then((fetchedData) => { | ||
| // custom success handling if needed | ||
| onSuccessEvent(fetchedData); | ||
| // delay loading reset to avoid flickering in case of very fast requests | ||
| if (delayMs) { | ||
| timeoutRef.current = setTimeout(() => { | ||
| setLoading(false); | ||
| }, delayMs); | ||
| } else { | ||
| setLoading(false); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| // treat manual abort as a special case => silently ignore | ||
| if (signal.aborted && signal.reason?.message === IGNORE_SIGNAL) { | ||
| return; | ||
| } | ||
| // other cases => treat as a normal error, including the timeout abort | ||
| setLoading(false); | ||
| // custom error handling if needed | ||
| onErrorEvent(error); | ||
| }); | ||
|
|
||
| // clean up | ||
| return () => { | ||
| controller?.abort(new Error(IGNORE_SIGNAL)); | ||
|
|
||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } | ||
| // custom cleanup if needed | ||
| onCleanupEvent(); |
There was a problem hiding this comment.
Drop late completions from cancelled or timed-out runs.
This still calls onSuccess when a stale request resolves instead of rejecting. That lets an older response overwrite newer state, which defeats the race-condition protection this hook is meant to centralize.
Possible fix
+ let disposed = false;
+
setLoading(true);
fetcherWithSignal
.then((fetchedData) => {
+ if (disposed || signal.aborted) {
+ return;
+ }
// custom success handling if needed
onSuccessEvent(fetchedData);
// delay loading reset to avoid flickering in case of very fast requests
if (delayMs) {
timeoutRef.current = setTimeout(() => {
- setLoading(false);
+ if (!disposed && !signal.aborted) {
+ setLoading(false);
+ }
}, delayMs);
} else {
setLoading(false);
}
})
.catch((error) => {
+ if (disposed) {
+ return;
+ }
// treat manual abort as a special case => silently ignore
if (signal.aborted && signal.reason?.message === IGNORE_SIGNAL) {
return;
}
// other cases => treat as a normal error, including the timeout abort
@@
// clean up
return () => {
+ disposed = true;
controller?.abort(new Error(IGNORE_SIGNAL));
if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/use-abortable-fetch.ts` around lines 59 - 91, The then-handler
currently calls onSuccessEvent for every completion, allowing stale responses to
overwrite newer state; update the success path in use-abortable-fetch so it
ignores late completions when the associated signal is already aborted (similar
to the catch check), i.e., before calling onSuccessEvent, check signal.aborted
and signal.reason?.message === IGNORE_SIGNAL (or a general signal.aborted) and
return early to drop the result; ensure setLoading and timeout handling also
only run for non-aborted completions so fetcherWithSignal, signal,
IGNORE_SIGNAL, onSuccessEvent, setLoading, and timeoutRef behave consistently
with the existing abort logic.
thangqp
changed the title
thangqp
changed the title
|
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
1 participant
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.
PR Summary
Following to this PR: #1082