fix(playground): stabilize onQueryChange callback to prevent stale closures

[keydunov]

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

Issue Reference this PR resolves

CUB-2396 — Handle report creation that does not save in IDE

Description of Changes Made

The onQueryChange callback in the useQueryBuilder hook was captured in a useEffect closure but not included in the dependency array. This is a classic React stale-closure bug.

When the parent component (e.g., the IDE's Explore or report creation view) provides a new onQueryChange callback — for example, after creating a new exploration or switching between reports — the old callback was still invoked instead of the new one. This caused report creation in the IDE to appear to succeed (the chart renders because the Core Data API query works fine) while the save/persist callback, provided by the parent, was stale and pointed to the wrong context.

Fix: Wrap onQueryChange with the existing useEvent hook (a useCallback+ref pattern already available in the codebase) to ensure the latest callback is always invoked without adding it to the useEffect dependency array (which would cause unnecessary re-fires on every parent render).

Changed file:

• packages/cubejs-playground/src/QueryBuilderV2/hooks/query-builder.ts

The useEvent hook is a shim of the React RFC for useEvent — it stores the latest callback in a ref and returns a stable function identity, solving both the stale closure and unnecessary effect re-triggering problems.

Linear Issue: CUB-2396

  

[claude]

Claude finished @keydunov's task in 1m 5s —— View job

---

PR Review: fix(playground): stabilize onQueryChange callback to prevent stale closures

• Gather context and read changed files
• Review code quality, correctness, and patterns
• Check for security, performance, and testing concerns
• Post review feedback

---

Verdict: Approve — This is a clean, correct fix for a real stale-closure bug.

Analysis

The bug: onQueryChange (a prop callback) was called inside a useEffect (line 1030–1032) but was not in that effect's dependency array [queryHash, chartType, pivotConfig]. When the parent swapped in a new onQueryChange (e.g., after creating a new report), the effect still captured the old reference, so the new callback was never invoked.

The fix wraps onQueryChange with the codebase's existing useEvent hook, which stores the latest callback in a ref (useSyncRef) and returns a stable function identity via useCallback(…, []). This is the standard pattern already used ~20 times across this codebase (e.g., getConnectedDimensionNames, selectCube, etc.).

Code Quality

The implementation is concise and idiomatic:

const onQueryChangeStable = useEvent(
  (...args: Parameters<NonNullable<typeof onQueryChange>>) => {
    onQueryChange?.(...args);
  }
);

• The optional chaining (?.) correctly handles the case where onQueryChange is undefined (since the prop is typed as optional).
• Using Parameters<NonNullable<typeof onQueryChange>> preserves type safety without duplicating the callback signature.
• The stable identity from useEvent means the effect dependency array [queryHash, chartType, pivotConfig] remains correct — no unnecessary re-fires from callback identity changes.

Minor Observation

The useEffect at line 1030–1032 still has query referenced in the body but uses queryHash in the deps array (a derived hash of the query object). This is an intentional pattern used elsewhere in the file (line 1024–1028 does the same) — it avoids re-firing on referentially-different-but-semantically-equal query objects. Not introduced by this PR, just noting it for context.

Security / Performance

No concerns. This is a pure React callback stabilization with no impact on data flow, API calls, or user input handling.

Testing

The PR doesn't add tests, which is reasonable — this is a closure-timing fix in a React hook that's difficult to unit-test without a full integration harness. The fix uses a well-tested pattern (useEvent) already validated across the codebase.

Summary

Minimal, surgical fix that follows established codebase conventions. The useEvent pattern is the right tool for this problem. LGTM.