fix(chrome-extension): keep background.mjs a text file - #1969
Conversation
A stray NUL byte in the candidate key's separator made git classify background.mjs as binary, so it shipped in #1967 with no readable diff. The review bot said as much, having to anchor a comment about that file onto an unrelated test line because a binary diff accepts none. The separator is now written as an escape, which is the same character without the file-level consequence. Also scope the tile-template brace restoration in serviceUrlParameter to the kinds that use `{z}/{x}/{y}`. Rewriting `%7B`/`%7D` for every kind would turn a brace that a WMS or ArcGIS URL legitimately encodes, in a signed parameter or token, into a literal the server never sent.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesService URL decoding
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR keeps the extension background file readable and limits URL brace restoration to supported template services; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code reviewReviewed the two functional changes in this PR: the Bugs: None found. The Security: None found. This actually improves an existing minor risk: previously a WMS/WFS/ArcGIS URL with a legitimately percent-encoded brace (e.g., in a signed token) could be silently corrupted into a literal brace before being sent to the origin server; now that rewriting is scoped away from those kinds. (Confidence: high) Performance: No concerns — the change adds a single Quality: Clean, well-scoped change with a clear explanatory comment and matching test coverage for both directions (WMS token preserved, WMTS template restored). No readability or naming concerns. CLAUDE.md: No applicable guidelines were violated; this is a small, self-contained bug fix consistent with the project's conventions (branch + PR, no direct main commits implied by this being a PR). No inline comments posted — I didn't find anything rising to a reportable issue in this diff. |
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
Follow-up to #1967.
background.mjs shipped as a binary file
A stray NUL byte sat in the separator of the candidate key, where a plain
separator was meant:
One NUL in the first 8000 bytes is all it takes for git to classify a file as
binary, so
background.mjsmerged with no readable diff. That is not cosmetic:the review bot had to anchor its comment about that file onto an unrelated line
in
tests/chrome-extension.test.ts, because a binary diff accepts no linecomments. Written as the escape ``, it is the same separator with the file
staying text.
Scoped brace restoration
From a review thread on #1967:
serviceUrlParameterrestored%7B/%7Dtobraces across the whole URL for every service kind, though only
xyz,wmtsand
ogc-vector-tilesuse{z}/{x}/{y}templates. For the others that rewritecould corrupt a URL whose query legitimately encodes a brace, in a signed
parameter or token, into one the origin server never sent. Restoration is now
limited to the three template kinds, with a test covering both directions.
Not addressed here
Two other observations from the last review round on #1967 are left open
deliberately:
stylesByTab,lastWrittenand the page scope live in module scope, so an evicted workerforgets a style it had already seen and a later tile can land without one. It
degrades rather than breaks (the entry is still addable, the style field is
just empty), and persisting that state is its own change.
VERSIONthe page negotiated is not carried over. A 1.3.0-onlyserver could fail the first Add before the user hits Retrieve. Worth doing for
the "no further typing" bar the README sets, as its own change.
Tests: 6285 pass. Verified the file reads as text again with
fileandgit diff.Summary by CodeRabbit
Bug Fixes
{z},{x}, and{y}placeholders.Tests