fix: warn for unsaved changes on event type setup page#28248
fix: warn for unsaved changes on event type setup page#28248TheSeydiCharyyev wants to merge 11 commits into
Conversation
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
| // Warn before closing/refreshing the page with unsaved changes | ||
| useEffect(() => { | ||
| const handleBeforeUnload = (e: BeforeUnloadEvent) => { | ||
| if (form.formState.isDirty) { | ||
| e.preventDefault(); | ||
| } | ||
| }; | ||
| window.addEventListener("beforeunload", handleBeforeUnload); | ||
| return () => { | ||
| window.removeEventListener("beforeunload", handleBeforeUnload); | ||
| }; | ||
| }, [form.formState.isDirty]); | ||
|
|
There was a problem hiding this comment.
I don't see any logic to show dialog?
| "retell-ai", | ||
| "synthflow", | ||
| "telli", | ||
| "link-as-an-app", |
There was a problem hiding this comment.
seems unintentional change, revert this change.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/modules/event-types/components/EventTypeWebWrapper.tsx">
<violation number="1" location="apps/web/modules/event-types/components/EventTypeWebWrapper.tsx:330">
P2: Unsaved-changes warning is incomplete: it only intercepts `pushState`, leaving `replaceState` and browser back/forward navigation (`popstate`) unhandled. Users can bypass the warning and lose edits by using router.replace() or the browser back button.</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.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/modules/event-types/components/EventTypeWebWrapper.tsx">
<violation number="1" location="apps/web/modules/event-types/components/EventTypeWebWrapper.tsx:353">
P2: Popstate interception reverts to a stale URL captured at mount, which can leave the user on the wrong tab/page when blocking navigation after a later in-page navigation. The `currentPageUrl` is captured once when the effect mounts but never updated when users navigate between tabs. If a user switches tabs while the form is clean, then makes changes and presses back, `currentPageUrl` will push them back to the original tab instead of keeping them on the current tab. Fix: Update `currentPageUrl` after each allowed navigation, use the current `window.location.href` in the handler, or store the URL before navigating in popstate.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Hey @TheSeydiCharyyev, great start on this fix! 👋 I was working on the same issue and noticed there are a couple of additional cases that aren't covered yet:
Happy to collaborate or share the implementation if it's helpful. Just wanted to make sure all the edge cases are covered for a complete fix! 🙌 |
|
This PR has been closed due to inactivity. Please feel free to reopen it if you'd like to continue the work. |
|
Hi @romitg2, this PR was automatically closed due to inactivity. All your review comments from v1 have been fully addressed across 4 iterations:
Could you please reopen this PR and re-review when you get a chance? Happy to make any additional changes if needed. Thank you! |
|
Hey, thanks for your work. We require video demo for prs involving ui/ux changes. Please add one |
|
Thanks @romitg2! I'll record a video demo and post it here shortly. |
|
Addressed cubic P1 comment: moved window.history.length access from render-time ref initialization into useEffect to prevent breakage in non-browser render contexts. |
|
Addressed in v8 (commit 3b4e31b): |
|
@cubic-dev-ai The P1 issue (render-time window access) was fixed in v8 (commit 3b4e31b) — |
@TheSeydiCharyyev I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/modules/event-types/components/EventTypeWebWrapper.tsx">
<violation number="1" location="apps/web/modules/event-types/components/EventTypeWebWrapper.tsx:522">
P2: Discard sets dirty ref false before navigation completes, allowing dirty-state/ref desync and suppressing later in-app unsaved-change prompts if navigation is aborted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
Addressed cubic P2: |
@TheSeydiCharyyev That makes sense. By moving Capturing the I also see you addressed the P1 by moving These changes look solid and resolve the previous concerns. Helpful links: |
📝 WalkthroughWalkthroughThe pull request implements unsaved changes detection and handling across the event type creation workflow. A new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/app-store/redirect-apps.generated.ts (1)
30-30:⚠️ Potential issue | 🟡 MinorRevert the generated artifact and change its source instead.
This file is marked autogenerated, so adding
"link-as-an-app"here is brittle and will be overwritten the next timeapp-store:buildruns. If this redirect target is really needed, please update the source that feeds generation and regenerate the file.As per coding guidelines, "Never modify
*.generated.tsfiles directly - they're created by app-store-cli".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-store/redirect-apps.generated.ts` at line 30, The change added the literal "link-as-an-app" to the autogenerated array in redirect-apps.generated.ts which will be overwritten; revert this edit in redirect-apps.generated.ts and instead locate and update the source input that feeds the generator (the app-store CLI source/manifest that defines redirect targets) to include "link-as-an-app", then run the generator (app-store:build) to regenerate redirect-apps.generated.ts so the new entry is persisted; do not modify any *.generated.ts files directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/modules/event-types/components/EventTypeWebWrapper.tsx`:
- Around line 329-375: The useEffect currently calls window.history.pushState
unconditionally on mount and doesn't clean it up, causing duplicate history
entries; update the effect that defines useEffect/handlePopState/handleClick so
pushState is only added when needed (e.g., when formIsDirtyRef.current is true
or when we haven't already pushed) and record a flag like hasInjectedHistoryRef
to avoid repeat injects, and add a cleanup that removes the injected entry on
unmount (use skipPopStateRef to avoid triggering the pop handler and call
window.history.back() if you injected an entry) as well as removing any event
listeners; references to locate changes: useEffect, handlePopState,
skipPopStateRef, formIsDirtyRef, unsavedChangesPendingUrl, and the unconditional
window.history.pushState call.
- Around line 315-326: The beforeunload handler in EventTypeWebWrapper currently
checks form.formState.isDirty (handleBeforeUnload) which can conflict with the
discard flow that flips formIsDirtyRef.current; update handleBeforeUnload to
read formIsDirtyRef.current instead of form.formState.isDirty and, when dirty,
set e.returnValue = '' (and call e.preventDefault()) so the browser shows the
prompt; keep adding/removing the listener in the same useEffect but remove the
form.formState.isDirty dependency (refs don’t need to be in the dependency
array).
---
Duplicate comments:
In `@packages/app-store/redirect-apps.generated.ts`:
- Line 30: The change added the literal "link-as-an-app" to the autogenerated
array in redirect-apps.generated.ts which will be overwritten; revert this edit
in redirect-apps.generated.ts and instead locate and update the source input
that feeds the generator (the app-store CLI source/manifest that defines
redirect targets) to include "link-as-an-app", then run the generator
(app-store:build) to regenerate redirect-apps.generated.ts so the new entry is
persisted; do not modify any *.generated.ts files directly.
🪄 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: 25de1c3c-abe9-4ba5-a3d4-b0c54f26ae38
📒 Files selected for processing (4)
apps/web/modules/event-types/components/EventTypeWebWrapper.tsxapps/web/modules/event-types/components/dialogs/UnsavedChangesDialog.tsxpackages/app-store/redirect-apps.generated.tspackages/i18n/locales/en/common.json
| // Warn before closing/refreshing the page with unsaved changes | ||
| useEffect(() => { | ||
| const handleBeforeUnload = (e: BeforeUnloadEvent) => { | ||
| if (form.formState.isDirty) { | ||
| e.preventDefault(); | ||
| } | ||
| }; | ||
| window.addEventListener("beforeunload", handleBeforeUnload); | ||
| return () => { | ||
| window.removeEventListener("beforeunload", handleBeforeUnload); | ||
| }; | ||
| }, [form.formState.isDirty]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its size
fd -t f "EventTypeWebWrapper.tsx" apps/web/modules/event-types/components/ | head -5Repository: calcom/cal.com
Length of output: 121
🏁 Script executed:
# Read the specific lines mentioned in the review (315-326) plus surrounding context
wc -l apps/web/modules/event-types/components/EventTypeWebWrapper.tsxRepository: calcom/cal.com
Length of output: 125
🏁 Script executed:
# Get the beforeunload handler and surrounding context
sed -n '310,340p' apps/web/modules/event-types/components/EventTypeWebWrapper.tsxRepository: calcom/cal.com
Length of output: 1092
🏁 Script executed:
# Search for formIsDirtyRef definition and all its usages
rg "formIsDirtyRef" apps/web/modules/event-types/components/EventTypeWebWrapper.tsx -B2 -A2Repository: calcom/cal.com
Length of output: 1426
🏁 Script executed:
# Find the discard flow (lines 521-535 per review)
sed -n '515,540p' apps/web/modules/event-types/components/EventTypeWebWrapper.tsxRepository: calcom/cal.com
Length of output: 1045
Use formIsDirtyRef.current in the beforeunload handler to match the discard flow.
Line 318 reads form.formState.isDirty, but the discard dialog clears formIsDirtyRef.current before navigation (lines 521–535). If history.go(-2) unloads to an external page, the beforeunload handler still sees the RHF state as dirty and shows a prompt, even though the user already confirmed discarding. This handler should check the ref and set returnValue for consistent behavior.
Suggested fix
useEffect(() => {
const handleBeforeUnload = (e: BeforeUnloadEvent) => {
- if (form.formState.isDirty) {
- e.preventDefault();
- }
+ if (!formIsDirtyRef.current) return;
+ e.preventDefault();
+ e.returnValue = "";
};
window.addEventListener("beforeunload", handleBeforeUnload);
return () => {
window.removeEventListener("beforeunload", handleBeforeUnload);
};
- }, [form.formState.isDirty]);
+ }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/modules/event-types/components/EventTypeWebWrapper.tsx` around lines
315 - 326, The beforeunload handler in EventTypeWebWrapper currently checks
form.formState.isDirty (handleBeforeUnload) which can conflict with the discard
flow that flips formIsDirtyRef.current; update handleBeforeUnload to read
formIsDirtyRef.current instead of form.formState.isDirty and, when dirty, set
e.returnValue = '' (and call e.preventDefault()) so the browser shows the
prompt; keep adding/removing the listener in the same useEffect but remove the
form.formState.isDirty dependency (refs don’t need to be in the dependency
array).
| useEffect(() => { | ||
| hadPriorHistoryRef.current = window.history.length > 1; | ||
|
|
||
| const handleClick = (e: MouseEvent) => { | ||
| if (!formIsDirtyRef.current) return; | ||
|
|
||
| // Don't intercept modifier clicks (new tab/window) | ||
| if (e.metaKey || e.ctrlKey || e.shiftKey || e.altKey || e.button !== 0) return; | ||
|
|
||
| const target = (e.target as HTMLElement).closest("a"); | ||
| if (!target) return; | ||
|
|
||
| // Don't intercept links with target attribute (e.g. target="_blank") | ||
| if (target.getAttribute("target")) return; | ||
|
|
||
| const href = target.getAttribute("href"); | ||
| if (!href || href.startsWith("#") || href.startsWith("http") || href.startsWith("mailto:")) return; | ||
|
|
||
| // Only intercept internal navigation to different pages | ||
| if (href === window.location.pathname) return; | ||
|
|
||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| unsavedChangesPendingUrl.current = href; | ||
| setIsOpenUnsavedChangesDialog(true); | ||
| }; | ||
|
|
||
| const handlePopState = () => { | ||
| if (skipPopStateRef.current) { | ||
| skipPopStateRef.current = false; | ||
| return; | ||
| } | ||
| if (formIsDirtyRef.current) { | ||
| // Push current URL back to prevent leaving the page | ||
| window.history.pushState(null, "", window.location.href); | ||
| unsavedChangesPendingUrl.current = null; | ||
| setIsOpenUnsavedChangesDialog(true); | ||
| } else { | ||
| // Form is clean — continue back navigation past the extra history entry | ||
| skipPopStateRef.current = true; | ||
| window.history.back(); | ||
| } | ||
| }; | ||
|
|
||
| // Add extra history entry so back button triggers popstate instead of leaving | ||
| window.history.pushState(null, "", window.location.href); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the EventTypeWebWrapper.tsx file
find . -type f -name "EventTypeWebWrapper.tsx" | head -5Repository: calcom/cal.com
Length of output: 123
🏁 Script executed:
# Read the complete useEffect hook to check dependency array and implementation
cat -n apps/web/modules/event-types/components/EventTypeWebWrapper.tsx | sed -n '320,380p'Repository: calcom/cal.com
Length of output: 2632
🏁 Script executed:
# Get more context to understand formIsDirtyRef initialization and updates
cat -n apps/web/modules/event-types/components/EventTypeWebWrapper.tsx | sed -n '1,100p'Repository: calcom/cal.com
Length of output: 4641
🏁 Script executed:
# Read lines around 375-390 to see the complete cleanup and dependency array
cat -n apps/web/modules/event-types/components/EventTypeWebWrapper.tsx | sed -n '375,395p'Repository: calcom/cal.com
Length of output: 780
🏁 Script executed:
# Look for where formIsDirtyRef is set or updated in the component
rg "formIsDirtyRef" apps/web/modules/event-types/components/EventTypeWebWrapper.tsx -B 2 -A 2Repository: calcom/cal.com
Length of output: 1426
🏁 Script executed:
# Check for any other useEffect hooks that might handle cleanup of history entries
rg "useEffect|history\." apps/web/modules/event-types/components/EventTypeWebWrapper.tsx | head -20Repository: calcom/cal.com
Length of output: 459
🏁 Script executed:
# Search for any other history cleanup or manipulation elsewhere in the component
rg "history\." apps/web/modules/event-types/components/EventTypeWebWrapper.tsx | grep -v "hadPriorHistoryRef\|handlePopState\|skipPopStateRef" | head -10Repository: calcom/cal.com
Length of output: 253
🏁 Script executed:
# Check if there are any other effects that might clean up or manage the history entry
rg "cleanup|return|unload" apps/web/modules/event-types/components/EventTypeWebWrapper.tsx | head -15Repository: calcom/cal.com
Length of output: 693
Remove or condition the unconditional pushState call on mount, and ensure cleanup.
Line 374 pushes a history entry unconditionally on every mount with no cleanup. Each revisit to this page adds another identical entry, causing the back button to walk through duplicates. The skipPopStateRef workaround only handles controlled back-navigation flow, not accumulation across remounts. Either condition the entry to be added only when the form is dirty, or explicitly remove it in the cleanup function when the component unmounts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/modules/event-types/components/EventTypeWebWrapper.tsx` around lines
329 - 375, The useEffect currently calls window.history.pushState
unconditionally on mount and doesn't clean it up, causing duplicate history
entries; update the effect that defines useEffect/handlePopState/handleClick so
pushState is only added when needed (e.g., when formIsDirtyRef.current is true
or when we haven't already pushed) and record a flag like hasInjectedHistoryRef
to avoid repeat injects, and add a cleanup that removes the injected entry on
unmount (use skipPopStateRef to avoid triggering the pop handler and call
window.history.back() if you injected an entry) as well as removing any event
listeners; references to locate changes: useEffect, handlePopState,
skipPopStateRef, formIsDirtyRef, unsavedChangesPendingUrl, and the unconditional
window.history.pushState call.
d6f96a6 to
969d1cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/modules/event-types/components/EventTypeWebWrapper.tsx (2)
315-326:⚠️ Potential issue | 🟠 MajorUse
formIsDirtyRef.currentfor consistency with the discard flow.The beforeunload handler reads
form.formState.isDirty, but the discard flow (lines 525, 529, 533) clearsformIsDirtyRef.currentbefore navigation. This mismatch could cause the browser prompt to appear even after the user confirmed discarding, since React state updates are asynchronous. Also, some browsers requiree.returnValueto be set.Suggested fix
useEffect(() => { const handleBeforeUnload = (e: BeforeUnloadEvent) => { - if (form.formState.isDirty) { - e.preventDefault(); - } + if (!formIsDirtyRef.current) return; + e.preventDefault(); + e.returnValue = ""; }; window.addEventListener("beforeunload", handleBeforeUnload); return () => { window.removeEventListener("beforeunload", handleBeforeUnload); }; - }, [form.formState.isDirty]); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/modules/event-types/components/EventTypeWebWrapper.tsx` around lines 315 - 326, The beforeunload handler uses form.formState.isDirty which can be out-of-sync with the discard flow that clears formIsDirtyRef.current; update the effect (useEffect) and handler (handleBeforeUnload) to read formIsDirtyRef.current instead of form.formState.isDirty and ensure cross-browser behavior by setting e.returnValue = '' when dirty; keep the same addEventListener/removeEventListener usage and dependency on formIsDirtyRef (or an empty deps array if you only read the ref) so the prompt is suppressed after formIsDirtyRef.current is cleared during discard.
373-383:⚠️ Potential issue | 🟠 MajorHistory entry accumulates on remount without cleanup.
Line 374 unconditionally pushes a history entry on every mount, but the cleanup only removes event listeners—it doesn't remove the injected entry. If this component remounts (e.g., tab switches, HMR), duplicate entries accumulate, causing the back button to step through identical entries.
Consider either:
- Track whether the entry was already injected using a ref and skip if already pushed
- Clean up the injected entry on unmount (set
skipPopStateRef.current = truethen callhistory.back())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/modules/event-types/components/EventTypeWebWrapper.tsx` around lines 373 - 383, The component unconditionally calls window.history.pushState on mount but the cleanup only removes listeners, causing duplicate history entries on remount; modify the effect around pushState/cleanup: either guard the push with a ref (e.g., introduced injectedHistoryRef) so pushState is only called once across mounts, or record that you injected an entry (e.g., skipPopStateRef.current = true) and on unmount perform a cleanup by setting skipPopStateRef and calling history.back() to remove the injected entry; ensure this logic ties into the existing handlePopState and handleClick handlers so you don't trigger your own pop handling when programmatically navigating back.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/modules/event-types/components/EventTypeWebWrapper.tsx`:
- Line 5: The import list in EventTypeWebWrapper.tsx includes useCallback which
is never used; remove useCallback from the named imports (the import line that
currently reads import { useCallback, useEffect, useRef, useState } from
"react";) so the file only imports the hooks actually used (useEffect, useRef,
useState).
---
Duplicate comments:
In `@apps/web/modules/event-types/components/EventTypeWebWrapper.tsx`:
- Around line 315-326: The beforeunload handler uses form.formState.isDirty
which can be out-of-sync with the discard flow that clears
formIsDirtyRef.current; update the effect (useEffect) and handler
(handleBeforeUnload) to read formIsDirtyRef.current instead of
form.formState.isDirty and ensure cross-browser behavior by setting
e.returnValue = '' when dirty; keep the same
addEventListener/removeEventListener usage and dependency on formIsDirtyRef (or
an empty deps array if you only read the ref) so the prompt is suppressed after
formIsDirtyRef.current is cleared during discard.
- Around line 373-383: The component unconditionally calls
window.history.pushState on mount but the cleanup only removes listeners,
causing duplicate history entries on remount; modify the effect around
pushState/cleanup: either guard the push with a ref (e.g., introduced
injectedHistoryRef) so pushState is only called once across mounts, or record
that you injected an entry (e.g., skipPopStateRef.current = true) and on unmount
perform a cleanup by setting skipPopStateRef and calling history.back() to
remove the injected entry; ensure this logic ties into the existing
handlePopState and handleClick handlers so you don't trigger your own pop
handling when programmatically navigating back.
🪄 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: a28d6745-e065-4c69-b977-9772fcde8ee4
📒 Files selected for processing (2)
apps/web/modules/event-types/components/EventTypeWebWrapper.tsxapps/web/modules/event-types/components/dialogs/UnsavedChangesDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/modules/event-types/components/dialogs/UnsavedChangesDialog.tsx
| import dynamic from "next/dynamic"; | ||
| import { usePathname, useRouter as useAppRouter } from "next/navigation"; | ||
| import { useEffect, useRef, useState } from "react"; | ||
| import { useCallback, useEffect, useRef, useState } from "react"; |
There was a problem hiding this comment.
Remove unused useCallback import.
useCallback is imported but not used anywhere in this file.
Suggested fix
-import { useCallback, useEffect, useRef, useState } from "react";
+import { useEffect, useRef, useState } from "react";📝 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.
| import { useCallback, useEffect, useRef, useState } from "react"; | |
| import { useEffect, useRef, useState } from "react"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/modules/event-types/components/EventTypeWebWrapper.tsx` at line 5,
The import list in EventTypeWebWrapper.tsx includes useCallback which is never
used; remove useCallback from the named imports (the import line that currently
reads import { useCallback, useEffect, useRef, useState } from "react";) so the
file only imports the hooks actually used (useEffect, useRef, useState).
Summary
Added a "beforeunload" event handler to the event type setup page that warns users when they attempt to close or refresh the browser tab with unsaved form changes.
Fixes #10180
Changes Made
How it was tested