Repository navigation
refactor(http): build the web UI with Vite and embed it gzipped in the firmware - #68
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (3)
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No confirmed issue from this change currently prevents merging after normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 4.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 12 files. (6 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary change: building the web UI with Vite and embedding its gzipped output in firmware. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
Full details: Docstring Coverage
-
-
Explanation
Docstring coverage is 4.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 12 files. (6 skipped: 6 unsupported.)
-
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
I like this direction... embedding the gzipped UI in the firmware feels like a cleaner fit for Tiny Engineer and removes the firmware/LittleFS version-coupling problem entirely. Since #60 has now been squash-merged, could you rebase this branch onto the new One small thing I noticed while reviewing the build script: Other than that, the approach looks solid to me. |
Vite builds ui/ into one self-contained page (vite-plugin-singlefile), and scripts/embed_ui.py gzips it into a generated header that the firmware serves with Content-Encoding: gzip. The UI always matches the firmware it ships in, so LittleFS only holds audio again: the /ui/* routes, the version stamp and check, and the filesystem-first OTA order are gone. The pre-script skips Node when ui/ is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUaQRAXtYTdrXQ4C4us2V5
Busy state re-enables only the controls it disabled, so the wizard no longer re-renders to undo it. Servo range changes notify the servo page and the calibration step instead of every caller refreshing both, and the servo_mins/servo_maxs format lives beside the ranges. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUaQRAXtYTdrXQ4C4us2V5
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
cb53e8e to
87e9754
Compare
|
Rebased onto |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (20)
ui/src/servo-ranges.js (1)
13-22: LGTM!Also applies to: 34-37
ui/src/servo.js (1)
2-2: LGTM!Also applies to: 12-16, 26-31, 40-40, 50-50, 88-90
ui/src/setup/calibration.js (1)
2-7: LGTM!Also applies to: 128-130
ui/src/setup/wizard.js (1)
4-4: LGTM!Also applies to: 32-36, 95-95, 108-108, 130-130
ui/src/shell.js (1)
158-158: LGTM!ui/src/status.js (1)
6-6: LGTM!Also applies to: 24-26, 28-28
scripts/embed_ui.py (2)
35-36:npm cidecision can miss a changedpackage.json.The marker check compares only
package-lock.jsonagainstnode_modules/.package-lock.json. If a developer editspackage.jsonand the lockfile together, the lockfile mtime rises and the check works. If the lockfile is unchanged butnode_modulesis missing a dependency, the build fails atnpm run build. The failure is clear and recoverable, so no change is required. Keep this in mind if CI cachesnode_modules.
30-31: 🎯 Functional CorrectnessStale-header check does not cover all possible Vite inputs.
The checked-in UI currently contains no
ui/public/directory or extra top-level build inputs. The listed source paths cover the files present in this change, so the comment’s examples do not establish a current defect. The concern is conditional on adding additional files that Vite reads in the future.ui/package.json (1)
27-27: LGTM!ui/vite.config.js (1)
3-3: LGTM!Also applies to: 25-27, 35-35
platformio.ini (1)
42-42: LGTM!scripts/audio_pack.py (1)
278-278: LGTM!Also applies to: 284-284, 287-287
AGENTS.md (2)
14-14: LGTM!
58-58: 📐 Maintainability & Code Quality
AGENTS.md:58already identifiesui/index.htmlas the HTML index and lists the endpoint documentation there. The stale-path concern is contradicted, and no wording change is needed for this file.docs/flash.md (1)
44-44: LGTM!Also applies to: 58-58, 122-123, 130-130
docs/settings.md (1)
94-94: LGTM!src/http/index_page.cpp (1)
3-8: LGTM!docs/api.md (1)
9-9: LGTM!Also applies to: 27-27
src/http/json.cpp (1)
49-49: LGTM!scripts/ota.py-36-36 (1)
36-36: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Add a timeout to the
subprocess.runcalls.Both calls run external tools with no
timeout. If the OTA transfer or thebuildfsrun hangs, the PlatformIO target blocks forever. The old code had a bounded reboot wait. The new code has no bound.Add a generous timeout, such as 300 seconds for the OTA upload. Use a longer timeout for the
buildfsrun. Catchsubprocess.TimeoutExpiredand exit with a clear message.The ast-grep and Ruff command-injection hints are false positives here. The command list holds no shell string. The
TINY_ENGINEER_URLhost comes from the local developer environment and is passed as a single argument.Proposed fix for the OTA upload call
- subprocess.run(command, check=True) + try: + subprocess.run(command, check=True, timeout=300) + except subprocess.TimeoutExpired: + raise SystemExit("OTA upload timed out after 300 s")Based on learnings: flag missing
timeoutarguments onsubprocess.run()calls, recommend a sensible default such as 300 s, and handlesubprocess.TimeoutExpired.Also applies to: 46-49
Sources: Learnings, Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0f4a43ab-a01a-49e0-8b92-e303c368d4d3
⛔ Files ignored due to path filters (2)
partitions.csvis excluded by!**/*.csvui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.gitignoreAGENTS.mddocs/api.mddocs/flash.mddocs/hardware/testing.mddocs/settings.mdplatformio.iniscripts/audio_pack.pyscripts/copy_assets.pyscripts/embed_ui.pyscripts/firmware_version.pyscripts/ota.pyscripts/ui_pack.pysrc/http/index_page.cppsrc/http/index_page.hsrc/http/json.cppsrc/http/routes.cppui/index.htmlui/package.jsonui/src/servo-ranges.jsui/src/servo.jsui/src/settings.jsui/src/setup/calibration.jsui/src/setup/wizard.jsui/src/shell.jsui/src/status.jsui/vite.config.js
💤 Files with no reviewable changes (9)
- ui/index.html
- src/http/index_page.h
- docs/hardware/testing.md
- ui/src/settings.js
- scripts/firmware_version.py
- scripts/ui_pack.py
- scripts/copy_assets.py
- src/http/routes.cpp
- .gitignore
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
|
@coderabbitai re |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -t f '^ota\.py$|^espota\.py$|^platformio\.ini$'
ast-grep outline scripts/ota.py --items all
nl -ba scripts/ota.py | sed -n '1,180p'
rg -n -C3 'platform|espota|ota|extra_scripts' platformio.iniLength of output: 4612
The socket timeouts you described address stalled OTA communication. They are not a total upload deadline, but a separate subprocess deadline is not required for the concern I raised. I withdraw the timeout recommendation. ✏️ Learnings added
You are interacting with an AI system. |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks for the follow-up! the LittleFS cleanup ( CI looks good on my side. Merging... |
What
Alternative to #60 with the same frontend work, but the page ships inside the firmware instead of on LittleFS. Pick one of the two.
src/http/index_page.cppintoui/(ES modules, Vite, Vitest), exactly as in refactor(http): serve web UI from LittleFS, bundled with Vite #60.vite-plugin-singlefilebuilds one self-contained, minifiedindex.html.scripts/embed_ui.py(a PlatformIO pre-script) gzips it into a generated header,$BUILD_DIR/generated/web_ui.h, and the firmware serves those bytes withContent-Encoding: gzipstraight from flash./ui/*routes, no version stamp or check, and-t ota/-t otafs/ LittleFS work exactly as onmain(audio only).main.embed_ui.pyskips Node when nothing underui/changed, souploadfs,otafsand no-op builds do not rebuild the UI; an identical result does not recompileindex_page.cpp.Checks
type(scope): summarytype!:plus aBREAKING CHANGE:footer — none; routes and responses are unchangedpio runand/orpio test -e nativewhen that code changed —pio run(default,ota,expression-demo),pio test -e native,python3 scripts/test_audio_pack.py,npm run lint --prefix ui,npm test --prefix uinpm test --prefix packages/tiny-engineer-cursor/…/tiny-engineer-antigravity/…/tiny-engineer-claude-codewhen that package changed — N/A, no package changesui/index.html) +docs/api.md(and README /docs/hardware/testing.mdif the route list changed) — route list unchanged; the HTML index now lives inui/index.htmldocs/settings.md; no rawaccess_tokenin logs — no new settings;docs/settings.mdweb UI section points atui/.f3d+ exported.3mf; CERN-OHL-S;AiEmblem.3mfnot used as a branding swap — N/Ainclude/pins.h+docs/hardware/together — N/A.env, tokens, or Wi-Fi passwords in logs or screenshotsPCB (if this PR changes a board)
N/A — no board changes.
Summary by CodeRabbit