From b8e65f39ee456a8b9cfc193c108d69af696444b4 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 12 Aug 2026 09:54:07 -0400 Subject: [PATCH 1/3] feat: add HyperDX Labs for per-user opt-in to unfinished features Shipping a half-finished feature previously meant either holding a long-lived branch or adding a hardcoded constant to the app's config and redeploying to change it. Labs replaces that with a per-user toggle so unfinished work can merge to main default-off and the people who want it can turn it on themselves. Opt-ins are stored on the User document and surfaced via GET /me, so they follow a user across browsers and devices. Adding a lab is a single entry in packages/app/src/labs/registry.ts plus a useIsLabEnabled call; the server validates the shape of what it stores but deliberately does not know the lab id list, which is what keeps it to one file. The registry ships empty. This is the mechanism, not any experiment. --- .changeset/hyperdx-labs.md | 12 + AGENTS.md | 3 + agent_docs/README.md | 1 + agent_docs/labs.md | 158 ++++++++++ packages/api/src/controllers/user.ts | 23 ++ packages/api/src/models/user.ts | 18 ++ .../src/routers/api/__tests__/me.int.test.ts | 168 +++++++++++ packages/api/src/routers/api/me.ts | 47 ++- packages/app/src/api.ts | 8 +- .../components/AppNav/AppNav.components.tsx | 14 +- packages/app/src/components/AppNav/AppNav.tsx | 5 + packages/app/src/config.ts | 9 + packages/app/src/labs/LabsModal.tsx | 119 ++++++++ .../app/src/labs/__tests__/registry.test.ts | 42 +++ .../labs/__tests__/useLabs.localMode.test.tsx | 59 ++++ .../app/src/labs/__tests__/useLabs.test.tsx | 279 ++++++++++++++++++ packages/app/src/labs/registry.ts | 47 +++ packages/app/src/labs/useLabs.ts | 144 +++++++++ .../e2e/components/LabsModalComponent.ts | 90 ++++++ .../app/tests/e2e/core/navigation.spec.ts | 21 ++ packages/common-utils/src/types.ts | 78 +++++ 21 files changed, 1342 insertions(+), 3 deletions(-) create mode 100644 .changeset/hyperdx-labs.md create mode 100644 agent_docs/labs.md create mode 100644 packages/api/src/routers/api/__tests__/me.int.test.ts create mode 100644 packages/app/src/labs/LabsModal.tsx create mode 100644 packages/app/src/labs/__tests__/registry.test.ts create mode 100644 packages/app/src/labs/__tests__/useLabs.localMode.test.tsx create mode 100644 packages/app/src/labs/__tests__/useLabs.test.tsx create mode 100644 packages/app/src/labs/registry.ts create mode 100644 packages/app/src/labs/useLabs.ts create mode 100644 packages/app/tests/e2e/components/LabsModalComponent.ts diff --git a/.changeset/hyperdx-labs.md b/.changeset/hyperdx-labs.md new file mode 100644 index 0000000000..cf18077799 --- /dev/null +++ b/.changeset/hyperdx-labs.md @@ -0,0 +1,12 @@ +--- +'@hyperdx/common-utils': minor +'@hyperdx/api': minor +'@hyperdx/app': minor +--- + +Add HyperDX Labs, a per-user opt-in for features that are still being built. +Open it from the user menu in the nav to see what's available and switch +individual experiments on or off. Everything is off by default, and your choices +are saved to your account rather than the browser, so they follow you across +devices. Adds `PATCH /me/labs` and surfaces the opt-ins on `GET /me`. No +experiments ship in this release — this is the mechanism they'll use. diff --git a/AGENTS.md b/AGENTS.md index 24c1c24400..94c83253a8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -65,6 +65,9 @@ directory: actively coding) - `agent_docs/observability.md` - Instrumentation standards (tracing, metrics, context) and the shared helpers (read when adding or changing a feature) +- `agent_docs/labs.md` - HyperDX Labs: putting an unfinished feature behind a + per-user opt-in instead of a branch or a redeploy, and how to graduate or + retire it (read before adding a feature flag of any kind) **Package-specific guides** (read when working on that package): diff --git a/agent_docs/README.md b/agent_docs/README.md index 3a1f5444ea..c7093582ef 100644 --- a/agent_docs/README.md +++ b/agent_docs/README.md @@ -20,6 +20,7 @@ Instead of stuffing all instructions into `AGENTS.md` (which goes into every con - **`data_viz_colors.md`** - Chart, heatmap, and semantic status colors. Read before adding or changing any color in a chart, sparkline, heatmap, legend, or status pill. - **`themes.md`** - How the brand theme system (HyperDX vs ClickStack) and color mode (light/dark/system) work. Read before changing anything in `packages/app/src/theme/`, adding semantic CSS variables, or touching brand-conditional UI. - **`evals.md`** - MCP eval framework: dual-slot setup, running A/B comparisons between branches, interpreting results. Read before running evals or benchmarking MCP changes. +- **`labs.md`** - HyperDX Labs: how to put a half-finished feature behind a per-user opt-in, and how to graduate or retire it. Read before adding a feature flag of any kind. ## Usage Pattern diff --git a/agent_docs/labs.md b/agent_docs/labs.md new file mode 100644 index 0000000000..80ee0024bc --- /dev/null +++ b/agent_docs/labs.md @@ -0,0 +1,158 @@ +# HyperDX Labs + +Per-user, server-persisted opt-ins for features that aren't finished yet. + +The point is to stop choosing between a long-lived branch and a redeploy. Merge +the half-built thing to `main` behind a lab, let the people who want it turn it +on, and collect feedback while you finish. Off by default, so nobody is +surprised. + +**Labs are a user choice.** If the toggle is really a deployment decision — this +install doesn't have the backend, or the feature is off everywhere until launch — +it isn't a lab, it's a constant in `packages/app/src/config.ts` next to +`IS_MTVIEWS_ENABLED`. + +## Adding a lab + +Two edits. Add an entry to `packages/app/src/labs/registry.ts`: + +```ts +export const LABS: readonly Lab[] = [ + { + id: 'trace-flamegraph', + title: 'Trace flamegraph', + description: + 'Renders the trace waterfall as a flamegraph. Span links are not drawn yet, and very wide traces can be slow to lay out.', + badge: 'Alpha', + addedAt: '2026-08-14', + owner: '@your-handle', + }, +]; +``` + +Then gate the feature: + +```ts +import { useIsLabEnabled } from '@/labs/useLabs'; + +const isFlamegraphEnabled = useIsLabEnabled('trace-flamegraph'); +... +{isFlamegraphEnabled ? : } +``` + +No API change, no schema change, no migration, no new endpoint. The server +stores whatever ids the client sends (bounded — see below), so it never needs to +learn about your lab. + +Write the `description` for someone deciding whether to opt in. Say what's rough, +not just what's new: "span links aren't drawn yet" beats "improved trace view". + +### Checklist + +- `id` is kebab-case (`[a-z0-9]+(-[a-z0-9]+)*`) and permanent. It's persisted on + the user document, so renaming it silently resets everyone's opt-in. +- `addedAt` is today, `owner` is you. Both feed the graduate-or-retire sweep. +- Add an E2E test that toggles the lab, reloads, and asserts it stuck. + `packages/app/tests/e2e/components/LabsModalComponent.ts` has `setLab(id, on)` + (clicks and waits for the PATCH) and `labSwitch(id)` (the input, for + `toBeChecked()`) waiting for exactly this. Both were verified against a + throwaway entry, but the committed suite only covers the empty state, so yours + will be the first run in CI. +- A changeset, if the lab is visible to users at all. + +## Gating rules + +`useIsLabEnabled(id)` returns `false` when the user hasn't opted in, while `/me` +is still loading, and always in local mode. Two consequences worth knowing: + +**The loading window is real.** `/me` isn't resolved on first paint — +`AuthLoadingBlocker` is only mounted on the landing page, so it does not gate the +app. A lab therefore reads OFF and then flips ON a moment later. For the common +shape (an extra tab, an extra button, an alternate renderer) that's fine. If the +flip is user-visible — a redirect, a default tab, a one-shot effect, a +mount-time fetch — use `useLabs()` and branch on `isLoading` first: + +```ts +const { enabled, isLoading } = useLabs(); +if (isLoading) return ; +``` + +**Local mode has no labs.** It has no API server and no user identity at all, so +there is nothing to persist an opt-in against; `IS_LABS_ENABLED` is false there +and the menu entry is hidden. If your feature *also* structurally cannot work +without a backend, say so at the call site the way `IS_IAC_EXPORT_ENABLED` does: + +```ts +const isEnabled = useIsLabEnabled('remote-mtviews') && !IS_LOCAL_MODE; +``` + +That's redundant at runtime, but it documents that the feature is impossible +there rather than merely un-opted-into. Drop it when that isn't true. + +## Graduating and retiring + +**A lab is a commitment to decide, not a commitment to ship.** About 60 days +after `addedAt`, the owner picks one of two exits. "Leave it in Labs" is not an +exit — that's how you end up with fourteen flags nobody can explain. + +- **Graduate** — delete the registry entry, delete the gate, keep the new branch + of the conditional. +- **Retire** — delete the registry entry, delete the gate, delete the feature. + +Either way: **after deleting the entry, grep for the id.** A gate whose id has no +registry entry silently reads `false` forever, so nothing breaks loudly — the +gated code just goes dead and stays in the tree. The grep is the whole retirement +checklist. + +Stored `true` values for a deleted id go inert immediately (the hook derives +state from the registry, not from what's stored) and are pruned from the document +the next time that user toggles anything. No migration, no cleanup job. + +Enforcement is a review habit, not CI. A date-based test would fail on somebody +else's unrelated PR at 2am and get its constant bumped within the hour. The +registry is one short file — reading it is a 30-second sweep, and that +single-file-ness is the actual anti-rot mechanism. + +## How it works + +| Piece | Where | +| --- | --- | +| Registry (ids + UI copy) | `packages/app/src/labs/registry.ts` | +| Hook — the only seam | `packages/app/src/labs/useLabs.ts` | +| Modal (nav user menu) | `packages/app/src/labs/LabsModal.tsx` | +| Deployment gate | `IS_LABS_ENABLED` in `packages/app/src/config.ts` | +| Storage | `labs` on the `User` document, `packages/api/src/models/user.ts` | +| Read / write | `GET /me` and `PATCH /me/labs`, `packages/api/src/routers/api/me.ts` | +| Shape contract | `UserLabsSchema` in `packages/common-utils/src/types.ts` | + +State is an **enabled-set**: a key present with `true` is on, an absent key is +off. Writes are **full replace** — the client always holds the whole registry, so +it can always compute the complete desired set, and that's what makes retired ids +self-pruning. + +**The server validates shape, not ids.** It bounds the key format (kebab-case, +which also excludes `$`, `.` and `_`, so Mongo operators, dotted paths and +`__proto__` can't be keys) and the entry count, but it deliberately does not know +which labs exist — that's what keeps adding one to a single file. The trade-off: +a typo'd id can't be rejected server-side, so +`packages/app/src/labs/__tests__/registry.test.ts` parses every registry id +against `LabIdSchema` to catch `my_lab` or `My-Lab` at CI time instead of as a +mystery 400. + +Reads compare `=== true` rather than truthiness, because a key like `constructor` +passes the id regex and is inherited from `Object.prototype`, where it's truthy. + +**Multi-tab / multi-device:** a toggle is visible immediately in the tab you +clicked it in (the mutation is optimistic), in other tabs on next focus, and on +other devices on next load. Two tabs toggling *different* labs from the same +snapshot can lose one — full replace is last-write-wins. The cost is "flip it +again"; if that ever stops being acceptable, merge server-side in `setUserLabs`. + +## Related + +`packages/app/src/hooks/useIsVariablesEnabled.ts` predates this and is the +clearest example of what labs are for: an env-var toggle +(`NEXT_PUBLIC_ENABLE_DASHBOARD_VARIABLES`) shipped because variable substitution +wasn't implemented yet, wrapped in a hook whose `isLoading: false` was left in +place, per its own comment, "to support team-level toggle loading in the future." +That's the shape `useLabs` now provides — a good first graduation candidate. diff --git a/packages/api/src/controllers/user.ts b/packages/api/src/controllers/user.ts index 55258c2db6..61a4b4a0b7 100644 --- a/packages/api/src/controllers/user.ts +++ b/packages/api/src/controllers/user.ts @@ -1,3 +1,4 @@ +import type { UserLabs } from '@hyperdx/common-utils/dist/types'; import mongoose from 'mongoose'; import type { ObjectId } from '@/models'; @@ -20,6 +21,28 @@ export function findUsersByTeam(team: string | ObjectId) { return User.find({ team }).sort({ createdAt: 1 }); } +/** + * Replaces a user's lab opt-ins wholesale. + * + * Whole-object `$set`, deliberately not `$set: { ['labs.' + id]: value }`: a + * dotted path is the one place a client-supplied key stops being update *data* + * and becomes part of the update *instruction*. Keeping keys in value position + * removes that class of bug outright, which is what lets LabIdSchema's key + * regex be defense-in-depth rather than the only defense. + * + * There is nothing to read first because the semantics are full-replace: the + * read-modify-write happens on the client, which is the only place that knows + * the current lab registry and therefore the only place that can prune the ids + * of retired labs. See agent_docs/labs.md. + */ +export function setUserLabs(userId: ObjectId, labs: UserLabs) { + return User.findByIdAndUpdate( + userId, + { $set: { labs } }, + { new: true, projection: { labs: 1 } }, + ); +} + export async function deleteTeamMember( teamId: string | ObjectId, userIdToDelete: string, diff --git a/packages/api/src/models/user.ts b/packages/api/src/models/user.ts index 8f60fadeb5..6e0513d51a 100644 --- a/packages/api/src/models/user.ts +++ b/packages/api/src/models/user.ts @@ -12,6 +12,17 @@ export interface IUser { email: string; name: string; team: ObjectId; + /** + * Per-user opt-ins for in-development features ("HyperDX Labs"). An + * enabled-set: a key present with `true` is on, an absent key is off. + * + * Deliberately not typed per-lab. The registry lives in + * packages/app/src/labs/registry.ts so that adding a lab never has to touch + * this file; ids and count are bounded on the write path by UserLabsSchema. + * Absent on every document created before labs existed, which reads as "no + * labs enabled". See agent_docs/labs.md. + */ + labs?: Record; } export type UserDocument = mongoose.HydratedDocument; @@ -30,6 +41,13 @@ const UserSchema = new Schema( return uuidv4(); }, }, + // Mixed, not Map: a Mongoose Map's own toJSON() returns a *native* Map + // unless handed { flattenMaps: true }, and `GET /me` passes this value + // straight to res.json() — it would serialize as `{}` forever. See the + // MongooseMap note in packages/api/src/models/webhook.ts. Mixed round-trips + // as a plain object. Its one weakness (no change tracking on nested paths) + // never comes up because setUserLabs only ever $sets the whole object. + labs: { type: Schema.Types.Mixed }, }, { timestamps: true, diff --git a/packages/api/src/routers/api/__tests__/me.int.test.ts b/packages/api/src/routers/api/__tests__/me.int.test.ts new file mode 100644 index 0000000000..23bec4122d --- /dev/null +++ b/packages/api/src/routers/api/__tests__/me.int.test.ts @@ -0,0 +1,168 @@ +import { getAgent, getLoggedInAgent, getServer } from '@/fixtures'; +import User from '@/models/user'; + +describe('me router', () => { + const server = getServer(); + + beforeAll(async () => { + await server.start(); + }); + + afterEach(async () => { + await server.clearDBs(); + }); + + afterAll(async () => { + await server.stop(); + }); + + describe('GET /me', () => { + it('returns an empty lab set for a user that has never opted in', async () => { + const { agent } = await getLoggedInAgent(server); + + const resp = await agent.get('/me').expect(200); + + // `labs` is absent on the document; the route normalizes it so the client + // never has to distinguish "no field" from "nothing enabled". + expect(resp.body.labs).toEqual({}); + }); + }); + + describe('PATCH /me/labs', () => { + it('persists an opt-in and reflects it on GET /me', async () => { + const { agent } = await getLoggedInAgent(server); + + const patchResp = await agent + .patch('/me/labs') + .send({ labs: { 'my-lab': true } }) + .expect(200); + + expect(patchResp.body).toEqual({ labs: { 'my-lab': true } }); + + const meResp = await agent.get('/me').expect(200); + expect(meResp.body.labs).toEqual({ 'my-lab': true }); + }); + + it('serializes labs as a plain object', async () => { + // Regression guard for the storage type. A Mongoose Map's toJSON() + // returns a native Map unless given { flattenMaps: true }, which + // JSON.stringify renders as `{}` — so a future switch away from Mixed + // would silently start returning nothing. See models/webhook.ts. + const { agent } = await getLoggedInAgent(server); + + await agent + .patch('/me/labs') + .send({ labs: { 'lab-one': true, 'lab-two': true } }) + .expect(200); + + const resp = await agent.get('/me').expect(200); + + expect(typeof resp.body.labs).toBe('object'); + expect(Array.isArray(resp.body.labs)).toBe(false); + expect(resp.body.labs).toEqual({ 'lab-one': true, 'lab-two': true }); + expect(Object.keys(resp.body.labs).sort()).toEqual([ + 'lab-one', + 'lab-two', + ]); + }); + + it('replaces the whole set rather than merging', async () => { + // Full-replace is what makes ids from retired labs self-pruning. + const { agent } = await getLoggedInAgent(server); + + await agent + .patch('/me/labs') + .send({ labs: { 'lab-a': true, 'lab-b': true } }) + .expect(200); + + await agent + .patch('/me/labs') + .send({ labs: { 'lab-a': true } }) + .expect(200); + + const resp = await agent.get('/me').expect(200); + expect(resp.body.labs).toEqual({ 'lab-a': true }); + }); + + it.each([ + ['an underscore', { my_lab: true }], + ['uppercase', { 'My-Lab': true }], + ['a dotted path', { 'a.b': true }], + ['a mongo operator', { $set: true }], + ['a leading dash', { '-lab': true }], + ['an empty id', { '': true }], + ])('rejects a lab id containing %s', async (_label, labs) => { + const { agent } = await getLoggedInAgent(server); + + await agent.patch('/me/labs').send({ labs }).expect(400); + }); + + it('rejects __proto__ as a lab id without polluting Object.prototype', async () => { + const { agent } = await getLoggedInAgent(server); + + // Sent as a raw JSON string so `__proto__` arrives as an own property, + // which is how express.json() parses it off the wire. + await agent + .patch('/me/labs') + .set('Content-Type', 'application/json') + .send('{"labs":{"__proto__":true}}') + .expect(400); + + expect(Object.getPrototypeOf({})).toBe(Object.prototype); + expect({}).not.toHaveProperty('labs'); + }); + + it('rejects a non-boolean value', async () => { + const { agent } = await getLoggedInAgent(server); + + await agent + .patch('/me/labs') + .send({ labs: { 'lab-a': 'true' } }) + .expect(400); + }); + + it('rejects more lab entries than a user may store', async () => { + const { agent } = await getLoggedInAgent(server); + + const tooMany: Record = {}; + for (let i = 0; i < 65; i++) { + tooMany[`lab-${i}`] = true; + } + + await agent.patch('/me/labs').send({ labs: tooMany }).expect(400); + }); + + it('rejects an unauthenticated request', async () => { + const anon = getAgent(server); + + await anon + .patch('/me/labs') + .send({ labs: { 'lab-a': true } }) + .expect(401); + }); + + it('scopes the write to the requesting user, not the team', async () => { + // The second user is created directly rather than via getLoggedInAgent: + // POST /register/password refuses once a team exists, so a second session + // would need the invite flow. The concern here is only that the $set + // targets req.user._id, which this covers. + const { agent, team, user: userA } = await getLoggedInAgent(server); + const userB = await User.create({ + email: 'second@example.com', + name: 'Second', + team: team._id, + }); + + await agent + .patch('/me/labs') + .send({ labs: { 'lab-a': true } }) + .expect(200); + + const storedA = await User.findById(userA._id); + expect(storedA?.labs).toEqual({ 'lab-a': true }); + + const storedB = await User.findById(userB._id); + expect(storedB?.labs).toBeUndefined(); + }); + }); +}); diff --git a/packages/api/src/routers/api/me.ts b/packages/api/src/routers/api/me.ts index b574928528..65d55df698 100644 --- a/packages/api/src/routers/api/me.ts +++ b/packages/api/src/routers/api/me.ts @@ -1,9 +1,17 @@ -import type { MeApiResponse } from '@hyperdx/common-utils/dist/types'; +import type { + MeApiResponse, + UpdateUserLabsApiResponse, +} from '@hyperdx/common-utils/dist/types'; +import { UpdateUserLabsRequestSchema } from '@hyperdx/common-utils/dist/types'; import express from 'express'; +import { processRequest } from 'zod-express-middleware'; import { AI_API_KEY, ANTHROPIC_API_KEY, USAGE_STATS_ENABLED } from '@/config'; import { getTeam } from '@/controllers/team'; +import { setUserLabs } from '@/controllers/user'; +import { getNonNullUserWithTeam } from '@/middleware/auth'; import { Api404Error } from '@/utils/errors'; +import { setBusinessContext } from '@/utils/instrumentation'; import { sendJson } from '@/utils/serialization'; const router = express.Router(); @@ -19,6 +27,7 @@ router.get('/', async (req, res: express.Response, next) => { accessKey, createdAt, email, + labs, name, team: teamId, } = req.user; @@ -37,10 +46,46 @@ router.get('/', async (req, res: express.Response, next) => { team, usageStatsEnabled: USAGE_STATS_ENABLED, aiAssistantEnabled: !!(AI_API_KEY || ANTHROPIC_API_KEY), + // Absent on every user created before labs existed; `{}` reads as + // "nothing enabled", which is what the client expects. + labs: labs ?? {}, }); } catch (e) { next(e); } }); +/** + * Replaces the caller's lab opt-ins. A sub-path rather than `PATCH /me` because + * `GET /me` also returns email, name, accessKey and team — an endpoint that + * merely *looks* like it accepts edits to those is a hazard worth avoiding. + */ +router.patch( + '/labs', + processRequest({ body: UpdateUserLabsRequestSchema }), + async (req, res: express.Response, next) => { + try { + const { userId } = getNonNullUserWithTeam(req); + const { labs } = req.body; + + const user = await setUserLabs(userId, labs); + if (user == null) { + throw new Api404Error(`User not found for id ${userId}`); + } + + // A wide-event span attribute, deliberately not a metric: lab ids are + // client-supplied, so using them as metric attribute values would be + // unbounded cardinality. Team/user context is already on the span from + // isUserAuthenticated. See agent_docs/observability.md. + setBusinessContext({ + 'hyperdx.user.labs.enabled': Object.keys(labs).filter(id => labs[id]), + }); + + return sendJson(res, { labs: user.labs ?? {} }); + } catch (e) { + next(e); + } + }, +); + export default router; diff --git a/packages/app/src/api.ts b/packages/app/src/api.ts index 21facd9c05..23a826cd3a 100644 --- a/packages/app/src/api.ts +++ b/packages/app/src/api.ts @@ -76,6 +76,12 @@ export const hdxServer = ( }); }; +/** + * Query key for `useMe`. Exported so code that writes into the `/me` cache + * (src/labs/useLabs.ts) can't drift from the key `useMe` reads. + */ +export const ME_QUERY_KEY = ['me'] as const; + const api = { useCreateAlert() { return useMutation<{ data: Alert }, Error, Alert>({ @@ -291,7 +297,7 @@ const api = { }, useMe() { return useQuery({ - queryKey: [`me`], + queryKey: ME_QUERY_KEY, queryFn: () => { if (IS_LOCAL_MODE) { return null; diff --git a/packages/app/src/components/AppNav/AppNav.components.tsx b/packages/app/src/components/AppNav/AppNav.components.tsx index 95344a749c..ed7fc64857 100644 --- a/packages/app/src/components/AppNav/AppNav.components.tsx +++ b/packages/app/src/components/AppNav/AppNav.components.tsx @@ -20,6 +20,7 @@ import { IconChevronDown, IconChevronRight, IconChevronUp, + IconFlask, IconHelp, IconKeyboard, IconLogout, @@ -28,7 +29,7 @@ import { IconUserCog, } from '@tabler/icons-react'; -import { IS_LOCAL_MODE } from '@/config'; +import { IS_LABS_ENABLED, IS_LOCAL_MODE } from '@/config'; import { ChangelogModal } from './ChangelogModal'; import { KeyboardShortcutsModal } from './KeyboardShortcutsModal'; @@ -68,6 +69,7 @@ type AppNavUserMenuProps = { teamName?: string; logoutUrl?: string | null; onClickUserPreferences?: () => void; + onClickLabs?: () => void; }; const getUserInitials = (userName: string) => { @@ -85,6 +87,7 @@ export const AppNavUserMenu = ({ teamName, logoutUrl, onClickUserPreferences, + onClickLabs, }: AppNavUserMenuProps) => { const { isCollapsed } = React.useContext(AppNavContext); const resolvedUserName = userName.trim() || 'User'; @@ -161,6 +164,15 @@ export const AppNavUserMenu = ({ > User Preferences + {IS_LABS_ENABLED && ( + } + onClick={onClickLabs} + > + HyperDX Labs + + )} {logoutUrl && ( <> diff --git a/packages/app/src/components/AppNav/AppNav.tsx b/packages/app/src/components/AppNav/AppNav.tsx index 13dc89e695..0d794fd116 100644 --- a/packages/app/src/components/AppNav/AppNav.tsx +++ b/packages/app/src/components/AppNav/AppNav.tsx @@ -34,6 +34,7 @@ import { IS_LOCAL_MODE } from '@/config'; import { Dashboard, useDashboards } from '@/dashboard'; import { useFavorites } from '@/favorites'; import InstallInstructionModal from '@/InstallInstructionsModal'; +import { LabsModal } from '@/labs/LabsModal'; import OnboardingChecklist from '@/OnboardingChecklist'; import { useSavedSearches } from '@/savedSearch'; import { useLogomark, useWordmark } from '@/theme/ThemeProvider'; @@ -269,6 +270,8 @@ export default function AppNav({ fixed = false }: { fixed?: boolean }) { { close: closeUserPreferences, open: openUserPreferences }, ] = useDisclosure(false); + const [labsOpen, { close: closeLabs, open: openLabs }] = useDisclosure(false); + const { userPreferences: { isUTC }, } = useUserPreferences(); @@ -518,6 +521,7 @@ export default function AppNav({ fixed = false }: { fixed?: boolean }) { userName={meData?.name} teamName={meData?.team?.name} onClickUserPreferences={openUserPreferences} + onClickLabs={openLabs} logoutUrl={IS_LOCAL_MODE ? null : `/api/logout`} /> {meData?.usageStatsEnabled && ( @@ -539,6 +543,7 @@ export default function AppNav({ fixed = false }: { fixed?: boolean }) { opened={UserPreferencesOpen} onClose={closeUserPreferences} /> + ); } diff --git a/packages/app/src/config.ts b/packages/app/src/config.ts index e687a6e4f7..358ee3e77e 100644 --- a/packages/app/src/config.ts +++ b/packages/app/src/config.ts @@ -81,3 +81,12 @@ const IS_IAC_HELPERS_ENABLED = true; // Terraform provider to talk to. Single definition — the alerts, dashboard, // search, and team-settings surfaces all read this one constant. export const IS_IAC_EXPORT_ENABLED = IS_IAC_HELPERS_ENABLED && !IS_LOCAL_MODE; + +// HyperDX Labs: per-user opt-in toggles for in-development features, persisted +// on the User document. Local mode has no API server and no user identity at +// all (`useMe()` returns null there), so there is nothing to persist an opt-in +// against — the local-mode analogue of a lab is a constant in this very file, +// like IS_MTVIEWS_ENABLED above. Same shape as IS_IAC_EXPORT_ENABLED: one +// exported gate, so no caller can forget the local-mode check. Every branch on +// this lives in src/labs/useLabs.ts. See agent_docs/labs.md. +export const IS_LABS_ENABLED = !IS_LOCAL_MODE; diff --git a/packages/app/src/labs/LabsModal.tsx b/packages/app/src/labs/LabsModal.tsx new file mode 100644 index 0000000000..fe933790a4 --- /dev/null +++ b/packages/app/src/labs/LabsModal.tsx @@ -0,0 +1,119 @@ +import * as React from 'react'; +import { Badge, Card, Group, Modal, Stack, Switch, Text } from '@mantine/core'; +import { IconFlask } from '@tabler/icons-react'; + +import type { Lab } from '@/labs/registry'; +import { LABS } from '@/labs/registry'; +import { useLabs } from '@/labs/useLabs'; + +const LabCard = ({ + lab, + enabled, + disabled, + onChange, +}: { + lab: Lab; + enabled: boolean; + disabled: boolean; + onChange: (enabled: boolean) => void; +}) => { + return ( + + +
+ + + {lab.title} + + {!!lab.badge && ( + + {lab.badge} + + )} + + + {lab.description} + +
+ {/* + Two testids on purpose. Mantine puts arbitrary props on the , + which it hides behind an aria-hidden track — so that one is assertable + (toBeChecked) but not clickable. wrapperProps lands on the enclosing + + + ); +}; + +export const LabsModal = ({ + opened, + onClose, +}: { + opened: boolean; + onClose: () => void; +}) => { + const { enabled, isLoading, setLabEnabled } = useLabs(); + + return ( + + +
+ HyperDX Labs + + Try features that are still being built + +
+ + } + size="lg" + padding="lg" + keepMounted={false} + opened={opened} + onClose={onClose} + > + {/* + Inner content carries its own testid: a Mantine Modal's root stays in the + DOM with zero dimensions, so it never reads as "visible" to Playwright. + Open-state assertions have to target content, closed-state the root. + */} + + {LABS.length === 0 ? ( + + No experiments are available right now. Check back soon. + + ) : ( + <> + + These are unfinished, so expect rough edges. Everything here is + off by default and you can turn it off again at any time. Your + choices are saved to your account, so they follow you across + browsers and devices. + + {LABS.map(lab => ( + setLabEnabled(lab.id, value)} + /> + ))} + + )} + + + ); +}; diff --git a/packages/app/src/labs/__tests__/registry.test.ts b/packages/app/src/labs/__tests__/registry.test.ts new file mode 100644 index 0000000000..1013c34cbb --- /dev/null +++ b/packages/app/src/labs/__tests__/registry.test.ts @@ -0,0 +1,42 @@ +import { LabIdSchema, LABS_MAX_KEYS } from '@hyperdx/common-utils/dist/types'; + +import { LABS } from '@/labs/registry'; + +/** + * The server bounds the *shape* of what a user can store but deliberately does + * not know the lab id list, so it cannot reject a typo'd id — see the Labs + * comment in common-utils/src/types.ts. These assertions are what catch + * `my_lab` or `My-Lab` at CI time instead of as a mystery 400 in the browser. + * + * They pass trivially while the registry is empty and start earning their keep + * with the first lab. + */ +describe('labs registry', () => { + it('gives every lab an id the API will accept', () => { + for (const lab of LABS) { + const result = LabIdSchema.safeParse(lab.id); + expect(result.success).toBe(true); + } + }); + + it('has no duplicate ids', () => { + const ids = LABS.map(lab => lab.id); + expect(new Set(ids).size).toBe(ids.length); + }); + + it('stays within the per-user storage cap', () => { + // Every lab in the registry can be enabled at once, so the registry itself + // must fit inside what UserLabsSchema will accept. + expect(LABS.length).toBeLessThanOrEqual(LABS_MAX_KEYS); + }); + + it('describes every lab well enough to opt into', () => { + for (const lab of LABS) { + expect(lab.title.trim()).not.toBe(''); + expect(lab.description.trim()).not.toBe(''); + expect(lab.owner.trim()).not.toBe(''); + // addedAt drives the graduate-or-retire sweep; see agent_docs/labs.md. + expect(lab.addedAt).toMatch(/^\d{4}-\d{2}-\d{2}$/); + } + }); +}); diff --git a/packages/app/src/labs/__tests__/useLabs.localMode.test.tsx b/packages/app/src/labs/__tests__/useLabs.localMode.test.tsx new file mode 100644 index 0000000000..931c5cdcf8 --- /dev/null +++ b/packages/app/src/labs/__tests__/useLabs.localMode.test.tsx @@ -0,0 +1,59 @@ +import React from 'react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook } from '@testing-library/react'; + +import { useLabs } from '@/labs/useLabs'; + +// Separate file because jest.mock is file-scoped and this is the only case that +// needs labs switched off at the deployment level. +jest.mock('@/config', () => ({ IS_LABS_ENABLED: false })); + +// Mirrors real local mode, where useMe()'s queryFn short-circuits to null. +jest.mock('@/api', () => { + const { useQuery } = jest.requireActual('@tanstack/react-query'); + return { + __esModule: true, + ME_QUERY_KEY: ['me'], + hdxServer: jest.fn(), + default: { + useMe: () => useQuery({ queryKey: ['me'], queryFn: () => null }), + }, + }; +}); + +jest.mock('@/labs/registry', () => ({ + LABS: [ + { + id: 'lab-a', + title: 'Lab A', + description: 'A', + addedAt: '2026-01-01', + owner: '@test', + }, + ], +})); + +jest.mock('@mantine/notifications', () => ({ + notifications: { show: jest.fn() }, +})); + +describe('useLabs in local mode', () => { + it('never reports loading, and every lab is off', () => { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + + const { result } = renderHook(() => useLabs(), { + wrapper: ({ children }) => ( + + {children} + + ), + }); + + // Labs are unavailable here, not pending — a loading state would never + // resolve, since there is no API server to answer. + expect(result.current.isLoading).toBe(false); + expect(result.current.enabled).toEqual({ 'lab-a': false }); + }); +}); diff --git a/packages/app/src/labs/__tests__/useLabs.test.tsx b/packages/app/src/labs/__tests__/useLabs.test.tsx new file mode 100644 index 0000000000..8b86cff47b --- /dev/null +++ b/packages/app/src/labs/__tests__/useLabs.test.tsx @@ -0,0 +1,279 @@ +import React from 'react'; +import type { MeApiResponse } from '@hyperdx/common-utils/dist/types'; +import { notifications } from '@mantine/notifications'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, renderHook, waitFor } from '@testing-library/react'; + +import { hdxServer } from '@/api'; +import { useIsLabEnabled, useLabs } from '@/labs/useLabs'; + +// A never-resolving /me keeps the query pending, which is how the OFF -> ON +// window is exercised. +const PENDING = Symbol('pending'); + +let meFixture: Partial | null | typeof PENDING = null; + +// `useMe` is mocked as a *real* useQuery over the same key rather than as a +// static return value. The optimistic update writes straight into the +// react-query cache, so the cache has to be the actual source of truth or the +// optimism and rollback assertions would be testing nothing. +jest.mock('@/api', () => { + const { useQuery } = jest.requireActual('@tanstack/react-query'); + return { + __esModule: true, + ME_QUERY_KEY: ['me'], + hdxServer: jest.fn(), + default: { + useMe: () => + useQuery({ + queryKey: ['me'], + queryFn: () => + meFixture === PENDING + ? new Promise(() => { + /* never resolves */ + }) + : meFixture, + }), + }, + }; +}); + +jest.mock('@/labs/registry', () => ({ + LABS: [ + { + id: 'lab-a', + title: 'Lab A', + description: 'A', + addedAt: '2026-01-01', + owner: '@test', + }, + { + id: 'lab-b', + title: 'Lab B', + description: 'B', + addedAt: '2026-01-01', + owner: '@test', + }, + ], +})); + +jest.mock('@mantine/notifications', () => ({ + notifications: { show: jest.fn() }, +})); + +const mockHdxServer = jest.mocked(hdxServer); +const mockShow = jest.mocked(notifications.show); + +function resolvePatch(body: unknown = { labs: {} }) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + mockHdxServer.mockReturnValue({ + json: () => Promise.resolve(body), + } as unknown as ReturnType); +} + +/** Returns a `resolve`/`reject` pair so a test can hold the PATCH in flight. */ +function deferPatch() { + let settle: { resolve: () => void; reject: () => void }; + const promise = new Promise((res, rej) => { + settle = { + resolve: () => res({ labs: {} }), + reject: () => rej(new Error('nope')), + }; + }); + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + mockHdxServer.mockReturnValue({ + json: () => promise, + } as unknown as ReturnType); + + return settle!; +} + +function renderLabs() { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + return renderHook(() => useLabs(), { + wrapper: ({ children }) => ( + {children} + ), + }); +} + +/** The PATCH body the hook sent, as an object. */ +function lastPatchBody() { + const call = mockHdxServer.mock.calls.at(-1); + return call?.[1]?.json; +} + +beforeEach(() => { + jest.clearAllMocks(); + meFixture = null; + resolvePatch(); +}); + +describe('useLabs', () => { + it('reports loading and every lab off while /me is pending', async () => { + meFixture = PENDING; + + const { result } = renderLabs(); + + expect(result.current.isLoading).toBe(true); + expect(result.current.enabled).toEqual({ + 'lab-a': false, + 'lab-b': false, + }); + }); + + it('reads every lab as off when /me carries no labs field', async () => { + // The pre-existing-user path: the field is absent on documents created + // before labs existed. + meFixture = { name: 'Test' }; + + const { result } = renderLabs(); + + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.enabled).toEqual({ + 'lab-a': false, + 'lab-b': false, + }); + }); + + it('reflects stored opt-ins', async () => { + meFixture = { labs: { 'lab-a': true } }; + + const { result } = renderLabs(); + + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + expect(result.current.enabled['lab-b']).toBe(false); + }); + + it('ignores stored ids that are not in the registry', async () => { + // A graduated or retired lab. It must not reach a consumer. + meFixture = { labs: { 'retired-lab': true, 'lab-a': true } }; + + const { result } = renderLabs(); + + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + expect(result.current.enabled).not.toHaveProperty('retired-lab'); + expect(Object.keys(result.current.enabled).sort()).toEqual([ + 'lab-a', + 'lab-b', + ]); + }); + + it('does not treat a non-boolean stored value as enabled', async () => { + // Guards the `=== true` comparison rather than truthiness. + meFixture = { + labs: { 'lab-a': 'true' } as unknown as { 'lab-a': boolean }, + }; + + const { result } = renderLabs(); + + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.enabled['lab-a']).toBe(false); + }); + + it('sends a full-replace payload that prunes unknown ids and omits disabled labs', async () => { + meFixture = { labs: { 'lab-a': true, 'retired-lab': true } }; + + const { result } = renderLabs(); + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + + act(() => result.current.setLabEnabled('lab-b', true)); + + await waitFor(() => expect(mockHdxServer).toHaveBeenCalled()); + expect(mockHdxServer).toHaveBeenCalledWith( + 'me/labs', + expect.objectContaining({ method: 'PATCH' }), + ); + // retired-lab is gone, and nothing is stored as `false`. + expect(lastPatchBody()).toEqual({ + labs: { 'lab-a': true, 'lab-b': true }, + }); + }); + + it('sends an empty set when the last enabled lab is switched off', async () => { + meFixture = { labs: { 'lab-a': true } }; + + const { result } = renderLabs(); + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + + act(() => result.current.setLabEnabled('lab-a', false)); + + await waitFor(() => expect(mockHdxServer).toHaveBeenCalled()); + expect(lastPatchBody()).toEqual({ labs: {} }); + }); + + it('applies the toggle optimistically before the request settles', async () => { + meFixture = { labs: {} }; + const settle = deferPatch(); + + const { result } = renderLabs(); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + + act(() => result.current.setLabEnabled('lab-b', true)); + + // Still in flight, but the UI already shows the new state. + await waitFor(() => expect(result.current.enabled['lab-b']).toBe(true)); + expect(result.current.isSaving).toBe(true); + + await act(async () => { + settle.resolve(); + }); + }); + + it('rolls back and notifies when the request fails', async () => { + meFixture = { labs: { 'lab-a': true } }; + const settle = deferPatch(); + + const { result } = renderLabs(); + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + + act(() => result.current.setLabEnabled('lab-b', true)); + await waitFor(() => expect(result.current.enabled['lab-b']).toBe(true)); + + await act(async () => { + settle.reject(); + }); + + await waitFor(() => expect(result.current.enabled['lab-b']).toBe(false)); + // The pre-existing opt-in survived the rollback. + expect(result.current.enabled['lab-a']).toBe(true); + expect(mockShow).toHaveBeenCalledWith( + expect.objectContaining({ color: 'red' }), + ); + }); +}); + +describe('useIsLabEnabled', () => { + function renderIsEnabled(labId: string) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + return renderHook(() => useIsLabEnabled(labId), { + wrapper: ({ children }) => ( + + {children} + + ), + }); + } + + it('returns true only for an enabled lab', async () => { + meFixture = { labs: { 'lab-a': true } }; + + const { result } = renderIsEnabled('lab-a'); + + await waitFor(() => expect(result.current).toBe(true)); + }); + + it('returns false for an id with no registry entry', async () => { + meFixture = { labs: { 'retired-lab': true } }; + + const { result } = renderIsEnabled('retired-lab'); + + // Deleting a registry entry is safe: the gate goes false rather than + // throwing. The flip side is that a typo fails silently. + await waitFor(() => expect(result.current).toBe(false)); + }); +}); diff --git a/packages/app/src/labs/registry.ts b/packages/app/src/labs/registry.ts new file mode 100644 index 0000000000..92fa00e77e --- /dev/null +++ b/packages/app/src/labs/registry.ts @@ -0,0 +1,47 @@ +/** + * The HyperDX Labs registry — the single source of truth for which experiments + * exist and how they're described to users. + * + * Adding a lab is intended to be a one-file change: add an entry here, then + * gate your feature with `useIsLabEnabled('your-id')`. No API change, no schema + * change, no migration. Read agent_docs/labs.md before adding one — in + * particular the graduate-or-retire rule, which is what keeps this file from + * accumulating zombie flags. + */ + +export type Lab = { + /** + * Stable kebab-case id, validated by `LabIdSchema` in common-utils. This is + * persisted on the user document, so changing it silently resets everyone's + * opt-in — treat it as permanent once shipped. + */ + id: string; + /** Short, user-facing name. Sentence case. */ + title: string; + /** + * What it does *and* what's still rough. Someone opting into unfinished work + * deserves to know where the edges are, so prefer "Span links aren't drawn + * yet and wide traces are slow" over "improved trace view". + */ + description: string; + /** Optional maturity hint shown next to the title, e.g. 'Alpha', 'Beta'. */ + badge?: string; + /** ISO day (YYYY-MM-DD). Drives the graduate-or-retire sweep. */ + addedAt: string; + /** Who decides this lab's fate. GitHub or Slack handle. */ + owner: string; +}; + +/** + * Deliberately empty. This ships the mechanism, not any experiments. + * + * Typed as `readonly Lab[]` rather than `[...] as const satisfies readonly + * Lab[]`. The const form is tempting because it would narrow lab ids to a + * literal union, making a deleted entry a compile error at every gate. But on + * an *empty* registry that union is `never`: every `lab.id` in a `.map` fails + * to typecheck, and `useIsLabEnabled` becomes uncallable, including from its + * own tests. Retirement is covered instead by the documented step of grepping + * for the id after deleting the entry — see agent_docs/labs.md. Worth + * revisiting once a few labs exist and the empty case is behind us. + */ +export const LABS: readonly Lab[] = []; diff --git a/packages/app/src/labs/useLabs.ts b/packages/app/src/labs/useLabs.ts new file mode 100644 index 0000000000..f10b5cc39e --- /dev/null +++ b/packages/app/src/labs/useLabs.ts @@ -0,0 +1,144 @@ +import * as React from 'react'; +import type { MeApiResponse, UserLabs } from '@hyperdx/common-utils/dist/types'; +import { notifications } from '@mantine/notifications'; +import { useMutation, useQueryClient } from '@tanstack/react-query'; + +import api, { hdxServer, ME_QUERY_KEY } from '@/api'; +import { IS_LABS_ENABLED } from '@/config'; +import { LABS } from '@/labs/registry'; + +export type LabsState = { + /** + * Enabled state for every lab in the current registry, keyed by lab id. Ids + * stored on the user but absent from the registry (a graduated or retired + * lab) are not surfaced here — see agent_docs/labs.md. + */ + enabled: Record; + /** + * True until the server's answer is known. Every lab reads OFF while this is + * true, so anything where an OFF -> ON flip is user-visible (a redirect, a + * default tab, a one-shot effect, a mount-time fetch) should branch on it + * rather than on `enabled` alone. Always false in local mode, where labs are + * unavailable rather than pending. + */ + isLoading: boolean; + /** True while a toggle is in flight. */ + isSaving: boolean; + setLabEnabled: (labId: string, enabled: boolean) => void; +}; + +/** + * Persists the caller's full lab set. Optimistic, because this backs a Switch: + * with a plain mutate-then-refetch the control would sit dead for two round + * trips. Shape mirrors src/favorites.ts, including the isMutating guard. + */ +function useUpdateUserLabs() { + const queryClient = useQueryClient(); + + return useMutation({ + // Shared with the /me query key so concurrent toggles coordinate their + // refetch rather than racing each other. + mutationKey: ME_QUERY_KEY, + mutationFn: (labs: UserLabs) => + hdxServer('me/labs', { + method: 'PATCH', + json: { labs }, + }).json<{ labs: UserLabs }>(), + onMutate: async (labs: UserLabs) => { + // Cancel outgoing /me refetches so they can't overwrite our optimistic update + await queryClient.cancelQueries({ queryKey: ME_QUERY_KEY }); + + const previous = queryClient.getQueryData( + ME_QUERY_KEY, + ); + + queryClient.setQueryData(ME_QUERY_KEY, old => + old ? { ...old, labs } : old, + ); + + return { previous }; + }, + onError: (_err, _labs, context) => { + if (context !== undefined) { + queryClient.setQueryData(ME_QUERY_KEY, context.previous); + } + notifications.show({ + color: 'red', + message: 'Failed to update HyperDX Labs', + }); + }, + onSettled: () => { + // Only refetch once the last in-flight toggle settles, so a /me refetch + // carrying partially-committed state can't clobber a still-pending + // optimistic update from another toggle. + if (queryClient.isMutating({ mutationKey: ME_QUERY_KEY }) === 1) { + queryClient.invalidateQueries({ queryKey: ME_QUERY_KEY }); + } + }, + }); +} + +/** + * The single seam for HyperDX Labs state. Local-mode branching, loading + * semantics, and the write all live here — consumers gating a feature should + * use {@link useIsLabEnabled} instead. + */ +export function useLabs(): LabsState { + const { data: me, isPending } = api.useMe(); + const { mutate: updateLabs, isPending: isSaving } = useUpdateUserLabs(); + + const stored = me?.labs; + + const enabled = React.useMemo(() => { + // Derived from the registry, not from what's stored: an id the registry no + // longer knows about can never reach a consumer or the modal. + const result: Record = {}; + for (const lab of LABS) { + // `=== true` rather than truthiness. A key like `constructor` passes the + // lab-id regex and is inherited from Object.prototype, where it is + // truthy; only an own `true` should read as enabled. + result[lab.id] = stored?.[lab.id] === true; + } + return result; + }, [stored]); + + const setLabEnabled = React.useCallback( + (labId: string, value: boolean) => { + // Full-replace payload rebuilt from the registry. Two things fall out of + // this for free: ids of retired labs are pruned, and only enabled entries + // are stored, which keeps the document small. + const next: UserLabs = {}; + for (const lab of LABS) { + const isOn = lab.id === labId ? value : stored?.[lab.id] === true; + if (isOn) { + next[lab.id] = true; + } + } + updateLabs(next); + }, + [stored, updateLabs], + ); + + return { + enabled, + // In local mode useMe() resolves to null immediately, so there is no + // pending state to advertise — labs are unavailable, not loading. + isLoading: IS_LABS_ENABLED && isPending, + isSaving, + setLabEnabled, + }; +} + +/** + * Whether the current user has opted into a lab. Returns `false` while `/me` is + * still loading and always `false` in local mode; if that transition is + * user-visible, use {@link useLabs} and branch on `isLoading`. + * + * An id with no registry entry always reads `false`, which is what makes + * deleting an entry safe — but it also means a typo fails silently, so grep for + * the id after you delete one. See agent_docs/labs.md. + */ +export function useIsLabEnabled(labId: string): boolean { + const { enabled } = useLabs(); + return enabled[labId] === true; +} diff --git a/packages/app/tests/e2e/components/LabsModalComponent.ts b/packages/app/tests/e2e/components/LabsModalComponent.ts new file mode 100644 index 0000000000..f11045fa69 --- /dev/null +++ b/packages/app/tests/e2e/components/LabsModalComponent.ts @@ -0,0 +1,90 @@ +/** + * LabsModalComponent - the "HyperDX Labs" modal opened from the nav user menu. + * + * Toggling a lab is deliberately not covered here: with an empty registry there + * is nothing to toggle. When the first lab lands, add a `setLab(id, enabled)` + * helper alongside `labSwitch` and assert the choice survives a reload — that is + * the test worth having. See agent_docs/labs.md. + */ +import { expect, Locator, Page } from '@playwright/test'; + +export class LabsModalComponent { + readonly page: Page; + private readonly userMenuTrigger: Locator; + private readonly menuItem: Locator; + private readonly modal: Locator; + private readonly content: Locator; + private readonly emptyState: Locator; + + constructor(page: Page) { + this.page = page; + this.userMenuTrigger = page.locator('[data-testid="user-menu-trigger"]'); + this.menuItem = page.locator('[data-testid="hyperdx-labs-menu-item"]'); + this.modal = page.locator('[data-testid="labs-modal"]'); + this.content = page.locator('[data-testid="labs-modal-content"]'); + this.emptyState = page.locator('[data-testid="labs-empty-state"]'); + } + + /** + * The modal root. Always present in the DOM with zero dimensions, so assert + * `toBeHidden()` on this for closed state and use {@link content} for open. + */ + get container() { + return this.modal; + } + + /** The modal body — this is what actually reads as visible when open. */ + get body() { + return this.content; + } + + /** The user-menu entry that opens this modal. */ + get trigger() { + return this.menuItem; + } + + /** Opens the nav user menu and waits for the Labs entry to be clickable. */ + async openUserMenu() { + await this.userMenuTrigger.scrollIntoViewIfNeeded(); + await this.userMenuTrigger.waitFor({ state: 'attached' }); + await this.userMenuTrigger.click({ timeout: 10000 }); + await expect(this.menuItem).toBeVisible(); + } + + /** Opens the modal from the nav user menu. */ + async open() { + await this.openUserMenu(); + await this.menuItem.click(); + await expect(this.content).toBeVisible(); + } + + /** + * A lab's checkbox input — use for state assertions (`toBeChecked()`). + * Mantine hides it behind an aria-hidden track, so it is not clickable; + * use {@link setLab} to change it. + */ + labSwitch(labId: string) { + return this.page.locator(`[data-testid="lab-switch-${labId}"]`); + } + + /** Toggles a lab and waits for the write to be persisted. */ + async setLab(labId: string, enabled: boolean) { + const input = this.labSwitch(labId); + if ((await input.isChecked()) === enabled) { + return; + } + + const persisted = this.page.waitForResponse( + r => r.url().includes('/me/labs') && r.request().method() === 'PATCH', + ); + // The label, not the input: the input is visually hidden. + await this.page.locator(`[data-testid="lab-toggle-${labId}"]`).click(); + const response = await persisted; + expect(response.status()).toBe(200); + await expect(input).toBeChecked({ checked: enabled }); + } + + async expectEmptyState() { + await expect(this.emptyState).toBeVisible(); + } +} diff --git a/packages/app/tests/e2e/core/navigation.spec.ts b/packages/app/tests/e2e/core/navigation.spec.ts index 08738a7da4..358add4dce 100644 --- a/packages/app/tests/e2e/core/navigation.spec.ts +++ b/packages/app/tests/e2e/core/navigation.spec.ts @@ -1,3 +1,4 @@ +import { LabsModalComponent } from '../components/LabsModalComponent'; import { expect, test } from '../utils/base-test'; test.describe('Navigation', { tag: ['@core'] }, () => { @@ -109,6 +110,26 @@ test.describe('Navigation', { tag: ['@core'] }, () => { //todo: Add tests that verify user pref behavior }); + test('should open HyperDX Labs from the user menu', async ({ page }) => { + const labsModal = new LabsModalComponent(page); + + await test.step('Wait for page to load', async () => { + await expect( + page.locator('[data-testid="nav-link-search"]'), + ).toBeVisible(); + }); + + await test.step('Open the Labs modal from the user menu', async () => { + await labsModal.open(); + }); + + await test.step('Verify the empty registry renders its empty state', async () => { + // No labs ship with the mechanism, so this is the expected content. + // Replace with a toggle assertion when the first lab lands. + await labsModal.expectEmptyState(); + }); + }); + test('should open help menu', async ({ page }) => { await test.step('Navigate to and click help menu trigger', async () => { // Wait for page to be fully loaded first diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 96a9883bff..21dee2ed36 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -2403,6 +2403,77 @@ export type InstallationApiResponse = z.infer< typeof InstallationApiResponseSchema >; +// Labs (per-user opt-in experiments) +// +// The lab *registry* — the ids and their UI copy — deliberately lives in +// packages/app/src/labs/registry.ts, not here. Adding a lab has to be a +// one-file change or the mechanism won't get used, and the server gains +// nothing from knowing the id list: the value space is `boolean` and the blast +// radius is the requester's own document. So what follows is a *shape* +// contract, not an allow-list. It bounds what a user can store without +// enumerating ids. +// +// The trade-off, stated plainly: the server cannot tell a typo'd lab id from a +// real one. That's caught client-side instead, by the registry unit test that +// parses every id against LabIdSchema. See agent_docs/labs.md. +export const LAB_ID_MAX_LENGTH = 64; +export const LABS_MAX_KEYS = 64; + +/** + * Strict kebab-case. This is a storage-safety rule, not cosmetics: it excludes + * `$` and `.` (Mongo operator and dotted-path characters) and `_` (so + * `__proto__` and friends are unrepresentable as keys). + * + * Split into a plain character class plus a dash-placement check rather than the + * idiomatic `^[a-z0-9]+(?:-[a-z0-9]+)*$`. That one-liner is in fact linear — + * the `-` separator is disjoint from `[a-z0-9]`, so there is no ambiguity to + * backtrack over — but it has the `(x+)*` shape that ReDoS linters flag, and on + * a validator guarding Mongo keys it is better to not have the argument than to + * suppress it. + */ +export const LabIdSchema = z + .string() + .min(1) + .max(LAB_ID_MAX_LENGTH) + .regex( + /^[a-z0-9-]+$/, + 'Lab ids may only contain lowercase letters, numbers and dashes', + ) + .refine( + id => !id.startsWith('-') && !id.endsWith('-') && !id.includes('--'), + { message: 'Lab ids must be kebab-case' }, + ); + +/** + * An enabled-set: a key present with `true` is on, an absent key is off. + * `false` is accepted on the write path so an older client is never rejected, + * but the current client only ever persists `true` entries. + */ +export const UserLabsSchema = z + .record(LabIdSchema, z.boolean()) + .refine(labs => Object.keys(labs).length <= LABS_MAX_KEYS, { + message: `A user may store at most ${LABS_MAX_KEYS} lab entries`, + }); + +export type UserLabs = z.infer; + +/** + * Full replace, not a patch: the client always holds the whole registry, so it + * can always compute the complete desired set — and that is what makes ids + * from retired labs self-pruning. See agent_docs/labs.md. + */ +export const UpdateUserLabsRequestSchema = z.object({ labs: UserLabsSchema }); + +export type UpdateUserLabsRequest = z.infer; + +export const UpdateUserLabsApiResponseSchema = z.object({ + labs: UserLabsSchema, +}); + +export type UpdateUserLabsApiResponse = z.infer< + typeof UpdateUserLabsApiResponseSchema +>; + // Me export const MeApiResponseSchema = z.object({ accessKey: z.string(), @@ -2418,6 +2489,13 @@ export const MeApiResponseSchema = z.object({ }).merge(TeamClickHouseSettingsSchema), usageStatsEnabled: z.boolean(), aiAssistantEnabled: z.boolean(), + /** + * Enabled-set of lab opt-ins. Optional because a rolling deploy can serve + * this from an older API pod that doesn't know about labs yet — the client + * already reads it as `me?.labs?.[id] === true`, so this is honest typing + * rather than a lie the hook would have to defend against. + */ + labs: UserLabsSchema.optional(), }); export type MeApiResponse = z.infer; From 71f96ede94994b9aa511273c18702e2fa76a3922 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 12 Aug 2026 10:05:53 -0400 Subject: [PATCH 2/3] feat(app): make Labs available everywhere instead of gating it off Drops the IS_LABS_ENABLED deployment gate so there is no flag to flip and no environment where the menu entry is missing. Local mode is the reason a gate existed: it has no API server and no user identity, so there is no account to hang an opt-in on and toggling would have failed. Opt-ins there go to localStorage under `hdx-labs` instead. The branch stays inside useLabs.ts, so nothing else knows which store is in play, and labs remain server-persisted wherever there is an account. One consequence for lab authors, now documented: a lab whose feature needs the API server must AND with !IS_LOCAL_MODE, because a local-mode user can now switch it on. --- .changeset/hyperdx-labs.md | 5 +- agent_docs/labs.md | 40 ++++--- .../components/AppNav/AppNav.components.tsx | 18 ++-- packages/app/src/config.ts | 9 -- .../labs/__tests__/useLabs.localMode.test.tsx | 102 +++++++++++++++--- .../app/src/labs/__tests__/useLabs.test.tsx | 4 + packages/app/src/labs/useLabs.ts | 45 +++++--- 7 files changed, 157 insertions(+), 66 deletions(-) diff --git a/.changeset/hyperdx-labs.md b/.changeset/hyperdx-labs.md index cf18077799..b2d59a4a88 100644 --- a/.changeset/hyperdx-labs.md +++ b/.changeset/hyperdx-labs.md @@ -8,5 +8,6 @@ Add HyperDX Labs, a per-user opt-in for features that are still being built. Open it from the user menu in the nav to see what's available and switch individual experiments on or off. Everything is off by default, and your choices are saved to your account rather than the browser, so they follow you across -devices. Adds `PATCH /me/labs` and surfaces the opt-ins on `GET /me`. No -experiments ship in this release — this is the mechanism they'll use. +devices (in local mode, where there is no account, they are kept in the browser). +Adds `PATCH /me/labs` and surfaces the opt-ins on `GET /me`. No experiments ship +in this release — this is the mechanism they'll use. diff --git a/agent_docs/labs.md b/agent_docs/labs.md index 80ee0024bc..307d33ece0 100644 --- a/agent_docs/labs.md +++ b/agent_docs/labs.md @@ -62,32 +62,38 @@ not just what's new: "span links aren't drawn yet" beats "improved trace view". ## Gating rules -`useIsLabEnabled(id)` returns `false` when the user hasn't opted in, while `/me` -is still loading, and always in local mode. Two consequences worth knowing: +`useIsLabEnabled(id)` returns `false` when the user hasn't opted in and while the +stored set is still loading. Two consequences worth knowing: -**The loading window is real.** `/me` isn't resolved on first paint — -`AuthLoadingBlocker` is only mounted on the landing page, so it does not gate the -app. A lab therefore reads OFF and then flips ON a moment later. For the common -shape (an extra tab, an extra button, an alternate renderer) that's fine. If the -flip is user-visible — a redirect, a default tab, a one-shot effect, a -mount-time fetch — use `useLabs()` and branch on `isLoading` first: +**The loading window is real** (outside local mode). `/me` isn't resolved on +first paint — `AuthLoadingBlocker` is only mounted on the landing page, so it +does not gate the app. A lab therefore reads OFF and then flips ON a moment +later. For the common shape (an extra tab, an extra button, an alternate +renderer) that's fine. If the flip is user-visible — a redirect, a default tab, a +one-shot effect, a mount-time fetch — use `useLabs()` and branch on `isLoading` +first: ```ts const { enabled, isLoading } = useLabs(); if (isLoading) return ; ``` -**Local mode has no labs.** It has no API server and no user identity at all, so -there is nothing to persist an opt-in against; `IS_LABS_ENABLED` is false there -and the menu entry is hidden. If your feature *also* structurally cannot work -without a backend, say so at the call site the way `IS_IAC_EXPORT_ENABLED` does: +**Labs are available everywhere, including local mode**, so there is no +deployment flag to flip and no environment where the menu entry is missing. Local +mode has no API server and no user identity, so opt-ins there live in +localStorage under `hdx-labs` instead of on the user document. That branch is +confined to `useLabs.ts`; nothing else needs to know which store is in play. + +What local mode does *not* change: whether your feature can actually run. If it +needs a backend, gate on that too, the way `IS_IAC_EXPORT_ENABLED` does: ```ts const isEnabled = useIsLabEnabled('remote-mtviews') && !IS_LOCAL_MODE; ``` -That's redundant at runtime, but it documents that the feature is impossible -there rather than merely un-opted-into. Drop it when that isn't true. +Now that a lab can be switched on in local mode, this is load-bearing rather +than decorative — without it, a local-mode user can opt into something that +cannot work. Add it whenever the feature depends on the API server. ## Graduating and retiring @@ -120,11 +126,14 @@ single-file-ness is the actual anti-rot mechanism. | Registry (ids + UI copy) | `packages/app/src/labs/registry.ts` | | Hook — the only seam | `packages/app/src/labs/useLabs.ts` | | Modal (nav user menu) | `packages/app/src/labs/LabsModal.tsx` | -| Deployment gate | `IS_LABS_ENABLED` in `packages/app/src/config.ts` | | Storage | `labs` on the `User` document, `packages/api/src/models/user.ts` | | Read / write | `GET /me` and `PATCH /me/labs`, `packages/api/src/routers/api/me.ts` | +| Local-mode storage | `hdx-labs` in localStorage, via the atom in `useLabs.ts` | | Shape contract | `UserLabsSchema` in `packages/common-utils/src/types.ts` | +There is no deployment flag. Labs is always present; individual labs are what +get toggled. + State is an **enabled-set**: a key present with `true` is on, an absent key is off. Writes are **full replace** — the client always holds the whole registry, so it can always compute the complete desired set, and that's what makes retired ids @@ -147,6 +156,7 @@ clicked it in (the mutation is optimistic), in other tabs on next focus, and on other devices on next load. Two tabs toggling *different* labs from the same snapshot can lose one — full replace is last-write-wins. The cost is "flip it again"; if that ever stops being acceptable, merge server-side in `setUserLabs`. +In local mode there is nothing to sync: the choice stays in that browser. ## Related diff --git a/packages/app/src/components/AppNav/AppNav.components.tsx b/packages/app/src/components/AppNav/AppNav.components.tsx index ed7fc64857..553b098f75 100644 --- a/packages/app/src/components/AppNav/AppNav.components.tsx +++ b/packages/app/src/components/AppNav/AppNav.components.tsx @@ -29,7 +29,7 @@ import { IconUserCog, } from '@tabler/icons-react'; -import { IS_LABS_ENABLED, IS_LOCAL_MODE } from '@/config'; +import { IS_LOCAL_MODE } from '@/config'; import { ChangelogModal } from './ChangelogModal'; import { KeyboardShortcutsModal } from './KeyboardShortcutsModal'; @@ -164,15 +164,13 @@ export const AppNavUserMenu = ({ > User Preferences - {IS_LABS_ENABLED && ( - } - onClick={onClickLabs} - > - HyperDX Labs - - )} + } + onClick={onClickLabs} + > + HyperDX Labs + {logoutUrl && ( <> diff --git a/packages/app/src/config.ts b/packages/app/src/config.ts index 358ee3e77e..e687a6e4f7 100644 --- a/packages/app/src/config.ts +++ b/packages/app/src/config.ts @@ -81,12 +81,3 @@ const IS_IAC_HELPERS_ENABLED = true; // Terraform provider to talk to. Single definition — the alerts, dashboard, // search, and team-settings surfaces all read this one constant. export const IS_IAC_EXPORT_ENABLED = IS_IAC_HELPERS_ENABLED && !IS_LOCAL_MODE; - -// HyperDX Labs: per-user opt-in toggles for in-development features, persisted -// on the User document. Local mode has no API server and no user identity at -// all (`useMe()` returns null there), so there is nothing to persist an opt-in -// against — the local-mode analogue of a lab is a constant in this very file, -// like IS_MTVIEWS_ENABLED above. Same shape as IS_IAC_EXPORT_ENABLED: one -// exported gate, so no caller can forget the local-mode check. Every branch on -// this lives in src/labs/useLabs.ts. See agent_docs/labs.md. -export const IS_LABS_ENABLED = !IS_LOCAL_MODE; diff --git a/packages/app/src/labs/__tests__/useLabs.localMode.test.tsx b/packages/app/src/labs/__tests__/useLabs.localMode.test.tsx index 931c5cdcf8..d7076520ce 100644 --- a/packages/app/src/labs/__tests__/useLabs.localMode.test.tsx +++ b/packages/app/src/labs/__tests__/useLabs.localMode.test.tsx @@ -1,12 +1,13 @@ import React from 'react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { renderHook } from '@testing-library/react'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import { hdxServer } from '@/api'; import { useLabs } from '@/labs/useLabs'; -// Separate file because jest.mock is file-scoped and this is the only case that -// needs labs switched off at the deployment level. -jest.mock('@/config', () => ({ IS_LABS_ENABLED: false })); +// Separate file because jest.mock is file-scoped and local mode is the only +// case that takes the localStorage path. +jest.mock('@/config', () => ({ IS_LOCAL_MODE: true })); // Mirrors real local mode, where useMe()'s queryFn short-circuits to null. jest.mock('@/api', () => { @@ -30,6 +31,13 @@ jest.mock('@/labs/registry', () => ({ addedAt: '2026-01-01', owner: '@test', }, + { + id: 'lab-b', + title: 'Lab B', + description: 'B', + addedAt: '2026-01-01', + owner: '@test', + }, ], })); @@ -37,23 +45,83 @@ jest.mock('@mantine/notifications', () => ({ notifications: { show: jest.fn() }, })); +const STORAGE_KEY = 'hdx-labs'; +const mockHdxServer = jest.mocked(hdxServer); + +function renderLabs() { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + return renderHook(() => useLabs(), { + wrapper: ({ children }) => ( + {children} + ), + }); +} + +beforeEach(() => { + jest.clearAllMocks(); + localStorage.clear(); +}); + describe('useLabs in local mode', () => { - it('never reports loading, and every lab is off', () => { - const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false } }, + it('never reports loading, since localStorage is synchronous', () => { + const { result } = renderLabs(); + + expect(result.current.isLoading).toBe(false); + expect(result.current.isSaving).toBe(false); + expect(result.current.enabled).toEqual({ 'lab-a': false, 'lab-b': false }); + }); + + it('reads opt-ins from localStorage', async () => { + localStorage.setItem(STORAGE_KEY, JSON.stringify({ 'lab-a': true })); + + const { result } = renderLabs(); + + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + expect(result.current.enabled['lab-b']).toBe(false); + }); + + it('persists a toggle to localStorage without calling the API', async () => { + const { result } = renderLabs(); + + act(() => result.current.setLabEnabled('lab-b', true)); + + await waitFor(() => expect(result.current.enabled['lab-b']).toBe(true)); + expect(JSON.parse(localStorage.getItem(STORAGE_KEY) ?? '{}')).toEqual({ + 'lab-b': true, }); + // There is no API server in local mode; a request here would 404. + expect(mockHdxServer).not.toHaveBeenCalled(); + }); + + it('prunes ids that are no longer in the registry on write', async () => { + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ 'lab-a': true, 'retired-lab': true }), + ); - const { result } = renderHook(() => useLabs(), { - wrapper: ({ children }) => ( - - {children} - - ), + const { result } = renderLabs(); + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + + act(() => result.current.setLabEnabled('lab-b', true)); + + await waitFor(() => expect(result.current.enabled['lab-b']).toBe(true)); + expect(JSON.parse(localStorage.getItem(STORAGE_KEY) ?? '{}')).toEqual({ + 'lab-a': true, + 'lab-b': true, }); + }); - // Labs are unavailable here, not pending — a loading state would never - // resolve, since there is no API server to answer. - expect(result.current.isLoading).toBe(false); - expect(result.current.enabled).toEqual({ 'lab-a': false }); + it('clears the entry when a lab is switched off', async () => { + localStorage.setItem(STORAGE_KEY, JSON.stringify({ 'lab-a': true })); + + const { result } = renderLabs(); + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + + act(() => result.current.setLabEnabled('lab-a', false)); + + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(false)); + expect(JSON.parse(localStorage.getItem(STORAGE_KEY) ?? '{}')).toEqual({}); }); }); diff --git a/packages/app/src/labs/__tests__/useLabs.test.tsx b/packages/app/src/labs/__tests__/useLabs.test.tsx index 8b86cff47b..3dcd9ae34e 100644 --- a/packages/app/src/labs/__tests__/useLabs.test.tsx +++ b/packages/app/src/labs/__tests__/useLabs.test.tsx @@ -38,6 +38,10 @@ jest.mock('@/api', () => { }; }); +// Explicit rather than relying on the env default: which store useLabs reads +// now hinges on this, and local mode is covered in useLabs.localMode.test.tsx. +jest.mock('@/config', () => ({ IS_LOCAL_MODE: false })); + jest.mock('@/labs/registry', () => ({ LABS: [ { diff --git a/packages/app/src/labs/useLabs.ts b/packages/app/src/labs/useLabs.ts index f10b5cc39e..f4ab08ed91 100644 --- a/packages/app/src/labs/useLabs.ts +++ b/packages/app/src/labs/useLabs.ts @@ -1,10 +1,12 @@ import * as React from 'react'; +import { useAtom } from 'jotai'; +import { atomWithStorage } from 'jotai/utils'; import type { MeApiResponse, UserLabs } from '@hyperdx/common-utils/dist/types'; import { notifications } from '@mantine/notifications'; import { useMutation, useQueryClient } from '@tanstack/react-query'; import api, { hdxServer, ME_QUERY_KEY } from '@/api'; -import { IS_LABS_ENABLED } from '@/config'; +import { IS_LOCAL_MODE } from '@/config'; import { LABS } from '@/labs/registry'; export type LabsState = { @@ -15,11 +17,11 @@ export type LabsState = { */ enabled: Record; /** - * True until the server's answer is known. Every lab reads OFF while this is + * True until the stored answer is known. Every lab reads OFF while this is * true, so anything where an OFF -> ON flip is user-visible (a redirect, a * default tab, a one-shot effect, a mount-time fetch) should branch on it - * rather than on `enabled` alone. Always false in local mode, where labs are - * unavailable rather than pending. + * rather than on `enabled` alone. Always false in local mode, which reads + * synchronously from localStorage. */ isLoading: boolean; /** True while a toggle is in flight. */ @@ -27,6 +29,17 @@ export type LabsState = { setLabEnabled: (labId: string, enabled: boolean) => void; }; +/** + * Local-mode store. Local mode has no API server and no user identity — + * `useMe()` resolves to null there — so there is no account to hang an opt-in + * on, and the choice lives in localStorage instead. Same jotai + localStorage + * shape as the other per-browser preferences in this app. + * + * This is the only reason `useLabs` branches on IS_LOCAL_MODE, and it is + * deliberately confined to this file. + */ +const localLabsAtom = atomWithStorage('hdx-labs', {}); + /** * Persists the caller's full lab set. Optimistic, because this backs a Switch: * with a plain mutate-then-refetch the control would sit dead for two round @@ -86,8 +99,10 @@ function useUpdateUserLabs() { export function useLabs(): LabsState { const { data: me, isPending } = api.useMe(); const { mutate: updateLabs, isPending: isSaving } = useUpdateUserLabs(); + const [localLabs, setLocalLabs] = useAtom(localLabsAtom); - const stored = me?.labs; + // The one branch: an account when there is one, this browser when there isn't. + const stored = IS_LOCAL_MODE ? localLabs : me?.labs; const enabled = React.useMemo(() => { // Derived from the registry, not from what's stored: an id the registry no @@ -114,25 +129,29 @@ export function useLabs(): LabsState { next[lab.id] = true; } } + + if (IS_LOCAL_MODE) { + setLocalLabs(next); + return; + } updateLabs(next); }, - [stored, updateLabs], + [stored, updateLabs, setLocalLabs], ); return { enabled, - // In local mode useMe() resolves to null immediately, so there is no - // pending state to advertise — labs are unavailable, not loading. - isLoading: IS_LABS_ENABLED && isPending, - isSaving, + // localStorage is read synchronously, so local mode is never pending. + isLoading: !IS_LOCAL_MODE && isPending, + isSaving: IS_LOCAL_MODE ? false : isSaving, setLabEnabled, }; } /** - * Whether the current user has opted into a lab. Returns `false` while `/me` is - * still loading and always `false` in local mode; if that transition is - * user-visible, use {@link useLabs} and branch on `isLoading`. + * Whether the current user has opted into a lab. Returns `false` while the + * stored set is still loading; if that transition is user-visible, use + * {@link useLabs} and branch on `isLoading`. * * An id with no registry entry always reads `false`, which is what makes * deleting an entry safe — but it also means a typo fails silently, so grep for From 93313df03790c73a918f1655f3369d34f285bf1b Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 12 Aug 2026 10:30:45 -0400 Subject: [PATCH 3/3] fix(app): serialize lab toggles so an older write cannot land last Two quick toggles each sent an independently computed full replacement, so if the first request's write landed after the second's, Mongo kept the older payload and the newer lab silently reverted on the next /me refetch. The mutation now carries a react-query scope, which sends same-scope mutations one at a time in call order. Each queued payload is built after the previous onMutate has written the cache, so serialized sends are cumulative rather than conflicting. Covered by a regression test that fails without the scope. Across two tabs or devices it is still last-write-wins, since those are separate clients with separate queues. Documented rather than fixed. Also drops the two eslint-disable comments the escape-hatch ratchet flagged: typing the hdxServer test double as the jest.Mock it actually is removes the need to assert a bare { json } stub into ky's generic ResponsePromise. --- agent_docs/labs.md | 13 ++-- .../app/src/labs/__tests__/useLabs.test.tsx | 67 ++++++++++++++++--- packages/app/src/labs/useLabs.ts | 7 ++ 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/agent_docs/labs.md b/agent_docs/labs.md index 307d33ece0..09a9d7897f 100644 --- a/agent_docs/labs.md +++ b/agent_docs/labs.md @@ -153,10 +153,15 @@ passes the id regex and is inherited from `Object.prototype`, where it's truthy. **Multi-tab / multi-device:** a toggle is visible immediately in the tab you clicked it in (the mutation is optimistic), in other tabs on next focus, and on -other devices on next load. Two tabs toggling *different* labs from the same -snapshot can lose one — full replace is last-write-wins. The cost is "flip it -again"; if that ever stops being acceptable, merge server-side in `setUserLabs`. -In local mode there is nothing to sync: the choice stays in that browser. +other devices on next load. In local mode there is nothing to sync: the choice +stays in that browser. + +Because a full replacement is only safe if the newest one lands last, the +mutation carries `scope: { id: 'user-labs' }`, which makes react-query send +toggles one at a time in call order. Flipping two switches quickly is therefore +safe within a tab. Across two tabs or two devices it is still last-write-wins — +they are separate clients with separate queues, so the loser has to flip again. +If that ever stops being acceptable, merge server-side in `setUserLabs`. ## Related diff --git a/packages/app/src/labs/__tests__/useLabs.test.tsx b/packages/app/src/labs/__tests__/useLabs.test.tsx index 3dcd9ae34e..bc62edd536 100644 --- a/packages/app/src/labs/__tests__/useLabs.test.tsx +++ b/packages/app/src/labs/__tests__/useLabs.test.tsx @@ -4,9 +4,20 @@ import { notifications } from '@mantine/notifications'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, renderHook, waitFor } from '@testing-library/react'; -import { hdxServer } from '@/api'; import { useIsLabEnabled, useLabs } from '@/labs/useLabs'; +/** + * The mocked `hdxServer`, typed as what it actually is rather than as ky's + * `ResponsePromise`. Reaching for the real signature would mean asserting a + * bare `{ json }` stub into a generic `json()` interface, and the honest + * narrow type here avoids that cast entirely. The hook only ever calls + * `.json()` on the result. + */ +type HdxServerMock = jest.Mock< + { json: () => Promise }, + [string, { method?: string; json?: unknown }?] +>; + // A never-resolving /me keeps the query pending, which is how the OFF -> ON // window is exercised. const PENDING = Symbol('pending'); @@ -65,14 +76,13 @@ jest.mock('@mantine/notifications', () => ({ notifications: { show: jest.fn() }, })); -const mockHdxServer = jest.mocked(hdxServer); +const { hdxServer: mockHdxServer } = jest.requireMock<{ + hdxServer: HdxServerMock; +}>('@/api'); const mockShow = jest.mocked(notifications.show); function resolvePatch(body: unknown = { labs: {} }) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - mockHdxServer.mockReturnValue({ - json: () => Promise.resolve(body), - } as unknown as ReturnType); + mockHdxServer.mockReturnValue({ json: () => Promise.resolve(body) }); } /** Returns a `resolve`/`reject` pair so a test can hold the PATCH in flight. */ @@ -84,10 +94,7 @@ function deferPatch() { reject: () => rej(new Error('nope')), }; }); - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - mockHdxServer.mockReturnValue({ - json: () => promise, - } as unknown as ReturnType); + mockHdxServer.mockReturnValue({ json: () => promise }); return settle!; } @@ -226,6 +233,46 @@ describe('useLabs', () => { }); }); + it('serializes overlapping toggles so an older payload cannot land last', async () => { + // Without a mutation scope both requests fly concurrently, and if the first + // one's response lands last the server keeps its payload — dropping the + // second lab, which the next /me refetch then reverts in the UI. + meFixture = { labs: {} }; + const first = deferPatch(); + + const { result } = renderLabs(); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + + act(() => result.current.setLabEnabled('lab-a', true)); + await waitFor(() => expect(result.current.enabled['lab-a']).toBe(true)); + + act(() => result.current.setLabEnabled('lab-b', true)); + await waitFor(() => expect(result.current.enabled['lab-b']).toBe(true)); + + // The second toggle is queued behind the first, not racing it. + expect(mockHdxServer).toHaveBeenCalledTimes(1); + expect(lastPatchBody()).toEqual({ labs: { 'lab-a': true } }); + + // Let the first finish; the queued one then sends the cumulative set. The + // fixture stands in for the persisted document once both writes have landed + // in order, so the refetch that onSettled triggers is what proves the second + // toggle survives rather than reverting. + meFixture = { labs: { 'lab-a': true, 'lab-b': true } }; + resolvePatch(); + await act(async () => { + first.resolve(); + }); + + await waitFor(() => expect(mockHdxServer).toHaveBeenCalledTimes(2)); + expect(lastPatchBody()).toEqual({ labs: { 'lab-a': true, 'lab-b': true } }); + await waitFor(() => + expect(result.current.enabled).toEqual({ + 'lab-a': true, + 'lab-b': true, + }), + ); + }); + it('rolls back and notifies when the request fails', async () => { meFixture = { labs: { 'lab-a': true } }; const settle = deferPatch(); diff --git a/packages/app/src/labs/useLabs.ts b/packages/app/src/labs/useLabs.ts index f4ab08ed91..69ac810106 100644 --- a/packages/app/src/labs/useLabs.ts +++ b/packages/app/src/labs/useLabs.ts @@ -52,6 +52,13 @@ function useUpdateUserLabs() { // Shared with the /me query key so concurrent toggles coordinate their // refetch rather than racing each other. mutationKey: ME_QUERY_KEY, + // Serializes toggles: react-query runs same-scope mutations one at a time, + // in call order. Without this, two quick toggles race on the network and an + // older full-replacement payload can land last, dropping the newer lab from + // Mongo — the UI would then revert it on the next /me refetch. Each queued + // payload is built after the previous onMutate has written the cache, so + // serialized sends are cumulative rather than conflicting. + scope: { id: 'user-labs' }, mutationFn: (labs: UserLabs) => hdxServer('me/labs', { method: 'PATCH',