Skip to content

feat(desktop): browser-rendered windowed desktop over WebUSB / Web Serial (component, web app, example) - #839

Merged
finger563 merged 32 commits into
mainfrom
feat/desktop
Oct 4, 2026
Merged

finger563 merged 32 commits into
mainfrom
feat/desktop

Conversation

@finger563

Copy link
Copy Markdown
Contributor

A TinyDesk-style windowed desktop for espp devices, rendered entirely in the browser: the firmware describes apps, windows and widgets; the web app draws them and streams input back over the espp stream_frame framing + dispatcher routing (module 9, espp.desktop v1), on WebUSB and Web Serial.

Component components/desktop

  • include/detail/desktop_protocol.hpp — host-buildable wire codec. Little-endian; [tag u8][len u16] records (unknown tags skipped); payloads split across frames at max_payload() (never truncated). Host→device: GET_DESKTOP, LAUNCH_APP, CLOSE_WINDOW, WINDOW_EVENT, WIDGET_EVENT (Click / Change / Submit / chunked Text / Select / Activate / Key / Scroll), DIALOG_RESULT. Device→host: DESKTOP (snapshot + unsolicited), WINDOW_OPEN (Snapshot flag on replay; large trees continue in WIDGET_ADD), WINDOW_CLOSE, WIDGET_SET (batched diffs; widget id 0 = the window: title / flags / geometry / focus), WIDGET_ADD, WIDGET_REMOVE, DIALOG / DIALOG_CLOSE, NOTIFY, OK, ERROR ([request][errno][msg]). Replies echo the correlation id; events carry none.
  • Widgets: a box model (Column / Row / Group containers with per-widget weight + layout bits → CSS flexbox in the browser) and Label, Button, Checkbox, TextBox, TextArea (console flags ReadOnly | Monospace | WantKeys | AutoScroll | Ansi — the hook for a later Terminal app), List, Table, Select, Progress, Slider, Separator, Spacer.
  • include/desktop.hpp — espp::Desktop: ONE per device, owns apps / windows / widgets, a dirty tracker that coalesces updates into WIDGET_SET frames every flush_period (50 ms), window timers on one due-time heap, dialogs (message_box / input_box) and notify. TinyDesk-style C++ API with value handles (Window, Widget): win.label(...), win.button("+1", on_click), widget.set_text("Count: {}", n), … All app callbacks run on the desktop task with no lock held; any task may call mutators. Threading rules documented in the header.
  • include/desktop_service.hpp — espp::DesktopService, the per-transport dispatcher module (decode + validate; malformed → ERROR; everything else queued to the desktop task). Events are broadcast to every attached sink.
  • include/console_capture.hpp — espp::ConsoleCapture: a write-only VFS device stdout/stderr are redirected to; it tees every write to the real console and a byte ring, so ESP_LOG, espp::Logger and printf output all reach the Log Viewer without losing the UART console (same mechanism as UsbDevice::route_console_to_cdc).
  • test/desktop_host_test.cpp + test/desktop_vectors.txt — golden byte vectors shared with the browser test; every-prefix truncation, frame splitting, text reassembly, dirty coalescing and flush ordering.

Web app components/desktop/web/desktop.html

Single file, no dependencies; shares the byte-identical connection / navigation / hand-back blocks with the other consoles (auto-connect, auto-reconnect, Device Hub hand-off). Desktop icons, start menu, taskbar with window buttons and clock, draggable / resizable / minimisable / maximisable windows with z-order and keyboard focus, modal dialogs, toasts, a frame-log drawer; window placement remembered per app + title in localStorage; link loss dims the desktop and reconnect resyncs through GET_DESKTOP. web/test/desktop_codec_test.js (pure block vs the shared vectors, 17 suites), registered in the console lints, plus web/test/smoke.sh (headless Chrome self-test with a fake transport driving every frame type and synthetic drag / resize / dialog interaction).

Example components/desktop/example (ESP32-S3, USB vendor + CDC, PID 0x0d38)

Standard services (System, Monitor, OTA, CoreDump) + Desktop on both transports, discovery served. Apps: Counter (the ~40-line API reference), About, System Monitor (heap gauges), Task Manager (task table), Log Viewer (ConsoleCapture → ANSI console), Files + Editor (LittleFS browse / create / rename / delete, edit text files; chunked text both ways), Settings (NVS: nickname, theme, accent, log capture). LittleFS partition added; CONFIG_DESKTOP_EXAMPLE_LOG_CAPTURE Kconfig.

