UN-3217 [FEAT] Show specific pipeline/API names in workflow deletion error message#1784
Merged
hari-kuriakose merged 15 commits intomainfrom Feb 26, 2026
Merged
Conversation
pk-zipstack
requested review from
hari-kuriakose,
harini-venkataraman and
muhammad-ali-e
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBackend now returns structured workflow usage data (can_update, pipeline_count, api_count, pipelines, api_names) when checking update/delete eligibility; frontend calls checkWorkflowUsage, formats contextual messages with getUsageMessage, and conditions deletion on usage.can_update with updated error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as "Frontend\n(Workflows.jsx)"
participant Backend as "Backend\n(workflow_helper.py)"
participant DB as "Database"
User->>Frontend: request delete workflow
Frontend->>Backend: checkWorkflowUsage(workflowId)
Backend->>DB: query/count pipelines (limit USAGE_DISPLAY_LIMIT)
Backend->>DB: query/count API deployments (limit USAGE_DISPLAY_LIMIT)
DB-->>Backend: pipeline_count, api_count, pipelines[], api_names[]
Backend-->>Frontend: { can_update, pipeline_count, api_count, pipelines, api_names }
alt can_update == true
Frontend->>Backend: delete workflow
Backend->>DB: delete workflow records
Backend-->>Frontend: success
Frontend-->>User: success message
else can_update == false
Frontend->>Frontend: getUsageMessage(workflowName, usage)
Frontend-->>User: show contextual usage message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Around line 149-159: checkWorkflowUsage currently mixes await with .then() and
swallows API errors by returning a default result; change checkWorkflowUsage to
use await/try and rethrow on failure (or return a rejected promise) so callers
can detect errors, and update deleteProject to wrap the call to
checkWorkflowUsage in a try/catch that on error invokes handleException and sets
setAlertDetails with the returned error (including a contextual message like
"Unable to delete workflow ${project.id}"); reference checkWorkflowUsage for the
API call change and deleteProject for adding the try/catch and
setAlertDetails(handleException(...)) error path.
🧹 Nitpick comments (1)
backend/workflow_manager/workflow_v2/workflow_helper.py (1)
998-1002: Pre-existing: redundant guard and wrong variable in error log.Two minor pre-existing issues in this method:
- Line 1001:
if not workflow or workflow is None:is dead code —Workflow.objects.get()raisesDoesNotExiston missing rows and never returnsNone/falsy. This branch is unreachable.- Line 1022: The error log references
id(Python built-in) instead ofworkflow_id.Neither is introduced by this PR, but since you're touching this method, it's a good opportunity to clean them up.
Suggested cleanup
def can_update_workflow(workflow_id: str) -> dict[str, Any]: try: workflow: Workflow = Workflow.objects.get(pk=workflow_id) - if not workflow or workflow is None: - raise WorkflowDoesNotExistError() pipeline_names = list(except Workflow.DoesNotExist: - logger.error(f"Error getting workflow: {id}") + logger.error(f"Error getting workflow: {workflow_id}") raise WorkflowDoesNotExistError()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Around line 158-169: getUsageMessage currently returns an empty string when
can_update is false but backend returns no pipeline_names/api_names, causing a
blank error toast; update the getUsageMessage function to return a sensible
fallback message (e.g., `Cannot delete "<workflowName>" because it is in use.`
or similar) when total === 0 so the UI always displays a readable error; ensure
you update the return for the total === 0 branch in getUsageMessage and keep
existing firstName/remaining logic intact.
🧹 Nitpick comments (1)
frontend/src/components/workflows/workflow/Workflows.jsx (1)
171-204: Mixingawaitwith.then()/.catch()insidedeleteProject.
checkWorkflowUsageis properlyawaited, but thedeleteProjectcall on Line 175 uses.then()/.catch()withoutawait. This works but is inconsistent. If you ever need to act after deletion completes (e.g., analytics), the missingawaitwill bite. Considerawaiting it for consistency.Suggested refactor
if (usage.can_update) { - projectApiService - .deleteProject(project.id) - .then(() => { - getProjectList(); - setAlertDetails({ - type: "success", - content: "Workflow deleted successfully", - }); - }) - .catch((err) => { - setAlertDetails( - handleException(err, `Unable to delete workflow ${project.id}`) - ); + try { + await projectApiService.deleteProject(project.id); + getProjectList(); + setAlertDetails({ + type: "success", + content: "Workflow deleted successfully", }); + } catch (err) { + setAlertDetails( + handleException(err, `Unable to delete workflow ${project.id}`) + ); + } } else {
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
frontend/src/components/workflows/workflow/Workflows.jsx (2)
168-168:displayLimit = 3mismatches the backend'sUSAGE_DISPLAY_LIMIT = 5.The backend fetches up to 5 names per type, but the frontend only renders 3. Items at positions 4–5 are always fetched and transmitted but silently discarded. Consider aligning both limits to the same value, or driving the frontend constant from a shared API contract, to avoid the wasted payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/workflows/workflow/Workflows.jsx` at line 168, The frontend constant displayLimit (declared as displayLimit = 3 in Workflows.jsx) does not match the backend USAGE_DISPLAY_LIMIT = 5 causing extra items to be fetched then discarded; update Workflows.jsx to use the same limit as the backend (set displayLimit to 5) or, better, derive it from the API/response (e.g., use a returned displayLimit field or the length capped by backend limit) so the frontend and backend limits match and avoid wasted payloads.
203-217: Mixing.then()/.catch()inside anasyncfunction —deleteProjectresolves before deletion completes.Because
projectApiService.deleteProject(project.id)is notawait-ed, theasyncfunction returns immediately after scheduling the call. The.then()/.catch()callbacks may execute after the component unmounts, risking state-update warnings. Refactoring toawaitalso makes the outertry/catchthe single error-handling boundary, eliminating the inner.catch().♻️ Proposed refactor
if (usage.canUpdate) { - projectApiService - .deleteProject(project.id) - .then(() => { - getProjectList(); - setAlertDetails({ - type: "success", - content: "Workflow deleted successfully", - }); - }) - .catch((err) => { - setAlertDetails( - handleException(err, `Unable to delete workflow ${project.id}`) - ); - }); + await projectApiService.deleteProject(project.id); + getProjectList(); + setAlertDetails({ + type: "success", + content: "Workflow deleted successfully", + }); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/workflows/workflow/Workflows.jsx` around lines 203 - 217, The code calls projectApiService.deleteProject(project.id) using .then()/.catch() inside an async handler which can return before deletion completes; change this to await projectApiService.deleteProject(project.id) and let the enclosing try/catch handle errors, then after awaiting call getProjectList() and setAlertDetails({type: "success", content: "Workflow deleted successfully"}); remove the inner .then()/.catch() and in the catch use setAlertDetails(handleException(err, `Unable to delete workflow ${project.id}`)) so you reference the same symbols (usage.canUpdate, projectApiService.deleteProject, getProjectList, setAlertDetails, handleException, project.id).backend/workflow_manager/workflow_v2/workflow_helper.py (3)
997-997: Consider movingUSAGE_DISPLAY_LIMITto the top of the class.Class-level constants are conventionally placed at the beginning of the class body for discoverability. Its current placement mid-class (between two methods) is valid Python but surprising to readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/workflow_manager/workflow_v2/workflow_helper.py` at line 997, Move the class-level constant USAGE_DISPLAY_LIMIT from its current mid-class location to the top of the class body (immediately after the class declaration and any class docstring), so it is declared alongside other class-level constants for discoverability; ensure any references to USAGE_DISPLAY_LIMIT in methods remain unchanged and adjust surrounding whitespace/comments as needed to keep style consistent.
1027-1029: Uselogger.exceptioninstead oflogger.errorinsideexceptblocks.
logger.exceptionautomatically captures and appends the current exception traceback, which aids debugging with no extra cost.logger.errorhere silently discards theDoesNotExisttraceback.♻️ Proposed fix
except Workflow.DoesNotExist: - logger.error(f"Error getting workflow: {workflow_id}") + logger.exception(f"Error getting workflow: {workflow_id}") raise WorkflowDoesNotExistError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/workflow_manager/workflow_v2/workflow_helper.py` around lines 1027 - 1029, Replace the logger.error call inside the Workflow.DoesNotExist except block with logger.exception so the caught exception traceback is recorded; locate the except Workflow.DoesNotExist block in workflow_helper.py (where workflow_id is used) and change logger.error(f"Error getting workflow: {workflow_id}") to logger.exception(...) while keeping the subsequent raise WorkflowDoesNotExistError() intact so the original error context is preserved in logs.
1002-1004: Dead code —Workflow.objects.get()never returnsNone.
Workflow.objects.get(pk=workflow_id)either returns an instance or raisesWorkflow.DoesNotExist(caught below at line 1027). Theif not workflow or workflow is Nonebranch is unreachable.🧹 Proposed cleanup
workflow: Workflow = Workflow.objects.get(pk=workflow_id) - if not workflow or workflow is None: - raise WorkflowDoesNotExistError() - limit = WorkflowHelper.USAGE_DISPLAY_LIMIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/workflow_manager/workflow_v2/workflow_helper.py` around lines 1002 - 1004, The conditional checking `if not workflow or workflow is None` after calling `Workflow.objects.get(pk=workflow_id)` is dead code because `Workflow.objects.get` either returns an instance or raises `Workflow.DoesNotExist`; remove that unreachable branch and rely on the existing exception handling (the `Workflow.DoesNotExist` catch and/or raising `WorkflowDoesNotExistError()` where appropriate) so you don't duplicate or hide error flow for the `Workflow` retrieval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Around line 164-166: The empty-usage edge case is already handled: when
totalCount === 0 the component returns a fallback message string using the
workflowName variable; ensure the return in Workflows.jsx (the block referencing
totalCount and workflowName) remains and is used by the toast/error handler so
the user sees "Cannot delete `workflowName` as it is currently in use." (keep
the interpolation/backticks as shown).
---
Nitpick comments:
In `@backend/workflow_manager/workflow_v2/workflow_helper.py`:
- Line 997: Move the class-level constant USAGE_DISPLAY_LIMIT from its current
mid-class location to the top of the class body (immediately after the class
declaration and any class docstring), so it is declared alongside other
class-level constants for discoverability; ensure any references to
USAGE_DISPLAY_LIMIT in methods remain unchanged and adjust surrounding
whitespace/comments as needed to keep style consistent.
- Around line 1027-1029: Replace the logger.error call inside the
Workflow.DoesNotExist except block with logger.exception so the caught exception
traceback is recorded; locate the except Workflow.DoesNotExist block in
workflow_helper.py (where workflow_id is used) and change logger.error(f"Error
getting workflow: {workflow_id}") to logger.exception(...) while keeping the
subsequent raise WorkflowDoesNotExistError() intact so the original error
context is preserved in logs.
- Around line 1002-1004: The conditional checking `if not workflow or workflow
is None` after calling `Workflow.objects.get(pk=workflow_id)` is dead code
because `Workflow.objects.get` either returns an instance or raises
`Workflow.DoesNotExist`; remove that unreachable branch and rely on the existing
exception handling (the `Workflow.DoesNotExist` catch and/or raising
`WorkflowDoesNotExistError()` where appropriate) so you don't duplicate or hide
error flow for the `Workflow` retrieval.
In `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Line 168: The frontend constant displayLimit (declared as displayLimit = 3 in
Workflows.jsx) does not match the backend USAGE_DISPLAY_LIMIT = 5 causing extra
items to be fetched then discarded; update Workflows.jsx to use the same limit
as the backend (set displayLimit to 5) or, better, derive it from the
API/response (e.g., use a returned displayLimit field or the length capped by
backend limit) so the frontend and backend limits match and avoid wasted
payloads.
- Around line 203-217: The code calls
projectApiService.deleteProject(project.id) using .then()/.catch() inside an
async handler which can return before deletion completes; change this to await
projectApiService.deleteProject(project.id) and let the enclosing try/catch
handle errors, then after awaiting call getProjectList() and
setAlertDetails({type: "success", content: "Workflow deleted successfully"});
remove the inner .then()/.catch() and in the catch use
setAlertDetails(handleException(err, `Unable to delete workflow ${project.id}`))
so you reference the same symbols (usage.canUpdate,
projectApiService.deleteProject, getProjectList, setAlertDetails,
handleException, project.id).
chandrasekharan-zipstack
approved these changes
Feb 18, 2026
muhammad-ali-e
approved these changes
Feb 18, 2026
Contributor
muhammad-ali-e
left a comment
muhammad-ali-e
left a comment
There was a problem hiding this comment.
Left a NIT comment. It would be better if it were addressed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/workflow_manager/workflow_v2/workflow_helper.py (3)
997-998: MoveUSAGE_DISPLAY_LIMITto the top of the class.Defining a class-level constant between method definitions is valid Python but makes it hard to discover. Grouping constants at the top of the class is the conventional pattern and improves readability.
♻️ Proposed refactor
class WorkflowHelper: + USAGE_DISPLAY_LIMIT = 5 + `@staticmethod` def get_workflow_by_id(id: str) -> Workflow: ...And remove it from its current position between
make_async_resultandcan_update_workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/workflow_manager/workflow_v2/workflow_helper.py` around lines 997 - 998, Move the class-level constant USAGE_DISPLAY_LIMIT from its current position between the methods make_async_result and can_update_workflow to the top of the class (with other class-level constants) so it's easy to discover; update the class definition to declare USAGE_DISPLAY_LIMIT = 5 immediately after the class declaration and remove the existing declaration between make_async_result and can_update_workflow.
1030-1036: Move the successreturnto anelseblock (Ruff TRY300).The
returnstatement inside atryblock means any exception it raises (however unlikely) would be silently caught by theexcept Workflow.DoesNotExistbelow, masking errors unrelated to workflow existence. Moving it to anelseblock tightens the exception scope.♻️ Proposed refactor
try: workflow: Workflow = Workflow.objects.get(pk=workflow_id) if not workflow or workflow is None: raise WorkflowDoesNotExistError() pipeline_count = Pipeline.objects.filter(workflow=workflow).count() api_count = APIDeployment.objects.filter(workflow=workflow).count() if (pipeline_count + api_count) == 0: return { "can_update": True, "pipelines": [], "api_names": [], "pipeline_count": 0, "api_count": 0, } limit = WorkflowHelper.USAGE_DISPLAY_LIMIT pipelines = list( Pipeline.objects.filter(workflow=workflow).values( "pipeline_name", "pipeline_type" )[:limit] ) api_names = list( APIDeployment.objects.filter(workflow=workflow).values_list( "display_name", flat=True )[:limit] ) - return { - "can_update": False, - "pipelines": pipelines, - "api_names": api_names, - "pipeline_count": pipeline_count, - "api_count": api_count, - } except Workflow.DoesNotExist: logger.error(f"Error getting workflow: {workflow_id}") raise WorkflowDoesNotExistError() + else: + return { + "can_update": False, + "pipelines": pipelines, + "api_names": api_names, + "pipeline_count": pipeline_count, + "api_count": api_count, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/workflow_manager/workflow_v2/workflow_helper.py` around lines 1030 - 1036, The early return that constructs the response dict is inside a try block that catches Workflow.DoesNotExist, which can mask unrelated exceptions; move that return into an else block paired with the try/except so the try only wraps the code that may raise Workflow.DoesNotExist (the lookup/operation using Workflow), then in the else return the dict ({ "can_update": False, "pipelines": pipelines, "api_names": api_names, "pipeline_count": pipeline_count, "api_count": api_count }) — reference the try that catches Workflow.DoesNotExist and the returned variables pipelines, api_names, pipeline_count, api_count to locate where to relocate the return.
1038-1038: Preferlogger.exceptionoverlogger.errorinexceptblocks (Ruff TRY400).
logger.exceptionautomatically attaches the current exception's traceback to the log record, which aids debugging.logger.errorwithoutexc_info=Truesilently drops it.♻️ Proposed fix
- logger.error(f"Error getting workflow: {workflow_id}") + logger.exception(f"Error getting workflow: {workflow_id}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/workflow_manager/workflow_v2/workflow_helper.py` at line 1038, In the except block that currently logs "Error getting workflow: {workflow_id}" using logger.error, switch to logger.exception (or add exc_info=True) so the traceback is included; specifically replace the call to logger.error(f"Error getting workflow: {workflow_id}") with logger.exception(f"Error getting workflow: {workflow_id}") in the workflow helper where logger and workflow_id are used (the error handling around getting the workflow).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/workflow_manager/workflow_v2/workflow_helper.py`:
- Around line 997-998: Move the class-level constant USAGE_DISPLAY_LIMIT from
its current position between the methods make_async_result and
can_update_workflow to the top of the class (with other class-level constants)
so it's easy to discover; update the class definition to declare
USAGE_DISPLAY_LIMIT = 5 immediately after the class declaration and remove the
existing declaration between make_async_result and can_update_workflow.
- Around line 1030-1036: The early return that constructs the response dict is
inside a try block that catches Workflow.DoesNotExist, which can mask unrelated
exceptions; move that return into an else block paired with the try/except so
the try only wraps the code that may raise Workflow.DoesNotExist (the
lookup/operation using Workflow), then in the else return the dict ({
"can_update": False, "pipelines": pipelines, "api_names": api_names,
"pipeline_count": pipeline_count, "api_count": api_count }) — reference the try
that catches Workflow.DoesNotExist and the returned variables pipelines,
api_names, pipeline_count, api_count to locate where to relocate the return.
- Line 1038: In the except block that currently logs "Error getting workflow:
{workflow_id}" using logger.error, switch to logger.exception (or add
exc_info=True) so the traceback is included; specifically replace the call to
logger.error(f"Error getting workflow: {workflow_id}") with
logger.exception(f"Error getting workflow: {workflow_id}") in the workflow
helper where logger and workflow_id are used (the error handling around getting
the workflow).
Resolved conflict in Workflows.jsx by keeping the improved error message using getUsageMessage() from the feature branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
hari-kuriakose
deleted the
feat/improve-workflow-deletion-error-message
branch
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
5 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.
What
Why
How
workflow_helper.py): Modifiedcan_update_workflowto returnpipeline_namesandapi_nameslists alongside the existingcan_updateboolean, by queryingPipelineandAPIDeploymentmodels for their display names.Workflows.jsx): AddedgetUsageMessagehelper that constructs a contextual message showing the first pipeline/API name and a count of remaining ones. Examples:Cannot delete "A" as it is used in "B".Cannot delete "A" as it is used in "B" and 2 other API/ETL/Task pipelines.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
can_updateboolean in the API response is unchanged in behavior. The two new fields (pipeline_names,api_names) are additive. The frontend gracefully defaults to empty arrays if the new fields are absent, so backward compatibility is preserved.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.