Skip to content

Fix review findings: router // paths, api responses, range/search/SlotArray, legacy dates - #9

Open
noah-meierlabs wants to merge 6 commits into
mainfrom
review-fixes
Open

noah-meierlabs wants to merge 6 commits into
mainfrom
review-fixes

Conversation

@noah-meierlabs

Copy link
Copy Markdown
Contributor

Fixes points 2, 12, 13, 16–21 of #6. One commit per package.

No API surface changes: no new or removed exports, parameters or types. The only behaviour that changes is the buggy behaviour.

chuchi

  • 2, // paths: pushState/replaceState get location.origin + uri instead of the bare uri. A same-host link like https://site.ch//evil.com/x used to throw a SecurityError (the uri //evil.com/x is read as another host). The URL that ends up in the address bar is the same as before.
    • crelte has the same bug: ClientRouter passes url.pathname + url.search + url.hash to pushState.

core

  • 12:
    • request returns null for an empty body (e.g. 204) instead of throwing a JSON parse error.
    • A non-JSON error response (e.g. a 502 HTML page) becomes OTHER: response code 502.
    • ApiError.fromJson no longer crashes on null, numbers or {}.
  • 13:
    • requestWithFile and requestTimeout accept every 2xx status, like request (before: only 200).
    • When the error body isn't JSON, they report the status code.
    • requestTimeout sends GET data as query params, like request (before: silently dropped).

utils

  • 16: range had an operator-precedence bug in its length. It now collects the values in a plain array: range(0, 10, 2) → [0,2,4,6,8], and range(-5, 5, 2) / range(5, 2) no longer throw.
  • 17: searchScore uses the previously unused searchSpaceRight for the right distance. Exact matches now rank better: foo/foo = 1, foo/foobarbaz = 7 (before: both 4).
  • 18: removing a SlotArray slot twice no longer hands out the same index twice.

legacy

  • 19: Date parses YYYY-MM-DD as a local date. Before, it was read as UTC midnight, so west of UTC it shifted a day on every round trip.
  • 20: ISO week uses the Thursday's year, with rounding for DST, in both Date and DateTime.
  • 21:
    • Duration picks the unit after rounding: 59.6s → "in one minute", 6.6 days → "in one week".
    • inViewY is true for an element taller than the viewport that covers it.

Testing

  • build, check-fmt, check, lint and check-jsr pass.
  • Router in headless Chromium: clicking a //evil.example/x same-host link works. On main it throws SecurityError: Failed to execute 'pushState'….
  • Api with mocked fetch/XMLHttpRequest: 204, 502 HTML, a JSON error, GET query params (also for requestTimeout), 201 and 204 over XHR.
  • week compared against the ISO week of every day from 2015 to 2030, for Date and DateTime (at 00:30 and 12:00), in America/New_York, Europe/Zurich, Australia/Sydney and UTC: 0 mismatches. A Date JSON round trip in New York keeps 2023-05-10.
  • Node scripts for range, searchScore, SlotArray and Duration.

🤖 Generated with Claude Code

noah-meierlabs and others added 5 commits October 30, 2026 05:40
The history got the uri alone, and a path like //evil.com/x is read as
another host, so pushState threw a SecurityError after the click was
already prevented and the request was left half open. The history now
gets the full url on the current origin.

Refs #6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
request returns null for an empty body (for example 204) instead of
failing with a json parse error, and an error response that is not json
(for example a 502 html page) becomes OTHER with the status code.
ApiError.fromJson no longer crashes on null, numbers or {}.

requestWithFile and requestTimeout accept every 2xx status like
request (before only 200), report the status if the error body is not
json, and requestTimeout sends the data of a GET as query params like
request instead of dropping it.

Refs #6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uble remove

range computed its length as end - start / step, so range(0, 10, 2) had
holes and range(-5, 5, 2) or range(5, 2) threw. It now collects the
values in a plain array.

searchScore measured the right distance with searchSpaceLeft, back over
the match itself, so an exact match scored like a match inside a longer
word. It now uses the unused searchSpaceRight from the last matched
character: foo/foo is 1, foo/foobarbaz is 7.

Removing a SlotArray slot twice added it to free twice, so two later
pushes got the same index and the first value was overwritten.

Refs #6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd inViewY

Date parsed YYYY-MM-DD with the js Date constructor, which reads it as
utc midnight, so west of utc it became the previous day and every
toJSON/parse round trip lost another day. It is now parsed as a local
date.

week used the year of the date instead of the year of its thursday, so
2024-12-30 was week 53 instead of 1 and 2021-01-01 week 0 instead of 53.
Checked against the iso week of every day from 2015 to 2030 in New
York, Zurich, Sydney and utc.

Duration picked the unit before rounding, so 59.6s was "in 60 seconds"
and 6.6 days "in 7 days". It now compares the rounded values.

inViewY returned false for an element taller than the viewport that
covers it.

Refs #6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A click that another handler already prevented, a click with another
button than the main one, and links into another window or frame
(_top, _parent, a named target, not only _blank) were opened by the
router. Found in zipp's studio, which had to stop such clicks itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collecting the values with push was about 3-4x slower than the old
preallocated array for 1000+ values. range now preallocates with the
correct length, which is as fast as before. A step of 0 or below
returns an empty array instead of looping forever.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant