Add initial JS proxy slice#48
Draft
emmapowers wants to merge 21 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
.gitignore (1)
67-71: Consolidate duplicate comment headers.The identical comment "# Auto-generated by wt (workflow-tools)" appears twice. Consider grouping both patterns under a single header for cleaner organization.
♻️ Proposed refactor
-# Auto-generated by wt (workflow-tools) -*.local.code-workspace - # Auto-generated by wt (workflow-tools) +*.local.code-workspace .envrc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 67 - 71, Consolidate the duplicate header "# Auto-generated by wt (workflow-tools)" by keeping a single comment line and listing both patterns (*.local.code-workspace and .envrc) under it; update the .gitignore so the header appears once followed by the two entries to remove redundancy and improve organization.src/trellis/core/rendering/session.py (1)
87-88: Consider adding a type annotation forproxy_transport.Using
tp.Anyloses type safety. If aProtocoldefining the required interface (e.g., a method for sending proxy calls) can be defined without circular imports, it would improve maintainability and enable static analysis of proxy transport usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/core/rendering/session.py` around lines 87 - 88, Add a concrete Protocol for the proxy transport and use it instead of tp.Any: define a ProxyTransportProtocol (name it exactly) that declares the methods used by session code (e.g., send/call/close or the actual method names used in this file), then change the attribute annotation from proxy_transport: tp.Any = None to proxy_transport: tp.Optional[ProxyTransportProtocol] = None; if importing concrete transport types would cause circular imports, declare the Protocol in this module or behind typing.TYPE_CHECKING and use forward references or TYPE_CHECKING guards to import implementations only for type checking.src/trellis/platforms/browser/client/src/PyodideWorker.ts (1)
13-21: Align send and receive message types for consistency.
WorkerInMessageproperly typespayload: Message, butWorkerOutMessageandMessageCallbackstill useRecord<string, unknown>. This forces themsg as unknown as Messagecast inTrellisApp.tsx(line 204) and defeats type checking at the worker boundary. Since the worker only emits protocol messages, changeWorkerOutMessage.messagevariant topayload: Messageand updateMessageCallbackaccordingly to type the callback end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/browser/client/src/PyodideWorker.ts` around lines 13 - 21, WorkerOutMessage and MessageCallback currently use Record<string, unknown> while WorkerInMessage uses payload: Message; update the worker-out types to match by changing the WorkerOutMessage "message" variant to use payload: Message (instead of Record<string, unknown>) and update MessageCallback's parameter type to Message so the worker boundary is typed end-to-end (this will remove the need for the msg as unknown as Message cast in TrellisApp.tsx). Ensure you update all references to WorkerOutMessage and MessageCallback signatures to the new Message type.src/trellis/platforms/server/client/src/TrellisClient.ts (1)
77-77: Consider adding error handling for async message processing.Using
voiddiscards the promise returned byhandleMessage. While this is a common pattern for event handlers, unhandled rejections from proxy call processing will not be caught here.If proxy calls can fail in ways that should surface to the user, consider wrapping with
.catch():this.handler.handleMessage(msg).catch((err) => { console.error("Message handling failed:", err); });This is optional if errors are already handled internally by
handleMessage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/client/src/TrellisClient.ts` at line 77, The voided call to this.handler.handleMessage(msg) discards its Promise so unhandled rejections can be lost; change the call in TrellisClient (where the message is dispatched) to attach error handling — e.g., call this.handler.handleMessage(msg).catch(err => /* log or surface error */) or wrap in an async fire-and-forget helper that logs/report errors to the existing logger or error-reporting path so any rejection from handleMessage is observed; use the this.handler.handleMessage symbol to locate and update the dispatch site.tests/py/unit/test_messages.py (1)
130-150: Consider adding error case coverage forProxyCallResponse.The test verifies the success case (
resultpopulated,errornull), but doesn't cover the error case whereerroranderror_typeare populated. This would ensure full round-trip coverage.💡 Optional: Add error case test
def test_proxy_call_response_msgpack_roundtrip(self) -> None: """ProxyCallResponse survives msgpack encode/decode.""" encoder = msgspec.msgpack.Encoder() decoder = msgspec.msgpack.Decoder(Message) original = ProxyCallResponse( request_id="req-1", result={"message": "hello"}, ) encoded = encoder.encode(original) decoded = decoder.decode(encoded) assert isinstance(decoded, ProxyCallResponse) assert decoded.request_id == "req-1" assert decoded.result == {"message": "hello"} assert decoded.error is None assert decoded.error_type is None + + def test_proxy_call_response_error_msgpack_roundtrip(self) -> None: + """ProxyCallResponse with error survives msgpack encode/decode.""" + encoder = msgspec.msgpack.Encoder() + decoder = msgspec.msgpack.Decoder(Message) + + original = ProxyCallResponse( + request_id="req-err", + result=None, + error="Something went wrong", + error_type="TypeError", + ) + encoded = encoder.encode(original) + decoded = decoder.decode(encoded) + + assert isinstance(decoded, ProxyCallResponse) + assert decoded.request_id == "req-err" + assert decoded.result is None + assert decoded.error == "Something went wrong" + assert decoded.error_type == "TypeError"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_messages.py` around lines 130 - 150, Add a new unit test that covers the error-case roundtrip for ProxyCallResponse: create a ProxyCallResponse with request_id set, result set to None (or omitted), and both error and error_type populated, then msgpack-encode with msgspec.msgpack.Encoder and decode with msgspec.msgpack.Decoder(Message) and assert the decoded object is a ProxyCallResponse and that request_id, error, and error_type match the originals and result is None; add this as a separate test method (e.g., test_proxy_call_response_msgpack_roundtrip_error) alongside the existing success-case test.examples/js_proxy_demo/js_proxy_demo/app.py (1)
48-53: Consider addingkw_only=Truefor consistency.Other
Statefuldataclasses in the codebase use@dataclass(kw_only=True). While not strictly required here (all fields have defaults), adding it maintains consistency:-@dataclass +@dataclass(kw_only=True) class DemoState(Stateful):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/js_proxy_demo/js_proxy_demo/app.py` around lines 48 - 53, The DemoState dataclass is missing kw_only=True on its decorator for consistency with other Stateful dataclasses; update the decorator on the DemoState class (the `@dataclass` on the DemoState definition) to `@dataclass`(kw_only=True) so fields remain keyword-only like other Stateful dataclasses (class DemoState, subclass of Stateful).docs/plans/2026-03-08-js-proxy-initial-slice.md (1)
161-172: Minor inconsistency between plan and implementation.The plan shows
method: str(line 164) but the actual implementation inmessages.pyusesmethod: str | Noneto support function proxies. This is fine since the implementation is correct; consider updating the plan if it will be kept as reference documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-08-js-proxy-initial-slice.md` around lines 161 - 172, The plan's ProxyCall declaration is inconsistent with the implementation: update the plan's ProxyCall struct to match the code by changing the field signature from method: str to method: str | None (to support function proxies), and optionally add a brief note explaining why method may be None; ensure the change is applied to the ProxyCall definition in the plan (the ProxyCall symbol) so it matches the messages.py implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/core/proxy.py`:
- Around line 150-156: The decorator currently injects a synthetic __init__ that
sets self._transport = None and assigns it to proxy_class.__init__, which
overwrites any existing base-class initializer and breaks slotted classes;
remove the synthetic initializer assignment (do not set proxy_class.__init__ =
__init__) and keep only the setattr assigning _CLASS_TARGET_ATTR to the class
(leave proxy_class as-is); rely on the existing _make_object_method logic which
already tolerates a missing _transport instead of creating a replacement
__init__.
In `@src/trellis/platforms/common/client/src/ClientMessageHandler.ts`:
- Around line 137-149: The code extracts object methods as bare functions
(objectTarget, method, msg.method) and then calls callableTarget(...msg.args)
which loses the original this; update the branch that handles ExportKind.OBJECT
so that when you assign callableTarget you bind the method to the object (e.g.,
callableTarget = method.bind(objectTarget)) or otherwise wrap the call to invoke
method with objectTarget as receiver, ensuring instance methods see the correct
this context while preserving the existing error check for typeof method !==
"function".
In `@tests/js/unit/ServerTrellisClient.test.ts`:
- Around line 81-92: The test currently assumes root path and can miss
regressions where ServerTrellisClient.connect() hardcodes "/"; update the test
to set a non-root pathname before calling ServerTrellisClient.connect() (e.g.,
change window.location.pathname or use history.pushState to "/nested/path") so
that when you call client.connect() and trigger fakeSocket.onopen, the sent
HELLO message's path must match that non-root value; modify the test around
ServerTrellisClient.connect / fakeSocket.onopen and the final
expect(message.path) to assert the chosen non-root path.
---
Nitpick comments:
In @.gitignore:
- Around line 67-71: Consolidate the duplicate header "# Auto-generated by wt
(workflow-tools)" by keeping a single comment line and listing both patterns
(*.local.code-workspace and .envrc) under it; update the .gitignore so the
header appears once followed by the two entries to remove redundancy and improve
organization.
In `@docs/plans/2026-03-08-js-proxy-initial-slice.md`:
- Around line 161-172: The plan's ProxyCall declaration is inconsistent with the
implementation: update the plan's ProxyCall struct to match the code by changing
the field signature from method: str to method: str | None (to support function
proxies), and optionally add a brief note explaining why method may be None;
ensure the change is applied to the ProxyCall definition in the plan (the
ProxyCall symbol) so it matches the messages.py implementation.
In `@examples/js_proxy_demo/js_proxy_demo/app.py`:
- Around line 48-53: The DemoState dataclass is missing kw_only=True on its
decorator for consistency with other Stateful dataclasses; update the decorator
on the DemoState class (the `@dataclass` on the DemoState definition) to
`@dataclass`(kw_only=True) so fields remain keyword-only like other Stateful
dataclasses (class DemoState, subclass of Stateful).
In `@src/trellis/core/rendering/session.py`:
- Around line 87-88: Add a concrete Protocol for the proxy transport and use it
instead of tp.Any: define a ProxyTransportProtocol (name it exactly) that
declares the methods used by session code (e.g., send/call/close or the actual
method names used in this file), then change the attribute annotation from
proxy_transport: tp.Any = None to proxy_transport:
tp.Optional[ProxyTransportProtocol] = None; if importing concrete transport
types would cause circular imports, declare the Protocol in this module or
behind typing.TYPE_CHECKING and use forward references or TYPE_CHECKING guards
to import implementations only for type checking.
In `@src/trellis/platforms/browser/client/src/PyodideWorker.ts`:
- Around line 13-21: WorkerOutMessage and MessageCallback currently use
Record<string, unknown> while WorkerInMessage uses payload: Message; update the
worker-out types to match by changing the WorkerOutMessage "message" variant to
use payload: Message (instead of Record<string, unknown>) and update
MessageCallback's parameter type to Message so the worker boundary is typed
end-to-end (this will remove the need for the msg as unknown as Message cast in
TrellisApp.tsx). Ensure you update all references to WorkerOutMessage and
MessageCallback signatures to the new Message type.
In `@src/trellis/platforms/server/client/src/TrellisClient.ts`:
- Line 77: The voided call to this.handler.handleMessage(msg) discards its
Promise so unhandled rejections can be lost; change the call in TrellisClient
(where the message is dispatched) to attach error handling — e.g., call
this.handler.handleMessage(msg).catch(err => /* log or surface error */) or wrap
in an async fire-and-forget helper that logs/report errors to the existing
logger or error-reporting path so any rejection from handleMessage is observed;
use the this.handler.handleMessage symbol to locate and update the dispatch
site.
In `@tests/py/unit/test_messages.py`:
- Around line 130-150: Add a new unit test that covers the error-case roundtrip
for ProxyCallResponse: create a ProxyCallResponse with request_id set, result
set to None (or omitted), and both error and error_type populated, then
msgpack-encode with msgspec.msgpack.Encoder and decode with
msgspec.msgpack.Decoder(Message) and assert the decoded object is a
ProxyCallResponse and that request_id, error, and error_type match the originals
and result is None; add this as a separate test method (e.g.,
test_proxy_call_response_msgpack_roundtrip_error) alongside the existing
success-case test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44c6b6ed-7bba-46eb-a8d6-ee07eac30a56
📒 Files selected for processing (35)
.gitignoredocs/plans/2026-03-08-js-proxy-initial-slice.mdexamples/js_proxy_demo/js_proxy_demo/__init__.pyexamples/js_proxy_demo/js_proxy_demo/app.pyexamples/js_proxy_demo/js_proxy_demo/client/__init__.pyexamples/js_proxy_demo/js_proxy_demo/client/demo_api.tsexamples/js_proxy_demo/pyproject.tomlexamples/js_proxy_demo/trellis_config.pysrc/trellis/__init__.pysrc/trellis/bundler/workspace.pysrc/trellis/core/__init__.pysrc/trellis/core/proxy.pysrc/trellis/core/rendering/session.pysrc/trellis/platforms/browser/client/src/BrowserClient.tssrc/trellis/platforms/browser/client/src/PyodideWorker.tssrc/trellis/platforms/browser/client/src/TrellisApp.tsxsrc/trellis/platforms/common/__init__.pysrc/trellis/platforms/common/client/src/ClientMessageHandler.tssrc/trellis/platforms/common/client/src/TrellisClient.tssrc/trellis/platforms/common/client/src/proxyTargets.tssrc/trellis/platforms/common/client/src/types.tssrc/trellis/platforms/common/handler.pysrc/trellis/platforms/common/messages.pysrc/trellis/platforms/desktop/client/src/DesktopClient.tssrc/trellis/platforms/server/client/src/TrellisClient.tssrc/trellis/registry/__init__.pytests/js/unit/BrowserClient.test.tstests/js/unit/ClientMessageHandler.test.tstests/js/unit/PyodideWorker.test.tstests/js/unit/ServerTrellisClient.test.tstests/py/integration/test_message_handler_integration.pytests/py/unit/test_messages.pytests/py/unit/test_proxy.pytests/py/unit/test_registry.pytests/py/unit/test_workspace.py
Comment on lines
+150
to
+156
| def __init__(self: tp.Any) -> None: | ||
| self._transport = None | ||
|
|
||
| setattr(cls, _CLASS_TARGET_ATTR, name or cls.__name__) | ||
| proxy_class = tp.cast("tp.Any", cls) | ||
| proxy_class.__init__ = __init__ | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid overwriting the decorated class initializer.
Line 155 replaces whatever __init__ the class would otherwise inherit, so base-class setup is skipped and slotted classes can fail on self._transport = None. _make_object_method already handles a missing _transport, so the decorator can drop the synthetic initializer entirely.
🔧 Proposed fix
- def __init__(self: tp.Any) -> None:
- self._transport = None
-
setattr(cls, _CLASS_TARGET_ATTR, name or cls.__name__)
- proxy_class = tp.cast("tp.Any", cls)
- proxy_class.__init__ = __init__
for attr_name, proxy_method in proxy_methods.items():
setattr(cls, attr_name, proxy_method)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def __init__(self: tp.Any) -> None: | |
| self._transport = None | |
| setattr(cls, _CLASS_TARGET_ATTR, name or cls.__name__) | |
| proxy_class = tp.cast("tp.Any", cls) | |
| proxy_class.__init__ = __init__ | |
| setattr(cls, _CLASS_TARGET_ATTR, name or cls.__name__) | |
| for attr_name, proxy_method in proxy_methods.items(): | |
| setattr(cls, attr_name, proxy_method) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/trellis/core/proxy.py` around lines 150 - 156, The decorator currently
injects a synthetic __init__ that sets self._transport = None and assigns it to
proxy_class.__init__, which overwrites any existing base-class initializer and
breaks slotted classes; remove the synthetic initializer assignment (do not set
proxy_class.__init__ = __init__) and keep only the setattr assigning
_CLASS_TARGET_ATTR to the class (leave proxy_class as-is); rely on the existing
_make_object_method logic which already tolerates a missing _transport instead
of creating a replacement __init__.
Comment on lines
+81
to
+92
| it("sends hello on websocket open", () => { | ||
| const client = new ServerTrellisClient(); | ||
|
|
||
| void client.connect(); | ||
| fakeSocket.onopen?.(); | ||
|
|
||
| expect(fakeSocket.send).toHaveBeenCalledTimes(1); | ||
|
|
||
| const encoded = fakeSocket.send.mock.calls[0][0] as Uint8Array; | ||
| const message = decode(encoded) as { type: string; path: string }; | ||
| expect(message.type).toBe(MessageType.HELLO); | ||
| expect(message.path).toBe("/"); |
Contributor
There was a problem hiding this comment.
Use a non-root URL in this regression test.
With the default JSDOM location, window.location.pathname is already /, so this still passes if ServerTrellisClient.connect() regresses to hard-coding root. Setting a nested path before connect() will actually lock in the live fix.
Suggested adjustment
it("sends hello on websocket open", () => {
- const client = new ServerTrellisClient();
-
- void client.connect();
- fakeSocket.onopen?.();
-
- expect(fakeSocket.send).toHaveBeenCalledTimes(1);
-
- const encoded = fakeSocket.send.mock.calls[0][0] as Uint8Array;
- const message = decode(encoded) as { type: string; path: string };
- expect(message.type).toBe(MessageType.HELLO);
- expect(message.path).toBe("/");
-
- client.disconnect();
+ const originalPath = window.location.pathname;
+ window.history.replaceState({}, "", "/docs/demo");
+ try {
+ const client = new ServerTrellisClient();
+
+ void client.connect();
+ fakeSocket.onopen?.();
+
+ expect(fakeSocket.send).toHaveBeenCalledTimes(1);
+
+ const encoded = fakeSocket.send.mock.calls[0][0] as Uint8Array;
+ const message = decode(encoded) as { type: string; path: string };
+ expect(message.type).toBe(MessageType.HELLO);
+ expect(message.path).toBe("/docs/demo");
+
+ client.disconnect();
+ } finally {
+ window.history.replaceState({}, "", originalPath);
+ }
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/js/unit/ServerTrellisClient.test.ts` around lines 81 - 92, The test
currently assumes root path and can miss regressions where
ServerTrellisClient.connect() hardcodes "/"; update the test to set a non-root
pathname before calling ServerTrellisClient.connect() (e.g., change
window.location.pathname or use history.pushState to "/nested/path") so that
when you call client.connect() and trigger fakeSocket.onopen, the sent HELLO
message's path must match that non-root value; modify the test around
ServerTrellisClient.connect / fakeSocket.onopen and the final
expect(message.path) to assert the chosen non-root path.
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
1 participant
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.
Summary
ExportKind.OBJECTand_registry.tsproxy target wiringproxy_call/proxy_call_responsemessages plus client and Python runtime transport supportjs_proxy_demoapp, focused regression tests, and the server client fix found during live E2E testingTesting
examples/js_proxy_democovering success, error, retry, and refresh flowsSummary by CodeRabbit
New Features
@js_proxyand@js_methoddecoratorsDocumentation
Chores
Tests