Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates the Admin Console to browser-based routing and restores native PatternFly JumpLinks behavior.
Changes:
- Adds server-side deep-link fallback and browser-router configuration.
- Routes tab clicks through React Router.
- Reworks ScrollForm anchors and updates affected Playwright URLs.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
services/.../AdminConsole.java |
Adds SPA deep-link fallback. |
js/libs/ui-shared/.../ScrollForm.tsx |
Uses native JumpLinks scrolling. |
js/libs/ui-shared/.../FormPanel.tsx |
Adds stable panel anchors. |
js/apps/admin-ui/test/.../saml-signature-defaults.spec.ts |
Updates browser-route URL. |
js/apps/admin-ui/test/.../main.ts |
Updates navigation helper URL. |
js/apps/admin-ui/test/.../default-trust.spec.ts |
Updates browser-route URL. |
js/apps/admin-ui/src/main.tsx |
Configures browser routing and legacy URL migration. |
js/apps/admin-ui/src/context/.../useHash.tsx |
Removes obsolete hash tracking. |
js/apps/admin-ui/src/context/.../RealmContext.tsx |
Derives realms from pathnames. |
js/apps/admin-ui/src/components/.../RoutableTabs.tsx |
Adds client-side tab navigation handlers. |
js/apps/admin-ui/src/App.tsx |
Removes hash cleanup logic. |
Comment on lines
+73
to
+75
| scrollableSelector={`#${mainPageContentId}`} | ||
| label={label} | ||
| offset={100} |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
js/libs/ui-shared/src/scroll-form/ScrollForm.tsx:73
- Scrolling still will not update the active jump link: the linked upstream issue patternfly/patternfly-react#12223 remains open, and its maintainer confirmed that click state works but scroll tracking does not. Keep the local scroll tracking (or consume a PatternFly fix) before closing #45494.
scrollableSelector={`#${mainPageContentId}`}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
js/libs/ui-shared/src/scroll-form/ScrollForm.tsx:85
- This sets
nodeto a string#.... In PatternFly JumpLinks,nodeis used to identify/associate the content section for scrollspy behavior, and passing a string here can be interpreted differently than intended (e.g., as a literal node/ReactNode rather than a target node/selector, depending on the component API/version). Ifhrefis sufficient for navigation, dropnodeentirely; otherwise, passnodein the format PatternFly expects (commonly an element/ref or an id without the leading#, depending on the type).
<JumpLinksItem
key={title}
href={`#${scrollId}`}
node={`#${scrollId}`}
data-testid={`jump-link-${scrollId}`}
js/apps/admin-ui/test/identity-providers/default-trust.spec.ts:33
- The cleanup swallows all errors, which can hide real failures (e.g., auth/network issues) and make the test suite harder to debug. Prefer ignoring only the expected 'not found' case (or specific error/status from
adminClient) and rethrowing unexpected errors.
try {
await adminClient.deleteIdentityProvider(alias);
} catch {
// The provider may already be deleted when creation fails.
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
services/src/main/java/org/keycloak/services/resources/admin/AdminConsole.java:328
@PathParam(\"path\")will not benullhere (it will typically be an empty string when the regex matches the root). Thepath == nullbranch is redundant; simplify the condition to only checkpath.isBlank().
@Path("{path:.*}")
public Response getMainPage(@PathParam("path") String path) throws IOException, FreeMarkerException {
services/src/main/java/org/keycloak/services/resources/admin/AdminConsole.java:334
@PathParam(\"path\")will not benullhere (it will typically be an empty string when the regex matches the root). Thepath == nullbranch is redundant; simplify the condition to only checkpath.isBlank().
// Redirect root URL to one with a trailing slash.
if ((path == null || path.isBlank()) && !adminUriInfo.getRequestUri().getPath().endsWith("/")) {
return Response.status(302).location(adminUriInfo.getRequestUriBuilder().path("/").build()).build();
js/apps/admin-ui/src/context/realm-context/RealmContext.tsx:29
decodeURIComponent(pathname)can throw on malformed percent-encoding and would crash the app during render. Since React Router’spathnameis already safe to consume as a path, consider avoidingdecodeURIComponententirely, or wrapping decoding in a try/catch and falling back to the rawpathname.
const { pathname } = useLocation();
js/apps/admin-ui/src/context/realm-context/RealmContext.tsx:36
decodeURIComponent(pathname)can throw on malformed percent-encoding and would crash the app during render. Since React Router’spathnameis already safe to consume as a path, consider avoidingdecodeURIComponententirely, or wrapping decoding in a try/catch and falling back to the rawpathname.
const locationRealm = decodeURIComponent(pathname);
const realm = locationRealm.split("/")[1] || environment.realm;
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:141
- The
to as Tocast is an unsafe escape hatch and suggests the hook’s parameter type doesn’t match the router API. Prefer changing the hook signature to acceptto: To(or otherwise ensure callers always provide a validTo) souseHref/useLinkClickHandlercan be used without casting.
export const useRoutableTab = (to: Partial<Path>) => {
const href = useHref(to);
const as To);
return {
eventKey: to.pathname ?? "",
href,
onClick,
};
};
js/apps/admin-ui/test/identity-providers/default-trust.spec.ts:33
- Catching and ignoring all errors here can hide real cleanup problems (e.g., auth failures, connectivity issues). Consider only suppressing the expected 'not found' case (e.g., based on status code/error type) and rethrowing (or at least logging) unexpected errors.
try {
await adminClient.deleteIdentityProvider(alias);
} catch {
// The provider may already be deleted when creation fails.
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
js/libs/ui-shared/src/scroll-form/ScrollForm.tsx:84
nodeis a CSS selector, butscrollIdis derived from translated or user-defined section titles and can contain selector metacharacters. For example, the ArmenianfineGrainOpenIdConnectConfigurationtitle contains parentheses, causing PatternFly's selector lookup to fail; escape the ID when constructingnode.
node={`#${scrollId}`}
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:100
- Preserve normal anchor behavior for modified clicks. As written, Ctrl/Cmd/Shift-clicking a different tab is prevented and navigates the current page instead of opening the link in a new tab/window.
if (typeof eventKey !== "string" || eventKey === decodeURI(pathname)) {
return;
}
event.preventDefault();
navigate(eventKey);
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:93
- A caller-provided
onSelectinotherPropsis spread ontoTabsafter this handler, replacing it entirely. In that case this delegation and the new client-side navigation never run; spread the remaining tab props before installing the composed handler.
eventKey) => {
otherProps.onSelect?.(event, eventKey);
js/apps/admin-ui/test/identity-providers/main.ts:96
- This helper is also called from
test/clients/advanced.spec.ts:238while the page is on a client's Advanced tab (and is called twice in a loop), where the provider card does not exist. The first call will time out; navigate to the provider add route/list before clicking.
await clickProviderCard(page, "default-trust");
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
js/libs/ui-shared/src/scroll-form/ScrollForm.tsx:84
nodeis a CSS selector, butscrollIdis derived from translated—and for user-profile forms, user-configured—titles. Titles beginning with a digit or containing selector punctuation produce an invalid selector, so PatternFly cannot find/observe the section; escape the ID when constructing the selector.
node={`#${scrollId}`}
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:93
- This routing handler is overridden whenever a caller supplies
onSelect, becauseotherProps(which still contains that prop) is spread afterward on line 110. Destructure the external callback out ofotherPropsand invoke it here so both the callback and client-side navigation run.
eventKey) => {
otherProps.onSelect?.(event, eventKey);
js/apps/admin-ui/src/main.tsx:25
- Preserve the encoded hash path here. Route parameters are deliberately percent-encoded, so decoding before
replaceStatecan change segment boundaries or create malformed paths (for example, a valid realm name containing%becomes a bare%); the browser router should receive the raw hash substring and decode route parameters itself.
const hashPath = decodeURIComponent(window.location.hash.substring(1));
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
js/apps/admin-ui/test/identity-providers/default-trust.spec.ts:32
- Catching every deletion failure makes authentication, connectivity, and server errors look like the expected “provider absent” case, allowing leaked test state to go unnoticed. Track whether creation succeeded or suppress only the not-found response and rethrow other cleanup failures.
try {
await adminClient.deleteIdentityProvider(alias);
} catch {
// The provider may already be deleted when creation fails.
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:93
otherPropsstill containsonSelectand is spread onto<Tabs>after this handler, so any consumer-provided callback replaces the router handler instead of being composed with it. ExtractonSelectseparately or place the composed handler after the spread so tab navigation remains client-side when a callback is supplied.
eventKey) => {
otherProps.onSelect?.(event, eventKey);
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:100
- This intercepts Ctrl/Cmd/Shift/Alt and non-primary-button clicks and calls
navigate, so routable tabs can no longer be opened in a new tab/window despite rendering as anchors. Only prevent the default action and navigate for an unmodified primary click; leave modified/non-primary clicks to the browser.
event.preventDefault();
navigate(eventKey);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (5)
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:100
- Clicking the already-active tab returns before cancelling the anchor's default action, so the browser reloads the current Admin Console URL. Prevent the default action before treating the current path as a no-op so all ordinary tab clicks remain client-side.
if (typeof eventKey !== "string" || eventKey === decodeURI(pathname)) {
return;
}
event.preventDefault();
navigate(eventKey);
js/apps/admin-ui/src/main.tsx:25
- Decoding the entire legacy hash also decodes reserved characters inside route parameters and query values before the value becomes a browser URL. For example,
%26becomes&and splits one query value into two parameters; preserve the encoded hash and let React Router decode route values.
const hashPath = decodeURIComponent(window.location.hash.substring(1));
js/libs/ui-shared/src/scroll-form/ScrollForm.tsx:84
nodeis a CSS selector, butscrollIdcomes from localized or user-defined titles and only whitespace is normalized. A title containing selector syntax such as:,., or(makes this selector invalid or point elsewhere, so the JumpLink cannot observe/focus its section; escape the ID for selector use.
node={`#${scrollId}`}
services/src/main/java/org/keycloak/services/resources/admin/AdminConsole.java:327
- This catch-all overlaps the existing
@Path("{indexhtml: index.html}")GET below, and both are single-parameter templates with an explicit regex, so JAX-RS cannot prefer the legacy index redirect for/index.html. Make that route a literal@Path("index.html"), as inAccountConsole.java:283, or exclude it from this fallback.
@Path("{path:.*}")
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:93
- A caller-provided
onSelectremains inotherPropsand is spread after this handler, so it replaces the wrapper entirely; the forwarding here never runs and those tabs still perform native navigation. DestructureonSelectfrom the rest props or spread the remaining props before installing this handler.
This issue also appears on line 95 of the same file.
eventKey) => {
otherProps.onSelect?.(event, eventKey);
Switch the Admin Console from hash routing to browser routing with server-side deep-link fallback, and handle routable tab clicks through React Router so tab changes stay client-side. With path-based routing in place, remove the ScrollForm hash-safe click override and rely on PatternFly JumpLinks href/node behavior, including stable anchors for bordered panels. Closes keycloak#39693 Closes keycloak#45494 Related: patternfly/patternfly-react#12223 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:101
- This compares
eventKeyagainstdecodeURI(pathname)but then navigates usingnavigate(eventKey). If the tab’s intended destination includes asearch/hash(or if encoding differs), selecting the tab can (a) trigger redundant navigations due to mismatched normalization and (b) drop query/hash parts when navigating. A more robust approach is to compare against the current location using the same normalization as your link generation, and navigate to the full target (e.g., the computed href/to object) rather than only the pathname.
eventKey) => {
otherProps.onSelect?.(event, eventKey);
if (typeof eventKey !== "string" || eventKey === decodeURI(pathname)) {
return;
}
event.preventDefault();
navigate(eventKey);
}}
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:137
- Changing prop order to
<Tab {...props} href={href}>forceshrefto be derived only fromeventKeyand overrides anyhrefpassed in throughprops. This can break callers that intentionally provide anhrefcomputed from a fullto(includingsearch/hash). If you want to support richer destinations, either preferprops.hrefwhen present or computehreffrom the sametoobject the caller uses (and keephrefconsistent with whatnavigate(...)uses).
const href = useHref(props.eventKey);
return (
<Tab {...props} href={href}>
{children}
</Tab>
js/apps/admin-ui/src/main.tsx:26
- The hash-to-path migration drops any existing
window.location.search(query string) that may be present on the legacy URL, becausereplaceStateis called with only a joined pathname. If query parameters are used (now or in the future) for page state (e.g., pagination/filtering), this migration will silently lose them. Consider preservingwindow.location.searchwhen rewriting the URL.
if (window.location.hash.startsWith("#/")) {
const hashPath = decodeURIComponent(window.location.hash.substring(1));
window.history.replaceState(null, "", joinPath(basename, hashPath));
}
js/apps/admin-ui/test/identity-providers/main.ts:97
- Defaulting to
{ rootRealm: 'master', realm: 'master' }when the regex doesn’t match can mask real navigation/state issues and send tests to the wrong realm without an obvious failure cause. Prefer failing fast (e.g., throw with a clear message) or returningundefinedand handling it at the call site, so mismatched URLs are surfaced immediately.
function getCurrentAdminConsoleRealms(page: Page) {
const { pathname } = new URL(page.url());
const match = /^\/admin\/([^/]+)\/console\/([^/]+)/.exec(pathname);
return {
rootRealm: match?.[1] ?? "master",
realm: match?.[2] ?? "master",
};
}
js/apps/admin-ui/src/main.tsx:27
- The new hash-to-path migration is behaviorally significant (backwards compatibility for legacy bookmarked URLs). Consider adding an automated Playwright test that loads a legacy
#/...deep link and asserts it is rewritten to the equivalent path route and renders the expected page (and, if applicable, preserves query parameters).
if (window.location.hash.startsWith("#/")) {
const hashPath = decodeURIComponent(window.location.hash.substring(1));
window.history.replaceState(null, "", joinPath(basename, hashPath));
}
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (6)
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:101
- Tab navigation uses
navigate(eventKey)whereeventKeyis a string derived fromto.pathname(seeuseRoutableTab). This drops any search params / hashes that may have been included in the originaltodestination, and can cause incorrect navigation for tabs that rely on query strings. Consider navigating to the same value used for the tab link target (the full href/path including search/hash), or changing the eventKey to contain the full destination string.
eventKey) => {
otherProps.onSelect?.(event, eventKey);
if (typeof eventKey !== "string" || eventKey === decodeURI(pathname)) {
return;
}
event.preventDefault();
navigate(eventKey);
}}
js/apps/admin-ui/src/components/routable-tabs/RoutableTabs.tsx:148
DynamicTabrecomputeshreffromprops.eventKey(a pathname string), which means anysearch/hashthatuseRoutableTab()computed intohrefwill be ignored at render time. This is a behavioral change from the previous implementation and can produce incorrect tab URLs whentoincludes query or hash. Prefer rendering the tab using thehrefpassed in props (so the full destination is preserved), or ensureeventKeycarries the full destination used to build thehref.
const href = useHref(props.eventKey);
return (
<Tab {...props} href={href}>
{children}
</Tab>
);
};
export const useRoutableTab = (to: Partial<Path>) => {
const href = useHref(to);
return {
eventKey: to.pathname ?? "",
href,
};
};
js/apps/admin-ui/src/main.tsx:25
decodeURIComponent(...)can throw on malformed percent-encoding in the hash (e.g. user-entered URLs), which would prevent the app from booting. Wrap the decode in a try/catch (falling back to the raw substring), or use a safer decoding approach so invalid hashes don't crash initial render.
if (window.location.hash.startsWith("#/")) {
const hashPath = decodeURIComponent(window.location.hash.substring(1));
window.history.replaceState(null, "", joinPath(basename, hashPath));
}
js/apps/admin-ui/src/context/realm-context/RealmContext.tsx:29
- Decoding the entire
pathnamewithdecodeURIComponentcan (1) throw on malformed encodings and (2) change path semantics by decoding reserved characters like%2Finto/, which would break realm extraction viasplit(\"/\"). Safer options: avoid decoding altogether (React Router already provides a usablepathname), or decode only the specific path segment that represents the realm after splitting.
const { pathname } = useLocation();
js/apps/admin-ui/src/context/realm-context/RealmContext.tsx:36
- Decoding the entire
pathnamewithdecodeURIComponentcan (1) throw on malformed encodings and (2) change path semantics by decoding reserved characters like%2Finto/, which would break realm extraction viasplit(\"/\"). Safer options: avoid decoding altogether (React Router already provides a usablepathname), or decode only the specific path segment that represents the realm after splitting.
const locationRealm = decodeURIComponent(pathname);
const realm = locationRealm.split("/")[1] || environment.realm;
js/apps/admin-ui/test/identity-providers/default-trust.spec.ts:33
- Catching all errors during cleanup can mask genuine operational issues (e.g., auth/network problems) and make failures harder to diagnose. If
adminClient.deleteIdentityProviderexposes the HTTP status / error shape, consider only swallowing the expected 'not found' case (e.g., 404) and rethrowing or logging unexpected errors.
try {
await adminClient.deleteIdentityProvider(alias);
} catch {
// The provider may already be deleted when creation fails.
}
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
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.
Switch the Admin Console from hash routing to browser routing with
server-side deep-link fallback, and handle routable tab clicks through
React Router so tab changes stay client-side. With path-based routing in
place, remove the ScrollForm hash-safe click override and rely on
PatternFly JumpLinks href/node behavior, including stable anchors for
bordered panels.
Closes #32979
Closes #45494
Related: patternfly/patternfly-react#12223
Signed-off-by: Erik Jan de Wit erikjan.dewit@gmail.com