T2 (#168) Embedded tooling UI baseline (Open Props + screenshot regen) #176
Conversation
Serve a session dashboard from echo-session-ws-gateway using vendored Open Props CSS, add Playwright smoke coverage, and allow the docs screenshot to be regenerated from e2e. Closes #168
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an embedded session dashboard with real-time metrics collection, hub observer integration, and E2E testing. The gateway now serves a dark-themed HTML dashboard, exposes a Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser Client
participant Dashboard as Dashboard UI
participant Gateway as Gateway /api/metrics
participant HubObs as Hub Observer Task
participant Hub as Hub Service (Unix Socket)
Note over Browser,Hub: Metrics Collection & Display Loop
Browser->>Dashboard: Load /dashboard
activate Dashboard
Dashboard->>Gateway: GET /api/metrics (every 1s)
activate Gateway
par Metrics Collection
Gateway->>HubObs: Read hub observer messages (async)
activate HubObs
HubObs->>Hub: Subscribe to WARPs (Unix socket)
activate Hub
Hub-->>HubObs: Stream observer messages (snapshots/diffs)
deactivate Hub
end
Gateway->>Gateway: Snapshot all metrics:<br/>connections, warps, counters,<br/>hub observer state
Gateway-->>Dashboard: MetricsResponse JSON
deactivate Gateway
Dashboard->>Dashboard: Update UI:<br/>numeric fields, WARP table,<br/>uptime, throughput
Dashboard->>Dashboard: Enqueue chart data &<br/>render D3 chart
Dashboard-->>Browser: Render metrics + chart
deactivate Dashboard
Note over Dashboard: Tick loop repeats every second
opt Hub Observer Error
HubObs->>HubObs: Backoff & retry
HubObs-->>Dashboard: Error state reflected<br/>in next metrics fetch
end
opt Theme Switch
Browser->>Dashboard: Toggle dark/light
Dashboard->>Dashboard: Switch CSS theme,<br/>persist to localStorage
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: The Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 |
![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.
ℹ️ 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".
| if let Err(err) = hub_observer_send_handshake(&mut writer).await { | ||
| warn!(?err, "hub observer: handshake send failed"); | ||
| let mut metrics = state.metrics.lock().await; | ||
| metrics.observe_hub_observer_error(err.to_string()); | ||
| continue; |
There was a problem hiding this comment.
The observer reconnect loop only backs off on connect errors, but a failed handshake or subscribe write immediately continues with no sleep (hub_observer: handshake send failed path here, subscribe path just below). If the hub closes the socket or rejects the handshake/subscription (e.g., wrong version or permissions), this loop will spin as fast as it can, repeatedly reconnecting and logging without delay, wasting CPU and flooding logs. Consider delaying or reusing the exponential backoff for these failure cases so the observer degrades gracefully when the hub isn't accepting handshakes.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockdocs/assets/wvp/session-dashboard.pngis excluded by!**/*.png
📒 Files selected for processing (18)
.gitignorecrates/echo-session-ws-gateway/Cargo.tomlcrates/echo-session-ws-gateway/README.mdcrates/echo-session-ws-gateway/assets/dashboard.htmlcrates/echo-session-ws-gateway/assets/vendor/buttons.min.csscrates/echo-session-ws-gateway/assets/vendor/normalize.min.csscrates/echo-session-ws-gateway/assets/vendor/open-props.LICENSEcrates/echo-session-ws-gateway/assets/vendor/open-props.min.csscrates/echo-session-ws-gateway/assets/vendor/theme.dark.switch.min.csscrates/echo-session-ws-gateway/assets/vendor/theme.light.switch.min.csscrates/echo-session-ws-gateway/src/main.rsdocs/assets/collision/diagrams.cssdocs/decision-log.mddocs/execution-plan.mddocs/guide/wvp-demo.mde2e/collision-dpo-tour.spec.tse2e/session-dashboard.spec.tsplaywright.config.ts
💤 Files with no reviewable changes (1)
- playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/echo-session-ws-gateway/src/main.rs
🧠 Learnings (1)
📚 Learning: 2025-12-28T23:14:28.103Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.103Z
Learning: Start each session by updating *Today's Intent* in `docs/execution-plan.md` and capture milestones, blockers, and decisions in the Decision Log.
Applied to files:
docs/execution-plan.md
🧬 Code graph analysis (1)
crates/echo-session-ws-gateway/src/main.rs (1)
crates/echo-session-proto/src/wire.rs (2)
decode_message(263-293)encode_message(232-260)
🪛 Biome (2.1.2)
crates/echo-session-ws-gateway/assets/vendor/open-props.min.css
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
[error] 1-1: Unexpected nonstandard direction
You should fix the direction value to follow the syntax.
See MDN web docs for more details.
(lint/correctness/noInvalidDirectionInLinearGradient)
⏰ 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). (2)
- GitHub Check: Cargo Audit (stable)
- GitHub Check: Cargo Audit (stable)
🔇 Additional comments (26)
.gitignore (1)
31-34: LGTM — Playwright artifacts correctly excluded.The ignore patterns are properly placed and standard for Playwright test output. The comment header maintains consistency with the existing structure.
crates/echo-session-ws-gateway/assets/vendor/open-props.LICENSE (1)
1-21: License attribution correct.Standard MIT license properly attributed to Adam Argyle for the vendored Open Props library. This satisfies license compliance requirements for the offline CSS baseline.
docs/assets/collision/diagrams.css (1)
61-61: LGTM — Hidden state handling for PiP images.The rule correctly implements visibility control for picture-in-picture images. Selector specificity and placement are appropriate.
crates/echo-session-ws-gateway/assets/vendor/theme.dark.switch.min.css (1)
1-1: Dark theme tokens properly configured.The vendored dark theme CSS correctly defines color scheme variables with appropriate selectors for multiple theme-switching patterns (
[data-theme=dark],.dark,.dark-theme). Token naming aligns with Open Props conventions and complements the light theme asset.crates/echo-session-ws-gateway/assets/vendor/open-props.min.css (1)
1-1: Disregard Biome's linear gradient warnings — unequivocal false positives.The
to bottom rightandto <side-or-corner>syntax for CSSlinear-gradient()is explicitly valid per W3C CSS Images Module Level 3 specification. This is not a stylistic preference or legacy syntax — it's the standard directional form and works in all modern browsers.This is the official Open Props library (confirmed by MIT LICENSE from Adam Argyle). Never modify vendored files. Period. Preservation of the upstream structure is non-negotiable for maintenance and update paths.
Biome's
noInvalidDirectionInLinearGradientcheck is malfunctioning on this vendored CSS. Implement one of these immediately:
- Add
crates/echo-session-ws-gateway/assets/vendor/to a.biomeignorefile- Disable the rule explicitly in
biome.jsonfor CSS under vendor paths- Suppress the rule globally for CSS if acceptable for your project
Do not rewrite the CSS.
docs/guide/wvp-demo.md (1)
99-120: No issues found; file references and configuration are correct.Both the screenshot asset (
docs/assets/wvp/session-dashboard.png) and the Playwright test file (e2e/session-dashboard.spec.ts) are present in the repository. The gateway port8787and socket path/tmp/echo-session.sockmatch their actual defaults in the codebase. The documentation is accurate.crates/echo-session-ws-gateway/assets/vendor/buttons.min.css (1)
1-1: No issues detected. All vendor CSS dependencies are properly vendored, the load order is optimal (normalize→open-props→buttons→ theme switches), and the minified file is syntactically complete.crates/echo-session-ws-gateway/assets/vendor/theme.light.switch.min.css (1)
1-1: LGTM — vendored theme asset.Light-theme switch CSS aligns with the dark counterpart. License verification requested in the normalize.min.css comment applies here as well.
e2e/collision-dpo-tour.spec.ts (1)
39-41: LGTM — defensive selector refinements.Filtering
.ruleby child.pagerpresence, asserting visibility, and disambiguating the button with.first()all reduce test flakiness. These are exactly the right adjustments for carousel interaction tests.crates/echo-session-ws-gateway/Cargo.toml (2)
20-22: LGTM — dependency additions for metrics/observer support.
echo-session-protofor protocol types andserdewith derive for serialization are appropriate for the new/api/metricsendpoint and hub observer structs.
7-7: Retract this concern—rust-version = "1.90.0"is intentional and consistent across the entire workspace.The version is not a typo or misconfiguration. It appears in 13 crate manifests, the root Cargo.toml, and matches the channel specified in
rust-toolchain.tomlexactly. This is a deliberate, forward-looking MSRV (Minimum Supported Rust Version) that the entire workspace is configured for. The pre-commit hook will verify toolchain consistency automatically.Likely an incorrect or invalid review comment.
docs/decision-log.md (1)
9-11: LGTM — decision log entries are well-documented.Clear context, rationale, and consequences for the Open Props adoption, Playwright smoke tests, and embedded dashboard decisions. These provide excellent traceability for the Demo 1 scope.
e2e/session-dashboard.spec.ts (3)
64-97: LGTM — well-structured process lifecycle management.Output capture with truncation, graceful SIGINT with SIGKILL fallback, and labeled output aggregation are all solid patterns for test process orchestration. The cleanup is properly defensive.
144-172: LGTM — sequential builds to avoid Cargo lock contention.Building crates upfront before spawning long-lived services is the right call. This prevents multiple
cargoinvocations from fighting over the same target directory locks.
245-252: LGTM — error augmentation with process output is excellent for debugging.Appending hub and gateway output to thrown errors makes CI failures actionable without manual log retrieval. This is the kind of DX detail that separates maintainable tests from debugging nightmares.
crates/echo-session-ws-gateway/assets/vendor/normalize.min.css (1)
1-1: License coverage confirmed—no action required.The
open-props.LICENSEfile (MIT) in the vendor directory properly coversnormalize.min.css. Vendored minified CSS files appropriately lack inline SPDX headers when accompanied by a LICENSE file in the same directory. Attribution requirements are satisfied.crates/echo-session-ws-gateway/assets/dashboard.html (2)
1-13: LGTM — License and document structure.License header present, HTML5 doctype correct,
lang="en"attribute for accessibility, viewport meta for responsiveness. The asset references align with the server-side handlers.
974-981: LGTM — Graceful degradation for D3.D3 loads asynchronously without blocking the main tick loop. Header metrics render regardless of chart availability. The
onerrorhandler provides appropriate console feedback.crates/echo-session-ws-gateway/src/main.rs (8)
1-4: LGTM — License header and module docs conform to coding guidelines.SPDX identifier and copyright notice present. Module-level
//!doc explains purpose.
100-108: LGTM — Metrics structs.Internal structs with appropriate derives.
MessageCountersusesCopyfor cheap cloning in snapshot.Defaultenables zero-initialization.
209-218: LGTM — Connection allocation with wrapping arithmetic.
wrapping_addonnext_conn_idandtotal_connectionsis intentional for monotonic counters.saturating_addonactive_connectionsprevents overflow. Connection ID collision after 2^64 allocations is not a practical concern.
661-668: Subscribe loopcontinuesilently ignores failures and attempts remaining warps.When
hub_observer_send_subscribefails, the error is logged and recorded, butcontinueproceeds to the nextwarp_id. This means partial subscription states are possible — the observer may be subscribed to warps 2 and 3 but not 1.If the intent is "fail-fast and reconnect," use a labeled break:
+ 'subscribe: for warp_id in &warps { - for warp_id in &warps { if let Err(err) = hub_observer_send_subscribe(&mut writer, *warp_id).await { warn!(?err, warp_id, "hub observer: subscribe send failed"); let mut metrics = state.metrics.lock().await; metrics.observe_hub_observer_error(err.to_string()); - continue; + break 'subscribe; } }If partial subscriptions are acceptable, add a comment clarifying the design choice.
838-841:Access-Control-Allow-Origin: *on/api/metricsexposes metrics to any web page.While convenient for development, this allows any website to read gateway metrics (connection counts, peer addresses, warp IDs). If
metrics.connections[].peeris considered sensitive (e.g., internal IPs), consider restricting CORS or adding an opt-in flag.
827-830: LGTM — Mutex lock scope is minimal.The lock is held only for
snapshot()computation. Serialization viaJson(snapshot)occurs after the lock is released. No risk of holding the mutex across async I/O.
1145-1149: LGTM — Connection cleanup after task termination.
remove_connis called after all tasks are joined/aborted, ensuring no dangling references inwarps.subscribersorwarps.publishers.
1235-1310: LGTM — Existing tests unchanged.Frame validation tests remain comprehensive. Consider adding unit tests for
GatewayMetricsmethods (alloc_conn, remove_conn, observe_message) as a follow-up if metrics behavior becomes critical.
Playwright's Session Dashboard smoke test expects to build and run a tiny publisher to generate deterministic snapshot+diff traffic. Add echo-session-client/examples/publish_pulse.rs and log the follow-up in docs.
Record that the Playwright Session Dashboard smoke test can now build/run from a clean checkout (publish_pulse example added).
Address review feedback: add observer backoff on handshake/subscribe failures, improve dashboard a11y and resiliency (drawer inert, metrics fetch timeout, in-flight guard), vendor D3 locally with SPDX-skip for vendor assets, and make the Playwright smoke test fail loudly when metrics never show warp activity.
|
@coderabbitai Addressed the remaining review feedback in commit a45d813. Please re-review the latest commit. |
Rate Limit Exceeded@flyingrobots have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 38 seconds before sending another message. |
|
@coderabbitai FYI the CodeRabbit status is currently failing with 'Review rate limit exceeded'. When capacity resets, please re-run review on the latest head commit. |
|
@flyingrobots: Understood. I'll re-review the latest commit now that capacity has reset. ✅ Actions performedReview triggered.
|
|
@coderabbitai Please re-review the latest commit when capacity allows and clear the "review rate limit exceeded" status. |
|
@flyingrobots: I'll re-review the latest commit now. Triggering a fresh review of the PR. ✅ Actions performedReview triggered.
|
Implements the embedded dashboard baseline for Demo 1:
echo-session-ws-gateway./dashboard+/api/metricsand run a built-in hub observer (UDS subscriber) so metrics work even when the gateway is not in the data path.ECHO_CAPTURE_DASHBOARD_SCREENSHOT=1.Closes #168