fix(gateway): only print dashboard URL when web_dist_dir is available - #7529
Conversation
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
The code change itself is clean and clearly correct β good work on the fix. One gate to clear before merge.
π΄ Blocking β Mandatory PR template sections are unfilled
The Validation Evidence, Security & Privacy, and Compatibility sections in the PR body still contain the raw template placeholder text (bullet labels without values, Yes/No without answers). These are required fields per the PR template and the definition of done.
What's needed before re-review:
Validation Evidence β paste literal tail output (or link a green CI run) for:
cargo fmt --all -- --check
cargo clippy --all-targets -- -D warnings
cargo test
CI passing is not a substitute for the author's local run; the template language says "the signal CI cannot replace."
Security & Privacy β answer each Yes/No question. For this change the answers are likely all No, but the blanks need to say that.
Compatibility β backward compatible and no CLI/config surface change? A one-liner is fine; just fill it in.
The Rollback section is low-risk so git revert <sha> is acceptable as written β that one's fine to leave short.
π’ What looks good β Change is minimal and correct
The if web_dist_dir.is_some() guard at line 1319 mirrors the distinction already made in the log path on lines 1291β1312, and the cargo web build command in the new warning matches what those log messages already say (lines 1302, 1309). The scope is exactly right: +5/-1 in one file, no collateral. The before/after output in the PR summary makes the behavior change clear.
π΅ Suggestion β Localization pass for the whole startup banner
The new println! is consistent with the bare-string style used throughout the startup banner (lines 1315+). When this section gets a Fluent pass, all the banner lines should move together β this PR isn't the place to fix half of them.
|
π @WareWolf-MoonWall β thank you for the review! The three missing PR template sections have been filled:
No code changes β the fix itself was already correct. Ready for re-review. |
When the Web dashboard is not found (no web/dist), the gateway still printed 'π Web Dashboard: http://...' at startup, which misleads users into expecting a working dashboard. Now the URL is only shown when the dashboard is available, and a clear build instruction is printed when it is not. Fixes zeroclaw-labs#7523
b50e609 to
0cc1d01
Compare
Audacity88
left a comment
There was a problem hiding this comment.
Reviewed head 0cc1d01 against the live PR body, WareWolf's prior CHANGES_REQUESTED review, the PR template validation requirements, current green CI, and the gateway startup source in crates/zeroclaw-gateway/src/lib.rs around web_dist_dir. There are no inline threads. WareWolf's block is still active, so I am leaving this as a comment rather than approving over it.
π’ What looks good β Banner now matches dashboard availability
The new guard prints the dashboard URL only when web_dist_dir resolved to an index-bearing directory. That matches the serving decision already used by the gateway and keeps route behavior unchanged.
π‘ Warning β Validation evidence still needs literal test output
The Validation Evidence section has been substantially filled, so the blank-section part of WareWolf's review is no longer the issue. cargo fmt and cargo clippy now show literal output. The remaining gap is the cargo test entry: it still says # all tests passing β CI green (Quality Gate) rather than pasting actual test output. Per the template requirement to paste literal output tails rather than βall passedβ summaries, please replace that summary with actual cargo test output, or say explicitly if the command was not run locally. Since WareWolf's active CHANGES_REQUESTED covered this template-evidence gap, that block remains valid until they convert or dismiss it.
singlerider
left a comment
There was a problem hiding this comment.
Reviewed at head 0cc1d01 against current master. The code is correct and CI is green; the remaining item is the procedural one in @WareWolf-MoonWall's review. Not approving over his active changes-requested.
π’ The guard exactly matches the real serving decision
web_dist_dir is set to Some(...) only when the path resolves to an index.html-bearing directory β auto-detect requires p.join("index.html").is_file() (line 1266) and the explicit-config arm requires explicit.join("index.html").is_file() (line 1275). So the new banner guard if web_dist_dir.is_some() prints the dashboard URL only when the dashboard will actually serve, and prints the "not available β build with cargo web build" warning otherwise. That precisely fixes the reported bug (advertising a dashboard URL that 404s when no web/dist is present), and it mirrors the distinction already made in the log path at lines 1291-1301. The cargo web build guidance matches what those log messages already say. Scope is minimal (+5/-1, one file).
On @WareWolf-MoonWall's π΄ (PR-template evidence)
Per @Audacity88's follow-up, the blank-section part is resolved β Validation Evidence is filled and cargo fmt / cargo clippy show literal output. The one remaining gap is the cargo test entry, which says "all tests passing β CI green" rather than pasted output. The Test job (plus Lint, Format, CI Required Gate) is green at this exact head, so the test signal exists; the body just needs to either paste the literal tail or state explicitly that the run was CI-only. That's for @WareWolf-MoonWall to clear since his is the active review β I'm noting the code is correct and CI covers the test concern so the substance is met.
π΅ Localization (per @WareWolf-MoonWall)
The new println! matches the bare-string style of the surrounding startup banner; when that banner gets a Fluent pass, all lines should move together β not this PR's job.
The fix is correct and verified against the serving logic; the path to merge is the cargo test evidence line. Leaving @WareWolf-MoonWall's changes-requested in force.
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
Re-review at head 0cc1d01. My CHANGES_REQUESTED from 2026-06-13 was that the mandatory template sections (Validation Evidence, Security & Privacy, Compatibility, Rollback) were unfilled. Those sections are now filled. Lifting the block and approving.
β Resolved β Mandatory template sections filled
The Validation Evidence, Security & Privacy, Compatibility, and Rollback sections have all been answered:
cargo fmtandcargo clippyare both clean with literal output.- All four security/privacy questions answered No.
- Compatibility marked backward-compatible with no CLI surface changes.
- Rollback instruction provided.
π’ What looks good β Fix is correct
if web_dist_dir.is_some() is exactly the right guard β it mirrors the condition that determines whether the web dashboard is actually being served (p.join("index.html").is_file()). The URL message is suppressed on source-build installs where no web dist is present. +5/-1 in a single, obvious location.
π‘ Warning β cargo test is a summary phrase, not pasted output
The validation evidence says "all tests passing β CI green (Quality Gate)" rather than a pasted terminal tail. The template asks for literal output. singlerider's 2026-06-15 note acknowledges that CI is green at this exact head (0cc1d01b) β I'm accepting the CI signal as sufficient here, but for future PRs the literal paste or a specific CI run SHA pointer is the expected format.
Audacity88
left a comment
There was a problem hiding this comment.
@chengzhichao-xydt thanks for sticking with this one.
I re-reviewed head 0cc1d01 against #7523, the current PR body, the one-file gateway diff, the prior WareWolf/Audacity88/singlerider review thread, the now-lifted template blocker, current green CI, and current master after #7554. The guard itself is the right fix, but the branch now needs one refresh before this is mergeable.
β Resolved β The earlier template blocker is cleared
WareWolf's original CHANGES_REQUESTED was about the required PR-template sections being blank. Those sections are now filled, and WareWolf re-reviewed the same head and approved it. I agree that the remaining cargo test line is a validation-format warning rather than a code blocker for this small banner fix, especially with the Test job green on this head.
π’ What looks good β The dashboard URL guard matches the serving decision
web_dist_dir is only Some when the configured path or one of the auto-detect candidates contains index.html, so guarding the startup dashboard URL on web_dist_dir.is_some() is the right behavior. It stops the gateway from advertising a dashboard URL when the dashboard will not actually serve.
π΄ Blocking β Refresh the fallback wording against current installer guidance
This branch is now 93 commits behind current master, and crates/zeroclaw-gateway/src/lib.rs changed on master after this PR's base. The exact merge result is clean, but it would print:
Web Dashboard: not available β build with `cargo web build`
That no longer matches the current gateway guidance from #7554. #7554 intentionally changed the dashboard-unavailable messages to point end users at the supported installer path (./install.sh --source on Linux/macOS, setup.bat on Windows) instead of telling them to run raw cargo web build. With this PR merged as-is, the startup banner would send users to the older raw-cargo instruction while GET / tells them to reinstall with the supported installer.
Please refresh the branch onto current master and update the new startup-banner fallback to match the current dashboard-unavailable guidance in static_files.rs / the nearby web_dist_dir startup logs. The guard can stay; the text just needs to align with the current installer-oriented message, then CI should rerun on the refreshed head.
singlerider
left a comment
There was a problem hiding this comment.
@chengzhichao-xydt β reviewed on its own merits, verified both factual claims against master.
π’ Correct gate. web_dist_dir: Option<PathBuf> is resolved earlier in run_gateway (explicit gateway.web_dist_dir β auto-detect of index.html) and is Some only when a dashboard is actually servable, so keying the banner line on web_dist_dir.is_some() exactly matches reality β the old unconditional print advertised a dashboard that 404s when web/dist was never built. The build hint is accurate too: cargo web is a real alias in .cargo/config.toml (run --release -p xtask --bin web --) and cargo web build is the established phrasing across the repo docs.
π’ This dovetails with the existing branch that already warns when a configured web_dist_dir is missing β it adds the complementary "not configured / not detected" case so the two messages cover the full space. Pure output change, no test surface needed.
LGTM.
|
@chengzhichao-xydt I did one more pass because there are now approvals after my earlier change request. The useful part of this PR is still correct: the merge result cleanly keeps the guard so the dashboard URL only prints when
With this branch merged as-is, startup would print a second unavailable-dashboard line that still says: That puts the startup banner back out of sync with the current installer-oriented guidance. Please refresh the branch against current |
The startup banner's unavailable-dashboard line still pointed users at `cargo web build`, which no longer matches the installer-oriented wording used by the 503 handler and the gateway INFO logs. Update the fallback println to reference the supported installer paths (`./install.sh --source` on Linux/macOS, `setup.bat` on Windows) so the startup banner stays consistent with the rest of the gateway's dashboard-unavailable messaging. The web_dist_dir guard is unchanged.
|
@Audacity88 pushed a maintainer commit to move this along: the startup banner fallback now uses the same installer-oriented wording ( |
|
@Audacity88 re-requested your review. The fallback wording is now aligned with the installer guidance per your note. If you're satisfied, please approve and merge. |
Audacity88
left a comment
There was a problem hiding this comment.
Reviewed head 8b1fbb9 against #7523, the current patch, prior reviews from WareWolf, singlerider, and my earlier notes, the gateway startup fallback text, and the current green CI. This approval clears my prior changes-requested review.
β Resolved β Dashboard fallback now matches installer guidance
The guard still only prints the dashboard URL when web_dist_dir exists. The maintainer follow-up commit also replaces the stale cargo web build fallback with the current installer-oriented guidance for Linux/macOS and Windows. That addresses my prior blocker.
π’ What looks good β Small banner-only fix
The patch remains a narrow gateway startup banner change. It does not change serving, config, auth, or route behavior, and current CI is green.
π΅ Suggestion β Keep the PR body example aligned
The PR body still shows the old cargo web build fallback in the "After (dashboard not found)" example. That is minor public-record drift, not author action; the reviewer-side body cleanup below makes the body match the current code.
Summary
When the Web dashboard is not found (no
web/dist), the gateway still printed "π Web Dashboard: http://..." at startup, which misleads users into expecting a working dashboard.Fix
web_dist_diris availableBefore
After (dashboard found)
After (dashboard not found)
Fixes #7523
Validation Evidence
if web_dist_dir.is_some()guardSecurity & Privacy Impact
Yes, describe the risk and mitigation: N/ACompatibility
NoorYesto either: exact upgrade steps for existing users: N/ARollback
Gateway banner-output fix:
git revert <sha>is the plan.