Skip to content

Grok repo idiomaticity review #4

Open

Description

@jakeboone02

Code review findings

Review of main (package svelte-querybuilder @ 0.1.1) against the stated goal: idiomatic Svelte 5 UI on @react-querybuilder/core, with query/DOM parity and a Svelte-shaped API.

Packaging / dependency

  • @react-querybuilder/core is resolved via a pkg.pr.new URL (...@3d5d992), not a stable npm version range. Unsuitable for consumers and for reproducible installs once published more widely. Pin to a released version (or an explicit range) and document the upgrade path.
  • Package version is 0.1.1 while the public surface (controls, props, re-exports of core, history API) is large. No explicit stability / 1.0 criteria in README or docs.

Types and any

  • Remaining any / loose bridges:
    • getSubQueryBuilderProps return typed as any
    • action handlers with optional context?: any
    • Control.svelte props typed as any
    • several as unknown as casts where core string identifiers meet stricter generics
  • Prefer narrower types or localized generics so public and internal call sites stay checkable without casts.

Maintainability

  • RuleComponents.svelte concentrates many getter-backed prop bags plus conditional control markup in one file. Prop-bag construction could be moved to small helpers (same pattern as withCommonProps) to reduce density without changing behavior.
  • Empty HTML comment joiners between sibling controls are required for DOM text parity with React fixtures. They are documented in AGENTS.md and comments; still easy to “clean up” in a formatter pass. Consider a short CONTRIBUTING note or a conformance failure message that points at this constraint.

Reactivity edge cases

  • createValueEditorReset is the main $effect-style path (timing for value reset on operator/type change). Keep dedicated tests on that path so behavior does not regress relative to core.
  • Documented constraint: rebuilding query as a new object every read (e.g. query={{ ...q }}) reverts edits. Worth a short “reactivity gotchas” subsection (stable query reference, getter context, getter-backed control props) aimed at advanced consumers and custom controls.

Core alignment / future-proofing

  • Compile-time guard accounts for core control keys not implemented here (dragHandle, ruleGroupBodyElements, ruleGroupHeaderElements). A CI check against core’s exported key list would make upgrades fail loudly when core adds keys.
  • enableDragAndDrop is forced false; root always emits data-dnd="disabled". Consistent with non-goals; no change needed unless the non-goal list changes.

Tooling

  • check:exports (attw + specifier lint) and deletion of test artifacts from dist are present. Keep both in the release path so Node ESM consumers and published tarballs stay clean.
  • SSR is gated via examples/sveltekit (test:ssr). Treat as a hard CI requirement; do not drop when iterating on the library package alone.

Docs

  • docs/differences-from-react-querybuilder.md covers intentional API divergences. Gaps relative to implementation detail:
    • stable query reference requirement (already in README warning; easy to miss)
    • why control props are getter-backed
    • context-as-getter requirement for nested / subquery builders
  • No explicit list of what would block a 1.0 (API freeze, core version policy, remaining non-goals).

Suggested follow-ups (priority)

  1. Replace pkg.pr.new core dependency with a stable npm version/range.
  2. Tighten or document remaining any / cast sites.
  3. Split prop-bag construction out of RuleComponents (optional refactor).
  4. Add reactivity gotchas + 1.0 / core-pin policy to docs.
  5. Optional CI: assert implemented control keys against core’s controlKeys / controlKind.

Activity

  1. jakeboone02 commented on Sep 26, 2026

    @jakeboone02
    MemberAuthor

    Will be addressed in in 0.2.0 (see #5):

    Packaging: core is already ^8.24.3 from npm, not pkg.pr.new. There's a new README section, "Stability and versioning", covering pre-1.0 semantics, the core version policy (core minor and conformance tag move together) and what blocks 1.0. The version bump happens at release.

    Types: getSubQueryBuilderProps now returns a named SubQueryBuilderProps type. Every context is unknown instead of any (breaking). Control.svelte is generic over the control's props, so each call site is type-checked; this caught a real valueSource typing bug in MatchModeEditor. Casts in RuleSubQuery, ruleParts and ValueEditor are removed. The remaining anys mirror core's RuleType.value and are commented as such.

    RuleComponents density: declined. Prop bags already go through withCommonProps, which is the pattern you suggested. Moving them into another module adds indirection without making anything clearer.

    Joiners: conformance failures caused only by whitespace in an element's text now point to the <!-- --> joiners and the AGENTS.md pitfall. No CONTRIBUTING.md; AGENTS.md plus the failure message covers it.

    Value-editor reset: already covered (8 dedicated tests in valueEditorEffect.svelte.test.ts, plus resetScaling and ValueEditor tests). No change.

    Reactivity gotchas: new docs/reactivity.md. Context-as-getter was already in customization.md and is now linked from the new page.

    Control keys: a compile-time guard already failed svelte-check when core added a key. A runtime test now also checks against core's controlKeys/controlKind.

    Also: a test caps $effect use in src/lib to an allow-list. DnD, check:exports and test:ssr are unchanged; all already run in CI and check:all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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