Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughChangesSchema-directed argument casting
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant ArgsForm
participant ExecutionPanel
participant castArgsToKnownTypes
participant encodeRunArgs
User->>ArgsForm: Edit enum or class argument
ArgsForm-->>ExecutionPanel: Marker-free JSON state
ExecutionPanel->>castArgsToKnownTypes: Parse and schema-cast arguments
castArgsToKnownTypes-->>ExecutionPanel: Transiently annotated arguments
ExecutionPanel->>encodeRunArgs: Encode cast arguments
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95df3940fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new Set(), | ||
| strictObject, | ||
| ); | ||
| if (fieldCast === CAST_FAILED) return CAST_FAILED; |
There was a problem hiding this comment.
When a known class contains a media/unsupported field, or a valid deep schema whose extraction tail was cut to unsupported, that child's cast always returns CAST_FAILED and this line discards the cast for the entire class. The encoder consequently serializes the marker-free object as a map rather than a classValue, so otherwise valid playground calls using classes with raw-editor fields fail with an "expected instance, got map" error. Outside strict union disambiguation, preserve the original child value while retaining the known parent class annotation.
Useful? React with 👍 / 👎.
| strictObject, | ||
| ); | ||
| if (entryCast === CAST_FAILED) return CAST_FAILED; | ||
| cast[key] = entryCast; |
There was a problem hiding this comment.
__proto__ entries when rebuilding maps
When a map<string, T> argument contains the valid JSON key __proto__, assigning it into the ordinary {} accumulator invokes the inherited prototype setter instead of creating an own property. Since encodeRunArgs later enumerates own entries, that map entry silently disappears from the execution payload. Build the result with Object.fromEntries, a null-prototype object, or an explicit own-property definition.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
typescript2/pkg-playground/src/ExecutionPanel.tsx (1)
2237-2241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe adjacent comment now understates what raw mode does.
Line 2229 still claims raw mode is "an exact escape hatch"; with this change raw-mode args are also schema-cast before encoding (only reconciliation is skipped). Worth clarifying so the next reader doesn't assume raw bytes are byte-for-byte what the user typed.
🤖 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 `@typescript2/pkg-playground/src/ExecutionPanel.tsx` around lines 2237 - 2241, The comment near the argument encoding flow should no longer describe raw mode as an “exact escape hatch.” Update it to state that raw mode skips reconciliation but still casts arguments through castArgsToKnownTypes before encodeRunArgs, so it does not guarantee byte-for-byte preservation of user input.typescript2/pkg-playground/src/args-form-model.test.ts (1)
319-434: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the strict-union and required-field-omission paths.
The suite exercises unique/ambiguous/invalid cases well, but two behaviors that decide whether a payload gets annotated are untested:
strictObjectunion matching (surplus class keys must make a union variant fail) and the class branch that fails the cast when a missing field is not omittable (schemaAllowsOmission). Both are the difference between an annotated payload and a silent fallback, so a regression there would be invisible.🤖 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 `@typescript2/pkg-playground/src/args-form-model.test.ts` around lines 319 - 434, Extend the castArgsToKnownTypes tests to cover strictObject union matching, verifying that surplus class keys reject that union variant and leave the payload unannotated, and cover the required-field omission path where schemaAllowsOmission is false, verifying the class cast fails and falls back without annotation. Add focused cases alongside the existing union and class-casting tests.
🤖 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.
Nitpick comments:
In `@typescript2/pkg-playground/src/args-form-model.test.ts`:
- Around line 319-434: Extend the castArgsToKnownTypes tests to cover
strictObject union matching, verifying that surplus class keys reject that union
variant and leave the payload unannotated, and cover the required-field omission
path where schemaAllowsOmission is false, verifying the class cast fails and
falls back without annotation. Add focused cases alongside the existing union
and class-casting tests.
In `@typescript2/pkg-playground/src/ExecutionPanel.tsx`:
- Around line 2237-2241: The comment near the argument encoding flow should no
longer describe raw mode as an “exact escape hatch.” Update it to state that raw
mode skips reconciliation but still casts arguments through castArgsToKnownTypes
before encodeRunArgs, so it does not guarantee byte-for-byte preservation of
user input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0dc3607-403b-4f0a-892c-6116c773bf46
📒 Files selected for processing (6)
typescript2/app-vscode-webview/src/ExecutionPanel.strict-mode.test.tsxtypescript2/pkg-playground/src/ArgsForm.tsxtypescript2/pkg-playground/src/ExecutionPanel.tsxtypescript2/pkg-playground/src/args-form-model.test.tstypescript2/pkg-playground/src/args-form-model.tstypescript2/pkg-playground/src/worker-protocol.ts
Summary
Why
The dynamic args form currently writes
$bamltransport markers into user-visible JSON. The parameter and type schemas already identify most values, so marker generation belongs at the execution boundary rather than editor state.Validation
pnpm --filter @b/pkg-playground test(132 tests)pnpm --filter @b/pkg-playground typecheckpnpm --filter app-vscode-webview typecheckpnpm --filter app-vscode-webview test:unit:run(21 tests)pnpm run build:wasmpnpm --filter app-vscode-webview buildLinear: B-814
Before / after
Before —
$bamlmarkers in editor stateAt the exact PR base (
2960e7a), the TypeScript2 playground's raw editor persists class and enum transport markers.After — marker-free editor with execution-boundary casting
At the exact PR head (
c4ec9b2), the same fixture and viewport keep ordinary JSON in the raw editor; the successful graph input rendersuser.Person, demonstrating that the known type is applied only at the execution boundary.