fix: improve typings for urls and paths

[Meierschlumpf]

Homarr

Thank you for your contribution. Please ensure that your pull request meets the following pull request:

• Builds without warnings or errors (pnpm build, autofix with pnpm format:fix)
• Pull request targets dev branch
• Commits follow the conventional commits guideline
• No shorthand variable names are used (eg. x, y, i or any abbrevation)
• Documentation is up to date. Create a pull request here.

[mairas]

I let Claude Code review the PR. At least 1-4 do look like valid findings to my untrained eye. What do you say?

At the end of the day, I'll of course merge this PR according to your preferences, so consider this review just a unsolicited public service. :-D

Claude's ramblings below.

---

Review

Thanks for picking this up. Overall direction — URL / Path types replacing strings, extracting buildUrl / isPath / parseExternalUrl into @homarr/common, removing removeTrailingSlash boilerplate at call sites — is sound and worth pursuing. A few things need addressing before merging, the first three in particular.

Blocking

1. Hash/fragment dropped for path-only externalUrl (SPA-routing regression)

packages/integrations/src/base/integration.ts:62-72, specifically line 68:

const result = buildUrl(new URL(`http://placeholder${this.integration.externalUrl}`), path, queryParams);
return `${result.pathname}${result.search}` as Path;

When path is e.g. /web/index.html#!/details?id=abc (Jellyfin/Emby hash-bang routing), new URL(...) parses everything after # as .hash. The return concatenates only pathname + search, so the hash is silently dropped.

That was the entire reason RenderablePath existed — its docblock spelled it out:

Path arguments that contain a fragment (e.g. Jellyfin / Emby hash-bang routes like /web/index.html#!/details?id=abc) keep the fragment as part of pathname, and any ? inside the hash-bang is treated as the query separator. This matches what Jellyfin / Emby SPA routers expect.

There's no test asserting hash preservation for the path-only case. Either restore the hash-preserving behavior, or explicitly decide hash-bang isn't supported — but that's a feature regression that deserves its own decision, not a side-effect of a typings refactor.

2. Broken test: null-externalUrl fallback

packages/integrations/test/base.spec.ts:66-73:

test("with null externalUrl should fallback to integration url", () => {
  const integration = new FakeIntegration(undefined, undefined, {
    externalUrl: null,
    url: new URL("https://example.com"),
  });
  const result = integration.buildExternalUrl("/items/42", { q: "search" });
  expect(result).toBe("https://example.com/items/42?q=search");
});

The fallback in externalUrl() returns this.url(path, queryParams) which returns a URL instance. expect(result).toBe(string) uses Object.is — a URL is never Object.is-equal to a string. The absolute case at line 53 correctly does expect(result.toString()).toBe(...). This one needs .toString() too.

If tests are running green, that's evidence this test isn't actually executing.

3. URL-typed values on the tRPC wire

packages/widgets/src/coolify/types.ts:10 types integrationUrl: URL. The same pattern is repeated as Modify<Integration, { ...; url: URL }> in eight subscription router files (calendar, dns-hole, downloads, firewall ×4, media-release, network-controller, notifications).

tRPC + SuperJSON does not transform URL — JSON.stringify(url) invokes URL.prototype.toJSON() and returns a string. So at runtime the client receives a string, but TypeScript thinks it's a URL.

This works today only because the two existing consumers do .toString().replace(...) (instance-card.tsx:41, single-instance-layout.tsx:40) — String.prototype.toString() is a no-op. Any future client code that does .origin, .searchParams, .pathname, etc., will get undefined or crash, with the type system happily allowing it.

Options: keep wire types as string and convert in-process server-side; register a SuperJSON URL transformer; or call .toString() at emit boundaries.

Should fix

4. externalUrl() return-type widening drops the unified URL-ish surface

Was URL | RenderablePath — both exposed hostname, pathname, searchParams, toString(). Callers were branch-free.

Now URL | Path — Path is `/${string}` (a string). The common surface is essentially just toString(). Anything else requires instanceof URL narrowing.

This already forced a UX regression in ntfy. packages/integrations/src/ntfy/ntfy-integration.ts:61:

• Was: title: notification.title ?? topicURL.hostname + topicURL.pathname
• Now: title: notification.title ?? notification.topic

Notification titles used to be e.g. ntfy.example.com/alerts, now they're just alerts. That's a user-visible change made as a workaround for the typing dead-end, not a deliberate UX decision.

If externalUrl() keeps returning a heterogenous union, every caller that wanted host/path info now either has to type-narrow (with separate code paths for the path-only case) or fall back to weaker data. That's the opposite of what the abstraction should do.

Options: (a) keep RenderablePath (or an equivalent thin wrapper) so callers see one uniform surface; (b) return a discriminated wrapper with explicit semantics; (c) accept the union but provide convenience helpers (hostnameOrEmpty(result), etc.) so call sites stay branch-free.

5. Dead variable in ntfy-integration.ts

packages/integrations/src/ntfy/ntfy-integration.ts:57:

const topicURL = this.externalUrl(`/${notification.topic}`);

topicURL is no longer referenced after the title fallback was rewritten. ESLint will flag this. Either remove the declaration, or — better — pick a fallback that actually uses the resolved URL (see #4).

6. Type imports flagged by ESLint config

tooling/eslint/base.js has:

• @typescript-eslint/consistent-type-imports: "warn" (prefer import type)
• import/consistent-type-specifier-style: "error" (prefer-top-level)

Many files do import { Path, QueryParams } from "@homarr/common" (value-style imports for types-only symbols). The error-level rule will fail CI. Mixed-source imports like base/integration.ts:6's import { buildUrl, isPath, Path, QueryParams } need to be split into a value import + a separate import type line.

7. buildUrl query-param precedence is undocumented and surprising

packages/common/src/url.ts:68-83. The implementation parses path-embedded query params into url.searchParams, then baseUrl.searchParams.forEach(...) overrides them with .set (line 73), then explicit queryParams overrides both (line 81).

So precedence is queryParams > baseUrl > path. That's a defensible choice, but counter to what callers might expect (typically the more-specific source wins). Tests don't cover the conflict case. Either document precedence in the JSDoc or reverse it.

Minor

8. URL.parse(input) instead of new URL(input)

packages/common/src/url.ts:112. URL.parse is the newer non-throwing static — returns URL | null on parse failure. Silently swallows malformed hrefs rather than throwing, which diverges from the rest of the codebase. It also requires Node 22.x. If non-throw semantics are intentional, fine — but worth a comment and a quick check that the target environment supports it.

9. as Path casts skip the runtime guard

packages/integrations/src/plex/plex-integration.ts:112 and packages/integrations/src/media-organizer/readarr/readarr-integration.ts:107 cast external-API strings directly to Path. The isPath guard is the only safe path to Path — codify that by avoiding the cast (use isPath(...) and short-circuit when false). These pre-date this PR as as `/${string}` casts, so this isn't a regression — just preserved.

---

Given #1 and #3 in particular, this isn't mergeable as-is. Happy to take a follow-up stack on top if you'd rather land the typing direction first and address the regressions separately — let me know what you'd prefer.