Issue #153: Design for triggering actions from the client#175
Issue #153: Design for triggering actions from the client#175nivethadhanakoti wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a design specification for triggering Meerkat actions from browser events and introduces a currently failing button test using ChangesClient Actions
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/design/client-actions.md`:
- Around line 93-97: Update the proposed Html event-attribute and DOM-rendering
design so browsers receive only opaque, server-issued handler identifiers rather
than action statements or captured environments. Bind each identifier to the
session and authorization context, and resolve it server-side to the original
closure before executing the action; do not expose or trust closure fields in
browser event listeners.
- Around line 99-104: Update the Html module design description to define the
safe-rendering boundary: interpolated text and ordinary attributes must be
emitted as data through safe DOM APIs, while trusted/raw HTML nodes must be
explicitly identified and handled separately. Clarify that adopting a structured
tree alone does not provide injection protection unless these rules are
enforced.
- Around line 75-85: Expand the browser-to-server section around remote_action
to define the end-to-end dispatch contract: specify the client transport and
request payload carrying action identity and arguments, session/authentication
handling, server routing into the existing Manager::remote_action path,
client-visible error behavior, and how resulting reactive updates are delivered
to subscribed clients. Keep the local-action path and existing reactive
machinery unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bf58035-e0db-4116-a215-ca8a936bd2f3
📒 Files selected for processing (2)
docs/design/client-actions.mdmeerkat/tests/client_button.mkt
| When the corresponding event fires, the runtime invokes the action. If the | ||
| action's `ServiceNetId` names the local node, it runs locally through the | ||
| ordinary action path. If it names another node, it is sent there through | ||
| `remote_action`. The captured environment and service identity supply the | ||
| execution context, so no new capture or scoping rules are required. | ||
|
|
||
| Any state the action changes propagates through the existing reactive | ||
| machinery. In the example above, clicking the button increments `x` on the | ||
| server, which recomputes `s1.y`, which pushes an update to the subscribed | ||
| client, which recomputes `z` and re-renders `html`. The click is the only new | ||
| step; everything after it already works. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define the browser-to-server dispatch contract.
remote_action in meerkat-lib/src/runtime/manager/mod.rs, Lines 1369-1473, is a node-to-node Manager method that sends an ActionRequest; a browser event cannot call it directly. Specify the client transport, action identity, session handling, error behavior, and update delivery needed to make this end-to-end flow implementable.
🤖 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 `@docs/design/client-actions.md` around lines 75 - 85, Expand the
browser-to-server section around remote_action to define the end-to-end dispatch
contract: specify the client transport and request payload carrying action
identity and arguments, session/authentication handling, server routing into the
existing Manager::remote_action path, client-visible error behavior, and how
resulting reactive updates are delivered to subscribed clients. Keep the
local-action path and existing reactive machinery unchanged.
There was a problem hiding this comment.
Partly addressed, but I think the premise needs a correction: the browser is a Meerkat node in this architecture. PR #143 gives the wasm client a Manager with a NetworkActor, and it already calls manager methods directly, so remote_action is reachable the same way it is on a native client. The real question underneath is how a DOM event reaches the runtime and identifies which handler fired, which the new trust-boundary section covers in part — the opaque-handle approach makes the click payload just an identifier.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Current design: event/runtime/trust-boundary references =='
rg -n -C 4 -i 'remote_action|trust.boundary|opaque|handle|DOM|event' docs/design/client-actions.md || true
echo
echo '== Runtime remote_action definition and call sites =='
rg -n -C 3 '\bremote_action\s*\(' meerkat-lib meerkat 2>/dev/null || true
echo
echo '== WASM/client Manager and NetworkActor references in checked-out PR =='
rg -n -C 3 '\b(Manager|NetworkActor)\b' --glob '*.rs' . || true
echo
echo '== Pull request `#143` metadata and changed files =='
gh pr view 143 --repo meerkat-lang/meerkat --json number,title,state,mergedAt,headRefName,baseRefName,url \
--jq '{number,title,state,mergedAt,headRefName,baseRefName,url}'
gh pr view 143 --repo meerkat-lang/meerkat --json files \
--jq '.files[] | {path, additions, deletions}' || trueLength of output: 49506
@nivethadhanakoti, you’re right — the premise of my finding was incorrect. #143’s merged WASM client creates a Manager, attaches a NetworkActor, and participates as a Meerkat node, so it can invoke Manager::remote_action through the ordinary runtime path.
The revised design also now correctly identifies the actual boundary: mapping a DOM event to a runtime handler and ensuring the handler reference cannot be forged. The proposed opaque, session-scoped identifier is an appropriate direction; the document explicitly leaves its final form open. I’m withdrawing the finding.
🐇🔍
✏️ Learnings added
Learnt from: nivethadhanakoti
Repo: meerkat-lang/meerkat PR: 175
File: docs/design/client-actions.md:75-88
Timestamp: 2026-07-23T07:00:26.362Z
Learning: In the Meerkat WASM client architecture introduced by merged pull request `#143`, the browser runs a `Manager` with a `NetworkActor` and is a Meerkat node. Browser-originated runtime operations, including `Manager::remote_action`, use the normal node-to-node path; client-action design reviews should focus on securely bridging DOM events to runtime handler identities rather than assuming an additional browser-to-server transport is needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
… trust boundary
1 participant
Design only, per the issue: no implementation, for a round of feedback first.
Adds
docs/design/client-actions.mdproposing a language extension that binds an action to a user interface event, andmeerkat/tests/client_button.mktas a demonstrating test case. The .mkt file does not work today by design — eventattributes are not recognised and an Html value cannot carry an action — and should work once the design is implemented.
The proposal in short
An attribute beginning with
onis an event attribute, and its interpolated expression must evaluate to an action:This reuses the existing
{expr}interpolation; only what the expression evaluates to differs. BecauseValue::ActionClosurealready carries aServiceNetId, the action can belong to another node — a browser can hold ahandler for a server-side action, and
remote_actionalready accepts exactly what the closure stores.Two things worth pushing back on
The design treats the
Htmlrepresentation change as a prerequisite, not an optional refinement: an Html value is currently a rendered string, which has nowhere to put a handler, so it would need to become a structured tree. That isa larger change than the syntax suggests, though the Html module documents a tree as its anticipated direction, and it also addresses the injection concern raised on #143.
Passing an input's value to an action is left unresolved. Action closures have no parameter list, so either they gain parameters or the event value is bound into the captured environment under a well known name. I have set out both
rather than picking one.
Summary by CodeRabbit
Documentation
Tests