feat: add context-aware getting-started onboarding checklist - #3084
Conversation
Add a second onboarding phase to the sidebar checklist that tracks product-usage milestones per user (explore data, build a dashboard, set up an alert, use the MCP server), persisted on user.onboardingData. Completion is recorded server-side so it is surface-agnostic: alert and dashboard tasks complete from the UI, external REST API v2, or MCP tools. The dashboard task requires at least one tile (a chart), not an empty shell, including tiles added to temporary URL-state dashboards. Exploring data completes on any non-trivial search; MCP usage is recorded in the tool-tracing chokepoint. All recording is fire-and-forget. The task registry (ONBOARDING_TASK_IDS) is a single source of truth in common-utils, typed so adding a task is a compile error until both the API validation enum and the frontend UI are updated. The checklist's done state is derived from whether every current task is complete, so adding a task later reopens the checklist for previously-finished users.
🦋 Changeset detectedLatest commit: d9dabd7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryAdds a context-aware second onboarding phase that records per-user product milestones across frontend, API, external API, and MCP workflows.
Confidence Score: 5/5The onboarding changes appear safe to merge; no new actionable failures remain after the latest updates. No accepted new findings remain. Every previous Greptile thread was manually resolved, including the checklist eligibility, query gating, cache-concurrency, search-submission, semantic-color, and file-size findings. The final semantic-color thread was also correctly withdrawn after brandon-pereira explained that Mantine’s
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/types.ts | Defines the canonical onboarding task registry, schemas, API contracts, and persistable-user guard. |
| packages/api/src/controllers/user.ts | Adds idempotent persistence helpers and non-blocking product-action recording. |
| packages/api/src/routers/api/me.ts | Exposes authenticated task-completion and dismissal routes and defaults legacy user state. |
| packages/api/src/controllers/dashboard.ts | Records dashboard completion only after a persisted dashboard contains at least one tile. |
| packages/api/src/controllers/alerts.ts | Records alert completion across create, edit, and dashboard-tile alert paths. |
| packages/app/src/OnboardingChecklist/useOnboardingCompletion.ts | Derives onboarding eligibility, phase progression, visibility, and celebration state. |
| packages/app/src/api.ts | Adds onboarding mutations with concurrency-safe cached /me state updates. |
| packages/app/src/DBSearchPage.tsx | Records qualifying interactive searches while excluding programmatic catch-up submissions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[User action] --> B{Action surface}
B -->|Search UI| C[POST onboarding task]
B -->|Dashboard or alert API| D[Server-side recorder]
B -->|MCP tool| D
C --> E[(User onboardingData)]
D --> E
E --> F[GET /me]
F --> G[Onboarding checklist]
G --> H{All current tasks complete?}
H -->|No| I[Show next task]
H -->|Yes| J[Show completion state]
Reviews (12): Last reviewed commit: "Merge branch 'main' into brandon/brandon..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 3 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
Deep Review✅ No critical issues found. No P0/P1 defects surfaced across the reviewers that returned or in a manual read of every changed hot path. The largest prior concerns (team-age gate removal, unbounded 🟡 P2 — recommended
🔵 P3 nitpicks (6)
Reviewers (7): security, testing, maintainability, performance, reliability, api-contract, previous-comments. Coverage caveat: correctness, adversarial, frontend-races, kieran-typescript, and project-standards were dispatched but did not return before synthesis. Their focus areas (celebration-latch effect ordering, cache-race safety, and semantic color tokens) were spot-checked manually and appear sound — the latch is gated on Testing gaps:
|
- Guard onboardingData deref in useMarkOnboardingTaskComplete so a stale me cache can't throw and fail a dashboard save - Skip the system.tables row-count query once the checklist is dismissed - Union completedTasks into the me cache instead of replacing, so an out-of-order completion response can't drop a just-completed task - Export recordDashboardOnboardingIfHasTiles and reuse it at all five dashboard write sites (DRY) - Drop PRODUCT_TASK_ORDER_WEIGHT by ordering ONBOARDING_TASK_IDS as the display order; remove unused isPhaseComplete from the hook return - Use Text variant=success for the celebration per semantic-token rule - Extract isNonTrivialSearch and add a unit test for the advancedQuery trigger; poll (waitForTask) in me.int recording tests to de-flake the fire-and-forget assertions
|
|
||
| // A tile alert never goes through the /alerts router, so this is the only | ||
| // place a dashboard-tile alert can complete the onboarding task. | ||
| if (result.length > 0) { |
There was a problem hiding this comment.
🔵 minor — Dashboard saves credit the alert and dashboard tasks to whoever saved last, not to whoever did the action
createOrUpdateDashboardAlerts records alert whenever the tile map is non-empty, and it is reached from updateDashboard → syncDashboardAlerts on every dashboard save that includes tiles (packages/api/src/controllers/dashboard.ts:104). So user B, who has never configured an alert, drags a tile on a dashboard where user A set up a tile alert, and B's alert task ticks off. The same shape applies at dashboard.ts:252: recordDashboardOnboardingIfHasTiles(userId, updatedDashboard.tiles) runs on any update of a tiled dashboard, so renaming or re-tagging someone else's dashboard completes "Build a dashboard". The frontend mirror markDashboardOnboarding in packages/app/src/dashboard.ts:139 has the same behaviour. Record only when the write actually changed the thing: diff alertsByTile against the existing alerts (the oldAlert lookup already inside the map gives you this) and diff old vs. new tiles in updateDashboard.
…rding-steps # Conflicts: # packages/api/src/controllers/alerts.ts # packages/api/src/mcp/tools/alerts/saveAlert.ts # packages/api/src/routers/api/alerts.ts # packages/api/src/routers/external-api/v2/alerts.ts # packages/app/src/dashboard.ts
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
| // isNonTrivialSearch). This runs on every search but the task is a | ||
| // one-time milestone, so skip once it's already recorded — otherwise | ||
| // every subsequent qualifying search fires a redundant (idempotent) POST. | ||
| if ( |
There was a problem hiding this comment.
🔵 minor — No test covers the search page actually recording the advancedQuery task
isNonTrivialSearch has a unit test and the local-dashboard path has useDashboardOnboarding.test.tsx, but the headline "explore your data" behaviour — the three-way guard !IS_LOCAL_MODE && !hasExploredData && isNonTrivialSearch(where, filters) inside onSubmit — is only stubbed out in DBSearchPage.directTrace.test.tsx. Add a test that submits a search with a where clause and asserts mutate('advancedQuery') fired, and that it does not fire a second time once me.onboardingData.completedTasks contains it.
E2E Test Results✅ All tests passed • 358 passed • 1 skipped • 1485s
Tests ran across 4 shards in parallel. |
- Keep isDismissed from the me cache when unioning completedTasks so a concurrent dismiss can't be overwritten by a completion response - Use semantic tokens (--color-*-success / --color-border / --color-text-muted) for the checklist status icons; drop hardcoded #fff - Remove redundant aria-label on the dismiss button (WCAG 2.5.3); select it by visible text in the test - Make the exhaustiveness test assert every PRODUCT_TASKS title renders instead of the tautological n/N badge; fix the stale sort comment - Record advancedQuery only on a genuine user form submit, not the shared onSubmit that also runs on saved-search catch-up submits
- Record advancedQuery from the shared onSubmit (covers Enter-to-search and filter-apply, not just the Run button) while a ref suppresses the programmatic catch-up submit on source/saved-search load. Fixes a regression from moving recording into onFormSubmit. - Skip recordOnboardingTaskCompletion when the id isn't a valid ObjectId (local app mode injects a synthetic _local_user_), avoiding a CastError warning on every save/tool call - Reuse isNonEmptyWhereExpr in isNonTrivialSearch (DRY) - Point the MCP task at /team?tab=api-agents#team-api-agents-mcp-server so it lands on the MCP panel, not the default Data tab - Drop the unused api.useCompleteOnboardingTask object entry; keep the named export both consumers use - Settle before the tileless-dashboard negative assertion so it can't pass vacuously
…ed tiles for cache - completeOnboardingTask/setOnboardingDismissed now no-op for a non-persistable user id (the synthetic _local_user_ in IS_LOCAL_APP_MODE), via a shared isPersistableUserId type guard reused by recordOnboardingTaskCompletion. Previously findByIdAndUpdate matched nothing and returned default state. - Client skips onboarding recording when me.id isn't a real ObjectId (isRecordableUserId), so the all-in-one-noauth image (server local mode, client non-local) no longer re-POSTs /me/onboarding/task on every search and every temp-dashboard edit forever. - Dashboard save hooks now key onboarding off the PATCH/POST response's persisted tiles instead of the request payload, so a name/tag-only save no longer leaves the me cache disagreeing with the server rule.
…rding-steps # Conflicts: # packages/app/src/OnboardingChecklist.tsx
…gate comment The enabled gate scopes system.tables to the 7-day product window but not to the card actually being visible (a 3-7 day team still in setup, or a fully completed team, is in-window but shows no card). Tightening is circular (isSetupComplete needs this query's hasData), so add a 5-minute staleTime to stop react-query refetching the probe on every mount/window-focus for those hidden-card cases, and fix the overclaiming comment.
Comments now flag only non-obvious decisions/edge cases rather than restating what the code does; changeset reduced to one paragraph.
| ); | ||
|
|
||
| // Tile alerts never hit the /alerts router, so record here. | ||
| if (result.length > 0) { |
There was a problem hiding this comment.
🔵 minor — Saving a dashboard credits "Set up an alert" to the saver for pre-existing, unchanged tile alerts
createOrUpdateDashboardAlerts records the task whenever alertsByTile is non-empty, and syncDashboardAlerts (packages/api/src/controllers/dashboard.ts:103) calls it on every save whose payload carries tiles. Concretely: user A configures a tile alert; user B later drags that tile (a PATCH with tiles) — B's onboardingData.completedTasks gains 'alert' even though B never set one up. The client twin at packages/app/src/dashboard.ts:147 does the same from the response tiles, and additionally fires for a name/tag-only PATCH where the server does not, so that credit appears and then vanishes on the next me refetch. Record only when the upsert actually created an alert — the map body already reads oldAlert, so collect oldAlert == null per tile and call recordOnboardingTaskCompletion only if any were new; mirror that on the client by keying off the tiles the caller changed rather than the whole saved set.
| ) | ||
| .expect(200); | ||
|
|
||
| // Clear the task recorded by create so the update is the only thing that |
There was a problem hiding this comment.
🔵 minor — The PUT /alerts/:id onboarding test can pass without updateAlert recording anything
createAlert fires recordOnboardingTaskCompletion fire-and-forget just before the POST responds, so that write is still in flight when the test clears completedTasks at line 215. If it lands after the clear, waitForTask(user._id, 'alert') succeeds on the create's write and the assertion holds even if the updateAlert recording were deleted. Await the create's write first — expect(await waitForTask(user._id, 'alert')).toBe(true) before the $set: { 'onboardingData.completedTasks': [] } — so the clear is ordered after it and the PUT is genuinely the only remaining source.
| > | ||
| <IconCheck size={11} stroke={3} /> | ||
| </ThemeIcon> | ||
| ) : step.isLoading ? ( |
There was a problem hiding this comment.
🔵 minor — The per-step loading spinner can never render
shouldShow requires inputsReady (packages/app/src/OnboardingChecklist/useOnboardingCompletion.ts:184), which already requires !isConnectionsLoading && !isSourcesLoading && sourceRowsSettled, so no step is ever rendered while its query is loading and the step.isLoading branch here is unreachable. The !isSourceRowsQueryEnabled || guard on line 183 rests on a premise that no longer holds either — on TanStack Query v5 isLoading is isPending && isFetching, so a disabled query reports isLoading: false, not "forever true". Drop the isLoading field from setupSteps and the Loader branch here (or, if an early skeleton is wanted, gate only the celebration latch on inputsReady rather than shouldShow).
Address jordan's review: gate the checklist on a real user id so the all-in-one-noauth image (synthetic _local_user_) never renders a card whose dismiss no-ops and whose tasks can never persist. Dedupe the 24-hex ObjectId regex into common-utils as isPersistableUserId and call it directly from the api controller and all app consumers.
| `MCP tool completed: ${toolName}`, | ||
| ); | ||
| // Only reliable signal the user exercised the MCP server. | ||
| recordOnboardingTaskCompletion(context.userId, 'mcp'); |
There was a problem hiding this comment.
🔵 minor — The mcp milestone and the search-page suppression logic have no test coverage
recordOnboardingTaskCompletion(context.userId, 'mcp') sits on the !result.isError branch of the tool-tracing chokepoint and is the only way the "Connect the MCP server" task is ever completed, yet no test asserts it — neither that a successful call records it nor that an error result does not. The existing MCP suites (packages/api/src/mcp/__tests__/alerts.int.test.ts, queryTool.int.test.ts) already build an McpContext from a real user._id and would need only an assertion on User.findById(...).onboardingData.completedTasks. Likewise, the trickiest new frontend logic — the recordExploration split between onSubmit and debouncedCatchUpSubmit in DBSearchPage.tsx:1282-1327 — is untested; onboardingTasks.test.ts only covers the pure isNonTrivialSearch predicate, and DBSearchPage.directTrace.test.tsx stubs useCompleteOnboardingTask with an unasserted jest.fn().
| method: 'POST', | ||
| json: { taskId }, | ||
| }).json<OnboardingDataApiResponse>(), | ||
| onSuccess: data => { |
There was a problem hiding this comment.
🔵 minor — The me-cache completedTasks append is implemented twice in the same file
useCompleteOnboardingTask's onSuccess (lines 102-124) and useMarkOnboardingTaskComplete (lines 132-156) both implement "append task ids to me.onboardingData.completedTasks via setQueryData, without duplicating and while preserving sibling object identity", 8 lines apart. Have the mutation's onSuccess delegate — data.onboardingData.completedTasks.forEach(markOnboardingTaskComplete) — so the union/identity rules that useMarkOnboardingTaskComplete.test.tsx pins live in one place. (useDismissOnboarding.onSuccess at line 371 is a third, narrower copy of the same setQueryData shape.)
| // in-session completion (wrongly showing + celebrating on load). A disabled | ||
| // query reports isLoading forever, so only wait on the probe when enabled. | ||
| const sourceRowsSettled = !isSourceRowsQueryEnabled || !isSourceRowsLoading; | ||
| const inputsReady = |
There was a problem hiding this comment.
🔵 minor — Per-step isLoading is now unreachable — the card can never render while a setup query is in flight
inputsReady requires !isConnectionsLoading && !isSourcesLoading && sourceRowsSettled, and shouldShow requires inputsReady, so by the time StepRow renders, every isLoading on setupSteps (lines 116/124/132) is false and the Loader branch in StepRow.tsx:1595 is dead code (productSteps never set it at all). The !isSourceRowsQueryEnabled || half of sourceRowsSettled is also dead: the comment says "a disabled query reports isLoading forever", but this is @tanstack/react-query v5 (packages/app/package.json:53), where isLoading = isPending && isFetching is false for a disabled query — useQueriedChartConfig returns query.isLoading || isLoadingMVOptimization (packages/app/src/hooks/useChartConfig.tsx:491) and both are false when disabled. The user-visible effect is a regression from the old component, which rendered immediately and showed a spinner per step: the card now pops in only after the system.tables probe settles. Either gate only the celebration latch on inputsReady (letting the card render with spinners as before) or delete isLoading from OnboardingStep, setupSteps, and the Loader branch in StepRow.
| const { data: me } = api.useMe(); | ||
| // A non-persistable user counts as "already built" so the temp-dashboard POST | ||
| // never fires (see isPersistableUserId). | ||
| const hasBuiltDashboard = |
There was a problem hiding this comment.
🔵 minor — The "already done for a persistable user" guard is duplicated verbatim in two modules
hasBuiltDashboard here and hasExploredData in DBSearchPage.tsx:1260 are the same expression (!isPersistableUserId(me?.id) || (me?.onboardingData?.completedTasks.includes(<id>) ?? false)) with only the task id differing, and useOnboardingCompletion.ts:35 builds a third reader of the same field. Extract one useIsOnboardingTaskComplete(taskId: OnboardingTaskId) hook next to the task registry in OnboardingChecklist/onboardingTasks.ts and call it from all three, so the non-persistable-user rule lives in one place (repo convention: "DRY — grep for an existing implementation before adding a helper", and "Look for duplicate code across the affected area").
| filters, | ||
| orderBy, | ||
| }); | ||
| if ( |
There was a problem hiding this comment.
🔵 minor — The client-side advancedQuery recording branch has no test
Only the pure helper is covered (__tests__/onboardingTasks.test.ts); the branch that actually decides whether "Explore your data" completes — the recordExploration flag, the hasExploredData short-circuit, and the debouncedSubmit/debouncedCatchUpSubmit split at lines 1323-1327 — is never exercised. The interesting regression is silent: because one debouncer is shared and Mantine's useDebouncedCallback keeps the last call's args, a handleSetFilters submit landing within 1s of debouncedCatchUpSubmit() (e.g. source switch → filter reconcile at line 1478) overrides the suppression and credits the task without a user search. Add a DBSearchPage test that asserts completeOnboardingTask.mutate is called for a user-initiated submit with a non-empty where and not called for the programmatic catch-up submit.
2 participants
What
Adds a second phase to the sidebar onboarding checklist. After the existing setup steps (connect ClickHouse, create sources, add data) complete, a product-usage phase tracks four "getting started" milestones per user:
Each task links to where to do it, the card can be dismissed, and a brief celebration shows when the final task is completed in-session.
Why
The existing checklist stops once a team is technically set up. New users still need a nudge toward the actions that deliver value (searching, charting, alerting, querying via an agent). This extends the same surface to cover first real usage without adding a separate onboarding UI.
How it works
ONBOARDING_TASK_IDS) lives incommon-utils. It's typed so adding a task is a compile error until both the API validation enum and the frontend UI (copy + link) are updated — a new task can't be silently untracked.onboardingDatain the cachedmeobject — nomerefetch — so the manyuseMeconsumers (metadata, ClickHouse settings, nav) are untouched.isDismissedis only for the manual X (opting out early).ONBOARDING_TASK_IDS(so removing a task can't 500GET /me), while the write boundary stays a strict enum.onboardingDatais defaulted for users created before the field existed.Demo
demo.mp4
Persistence
New optional
user.onboardingDatasubdocument (completedTasks,isDismissed). Two new routes:POST /me/onboarding/taskandPATCH /me/onboarding/dismiss.Testing
make ci-lint— lint + tsc + styles + escape-hatch ratchet: greenmake ci-unitscope — app (onboarding suites + DBSearchPage + dashboard, 274 tests), common-utils (2424), api (825): all passme.int.test.ts, externaldashboards.int.test.ts); they require the Docker stack (make dev-int) and were not run in this session.