Skip to content

Commit fc8e674

Browse files
mairasmairas
committed
revert: keep ping pub/sub URL-keyed (drop id-based dedup)
Reverts the redis/cron-jobs/widgets-app subscription-key change. The URL-keyed implementation is already correct under path-only hrefs -- each client's filter naturally matches its own resolved URL. The cost is a small per-extra-hostname duplication of ping traffic, which is acceptable in practice (low N) and arguably more informative (per- hostname status divergence is preserved). Comment in the source notes the trade-off. This shrinks the patch surface, dropping three architectural changes from the upstream PR that weren't load-bearing for the path-only behavior. R5 audit updated accordingly.
1 parent 2a69968 commit fc8e674

4 files changed

Lines changed: 58 additions & 70 deletions

File tree

‎R5-AUDIT.md‎

Lines changed: 23 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -16,26 +16,23 @@ Performed on the `feat/path-only-app-hrefs` branch of `hatlabs/homarr` (cut from
1616
raw path; bookmarks widget gets explicit fallback for `new URL(...).hostname`
1717
parsing failure (R3).
1818

19-
| # | Path | Description | Class |
20-
|---|------|-------------|-------|
21-
| 1 | `packages/validation/src/app.ts:3-10` | `appHrefSchema` definition | **R1 target** — relaxed to accept path-only |
22-
| 2 | `packages/api/src/router/widgets/app.ts:32` | Ping query: `pingUrl ?? href` | **Server-internal** — route through `resolveServerUrl` |
23-
| 3 | `packages/api/src/router/widgets/app.ts:73` | Ping subscription: `pingUrl ?? href` | **Server-internal** — route through `resolveServerUrl` |
24-
| 4 | `packages/api/src/router/widgets/app.ts:73,82,88,94` | Subscription channel key (was URL-based) | **Architectural** — switch to `app.id` (F3 mitigation) |
25-
| 5 | `packages/cron-jobs/src/jobs/ping.ts:28-46` | Cron-job dedup + publish | **Architectural** — adapt to new `{id,url}` channel shape |
26-
| 6 | `packages/redis/src/index.ts:21-24` | `pingChannel` / `pingUrlChannel` types | **Architectural** — message gains `id`, list carries `{id,url}` |
27-
| 7 | `packages/request-handler/src/lib/cached-request-integration-job-handler.ts:97` | `externalUrl: integration.app?.href ?? null` | **Server-internal** — background job, no headers; resolves to `null` for path-only |
28-
| 8 | `packages/api/src/middlewares/integration.ts:69` | `externalUrl: rest.app?.href ?? null` (one) | **Server-internal** — route via `resolveServerUrl(app, ctx.headers)` |
29-
| 9 | `packages/api/src/middlewares/integration.ts:131` | `externalUrl: rest.app?.href ?? null` (many) | **Server-internal** — route via `resolveServerUrl(app, ctx.headers)` |
30-
| 10 | `packages/api/src/router/app.ts:122,141,168` | App CRUD persistence | **Storage** — pass through; relaxation propagates via schema |
31-
| 11 | `packages/api/src/router/integration/integration-router.ts:686-701` | App creation during integration setup | **Storage** — pass through |
32-
| 12 | `packages/old-import/src/mappers/map-app.ts:26` | Old-Marr import mapper | **Storage** — pass through; uses `appCreateManySchema` downstream |
33-
| 13 | `packages/old-import/src/import/import-single-oldmarr.ts:20` | Old-Marr import filter (`href !== null`) | **Storage** — null-tolerant; no change |
34-
| 14 | `packages/widgets/src/app/component.tsx:38,49,64,82,132` | App widget `<a href={app.href}>` and conditional rendering | **Navigation-only** — no change (R4 verified — `UnstyledButton component="a"`, no `next/link`) |
35-
| 15 | `packages/widgets/src/bookmarks/component.tsx:36,103,161` | Bookmarks widget `<a href={app.href}>` | **Navigation-only** — no change |
36-
| 16 | `packages/widgets/src/bookmarks/component.tsx:235,282` | Bookmarks widget sub-label `new URL(app.href).hostname` | **Display (R3)** — try/catch fallback to trailing-slash-trimmed path |
37-
| 17 | `apps/nextjs/src/app/[locale]/manage/apps/page.tsx:109-111` | Admin apps list — Anchor display + navigation | **Navigation-only / Display** — works with path-only (anchor resolves; text shows path) |
38-
| 18 | `apps/nextjs/src/components/board/sections/category/category-menu-actions.tsx:119,122` | "Open all" — `window.open(app.href)` | **Navigation-only** — `window.open` accepts relative URLs (resolved against current document) |
19+
| # | Path | Description | Class |
20+
| --- | -------------------------------------------------------------------------------------- | ---------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
21+
| 1 | `packages/validation/src/app.ts:3-10` | `appHrefSchema` definition | **R1 target** — relaxed to accept path-only |
22+
| 2 | `packages/api/src/router/widgets/app.ts:32` | Ping query: `pingUrl ?? href` | **Server-internal** — route through `resolveServerUrl` |
23+
| 3 | `packages/api/src/router/widgets/app.ts:73` | Ping subscription: `pingUrl ?? href` | **Server-internal** — route through `resolveServerUrl`. Subscription stays URL-keyed; under path-only hrefs two clients on different hostnames produce two pings per cycle, each matched by its own client's URL filter. Minor traffic overhead, no correctness issue (commented in source). |
24+
| 4 | `packages/request-handler/src/lib/cached-request-integration-job-handler.ts:97` | `externalUrl: integration.app?.href ?? null` | **Server-internal** — background job, no headers; resolves to `null` for path-only |
25+
| 5 | `packages/api/src/middlewares/integration.ts:69` | `externalUrl: rest.app?.href ?? null` (one) | **Server-internal** — route via `resolveServerUrl(app, ctx.headers)` |
26+
| 6 | `packages/api/src/middlewares/integration.ts:131` | `externalUrl: rest.app?.href ?? null` (many) | **Server-internal** — route via `resolveServerUrl(app, ctx.headers)` |
27+
| 7 | `packages/api/src/router/app.ts:122,141,168` | App CRUD persistence | **Storage** — pass through; relaxation propagates via schema |
28+
| 8 | `packages/api/src/router/integration/integration-router.ts:686-701` | App creation during integration setup | **Storage** — pass through |
29+
| 9 | `packages/old-import/src/mappers/map-app.ts:26` | Old-Marr import mapper | **Storage** — pass through; uses `appCreateManySchema` downstream |
30+
| 10 | `packages/old-import/src/import/import-single-oldmarr.ts:20` | Old-Marr import filter (`href !== null`) | **Storage** — null-tolerant; no change |
31+
| 11 | `packages/widgets/src/app/component.tsx:38,49,64,82,132` | App widget `<a href={app.href}>` and conditional rendering | **Navigation-only** — no change (R4 verified — `UnstyledButton component="a"`, no `next/link`) |
32+
| 12 | `packages/widgets/src/bookmarks/component.tsx:36,103,161` | Bookmarks widget `<a href={app.href}>` | **Navigation-only** — no change |
33+
| 13 | `packages/widgets/src/bookmarks/component.tsx:235,282` | Bookmarks widget sub-label `new URL(app.href).hostname` | **Display (R3)** — try/catch fallback to trailing-slash-trimmed path |
34+
| 14 | `apps/nextjs/src/app/[locale]/manage/apps/page.tsx:109-111` | Admin apps list — Anchor display + navigation | **Navigation-only / Display** — works with path-only (anchor resolves; text shows path) |
35+
| 15 | `apps/nextjs/src/components/board/sections/category/category-menu-actions.tsx:119,122` | "Open all" — `window.open(app.href)` | **Navigation-only** — `window.open` accepts relative URLs (resolved against current document) |
3936

4037
Coverage of the audit scope mandated by the plan:
4138

@@ -49,7 +46,9 @@ Coverage of the audit scope mandated by the plan:
4946
- Import/export flows — ✅ (`packages/old-import/`)
5047
- `apps/nextjs/**` — ✅
5148

52-
No deeply-coupled consumer was discovered that resists path-only migration. The
53-
architectural change in items 4–6 (subscription channel keying) is the only
54-
non-trivial behavioral change; it is a small correctness improvement
55-
independent of HaLOS use cases.
49+
No deeply-coupled consumer was discovered that resists path-only migration.
50+
All findings are either navigation-only (no change), routed through
51+
`resolveServerUrl` (small additive change), or storage paths that pass through
52+
once the schema accepts the new shape. Ping pub/sub keeps its existing
53+
URL-keyed shape; the path-only case produces a small per-extra-hostname
54+
duplication of ping traffic with no correctness consequence.

‎packages/api/src/router/widgets/app.ts‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { z } from "zod/v4";
55
import { resolveServerUrl } from "@homarr/common";
66
import { getServerSettingByKeyAsync } from "@homarr/db/queries";
77
import { sendPingRequestAsync } from "@homarr/ping";
8-
import type { PingResult } from "@homarr/redis";
98
import { pingChannel, pingUrlChannel } from "@homarr/redis";
109

1110
import { createTRPCRouter, publicProcedure } from "../../trpc";
@@ -81,22 +80,25 @@ export const appRouter = createTRPCRouter({
8180
});
8281
}
8382

