Skip to content

feat(inspector): let run scripts declare their Open menu URL - #939

Merged
dohooo merged 4 commits into
dohooo:mainfrom
dalkommatt:dalkommatt/helmor-open-menu-url
Jul 24, 2026
Merged

feat(inspector): let run scripts declare their Open menu URL#939
dohooo merged 4 commits into
dohooo:mainfrom
dalkommatt:dalkommatt/helmor-open-menu-url

Conversation

@dalkommatt

Copy link
Copy Markdown
Contributor

Problem

The Run tab's Open menu is derived by regex-sniffing script stdout for http://{localhost,127.0.0.1,0.0.0.0}:PORT banners (detect-urls.ts). That works for a plain dev server, but breaks under a reverse proxy.

With portless (and equally Caddy, ngrok, Tailscale Funnel), each workspace gets a named URL like https://achernar.localhost and the underlying services are assigned ephemeral ports. Helmor surfaces those raw ports — localhost:4761, localhost:4786 — which:

  • aren't reachable through the proxy,
  • aren't the address the app actually serves on,
  • change on every boot.

The run script knows the right URL and already prints it, but there's no way to tell Helmor about it.

Solution

A run script can declare its URL by printing a marker line:

echo "helmor:url=https://${HELMOR_WORKSPACE_NAME}.localhost"

Chosen over a schema column or a well-known file because it needs no per-repo config beyond one echo, and works for any proxy-based setup. HELMOR_WORKSPACE_NAME is already exported into the script environment (workspace/scripts.rs:416), so per-workspace interpolation comes for free — no migration, no new CLI flag, no backend change at all.

Behaviour

  • Any host accepted. Unlike LOCAL_URL_RE, the marker's host is unrestricted — named domains are the entire point.
  • Declared replaces sniffed. A script that declares its address is telling us the sniffed ports are wrong, not that they're extra options. Once a declaration lands, sniffing stops.
  • Multiple markers supported. A monorepo printing web + api URLs gets the existing 2+ picker.
  • Line-anchored. ^[ \t]*helmor:url= so a URL in prose can't spoof it.
  • ANSI-safe. Parsed after stripAnsi, so a colored marker still matches.
  • Marker stays visible in the terminal — it doubles as a debugging signal when detection isn't firing.

OpenDevServerButton needed no change: it already falls back to a plain "Open" label when extractPort returns null, which is exactly the portless case.

Chunk-boundary correctness

PTY output splits on arbitrary 4096-byte boundaries, so a marker can straddle two chunks. The naive fix — carry the partial line forward and rescan — is wrong: helmor:url=https://ach parses as a perfectly well-formed URL and gets committed truncated. Detection therefore only accepts declarations from newline-terminated lines, with the incomplete tail carried into the next chunk (capped at 2 KB so a \r-redrawing progress bar can't grow the buffer unbounded). Covered by a regression test.

Changes

File
detect-urls.ts extractDeclaredUrls(), trailingPartialLine()
script-store.ts declaredUrls + tail on ScriptEntry; effectiveScriptUrls()
sections/run.tsx re-attach replay path routed through effectiveScriptUrls so declared URLs survive a tab switch

Frontend only — no Rust, no schema.rs, no pipeline/, so no snapshot coverage required.

Testing

15 new tests (8 parser, 4 boundary helper, 6 store-level precedence/dedup/notification). Full frontend suite green at 833 passing. tsc clean for src/; biome clean.

Note: sidecar/test/codex-app-server-manager.test.ts has two pre-existing SendMessageParams typecheck errors on main, untouched by this PR.

Copilot AI review requested due to automatic review settings July 18, 2026 10:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a marker-based mechanism for Run scripts to explicitly declare the URL(s) shown in the Inspector Run tab’s Open menu, so reverse-proxy dev setups (e.g. portless/ngrok/Tailscale) can surface stable, reachable hostnames instead of ephemeral localhost:PORT banners.

Changes:

  • Introduces helmor:url=<URL> parsing (ANSI-safe, line-anchored) and chunk-boundary-safe buffering via a trailing partial-line carry.
  • Extends the script store to track declaredUrls (which take precedence over sniffed localhost URLs) and to stop collecting sniffed URLs once declarations exist.
  • Ensures the Run tab replay path uses the effective URL set so declared URLs survive tab switches/re-attach.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/features/inspector/sections/run.tsx Replays URLs using effectiveScriptUrls() so declared URLs are reflected immediately on mount.
src/features/inspector/script-store.ts Tracks declared URLs and tail buffering; applies declared-over-sniffed precedence during streaming.
src/features/inspector/script-store.test.ts Adds store-level tests for precedence, boundary handling, dedup/order, and listener notifications.
src/features/inspector/detect-urls.ts Adds declared-marker extraction and trailingPartialLine() helper for chunk-boundary correctness.
src/features/inspector/detect-urls.test.ts Adds parser/helper tests for declared URLs and trailing partial line behavior.
.changeset/run-script-declared-url.md Adds a patch changeset documenting the new marker-based URL declaration feature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/features/inspector/script-store.ts Outdated
Comment on lines 173 to 179
if (
entry.urls.length + entry.declaredUrls.length > 0 &&
!event.data.includes("http")
) {
entry.tail = trailingPartialLine(entry.tail + event.data);
break;
}
Helmor derives the Run tab's Open menu by regex-sniffing stdout for
`http://{localhost,127.0.0.1,0.0.0.0}:PORT` banners. That breaks under a
reverse proxy: with portless, Caddy, ngrok, or Tailscale Funnel the
services behind the proxy print ephemeral ports that are neither
reachable nor stable across boots, while the address users actually need
is a named domain nobody prints in a recognizable banner.

