Repository navigation
feat(walkthrough): multi-page guided tour across key app sections - #1218
Merged
senamakel merged 5 commits intoMay 5, 2026
Merged
Conversation
Extend the react-joyride walkthrough to navigate users through /chat, /skills, /intelligence, and /settings — not just highlight tab icons on /home. Uses v3 Step.before async hooks for cross-page navigation with a waitForTarget utility that polls until DOM targets mount after route changes. - 9-step tour visiting 5 pages with warm, conversational copy - Add data-walkthrough targets to Accounts, Skills, Intelligence, Settings - Add "Restart Tour" menu item in Settings - Preserve existing skip/dismiss, one-time trigger, localStorage flags - Full test coverage for new utilities and behavior Closes tinyhumansai#1212
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR converts the walkthrough from a static home-only tour into a multi-page, programmatic 9-step Joyride tour that navigates across routes, polls DOM targets with ChangesMulti-Page Walkthrough
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings as SettingsPage
participant App as AppWalkthrough
participant Router
participant Page as Page DOM
User->>Settings: clicks "Restart Tour"
Settings->>App: call resetWalkthrough()
App->>Window: dispatch walkthrough:restart
App->>Router: createWalkthroughSteps(navigate)
App->>App: run = true (start Joyride)
App->>Router: navigate('/chat') -- before hook
Router->>Page: render /chat
App->>Page: waitForTarget("chat-agent-panel")
Page-->>App: element present -> proceed to next step
App->>Router: navigate('/skills') -- before hook
Router->>Page: render /skills
App->>Page: waitForTarget("skills-grid")
Page-->>App: element present -> continue...
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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: 1
🧹 Nitpick comments (1)
app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx (1)
595-617: ⚡ Quick winMake
waitForTargetasync tests deterministic with fake timers.Lines 595–617 use real wall-clock timing via
Date.now()in thewaitForTargetimplementation, and the "added after delay" test (line 599–604) adds the element immediately despite the comment suggesting a delay. This makes the tests time-dependent and flaky.Per coding guidelines: "avoid real network calls and time-dependent assertions" in
app/src/**/*.test.{ts,tsx}. Consider refactoring with fake timers:Proposed deterministic test update
Note: The
waitForTargetimplementation also usesDate.now()directly, so fake timers in the test alone won't suffice. You'll need to either mockDate.now()or updatewaitForTargetto be compatible withvi.useFakeTimers().it('resolves when element is added to DOM after a delay', async () => { + vi.useFakeTimers(); const el = document.createElement('div'); el.setAttribute('data-walkthrough', 'async-target'); - // Add element after a short delay using setTimeout (resolved via fake timers). - const addEl = () => document.body.appendChild(el); - const promise = waitForTarget('async-target', 500); - // Add the element before the timeout expires. - addEl(); + setTimeout(() => document.body.appendChild(el), 100); + await vi.advanceTimersByTimeAsync(120); try { await expect(promise).resolves.toBeUndefined(); } finally { + vi.useRealTimers(); if (el.parentNode) document.body.removeChild(el); } }); it('rejects when element is not found before timeout', async () => { - // Use a very short timeout so the test completes quickly. - await expect(waitForTarget('nonexistent-target', 50)).rejects.toThrow( - '[walkthrough] waitForTarget timed out' - ); + vi.useFakeTimers(); + try { + const promise = waitForTarget('nonexistent-target', 50); + await vi.advanceTimersByTimeAsync(60); + await expect(promise).rejects.toThrow('[walkthrough] waitForTarget timed out'); + } finally { + vi.useRealTimers(); + } });🤖 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 `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx` around lines 595 - 617, The tests are flaky because they rely on real time; update the tests to use fake timers and either mock Date.now or make waitForTarget compatible with fake timers: in the test for resolving, call vi.useFakeTimers(), schedule adding the element with setTimeout(() => document.body.appendChild(el), 100), call const promise = waitForTarget('async-target', 500), then call vi.advanceTimersByTime(100) and await expect(promise).resolves.toBeUndefined(); in the timeout/reject test use vi.useFakeTimers(), call const p = waitForTarget('nonexistent-target', 50), vi.advanceTimersByTime(50) and await expect(p).rejects.toThrow('[walkthrough] waitForTarget timed out'); if waitForTarget relies on Date.now(), either mock Date.now via vi.spyOn(Date, 'now').mockImplementation(() => mockedNow) and advance mockedNow appropriately or refactor waitForTarget to use setTimeout loops so vi.useFakeTimers() controls time; restore timers/mocks after each test.
🤖 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 `@app/src/pages/Accounts.tsx`:
- Line 240: The walkthrough target "chat-agent-panel" is mounted conditionally
under isAgentSelected which can cause the tour to dead-end on /chat; always
render the element with data-walkthrough="chat-agent-panel" whenever the current
route is /chat (but keep it visually hidden or inert when no agent is selected)
so the step can find the target; update the Accounts.tsx JSX around the div that
currently uses isAgentSelected to instead check the route (or add a persistent
placeholder div with data-walkthrough="chat-agent-panel" near the existing agent
panel) while preserving existing behavior when isAgentSelected is false.
---
Nitpick comments:
In `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx`:
- Around line 595-617: The tests are flaky because they rely on real time;
update the tests to use fake timers and either mock Date.now or make
waitForTarget compatible with fake timers: in the test for resolving, call
vi.useFakeTimers(), schedule adding the element with setTimeout(() =>
document.body.appendChild(el), 100), call const promise =
waitForTarget('async-target', 500), then call vi.advanceTimersByTime(100) and
await expect(promise).resolves.toBeUndefined(); in the timeout/reject test use
vi.useFakeTimers(), call const p = waitForTarget('nonexistent-target', 50),
vi.advanceTimersByTime(50) and await expect(p).rejects.toThrow('[walkthrough]
waitForTarget timed out'); if waitForTarget relies on Date.now(), either mock
Date.now via vi.spyOn(Date, 'now').mockImplementation(() => mockedNow) and
advance mockedNow appropriately or refactor waitForTarget to use setTimeout
loops so vi.useFakeTimers() controls time; restore timers/mocks after each test.
🪄 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: e9448486-d255-47ff-9567-33aefa194f3a
📒 Files selected for processing (10)
.claude/memory.mdapp/src/components/BottomTabBar.tsxapp/src/components/settings/SettingsHome.tsxapp/src/components/walkthrough/AppWalkthrough.tsxapp/src/components/walkthrough/WalkthroughTooltip.tsxapp/src/components/walkthrough/__tests__/AppWalkthrough.test.tsxapp/src/components/walkthrough/walkthroughSteps.tsapp/src/pages/Accounts.tsxapp/src/pages/Intelligence.tsxapp/src/pages/Skills.tsx
…ment, use fake timers in tests - Move data-walkthrough="chat-agent-panel" from conditional div to the always-present <main> wrapper so the tour doesn't dead-end when no agent is selected - Refactor waitForTarget to use setTimeout (not rAF) for fake-timer compatibility in tests - Update tests to use vi.useFakeTimers() for deterministic assertions
Contributor
Author
|
Also addressed the nitpick in 2377dd1: refactored |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx (1)
560-577: 💤 Low valueConsider providing a minimal
TourDatastub instead of double-casting.The
beforehook signature in react-joyride v3 is(data: TourData) => Promise<void> | void. While the current implementation doesn't use the argument, the double-cast throughunknownis brittle.♻️ Suggested improvement
- // Cast to unknown→Function to avoid needing a full TourData stub — we only care navigate was called. - await (steps[2].before as unknown as (() => Promise<void>) | undefined)?.(); + // Provide minimal stub matching TourData shape; we only care navigate was called. + const stubTourData = { index: 2, step: steps[2], size: 9 } as Parameters<NonNullable<typeof steps[2]['before']>>[0]; + await steps[2].before?.(stubTourData);🤖 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 `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx` around lines 560 - 577, The test is brittlely double-casting the before hook to ignore its required TourData parameter; instead provide a minimal TourData stub when invoking the hook so the signature matches react-joyride v3. Update the test to call (steps[2].before)(tourDataStub) where tourDataStub is a minimal object satisfying the TourData shape (e.g., { action: 'start', index: 2, lifecycle: 'init' } or similar) so you avoid the unknown→Function cast and keep the call to createWalkthroughSteps(navigate) and the assertion that navigate was called with '/chat'.
🤖 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.
Nitpick comments:
In `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx`:
- Around line 560-577: The test is brittlely double-casting the before hook to
ignore its required TourData parameter; instead provide a minimal TourData stub
when invoking the hook so the signature matches react-joyride v3. Update the
test to call (steps[2].before)(tourDataStub) where tourDataStub is a minimal
object satisfying the TourData shape (e.g., { action: 'start', index: 2,
lifecycle: 'init' } or similar) so you avoid the unknown→Function cast and keep
the call to createWalkthroughSteps(navigate) and the assertion that navigate was
called with '/chat'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8d2af8a-df88-4703-836a-59831c68d627
📒 Files selected for processing (3)
app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsxapp/src/components/walkthrough/walkthroughSteps.tsapp/src/pages/Accounts.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/pages/Accounts.tsx
Exercise the before hooks for steps 3-6 (skills, intelligence, settings, home-return) so their navigate + waitForTarget calls are hit by coverage.
The spotlight was covering entire pages, pushing tooltips off-screen. Now targets small, specific elements: - Chat: thread header bar (Conversations.tsx) instead of <main> - Skills: "Integrations" h2 heading instead of full content div - Intelligence: h1 title instead of header flex container - Settings: SettingsHeader wrapper instead of full menu list Spotlights are now tight and tooltips stay visible without scrolling.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/components/walkthrough/walkthroughSteps.ts (2)
11-11: ⚡ Quick winUse camelCase for the polling interval constant.
Please rename
POLL_INTERVALto a camelCase identifier (e.g.,pollIntervalMs) to match repository naming rules.As per coding guidelines,
Use camelCase for variable and function names.🤖 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 `@app/src/components/walkthrough/walkthroughSteps.ts` at line 11, Rename the constant POLL_INTERVAL in walkthroughSteps.ts to a camelCase name like pollIntervalMs and update every reference to that symbol (e.g., any usages inside functions that read the interval) to the new identifier; ensure the new name conveys units (Ms) and preserve the same value (50) and semantics wherever POLL_INTERVAL was used (including exports or imports if applicable).
79-82: ⚡ Quick winExtract repeated
beforenavigation/wait logic into one helper.The same
navigate(path)+waitForTarget(target)sequence is repeated across multiple steps; centralizing it will reduce drift when you tune timeout/error behavior later.♻️ Proposed refactor
export function createWalkthroughSteps(navigate: NavigateFunction): Step[] { + const navigateAndWait = async (path: string, target: string) => { + navigate(path); + await waitForTarget(target); + }; + return [ @@ skipBeacon: true, - before: async () => { - navigate('/chat'); - await waitForTarget('chat-agent-panel'); - }, + before: async () => navigateAndWait('/chat', 'chat-agent-panel'), @@ skipBeacon: true, - before: async () => { - navigate('/skills'); - await waitForTarget('skills-grid'); - }, + before: async () => navigateAndWait('/skills', 'skills-grid'), @@ skipBeacon: true, - before: async () => { - navigate('/intelligence'); - await waitForTarget('intelligence-header'); - }, + before: async () => navigateAndWait('/intelligence', 'intelligence-header'), @@ skipBeacon: true, - before: async () => { - navigate('/settings'); - await waitForTarget('settings-menu'); - }, + before: async () => navigateAndWait('/settings', 'settings-menu'), @@ skipBeacon: true, - before: async () => { - navigate('/home'); - await waitForTarget('tab-chat'); - }, + before: async () => navigateAndWait('/home', 'tab-chat'),Also applies to: 93-96, 107-110, 121-124, 134-137
🤖 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 `@app/src/components/walkthrough/walkthroughSteps.ts` around lines 79 - 82, Multiple step objects in walkthroughSteps.ts repeat the same before logic (navigate(path); await waitForTarget(target)); extract this into a single async helper (e.g., navigateAndWait) that accepts the route and target name, calls navigate(route) then await waitForTarget(target), and replace each step's before function (the anonymous async before blocks that call navigate and waitForTarget) with a call to that helper (for example before: () => navigateAndWait('/chat','chat-agent-panel')). Update all repeated occurrences (the before functions around the navigate/wait calls) to use this helper so timeout/error handling is centralized.
🤖 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.
Nitpick comments:
In `@app/src/components/walkthrough/walkthroughSteps.ts`:
- Line 11: Rename the constant POLL_INTERVAL in walkthroughSteps.ts to a
camelCase name like pollIntervalMs and update every reference to that symbol
(e.g., any usages inside functions that read the interval) to the new
identifier; ensure the new name conveys units (Ms) and preserve the same value
(50) and semantics wherever POLL_INTERVAL was used (including exports or imports
if applicable).
- Around line 79-82: Multiple step objects in walkthroughSteps.ts repeat the
same before logic (navigate(path); await waitForTarget(target)); extract this
into a single async helper (e.g., navigateAndWait) that accepts the route and
target name, calls navigate(route) then await waitForTarget(target), and replace
each step's before function (the anonymous async before blocks that call
navigate and waitForTarget) with a call to that helper (for example before: ()
=> navigateAndWait('/chat','chat-agent-panel')). Update all repeated occurrences
(the before functions around the navigate/wait calls) to use this helper so
timeout/error handling is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41e861d6-0308-48bb-a806-afad3b2e1a34
📒 Files selected for processing (5)
app/src/components/settings/SettingsHome.tsxapp/src/components/walkthrough/walkthroughSteps.tsapp/src/pages/Conversations.tsxapp/src/pages/Intelligence.tsxapp/src/pages/Skills.tsx
✅ Files skipped from review due to trivial changes (2)
- app/src/pages/Conversations.tsx
- app/src/pages/Intelligence.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/pages/Skills.tsx
- app/src/components/settings/SettingsHome.tsx
- Target small headers/titles instead of full page containers so spotlights are tight and tooltips stay visible without scrolling - Add step 5 "Chat where you already are" highlighting the Channels section heading on the Skills page (10 steps total now) - Update step icons and test assertions for the new step count
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx (1)
494-504: ⚡ Quick winProgress-bar assertion is too implementation-coupled.
Checking an exact width fragment (
33.3) is brittle to harmless formatting/rounding changes. Prefer a behavior-level assertion (valid percentage range / non-zero progress for index > 0).As per coding guidelines "Prefer testing behavior over implementation details; use existing helpers from
app/src/test/before adding new harness code".🤖 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 `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx` around lines 494 - 504, The test in AppWalkthrough.test.tsx tightly couples to the CSS formatting by asserting an exact "33.3" in WalkthroughTooltip's progress bar style; instead, parse the inline width from the element returned by container.querySelector('div.bg-gradient-to-r') (or use a helper from the test utilities) and assert a behavior-level expectation such as the percent is >0 for index>0 and within an expected range (e.g., 30–36%) for index 2 of 9, or simply assert the computed width string contains a '%' and is non-zero; update the assertions to use makeTooltipProps({ index: 2, size: 9 }), read bar.getAttribute('style') (or computed style) and validate the numeric percentage range rather than matching "33.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 `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx`:
- Around line 12-13: The header comment above the tests is stale (it says "9
steps") and must match the actual test expectation of 10 steps; update the
comment to indicate "10 steps" so it aligns with the assertion in the test that
references createWalkthroughSteps (and leave the waitForTarget description
intact). Locate the comment near the test file's top that mentions
createWalkthroughSteps/waitForTarget and change the step count to 10.
- Around line 560-579: The test for createWalkthroughSteps is missing a negative
assertion for the case where route is null (idx 4 / target 'skills-channels');
after invoking (steps[idx].before) ensure you assert that the navigate mock was
NOT called (e.g., expect(navigate).not.toHaveBeenCalled() or
toHaveBeenCalledTimes(0)) when route === null, so accidental navigation in
steps[idx].before will fail the test.
- Around line 620-628: The test using vi.useFakeTimers() for waitForTarget
should always restore timers in a finally block to avoid leaking fake timers;
update the test case that calls waitForTarget('nonexistent-target', 100) so that
vi.useRealTimers() is invoked inside a finally after awaiting the promise (or
after advancing timers) instead of at the end of the try path—wrap the async
assertions in try/catch/finally (or use try/finally) around
vi.useFakeTimers()/vi.advanceTimersByTimeAsync(...) and ensure
vi.useRealTimers() runs in finally; reference the test body that calls
vi.useFakeTimers(), vi.advanceTimersByTimeAsync, waitForTarget and
vi.useRealTimers to locate where to change.
---
Nitpick comments:
In `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx`:
- Around line 494-504: The test in AppWalkthrough.test.tsx tightly couples to
the CSS formatting by asserting an exact "33.3" in WalkthroughTooltip's progress
bar style; instead, parse the inline width from the element returned by
container.querySelector('div.bg-gradient-to-r') (or use a helper from the test
utilities) and assert a behavior-level expectation such as the percent is >0 for
index>0 and within an expected range (e.g., 30–36%) for index 2 of 9, or simply
assert the computed width string contains a '%' and is non-zero; update the
assertions to use makeTooltipProps({ index: 2, size: 9 }), read
bar.getAttribute('style') (or computed style) and validate the numeric
percentage range rather than matching "33.3".
🪄 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: 2b659346-26f1-4c3d-b9ce-b8b2298eb412
📒 Files selected for processing (4)
app/src/components/walkthrough/WalkthroughTooltip.tsxapp/src/components/walkthrough/__tests__/AppWalkthrough.test.tsxapp/src/components/walkthrough/walkthroughSteps.tsapp/src/pages/Skills.tsx
✅ Files skipped from review due to trivial changes (1)
- app/src/components/walkthrough/WalkthroughTooltip.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/pages/Skills.tsx
- app/src/components/walkthrough/walkthroughSteps.ts
Comment on lines
+12
to
+13
| * - createWalkthroughSteps: 9 steps, cross-page steps have before functions | ||
| * - waitForTarget: resolves when element added, rejects on timeout |
Contributor
There was a problem hiding this comment.
The test header comment has a stale step count.
Line 12 says “9 steps” but Line 513 asserts 10 steps. Please align the header comment to avoid confusion.
🤖 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 `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx` around
lines 12 - 13, The header comment above the tests is stale (it says "9 steps")
and must match the actual test expectation of 10 steps; update the comment to
indicate "10 steps" so it aligns with the assertion in the test that references
createWalkthroughSteps (and leave the waitForTarget description intact). Locate
the comment near the test file's top that mentions
createWalkthroughSteps/waitForTarget and change the step count to 10.
Comment on lines
+560
to
+579
| it.each([ | ||
| { idx: 2, route: '/chat', target: 'chat-agent-panel' }, | ||
| { idx: 3, route: '/skills', target: 'skills-grid' }, | ||
| { idx: 4, route: null, target: 'skills-channels' }, | ||
| { idx: 5, route: '/intelligence', target: 'intelligence-header' }, | ||
| { idx: 6, route: '/settings', target: 'settings-menu' }, | ||
| { idx: 7, route: '/home', target: 'tab-chat' }, | ||
| ])('before hook for step $idx calls navigate("$route")', async ({ idx, route, target }) => { | ||
| const navigate = vi.fn(); | ||
|
|
||
| const el = document.createElement('div'); | ||
| el.setAttribute('data-walkthrough', target); | ||
| document.body.appendChild(el); | ||
|
|
||
| try { | ||
| const steps = createWalkthroughSteps(navigate); | ||
| await (steps[idx].before as unknown as (() => Promise<void>) | undefined)?.(); | ||
| if (route) { | ||
| expect(navigate).toHaveBeenCalledWith(route); | ||
| } |
Contributor
There was a problem hiding this comment.
Add a negative assertion for the route: null case.
For Line 563 (route: null), the test currently makes no navigation assertion, so accidental navigation regressions can pass unnoticed.
✅ Suggested fix
await (steps[idx].before as unknown as (() => Promise<void>) | undefined)?.();
if (route) {
expect(navigate).toHaveBeenCalledWith(route);
+ } else {
+ expect(navigate).not.toHaveBeenCalled();
}🤖 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 `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx` around
lines 560 - 579, The test for createWalkthroughSteps is missing a negative
assertion for the case where route is null (idx 4 / target 'skills-channels');
after invoking (steps[idx].before) ensure you assert that the navigate mock was
NOT called (e.g., expect(navigate).not.toHaveBeenCalled() or
toHaveBeenCalledTimes(0)) when route === null, so accidental navigation in
steps[idx].before will fail the test.
Comment on lines
+620
to
+628
| it('rejects when element is not found before timeout', async () => { | ||
| vi.useFakeTimers(); | ||
| const promise = waitForTarget('nonexistent-target', 100).catch((e: Error) => e); | ||
| await vi.advanceTimersByTimeAsync(150); | ||
| const result = await promise; | ||
| expect(result).toBeInstanceOf(Error); | ||
| expect((result as Error).message).toContain('[walkthrough] waitForTarget timed out'); | ||
| vi.useRealTimers(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx | sed -n '615,635p'Repository: tinyhumansai/openhuman
Length of output: 690
🏁 Script executed:
grep -n "vi\.useFakeTimers()" app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsxRepository: tinyhumansai/openhuman
Length of output: 122
🏁 Script executed:
cat -n app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx | sed -n '600,618p'Repository: tinyhumansai/openhuman
Length of output: 789
Always restore fake timers in a finally block.
If an assertion fails on line 625 or 626, vi.useRealTimers() is skipped and fake timers can leak into later tests. The preceding test (lines 600–618) demonstrates the correct pattern.
✅ Suggested fix
it('rejects when element is not found before timeout', async () => {
vi.useFakeTimers();
- const promise = waitForTarget('nonexistent-target', 100).catch((e: Error) => e);
- await vi.advanceTimersByTimeAsync(150);
- const result = await promise;
- expect(result).toBeInstanceOf(Error);
- expect((result as Error).message).toContain('[walkthrough] waitForTarget timed out');
- vi.useRealTimers();
+ try {
+ const promise = waitForTarget('nonexistent-target', 100).catch((e: Error) => e);
+ await vi.advanceTimersByTimeAsync(150);
+ const result = await promise;
+ expect(result).toBeInstanceOf(Error);
+ expect((result as Error).message).toContain('[walkthrough] waitForTarget timed out');
+ } finally {
+ vi.useRealTimers();
+ }
});📝 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
| it('rejects when element is not found before timeout', async () => { | |
| vi.useFakeTimers(); | |
| const promise = waitForTarget('nonexistent-target', 100).catch((e: Error) => e); | |
| await vi.advanceTimersByTimeAsync(150); | |
| const result = await promise; | |
| expect(result).toBeInstanceOf(Error); | |
| expect((result as Error).message).toContain('[walkthrough] waitForTarget timed out'); | |
| vi.useRealTimers(); | |
| }); | |
| it('rejects when element is not found before timeout', async () => { | |
| vi.useFakeTimers(); | |
| try { | |
| const promise = waitForTarget('nonexistent-target', 100).catch((e: Error) => e); | |
| await vi.advanceTimersByTimeAsync(150); | |
| const result = await promise; | |
| expect(result).toBeInstanceOf(Error); | |
| expect((result as Error).message).toContain('[walkthrough] waitForTarget timed out'); | |
| } finally { | |
| vi.useRealTimers(); | |
| } | |
| }); |
🤖 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 `@app/src/components/walkthrough/__tests__/AppWalkthrough.test.tsx` around
lines 620 - 628, The test using vi.useFakeTimers() for waitForTarget should
always restore timers in a finally block to avoid leaking fake timers; update
the test case that calls waitForTarget('nonexistent-target', 100) so that
vi.useRealTimers() is invoked inside a finally after awaiting the promise (or
after advancing timers) instead of at the end of the try path—wrap the async
assertions in try/catch/finally (or use try/finally) around
vi.useFakeTimers()/vi.advanceTimersByTimeAsync(...) and ensure
vi.useRealTimers() runs in finally; reference the test body that calls
vi.useFakeTimers(), vi.advanceTimersByTimeAsync, waitForTarget and
vi.useRealTimers to locate where to change.
6 tasks done
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
senamakel
pushed a commit
to nocstah/openhuman
that referenced
this pull request
Sep 11, 2026
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
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
/home → /chat → /skills → /intelligence → /settings → /home) instead of only highlighting tab icons on the home screenStep.beforeasync hooks for cross-page navigation with awaitForTargetutility that polls until DOM targets mount after route changesTour steps
Changes
walkthroughSteps.tswaitForTargetutilityAppWalkthrough.tsxuseNavigate,resetWalkthrough()export, restart event listenerWalkthroughTooltip.tsxSTEP_ICONSto 10 entriesBottomTabBar.tsxtab-automation→tab-notificationsattrConversations.tsxdata-walkthrough="chat-agent-panel"on chat header barSkills.tsxdata-walkthroughon Integrations + Channels headingsIntelligence.tsxdata-walkthrough="intelligence-header"on h1 titleSettingsHome.tsxdata-walkthroughon Settings headerAppWalkthrough.test.tsxTest plan
pnpm typecheck— cleanpnpm lint— 0 errors (31 pre-existing warnings)pnpm format:check— cleanpnpm build— successCloses #1212