84-
const entry = { id: app.id, url: pingUrl };
85-
await pingUrlChannel.addAsync(entry);
86-
87-
return observable<PingResult>((emit) => {
88-
const unsubscribe = pingChannel.subscribe((message) => {
89-
// Filter by app id rather than URL: path-only hrefs resolve to
90-
// different absolute URLs across clients on different hostnames,
91-
// but the app id is stable.
92-
if (message.id !== app.id) return;
93-
emit.next(message);
94-
});
95-
96-
return () => {
97-
unsubscribe();
98-
void pingUrlChannel.removeAsync(entry);
99-
};
100-
});
83+
await pingUrlChannel.addAsync(pingUrl);
84+
85+
return observable<{ url: string; statusCode: number; durationMs: number } | { url: string; error: string }>(
86+
(emit) => {
87+
const unsubscribe = pingChannel.subscribe((message) => {
88+
// Only emit if same url. Note: under path-only hrefs, two clients
89+
// on different hostnames produce two distinct pingUrls for the
90+
// same app and each subscription matches its own. That's the
91+
// desired behavior — each client sees the status reachable from
92+
// its own origin — at the cost of one extra ping per cycle.
93+
if (message.url !== pingUrl) return;
94+
emit.next(message);
95+
});
96+
97+
return () => {
98+
unsubscribe();
99+
void pingUrlChannel.removeAsync(pingUrl);
100+
};
101+
},
102+
);
101103
}),
102104
});

‎packages/cron-jobs/src/jobs/ping.ts‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,29 +25,26 @@ export const pingJob = createCronJob("ping", EVERY_MINUTE, {
2525
return;
2626
}
2727

28-
const entries = await pingUrlChannel.getAllAsync();
29-
30-
// Dedup by id — multiple subscribers for the same app coalesce into one ping.
31-
// (Two distinct apps that happen to share a URL still ping independently.)
32-
const uniqueById = new Map<string, { id: string; url: string }>();
33-
for (const entry of entries) {
34-
if (!uniqueById.has(entry.id)) uniqueById.set(entry.id, entry);
35-
}
36-
37-
await Promise.allSettled([...uniqueById.values()].map(pingAsync));
28+
const urls = await pingUrlChannel.getAllAsync();
29+
30+
// Dedup by URL string. Note: with path-only `app.href` resolved through
31+
// `resolveServerUrl(app, headers)`, two clients on different hostnames
32+
// produce two distinct pingUrls for the same app and therefore generate two
33+
// separate pings per cycle — minor traffic overhead, harmless because each
34+
// client's URL-keyed subscription filter still matches its own result.
35+
await Promise.allSettled([...new Set(urls)].map(pingAsync));
3836
});
3937

40-
const pingAsync = async ({ id, url }: { id: string; url: string }) => {
38+
const pingAsync = async (url: string) => {
4139
const pingResult = await sendPingRequestAsync(url);
4240

4341
if ("statusCode" in pingResult) {
44-
logger.debug("Executed ping successfully", { id, url, statusCode: pingResult.statusCode });
42+
logger.debug("Executed ping successfully", { url, statusCode: pingResult.statusCode });
4543
} else {
46-
logger.error(new ErrorWithMetadata("Executing ping failed", { id, url }, { cause: pingResult.error }));
44+
logger.error(new ErrorWithMetadata("Executing ping failed", { url }, { cause: pingResult.error }));
4745
}
4846

4947
await pingChannel.publishAsync({
50-
id,
5148
url,
5249
...pingResult,
5350
});

‎packages/redis/src/index.ts‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,10 @@ export {
1818
export { createIntegrationHistoryChannel } from "./lib/channels/history-channel";
1919

2020
export const exampleChannel = createSubPubChannel<{ message: string }>("example");
21-
22-
// Ping pub/sub messages and the list of pending entries are keyed by the
23-
// owning app's id rather than its URL. Path-only `app.href` values resolve to
24-
// different absolute URLs across clients on different hostnames; using the
25-
// stable `app.id` keeps subscribers correctly matched.
26-
export interface PingEntry {
27-
id: string;
28-
url: string;
29-
}
30-
export type PingResult =
31-
| { id: string; url: string; statusCode: number; durationMs: number }
32-
| { id: string; url: string; error: string };
33-
export const pingChannel = createSubPubChannel<PingResult>("ping");
34-
export const pingUrlChannel = createListChannel<PingEntry>("ping-url");
21+
export const pingChannel = createSubPubChannel<
22+
{ url: string; statusCode: number; durationMs: number } | { url: string; error: string }
23+
>("ping");
24+
export const pingUrlChannel = createListChannel<string>("ping-url");
3525

3626
export const homeAssistantEntityState = createSubPubChannel<{
3727
entityId: string;

0 commit comments

Comments
 (0)