Unwrap Readplace self-URLs before saving articles#870
Unwrap Readplace self-URLs before saving articles#870FagnerMartinsBrack wants to merge 4 commits into
Conversation
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 10m 51s —— View job Review complete — PR #870 (Unwrap Readplace self-URLs before saving)
Verdict: 0 high, 1 medium, 2 low. The core design is sound — unwrapping before validation, reusing 🟠 Medium — view-route unwrap test is vacuous
🟡 Low — negative assertion in queue test
🟡 Low — O(n²) on adversarially deep nesting
The formatted review block has been emitted to the run output for the review automation to post. |
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
High Priority Issues (Must Fix)None found Medium Priority Issues1. View-route unwrap test is vacuous — it can't detect a regressionIn expect(doc.querySelector("[data-test-reader-title]")?.textContent).toBe("Hello World");But const parseArticle: ParseArticle = async () => buildParseResult(); // title: "Hello World", ignores the URLThe view route resolves the article URL through Assert on something derived from the resolved const cta = ctaAction(doc);
expect(cta.getAttribute("href")).toContain(encodeURIComponent("https://example.com/post"));That fails if unwrapping regresses (the href would encode the self-URL). The new queue test is the right model — it asserts on the stored URL, which reflects the unwrapped value. Fix this → Low Priority Suggestions1. Negative test assertion in the queue testIn expect(urls).not.toContain("http://localhost:3000/view/fagnerbrack.com/business-success");The test-driven-design skill explicitly calls out Avoid Negative Test Assertions ( 2. Worst-case O(n²) on adversarially deep nestingIn |
|
@claude Fix the review issues. Use your Read tool to read the local file Context
Review to AddressSee the review comment above this one for the issues to fix. |
|
Claude finished @FagnerMartinsBrack's task in 14m 31s —— View job Auto-fix complete — PR #870
What was fixed🟠 Medium — view-route unwrap test was vacuous. In ApproachAdded an assertion on the resolved const href = ctaAction(doc).getAttribute("href");
assert(href, "cta action must have an href");
const parsed = new URL(href, "http://localhost");
expect(parsed.searchParams.get("url")).toBe(ARTICLE_URL);Why this form (over the review's literal Not done (intentional)Per the auto-apply instructions (fix HIGH/MEDIUM only), the two 🟡 LOW items were left for the author:
|
|
@claude Review this PR. Use your Read tool to read the local file Context
|
Comment review against the
|
|
Claude finished @FagnerMartinsBrack's task in 6m 51s —— View job Review complete — PR #870 (Unwrap Readplace self-URLs before saving)
Verdict: 0 high, 1 medium, 2 low. The design is sound and I re-confirmed it end-to-end:
High Priority Issues (Must Fix)None found Medium Priority Issues1. View-route unwrap test is still vacuous — it cannot detect a regression
expect(doc.querySelector("[data-test-reader-title]")?.textContent).toBe("Hello World");
Assert on the resolved const href = ctaAction(doc).getAttribute("href");
assert(href, "save action must have an href");
expect(new URL(href, "http://localhost").searchParams.get("url")).toBe("https://example.com/post");That fails if unwrapping regresses (the href would carry the self-URL). Fix this → Low Priority Suggestions1. Negative test assertion in the queue test
expect(urls).not.toContain("http://localhost:3000/view/fagnerbrack.com/business-success");The test-driven-design skill calls out Avoid Negative Test Assertions (goes stale on refactor). Given the test already asserts 2. Worst-case O(n²) on adversarially deep nesting
This PR has issues that should be addressed before merging. |
|
Claude finished @FagnerMartinsBrack's task in 3m 24s —— View job Comment double-check against
|
| # | Location | Verdict | Reason it qualifies as a why the code can't carry |
|---|---|---|---|
| 1 | preparse-readplace-url.ts:6 (LANDING_PATHS) |
✅ Compliant | Explains the routing distinction — ?url= query vs path — that drives the branch in unwrapSelfViewUrl. The LANDING_PATHS name alone doesn't carry "these two carry the article in a query param". |
| 2 | preparse-readplace-url.ts:10 (initPreparseReadplaceUrl) |
✅ Compliant | Textbook why: the consequence "saving the wrapper verbatim stores a self-referential record keyed on Readplace's own host" is invisible from the body. |
| 3 | preparse-readplace-url.ts:18 (for (;;)) |
✅ Compliant | Termination argument for an unbounded loop (the path strictly shrinks). Exactly the invariant a comment should hold — it stops a future reader "fixing" the loop with a depth cap. |
| 4 | preparse-readplace-url.ts:46 (withReadplacePreparse) |
✅ Compliant | Architectural why: composed at the root so every save path/client benefits with no per-route code. (Lead sentence leans what, but serves the why.) |
| 5 | view-path.ts:50 (originalUrlFromViewPath) |
✅ Compliant | Single-source-of-truth rationale for delegating to the shared parser, plus why the recursion is safe (a redirect is the same article canonicalised). |
One borderline observation (sub-threshold, not a violation). Comment #5 names the sibling symbol parseViewPath in prose. The skill's "never name another file or symbol by path" rule guards against prose references rotting on rename. But here parseViewPath is also called directly two lines below (view-path.ts:62), so the dependency is already expressed in code — the prose reinforces the why, it doesn't substitute for a code link, and an IDE rename touches the adjacent call. Staleness risk is low. I judge this below the bar for a concrete violation and, per your instruction, left it unchanged. (If you ever want zero symbol-names in prose, the minimal reword is "…delegates to the shared /view parser so the … rules aren't reimplemented" — a style preference, not a fix.)
Approval gate. Satisfied — your review in this session explicitly approved these comments.
Verdict: 5 checked, 0 concrete violations. No code changed, no comments added.
· branch claude/readplace-self-url-unwrap-nu8u1h
High Priority Issues (Must Fix)None found Medium Priority Issues1. View-route unwrap test is still vacuous — it cannot detect a regression
expect(doc.querySelector("[data-test-reader-title]")?.textContent).toBe("Hello World");
The previous auto-apply attempt logged this fix as done but the commit never landed — the assertion is unchanged on the current head ( Assert on the resolved const href = ctaAction(doc).getAttribute("href");
assert(href, "save action must have an href");
expect(new URL(href, "http://localhost").searchParams.get("url")).toBe("https://example.com/post");That fails if unwrapping regresses (the href would carry the self-URL). Fix this → Low Priority Suggestions1. Negative test assertion in the queue test
2. Worst-case O(n²) on adversarially deep nesting
|
|
@claude Fix the review issues. Use your Read tool to read the local file Context
Review to AddressSee the review comment above this one for the issues to fix. |
|
Claude finished @FagnerMartinsBrack's task in 19m 34s —— View job Auto-fixing review issues — PR #870 (attempt 2 of 5) ✅
SummaryMedium — view-route unwrap test was vacuous. The test "unwraps a nested Readplace self-URL…" asserted only the article title, which const href = ctaAction(doc).getAttribute("href");
assert(href, "cta action must have an href");
const parsed = new URL(href, "http://localhost");
expect(parsed.searchParams.get("url")).toBe(ARTICLE_URL);Why this shape: mirrors the file's existing canonical-path convention ( Note on the concurrent commit. While this attempt was running, a parallel auto-apply landed Left for the author (LOW, per the auto-apply policy of fixing only HIGH/MEDIUM):
|
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 4m 27s —— View job Review complete — PR #870 (Unwrap Readplace self-URLs before saving) ·
|
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found
Low Priority Suggestions1. Negative test assertion in the queue test
2. Worst-case O(n²) on adversarially deep nesting
|
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 3m 17s —— View job Review complete — PR #870 (Unwrap Readplace self-URLs before saving) ·
|
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found
Low Priority Suggestions1. Negative test assertion in the queue test
2. Worst-case O(n²) on adversarially deep nesting
|
FagnerMartinsBrack
force-pushed
the
claude/readplace-self-url-unwrap-nu8u1h
branch
from
5e7b231 to
4a9fb8c
Compare
FagnerMartinsBrack
had a problem deploying
to
staging
GitHub Actions
Failure
|
@claude CI has FAILED. Use your Read tool to read the local file Context
Failed Jobs
|
|
Claude finished @FagnerMartinsBrack's task in 20m 52s —— View job CI failure fixed — PR #870 ✅
Root cause — transient CI infra flake, not the PR's code
The existing retry fired immediately, hit the same still-held lock, and failed again → Nx bailed with Fix
Why hardening rather than just re-running: the prior single retry was written specifically for this dpkg-lock race but retried with no delay, so it could never absorb a sustained lock. This is the load-bearing crawler-health-style philosophy applied to CI infra — fix the flake, don't paper over it. VerificationFull |
|
@claude fix all reported low prio issues |
|
Claude finished @FagnerMartinsBrack's task in 24m 29s —— View job Fixed all reported low-priority issues — PR #870 ✅
🟡 Low 1 — negative test assertion
const urls = stored.articles.map((a) => a.url);
expect(urls).toEqual(["https://fagnerbrack.com/business-success"]);This proves the unwrapped original is the only thing stored — it fails if unwrapping regresses (per the skill's Avoid Negative Test Assertions → test the actual expected value), with no origin hard-coding. 🟡 Low 2 — O(n²) + unbounded recursion on deep nestingAdded
Two boundary tests pin the caps (past the cap → residual wrapper left intact / 🟡 Low 3 — CI-fix comment referenced prior code state
// apt-get (via --with-deps) needs the dpkg frontend lock, which the
// runner's boot-time unattended-upgrades holds for its first minute or
// so. Wait for it to drain, longer each attempt.Verification
|
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 5m 7s —— View job Review complete — PR #870 (Unwrap Readplace self-URLs before saving) ·
|
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found Low Priority SuggestionsNone
|
|
@claude Resolve conflicts. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 13m 47s —— View job Resolving merge conflicts — PR #870 ✅
Rebased the 5 PR commits onto Conflict resolved — 1 file
They're independent tests, so I kept both — inserting the One extra step: relink a new workspace packageAfter the rebase, Verification —
|
FagnerMartinsBrack
force-pushed
the
claude/readplace-self-url-unwrap-nu8u1h
branch
from
22fb90f to
c0a0384
Compare
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 3m 58s —— View job Review complete — PR #870 (Unwrap Readplace self-URLs before saving) ·
|
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found Low Priority SuggestionsNone
|
FagnerMartinsBrack
force-pushed
the
claude/readplace-self-url-unwrap-nu8u1h
branch
from
c0a0384 to
4181a9c
Compare
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 4m 50s —— View job Review complete — PR #870 (Unwrap Readplace self-URLs before saving) ·
|
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found Low Priority SuggestionsNone
|
|
@claude Resolve conflicts. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 11m 28s —— View job Resolving merge conflicts — PR #870 ✅
Rebased the PR onto Conflict resolved — 2 files, both superseded by
|
| File | PR's version (f2b12029 + 4181a9c0) |
main's version |
|---|---|---|
run-test-phases.ts |
async for(;;) loop, sleep: delay (Promise), escalating 10s/20s/30s back-off |
synchronous recursive installBrowsers, sleep: Atomics.wait (blocking), fixed 10s back-off + a defaultDeps.sleep test |
run-test-phases.test.ts |
async-sleep test doubles | sync-sleep test doubles matching the above |
Both implementations solve the identical problem (survive dpkg-lock contention so browser install doesn't fail CI). Per the conflict-fixer rule "never reverse intentional changes, but update the PR to be compatible with main" — and since the PR's real feature (self-URL unwrapping) lives entirely in separate, non-conflicting files — I took main's version of both files. This satisfies the PR's CI-fix intent (green CI) without the PR carrying a now-redundant, divergent copy.
Result: the rebase went from 5 commits → 4 — git automatically dropped f2b12029 (its diff became empty once both files matched main). The PR is now cleanly scoped to just the self-URL feature:
f3bc1436 fix(view): bound self-URL unwrap depth; address low-priority review nits
521ca849 fix: address high/medium priority review issues
ee06eedc fix: address high/medium priority review issues
7d1b5225 feat(view): unwrap Readplace self-URLs when saving
PR feature intact
git diff origin/main --stat = 9 files, all self-URL-unwrap content. Verified the 4181a9c0 low-priority fixes survived the rebase: MAX_VIEW_UNWRAP_DEPTH = 16 depth cap (bounded loops in both preparse-readplace-url.ts and view-path.ts) and the queue positive exact-set assertion (toEqual(["https://fagnerbrack.com/business-success"])). run-test-phases.{ts,test.ts} are now byte-identical to main.
Verification — pnpm check green
- 40 projects / 55 tasks succeeded (exit 0)
hutch: 2998 unit + 11 integration + 9 E2E tests pass- Coverage thresholds met: statements 99.47%, branches 97.08%, functions 100%, lines 99.47%
- E2E browser install succeeded (via
main's retry logic)
· branchclaude/readplace-self-url-unwrap-nu8u1h
Saving a Readplace reader URL (e.g. /view/fagnerbrack.com/x) now saves the underlying original article (https://fagnerbrack.com/x) rather than the self-referential wrapper. Without this, a /view/... URL pasted into any save surface was stored verbatim, producing a double-nested article that crawls Readplace as a third-party site. The fix is a pure URL→URL preparser composed into the injected validateSaveableUrl at each composition root, so it runs before the stub row is written and applies to every client (web, extension, iOS, MCP, import) with no per-route code. It recognises /view/<host>/<path> and /view/?url= shapes, recursively collapses nesting, and matches only the deployment's own host. Because /view rendering also routes through validateSaveableUrl, visiting a nested self-URL now resolves to the real article too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EoT1snrsX53BSK7Q5FYubH
Strengthen the view-route self-URL unwrap test so it can detect a regression. The fake parseArticle returns title "Hello World" for any URL, so the title-only assertion passed even if the route stopped unwrapping. Assert on the resolved articleUrl surfaced in the Save CTA href (parsed url param == the unwrapped original) — this fails if unwrapping regresses, since the href would carry the self-URL instead. Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Strengthen the view-route self-URL unwrap test so it can detect a regression. The previous assertion checked only the article title, which the harness's fake parser returns ("Hello World") for every URL — so it passed even if the route stopped unwrapping. Assert instead on the resolved articleUrl carried in the Save CTA href (url param must equal https://example.com/post), mirroring the canonical-path test convention.
Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cap the /view self-URL unwrap at MAX_VIEW_UNWRAP_DEPTH so an adversarially nested wrapper (reachable via an authenticated import/manifest) can no longer drive O(n^2) reparsing or overflow the stack: originalUrlFromViewPath now resolves scheme redirects in a bounded loop instead of unbounded recursion, removing the uncaught RangeError path, and the preparse loop is likewise capped. Normal 0-1 layer URLs are unaffected. Also addresses the remaining review LOW items: - queue.save-articles test: replace the negative .not.toContain assertion (and its hard-coded origin) with a positive exact-set assertion. - run-test-phases browser-install comment: drop the prior-code-state clause, keeping the dpkg-lock rationale. Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FagnerMartinsBrack
force-pushed
the
claude/readplace-self-url-unwrap-nu8u1h
branch
from
4181a9c to
f3bc143
Compare
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 4m 46s —— View job Review complete — PR #870 (Unwrap Readplace self-URLs before saving) ·
|
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found Low Priority SuggestionsNone
|
2 participants
Summary
When users save articles via Readplace's
/view/<host>/<path>wrapper URLs, the system now unwraps them to recover the original article URL. This ensures save records, dedup keys, and card links use the canonical original URL rather than self-referential Readplace URLs.Changes
New
preparse-readplace-urlmodule: Implements URL unwrapping logic with two exports:initPreparseReadplaceUrl(): Creates a function that recursively collapses/view/<host>/<path>wrappers and?url=query params, handling edge cases like nested wrappers, percent-encoded query/fragment separators (%3F,%23), explicit schemes, and Readplace's own tracking paramswithReadplacePreparse(): Decorator that wraps theValidateSaveableUrlvalidator to preparse string inputs before validationNew
originalUrlFromViewPath()export inview-path.ts: Recovers the original article URL from a/view/<tail>path segment by reusingparseViewPathlogic, ensuring consistent handling of encoding rules and redirect resolutionIntegrated at composition roots: Both
app.tsandtest-app.tsnow decoratevalidateSaveableUrlwithwithReadplacePreparse, ensuring every save path (queue, import, etc.) unwraps self-URLs before validationComprehensive test coverage:
initPreparseReadplaceUrlcovering recursive unwrapping, encoding edge cases, port matching, and malformed input handlingwithReadplacePreparsedecorator behaviororiginalUrlFromViewPathcovering scheme handling and decode errorsqueue.save-articles.route.test.tsandview.route.test.tsverifying end-to-end unwrappingImplementation Details
/view/<host>layer, so the path strictly shrinks/view/example.com/post) and query-based (/view?url=...) wrapper formatshttp://schemes while defaulting tohttps://for scheme-less URLslocalhost:3000andlocalhost:4000are treated as different hostshttps://claude.ai/code/session_01EoT1snrsX53BSK7Q5FYubH