Use a Managed Export CSV component that handles various states - #1222
Merged
Conversation
khouadrired
requested review from
TheMaskedTurtle,
antoinebhs,
ayolab,
flomillot and
klesaulnier
as code owners
July 2, 2026 15:38
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds ChangesManaged CSV export flow
Sequence Diagram(s)sequenceDiagram
participant User
participant ManagedExportCsvButton
participant ExportCsvButton
participant CsvExport
participant CsvExportService
User->>ExportCsvButton: click
ExportCsvButton->>ManagedExportCsvButton: handleClick
ManagedExportCsvButton->>ManagedExportCsvButton: set isLoading=true
ManagedExportCsvButton->>CsvExport: exportCsv()
CsvExport->>CsvExportService: getData(...)
CsvExportService-->>CsvExport: result/error
CsvExport-->>ManagedExportCsvButton: resolve/reject
alt success
ManagedExportCsvButton->>ManagedExportCsvButton: set isSuccessful=true
ManagedExportCsvButton->>ManagedExportCsvButton: onSuccess()
else error
ManagedExportCsvButton->>ManagedExportCsvButton: onError()
end
ManagedExportCsvButton->>ExportCsvButton: re-render with updated state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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
🧹 Nitpick comments (2)
src/components/ui/csvDownloader/csv-export.tsx (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWiden return type from
JSX.ElementtoReact.ReactNode.Static analysis flags this as too narrow:
JSX.Elementexcludesnull, strings, numbers, and fragments that components may return in the future.🔧 Proposed fix
-}: CsvExportProps): JSX.Element { +}: CsvExportProps): React.ReactNode {🤖 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 `@src/components/ui/csvDownloader/csv-export.tsx` at line 23, The CsvExport component’s return type is too narrow in its function signature, so update the `CsvExport` declaration to return `React.ReactNode` instead of `JSX.Element`. Keep the implementation aligned with the component’s actual output so it can safely return fragments, null, or other valid React nodes in the future.Source: Linters/SAST tools
src/components/ui/csvDownloader/managed-export-csv-button.tsx (1)
26-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo booleans could drift into an inconsistent combination.
isLoadingandisSuccessfulare independent state variables; nothing enforces mutual exclusivity beyond the ordering inhandleClick. Consider consolidating into a single status enum ('idle' | 'loading' | 'success' | 'error') to make invalid combinations unrepresentable and simplify the twouseEffectresets into one.🤖 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 `@src/components/ui/csvDownloader/managed-export-csv-button.tsx` around lines 26 - 38, The state in managed-export-csv-button’s component can drift because isLoading and isSuccessful are managed independently. Refactor the related logic in the component (including handleClick and the useEffect resets) to use a single status value such as 'idle' | 'loading' | 'success' | 'error' so invalid combinations cannot occur. Update the reset behavior for resetKey and disabled to set that one status consistently instead of toggling two separate booleans.
🤖 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 `@src/components/ui/csvDownloader/managed-export-csv-button.tsx`:
- Around line 40-54: handleClick in ManagedExportCsvButton can update state and
invoke callbacks after the component unmounts while exportCsv is still pending.
Add a mounted ref (set in the component lifecycle) and check it before calling
setIsSuccessful, setIsLoading, onSuccess, or onError, so only mounted instances
are updated. Keep the async flow in handleClick the same, but guard every
post-await branch and the finally cleanup with the mounted check.
---
Nitpick comments:
In `@src/components/ui/csvDownloader/csv-export.tsx`:
- Line 23: The CsvExport component’s return type is too narrow in its function
signature, so update the `CsvExport` declaration to return `React.ReactNode`
instead of `JSX.Element`. Keep the implementation aligned with the component’s
actual output so it can safely return fragments, null, or other valid React
nodes in the future.
In `@src/components/ui/csvDownloader/managed-export-csv-button.tsx`:
- Around line 26-38: The state in managed-export-csv-button’s component can
drift because isLoading and isSuccessful are managed independently. Refactor the
related logic in the component (including handleClick and the useEffect resets)
to use a single status value such as 'idle' | 'loading' | 'success' | 'error' so
invalid combinations cannot occur. Update the reset behavior for resetKey and
disabled to set that one status consistently instead of toggling two separate
booleans.
🪄 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: eda20c91-2315-473b-8585-e6bc9d9f41e5
📒 Files selected for processing (4)
src/components/ui/csvDownloader/csv-export.tsxsrc/components/ui/csvDownloader/csv-export.type.tssrc/components/ui/csvDownloader/index.tssrc/components/ui/csvDownloader/managed-export-csv-button.tsx
Comment on lines
+40
to
+54
| const handleClick = useCallback(async () => { | ||
| setIsSuccessful(false); | ||
| setIsLoading(true); | ||
|
|
||
| try { | ||
| await exportCsv(); | ||
| setIsSuccessful(true); | ||
| onSuccess?.(); | ||
| } catch (error) { | ||
| setIsSuccessful(false); | ||
| onError?.(error); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }, [exportCsv, onSuccess, onError]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against state updates after unmount.
handleClick awaits exportCsv() and then calls setIsSuccessful/setIsLoading/onSuccess/onError regardless of whether the component is still mounted. If the consumer unmounts while the export is in flight (e.g., navigating away mid-download), these calls will fire on an unmounted component, triggering React state-update warnings and potentially masking a stale closure over onSuccess/onError.
🔧 Proposed fix using a mounted ref
+ const isMountedRef = useRef(true);
+ useEffect(() => {
+ return () => {
+ isMountedRef.current = false;
+ };
+ }, []);
+
const handleClick = useCallback(async () => {
setIsSuccessful(false);
setIsLoading(true);
try {
await exportCsv();
- setIsSuccessful(true);
- onSuccess?.();
+ if (isMountedRef.current) {
+ setIsSuccessful(true);
+ onSuccess?.();
+ }
} catch (error) {
- setIsSuccessful(false);
- onError?.(error);
+ if (isMountedRef.current) {
+ setIsSuccessful(false);
+ onError?.(error);
+ }
} finally {
- setIsLoading(false);
+ if (isMountedRef.current) {
+ setIsLoading(false);
+ }
}
}, [exportCsv, onSuccess, onError]);📝 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 handleClick = useCallback(async () => { | |
| setIsSuccessful(false); | |
| setIsLoading(true); | |
| try { | |
| await exportCsv(); | |
| setIsSuccessful(true); | |
| onSuccess?.(); | |
| } catch (error) { | |
| setIsSuccessful(false); | |
| onError?.(error); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }, [exportCsv, onSuccess, onError]); | |
| const isMountedRef = useRef(true); | |
| useEffect(() => { | |
| return () => { | |
| isMountedRef.current = false; | |
| }; | |
| }, []); | |
| const handleClick = useCallback(async () => { | |
| setIsSuccessful(false); | |
| setIsLoading(true); | |
| try { | |
| await exportCsv(); | |
| if (isMountedRef.current) { | |
| setIsSuccessful(true); | |
| onSuccess?.(); | |
| } | |
| } catch (error) { | |
| if (isMountedRef.current) { | |
| setIsSuccessful(false); | |
| onError?.(error); | |
| } | |
| } finally { | |
| if (isMountedRef.current) { | |
| setIsLoading(false); | |
| } | |
| } | |
| }, [exportCsv, onSuccess, onError]); |
🤖 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 `@src/components/ui/csvDownloader/managed-export-csv-button.tsx` around lines
40 - 54, handleClick in ManagedExportCsvButton can update state and invoke
callbacks after the component unmounts while exportCsv is still pending. Add a
mounted ref (set in the component lifecycle) and check it before calling
setIsSuccessful, setIsLoading, onSuccess, or onError, so only mounted instances
are updated. Keep the async flow in handleClick the same, but guard every
post-await branch and the finally cleanup with the mounted check.
FranckLecuyer
approved these changes
Jul 3, 2026
flomillot
approved these changes
Jul 7, 2026
|
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
3 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.
No description provided.