feat(ui): App Mode — port admin logic + tenant page to OSS - #31939
Closed
chirag-madlani wants to merge 7 commits into
Closed
feat(ui): App Mode — port admin logic + tenant page to OSS#31939chirag-madlani wants to merge 7 commits into
chirag-madlani wants to merge 7 commits into
Conversation
registerRoutes now accepts an optional { labelKey, icon } describing how
the mode should render in pickers. Fallback map covers DEFAULT/AI when a
caller omits it (kept for pre-metadata Collate builds). Explicit metadata
always wins; unregisterRoutes clears both routes and metadata.
default-app-mode, no-default, and two description keys — with real translations in every non-English locale. label.default and label.app-mode already existed in every locale from a prior port and were left untouched.
Radio picker sourced from useAppRoutesRegistry metadata + a 'No default' option. Reads/writes appConfiguration.defaultAppMode via existing /system/settings/appConfiguration endpoints. Save disabled until the value changes.
server.entity-updated-successfully does not exist in en-us.json; the real key is server.entity-updated-success. Also drop the test's local react-i18next mock (which had papered over the typo) in favor of the repo's global key-echoing mock, and assert the success toast fires with the exact correct key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New route at /settings/preferences/appMode with an admin-gated entry in the Preferences category. Icon reuses assets/svg/app-mode.svg. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AppRouter already dispatches the authenticated router through the
registry (routes[appMode] ?? AuthenticatedRoutes), but DEFAULT_APP_MODE
itself was never registered, so its metadata never appeared in
useAppRoutesRegistry.metadata. That left the tenant admin's default
app-mode picker (DefaultAppModePage) unable to list "Classic/Default"
as an option — only "No default" plus whatever plugin modes (e.g.
Collate's AI mode) happened to be registered.
Export AuthenticatedRoutes from AppRouter.tsx and register it under
DEFAULT_APP_MODE with { labelKey: 'label.default' } on mount,
unregistering on unmount — symmetric with how a downstream plugin
registers its own mode. Purely additive: DEFAULT_APP_MODE was already
special-cased as "always valid" by isModeRoutesPending and
useResolvedAppMode's isModeRegistered, and ModeRoutes resolves to the
same AuthenticatedRoutes component whether or not this effect has run.
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Comment on lines
+45
to
+59
| const metadata = useAppRoutesRegistry((state) => state.metadata); | ||
| const [initialValue, setInitialValue] = useState<string>(NO_DEFAULT_VALUE); | ||
| const [currentValue, setCurrentValue] = useState<string>(NO_DEFAULT_VALUE); | ||
| const [isLoading, setIsLoading] = useState(true); | ||
| const [isSaving, setIsSaving] = useState(false); | ||
|
|
||
| const options = useMemo<AppModeOption[]>(() => { | ||
| const modeOptions = Object.entries(metadata).map(([value, m]) => ({ | ||
| value, | ||
| labelKey: m.labelKey, | ||
| })); | ||
|
|
||
| return [ | ||
| { value: NO_DEFAULT_VALUE, labelKey: 'label.no-default' }, | ||
| ...modeOptions, |
There was a problem hiding this comment.
💡 Edge Case: Saved mode outside registry leaves radio group with no selection
currentValue/initialValue are seeded from getAppConfiguration().defaultAppMode, but the radio options come only from useAppRoutesRegistry.metadata. If the stored default is a mode whose plugin isn't loaded in the current build (e.g. 'ai' or 'classic' in an OSS-only deploy), no RadioButton matches the value, so the group renders with nothing selected and the admin cannot see the currently-configured default. Consider surfacing the unmatched value as a disabled/labelled option, or falling back to a synthetic option for the loaded value.
Fix:
const options = useMemo<AppModeOption[]>(() => {
const modeOptions = Object.entries(metadata).map(([value, m]) => ({ value, labelKey: m.labelKey }));
const base = [{ value: NO_DEFAULT_VALUE, labelKey: 'label.no-default' }, ...modeOptions];
// Surface a stored value that no loaded plugin registers so it stays visible/selected.
if (initialValue !== NO_DEFAULT_VALUE && !base.some((o) => o.value === initialValue)) {
base.push({ value: initialValue, labelKey: 'label.app-mode' });
}
return base;
}, [metadata, initialValue]);
Was this helpful? React with 👍 / 👎
| currentValue === NO_DEFAULT_VALUE | ||
| ? null | ||
| : (currentValue as unknown as DefaultAppMode); | ||
| await patchAppConfiguration({ defaultAppMode }); |
There was a problem hiding this comment.
💡 Quality: patchAppConfiguration replaces whole config despite Partial signature
patchAppConfiguration accepts Partial<AppConfiguration> and is named like a merge, but it PUTs config_value: patch which fully replaces the stored appConfiguration. Today AppConfiguration has only defaultAppMode, so there is no live data loss, but the moment another field is added to this config, saving the default app mode from this page will silently wipe it. Consider merging over the fetched config before PUT (or renaming to make the replace-semantics explicit).
Fix:
export const patchAppConfiguration = async (
patch: Partial<AppConfiguration>
): Promise<AppConfiguration> => {
const current = await getAppConfiguration();
const response = await axiosClient.put<Settings>(`/system/settings`, {
config_type: SettingType.AppConfiguration,
config_value: { ...current, ...patch },
});
return (response.data.config_value as AppConfiguration) ?? {};
};
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 2 findingsPorts the App Mode admin logic and tenant-default settings UI to OSS with comprehensive test coverage. Consider handling saved modes outside the registry and verifying the patchAppConfiguration payload replacement behavior. 💡 Edge Case: Saved mode outside registry leaves radio group with no selection📄 openmetadata-ui/src/main/resources/ui/src/pages/Settings/DefaultAppModePage/DefaultAppModePage.tsx:45-59 📄 openmetadata-ui/src/main/resources/ui/src/pages/Settings/DefaultAppModePage/DefaultAppModePage.tsx:120-133
Fix💡 Quality: patchAppConfiguration replaces whole config despite Partial signature
Fix🤖 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 |
Contributor
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
❌ I18n SyncTranslation locale files are out of sync with Affected files
🔍 ESLint findings in this PR's files — 0 error(s), 25 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 25 warning(s) across 3 changed file(s).
All findings
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports the App Mode admin logic and tenant-default settings UI from Collate into OSS so downstream plugins can register modes symmetrically:
useAppRoutesRegistrywith an optional{ labelKey, icon }metadata field per registration, plus a runtime fallback map so pre-metadata Collate builds keep compiling.DefaultAppModePageat/settings/preferences/appMode(admin-gated in the Preferences category). Options come from the registry metadata + a "No default" sentinel; save writesappConfiguration.defaultAppModevia the existing/system/settings/appConfigurationendpoint.DEFAULT_APP_MODEinAppRouter.tsxwith metadata (the registry-driven dispatch already lived there — this just makes the default show up in the picker).playwright/e2e/Features/AppMode/DefaultAppModePage.spec.ts.Non-breaking guarantee
useAppMode/useResolvedAppMode/resolveEffectiveAppMode, orSystemResource.java.useAppRoutesRegistry.registerRoutesmetadata arg is optional at the type level; runtime fallback map keys offDEFAULT_APP_MODE/AI_APP_MODE. Collate builds on the pre-merge OSS pointer continue to compile.openmetadata-spec/,SystemResource.java,settingConfigAPI.ts,useAppMode.ts,useResolvedAppMode.ts: empty.Not in this PR (deferred by design)
pages/SettingsAppModePage/SettingsAppModePage.tsx, reached viaPersonaDetailsPage's "Customize UI" tab →app-modecategory). Today it hardcodes[AppMode.Classic, AppMode.AI]rather than reading from the new registry metadata — refactor is a follow-up ticket.DefaultAppModePage.tsx:97-98casts the registry's generic string key to the closedDefaultAppModeenum. In OSS-only deployments (onlyDEFAULT_APP_MODE = 'default'registered), the picker can only PUTnull— no path to'ai'or'classic'without a plugin registering that mode. Pre-existing design tension; recommend a follow-up ticket to unify.AppModeSwitcherpopover UI: intentionally stays in Collate for now; arrives in OSS later together with any OSS-native AI routes.Test plan
yarn test --testPathPattern=useAppRoutesRegistry— 9/9 passyarn test --testPathPattern=DefaultAppModePage— 4/4 passyarn test --testPathPattern=AppRouter— 9/9 pass (7 pre-existing + 2 new)npx playwright test Features/AppMode/DefaultAppModePage --list— 1 spec discoverednpx tsc --project playwright/tsconfig.json --noEmit— 0 new errorsnpx eslint— 0 errors on new filesCompanion Collate cleanup will land after this merges — deletes the duplicate
AppModeSettingsPage,AdminOpsServletapp-configurationhandlers + tests, and updates Collate's threeregisterRoutes(...)call sites to pass explicit metadata.