fix(api): send YouTube's utcOffsetMinutes sign, fixing YTMusicClient - #423
fix(api): send YouTube's utcOffsetMinutes sign, fixing YTMusicClient#423YuriNachos wants to merge 1 commit into
Conversation
sozercan
left a comment
There was a problem hiding this comment.
Could we double-check the sign convention here? YouTube’s current WEB and Music clients appear to send -Date.getTimezoneOffset(). Since Foundation’s secondsFromGMT() already has the opposite sign from getTimezoneOffset(), wouldn’t secondsFromGMT() / 60 be the correct equivalent?
For example, Los Angeles yields JS +420, while YouTube sends -420 and Foundation returns -420. Would the added negation therefore incorrectly send +420? If so, might YTMusicClient be the existing incorrect call site, with these new test expectations needing reversal?
`YTMusicClient` negated `TimeZone.secondsFromGMT() / 60` before putting it in the InnerTube `client` context, so every non-UTC user sent an inverted offset (Los Angeles went out as `+420` instead of `-420`). YouTube's own web client computes `-Date.getTimezoneOffset()`, and Foundation's `secondsFromGMT()` already carries the opposite sign from `getTimezoneOffset()`, so `secondsFromGMT() / 60` is already the value YouTube expects and no negation belongs anywhere. Route all three call sites through one tested `InnerTubeSupport.utcOffsetMinutes(for:)` helper and pin the convention with unit tests, including the Los Angeles reference datum. Co-authored-by: Claude <noreply@anthropic.com>
380fa3b to
e87b7d5
Compare
|
You are right, and my original premise was backwards. Thank you for catching it — I went and checked against an external reference rather than re-reasoning from the same assumption. The reference implementation agrees with you. YouTube.js utcOffsetMinutes: -Math.floor((new Date()).getTimezoneOffset()),So for Los Angeles, YouTube sends
Exactly as you said: What the PR does now, rewritten as one commit (
I kept the shared helper since it was the part you did not object to, and I squashed the branch so the false premise is not sitting in the history. Gate on the rewritten branch: One unrelated thing I noticed while running the suite locally, in case it is useful: the full run is nondeterministic on this machine. Three consecutive |
|
Addressed in |
Description
YTMusicClientnegatesTimeZone.secondsFromGMT() / 60before putting it into the InnerTubeclientcontext, so every non-UTC user sends an invertedutcOffsetMinutes(Los Angeles goes out as+420instead of-420).This PR removes the negation, routes all three call sites through one tested helper, and pins the convention with unit tests.
AI Prompt
Assisted with Claude Code. The prompt for the revision was, in substance: "The maintainer's review says the sign convention is inverted. Verify against an external reference implementation of the InnerTube context rather than against our own reasoning; if the maintainer is right, invert the change, make
YTMusicClientthe fixed call site, reverse the test expectations, and add a case pinning the Los Angeles datum from the review discussion. Prove the tests are non-vacuous by reintroducing the negation and showing them go red."Root cause
Three facts, each verifiable:
Date.prototype.getTimezoneOffset()returnsUTC − local, so Los Angeles yields+420.YouTube's web client sends
-Date.getTimezoneOffset(), i.e.-420for Los Angeles. In YouTube.js,src/core/Session.ts:Foundation's
TimeZone.secondsFromGMT()already carries the opposite sign fromgetTimezoneOffset():secondsFromGMT() / 60America/Los_Angeles-420America/New_York-240Asia/Kolkata+330Europe/Berlin+120UTC0So
secondsFromGMT() / 60is already the value YouTube expects, and no negation belongs anywhere.YouTubeClientandAskVideoAuditwere correct;YTMusicClientwas the one genuine bug.As a cross-check,
yt-dlpandNewPipeExtractorboth sidestep the question entirely by pinningtimeZone: "UTC"withutcOffsetMinutes: 0, which is consistent with the sign only mattering when the value is non-zero.Type of Change
Related Issues
None — self-found while reading the InnerTube context builders.
Changes Made
InnerTubeSupport.utcOffsetMinutes(for:)— new shared helper returningtimeZone.secondsFromGMT() / 60, with a doc comment stating the convention and why there is no negation.YTMusicClient.swift:1962— the actual fix: dropped the negation by routing through the helper.YouTubeClient.swift:692— routed through the helper; emitted value unchanged.AskVideoAudit.swift:406— unchanged value, with a comment explaining why it is inlined (APIExplorer cannot import the Kaset executable target).InnerTubeSupportTests.swift— 5 cases covering UTC-behind, UTC-ahead, UTC, the half-hour India offset, and the Los Angeles reference datum. All useTimeZone(secondsFromGMT:)so DST cannot move an assertion.Testing
swift build— cleanswift test --skip KasetUITests— 2958 tests, 231 suites, passingswiftlint --strict— 0 violations in 615 filesswiftformat --lint .— 0 files require formattingNon-vacuity check: reintroducing the negation in the helper turns 4 of the 9
InnerTubeSupporttests red; restoring it makes them green again. The tests genuinely constrain the sign rather than merely describing it.Checklist