feat(user): persist per-user UI preferences on the User entity (starting with appMode) - #31030
feat(user): persist per-user UI preferences on the User entity (starting with appMode)#31030chirag-madlani wants to merge 4 commits into
Conversation
Adds a `preferences` property to the User JSON schema as an opaque,
server-side Map<String, Object> bag (existingJavaType pins it to
java.util.Map<java.lang.String, java.lang.Object> instead of letting
jsonschema2pojo generate a wrapper POJO). Default is {} so downstream
code can iterate without null checks.
Adds UserResourceIT#patch_preferences_add_appMode_returnsIt, which
PATCHes preferences.appMode via JSON-Patch and reads it back with
?fields=preferences.
No repository/resource-level validation or field-gating is added here
(opaque by design) — that is scoped to a follow-up task.
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const pendingPatch = new Map<string, unknown>(); | ||
| const previousValues = new Map<string, unknown>(); | ||
| let flushTimer: ReturnType<typeof setTimeout> | null = null; | ||
| let serverKnown: Partial<UserPreferences> = {}; |
There was a problem hiding this comment.
⚠️ Bug: resetBackendSyncState never called on logout — module state leaks across sessions
The module-level sync bookkeeping (pendingPatch, previousValues, flushTimer, serverKnown) lives at module scope and is only reset via resetBackendSyncState, which the comment claims is "safe to call on logout" but is in fact never invoked anywhere outside the store file and its test (verified via grep). Because syncBackendKeys short-circuits when flushTimer !== null, a pending debounce from user A that survives a logout/login on the same tab will (a) prevent user B's writes and the one-shot migration in hydrateBackendSyncedPreferences from scheduling a new flush, and (b) fire flushPendingPatch(userA.name, userA.id) while pendingPatch now holds user B's value — writing B's preference to A's id. Wire resetBackendSyncState() into the logout path (e.g. onLogoutHandler/handleSuccessfulLogout) so state does not carry across users.
Was this helpful? React with 👍 / 👎
| return { op: 'remove', path: `/preferences/${key}` } as Operation; | ||
| } | ||
|
|
||
| const op = key in serverKnown ? 'replace' : 'add'; |
There was a problem hiding this comment.
💡 Bug: serverKnown not updated after field-gated PATCH response omits preferences
After a successful PATCH, flushPendingPatch seeds serverKnown from updated?.preferences ?? {}. But per the new field-gating in UserRepository.clearFields, the PATCH response omits preferences unless fields=preferences is requested, so updated.preferences is null and serverKnown never records the added key. Consequently the op for a later write is computed as add instead of replace (line 150). JSON-Patch add on an existing object member behaves like replace per RFC 6902, so it works, but the intent-tracking is effectively dead. Either request fields=preferences on the PATCH round-trip or drop the add/replace distinction. Note the UI test mocks a response that does carry preferences, which may not reflect the real endpoint.
Was this helpful? React with 👍 / 👎
| flushTimer = null; | ||
| if (pendingPatch.size === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const attempted = new Map(pendingPatch); | ||
| pendingPatch.clear(); | ||
| const attemptedPrevious = new Map(previousValues); | ||
| previousValues.clear(); | ||
|
|
||
| const ops: Operation[] = Array.from(attempted.entries()).map( | ||
| ([key, value]) => { | ||
| if (value === null) { | ||
| return { op: 'remove', path: `/preferences/${key}` } as Operation; | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: In-flight PATCH failure can roll back a newer optimistic write
flushPendingPatch clears pendingPatch/previousValues and sets flushTimer = null before awaiting updateUserDetail. A user write during the in-flight request starts a fresh debounce cycle with a new optimistic local value. If the earlier request then rejects, the catch handler calls setUserPreference(userName, rollback) with the stale attemptedPrevious, clobbering the newer value the user just set. Consider skipping rollback for keys whose local value has changed since the attempt, or serialising flushes so a new write isn't overwritten by a failed older one.
Was this helpful? React with 👍 / 👎
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
🔍 ESLint findings in this PR's files — 0 error(s), 22 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 22 warning(s) across 3 changed file(s).
All findings
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source
Jest test CoverageUI tests summary
|
|
Summary
Persist the user's app-mode preference (AI vs Classic) — and any future opaque per-user UI preference — on the backend
Userentity, so the choice follows the user across devices, browsers, and profiles instead of only living in localStorage.What changed
Schema —
openmetadata-spec/.../teams/user.jsonpreferences: Map<String, Object>field, opaque to the server, stored inside the existinguser_entity.jsonJSON blob. No DB migration.existingJavaTypedirective ensures the Java shape isMap<String, Object>rather than a generated wrapper POJO.Backend —
openmetadata-serviceUserRepository.clearFieldsgatespreferencesbehindfields=preferences. The field is serialised only when explicitly requested — never in list responses, and never in other users' GETs.UserResource.getCurrentLoggedInUserforcespreferencesinto its fetched field set viaEntityUtil.addField, so the UI can hydrate on bootstrap without every caller needing to remember the query param.FIELDSexample updated.allowedFieldsis auto-derived from@JsonPropertyOrderon generatedUser.java, so no manual allow-list edit was needed.UI —
openmetadata-ui/.../hooks/currentUserStore/useCurrentUserStore.tsBACKEND_SYNCED_KEYS = new Set(['appMode'])— the whitelist of preference keys that also live on the backend. All otherUserPreferencesfields remain purely local (persisted by the existingpersistmiddleware, unchanged).hydrateBackendSyncedPreferences(user)is called fromAuthProviderright aftergetLoggedInUser()resolves (both the returning-session bootstrap and the interactive-login paths). It:addup to the backend.setPreference(patch)— the consumer-facing hook — is unchanged. Under the hood, writes to whitelisted keys additionally enqueue a 300ms-debouncedupdateUserDetail(userId, ops)JSON-Patch:null→remove, key not previously on server →add, otherwise →replace. Coalesces rapid writes into one PATCH.showErrorToastfires. No silent divergence between client and server.beforeunloadflushes any pending debounced patch best-effort.persistmiddleware is not removed — sidebar collapse state, recently-viewed, etc. still live on localStorage as before.Types
openmetadata-ui/.../generated/entity/teams/user.tsregenerated with the newpreferences?field.Testing
Java IT tests (
openmetadata-integration-tests/.../UserResourceIT.java) — 4 tests added:patch_preferences_add_appMode_returnsIt— JSON-Patchaddround-trip.get_user_without_fields_omitsPreferences— field-gating works both directions (withoutfields=preferences, the response omits it; with it, the response carries it).patch_preferences_replaceOp_persists—replaceop semantics.patch_preferences_removeOp_clearsKey—removeop semantics.list_users_omitsPreferencesForAllEntries— list-response omission across a page.These tests were drafted but not executed locally — they need Docker/MySQL/ES containers that weren't available in the authoring environment. CI on this PR is the first real run.
UI Jest tests (
useCurrentUserStore.test.ts) — 9 new tests, all passing locally:appModewhen server has a value (server wins).setPreference({ appMode })fires a debounced PATCH.appMode: nullemits aremoveop.appModemigrates to backend when the server has none.Full store test suite: 15/15 passing. Pre-existing
AuthProvider.test.tsx(12 tests) still passing after wiring the bootstrap call.Design notes / decisions
preferencesis opaque server-side. No validation of individual keys or values — the schema/enum lives in the UI. This keeps future preferences (likeconnectionsViewMode, or Collate-only keys) landable without schema changes.appModeis backend-synced in this PR. OtherUserPreferencesfields (sidebar collapse state, recently viewed, marketplace history, etc.) intentionally stay per-device. A future PR can categorize each and expandBACKEND_SYNCED_KEYS.fields=preferences,GET /users/{id}and list responses omit the field entirely. Only the loggedInUser endpoint opts in by default.existingJavaTypeon the schema preventsjsonschema2pojofrom generating a wrapperPreferencesPOJO with@JsonAnyGetter/@JsonAnySetter, which would have made the Java handling inconsistent with the "opaque bag" design.Follow-ups (out of scope for this PR)
UserPreferencesfields and expand the backend-synced whitelist (Collate'sAppModeSwitchershippedappModefirst, other fields will follow if they cross device).preferencesversioning if the shape ever needs breaking changes on the wire.Test plan for reviewers
/v1/users/{id}fires within ~300ms withop: add|replace|removeon/preferences/appMode.getLoggedInUserreturnspreferences.appMode; AppModeSwitcher reflects the choice.GET /v1/users/{id}(withoutfields=preferences) returnspreferences: null.GET /v1/userslist returns every user withpreferences: null.🤖 Generated with Claude Code