feat(admin-ui): revamp Home -> WebHooks page as per Figma (#2629)#2663
Merged
feat(admin-ui): revamp Home -> WebHooks page as per Figma (#2629)#2663
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLarge refactor introducing a typed Webhooks feature (API, hooks, UI, forms, styles, tests) and broad migration from legacy JS Redux/sagas/APIs to TypeScript/react-query/hooks; adds centralized theming/custom CSS variables, new query utilities, many component refactors (tables, inputs, select, tooltips), test setup improvements, and replaces console logs with a devLogger. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Webhook UI
participant Hook as React‑Query Hook
participant API as WebhookApi
participant Q as QueryClient
User->>UI: open list / trigger webhook / submit form
UI->>Hook: read/query or mutate
Hook->>API: call typed WebhookApi (HTTP)
API-->>Hook: response / error
Hook->>Q: invalidate/refetch via queryUtils
Q-->>UI: update cache → UI re-renders
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
comp-admin-ui
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 42
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
admin-ui/app/routes/Dashboards/DashboardPage.style.ts (1)
8-15: 🧹 Nitpick | 🔵 Trivial
DashboardThemeColorsinterface has 4 unused fields in themakeStylescallback.Only
cardBgandtextare referenced inside themakeStylescallback.cardBorder,textSecondary,statusCardBg, andstatusCardBorderare never consumed by the style function (they're used directly ondashboardThemeColorsinDashboardPage.tsx, not via styles). The over-specified interface adds noise to the makeStyles type parameter.♻️ Proposed trimmed interface
interface DashboardThemeColors { cardBg: string - cardBorder: string text: string - textSecondary: string - statusCardBg: string - statusCardBorder: string }Then in
DashboardPage.tsx, typedashboardThemeColorswith a wider local type (oras const) that includes the extra fields, so they can still be used for component props.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Dashboards/DashboardPage.style.ts` around lines 8 - 15, The DashboardThemeColors interface used as the makeStyles type parameter is over-specified: only cardBg and text are referenced by the makeStyles callback (function makeStyles / styles object), so remove unused fields (cardBorder, textSecondary, statusCardBg, statusCardBorder) from the DashboardThemeColors interface in DashboardPage.style.ts; then keep the extra fields in the concrete dashboardThemeColors value in DashboardPage.tsx by typing it with a wider local type or using `as const` so props/components can still consume them. Ensure the only keys in the styles type are cardBg and text to avoid the noise in makeStyles while preserving the full theme shape at the usage site.admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx (2)
151-156:⚠️ Potential issue | 🟡 Minor
tabIndex={-1}on tooltip trigger breaks keyboard accessibility.Setting
tabIndex={-1}removes theHelpOutlineicon from the tab order, making the tooltip unreachable for keyboard and screen reader users. Tooltip triggers should be keyboard-focusable (tabIndex={0}) and show their tooltip on focus as well as hover.♿ Proposed fix
<HelpOutline - tabIndex={-1} + tabIndex={0} style={{ width: 18, height: 18, marginLeft: 6, marginRight: 6 }} data-tooltip-id={tooltip} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx` around lines 151 - 156, The HelpOutline icon uses tabIndex={-1} which removes it from keyboard focus; change tabIndex to 0 on the HelpOutline component so it is keyboard-focusable and ensure the tooltip (data-tooltip-id/data-for using the tooltip variable) also appears on focus as well as hover—add an onFocus handler (matching existing onMouseEnter/onMouseLeave behavior) or ensure the tooltip library is configured to show on focus, and add appropriate aria attributes (e.g., aria-describedby referencing the tooltip id) so screen readers can access it.
148-158:⚠️ Potential issue | 🟡 MinorRemove stale
data-forattribute.
data-foris a react-tooltip v4 anchor attribute; react-tooltip v5 usesdata-tooltip-id. Since the codebase is using v5 (^5.28.0),data-foris dead code and should be removed to avoid confusion.This issue appears in multiple files: GluuProperties.tsx, GluuLabel.tsx (2 instances), and ShortcodePopover.tsx. Consider a broader cleanup across the codebase.
🧹 Proposed cleanup for this file
<HelpOutline tabIndex={-1} style={{ width: 18, height: 18, marginLeft: 6, marginRight: 6 }} data-tooltip-id={tooltip} - data-for={tooltip} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx` around lines 148 - 158, Remove the stale react-tooltip v4 anchor attribute `data-for` wherever it appears; specifically in the JSX around the GluuTooltip/HelpOutline usage (e.g., the fragment in GluuProperties.tsx referencing GluuTooltip and HelpOutline) and the other reported locations (GluuLabel occurrences and ShortcodePopover.tsx). Delete the `data-for={tooltip}` prop and keep `data-tooltip-id={tooltip}` as the single tooltip identifier, ensuring no other v4-specific attributes remain to avoid confusion with react-tooltip v5.admin-ui/app/routes/Apps/Gluu/Tests/GluuSelectRow.test.tsx (1)
38-56: 🧹 Nitpick | 🔵 TrivialDuplicate assertion in "renders select with all options".
Line 55 (
expect(select).toHaveValue(value)) repeats the sole assertion from the "renders select with the correct initial value" test. Since this test's purpose is verifying all options are rendered, the final assertion is redundant and can be removed to keep test scope focused.♻️ Proposed cleanup
test('renders select with all options', () => { ... const select = screen.getByRole('combobox') values.forEach((optionValue) => { expect(screen.getByRole('option', { name: optionValue })).toBeInTheDocument() }) - expect(select).toHaveValue(value) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/Tests/GluuSelectRow.test.tsx` around lines 38 - 56, The test "renders select with all options" contains a redundant assertion checking the select's value; remove the final expect(select).toHaveValue(value) from the test that renders <GluuSelectRow ... /> so the test only asserts that each option (values array) is present via getByRole('option'); keep the separate "renders select with the correct initial value" test to cover value assertions and ensure the test name and remaining assertions in the "renders select with all options" test reflect its sole purpose of verifying options are rendered.admin-ui/plugins/admin/components/Webhook/WebhookEditPage.tsx (1)
14-43: 🧹 Nitpick | 🔵 Trivial
WebhookEditPageandWebhookAddPageare nearly identical — extract the shared shell.Both components share the exact same structure (theming setup, Cedarling permission check,
GluuViewWrapper, styled container,WebhookForm) and differ only in theSetTitlestring. This is a full copy-paste duplication that will diverge silently over time.Consider extracting the repeated scaffolding into a single component or hook:
// Shared shell, e.g. WebhookPageShell.tsx interface WebhookPageShellProps { titleKey: string; defaultTitle: string } const WebhookPageShell: React.FC<WebhookPageShellProps> = ({ titleKey, defaultTitle }) => { // ... shared theme/permission/style setup SetTitle(t(titleKey, { defaultValue: defaultTitle })) return ( <GluuPageContent> <GluuViewWrapper canShow={canReadWebhooks}> <div className={classes.formCard}> <div className={classes.content}><WebhookForm /></div> </div> </GluuViewWrapper> </GluuPageContent> ) } // WebhookAddPage export default memo(() => <WebhookPageShell titleKey="titles.add_webhook" defaultTitle="Add Webhook" />) // WebhookEditPage export default memo(() => <WebhookPageShell titleKey="titles.edit_webhook" defaultTitle="Edit Webhook" />)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Webhook/WebhookEditPage.tsx` around lines 14 - 43, WebhookAddPage and WebhookEditPage duplicate the same scaffolding (theme setup via useTheme/getThemeColor, isDark, useStyles, permission check via useCedarling, webhookResourceId/canReadWebhooks, SetTitle, GluuViewWrapper and WebhookForm); extract this into a single WebhookPageShell component that accepts titleKey/defaultTitle props and performs the shared logic (useTheme, getThemeColor, THEME_DARK, useStyles, hasCedarReadPermission, webhookResourceId, SetTitle) and renders the GluuPageContent → GluuViewWrapper → formCard → content → WebhookForm structure, then refactor WebhookAddPage and WebhookEditPage to each return memo(() => <WebhookPageShell ...>) with the appropriate titleKey/defaultTitle.admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx (1)
45-57: 🧹 Nitpick | 🔵 Trivial
formik?.setFieldValuein the dependency array — useformikdirectly per codebase convention.Same pattern as in GluuSelectRow: extracting individual Formik methods into dependency arrays can cause issues with Formik's internal field registration and focus management.
Proposed change
[formik?.setFieldValue, name, onChange], + [formik, name, onChange],Based on learnings: "In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx` around lines 45 - 57, The handleChange callback in GluuTypeAhead currently lists formik?.setFieldValue in its dependency array which fragments Formik usage; update the dependency array to include the whole formik object instead (e.g., [formik, name, onChange]) so Formik's internal registration and focus management remain stable—adjust the useCallback for handleChange (and mirror the same pattern used in GluuSelectRow) to reference formik directly rather than individual Formik methods like setFieldValue.admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx (1)
49-57: 🧹 Nitpick | 🔵 TrivialDependency array uses
formik.handleChange— verify stability.
formik.handleChangein theuseCallbackdependency array is acceptable here sinceformikis a prop (not the localuseFormikreturn). However, if the parent passes a new formik object reference on each render, this callback will be recreated unnecessarily. Based on learnings, keeping the entireformikobject in the dependency array is the recommended pattern in this codebase.Consider using `formik` directly in deps
const handleSelectChange = useCallback( (event: React.ChangeEvent<HTMLSelectElement>) => { formik.handleChange(event) if (handleChange) { handleChange(event) } }, - [formik.handleChange, handleChange], + [formik, handleChange], )Based on learnings: "In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx` around lines 49 - 57, The dependency array for the useCallback defining handleSelectChange currently references formik.handleChange which can cause unnecessary recreations if the parent passes a new formik object reference; update the dependency array to include the whole formik object instead of formik.handleChange (i.e., use [formik, handleChange]) while keeping the body calling formik.handleChange(event) and conditionally calling the prop handleChange(event) so the callback stability follows the codebase pattern.admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (1)
246-263: 🧹 Nitpick | 🔵 Trivial
handleSelectShortcodeis not wrapped inuseCallbackbut is passed as a prop toShortcodePopover.It captures
cursorPositionandformikValues, so a new function reference is created on every render. IfShortcodePopoveris wrapped inReact.memo(or uses the handler in a dependency array), this causes unnecessary re-renders on every keystroke. Wrap it inuseCallbackwith[cursorPosition, formikValues, setFieldValue, setCursorPosition].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx` around lines 246 - 263, handleSelectShortcode is recreated each render and passed to ShortcodePopover causing unnecessary re-renders; wrap the handler in React's useCallback so it memoizes its reference. Change the declaration of handleSelectShortcode to useCallback(() => { ... }, [cursorPosition, formikValues, setFieldValue, setCursorPosition]) (keeping the same parameters and logic) so it only changes when those dependencies change; continue passing the memoized handleSelectShortcode to ShortcodePopover.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/app/components/Card/Card.scss`:
- Around line 48-50: The shadow color changed from the original blue-gray tint
to pure black because the .Card &--shadow rule now uses
rgba(var(--custom-black-rgb), 0.07); restore the intended blue-gray shadow by
either replacing rgba(var(--custom-black-rgb), 0.07) with the original
hard-coded value rgba(31, 45, 61, 0.07) in the &--shadow rule or introduce/point
to a new CSS variable (e.g., --custom-shadow-rgb or --custom-blue-gray-rgb) set
to "31, 45, 61" and use rgba(var(--custom-shadow-rgb), 0.07) so the shadow
matches the Figma spec.
In `@admin-ui/app/components/GluuSearchToolbar/GluuSearchToolbar.tsx`:
- Around line 73-87: The effect conflates two concerns by using lastNotifiedRef
for both the last prop value seen and the last value notified via onSearch;
create a separate ref (e.g., lastPropRef) initialized to searchValue and use it
to detect prop changes, leaving lastNotifiedRef to only track the last value
passed to onSearch. Specifically, in the useEffect for GluuSearchToolbar:
replace the check/assignment that compares searchValue with
lastNotifiedRef.current to compare with lastPropRef.current and update
lastPropRef.current when props change (then setLocalSearch(searchValue) and
return), and keep the debounced notification branch using lastNotifiedRef to
prevent duplicate onSearch calls (update lastNotifiedRef.current when calling
onSearch with debouncedSearch). Ensure debouncedSearch, searchOnType, onSearch
and searchValue remain in the dependency array.
In `@admin-ui/app/components/GluuTable/GluuTable.tsx`:
- Around line 198-203: The sort chevron (ChevronIcon rendered inside
classes.sortIconWrap) is misleading for inactive columns because it always
points "up" even though the table only toggles between null and 'desc'; update
the rendering so the ChevronIcon is not shown for inactive columns (i.e., render
ChevronIcon only when isActive is true) or alternatively hide it by default and
reveal it on header hover via the data-sort-icon wrapper CSS; adjust any related
styles/logic that assume the icon is always present and ensure behavior
references sortDirection and isActive.
In `@admin-ui/app/context/theme/config.ts`:
- Line 35: Remove the dead property table.headerText from the ThemeConfig
interface and from the theme objects in config.ts: delete the table.headerText
declaration in the ThemeConfig type and remove the corresponding headerText
entries in the light and dark theme configs, leaving table.headerColor as the
sole header-related property; ensure no other code references table.headerText
before committing.
In `@admin-ui/app/customColors.ts`:
- Around line 112-120: getCustomColorsAsCssVars currently recomputes the same
map on every call even though customColors is a const; instead compute the map
once at module load (e.g. build a module-level constant like
CUSTOM_COLORS_CSS_VARS) by iterating over customColors and using toKebabCase and
hexToRgb, and then have getCustomColorsAsCssVars return that precomputed map
(optionally return a shallow copy or Object.freeze to prevent external
mutation). Ensure you reference the existing symbols getCustomColorsAsCssVars,
customColors, toKebabCase, and hexToRgb when locating where to move the
iteration.
In `@admin-ui/app/i18n.ts`:
- Around line 17-22: Replace the dynamic CommonJS require inside
parseMissingKeyHandler with a static ES import: add a top-level "import { toast
} from 'react-toastify'" and remove the inline require and eslint-disable; keep
the existing call site (toast.warning(...)) and options (autoClose, toastId)
unchanged so parseMissingKeyHandler and its usage (isDevelopment ?
parseMissingKeyHandler : undefined) continue to work the same way.
In `@admin-ui/app/routes/Apps/Gluu/GluuInputRow.tsx`:
- Around line 60-79: The current casts in stepUp and stepDown synthesize a plain
object as React.ChangeEvent<HTMLInputElement>; update the GluuInputRowProps
handleChange type to reflect reality by allowing a union parameter such as (e:
React.ChangeEvent<HTMLInputElement> | { target: { name: string; value: string }
}) => void (or an equivalent custom type) so callers can pass either real events
or plain { target: { name, value } } objects, then remove the unsafe "as
React.ChangeEvent<HTMLInputElement>" casts in stepUp and stepDown (or
alternatively remove the casts entirely and let TypeScript infer the union) to
keep types accurate for handleChange, formik.setFieldValue usage, and consumers
like UserForm/SmtpForm.
In `@admin-ui/app/routes/Apps/Gluu/GluuLabel.tsx`:
- Around line 64-88: The inline style on the HelpOutline icon redundantly sets
both color and fill; remove the redundant fill property so the SvgIcon uses
color for its fill. Update the JSX for the HelpOutline element (the component
instance that uses props data-tooltip-id={doc_entry} and data-for={doc_entry})
to keep style.width, style.height, style.marginLeft, style.marginRight and
style.color (labelColor) but remove style.fill, ensuring GluuTooltip (with
tooltipOnly) and the HelpOutline trigger remain unchanged.
In `@admin-ui/app/routes/Apps/Gluu/GluuTooltip.tsx`:
- Around line 42-49: The conditional for tooltipContent (variables:
tooltipContent, contentOverride, isDirect, doc_category, doc_entry) returns
doc_category when isDirect is truthy and ignores doc_entry, which is
non-obvious; update the code by adding a concise inline comment immediately
above or beside the isDirect branch explaining the intended behavior (e.g.,
"when isDirect is true we render the raw doc_category string rather than a
localized entry — preserves legacy direct links"), so future maintainers
understand why doc_entry is not used in that branch; keep the comment short and
reference the reasoning (legacy behavior/intent) and leave the conditional logic
unchanged unless you intend to change behavior.
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuInputRow.style.ts`:
- Around line 27-54: The style objects numberWrapper and numberStepper use
quoted keys for plain CSS properties (e.g., 'position', 'display', 'alignItems',
'width') which is inconsistent with colWrapper; update numberWrapper and
numberStepper to use unquoted property names for standard CSS keys while keeping
selector keys (like '& input' or nested selectors) quoted, preserving computed
values such as the borderLeft expression in numberStepper and the nested '&
button' rules.
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuTooltip.style.ts`:
- Around line 8-20: Add a brief inline comment inside the getLabelTooltipStyle
function explaining that colors are intentionally derived from
getThemeColor(THEME_LIGHT) for both branches to create an inverted-contrast
tooltip (i.e., tooltip intentionally uses light-theme colors on dark pages and
inverted colors on light pages) so future maintainers do not refactor to use
THEME_DARK when isDarkTheme is true; reference getLabelTooltipStyle, isDarkTheme
and getThemeColor(THEME_LIGHT) in the comment for clarity.
In `@admin-ui/app/routes/Apps/Gluu/types/GluuInputEditor.types.ts`:
- Around line 3-9: The type GluuInputEditorCursorValue uses the internal Ace
property document?.$lines which is unstable; update the shape for the
cursor.document to use public Ace Document methods (e.g., replace document?: {
$lines: string[] } with document?: { getAllLines(): string[] } or document?: {
getLine(row: number): string; getLength(): number } or reference the official
Ace Document type) and, if you must keep the legacy array shape for
compatibility, add a comment stating the exact Ace version this mapping was
verified against; update references to GluuInputEditorCursorValue, cursor, and
document accordingly so callers use getAllLines()/getLine()/getLength() instead
of accessing $lines directly.
In `@admin-ui/app/routes/Apps/Gluu/types/GluuInputRow.types.ts`:
- Around line 1-25: The GluuInputRowProps type references React types but React
isn't imported; add a type-only import from 'react' (import type { ChangeEvent,
FocusEvent, ReactNode } from 'react') and update GluuInputRowProps to use
ChangeEvent<HTMLInputElement> for handleChange, FocusEvent<HTMLInputElement> for
onFocus, and ReactNode → ReactNode for shortcode so the handleChange, onFocus,
and shortcode properties use the imported types; keep the import type syntax to
avoid runtime import.
In `@admin-ui/app/routes/Apps/Profile/ProfilePage.tsx`:
- Around line 175-176: The inline computed constants displayName and displayMail
should be moved into useMemo to match the rest of the component's derived values
and avoid unnecessary recomputation; replace the current inline expressions with
two useMemo hooks (e.g., const displayName = useMemo(() =>
profileDetails?.displayName ?? userinfo?.name ?? userinfo?.email ?? '-',
[profileDetails, userinfo]) and const displayMail = useMemo(() =>
profileDetails?.mail ?? userinfo?.email ?? '-', [profileDetails, userinfo])) so
they only recompute when profileDetails or userinfo change and preserve the
existing fallback order.
In `@admin-ui/app/styles/bootstrap.scss`:
- Line 2: The global primary color "#00b875" defined in bootstrap.scss fails
WCAG contrast against white; update the primary token (the "primary" value in
bootstrap.scss) to an accessible alternative (e.g., replace with "#008f5a" or a
darker shade that achieves at least 3:1 for UI components or 4.5:1 for normal
text depending on project policy), then re-check all components that use
$primary or .btn-primary/.text-primary/.rbt-token to ensure white text remains
compliant and adjust the chosen hex until automated contrast checks pass;
document the final hex and compliance level chosen.
In `@admin-ui/app/styles/main.scss`:
- Around line 417-419: The hover state for the table sort label uses a border
token instead of a text token: update the rule targeting .MuiTableCell-head
.MuiTableSortLabel-root:hover to use the theme-aware header text token(s) (e.g.
var(--custom-table-header-text-light) for light theme and/or
var(--custom-table-header-text-dark) for dark theme) instead of
var(--custom-light-border) so the hover color has sufficient contrast for text
in headers.
In `@admin-ui/app/utils/ApiKeyRedirect.tsx`:
- Around line 47-49: The current showRedirectingLoader logic (using
islicenseCheckResultLoaded and isConfigValid) can hang indefinitely if those
flags never resolve; add a timeout/retry fallback so the app surfaces an error
instead of an eternal spinner. In the component that computes
showRedirectingLoader, introduce a short-lived state (e.g., initialLoadTimedOut)
driven by a useEffect timer or a retry counter that flips after a configurable
timeout/retry limit, then include that state in the condition that disables the
loader and triggers an error/fallback UI (show an explicit error message or
route to an error component), and ensure you check backendStatus or surface
underlying network errors so users get actionable feedback; update references to
showRedirectingLoader, islicenseCheckResultLoaded, and isConfigValid
accordingly.
- Around line 47-54: The loader can block the GluuServiceDownModal when the
license check hasn't completed; update the showRedirectingLoader logic so it
also requires backendStatus.active before bailing into the loader (i.e., ensure
the early-exit branches that check islicenseCheckResultLoaded and isConfigValid
also include a backendStatus.active guard or make backendStatus.active required
for the entire expression), so GluuServiceDownModal will be reachable when
backendStatus.active is false; change the expression around
showRedirectingLoader (referencing showRedirectingLoader,
islicenseCheckResultLoaded, isConfigValid, and backendStatus.active)
accordingly.
In `@admin-ui/app/utils/queryUtils.ts`:
- Around line 24-27: The function removeQueriesByKey currently returns
Promise<void> while calling the synchronous QueryClient.removeQueries; change
its signature and implementation to return void (not a Promise) so it matches
the sync behavior and is consistent with its namesake, or alternatively document
why it must be async; specifically update the exported function
removeQueriesByKey to have a void return type and remove the Promise.resolve
wrapper (locate usages to ensure callers don't await it), and align its API
surface with invalidateQueriesByKey and refetchQueriesByKey.
- Around line 3-14: The object queryDefaults currently duplicates time keys
(staleTimeMs/gcTimeMs) at top-level and inside queryOptions alongside
TanStack-compatible keys (staleTime/gcTime), causing ambiguity and drift; remove
the redundant *Ms aliases and keep only TanStack option names (staleTime,
gcTime, refetchOnWindowFocus) so queryDefaults.queryOptions can be spread
directly into queries, or alternatively derive staleTime/gcTime from the *Ms
values programmatically; update the queryDefaults export (and any callers using
staleTimeMs/gcTimeMs) to use the unified keys and ensure queryOptions is the
canonical source for configuration.
In `@admin-ui/app/utils/RouteLoader.tsx`:
- Around line 44-63: The current dynamic import catch blocks for plugin route
loading (the import(`../../plugins/${pluginName}/components/${componentPath}`)
promise chain in RouteLoader.tsx) only log via console.warn when isDevelopment
is true, leaving production failures unreported; update the two catch handlers
to call the telemetry/error-reporting utility (e.g., captureException or
reportError) unconditionally with the error and context (include pluginName and
componentPath) before continuing — for the primary catch send the primary error,
and for the fallback catch send both primaryError and fallbackError — then
rethrow the fallbackErr as before so behavior doesn't change.
In `@admin-ui/plugins/admin/components/Mapping/hooks/useMappingApi.ts`:
- Line 64: The call to extractErrorMessage narrows the thrown value to Error |
ApiError and drops the Record<string, never> branch; change the cast so the full
accepted union is preserved (pass the error as Error | ApiError | Record<string,
never> or remove the cast and let the function accept the runtime value) when
calling extractErrorMessage(error, t('messages.error_updating_mapping')) in
useMappingApi (also update the other occurrence at the later call around line
103) so resolveErrorMessage/extractErrorMessage can handle plain object throws
safely.
In `@admin-ui/plugins/admin/components/Mapping/types/MappingTypes.ts`:
- Around line 23-27: The ThemeContextValue interface is unrelated to Mapping and
should be moved out of MappingTypes; create or use a shared/global context types
module (e.g., a new shared/types or contexts/types file), move the
ThemeContextValue definition there, and update all references/imports (including
any usages in MappingTypes.ts and components that import ThemeContextValue) to
import it from the new shared module so the Mapping plugin no longer owns a
generic theme type.
- Around line 5-8: The MutationCallbacks type is duplicated; extract the single
type definition (MutationCallbacks) into a shared types module (e.g.,
commonTypes.ts in a shared or root types barrel), export it from there, remove
the duplicate definitions from the MappingTypes and WebhookTypes files, and
update the modules that currently re-export it to import and re-export the
canonical MutationCallbacks from the new shared module so both plugins use the
same exported type.
In `@admin-ui/plugins/admin/components/Settings/SettingsPage.style.ts`:
- Around line 200-207: The styles for customParamsActionBtn use '!important' on
minHeight and height; remove those '!important' entries and instead increase
specificity or use MUI's sx prop: either wrap the size rules under a
higher-specificity selector in the style object (e.g., add '&&': { minHeight:
44, height: 44, gap: 8, flexShrink: 0 } inside customParamsActionBtn) or delete
the height fields here and set them where the Button is rendered via the sx prop
(e.g., sx={{ minHeight: 44, height: 44 }}). Ensure values are numeric (44) not
strings with '!important' and keep the rest of customParamsActionBtn
(minWidth/width/gap/flexShrink) intact.
- Line 123: The constant MAPPING_SPACING.INFO_ALERT_BORDER_RADIUS is
semantically misleading because it's used across general UI elements (e.g., form
inputs) — update the shared token to a more general name or create a clear alias
and replace usages: either rename MAPPING_SPACING.INFO_ALERT_BORDER_RADIUS to
MAPPING_SPACING.SMALL_BORDER_RADIUS (or MAPPING_SPACING.INPUT_BORDER_RADIUS) in
the mapping definition and update all references (including
SettingsPage.style.ts borderRadius) accordingly, or add a new alias
MAPPING_SPACING.INPUT_BORDER_RADIUS that points to the same value and update
files to use the new name to make intent explicit; ensure exports/types are
updated so consumers import the new symbol.
In `@admin-ui/plugins/admin/components/Settings/SettingsPage.tsx`:
- Around line 425-429: Extract the repeated empty-state check for
formik.values.additionalParameters into a local boolean (e.g.,
isAdditionalParamsEmpty) and use that variable in the JSX className expressions
for classes.customParamsBox and classes.customParamsHeader instead of
recomputing (formik.values.additionalParameters || []).length === 0; update any
other places in SettingsPage.tsx that perform the same check to use this single
variable so the logic is clearer and not duplicated.
- Around line 337-354: The paging-size select currently triggers
formik.handleChange twice because GluuSelectRow.handleSelectChange already calls
formik.handleChange(event) and the inline handler in SettingsPage.tsx also calls
formik.handleChange(e); remove the redundant call from the inline handleChange
prop. In SettingsPage.tsx, update the inline handler passed to GluuSelectRow
(the function that currently calls formik.handleChange(e), parses the value, and
calls handlePagingSizeChange) to only parse Number.parseInt(e.target.value, 10),
check for NaN, and call handlePagingSizeChange(n); keep formik handling
delegated to GluuSelectRow.handleSelectChange and leave references to formik,
handlePagingSizeChange, and GluuSelectRow intact.
In `@admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts`:
- Around line 25-31: Move the AuditInit interface and ActionData type out of the
hook function body so they are declared at module scope (make them top-level
declarations in useWebhookAudit.ts); update the index signatures from [key:
string]: string | { user_inum: string; userId: string } | object to use unknown
instead of object (e.g. [key: string]: string | { user_inum: string; userId:
string } | unknown) and change any occurrences inside ActionData similarly so
arrays/functions/classes aren't incorrectly allowed; ensure the hook
(useWebhookAudit) and any references still import/see the renamed module-scope
AuditInit and ActionData definitions (also apply the same move/change for the
second declaration referenced around line 45).
In `@admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts`:
- Around line 43-44: The toast messages in useWebhookMutations are hardcoded
English strings (e.g., the dispatch(updateToast(...)) calls inside
useWebhookMutations) which bypass i18n; change the hook signature to accept
either a translation function (t: (key:string)=>string) or pre-translated
message strings via input params/callbacks and replace all hardcoded messages
(used in dispatch(updateToast(...)) and any error handling paths before
queryUtils.invalidateQueriesByKey/getGetAllWebhooksQueryKey() calls) to use
those provided translated strings; ensure callers pass in messages like
messages.failed_to_create_webhook/messages.webhook_created_successfully so the
hook no longer directly depends on useTranslation.
In `@admin-ui/plugins/admin/components/Webhook/styles/ShortcodePopover.style.ts`:
- Around line 46-48: Update the divider style to use the theme's border color
instead of the font color: in ShortcodePopover.style.ts change the divider
object so its borderColor references themeColors.borderColor (the same
ThemeConfig value used by the paper border) rather than themeColors.fontColor;
this keeps Divider styling consistent with the paper border.
In `@admin-ui/plugins/admin/components/Webhook/styles/WebhookFormPage.style.ts`:
- Around line 40-41: The two variables are assigned in reverse: swap the values
so headersBoxBg gets settings?.customParamsBox (or falls back to cardBg) and
headersInputBg gets settings?.customParamsInput (or falls back to formInputBg);
update the assignments for headersBoxBg and headersInputBg in
WebhookFormPage.style.ts accordingly so the outer container uses customParamsBox
and the input fields use customParamsInput.
- Around line 34-43: The optional chaining on required ThemeConfig fields is
unnecessary: update the three constants infoBg, infoBorder, and infoText in
WebhookFormPage.style.ts to access themeColors.infoAlert via dot notation
(themeColors.infoAlert.background, .border, .text) instead of using ?. so the
fallback logic (customColors.*) remains the same; leave other optional checks
(e.g., settings?.*) unchanged.
In `@admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts`:
- Around line 8-11: The MutationCallbacks type is duplicated; extract the type
alias MutationCallbacks into a new shared types module (e.g., common.ts), export
it from there, then remove the local MutationCallbacks definitions in the
Webhook and Mapping modules and replace them with an import of MutationCallbacks
from the new shared module; update the WebhookTypes.ts and MappingTypes.ts files
to import the shared MutationCallbacks and run type checks to ensure no
remaining duplicates or import errors.
In `@admin-ui/plugins/admin/components/Webhook/WebhookAddPage.tsx`:
- Line 22: The useMemo wrapper around the constant assignment for
webhookResourceId is unnecessary; replace the useMemo expression in
WebhookAddPage.tsx (the webhookResourceId declaration) with a plain constant
assignment to ADMIN_UI_RESOURCES.Webhooks (no hooks needed) so the value is
computed once at module render without the overhead of useMemo.
In `@admin-ui/plugins/admin/components/Webhook/WebhookEditPage.tsx`:
- Line 22: Replace the unnecessary useMemo wrapper for the static resource:
instead of const webhookResourceId = useMemo(() => ADMIN_UI_RESOURCES.Webhooks,
[]), assign the constant directly with const webhookResourceId =
ADMIN_UI_RESOURCES.Webhooks (update in WebhookEditPage.tsx where
webhookResourceId and ADMIN_UI_RESOURCES.Webhooks are referenced).
In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx`:
- Around line 477-509: The list rendering uses key={index} causing
reconciliation/focus bugs; assign a stable unique id to each header object when
created (update addHeader to push headers with a generated _id, e.g., UUID or
timestamp), extend the HttpHeader type to include optional _id, use that _id as
the React key in the map instead of index, ensure changeHeader and removeHeader
operate by _id or by locating the item index first, and strip the internal _id
from headers when building the payload in submitForm so it is not sent to the
server.
- Around line 345-363: In WebhookForm, avoid shadowing the route param id and
recreating the inline formik object each render: rename the local variable
inside the select change handler (e.g., selectedId) so it does not shadow const
{ id } = useParams(), and move the formik object (or at least the handleChange
function) out of the JSX into a stable reference using useCallback/useMemo;
implement handleChange as a useCallback that reads (e.target as
HTMLSelectElement).value into selectedId, finds the AuiFeature from features and
calls setSelectedFeatures, then pass the memoized formik object to the
GluuSelectRow props to prevent unnecessary re-renders/effect triggers.
- Around line 296-306: The current useEffect in WebhookForm that reads
headersBodyRef and sets inline styles with setProperty('background-color', ...,
'important') should be removed; instead, pass headerInputBg as a style prop (or
a dedicated prop like inputStyle) directly to the Input elements that render
entries from formikValues.httpHeaders (where inputs are created) so each Input
receives backgroundColor: headerInputBg inline, and remove headersBodyRef and
the DOM mutation logic; if the Input's CSS still overrides inline styles, fix
the Input component's stylesheet or expose a higher-priority styling prop rather
than using !important.
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Around line 344-356: The delete action is using classes.editIcon for the
DeleteOutlined icon; add a new CSS class name (e.g., deleteIcon) in the
component's styles and replace the use of classes.editIcon on the DeleteOutlined
element inside the action object (the block guarded by canDeleteWebhooks in the
function that builds the list) with classes.deleteIcon so delete and edit icons
can be styled independently; ensure the new deleteIcon is exported from the same
makeStyles/useStyles and applied only to DeleteOutlined (leaving other uses of
editIcon untouched).
- Around line 30-32: toWebhookEntries currently force-casts
PagedResultEntriesItem[] to WebhookEntry[] which discards type safety; replace
the unsafe cast with a real mapper or tighten types: implement a mapping
function in toWebhookEntries that converts each PagedResultEntriesItem ->
WebhookEntry by explicitly selecting/transforming the required fields (or add a
runtime shape-assertion/validation) and return the mapped array, or
alternatively make WebhookEntry a structural alias/extension of
PagedResultEntriesItem so the cast is safe and add a short comment explaining
compatibility; reference toWebhookEntries, PagedResultEntriesItem and
WebhookEntry to locate and fix the code.
- Around line 422-426: The delete-dialog label construction allows a leading '-'
when deleteData.url is empty but deleteData.inum exists; update the label logic
in WebhookListPage.tsx (the label prop that uses modal && deleteData ?
`${t('messages.action_deletion_for')} ${t('messages.webhook_entity')}
(${deleteData.url || ''}${deleteData.inum ? `-${deleteData.inum}` : ''})` : ''`)
so the dash is only added when both deleteData.url and deleteData.inum are
present—e.g., build the inside text from two parts (urlPart = deleteData.url ||
'' and inumPart = deleteData.inum ? `-${deleteData.inum}` : '') but only prepend
the '-' when urlPart is non-empty, or join non-empty parts with a '-' between
them.
---
Outside diff comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx`:
- Around line 151-156: The HelpOutline icon uses tabIndex={-1} which removes it
from keyboard focus; change tabIndex to 0 on the HelpOutline component so it is
keyboard-focusable and ensure the tooltip (data-tooltip-id/data-for using the
tooltip variable) also appears on focus as well as hover—add an onFocus handler
(matching existing onMouseEnter/onMouseLeave behavior) or ensure the tooltip
library is configured to show on focus, and add appropriate aria attributes
(e.g., aria-describedby referencing the tooltip id) so screen readers can access
it.
- Around line 148-158: Remove the stale react-tooltip v4 anchor attribute
`data-for` wherever it appears; specifically in the JSX around the
GluuTooltip/HelpOutline usage (e.g., the fragment in GluuProperties.tsx
referencing GluuTooltip and HelpOutline) and the other reported locations
(GluuLabel occurrences and ShortcodePopover.tsx). Delete the
`data-for={tooltip}` prop and keep `data-tooltip-id={tooltip}` as the single
tooltip identifier, ensuring no other v4-specific attributes remain to avoid
confusion with react-tooltip v5.
In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx`:
- Around line 49-57: The dependency array for the useCallback defining
handleSelectChange currently references formik.handleChange which can cause
unnecessary recreations if the parent passes a new formik object reference;
update the dependency array to include the whole formik object instead of
formik.handleChange (i.e., use [formik, handleChange]) while keeping the body
calling formik.handleChange(event) and conditionally calling the prop
handleChange(event) so the callback stability follows the codebase pattern.
In `@admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx`:
- Around line 45-57: The handleChange callback in GluuTypeAhead currently lists
formik?.setFieldValue in its dependency array which fragments Formik usage;
update the dependency array to include the whole formik object instead (e.g.,
[formik, name, onChange]) so Formik's internal registration and focus management
remain stable—adjust the useCallback for handleChange (and mirror the same
pattern used in GluuSelectRow) to reference formik directly rather than
individual Formik methods like setFieldValue.
In `@admin-ui/app/routes/Apps/Gluu/Tests/GluuSelectRow.test.tsx`:
- Around line 38-56: The test "renders select with all options" contains a
redundant assertion checking the select's value; remove the final
expect(select).toHaveValue(value) from the test that renders <GluuSelectRow ...
/> so the test only asserts that each option (values array) is present via
getByRole('option'); keep the separate "renders select with the correct initial
value" test to cover value assertions and ensure the test name and remaining
assertions in the "renders select with all options" test reflect its sole
purpose of verifying options are rendered.
In `@admin-ui/app/routes/Dashboards/DashboardPage.style.ts`:
- Around line 8-15: The DashboardThemeColors interface used as the makeStyles
type parameter is over-specified: only cardBg and text are referenced by the
makeStyles callback (function makeStyles / styles object), so remove unused
fields (cardBorder, textSecondary, statusCardBg, statusCardBorder) from the
DashboardThemeColors interface in DashboardPage.style.ts; then keep the extra
fields in the concrete dashboardThemeColors value in DashboardPage.tsx by typing
it with a wider local type or using `as const` so props/components can still
consume them. Ensure the only keys in the styles type are cardBg and text to
avoid the noise in makeStyles while preserving the full theme shape at the usage
site.
In `@admin-ui/plugins/admin/components/Webhook/WebhookEditPage.tsx`:
- Around line 14-43: WebhookAddPage and WebhookEditPage duplicate the same
scaffolding (theme setup via useTheme/getThemeColor, isDark, useStyles,
permission check via useCedarling, webhookResourceId/canReadWebhooks, SetTitle,
GluuViewWrapper and WebhookForm); extract this into a single WebhookPageShell
component that accepts titleKey/defaultTitle props and performs the shared logic
(useTheme, getThemeColor, THEME_DARK, useStyles, hasCedarReadPermission,
webhookResourceId, SetTitle) and renders the GluuPageContent → GluuViewWrapper →
formCard → content → WebhookForm structure, then refactor WebhookAddPage and
WebhookEditPage to each return memo(() => <WebhookPageShell ...>) with the
appropriate titleKey/defaultTitle.
In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx`:
- Around line 246-263: handleSelectShortcode is recreated each render and passed
to ShortcodePopover causing unnecessary re-renders; wrap the handler in React's
useCallback so it memoizes its reference. Change the declaration of
handleSelectShortcode to useCallback(() => { ... }, [cursorPosition,
formikValues, setFieldValue, setCursorPosition]) (keeping the same parameters
and logic) so it only changes when those dependencies change; continue passing
the memoized handleSelectShortcode to ShortcodePopover.
---
Duplicate comments:
In `@admin-ui/plugins/admin/components/Webhook/WebhookAddPage.tsx`:
- Around line 14-43: WebhookAddPage is a near-duplicate of WebhookEditPage;
extract their common layout/shell (the GluuPageContent + GluuViewWrapper +
styling/permission logic) into a shared component (e.g., WebhookShell) and have
both WebhookAddPage and WebhookEditPage render that shell with different
children or props (pass WebhookForm and any edit-specific props/handlers). Move
shared hooks/values (useTranslation, useTheme/getThemeColor, useCedarling
permission check, SetTitle) into the shell and keep only page-specific behavior
(like supplying form mode or initial data) in the individual pages to eliminate
duplication.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
admin-ui/app/utilities.tsx (1)
18-24:⚠️ Potential issue | 🟡 Minor
randomArrayis not random — it always returnsarr[0].The function name implies a random pick, but the implementation unconditionally returns the first element.
randomAvatar()is actively used in ProfilePage.tsx to set avatar images, so every user profile will display the same avatar instead of varying avatars.🐛 Proposed fix
export function randomArray<T>(arr: T[]): T { - return arr[0] + return arr[Math.floor(Math.random() * arr.length)] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/utilities.tsx` around lines 18 - 24, randomArray currently always returns the first element (arr[0]) causing randomAvatar to never vary; change randomArray<T>(arr: T[]) to select and return a uniformly random element from arr (e.g. using Math.random() * arr.length) and handle the empty-array case (throw or return undefined) so randomAvatar() (which calls randomArray(allAvatars)) will produce varied avatars; update function names randomArray and randomAvatar usages accordingly and add a small unit check or guard to ensure allAvatars is non-empty before calling.admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx (2)
138-165: 🧹 Nitpick | 🔵 TrivialDead code: the
navigateToRoutecall in theactiveIndex !== -1branch is unreachable.
activeIndexis derived from:tabNames.findIndex((tab) => isNavigationTab(tab) && tab.path === path.pathname)So when
activeIndex !== -1,activeTab.pathis guaranteed equal topath.pathnameby the findIndex predicate. The guard on line 162 (activeTab.path !== path.pathname) therefore always evaluates tofalse, andnavigateToRoutecan never be called here. OnlysetValue(activeIndex)on line 165 is reachable.♻️ Proposed cleanup
const activeTab = tabNames[activeIndex] ?? null - if (isNavigationTab(activeTab) && activeTab.path !== path.pathname) { - navigateToRoute(activeTab.path, { replace: true }) - } setValue(activeIndex)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx` around lines 138 - 165, The branch that calls navigateToRoute when activeIndex !== -1 is dead because activeIndex was found by matching tab.path === path.pathname; remove the unreachable navigateToRoute check or adjust logic: in the useEffect that computes activeIndex from tabNames.findIndex((tab) => isNavigationTab(tab) && tab.path === path.pathname), delete the if (isNavigationTab(activeTab) && activeTab.path !== path.pathname) navigateToRoute(...) block and retain only setValue(activeIndex), or if navigation should occur when path differs, change the findIndex predicate and related checks so activeIndex no longer guarantees path equality; update references to activeTab, isNavigationTab, navigateToRoute, and setValue accordingly.
107-134: 🧹 Nitpick | 🔵 Trivial
useMemowith empty[]deps for static style objects adds overhead with no benefit.
tabsSxandtabsContainerSxreference only module-level constants (customColors.*and MUI tokens). AuseMemo([])is semantically identical to a module-levelconstbut incurs an extra hook call and closure allocation on every component mount.♻️ Hoist to module-level constants
+const tabsSx = { + '& .MuiTab-root.Mui-selected': { + color: customColors.lightBlue, + fontWeight: 600, + background: customColors.lightBlue, + WebkitBackgroundClip: 'text', + WebkitTextFillColor: 'transparent', + backgroundClip: 'text', + position: 'relative', + }, + '& .MuiTabs-indicator': { + background: customColors.lightBlue, + height: 3, + borderRadius: '2px', + boxShadow: `0 2px 4px ${customColors.logo}`, + }, +} + +const tabsContainerSx = { + borderBottom: 1, + borderColor: 'divider', +} export default function GluuTabs(...) { ... - const tabsSx = useMemo( - () => ({ - ... - }), - [], - ) - - const tabsContainerSx = useMemo( - () => ({ - ... - }), - [], - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx` around lines 107 - 134, The style objects tabsSx and tabsContainerSx are created via useMemo with empty deps which is unnecessary — hoist them to module-level constants to avoid the extra hook/closure allocation; replace the useMemo(() => ({...}), []) calls for tabsSx and tabsContainerSx with top-level const declarations (using the same object shapes and referencing customColors and MUI tokens) and then reference those constants inside the component instead of calling useMemo.admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (1)
291-304:⚠️ Potential issue | 🟡 Minor
shouldShowErrorthird condition triggers errors on pre-populated fields immediately at form mount.The newly added condition
value !== undefined && value !== null && String(value).length > 0(Line 300) causesshowErrorto fire for any field that has a non-empty value and a validation error — regardless of whether the user has interacted with the field or submitted the form. In edit mode, every pre-populated field with a validation error will display that error the instant the form renders, before any user action.The parallel
WebsiteSsoServiceProviderFormuses only(touched || submitCount > 0)and does not include this "non-empty value" trigger, making the two forms behave inconsistently.If showing errors immediately for pre-populated invalid data is intentional, document it; otherwise, remove the third condition:
🐛 Proposed fix (align with SP form pattern)
return Boolean( error && - (touched || - formik.submitCount > 0 || - (value !== undefined && value !== null && String(value).length > 0)), + (touched || formik.submitCount > 0), )And update the dependency array accordingly:
- [formik.errors, formik.touched, formik.values, formik.submitCount], + [formik.errors, formik.touched, formik.submitCount],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx` around lines 291 - 304, The shouldShowError callback (function shouldShowError) currently treats any non-empty field value as a reason to show validation errors immediately, causing pre-populated fields to display errors on mount; remove the third condition checking value !== undefined && value !== null && String(value).length > 0 so the predicate matches the other form (only touched || formik.submitCount > 0), and update the hook dependencies to remove formik.values (leave [formik.errors, formik.touched, formik.submitCount]) so dependencies reflect the simplified logic.admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
436-442:⚠️ Potential issue | 🟡 Minor
spMetaDataSourceTypemissing|| formik.submitCount > 0— inconsistent with all other required fields.Every other required field in this form was updated to
(formik.touched.X || formik.submitCount > 0), butspMetaDataSourceTypestill uses the old pattern (touched-only). A user who clicks Submit without ever focusing the dropdown will not see a validation error for this required field, while errors for all sibling fields will appear.🐛 Proposed fix
showError={Boolean( - formik.errors.spMetaDataSourceType && formik.touched.spMetaDataSourceType, + formik.errors.spMetaDataSourceType && + (formik.touched.spMetaDataSourceType || formik.submitCount > 0), )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx` around lines 436 - 442, The showError check for the spMetaDataSourceType field currently only shows errors when the field is touched; update the condition used in the component where showError is set (the prop for spMetaDataSourceType inside WebsiteSsoServiceProviderForm) to mirror other required fields by using (formik.touched.spMetaDataSourceType || formik.submitCount > 0) combined with formik.errors.spMetaDataSourceType so the validation message appears after a submit even if the dropdown was never focused.admin-ui/plugins/admin/components/Settings/SettingsPage.tsx (2)
205-205: 🛠️ Refactor suggestion | 🟠 MajorAvoid extracting
resetFormfrom Formik.Destructuring
resetFormout offormikand using it inhandleCancel's dependency array (line 233) contradicts the established pattern of keeping the entireformikobject in dependency arrays. Extracting individual methods can break Formik's internal field registration and focus management.♻️ Proposed fix
- const { resetForm } = formik - const isPagingSizeChanged = currentPagingSize !== baselinePagingSizeThen in
handleCancel:const handleCancel = useCallback(() => { const resetValues = transformToFormValues(config) - resetForm({ values: resetValues }) + formik.resetForm({ values: resetValues }) setCurrentPagingSize(baselinePagingSize) - }, [config, transformToFormValues, baselinePagingSize, resetForm]) + }, [config, transformToFormValues, baselinePagingSize, formik])Based on learnings: "In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Settings/SettingsPage.tsx` at line 205, The code currently destructures resetForm from formik, which can break Formik's internals; revert this by removing the const { resetForm } = formik extraction and instead reference formik.resetForm inside handleCancel (and any other uses), and ensure handleCancel's dependency array uses the whole formik object (formik) rather than a destructured resetForm variable; update any imports/usages so only the formik object is referenced in dependencies and calls to resetForm are invoked via formik.resetForm().
87-91: 🧹 Nitpick | 🔵 Trivial
useMemowrapping constant values is unnecessary.
ADMIN_UI_RESOURCES.SettingsandCEDAR_RESOURCE_SCOPES[...]are both stable constants. These can be plain module-level assignments likewebhookResourceIdwas refactored to inWebhookAddPage.tsx.♻️ Proposed fix
Move outside the component:
+const settingsResourceId = ADMIN_UI_RESOURCES.Settings +const settingsScopes = CEDAR_RESOURCE_SCOPES[settingsResourceId] || [] + const SettingsPage: React.FC = () => { ... - const settingsResourceId = useMemo(() => ADMIN_UI_RESOURCES.Settings, []) - const settingsScopes = useMemo( - () => CEDAR_RESOURCE_SCOPES[settingsResourceId] || [], - [settingsResourceId], - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Settings/SettingsPage.tsx` around lines 87 - 91, Remove unnecessary useMemo wrappers around stable constants: replace the inside-component const settingsResourceId = useMemo(() => ADMIN_UI_RESOURCES.Settings, []) and settingsScopes = useMemo(() => CEDAR_RESOURCE_SCOPES[settingsResourceId] || [], [settingsResourceId]) with module-level (or plain in-component) constant assignments using ADMIN_UI_RESOURCES.Settings and CEDAR_RESOURCE_SCOPES directly (e.g., const settingsResourceId = ADMIN_UI_RESOURCES.Settings; const settingsScopes = CEDAR_RESOURCE_SCOPES[settingsResourceId] || [];), keeping the same identifiers so SettingsPage and any references continue to work.admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx (1)
50-58: 🛠️ Refactor suggestion | 🟠 MajorUse
formik(notformik.handleChange) in the dependency array.Line 57 lists
formik.handleChangein theuseCallbackdependency array. Per the established pattern, the entireformikobject should be in the dependency array to avoid issues with Formik's internal field registration and focus management.♻️ Proposed fix
const handleSelectChange = useCallback( (event: React.ChangeEvent<HTMLSelectElement>) => { formik.handleChange(event) if (handleChange) { handleChange(event) } }, - [formik.handleChange, handleChange], + [formik, handleChange], )Based on learnings: "In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx` around lines 50 - 58, The dependency array for the useCallback creating handleSelectChange currently uses formik.handleChange; update it to depend on the whole formik object instead to follow Formik's recommended pattern (prevent issues with internal field registration/focus). Locate the handleSelectChange callback in GluuSelectRow (the useCallback that calls formik.handleChange and optional handleChange) and replace formik.handleChange in the dependency array with formik (keeping handleChange as-is).admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (1)
276-333: 🧹 Nitpick | 🔵 Trivial
resetFormandsetBaselineSelectedFeaturesafter mutation are effectively dead code.
onSuccesscallbacks (lines 80, 83) navigate to the list page, which unmounts this component. Lines 315–316 then execute on an unmounting/unmounted component. React 18 ignores this silently, but the code is misleading — it suggests the form stays mounted post-save.If keeping the form mounted post-save is ever intended (e.g., "save and continue editing"), the reset should use the server response rather than
formikValues. Otherwise, consider removing lines 315–316 or moving the navigation intosubmitFormitself so the intent is clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx` around lines 276 - 333, The post-mutation calls resetForm and setBaselineSelectedFeatures in submitForm are dead/misleading because the mutation onSuccess handlers navigate away (unmounting the component); remove lines that call resetForm({ values: formikValues }) and setBaselineSelectedFeatures([...selectedFeatures]) from the submitForm callback, or alternatively stop relying on onSuccess navigation and move navigation into submitForm so you can reset using the server response (use returned webhook payload to reset the form and setBaselineSelectedFeatures). Target the submitForm function and the resetForm and setBaselineSelectedFeatures calls when applying this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/app/locales/pt/translation.json`:
- Around line 971-973: Move the "request_body_error" key out of the
"placeholders" section and into the "messages" section so that
webhookValidation.ts can find messages.request_body_error; update the JSON by
cutting the "request_body_error" entry from the "placeholders" block and pasting
it under "messages" alongside other message keys (keeping the Portuguese
translation string unchanged) to match the structure used by
English/Spanish/French.
In `@admin-ui/app/routes/Apps/Gluu/GluuInputRow.tsx`:
- Around line 149-156: The visibility icons in GluuInputRow.tsx (inside the
password block that checks type === 'password') are currently plain icons using
onClick with customType and setVisibility and lack keyboard/screen-reader
accessibility; wrap the Visibility/VisibilityOff icons in a semantic <button
type="button"> (or an accessible element with role="button"), give it an
accessible label (aria-label like "Toggle password visibility" or aria-pressed
reflecting customType === 'text'), ensure it receives focus (no tabIndex needed
if a button), and keep the existing onClick handler (setVisibility); also ensure
the wrapper uses classes.passwordToggle for layout and that any onKeyDown
handles Enter/Space if you choose a non-button element.
In `@admin-ui/app/routes/Apps/Gluu/GluuLabel.tsx`:
- Around line 35-37: GluuLabel.tsx calls useTheme (via "const { state:
themeState } = useTheme()") which throws when rendered outside a ThemeProvider;
update the tests in GluuLabel.test.tsx to render the component inside the
ThemeProvider (or use your shared custom render util that injects ThemeProvider)
so useTheme has a provider, or alternatively change GluuLabel to tolerate
missing theme by not calling useTheme unguarded (e.g., accept an isDarkProp
fallback and only call useTheme when a ThemeProvider is present). Ensure
references are to useTheme, themeState, isDarkProp, GluuLabel.tsx and that all
test renders wrap the component with ThemeProvider.
In `@admin-ui/app/routes/Apps/Gluu/GluuText.tsx`:
- Line 60: The JSX spread {...rest} should be placed before the explicit props
so explicit bindings win; update the render in GluuText (the <Component ...>
element) to move {...rest} before className, style (combinedStyle), and id —
i.e., ensure {...rest} appears first in the prop list while keeping
className={className}, style={combinedStyle}, and id={id} as explicit props so
they cannot be accidentally overridden by rest.
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuSelectRow.style.ts`:
- Line 35: Replace the magic number zIndex: 6 in the style object with a named
constant; add an exported constant (e.g., SELECT_ROW_Z_INDEX or
GLUU_SELECT_ROW_Z_INDEX) near the top of GluuSelectRow.style.ts and use that
constant in place of the literal in the style definition (the object containing
zIndex). This makes the intent explicit and centralizes the value for future
tweaks.
In `@admin-ui/app/routes/Apps/Gluu/types/GluuInputEditor.types.ts`:
- Line 27: The property declaration "shortcode?: React.ReactNode" in
GluuInputEditor.types.ts references the React namespace but this is a .ts file
without React in scope; import the type directly by adding "import { ReactNode }
from 'react'" at the top of the file and change the type to "ReactNode" (or
alternatively add a default React import if you prefer), and apply the same
pattern to other affected .ts type files (e.g., GluuDropdown/types.ts,
Layout/types.ts, Sidebar/types.ts) that use "React.ReactNode".
In `@admin-ui/app/styles/main.scss`:
- Around line 417-419: The hover rule for .MuiTableCell-head
.MuiTableSortLabel-root:hover currently uses only
--custom-table-header-text-light; update it to be theme-aware by scoping the
rule under html.theme-light and html.theme-dark like the existing pattern and
use --custom-table-header-text-light for html.theme-light and
--custom-table-header-text-dark for html.theme-dark so the hover color matches
the active theme.
In `@admin-ui/plugins/admin/components/Mapping/hooks/useMappingApi.ts`:
- Line 70: The call to callbacksRef.current?.onError currently collapses
non-Error throws into a plain Error (callbacksRef.current?.onError?.(error
instanceof Error ? error : new Error(String(error)))), losing structured
ApiError data; change this to pass Error instances unchanged and for non-Error
throws preserve the original object by wrapping with an Error that includes the
original as the cause/attached property (so callers can inspect the original
ApiError fields) instead of only String(error); also remove the internal
error-toast dispatches inside useUpdateMappingWithAudit and
useAddMappingWithAudit so these hooks follow the SAML plugin pattern (no
internal toasts) and leave toast responsibility to callers.
In `@admin-ui/plugins/admin/components/Webhook/hooks/useGetWebhook.ts`:
- Line 9: In the useGetWebhook hook, stop passing webhookId as the server-side
text search "pattern" — set pattern to an empty string (pattern: '') and then
client-side filter the returned list by inum (e.g., entries.find(e => e.inum ===
webhookId)); also remove the unreachable entries[0] branch (it can't be hit
because enabled: Boolean(webhookId) ensures the query only runs for truthy
webhookId). If a single-webhook backend endpoint exists, prefer calling that
instead of list+filter.
In `@admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts`:
- Line 25: Remove the accidental `| unknown` members that collapse the unions:
change the index signature ` [key: string]: string | { user_inum: string;
userId: string } | unknown` to not include `unknown` in that union (keep `string
| { user_inum: string; userId: string }`), and similarly remove `| unknown` from
`string | number | boolean | unknown | null` so it becomes `string | number |
boolean | null`; if your intent was to replace a previous `object` catch-all
with `unknown`, do that by replacing the old `object` index signature with a
single `unknown` index signature (not by adding `unknown` into existing unions).
In `@admin-ui/plugins/admin/components/Webhook/styles/ShortcodePopover.style.ts`:
- Around line 56-63: The helpIcon style sets both color and fill which is
redundant for MUI SvgIcon because color maps to the SVG via currentColor; remove
the fill: themeColors.fontColor entry from the helpIcon style (the block
referencing helpIcon, HELP_ICON_SIZE, HELP_ICON_MARGIN and
themeColors.fontColor) so the SvgIcon relies on color only, and verify no other
code depends on an explicit fill property.
In `@admin-ui/plugins/admin/components/Webhook/styles/WebhookFormPage.style.ts`:
- Around line 147-151: The deeply nested selector '& > div:first-of-type label,
& > div:first-of-type label h5, & > div:first-of-type label span' in
WebhookFormPage.style.ts is fragile and uses !important; replace this with a
dedicated CSS class (e.g., .webhook-form-label) defined in the same style file
and apply the styles without !important, then update the corresponding
label/h5/span elements in the Webhook form component to include that class (or a
parent wrapper class) so styling no longer depends on the exact DOM nesting.
In `@admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts`:
- Line 97: The narrowing of iconProps to Record<string, string | number |
boolean> breaks callers that pass nested objects like style and sx; restore a
permissive type on iconProps (e.g., Record<string, any> or Record<string,
unknown>, or a union that includes CSSProperties/SxProps) in WebhookTypes'
iconProps declaration so existing usages (components passing style/sx objects)
continue to compile, or alternatively update all call sites (LdapListPage,
UserList, SsaListPage, AgamaAliasListPage, etc.) to remove object-valued
properties; prefer changing iconProps back to a broad type (iconProps?:
Record<string, unknown> or any) to minimize churn.
In `@admin-ui/plugins/admin/components/Webhook/WebhookAddPage.tsx`:
- Around line 23-27: Replace the read-permission check with a write-permission
check in WebhookAddPage: change the useMemo that computes canReadWebhooks (which
currently calls hasCedarReadPermission(webhookResourceId)) to call
hasCedarWritePermission(webhookResourceId) instead, and rename the variable to
reflect its purpose (e.g., canWriteWebhooks) so GluuViewWrapper gates the form
by write access; mirror the pattern used in WebhookListPage where the "Add"
control is disabled for users without write permission.
In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx`:
- Around line 340-344: The useEffect in WebhookForm references formik.errors and
formik.validateForm but intentionally omits formik from the dependency array
which trips exhaustive-deps; add a clear suppression comment above this effect
(// eslint-disable-next-line react-hooks/exhaustive-deps) and a brief inline
justification comment mentioning why formik is excluded, and optionally extract
the errors length into a stable ref if you later need stability—keep references
to formik.errors and formik.validateForm inside the existing useEffect tied to
i18n.language so the behavior remains unchanged.
- Around line 558-611: The inline anonymous onCursorChange passed to
GluuInputEditor should be extracted into a stable callback using
React.useCallback to avoid recreating the function every render; create a named
callback (e.g., handleHttpRequestBodyCursorChange) that accepts the same value
param, performs the same index calculation and calls setCursorPosition((prev) =>
({ ...prev, httpRequestBody: index })), and then replace the inline prop with
this callback on GluuInputEditor; include only necessary dependencies in the
useCallback deps (setCursorPosition is sufficient if you keep the functional
state update) and ensure the callback signature/type matches the current
onCursorChange handler.
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Around line 209-225: The nested ternary in the useMemo that builds sortOptions
is hard to read; replace it with a simple lookup map keyed by the column name
(used inside SORT_COLUMNS) mapping to the translation key, then inside the
useMemo map each value to { value, label: t(lookup[value] || 'fields.status') }.
Update the referenced symbol names: sortOptions, SORT_COLUMNS and t to use that
lookup (e.g., labelMap) so the logic is clear and easy to extend.
- Around line 139-158: The catch block in the submitForm callback is duplicating
an error toast because useDeleteWebhookWithAudit (used by deleteWebhook) already
dispatches an error and re-throws; remove the duplicate dispatch call from the
catch block in submitForm (the dispatch(updateToast(true, TOAST_TYPE_ERROR,
...)) call) so the mutation's error handling provides the toast once—keep the
try/catch to clear deleteData and handle refetch, but do not dispatch another
toast in submitForm.
- Line 370: getRowKey currently only accepts the row and returns an empty string
for entries without inum, causing key collisions; update the getRowKey signature
to accept both (row: WebhookEntry, index: number) and return a stable unique
key, e.g., use row.inum when present and fall back to a deterministic
index-based key (like `no-inum-${index}`) so GluuTable receives distinct keys
for reconciliation.
In `@admin-ui/plugins/admin/helper/webhook.ts`:
- Line 11: The type for webhook.httpRequestBody is too broad and redundant:
remove "| object | string" and constrain it to JsonValue (or JsonValue |
Record<string, unknown> if you need plain objects) so
JSON.stringify(webhook.httpRequestBody, ...) cannot throw on non-plain objects;
update the property definition (the httpRequestBody type) where it's declared
and adjust any callers if they relied on accepting arbitrary objects or strings.
In `@admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx`:
- Around line 438-441: The form shows errors inconsistently because top-level
fields use shouldShowError() (which includes a "non-empty value" check) while
the eight manual-metadata fields use inline showError logic; make the behavior
uniform by swapping the inline boolean expressions for those manual fields
(e.g., idpEntityId and the other manual-metadata inputs at the ranges noted) to
call shouldShowError('fieldName') instead of the current inline checks, so all
fields use the same visibility rule implemented in shouldShowError;
alternatively, if you prefer to remove the "non-empty value" rule globally,
remove that third condition from the shouldShowError implementation and keep the
inline expressions as-is.
---
Outside diff comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx`:
- Around line 50-58: The dependency array for the useCallback creating
handleSelectChange currently uses formik.handleChange; update it to depend on
the whole formik object instead to follow Formik's recommended pattern (prevent
issues with internal field registration/focus). Locate the handleSelectChange
callback in GluuSelectRow (the useCallback that calls formik.handleChange and
optional handleChange) and replace formik.handleChange in the dependency array
with formik (keeping handleChange as-is).
In `@admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx`:
- Around line 138-165: The branch that calls navigateToRoute when activeIndex
!== -1 is dead because activeIndex was found by matching tab.path ===
path.pathname; remove the unreachable navigateToRoute check or adjust logic: in
the useEffect that computes activeIndex from tabNames.findIndex((tab) =>
isNavigationTab(tab) && tab.path === path.pathname), delete the if
(isNavigationTab(activeTab) && activeTab.path !== path.pathname)
navigateToRoute(...) block and retain only setValue(activeIndex), or if
navigation should occur when path differs, change the findIndex predicate and
related checks so activeIndex no longer guarantees path equality; update
references to activeTab, isNavigationTab, navigateToRoute, and setValue
accordingly.
- Around line 107-134: The style objects tabsSx and tabsContainerSx are created
via useMemo with empty deps which is unnecessary — hoist them to module-level
constants to avoid the extra hook/closure allocation; replace the useMemo(() =>
({...}), []) calls for tabsSx and tabsContainerSx with top-level const
declarations (using the same object shapes and referencing customColors and MUI
tokens) and then reference those constants inside the component instead of
calling useMemo.
In `@admin-ui/app/utilities.tsx`:
- Around line 18-24: randomArray currently always returns the first element
(arr[0]) causing randomAvatar to never vary; change randomArray<T>(arr: T[]) to
select and return a uniformly random element from arr (e.g. using Math.random()
* arr.length) and handle the empty-array case (throw or return undefined) so
randomAvatar() (which calls randomArray(allAvatars)) will produce varied
avatars; update function names randomArray and randomAvatar usages accordingly
and add a small unit check or guard to ensure allAvatars is non-empty before
calling.
In `@admin-ui/plugins/admin/components/Settings/SettingsPage.tsx`:
- Line 205: The code currently destructures resetForm from formik, which can
break Formik's internals; revert this by removing the const { resetForm } =
formik extraction and instead reference formik.resetForm inside handleCancel
(and any other uses), and ensure handleCancel's dependency array uses the whole
formik object (formik) rather than a destructured resetForm variable; update any
imports/usages so only the formik object is referenced in dependencies and calls
to resetForm are invoked via formik.resetForm().
- Around line 87-91: Remove unnecessary useMemo wrappers around stable
constants: replace the inside-component const settingsResourceId = useMemo(() =>
ADMIN_UI_RESOURCES.Settings, []) and settingsScopes = useMemo(() =>
CEDAR_RESOURCE_SCOPES[settingsResourceId] || [], [settingsResourceId]) with
module-level (or plain in-component) constant assignments using
ADMIN_UI_RESOURCES.Settings and CEDAR_RESOURCE_SCOPES directly (e.g., const
settingsResourceId = ADMIN_UI_RESOURCES.Settings; const settingsScopes =
CEDAR_RESOURCE_SCOPES[settingsResourceId] || [];), keeping the same identifiers
so SettingsPage and any references continue to work.
In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx`:
- Around line 276-333: The post-mutation calls resetForm and
setBaselineSelectedFeatures in submitForm are dead/misleading because the
mutation onSuccess handlers navigate away (unmounting the component); remove
lines that call resetForm({ values: formikValues }) and
setBaselineSelectedFeatures([...selectedFeatures]) from the submitForm callback,
or alternatively stop relying on onSuccess navigation and move navigation into
submitForm so you can reset using the server response (use returned webhook
payload to reset the form and setBaselineSelectedFeatures). Target the
submitForm function and the resetForm and setBaselineSelectedFeatures calls when
applying this change.
In `@admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx`:
- Around line 291-304: The shouldShowError callback (function shouldShowError)
currently treats any non-empty field value as a reason to show validation errors
immediately, causing pre-populated fields to display errors on mount; remove the
third condition checking value !== undefined && value !== null &&
String(value).length > 0 so the predicate matches the other form (only touched
|| formik.submitCount > 0), and update the hook dependencies to remove
formik.values (leave [formik.errors, formik.touched, formik.submitCount]) so
dependencies reflect the simplified logic.
In `@admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx`:
- Around line 436-442: The showError check for the spMetaDataSourceType field
currently only shows errors when the field is touched; update the condition used
in the component where showError is set (the prop for spMetaDataSourceType
inside WebsiteSsoServiceProviderForm) to mirror other required fields by using
(formik.touched.spMetaDataSourceType || formik.submitCount > 0) combined with
formik.errors.spMetaDataSourceType so the validation message appears after a
submit even if the dropdown was never focused.
---
Duplicate comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuLabel.tsx`:
- Around line 64-87: No change needed: the tooltip/icon rendering block using
GluuTooltip and HelpOutline (with props doc_entry, doc_category, place="right",
data-tooltip-id and data-for set to doc_entry and styled via labelColor) is
correct and can be approved as-is; leave the conditional JSX around doc_category
&& doc_entry && i18n.exists('documentation.' + doc_category + '.' + doc_entry)
intact and do not modify GluuTooltip or HelpOutline usage.
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuInputRow.style.ts`:
- Around line 29-57: The object uses inconsistent quoting for regular CSS
property keys in numberWrapper and numberStepper; unquote standard keys like
position, display, alignItems, width, top, bottom, right, borderLeft,
pointerEvents, etc., so they match the unquoted style used in colWrapper,
leaving only selector strings (e.g., '& input' and
'&::-webkit-outer-spin-button, &::-webkit-inner-spin-button') quoted; update the
numberWrapper and numberStepper objects to remove the unnecessary string quotes
while keeping dynamic values (like the fontColor expression) intact.
In `@admin-ui/app/routes/Apps/Gluu/types/GluuInputEditor.types.ts`:
- Around line 3-9: Remove the unstable internal `$lines` field from the
GluuInputEditorCursorValue type so callers must use the stable API method:
update the `document` shape inside the `GluuInputEditorCursorValue` type by
deleting `$lines: string[]` and keeping only `getAllLines(): string[]`, ensuring
all references to `document.$lines` in code are replaced to call
`document.getAllLines()` instead; locate the type `GluuInputEditorCursorValue`
and any usages that access `document.$lines` to migrate them to `getAllLines()`
accordingly.
In `@admin-ui/app/routes/Apps/Gluu/types/GluuInputRow.types.ts`:
- Around line 17-24: The file uses React.ChangeEvent, React.ReactNode and
React.FocusEvent in the GluuInputRow types but React isn't imported (tsconfig
"types": null), so add a type-only import and switch to the imported names: add
"import type { ChangeEvent, FocusEvent, ReactNode } from 'react'" at the top,
then update the signatures that reference React.ChangeEvent, React.ReactNode,
and React.FocusEvent (e.g. handleChange, shortcode, onFocus) to use
ChangeEvent<HTMLInputElement>, ReactNode, and FocusEvent<HTMLInputElement>
respectively.
In `@admin-ui/plugins/admin/components/Mapping/hooks/useMappingApi.ts`:
- Around line 64-70: The error cast has been corrected for extractErrorMessage
but callbacksRef.current?.onError still narrows to new Error(String(error));
update the onError call in useMappingApi.ts so it uses the same union passed to
extractErrorMessage (Error | ApiError | Record<string, never>) and surface the
full extracted message when constructing an Error to pass to
callbacksRef.current?.onError (i.e., call extractErrorMessage(...) and wrap its
result in new Error(...) if you must pass an Error instance), ensuring
extractErrorMessage and callbacksRef.current?.onError use consistent handling of
the error union.
In `@admin-ui/plugins/admin/components/Webhook/styles/ShortcodePopover.style.ts`:
- Around line 46-48: The divider style has been updated to use
themeColors.borderColor (see divider.borderColor), which resolves the prior
issue; no code changes are needed—keep divider: { borderColor:
themeColors.borderColor } as-is and mark the change approved/merge-ready.
In `@admin-ui/plugins/admin/components/Webhook/styles/WebhookFormPage.style.ts`:
- Around line 43-50: The theme's infoAlert fields are required but the code uses
optional chaining; change the expressions that set infoBg, infoBorder, and
infoText to access required properties directly (e.g., replace
themeColors.infoAlert?.background with themeColors.infoAlert.background, and
similarly for themeColors.infoAlert.border and themeColors.infoAlert.text) while
keeping the fallback to customColors.cedarInfoBgLight / cedarInfoBorderLight /
cedarInfoTextLight; leave the other assignments (cardBg, formInputBg,
inputBorderColor, headersBoxBg, headersInputBg) unchanged.
In `@admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts`:
- Around line 8-11: The type MutationCallbacks is duplicated (also present in
MappingTypes.ts); extract it into a shared types module (e.g., create a new file
exporting MutationCallbacks) and update both WebhookTypes.ts and MappingTypes.ts
to import MutationCallbacks from that new module; ensure the exported type
signature remains identical (onSuccess?: () => void, onError?: (error: Error) =>
void) and update any consumers/imports referencing MutationCallbacks
accordingly.
In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx`:
- Around line 353-360: The effect with headersBodyRef in useEffect mutates DOM
styles directly (querySelectorAll + style.setProperty(..., 'important')) which
bypasses React and is brittle; instead remove this direct DOM mutation and pass
the background color via props on the rendered Input components (use
headerInputBg from state/props) so each <Input> sets style={{ backgroundColor:
headerInputBg }} (or className driven by that value) and ensure the map that
renders http header inputs (driven by formikValues.httpHeaders) uses that prop
so newly added inputs receive the style without imperative DOM queries.
- Around line 389-403: The inline formik object passed to GluuSelectRow
(formik={{ handleChange: handleFeatureChange }}) is recreated every render;
memoize it so the prop identity is stable — create a memoized object (e.g.,
featureFormik) using useMemo with handleFeatureChange in its dependency array
and pass featureFormik to GluuSelectRow; ensure handleFeatureChange itself is
stable (wrap in useCallback if necessary) so GluuSelectRow does not receive a
new formik reference each render.
- Around line 516-549: The list uses key={index} causing reconciliation bugs
when items are added/removed; modify addHeader to attach a stable unique id
(e.g., crypto.randomUUID() or an incrementing counter) to each header object
(ensure existing formikValues.httpHeaders without ids are seeded with ids on
load), change the mapped element key to use header.id, and update
removeHeader/changeHeader to operate safely using the header's id or index as
appropriate so keys remain stable across reorders/removals; reference the
addHeader, removeHeader, changeHeader functions and formikValues.httpHeaders
when making these changes.
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Around line 30-32: The toWebhookEntries function unsafely casts
PagedResultEntriesItem[] to WebhookEntry[]; replace the blanket "as unknown as
WebhookEntry[]" with an explicit transformation or type-guard: when mapping
entries (in toWebhookEntries), handle undefined by returning [] and convert each
PagedResultEntriesItem into a WebhookEntry by explicitly selecting/mapping the
required fields (or run a validation/typeGuard function for each item) so
structure mismatches are caught at runtime/compile time rather than erased by a
cast; update or add a helper (e.g., isWebhookEntry or
mapPagedItemToWebhookEntry) to centralize the mapping/validation used by
toWebhookEntries.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 66
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
admin-ui/plugins/admin/__tests__/components/Roles/roles.test.ts (2)
8-24:⚠️ Potential issue | 🟡 Minor
api-adminandapi-viewerdescriptions appear to be swapped.
api-adminis described as "Not possible for this role to perform edition nor deletion" — this is read-only semantics, matching a viewer.api-vieweris described as "allows a user to perform all possible actions on api objects" — this is full-access semantics, matching an admin.The descriptions should match the role names.
🐛 Proposed fix
{ role: 'api-admin', - description: - 'This role allows a user to access all list and search features available. Not possible for this role to perform edition nor deletion', + description: 'This role allows a user to perform all possible actions on api objects', scopes: [ 'https://jans.io/oauth/config/attributes.readonly', 'https://jans.io/oauth/config/acrs.readonly', ], }, { role: 'api-viewer', - description: 'This role allows a user to perform all possible actions on api objects', + description: + 'This role allows a user to access all list and search features available. Not possible for this role to perform edition nor deletion', scopes: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/__tests__/components/Roles/roles.test.ts` around lines 8 - 24, The two role description strings for the objects with role: 'api-admin' and role: 'api-viewer' are inverted; update the description for the 'api-admin' object to the full-access text ("This role allows a user to perform all possible actions on api objects") and change the 'api-viewer' object's description to the read-only text ("This role allows a user to access all list and search features available. Not possible for this role to perform edition nor deletion"), leaving scopes and role keys unchanged.
1-45: 🧹 Nitpick | 🔵 TrivialRename this file to
roles.fixture.tsorroles.mock.ts.This file contains only fixture data, not test cases. Jest will collect it as a test file due to the
__tests__/location and.test.tsnaming, resulting in an empty test suite. Since no test files import from it and the data is self-contained, renaming it clearly reflects its actual purpose and removes it from test collection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/__tests__/components/Roles/roles.test.ts` around lines 1 - 45, This file defines test fixture data (Role interface and exported const roles/default export) but is named and located like a test; rename the file from roles.test.ts to roles.fixture.ts or roles.mock.ts to stop Jest collecting it as an empty test file, and update any imports that reference this module (if any) to the new filename while keeping the exported symbols (Role, roles, default export) unchanged.admin-ui/app/utils/hooks/useWebhookDialogAction.tsx (2)
80-82:⚠️ Potential issue | 🟠 MajorIncorrect
useEffectdependency array — modal state goes stale whenjansEnabledtoggles without changing array lengthuseEffect(() => { dispatch(setWebhookModal(enabledFeatureWebhooks?.length > 0)) }, [featureWebhooks?.length]) // ← only length; misses jansEnabled changesTwo bugs:
- Wrong dep: the effect reads
enabledFeatureWebhooks(filtered byjansEnabled) but depends onfeatureWebhooks?.length. If an item is enabled/disabled without adding/removing a webhook, the effect never re-fires andwebhookModalstays stale.- Missing
dispatch:react-hooks/exhaustive-depsrequires it (even thoughdispatchis stable, omitting it silences the lint rule and can mask future regressions).🐛 Proposed fix
-}, [featureWebhooks?.length]) +}, [enabledFeatureWebhooks, dispatch])This requires
enabledFeatureWebhooksto be memoized (see the comment on line 48 above) so the dep comparison is stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/utils/hooks/useWebhookDialogAction.tsx` around lines 80 - 82, The useEffect currently reads enabledFeatureWebhooks but only depends on featureWebhooks?.length, so toggling jansEnabled (which changes enabledFeatureWebhooks) won’t re-run and the modal state goes stale; also dispatch is missing from the deps. Fix by memoizing enabledFeatureWebhooks (so its identity is stable) and update the useEffect dependencies to include enabledFeatureWebhooks (or its length) and dispatch, then call dispatch(setWebhookModal(enabledFeatureWebhooks?.length > 0)) inside that effect; reference: useEffect, enabledFeatureWebhooks, featureWebhooks, jansEnabled, dispatch, and setWebhookModal.
126-126:⚠️ Potential issue | 🟡 Minor
<Box flex flexDirection="column" …>won't render as a flex container in MUI v5The
flexprop onBoxmaps to the CSSflexshorthand property (which controls flex sizing), notdisplay: flex. Using a bareflexboolean is invalid CSS and gets ignored by the browser. TheflexDirection="column"has no effect withoutdisplay: flexto establish the flex container.Fix
-<Box flex flexDirection="column" px={2}> +<Box display="flex" flexDirection="column" px={2}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/utils/hooks/useWebhookDialogAction.tsx` at line 126, The Box in useWebhookDialogAction.tsx is using an invalid boolean "flex" prop so flexDirection has no effect; update the Box to be a flex container by replacing the boolean "flex" prop with a display: 'flex' (e.g., display="flex" or sx={{ display: 'flex' }}) and keep flexDirection="column" (remove or change any incorrect use of the "flex" shorthand if present) so the layout behaves as intended.admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
91-115:⚠️ Potential issue | 🟡 MinorDual server+client sort model may produce confusing results.
Server sort (
sortBy/sortOrder) controls the API query ordering, whileclientSortre-sorts the already-paginated results. When the user changesclientSortvia column headers, only the current page's rows are reordered — rows on other pages remain in server-sort order. This could lead to surprising behavior where a user clicks a column header expecting a full sort but only sees the current page rearranged.Consider either:
- Making column-header clicks also drive server-side sorting (and resetting to page 0), or
- Adding a visual indicator that column sorting is page-local only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx` around lines 91 - 115, The current mix of server-side sorting (useGetAllWebhooks with serverSort) and client-side reordering (clientSort + sortRows) causes only the current page to be resorted; change column-header interactions so they drive server-side sorting instead: when clientSort is updated, set serverSort.column and serverSort.desc accordingly, reset pageNumber to 0, and call refetch so that useGetAllWebhooks returns globally-sorted results (remove or disable the client-side re-sort in the webhooks useMemo when server-driven sorting is active). Target symbols: useGetAllWebhooks, serverSort, clientSort, sortRows, pageNumber, refetch, and the webhooks useMemo.admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (1)
320-327: 🛠️ Refactor suggestion | 🟠 MajorInconsistent body-method check: inline condition vs.
hasHttpBody.Line 320 uses
formikValues.httpMethod !== 'GET' && formikValues.httpMethod !== 'DELETE'to decide whether to parse the request body, while the rest of the file (lines 162, 166, 283) uses the centralizedhasHttpBody()helper. If the set of body-capable methods ever changes (e.g., addingOPTIONS), this spot would be missed.Proposed fix
- if (formikValues.httpMethod !== 'GET' && formikValues.httpMethod !== 'DELETE') { + if (formikValues.httpMethod && hasHttpBody(formikValues.httpMethod)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx` around lines 320 - 327, Replace the inline method check in the payload parsing block with the centralized hasHttpBody helper to keep body-capable logic consistent: instead of testing formikValues.httpMethod !== 'GET' && formikValues.httpMethod !== 'DELETE', call hasHttpBody(formikValues.httpMethod) (or the existing hasHttpBody helper used elsewhere) before attempting JSON.parse of formikValues.httpRequestBody; on parse failure continue to call setFieldError('httpRequestBody', t('messages.invalid_json_error')) and return as before so error handling remains unchanged.admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx (1)
26-45:⚠️ Potential issue | 🔴 CriticalCritical: React Rules of Hooks violation — hooks called after conditional early return
useTheme()(line 31), threeuseMemocalls (lines 34–39), anduseEffect(lines 41–45) are all invoked after the early return at line 26. React requires all hooks to be called unconditionally and in the same order on every render. WhenwebhookStatetoggles between falsy and truthy, React will throw"Rendered more/fewer hooks than expected".All hooks must be moved above the early return guard:
🐛 Proposed fix
const GluuWebhookErrorDialog = () => { const { t } = useTranslation() const dispatch = useDispatch() const webhookState = useAppSelector((state) => state.webhookReducer) const { hasCedarReadPermission, authorizeHelper } = useCedarling() + const { state: themeState } = useTheme() + const selectedTheme = themeState.theme + + const webhookResourceId = useMemo(() => ADMIN_UI_RESOURCES.Webhooks, []) + const webhookScopes = useMemo(() => CEDAR_RESOURCE_SCOPES[webhookResourceId], [webhookResourceId]) + const canReadWebhooks = useMemo( + () => hasCedarReadPermission(webhookResourceId), + [hasCedarReadPermission, webhookResourceId], + ) + + useEffect(() => { + if (webhookScopes && webhookScopes.length > 0) { + authorizeHelper(webhookScopes) + } + }, [authorizeHelper, webhookScopes]) if (!webhookState) return null const { triggerWebhookMessage, webhookTriggerErrors, triggerWebhookInProgress, showErrorModal } = webhookState - const { state: themeState } = useTheme() - const selectedTheme = themeState.theme - - const webhookResourceId = useMemo(() => ADMIN_UI_RESOURCES.Webhooks, []) - const webhookScopes = useMemo(() => CEDAR_RESOURCE_SCOPES[webhookResourceId], [webhookResourceId]) - const canReadWebhooks = useMemo( - () => hasCedarReadPermission(webhookResourceId), - [hasCedarReadPermission, webhookResourceId], - ) - - useEffect(() => { - if (webhookScopes && webhookScopes.length > 0) { - authorizeHelper(webhookScopes) - } - }, [authorizeHelper, webhookScopes])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx` around lines 26 - 45, Move all React hooks so they run unconditionally before the early return guard that checks webhookState; specifically relocate useTheme(), the three useMemo hooks that compute webhookResourceId (ADMIN_UI_RESOURCES.Webhooks), webhookScopes (CEDAR_RESOURCE_SCOPES[webhookResourceId]) and canReadWebhooks (hasCedarReadPermission(webhookResourceId)), and the useEffect that calls authorizeHelper(webhookScopes) to above the "if (!webhookState) return null" line so hooks are always invoked in the same order regardless of webhookState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/__mocks__/`@janssenproject/cedarling_wasm.ts:
- Around line 1-3: The mockCedarling.authorize default always resolves to {
decision: 'Allow' }, which prevents easy negative-scenario tests; add a brief
comment above the mockCedarling declaration explaining that tests can override
the default per-test using Jest's mockResolvedValueOnce (e.g.
(mockCedarling.authorize as jest.Mock).mockResolvedValueOnce({ decision: 'Deny'
})) or by mocking mockCedarling.authorize directly, and note that the current
jest.fn().mockResolvedValue sets a global default for other tests.
In `@admin-ui/__tests__/setup.ts`:
- Around line 22-23: The global test suppression that checks msg.includes('_t is
not a function') and msg.includes('was not wrapped in act') in
admin-ui/__tests__/setup.ts should be removed or narrowed: locate the
error/console override where these msg.includes checks are applied and either
delete those two clauses so real i18n and React act errors surface, or change
the logic to only suppress these messages for specific test files/components
(e.g., by checking a test filename or a passed-in flag); ensure related fixes
are implemented (bind the i18n translation function in the i18n setup code and
wrap async state updates in act in the affected component tests) rather than
globally silencing the errors.
- Around line 34-36: The current mock for URL.createObjectURL uses an
unnecessary and fragile double-indirection cast through globalThis.window;
replace that with a direct mock on the global URL object (e.g., set
globalThis.URL.createObjectURL = jest.fn() or global.URL.createObjectURL =
jest.fn()) so the test reliably stubs createObjectURL (referencing
createObjectURL and globalThis.window in the existing code) without relying on
window being defined.
- Around line 14-15: Add an afterAll teardown that calls jest.restoreAllMocks()
to undo the top-level spies created with jest.spyOn(global.console, 'log') and
jest.spyOn(global.console, 'warn'); specifically, add an afterAll(() =>
jest.restoreAllMocks()) so console.log/console.warn (and console.error if also
spied) are restored after tests and the global console behavior is not
permanently changed.
In `@admin-ui/app/index.tsx`:
- Around line 15-18: The spread of shared defaults is currently last so keys in
queryDefaults.queryOptions can override the explicit per-client settings; change
the object so ...queryDefaults.queryOptions is spread first and the explicit
retry and retryDelay fields come after (the object around retry/retryDelay and
...queryDefaults.queryOptions in admin-ui/app/index.tsx) so the client-specific
retry/exponential-backoff values always override the shared defaults.
In `@admin-ui/app/locales/fr/translation.json`:
- Around line 811-813: Update the French validation messages in translation.json
to use sentence-case: change "Méthode" to "méthode" in the "http_method_error"
string and "Nom" to "nom" in the "display_name_error" string (keep "URL" as-is);
locate these keys ("http_method_error", "display_name_error", "url_error") and
replace the capitalized words to match the surrounding sentence-case style.
In `@admin-ui/app/redux/api/HealthApi.ts`:
- Around line 3-6: The HealthData interface is currently unexported but used as
the public return type of getHealthStatus; export it so callers (sagas,
components) can reference it directly. Change the declaration of HealthData to
an exported interface (export interface HealthData { status?: string;
db_status?: string }) and update any local references/imports where consumers
should import HealthData alongside the getHealthStatus API.
In `@admin-ui/app/redux/api/HealthCheckApi.ts`:
- Around line 3-16: Export the two interfaces used in the public API so callers
can reference them directly: add exports for ServiceStatusInput and
ServiceStatusResponse; update their declarations (ServiceStatusInput and
ServiceStatusResponse) to be exported and ensure the HealthCheckApiInstance and
its method getServiceStatus keep the same signatures so external callers can
type inputs and responses without using Parameters/ReturnType workarounds.
In `@admin-ui/app/redux/sagas/HealthSaga.ts`:
- Around line 65-67: The catch block in the getHealthServerStatus saga swallows
errors; change it to catch the error (e.g., catch (err)) and emit a structured
warning before dispatching the fallback response — for example call console.warn
with a clear message and the error object — while keeping the existing yield
put(getHealthServerStatusResponse(null)) behavior so failures are visible during
development/monitoring; update the catch in the generator function
getHealthServerStatus accordingly.
In `@admin-ui/app/redux/types/index.ts`:
- Around line 202-215: StoredTriggerPayload.payload currently uses the broad
"object" type; replace that branch with the canonical JsonValue union used
elsewhere (import JsonValue from admin-ui/app/routes/Apps/Gluu/types/common.ts)
so StoredTriggerPayload.payload becomes: string | number | boolean | null |
JsonValue (or include JsonValue directly in the union) to match
TriggerWebhookSagaPayload.createdFeatureValue; also remove or consolidate the
duplicate WebhookTriggerResponseItem by re-exporting the type from
admin-ui/plugins/admin/redux/api/WebhookApi.ts (or replace the local declaration
with an import/re-export) to avoid maintaining two identical definitions.
In `@admin-ui/app/routes/Apps/Gluu/GluuInputRow.tsx`:
- Around line 60-80: The stepDown callback can produce a negative result when
value is fractional (e.g., 0.5), so update stepDown (which uses numValue,
formik, handleChange, name) to avoid any decrement that would go below zero by
adding a guard like checking if current <= 0 || current - 1 < 0 and return early
(or compute next as Math.max(0, current - 1) before calling
formik.setFieldValue/handleChange) to ensure no transient negative value is
emitted.
- Line 99: The onKeyDown handler in GluuInputRow.tsx currently blocks only the
lowercase 'e'; update the onKeyDown prop on the input (the onKeyDown handler in
the GluuInputRow component) to block both 'e' and 'E' for number inputs—either
by checking evt.key.toLowerCase() === 'e' or testing /[eE]/—so
scientific-notation entry via uppercase 'E' is also prevented when type ===
'number'.
In `@admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx`:
- Line 18: The local duplicate interface WebhookTriggerResponseItem should be
removed from the app redux types and replaced with an import that uses the
canonical definition re-exported by the saga types; specifically, delete the
local interface declaration for WebhookTriggerResponseItem in
app/redux/types/index.ts and add an import like "import type {
WebhookTriggerResponseItem } from 'Plugins/admin/redux/sagas/types/webhook'" and
ensure any exports/usage in that file reference the imported symbol instead of
the local definition.
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuInputRow.style.ts`:
- Line 58: Replace the fragile `${fontColor}40` opacity hack with MUI's alpha()
utility: import alpha from '@mui/material/styles' (or use theme.palette.mode's
alpha helper) and change the 'borderLeft' expression (and the similar occurrence
at the other site around line 84) to use alpha(fontColor, 0.25) (falling back to
theme.palette.divider when fontColor is falsy) so the border color works for
hex, rgb, hsl or CSS variables; ensure you reference the same fontColor variable
and preserve the fallback to theme.palette.divider.
In `@admin-ui/app/routes/Dashboards/__tests__/DashboardPage.test.tsx`:
- Around line 88-93: Add at least two more tests to DashboardPage.test.tsx: one
that renders DashboardPage with the relevant mocked hooks set to a loading state
and asserts loading indicators (e.g., spinner or "Loading" text) are shown, and
another that mocks the permission toggle (hasCedarReadPermission=false) and
asserts the restricted UI or permission-denied message is rendered; reuse the
existing render(<DashboardPage />, { wrapper: Wrapper }) pattern and the same
test utilities (screen, render) and adjust the mock implementations for the
hooks used by DashboardPage (the mocks already present in the test file) to
return loading/error/no-permission states before calling render, then
restore/reset mocks between tests.
- Line 78: Remove the unused mock reducer named noReducer from the mock store
configuration: locate the mock store setup in DashboardPage.test.tsx where
noReducer is defined as noReducer: (state = {}) => state and delete that entry
so the store only contains real slices used by selectors/components; ensure any
references to noReducer are also removed (there should be none) and the mock
store still constructs correctly after the entry is deleted.
- Around line 8-24: Remove the redundant ADMIN_UI_RESOURCES and
CEDAR_RESOURCE_SCOPES exports from the top-level jest.mock of '@/cedarling' in
DashboardPage.test.tsx so only useCedarling is exported there; keep the separate
mocks for '@/cedarling/utility' (ADMIN_UI_RESOURCES) and
'@/cedarling/constants/resourceScopes' (CEDAR_RESOURCE_SCOPES) intact, i.e.,
edit the jest.mock('@/cedarling', ...) to return just useCedarling (and its
mocked methods hasCedarReadPermission, hasCedarWritePermission, authorizeHelper)
and delete the duplicate ADMIN_UI_RESOURCES and CEDAR_RESOURCE_SCOPES keys.
In `@admin-ui/app/utils/hooks/useWebhookDialogAction.tsx`:
- Line 144: Replace the unstable composite key for the table row with the
canonical unique identifier: locate the JSX that sets the row key (currently
key={`${item.displayName}-${item.url}`} in useWebhookDialogAction.tsx) and
change it to use the server-assigned unique id (item.inum) so the row key
becomes key={item.inum}; ensure you reference the same `item` object used to
render the row and that `item.inum` is present for all rows.
- Line 48: The local array enabledFeatureWebhooks is recreated every render
which invalidates the useCallback for onCloseModal; wrap the filter in a useMemo
(e.g. const enabledFeatureWebhooks = useMemo(() => featureWebhooks.filter(item
=> item.jansEnabled), [featureWebhooks])) inside the useWebhookDialogAction hook
and keep enabledFeatureWebhooks in onCloseModal's dependency array so the
callback remains stable unless featureWebhooks actually changes.
- Line 2: The file imports the untyped useDispatch but uses typed selectors;
replace useDispatch with the app-typed hook to maintain dispatch type safety:
change the import of useDispatch to import useAppDispatch from '@/redux/hooks'
and update where useDispatch() is called (e.g., in the hook's body) to use
useAppDispatch() instead, ensuring the hook signature and dispatched actions
remain unchanged (references: useDispatch, useAppDispatch, useAppSelector).
In `@admin-ui/jest.config.ts`:
- Around line 6-10: Update babel.config.js so the test environment's presets
include '@babel/preset-typescript'; specifically, in the env.test.presets array
(the block labeled env: { test: { presets: [...] } }) add
'@babel/preset-typescript' alongside '@babel/preset-react', '@babel/preset-env',
and 'jest' so TypeScript is compiled when NODE_ENV=test.
In `@admin-ui/plugins/admin/__tests__/api/CustomScripts.test.ts`:
- Around line 60-66: The beforeAll is currently catching errors from
beforeAllAsync(formInitState) and only logging them, which lets the suite
continue with a bad initialState; update the beforeAll handler to rethrow the
caught error (or call the test runner failure helper like done.fail) after
logging so any rejection from beforeAllAsync(formInitState) fails the test
suite—locate the beforeAll wrapper around beforeAllAsync and change the catch
block to rethrow the error (or invoke done.fail) instead of merely calling
log().
- Line 39: Remove the unconventional import "import { log } from 'console'" and
replace its three call-sites (the log(...) usages in the CustomScripts.test.ts
tests) with either console.warn(...) or console.error(...); alternatively, if
the guard-clause bug is fixed, delete those debug calls entirely so tests use
the global console methods instead of the imported log.
- Around line 135-163: The tests "should update newly created script" and
"should delete newly created script" currently guard with `if (!createdScript) {
log(...); return }` which silently passes; replace those guard clauses by
explicitly asserting the precondition (e.g.
`expect(createdScript).not.toBeNull()` or `expect(createdScript).toBeDefined()`)
at the start of each test (tests referencing createdScript, editScript saga and
deleteScript saga) so the test fails when the setup didn't create the script
rather than returning early; ensure you reference the same `createdScript`
variable used in these tests and keep the subsequent code unchanged.
- Around line 129-132: The test is unsafely casting result.returnValue to
CreatedScript even though the mock only supplies { inum, name }; update the test
to either (a) use a narrower type like const createdScript = result.returnValue
as Pick<CreatedScript, 'inum'> or an explicit inline shape { inum: string }, or
(b) perform a runtime check (e.g., assert typeof result.returnValue === 'object'
&& 'inum' in result.returnValue) before accessing .inum, or (c) change the
addCustomScript mock to return the full CreatedScript/CustomScriptItem shape;
target the usage around result.returnValue, createdScript and the
addCustomScript mock.
In
`@admin-ui/plugins/admin/__tests__/components/Cedarling/CedarlingConfigPage.test.tsx`:
- Around line 40-57: The tests create shared instances at module scope
(queryClient, store, Wrapper) which causes cache/state bleed between tests; move
creation of queryClient and store into a beforeEach (or alternatively add an
afterEach that calls queryClient.clear() and resets the Redux store) so each
test gets a fresh QueryClient and Redux store; update Wrapper to reference the
per-test queryClient/store (created inside beforeEach) instead of the
module-level ones to ensure test isolation for queries and dispatched actions.
- Around line 62-63: Replace the broad screen.findAllByText usage
(policyStoreElements) with a more specific query that targets the exact element
expected on successful render—e.g., use screen.findByRole('heading', { name:
/Policy Store/i }) if it's a heading, or screen.findByLabelText /
screen.findByText with the exact label/text for the form field; then assert the
returned element is in the document instead of checking .length > 0. This
removes the redundant existence guard and pins the test to the correct node
(replace references to policyStoreElements and screen.findAllByText
accordingly).
In
`@admin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptAddPage.test.tsx`:
- Line 6: The test imports `item` incorrectly as a default; change the import in
CustomScriptAddPage.test.tsx to use the named export from item.test (e.g. import
{ item } from './item.test') so the preloaded customScriptReducer state gets a
real object instead of undefined in its `items` array, ensuring the `items`
value used by the test (the `item` variable and the reducer's `items` state) is
valid.
- Around line 55-57: The shared QueryClient instance (queryClient) at module
scope can leak cache between tests; fix by resetting it between tests — either
move the QueryClient instantiation into a beforeEach so each test gets a fresh
instance (create QueryClient(...) in beforeEach) or add an afterEach that calls
queryClient.clear() / queryClient.remove() (or otherwise reset the client) to
ensure no cached queries persist; update references to the queryClient variable
accordingly so each test runs with a clean QueryClient.
In
`@admin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptDetailPage.test.tsx`:
- Line 3: The test imports `item` as a default but `item.test.ts` only exports
it as a named export, so `item` will be undefined at runtime; update the import
in CustomScriptDetailPage.test.tsx to use a named import (the same fix applied
in CustomScriptForm.test.tsx) so the `item` object used in assertions
(properties like `item.name` and `item.scriptType`) is the actual exported
symbol from `item.test.ts`.
In
`@admin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptEditPage.test.tsx`:
- Around line 94-96: The QueryClient instance is declared at module scope as
queryClient and its cache can leak between tests; add an afterEach hook that
calls queryClient.clear() (e.g., afterEach(() => queryClient.clear())) to ensure
cached queries are cleared between test cases so tests remain isolated; update
the test file to include this cleanup near the other test lifecycle hooks
referencing the module-scope queryClient.
- Around line 55-63: Extract the duplicated cedarling mocks, JansConfigApi
mocks, Redux store factory, and Wrapper into a shared test setup module (e.g.,
customScriptsTestSetup.ts) and co-locate the jest.mock(...) calls there so all
CustomScript tests use the same mocks; specifically move the mock
implementations for useCustomScriptTypes and useMutationEffects and the store
factory that creates the ADMIN_UI_RESOURCES-shaped state into that file, export
the Wrapper component and a createStoreFactory function, then update
CustomScriptAddPage.test.tsx and CustomScriptEditPage.test.tsx to import the
shared Wrapper and store factory rather than redefining them to eliminate the
ADMIN_UI_RESOURCES discrepancy and keep mocks in one place.
- Line 5: The test imports the fixture incorrectly so `item` is undefined;
change the import from a default to the named export that the fixture module
provides (use `{ item }` from './item.test') so `items: [item]` and `item: item`
populate Redux correctly; ensure the `CustomScriptEditPage.test.tsx` references
the named `item` and that `useCustomScript` mock remains unchanged.
In
`@admin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptForm.test.tsx`:
- Line 5: The test imports `item` incorrectly as a default; change the import in
CustomScriptForm.test.tsx from a default import to a named import to match the
export in item.test.ts (the module exports `export const item = ...`), so
replace the current `import item from './item.test'` with a named import `import
{ item } from './item.test'` and ensure the `item` passed to `CustomScriptForm`
(and any references to `item.inum`) now reference the defined `item` object.
- Around line 17-23: The mocked ADMIN_UI_RESOURCES and CEDAR_RESOURCE_SCOPES in
the test are missing the CustomScripts entry, causing permission checks in
CustomScriptForm to read undefined; update the mocks in
CustomScriptForm.test.tsx so ADMIN_UI_RESOURCES includes CustomScripts:
'customscripts' and CEDAR_RESOURCE_SCOPES includes a matching customscripts
array, ensuring permission lookups in CustomScriptForm (and related
CustomScriptAddPage/CustomScriptEditPage tests) behave correctly.
In `@admin-ui/plugins/admin/__tests__/components/Health/HealthPage.test.tsx`:
- Around line 11-13: The tests tightly couple to HealthPage's rendered text
formatting (kebab names like 'jans-auth' expected to render as 'Jans Auth' and
statuses as 'Active'), so either extract the name/status formatting into a pure
util (e.g., create and export a formatServiceName/formatStatus function used by
HealthPage) and add unit tests for those functions, or modify the HealthPage
tests to assert on stable attributes instead of rendered text by adding
data-testid attributes (e.g., data-testid={`service-name-${id}`} and
data-testid={`service-status-${id}`}) in the HealthPage render and querying
those in HealthPage.test.tsx; update mocks to keep service identifiers (like
'jans-auth' / 'jans-config-api') and change assertions to use the util tests or
the data-testid queries to avoid coupling to display formatting.
In
`@admin-ui/plugins/admin/__tests__/components/Mapping/RolePermissionMappingPage.test.tsx`:
- Around line 46-50: The test uses screen.getAllByText(/Cedarling/i) which
already throws if no matches exist, so the subsequent
expect(...length).toBeGreaterThan(0) is redundant; update the test in
RolePermissionMappingPage.test.tsx to either use screen.getByText(/Cedarling/i)
and assert its presence (e.g., expect(getByText(...)).toBeInTheDocument()) or
use screen.queryAllByText(/Cedarling/i) and keep the length assertion, ensuring
you modify the call referenced in the it('shows the info description text', ...)
test that renders RolePermissionMappingPage.
In `@admin-ui/plugins/admin/__tests__/components/MAU/MauPage.test.tsx`:
- Around line 42-46: Add two additional tests for MauPage to cover the Alert
branches: one that simulates an error state (set the data-fetching hook or prop
used by MauPage to return isError=true) and asserts the error Alert renders, and
another that simulates a successful fetch with empty data and asserts the
empty-data Alert renders; locate the component under test (MauPage) and the
wrapper used in existing test (Wrapper), mock the relevant data-fetching hook or
provider used by MauPage so you can return isError=true and an empty dataset
respectively, render with render(<MauPage />, { wrapper: Wrapper }), and assert
the corresponding Alert text appears to ensure both branches are exercised.
In `@admin-ui/plugins/admin/__tests__/components/Roles/roles.test.ts`:
- Line 7: The file currently exports the same binding twice — as a named export
"roles: Role[]" and again as a default export; remove the default export to keep
a single canonical named export. Delete the default export statement and update
any imports in tests or consumers to use the named import { roles } (and keep
the existing Role type usage), ensuring no other duplicate export remains.
- Around line 1-5: The test file defines a local interface named Role which
collides with the shared Role from user-management's CommonTypes; either import
the shared Role from admin-ui/plugins/user-management/types/CommonTypes.ts and
extend it (e.g. interface TestRole extends Role { description: string; scopes:
string[] }) or rename the local interface to a distinct name such as TestRole or
RoleFixture and update usages in roles.test.ts (search for the local Role
symbol) to avoid the naming collision and keep types DRY.
In `@admin-ui/plugins/admin/__tests__/components/Settings/SettingsPage.test.tsx`:
- Around line 67-71: Add three additional tests for SettingsPage: (1) Loading
state — mock the hook useGetAdminuiConf to return { isLoading: true } / {
isFetching: true }, render <SettingsPage /> with Wrapper and assert a
loader/spinner or loading text is present and the main form fields (e.g., "List
paging size") are absent; (2) Error/empty state — mock useGetAdminuiConf to
return { data: null } or an error and assert the component gracefully renders an
empty/error message and does not render form inputs; (3) Form interaction — mock
useGetAdminuiConf to return existing config with listPagingSize, mock
useEditAdminuiConf.mutateAsync as a jest.fn(), use userEvent to change the
listPagingSize input and submit the form, then assert
useEditAdminuiConf.mutateAsync was called with the updated listPagingSize
payload; reference SettingsPage, useGetAdminuiConf, useEditAdminuiConf, and
listPagingSize to locate the hooks/field to mock and interact with.
- Around line 55-57: The module-scoped queryClient (instance of QueryClient)
causes shared cache between tests; move its creation into a beforeEach (or
create per-test) so each test gets a fresh QueryClient. Specifically, remove the
top-level const queryClient = new QueryClient(...) and instead instantiate a new
QueryClient inside a beforeEach (or inside each test) and use that instance when
rendering/mounting (where the tests currently reference queryClient) to avoid
cache bleed and intermittent failures.
- Around line 20-26: Delete the two redundant jest.mock calls that mock
'@/cedarling/utility' and '@/cedarling/constants/resourceScopes' since
SettingsPage imports ADMIN_UI_RESOURCES and CEDAR_RESOURCE_SCOPES from the
barrel '@/cedarling' and those values are already provided by the existing
barrel mock; remove the mock blocks referencing ADMIN_UI_RESOURCES: { Settings:
'settings' } and CEDAR_RESOURCE_SCOPES: { settings: [] } so only the barrel mock
remains.
- Around line 43-53: The reducer map passed to configureStore in
SettingsPage.test.tsx includes an unnecessary noReducer entry; remove the
noReducer key from the combineReducers call so the reducer object only includes
authReducer (which provides userinfo and config.clientId) and update any test
setup references if present (search for configureStore, combineReducers,
authReducer, and noReducer to locate the code).
In
`@admin-ui/plugins/admin/__tests__/components/Webhook/WebhookListPage.test.tsx`:
- Around line 63-67: Add tests that go beyond mount-only verification: render
WebhookListPage with mock webhook data and assert webhook rows appear (look for
row text or count), simulate different permission sets to assert action buttons
visibility (e.g., edit/delete buttons present or absent), and add a deletion
flow test that clicks the delete button on a row, asserts a confirmation dialog
appears, confirms the deletion, and verifies the row is removed (or a success
callback is called). Use the existing Wrapper to provide necessary context and
mock interfaces used by WebhookListPage (API/stores) so you can control returned
webhooks and permission flags while asserting UI changes.
- Around line 35-40: The mock for useDeleteWebhookWithAudit returns isDeleting
but the consumer in WebhookListPage destructures isLoading (aliased to
isDeleting), so update the jest.mock to return isLoading instead of isDeleting;
specifically change the mock object returned by useDeleteWebhookWithAudit to
include isLoading: false alongside deleteWebhook: jest.fn() so tests accurately
reflect the hook shape used by WebhookListPage.
In `@admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts`:
- Around line 42-44: In useWebhookMutations, the .catch handlers after
logAction(...) currently only call console.error when isDevelopment, which drops
audit failures in production; update those catch blocks (all occurrences around
the logAction calls) to send the auditError to your telemetry/error-tracking
sink (e.g., Sentry.captureException, a metrics counter, or processLogger.error)
while still keeping the console.error in dev, and ensure the telemetry call
includes context (action name, webhook id or payload) so production audit
failures are observable and traceable.
- Around line 17-24: The local ApiError interface here conflicts with the shared
type in agamaTypes.ts and extractErrorMessage duplicates existing logic in
schema/utils/errorHandler.ts; rename the local interface to WebhookApiError and
update its usages (e.g., the type on the extractErrorMessage parameter) to avoid
collision, then either import and reuse the shared extractErrorMessage from
errorHandler.ts or keep a webhook-specific extractor but give it a distinct name
(e.g., extractWebhookErrorMessage) to make the intent explicit; ensure
references to ApiError, extractErrorMessage, and any imports are updated
accordingly.
In `@admin-ui/plugins/admin/components/Webhook/styles/ShortcodePopover.style.ts`:
- Around line 22-26: The inline dimension magic numbers in
ShortcodePopover.style.ts (maxHeight: '300px', minWidth: 200, maxWidth: 260)
should be extracted into named constants mirroring the existing
HELP_ICON_SIZE/HELP_ICON_MARGIN pattern; add constants like MAX_POPOVER_HEIGHT,
MIN_POPOVER_WIDTH, and MAX_POPOVER_WIDTH at the top of the file and replace the
literal values in the style object (where maxHeight, minWidth, maxWidth are set)
to use those constants so sizing is consistent and self-descriptive.
- Around line 12-13: The current use of optional chaining on
themeColors.settings and themeColors.card is misleading and can silently yield
undefined for paperBg/hoverBg; update the expressions to use direct property
access (remove the ?. on settings and card) and keep/adjust the nullish
coalescing fallback so paperBg = themeColors.settings.cardBackground ??
themeColors.card.background and hoverBg = themeColors.settings.customParamsBox
?? themeColors.inputBackground, and if you want extra safety add an explicit
default color string fallback after those expressions to guarantee background
values (reference: themeColors, ThemeConfig, settings, card, paperBg, hoverBg).
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Around line 138-152: The submitForm callback includes an unnecessary
dependency 't' in its dependency array; update the submitForm useCallback
(referencing submitForm, deleteData, deleteWebhook, refetch) to remove 't' so
the dependency array becomes [deleteData, deleteWebhook, refetch] (ensure no
other references to 't' remain in submitForm before changing).
- Around line 166-168: handleSearchSubmit currently only calls setPageNumber(0)
so when the user is already on page 0 a search submit does not trigger a
refetch; update handleSearchSubmit to also call refetch() (or call refetch
conditionally when pageNumber === 0) after setPageNumber(0) so the list is
reloaded on explicit submit, following the same pattern used in handleRefresh;
locate the function handleSearchSubmit and the refetch function used elsewhere
in this file and add the refetch invocation accordingly.
In `@admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts`:
- Around line 76-78: Add JSDoc above normalizeUrlForValidation to document its
limitation: explain that placeholders replaced by REGEX_URL_PLACEHOLDER become
'x' and thus hostname placeholders like "https://${host}/path" will normalize to
"https://x/path" and fail PATTERN/hostname validation because 'x' lacks a dot;
state this is expected/safe behavior and callers that need to allow placeholder
hostnames should avoid full-hostname placeholders or adjust validation
accordingly.
In `@admin-ui/plugins/admin/helper/shortCodes.json`:
- Around line 846-883: The entry with "key": "userId" has mismatched i18n
keys—its label uses "documentation.user.userName" while its description uses
"documentation.user.userId"; decide whether this field should be "userName" or
"userId" and make both the "label" and "description" use the same chosen key
(e.g., change the description to "documentation.user.userName" if it's
username), and apply the same consistent change in the corresponding
"users_delete" entry so label and description keys match there as well.
In `@admin-ui/plugins/admin/helper/utils.ts`:
- Around line 54-58: The httpRequestBody property in interface WebhookWithBody
is typed too broadly (uses object) — change its type to Record<string,
JsonValue> to match the cast performed later and prevent passing functions/class
instances; update the import or add a local JsonValue type if needed (e.g., type
JsonValue = string | number | boolean | null | JsonValue[] | { [k: string]:
JsonValue }) and replace Record<string, string | number | boolean | null |
object> with Record<string, JsonValue> in the WebhookWithBody interface (ensure
any call sites conform to the narrower JsonValue shape).
In `@admin-ui/plugins/admin/helper/validations/webhookValidation.ts`:
- Around line 12-15: The when() callback may pass undefined to hasHttpBody,
violating hasHttpBody(method: string); update the type/usage to accept undefined
and make the when callback type-safe: change hasHttpBody to accept (method?:
string) or add a small wrapper in the when calls like (method?: string) =>
hasHttpBody(method) so the parameter can be undefined safely; apply this same
pattern to the other when() usage on line 26 (both references: hasHttpBody and
the httpRequestBody Yup.string().when(...) call).
In `@admin-ui/plugins/admin/redux/api/WebhookApi.ts`:
- Around line 14-18: The WebhookOutputItem.shortcodeValueMap uses an overly
broad type (Record<string, string | number | boolean | null | object>) that
conflicts with helper/utils.ts which defines shortcodeValueMap as Record<string,
JsonValue>; update WebhookOutputItem to use JsonValue consistently by importing
JsonValue and changing the shortcodeValueMap type to Record<string, JsonValue>
so all consumers (e.g., webhook.ts) share the same recursive JSON-safe type.
- Around line 4-12: The interface WebhookTriggerResponseItem is duplicated;
remove the local declaration inside WebhookApi.ts and instead import the
canonical WebhookTriggerResponseItem from the shared types module (the existing
shared types file where the other definition lives), then use that imported type
in this file and re-export it if other modules expect it from here; specifically
delete the WebhookTriggerResponseItem interface block in this diff, add an
import for WebhookTriggerResponseItem from the shared types module, and update
any exports or references in this file to use the imported type.
In `@admin-ui/plugins/admin/redux/features/mauSlice.ts`:
- Around line 12-19: The mauSlice currently defines reducers: {} so actions is
empty and MauState (initialState) can never change; either remove the entire
mauSlice registration (remove mauSlice, export, and
reducerRegistry.register('mauReducer', reducer)) if the MAU page uses React
Query only, or add explicit reducers and action creators to update each field
(e.g., setStat, setLoading, setStartMonth, setEndMonth or a generic updateMau)
inside createSlice so actions and reducer actually update MauState; ensure you
export the actions and keep reducerRegistry.register('mauReducer', reducer) only
if the reducer is meaningful.
In `@admin-ui/plugins/admin/redux/features/WebhookSlice.ts`:
- Around line 57-60: The assignment in the WebhookSlice sets
state.triggerPayload.payload using a cast to SerializableValue which weakens
types; update the StoredTriggerPayload (or the state.type) so that
TriggerPayload.payload is typed as JsonValue (the recursive JSON union) instead
of flat SerializableValue, or change SerializableValue to a recursive definition
to match JsonValue; locate the assignment in WebhookSlice where
state.triggerPayload is set and modify the payload type declarations
(StoredTriggerPayload / TriggerPayload / SerializableValue) so the payload field
uses JsonValue, then remove the unsafe cast in the state assignment.
- Around line 7-12: StoredTriggerPayload and SerializableValue are duplicated
here; remove the local declarations in WebhookSlice.ts and import the shared
types instead (use the exported SerializableValue and StoredTriggerPayload from
the app/redux/types index), updating any local references in functions or type
annotations (e.g., in WebhookSlice.ts where StoredTriggerPayload or
SerializableValue are used) to rely on the imported types so the project uses
the single source of truth and avoids divergence.
In `@admin-ui/plugins/admin/redux/sagas/types/common.ts`:
- Around line 7-11: The isHttpLikeError predicate currently only checks for
non-null objects which is too broad; update isHttpLikeError to perform
structural checks for an HTTP-like shape by verifying either a numeric top-level
status property or a response object with a numeric status (e.g. check typeof
error === 'object' && error !== null && (( 'status' in error && typeof (error as
any).status === 'number') || ('response' in error && typeof (error as
any).response === 'object' && ( 'status' in (error as any).response && typeof
(error as any).response.status === 'number')))); keep the function name
isHttpLikeError and return type the same so the type predicate is sound.
In `@admin-ui/plugins/admin/redux/sagas/types/webhook.ts`:
- Around line 3-12: The module re-exports two incompatible WebhookOutputItem
types: the one imported as WebhookOutputItem from helper/utils and the one
referenced inside TriggerWebhookApiPayload in WebhookApi.ts; consolidate to a
single canonical definition by moving/choosing one WebhookOutputItem (preferably
the one with the correct shortcodeValueMap shape) and update both helper/utils
and WebhookApi to import that single definition, then update this file to import
and re-export only that canonical WebhookOutputItem and the
TriggerWebhookApiPayload so both use the exact same type.
In `@admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts`:
- Around line 76-81: featureWebhooks can be null/undefined so
enabledFeatureWebhooks may become undefined and accessing .length throws; fix by
ensuring enabledFeatureWebhooks is always an array before using .length — e.g.
coalesce featureWebhooks to [] when filtering (use featureWebhooks ?? [] in the
filter expression) so the const enabledFeatureWebhooks = (featureWebhooks ??
[]).filter((item: WebhookEntry) => item.jansEnabled) and then keep the existing
if check that uses enabledFeatureWebhooks.length and featureToTrigger; this
prevents a TypeError from select((state: RootState) =>
state.webhookReducer.featureWebhooks) returning undefined.
- Around line 124-137: In the catch block handling errors in WebhookSaga, ensure
403 errors are audited before logout: move or duplicate the
addAdditionalData(audit, FETCH, `/webhook/${featureToTrigger || 'trigger'}`,
{...}) and yield call(postUserAction, audit) so they execute before calling
yield* redirectToLogout() when isHttpLikeError(e) && isFourZeroThreeError(e) is
true; keep the existing toast/modal/trigger response behavior and avoid
double-posting the audit for non-403 paths.
- Around line 140-146: Replace the hardcoded action type strings in
watchGetWebhooksByFeatureId and watchGetTriggerWebhook with the .type property
of the corresponding action creators exported from WebhookSlice: import the
action creators (e.g., getWebhooksByFeatureId and triggerWebhook) under distinct
aliases to avoid name collisions with the saga functions (for example
getWebhooksByFeatureIdAction, triggerWebhookAction) and use
getWebhooksByFeatureIdAction.type and triggerWebhookAction.type as the patterns
in takeLatest so the sagas stay in sync with the slice.
---
Outside diff comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx`:
- Around line 26-45: Move all React hooks so they run unconditionally before the
early return guard that checks webhookState; specifically relocate useTheme(),
the three useMemo hooks that compute webhookResourceId
(ADMIN_UI_RESOURCES.Webhooks), webhookScopes
(CEDAR_RESOURCE_SCOPES[webhookResourceId]) and canReadWebhooks
(hasCedarReadPermission(webhookResourceId)), and the useEffect that calls
authorizeHelper(webhookScopes) to above the "if (!webhookState) return null"
line so hooks are always invoked in the same order regardless of webhookState.
In `@admin-ui/app/utils/hooks/useWebhookDialogAction.tsx`:
- Around line 80-82: The useEffect currently reads enabledFeatureWebhooks but
only depends on featureWebhooks?.length, so toggling jansEnabled (which changes
enabledFeatureWebhooks) won’t re-run and the modal state goes stale; also
dispatch is missing from the deps. Fix by memoizing enabledFeatureWebhooks (so
its identity is stable) and update the useEffect dependencies to include
enabledFeatureWebhooks (or its length) and dispatch, then call
dispatch(setWebhookModal(enabledFeatureWebhooks?.length > 0)) inside that
effect; reference: useEffect, enabledFeatureWebhooks, featureWebhooks,
jansEnabled, dispatch, and setWebhookModal.
- Line 126: The Box in useWebhookDialogAction.tsx is using an invalid boolean
"flex" prop so flexDirection has no effect; update the Box to be a flex
container by replacing the boolean "flex" prop with a display: 'flex' (e.g.,
display="flex" or sx={{ display: 'flex' }}) and keep flexDirection="column"
(remove or change any incorrect use of the "flex" shorthand if present) so the
layout behaves as intended.
In `@admin-ui/plugins/admin/__tests__/components/Roles/roles.test.ts`:
- Around line 8-24: The two role description strings for the objects with role:
'api-admin' and role: 'api-viewer' are inverted; update the description for the
'api-admin' object to the full-access text ("This role allows a user to perform
all possible actions on api objects") and change the 'api-viewer' object's
description to the read-only text ("This role allows a user to access all list
and search features available. Not possible for this role to perform edition nor
deletion"), leaving scopes and role keys unchanged.
- Around line 1-45: This file defines test fixture data (Role interface and
exported const roles/default export) but is named and located like a test;
rename the file from roles.test.ts to roles.fixture.ts or roles.mock.ts to stop
Jest collecting it as an empty test file, and update any imports that reference
this module (if any) to the new filename while keeping the exported symbols
(Role, roles, default export) unchanged.
In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx`:
- Around line 320-327: Replace the inline method check in the payload parsing
block with the centralized hasHttpBody helper to keep body-capable logic
consistent: instead of testing formikValues.httpMethod !== 'GET' &&
formikValues.httpMethod !== 'DELETE', call hasHttpBody(formikValues.httpMethod)
(or the existing hasHttpBody helper used elsewhere) before attempting JSON.parse
of formikValues.httpRequestBody; on parse failure continue to call
setFieldError('httpRequestBody', t('messages.invalid_json_error')) and return as
before so error handling remains unchanged.
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Around line 91-115: The current mix of server-side sorting (useGetAllWebhooks
with serverSort) and client-side reordering (clientSort + sortRows) causes only
the current page to be resorted; change column-header interactions so they drive
server-side sorting instead: when clientSort is updated, set serverSort.column
and serverSort.desc accordingly, reset pageNumber to 0, and call refetch so that
useGetAllWebhooks returns globally-sorted results (remove or disable the
client-side re-sort in the webhooks useMemo when server-driven sorting is
active). Target symbols: useGetAllWebhooks, serverSort, clientSort, sortRows,
pageNumber, refetch, and the webhooks useMemo.
---
Duplicate comments:
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuInputRow.style.ts`:
- Around line 35-86: The style objects numberWrapper, numberStepper, and
numberStepperBtn use unnecessary quoted CSS property keys (e.g., 'position',
'display', 'alignItems') — update these three objects to use unquoted property
names (position, display, alignItems, etc.) while preserving quoted selector
keys like '& input' and '&:hover:not(:disabled)'; ensure values and expressions
(e.g., borderLeft using theme.palette.divider and fontColor) remain unchanged.
In `@admin-ui/plugins/admin/components/Webhook/hooks/useGetWebhook.ts`:
- Around line 4-20: In useGetWebhook, the server-side text search parameter
`pattern` is incorrectly set to `webhookId`, which can filter out results before
the client-side selector runs; update the call to useGetAllWebhooks so `pattern`
is always an empty string (e.g., `pattern: ''`) while keeping `enabled:
Boolean(webhookId)` and the `select` function that finds the entry by `inum` (so
useGetWebhook and the select callback still locate the webhook by inum without
the server-side text search interfering).
In `@admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts`:
- Around line 20-28: AuditInit's index signature and the ActionData alias are
too permissive because using "object" allows arrays, functions, and instances;
update the types by changing AuditInit's index signature value type from "string
| { user_inum: string; userId: string } | object" to "string | { user_inum:
string; userId: string } | unknown" (or just "unknown" for the catch-all) and
remove "object" from the ActionData union so ActionData becomes Record<string,
string | number | boolean | null | unknown> (or drop unknown if not needed);
locate and update the declarations for AuditInit and ActionData in
useWebhookAudit.ts accordingly.
In `@admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts`:
- Line 45: The toast messages in createWebhook, updateWebhook, and deleteWebhook
were missing i18n previously; ensure each dispatch(updateToast(...)) call (e.g.,
the calls around dispatch(updateToast(true, 'success',
t('messages.webhook_created_successfully'))) in the createWebhook,
updateWebhook, and deleteWebhook callbacks) uses the translation function t and
that t is included in each useCallback dependency array so React hooks are
correct; no code change beyond confirming those dispatch calls use t(...) and
that t appears in the dependency arrays for the corresponding functions is
required.
In `@admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx`:
- Around line 524-557: The mapped header rows use key={index}, which causes
unstable keys when headers are reordered/removed; update the header objects to
carry a stable unique id (e.g., add id: crypto.randomUUID() when creating or
initializing each entry) and change the map to use key={header.id}; ensure any
addHeader/initialize logic (where formikValues.httpHeaders is populated)
creates/preserves header.id so functions referenced (changeHeader, removeHeader,
the Inputs) operate on items by id rather than relying on array index.
- Around line 363-367: Add an explicit eslint disable comment to document the
intentional omission of `formik` from the dependency array: above the useEffect
inside the WebhookForm component (the effect that checks
Object.keys(formik.errors) and calls formik.validateForm on i18n.language
change), insert `// eslint-disable-next-line react-hooks/exhaustive-deps` so the
linter knows this is deliberate while keeping the dependency array as
`[i18n.language]`.
- Around line 396-410: The inline object passed as the formik prop to
GluuSelectRow (formik={{ handleChange: handleFeatureChange }}) is recreated on
every render causing unnecessary re-renders/effects; fix by creating a stable
reference for that prop (e.g., memoize the object with useMemo or lift it to a
const) and pass that memoized object to GluuSelectRow instead of an inline
literal—refer to GluuSelectRow, handleFeatureChange, and the surrounding usage
where selectedFeatures/adminUiFeatureOptions are provided.
In `@admin-ui/plugins/admin/helper/shortCodes.json`:
- Around line 889-897: The "userId" shortcode entry is inconsistent: its "label"
and "description" currently use documentation.user.userName while the "key" is
"userId" (compare with the "inum" entry), so update the "userId" entry in
shortCodes.json to use documentation.user.userId for both "label" and
"description (or change the key to userName if that was intended); ensure delete
shortcodes follow the same naming convention as the earlier corrected entries so
label/description match the key.
In `@admin-ui/plugins/admin/helper/webhook.ts`:
- Around line 58-59: The WebhookData.httpRequestBody type is too broad and
redundant — replace "JsonValue | object | string" with a narrower type
(preferably just JsonValue, or JsonObject/Record<string, unknown> if you want a
plain object) in the WebhookData interface and update any callers accordingly;
also add a runtime guard before JSON.stringify(webhook.httpRequestBody, ...) to
ensure the value is JSON-serializable (e.g., typeof check or
Array.isArray/typeof === 'object' null-check) to avoid exceptions when non-plain
objects are passed.
admin-ui/plugins/admin/components/Webhook/styles/ShortcodePopover.style.ts
Outdated
Show resolved
Hide resolved
admin-ui/plugins/admin/components/Webhook/styles/ShortcodePopover.style.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
admin-ui/plugins/admin/components/Health/components/ServiceStatusCard.tsx (1)
33-36: 🧹 Nitpick | 🔵 Trivial
useMemodependency on object referenceservicecan cause unnecessary recomputation.Line 35 uses
[service, t]whilegetStatusMessageonly readsservice.statusandservice.error. If the parent re-creates theserviceobject on each render (even with identical fields), the memo is invalidated unnecessarily — inconsistent with howbadgeColorscorrectly depends only onservice.status.♻️ Proposed fix — depend on primitives
- const statusMessage = useMemo( - () => getStatusMessage(service, t).replace(REGEX_TRAILING_PERIOD, ''), - [service, t], - ) + const statusMessage = useMemo( + () => getStatusMessage(service, t).replace(REGEX_TRAILING_PERIOD, ''), + [service.status, service.error, t], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Health/components/ServiceStatusCard.tsx` around lines 33 - 36, The useMemo using the object reference service causes unnecessary recomputation; change the dependency list on the useMemo that defines statusMessage (which calls getStatusMessage and uses REGEX_TRAILING_PERIOD) to use the primitive fields read by that function instead of the whole object — e.g., depend on service.status and service.error along with t (mirroring how badgeColors depends on service.status) so the memo only invalidates when those actual values change.admin-ui/plugins/admin/helper/utils.ts (1)
65-67: 🧹 Nitpick | 🔵 TrivialRedundant intersection type — simplify the cast.
Now that
WebhookWithBody.httpRequestBodyis already typed asRecord<string, JsonValue>(line 57), the& { httpRequestBody?: Record<string, JsonValue> }fragment in the cast is identical to whatWebhookWithBodyalready declares and adds nothing. It was presumably needed before the type was tightened; it can be removed.♻️ Proposed cleanup
- const webhook = cloneDeep(originalWebhook) as WebhookWithBody & { - httpRequestBody?: Record<string, JsonValue> - } + const webhook = cloneDeep(originalWebhook) as WebhookWithBody🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/helper/utils.ts` around lines 65 - 67, The cast on the cloned webhook is redundant: remove the unnecessary intersection "& { httpRequestBody?: Record<string, JsonValue> }" from the line that creates const webhook (which uses cloneDeep(originalWebhook)) and simply cast to WebhookWithBody (or rely on inferred type) since WebhookWithBody already declares httpRequestBody; update the declaration that references cloneDeep and originalWebhook accordingly.admin-ui/app/redux/sagas/LockSaga.ts (1)
17-31:⚠️ Potential issue | 🟡 MinorPre-existing:
auditresult frominitAudit()is never used.The return value of
yield* initAudit()is captured inauditbut never referenced in the rest ofgetLockMau. Other sagas in the same directory pass the audit context into their API calls to record results. If audit logging is expected for lock MAU operations, this is silently broken.🔍 Proposed fix (if audit logging is intended)
export function* getLockMau({ payload }) { - const audit = yield* initAudit() + yield* initAudit() try {Or, if audit logging should be recorded on completion (matching the pattern of other sagas), thread
auditinto the API call result handling instead of suppressing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/redux/sagas/LockSaga.ts` around lines 17 - 31, The audit value returned by initAudit() is captured but never used in getLockMau; thread it through the flow so audit logging is recorded: pass the audit context into the API call (the lockapi.getLockMau invocation) or include it when dispatching the response (getLockStatusResponse) and in the error path, so initAudit(), getLockMau, and getLockStatusResponse all receive/handle the audit info consistently with other sagas; ensure redirectToLogout() path still returns after handling audit.admin-ui/plugins/admin/helper/shortCodes.json (1)
847-898:⚠️ Potential issue | 🟠 Major
labelfields inusers_editandusers_deleteare inconsistent with the rest of the file.All other sections (
custom_script_write,attributes_write,scopes_write,oidc_clients_write, etc.) follow a strict convention:
"label"→"fields.<name>"(concise UI label key)"description"→"documentation.<section>.<name>"(longer help text)In these two blocks, both
labelanddescriptionuse"documentation.user.*"keys. This breaks the pattern and causes labels to display full documentation strings.However, the proposed fix has an issue:
fields.middleNameandfields.userIddo not exist in the i18n files. Onlyfields.givenName,fields.sn,fields.mail,fields.displayName,fields.inum, andfields.statusare defined.For the six existing keys, use:
fields.givenName, fields.sn, fields.mail, fields.displayName, fields.inum, fields.statusFor the two missing keys, either:
- Add
fields.middleNameandfields.userIdto the translation files, or- Use the existing
fields.userName(alternative to userId) and keepdocumentation.user.middleNamefor now🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/helper/shortCodes.json` around lines 847 - 898, In the users_edit and users_delete blocks the "label" values wrongly use documentation.user.* keys; update the "label" entries to use the concise fields.* keys for the existing translations (fields.givenName, fields.sn, fields.mail, fields.displayName, fields.inum, fields.status) and keep the "description" values as documentation.user.*; for the two missing label keys (middleName and userId) either add fields.middleName and fields.userId to the i18n files or (preferred) set the label for userId to fields.userName and leave middleName's label pointing to documentation.user.middleName until a fields.middleName translation is added so labels render correctly without changing other sections.admin-ui/babel.config.js (1)
20-37: 🧹 Nitpick | 🔵 Trivial
@babel/preset-typescriptplacement inverts execution order relative to the top-level config.Babel applies presets in reverse array order (last → first). In
env.test,@babel/preset-typescriptat index 0 runs last, after@babel/preset-reacthas already transformed JSX. In the top-level config,@babel/preset-typescriptis at index 3 (last in array), so it runs first, before React processes JSX. This inversion is problematic: TypeScript should always run before React's JSX transform to correctly handle TypeScript-specific JSX constructs like constrained generic arrow functions (<T,>() => ...).Move
@babel/preset-typescriptafter@babel/preset-reactin the test presets array to match the top-level execution order:Reordered presets
test: { presets: [ - '@babel/preset-typescript', [ '@babel/preset-react', { runtime: 'automatic', }, ], + '@babel/preset-typescript', [ '@babel/preset-env', { targets: { node: 'current', }, }, ], 'jest', ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/babel.config.js` around lines 20 - 37, In the env.test presets array, move '@babel/preset-typescript' to come immediately after the '@babel/preset-react' entry so the presets order matches the top-level config (ensuring TypeScript preset runs before the React JSX transform); update the presets array used in the env.test block rather than the top-level one.admin-ui/plugins/admin/helper/validations/webhookValidation.ts (1)
18-19:⚠️ Potential issue | 🟡 Minor
httpHeadersarray should be marked.nullable()to match potential backend DTO nullability.Without
.nullable(), if the API returnshttpHeaders: null, Yup's array type-check will throw a type error ("this must be a \array` type") *before* the.test()guard on line 32 (!headers || headers.length === 0) is ever reached — making that guard dead code fornull`. This is the same alignment pattern highlighted for other Admin UI validation schemas.🛡️ Proposed fix
- httpHeaders: Yup.array() + httpHeaders: Yup.array() + .nullable() .of(If you also want to guarantee
min(1)is still enforced when the field is non-null, chain.nullable().default([])to ensurenullis coerced to[]before the conditionalmin(1)check runs:- httpHeaders: Yup.array() + httpHeaders: Yup.array() + .nullable() + .default([]) .of(Based on learnings: "ensure Yup schema marks fields as nullable when the backend DTOs permit nulls and do not mark them as required — align frontend validation with backend validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/helper/validations/webhookValidation.ts` around lines 18 - 19, The httpHeaders Yup schema in webhookValidation.ts should be marked nullable to match backend DTO nullability: update the httpHeaders array schema (the httpHeaders definition) to chain .nullable() (and optionally .default([]) if you want null coerced to an empty array) so that the existing .test() guard and any .min(1) validation apply only when the field is non-null; in short, add .nullable() to the httpHeaders array schema (optionally .nullable().default([])) to prevent Yup from throwing an "must be an array" error when the API returns null.admin-ui/app/routes/Apps/Gluu/GluuSessionTimeout.tsx (1)
41-41:⚠️ Potential issue | 🟡 MinorTypo in audit log message: "mannually" → "manually".
This string is dispatched to
auditLogoutLogsand may appear in persistent audit records.✏️ Fix typo
- message: isTimedOut ? 'User session timed out' : 'User logged out mannually', + message: isTimedOut ? 'User session timed out' : 'User logged out manually',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuSessionTimeout.tsx` at line 41, The audit log message contains a typo: change the string in the message field used when dispatching auditLogoutLogs (in GluuSessionTimeout's logout handling) from 'User logged out mannually' to 'User logged out manually' so persistent audit records use the correct spelling; update the message expression where message: isTimedOut ? 'User session timed out' : 'User logged out mannually' is defined.admin-ui/app/redux/api/LicenseApi.ts (1)
25-54: 🧹 Nitpick | 🔵 TrivialInner promise
rejectshadows outer promisereject— potential silent swallow on retry exhaustion.In
getIsActive(and identically incheckAdminuiLicenseConfig), the innerPromiseconstructor parameterrejecton line 30 shadows the outerrejectfrom line 28. Currently this works because the inner rejection is caught by.catch()which then either retries or callshandleError(error, reject)(using the outerrejectvia closure). However, this shadowing is fragile — a future refactor could easily reference the wrongreject.Consider renaming the inner reject to
rejectInner(matching theresolveInnernaming already used) for clarity.♻️ Suggested rename for clarity
- new Promise<LicenseResponse | null>((resolveInner, reject) => { + new Promise<LicenseResponse | null>((resolveInner, rejectInner) => { this.api.isLicenseActive((error, data) => { if (error) { - reject(error) + rejectInner(error) } else { resolveInner(data) }Apply the same pattern to
checkAdminuiLicenseConfig(lines 56-84).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/redux/api/LicenseApi.ts` around lines 25 - 54, In getIsActive (and likewise in checkAdminuiLicenseConfig) the inner Promise parameter named reject shadows the outer reject; rename the inner reject to rejectInner (matching resolveInner) and update its usage inside the inner Promise so the outer reject from the outer Promise remains unshadowed and handleError(error, reject) continues to reference the outer reject via closure.admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
91-115: 🧹 Nitpick | 🔵 TrivialDual sort mechanism (server + client) may cause user confusion.
serverSortis sent as API query params (lines 96–97), whileclientSortre-sorts the page-level results in-memory (lines 109–114). When a user clicks a column header (handleSort→clientSort), only the current page's rows are reordered, not the full dataset. Meanwhile, the "Sort By" filter dropdown (handleSortByFilter→serverSort) triggers a server-side sort that resetsclientSort.This is not a bug, but the two sort paths could be confusing if users expect column-header sorting to be server-side. Consider either documenting this behavior or unifying both paths to use server-side sorting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx` around lines 91 - 115, The code mixes server-side sorting (serverSort used in useGetAllWebhooks) with client-side reordering (clientSort applied in the webhooks useMemo), which can confuse users; unify sorting to server-side by removing the client-side re-sort and making header clicks update serverSort instead. Concretely: stop using clientSort in the webhooks useMemo (return webhooksRaw directly), update the header click handler (handleSort) to call the same setter used by handleSortByFilter (set serverSort column and desc, reset pageNumber/startIndex if needed, and clear clientSort/state), and trigger a refetch (or rely on the query updating) so the API returns the correctly sorted full dataset via useGetAllWebhooks; keep sortRows only for any local stable fallback but do not apply it to displayed rows when serverSort is active.admin-ui/app/utils/hooks/useWebhookDialogAction.tsx (2)
116-123:⚠️ Potential issue | 🟡 MinorAccessibility: interactive handler on
aria-hidden="true"element;role="img"conflicts withonClick.Three issues with this
<i>element:
aria-hidden="true"should not be used on elements that can receive focus, and placingonClickon it means sighted users can click a "hidden" element — creating an accessibility "black hole" where screen-reader users miss out on functionality.role="img"declares this as a decorative image, which directly contradicts theonClick={closeWebhookTriggerModal}intent. An interactive close control should userole="button".onKeyDown={() => {}}is a no-op — keyboard users cannot activate this element despite its apparent interactivity.Since
ModalHeaderis already wired withtoggle={closeWebhookTriggerModal}, theonClickon the icon is entirely redundant. The simplest fix is to removeonClick/onKeyDownand keeparia-hidden="true"so the icon is purely decorative:♻️ Proposed fix — remove redundant interactive handlers from decorative icon
<i - onClick={closeWebhookTriggerModal} - onKeyDown={() => {}} style={{ color: customColors.logo }} className="fa fa-2x fa-info fa-fw modal-icon mb-3" role="img" aria-hidden="true" ></i>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/utils/hooks/useWebhookDialogAction.tsx` around lines 116 - 123, The <i> icon currently has interactive handlers but is marked aria-hidden and role="img": remove the redundant interactivity by deleting onClick={closeWebhookTriggerModal} and onKeyDown={() => {}} from the <i> element (leave aria-hidden="true" and role="img" so it remains decorative); rely on the existing ModalHeader toggle={closeWebhookTriggerModal} handler instead to provide keyboard-accessible closing — locate the element near closeWebhookTriggerModal usage in useWebhookDialogAction.tsx and update that <i> element accordingly.
66-71:⚠️ Potential issue | 🟡 Minor
onCloseModalshould dispatchsetWebhookModal(false)for proper cleanup.Currently,
dispatch(setWebhookModal(enabledFeatureWebhooks?.length > 0))maintains the webhook modal state based on whether webhooks exist, which is redundant with the effect at lines 85–87 that already syncs this state. WhenonCloseModalis called on parent modal close, this dispatch re-enables the webhook modal state instead of clearing it, preventing clean state reset for the next dialog open cycle. The effect will automatically restorewebhookModalto the correct value when webhooks are loaded again.♻️ Proposed fix
const onCloseModal = useCallback(() => { - dispatch(setWebhookModal(enabledFeatureWebhooks?.length > 0)) + dispatch(setWebhookModal(false)) dispatch(setWebhookTriggerErrors([])) dispatch(setTriggerWebhookResponse('')) dispatch(setFeatureToTrigger('')) - }, [dispatch, enabledFeatureWebhooks]) + }, [dispatch])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/utils/hooks/useWebhookDialogAction.tsx` around lines 66 - 71, onCloseModal currently re-enables the webhook modal by dispatching setWebhookModal(enabledFeatureWebhooks?.length > 0); update onCloseModal (the useCallback) to dispatch setWebhookModal(false) instead so the modal state is explicitly cleared on close, while leaving the other cleanup dispatches (setWebhookTriggerErrors([]), setTriggerWebhookResponse(''), setFeatureToTrigger('')) intact; this ensures proper reset until the existing effect that syncs webhookModal from enabledFeatureWebhooks runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/__tests__/setup.ts`:
- Line 35: Replace the direct prototype assignment
HTMLAnchorElement.prototype.click = jest.fn() with a spy created via jest.spyOn
so that the existing teardown (jest.restoreAllMocks() called in afterAll) will
clean it up; locate the setup where HTMLAnchorElement.prototype.click is
assigned and change it to use jest.spyOn(HTMLAnchorElement.prototype, "click")
to create the mock, ensuring any existing mock implementation is provided to the
spy if needed.
In `@admin-ui/app/components/App/PermissionsPolicyInitializer.tsx`:
- Line 118: Change the warning message emitted by devLogger.warn in
PermissionsPolicyInitializer (the line that currently logs `❌ Cedarling got
failed. Retrying in 1000ms`) to a grammatically correct phrase such as `❌
Cedarling initialization failed. Retrying in 1000ms` so the log reads clearly
and consistently; update the string passed to devLogger.warn accordingly in the
PermissionsPolicyInitializer component.
In `@admin-ui/app/locales/fr/translation.json`:
- Line 764: The French translations are inconsistent: the key
"no_shortcodes_found" uses the loanword "shortcode" while another entry on the
same file uses "code court"; update that other translation to use "shortcode" so
the UI term is consistent. Locate the entry that currently contains the French
phrase "code court" (the tooltip/label around line ~952) and replace "code
court" with "shortcode", keeping punctuation/spacing consistent with
"no_shortcodes_found". Also scan the rest of the file for any other occurrences
of "code court" and standardize them to "shortcode".
- Line 786: The translation value for the key "request_body_error" uses
incorrect mid-sentence capitalization; update the string to use sentence-case by
lowercasing "Corps" and "Requête" so it reads: "Le corps de la requête est
requis pour la méthode HTTP sélectionnée." Replace the existing value for
"request_body_error" in translation.json accordingly.
In `@admin-ui/app/redux/api/backend-api.ts`:
- Around line 75-93: In postUserAction, avoid shallow-clone-and-delete by
destructuring userAction directly; replace "const payload = { ...userAction };
delete payload.headers" with "const { headers, ...payload } = userAction" so you
build the request body without mutating an intermediate object (keep the rest of
the function and the payload variable name unchanged).
In `@admin-ui/app/redux/api/HealthCheckApi.ts`:
- Around line 11-16: The interface HealthCheckApiInstance is missing an export
causing callers to be forced to use ConstructorParameters workarounds; fix by
adding the export keyword to the HealthCheckApiInstance declaration so it
becomes exported (export interface HealthCheckApiInstance) and ensure it matches
the export pattern used by the other interfaces and the HealthCheckApi
constructor parameter type.
In `@admin-ui/app/redux/api/LicenseApi.ts`:
- Line 92: The use of unchecked casts like "d as LicenseResponse | null"
bypasses type safety; update the flow by adding lightweight runtime validation
or a generic wrapper so handleResponse returns a typed value instead of unknown:
either (A) make handleResponse generic (e.g., handleResponse<T>) and call it
with LicenseResponse so the returned data is typed, or (B) add a small type
guard function isLicenseResponse(obj) that checks required fields and use it to
validate d before resolving in the LicenseApi methods (resolve only when
isLicenseResponse(d) or allow null), referencing handleResponse and
LicenseResponse to locate the call sites where the casts occur.
In `@admin-ui/app/redux/api/types/BackendApi.ts`:
- Around line 25-27: The API payload field "date" is typed as Date but should
reflect JSON wire format; change the property declaration "date?: Date" in
BackendApi types to "date?: string" (and ensure consistency with "performedOn?:
string | Date" by preferring string for payload fields), then update any callers
to serialize Date instances to ISO strings before sending.
In `@admin-ui/app/redux/sagas/AttributesSaga.ts`:
- Around line 41-43: In the AttributesSaga, the finally block always dispatches
toggleInitAttributeLoader(false) even when payload.init was falsy; update the
finally block to mirror the guard at the top by only yielding
put(toggleInitAttributeLoader(false)) when payload.init is truthy (same
conditional used where you yield put(toggleInitAttributeLoader(true))). Apply
the same conditional fix to the other occurrence around lines 56-58 so both
enable/disable actions are paired and only dispatched when payload.init is true.
- Line 46: Remove the redundant TypeScript cast on the yielded value: in
AttributesSaga.ts replace "const attributeApi = (yield* newFunction()) as
AttributeApi" with "const attributeApi = yield* newFunction()" (or simply "const
attributeApi = yield* getAttributeApi()" if you've renamed newFunction per prior
suggestion), since newFunction's Generator<SelectEffect, AttributeApi, string>
signature already infers AttributeApi.
- Around line 27-33: Rename the placeholder generator function newFunction to a
descriptive name like createAttributeApiClient (or similar) to reflect that it
selects authReducer.issuer and returns an AttributeApi instance; update the
function declaration and all call sites that reference newFunction to use the
new name, keeping the body intact (selecting issuer, constructing
JansConfigApi.AttributeApi via getClient, and returning new AttributeApi(api)).
In `@admin-ui/app/redux/sagas/AuthSaga.ts`:
- Line 73: Several auth-critical catch blocks (getOAuth2ConfigWorker,
getAPIAccessTokenWorker, createAdminUiSessionWorker) currently only call
devLogger.error (guarded by isDevelopment) which silences errors in production;
update those catch handlers to also emit the full error to a non-development
sink (e.g., call console.error(error) or use a production-aware logger) before
dispatching any subsequent saga effects so the complete error object is visible
in staging/production; ensure the log call includes the full error object (not
just error.message) and keep devLogger.error as supplemental for dev-only
context.
In `@admin-ui/app/redux/sagas/HealthSaga.ts`:
- Around line 17-31: The selector state annotation in createHealthApi and
createHealthCheckApi uses an inline type ({ authReducer: { issuer: string } })
which should be replaced with the project's RootState to keep types consistent;
import RootState from the app's types (or the same location used in
SagaUtils.ts), change the select generics to use RootState (e.g., yield
select((state: RootState) => state.authReducer.issuer)), and update any import
statements accordingly so both HealthSaga generator helpers reference RootState
instead of the inline shape.
- Around line 52-70: The getHealthServerStatus saga currently swallows 403
responses and uses an unannotated catch variable; update getHealthServerStatus
to mirror getHealthStatus by detecting a 403 from
healthApi.getHealthServerStatus and triggering the same logout/redirect flow (or
calling the same session-expiry handler) so session-expiry isn't ignored, and
change the catch binding to an explicit typed variable (catch (e: unknown)) to
match style and satisfy useUnknownInCatchVariables; ensure you reference the
existing initAudit/addAdditionalData/postUserAction calls and preserve the
existing error log (devLogger.warn) while passing the typed error variable into
it.
- Around line 44-48: The catch block uses e: unknown but calls
isHttpLikeError(e) and isFourZeroThreeError(e) which causes TS strict-type
errors; update the calls to cast e to the expected union type (e.g., as Error |
SagaErrorShape) before passing it to isHttpLikeError and
isFourZeroThreeError—mirror the explicit cast pattern used in WebhookSaga (the
two-call sites referenced) so the logic around yield
put(getHealthStatusResponse(null)) and yield* redirectToLogout() remains
unchanged.
In `@admin-ui/app/redux/sagas/LicenseSaga.ts`:
- Around line 218-221: The hardcoded English error message passed into
uploadNewSsaTokenResponse in the LicenseSaga should be replaced with a localized
string using i18n.t(); update the yield put(...) call that currently passes
"Invalid SSA. Please contact Gluu's team to verify if SSA is correct." to call
i18n.t('license.invalidSsa') (or another appropriate translation key),
add/import i18n if missing, and ensure the new translation key is added to the
locale files so the message is localized across locales.
- Around line 75-77: The code passes parseInt(mauThreshold?.value ?? '', 10)
which can produce NaN and cause silent failures in checkMauThreshold; change
this to validate the parsed number before yielding checkMauThreshold: locate the
variables arr and mauThreshold and replace the direct parseInt call with parsing
into a variable (e.g., parsed = parseInt(...)), then if Number.isNaN(parsed) use
a sensible default (like 0 or null) or bail out early, and finally yield*
checkMauThreshold with that validated/defaulted value so checkMauThreshold never
receives NaN.
In `@admin-ui/app/redux/sagas/LockSaga.ts`:
- Line 9: Remove the unused audit initialization by deleting the unused "const
audit = yield* initAudit()" statement in LockSaga (the call to initAudit is the
leftover); locate the saga function in LockSaga.ts and remove that line so there
is no unused variable or unused yield* call, keeping the rest of the import of
JansConfigApi and getClient usage intact.
In `@admin-ui/app/redux/sagas/SagaUtils.ts`:
- Around line 31-40: The redirectToLogout generator places window.location.href
= '/admin/logout' inside the try block so if fetchApiTokenWithDefaultScopes or
deleteAdminUiSession throws the redirect is skipped; move the redirect into a
finally block (or place it after the try/catch) so it always runs regardless of
errors, while keeping the auditLogoutLogs put and the devLogger.error in the
catch and retaining the ApiTokenResponse cast for response.access_token in the
deleteAdminUiSession call.
In `@admin-ui/app/redux/sagas/types/audit.ts`:
- Around line 4-9: The types HttpErrorLike and SagaError (SagaErrorShape) are
duplicated across app/ and plugins/admin/; consolidate them into a single shared
module (e.g., redux/sagas/types/common or a new shared types file) and update
both consumers to import the canonical types instead of redefining them: remove
the duplicate definitions of HttpErrorLike and SagaError in
admin-ui/app/redux/sagas/types/audit.ts, export the single definitions from the
shared module, and update any references/exports that used SagaErrorShape or
SagaError to import the shared type so both app and plugins/admin use the same
type symbols (HttpErrorLike and SagaError or a single agreed name).
In `@admin-ui/app/routes/Apps/Gluu/GluuInputRow.tsx`:
- Around line 120-149: The number stepper (classes.numberStepper inside the
numberWrapper block) can overlap with a provided shortcode when type ===
'number'; add a defensive check in GluuInputRow to prevent rendering shortcode
with numeric inputs (e.g., ignore/omit the shortcode prop when props.type ===
'number') or, if you prefer not to change behavior, add a clear inline comment
next to the number rendering branch explaining that shortcode and type="number"
are mutually exclusive and must not be used together; reference the number
rendering branch that conditionally renders inputEl and the numberStepper so
reviewers can find and enforce the guard.
In `@admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx`:
- Around line 26-32: The early return using webhookState is placed before hooks
(useTheme, useMemo, useEffect) which violates the Rules of Hooks; update the
GluuWebhookErrorDialog component so that all hook calls (including useTheme, any
useMemo and useEffect) are invoked unconditionally at the top of the component,
and then guard rendering by either moving the "if (!webhookState) return null"
check into the JSX return (e.g., render null or a placeholder when webhookState
is falsy) or by checking webhookState after hooks and using conditional JSX;
ensure you reference the existing webhookState variable and the hook calls in
GluuWebhookErrorDialog so hooks always run in the same order on every render.
In `@admin-ui/app/routes/Pages/ByeBye.tsx`:
- Around line 15-19: The current double-cast of EmptyLayout to extract
EmptyLayout.Section is fragile; instead adjust the EmptyLayout component's
module to export a typed Section (e.g., export the Section component and/or its
prop type) so consumers can import it with correct types and avoid assertions;
update the EmptyLayout module to export the Section symbol and its props type,
then replace the casted EmptyLayoutSection usage in this file with a direct
import of EmptyLayout.Section or the exported Section component/prop type to
preserve type-safety (references: EmptyLayout, EmptyLayoutSection).
In `@admin-ui/app/utils/devLogger.ts`:
- Around line 1-16: devLogger.error currently no-ops in production (guards on
isDevelopment) which hides real errors; change the devLogger implementation so
error(...args) always forwards to console.error (remove or bypass the
isDevelopment check for the error method) so catch blocks (e.g., in
GluuSessionTimeout.tsx, routes/index.tsx, LicenseApi.ts) still emit errors in
production; keep log/warn/debug gated by isDevelopment or alternatively add a
separate prodError method that always logs and use that in catch blocks—refer to
the devLogger.error symbol when making the change.
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Around line 138-152: submitForm currently clears deleteData immediately which
prevents retry on failure; update submitForm so it captures inumToDelete at
start but only calls setDeleteData(null) after a successful delete (i.e., move
setDeleteData(null) into the try block after await deleteWebhook(...) and
refetch()), and do not clear deleteData in the catch so the UI/dialog still
knows which item failed; optionally add a local submitting state to prevent
double-submits while awaiting deleteWebhook; reference submitForm, deleteData,
deleteWebhook, and refetch when making the change.
- Around line 267-288: The nested ternary in the render function determining
methodStyle is hard to read and should be replaced with a lookup map; create a
constant map (e.g., httpMethodBadgeMap) that maps normalized methods
('GET','POST','PUT','PATCH','DELETE') to the corresponding entries in
badgeStyles and fall back to badgeStyles.httpMethodBadgeDefault, then replace
the current methodStyle computation in the render callback (the function that
uses row.httpMethod and methodStyle) to look up methodStyle via the map using
(row.httpMethod || '').toUpperCase(); this mirrors the SORT_COLUMN_LABELS
pattern and makes adding new methods easier.
In `@admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts`:
- Around line 76-77: normalizeUrlForValidation currently replaces
REGEX_URL_PLACEHOLDER with a generic 'x', which corrupts templates that place
the placeholder in the port (e.g. https://host:${port}) and causes new URL(...)
inside isAllowed to throw; update normalizeUrlForValidation so that when the
placeholder appears in a port position it substitutes a valid numeric port (e.g.
'8080' or '443') instead of 'x' — keep using REGEX_URL_PLACEHOLDER to locate
placeholders but detect/handle the port case before returning the normalized
string so isAllowed's URL parsing succeeds.
In `@admin-ui/plugins/admin/helper/validations/webhookValidation.ts`:
- Around line 27-30: Verify whether the backend enforces the "at least one
header for POST/PUT/PATCH" rule; if it does not, either remove the frontend
.min(1) check (in webhookValidation.ts where hasHttpBody is used) or add
matching server-side validation; update docs/admin/admin-ui/webhooks.md to state
headers are required for body methods if you keep the rule; ensure data
compatibility by adding a migration or editor fallback that converts existing
httpHeaders undefined/[] (sanitized by sanitizeWebhookHeaders) into a compliant
shape (e.g., add a default header or allow an empty placeholder) so existing
webhooks do not break when opened for editing.
In `@admin-ui/plugins/admin/redux/sagas/types/audit.ts`:
- Around line 15-17: The two AuditLog interfaces use incompatible payload types
(payload?: AuditRecord vs payload?: Record<string, JsonValue>), so unify them by
creating and using a single shared payload type (e.g., export type AuditPayload
= Record<string, JsonValue | undefined> or adjust JsonValue to allow undefined)
and replace both occurrences (references to payload, AuditRecord, and the
AuditLog interfaces) to use that shared AuditPayload so both sagas reference the
exact same domain type.
- Line 1: Remove the dead AuditRecord type declared in this file: delete the
export type AuditRecord = ... since no modules import it directly; update any
local imports if present to reference the shared AuditRecord definition in
types/common (the one used by WebhookSaga and plugins), and consolidate
duplicate definitions by keeping the single shared AuditRecord in common.ts so
there’s only one canonical type used across app and plugin sagas.
In `@admin-ui/plugins/admin/redux/types/webhook.ts`:
- Line 36: The union type TriggerWebhookReducerPayload is not safely
discriminable because both members TriggerPayloadActionPayload and
TriggerWebhookActionPayload have only optional properties; add a literal
discriminant field (e.g., type: 'payload' | 'webhook' or kind: 'payload' |
'webhook') to each interface so consumers can reliably narrow the union at
runtime (update the definitions of TriggerPayloadActionPayload and
TriggerWebhookActionPayload to include the chosen discriminant and adjust places
that construct or check these objects to set/check that literal).
---
Outside diff comments:
In `@admin-ui/app/redux/api/LicenseApi.ts`:
- Around line 25-54: In getIsActive (and likewise in checkAdminuiLicenseConfig)
the inner Promise parameter named reject shadows the outer reject; rename the
inner reject to rejectInner (matching resolveInner) and update its usage inside
the inner Promise so the outer reject from the outer Promise remains unshadowed
and handleError(error, reject) continues to reference the outer reject via
closure.
In `@admin-ui/app/redux/sagas/LockSaga.ts`:
- Around line 17-31: The audit value returned by initAudit() is captured but
never used in getLockMau; thread it through the flow so audit logging is
recorded: pass the audit context into the API call (the lockapi.getLockMau
invocation) or include it when dispatching the response (getLockStatusResponse)
and in the error path, so initAudit(), getLockMau, and getLockStatusResponse all
receive/handle the audit info consistently with other sagas; ensure
redirectToLogout() path still returns after handling audit.
In `@admin-ui/app/routes/Apps/Gluu/GluuSessionTimeout.tsx`:
- Line 41: The audit log message contains a typo: change the string in the
message field used when dispatching auditLogoutLogs (in GluuSessionTimeout's
logout handling) from 'User logged out mannually' to 'User logged out manually'
so persistent audit records use the correct spelling; update the message
expression where message: isTimedOut ? 'User session timed out' : 'User logged
out mannually' is defined.
In `@admin-ui/app/utils/hooks/useWebhookDialogAction.tsx`:
- Around line 116-123: The <i> icon currently has interactive handlers but is
marked aria-hidden and role="img": remove the redundant interactivity by
deleting onClick={closeWebhookTriggerModal} and onKeyDown={() => {}} from the
<i> element (leave aria-hidden="true" and role="img" so it remains decorative);
rely on the existing ModalHeader toggle={closeWebhookTriggerModal} handler
instead to provide keyboard-accessible closing — locate the element near
closeWebhookTriggerModal usage in useWebhookDialogAction.tsx and update that <i>
element accordingly.
- Around line 66-71: onCloseModal currently re-enables the webhook modal by
dispatching setWebhookModal(enabledFeatureWebhooks?.length > 0); update
onCloseModal (the useCallback) to dispatch setWebhookModal(false) instead so the
modal state is explicitly cleared on close, while leaving the other cleanup
dispatches (setWebhookTriggerErrors([]), setTriggerWebhookResponse(''),
setFeatureToTrigger('')) intact; this ensures proper reset until the existing
effect that syncs webhookModal from enabledFeatureWebhooks runs.
In `@admin-ui/babel.config.js`:
- Around line 20-37: In the env.test presets array, move
'@babel/preset-typescript' to come immediately after the '@babel/preset-react'
entry so the presets order matches the top-level config (ensuring TypeScript
preset runs before the React JSX transform); update the presets array used in
the env.test block rather than the top-level one.
In `@admin-ui/plugins/admin/components/Health/components/ServiceStatusCard.tsx`:
- Around line 33-36: The useMemo using the object reference service causes
unnecessary recomputation; change the dependency list on the useMemo that
defines statusMessage (which calls getStatusMessage and uses
REGEX_TRAILING_PERIOD) to use the primitive fields read by that function instead
of the whole object — e.g., depend on service.status and service.error along
with t (mirroring how badgeColors depends on service.status) so the memo only
invalidates when those actual values change.
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Around line 91-115: The code mixes server-side sorting (serverSort used in
useGetAllWebhooks) with client-side reordering (clientSort applied in the
webhooks useMemo), which can confuse users; unify sorting to server-side by
removing the client-side re-sort and making header clicks update serverSort
instead. Concretely: stop using clientSort in the webhooks useMemo (return
webhooksRaw directly), update the header click handler (handleSort) to call the
same setter used by handleSortByFilter (set serverSort column and desc, reset
pageNumber/startIndex if needed, and clear clientSort/state), and trigger a
refetch (or rely on the query updating) so the API returns the correctly sorted
full dataset via useGetAllWebhooks; keep sortRows only for any local stable
fallback but do not apply it to displayed rows when serverSort is active.
In `@admin-ui/plugins/admin/helper/shortCodes.json`:
- Around line 847-898: In the users_edit and users_delete blocks the "label"
values wrongly use documentation.user.* keys; update the "label" entries to use
the concise fields.* keys for the existing translations (fields.givenName,
fields.sn, fields.mail, fields.displayName, fields.inum, fields.status) and keep
the "description" values as documentation.user.*; for the two missing label keys
(middleName and userId) either add fields.middleName and fields.userId to the
i18n files or (preferred) set the label for userId to fields.userName and leave
middleName's label pointing to documentation.user.middleName until a
fields.middleName translation is added so labels render correctly without
changing other sections.
In `@admin-ui/plugins/admin/helper/utils.ts`:
- Around line 65-67: The cast on the cloned webhook is redundant: remove the
unnecessary intersection "& { httpRequestBody?: Record<string, JsonValue> }"
from the line that creates const webhook (which uses cloneDeep(originalWebhook))
and simply cast to WebhookWithBody (or rely on inferred type) since
WebhookWithBody already declares httpRequestBody; update the declaration that
references cloneDeep and originalWebhook accordingly.
In `@admin-ui/plugins/admin/helper/validations/webhookValidation.ts`:
- Around line 18-19: The httpHeaders Yup schema in webhookValidation.ts should
be marked nullable to match backend DTO nullability: update the httpHeaders
array schema (the httpHeaders definition) to chain .nullable() (and optionally
.default([]) if you want null coerced to an empty array) so that the existing
.test() guard and any .min(1) validation apply only when the field is non-null;
in short, add .nullable() to the httpHeaders array schema (optionally
.nullable().default([])) to prevent Yup from throwing an "must be an array"
error when the API returns null.
---
Duplicate comments:
In `@admin-ui/app/redux/api/HealthApi.ts`:
- Around line 1-29: Ensure the HealthData interface is exported and consistently
used: export the HealthData interface (it should be referenced by other
modules), keep the AuthServerHealthCheckApi callback typing, and preserve the
error path in HealthApi.getHealthStatus where handleError(error, reject) is
called with an early return so resolve(data) only runs on success; verify the
class property name api and method getHealthStatus remain unchanged.
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuInputRow.style.ts`:
- Around line 1-92: The borderColor calculation in useStyles uses string
interpolation `${fontColor}40` which breaks if fontColor isn't a 6‑char hex;
change it to compute a safe color fallback by converting fontColor to an
RGBA/hex-with-alpha reliably (e.g., a helper that accepts fontColor and alpha
and returns a valid CSS color) and use that helper when computing borderColor
inside useStyles (referencing the fontColor param and borderColor variable);
ensure theme.palette.divider remains the fallback when conversion fails.
In `@admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts`:
- Around line 21-29: The index signature on AuditInit and the ActionData type
are too broad (using object); update AuditInit's dynamic property type to [key:
string]: unknown and tighten ActionData by removing object so it only allows
primitive and null values (e.g., keep string|number|boolean|null) to avoid
unintentionally permitting arrays/functions/class instances; locate the
AuditInit interface and the ActionData type in useWebhookAudit.ts and apply
these type changes to those symbols.
In `@admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts`:
- Around line 44-46: The audit log .catch blocks in useWebhookMutations (calls
to logAction) currently only console.error when isDevelopment, which hides
failures in production; update each catch (around the logAction calls in
useWebhookMutations) to always surface failures — at minimum call
processLogger.error or forward the error to the existing error-reporting
mechanism (Sentry/telemetry) and include the auditError and context (action name
/ webhook id) in the message, while optionally also preserving the isDevelopment
console.debug path; ensure you update all occurrences referenced (the catch
blocks near lines handling audit logging, e.g., the callbacks around logAction
calls) so production audit failures are logged to the centralized
logger/error-tracker instead of being silently discarded.
In `@admin-ui/plugins/admin/components/Webhook/styles/ShortcodePopover.style.ts`:
- Around line 14-19: The optional chaining used on required ThemeConfig fields
is misleading; update the useStyles implementation to access required properties
directly: replace themeColors.settings?.cardBackground with
themeColors.settings.cardBackground and themeColors.card?.background with
themeColors.card.background inside the useStyles callback (keeping the existing
fallback to themeColors.inputBackground), and ensure
ShortcodePopoverStylesParams / calling sites rely on the non-null contract of
themeColors.settings and themeColors.card rather than masking missing values
with ?.
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Around line 356-359: The previous React key-collision bug is fixed by updating
the getRowKey callback to accept the index parameter and use it in the fallback;
ensure the useCallback for getRowKey signature is (row: WebhookEntry, index:
number) and that it returns row.inum ?? `no-inum-${index}` so each row gets a
stable unique key when inum is missing.
- Around line 407-416: The label construction for GluuCommitDialog is already
correct—keep the current expression in the label prop that uses [deleteData.url,
deleteData.inum].filter(Boolean).join('-') to avoid a leading dash when url is
empty; no code change needed, just ensure the label remains on the
GluuCommitDialog component and references deleteData.url and deleteData.inum as
shown.
In `@admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts`:
- Around line 76-77: Add a JSDoc comment above the normalizeUrlForValidation
function documenting its limitation: explain that replacing placeholders via
url.replace(REGEX_URL_PLACEHOLDER, 'x') can break hostname validation (e.g.,
"https://${host}/path" → "https://x/path" fails PATTERN because 'x' has no dot),
note that this function only normalizes placeholders for basic checks and
suggest callers use a fully qualified host placeholder (or adjust replacement to
include a dot like 'x.com') if strict hostname patterns are required; reference
normalizeUrlForValidation and REGEX_URL_PLACEHOLDER in the comment so future
readers know the exact symbols implicated.
In `@admin-ui/plugins/admin/helper/utils.ts`:
- Line 9: Importing JsonValue from Routes/Apps/Gluu/types/common and
re-exporting it as a type fixes the previous overly-broad typing (object) for
httpRequestBody; ensure the import in admin-ui/plugins/admin/helper/utils.ts
uses a type-only import (import type { JsonValue } ...) and that JsonValue is
re-exported (export type { JsonValue }) so there is no runtime import or
circular dependency while httpRequestBody and related signatures use JsonValue
for accurate typing.
In `@admin-ui/plugins/admin/helper/validations/webhookValidation.ts`:
- Around line 3-5: The wrapper around the imported hasHttpBody (imported as
hasHttpBodyStrict) addresses undefined propagation by exposing a local
hasHttpBody(method?: string): boolean which calls hasHttpBodyStrict(method ??
''), so keep this wrapper as-is; ensure both places that use the local
hasHttpBody (the two is: hasHttpBody usages) remain and no changes to the
upstream helper are made.
In `@admin-ui/plugins/admin/redux/features/WebhookSlice.ts`:
- Line 13: The re-export in WebhookSlice.ts is correct—keep the line exporting
WebhookSliceState and WebhookSliceTriggerPayload and ensure the file imports
these types from ../types/webhook (remove any local duplicate definitions such
as StoredTriggerPayload); confirm the symbols WebhookSliceState and
WebhookSliceTriggerPayload are only provided via the central types module and
not duplicated in this slice.
In `@admin-ui/plugins/admin/redux/sagas/types/common.ts`:
- Around line 16-27: The updated isHttpLikeError type guard (function
isHttpLikeError) correctly checks for a numeric status on the top-level error or
nested under error.response, so no further change is required; keep the current
implementation as-is in common.ts and ensure any callers rely on the refined
type predicate (SagaErrorShape & HttpErrorLike) for downstream type-narrowing.
In `@admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts`:
- Around line 85-90: The explicit type annotation on the yield select result is
misleading because select can return undefined; update the declaration for
featureWebhooks to reflect that (e.g., allow WebhookEntry[] | undefined) or
remove the fixed WebhookEntry[] annotation so the compiler infers the possibly
undefined type from the select call, keeping the existing nullish-coalescing
usage for enabledFeatureWebhooks ((featureWebhooks ?? []).filter(...)) intact;
locate the variable featureWebhooks and the select call in WebhookSaga.ts to
make this change.
- Around line 138-151: The audit posting currently happens before the 403
redirect check, which is correct; ensure the code always calls
addAdditionalData(audit, FETCH, `/webhook/${featureToTrigger || 'trigger'}`,
{...}) and yield call(postUserAction, audit as UserActionPayload) before
performing the 403 check and yield* redirectToLogout(); move or keep the yield
call(postUserAction, ...) and its preceding addAdditionalData call above the
isHttpLikeError/isFourZeroThreeError check so audit records (via
addAdditionalData and postUserAction) are always sent prior to calling
redirectToLogout().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx (1)
46-50:⚠️ Potential issue | 🟠 Major
closeModalresetstriggerWebhookMessageto a hardcoded error string, leaving stale state.dispatch(setTriggerWebhookResponse('Something went wrong while triggering webhook.'))
setTriggerWebhookResponsesets the value rendered in red astriggerWebhookMessage(line 73–75). Resetting it to this error string on every close means the next modal open — even after a successful trigger — will display this stale error until a new trigger overwrites it. The reset value should benull,'', or whatever the slice'sinitialStateuses for that field.🐛 Proposed fix
const closeModal = () => { dispatch(setShowErrorModal(!showErrorModal)) dispatch(setWebhookTriggerErrors([])) - dispatch(setTriggerWebhookResponse('Something went wrong while triggering webhook.')) + dispatch(setTriggerWebhookResponse(null)) }Adjust
nullto match the slice's declared initial value fortriggerWebhookMessage(e.g.,''ornull).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx` around lines 46 - 50, The closeModal handler resets triggerWebhookMessage to a hardcoded error string causing stale UI; update closeModal (function name: closeModal) to dispatch setTriggerWebhookResponse with the slice's initial empty value (e.g., null or '' matching the reducer) instead of `'Something went wrong while triggering webhook.'`, so the red message (triggerWebhookMessage rendered around lines 73–75) is cleared on close; pick the exact initial value used in the slice for triggerWebhookMessage to keep state consistent.admin-ui/app/components/App/PermissionsPolicyInitializer.tsx (3)
29-32: 🧹 Nitpick | 🔵 Trivial
retryCount.current.callMethodis dead code — it is written but never read.
callMethodis initialised totrue(line 31) and reset tofalseon success (line 112), but the field is never consumed anywhere. Remove it to reduce confusion.♻️ Proposed fix
const retryCount = useRef({ tryCount: 0, - callMethod: true, })- retryCount.current = { tryCount: 0, callMethod: false } + retryCount.current.tryCount = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/components/App/PermissionsPolicyInitializer.tsx` around lines 29 - 32, The retryCount ref includes an unused property callMethod; remove callMethod from the useRef initialization (const retryCount = useRef({ tryCount: 0 })) and also remove any writes that set retryCount.current.callMethod = false elsewhere (e.g., in the success path inside PermissionsPolicyInitializer) so only tryCount remains; this cleans up dead code while keeping the retry logic driven by retryCount.current.tryCount.
34-34: 🧹 Nitpick | 🔵 Trivial
useState(10)for an immutable value is misleading — use a module-level constant instead.
maxRetriesis initialized once and the setter is never called, yetuseStateimplies the value could change. This also makesmaxRetriestechnically missing from theuseEffectdependency array (line 129), which would trigger thereact-hooks/exhaustive-depslint rule.♻️ Proposed refactor
+const MAX_RETRIES = 10 + const PermissionsPolicyInitializer = () => { const dispatch = useDispatch() const retryCount = useRef({ tryCount: 0, callMethod: true, }) - const [maxRetries] = useState(10)Then replace all usages of
maxRetrieswithMAX_RETRIES.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/components/App/PermissionsPolicyInitializer.tsx` at line 34, Replace the ephemeral useState for an immutable retry constant: remove the declaration const [maxRetries] = useState(10) and instead add a module-level constant MAX_RETRIES = 10; update all usages of maxRetries to MAX_RETRIES (e.g., inside the PermissionsPolicyInitializer component and any effects such as the useEffect that references maxRetries) so the value is clearly immutable and no longer needs to be included in effect dependency arrays.
46-101: 🧹 Nitpick | 🔵 TrivialRedundant JSON parsing —
isValidPolicyStoreand the outertry/catchboth parse the same string.
isValidPolicyStorealready callsJSON.parseto validate the string (line 57). Immediately after, the outertry/catchblock callsJSON.parseagain (line 91) on the same value. The second parse is a no-op — if the first succeeded, the second will too.Additionally, the parameter type
policyStore: string | unknownsimplifies to justunknownsinceunknownis the top type and subsumesstring.♻️ Proposed refactor
- const isValidPolicyStore = (policyStore: string | unknown): boolean => { + const isValidPolicyStore = (policyStore: unknown): boolean => {After
shouldTryInitis confirmed (meaningisValidPolicyStorealready validated the string), the outertry/catchcan be simplified to just a stringify step, trusting the prior validation:- let policyStoreString: string - try { - if (typeof policyStoreJson === 'string') { - // Already a string, but verify it's valid JSON - JSON.parse(policyStoreJson) - policyStoreString = policyStoreJson - } else { - // It's an object, stringify it - policyStoreString = JSON.stringify(policyStoreJson) - } - } catch (error) { - devLogger.error('Invalid policy store JSON format:', error) - dispatch(setCedarlingInitializing(false)) - return - } + const policyStoreString = + typeof policyStoreJson === 'string' ? policyStoreJson : JSON.stringify(policyStoreJson)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/components/App/PermissionsPolicyInitializer.tsx` around lines 46 - 101, isValidPolicyStore currently redundantly parses JSON and accepts a parameter typed as string | unknown; change its signature to accept unknown (policyStore: unknown) and keep its existing validation logic, then remove the second JSON.parse in the outer block — after shouldTryInit is true, if typeof policyStoreJson === 'string' assign policyStoreString = policyStoreJson directly, otherwise wrap JSON.stringify(policyStoreJson) in a try/catch to handle stringify errors (keep devLogger.error and dispatch(setCedarlingInitializing(false)) on catch); update references to isValidPolicyStore, shouldTryInit, policyStoreJson, and policyStoreString accordingly.admin-ui/app/redux/sagas/AuthSaga.ts (1)
111-117: 🧹 Nitpick | 🔵 TrivialInconsistent error logging in
putConfigWorkercatch block.Every other worker's catch block calls
devLogger.error(...)(or previouslyconsole.error), butputConfigWorkersilently swallows the error details — only dispatching a generic error toast and returning the raw error. Adding a log call here would keep the pattern consistent and aid debugging of config-save failures.Also,
return erroron line 117 is unusual for a saga worker invoked viatakeEvery(the return value is discarded). If it's intentional (e.g., for testing), a comment would clarify intent; otherwise it can be removed.♻️ Proposed fix
} catch (error: unknown) { + const err = error as ApiErrorLike + devLogger.error('Problems saving configuration.', err?.response?.data ?? error) yield put(updateToast(true, 'error')) - if (isFourZeroThreeError(error as ApiErrorLike)) { + if (isFourZeroThreeError(err)) { yield* redirectToLogout() return } - return error } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/redux/sagas/AuthSaga.ts` around lines 111 - 117, The catch block in putConfigWorker currently only dispatches updateToast and returns the raw error; add a devLogger.error call to log the error (include context like "putConfigWorker error" and the error object) to match other workers, and remove the final "return error" since saga handlers run under takeEvery and the return value is ignored (or if the return is needed for tests, retain it but add a clarifying comment). Locate the catch in putConfigWorker and update it to call devLogger.error(error, 'putConfigWorker') before handling the 403 check and remove or comment the trailing return accordingly.admin-ui/app/redux/api/LicenseApi.ts (1)
25-84: 🧹 Nitpick | 🔵 TrivialFour compounding issues in the shared retry pattern — refactor both
getIsActiveandcheckAdminuiLicenseConfigThe retry implementation in both methods has the following issues:
rejectshadowing (lines 30, 60): The inner promise's second constructor parameter is also namedreject, shadowing the outer promise'sreject. Functionally, the.then()/.catch()chains capture the outerreject(because the inner parameter's scope ends at the inner constructor callback's closing brace). However, this is a silent maintenance trap — any reader or future contributor must carefully trace lexical scope to confirm whichrejectis active in the.catch()block. Rename the inner parameters toinnerResolve/innerReject.Redundant
const retries = 0(lines 26, 57): Thisconstis only ever used to pass0to the firstmakeRequest(retries)call. The mutation (retries++) operates on themakeRequestparameter, not this outer const. Inline asmakeRequest(0).Mutating the closure-captured parameter (lines 45, 75):
retries++thenmakeRequest(retries)mutates the local parameter before passing it. UsemakeRequest(retries + 1)directly.Inner promise chain not returned from
makeRequest(lines 30, 60): If any handler in the.then()/.catch()chain throws synchronously, the rejection is silently swallowed. Return the chained promise frommakeRequest.♻️ Proposed refactor (same fix applies to
checkAdminuiLicenseConfig)- getIsActive = (): Promise<LicenseResponse | null> => { - const retries = 0 - - return new Promise((resolve, reject) => { - const makeRequest = (retries: number) => { - new Promise<LicenseResponse | null>((resolveInner, reject) => { - this.api.isLicenseActive((error, data) => { - if (error) { - reject(error) - } else { - resolveInner(data) - } - }) - }) - .then((response) => { - resolve(response) - }) - .catch((error) => { - if (retries < MAX_RETRIES) { - devLogger.error(`Request failed. Retrying... (${retries + 1}/${MAX_RETRIES})`) - retries++ - makeRequest(retries) - } else { - handleError(error, reject) - } - }) - } - makeRequest(retries) - }) - } + getIsActive = (): Promise<LicenseResponse | null> => { + return new Promise((resolve, reject) => { + const makeRequest = (retries: number) => { + return new Promise<LicenseResponse | null>((innerResolve, innerReject) => { + this.api.isLicenseActive((error, data) => { + if (error) { + innerReject(error) + } else { + innerResolve(data) + } + }) + }) + .then((response) => { + resolve(response) + }) + .catch((error) => { + if (retries < MAX_RETRIES) { + devLogger.error(`Request failed. Retrying... (${retries + 1}/${MAX_RETRIES})`) + makeRequest(retries + 1) + } else { + handleError(error, reject) + } + }) + } + makeRequest(0) + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/redux/api/LicenseApi.ts` around lines 25 - 84, Both getIsActive and checkAdminuiLicenseConfig share a broken retry pattern: rename the inner promise constructor parameters to innerResolve/innerReject to avoid shadowing the outer reject, remove the unused outer "const retries = 0" and call makeRequest(0) directly, stop mutating the parameter with retries++ and instead call makeRequest(retries + 1) when retrying, and ensure makeRequest returns the inner promise chain so thrown synchronous errors are propagated; apply these changes in the getIsActive and checkAdminuiLicenseConfig methods (identify by those method names and their makeRequest inner functions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/__tests__/setup.ts`:
- Around line 36-38: Replace the direct assignment to
globalThis.URL.createObjectURL with a spy so it is restored by
jest.restoreAllMocks(); specifically, if typeof globalThis.URL !== 'undefined'
use jest.spyOn(globalThis.URL, 'createObjectURL').mockImplementation(jest.fn())
(mirroring the existing HTMLAnchorElement.prototype.click spy) so teardown via
restoreAllMocks() undoes the mock; keep the existing conditional guard around
globalThis.URL to avoid runtime errors.
- Around line 8-12: The beforeAll hook currently awaits i18n.init() without
error handling, which hides init failures; wrap the await in a try/catch inside
the beforeAll (checking i18n.isInitialized as now) and in the catch log or
rethrow the caught error (include the error message and stack) so Jest surfaces
the root cause; modify the beforeAll that calls i18n.init() to catch and surface
initialization errors.
In `@admin-ui/app/components/App/PermissionsPolicyInitializer.tsx`:
- Around line 98-99: The devLogger.error calls for the critical init failures
are development-only and get silenced in production; replace the devLogger.error
invocation that logs "Invalid policy store JSON format" and the devLogger.error
used on permanent max-retry failure with a production-visible logger (e.g.,
console.error) or call a new always-on method (e.g., devLogger.criticalError) so
those errors are always emitted in production, and make sure the log statement
runs before dispatch(setCedarlingInitializing(false)) so the failure is
recorded.
In `@admin-ui/app/locales/en/translation.json`:
- Around line 1141-1142: The JSON contains duplicate strings: titles.add_webhook
and messages.add_webhook both equal "Add Webhook"; remove the duplication by
consolidating to a single canonical key (either keep messages.add_webhook or
titles.add_webhook), delete the other key, and update all code/usages to
reference the chosen key (search for titles.add_webhook and
messages.add_webhook). Also ensure the corresponding Portuguese translation is
updated/removed to match the canonical key so translations stay in sync.
- Around line 823-826: Update the translation value for the key
"http_method_error" in translation.json to use the correct indefinite article by
changing the string to "Please select an HTTP Method." so the message reads
grammatically correct when displayed.
In `@admin-ui/app/locales/es/translation.json`:
- Line 1008: The translation uses inconsistent terminology for “shortcode”: the
key "insert_shortcode" currently maps to "Insertar código corto" while elsewhere
(e.g., the "shortcodes" key) uses "shortcodes"; update the "insert_shortcode"
value to use the same Spanish term chosen project-wide (replace "Insertar código
corto" with the consistent term used by "shortcodes") so all keys like
"insert_shortcode" and "shortcodes" use the identical translated word.
In `@admin-ui/app/redux/api/LicenseApi.ts`:
- Around line 141-155: getTrialLicense and retrieveLicense are missing the
transient-failure retry that getIsActive and checkAdminuiLicenseConfig use; wrap
the JansConfigApi calls inside the same retry pattern (use MAX_RETRIES = 1) or
the existing retry helper so network hiccups/timeouts are retried once before
rejecting. Locate getTrialLicense and retrieveLicense and modify their Promise
logic to retry the this.api.getTrialLicense / this.api.retrieveLicense calls
(using the same loop or helper used by getIsActive/checkAdminuiLicenseConfig)
and then call handleTypedResponse with the final error/data result.
- Line 92: The call sites in LicenseApi.ts are converting undefined to null
(e.g., handleTypedResponse<LicenseResponse | null>(error, reject, resolve, data
?? null, null)), which prevents handleTypedResponse’s "no data" branch from ever
rejecting; either remove the null-coalescing and pass data directly so
handleTypedResponse can reject on undefined (i.e., use data, not data ?? null)
at all occurrences (the calls around the handleTypedResponse invocations noted,
e.g., the call at line 92 and the other locations mentioned), or if
undefined→null is an intentional semantic (undefined means “no license”), add a
clear comment at each call explaining that intention and why resolve(null) is
desired.
- Around line 97-100: The variable name "option" in uploadSSAtoken should be
renamed to the plural "options" to match the rest of the file; update the const
declaration in uploadSSAtoken and any local references/types (e.g., the type {
sSARequest: SSARequest }) to use "options" so it is consistent with other
methods in LicenseApi.ts and avoid naming inconsistency.
In `@admin-ui/app/redux/sagas/AuthSaga.ts`:
- Around line 29-32: The ApiErrorLike interface is duplicated; extract it to a
shared types location and import it where needed: create a shared type (e.g.,
export interface ApiErrorLike { response?: { data?: { responseMessage?: string;
message?: string }; status?: number }; message?: string }) inside a common file
such as SagaUtils.ts or types/saga.ts, update AuthSaga.ts to import ApiErrorLike
instead of declaring it locally, and replace the duplicate declarations in other
saga files with imports to ensure a single source of truth for error shape.
- Around line 214-238: The watchers use hardcoded action-type strings and cast
takeEvery/takeLatest to any; import the missing action creators from authSlice
(getAPIAccessToken, getUserLocation, putConfigWorker, deleteAdminUiSession) and
replace the string literals in getApiTokenWatcher, getOAuth2ConfigWatcher,
getLocationWatcher, putConfigWorkerWatcher, createAdminUiSessionWatcher, and
deleteAdminUiSessionWatcher with the corresponding actionCreator.type (e.g.
getAPIAccessToken.type) and remove the `(takeEvery as any)`/`(takeLatest as
any)` casts so you call takeEvery/takeLatest with the typed action type and the
existing worker functions (getAPIAccessTokenWorker, getOAuth2ConfigWorker,
getLocationWorker, putConfigWorker, createAdminUiSessionWorker,
deleteAdminUiSessionWorker).
- Around line 194-197: The createAdminUiSessionWorker function is typed as
Generator<unknown, void, unknown>, causing a type mismatch that forced the use
of an unnecessary cast on the redirectToLogout call; change the return type of
createAdminUiSessionWorker to the bare Generator (matching other workers in this
file) and remove the cast so you call yield* redirectToLogout() directly
(referencing createAdminUiSessionWorker and redirectToLogout to locate the
change).
In `@admin-ui/app/routes/Apps/Gluu/GluuInputRow.tsx`:
- Around line 124-144: The stepper container rendered by classes.numberStepper
sets aria-hidden which removes its children from the accessibility tree, making
the child button aria-labels on the numberStepperBtn elements (and their
Increment/Decrement labels) unreachable; remove the redundant aria-label
attributes (and optionally keep tabIndex={-1}) from the buttons wired to stepUp
and stepDown and ensure the native <input type="number"> remains keyboard
accessible so the custom stepper stays visual-only.
In `@admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx`:
- Around line 79-81: The list uses key={item.responseMessage} which is optional
and can be duplicate/undefined; update the mapping in GluuWebhookErrorDialog.tsx
(the webhookTriggerErrors map over WebhookTriggerResponseItem) to use a stable
unique key: prefer an existing unique identifier on the item (e.g., item.id or
item.requestId if present) and fall back to a composite key that includes the
array index only as last resort (e.g., `${uniqueField ?? ''}-${index}`) so keys
are never undefined and remain stable across renders; ensure you update the key
reference in the map callback and remove any reliance on responseMessage as the
sole key.
In `@admin-ui/app/utils/regex.ts`:
- Around line 27-28: The REGEX_WEBHOOK_URL currently allows arbitrarily large
numeric ports via "(:\d+)?"; tighten this by restricting the port to at most
five digits (e.g., "(:\d{1,5})?") or replace with a range-checking lookahead to
enforce 0–65535 in the regex for stricter validation, and update the JSDoc above
REGEX_WEBHOOK_URL to note that the host can be a domain, IPv4, or IPv6 address
so IPv4 addresses are documented as supported.
In `@admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx`:
- Line 95: handleSortByFilter updates serverSort but doesn't reset pageNumber,
so the query uses a stale startIndex (startIndex: pageNumber * limit) and can
issue out-of-range requests; update handleSortByFilter to reset pageNumber to 0
(or compute and set effectivePage) when changing sort, and/or change the query
to use effectivePage (instead of pageNumber) when computing startIndex so
refetches never send an out-of-bounds offset; apply the same fix to the other
query usage around the second occurrence noted (the block using startIndex at
lines ~199-202).
- Around line 188-193: The handlePagingSizeSync function is a no-op wrapper
around onPagingSizeSync and the useCallback is unnecessary; remove the
handlePagingSizeSync declaration and replace any usages of handlePagingSizeSync
with onPagingSizeSync (or pass onPagingSizeSync directly to components/props) in
WebhookListPage.tsx to eliminate the extra allocation and simplify the code.
- Around line 269-279: The httpMethodBadgeMap is being allocated inside the cell
render path and thus recreated for every row render; move the httpMethodBadgeMap
definition out of the render callback and into the enclosing columns useMemo
(the memo factory scope where badgeStyles is available) so it’s created once per
badgeStyles change; update the render to reference the hoisted
httpMethodBadgeMap (used to compute methodStyle via httpMethodBadgeMap[upper] ??
badgeStyles.httpMethodBadgeDefault) instead of rebuilding it inline.
---
Outside diff comments:
In `@admin-ui/app/components/App/PermissionsPolicyInitializer.tsx`:
- Around line 29-32: The retryCount ref includes an unused property callMethod;
remove callMethod from the useRef initialization (const retryCount = useRef({
tryCount: 0 })) and also remove any writes that set
retryCount.current.callMethod = false elsewhere (e.g., in the success path
inside PermissionsPolicyInitializer) so only tryCount remains; this cleans up
dead code while keeping the retry logic driven by retryCount.current.tryCount.
- Line 34: Replace the ephemeral useState for an immutable retry constant:
remove the declaration const [maxRetries] = useState(10) and instead add a
module-level constant MAX_RETRIES = 10; update all usages of maxRetries to
MAX_RETRIES (e.g., inside the PermissionsPolicyInitializer component and any
effects such as the useEffect that references maxRetries) so the value is
clearly immutable and no longer needs to be included in effect dependency
arrays.
- Around line 46-101: isValidPolicyStore currently redundantly parses JSON and
accepts a parameter typed as string | unknown; change its signature to accept
unknown (policyStore: unknown) and keep its existing validation logic, then
remove the second JSON.parse in the outer block — after shouldTryInit is true,
if typeof policyStoreJson === 'string' assign policyStoreString =
policyStoreJson directly, otherwise wrap JSON.stringify(policyStoreJson) in a
try/catch to handle stringify errors (keep devLogger.error and
dispatch(setCedarlingInitializing(false)) on catch); update references to
isValidPolicyStore, shouldTryInit, policyStoreJson, and policyStoreString
accordingly.
In `@admin-ui/app/redux/api/LicenseApi.ts`:
- Around line 25-84: Both getIsActive and checkAdminuiLicenseConfig share a
broken retry pattern: rename the inner promise constructor parameters to
innerResolve/innerReject to avoid shadowing the outer reject, remove the unused
outer "const retries = 0" and call makeRequest(0) directly, stop mutating the
parameter with retries++ and instead call makeRequest(retries + 1) when
retrying, and ensure makeRequest returns the inner promise chain so thrown
synchronous errors are propagated; apply these changes in the getIsActive and
checkAdminuiLicenseConfig methods (identify by those method names and their
makeRequest inner functions).
In `@admin-ui/app/redux/sagas/AuthSaga.ts`:
- Around line 111-117: The catch block in putConfigWorker currently only
dispatches updateToast and returns the raw error; add a devLogger.error call to
log the error (include context like "putConfigWorker error" and the error
object) to match other workers, and remove the final "return error" since saga
handlers run under takeEvery and the return value is ignored (or if the return
is needed for tests, retain it but add a clarifying comment). Locate the catch
in putConfigWorker and update it to call devLogger.error(error,
'putConfigWorker') before handling the 403 check and remove or comment the
trailing return accordingly.
In `@admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx`:
- Around line 46-50: The closeModal handler resets triggerWebhookMessage to a
hardcoded error string causing stale UI; update closeModal (function name:
closeModal) to dispatch setTriggerWebhookResponse with the slice's initial empty
value (e.g., null or '' matching the reducer) instead of `'Something went wrong
while triggering webhook.'`, so the red message (triggerWebhookMessage rendered
around lines 73–75) is cleared on close; pick the exact initial value used in
the slice for triggerWebhookMessage to keep state consistent.
---
Duplicate comments:
In `@admin-ui/app/components/App/PermissionsPolicyInitializer.tsx`:
- Line 118: The warning message logged via devLogger.warn inside
PermissionsPolicyInitializer has already been corrected to "Cedarling
initialization failed. Retrying in 1000ms", so no code changes are needed;
verify there are no other occurrences of the old "Cedarling got failed" string
in the module (search for devLogger.warn and the cedarling message) and leave
the current log entry as-is.
In `@admin-ui/app/redux/sagas/AuthSaga.ts`:
- Around line 80-86: Catch blocks in AuthSaga.ts are only logging via
devLogger.error which is disabled outside development; update each catch (where
error is cast to ApiErrorLike and handled with devLogger.error, e.g., the block
around isFourZeroThreeError and other catches in this file) to also output the
full error to a production-capable sink: add a console.error(error) (or call a
production logger) alongside the existing devLogger.error so the entire error
object is visible in staging/prod, leaving the existing devLogger.error calls
intact and ensuring redirectToLogout and isFourZeroThreeError logic remains
unchanged.
In `@admin-ui/app/routes/Apps/Gluu/GluuInputRow.tsx`:
- Around line 120-149: The code currently silently omits shortcode for number
inputs; make the behavior explicit by rendering shortcode only when type !==
'number' (e.g., place the conditional `{type !== 'number' ? shortcode : null}`
in the non-number branch of the JSX) so that GluuInputRow clearly shows
shortcode for non-number inputs and never for type === 'number'; adjust the JSX
around inputEl/numberWrapper (and related classes numberWrapper/numberStepper)
to ensure the shortcode conditional is located outside the number-specific
markup and only rendered when appropriate.
In `@admin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsx`:
- Around line 41-44: Ensure the hooks (useTheme, useMemo, useEffect) remain
called unconditionally before the early return and keep the destructuring of
webhookState using "const { triggerWebhookMessage, webhookTriggerErrors,
triggerWebhookInProgress, showErrorModal } = webhookState ?? {}" as a non-hook
expression; also remove the duplicate review marker/comment that produced
[duplicate_comment].
In `@admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts`:
- Around line 73-79: Add a JSDoc comment for normalizeUrlForValidation
explaining the limitation: when an entire hostname is a placeholder (e.g.
`https://${host}/path`) the function substitutes 'x' (or '8080' for port
positions) so the result has no dot and will not match REGEX_WEBHOOK_URL's
domain portion, causing the URL to be rejected; document this behavior and the
port-substitution special-case so future maintainers understand why such
host-placeholder URLs fail validation.
duttarnab
requested changes
Feb 23, 2026
Contributor
There was a problem hiding this comment.
-
Edit a existing webhook by changing and saving
Admin UI Featuresfield. Open that webhook again. The changed field value will display the old value. -
I have added
TESTING_GETwebhook toUsers-Add/Editfeature. But the webhook is not triggered by calling
/admin-ui/webhook/trigger/{featureId}on editing a user
|
duttarnab
approved these changes
Feb 24, 2026
moabu
approved these changes
Feb 24, 2026
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
4 participants
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.
feat(admin-ui): revamp Home -> WebHooks page as per Figma (#2629)
Summary
This PR revamps the Home → WebHooks page to align with the latest Figma designs as part of the Admin UI redesign initiative.
Changes Included
Result
Parent Initiative
This work is part of the following parent feature:
feat(admin-ui): Revamp the existing design of the admin-ui according to the new design🔗 Ticket
Closes: #2629
Summary by CodeRabbit
New Features
Improvements
Bug Fixes