Make the factorioprints comment agree with itself, and strip www. once - #294
Merged
wormeyman merged 1 commit intoSep 1, 2026
Conversation
The comment's first paragraph said rewriting an /api/blueprintData/<sha>/ position/<i> link to firebase "would drop the position and open the book"; its second paragraph said the same rewrite 200s with a null body and throws. Only the second is true - `pathParts[1]` of an API path is the literal `blueprintData`, so the rewrite fetches blueprints/blueprintData.json, a record that does not exist. Put the `pathParts[1]` explanation back (the pre-FactoryGameFan#291 comment had it) and drop the book consequence, so both paragraphs describe the same failure. Also hoist the `www.` strip into one `const host` read by both the switch discriminant and the factorioprints host check, instead of two copies that nothing keeps in agreement. Pure refactor, no behaviour change. While in there, note on `case 'factorio'` that it keys on the path alone because no other factorio.<tld> is known to reach it, not because that is inherently safe - FactoryGameFan#291 contrasted the two arms without saying so. Closes FactoryGameFan#293 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
wormeyman
merged commit Sep 1, 2026
65eec0b
into
FactoryGameFan:wormeyman-space-age-support
9 checks passed
This was referenced Sep 1, 2026
wormeyman
added a commit
that referenced
this pull request
Sep 1, 2026
Comments only, in tests/blueprint-sources.spec.ts. No code or test change. The first said the firebase record a factorioprints API link rewrites to "holds only the whole book - so a rewrite here would load the book and lose the position". That is the claim #293 refuted: `pathParts[1]` of an API path is the literal string `blueprintData`, not a blueprint key, so the rewrite asks for `blueprints/blueprintData.json`, which answers 200 with a body of `null`, and `data.blueprintString` throws. It never reaches the book. The next test in the file already pins that exact target, so the comment now points at it. The second said that without the `www.` strip a link to `www.factorioprints.xyz` "would take the firebase rewrite". That was true while the host check had its own copy of the strip. #294 hoisted the two copies into one `const host`, so deleting the single strip now keys the switch on `www`, which falls to `default` - the same `fetchData(url.href).then(r => r.text())` as the pass-through, so this test stays green. Measured on the merged head: deleting it fails exactly one test, `factorio.school reads the doubly nested blueprintString`. The comment says so, and points the strip's own coverage at that test. Verified with `vp check` and `npx playwright test tests/blueprint-sources.spec.ts` (15 passed). Follow-up to #294 and #293. Claude-Session: https://claude.ai/code/session_014hQpEsZojx1pWE9kHn75MH Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Closes #293.
The contradiction
The
factorioprintsarm's comment inbpString.tshad two paragraphs that disagreed:/api/blueprintData/<sha>/position/<i>link to firebase "would drop the position and open the book instead of the blueprint that was linked."nullanddata.blueprintStringthrows."The second is right. The rewrite uses
pathParts[1], which for an API path is the literal stringblueprintData, not a blueprint key, so it fetchesblueprints/blueprintData.json— a record that does not exist — and throws on the parse. It never reaches the book.The pre-#291 comment had the accurate
pathParts[1]explanation; #291's review asked for a wrong trailing clause to be fixed, and the rewrite dropped the accurate half with it. This restores it and drops the book consequence, so paragraph one now describes the same failure as paragraph two (same shape as #284).While in there: strip
www.oncebpString.tsstripped a leadingwww.twice — once to pick the switch label, once to compare the host in the factorioprints arm — with nothing keeping the two copies in agreement (per #293, they were held up by different spec mutations). Hoisted into oneconst hostabove the switch, read in both places. Pure refactor, no behaviour change.Note added, not a task
Added a sentence on
case 'factorio'explaining it keys on the path alone because no otherfactorio.<tld>is known to reach it — not because that is inherently safe. #291's comment contrasted the two arms without saying why the school arm is fine without a host check.Verification
Comment change plus a hoist of an identical expression into a
const. The Vite+ toolchain isn't installed in my environment so I couldn't runvp check/ the Playwrightblueprint-sources.spec.tslocally; CI covers both. The twowww.-strip mutations from #293 (a www.factorioprints.xyz API url is passed through unchangedandfactorio.school reads the doubly nested blueprintString) both now exercise the singlehostbinding.🤖 Generated with Claude Code