A follow-up PR adds the Kconfig-gated hardware apps (CANopen / DS402 with the simulated node, I2C scanner, Network). Docs: doc/en/desktop/*, the web apps page, the dispatcher module tables; CI matrix entry for the example.

Verification

  • IDF_COMPONENT_MANAGER=0 idf.py build (esp32s3): clean, 54 % app partition free.
  • Host test ALL TESTS PASSED; desktop_codec_test.js 17 passed; resolve_module_id_test.js + apps_registry_test.js pass; smoke.sh PASS ×3.
  • Not yet tested on hardware; README screenshots to be added after.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

finger563 and others added 6 commits October 3, 2026 22:55
…st and golden vectors

The contract for the browser-rendered desktop (`espp.desktop` v1, dispatcher
module 9): detail/desktop_protocol.hpp (host-buildable codec with
frame-splitting encoders for every device->host message and decoders for
both directions), detail/desktop_model.hpp (ESP-free apps / windows / widget
trees / dialogs, the DirtyTracker coalescing rules and the chunked-Text
reassembler), test/desktop_vectors.txt (shared fixture, one vector per
message type, generated by the host test's --gen) and
test/desktop_host_test.cpp.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
… example with the core apps

espp::Desktop (apps, windows, widget handles, dialogs, notifications, timers,
sinks; every app callback on the desktop task, changes coalesced and flushed
per period), espp::DesktopService (module 9 dispatcher adapter: decode +
validate, hand over to the desktop), espp::ConsoleCapture (stdout/stderr tee
into a byte ring through a write-only VFS device), and the USB example with
the Counter, About, System Monitor, Task Manager, Log Viewer, Files + Editor
and Settings apps next to the standard System / Monitor / OTA / CoreDump
services. Builds for esp32s3 with the component manager off.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
… Doxyfile inputs and CI entry

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
…t, headless smoke test

components/desktop/web/desktop.html renders a windowed desktop entirely in the
browser from the widget tree an espp::DesktopService streams over WebUSB /
Web Serial (stream_frame framing, dispatcher module 9, `espp.desktop` v1):
app icons + start menu + taskbar, a window manager (pointer drag / resize
with clamping, z-order and focus, minimise / maximise / restore, F6 cycling,
modal windows, stored geometry per app + title), every widget type of the
protocol (flex box model, console-mode TextArea with an ANSI line ring, key
events, chunked Text events, lists / tables / selects by item range), dialogs
with a focus trap, notifications as toasts, a collapsible device bar, a frame
log drawer and link-loss dimming with resync on reconnect. The shared console
blocks (transports, auto-connect / reconnect, discovery, navigation, hand-back)
are byte-identical copies of the system console's.

The pure codec / geometry / layout logic sits between DESKTOP:BEGIN-PURE /
END-PURE markers and is tested by web/test/desktop_codec_test.js against the
shared golden vectors (decode == JSON for every d2h vector, encode == hex for
every h2d one, truncation rejected at every prefix, unknown tags skipped);
web/test/smoke.sh drives the page in headless Chrome through a ?selftest=1
hook (fake transport, the d2h vectors, synthetic drag / resize / minimise /
dialog answers). The page is registered with the dispatcher web lints and
listed in doc/en/web_apps.rst.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
…ded one)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
Copilot AI balanced review requested due to automatic review settings October 4, 2026 04:39
@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

✅Static analysis result - no issues found! ✅

…ders and host test

Rename the locals / arguments that shadowed member functions (id, launch,
widget, app, items), replace the lookup / collect loops with find_if /
transform / copy_if / move / min_element / accumulate, drop the dead
`room` reassignment in WidgetTreeWriter::add, make two pointers const,
initialise the test's Vector members, and split the deliberate
"second unregister returns false" check into two CHECKs. One inline
suppression with a reason: set_sink_active mutates the sink through its
shared_ptr, so it must not be const.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Snapshot delivery, filesystem input handling, protocol bounds, concurrency, publishing, and CI issues remain unresolved.

Review effort: Balanced
Findings: 8 High severity · 9 Medium severity · 1 Low severity

Open (18)
What changed in this PR

Adds a browser-rendered desktop subsystem, WebUSB/Web Serial client, ESP32-S3 demonstration firmware, tests, and documentation.

Changes:

  • Introduces the desktop protocol, retained model, transport service, and console capture.
  • Adds the browser desktop and example applications.
  • Integrates documentation and example-build CI.
File Description
.github/​workflows/​build.yml Builds the desktop example.
components/​desktop/​CMakeLists.txt Registers component dependencies.
components/​desktop/​README.md Documents APIs and protocol.
components/​desktop/​idf_component.yml Defines component metadata.
components/​desktop/​include/​console_capture.hpp Adds console log capture.
components/​desktop/​include/​desktop.hpp Implements the public desktop API and task.
components/​desktop/​include/​desktop_service.hpp Adds dispatcher transport integration.
components/​desktop/​include/​detail/​desktop_model.hpp Implements retained state and dirty tracking.
components/​desktop/​include/​detail/​desktop_protocol.hpp Defines the wire codec.
components/​desktop/​test/​desktop_host_test.cpp Tests the host-side codec and model.
components/​desktop/​test/​desktop_vectors.txt Provides shared protocol vectors.
components/​desktop/​web/​desktop.html Implements the browser desktop.
components/​desktop/​web/​test/​desktop_codec_test.js Tests browser codec and UI helpers.
components/​desktop/​web/​test/​smoke.sh Adds a headless browser smoke test.
components/​desktop/​example/​CMakeLists.txt Configures the example project.
components/​desktop/​example/​README.md Documents the example.
components/​desktop/​example/​partitions.csv Adds OTA, coredump, and LittleFS partitions.
components/​desktop/​example/​sdkconfig.defaults Configures ESP32-S3 USB and monitoring.
components/​desktop/​example/​main/​CMakeLists.txt Registers example dependencies.
components/​desktop/​example/​main/​Kconfig.projbuild Adds log-capture settings.
components/​desktop/​example/​main/​desktop_example.cpp Wires transports and services.
components/​desktop/​example/​main/​apps/​about_app.hpp Adds device information UI.
components/​desktop/​example/​main/​apps/​counter_app.hpp Adds the reference counter app.
components/​desktop/​example/​main/​apps/​files_app.hpp Adds file browsing and editing.
components/​desktop/​example/​main/​apps/​log_viewer_app.hpp Adds live log viewing.
components/​desktop/​example/​main/​apps/​settings_app.hpp Adds persistent desktop settings.
components/​desktop/​example/​main/​apps/​system_monitor_app.hpp Adds heap and uptime monitoring.
components/​desktop/​example/​main/​apps/​task_manager_app.hpp Adds task monitoring.
components/​dispatcher/​README.md Documents desktop module ID 9.
components/​dispatcher/​web/​test/​resolve_module_id_test.js Registers the desktop web console.
doc/​Doxyfile Adds desktop API and example sources.
doc/​en/​index.rst Adds desktop documentation navigation.
doc/​en/​web_apps.rst Describes the hosted desktop app.
doc/​en/​desktop/​index.rst Adds the desktop documentation section.
doc/​en/​desktop/​desktop.rst Documents desktop behavior and APIs.
doc/​en/​desktop/​desktop_example.md Includes the example guide.
doc/​en/​dispatcher/​dispatcher.rst Adds the desktop protocol assignment.
doc/​en/​dispatcher/​custom_modules.rst Documents desktop discovery metadata.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread components/desktop/example/main/apps/files_app.hpp Outdated
Comment thread components/desktop/example/main/apps/files_app.hpp Outdated
Comment thread components/desktop/example/main/apps/settings_app.hpp
Comment thread components/desktop/include/desktop.hpp Outdated
Comment thread components/desktop/include/desktop.hpp
Comment thread components/desktop/include/desktop.hpp
Comment thread components/desktop/include/detail/desktop_model.hpp
Comment thread components/desktop/include/detail/desktop_protocol.hpp Outdated
Comment thread components/desktop/web/desktop.html
Comment thread doc/Doxyfile
finger563 and others added 3 commits October 4, 2026 00:02
… share its batch

Frames parsed from one byte batch are dispatched synchronously, but the
GET_DESKTOP transaction applied its reply from refreshDesktop() after an
await, i.e. as a microtask AFTER the whole batch: a Snapshot WINDOW_OPEN
right behind the DESKTOP reply was opened while `apps` was still empty, so
its geometry was looked up under "app <id>" instead of the real app name and
the stored placement was not restored. transact() now takes an onReply hook
that dispatchFrame runs synchronously when the matching reply arrives
(before resolving the promise); refreshDesktop() applies the DESKTOP state
there. The self-test delivers the DESKTOP reply and the snapshot WINDOW_OPEN
in ONE handleRxBytes batch with a pre-stored geometry for the window's real
app name + title and asserts it is restored (fails without the fix).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
- Editor: a file larger than the TextArea bound opens read-only (head shown,
  no Save), and Reload never offers a truncated tail; Files: dialog names are
  validated as a single path component before create / rename.
- Settings: bounds-check the host-supplied Select index.
- Desktop: command queue overflow refuses a request with ERROR(EAGAIN) and
  drops an event (nothing queued is evicted; max_queued_commands >= 1);
  max_frame_bytes clamped to kMinFrameBytes; replies to LAUNCH_APP /
  CLOSE_WINDOW are staged under the lock and sent after it; the sink send
  returns whether the frame was queued and a drop flags the sink as needing a
  resync (DesktopService::needs_resync); registry limits (kMaxApps, name /
  icon / description / device name / firmware byte caps) keep DESKTOP one
  frame, encode_desktop trims rather than overflow a smaller cap; dialogs and
  notifications are single frames refused at the API instead of truncated.
- Model: the TextArea line bound now matches the browser ring (the text after
  the last newline is a line); a byte-bound trim sends a full Text
  replacement so both sides hold the same text.
- ConsoleCapture: stderr freopen failure rolls back; the tee fd is held open
  under its own mutex across write().
- Example: send through write_vendor / write_cdc (bounded wait, all-or-nothing)
  instead of a FIFO preflight that dropped snapshot frames.
- CI: desktop_tests.yml runs the C++ host test, the node codec test, the
  dispatcher web lints and the headless-Chrome smoke test; components/desktop
  added to upload_components.yml; Doxyfile entries in alphabetical order.
- README / docs updated for the changed behaviour.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Protocol size handling, snapshot reconciliation, and modal layering contain correctness issues that can desynchronize or block the desktop UI.

Review effort: Balanced
Findings: 3 High severity · 5 Medium severity

Open (8)
Resolved since last review (18)

Comment thread components/desktop/include/desktop.hpp
Comment thread components/desktop/include/detail/desktop_protocol.hpp Outdated
Comment thread components/desktop/web/desktop.html Outdated
Comment thread components/desktop/include/console_capture.hpp Outdated
Comment thread components/desktop/include/detail/desktop_model.hpp
Comment thread components/desktop/include/detail/desktop_model.hpp Outdated
Comment thread components/desktop/include/detail/desktop_protocol.hpp
Comment thread components/desktop/include/detail/desktop_protocol.hpp
finger563 and others added 4 commits October 4, 2026 00:25
…window list is not authoritative

A kWinModal window got the same inline z-index as every other window
(zTop, starting near 10), which overrode the `.window.modal` class rule and
left it BEHIND the #modal backdrop (z-index 10000), unclickable. Windows now
keep a band-less order (w.z) and applyZ() stamps the inline value as that
order plus MODAL_Z_BAND (20000) for Modal windows; the backdrop sits at
19999, plain windows below it, and the taskbar / start menu / toasts above
everything (30000+). The self-test opens a kWinModal WINDOW_OPEN, asserts
its computed z-index is above the backdrop's and that a hit test at its
button lands on the button, clicks it and checks the Click event (fails
with the band disabled: "19 vs 19999").

DESKTOP flags bit1 (DESKTOP_WINDOWS_TRUNCATED, 0x02) marks an incomplete
window list: applyDesktop() only reconciles (drops unlisted local windows)
when the list is complete and otherwise keeps them; the self-test feeds an
unsolicited DESKTOP with the bit set and an empty list (window kept), then
a complete empty one (window dropped). The codec test checks the constants
and that the flags byte decodes as-is.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
- unregister_app closes the app's windows (WINDOW_CLOSE reason Shutdown, on_close
  posted to the desktop task); app ids are allocated monotonically and never
  reused while an app or an open window references them.
- DESKTOP: new flags bit1 WindowListComplete (set by the encoder unless it had
  to trim the window list); trimming order is now descriptions, then apps, then
  the window list last. kMinPayloadBytes (256) is derived so the maximal
  record set (kDesktopRecordsMaxBytes, static_assert) always fits, and
  Desktop::kMinFrameBytes follows it; theme is validated (auto/light/dark).
- Dialogs / notifications are checked for representability (title and buttons
  <= 255 B, <= 255 buttons, text / default <= 65535 B) before encoding.
- Unsplittable widget values are bounded at the API: window Title, Placeholder,
  Tooltip <= 255 B; Columns <= 255 names of <= 255 B fitting a frame; every
  Items entry small enough for a frame -- validated on the strings (new
  Model::set_columns / set_items, the Widget helpers route through them) so a
  Prop builder can never cut them; create_window / add_widget / set_prop refuse
  and log.
- ConsoleCapture strip_ansi keeps a lone ESC that does not start a CSI.
- files_app: none_of instead of the raw loop (cppcheck).
- Fixture regenerated (desktop vector carries flags 3; new
  desktop_window_list_trimmed vector); host test covers trimmed vs complete,
  the minimum cap, representability rejections and the id-reuse rule.
- README / docs updated.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
…es the firmware)

The web fix assumed bit1 = truncated; the protocol header defines
kDesktopWindowListComplete = 0x02 (set when the list is intact). Reconcile
local windows only when the bit is set; selftest and codec test updated to
the golden vectors (complete: flags 3, trimmed: flags 1).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Transport lifecycle, protocol validation, item bounds, and multi-client text synchronization have unresolved correctness issues.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (8)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Reset dispatcher workers on USB disconnect

components/​desktop/​example/​main/​desktop_example.cpp:191

Reset both dispatcher workers when USB disconnects (and preferably on mount as well). DispatcherWorker explicitly retains queued bytes and parser state until request_reset() is called; detaching only the desktop sinks leaves a half frame from the old USB session able to consume bytes from the next connection. The telemetry example follows this lifecycle contract at components/telemetry/example/main/telemetry_example.cpp:149-157.

Medium severity Key text assembly by source sink

components/​desktop/​include/​desktop.hpp:1384

Text assembly must also be keyed by the source sink. The example exposes the same Desktop through vendor and CDC services, so two attached clients can send chunks for the same (window, widget); one client's offset-0 chunk currently replaces the other's buffer, and subsequent chunks are rejected or can be combined under one shared total. Carry Command::sink into TextAssembler::feed, include it in the key, and clear that sink's partial assemblies on detach.

Medium severity Reject unknown window-event kinds

components/​desktop/​include/​detail/​desktop_protocol.hpp:1602

Reject unknown window-event kinds before constructing the event. Unlike decode_widget_event, this decoder accepts every byte; apply_window_event then falls through its switch and returns success, so malformed input can invoke an app's window callback as if it were valid.

Comment thread components/desktop/include/desktop.hpp Outdated
- Items ranges are validated before anything is mutated: a vector or a range
  beyond the u16 index space (start / count / ItemCount on the wire) is refused,
  and the replace-all path (new Model::replace_items) checks first so ItemCount
  is never changed for a refused range.
- The TextAssembler keys partial texts by (sink, window, widget): two attached
  hosts cannot interleave chunks into one buffer; a sink's partial assemblies
  are dropped when it detaches or is removed.
- decode_window_event rejects unknown event kinds (only 1..7 decode), so a
  window handler never sees an invalid kind.
- Example: both DispatcherWorkers get request_reset() (and the TX FIFOs are
  cleared) on USB unmount and mount, so a half frame of the old session cannot
  eat the first bytes of the next.
- Host test covers each: range / replace_items refusals leave the model
  untouched, per-sink assembly + forget_sink, unknown window event kinds.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
@finger563

Copy link
Copy Markdown
Contributor Author

Round 3 summary-only findings, all addressed in 11f4392:

  • desktop_example.cpp (worker reset on USB disconnect): both DispatcherWorkers now get request_reset() on unmount and on mount (plus vendor_write_clear() / cdc_write_clear()), following the telemetry example, so queued bytes / a half frame of the old session can never eat the first bytes of the next one.
  • desktop.hpp (TextAssembler keyed by sink): partial Text assemblies are keyed by (sink, window, widget) — Command::sink is carried into TextAssembler::feed — so a vendor and a CDC client editing the same widget never interleave; set_sink_active(id, false) (detach) and remove_sink drop that sink's partial assemblies. Host test covers interleaving and forget_sink.
  • desktop_protocol.hpp (decode_window_event): unknown event kinds are rejected (only 1..7 decode), like decode_widget_event, so apply_window_event / a window handler never sees an invalid kind. Host test covers kind 0, 8 and the boundary 7. (No fixture vector: the fixture format carries only decodable h2d vectors, so the rejection lives in the host test.)

@finger563
finger563 requested a balanced review from Copilot October 4, 2026 06:46
…pcheck no longer reports it)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The broad protocol, transport, VFS, and browser changes still contain framing, retry, persistence-key, and smoke-test correctness issues.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Failed frame retries stall desktop task and transports

components/​desktop/​include/​desktop.hpp:1500

After a failed frame the sink remains active, so this loop tries every remaining frame and every later flush keeps retrying it. In the example, each write_vendor/write_cdc attempt may wait up to 250 ms for FIFO space; one non-draining transport can therefore stall the desktop task and delay the other transport repeatedly, even though its mirror already requires a full resync. Stop the batch and deactivate the sink after the first failure; the next GET_DESKTOP already reactivates it.

Medium severity DOM grep falsely passes without verifying script initialization

components/​desktop/​web/​test/​smoke.sh:30

--dump-dom serializes the inline <script> source, which itself contains the literal espp Desktop ready near the end of desktop.html. Therefore this grep succeeds even if execution throws before reaching that statement, so neither the normal nor ?autoconnect=1 check verifies what its PASS message claims. Set a DOM attribute/element only after initialization completes and assert that rendered marker instead.

Comment thread components/desktop/include/detail/desktop_protocol.hpp Outdated
Comment thread components/desktop/include/detail/desktop_protocol.hpp
…its are bounded and resynced

- Files / Editor: a shared write_file() opens, writes, close()s and only then
  checks fail() / bad() (a buffered write reaches the medium on the final
  flush), so Save and New file report a failed write instead of "saved".
- Model: a host-originated Text (or Submit) on a TextArea is bounded by the
  same max_lines / max_text_bytes rules as device writes; when the bound
  shortened the edit, the bounded Text is marked dirty so the next flush sends
  it back and the browser's editable value resynchronises. Host test: an edit
  within the bound emits nothing; one over max_lines keeps the last max_lines
  lines and emits a full Text replacement; the byte bound likewise.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

NVS updates are not committed and modal browser UI still permits interaction outside the advertised modal context.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Counter updates are not committed to NVS

components/​desktop/​example/​main/​apps/​counter_app.hpp:30

NvsHandle::set() only stages a value; persistence requires commit() (components/nvs/include/nvs_handle_espp.hpp:330-348,418-429). Without it, counter updates can disappear on reboot despite this app claiming the count is kept in NVS. Commit after staging the new count (and handle the resulting error if desired).

Medium severity log_tee setting is not committed to NVS

components/​desktop/​example/​main/​apps/​settings_app.hpp:96

This stages log_tee but never commits it. NvsHandle::set() is explicitly non-committing, so the checkbox's state is not reliably restored after reboot as documented. Commit the update and surface a commit failure.

This issue also appears on line 110 of the same file.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

NVS writes are not committed, and oversized browser input can be silently lost or exceed protocol frame limits.

Review effort: Balanced
Findings: 6 Medium severity

Open (6)

Comment thread components/desktop/example/main/apps/counter_app.hpp Outdated
Comment thread components/desktop/example/main/apps/settings_app.hpp Outdated
Comment thread components/desktop/include/detail/console_ring.hpp Outdated
Comment thread components/desktop/include/detail/desktop_model.hpp Outdated
Comment thread components/desktop/web/desktop.html Outdated
Comment thread components/desktop/web/desktop.html Outdated
finger563 and others added 3 commits October 4, 2026 07:53
… device's MaxTextBytes

SUBMIT (TextBox Enter) and DIALOG_RESULT are single, unsplittable frames:
a value longer than MaxPayload - 5 / - 3 UTF-8 bytes made buildFrame throw
and the input was lost. Enter on an oversized TextBox now keeps the value
and reports it (toast); an oversized input-dialog answer keeps the dialog
open with an inline validation message (cleared on the next edit); a value
exactly at the bound goes out in one frame.

DESKTOP record 7, MaxTextBytes (u32 LE, default 16384), is read by
desktopSettings() (zero or malformed keeps the default; the fixture may or
may not carry it). Host edits are bounded by it: an editable TextArea is cut
on a code point boundary at MaxTextBytes while typing, a TextBox at
min(MaxTextBytes, MaxPayload - 5) so Enter always fits, both with a warning
border and a one-time toast; Text events are chunked from the bounded value.

Codec test: record 7 absent / present / zero / malformed, the UTF-8 helpers
(utf8ByteLength, truncateUtf8 never cutting a sequence), the two single-frame
bounds. Self-test: oversized dialog answer and SUBMIT are not sent (message
/ toast), values at the bound are sent as one full-size frame, TextBox and
TextArea input are capped on a code point boundary.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
… host text edits, DESKTOP MaxTextBytes

- Counter / Settings: NvsHandle::set() only stages; every write is followed
  by commit() and a failure is surfaced (label suffix / error toast).
- ConsoleRing::clear() also resets the ANSI parser state, so a pending ESC /
  CSI prefix never swallows the first bytes after a Clear (host-tested).
- Host Text edits longer than max_text_bytes are no longer rejected: the
  TextAssembler streams them keeping only the tail (last max_text_bytes),
  reports that it did, and the model stores the bounded value (UTF-8 boundary
  respected) and marks it dirty, so the next flush echoes the full bounded
  Text back and the browser resynchronises (host-tested: total 2x the bound
  -> model holds the tail, one replacement emitted).
- New DESKTOP record tag 7 MaxTextBytes (u32) advertises the bound to the
  host; protocol header, README table, kDesktopRecordsMaxBytes and the
  fixture (regenerated) updated.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Heap gauge overflow, unsafe partial-file editing, invalid color acceptance, and reserved module IDs remain unresolved.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (6)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Validate complete six-digit accent color input

components/​desktop/​example/​main/​apps/​settings_app.hpp:124

The accent field is host-controlled, but strtoul(..., nullptr, 16) accepts partial or wholly invalid input ("zzzzzz" becomes black and "12junk" becomes 0x12). Validate the complete six-digit rrggbb value before staging any NVS changes; otherwise Save reports success for a color the user did not enter.

Medium severity Prevent 32-bit overflow in heap usage gauge calculation

components/​desktop/​example/​main/​apps/​system_monitor_app.hpp:54

On ESP32, size_t is 32-bit, so used * 1000 overflows once heap usage exceeds about 4.1 MiB. For example, 6 MiB used out of 8 MiB renders as 238/1000 instead of 750/1000. Widen before multiplying so PSRAM gauges remain accurate.

Medium severity Reject reserved module IDs before registering the service

components/​desktop/​include/​desktop_service.hpp:82

Config::module documents 0x00..0xEF, but the constructor registers any byte unchanged. In particular, 0xFF collides with capability discovery and is later replaced by serve_discovery(), leaving this service's outbound frames stamped as discovery while requests never reach it. Reject reserved values before calling add_sink (the reservation is defined in components/dispatcher/include/dispatcher.hpp:117-121).

Comment thread components/desktop/example/main/apps/files_app.hpp Outdated
read_head() now reports size / open / short-read failures; on any of them
the Editor opens read-only (showing what was read) and a failing Reload
flips an open window to read-only, so Save can never truncate a file to a
partial buffer.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Zero-sized text configuration, repeated backpressure waits, and 32-bit heap-percentage overflow can produce incorrect or unreliable behavior.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Prevent overflow when calculating heap usage percentage

components/​desktop/​example/​main/​apps/​system_monitor_app.hpp:54

On ESP32-S3, size_t is 32-bit, so used * 1000 overflows once heap usage exceeds about 4.1 MiB. An 8 MiB PSRAM region can therefore display a much smaller, incorrect percentage; widen before multiplying.

Medium severity Reject zero text capacity to prevent empty firmware values

components/​desktop/​include/​desktop.hpp:182

This public bound accepts zero, but firmware then constructs a zero-capacity TextAssembler and retains empty TextArea values, while the browser treats an advertised zero as absent and uses its 16 KiB default. Thus the UI accepts edits that firmware immediately replaces with empty text. Define a positive minimum and clamp/reject zero before constructing the assembler, or make zero semantics consistent on both sides.

Medium severity Stop processing frames after the first failed callback

components/​desktop/​include/​desktop.hpp:1505

Stop after the first failed frame. The USB callbacks may wait 250 ms before returning false, so continuing through every remaining snapshot/diff frame can stall the single desktop task for 250 ms × N even though the host is already known to need a full resync and any received suffix is unusable.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved configuration, arithmetic, rename-safety, and console-rollback bugs can cause desynchronization, incorrect monitoring, or data loss.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity

Open (4)

Comment thread components/desktop/example/main/apps/files_app.hpp
Comment thread components/desktop/example/main/apps/system_monitor_app.hpp Outdated
Comment thread components/desktop/include/console_capture.hpp
Comment thread components/desktop/include/desktop.hpp Outdated
…le rollback uses the available device, max_text_bytes >= 1

- Files: Rename refuses an existing destination and reports rename errors
  instead of refreshing as if it succeeded
- System Monitor: the gauge value is computed in 64 bits (used * 1000
  overflowed a 32-bit size_t past ~4 MiB)
- ConsoleCapture: a failed install restores stdout / stderr through the
  same fallback sequence as the tee (/dev/console, console UART,
  USB-Serial-JTAG) instead of only /dev/console
- Desktop: Config::max_text_bytes is normalized to at least 1 before the
  assembler, model, accessor and advertised record use it (the browser treats
  an advertised 0 as its own default)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Filesystem failures, invalid accent input, and console-capture installation failures are currently handled incorrectly or silently.

Review effort: Balanced
Findings: None

Resolved since last review (4)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Handle directory creation failures before refreshing

components/​desktop/​example/​main/​apps/​files_app.hpp:296

create_directory can fail (full filesystem, invalid state, or an existing name), but the result is discarded and the UI refreshes as if creation succeeded. Report the failure and only refresh after a directory was actually created.

This issue also appears on line 359 of the same file.

Medium severity Validate exactly six hexadecimal digits for color input

components/​desktop/​example/​main/​apps/​settings_app.hpp:124

strtoul accepts an empty string, non-hex prefixes, and trailing junk; those inputs are silently saved/applied as black or a partial value despite the field requiring rrggbb. Validate exactly six hexadecimal digits before staging any NVS changes.

Medium severity Report Log Viewer installation failures

components/​desktop/​example/​main/​desktop_example.cpp:59

The install result is discarded, so VFS registration or freopen failure silently disables the documented Log Viewer capture. Report ec on the restored console when installation fails so this default-on feature is diagnosable.

…op icons

#windows is a full-size absolute layer after #icons in the DOM, so with
no window open it still sat over the icons and took every pointer hit
(the start menu was the only way to launch an app). The layer now passes
pointer events through except on the windows themselves. The selftest
probes an icon with elementFromPoint, which fails without the fix.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
@finger563

Copy link
Copy Markdown
Contributor Author

Hardware feedback: desktop icons did not react to clicks (only the start menu launched apps). Cause: the full-size #windows layer sits after #icons in the DOM and swallowed every pointer hit even with no window open; the programmatic click() in the selftest bypassed hit-testing. Fixed in 2ccebc9 (pointer-events: none on the layer, auto on the windows) with an elementFromPoint probe in the selftest that fails without the fix.

finger563 and others added 2 commits October 4, 2026 11:17
…anager

- Table flag bit0 Sortable (kTableSortable): the browser sorts by a clicked
  column header (ascending, descending, then the firmware's order), numeric-
  aware and stable; sorting is a view only, so Select / Activate events and the
  selection highlight keep the firmware's row index; keyboard navigation
  follows the display order; re-applied on every row update
- Task Manager: name filter (substring, case-insensitive; applies on Enter and
  blur) and core filter (all / 0 / 1 / unpinned), count shows "N of M" when
  filtering; the table is sortable
- pagehide also starts closing the transport (second listener) so a reloaded
  page can re-claim the device immediately
- selftest covers sort order, SELECT index mapping, resort on update; codec
  test covers sortedOrder / compareCells

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
… device on load and plug-in

Without a hand-off link (a plain open or a page reload) the page now opens
the single previously granted espp USB device (else the single espp serial
port) without a chooser, and does the same when one is plugged in while idle;
several candidates are never guessed. Gated by the auto-reconnect checkbox,
whose title now describes every automatic connect. The candidate choice is a
pure helper covered by the codec test; README updated.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
@finger563
finger563 merged commit 4463d49 into main Oct 4, 2026
176 checks passed
@finger563
finger563 deleted the feat/desktop branch October 4, 2026 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants