fix(appMode): unify precedence between boot and resolver; session tuple wins unconditionally - #31369
Conversation
…le wins unconditionally
The AppMode feature had two contradictory precedence chains — the
async runtime resolver used one, the boot-time pre-session write used
another — and the runtime resolver silently clobbered a valid session
tuple whenever the persona changed. This PR fixes both.
## Unified precedence
`resolveEffectiveAppMode(userPref, personaMode, appDefault)` in
`useAppMode.ts` is now the single source of truth for the "no session
tuple, no fresh hint" case:
userPref -> personaMode -> appDefault -> DEFAULT_APP_MODE
Both entry points use it:
* `AuthProvider.hydrateAndResolveAppMode` (boot-time pre-session
write — passes `null` for persona, since persona isn't known
synchronously).
* `useResolvedAppMode`'s async `candidate` (passes the resolved
persona once its docStore doc has settled).
Before this PR the resolver placed persona BEFORE userPref in its
`candidate` chain (`useResolvedAppMode.ts:236-240`, deleted here),
which meant an admin persona edit silently overrode a user's server-
side "remember" choice for the resolver run — while the boot-time
write kept respecting userPref. The two writes disagreed within one
session.
## Session tuple wins unconditionally
`useResolvedAppMode.ts:204` previously kept the session only when
`session.personaAppMode === currentPersonaAppMode`. If persona had
changed since the tuple was written, the resolver fell through and
overwrote the user's manual in-tab switch. Rung 1 of the target
precedence is "manual in-tab switch wins" — an admin persona edit
must not silently revert a user's right-now click. Drop the persona-
match check: `if (validSession) return;`.
## New precedence (final)
Top wins:
1. Session tuple (manual in-tab switch) — sessionStorage, per-tab.
2. Fresh cross-tab hint (`omAppModeHint`, TTL-gated) — localStorage.
3. User pref (`user_preferences.appMode`, server-persisted "remember").
4. Persona (docStore `personaPreferences[].appMode`).
5. Tenant default (`appConfiguration.defaultAppMode`).
6. `DEFAULT_APP_MODE` constant.
Boot-time write (`hydrateAndResolveAppMode`) bails on rungs 1 & 2 (my
earlier #31316 + #31335 guards, unchanged here). The remaining rungs
(3 → 6) are what `resolveEffectiveAppMode` encodes.
## Tests
* Rewrote `useResolvedAppMode.test.ts:248` ("persona overrides
session when personaAppMode differs from tuple's snapshot") into
a regression guard for the opposite: session survives persona
change (rung 1).
* Added three new tests covering rung transitions:
- user pref beats persona
- persona beats tenant default
- tenant default beats DEFAULT
* `beforeEach` now resets `setAppDefaultMode(null)` so tenant-
default tests don't leak into siblings.
All 69 existing + new tests pass.
## Not in scope
`resolveInitialAppMode` (`useAppMode.ts:346`) still considers only
session/hint/userPref — no persona, no tenant default. It's used by
the post-login redirect ONLY to pick a landing route, and the resolver
corrects a moment later. Fixing that helper is a separate cleanup.
❌ 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}
| // Shared with `resolveEffectiveAppMode` in `useAppMode.ts` so the | ||
| // async resolver here and the boot-time write in `AuthProvider` | ||
| // encode exactly the same policy. | ||
| const candidate = resolveEffectiveAppMode( |
There was a problem hiding this comment.
💡 Edge Case: Install gate only tests the top candidate, may strand user
With the reorder, candidate is now userPref first. The install gate (if (candidate !== DEFAULT_APP_MODE && !isModeRegistered(candidate)) return;) checks only that single top-priority candidate and never falls through to the next registered rung. So if a user's server-remembered appMode points at a plugin that has since been uninstalled (not in registeredRoutes), the resolver waits indefinitely and never applies the persona/tenant-default fallback, leaving the tab on whatever the boot-time write picked. Consider resolving down the chain to the first registered candidate rather than gating on only the top one. This is a pre-existing gate limitation that the reorder now routes through userPref, so severity is minor/speculative.
Was this helpful? React with 👍 / 👎
|
| Count | Rule |
|---|---|
| 9 | react-hooks/exhaustive-deps |
| 3 | sonarjs/no-nested-functions |
| 3 | sonarjs/cyclomatic-complexity |
| 1 | sonarjs/expression-complexity |
| 1 | sonarjs/cognitive-complexity |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:280:9 |
react-hooks/exhaustive-deps |
The 'onLoginHandler' function makes the dependencies of useMemo Hook (at line 918) change on every render. Move it inside the useMemo callback. Alternatively, w |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:357:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'navigate', 'setApplicationLoading', 'setCurrentUser', and 'setIsAuthenticated'. Either include them or remove |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:415:9 |
react-hooks/exhaustive-deps |
The 'resetUserDetails' function makes the dependencies of useMemo Hook (at line 918) change on every render. To fix this, wrap the definition of 'resetUserDetai |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:497:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'startTokenExpiryTimer'. Either include it or remove the dependency array. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:540:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'startTokenExpiryTimer'. Either include it or remove the dependency array. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:550:9 |
react-hooks/exhaustive-deps |
The 'handleFailedLogin' function makes the dependencies of useMemo Hook (at line 918) change on every render. Move it inside the useMemo callback. Alternatively |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:608:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'authConfig?.provider', 'handledVerifiedUser', 'navigate', 'resetUserDetails', and 'startTokenExpiryTimer'. Eit |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:653:9 |
react-hooks/exhaustive-deps |
The 'initializeAxiosInterceptors' function makes the dependencies of useMemo Hook (at line 918) change on every render. To fix this, wrap the definition of 'ini |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:721:67 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:740:37 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:749:27 |
sonarjs/no-nested-functions |
Refactor this code to not nest functions more than 4 levels deep. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:818:30 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 17 which is greater than 10 authorized.","cost":7,"secondaryLocations":[{"line":818,"column":29,"endLine":818,"endColum |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:907:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'cleanup', 'fetchAuthConfig', 'initializeAxiosInterceptors', and 'startTokenExpiryTimer'. Either include them or |
| 🟡 | src/hooks/useAppMode.ts:75:47 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 12 which is greater than 10 authorized.","cost":2,"secondaryLocations":[{"line":75,"column":46,"endLine":75,"endColumn" |
| 🟡 | src/hooks/useAppMode.ts:165:7 |
sonarjs/expression-complexity |
Reduce the number of conditional operators (5) used in the expression (maximum allowed 3). |
| 🟡 | src/hooks/useResolvedAppMode.ts:180:16 |
sonarjs/cognitive-complexity |
Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed. |
| 🟡 | src/hooks/useResolvedAppMode.ts:180:16 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 22 which is greater than 10 authorized.","cost":12,"secondaryLocations":[{"line":180,"column":15,"endLine":180,"endColu |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
Jest test CoverageUI tests summary
|
✅ Playwright Results — workflow succeededValidated commit ✅ 795 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 56m 52s ⏱️ Max setup 3m 5s · max shard execution 18m 44s · max shard-job elapsed before upload 22m 21s · reporting 5s 🌐 221.42 requests/attempt · 2.67 app boots/UI scenario · 5.16% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
…side pref Under the unified precedence (userPref -> persona -> appDefault -> DEFAULT), `hydrateAndResolveAppMode` was writing to the session tuple at boot with `personaMode = null` (persona isn't known synchronously). If the user had no userPref but DID have a defaultPersona, the boot write computed the wrong candidate — falling through to `appDefault` or `DEFAULT_APP_MODE` instead of persona's opinion. Once the tuple was written, `useResolvedAppMode`'s `if (validSession) return;` guard kept the wrong mode; the persona- doc fetch that ran a moment later never got to influence the write. Symptom: `AppModePrecedence.spec.ts:242` (rung 4 persona beats rung 5 tenant default) — fresh user, persona=AI, tenant default=Classic, no user pref. Boot wrote 'default' (falling through to appDefault since persona wasn't known and userPref was null); resolver's session guard locked it. Expected 'ai', got 'default'. Fix: third guard in `hydrateAndResolveAppMode` — skip the boot write when `userPref === null && user.defaultPersona`. Defer to `useResolvedAppMode`, which fetches the persona doc and can compute the correct candidate against the full chain. When userPref IS set it wins over persona under the unified precedence, so writing at boot remains safe in that branch. All 69 existing Jest tests still pass.
|
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsUnifies the AppMode precedence between boot and runtime resolver to ensure manual session selections win unconditionally over persona changes. Consider addressing the install gate check which currently only tests the top candidate. 💡 Edge Case: Install gate only tests the top candidate, may strand user📄 openmetadata-ui/src/main/resources/ui/src/hooks/useResolvedAppMode.ts:258-269 With the reorder, 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Summary
The AppMode feature had two contradictory precedence chains for the same fallback logic — the async runtime resolver used one, the boot-time pre-session write used another — and the runtime resolver silently clobbered a valid session tuple whenever the persona changed. This PR fixes both.
Unified precedence
`resolveEffectiveAppMode(userPref, personaMode, appDefault)` in `useAppMode.ts` is the single source of truth for the "no session tuple, no fresh hint" case:
Both entry points use it:
Before this PR the resolver placed persona BEFORE userPref in its `candidate` chain (`useResolvedAppMode.ts:236-240`), which meant an admin persona edit silently overrode a user's server-side "remember" — while the boot-time write kept respecting userPref. The two writes disagreed within one session.
Session tuple wins unconditionally
`useResolvedAppMode.ts:204` previously kept the session only when `session.personaAppMode === currentPersonaAppMode`. If persona had changed since the tuple was written, the resolver fell through and overwrote the user's manual in-tab switch. Rung 1 of the target precedence is "manual in-tab switch wins" — an admin persona edit must not silently revert a user's right-now click. Drop the persona-match check: `if (validSession) return;`.
New precedence (final, top wins)
Boot-time write bails on rungs 1 & 2 (via my earlier #31316 + #31335 guards, unchanged here). The remaining rungs (3 → 6) are what `resolveEffectiveAppMode` encodes.
Tests
All 69 tests pass (`yarn jest --testPathPattern='hooks/useAppMode|hooks/useResolvedAppMode'`).
Not in scope
`resolveInitialAppMode` (`useAppMode.ts:346`) still considers only session/hint/userPref — no persona, no tenant default. It's used by the post-login redirect only to pick a landing route, and the resolver corrects a moment later. Fixing that helper is a separate cleanup.
Test plan
🤖 Generated with Claude Code