[Netmanager] Proper error handling for data export #2306
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New netmanager changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
netmanager/src/views/pages/ExportData/index.js (1)
424-431: Enhance CSV validation robustnessThe CSV validation is good but could be more comprehensive.
Consider this enhanced validation:
if (response) { if (fileType.value === 'csv') { - if (typeof response !== 'string') { + if (typeof response !== 'string' || !response.trim()) { throw new Error('Invalid CSV data format.'); } + // Validate basic CSV structure + const firstLine = response.split('\n')[0]; + if (!firstLine || !firstLine.includes(',')) { + throw new Error('Invalid CSV structure.'); + } exportData(response, filename, 'text/csv;charset=utf-8;'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
netmanager/src/views/pages/ExportData/index.js(3 hunks)
🔇 Additional comments (2)
netmanager/src/views/pages/ExportData/index.js (2)
471-474: LGTM! Improved error message handling
The error message handling is now more robust with proper type checking and fallback message.
537-537: LGTM! Parameter naming consistency
The change from 'devices' to 'device_names' improves API consistency.
Let's verify this change is consistent across the codebase:
✅ Verification successful
Parameter name change is consistent with the API integration
The change from 'devices' to 'device_names' appears to be an intentional API parameter update. The search results show that this parameter is only used in the ExportData component when making API calls through downloadDataApi, and there are no other occurrences of either parameter name in the codebase that would be affected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of 'devices' in API calls
rg -l 'devices["\']?\s*:' --type js
Length of output: 116
Script:
#!/bin/bash
# Let's try a more robust search for 'devices' parameter in API calls and object properties
rg -g '*.js' -g '*.jsx' 'devices:\s*|devices"\s*:|devices''\s*:' -l
# Also search for the new parameter name for context
rg -g '*.js' -g '*.jsx' 'device_names:\s*|device_names"\s*:|device_names''\s*:' -l
# Let's also check the specific file content for context
cat netmanager/src/views/pages/ExportData/index.js
Length of output: 46303
Script:
#!/bin/bash
# Let's search for API calls or request bodies that use either 'devices' or 'device_names'
rg -g '*.js' -g '*.jsx' 'downloadDataApi.*device' -B 2 -A 2
# Also search for the API definition
rg -g '*.js' 'downloadDataApi.*=.*' -B 2 -A 2
# Let's check if there are any other files that might be using this parameter
rg -g '*.js' -g '*.jsx' 'device_names.*:' -B 2 -A 2
Length of output: 920
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
netmanager/src/views/pages/ExportData/index.js (3)
424-431: Enhance CSV validation robustnessThe CSV validation is good but could be more comprehensive.
Consider this enhanced validation:
if (response) { if (fileType.value === 'csv') { - if (typeof response !== 'string') { + if (typeof response !== 'string' || !response.trim()) { throw new Error('Invalid CSV data format.'); } + // Validate basic CSV structure + const firstLine = response.split('\n')[0]; + if (!firstLine || !firstLine.includes(',')) { + throw new Error('Invalid CSV structure.'); + } exportData(response, filename, 'text/csv;charset=utf-8;'); }
471-474: Improve error message clarityThe error message handling is good but could be more informative.
Consider this enhancement:
errorMessage = - typeof err.response.data.message === 'string' - ? err.response.data.message - : 'An error occurred while downloading data'; + typeof err.response.data.message === 'string' + ? err.response.data.message + : `An error occurred while downloading data. Please try again or contact support if the issue persists. (Error: ${err.response.status})`;
Line range hint
1-1121: Consider implementing rate limiting for data exportsThe component handles large date ranges by showing a warning message, but there's no rate limiting implementation.
Consider implementing:
- Rate limiting for consecutive export requests
- Queue system for large exports
- Progress tracking for long-running exports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
netmanager/src/views/pages/ExportData/index.js(3 hunks)
🔇 Additional comments (1)
netmanager/src/views/pages/ExportData/index.js (1)
537-537: LGTM! Parameter rename for API consistency
The rename from 'devices' to 'device_names' improves API clarity and consistency.
Summary of Changes (What does this PR do?)
This pull request includes several changes to the
ExportDatacomponent in thenetmanager/src/views/pages/ExportData/index.jsfile to improve error handling, data export functionality, and parameter naming consistency.Improvements to data export functionality:
Error handling enhancements:
err.response.data.messageis a string, and providing a default error message if it is not.Parameter naming consistency:
devicesparameter todevice_namesin the body of the API request to maintain consistency with the expected parameter names.Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Bug Fixes
New Features