feat: add delete confirmation dialog to webhook list items#26305
feat: add delete confirmation dialog to webhook list items#26305romitg2 merged 27 commits intocalcom:mainfrom
Conversation
|
@KartikLabhshetwar is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
dhairyashiil
left a comment
dhairyashiil
left a comment
There was a problem hiding this comment.
We should add the confirmation modal for event type webhook settings as well.
I can see {{appName}} in the confirmation message. Should we create a new message for each case or use a more generic message that covers both cases?
Please check existing confirmation dialog messages in the codebase and align with that pattern
refer code: "delete_webhook_confirmation_message": "Are you sure you want to delete this webhook? You will no longer receive {{appName}} meeting data at a specified URL, in real-time, when an event is scheduled or canceled.", |
Changes SummaryThis PR adds a confirmation dialog to webhook deletion actions in both global and event-type webhook list items to prevent accidental deletions. It implements proper error handling, loading states, and data invalidation after deletion. Type: feature Components Affected: WebhookListItem, EventTypeWebhookListItem Files Changed
Architecture Impact
Risk Areas: User experience during webhook deletion (requires extra confirmation step), Error handling ensures users are notified when deletion fails, Loading states prevent multiple simultaneous deletion attempts Suggestions
Full review in progress... | Powered by diffray |
Review Summary
Validated 57 issues: 22 kept, 35 filtered Issues Found: 22💬 See 18 individual line comment(s) for details. 📊 12 unique issue type(s) across 22 location(s) 📋 Full issue list (click to expand)🔴 CRITICAL - Switch onCheckedChange uses stale prop value instead of checked parameterAgent: react Category: bug File: Description: The onCheckedChange callback ignores the 'checked' parameter and calculates new state from webhook.active prop. Combined with defaultChecked (uncontrolled), this causes incorrect toggle behavior when props/state diverge. Suggestion: Change to use the checked parameter: Confidence: 95% Rule: 🟠 HIGH - Uncontrolled Switch with defaultChecked won't update on prop changes (2 occurrences)Agent: react Category: bug 📍 View all locations
Rule: 🟠 HIGH - New error handler lacks test coverage (6 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Error callback discards error context for debuggingAgent: react Category: quality File: Description: The onError callback in the deleteWebhook mutation receives an error parameter but doesn't use it. This loses critical debugging context when webhook deletion fails. Suggestion: Accept the error parameter and log it: Confidence: 75% Rule: 🟡 MEDIUM - Error not logged in error handlerAgent: react Category: quality File: Description: The onError callback handles the error by showing a generic toast message to the user but doesn't log the actual error details. This makes debugging production issues difficult. Suggestion: Add error logging before showing the user-facing toast: Confidence: 75% Rule: 🟡 MEDIUM - Potential undefined access on optional property canEditWebhook (2 occurrences)Agent: bugs Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Duplicated mutation success logic should be extracted (2 occurrences)Agent: react Category: quality 📍 View all locations
Rule: 🔵 LOW - Missing error handler for toggleWebhook mutation (2 occurrences)Agent: quality Category: quality 📍 View all locations
Rule: 🔵 LOW - Unused prop parameterAgent: refactoring Category: quality File: Description: The prop 'canEditWebhook' is declared in the component props but never read. The code uses permissions.canEditWebhook instead throughout the component. Suggestion: Remove the unused 'canEditWebhook' prop from the props interface since it's not used anywhere in the component. Confidence: 92% Rule: 🔵 LOW - Redundant double negation conditionAgent: refactoring Category: quality File: Description: Using '!!props.readOnly' performs an unnecessary double negation. Since the Badge is only rendered when readOnly is truthy, a simple truthy check would be more idiomatic. Suggestion: Replace '!!props.readOnly' with 'props.readOnly' in the conditional rendering. Confidence: 75% Rule: 🔵 LOW - Incomplete destructuring of props parameter (2 occurrences)Agent: react Category: quality 📍 View all locations
Rule: 🔵 LOW - Line exceeds 100 charactersAgent: react Category: style File: Description: This line contains approximately 115 characters, which may reduce readability in code review tools and split-pane editors. Suggestion: Break the line at a logical point, such as: 'content={t("webhook_version_docs", { Confidence: 60% Rule: ℹ️ 4 issue(s) outside PR diff (click to expand)
🟠 HIGH - Uncontrolled Switch with defaultChecked won't update on prop changes (2 occurrences)Agent: react Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Potential undefined access on optional property canEditWebhookAgent: bugs Category: bug File: Description: The Switch component's disabled state checks Suggestion: Use explicit comparison: Confidence: 70% Rule: 🔵 LOW - Line exceeds 100 charactersAgent: react Category: style File: Description: This line contains approximately 115 characters, which may reduce readability in code review tools and split-pane editors. Suggestion: Break the line at a logical point, such as: 'content={t("webhook_version_docs", { Confidence: 60% Rule: Review ID: |
|
hi @dhairyashiil can you please see the comments? |
romitg2
left a comment
romitg2
left a comment
There was a problem hiding this comment.
@KartikLabhshetwar Thanks for the PR! can you please resolve merge conflicts?
(making it draft until then, feel free reopen)
Devin AI is resolving merge conflictsThis PR has merge conflicts with the Devin will:
If you prefer to resolve conflicts manually, you can close the Devin session and handle it yourself. |
Co-Authored-By: unknown <>
|
@cubic-bot-ai |
|
@romitg2 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/playwright/webhook.e2e.ts">
<violation number="1" location="apps/web/playwright/webhook.e2e.ts:894">
P2: Rule violated: **E2E Tests Best Practices**
Avoid text locators in E2E tests; use a data-testid locator for the webhook item instead to comply with E2E Tests Best Practices.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/playwright/webhook.e2e.ts">
<violation number="1" location="apps/web/playwright/webhook.e2e.ts:891">
P2: Rule violated: **E2E Tests Best Practices**
Add expect(page).toHaveURL(...) after navigation so the test fails fast on unexpected redirects, as required by the E2E tests best practices.</violation>
<violation number="2" location="apps/web/playwright/webhook.e2e.ts:894">
P1: Rule violated: **E2E Tests Best Practices**
Avoid text locators in E2E tests; add a data-testid for the webhook item and use page.getByTestId instead per the E2E tests best practices.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
romitg2
added
run-ci
6 participants
What does this PR do?
Visual Demo (For contributors especially)
Video Demo (if applicable):
before:
Screen.Recording.2025-12-30.at.4.05.09.PM.mov
after:
Screen.Recording.2025-12-30.at.4.05.31.PM.mov
Mandatory Tasks (DO NOT REMOVE)
Summary by cubic
Adds a delete confirmation dialog to webhook list items (global and event-type) to prevent accidental deletions. Implements pending states, error handling, and data refresh; aligns with CAL-6996.
Written for commit 5b41fba. Summary will update on new commits.