UN-1720 [FIX] Refactor to resolve SonarQube duplicate return issue #1497
Conversation
…WorkflowSerializer Changed validation error to use field-specific error dictionary format for better API response consistency when workflow_id is missing. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
for more information, see https://pre-commit.ci
…code review - Use RequestKey.REQUEST constant instead of string literal for consistency - Make file count validation error field-specific (similar to workflow_id) - Extract MAX_EXECUTION_FILES as a module constant for better maintainability - Improve error message structure for better API response consistency 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
for more information, see https://pre-commit.ci
- Moved MAX_EXECUTION_FILES to Django settings as MAX_WORKFLOW_EXECUTION_FILES - Changed workflow_id field to required=True and removed redundant validation - Fixed error message formatting by adding quotes around file count - Added environment variable documentation in sample.env 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
for more information, see https://pre-commit.ci
- Created WfConstants.js with MAX_WORKFLOW_EXECUTION_FILES constant - Updated FileUpload component to validate max 2 files before submission - Added client-side validation with proper error messages - Enhanced Upload component with maxCount and multiple file support - Improved user experience with immediate validation feedback 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ling Added 'max_file_limit_exceeded' error code to ValidationError in ExecuteWorkflowSerializer to improve frontend error handling and API response consistency. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
for more information, see https://pre-commit.ci
- Renamed setting to better indicate it's specific to workflow page functionality - Updated backend settings, environment variables, and serializer - Updated frontend constants and all references in FileUpload component - Maintains same functionality with clearer naming convention 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Consolidated multiple return false statements into single return - Added comment explaining why false is always returned (manual upload) - Resolves SonarQube code smell about functions with duplicate returns 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Summary by CodeRabbit
WalkthroughIntroduces a new max-files-per-execution setting exposed via env and used in backend validation. Updates serializer to require workflow_id and enforce file count limit from settings. Adds frontend constants and client-side checks mirroring the limit, updates the file upload component, and modifies the continueWfExecution call to include a third parameter. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as FileUpload.jsx
participant Const as WfConstants.js
participant API as Backend API
participant Ser as ExecuteWorkflowSerializer
User->>UI: Select files
UI->>Const: Read WORKFLOW_PAGE_MAX_FILES
alt Files exceed limit
UI-->>User: Show MAX_FILES_EXCEEDED
else Within limit
User->>UI: Click Submit
alt No files
UI-->>User: Show NO_FILES_SELECTED
else OK
UI->>API: continueWfExecution(wfId, files, params)
API->>Ser: Validate request (files, workflow_id)
alt Exceeds server limit
Ser-->>API: ValidationError (max_file_limit_exceeded)
API-->>UI: 400 with error
UI-->>User: Show error
else Valid
Ser-->>API: OK
API-->>UI: 200 success
UI-->>User: Close modal / proceed
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
athul-rs
changed the title
athul-rs
requested review from
chandrasekharan-zipstack,
Deepak-Kesavan,
jagadeeswaran-zipstack,
vishnuszipstack and
gaya3-zipstack
Signed-off-by: Athul <[email protected]>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/agency/file-upload/FileUpload.jsx (1)
17-19: Use functional state updates in onRemove to avoid stale closures.setFileList(fileList.filter(...)) captures a potentially stale fileList when Ant Design batches updates. Use the functional form.
Apply this diff:
- const onRemove = (file) => { - setFileList(fileList.filter((item) => item.uid !== file.uid)); - }; + const onRemove = (file) => { + setFileList((prev) => prev.filter((item) => item.uid !== file.uid)); + };
🧹 Nitpick comments (6)
backend/sample.env (1)
186-187: Good addition, but avoid FE/BE drift for this limit.Defining WORKFLOW_PAGE_MAX_FILES here is useful, but the frontend currently hard-codes the same numeric value. If ops change this env in production without a coordinated FE deploy, the UX and server validation will diverge.
- Consider exposing this limit via a small public config endpoint (or injecting via window.APP_CONFIG at runtime) so the FE reads it dynamically, or pass it as a prop from a parent that fetches config once.
- Document the expected range (>= 1) in this sample file.
frontend/src/components/agency/file-upload/WfConstants.js (2)
5-7: Don’t hard-code the max file count in FE; make it configurable.Hard-coding 2 here risks configuration drift with the backend setting. Prefer reading from a runtime config (e.g., window.APP_CONFIG.WORKFLOW_PAGE_MAX_FILES with a sane default) or pass it down as a prop so dynamic environments don’t require FE rebuilds.
Apply this minimal diff if you already have a runtime config object:
-// Maximum number of files allowed per workflow page execution -export const WORKFLOW_PAGE_MAX_FILES = 2; +// Maximum number of files allowed per workflow page execution (fallback to 2) +export const WORKFLOW_PAGE_MAX_FILES = + (window.__APP_CONFIG__ && Number(window.__APP_CONFIG__.WORKFLOW_PAGE_MAX_FILES)) || 2;
9-13: Consider centralizing messages into your i18n/locale system.If the project uses a localization framework elsewhere, move these strings to the shared messages catalog to keep UX consistent and localizable.
frontend/src/components/agency/file-upload/FileUpload.jsx (1)
69-69: Update button label for multiple selection.Minor UX nit: with multiple={true}, label should be plural.
Apply this diff:
- <Button icon={<UploadOutlined />}>Select File</Button> + <Button icon={<UploadOutlined />}>Select Files</Button>backend/backend/settings/base.py (1)
69-71: Clamp/validate the limit to avoid misconfiguration.If WORKFLOW_PAGE_MAX_FILES is unset or set to an invalid/negative value, behavior becomes undefined. Clamp at a minimum of 1 (and optionally enforce a reasonable upper bound).
Apply this diff:
-# Maximum number of files allowed per workflow page execution -WORKFLOW_PAGE_MAX_FILES = int(os.environ.get("WORKFLOW_PAGE_MAX_FILES", 2)) +# Maximum number of files allowed per workflow page execution +# Clamp to minimum of 1 to avoid accidental 0/negative values +WORKFLOW_PAGE_MAX_FILES = max(1, int(os.environ.get("WORKFLOW_PAGE_MAX_FILES", 2)))backend/workflow_manager/workflow_v2/serializers.py (1)
90-103: Server-side enforcement is good; align form field name and improve message.
- The validator looks for request.FILES.getlist("files"). Confirm the FE uses the same field name ("files") when constructing FormData; otherwise this will silently not enforce the limit.
- Consider singular/plural grammar and removing single quotes around numbers.
- Optional: centralize the error message (and limit) so FE and BE share one source of truth.
If the field name differs, adapt as:
- files = request.FILES.getlist("files") + files = request.FILES.getlist("files") or request.FILES.getlist("file")Or make it configurable.
Also, tweak the copy:
- f"Maximum {settings.WORKFLOW_PAGE_MAX_FILES} files are allowed for workflow execution. " - f"You have uploaded '{len(files)}' files." + f"Maximum {settings.WORKFLOW_PAGE_MAX_FILES} files are allowed for workflow execution. " + f"You have uploaded {len(files)}."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (5)
backend/backend/settings/base.py(1 hunks)backend/sample.env(1 hunks)backend/workflow_manager/workflow_v2/serializers.py(3 hunks)frontend/src/components/agency/file-upload/FileUpload.jsx(3 hunks)frontend/src/components/agency/file-upload/WfConstants.js(1 hunks)
🔇 Additional comments (5)
frontend/src/components/agency/file-upload/FileUpload.jsx (4)
31-38: Client-side submit validation looks good.Redundant with the strengthened beforeUpload/maxCount checks, but keeping this defensive check is fine and improves UX.
66-67: Nice: Using maxCount and multiple for UX.maxCount guards the UI, while we also validate in code and on the server. Good layering.
4-7: Potential scope drift vs PR description.PR description mentions a minor refactor to deduplicate return statements in beforeUpload, but this change also introduces new constants and ties to backend limits. Please update the PR description and test plan accordingly.
39-43: ConfirmhandleWfExecutionsignature for the new 3-arg call
- In FileUpload (frontend/src/components/agency/file-upload/FileUpload.jsx lines 39–43), you now invoke
continueWfExecution( wfExecutionParams[0], wfExecutionParams[1], wfExecutionParams[2] );- The
continueWfExecutionprop is wired up in Agency (frontend/src/components/agency/agency/Agency.jsx at line 1276) ascontinueWfExecution={handleWfExecution}.I wasn’t able to locate the definition of
handleWfExecutionto confirm its parameters. Please manually verify thathandleWfExecutionhas been updated to accept three arguments—otherwise passing three values here will result inundefinedor unexpected behavior.backend/workflow_manager/workflow_v2/serializers.py (1)
69-69: workflow_id is now mandatory: please verify all usagesThe
workflow_idfield onExecuteWorkflowSerializerhas been changed torequired=True. While this is the correct enforcement, it risks breaking any existing clients or tests that submit requests without this field. Our automated search ofbackend/**/test_*.pyfiles did not reveal any direct instantiations ofExecuteWorkflowSerializerin tests, but coverage may be incomplete—especially outside of theendpoint_v2/testsdirectory.Please manually audit the following before merging:
- All backend tests that instantiate or use
ExecuteWorkflowSerializer(including any custom or non–test_*.py patterns).- Any frontend or SDK clients that construct requests against the “execute workflow” endpoint.
- API documentation (OpenAPI/Swagger, Markdown docs, etc.) to ensure
workflow_idis always listed as required.- Any code generation or code samples in developer guides to include
workflow_id.If all usages are updated and documentation is aligned, this change can be safely merged.
| const beforeUpload = (file) => { | ||
| if (fileList.length >= WORKFLOW_PAGE_MAX_FILES) { | ||
| message.error(WORKFLOW_VALIDATION_MESSAGES.MAX_FILES_EXCEEDED); | ||
| return false; | ||
| } else { | ||
| setFileList([...fileList, file]); | ||
| } | ||
| setFileList([...fileList, file]); | ||
| return false; | ||
| return false; // Always prevent automatic upload (manual upload on submit) | ||
| }; |
There was a problem hiding this comment.
Fix multi-select race: use functional updates inside beforeUpload.
When selecting multiple files at once (multiple={true}), React may batch setState calls and each beforeUpload invocation reads the same captured fileList. This can drop files or exceed limits. Use a functional updater and decide based on prev length. Also de-duplicate the error message with a stable key.
Apply this diff:
- const beforeUpload = (file) => {
- if (fileList.length >= WORKFLOW_PAGE_MAX_FILES) {
- message.error(WORKFLOW_VALIDATION_MESSAGES.MAX_FILES_EXCEEDED);
- } else {
- setFileList([...fileList, file]);
- }
- return false; // Always prevent automatic upload (manual upload on submit)
- };
+ const beforeUpload = (file) => {
+ let rejected = false;
+ setFileList((prev) => {
+ if (prev.length >= WORKFLOW_PAGE_MAX_FILES) {
+ rejected = true;
+ return prev;
+ }
+ return [...prev, file];
+ });
+ if (rejected) {
+ message.error({
+ key: "max-files",
+ content: WORKFLOW_VALIDATION_MESSAGES.MAX_FILES_EXCEEDED,
+ });
+ }
+ return false; // Always prevent automatic upload (manual upload on submit)
+ };📝 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.
| const beforeUpload = (file) => { | |
| if (fileList.length >= WORKFLOW_PAGE_MAX_FILES) { | |
| message.error(WORKFLOW_VALIDATION_MESSAGES.MAX_FILES_EXCEEDED); | |
| return false; | |
| } else { | |
| setFileList([...fileList, file]); | |
| } | |
| setFileList([...fileList, file]); | |
| return false; | |
| return false; // Always prevent automatic upload (manual upload on submit) | |
| }; | |
| const beforeUpload = (file) => { | |
| let rejected = false; | |
| setFileList((prev) => { | |
| if (prev.length >= WORKFLOW_PAGE_MAX_FILES) { | |
| rejected = true; | |
| return prev; | |
| } | |
| return [...prev, file]; | |
| }); | |
| if (rejected) { | |
| message.error({ | |
| key: "max-files", | |
| content: WORKFLOW_VALIDATION_MESSAGES.MAX_FILES_EXCEEDED, | |
| }); | |
| } | |
| return false; // Always prevent automatic upload (manual upload on submit) | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/agency/file-upload/FileUpload.jsx around lines 21 to
28, the beforeUpload handler reads a stale fileList when multiple files are
selected causing lost files and duplicated messages; change setFileList to use
the functional updater (setFileList(prev => { compute newList based on prev,
enforce WORKFLOW_PAGE_MAX_FILES limit, and return the new array })) and base the
limit check on prev.length so each call is consistent; also dedupe the error
notification by passing a stable key to message.error (e.g.
'max-files-exceeded') so the error is shown only once per selection; keep
returning false to prevent automatic upload.
There was a problem hiding this comment.
@athul-rs check if this is valid
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @CodeRabbit in a new comment, and I'll come hopping out!
|
|
Summary
FileUpload.jsxwherebeforeUploadfunction had multiple identicalreturn falsestatementsChanges
beforeUploadfunction in/frontend/src/components/agency/file-upload/FileUpload.jsxTest Plan
🤖 Generated with Claude Code