fix(piefed): normalize max_depth to canonical comment levels - #75
Merged
Conversation
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.
…anches 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
max_depthis a canonical parameter, but the two servers count it from different places — so the same request returned a deeper tree on PieFed, and consumers had no way to know.Verified live (my own probes, on
piefed.socialand theds9.lemmy.mlv1 nightly), noparent_id:max_depthWith a
parent_idthey agree (0→ the parent alone,1→ parent + children), so that path is untouched.The PieFed adapter now requests one less when there's no
parent_id. End-to-end through the client against both live servers after the change:max_depth: 1→ depth 1 on both,max_depth: 2→ depths 1–2 on both.The fake's request decoder inverts the same adjustment so
callsTo("getComments")payloads stay canonical — and the matrix test that previously had to passmode === "piefed" ? 0 : 1now passes1to both, which is the clearest statement that the divergence is gone. A new matrix test also pins that the canonical payload survives the wire adjustment.Known edge: canonical
max_depth: 0("no comments") has no PieFed equivalent — its minimum still returns top-level — so it clamps rather than going negative. Documented at the call site.Consumer note: Voyager's e2e fixture currently encodes the old divergence (
INITIAL_COMMENT_DEPTH6 vs 7); that collapses to one value once this ships.