-
Notifications
You must be signed in to change notification settings - Fork 1
Fix overlapping wasm bindings #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughRemoved wasm-specific TypeScript augmentation from crates/cli/src/meta/mod.rs and replaced dependent tsify annotations in crates/cli/src/meta/types/dotrain/gui_state_v1.rs to use inline 0x-prefixed string types. No Rust types, runtime logic, or non-wasm behavior changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
crates/cli/src/meta/mod.rs(0 hunks)crates/cli/src/meta/types/dotrain/gui_state_v1.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/cli/src/meta/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (macos-latest, rainix-rs-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (macos-latest, rainix-rs-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts)
🔇 Additional comments (1)
crates/cli/src/meta/types/dotrain/gui_state_v1.rs (1)
34-36: The script output only shows the first search—please manually verify that the other checks ran and returned no matches:
- No
TS_APPEND_CONTENT,TS_DECLARATIONS, orts_appendmacros anywhere- No
.ts/.d.tsfiles still referring toAddressorHexConfirm the full script ran and produced no output.
| #[cfg_attr(target_family = "wasm", tsify(type = "`0x${string}`"))] | ||
| pub dotrain_hash: B256, | ||
| /// User-configured field values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
B256 → 0x${string} narrows shape but loses length semantics; document and note breaking change
This matches serde’s string output, but TS can’t enforce 32-byte length. Add a short doc hint (e.g., “0x-prefixed 32-byte hex”) and call out the TS-facing breaking change in the changelog/release notes.
Confirm the runtime JSON across the wasm boundary is a lowercased 0x-prefixed hex string (no ArrayBuffer/bytes) and that any consumers didn’t rely on a distinct Hex type for narrowing.
Consider adding a wasm-only comment near the field to state the expected length for maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its ok, no worries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rouzwelt Thanks for confirming! I understand you're comfortable with this change. The type binding adjustment looks good to go.
🧠 Learnings used
Learnt from: brusherru
PR: rainlanguage/rain.metadata#80
File: crates/cli/src/metaboard.rs:36-45
Timestamp: 2025-08-14T16:28:50.322Z
Learning: In crates/cli/src/metaboard.rs, the validate_dotrain_content function intentionally only checks for non-empty content. Full .rain syntax validation is planned for future work but was considered out of scope for the initial implementation.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
0x${string}literal types for hex values.