Skip to content

Current User: Fix reload not fetching fresh data when entity events fire#22719

Merged
nielslyngsoe merged 2 commits into
mainfrom
v17/bugfix/clear-promise-when-reloading-current-user
May 5, 2026
Merged

Current User: Fix reload not fetching fresh data when entity events fire#22719
nielslyngsoe merged 2 commits into
mainfrom
v17/bugfix/clear-promise-when-reloading-current-user

Conversation

@madsrasmussen

@madsrasmussen madsrasmussen commented May 5, 2026

Copy link
Copy Markdown
Member

Summary

A local "cache" has been introduced in the current user context that always returns the last resolved promise with data. This prevents the context from reloading current user data when a user or user group is updated. The PR ensures that the cached promise is cleared before reloading the current user.

The PR is kept internal because the reload feature has not been released yet.

Test plan

  • Ensure that the current user is reloaded whenever the user or any user group they belong to is updated.

Copilot AI review requested due to automatic review settings May 5, 2026 17:21
@claude

claude Bot commented May 5, 2026

Copy link
Copy Markdown

Claude finished @madsrasmussen's task in 3m 18s —— View job


PR Review

Target: origin/main · Based on commit: b8cc72c8 · Classified as: Standard

Fixes a stale-promise bug in UmbCurrentUserContext where event-triggered reloads returned the already-resolved initial promise instead of fetching fresh data, and adds a data-driven integration test to prove the fix works end to end.

  • Other changes: Event-triggered current-user reloads now always fetch fresh data from the server (behaviour change for consumers that trigger entity-updated/deleted events on user or user-group entities).

Suggestions

  • current-user.context.test.ts:13: UmbUserGroupDetailStore is imported via a relative path (../user-group/repository/detail/user-group-detail.store.js) while the store is only exported as a type from the barrel and not as a value — so the relative import is the only viable option for instantiating it here. This is fine as-is, but it would be worth re-exporting the concrete class (not just the type) from @umbraco-cms/backoffice/user-group so future test authors can use the canonical import path.

Approved

The fix is minimal and correct — clearing #loadPromise before delegating to load() in the debounced handler is exactly the right place to break the cache. The new test validates the actual data refresh (not just call-count), which gives real confidence that the bug won't regress. aTimeout cleanup is a nice improvement throughout. Good work!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a regression/bug in the Backoffice current-user context where event-triggered reloads could return an already-resolved load() promise instead of performing a fresh fetch, and adds a regression test that verifies the current user state actually updates after related entity changes.

Changes:

  • Clear #loadPromise before running the debounced reload, ensuring entity events always trigger a fresh load().
  • Add a regression test that mutates a user group and verifies allowedSections changes after an entity-updated event.
  • Simplify test timing helpers by using aTimeout from @open-wc/testing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Umbraco.Web.UI.Client/src/packages/user/current-user/current-user.context.ts Clears cached load() promise on debounced reload so event-driven reloads fetch fresh data.
src/Umbraco.Web.UI.Client/src/packages/user/current-user/current-user.context.test.ts Adds regression coverage for “fresh data” reload behavior and refactors delays to aTimeout.

@nielslyngsoe nielslyngsoe enabled auto-merge (squash) May 5, 2026 18:56
@nielslyngsoe

Copy link
Copy Markdown
Member

@claude what do you think about introducing an abort controller in this case? Cause what if a current load is still executing when the loadDebounce gets triggered?

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix — the root cause is clear and the solution is minimal and well-targeted.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See full review below - posted in parts due to tooling constraints.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix — the root cause is clear and the solution is minimal and well-targeted.

Summary

The load() method intentionally caches its in-flight promise in #loadPromise so concurrent callers coalesce onto a single fetch. The bug was that once the promise resolved it was never cleared, so any subsequent call to load() (including the event-triggered debounced reload) returned the settled promise immediately without re-fetching.

The fix — clearing #loadPromise inside #loadDebounced before calling load() — is the right approach: it invalidates the cache only on the event-driven reload path, leaving the coalescing behaviour intact for concurrent initial loads.

What looks good

  • Fix is surgically scoped. Only the debounced event-reload path clears the promise; direct load() calls still share the in-flight promise as intended.
  • No double-fetch risk. #loadPromise = undefined is set synchronously before load(), so load() always creates a fresh #doLoad() assignment. The observer key 'observeUser' ensures the old subscription is replaced rather than duplicated.
  • Test cleanup is a net improvement. Replacing the inline new Promise((resolve) => setTimeout(resolve, 200)) with aTimeout(200) from @open-wc/testing is idiomatic and removes the duplicated local wait helper.
  • New regression test is solid. The reload fetches fresh data suite exercises the actual data flow end-to-end (repo save → entity event → reload → observable value check), which directly validates the bug scenario rather than just the reload counter.
  • UmbUserGroupDetailStore added to the test host — correctly supplies the dependency the new test needs.

Minor note

The 200 ms aTimeout appears in every test case (both old and new). That's fine for correctness, but worth noting that if this suite ever grows large, the cumulative wait time could slow CI. Not a blocker — just something to keep in mind if the suite ever needs a speed pass.

Overall this is a clean, low-risk bugfix with good test coverage.

@nielslyngsoe nielslyngsoe merged commit 2c88f2a into main May 5, 2026
34 checks passed
@nielslyngsoe nielslyngsoe deleted the v17/bugfix/clear-promise-when-reloading-current-user branch May 5, 2026 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants