feat: add context-aware getting-started onboarding checklist - #3084
feat: add context-aware getting-started onboarding checklist#3084brandon-pereira wants to merge 1 commit into
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: daf9c78 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 per-user onboarding persistence and API mutations, records milestones across search, dashboard, alert, and MCP surfaces, and replaces the sidebar checklist with a two-phase setup/product experience.
Confidence Score: 4/5The PR should not merge until existing-user rollout is gated and the explicit semantic-text requirement is satisfied; the query and cache issues should also be corrected. Legacy users default to an open onboarding state after the former team-age gate is removed, so the checklist appears across the existing installed base. The implementation also queries ClickHouse after the card is irrelevant and can regress cached completion during overlapping mutations. Files Needing Attention: packages/app/src/OnboardingChecklist/useOnboardingCompletion.ts, packages/app/src/api.ts, packages/app/src/OnboardingChecklist/OnboardingChecklist.tsx
|
| Filename | Overview |
|---|---|
| packages/app/src/OnboardingChecklist/useOnboardingCompletion.ts | Implements phase selection and visibility, but removes legacy-user eligibility gating and runs the setup data query after the card is no longer relevant. |
| packages/app/src/api.ts | Adds onboarding mutations and cache updates; concurrent full-state replacements can regress completed tasks locally. |
| packages/api/src/controllers/user.ts | Adds idempotent persisted completion and dismissal helpers with fire-and-forget recording. |
| packages/api/src/routers/api/me.ts | Exposes defaulted onboarding state and authenticated mutation endpoints with strict request validation. |
| packages/common-utils/src/types.ts | Defines the shared task registry and read-tolerant, write-strict onboarding schemas. |
| packages/app/src/OnboardingChecklist/OnboardingChecklist.tsx | Renders the redesigned checklist and celebration, with one theme-token rule violation. |
| packages/app/src/DBSearchPage.tsx | Records the exploration milestone for non-trivial submitted searches. |
| packages/api/src/mcp/utils/tracing.ts | Records MCP onboarding completion only for tool results not classified as errors. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[User product action] --> B{Action surface}
B -->|Search or temporary dashboard| C[POST /me/onboarding/task]
B -->|Alert or saved dashboard| D[Server-side completion recorder]
B -->|Successful MCP tool call| D
C --> E[(User onboardingData)]
D --> E
E --> F[GET /me and client cache]
F --> G{Setup complete?}
G -->|No| H[Setup checklist]
G -->|Yes| I[Product-usage checklist]
I --> J[Derived completion or dismissal]
Reviews (1): Last reviewed commit: "feat: add context-aware getting-started ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const shouldShow = | ||
| inputsReady && | ||
| wasCompleteOnLoad !== null && | ||
| !onboardingData.isDismissed && | ||
| (!allTasksComplete || isCelebrating); |
There was a problem hiding this comment.
Removing the previous three-day team-age gate exposes this checklist to every existing configured user. Legacy users default to no completed tasks and isDismissed: false, so they see an unsolicited onboarding card until they manually dismiss it. Preserve an eligibility gate or initialize legacy users appropriately.
| useQueriedChartConfig(sourceRowsConfig, { | ||
| // Skip the chart query when there's no connection to query against. | ||
| // Without this guard it fires with `connection: ''` and fails Zod | ||
| // validation on the API's clickhouse-proxy. | ||
| enabled: !!firstConnection?.id, | ||
| }); |
There was a problem hiding this comment.
This row-count query is enabled whenever a connection exists, before dismissal or completion is checked. Connected users whose checklist cannot render will therefore still query system.tables on fresh AppNav mounts, adding unnecessary ClickHouse traffic. Gate the query on checklist eligibility.
| onSuccess: data => { | ||
| queryClient.setQueryData<MeApiResponse | null>(['me'], prev => | ||
| prev == null ? prev : { ...prev, onboardingData: data.onboardingData }, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
If two different task-completion requests overlap and the older response arrives last, replacing the entire cached onboardingData object can remove the newer task from the checklist even though both tasks remain persisted. Merge completed task IDs into the current cache or invalidate and reconcile the query instead.
| ))} | ||
|
|
||
| {isCelebrating && ( | ||
| <Text size="sm" c="green" fw="bold" ta="center" mt="xs" p="xs"> |
There was a problem hiding this comment.
The new celebration uses the raw Mantine color c="green" for semantic success text. The repository requires inline success text to use the theme-aware Text success variant so it remains consistent and accessible across HyperDX and ClickStack themes. This requirement must be satisfied before merging.
| <Text size="sm" c="green" fw="bold" ta="center" mt="xs" p="xs"> | |
| <Text | |
| size="sm" | |
| variant="success" | |
| fw="bold" | |
| ta="center" | |
| mt="xs" | |
| p="xs" | |
| > |
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // otherwise we'd flash the card during the setup-query load. Then hide when | ||
| // dismissed, or once all current tasks are complete (after any in-session | ||
| // celebration). | ||
| const shouldShow = |
There was a problem hiding this comment.
🟠 major — The "new team" gate was dropped — every existing user now gets a Get-started card in the sidebar
The old component computed shouldShow = isTeamLoading === false && isNewTeam (team created < 3 days ago, deleted packages/app/src/OnboardingChecklist.tsx); the replacement's shouldShow has no team-age condition at all, and api.useTeam() is no longer called. Every non-local user of every team age will now see "Get started with HyperDX 0/4" (nobody has persisted completedTasks yet) until they complete all four tasks — including connecting an MCP server — or click Dismiss. If reintroducing the card for established teams is intended, say so in the PR description; otherwise re-add the team-age gate (or gate at least the phase-2 card on team.createdAt).
| // Skip the chart query when there's no connection to query against. | ||
| // Without this guard it fires with `connection: ''` and fails Zod | ||
| // validation on the API's clickhouse-proxy. | ||
| enabled: !!firstConnection?.id, |
There was a problem hiding this comment.
🟠 major — The system.tables row-count query now runs for every user on every window focus, including dismissed/completed ones
enabled dropped the shouldShow && term the old file had (enabled: shouldShow && !!firstConnection?.id), where shouldShow required a team younger than 3 days. The hook now runs for every user whenever the sidebar is expanded, and because hooks run before if (!shouldShow) return null, it also runs for users who dismissed the card or finished every task. useQueriedChartConfig sets no staleTime and _app.tsx sets no query defaults, so with refetchOnWindowFocus on by default this fires a ClickHouse sum(total_rows) FROM system.tables on every refocus, forever. Gate it, e.g. enabled: !!firstConnection?.id && !onboardingData?.isDismissed.
| // dashboard-tile alerts, which never hit the /alerts router). These verify | ||
| // the recording end-to-end through GET /me. | ||
| describe('onboarding task recording via product actions', () => { | ||
| it('records the dashboard task when a dashboard with a tile is created', async () => { |
There was a problem hiding this comment.
🔵 minor — Integration tests assert a fire-and-forget write synchronously, so they can flake
recordOnboardingTaskCompletion issues void User.updateOne(...) (packages/api/src/controllers/user.ts:73) and is not awaited by createDashboard/createAlert, so the HTTP response can be sent before the Mongo write lands and the immediately-following GET /me may read the pre-write state — the negative assertion not.toContain('alert') on line 115 can also pass vacuously. The same author already handled this in packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts with a waitForTask poll; reuse that polling helper here for all five tests in this describe block.
| ); | ||
| } | ||
|
|
||
| recordDashboardOnboardingIfHasTiles(userId, updatedDashboard.tiles); |
There was a problem hiding this comment.
🔵 minor — 'Build a dashboard' / 'Set up an alert' complete on edits where the user built nothing
updateDashboard records dashboard on any successful update of a dashboard that already has tiles — renaming it, changing tags, or dragging a tile all tick the box. Likewise createOrUpdateDashboardAlerts (packages/api/src/controllers/alerts.ts:411) records alert whenever the saved dashboard carries any tile alert, so a user who opens a teammate's dashboard and moves one tile gets both milestones without creating either. updateDashboard already loads oldDashboard, so record only when the tile count went from 0 to >0; for alerts, record only for the tiles whose alert was actually inserted (the upsert result exposes this).
| // tile (matches the internal controllers; an empty dashboard shell does | ||
| // not count). Fire-and-forget, so it never affects the response. | ||
| if (newDashboard.tiles.length > 0) { | ||
| recordOnboardingTaskCompletion(req.user?._id, 'dashboard'); |
There was a problem hiding this comment.
🔵 minor — The "dashboard has tiles" rule is re-implemented at five call sites instead of using the new helper
recordDashboardOnboardingIfHasTiles (packages/api/src/controllers/dashboard.ts:34) encodes the rule but isn't exported, so the same if (tiles.length > 0) recordOnboardingTaskCompletion(userId, 'dashboard') is copied here (lines 2652 and 2922), in packages/api/src/mcp/tools/dashboards/saveDashboard.ts (226, 395) and patchDashboard.ts (237). Export the helper and call it from all five, so changing the definition of the milestone is a one-line change (the repo's DRY rule is marked REQUIRED).
| if (prev == null) { | ||
| return prev; | ||
| } | ||
| if (prev.onboardingData.completedTasks.includes(taskId)) { |
There was a problem hiding this comment.
🔵 minor — useMarkOnboardingTaskComplete dereferences onboardingData unguarded, unlike its two sibling readers
prev.onboardingData.completedTasks.includes(taskId) throws if a cached me predates the field (an app bundle talking to an API pod that hasn't rolled yet returns me without onboardingData). It runs inside useCreateDashboard/useUpdateDashboard onSuccess, and a throw there puts the mutation into the error state, so setDashboard shows "Unable to save dashboard" for a dashboard that actually saved. DBSearchPage.tsx:1258 and dashboard.ts:236 already use ?. for the same read — do the same here (prev.onboardingData?.completedTasks) and bail out when it's missing.
| ))} | ||
|
|
||
| {isCelebrating && ( | ||
| <Text size="sm" c="green" fw="bold" ta="center" mt="xs" p="xs"> |
There was a problem hiding this comment.
🔵 minor — New checklist UI uses raw Mantine palette colors instead of the semantic Text variant / tokens
<Text c="green"> for the celebration should be <Text variant="success"> per the semantic-variant rule ("don't reach for raw palette colors for semantic status text"). Same in StepRow.tsx: ThemeIcon color="green" (27), hardcoded IconCheck color="#fff" (28) and ThemeIcon color="gray.4" (33) — use var(--color-*) tokens so both brands and light/dark stay consistent.
| // id added to ONBOARDING_TASK_IDS is a compile error here until it's given a | ||
| // weight — and because the order is derived by sorting ONBOARDING_TASK_IDS | ||
| // (the SSOT), that new id always renders and can't be silently untracked. | ||
| const PRODUCT_TASK_ORDER_WEIGHT: Record<OnboardingTaskId, number> = { |
There was a problem hiding this comment.
🔵 minor — PRODUCT_TASK_ORDER_WEIGHT is a third registry that exists only to re-sort the single source of truth
ONBOARDING_TASK_IDS is already an ordered tuple and PRODUCT_TASKS already forces exhaustiveness via Record<OnboardingTaskId, …>; the weight map plus the .sort() only exist because the enum is declared in a different order than the UI wants. Order ONBOARDING_TASK_IDS in common-utils/src/types.ts as the display order and delete the weight map and the sort — one fewer place to update when a task is added.
| steps, | ||
| phaseLabel, | ||
| completedCount, | ||
| isPhaseComplete, |
There was a problem hiding this comment.
🔵 minor — isPhaseComplete is exported from the hook but no caller reads it
The only consumer, OnboardingChecklist.tsx:28-37, does not destructure it, and grep finds no other caller of useOnboardingCompletion. Drop it from the returned object and from the OnboardingCompletion interface; the local const isPhaseComplete is still needed for allTasksComplete.
| // every subsequent qualifying search fires a redundant (idempotent) POST. | ||
| const hasWhere = where.trim() !== ''; | ||
| const hasFilters = (filters ?? []).length > 0; | ||
| if (!IS_LOCAL_MODE && !hasExploredData && (hasWhere || hasFilters)) { |
There was a problem hiding this comment.
🔵 minor — The advancedQuery trigger — the only client-recorded task with real conditions — has no test
The new tests cover the checklist UI, the cache patcher, the local-dashboard branch and the API, but nothing exercises this condition: that a blank search records nothing, that a where-clause or an applied filter records advancedQuery, and that hasExploredData short-circuits repeat POSTs. DBSearchPage.directTrace.test.tsx only stubs the hook. Add a small test around onSubmit (or extract the predicate into a pure helper and test that).
PR Review10 finding(s): 🔴 0 critical · 🟠 2 major · 🔵 8 minor 10 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 ship-blockers surfaced across 12 reviewers. The onboarding routes are authz-safe (user id from session, never the body), taskId is enum-validated, 🟡 P2 — recommended
🔵 P3 nitpicks (9)
Reviewers (12): correctness, testing, maintainability, project-standards, security, api-contract, reliability, kieran-typescript, julik-frontend-races, adversarial, agent-native, learnings-researcher. Testing gaps:
|
1 participant
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.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.