Run scripts can now declare it explicitly by printing a marker line:

    echo "helmor:url=https://${HELMOR_WORKSPACE_NAME}.localhost"

Declared URLs accept any host (not just the three local forms), and take
precedence over sniffed ones rather than joining them — a script that
declares its address is telling us the sniffed ports are wrong, not that
they are extra options worth offering. Repeating the marker declares
multiple URLs, which surface in the existing 2+ picker.

Detection waits for the terminating newline before committing a match,
since PTY output splits on arbitrary 4096-byte boundaries and a marker
truncated mid-URL still parses as a well-formed one.

No backend change: HELMOR_WORKSPACE_NAME is already exported into the
script environment, so per-workspace interpolation works with no schema
column and no new CLI flag.
@dalkommatt
dalkommatt force-pushed the dalkommatt/helmor-open-menu-url branch from efb1330 to 401e076 Compare July 18, 2026 10:27
The short-circuit guarding URL detection used a case-sensitive
`event.data.includes("http")`, but both URL regexes it guards carry the
`i` flag. A script printing `helmor:url=HTTPS://…` after any URL had
already been detected hit the guard and was dropped, so the declaration
never landed and the Open menu kept showing the stale port.

Use a no-flag `/http/i` regex `test` instead: case-correct, allocation-
free (unlike `toLowerCase()`), and no `lastIndex` state to reset — so the
fast path stays fast while agreeing with the parsers it guards.

Reported by Copilot review on dohooo#939.
@dalkommatt

Copy link
Copy Markdown
Contributor Author

Good catch — fixed in b20fe16. This was a real bug, not a theoretical one.

Both LOCAL_URL_RE and DECLARED_URL_RE carry the i flag, so the case-sensitive includes("http") fast path disagreed with the parsers it was guarding. Concretely: once any URL had been detected, a script printing helmor:url=HTTPS://achernar.localhost hit the short-circuit and was dropped, so the declaration never landed and the Open menu kept showing the stale ephemeral port — exactly the failure this PR exists to fix.

I went with a no-flag /http/i regex test rather than toLowerCase(), since the point of the fast path is to avoid work on steady-state HMR noise and lowercasing every chunk would allocate a full copy. No g flag, so there's no lastIndex state to reset between calls.

Added a regression test that reproduces the reported sequence (sniffed lowercase URL, then an uppercase-scheme declaration). Verified it fails against the old includes("http") and passes with the fix, so it's pinning the actual behaviour rather than just documenting it.

@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

@dohooo is attempting to deploy a commit to the Caspian's Team Team on Vercel.

A member of the Team first needs to authorize it.

The fast-path skip guarding URL detection probed only the fresh chunk
for "http". But the tail carry exists precisely because a helmor:url=
marker can straddle a PTY chunk boundary — and when it does, the
marker's "http" bytes can sit in the carried tail while the completing
fragment has none. With any URL already detected, such a chunk took the
fast path, which recomputed the tail and threw away the just-completed
marker line unscanned. The declaration was silently lost for the rest
of the run: the marker prints once and replay never re-parses.

Probe the rejoined tail + chunk instead. The concatenation was already
built on both paths, so the fast path stays allocation-neutral.

Found in review on dohooo#939. Two regression tests: a split marker after a
URL was already sniffed, and the harder variant where the scheme itself
straddles the boundary so neither fragment contains "http" alone.
@dohooo

dohooo commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Pushed ff68185 — one correctness fix found while reviewing the chunk-boundary handling.

The fast-path probe couldn't see the carried tail. The skip guard tested event.data alone, but once any URL has been detected, a marker split across a PTY boundary can leave its http bytes in entry.tail while the completing fragment has none:

chunk 1: "helmor:url=https://ach"        → probe passes, tail carries it
chunk 2: "ernar.localhost\nready\n"      → no "http" → fast path → tail recomputed,
                                           completed marker line discarded unscanned

The declaration is silently lost for the whole run — the marker prints once and replay never re-parses. The existing boundary regression test stays green because it exercises the split before any URL is sniffed, where the fast path can't trigger; with portless the ephemeral ports typically land first, which arms it.

Fix: probe the rejoined tail + chunk instead — it was already being concatenated on both paths, so the fast path stays allocation-neutral. Added two regression tests (split after a sniffed URL, and the harder variant where the scheme itself straddles the boundary so neither fragment contains "http" alone). Full frontend suite green at 1688.

@dohooo
dohooo merged commit deda300 into dohooo:main Jul 24, 2026
13 of 14 checks passed
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.

3 participants