Skip to content

fix(widget): merge anonymous visitor history in cookie authentication mode (#104) - #129

Open
rishu685 wants to merge 3 commits into
extra-org:mainfrom
rishu685:fix/issue-104-cookie-mode-merge-v2
Open

fix(widget): merge anonymous visitor history in cookie authentication mode (#104)#129
rishu685 wants to merge 3 commits into
extra-org:mainfrom
rishu685:fix/issue-104-cookie-mode-merge-v2

Conversation

@rishu685

Copy link
Copy Markdown
Contributor

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_token mode). 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-side refreshIdentity() calls.


What Changed

1. Zero-Code Cookie History Hand-off (tokenSource.ts)

  • Updated TokenSource.current() in Cookie Mode (isCookieMode()) to re-evaluate identity resolution whenever an unlinked storedPass() exists in localStorage.
  • When a visitor signs in via cookie in the same SPA, the next widget action automatically triggers claimVisitorHistory(null) with credentials: "include".
  • Once /auth/link succeeds (conversations_moved > 0), the visitor pass is cleared from localStorage, this.cached becomes null (session cookie speaks), and all subsequent calls return null immediately with 0 overhead.

2. Deterministic Execution Ordering

  • Reverted background void claimVisitorHistory() to await claimVisitorHistory() in hostToken().
  • Guarantees that POST /auth/link finishes before token resolution completes, ensuring the first GET /conversations or history request after login observes the merged history.

3. Clear Pass Lifecycle

  • Clears the visitor pass from localStorage only if (data?.conversations_moved ?? 0) > 0 (or hostToken !== null in Bearer mode). If conversations_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 test visitor 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:widgetwidget 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.

@Asaf-prog

Copy link
Copy Markdown
Collaborator

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, current() now re-resolves identity whenever a stored visitor pass exists:

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 /auth/link attempt first:

anonymous request
→ POST /auth/link
→ 401
→ actual API request

next anonymous request
→ POST /auth/link
→ 401
→ actual API request

So anonymous usage can pay an extra network round-trip on every request until login happens.

There is already a lastClaimAttemptPass field with the comment:

/** 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:

  • repeated requests while still anonymous do not each call /auth/link;
  • the widget still retries later so a newly available login cookie can be detected;
  • once login is detected, the merge completes before history that depends on it is loaded.

It would also be good to add a regression test with multiple consecutive anonymous requests and assert that /auth/link is not called once per request.

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
@rishu685

Copy link
Copy Markdown
Contributor Author

Thanks @Asaf-prog! Great catch on throttling anonymous retries.

I've implemented lastClaimAttemptPass and lastClaimAttemptTime with a 15-second cooling-off window (CLAIM_RETRY_INTERVAL_MS = 15000) in commit 8d05239f:

How it works:

  1. Throttled Anonymous Requests: When a visitor is signed out, the first request attempts POST /auth/link. Upon receiving 401, TokenSource records lastClaimAttemptPass = pass and lastClaimAttemptTime = Date.now(). Subsequent anonymous requests within 15 seconds skip /auth/link and return this.cached immediately with 0 extra network calls.
  2. Zero-Code SPA Cookie Login: When the user signs in on the host app, the next turn after the interval (or upon page reload / refreshIdentity()) attempts POST /auth/link. On 200 OK with conversations_moved > 0, the visitor pass is cleared, this.cached transitions to null (session cookie speaks), and history is merged deterministically.
  3. Regression Unit Test: Added a test in widget.test.mjs verifying that 5 consecutive anonymous requests trigger POST /auth/link only once, not 5 times.

All 937 pytest tests, 40 Playwright E2E tests, and widget unit self-checks pass cleanly. Ready for final review!

@Asaf-prog

Copy link
Copy Markdown
Collaborator

Thanks — the throttling improvement solves the repeated /auth/link calls while the user is still anonymous, and the regression coverage is useful.

I still see one lifecycle issue before approving.

With the 15-second cooldown, this flow is possible:

anonymous request
→ POST /auth/link
→ 401
→ cooldown starts

user signs in 2 seconds later

user immediately opens conversation history
→ current() is still inside the cooldown window
→ /auth/link is skipped
→ GET /conversations runs as the signed-in user
→ anonymous conversations have not been merged yet

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 refreshIdentity() / reset() when the cookie appears.

I think we need to preserve both properties:

  • anonymous requests should not trigger a blocking /auth/link on every call;
  • if the user signs in, the next request that depends on authenticated history should not be forced to wait for an arbitrary retry interval before the hand-off can happen.

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:

anonymous claim attempt → 401
user signs in during the retry window
no reset / refreshIdentity
next history request
→ history is already merged

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
@rishu685

rishu685 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

I completely agree magic time-based cooldowns were introducing race condition windows where login history couldn't be deterministically observed.

I've eliminated CLAIM_RETRY_INTERVAL_MS entirely and implemented an event-driven snapshot model in commit 852be0ce:

1. Event-Driven & Cookie Snapshot Tracking (TokenSource.ts)

  • document.cookie Snapshot: TokenSource tracks lastCookieSnapshot. Any cookie change triggers identity re-evaluation.
  • Window Event Listeners: Attached listeners for window.focus, document.visibilitychange, and window.storage. Returning to the tab after logging in marks identity dirty.
  • Fast Anonymous Turns: Consecutive anonymous requests in the same tab return this.cached immediately without blocking on /auth/link every turn.
  • Deterministic History Hand-Off: AgentChatClient.listConversations() passes { forceCheck: true } to current(), guaranteeing POST /auth/link is await-ed before GET /conversations executes.

2. Zero-Code Regression Unit Test (widget.test.mjs)

Added a unit test verifying the exact zero-code transition without calling tokens.reset() or refreshIdentity():

// Anonymous turn -> 401
await client.createConversation();

// 5 consecutive turns -> 0 extra /auth/link calls
await client.createConversation();

// User logs in via cookie (zero-code: host calls no reset/refreshIdentity)
loggedInViaCookie = true;

// Next history request -> POST /auth/link succeeds (200 OK), pass is cleared, no bearer sent
await client.listConversations();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Anonymous→account merge never runs in host_token (cookie) mode

2 participants