fix: clicking on previous step navigates to correct step instead of next #22146
Conversation
|
@subediDarshan is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
|
Graphite Automations"Add community label" took an action on this PR • (06/30/25)1 label was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (07/09/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
cubic found 2 issues across 5 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
There was a problem hiding this comment.
Hey @subediDarshan thanks for the pr , can you please fix the issues suggested by cubic. and also can you make a loom for making sure that the steps only go to previous adjacent one or any step in the previous ?
|
Hey @Devanshusharma2005 pr-3.mp4 |
E2E results are ready! |
|
@subediDarshan pls fix the conflicts |
…bediDarshan/cal.com into fix/step-indicator-navigation
| @@ -25,7 +25,7 @@ export function useWizardState(defaultStep = 1, maxSteps: number) { | |||
|
|
|||
| const goToStep = useCallback( | |||
| (newStep: number) => { | |||
| setStep(Math.min(Math.max(newStep, 1), maxSteps)); | |||
| setStep(Math.min(Math.max(newStep + 1, 1), maxSteps)); | |||
There was a problem hiding this comment.
In 'Steps' component (where this 'goToStep' function is passed as prop),
0-based indexes are passed in goToFunction as newStep,
but 1-based indexes is being used in URL query param (?step=1),
So, +1 conversion is needed here
There was a problem hiding this comment.
pls add a comment in the code mentioning the same
| stepLabel={stepLabel} | ||
| data-testid="wizard-step-component" | ||
| disableNavigation={disableNavigation} |
There was a problem hiding this comment.
What is the reason for removal of this props?
There was a problem hiding this comment.
By not passing this prop, disableNavigation=false is set by default (default parameter set in Steps component), which is what we need since we are navigating between previous steps here through 'navigateToStep' function.
|
This PR is being marked as stale due to inactivity. |
WalkthroughThis set of changes refactors the navigation logic for multi-step forms and wizards across several components and modules. The primary update replaces the previous "next step" navigation pattern with a more general "go to step" approach, allowing direct navigation to any step by index. This involves renaming props and types from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Complexity label: Moderate Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/app-store/googlecalendar/lib/CalendarService.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/ui/components/form/step/Steps.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx(2 hunks)packages/app-store/googlecalendar/lib/CalendarService.ts(1 hunks)packages/ui/components/form/step/Steps.tsx(3 hunks)packages/ui/components/form/step/steps.test.tsx(1 hunks)packages/ui/components/form/wizard/WizardForm.tsx(2 hunks)packages/ui/components/form/wizard/useWizardState.ts(1 hunks)packages/ui/components/layout/WizardLayout.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/ui/components/layout/WizardLayout.tsxpackages/ui/components/form/step/steps.test.tsxpackages/ui/components/form/wizard/useWizardState.tsapps/web/modules/getting-started/[[...step]]/onboarding-view.tsxpackages/ui/components/form/wizard/WizardForm.tsxpackages/app-store/googlecalendar/lib/CalendarService.tspackages/ui/components/form/step/Steps.tsx
🧠 Learnings (4)
packages/ui/components/form/wizard/useWizardState.ts (1)
Learnt from: alishaz-polymath
PR: #22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the currentLink.maxUsageCount ?? 1 fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (1)
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
packages/ui/components/form/wizard/WizardForm.tsx (1)
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
packages/app-store/googlecalendar/lib/CalendarService.ts (1)
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.770Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
🧬 Code Graph Analysis (1)
packages/ui/components/form/wizard/WizardForm.tsx (1)
packages/ui/components/form/wizard/useWizardState.ts (1)
useWizardState(22-55)
🔇 Additional comments (10)
packages/ui/components/form/step/steps.test.tsx (1)
16-16: LGTM! Test prop updated to match component interface.The prop rename from
nextSteptonavigateToStepcorrectly aligns with the component interface changes while maintaining the same test coverage.packages/ui/components/layout/WizardLayout.tsx (1)
60-60: LGTM! UsingdisableNavigationis cleaner than noop functions.Explicitly disabling navigation is more intentional and readable than passing a no-operation function. This change aligns well with the component's purpose in the wizard layout.
packages/ui/components/form/wizard/useWizardState.ts (1)
28-29: LGTM! Index conversion is correctly implemented.The conversion from 0-based component interface to 1-based URL parameters is properly handled with appropriate bounds checking. The added comment clearly explains the conversion logic.
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (1)
167-167: LGTM! Navigation prop correctly updated.The change from
nextSteptonavigateToStepwith the newgoToStepfunction properly enables navigation to any previous step, which aligns with the PR objective of fixing the navigation behavior.packages/ui/components/form/wizard/WizardForm.tsx (2)
49-49: LGTM! Correctly extracting goToStep for navigation.Adding
goToStepto the destructured return enables the step navigation functionality that was previously unused.
74-74: LGTM! Navigation prop updated consistently with the refactor.The change from
nextSteptonavigateToStepwithgoToStepfunction enables direct navigation to any step, which aligns with the PR objective and the broader refactor pattern across the codebase.packages/ui/components/form/step/Steps.tsx (4)
6-6: LGTM - Type definition correctly updated.The function signature change from
nextSteptonavigateToStep: (step: number) => voidproperly supports navigation to any step by index, aligning with the PR objective.
14-14: LGTM - Maintains type safety for disabled navigation.The optional undefined type correctly excludes navigation functionality when
disableNavigationis true.
26-26: LGTM - Prop destructuring updated consistently.The prop name change from
nextSteptonavigateToStepis consistent with the type definition updates.
38-38: LGTM - Click handler correctly passes step index.The navigation logic now correctly passes the clicked step's index to
navigateToStep. This fixes the issue described in the PR where clicking previous steps would navigate to the next step instead.Note: The handler passes zero-based indices - ensure consuming components handle this correctly.
| const goToStep = (step: number) => { | ||
| const newStep = steps[step]; | ||
| startTransition(() => { | ||
| router.push(`/getting-started/${stepTransform(newStep)}`); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking for step parameter.
The goToStep function should validate that the step parameter is within valid bounds before accessing steps[step]. Currently, if step is out of bounds, it will cause steps[step] to return undefined, leading to unexpected navigation to INITIAL_STEP.
const goToStep = (step: number) => {
+ if (step < 0 || step >= steps.length) {
+ return; // or throw an error, or handle gracefully
+ }
const newStep = steps[step];
startTransition(() => {
router.push(`/getting-started/${stepTransform(newStep)}`);
});
};📝 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 goToStep = (step: number) => { | |
| const newStep = steps[step]; | |
| startTransition(() => { | |
| router.push(`/getting-started/${stepTransform(newStep)}`); | |
| }); | |
| }; | |
| const goToStep = (step: number) => { | |
| if (step < 0 || step >= steps.length) { | |
| return; // or throw an error, or handle gracefully | |
| } | |
| const newStep = steps[step]; | |
| startTransition(() => { | |
| router.push(`/getting-started/${stepTransform(newStep)}`); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx around lines
127 to 132, the goToStep function lacks bounds checking for the step parameter,
which can cause steps[step] to be undefined and lead to incorrect navigation.
Add a check to ensure step is within the valid range of the steps array indices
before accessing steps[step]. If step is out of bounds, handle it gracefully,
for example by returning early or defaulting to a valid step, to prevent
unexpected navigation.
| const oneDayMs = 1000 * 60 * 60 * 24; | ||
| const diff = Math.floor((toDate.getTime() - fromDate.getTime()) / (oneDayMs)); | ||
| const diff = Math.floor((toDate.getTime() - fromDate.getTime()) / oneDayMs); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Reuse existing MS_PER_DAY constant and guard against over-90-day edge case
You redeclare the day-length constant and still rely on Math.floor().
Two issues arise:
- Duplication/maintainability –
MS_PER_DAY(line 38) already captures the same value. - Off-by-one risk –
Math.floor()lets a range of “90 days + ε” (e.g. 90 days 23 h) slip through as 90, provoking a 400 error from Google’s 90-day cap.
Consider:
-const oneDayMs = 1000 * 60 * 60 * 24;
-const diff = Math.floor((toDate.getTime() - fromDate.getTime()) / oneDayMs);
+// Use the shared constant for consistency and avoid silent “90 days + ε” passes.
+const diff = (toDate.getTime() - fromDate.getTime()) / MS_PER_DAY;
+
+// Google API allows *up to* 90 full days
+if (diff > 90) {
+ // fall-back to chunking logic below
+} else {
+ ...
+}This eliminates duplication and relies on a direct millisecond check, preventing borderline ranges from bypassing the limit.
🏁 Script executed:
#!/bin/bash
set -e
echo "🔍 Searching for MS_PER_DAY occurrences..."
rg -n "MS_PER_DAY" packages/app-store/googlecalendar/lib/CalendarService.ts || true
echo
echo "📄 Showing top of file (lines 1–100) to locate the constant definition..."
sed -n '1,100p' packages/app-store/googlecalendar/lib/CalendarService.ts
echo
echo "📋 Showing lines 670–720 around the oneDayMs/diff snippet..."
sed -n '670,720p' packages/app-store/googlecalendar/lib/CalendarService.tsLength of output: 6691
Use shared MS_PER_DAY and prevent “90 days + ε” slip
Rather than redeclaring a local oneDayMs and flooring to truncate, reuse the existing MS_PER_DAY constant and compare the raw millisecond difference against 90 * MS_PER_DAY. This removes duplication and avoids allowing 90 days plus fractional hours to be treated as exactly 90 days.
• File: packages/app-store/googlecalendar/lib/CalendarService.ts
Lines ~675–680
Suggested diff:
- const oneDayMs = 1000 * 60 * 60 * 24;
- const diff = Math.floor((toDate.getTime() - fromDate.getTime()) / oneDayMs);
+ const diffMs = toDate.getTime() - fromDate.getTime();
+
+ // Google API allows up to 90 full days
+ if (diffMs <= 90 * MS_PER_DAY) {
+ // single-range freebusy call
+ const freeBusyData = await this.getCacheOrFetchAvailability(
+ { timeMin: dateFrom, timeMax: dateTo, items: calendarIds.map((id) => ({ id })) },
+ shouldServeCache
+ );
+
+ if (!freeBusyData) throw new Error("No response from google calendar");
+ return freeBusyData.map((b) => ({ start: b.start, end: b.end }));
+ }
+
+ // chunk into multiple 90-day windows
// …This ensures consistency with the shared constant and prevents borderline ranges (e.g. 90 days + 23 h) from bypassing the cap.
📝 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 oneDayMs = 1000 * 60 * 60 * 24; | |
| const diff = Math.floor((toDate.getTime() - fromDate.getTime()) / (oneDayMs)); | |
| const diff = Math.floor((toDate.getTime() - fromDate.getTime()) / oneDayMs); | |
| const diffMs = toDate.getTime() - fromDate.getTime(); | |
| // Google API allows up to 90 full days | |
| if (diffMs <= 90 * MS_PER_DAY) { | |
| // single-range freebusy call | |
| const freeBusyData = await this.getCacheOrFetchAvailability( | |
| { timeMin: dateFrom, timeMax: dateTo, items: calendarIds.map((id) => ({ id })) }, | |
| shouldServeCache | |
| ); | |
| if (!freeBusyData) throw new Error("No response from google calendar"); | |
| return freeBusyData.map((b) => ({ start: b.start, end: b.end })); | |
| } | |
| // chunk into multiple 90-day windows | |
| // … |
🤖 Prompt for AI Agents
In packages/app-store/googlecalendar/lib/CalendarService.ts around lines 679 to
681, replace the local declaration of oneDayMs with the shared MS_PER_DAY
constant. Instead of flooring the day difference, compare the raw millisecond
difference directly against 90 times MS_PER_DAY to avoid counting fractional
days as full days. This removes duplication and ensures the 90-day cap is
enforced precisely.
What does this PR do?
This PR ensures that when a user clicks on a previous step's progress bar segment (white bar), they are navigated to that specific step, rather than the next step. This aligns with expected user behavior.
Visual Demo
Before:
Bug-Capture-qyF3Ypn8-wNKkCV3jrjEGbZKBCTI8Xg2iuVoDxqxh0Ew.mp4
After:
Untitled.video.-.Made.with.Clipchamp.1.mp4
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
http://localhost:3000/getting-started/connected-video.Checklist
I haven't read the contributing guideMy code doesn't follow the style guidelines of this projectI haven't commented my code, particularly in hard-to-understand areasI haven't checked if my changes generate no new warningsSummary by cubic
Clicking on a previous step in the progress bar now takes users to that exact step, instead of moving them to the next step. This makes navigation match what users expect.