fix(widget): merge anonymous visitor history in cookie authentication mode (#104) - #129
fix(widget): merge anonymous visitor history in cookie authentication mode (#104)#129rishu685 wants to merge 3 commits into
Conversation
…e authentication mode (extra-org#104)"" This reverts commit a14b09b.
|
Thanks for re-opening this as a clean follow-up to #127. The restore itself looks correct and CI is green, but I think one blocker from the previous review is still present. In cookie mode, async current(): Promise<string | null> {
if (!this.cached || (this.isCookieMode() && this.storedPass() !== null)) {
await this.resolve(() => this.storedPass());
}
return this.cached;
}This solves the zero-code same-SPA login case, but while the user is still signed out it can cause every API request to perform a blocking So anonymous usage can pay an extra network round-trip on every request until login happens. There is already a /** Avoid repeating unauthenticated claim attempts for the same pass in cookie mode. */but it is currently unused, which looks like this case was anticipated but not completed. I think we should keep the zero-code login detection while avoiding a blocking link attempt on every anonymous request. I don’t want to prescribe the exact mechanism — retry/backoff/state tracking are all reasonable — but the behavior should ensure that:
It would also be good to add a regression test with multiple consecutive anonymous requests and assert that Once that is addressed, I think this should be very close to approval. |
…cookie mode - Use lastClaimAttemptPass and CLAIM_RETRY_INTERVAL_MS (15s) to avoid repeating blocking POST /auth/link calls on every consecutive anonymous request - Preserves zero-code cookie login hand-off after retry interval or reset - Adds regression unit test verifying multiple consecutive anonymous turns do not repeat /auth/link
|
Thanks @Asaf-prog! Great catch on throttling anonymous retries. I've implemented How it works:
All 937 pytest tests, 40 Playwright E2E tests, and widget unit self-checks pass cleanly. Ready for final review! |
|
Thanks — the throttling improvement solves the repeated I still see one lifecycle issue before approving. With the 15-second cooldown, this flow is possible: So we avoid repeated anonymous link attempts, but we now have a window where the first authenticated request after login can still observe incomplete history. The new unit test also does: loggedInViaCookie = true;
tokens.reset();before verifying the successful merge. That proves the flow works when identity is explicitly reset, but the original requirement for cookie mode is the zero-code case where the host does not need to call I think we need to preserve both properties:
I don’t want to prescribe a specific implementation, but the current fixed cooldown alone does not give us a reliable signal that authentication changed. Could we also add a regression test for the actual zero-code transition: One additional point: if the proposed solution relies on a timeout, sleep, polling interval, or any other fixed time-based delay, I’d like to see a clear justification for why that specific timing is correct and what invariant it is enforcing. A magic delay should not be used to approximate an authentication state transition unless there is a concrete reason it is safe and reliable. Once that lifecycle is deterministic without host intervention, I think this will be ready to approve. |
… and deterministic history hand-off - Remove CLAIM_RETRY_INTERVAL_MS magic cooldown delay - Add cookie snapshot tracking (document.cookie) and window focus/visibility listeners to TokenSource - Ensure listConversations passes forceCheck to guarantee POST /auth/link completes before GET /conversations - Add zero-code transition regression test in widget.test.mjs
|
I completely agree magic time-based cooldowns were introducing race condition windows where login history couldn't be deterministically observed. I've eliminated 1. Event-Driven & Cookie Snapshot Tracking (
|
Summary
Restores the changes from #127 (re-submitted following accidental merge & revert on
main).Fixes #104 by enabling automatic visitor history hand-off when using cookie-based authentication (
host_tokenmode). Visitors who start chatting anonymously and subsequently sign in via a host session cookie will have their pre-login conversations merged into their account without requiring page reloads or host-siderefreshIdentity()calls.What Changed
1. Zero-Code Cookie History Hand-off (
tokenSource.ts)TokenSource.current()in Cookie Mode (isCookieMode()) to re-evaluate identity resolution whenever an unlinkedstoredPass()exists inlocalStorage.claimVisitorHistory(null)withcredentials: "include"./auth/linksucceeds (conversations_moved > 0), the visitor pass is cleared fromlocalStorage,this.cachedbecomesnull(session cookie speaks), and all subsequent calls returnnullimmediately with 0 overhead.2. Deterministic Execution Ordering
void claimVisitorHistory()toawait claimVisitorHistory()inhostToken().POST /auth/linkfinishes before token resolution completes, ensuring the firstGET /conversationsor history request after login observes the merged history.3. Clear Pass Lifecycle
localStorageonly if(data?.conversations_moved ?? 0) > 0(orhostToken !== nullin Bearer mode). Ifconversations_moved === 0(user still signed out), the pass is kept intact so pre-login chatting continues seamlessly.4. Test Suite Coverage
widget.test.mjs: Unit test for same-SPA zero-code cookie mode hand-off without page reload.widget.spec.ts: Playwright E2E testvisitor pass cached, cookie login hand-off merges history and first thread list request observes merged threads.test_api.py: Backend integration tests for cookie-authenticated/auth/link(200 OK) and unauthenticated attempts (401 Unauthorized).Verification
npm run build:widget— Clean build ✅npm run test:widget—widget self-check: OK✅npm run typecheck:widget&npm run typecheck:e2e— 0 errors ✅ruff&mypy— 0 lint/type errors across 56 source files ✅pytest— 937/937 passed ✅playwright test— 40/40 passed ✅Closes #104.