Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
name: review-change
name: review-pr
description: Review a code change against QuestDB Web Console coding standards
argument-hint: [PR number, PR URL, commit hash, unstaged changes, staged changes] [--level=0..3]
allowed-tools: Bash(gh *), Bash(git diff*), Bash(yarn test:unit), Bash(yarn lint), Bash(yarn typecheck), Bash(yarn build), Read, Grep, Glob, Agent
Expand Down Expand Up @@ -161,7 +161,7 @@ Launch the following agents in parallel. (Level 1 launches only Agents 1, 2, 3,

**Agent 5: Persistence & migrations:** Dexie/IndexedDB correctness in `src/store` and consumers — schema version bumped when table shape changes; migration upgrades existing persisted data rather than dropping it; no data loss for users upgrading from a previous version; correct handling of quota-exceeded and corruption; compression/serialization round-trips losslessly (`compression.ts`, `buffers.ts`); reads tolerate older/missing fields written by prior versions; no blocking of the main thread on large persisted payloads.

**Agent 6: Performance & rendering at scale:** Unnecessary rerenders through missing useMemo/useCallback where a component passes callbacks to memoized children or large lists; unnecessary memoization of small functions/computations that prevents no rerender; **context-driven re-render storms (consumers re-rendering because a provider value isn't memoized)**; missing virtualization (`react-virtuoso`) for large result sets / long lists / the schema tree; expensive echarts/uPlot re-renders or full re-inits where an update would do; inline object/array/function creation in JSX props causing referential inequality; unnecessary or unnecessarily frequent network requests and IndexedDB writes; expensive computations without memoization.
**Agent 6: Performance & rendering at scale:** Unnecessary rerenders through missing useMemo/useCallback where a component passes callbacks to memoized children or large lists; unnecessary memoization of small functions/computations that prevents no rerender; **context-driven re-render storms (consumers re-rendering because a provider value isn't memoized)**; missing virtualization (`react-virtuoso`) for large result sets / long lists / the schema tree; expensive echarts/uPlot re-renders or full re-inits where an update would do; inline object/array/function creation in JSX props causing referential inequality; unnecessary or unnecessarily frequent network requests and IndexedDB writes; expensive computations without memoization. **Algorithmic optimality:** for every loop, traversal, or lookup added or changed in render, effects, selectors, query/result parsing, or schema-tree handling, state the time complexity and flag sub-optimal choices — an O(n) `.find`/`.indexOf`/`.includes`/`.filter` linear scan where a `Map`/`Set`/object index gives O(1); an O(n²) nested `.find`/`.some`/`.filter` inside a `.map` over result rows or schema items; rebuilding a lookup structure (Map/Set/index) on every render instead of constructing it once and memoizing; re-parsing or re-deriving already-computed data; and multiple passes over the same result set that could be fused into one. The bar is the best known approach, not merely “avoids quadratic” — these costs compound on large result sets, long buffer lists, and wide schema trees.

**Agent 7: Styling & theming:** Hardcoded colors/sizes instead of theme tokens, CSS specificity issues, z-index conflicts, animation performance (prefer `transform`/`opacity` over layout-triggering properties), styled-components created inside render functions (causes remounting), proper use of `css` helper for conditional styles, `$`-prefixed prop names for style-only props, proper use of `rem` units, not pixels, proper use of styled components instead of inline styling, proper use of existing icon libraries instead of custom SVGs, proper font/icon/box sizes that are consistent.

Expand Down Expand Up @@ -217,7 +217,7 @@ For each finding in the draft report:
3. **For data-integrity claims (Agent 1):** trace the actual result shape the client returns and confirm the consumer can really receive the empty/error/partial/null case claimed. A wrong-data or no-data-without-error finding is critical only if a real query path produces it.
4. **For stale-closure / stale-state claims:** verify the closure actually captures a stale value AND that a fresh value is needed there. Check whether the value is read from a ref or passed fresh — if so, drop it.
5. **For missing-cleanup claims:** verify the effect actually sets up something that needs cleanup (timer, subscription, listener, AbortController, Monaco disposable) and that the component can unmount or the deps can change while it is live. A one-shot effect that cannot re-run or unmount mid-flight is not a leak.
6. **For re-render / performance claims (incl. unmemoized context values):** verify the provider/parent actually re-renders often enough to matter and that consumers really re-render as a result. Do not flag memoization that prevents no real re-render, or `useCallback`/`useMemo` whose deps churn anyway.
6. **For re-render / performance claims (incl. unmemoized context values):** verify the provider/parent actually re-renders often enough to matter and that consumers really re-render as a result. Do not flag memoization that prevents no real re-render, or `useCallback`/`useMemo` whose deps churn anyway. **For algorithmic-complexity claims** (O(n) where O(1) is achievable, O(n²) scans, redundant passes): confirm the complexity analysis is correct and the path is reachable with realistic data (large result sets, long buffer lists, wide schema trees). Such findings are valid regardless of the *current* data size — do not downgrade one just because today's input is small; only drop it if the analysis is wrong or the collection is bounded by a small constant (e.g. column count, a fixed enum).
7. **For race-condition claims:** trace the actual async ordering and verify two operations can realistically interleave via real user actions (rapid clicks, navigation mid-request, unmount mid-fetch, abort-then-restart). If the ordering is structurally impossible, drop it. For a missing-query-abort claim specifically, confirm the two queries actually compete for the same single-slot result surface (the result grid) — if they write to different targets, or are background queries meant to run in parallel, there is no stale-overwrite race and the missing abort is not a bug.
8. **For persistence/migration claims (Agent 5):** confirm the schema version and migration path, and whether existing persisted data is actually at risk. Verify against `migrations.ts` rather than assuming.
9. **For cross-context findings (Agent 12):** re-read the callsite in full, including its callers up two levels, and confirm the broken behavior is reachable from production code paths. Cross-context findings are high-value but also the easiest to overstate — verify carefully.
Expand Down
Loading
Loading