Move the Worker API clients into core, on the config seam - #24
Move the Worker API clients into core, on the config seam#24playforge-coding wants to merge 2 commits into
Conversation
Phase 1 of decoupling apps/web from its bundler. Twelve modules — lessons, comments, users, profile, notifications, moderation, pixabay, aiSuggest, spellingWords, mcpOAuth, imagesClient and git/remote — each opened with const API_URL = import.meta.env.VITE_API_URL; which is Rsbuild-specific, substituted at build time, and absent everywhere else. They now read apiUrl()/hasApi() through the seam added with the browser tier, and move to core. Four exported flags turned out to be the same predicate under four names: lessonHubEnabled, notificationsEnabled, profilesEnabled and gitRemoteEnabled were all Boolean(API_URL). They are replaced by core's existing hasApi() at 43 call sites. They also had to go regardless: each was evaluated at module scope, so under a lazily-resolved config they would every one have been false. The lint rule from the browser-tier PR then caught what review would not have: lessons.js and users.js use DOMParser, so neither is runtime-neutral. Both were parsing an Atom feed, and on inspection with the same parser, the same entry shape and the same helper — a third copy-paste pair. That reading moves to browser/feeds.js as one implementation with the error wording injected, so each caller keeps the message its UI already showed. The URL builders stay neutral, which keeps the layering one-way: the browser tier imports the neutral tier, never the reverse. Every apiUrl()/hasApi() call site was checked to be inside a function body rather than at module scope, which is what makes the lazy read safe against import hoisting. Also drops a redundant `const base = () => apiUrl()` wrapper in users.js, and retires the "VITE_API_URL is not configured" error text, which named a variable core cannot see. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (43)
Comment |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Phase 1 of decoupling
apps/webfrom its bundler — the prerequisite for the SvelteKit plan, and worth doing on its own regardless.What moved
Twelve modules —
lessons,comments,users,profile,notifications,moderation,pixabay,aiSuggest,spellingWords,mcpOAuth,imagesClient,git/remote— each opened with:That's Rsbuild-specific, substituted at build time, and absent in Node, the Worker, or any other bundler. They now read
apiUrl()/hasApi()through the config seam introduced with the browser tier, and move to core.Four names for one predicate
lessonHubEnabled,notificationsEnabled,profilesEnabledandgitRemoteEnabledwere allBoolean(API_URL)— four exported flags computing the identical thing. Replaced by core's existinghasApi()at 43 call sites.They had to go regardless: each was evaluated at module scope, so under a lazily-resolved config every one of them would have been
false.The lint rule earned its keep
packages/coreis linted against theworkerenv, and it immediately rejectedlessons.jsandusers.jsfor usingDOMParser— so neither is runtime-neutral, which review would not have caught.On inspection both were parsing an Atom feed, with the same parser, the same entry shape and the same
entryTexthelper. A third copy-paste pair, afterwikimediaandrichText.That reading moves to
browser/feeds.jsas one implementation, with the error wording injected so each caller keeps the message its UI already showed. The URL builders (lessonsFeedUrl,userFeedUrl) stay neutral, which keeps the layering one-way: the browser tier imports the neutral tier, never the reverse.Import-hoisting safety
The seam only works because reads are lazy —
configureCoreruns after every module in the graph is evaluated. EveryapiUrl()/hasApi()call site was checked to sit inside a function body rather than at module scope, in both core and web.Also
const base = () => apiUrl()wrapper inusers.js."VITE_API_URL is not configured."error text, which named a variable core cannot see.Verification
pnpm install --frozen-lockfile,pnpm lint,pnpm test(core 38, api 27, mcp 14),pnpm build,pnpm build:docs— all pass. Bundle total unchanged.What's left in phase 1
supabase,turnstileandgoogleDrivestill readimport.meta.env.supabase.jsis the awkward one — it builds its client at module scope, so it needs a lazy singleton rather than a straight substitution. That's the next PR.