feat: add guided tour#3051
Open
yashmehrotra wants to merge 6 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a guided tour with Joyride, touchpoint tracking, a Getting Started page, route wiring, and tour markers across the UI. Also updates Kratos error handling to use optional chaining for nested redirect fields. ChangesGuided Tour Feature
Kratos Error Handler Fix
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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: 3
🧹 Nitpick comments (1)
src/components/GuidedTour/__tests__/guidedTourSteps.unit.test.ts (1)
27-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for empty-param gating and PREV-at-zero behavior.
These two boundaries are currently untested and are easy to regress in the pure reducers.
Suggested test additions
describe("resolveAutoAdvance", () => { @@ it("does not advance when the gated param is absent", () => { expect(resolveAutoAdvance(steps, 2, ctx("/health"))).toBeNull(); }); + + it("does not advance when the gated param is present but empty", () => { + expect(resolveAutoAdvance(steps, 2, ctx("/health", "checkId="))).toBeNull(); + }); @@ describe("reduceTourEvent", () => { @@ it("goes back on a prev-button step:after event", () => { expect(reduceTourEvent({ ...base, action: ACTIONS.PREV })).toEqual({ run: true, stepIndex: 0 }); }); + + it("does not go below zero on prev from the first step", () => { + expect( + reduceTourEvent({ ...base, action: ACTIONS.PREV, index: 0 }) + ).toEqual({ + run: true, + stepIndex: 0 + }); + });Also applies to: 152-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/GuidedTour/__tests__/guidedTourSteps.unit.test.ts` around lines 27 - 50, Add two new test cases to the resolveAutoAdvance test suite. First, add a test that verifies the behavior when a parameter is present but empty (like "checkId=" with no value) to cover empty-param gating scenarios. Second, add a test that verifies what happens when attempting to navigate to the previous step (using PREV) when currently at step index 0, which is a boundary condition that should be tested. These tests should follow the same pattern as the existing tests in the suite and use the ctx() helper function with appropriate parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Authentication/Kratos/ory/hooks.ts`:
- Line 54: The code assigns err.response?.data?.redirect_browser_to directly to
window.location.href without verifying that redirect_browser_to is actually
defined. If Kratos returns an incomplete response where redirect_browser_to is
undefined, this will cause uncontrolled navigation. Add a guard condition to
check if redirect_browser_to exists before assigning it to window.location.href,
and implement a controlled fallback behavior (such as redirecting to a safe
route or resetting the state) when it is undefined. This same fix needs to be
applied to both instances mentioned (line 54 and line 85).
In `@src/components/GuidedTour/guidedTourSteps.ts`:
- Around line 588-590: The PREV action handler does not guard against
decrementing the step index below zero, which occurs when at index 0. Add a
guard condition in the block where action === ACTIONS.PREV to check if index is
greater than 0 before returning the decremented stepIndex. If the index is
already at 0, either return the current state unchanged or handle it
appropriately to prevent the invalid negative index that causes tour state
desyncing.
- Around line 522-523: The paramPresent variable in guidedTourSteps.ts currently
checks if the query parameter is non-null, but URLSearchParams.get() returns an
empty string for parameters like ?param=, which passes the null check and
incorrectly advances the tour. Modify the condition to also verify that the
parameter value is not an empty string, so only parameters with actual non-empty
values are treated as present for the auto-advance gate logic.
---
Nitpick comments:
In `@src/components/GuidedTour/__tests__/guidedTourSteps.unit.test.ts`:
- Around line 27-50: Add two new test cases to the resolveAutoAdvance test
suite. First, add a test that verifies the behavior when a parameter is present
but empty (like "checkId=" with no value) to cover empty-param gating scenarios.
Second, add a test that verifies what happens when attempting to navigate to the
previous step (using PREV) when currently at step index 0, which is a boundary
condition that should be tested. These tests should follow the same pattern as
the existing tests in the suite and use the ctx() helper function with
appropriate parameters.
🪄 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: 486ff875-9f0b-40c4-8be0-e4ef5ead1b0e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
package.jsonsrc/components/Authentication/Kratos/KratosUserProfileDropdown.tsxsrc/components/Authentication/Kratos/ory/hooks.tssrc/components/Canary/CanaryPopup/CheckDetails.tsxsrc/components/Canary/CanaryPopup/CheckDetailsTabs.tsxsrc/components/Canary/CanaryPopup/CheckRunNow.tsxsrc/components/Canary/CanaryTable.tsxsrc/components/Canary/index.tsxsrc/components/Configs/ConfigList/MRTConfigListColumn.tsxsrc/components/Configs/ConfigSummary/Cells/ConfigSummaryTableVirtualAggregateColumn.tsxsrc/components/Configs/ConfigSummary/ConfigSummaryList.tsxsrc/components/GuidedTour/GuidedTour.tsxsrc/components/GuidedTour/TourMenu.tsxsrc/components/GuidedTour/TourTooltip.tsxsrc/components/GuidedTour/__tests__/guidedTourSteps.unit.test.tssrc/components/GuidedTour/guidedTourState.tssrc/components/GuidedTour/guidedTourSteps.tssrc/components/Layout/AppSidebar.tsxsrc/components/Playbooks/Runs/PlaybookRunsList.tsxsrc/components/Playbooks/Runs/Submit/SubmitPlaybookRunForm.tsxsrc/components/Playbooks/Settings/PlaybookSpecCard.tsxsrc/components/Users/UserProfile.tsxsrc/components/VersionInfo/VersionInfo.tsxsrc/pages/config/ConfigList.tsxsrc/pages/config/details/ConfigDetailsPage.tsxsrc/pages/playbooks/PlaybookRunsDetails.tsxsrc/pages/views/components/ViewContainer.tsxsrc/ui/Buttons/DialogButton.tsxsrc/ui/Layout/SidebarLayout.tsxsrc/ui/Tabs/TabbedLinks.tsx
| case "session_refresh_required": | ||
| // We need to re-authenticate to perform this action | ||
| window.location.href = err.response?.data.redirect_browser_to; | ||
| window.location.href = err.response?.data?.redirect_browser_to; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard redirect_browser_to before assigning window.location.href
Both branches now allow undefined to flow into window.location.href. If Kratos returns a partial payload, this can trigger incorrect navigation rather than a controlled fallback. Add a null check (and fallback route/reset) before redirecting.
Suggested fix
case "session_refresh_required":
// We need to re-authenticate to perform this action
- window.location.href = err.response?.data?.redirect_browser_to;
+ if (err.response?.data?.redirect_browser_to) {
+ window.location.href = err.response.data.redirect_browser_to;
+ return;
+ }
+ resetFlow(undefined);
+ await router.push("/" + flowType);
return;
@@
case "browser_location_change_required":
// Ory Kratos asked us to point the user to this URL.
- window.location.href = err.response?.data?.redirect_browser_to;
+ if (err.response?.data?.redirect_browser_to) {
+ window.location.href = err.response.data.redirect_browser_to;
+ return;
+ }
+ resetFlow(undefined);
+ await router.push("/" + flowType);
return;Also applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Authentication/Kratos/ory/hooks.ts` at line 54, The code
assigns err.response?.data?.redirect_browser_to directly to window.location.href
without verifying that redirect_browser_to is actually defined. If Kratos
returns an incomplete response where redirect_browser_to is undefined, this will
cause uncontrolled navigation. Add a guard condition to check if
redirect_browser_to exists before assigning it to window.location.href, and
implement a controlled fallback behavior (such as redirecting to a safe route or
resetting the state) when it is undefined. This same fix needs to be applied to
both instances mentioned (line 54 and line 85).
Comment on lines
+522
to
+523
| const paramPresent = | ||
| !!data.advanceOnParam && ctx.params.get(data.advanceOnParam) != null; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat empty query-param values as absent for auto-advance gates.
URLSearchParams.get() returns "" for ?param=, and the current condition advances on that. This can skip gated steps before a real selection exists.
Suggested fix
- const paramPresent =
- !!data.advanceOnParam && ctx.params.get(data.advanceOnParam) != null;
+ const paramValue = data.advanceOnParam
+ ? ctx.params.get(data.advanceOnParam)
+ : null;
+ const paramPresent = paramValue != null && paramValue !== "";📝 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.
Suggested change
| const paramPresent = | |
| !!data.advanceOnParam && ctx.params.get(data.advanceOnParam) != null; | |
| const paramValue = data.advanceOnParam | |
| ? ctx.params.get(data.advanceOnParam) | |
| : null; | |
| const paramPresent = paramValue != null && paramValue !== ""; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/GuidedTour/guidedTourSteps.ts` around lines 522 - 523, The
paramPresent variable in guidedTourSteps.ts currently checks if the query
parameter is non-null, but URLSearchParams.get() returns an empty string for
parameters like ?param=, which passes the null check and incorrectly advances
the tour. Modify the condition to also verify that the parameter value is not an
empty string, so only parameters with actual non-empty values are treated as
present for the auto-advance gate logic.
Comment on lines
+588
to
+590
| if (action === ACTIONS.PREV) { | ||
| return { run: true, stepIndex: index - 1 }; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard PREV transitions from producing a negative step index.
A PREV event at index 0 currently returns -1, which is an invalid controlled index and can desync tour state.
Suggested fix
if (action === ACTIONS.PREV) {
- return { run: true, stepIndex: index - 1 };
+ return { run: true, stepIndex: Math.max(0, index - 1) };
}📝 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.
Suggested change
| if (action === ACTIONS.PREV) { | |
| return { run: true, stepIndex: index - 1 }; | |
| } | |
| if (action === ACTIONS.PREV) { | |
| return { run: true, stepIndex: Math.max(0, index - 1) }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/GuidedTour/guidedTourSteps.ts` around lines 588 - 590, The
PREV action handler does not guard against decrementing the step index below
zero, which occurs when at index 0. Add a guard condition in the block where
action === ACTIONS.PREV to check if index is greater than 0 before returning the
decremented stepIndex. If the index is already at 0, either return the current
state unchanged or handle it appropriately to prevent the invalid negative index
that causes tour state desyncing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/GuidedTour/GuidedTour.tsx`:
- Around line 109-118: The visibility check using `offsetParent === null` in the
GuidedTour component incorrectly identifies fixed-position elements as hidden,
causing tour targets in the AppSidebar (which uses position: fixed) to be
skipped. Replace this visibility check with a more robust approach that
tolerates fixed positioning by using getBoundingClientRect() to verify the
element has visible dimensions, or alternatively use getClientRects().length to
confirm the element renders. This will ensure fixed-position tour targets are
correctly identified as visible and clickable rather than being incorrectly
skipped.
🪄 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: a2dfcf41-26d9-464b-9cbd-2f2a345aae5d
📒 Files selected for processing (3)
src/components/GuidedTour/GuidedTour.tsxsrc/components/GuidedTour/TourMenu.tsxsrc/components/GuidedTour/guidedTourSteps.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/GuidedTour/guidedTourSteps.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/GuidedTour/TourMenu.tsx
Comment on lines
+109
to
+118
| if (element) { | ||
| // Present but hidden (e.g. the Graph tab on tall viewports) can never | ||
| // be clicked — skip now rather than waiting it out. | ||
| if (element.offsetParent === null) { | ||
| skip(); | ||
| return; | ||
| } | ||
| attached = element; | ||
| element.addEventListener("click", onClick, { once: true }); | ||
| return; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the data-tour targets and their containers to see if any are position:fixed.
rg -n 'data-tour' src --type=tsx --type=ts -C1 | head -n 80Repository: flanksource/flanksource-ui
Length of output: 192
🏁 Script executed:
# Find data-tour attributes in tsx/ts files without type filter
rg 'data-tour' src --type ts -C 1 | head -100Repository: flanksource/flanksource-ui
Length of output: 7141
🏁 Script executed:
# Inspect the GuidedTour.tsx file to understand the context better
ast-grep outline src/components/GuidedTour/GuidedTour.tsxRepository: flanksource/flanksource-ui
Length of output: 374
🏁 Script executed:
# Search for any CSS or inline styles that set position:fixed in relation to tour elements
rg 'position.*fixed' src --type ts --type js -C 2 | head -100Repository: flanksource/flanksource-ui
Length of output: 3291
🏁 Script executed:
# Read GuidedTour.tsx around lines 109-118 to see the full context
sed -n '100,130p' src/components/GuidedTour/GuidedTour.tsxRepository: flanksource/flanksource-ui
Length of output: 1037
🏁 Script executed:
# Check AppSidebar styling and positioning
grep -n 'className\|style\|position' src/components/Layout/AppSidebar.tsx | head -40Repository: flanksource/flanksource-ui
Length of output: 1583
🏁 Script executed:
# Search for CSS classes or Tailwind utilities that might apply position:fixed to AppSidebar
rg 'fixed|sticky' src/components/Layout/AppSidebar.tsx -B2 -A2Repository: flanksource/flanksource-ui
Length of output: 164
🏁 Script executed:
# View the full AppSidebar component to understand its structure
cat -n src/components/Layout/AppSidebar.tsx | head -100Repository: flanksource/flanksource-ui
Length of output: 3828
🏁 Script executed:
# Check what Sidebar component is being used and if it has fixed positioning
rg 'from.*Sidebar|import.*Sidebar' src/components/Layout/AppSidebar.tsx -A5Repository: flanksource/flanksource-ui
Length of output: 164
🏁 Script executed:
# Check the Layout or main app structure to see if AppSidebar is positioned fixed
rg -l 'AppSidebar' src --type ts --type tsx | xargs grep -l 'fixed\|sticky\|position' | head -5Repository: flanksource/flanksource-ui
Length of output: 192
🏁 Script executed:
# Find the sidebar component file
find src -name 'sidebar*' -type fRepository: flanksource/flanksource-ui
Length of output: 246
🏁 Script executed:
# Check the sidebar component for positioning styles
cat src/components/ui/sidebar.tsx | head -150Repository: flanksource/flanksource-ui
Length of output: 4669
🏁 Script executed:
# Search for position styling in the sidebar component
grep -n 'position\|fixed\|sticky' src/components/ui/sidebar.tsxRepository: flanksource/flanksource-ui
Length of output: 300
offsetParent === null misclassifies fixed-position targets as hidden.
The Sidebar component (used by AppSidebar) applies position: fixed via the CSS class "fixed" (sidebar.tsx:245). Since AppSidebar contains elements with data-tour attributes for navigation targets, these tour targets would have offsetParent === null despite being fully visible and clickable. The check at lines 109–118 would incorrectly skip() these targets. Consider a visibility check that tolerates fixed positioning (e.g. getBoundingClientRect() dimensions or getClientRects().length).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/GuidedTour/GuidedTour.tsx` around lines 109 - 118, The
visibility check using `offsetParent === null` in the GuidedTour component
incorrectly identifies fixed-position elements as hidden, causing tour targets
in the AppSidebar (which uses position: fixed) to be skipped. Replace this
visibility check with a more robust approach that tolerates fixed positioning by
using getBoundingClientRect() to verify the element has visible dimensions, or
alternatively use getClientRects().length to confirm the element renders. This
will ensure fixed-position tour targets are correctly identified as visible and
clickable rather than being incorrectly skipped.
moshloop
reviewed
Jun 24, 2026
| } | ||
| ]; | ||
|
|
||
| const healthSteps: Step[] = [ |
Member
There was a problem hiding this comment.
- Tour file per "guide"
- Use onboarding checklist (like storybook) - with floating panel (minimizable / dismissisable to localstorage) and Menu -> Getting Started link
- Split tours into Admin vs Operator and then by category
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Canary/CanaryPopup/CheckRunNow.tsx`:
- Around line 56-58: The touchpoint for health.run-now is being recorded too
early in CheckRunNow’s onClick handler, before runNow(check.id) succeeds. Move
recordTouchpoint("health.run-now") into the successful completion path of the
runNow mutation/callback for CheckRunNow so the action is only marked after a
successful run, and keep the existing onClick and runNow(check.id) flow
otherwise unchanged.
In `@src/components/GuidedTour/steps/views.ts`:
- Line 4: The “Views” opener currently falls back to the dashboard via
findViewTarget, so the tour step can highlight the wrong element instead of
skipping. Update the lookup logic in findViewTarget (and the Views step that
uses tourTarget/TourStepData) to stop returning the dashboard as a fallback for
the views opener, so onMissing: "skipSection" can trigger when no real views
entrypoint exists. Keep the target selection specific to the views entrypoint
and leave dashboard fallback only for contexts that explicitly need it.
In `@src/components/Playbooks/Runs/Submit/SubmitPlaybookRunForm.tsx`:
- Around line 113-115: Move the playbooks.run touchpoint recording in
SubmitPlaybookRunForm so it happens only after submitPlaybookRun succeeds
instead of before the async call. Update the onSubmit handler to await/handle
the result from submitPlaybookRun(values as SubmitPlaybookRunFormValues) and
call recordTouchpoint("playbooks.run") only on success, keeping the change
localized to SubmitPlaybookRunForm and its onSubmit flow.
🪄 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: 9b0e7d2a-2140-488c-ac59-fe206df064cd
📒 Files selected for processing (23)
src/App.tsxsrc/api/services/touchpoints.tssrc/components/Authentication/Kratos/KratosUserProfileDropdown.tsxsrc/components/Canary/CanaryPopup/CheckDetails.tsxsrc/components/Canary/CanaryPopup/CheckRunNow.tsxsrc/components/Configs/ConfigDetailsTabs.tsxsrc/components/GuidedTour/TouchpointObserver.tsxsrc/components/GuidedTour/TourMenu.tsxsrc/components/GuidedTour/__tests__/guidedTourSteps.unit.test.tssrc/components/GuidedTour/guidedTourState.tssrc/components/GuidedTour/guidedTourSteps.tssrc/components/GuidedTour/steps/catalog.tssrc/components/GuidedTour/steps/dashboard.tssrc/components/GuidedTour/steps/health.tssrc/components/GuidedTour/steps/playbooks.tssrc/components/GuidedTour/steps/shared.tssrc/components/GuidedTour/steps/views.tssrc/components/GuidedTour/touchpoints.tssrc/components/GuidedTour/useTouchpoints.tssrc/components/Playbooks/Runs/Submit/SubmitPlaybookRunForm.tsxsrc/components/Users/UserProfile.tsxsrc/pages/GettingStartedPage.tsxsrc/ui/Layout/SidebarLayout.tsx
✅ Files skipped from review due to trivial changes (2)
- src/components/GuidedTour/steps/playbooks.ts
- src/components/GuidedTour/touchpoints.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ui/Layout/SidebarLayout.tsx
- src/components/GuidedTour/guidedTourState.ts
- src/components/Canary/CanaryPopup/CheckDetails.tsx
- src/components/GuidedTour/TourMenu.tsx
Comment on lines
56
to
58
| onClick={() => { | ||
| recordTouchpoint("health.run-now"); | ||
| runNow(check.id); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Move touchpoint recording into the mutation success path.
Line 57 records health.run-now before runNow(check.id) completes. If the request fails, the checklist still shows the action as completed even though no successful run happened.
Proposed fix
const { isLoading, mutate: runNow } = useMutation({
mutationFn: runHealthCheckNow,
onSuccess: (data) => {
+ recordTouchpoint("health.run-now");
onSuccessfulRun(data.data);
}
});
@@
onClick={() => {
- recordTouchpoint("health.run-now");
runNow(check.id);
}}📝 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.
Suggested change
| onClick={() => { | |
| recordTouchpoint("health.run-now"); | |
| runNow(check.id); | |
| onClick={() => { | |
| runNow(check.id); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Canary/CanaryPopup/CheckRunNow.tsx` around lines 56 - 58, The
touchpoint for health.run-now is being recorded too early in CheckRunNow’s
onClick handler, before runNow(check.id) succeeds. Move
recordTouchpoint("health.run-now") into the successful completion path of the
runNow mutation/callback for CheckRunNow so the action is only marked after a
successful run, and keep the existing onClick and runNow(check.id) flow
otherwise unchanged.
| // ABOUTME: The views section of the guided tour and its checklist touchpoint. | ||
| // ABOUTME: Opens a custom view and explains that views compose graphs, tables and charts. | ||
| import { type Step } from "react-joyride"; | ||
| import { findViewTarget, tourTarget, type TourStepData } from "./shared"; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop falling back to the dashboard for the “Views” opener.
onMissing: "skipSection" cannot fire here because findViewTarget in src/components/GuidedTour/steps/shared.ts:130-138 falls back to [data-tour="dashboard"]. When no real views entrypoint is rendered, this step highlights Dashboard, asks the user to open a view, and advances on the wrong click instead of skipping the section.
Suggested fix
-import { findViewTarget, tourTarget, type TourStepData } from "./shared";
+import { tourTarget, type TourStepData } from "./shared";
@@
- target: findViewTarget,
+ target: () =>
+ document.querySelector<HTMLElement>(
+ '[data-tour="views"][data-tour-name="system"]'
+ ) ?? document.querySelector<HTMLElement>('[data-tour="views"]'),Also applies to: 8-21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/GuidedTour/steps/views.ts` at line 4, The “Views” opener
currently falls back to the dashboard via findViewTarget, so the tour step can
highlight the wrong element instead of skipping. Update the lookup logic in
findViewTarget (and the Views step that uses tourTarget/TourStepData) to stop
returning the dashboard as a fallback for the views opener, so onMissing:
"skipSection" can trigger when no real views entrypoint exists. Keep the target
selection specific to the views entrypoint and leave dashboard fallback only for
contexts that explicitly need it.
Comment on lines
113
to
115
| onSubmit={(values) => { | ||
| recordTouchpoint("playbooks.run"); | ||
| submitPlaybookRun(values as SubmitPlaybookRunFormValues); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record the touchpoint only after the run succeeds.
Line 114 marks playbooks.run complete before submitPlaybookRun resolves. Any validation, permission, or network failure will still persist the checklist item as done, so the guided-tour state can diverge from the actual outcome.
Proposed fix
const { mutate: submitPlaybookRun, isLoading } = useSubmitPlaybookRunMutation(
{
onSuccess: (run) => {
+ recordTouchpoint("playbooks.run");
navigate(`/playbooks/runs/${run.run_id}`);
},
onError: (error) => {
const message =
((error as AxiosError).response?.data as any)?.error ?? error.message;
@@
initialValues={initialValues}
validateOnMount
onSubmit={(values) => {
- recordTouchpoint("playbooks.run");
submitPlaybookRun(values as SubmitPlaybookRunFormValues);
}}
>📝 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.
Suggested change
| onSubmit={(values) => { | |
| recordTouchpoint("playbooks.run"); | |
| submitPlaybookRun(values as SubmitPlaybookRunFormValues); | |
| onSubmit={(values) => { | |
| submitPlaybookRun(values as SubmitPlaybookRunFormValues); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Playbooks/Runs/Submit/SubmitPlaybookRunForm.tsx` around lines
113 - 115, Move the playbooks.run touchpoint recording in SubmitPlaybookRunForm
so it happens only after submitPlaybookRun succeeds instead of before the async
call. Update the onSubmit handler to await/handle the result from
submitPlaybookRun(values as SubmitPlaybookRunFormValues) and call
recordTouchpoint("playbooks.run") only on success, keeping the change localized
to SubmitPlaybookRunForm and its onSubmit flow.
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
2 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.
Summary by CodeRabbit
Release Notes