Skip to content

Grok repo idiomaticity review #3

Description

@jakeboone02

Code review findings — opportunities for improvement

Solid 2 port of React Query Builder on @react-querybuilder/core. Target: idiomatic Solid 2, DOM parity with React Query Builder, hybrid QueryManager (writes) + store projection (reads).

Reactivity and state boundary

  • Controlled mode + parent createStore proxy path relies on snapshot() + signatureOf / unchangedSignature. Needs explicit test coverage for the case where the parent holds the query in its own store and passes the proxy back.
  • Deep trees (100+ rules): measure update cost on a single value edit. Confirm only the edited rule’s effects re-run and that createRuleState / createRuleGroupState resolver/handler allocation stays acceptable.
  • preview in createRuleActions clones the manager for vetoes that inspect the resulting query. Cost on large trees under frequent vetoes is unmeasured.
  • Multi-step UI actions that call several actions in one event are not automatically batched. Core’s manager.batch is available; call sites that grow past one mutation must use it explicitly.

Types and custom control surface

  • Widening via as unknown as RuleProps / as never appears at component boundaries because Solid components are plain generic functions with no compile-time prop enumeration. Expected shape for replacement rule / ruleGroup / control elements should be documented more explicitly in docs/customization.md.
  • Headless usage guidance (“read from state.rootGroup, not state.query”) is present but easy to miss. Elevate in JSDoc on QueryBuilderState and in the headless example.
  • Path / rule helpers used by custom UIs are mostly in core; discoverability from this package is limited. Consider a small re-export or headless example that shows path math.

Solid 2 / SSR / packaging

  • Peer range is solid-js@^2.0.0-rc.0 and @solidjs/web@^2.0.0-rc.0. RC churn on createProjection, split createEffect, and context APIs can break the load-bearing path. check:versions helps; keep it in CI.
  • Deferred writes (read after set still sees previous value until next microtask / flush()) are documented but will surprise React migrants. Examples should demonstrate the pattern.
  • SSR gate is a hand-rolled Vite example. No SolidStart 2 coverage (SolidStart remains Solid 1). Router, server functions, and meta-framework pipeline are untested. Replace examples/ssr when SolidStart 2 ships.
  • createUniqueId for radio groups: confirm SSR + hydration ID stability under the hand-rolled setup.

Testing gaps

  • Conformance suite covers DOM parity against React fixtures. Independent-combinator edge cases, obscure controlElements override paths, and validation-map interactions should be checked against the React suite for completeness.
  • Controlled + store-proxy feedback loop (above) lacks a dedicated test.
  • No published measurement of fine-grained update behavior on large queries.

Documentation nits

  • README controlled-mode example uses query={query()}. Correct for Solid; add a one-line note that the prop is a getter and that immediate post-set reads are stale.
  • Document that an externally supplied manager prop is never reconfigured by the component (consumer owns it).

Scope (non-goals already stated)

DnD, UI-kit packages, async option lists, deprecated props, Redux/qbId, preserveQueryStateOnUnmount remain out of scope. No change requested; listed only for completeness.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions