From f293501184d75a5a269e44c9dca885c35812f77d Mon Sep 17 00:00:00 2001 From: Alexander Harding Date: Sat, 25 Jul 2026 17:31:26 -0500 Subject: [PATCH 1/2] fix(piefed): normalize max_depth to canonical comment levels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PieFed counts max_depth from below top-level while Lemmy counts from the post, so the same canonical request reached a level deeper on PieFed — consumers got comment trees one level bigger there, silently. Verified live on piefed.social and ds9.lemmy.ml: max_depth=1 with no parent_id returns top-level on Lemmy but top-level plus children on PieFed; with a parent_id the two agree. The piefed adapter now asks for one less when there's no parent_id, so 'levels of comments to return' means the same thing everywhere. Verified end-to-end through the client against both live servers: max_depth=1 now returns depth 1 on both, max_depth=2 returns depths 1-2 on both. The fake's decoder inverts the same adjustment, keeping callsTo() payloads canonical — and the matrix test that used to pass a provider-specific depth now passes the same value to both. --- src/providers/piefed/index.ts | 24 ++++++++++++++++++++++++ src/testing/piefed/index.ts | 12 ++++++++++-- test/testing-seed-matrix.test.ts | 17 +++++++++++++---- 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/providers/piefed/index.ts b/src/providers/piefed/index.ts index 062144c..545f1d9 100644 --- a/src/providers/piefed/index.ts +++ b/src/providers/piefed/index.ts @@ -79,6 +79,29 @@ const piefedMiddleware: Middleware = { /** Canonical search types PieFed can actually serve (its enum has no "All") */ type SearchableType = Exclude; +/** + * PieFed counts `max_depth` from *below* top-level, Lemmy counts from the + * post, so the same request reaches a level deeper on PieFed — verified + * live: with `max_depth=1` and no `parent_id`, Lemmy returns top-level + * comments while PieFed returns those plus their children. Requesting one + * less keeps the canonical meaning ("levels of comments to return") + * identical on both. With a `parent_id` the two agree, so it passes + * through untouched. + * + * Canonical `max_depth: 0` ("no comments") has no PieFed equivalent — its + * minimum still returns top-level — so it clamps rather than going + * negative. + */ +function toPiefedMaxDepth( + payload: Parameters[0], +): number | undefined { + const { max_depth, parent_id } = payload; + + if (max_depth === undefined || parent_id !== undefined) return max_depth; + + return Math.max(0, max_depth - 1); +} + const PIEFED_SEARCH_TYPE = { comments: "Comments", communities: "Communities", @@ -399,6 +422,7 @@ export class UnsafePiefedClient implements BaseClient { const { type_, ...rest } = compat.fromPageParams(payload); const query = { ...rest, + max_depth: toPiefedMaxDepth(payload), ...(type_ && { type_: compat.fromListingType(type_) }), } satisfies paths["/api/alpha/comment/list"]["get"]["parameters"]["query"]; diff --git a/src/testing/piefed/index.ts b/src/testing/piefed/index.ts index 2e8fc08..3c4ad58 100644 --- a/src/testing/piefed/index.ts +++ b/src/testing/piefed/index.ts @@ -115,12 +115,20 @@ const PIEFED_OPERATIONS = { getComments: { decode: (call: RecordedCall): Payload<"getComments"> => { const q = query(call); + const parentId = numberish(q.parent_id); + const wireDepth = numberish(q.max_depth); + return { limit: numberish(q.limit), - max_depth: numberish(q.max_depth), + // Invert the adapter's piefed depth adjustment (see + // toPiefedMaxDepth) so the decoded payload is canonical + max_depth: + wireDepth === undefined || parentId !== undefined + ? wireDepth + : wireDepth + 1, // piefed pages with numbers; canonical page_cursor is the string page_cursor: q.page, - parent_id: numberish(q.parent_id), + parent_id: parentId, post_id: numberish(q.post_id), sort: q.sort, } as Payload<"getComments">; diff --git a/test/testing-seed-matrix.test.ts b/test/testing-seed-matrix.test.ts index 2180591..d3a1895 100644 --- a/test/testing-seed-matrix.test.ts +++ b/test/testing-seed-matrix.test.ts @@ -304,6 +304,16 @@ describe.each([ expect(second.data).toHaveLength(0); }); + it("asks for the same comment depth regardless of provider", async () => { + const { client, fake, post } = setup(); + + await client.getComments({ max_depth: 3, post_id: post.id }); + + // Canonical payloads stay canonical even where the wire request had to + // be adjusted for the provider + expect(fake.callsTo("getComments")[0]).toMatchObject({ max_depth: 3 }); + }); + it("honors max_depth relative to the requested parent", async () => { const { client, fake, post } = setup(); @@ -322,11 +332,10 @@ describe.each([ post, }); - // Shallowest depth = top-level only. The providers count differently - // without a parent (verified against live servers): Lemmy counts from - // the post, PieFed counts levels below top-level. + // max_depth means the same thing on every provider: the piefed adapter + // absorbs that server's different base (see toPiefedMaxDepth) const shallow = await client.getComments({ - max_depth: mode === "piefed" ? 0 : 1, + max_depth: 1, post_id: post.id, }); expect(shallow.data.map((view) => view.comment.content)).toEqual([ From 7ba80e0cd0cef63e2bce6b4c4fd6319c9593bc71 Mon Sep 17 00:00:00 2001 From: Alexander Harding Date: Sat, 25 Jul 2026 17:43:24 -0500 Subject: [PATCH 2/2] fix: answer zero-level comment requests directly, cover both depth branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch: clamping canonical max_depth 0 onto piefed's wire 0 collapsed two inputs into one, so the fake's decoder — which has to invert the adjustment — reported max_depth 1 for a request that asked for 0 (or -3). A decoder that can't be inverted breaks the promise that callsTo() payloads are what the caller passed. Zero levels now short-circuits: PieFed's shallowest response still contains top-level comments, so there's nothing to ask it for. That also makes the data uniform (Lemmy returns nothing for max_depth 0) and leaves wire 0 unambiguously meaning canonical 1. Coverage the review found missing: neither depth branch was round-trip tested (the only getComments scenario passed no max_depth at all), and nothing pinned the live semantics this rests on. Adds both decoder scenarios, a matrix test for zero levels, and a live-smoke assertion that max_depth 1 returns only top-level comments on every real instance. --- src/providers/piefed/index.ts | 20 ++++++++++++++++---- test/live-smoke.test.ts | 26 ++++++++++++++++++++++++++ test/testing-request-decoders.test.ts | 15 +++++++++++++++ test/testing-seed-matrix.test.ts | 11 +++++++++++ 4 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/providers/piefed/index.ts b/src/providers/piefed/index.ts index 545f1d9..5f6dcec 100644 --- a/src/providers/piefed/index.ts +++ b/src/providers/piefed/index.ts @@ -88,9 +88,10 @@ type SearchableType = Exclude; * identical on both. With a `parent_id` the two agree, so it passes * through untouched. * - * Canonical `max_depth: 0` ("no comments") has no PieFed equivalent — its - * minimum still returns top-level — so it clamps rather than going - * negative. + * Requests for zero levels never reach here (getComments answers those + * directly), so the adjusted value can't go negative — and wire `0` + * unambiguously means canonical `1`, which is what lets the fake's decoder + * invert this. */ function toPiefedMaxDepth( payload: Parameters[0], @@ -99,7 +100,7 @@ function toPiefedMaxDepth( if (max_depth === undefined || parent_id !== undefined) return max_depth; - return Math.max(0, max_depth - 1); + return max_depth - 1; } const PIEFED_SEARCH_TYPE = { @@ -419,6 +420,17 @@ export class UnsafePiefedClient implements BaseClient { `Connected to piefed, ${payload.mode} is not supported`, ); + // PieFed's shallowest response still contains top-level comments, so a + // canonical request for zero levels has no PieFed equivalent — answer + // it directly rather than asking for something else and returning more + // than the caller wanted. + if ( + payload.max_depth !== undefined && + payload.max_depth <= 0 && + payload.parent_id === undefined + ) + return { ...compat.toPageResponse(payload, { items: 0 }), data: [] }; + const { type_, ...rest } = compat.fromPageParams(payload); const query = { ...rest, diff --git a/test/live-smoke.test.ts b/test/live-smoke.test.ts index 6b49b35..e486c25 100644 --- a/test/live-smoke.test.ts +++ b/test/live-smoke.test.ts @@ -58,6 +58,32 @@ describe.runIf(process.env.LIVE_SMOKE)("live smoke", () => { expect(data.length).toBeGreaterThan(0); }); + it( + "max_depth means the same depth on every provider", + OPTIONS, + async () => { + // The adapters absorb each server's own base (PieFed counts from + // below top-level, Lemmy from the post). If a server changes that, + // this catches it before consumers do. + const { data: posts } = await client.getPosts({ + limit: 20, + type_: "local", + }); + const post = posts.find((view) => view.post.comments > 0); + expect(post, "no post with comments to probe").toBeDefined(); + + const { data } = await client.getComments({ + limit: 50, + max_depth: 1, + post_id: post!.post.id, + }); + + // Depth 1 is top-level only: paths look like `0.` + for (const view of data) + expect(view.comment.path.split(".")).toHaveLength(2); + }, + ); + it("all-type search passes canonical validation", OPTIONS, async () => { // PieFed has no all-type search endpoint — the adapter fans out and // merges, so an unspecified type_ must work everywhere diff --git a/test/testing-request-decoders.test.ts b/test/testing-request-decoders.test.ts index 0b589cf..20e2921 100644 --- a/test/testing-request-decoders.test.ts +++ b/test/testing-request-decoders.test.ts @@ -107,6 +107,21 @@ const SCENARIOS = [ c.getComments({ limit: 5, parent_id: 7, post_id: 42 }), operation: "getComments", }, + { + // No parent: piefed's wire depth is adjusted, so the decoder has to + // undo it to report what the caller asked for + expected: { max_depth: 3, post_id: 42 }, + invoke: (c: ThreadiverseClient) => + c.getComments({ max_depth: 3, post_id: 42 }), + operation: "getComments", + }, + { + // With a parent the providers agree, so depth passes through untouched + expected: { max_depth: 2, parent_id: 7, post_id: 42 }, + invoke: (c: ThreadiverseClient) => + c.getComments({ max_depth: 2, parent_id: 7, post_id: 42 }), + operation: "getComments", + }, { expected: { search_term: "cats", type_: "communities" }, invoke: (c: ThreadiverseClient) => diff --git a/test/testing-seed-matrix.test.ts b/test/testing-seed-matrix.test.ts index d3a1895..99702f9 100644 --- a/test/testing-seed-matrix.test.ts +++ b/test/testing-seed-matrix.test.ts @@ -304,6 +304,17 @@ describe.each([ expect(second.data).toHaveLength(0); }); + it("returns no comments when asked for zero levels", async () => { + const { client, post } = setup(); + + const { data } = await client.getComments({ + max_depth: 0, + post_id: post.id, + }); + + expect(data).toEqual([]); + }); + it("asks for the same comment depth regardless of provider", async () => { const { client, fake, post } = setup();