docs(wren-core-wasm): refresh onboarding for inline-mode default#2290
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughDocs updated in core/wren-core-wasm: Inline Mode is now primary for local/bundled use, URL Mode documents HTTP Range/CORS and server checks, Node.js WASM init requires a BufferSource ChangesWren Core WASM Documentation Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
50886e0 to
c206cc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/wren-core-wasm/README.md (1)
115-115: 💤 Low valueMinor markdown inconsistency.
For consistency with AGENT_GUIDE.md line 88, consider using backticks:
(not \200`)instead of(not 200)`.🤖 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 `@core/wren-core-wasm/README.md` at line 115, The markdown text in the README Quick check line shows "(not 200)" which is inconsistent with AGENT_GUIDE.md; update that fragment to use inline code formatting so it reads "(not `200`)". Locate the Quick check sentence in core/wren-core-wasm/README.md (the line containing curl -I -H "Range: bytes=0-1023" ...) and replace the plain 200 with backticked `200` to match the style used in AGENT_GUIDE.md.core/wren-core-wasm/AGENT_GUIDE.md (1)
215-215: 💤 Low valueConsider clarifying the
file://protocol limitation.The statement "
file://protocol won't work for fetching remote Parquet" is technically correct but potentially confusing sincefile://URLs are inherently local. Consider rephrasing to: "URL mode requires HTTP(S);file://won't work as a data source."🤖 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 `@core/wren-core-wasm/AGENT_GUIDE.md` at line 215, Edit the AGENT_GUIDE.md entry that currently reads "5. **CORS required** for URL mode — `file://` protocol won't work for fetching remote Parquet." and rephrase it to clarify the limitation: say that URL mode requires HTTP(S) and that file:// is a local protocol and cannot be used as a data source (for example: "5. **CORS required** for URL mode — URL mode requires HTTP(S); file:// cannot be used as a data source."). Ensure the updated sentence replaces the original wording for clarity.
🤖 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 `@core/wren-core-wasm/AGENT_GUIDE.md`:
- Around line 79-86: Update the server support table row notes to call out known
range-request caveats: for "npx serve" and "Vite / webpack-dev-server" add text
that they can mis-handle range validation (leading to 416 on misaligned ranges)
and that "webpack-dev-server" only supports single-range requests (multipart
ranges fall back to returning the whole resource); optionally add a short
pointer to the validation workaround or link to it. Target the table entries for
the server names "npx serve", "Vite / webpack-dev-server", and
"webpack-dev-server" in AGENT_GUIDE.md and expand their Notes column
accordingly.
---
Nitpick comments:
In `@core/wren-core-wasm/AGENT_GUIDE.md`:
- Line 215: Edit the AGENT_GUIDE.md entry that currently reads "5. **CORS
required** for URL mode — `file://` protocol won't work for fetching remote
Parquet." and rephrase it to clarify the limitation: say that URL mode requires
HTTP(S) and that file:// is a local protocol and cannot be used as a data source
(for example: "5. **CORS required** for URL mode — URL mode requires HTTP(S);
file:// cannot be used as a data source."). Ensure the updated sentence replaces
the original wording for clarity.
In `@core/wren-core-wasm/README.md`:
- Line 115: The markdown text in the README Quick check line shows "(not 200)"
which is inconsistent with AGENT_GUIDE.md; update that fragment to use inline
code formatting so it reads "(not `200`)". Locate the Quick check sentence in
core/wren-core-wasm/README.md (the line containing curl -I -H "Range:
bytes=0-1023" ...) and replace the plain 200 with backticked `200` to match the
style used in AGENT_GUIDE.md.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d03672e9-7848-4d59-b312-56082a2a6ba2
📒 Files selected for processing (2)
core/wren-core-wasm/AGENT_GUIDE.mdcore/wren-core-wasm/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@core/wren-core-wasm/README.md`:
- Line 17: The README import line references an unpublished CDN tag; change the
import URL that pulls WrenEngine from
'https://unpkg.com/@wrenai/wren-core-wasm@0.4.0/dist/index.js' to use the
published tag '@0.3.0' instead (update the URL to '...@0.3.0/...'), and scan for
any other occurrences of '@wrenai/wren-core-wasm@0.4.0' in the README to update
them as well so examples import WrenEngine from the published version.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eee03dc9-2bf0-4b7d-85e6-371a76f31afc
📒 Files selected for processing (2)
core/wren-core-wasm/AGENT_GUIDE.mdcore/wren-core-wasm/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- core/wren-core-wasm/AGENT_GUIDE.md
Improvements based on a real end-to-end onboarding session: - Bump CDN version references from @0.1.0 to @0.3.0 to match the currently published @wrenai/wren-core-wasm tarball. - Reorder Inline Mode before URL Mode in both README and AGENT_GUIDE; inline is the path of least resistance for local dev and bundled dashboards (< ~50 MB), and avoids the entire HTTP Range / CORS class of bugs. - Add a "Choosing a local dev server" section explaining that `python -m http.server` lacks `Range:` support (silently breaks URL mode), with a comparison table and a `curl -I` smoke check. - Add a "Node.js usage" section showing the `wasmUrl: BufferSource` pattern — `WrenEngine.init()` defaults to fetching via `import.meta .url` which resolves to `file://` in Node and fails. Useful for `node --test` smoke checks. - Expand AGENT_GUIDE "Common Pitfalls" with range-support, Node `BufferSource`, and sequential-registration guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c206cc2 to
846702a
Compare
|
Addressed @coderabbitai's review in 846702a:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3506
Everything looks good. Thanks for the thorough follow-through! 🐇 ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Summary
@0.1.0→@0.3.0(matches the currently published@wrenai/wren-core-wasmpackage).Range:requirement up front.python -m http.server❌,npx serve✅,RangeHTTPServer✅, etc.) plus acurl -I -H "Range: ..."smoke check.wasmUrl: BufferSourcepattern —WrenEngine.init()without options fails immediately under Node becausefile://fetch isn't supported. Required for anynode --testsmoke checks against the published package.BufferSource, and "register sequentially" (the engine is single-threaded; concurrentregisterParquetcorrupts state).Motivation
These changes were gathered from an end-to-end onboarding session where the agent and user hit four predictable, time-costing failure modes documented in this PR:
@0.1.0).python -m http.server) silently breaks it — fetches stall after the Parquet footer with no error.init()before any user code runs.registerParquetcalls corrupt the engine state.Each item above maps to a concrete friction point that previously cost time during real onboarding.
Test plan
grepshows no remaining@0.1.0references in the touched files.npx serve, and confirm the query returns rows.node --testand confirmWrenEngine.init()resolves with the BufferSource.curl -I -H "Range: bytes=0-1023"against a Parquet served bypython -m http.serverto confirm the docs match observed behavior (no206).Out of scope
A companion plan (
IMPROVEMENT_PLAN_skill_and_docs.mdin the contributor's workspace) also called for:cubeQuery/listCubesAPI reference. Skipped here — those APIs are not yet onmain. They should land with the feature, not ahead of it.registerCsv. Same reason.~/.claude/skills/wren-dlt-connector/). Those live outside this repo.🤖 Generated with Claude Code
Summary by CodeRabbit