fix: clicking on previous step navigates to correct step instead of next#22146
fix: clicking on previous step navigates to correct step instead of next#22146anikdhabal merged 17 commits intocalcom:mainfrom
Conversation
|
@subediDarshan is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
|
github-actions
bot
added
ui
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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.
Devanshusharma2005
left a comment
Devanshusharma2005
left a comment
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
| 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
kart1ka
left a comment
kart1ka
left a comment
There was a problem hiding this comment.
Looks good. Thanks for the PR.
| navigateToStep={goToStep} | ||
| 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. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces step navigation callback from nextStep to navigateToStep(step: number) across the wizard and onboarding flows. Steps component types and click handler were updated to call navigateToStep with the clicked index. OnboardingPage and WizardForm now pass a goToStep function to Steps. useWizardState.goToStep was adjusted to convert a zero-based input to a one-based clamped step. WizardLayout removes nextStep={noop} and uses disableNavigation. Tests updated to match the new prop name. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ 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
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (1)
127-132: Bounds-check and ignore redundant navigation in goToStep (repeat from earlier review).Without bounds checking, an out-of-range step leads to newStep being undefined and stepTransform(newStep) falling back to INITIAL_STEP. Also, clicking the current step should no-op to avoid a needless route push.
Apply this diff:
- const goToStep = (step: number) => { - const newStep = steps[step]; - startTransition(() => { - router.push(`/getting-started/${stepTransform(newStep)}`); - }); - }; + const goToStep = (step: number) => { + // Guard against invalid or redundant indices + if (step < 0 || step >= steps.length || step === currentStepIndex) return; + const newStep = steps[step]; + startTransition(() => { + router.push(`/getting-started/${stepTransform(newStep)}`); + }); + };
🧹 Nitpick comments (1)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (1)
157-159: Localize fallback text for missing title.Per guidelines, avoid hardcoded UI strings. Use t() for the fallback.
- {headers[currentStepIndex]?.title || "Undefined title"} + {headers[currentStepIndex]?.title || t("undefined_title", "Undefined title")}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (1)
167-167: Index alignment verified – no changes requiredVerified that the
Stepscomponent passes a 0-basedindextonavigateToStep, which aligns with the 0-based indexing expected bygoToStep. The current implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (1)
128-133: Harden goToStep: clamp index and avoid redundant navigationCheap defensive guard to prevent OOB access and unnecessary route pushes.
- const goToStep = (step: number) => { - const newStep = steps[step]; - startTransition(() => { - router.push(`/getting-started/${stepTransform(newStep)}`); - }); - }; + const goToStep = (step: number) => { + const clamped = Math.max(0, Math.min(step, steps.length - 1)); + if (clamped === currentStepIndex) return; + const newStep = steps[clamped]; + if (!newStep) return; // defensive + startTransition(() => { + router.push(`/getting-started/${stepTransform(newStep)}`); + }); + };Run to confirm the index base used by Steps.navigateToStep across the repo (0-based vs 1-based); adjust by
step - 1if needed:#!/bin/bash # Inspect Steps props/usage rg -nP -C3 '\bnavigateToStep\b' --type=ts --type=tsx rg -nP -C3 '\bcurrentStep\b.*\{|\bmaxSteps\b.*\{' --type=ts --type=tsx # Try to locate Steps component definition fd -t f -a 'Steps.tsx' | xargs -I{} rg -nP -C3 'navigateToStep|currentStep|maxSteps' {}
🧹 Nitpick comments (1)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx (1)
169-169: Steps wiring looks correct; verify index base and fix nearby i18n nit
- currentStep is 1-based; confirm navigateToStep receives a 0-based index from Steps to avoid off-by-one.
Optional i18n tidy (outside this hunk): replace hardcoded “Undefined title” with a localized fallback.
- {headers[currentStepIndex]?.title || "Undefined title"} + {headers[currentStepIndex]?.title || t("undefined_title")}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/modules/getting-started/[[...step]]/onboarding-view.tsx
7 participants
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.