Migrate off Sandpack - #1083
Conversation
✅ Deploy Preview for react-querybuilder ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe website replaces Sandpack with a Sucrase-based live example system. It compiles MDX code fences, resolves dependencies through esm.sh, runs examples in sandboxed iframes, and updates the external-controls documentation. ChangesLive example execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SandpackRQB
participant LiveExample
participant Runtime
participant Iframe
participant esm.sh
SandpackRQB->>LiveExample: Pass virtual files and dependencies
LiveExample->>Runtime: Compile files and build srcdoc
Runtime->>esm.sh: Resolve bare imports and CSS
Runtime-->>LiveExample: Return compiled iframe document
LiveExample->>Iframe: Mount srcdoc
Iframe-->>LiveExample: Send readiness, height, theme, and error events
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
More templates
@react-querybuilder/antd
@react-querybuilder/bootstrap
@react-querybuilder/bulma
@react-querybuilder/chakra
@react-querybuilder/core
@react-querybuilder/datetime
@react-querybuilder/dnd
@react-querybuilder/expr
@react-querybuilder/fluent
@react-querybuilder/mantine
@react-querybuilder/material
@react-querybuilder/native
@react-querybuilder/prime
react-querybuilder
@react-querybuilder/rules-engine
@react-querybuilder/tremor
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1083 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 362 362
Lines 11085 11085
Branches 4778 4814 +36
=========================================
Hits 11085 11085 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
website/docs/tips/external-controls.mdx (1)
25-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse type-only imports in both TSX virtual files.
FieldandRuleGroupTypeonly occur in type positions.
website/docs/tips/external-controls.mdx#L25-L39: moveFieldandRuleGroupTypeto animport typedeclaration.website/versioned_docs/version-7/tips/external-controls.mdx#L25-L39: make the same import change.As per coding guidelines,
**/*.{ts,tsx}requiresimport typefor type-only imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/docs/tips/external-controls.mdx` around lines 25 - 39, Update the imports in website/docs/tips/external-controls.mdx (lines 25-39) and website/versioned_docs/version-7/tips/external-controls.mdx (lines 25-39) so Field and RuleGroupType are moved to an import type declaration, while runtime imports remain unchanged.Source: Coding guidelines
website/src/components/SandpackRQB.tsx (2)
117-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the entry-path list with the runtime.
runtime.tsline 299 accepts/App.tsx,/App.js,/App.jsx, and/App.tsas the entry module. This check only looks at/App.tsxand/App.js. An/App.jsxentry that already imports the stylesheet still gets the extra<link>. The duplicate is harmless today becausecollectCSSLinksdedupes, but the two lists will drift. Export the entry-path list fromruntime.tsand reuse it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/components/SandpackRQB.tsx` around lines 117 - 120, Export the shared entry-path list from runtime.ts and reuse it in the extraCSSImports useMemo in SandpackRQB, replacing the local /App.tsx and /App.js checks so /App.jsx and /App.ts are handled consistently with the runtime entry-module logic.
55-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
anyelement cast with a narrow fence type.The guideline asks for strict TypeScript. Line 56 casts to
React.ReactElement<any>, soprops.metastring,props.className, andprops.childrenare unchecked. Line 86 then re-assertsprops.className as string | undefined. Declare a localinterface FenceProps { metastring?: string; className?: string; children: string }and cast once toReact.ReactElement<{ children: React.ReactElement<FenceProps> }>. The oxlint suppression then becomes unnecessary.As per coding guidelines: "
**/*.{ts,tsx}: Use strict TypeScript with constrained generics, conditional types, branded types, andimport typefor type-only imports."Also applies to: 86-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/components/SandpackRQB.tsx` around lines 55 - 56, In SandpackRQB, replace the React.ReactElement<any> cast for codeSnippets with a local FenceProps interface defining optional metastring and className strings plus required string children, then cast once to React.ReactElement<{ children: React.ReactElement<FenceProps> }>. Remove the oxlint suppression and the redundant props.className as string | undefined assertion, relying on the constrained FenceProps type.Source: Coding guidelines
website/src/components/LiveExample/LiveExample.tsx (1)
176-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the tab ARIA pattern.
The tab list declares
role="tablist"androle="tab", but no element declaresrole="tabpanel", and the tabs have noaria-controls. Screen readers announce a tab widget that has no associated panel. Thetablistrole also implies arrow-key navigation between tabs, which is not implemented.Add
idandaria-controlslinks plusrole="tabpanel"on the code container, or drop the tab roles and keep plain buttons.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/components/LiveExample/LiveExample.tsx` around lines 176 - 195, The tab widget in the LiveExample render lacks an accessible panel and keyboard navigation support. Either complete the ARIA pattern by assigning stable tab and panel IDs, linking each tab’s aria-controls to the code container, adding role="tabpanel" with the active tab’s aria-labelledby, and implementing arrow-key navigation, or remove the tablist/tab roles and retain plain buttons.website/src/components/LiveExample/runtime.ts (1)
211-221: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReconsider the unconditional 250 ms polling loop.
ResizeObserveron#rootplus theloadlistener already cover content-driven changes. The interval at line 221 adds a forced layout read every 250 ms for the life of each iframe. A docs page with several examples pays this cost continuously, including while the examples are off screen.If the interval only covers absolutely positioned overflow that
ResizeObservermisses, then observedocument.bodyas well and remove the interval, or stop the interval once the height stabilizes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/components/LiveExample/runtime.ts` around lines 211 - 221, Remove the unconditional setInterval polling from the report setup and extend ResizeObserver coverage to document.body alongside `#root`, preserving the existing load listener and height-change reporting behavior in report.website/src/components/LiveExample/styles.module.css (1)
1-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis file deviates from the stylesheet guideline.
The guideline asks for SCSS with BEM-like class names and SCSS variables for design tokens. This file is plain CSS with camelCase CSS-module class names. The camelCase names are required by the
styles.codeColumnaccess pattern inLiveExample.tsx, so confirm that the guideline is intended to cover Docusaurus CSS modules. If it is, rename the file tostyles.module.scss.As per coding guidelines: "
**/*.{scss,css}: Use SCSS with CSS custom properties, BEM-like class names such as.queryBuilder-rule, and SCSS variables for design tokens."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/components/LiveExample/styles.module.css` around lines 1 - 19, Convert LiveExample’s stylesheet from styles.module.css to styles.module.scss, preserving the existing styles.codeColumn and related CSS-module access patterns in LiveExample.tsx. Retain CSS custom properties for design tokens and update class selectors only as needed to follow the project’s BEM-like naming guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/src/components/LiveExample/LiveExample.tsx`:
- Around line 63-95: Validate the message sender before processing payloads in
both handlers: in LiveExample.tsx, have the onMessage handler return unless
event.source equals iframeRef.current?.contentWindow before handling height or
error data; in website/src/components/LiveExample/runtime.ts lines 240-244,
return unless e.source equals parent before toggling the dark class. These are
the only required changes at the listed sites.
- Around line 25-28: Update loadSucrase so a rejected dynamic import clears
sucrasePromise before propagating the error, allowing subsequent LiveExample
instances or client-side navigations to retry the load while preserving the
single shared promise for successful loads.
In `@website/src/components/LiveExample/runtime.ts`:
- Line 68: Update buildImportMap and the REACT_VERSION configuration so
customSetup.dependencies entries for react and react-dom are either honored when
constructing imports or explicitly rejected with a clear error; do not silently
ignore requested versions while forcing React 18. Preserve React Query Builder’s
React >=18 compatibility when no custom versions are provided.
In `@website/src/components/SandpackRQB.tsx`:
- Around line 54-93: Update parseFences and its useMemo caller to avoid throwing
for malformed author input: validate each child’s fence shape before accessing
codeSnippet.props.children, and return a parse-error result instead of throwing
for invalid children, missing filenames, or duplicate paths. Propagate that
error to the component state and render it beside the existing compile-error
overlay, while preserving normal LiveFile parsing for valid fences.
---
Nitpick comments:
In `@website/docs/tips/external-controls.mdx`:
- Around line 25-39: Update the imports in
website/docs/tips/external-controls.mdx (lines 25-39) and
website/versioned_docs/version-7/tips/external-controls.mdx (lines 25-39) so
Field and RuleGroupType are moved to an import type declaration, while runtime
imports remain unchanged.
In `@website/src/components/LiveExample/LiveExample.tsx`:
- Around line 176-195: The tab widget in the LiveExample render lacks an
accessible panel and keyboard navigation support. Either complete the ARIA
pattern by assigning stable tab and panel IDs, linking each tab’s aria-controls
to the code container, adding role="tabpanel" with the active tab’s
aria-labelledby, and implementing arrow-key navigation, or remove the
tablist/tab roles and retain plain buttons.
In `@website/src/components/LiveExample/runtime.ts`:
- Around line 211-221: Remove the unconditional setInterval polling from the
report setup and extend ResizeObserver coverage to document.body alongside
`#root`, preserving the existing load listener and height-change reporting
behavior in report.
In `@website/src/components/LiveExample/styles.module.css`:
- Around line 1-19: Convert LiveExample’s stylesheet from styles.module.css to
styles.module.scss, preserving the existing styles.codeColumn and related
CSS-module access patterns in LiveExample.tsx. Retain CSS custom properties for
design tokens and update class selectors only as needed to follow the project’s
BEM-like naming guideline.
In `@website/src/components/SandpackRQB.tsx`:
- Around line 117-120: Export the shared entry-path list from runtime.ts and
reuse it in the extraCSSImports useMemo in SandpackRQB, replacing the local
/App.tsx and /App.js checks so /App.jsx and /App.ts are handled consistently
with the runtime entry-module logic.
- Around line 55-56: In SandpackRQB, replace the React.ReactElement<any> cast
for codeSnippets with a local FenceProps interface defining optional metastring
and className strings plus required string children, then cast once to
React.ReactElement<{ children: React.ReactElement<FenceProps> }>. Remove the
oxlint suppression and the redundant props.className as string | undefined
assertion, relying on the constrained FenceProps type.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb89b7aa-f301-4637-bedd-6d377dcaba17
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
website/docs/tips/external-controls.mdxwebsite/package.jsonwebsite/src/components/LiveExample/LiveExample.tsxwebsite/src/components/LiveExample/runtime.tswebsite/src/components/LiveExample/styles.module.csswebsite/src/components/SandpackRQB.tsxwebsite/versioned_docs/version-7/tips/external-controls.mdx
1 participant
Remove dependencies on Sandpack and implement a new live example using the updated components.
Summary by CodeRabbit
New Features
Bug Fixes