Conversation
159c47d to
a8595df
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new useSyncCurrentUserId mutates module-level state during render (not in an effect), which can cause inconsistent state under React 18 concurrent rendering/StrictMode.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses offline-mode privacy on shared computers by ensuring requests include a stable per-user identifier so downstream offline caching can be scoped to the currently logged-in user (and not accidentally reused across sessions).
Changes:
- Added in-memory tracking of the current access token + user id, and conditionally sends an
X-Editor-User-Idrequest header when the token matches. - Introduced
useSyncCurrentUserIdand wired it intoWebComponentLoaderto keep the in-memory user-id tracking in sync with auth state. - Added unit tests covering header injection behavior and the user-id sync logic.
File summaries
| File | Description |
|---|---|
| src/utils/apiCallHandler.js | Adds setCurrentUser/stable-user-id logic and conditionally includes X-Editor-User-Id in request headers. |
| src/utils/apiCallHandler.test.js | Adds tests ensuring the new header is added/omitted in the intended cases. |
| src/hooks/useSyncCurrentUserId.js | Adds a helper “hook” to derive user id from auth state and sync it to apiCallHandler. |
| src/hooks/useSyncCurrentUserId.test.js | Adds tests verifying user-id selection (profile.user vs profile.sub) and logout clearing behavior. |
| src/containers/WebComponentLoader.jsx | Calls the new sync hook so header behavior is driven by the current Redux auth user. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I am worried about the complexity and safety of this - it feels like it would be easy to perform API requests without this header that would be cached. Is is possible to clear all cache data when someone logs out? If so, what are the pros and cons of this approach over that? EDIT: I saw you are clearing all unlabeled request data in your Standalone PR. I still wonder if it's possible to avoid having to label requests and clear all data on logout |
|
Going to look at avoiding the need for this PR and not looking to cache per user - this does add a lot of complexity so have changed https://github.com/RaspberryPiFoundation/editor-standalone/pull/1084 to focus on improving the clearing of data on logout. |
Summary
related to Issue: 1579
Was part of https://github.com/RaspberryPiFoundation/editor-standalone/pull/1084
What this fixes
On a shared or classroom computer, if one person opens a project and then
someone else uses the same browser afterwards, the offline cache could show
the second person the first person's project — even though they never had
access to it. This branch fixes that for the editor.
How it works
tab's own memory rather than anything shared across tabs.
asking. This lets the offline cache (handled in a companion fix in
editor-standalone) store each person's projects separately, instead ofin one shared pile.
0would havebeen silently treated as "nobody," meaning that account would never get
this protection.
Why this repo needed its own fix
The main project-loading and saving screen (
<editor-wc>) is driven by thisrepo, not by
editor-standalone. The original fix only coverededitor-standalone's own code, so it missed the actual screen most peopleuse day to day. This branch applies the same fix here.
Testing
nobody is logged in, it's left off if the login state hasn't caught up yet
(rather than risk using the wrong person's marker), and the account-id-
0case.
editor-standalonebranch, which is where the offline cache itself is scoped per person.