Canvas SDK: post-merge review followups (PR #1401)#1420
Open
jmoseley wants to merge 2 commits into
Open
Conversation
Cross-language: - Mark canvas surface as Experimental in Rust (doc-comment warning), Node (JSDoc @experimental tag), Python (module + per-type comment), and Go (per-symbol doc comment). .NET already had [Experimental(Diagnostics.Experimental)]. Rust: - Add Default impls (via #[derive(Default)]) to CanvasDeclaration, CanvasOpenContext, CanvasActionContext, and CanvasLifecycleContext for forward-proofing against new optional members. Node: - Extract CanvasHostCapabilities as a named exported interface (previously inlined into CanvasHostContext). Matches the other SDKs. .NET: - Rename CanvasError -> CanvasException (C# *Exception convention). - Convert init-only setters to set on canvas context types (consistent with the rest of the SDK). - Simplify CanvasErrorHelpers.Build using SerializeToElement with a JsonObject + TypesJsonContext; drop the bespoke CanvasErrorPayload type and now-empty CanvasJsonContext. - Cache JsonElement for the JSON literal 'null' to avoid parsing on every canvas action that returns void/null. - Document why JsonRpc.HandleIncomingMethodAsync unwraps TargetInvocationException (handlers are dispatched via reflection). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies post-merge follow-ups to the Canvas SDK surface (added in #1401), focusing on cross-language “experimental” labeling and a set of .NET API/implementation cleanups for consistency and reduced allocations.
Changes:
- Mark the canvas surface as Experimental across Rust/Node/Python/Go (Rust rustdoc warnings, Node JSDoc
@experimental, Python module note + per-type comments, Go per-symbol doc comments). - Rust: add
Defaultderives to several canvas declaration/context types to ease forward-compatible construction as optional fields evolve. - .NET: rename
CanvasError→CanvasException, switch canvas context properties frominittoset, simplify error payload construction viaJsonObject+TypesJsonContext, cache anullJsonElement, and documentTargetInvocationExceptionunwrapping rationale in JSON-RPC dispatch.
Show a summary per file
| File | Description |
|---|---|
| rust/src/canvas.rs | Adds experimental rustdoc warnings and derives Default for canvas declaration/context types. |
| python/copilot/canvas.py | Adds module-level experimental note and per-type experimental comments. |
| nodejs/src/index.ts | Re-exports CanvasHostCapabilities from the public Node entrypoint. |
| nodejs/src/canvas.ts | Adds module/type @experimental docs and extracts CanvasHostCapabilities into a named exported interface. |
| go/canvas.go | Adds per-symbol experimental doc comments for the Go canvas surface. |
| dotnet/test/Unit/CanvasTests.cs | Updates unit tests for CanvasError → CanvasException rename. |
| dotnet/src/Types.cs | Adds JsonObject to source-generated serialization context for canvas error payloads. |
| dotnet/src/Session.cs | Updates exception handling for CanvasException and caches a null JsonElement for action results. |
| dotnet/src/JsonRpc.cs | Documents (and continues) unwrapping TargetInvocationException during handler dispatch. |
| dotnet/src/Canvas.cs | Renames CanvasError → CanvasException, converts context init setters to set, and simplifies error payload generation. |
Copilot's findings
- Files reviewed: 10/10 changed files
- Comments generated: 0
This comment has been minimized.
This comment has been minimized.
Main's #1413 aligned canvas with the codegen pipeline. Re-applied followups on top: - Re-applied Experimental markers to hand-written canvas surfaces (Rust doc-comments, Node @experimental JSDoc, Python module/class docstrings, Go per-symbol doc comments). - Re-applied Default derive on CanvasDeclaration in Rust. - Re-applied .NET CanvasError -> CanvasException rename across Canvas.cs, Session.cs, and CanvasTests.cs. - Re-applied CanvasErrorHelpers.Build simplification (JsonObject + TypesJsonContext.Default.JsonObject); removed CanvasErrorPayload and CanvasJsonContext partial. - Re-applied Session.cs NullJsonElement cache and simplified SerializeActionResult. - Updated index.ts re-export to CanvasHostContextCapabilities to match codegen rename. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅The PR does a good job of maintaining cross-language consistency. Changes reviewed:
One minor gap:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1420 · ● 7.4M
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to the canvas SDK surface added in #1401. Scope: cross-language polish + .NET cleanups. Excludes the
Canvasescapability rename (separate session) and E2E canvas tests (separate session).Cross-language
@experimental), Python (module + per-type comment), and Go (per-symbol doc comment). .NET already had[Experimental(Diagnostics.Experimental)].Rust
Defaultimpls (via#[derive(Default)]) toCanvasDeclaration,CanvasOpenContext,CanvasActionContext, andCanvasLifecycleContextfor forward-proofing against new optional members.Node
CanvasHostCapabilitiesas a named exported interface; matches the other SDKs..NET
CanvasError→CanvasException(C#*Exceptionconvention).initsetters →seton canvas context types (consistent with the rest of the SDK).CanvasErrorHelpers.BuildusingSerializeToElementwith aJsonObject+TypesJsonContext; drop the bespokeCanvasErrorPayloadtype and now-emptyCanvasJsonContext.JsonElementfor the JSON literalnullto avoidJsonDocument.Parseon every canvas action that returns void/null.JsonRpc.HandleIncomingMethodAsyncunwrapsTargetInvocationException(handlers are dispatched via reflection /DynamicInvoke).Validation
cargo fmt --check,cargo clippy --all-features --all-targets -- -D warnings,cargo test --all-features(129 passed).npm run typecheck,npm run lint(0 errors),vitest run(86 passed).gofmt -l .,go build ./...,go vet ./..., unit tests pass.ruff check,ty check copilot(all clean), unit tests (80 passed).dotnet format --verify-no-changes,dotnet build -f net8.0, canvas tests pass (9/9).Related: #1401
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com