Conversation
…dules obtain the correct jsnums library that now closes over an appropriate errbacks parameter
Joe says: This seems fairly straightforward. It does slightly change the permissions dialog to say “Email and profile photo” – we don't want or ask for anyone's profile photo, but we have to use the `profile` scope for the new API. There's not really a way around this that I can see; there's no email-only API. Claude says: The Legacy People API is deprecated and cannot be enabled for new Cloud Console projects. This migrates all user profile lookups to the modern People API (people/v1), which is the supported replacement. Changes: - Load 'people' v1 instead of 'plus' v1 via gwrap - Use resourceName/personFields params instead of userId - Update response field access (emailAddresses, names) - Add 'profile' OAuth scope required by People API Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… a lot smarter about our rounded corner borders
make sure rotate elts in the table (like roughnums!) don't punch through
This seems like it is a race on loading sheets and checking if sheets is loaded, which caused an array to be given to callers of gwrap.load rather than the single API that was asked for, breaking People API calls. Added a new case to handle asking for a specific API even when multiple new ones are seen in processDelta, and a log so we can check if the array is ever returned We are super suspicious of this code even needing to exist. It seems like an extra, complex, over-abstracted wrapper around just using gapi directly. This commit also has a change to make sure if LOG_USER or LOG_URL aren't set, they don't stop the logger from loading.
Joe says: We really want to make sure that shareurl works from raw.githubusercontent.com. Notably this proxies *only* that domain for now, specifically to make sure we support this common use case. In general I expect that less constrained users (e.g. ugrad) just fetch code happily with CORS from the browser in the long run. This also comes with some improvements to proxying, extracted into a helper: - Real size limits - Streaming, not blocking waiting for the whole response - CSP headers so if someone tries to load something executable (e.g. in SVG) it really can't run - Some filtering on redirects to avoid weird cases of redirecting to 127.0.0.1 inside the server Since we have existing proxying for images it's nice to improve this and have a good helper. Claude says (lightly edited by Joe): Some classrooms will likely load curriculum via #shareurl from raw.githubusercontent.com on networks that block GitHub. The new /load-shareurl endpoint fetches an allowlisted URL on the browser's behalf and streams the response back, so those classrooms get a working path. A small window.fetch shim in beforePyret.js routes any fetch to an allowlisted host through the proxy automatically. That covers every browser fetch path in one place: drive.js's makeUrlFile, the url/url-file import prefetches in cpo-main.js, and the Pyret runtime's F.fetch via cross-fetch. /downloadImg moves to share the same streaming-fetch helper, picking up nosniff + sandbox response headers, an IP-filtering HTTP agent (request-filtering-agent), a response size cap, and a request timeout. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude says (Joe felt little need to edit): Replaces the four magic numbers in the /downloadImg and /load-shareurl proxy call sites with named module-level constants. Bumps the image proxy to 20 MB / 30 s so that fetches of phone-sized images and slow Drive ?export= responses don't get cut off; /load-shareurl stays at 1 MB / 10 s since it's plaintext from a fast host. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Joe says: Constraints: - Don't want to proxy everything (be kind to our servers) - Don't want to wait a long time for the direct request to fail (flaky network, weird school blocker) So we kick off both the proxy and the direct request the first time we have a proxy-able candidate. - If the direct request succeeds, cancel the proxy request to CPO and do direct requests for that domain from this page - If the direct request fails, use only proxy requests for that domain for this page A direct request counts as success if it returns 200 OK and text/plain (making sure we don't weirdly accept 200 OK with like... a HTML login page because of a school SSO portal) I think this is a good balance of server load and letting working clients do their thing. Claude says: Responding to Ben's PR feedback to do better than always-proxy. The first fetch to an allowlisted host now runs direct + /load-shareurl in parallel; whichever returns first that verifies (direct = 2xx text/plain) is served to the caller, and the outcome of the direct request locks per-host shouldProxy state for the rest of the page-load. So unrestricted networks pay the proxy hop only once and rapidly switch to direct; blocked networks see direct fail and switch to proxy. Header-only verification (rather than body comparison) lets us abort the in-flight proxy fetch as soon as direct is trusted, keeping server load near zero on the healthy path. proxyStreamFetch now also tears down the upstream connection when the client disconnects, so the abort actually saves bandwidth on the server side too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Joe says:
I think I constrained Claude so much in this session that it had to say things
I like below :-)
This is post-convo with Ben where we were still unhappy with (a) variable names
and (b) closed over mutable variables instead of declarative promise APIs.
Claude says:
Following a second pass of code review: the previous version used a
new Promise((resolve, reject) => { let gotSomeResponse = false; ... })
block with a flag tracking which side had won. That conflated two
concerns — "first success wins the response" and "abort proxy iff direct
beat it" — into one imperative state machine.
This pass separates them. The race is expressed as three composed
Promise primitives:
- directP: direct fetch, gated on content-type verification (rejects
if direct returned the wrong shape, so it drops out of the response
race entirely).
- shouldProxyPromise (Promise.race): false iff direct verified before
the timeout; locks the per-host policy for the page-load.
- directFinishedSuccessfullyAndFirstP (Promise.race): does direct
settle before proxy, AND was that settlement a verified success?
Only then do we proxyCtrl.abort(). This keeps the abort safe — we
never abort once proxy has already returned headers, which would
error the caller's body stream mid-read.
- responsePromise (Promise.any): whichever of directP or proxyP
fulfills first.
Tested both healthy and DevTools-blocked flows. Observed the timing
edge case where both proxy and direct return 200 within a few ms of
each other: shouldProxy still locks 'direct' as expected, since the
verdict is decoupled from whether the abort caught proxy in time.
Also adds a comment explaining the deliberate non-handling of caller-
provided init.signal: the signal overwrite means a caller aborting
won't cancel the proxy fetch, but the alternative (event-listener
forwarding or AbortSignal.any) is more complexity than the
not-quite-fully-aborted case justifies right now.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add /load-shareurl proxy for hosts blocked on some school networks
corresponding changes to brownplt/pyret-lang#1839
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
3 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.
Recent changes