diff --git a/.agents/skills/README.md b/.agents/skills/README.md index 5943f4b662402..3824d7cd92d01 100644 --- a/.agents/skills/README.md +++ b/.agents/skills/README.md @@ -46,3 +46,10 @@ choosing which framework to use, start with **mz-test**. | Skill | When to use | What it does | |---|---|---| | **mz-adapter-guide** | Working on or asking about the adapter layer | Correctness invariants and architectural notes for the coordinator, pgwire, peek paths, timestamp oracle, and related crates | + +## Console + +| Skill | When to use | What it does | +|---|---|---| +| **mz-console-review** | Reviewing a PR or diff under `console/` | Checklist of what Console reviewers actually flag (distilled from ~570 review comments), plus running the change to verify claims and posting findings as one GitHub review | +| **mz-query-perf** | A Console/catalog SQL query is slow | Diagnoses the plan on real `mz_catalog` / `mz_internal` relations, then measures candidate rewrites with a synthetic-fleet sweep | diff --git a/.agents/skills/mz-console-review/SKILL.md b/.agents/skills/mz-console-review/SKILL.md new file mode 100644 index 0000000000000..5cb8942a47848 --- /dev/null +++ b/.agents/skills/mz-console-review/SKILL.md @@ -0,0 +1,402 @@ +--- +name: mz-console-review +description: > + Review a Console (`console/`) PR or diff the way Console reviewers do: run the + change to verify claims, check it against the patterns and helpers the app + already has, and post findings as one GitHub review. Trigger: "review this + console PR", "review my console changes", "does this console change look ok", + a github.com/MaterializeInc/materialize/pull/ link whose files are under + `console/`, or a request to look over a React/TypeScript diff in this repo. + For Rust changes use mz-pr-review instead. +argument-hint: [pr-number] +allowed-tools: [Bash, Read, Grep, Glob] +--- + +# Console code review + +`mz-pr-review` covers the Rust checklists, none of which apply under `console/`. +The canonical Console rules live in `console/CLAUDE.md` (component conventions, +import restrictions, TypeScript and React patterns, testing philosophy) and in +`console/doc/` (`guide-testing.md`, `guide-tanstack-table.md`, +`mz-backwards-compatibility.md`). Read the sections the diff touches. Do not +restate them in comments, cite them. + +This skill is what Console reviewers actually flag. Section 4 is distilled from +roughly 570 reviewer comments on about 150 Console PRs from 2025 and 2026, and is +ordered by how often each kind of feedback came up. Sections 5 and 6 are not from +that tally. They cover cost and structure that reviewers rarely catch by +reading, measured on `mz_catalog_server` and in render-count probes. + +## 1. Get the diff, and make it runnable + +Remotes are usually `upstream` = `MaterializeInc/materialize` and `origin` = a +personal fork, so a PR branch will not resolve against `origin`: + +```bash +git fetch upstream pull//head:pr--review +``` + +To run anything you need the files inside `console/`, because `node_modules` +lives at `console/node_modules` and a fresh worktree does not have it. Check the +files into a tree with no local edits under ``, run, then clean up. The +restore removes files the PR added and reverts the ones it changed, and it also +discards any local edits under ``: + +```bash +git checkout pr--review -- console/src/ +(cd console && npx vitest run src/) +git restore --source=HEAD --staged --worktree -- console/src/ +git branch -D pr--review +``` + +Confirm `git status --short` is empty before you finish. + +## 2. Verify every behavioral claim before writing it + +Run the suite and state the result ("30/30 pass"). Probe anything you intend to +assert. A claim about draw order, nullability, or event handling that you reasoned +out but did not execute is a claim you will get wrong. + +Vitest hides `console.log` in passing tests. Assert against a sentinel to see a +value: `expect(actual).toEqual("MARKER")`. + +jsdom makes graph and pointer assertions silently vacuous: + +| Symptom | Cause | Fix in the probe | +|---|---|---| +| Geometric assertions pass trivially | `ParentSize` measures nothing, width is 0 | `vi.mock("@visx/responsive/lib/components/ParentSize", ...)` with a fixed size | +| Pointer handler commits `NaN` | no `createSVGPoint` / `getScreenCTM`, so visx `localPoint` fails | `vi.mock("@visx/event", () => ({ localPoint: vi.fn(() => ({ x, y })) }))` | + +Measure colors instead of judging them: `d3.lab(color).l` gives perceived +lightness. `d3` is already a dependency. + +For SQL claims, compile the builder and read the SQL. Kysely's `.select` appends, +it does not replace, so a "fallback" select can emit a duplicate column. + +## 3. Read the PR before the code + +Reviewers raise these before any line comment: + +* **Motivation is traceable.** A linked issue (a Linear key such as `Closes: CNS-192`, or `Fixes database-issues#N` for a legacy GitHub issue), a Slack thread, or a design doc. If a reviewer cannot tell why the change exists, ask for context before reviewing. +* **UI changes link the Figma design** in the description. +* **Stacked PRs are linked** in each description, or the work is one PR with logical commits. Review semantic commits one at a time when the author asks. +* **Commit messages are imperative and descriptive** per `doc/developer/guide-changes.md`, not "addressed feedback". +* **A removed compatibility shim states its floor.** If the PR deletes a version map or gate, the description names the minimum supported environment version. +* **WIP code is marked** with `TODO` (spelled `TODO`, not `TO-DO`), including leftover `console.log`. +* **New pages and routes sit behind a LaunchDarkly flag.** The point is developer experience: it lets the work merge incrementally. Self-managed flags default to `true` in `src/config/flexibleDeploymentFlags.ts`, so a flag meant to stay off there must be set to `false` explicitly. + +## 4. What reviewers flag, most frequent first + +### 4.1 Remove and simplify + +The single most common request. Look for: + +* Unused variables, atoms, exports, props, and query keys. Variables that are always `true`. +* Redundant fallbacks (`?? 0` behind a value that cannot be null) and conditionals that render nothing. +* Wrapper hooks that re-expose library options as custom `enable*` props. Keep wrappers thin, since the library's own options are better documented. +* Features with no requirement behind them (column pinning, LRU layers). Ask "is there a requirement for this?" +* A mini rules engine where two `if` statements would do. +* Extra state flags (`destroyed`, `initialized`) that every method must check, and piles of refs. Stale refs are a bug source. +* A copied error state when the mutation hook already exposes `error`. +* `!!x` inside an expression that is already boolean, and several early returns that can merge into one. + +### 4.2 SQL and catalog correctness + +* **Nullable columns.** `disk_bytes` and friends are nullable. Coalesce, and fix the cast. When dividing, check the *denominator* for zero, not each operand. +* **`ORDER BY ... DESC` puts nulls first.** Add `nulls last` where the column is nullable. +* **User objects are `id LIKE 'u%'`**, via `isSystemId` in `~/api/materialize`. Not `name NOT LIKE 'mz_%'`, and not a bare `startsWith("u")`. +* **Statistics tables keep rows for dropped replicas** for up to a day. Aggregates like `bool_and(snapshot_committed)` must filter to extant replicas. +* **Sources:** filter to top-level sources the way `sourceList.ts` does. Subsources and progress collections re-enter through joins on `mz_hydration_statuses`. Newer sources expose subsources as tables. +* **Latest vs max.** Is the query returning the most recent sample or the largest one, and is that what the UI claims? +* **A windowed flag must not drive a point-in-time answer.** "Unreadable at some point in the window" is not "unreadable now". Check which columns and predicates a window-wide flag reaches. +* **Multi-process skew.** Summing usage over processes hides a single process about to OOM. Prefer `MAX`, and prefer existing catalog views over recomputing their math client-side. +* **Two surfaces, two answers.** When a PR touches one of several queries for the same metric (list vs detail page), check the staleness bounds and column semantics match. A column can mean different things in the old and new relation. +* Factor a repeated join or subquery into `src/api/materialize/expressionBuilders.ts` and left-join it, rather than `GROUP BY` just to pull one column. +* Aggregate with `jsonb_agg` / `list_agg` and format on the client, instead of `STRING_AGG` or `pg_size_pretty` in SQL. +* Raw SQL goes through the `sql` tagged template. Qualified names are structured (`{ databaseName, schemaName, name }` quoted with `sql.id()` or `quoteIdentifier`), never a dotted string, which breaks on names containing periods. +* Comment catalog behavior the docs do not cover (for example `object_sub_id IS NULL` meaning the relation itself), and rewrite clever predicates for intent (`BOOL_AND(x IS NOT NULL)` over count arithmetic). + +### 4.3 Reuse before re-rolling + +Before accepting any new helper, grep for what it replaces. Reviewers name these most: + +| Re-rolled | Already exists | +|---|---| +| clamp, pluralize, capitalize, null-guard | `clamp`, `pluralize`, `capitalizeSentence`, `notNullOrUndefined` in `~/util` | +| Percent and byte text | `formatPercentage(fraction, decimals)` (takes a fraction. Call it directly and handle null inline, since a local `formatPercent` wrapper reads as a re-roll), `formatBytesShort`, `humanizeBytes`, `formatMemoryUsage` in `~/utils/format` | +| Sorting | d3 `sort(iterable, ...accessors)` (multi-key ties, returns a copy), `handleSortOrder` in `~/utils/sort`, `sortingFunctions` (`nullsLast`, `numericNullsLast`) in `components/Table/tableColumnBuilders.ts`. The same comparator written twice becomes one shared helper | +| `60_000`-style time constants | `millisecondsInMinute`, `millisecondsInHour` from `date-fns/constants` (not the `date-fns` root in v4) | +| Object type text | `kebabToTitleCase`, `kebabToSentenceCase`, `kebabToScreamingSpaceCase` (SQL keywords such as `MATERIALIZED VIEW`) in `~/util`, `formatObjectType` in `platform/clusters/format.ts` | +| Route strings built by hand | path helpers in `~/platform/routeHelpers` (`regionPath`, `useBuildWorkflowGraphPath`, ...). Add one there when it is missing | +| Logic copied from a neighboring page | extract one helper and switch both call sites to it, so the copies cannot drift | +| System-object check | `isSystemId` in `~/api/materialize` | +| Duration formatting, lag caps | `formatInterval`, `formatDurationForAxis` in `~/utils/format` (fractional units are reserved for seconds). `formatDurationForAxis` is for axes: it rounds to whole minutes and caps at 24h, so 90s reads "2m" in text. The 24h cap `MAX_LAG_MS` is module-private there and `FreshnessGraph.tsx` declares its own copy, so export one instead of adding a third | +| An anchor with custom hover/underline | `TextLink` from `~/components/TextLink` | +| Status colors copied from a pill | export `colorSchemeToStyles` from `StatusPill.tsx` | +| Layout props threaded one by one | the `containerProps` pattern, see `Gauge.tsx` | +| A new cluster-sizes query | `useAvailableClusterSizes` / `fetchAvailableClusterSizes` | +| A new "all objects" fetch | `useAllObjects` and the global subscribes (`useGlobalUpsertSubscribe`) | +| A new filter dropdown | mirror `SchemaFilter.tsx` / `DatabaseFilter.tsx` (reserved "all" id, caller sets width) | +| A new DAG layout | `dagreGraphHelpers.ts` and the workflow graph components | +| A `useEffect` + timeout debounce | `lodash.debounce` (see `ObjectExplorer.tsx`) | +| Previous-value or latest-value refs, or a callback read inside a debounce or timer | `usePrevious`, `useLatestRef` from `@chakra-ui/react` (see `useAutomaticallyConnectSocket.ts`) | +| Tab active state from `location.pathname` | `NavLink`'s `isActive`, see `PageTabStrip` | +| Set diffing by hand | `Set.prototype.difference` | +| Secret masking | export `formatAppPassword` from `~/queries/frontegg` | +| A per-tenant localStorage key | export `buildCacheKey` from `platform/shell/commandCache.ts` | +| Chart axes, grid, tooltip, legend, cursor | `buildXYGraphLayoutProps` and friends in `~/utils/graph`, `~/components/graphComponents`, `useChartLegend`, `useCursorState`, `colors.lineGraph` | +| visx plumbing | `Line` from `@visx/shape` for straight lines (as `GraphLineCursor` does), `Group` from `@visx/group` instead of repeating offset arithmetic on each child, `ParentSizeProvidedProps` to type a `ParentSize` child's props, `useDrag` from `~/hooks/useDrag` for draggable handles | +| Several small components for one feature | one `xComponents.tsx` file, like `formComponents.tsx` | + +When a new component duplicates an existing one, say which pages it lands on and +what those pages have today. + +### 4.4 Tests + +Value over volume. Look for: + +* **Tests that assert on query-builder string output, SQL snapshots, CSS attributes, or class names.** Replace with SQL tests (`*.test.sql.ts`) that create real objects and assert results, or with user-visible assertions. +* **Missing corner cases.** Ask for the harder fixture: a deeper role hierarchy, overlapping buckets, multiple inherited roles, users that can log in. For graph fixtures, an ASCII diagram in a comment helps. +* **Assert the outcome, not the call.** After a `GRANT`, query `mz_internal.mz_show_all_privileges`. +* **Fixtures that do not look like real data.** Mock only the relation under test, and make values realistic. +* **`vi.mock` / `vi.spyOn` on app modules.** Reviewers dislike them (unstable, break on circular imports). Prefer MSW via `buildSqlQueryHandlerV2`, with per-call query keys when one mutation issues several statements, or dependency injection. +* **Redundant happy-path slices.** One happy-path test plus separate corner cases beats ten tests that all break together. Multiple assertions per integration test are fine. +* Unnecessary setup (`GRANT CREATEROLE` when the test already logs in as a superuser). +* Regex over `toContain` for generated code, `!` over `?.` inside tests so they fail early, no `if` branches around expects, constants hoisted out of `describe` in `SCREAMING_SNAKE_CASE`. +* Every bug found in review gets a regression test. New helpers and classes get unit tests. +* A test that asserts a function's arity or a mock's call shape breaks when someone adds an optional parameter. Assert what the user sees instead. + +### 4.5 Types and nullability + +* No `any`, no casts that the types do not need (`as unknown as T`). +* Ask why each field is nullable when every caller defines it. +* Fields that are always null together become one nested object (`cluster: { id, name } | null`). +* Two booleans that can conflict (`usersOnly`, `rolesOnly`) become a discriminated union. +* Boolean props are optional (`canIssue?: boolean`) so `` works. An error prop is `error?: boolean`, not `unknown`. +* Type a required key as required instead of throwing when it is missing. +* `===` over `==`. Prefer `undefined` over `null` for optional values. + +### 4.6 Naming + +* Units are suffixes: `durationMs`, not `ms`. +* Booleans are present tense (`hasActiveSubscription`). Components are nouns, not verbs (`LicenseKeyCtaContent`, not `GetLicenseKey`). +* `Inner` suffix only for a suspense boundary split, `Content` for a block of content. +* `build...Query` is reserved for SQL builders. Transforms are `transform...`, fetchers `fetch...`, DOM listener setup `register...`. +* Do not alias a name as an id (`parent.name as parentId`). SQL aliases are camelCase, named for the general concept (`relationComment`, not `tableComment`). +* Callback props keep parity with DOM events (`onPointMove`, `onClick`), not new verbs like `onPointSelect`. +* A variant that changes styling is a `variant` prop, not inferred from which callbacks were passed. +* Icons end in `Icon`. Version gates are named constants (`MCP_OAUTH_MIN_VERSION`), not inline strings. +* Loop variables say what they index (`bucketIndex`, not `j`). + +### 4.7 Code comments + +The rules are in the root `CLAUDE.md` "Code comments" section. What reviewers +flag in practice: comments with fluff, details that make no sense out of context +("avoids alphanumeric sorting issues"), and comments that no longer match the +code. They ask *for* comments on undocumented catalog behavior, on unconventional +uses (react-query used only to get loading/error state for an initializer), on +non-obvious algorithms, and on props whose meaning is subtle. It is not necessary +to describe the evolution of the code by referring to code that has been removed +("use quicksort because bubble sort is inefficient"). + +### 4.8 Loading, error, and empty states + +* Every `useQuery` renders distinct loading, error (`isError`), and empty UI. A mutation failure is visible to the user, not only `console.error`. +* No state falls through to an endless spinner (a disabled region is an error, not "loading"). +* Loading is a spinner inside the card, not "Loading..." text, which shifts layout on every refetch. Empty states render inside the card, not as a blank page. +* Prefer `useQuery` with explicit states over new `useSuspenseQuery`. +* **The global subscribes start empty.** `useAllClusters` and `useAllObjects` return `data: []` until `snapshotComplete`, so a page that reads empty as "none" shows "No clusters" or a false success on a cold load. Gate on `snapshotComplete`, and render the subscribe's error. +* An entity with nothing to report gets one empty state, not a green "0 of 0 within target" summary over an empty chart. + +### 4.9 Data layer + +* Query keys start with `buildRegionQueryKey(...)` (`~/api/buildQueryKeySchema`), so switching region or organization never serves another region's cache. +* Queries live under `src/api/materialize/...`, not in `platform/`. +* `staleTime` / `gcTime` are deliberate. If data never goes stale, use `Infinity`, and invalidate on logout or mutation. +* Refresh derived data by putting its query key in the namespace that mutations already invalidate, rather than polling (a 60s poll means 60s-stale ownership). +* No new `useSql*` hook calls. `console/CLAUDE.md` ("React Query") has the read and write pattern. +* Global subscribes are hoisted to the route that owns the section, or navigating remounts them and the view jitters. +* Validate user-supplied parameters at the `build...Query` layer (see the start-time check in `queryHistoryList.ts`), not only in the UI. +* Anything persisted to localStorage is scoped by organization and region, and cleared on region switch. An unscoped key leaks one tenant's catalog into another session. + +### 4.10 React hooks and state + +* No `useEffect` to sync props into state or to react to a user action. Do it in the event handler, or initialize state in the modal's `onOpen`. +* Effect dependencies hold the primitive that matters (the JWT string, not the whole query result), so work reruns only when it should. +* TanStack table `columns` are static. Dynamic data flows through `data`, or cells remount and tooltips vanish. +* One owner per lifecycle: a connection manager owns connect, disconnect, and retry, and the hook is a thin shell. Shell output changes go through the `websocketFsm.ts` state machine. +* For Jotai, a derived write-only atom with an early return, not returning the previous object and hoping it skips a render. +* A helper that mutates variables outside its scope is harder to read than the same code inline with a comment. + +### 4.11 Routing and URL state + +* View modes and modal flows (graph vs table, edit role) are subroutes, so a reload keeps the user's place. +* Filter and lookback state belongs in the URL, not localStorage, when it should survive reload and share. +* Do not branch on `location.pathname` when `` already branches. +* A URL-selected entity that does not exist (a dropped cluster in an old link) must not silently show a different one. Write the fallback into the URL, or say it was not found. +* Imports use `~/` unless the file shares a parent directory. + +### 4.12 Versions, flags, and deployment modes + +* A query that reads a new column or relation breaks older environments. Gate it with `useEnvironmentGate` and a named version constant, per `console/doc/mz-backwards-compatibility.md`. Self-managed can run a new Console against much older environments. +* Test across versions with emulator images on another port (`misc/docker/README.md` in the Console tree) or a local kind cluster. +* Cloud-only initialization goes in `CloudAppInitializer`, not `AppInitializer` (flexible deployments and impersonation). +* `forceShow` on a nav item breaks when the environment is disabled. +* Superuser checks through `usePrivileges` hit `has_table_privilege` and load environmentd. Use `useIsSuperUser`. +* An `AppConfigSwitch` whose branches render the same thing is noise. + +### 4.13 Visual and UX QA + +Reviewers run the branch and attach screenshots or recordings. If you can run it +(`console/CLAUDE.md` has the dev commands), do. Otherwise list these for the human +reviewer: + +* Overflow, especially in modals, filters, and form footers. +* Dark mode. Any literal color (`colors.green[50]`, hex, `hsl(`) is suspect, since it does not flip. +* Spacing on the 4px grid in relative units (`3`, not `"12px"`). Valid `textStyle` values come from `src/theme/typography.ts`. Valid Chakra tokens for `zIndex`. Chakra style props over `style`. +* **Types do not catch invalid style values.** Chakra passes `alignItems="left"` straight through (Chrome computes it as `normal`, which `CSS.supports("align-items", "left")` confirms), and an undefined `textStyle` such as `text-ui-sm` falls back to the default without an error. +* Consistency with existing patterns: the app's purple color scheme, existing button hover behavior, filter chips like query history, an Apply button for unit inputs. "We don't do this anywhere else" is a common and valid objection. +* Layout shift on refetch, jitter on navigation, flicker on load. +* Thresholds that agree with each other (a value turns orange at 5s but the filter starts at a minute). +* Copy: title case in titles, correct terminology ("role with LOGIN attribute", not "privilege"), where to save a generated config, a link to the docs page. +* Semantic elements (`ul`/`li` for lists). +* **Mouse-only click targets.** A clickable `Tr`, `HStack` or legend pill with `onClick` cannot be reached from the keyboard. Make it a button (`as="button"`, `type="button"`) with `aria-pressed` for toggles (see `connectComponents.tsx`), and prefer `:focus-visible` over an always-on focus ring. +* One placeholder for a missing value across a page (`-` or `—`, not both). + +### 4.14 Security and session traps + +Rare but high severity, and most often caught by automated QA review after merge: + +* Tenant or region state that survives a switch: caches, module-level singletons, persisted rows seeded with `snapshotComplete: true`. +* Auth flows where the client clock or cached token decides a redirect. Redirects should be server-authoritative, or users loop. +* Two concurrent sessions (password and OIDC) where queries silently run as the wrong account. +* State-changing `GET`s, and protections that rely on another system's cookie config (`SameSite`) instead of checking `Origin` / `Sec-Fetch-Site`. +* `btoa` on non-ASCII input. Service accounts where the code assumes a user email. + +### 4.15 Charts and graphs + +Graph PRs collect the same gotchas. Check each: + +* **Reuse first.** The 4.3 rows for graph helpers and visx plumbing. A block near identical to `FreshnessGraph`, `UtilizationGraph` or `ThresholdLineGraph` should come from the same pieces. +* **Hover redraws only the cursor and tooltip.** Mark layers (lines, bars, rows) are `React.memo` with stable props: scales and layouts memoized on width and domain, series memoized. A memo only helps if callers hoist their arrays. One built inline reruns every render. +* **Volatile inputs get their own layer.** Anything derived from `useAllObjects` or `useAllClusters` gets a new identity on any change anywhere in the environment. Draw it as its own memoized layer, or key its memo by content, so an unrelated DDL does not redraw the chart. +* **Which point is hovered.** `findNearestDatum` returns the nearest bucket start, which near a bar's right edge is the next bar. For bucketed bars, find the bucket that contains the time. +* **Decorative layers keep `pointerEvents="none"`** and sit under the event overlay, or a full-width strip swallows hover. +* **Labels state only what the data says.** A max-per-bucket series is a peak, not an average. Name the unit precisely ("% of replica CPU allocation"), and do not label an inferred state as recorded. +* **Wide windows stay readable.** Bars narrower than a few pixels alias into stripes. Merge buckets client-side, keeping each metric's peak. +* **Colors.** Series take `colors.lineGraph`. A generated palette varies perceived lightness (`d3.hcl` / `d3.lab`), not HSL lightness. +* **Axis formatters are for axes.** `formatDurationForAxis` and tick formatters round. Headline and table values need the unrounded formatter. +* **Refresh cost.** A chart that polls re-reads its whole window on every refetch. See 5.1 in `cost-and-structure.md`. + +### 4.16 Design docs + +For `doc/design/` PRs, reviewers ask for concrete ranges and limits, error states +(an unresponsive cluster), reuse of existing global subscribes and components, +consistency when two polled queries resolve at different times (a subscribe +avoids it), client-side formatting, and a scoped first version (superuser only) +before the general case. + +## 5. Check what the change costs + +When the diff changes a query builder, a subscribe, a `refetchInterval` or +`staleTime`, or anything that runs at pointer or keystroke rate, read section 5 +of `cost-and-structure.md` in this skill's directory and work through it. The +`mz_catalog_server` cost and the render cost it checks do not show up in a read +of the diff. + +## 6. Audit hooks and component structure + +When the diff adds or moves a `useEffect`, `useMemo`, `useCallback`, or +`React.memo`, or splits a component, read section 6 of `cost-and-structure.md`. + +## 7. Write the comment the way a Console reviewer writes it + +Findings are triaged with the person you are reviewing for first. Present the +list, let them cut and rephrase, then post. Do not post a review off your own draft. + +Comments are short, hedged, and collaborative. State the claim, give at most one +clause of why, name the alternative, stop. Typical openers: `Nit:`, `I think we +should`, `Maybe we can`, `Could we reuse`, `Let's just`, `I'd recommend`, +`I wonder if`, `Kinda feels like`, `Why is X nullable here?`, `How does this look +in dark mode?`. + +Real comments at the right length: + +> nit: We usually like to use `<>` for react fragments + +> Can most likely reuse the TextLink component + +> Why is objectId nullable here? Usage of this hook shows me that it's always defined + +> Kinda feels like these should be grouped into a `cluster` object, especially if both of them can be null + +> Let's remove this comment. + +Keep measurements and probe output out of the comment. They belong in your reply +to the person you are reviewing for. A bug claim is the exception: attach the +repro (a screenshot, a recording, or a short diff that triggers it), because that +is what makes it actionable. + +Point at the existing pattern with a permalink rather than describing it. Offer +pseudocode or a suggestion block when the fix is not obvious. Say when something +is not blocking: "Doesn't need to be blocking but I suggest in a followup PR," +"We can do this as a followup." + +## 8. Post as a single review + +One review, not N comments, so the author gets one notification: + +```bash +gh api repos/MaterializeInc/materialize/pulls//reviews \ + --method POST --input review.json +``` + +`review.json` is `{commit_id, body, event: "COMMENT", comments: [...]}`. Use +`event: "COMMENT"` unless the person asked to approve or request changes. Each +inline comment is `{path, line, side: "RIGHT", body}`, and a multi-line anchor adds +`start_line` and `start_side`. General findings go in `body`. + +Permalink every code reference and pin the SHA: + +* Code already on main gets the PR's **base** SHA (`gh pr view --json baseRefOid`). +* Code inside the PR gets the **head** SHA. +* Verify each cited line resolves before posting: `git show ":" | sed -n 'p'`. When the SHA is in a shell variable, brace it (`"${sha}:${path}"`). zsh reads `$sha:c` as a modifier and drops the colon, even inside quotes. + +Afterwards, confirm what landed: + +```bash +gh api "repos/MaterializeInc/materialize/pulls//comments?per_page=100" \ + --jq '[.[] | select(.pull_request_review_id == )] | length' +``` + +Before posting, check for a pending review. GitHub allows one pending review per +person per PR, so a stray one blocks a new review, and one pinned to an older +commit cannot take new comments because their lines resolve against that commit: + +```bash +gh api "repos/MaterializeInc/materialize/pulls//reviews?per_page=100" \ + --jq '[.[] | select(.state == "PENDING")]' +``` + +* **Inline comments must fall inside a diff hunk.** Read the ranges with `gh api "repos/MaterializeInc/materialize/pulls//files" --jq '.[].patch' | grep '^@@'`. Anything outside goes on the file (`subjectType: FILE`) or in the body. +* **Replies and new threads in one review** need GraphQL: `addPullRequestReview` with `commitOID`, then `addPullRequestReviewThread` and `addPullRequestReviewThreadReply` with that `pullRequestReviewId`, then `submitPullRequestReview` with `event: COMMENT`. +* **Pending comments are invisible to REST**, where `pulls/comments/` returns 404. Read and edit them through GraphQL (`updatePullRequestReviewComment`). +* **Link the priorities.** When a review mixes bugs and nits, list the bugs in the body with `#discussion_r` links so the author fixes those first. +* **Pace the batches.** When the author has just received a large review, post the blocking items and hold structural suggestions until they respond. + +## 9. Re-review after the author pushes + +* **Diff from the last SHA you reviewed, not from the base:** `git fetch upstream pull//head:pr--r2 && git diff pr--r2`. +* **Sort each earlier comment into fixed, partly fixed, or open by reading the code at the new head.** Replies do not count, and a fix without a reply is fine. Thread state comes from GraphQL `reviewThreads { isResolved isOutdated }`. +* **Probe every fix for the bug it might introduce.** Removing an effect left a debounce that could strand a drag handle, and scoring unreadable objects as breaching reached a "right now" column. Both passed the existing tests. +* **When the author pushes back with numbers, check they measured what you suggested,** and time a plan label rather than trusting it. `lookup` against `differential join` said nothing about which was faster. Reply in the thread with your own measurement, since numbers belong in that reply. +* **Re-run the suite on the new head,** and say so in the body. + +## Output + +Group findings as **Blocking**, **Strong suggestions**, **Nits**, and mark +follow-up candidates. Open the summary with one specific sentence on what the PR +does well ("Loving the tests!", "Great work making this page dynamic, and it uses +a lot of our existing patterns"), then the verdict ("Small nits but otherwise +looking really good", "Once my comments are addressed, LGTM"). Name the test +result. Flag anything you could not verify rather than asserting it. diff --git a/.agents/skills/mz-console-review/cost-and-structure.md b/.agents/skills/mz-console-review/cost-and-structure.md new file mode 100644 index 0000000000000..1a12055e7aa6b --- /dev/null +++ b/.agents/skills/mz-console-review/cost-and-structure.md @@ -0,0 +1,120 @@ +# Cost and structure checks for mz-console-review + +The full checks behind SKILL.md sections 5 and 6. Section numbers match, so +cross-references in either file resolve. + +## 5. Check what the change costs + +Every Console query runs on `mz_catalog_server`, often a single worker, and every +page view, poll, and open tab pays for it. The page itself runs in a browser on +every interaction. Neither cost shows up in a read of the diff. + +### 5.1 catalog_server cost + +* **A change to a shared builder changes every caller.** `git grep -n` the builder, list which pages and which polls call it, and say which of them pay for a new join or column. A join added for one page ships to the others whether or not that page's flag is on. +* **EXPLAIN the compiled query on `mz_catalog_server`,** following Phase 1 of the `mz-query-perf` skill. Compile the builder (`.compile()`, parameters inlined) from a throwaway vitest probe, then run it with the Console's session against `materialize/materialized:unstable`: + + ```sql + SET search_path = mz_catalog, mz_internal, mz_catalog_unstable, mz_introspection; + SET cluster = mz_catalog_server; + SET transaction_isolation = serializable; + EXPLAIN OPTIMIZED PLAN AS TEXT FOR ; + ``` + + The `search_path` is `SEARCH_PATH` in `src/api/materialize/executeSql.tsx`. Without it and `SET cluster`, the plan is not the one the Console gets. Read the `Used Indexes` footer for every call shape on base and head. A new `*** full scan ***` on `mz_objects`, `mz_schemas`, `mz_databases`, or a history relation is the finding. +* **Label rows from the global subscribes, not from joins.** `useAllObjects` (mounted in `AppInitializer`) carries id, name, schema, database, `objectType`, `clusterId`, and `sourceType`, and `useAllClusters` carries clusters. A query that joins `mz_objects`, `mz_clusters`, or `mz_object_fully_qualified_names` only to label its rows scans them on every load. Pass the ids and attach labels client-side. +* **How ids reach an index.** Observed on v26.45: a literal `object_id IN (...)` on `mz_wallclock_global_lag_recent_history_ind` is a `(lookup)` up to 330 ids and a full scan from 331. `JOIN unnest(ARRAY[...]::text[]) AS ids(id) ON ids.id = object_id` stays a `(differential join)` at any size. `IN (SELECT id FROM mz_objects WHERE ...)` scans `mz_objects`. +* **A two-column index is not an index on its first column.** `mz_hydration_statuses_ind` is keyed `(object_id, replica_id)`, so filtering by `object_id` alone is a full scan, and EXPLAIN prints a "too wide to use for literal equalities" notice. +* **Fast path is a fact to measure, not a goal.** A plain Map/Filter/Project over an index reads the arrangement without building a dataflow (`Explained Query (fast path)`). A join, `GROUP BY`, or `DISTINCT ON` builds one per query. Only argue that the fast path matters when timing shows it: on `mz_hydration_statuses`, a 2000-id literal list on the fast path took 666 ms where an `unnest` join that builds a dataflow took 50 ms. +* **A subscribe costs per viewer for as long as the tab is open.** A `SUBSCRIBE` over a joined query keeps a dataflow per session at roughly 4.5 microcores per operator per viewer. A bare subscribe over an indexed relation is near zero. Prefer the existing global subscribes, or a bare subscribe joined client-side, to a new joined subscribe. +* **Polls multiply by viewers.** A `refetchInterval` is the query's cost times every open tab. Match `staleTime` and `refetchInterval` to when the answer can change, not to the sampling rate, and remember the default `staleTime` of 0 refetches on every window focus. Wallclock lag is sampled once a minute, but at a 24h range a bin is 24 minutes, so a 60s refetch of a 500 ms query costs roughly 8 mCPU per open tab, against about 2.5 mCPU per user for the whole Console on an M.1-nano `mz_catalog_server`. +* **Size a client-side alternative before calling it cheap.** Build the payload in Node (V8, as in Chrome) and time the parse and the work. For 300 objects with 1440 readings each it was 20 MB uncompressed and about 95 ms, so the payload, not the CPU, was the limit. +* **Per-object columns do not belong in per-bucket rows.** Names, types, and hydration repeated on every time bucket multiply the payload by the bucket count. Return them once, or take them from the client lookup. + +### 5.2 Render performance on the hot interaction + +Find the interaction that runs at pointer or keystroke rate (a drag, a slider, +typing into a filter) and count what it re-renders. Reason about it only after +you have counted. + +* **Probe render counts.** In a throwaway test, bump a counter in the component body or the expensive helper, mock the data hooks with a realistic fixture (`vi.hoisted` for any value a `vi.mock` factory reads), and drive the interaction under fake timers. Report before and after counts in the reply. jsdom inflates absolute times, so report the ratio. + + ```tsx + // In the component or helper under test, for the probe only: + (globalThis as any).__renders = ((globalThis as any).__renders ?? 0) + 1; + + vi.useFakeTimers({ shouldAdvanceTime: true }); + await renderComponent(, { initialRouterEntries: ["/?cluster=u1"] }); + const before = (globalThis as any).__renders ?? 0; + for (let i = 1; i <= 10; i++) { + act(() => { fireEvent.change(input, { target: { value: String(i) } }); }); + act(() => { vi.advanceTimersByTime(10); }); + } + expect({ renders: (globalThis as any).__renders - before }).toEqual("MARKER"); + ``` + +* **An unstable identity breaks every memo below it.** A hook that returns a freshly parsed array or object each render (`parseX(searchParams.get(...))`) invalidates every `useMemo` keyed on it, so the whole derived pipeline reruns per frame. Memoize on the raw primitive. +* **Give the fast value only to what draws it.** When a value changes per frame (a drag handle) and the rest of the page follows a settled copy, pass the fast value only to the component that draws it. Otherwise memoize derived arrays (`rows.filter(...)`) and inline JSX props, and wrap heavy children in `React.memo`. A comment claiming a drag "does not re-render a row per object" is a claim to probe. +* **Bound what mounts.** A table with a row per object needs pagination, which `useUniversalTable` provides. A chart with a line per object needs a cap or an aggregate (see `LINE_MAX_COUNT` in `platform/clusters/queries.ts`). +* **Suspense on a changing key blanks the section.** `useSuspenseQuery` throws on every new key (window, cluster, or an id list in the key), so everything under the boundary swaps to its fallback. `useQuery` with `placeholderData: keepPreviousData` keeps the old data up while the new key loads. +* **Check input behavior in a real browser.** jsdom does not model number-input sanitization or layout. Bundle a minimal repro with the Console's `esbuild` and drive it with Playwright. If the bundled browsers do not match the installed Playwright, launch with `channel: "chrome"`. + +* **Probe URL-state and debounce hooks with `renderHook`.** Wrap them in a `MemoryRouter`, read `useLocation().search`, use `useNavigate()` for a pasted link, and advance fake timers past the settle window: + + ```tsx + const wrapper = ({ children }) => ( + {children} + ); + const { result } = renderHook(() => { + const params = useFreshnessParams(); + const settled = useSettledThreshold(params.threshold, params.setThreshold); + return { params, settled, search: useLocation().search }; + }, { wrapper }); + act(() => result.current.settled.onChange(5000)); + act(() => { vi.advanceTimersByTime(50); }); + act(() => result.current.params.setPredicate("current")); // URL change inside the window + act(() => { vi.advanceTimersByTime(500); }); + expect(result.current.search).toEqual("MARKER"); + ``` + +* **Probe request order with MSW, not `vi.mock`.** A `vi.mock` of an API module did not reach imports inside `platform/clusters/queries.ts`. Record start times in a handler instead: + + ```tsx + server.use( + http.post("*/api/sql", async ({ request }) => { + const body = await request.clone().json(); + const text = body.queries.map((q: any) => q.query).join(" "); + starts[text.includes("mz_hydration_statuses") ? "hydration" : "lag"] = performance.now(); + await delay(200); + return HttpResponse.json({ + results: body.queries.map(() => mapKyselyToTabular({ rows: [], columns })), + }); + }), + ); + ``` + +## 6. Audit hooks and component structure + +### 6.1 Hooks + +List every `useEffect`, `useMemo`, and `useCallback` in the diff and trace who +consumes each value: a memoized child's prop, another hook's dependency list, a +debounce or subscription that must keep one identity, or nothing. Nothing means +the hook can go. + +* **A `useMemo` inside a `React.memo` component, keyed only on that component's props or state, never hits.** A render there already means one of them changed. +* **React Query hashes keys by value.** An array memoized only to build a query key buys nothing. +* **A `useCallback` earns its place only when the consumer is memoized or lists it as a dependency.** Inline arrows into unmemoized controls (`SearchableSelect`, `TimePeriodSelect`, native inputs) are fine. +* **`useEffect` is for external sync and cleanup.** After the audit, every effect left should name the external system it syncs with. +* **Check a dependency's identity in its source.** react-router 7's `setSearchParams` depends on `[navigate, searchParams]`, so every URL change gives it, and every `useCallback` built on it, a new identity. +* **A debounce built in `useMemo` from a callback rebuilds when the callback changes, and its cleanup cancels the pending call.** Hold the callback in `useLatestRef` and build the debounce once. The setters feeding it then need no `useCallback` at all. +* **Two `useSuspenseQuery` calls in one component run in series,** because the first throws before the second is reached. Batch the statements into one `executeSqlV2` request, or use `useSuspenseQueries`. +* **Prove both directions.** Before calling a hook unnecessary, remove it in a probe and show render counts and tests unchanged (5.2). Before calling it necessary, remove it and show what breaks. Dropping the setters' `useCallback`s without the `useLatestRef` fix meant a drag never committed. + +### 6.2 Components + +* **Split loading, deriving, and rendering.** A component that runs data hooks, a chain of memos, and the layout is hard to probe and easy to break. Move a derivation chain whose identity is a performance contract into one hook, with a `renderHook` test that its output keeps its identity across the hot interaction. +* **Keep fast state low.** A value that changes per frame belongs in the lowest component that draws it. When two distant pieces need it (a header input and a graph), move large static blocks such as the control bar into their own component, so they stop re-rendering on every move. +* **Do not pass JSX into a memoized child.** It forces a `useMemo` around an element. Render the alternate state (empty, error) in the parent, and pass the child data. +* **Key the boundary, not the child.** An error boundary that has caught renders its fallback, so re-keying a child it no longer renders does nothing. Put the reset key on `AppErrorBoundary`. +* **Use the layout the neighboring pages use.** The Objects panel's sections are Chakra `Card`s (`ObjectFreshness.tsx`), and the Freshness page's are an `Accordion` (`FreshnessPage.tsx`). A new section layout should match the page it sits beside. Two tables of the same rows with independent sort state is a design question to raise, not a nit. diff --git a/.agents/skills/mz-pr-review/SKILL.md b/.agents/skills/mz-pr-review/SKILL.md index 8a7bd33af193b..bfc9ebf0d5f8d 100644 --- a/.agents/skills/mz-pr-review/SKILL.md +++ b/.agents/skills/mz-pr-review/SKILL.md @@ -5,6 +5,7 @@ description: > "review my code", "review my changes", "check my diff", "does this look ok", "what do you think of this PR", "code review", or look over changes before merging. Also PR number + wants feedback on quality, style, correctness. + For changes under `console/`, use mz-console-review instead. argument-hint: [base-branch] allowed-tools: [Bash, Read, Grep, Glob, Task] --- diff --git a/console/AGENTS.md b/console/AGENTS.md index b1cfd66e095f1..ddcd37231e7a4 100644 --- a/console/AGENTS.md +++ b/console/AGENTS.md @@ -80,7 +80,7 @@ Use `~/` for imports from src: `import { foo } from "~/api/materialize"` ### State Management - **Jotai** for global state (sparingly used - prefer component state) -- **React Query** for polled/one-shot server state (`useSql` hooks) +- **React Query** for polled/one-shot server state (`useQuery` / `useMutation`, see API & Data Fetching) - **Sync engine** for live catalog data: SUBSCRIBE-fed sessions reduce into jotai atoms and TanStack DB collections (`useLiveQuery` consumers, scoped instant-load cache). See `doc/design/20260902_sync_engine.md`. Wiring between @@ -152,8 +152,8 @@ These are enforced by ESLint to ensure consistent patterns: - **Keep callback references stable** for hooks that return functions: ```tsx - // Functions from useSql hooks may be used in dependency arrays - const { refetch } = useSqlTyped(query); + // Functions from useQuery may be used in dependency arrays + const { refetch } = useQuery({ queryKey, queryFn }); ``` - **Use `useMemo` for expensive computations** or to keep prop references stable for child components @@ -163,7 +163,7 @@ These are enforced by ESLint to ensure consistent patterns: const HeavyComponent = React.lazy(() => import("./HeavyComponent")); ``` -- **Use `useSqlLazy` for imperative calls** (form submissions, user actions) instead of `useSql` +- **Use `useMutation` for imperative calls** (form submissions, user actions) - **Prefer `await` over callbacks** for cleaner async flow in form handlers @@ -201,11 +201,11 @@ export const buildClustersQuery = () => { - Use `sql` tagged template for raw SQL to prevent injection - Use `$if()` only for optional selects (per Kysely docs) -### useSql Hooks +### React Query -- `useSql` / `useSqlTyped` - Declarative data fetching with automatic polling/refetch -- `useSqlLazy` - Imperative calls for form submissions and user actions -- `useSqlMany` - Multiple queries in a single request +- Reads use `useQuery` with a key from a feature's query key factory (built on `buildRegionQueryKey`) and a `queryFn` that calls a fetch function from `src/api/materialize/`. See `useMaxReplicasPerCluster` in `src/platform/clusters/queries.ts`. +- Writes use `useMutation` and invalidate the affected query keys in `onSuccess`. See `useAlterCluster` in the same file. +- `useSqlTyped`, `useSqlLazy`, `useSqlMany`, and `useSqlManyTyped` are legacy. Do not use them in new code. ### Error Handling