fix(services): expand banner box to prevent │ from corrupting public URL (cloudflared) - #1419
fix(services): expand banner box to prevent │ from corrupting public URL (cloudflared)#1419MauroDruwel wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThree banner renderings were replaced with a shared, terminal-aware Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/start-services.sh (1)
199-210: Reduce magic-number width math in banner rows.The width offsets (
19,15,29,37,50) are easy to desync during text edits. Consider deriving them from string lengths so the box stays correct if labels change.Suggested refactor (length-derived widths)
- printf " │ NemoClaw Services%-*s│\n" $(( inner - 19 )) "" + local title_text=" NemoClaw Services" + local url_prefix=" Public URL: " + local telegram_running_text=" Telegram: bridge running" + local telegram_stopped_text=" Telegram: not started (no token)" + local footer_text=" Run 'openshell term' to monitor egress approvals" + + printf " │%s%-*s│\n" "$title_text" $(( inner - ${`#title_text`} )) "" printf " │%-*s│\n" "$inner" "" if [ -n "$tunnel_url" ]; then - printf " │ Public URL: %-*s│\n" $(( inner - 15 )) "$tunnel_url" + printf " │%s%-*s│\n" "$url_prefix" $(( inner - ${`#url_prefix`} )) "$tunnel_url" fi if is_running telegram-bridge; then - printf " │ Telegram: bridge running%-*s│\n" $(( inner - 29 )) "" + printf " │%s%-*s│\n" "$telegram_running_text" $(( inner - ${`#telegram_running_text`} )) "" else - printf " │ Telegram: not started (no token)%-*s│\n" $(( inner - 37 )) "" + printf " │%s%-*s│\n" "$telegram_stopped_text" $(( inner - ${`#telegram_stopped_text`} )) "" fi printf " │%-*s│\n" "$inner" "" - printf " │ Run 'openshell term' to monitor egress approvals%-*s│\n" $(( inner - 50 )) "" + printf " │%s%-*s│\n" "$footer_text" $(( inner - ${`#footer_text`} )) ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/start-services.sh` around lines 199 - 210, Replace the hardcoded magic-number width offsets by computing pad widths from the actual text lengths before each printf; for each banner line (the printf calls that use "inner" and constants) build the text portion (e.g., the labels like "NemoClaw Services", "Public URL: $tunnel_url", "Telegram: bridge running" or "Telegram: not started (no token)"), compute pad = inner - ${`#text`} (or use expr/parameter expansion) and then pass that pad to printf. Update the printf lines in scripts/start-services.sh that reference inner and numeric offsets so they use the computed pad variable and the text variable instead of the hardcoded numbers, and ensure the is_running branch constructs its text before measuring length.src/lib/services.ts (1)
354-359: Harden width calculation against future text edits.
pad()can throw if any row text grows pastinnerlater. Consider derivinginnerfrom all row candidates and clamping repeat count.Suggested hardening
- const minInner = 53; - const inner = tunnelUrl ? Math.max(minInner, urlPrefix.length + tunnelUrl.length + 2) : minInner; - - const pad = (s: string) => s + " ".repeat(inner - s.length); + const minInner = 53; + const rows = [ + titleText, + telegramText, + footerText, + tunnelUrl ? `${urlPrefix}${tunnelUrl}` : "", + ]; + const inner = Math.max(minInner, ...rows.map((s) => s.length + 2)); + + const pad = (s: string) => s + " ".repeat(Math.max(0, inner - s.length));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services.ts` around lines 354 - 359, The current width logic (minInner, inner, pad, hBar) can break if any future row grows beyond inner; update the calculation to compute inner from the lengths of all candidate row strings (e.g., urlPrefix, tunnelUrl and any other lines you render) taking the maximum length plus your extra padding, then clamp inner with Math.max(minInner, computedMax) and ensure pad uses Math.max(0, inner - s.length) when repeating spaces and hBar uses "─".repeat(Math.max(0, inner)) so no negative repeat occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/start-services.sh`:
- Around line 199-210: Replace the hardcoded magic-number width offsets by
computing pad widths from the actual text lengths before each printf; for each
banner line (the printf calls that use "inner" and constants) build the text
portion (e.g., the labels like "NemoClaw Services", "Public URL: $tunnel_url",
"Telegram: bridge running" or "Telegram: not started (no token)"), compute
pad = inner - ${`#text`} (or use expr/parameter expansion) and then pass that pad
to printf. Update the printf lines in scripts/start-services.sh that reference
inner and numeric offsets so they use the computed pad variable and the text
variable instead of the hardcoded numbers, and ensure the is_running branch
constructs its text before measuring length.
In `@src/lib/services.ts`:
- Around line 354-359: The current width logic (minInner, inner, pad, hBar) can
break if any future row grows beyond inner; update the calculation to compute
inner from the lengths of all candidate row strings (e.g., urlPrefix, tunnelUrl
and any other lines you render) taking the maximum length plus your extra
padding, then clamp inner with Math.max(minInner, computedMax) and ensure pad
uses Math.max(0, inner - s.length) when repeating spaces and hBar uses
"─".repeat(Math.max(0, inner)) so no negative repeat occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e6127af-2e21-4524-ae91-cb29eaffed83
📒 Files selected for processing (2)
scripts/start-services.shsrc/lib/services.ts
|
✨ Thanks for submitting this pull request, which proposes a way to fix a display issue in the CLI banner where the cloudflare tunnel URL gets corrupted by a box-drawing character. This could improve observability by ensuring URLs remain clickable and correctly rendered. |
|
Thanks for the banner box fix — that kind of visual corruption on the public URL is a real usability problem. The codebase has changed significantly since April 3 — including a full TypeScript migration — so this will need a rebase on |
2ff19f2 to
fb52c95
Compare
When the cloudflare tunnel URL is longer than 40 chars (typical for trycloudflare.com URLs like ~50 chars), padEnd(40) adds no padding, so the closing │ box character is printed immediately after the URL. Terminals auto-detect this as part of the URL and Punycode-encode the │ (U+2502), turning `.com│` into `.xn--com-hs4a`, producing a broken non-clickable URL. Fix by computing the box inner width dynamically: default 53 chars, but expanded to fit `url + 2 trailing spaces` when the URL is longer. All box lines are rendered with the computed width so borders stay aligned. Same fix applied to scripts/start-services.sh (bash variant). https://claude.ai/code/session_01VBztUR9CyS1QtWaDf5zCFp
Apply the same dynamic inner-width logic used in src/lib/services.ts to the NemoClaw registered banner in nemoclaw/src/index.ts. The old code used hardcoded .padEnd(40) which would push the closing │ beyond the fixed 53-char horizontal bars whenever an endpoint, provider, or model string exceeded 40 characters, making all lines different lengths. Now inner width = max(53, longestValue + prefix(13) + 2 trailing spaces), so all lines are always the same length regardless of value lengths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
tr is a byte-level tool — 'tr " " "─"' only emits the first byte (\xe2) of the 3-byte UTF-8 sequence for ─ (U+2500), producing invalid UTF-8 that renders as garbled characters in the terminal. Replace with a bash += loop which correctly appends the full multi-byte character on each iteration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fb52c95 to
8a0c7e7
Compare
… add tests Replace inline box-drawing in services.ts, nemoclaw/src/index.ts, and start-services.sh with a shared renderBox helper that: - Derives inner width dynamically from content (fixes the padEnd(40) bug) - Caps at terminal width minus 4 so the box never overflows (uses process.stdout.columns with a 100-col fallback, floored at 60) - Guarantees at least 2 trailing spaces before each closing │ src/lib/banner.ts and nemoclaw/src/banner.ts are identical; they cannot share a module because nemoclaw/ is a separate npm project. Each banner.test.ts covers: border shape, minInner (default + custom), null blank lines, equal-width alignment, long-line expansion, terminal capping, undefined-columns fallback, no-throw on overflow, and the core ≥2-trailing-spaces invariant (columns mocked to 120 for determinism). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/banner.ts (1)
1-35: Consider extracting shared utility.This
renderBoximplementation is identical tonemoclaw/src/banner.ts. While the duplication is understandable given separate TypeScript project configurations (tsconfig.cli.jsonvsnemoclaw/tsconfig.json), consider extracting to a shared internal package if duplication grows.For this PR, the duplication is acceptable since the fix is isolated and both locations need the same behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/banner.ts` around lines 1 - 35, The renderBox function is duplicated (same implementation as in nemoclaw/src/banner.ts); extract it into a shared internal module (e.g., packages/ui-utils or src/shared/banner.ts) and export renderBox so both projects import the single implementation; update the callers to import renderBox instead of their local copy, ensure the signature (renderBox(lines: (string|null)[], { minInner?: number } = {})) and behavior (uses process.stdout.columns, pad/hBar/blank logic) remain identical, and update tsconfig/paths or package.json workspaces so both builds resolve the shared module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/banner.ts`:
- Around line 1-35: The renderBox function is duplicated (same implementation as
in nemoclaw/src/banner.ts); extract it into a shared internal module (e.g.,
packages/ui-utils or src/shared/banner.ts) and export renderBox so both projects
import the single implementation; update the callers to import renderBox instead
of their local copy, ensure the signature (renderBox(lines: (string|null)[], {
minInner?: number } = {})) and behavior (uses process.stdout.columns,
pad/hBar/blank logic) remain identical, and update tsconfig/paths or
package.json workspaces so both builds resolve the shared module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bc9fe98-37e8-46ae-a197-786258eaa96b
📒 Files selected for processing (7)
nemoclaw/src/banner.test.tsnemoclaw/src/banner.tsnemoclaw/src/index.tsscripts/start-services.shsrc/lib/banner.test.tssrc/lib/banner.tssrc/lib/services.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/services.ts
I don't think this change is good, we don't need shared module, so this PR is ready for review |
PR Review: fix/cloudflare-url-box-overflowFiles Changed: 7 Nice fix — the root cause analysis is spot-on and the 🔴 Blocker (must fix)
Affected tests: "caps inner width", "falls back to 100-column", "does not throw when content exceeds", and "always provides at least 2 trailing spaces". Suggested fix — use // Instead of:
vi.spyOn(process.stdout, "columns", "get").mockReturnValue(70);
// Use:
const original = Object.getOwnPropertyDescriptor(process.stdout, "columns");
Object.defineProperty(process.stdout, "columns", { get: () => 70, configurable: true });
// ... test ...
// Restore in afterEach:
if (original) Object.defineProperty(process.stdout, "columns", original);
else delete (process.stdout as any).columns;🟡 Warnings (should fix)
🔵 Suggestion (nice to have)
✅ What's Good
|
|
Thanks for the contribution @MauroDruwel! This has gone stale and main has diverged significantly in the affected files (services.ts was refactored in #1307, #1819, #2422, #2502, #2661). We're going to rework this from scratch on latest main. The core idea (dynamic Filing a new tracking issue now. |
wscurran
added
area: cli
4 participants
Summary
When the cloudflare tunnel URL (~50 chars) is printed inside a fixed-width ASCII box using
padEnd(40), no padding is added and the closing│(U+2502) box character lands immediately after the URL. Terminals auto-detect the│as part of the URL and Punycode-encode.com│→.xn--com-hs4a, producing a broken, non-clickable link. This PR fixes the banner in bothsrc/lib/services.tsandscripts/start-services.shto expand the box width dynamically so there are always at least 2 trailing spaces before the closing border.Screenshot of issue
Changes
src/lib/services.ts: replaced hardcodedpadEnd(40)banner with a dynamic-width renderer that computesinner = max(53, urlPrefix.length + url.length + 2)and pads all box lines to match.scripts/start-services.sh: same fix usingprintf %-*swith a dynamically computed field width.Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.Screenshot after
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Signed-off-by: Mauro Druwel <mauro.druwel@gmail.com>
Summary by CodeRabbit
Refactor
Tests