fix(analytics): skip the identity lookup when no token is set - #255
Conversation
On a public page the analytics module still issued a `User/me` at client construction: `trackInitializationEvent` queues an event, the processor flushes the first batch immediately, and `flush` awaits `getSessionContext`, which calls `auth.me()` unconditionally. With no session that request can only answer 401, and the browser logs it to the console from the network layer -- before any handler in the SDK sees the rejection. The existing `.catch` in `getSessionContext` and the try/catch in `flush` already keep the rejection from escaping, so no amount of additional catching removes what users actually see. Skip the lookup when no access token is set. Anonymous events already reported `user_id: null` through that same `.catch`, so no analytics data changes -- the request is dropped only in the case where its sole possible outcome was a 401. The dedupe from #245 is unaffected: when a token is present, analytics still resolves through `auth.me()` and shares the app's in-flight request, so an authenticated cold load continues to issue exactly one `User/me`. Token presence is tracked in the auth module rather than read off `axios.defaults`, so it follows the identity transitions that already exist there (`setToken`, `logout`) and is seeded from the constructor token, which is how the server-side SDK carries a session it never sets explicitly. The skip is deliberately not memoized: a visitor who logs in mid-session must start resolving an identity again. This does not cover a stale or expired token, which still 401s. Removing that one needs `User/me` to answer 200 with a null user instead of 401, which is a backend change. Tests: the two analytics tests that exercise identity resolution now build a token-bearing client; the axios mock gains the `defaults` that `setToken` and `logout` write through. Three new tests cover the anonymous skip, resolution resuming after `setToken`, and token presence across identity changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MtKFfzaVR6nWAi8tNvJqfw
|
Claude finished @guyofeck's task —— View job Claude Code ReviewReviewed all 5 changed files. This is a clean, well-reasoned PR. The extensive commit message and inline comments match the actual behavior, and the fix is correctly scoped. Below is my feedback.
SummaryThe fix is correct and minimal. Strengths
Observations / Minor Points
Bugs / Security / Performance
Test CoverageSolid. Three new tests cover the three behaviors that matter:
The one gap, acknowledged in the PR, is that there's no browser-level assertion that the console stays clean — the fix is verified at the request level, which is the appropriate boundary for a unit test here. VerdictLooks good to merge. The change is small, correct, well-tested, and thoroughly documented. No blocking issues. --- • |
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/sdk@0.8.42-pr.255.c035d2fPrefer not to change any import paths? Install using npm alias so your code still imports npm i "@base44/sdk@npm:@base44-preview/sdk@0.8.42-pr.255.c035d2f"Or add it to your {
"dependencies": {
"@base44/sdk": "npm:@base44-preview/sdk@0.8.42-pr.255.c035d2f"
}
}
Preview published to npm registry — try new features instantly! |
|
Two checks are red, and neither is caused by this diff. Recording the diagnosis so it isn't re-investigated at review time.
This reproduces on #254, a CI-only PR that touches no source files, so it predates and is independent of this change. It's the failure mode anticipated in #248's checklist —
The gateway is refusing that tarball, consistent with the 14-day Both need workflow-level changes (proxying the audit endpoint or moving to the bulk endpoint; allowing or pinning the agent-sdk tarball). I've deliberately left them alone rather than editing workflow files from a behavioral PR — they belong in their own change, alongside the other #248 follow-ups. The checks that do exercise this diff — Generated by Claude Code |
Review feedback on #255: despite @internal, hasToken() landed in the published .d.ts (tsc does not strip @internal members without stripInternal), so every consumer saw it on base44.auth. Move it to InternalAuthModule, an extension of AuthModule that only the factory and the analytics module reference. It is not exported from the package index, and Base44Client.auth stays typed as AuthModule, so the runtime object still carries the method but the public type surface is unchanged from before this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MtKFfzaVR6nWAi8tNvJqfw
|
Claude finished @guyofeck's task —— View job Claude Code Review
SummaryRe-reviewing after the The
|
Problem
On a public page, the SDK issues a
GET /entities/User/methat can only ever answer 401, and the browser writesFailed to load resource: the server responded with a status of 401 (Unauthorized)to the console. Users read that as a broken app. (reported here — app_id69f14bfdfba5c17e67d8e81b, where the user won't ship while there are console errors.)The request comes from analytics, not from the app.
createAnalyticsModuleruns at client construction →trackInitializationEventqueues an event → the processor flushes the first batch immediately →flushawaitsgetSessionContext→auth.me(), unconditionally, whether or not a session exists.Catching it harder does not help, and that is the main thing to know about this PR. The rejection is already handled at three levels:
analytics.ts:343.catch(() => ({ user_id: null, session_id }))analytics.ts:121try { ... } catch { /* do nothing */ }influshaxios-client.ts:261Base44Error, consumed by the aboveNothing escapes as an unhandled rejection. The console line is emitted by the browser's network stack before any JS handler runs, so no
try/catchcan suppress it. The only way to remove it is to not send the request.Fix
Skip the identity lookup when no access token is set:
Anonymous events already reported
user_id: nullthrough the existing.catch, so no analytics data changes — the request is dropped only in the case where its sole possible outcome was a 401.Token presence is tracked in the auth module rather than read off
axios.defaults, so it follows the identity transitions that already live there (setToken,logout) instead of a header a caller may have set on the instance directly. It is seeded from the constructor token, which is how the server-side SDK carries a session it never sets explicitly — without that seeding this would have silently stopped resolvinguser_idin backend functions.The skip is deliberately not memoized: a visitor who loads a page anonymously and then logs in has to start resolving an identity again.
This does not revert or weaken #245
#245 removed a duplicate
User/meon authenticated cold loads by sharing the in-flight promise inauth.me(). That optimization is untouched and still exercised: when a token is present, analytics resolves throughauth.me()exactly as before and shares the app's in-flight request.me()me()The two PRs address different halves of the same endpoint: #245 cut the redundant request for logged-in users, this one cuts the impossible request for anonymous ones.
Scope — what this does not fix
A stale or expired token in localStorage still produces one 401. A token is present, so the request goes out; its validity isn't knowable client-side. Removing that one requires
User/meto answer200with a null user instead of401— a backend change, not this repo. If the goal is "zero 401s on/User/meunder any condition", that is the change to make, and this PR is not a substitute for it.Also unchanged:
[Base44 SDK Error] 401: ...fromsafeErrorLog(axios-client.ts:255) is already gated behindprocess.env.NODE_ENV !== "production"and is stripped from prod builds. Worth confirming published apps are served as production builds, otherwise that line appears alongside the browser's.As with #245, the SDK is pinned at
^0.8.xin the app templates, so apps pick this up on their next install or rebuild — the deployed fleet trails.Testing
npm run lint,npm run build,npm run test:typesclean.npm run test:unit— 204 pass (3 new).New tests:
me()when no token is setsetToken(guards the not-memoized behavior)hasToken()trackssetToken→logoutVerified the first genuinely fails with the gate disabled, rather than passing vacuously.
Two existing tests changed, and the reason is worth a look rather than a rubber stamp:
should not restore the pre-reset identity when a lookup settles lateandshould track multiple eventsboth exercise the identity-resolution path, so their client is now token-bearing. With the token restored, their batching and throttle assertions pass unchanged — which is what rules out a regression in the processor loop, since removing anawaitfromflushshifts microtask ordering. The file's axios mock also gains thedefaultsobject thatsetToken/logouthave always written through.Not covered: no browser-level test asserting the console stays clean; the fix is asserted at the request level.
Generated by Claude Code