Skip to content

Add session persistence, session timer recovery, and store reset methods#7932

Open
cstns wants to merge 3 commits into
mainfrom
7915_persist-expert-chat-in-local-storage
Open

Add session persistence, session timer recovery, and store reset methods#7932
cstns wants to merge 3 commits into
mainfrom
7915_persist-expert-chat-in-local-storage

Conversation

@cstns

@cstns cstns commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Description

store expert chats in localStorage

Related Issue(s)

closes #7915

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production
  • Link to Changelog Entry PR, or note why one is not needed.

Labels

  • Includes a DB migration? -> add the area:migration label

@cstns
cstns requested a review from andypalmi July 24, 2026 12:19
@cstns cstns self-assigned this Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.62%. Comparing base (b327fda) to head (9068287).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7932   +/-   ##
=======================================
  Coverage   75.61%   75.62%           
=======================================
  Files         434      434           
  Lines       23232    23235    +3     
  Branches     6172     6174    +2     
=======================================
+ Hits        17568    17571    +3     
  Misses       5664     5664           
Flag Coverage Δ
backend 75.62% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@andypalmi

Copy link
Copy Markdown
Contributor

A couple of things I noticed:

  1. clearStores() (account-auth.js) is called inside checkState()'s catch block, the "not logged in" branch, not the successful-login branch. checkState() runs both at app boot and right after loginWithCredentials() succeeds, but on a successful login userApi.getUser() takes the try path, so this call never fires there. In the usual flow (logout or session expiry, then login page loads, then submit credentials) it still works out, since reaching the login page itself goes through the not-logged-in branch first. But it means the clear is tied to "detected as not logged in" rather than "just logged in," so a flow that goes straight from one authenticated identity to another without an intervening logged-out state (an SSO account switch, for example) would skip it. Was this intentional, or would it make more sense to move this into the success path, maybe loginWithCredentials itself?

  2. resumeSessionTimer() in product-expert.js duplicates the interval callback from startSessionTimer() almost line for line (same threshold checks, same addSystemMessage calls), just without resetting sessionStartTime. Might be worth pulling the shared interval logic into one helper both functions call, so there's only one copy to keep in sync.

@cstns

cstns commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

good catch, fixing now

@andypalmi

Copy link
Copy Markdown
Contributor

Thanks for the quick turnaround, the session timer refactor into _startSessionCheckInterval() looks good, that resolves the duplication.

One more thing on the login-clearing fix though: login(user) now unconditionally $reset()s the expert stores, but this.login(user) (account-auth.js:78) is called from checkState()'s try branch, which runs on every app boot, including an ordinary page refresh while already logged in, not just on an actual new login. App.vue calls checkState() on mount, so this fires every time. That means the expert stores get reset, and the reset gets persisted, wiping localStorage, on every refresh, which reintroduces the exact bug #7915 was meant to fix, just through this new code path.

The trouble is login() fires for two different situations that need different handling: a genuinely new authenticated identity should clear, but the same user's session just being reconfirmed on refresh should not. Since this.user isn't persisted, there's nothing to diff against in memory after a reload to tell these apart.

Also worth noting, moving the clear into loginWithCredentials() instead wouldn't fully cover this either, since both SSO paths (Google and direct/SAML) do a full window.location redirect and never call loginWithCredentials(). They land back on the same boot-time checkState() path.

Would persisting a userId on the account-auth store and only resetting when the incoming user.id differs from the persisted one work here? That would cover password login, MFA, and both SSO flows the same way, while leaving an ordinary refresh by the same user alone.

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.

Persist active expert chat session in local storage

2 participants