feat(admin-ui): revamp Scripts module as per Figma with partial TS migration#2680
Closed
faisalsiddique4400 wants to merge 40 commits intomainfrom
Closed
feat(admin-ui): revamp Scripts module as per Figma with partial TS migration#2680faisalsiddique4400 wants to merge 40 commits intomainfrom
faisalsiddique4400 wants to merge 40 commits intomainfrom
Conversation
…nt casing Made-with: Cursor
Contributor
|
Important Review skippedToo many files! This PR contains 170 files, which is 20 over the limit of 150. 📒 Files selected for processing (170)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
admin-ui/plugins/auth-server/components/Configuration/Keys/KeysPage.tsx (1)
7-12: 🧹 Nitpick | 🔵 TrivialInconsistent import path style for routes.
Line 7 uses the alias-based path
@/routes/Apps/Gluu/...(lowercaseroutes), while lines 11-12 use the non-alias pathRoutes/Apps/Gluu/...(capitalRoutes). Consider aligning all route imports to the same style for consistency.♻️ Suggested fix to standardize imports
If the project is moving toward alias-based imports (as indicated by the PR summary), update lines 11-12 to match:
-import GluuViewWrapper from 'Routes/Apps/Gluu/GluuViewWrapper' -import GluuFormFooter from 'Routes/Apps/Gluu/GluuFormFooter' +import GluuViewWrapper from '@/routes/Apps/Gluu/GluuViewWrapper' +import GluuFormFooter from '@/routes/Apps/Gluu/GluuFormFooter'Alternatively, if
Routes(non-alias) is the established convention, revert line 7:-import applicationStyle from '@/routes/Apps/Gluu/styles/applicationStyle' +import applicationStyle from 'Routes/Apps/Gluu/styles/applicationStyle'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/auth-server/components/Configuration/Keys/KeysPage.tsx` around lines 7 - 12, The imports in KeysPage.tsx mix alias-based and non-alias route paths; update the GluuViewWrapper and GluuFormFooter imports (symbols: GluuViewWrapper, GluuFormFooter) to use the same alias-based route style as applicationStyle (e.g., change their import paths from "Routes/Apps/Gluu/..." to "@/routes/Apps/Gluu/...") so all route imports are consistent across the file.admin-ui/plugins/user-management/components/User2FADevicesModal.tsx (1)
192-219: 🧹 Nitpick | 🔵 TrivialRemove unnecessary dependency from useCallback.
The
otpDevicesListdependency on line 218 is unnecessary because line 214 uses the functional update formsetOTPDevicesList((prev) => ...), which doesn't requireotpDevicesListto be in the dependency array. Including it causes the callback to be recreated on every device list change, leading to unnecessary re-renders.⚡ Performance optimization
}, - [userDetails, deleteFido2Mutation, updateUserData, otpDevicesList], + [userDetails, deleteFido2Mutation, updateUserData], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/user-management/components/User2FADevicesModal.tsx` around lines 192 - 219, The handleRemove2Fa useCallback lists otpDevicesList in its dependency array but only uses setOTPDevicesList with a functional updater, so remove otpDevicesList from the dependencies to avoid unnecessary recreations; update the dependency array for handleRemove2Fa to [userDetails, deleteFido2Mutation, updateUserData] (keeping the other referenced symbols intact).admin-ui/plugins/auth-server/components/Clients/ClientActiveTokens.js (3)
26-26: 🧹 Nitpick | 🔵 TrivialRemove unnecessary
useMemofor constant empty array.Since
myActionsis always an empty array, wrapping it inuseMemoadds unnecessary overhead. Consider defining it as a constant outside the component or passing an empty array directly to theactionsprop.♻️ Simplified approach
Option 1 - Constant outside component:
+const EMPTY_ACTIONS = [] + function ClientActiveTokens({ client }) { - const myActions = useMemo(() => [], [])Option 2 - Inline (if only used once):
- const myActions = useMemo(() => [], []) // ... later in MaterialTable - actions={myActions} + actions={[]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/auth-server/components/Clients/ClientActiveTokens.js` at line 26, myActions is defined as useMemo(() => [], []) but it's a constant empty array, so remove the unnecessary useMemo and replace with a plain constant (e.g., const myActions = []) or pass [] directly to the actions prop used in the ClientActiveTokens component; update references to myActions (or the actions prop usage) to use the plain array so the component no longer imports or uses useMemo.
422-424: 🧹 Nitpick | 🔵 TrivialImprove PropTypes specificity.
Using
PropTypes.anyprovides no meaningful type validation. Consider defining a more specific shape, especially since the component relies onclient.inum.🔍 More specific PropTypes
ClientActiveTokens.propTypes = { - client: PropTypes.any, + client: PropTypes.shape({ + inum: PropTypes.string.isRequired, + }).isRequired, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/auth-server/components/Clients/ClientActiveTokens.js` around lines 422 - 424, ClientActiveTokens.propTypes currently uses PropTypes.any for the client, which loses validation; change it to a more specific PropTypes.shape that at minimum requires the inum property (e.g. client: PropTypes.shape({ inum: PropTypes.string.isRequired })) and add any other expected fields used by ClientActiveTokens to the shape so the component receives validated props.
350-352: 🧹 Nitpick | 🔵 TrivialSimplify redundant ternary expression.
The ternary operator returning
true : falseis redundant since the condition already evaluates to a boolean.♻️ Simplified version
disabled={ - pattern.dateAfter === null || pattern.dateBefore === null ? true : false + pattern.dateAfter === null || pattern.dateBefore === null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/auth-server/components/Clients/ClientActiveTokens.js` around lines 350 - 352, The disabled prop in the ClientActiveTokens component uses a redundant ternary; replace the expression disabled={pattern.dateAfter === null || pattern.dateBefore === null ? true : false} with the simpler boolean form disabled={pattern.dateAfter === null || pattern.dateBefore === null}, referencing the pattern.dateAfter and pattern.dateBefore checks where the disabled prop is set.admin-ui/app/routes/Apps/Gluu/GluuTypeAheadWithAdd.tsx (1)
57-58:⚠️ Potential issue | 🟠 Major
optsis conflating available options with selected values, causing dropdown corruption.Line [72] overwrites
optswith current selection, while Line [58] appends all selected items back into options. This can collapse available choices and introduce duplicates after a few edits.Proposed fix
const addItem = useCallback(() => { const input = document.getElementById(inputId) as HTMLInputElement | null - const newItem = input?.value ?? '' - if (input) input.value = '' + const newItem = input?.value ?? '' if (validator(newItem)) { const updatedItems = items ? [...items] : [] updatedItems.push(newItem) - const temp = opts ? [...opts] : [] - temp.push(...updatedItems) + const temp = opts.includes(newItem) ? opts : [...opts, newItem] setOpts(temp) setItems(updatedItems) formik.setFieldValue(name, updatedItems) + if (input) input.value = '' if (handler) { handler(name, updatedItems) } } }, [inputId, items, name, opts, formik, handler, validator]) const handleChange = useCallback( (aName: string, selected: string[]) => { - setOpts(selected) setItems(selected) formik.setFieldValue(aName, selected) }, [formik], )Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuTypeAheadWithAdd.tsx` around lines 57 - 58, The dropdown corruption comes from conflating opts (available options) with selected values: instead of pushing updatedItems into temp and overwriting opts later, keep available options and selected values separate. Change the logic around temp/opts/updatedItems so you create availableOptions = opts ? [...opts] : [], then remove any selected values (updatedItems) from availableOptions and dedupe, rather than doing temp.push(...updatedItems) or reassigning opts to the selection; update the code paths referencing temp, opts, and updatedItems (e.g., the temp creation, the temp.push(...updatedItems) call, and the later overwrite of opts) to use availableOptions and a separate selectedItems variable so options never contain selected values and duplicates are prevented.admin-ui/app/routes/Apps/Gluu/GluuRemovableInputRow.tsx (1)
94-101:⚠️ Potential issue | 🟡 MinorAccessibility:
onKeyDownshould filter for activation keys.The
onKeyDownhandler triggers on any key press. For proper keyboard accessibility, it should only activate onEnterorSpacekeys.🛡️ Proposed fix
<div role="button" style={applicationStyle.removableInputRow as React.CSSProperties} - onKeyDown={() => handler()} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + handler() + } + }} onClick={() => handler()} + tabIndex={0} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuRemovableInputRow.tsx` around lines 94 - 101, The clickable div in GluuRemovableInputRow currently calls handler() for any key press; update the onKeyDown to accept the keyboard event, check for activation keys (event.key === 'Enter' || event.key === ' ' || event.key === 'Spacebar' || event.keyCode === 13 || event.keyCode === 32) and only then invoke handler(), calling event.preventDefault() for the Space key to avoid scrolling; also ensure the div with role="button" is focusable by adding tabIndex={0} if not already present so keyboard users can reach it.admin-ui/plugins/admin/components/Health/components/ServiceStatusCard.tsx (1)
46-66: 🧹 Nitpick | 🔵 TrivialInconsistent theming for badge colors across status types.
The
upstatus now usesthemeColors.badges.*for dynamic theming, butdown,degraded, andunknownstatuses still use hardcodedcustomColors.*. This creates an inconsistency where theme changes will only affect the "up" badge appearance.Consider using theme-derived colors for all status types, or add corresponding badge colors (e.g.,
statusInactiveBg,statusInactive) to the theme configuration.🤖 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 46 - 66, The badgeColors useMemo currently mixes themeColors and hardcoded customColors causing inconsistent theming; update the switch in badgeColors (the useMemo that checks service.status) to derive all bg/text values from themeColors (e.g., themeColors.badges.statusInactiveBg, themeColors.badges.statusInactive, etc.) instead of customColors so theme toggles affect every status, and add any missing theme keys to theme configuration if they don't exist; ensure the dependency array (service.status, isDark, themeColors) remains accurate after the 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/__tests__/setup.ts`:
- Around line 32-33: The current global suppression in
admin-ui/__tests__/setup.ts filters out React's "not wrapped in act()" warnings
(the msg.includes('not wrapped in act(') branch) which hides real async test
issues; remove that suppression and update the tests producing the warnings to
properly await state updates using React Testing Library utilities (e.g.,
waitFor, findBy*, or userEvent async flows) or wrap the specific updates in
act() where appropriate so state changes are awaited rather than globally
silencing the error.
In `@admin-ui/app/components/Accordion/index.ts`:
- Around line 6-12: The local redefinition of AccordionComponent should be
removed and the shared AccordionComponent type reused: import the existing
AccordionComponent type and then cast AccordionBase using that imported type
(keep references to AccordionBase, AccordionBody, AccordionHeader,
AccordionIndicator), instead of declaring a new type locally; delete the local
type alias and replace "const Accordion = AccordionBase as AccordionComponent"
with a cast to the imported/shared AccordionComponent to ensure the public API
type is consistent.
In `@admin-ui/app/components/CustomInput/CustomInput.tsx`:
- Around line 13-21: The data-testid prop on RSCustomInput can be undefined when
name is undefined; update CustomInput.tsx to avoid rendering data-testid when
name is falsy by conditionally including that attribute (e.g., only spread
{'data-testid': name} when name is truthy) so RSCustomInput receives data-testid
only for valid names; keep the rest of the props (className=inputClass, label,
name, ...otherProps) unchanged.
In `@admin-ui/app/components/GluuDetailGrid/__tests__/GluuDetailGrid.test.tsx`:
- Around line 16-36: Wrap the existing tests in a describe('GluuDetailGrid', ()
=> { ... }) block and add unit tests in the same file that cover: rendering when
fields=[] (expect no labels/values or a placeholder), passing different layout
values to the GluuDetailGrid component (verify DOM layout/class changes),
providing a className prop (assert it appears on the root element), and handling
missing/undefined field values in mockFields (assert no crashes and appropriate
fallback UI). Use the existing mockFields and Wrapper to build these tests and
reference the GluuDetailGrid component and its props (fields,
defaultDocCategory, labelStyle, layout, className) when locating where to add
the new test cases.
In `@admin-ui/app/components/GluuInlineAlert/index.ts`:
- Around line 1-2: The current file re-exports the component as a default export
which causes inconsistent import styles vs GluuDetailGrid; change the re-export
to a named export so consumers can import { GluuInlineAlert } consistently —
update the component export to re-export the default under the name
GluuInlineAlert (leave the type re-exports like GluuInlineAlertProps and
GluuInlineAlertSeverity intact) so imports mirror GluuDetailGrid's pattern.
In `@admin-ui/app/components/GluuSpinner/GluuSpinner.tsx`:
- Around line 13-15: GluuSpinner accepts any numeric size (including 0 or
negatives) and passes it straight into useStyles, which can produce
invalid/invisible output; update the GluuSpinner component to validate and
normalize the size prop before calling useStyles and rendering: if size is not a
positive number (NaN, <= 0, or undefined), substitute DEFAULT_SPINNER_SIZE (or
clamp to a minimum positive value) and then call useStyles({ size: validatedSize
}) and use validatedSize for any inline style/attributes; reference the
GluuSpinner component, the size prop, DEFAULT_SPINNER_SIZE, and the useStyles
call when applying this change.
In `@admin-ui/app/components/Widgets/GroupedButtons/Counter.tsx`:
- Around line 21-23: The handleDecrement callback can produce negative counters;
update it to guard against going below zero by checking the current counter
before calling onCounterChange (e.g., only call onCounterChange when counter > 0
or pass Math.max(0, counter - 1)); modify the handleDecrement function so it
never invokes onCounterChange with a negative value to protect against
programmatic calls or race conditions.
In `@admin-ui/app/locales/pt/translation.json`:
- Line 1175: The "yes" translation value is lowercase ("sim") which breaks UI
casing consistency; update the "yes" key in translation.json so its value is
capitalized ("Sim") to match neighboring option labels and preserve localization
key "yes".
In `@admin-ui/app/routes/Apps/Gluu/GluuFormFooter.tsx`:
- Around line 208-209: Replace the manual class string concatenation with clsx
to match the rest of the file: in the JSX where style={cancelButtonStyle ??
BUTTON_STYLE} and className is built using
`${buttonLayout.cancel}${buttonStates.hasAllThreeButtons ? ' ms-4' : ''}
${cancelButtonClassName}`.trim(), instead call clsx(buttonLayout.cancel, {
'ms-4': buttonStates.hasAllThreeButtons }, cancelButtonClassName) so class
composition is clearer and consistent with the existing useMemo that uses clsx.
In `@admin-ui/app/routes/Apps/Gluu/GluuScriptErrorModal.tsx`:
- Line 62: The Close button's aria-label in GluuScriptErrorModal.tsx is
hardcoded as "Close"; replace the literal with the i18n translation call used in
this codebase (e.g., useTranslation().t or props.t) and pass the actions.close
key (or the appropriate translated key) instead so
aria-label={t('actions.close')} (or equivalent) is used on the close button
element to localize assistive text.
- Around line 37-41: The copyToClipboard handler currently sets isCopied before
navigator.clipboard.writeText completes, which can leave the UI stuck if the
write fails; change copyToClipboard to be async, await
navigator.clipboard.writeText(error), call setIsCopied(true) only after the
await succeeds, and add a try/catch that resets/ensures setIsCopied(false) on
failure (and optionally surface an error notification); keep the hook
dependencies (error, isCopied) consistent and avoid setting isCopied
preemptively so users can retry if the clipboard write fails.
In `@admin-ui/app/routes/Apps/Gluu/GluuTypeAheadWithAdd.tsx`:
- Around line 51-53: The code reads input.value into newItem then clears the DOM
input before validation, causing invalid entries to be erased; change the order
so validation runs first using newItem (call validator(newItem)) and only clear
input.value (or set input.value = '') if validator returns true (e.g., inside
the block guarded by validator(newItem)); reference the variables newItem,
input, and the validator call so the clearing happens after successful
validation.
- Line 25: The component GluuTypeAheadWithAdd declares a multiple prop but
ignores it when rendering (hardcoded multiple={true}); change the rendered
control to use the declared prop (pass multiple={multiple}) and ensure a
sensible default (e.g., defaultProps or default param) so callers' intent is
honored; locate the JSX where the Typeahead/Select control is rendered in
GluuTypeAheadWithAdd and replace the hardcoded boolean with the multiple prop
and add a default fallback if needed.
In `@admin-ui/app/routes/Apps/Gluu/Tests/GluuTypeAheadWithAdd.test.tsx`:
- Around line 7-31: The shared module-scoped mockFormik object (mockFormik) can
leak Jest call state between tests; replace it with a factory that returns a
fresh FormikProps-like mock per test (e.g., createMockFormik or getMockFormik)
and update tests to call the factory in beforeEach or inside each test so each
test receives a new object; ensure the factory returns the same shape
(setFieldValue, setFieldTouched, validateForm, handleSubmit, etc.) and use that
symbol (createMockFormik/getMockFormik) to locate and replace usages of
mockFormik in GluuTypeAheadWithAdd.test.tsx.
In `@admin-ui/app/routes/Dashboards/components/UserInfoItem.tsx`:
- Around line 20-24: The component currently mixes context-derived theme
(useTheme → theme/state/theme → getThemeColor → themeColors) with an optional
isDark prop for badge color branching; derive a single source of truth by
computing a local isDark boolean from theme?.state?.theme (falling back to
DEFAULT_THEME) and replace uses of the optional isDark prop in badge/variant
logic with this derived value (update references in UserInfoItem where
themeColors and badge branching are used, and remove or ignore the incoming
isDark prop to avoid divergence).
In `@admin-ui/app/routes/Dashboards/DashboardPage.style.ts`:
- Around line 15-16: Add a matching theme property for the inactive state and
switch the inactive dot to use it: update the DashboardThemeColors type to
include statusInactive (to mirror statusActive), populate that property in the
theme objects, and replace usages of customColors.statusInactive in
statusDotInactive with the new theme-driven color (same pattern used by
statusDotActive) so both active and inactive dots are theme-controlled.
In `@admin-ui/app/routes/Dashboards/DashboardPage.tsx`:
- Around line 252-260: visibleStatusDetails currently filters STATUS_DETAILS to
only include services whose status is 'up' or 'down', which hides any service
with other statuses instead of letting getServiceStatus provide a fallback;
update the filter in the useMemo (visibleStatusDetails) to include a
STATUS_DETAIL when a matching service exists (i.e., service !== undefined/null)
rather than checking service.status value, keeping healthLoading and allServices
in the dependency array and letting getServiceStatus handle non-up/down statuses
for rendering.
In
`@admin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptAddPage.test.tsx`:
- Around line 116-119: The test's direct DOM query for the level input
(document.querySelector('input#level') in CustomScriptAddPage.test.tsx) is
fragile; update the component that renders the level input to include a stable
attribute such as data-testid="level-input" (where the input is
created/returned), then change the test to select it via the testing-library
query (e.g., screen.getByTestId or within(rendered).getByTestId) and assert its
value; this replaces the brittle document.querySelector('input#level') usage
with a resilient data-testid lookup while keeping the same assertions on the
input value.
In
`@admin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptForm.test.tsx`:
- Around line 53-54: The mock for useUpdateCustomScript uses an outdated field
name (isLoading); update the mock shape to match the current hook by replacing
isLoading with isPending and ensure mutateAsync remains a jest.fn() and any
callers expecting isPending in the component (e.g., CustomScriptForm test usage)
will see the correct boolean shape; modify the mock returned by
useUpdateCustomScript in the test file so it returns { mutateAsync: jest.fn(),
isPending: false } (and adjust any related assertions if needed).
- Around line 279-289: Add a real assertion after clicking the Apply button to
verify the commit-dialog flow is triggered: after the fireEvent.click(applyBtn)
inside the test "triggers commit dialog flow when Apply is clicked on dirty
form", waitFor and assert that the commit dialog UI is present (e.g., look for
the commit dialog title/text or a dialog confirm button) or assert that the
dialog-related callback is invoked, using the rendered CustomScriptForm and the
same test helpers (screen.findByTestId, screen.queryByText) so the test fails if
the commit dialog does not appear.
In `@admin-ui/plugins/admin/components/Audit/AuditListPage.style.ts`:
- Around line 63-77: The current styles that target columns using '&
[data-audit-table] table th:nth-of-type(1)...' and nth-of-type(2/3) are brittle;
change them to use stable per-column identifiers (e.g., add a data-col or class
on the corresponding th/td such as data-col="timestamp", data-col="user",
data-col="action") and update the CSS in AuditListPage.style.ts to target those
attributes instead of nth-of-type; ensure any padding rules that reference
CEDARLING_CONFIG_SPACING, SPACING or MAPPING_SPACING are moved to the new
selectors and coordinate with the component markup that renders the table
header/cells to include the new data attributes or classes.
- Line 3: The Audit styles import uses cross-feature tokens
CEDARLING_CONFIG_SPACING and MAPPING_SPACING; replace those with the shared
SPACING token or define local Audit-specific spacing constants (e.g.,
AUDIT_SPACING) and update all usages in this file (references around the block
that previously used CEDARLING_CONFIG_SPACING and MAPPING_SPACING between lines
~69-77) to use the new local/shared tokens; also remove the unused cross-feature
imports from the import list so only BORDER_RADIUS and SPACING (or your new
local constants) remain referenced.
In `@admin-ui/plugins/scripts/components/CustomScripts/constants.ts`:
- Around line 9-15: Add brief inline comments in the SCRIPT_CACHE_CONFIG
definition explaining the rationale for the non-default timings: annotate
SINGLE_SCRIPT_STALE_TIME to note why a short 2-minute TTL is used (e.g.,
expected frequent edits/views), annotate SCRIPT_TYPES_STALE_TIME to explain the
30-minute caching rationale (e.g., relatively static list but moderately fresh),
and annotate SCRIPT_TYPES_GC_TIME to justify the 60-minute GC window (e.g.,
memory retention tradeoff). Place the comments next to the corresponding keys
(SINGLE_SCRIPT_STALE_TIME, SCRIPT_TYPES_STALE_TIME, SCRIPT_TYPES_GC_TIME) inside
the SCRIPT_CACHE_CONFIG object so future maintainers can quickly understand the
caching strategy.
In `@admin-ui/plugins/scripts/components/CustomScripts/CustomScriptListPage.tsx`:
- Around line 185-192: The two useCallback hooks for handlePageChange and
handleRowsPerPageChange omit required dependencies; update the dependency arrays
to include the state setters used: add setPageNumber to the dependency array of
handlePageChange, and add both setLimit and setPageNumber to the dependency
array of handleRowsPerPageChange so the callbacks correctly capture the latest
setter references (functions referenced: handlePageChange,
handleRowsPerPageChange, setPageNumber, setLimit).
- Around line 175-178: The useCallback for handleTypeChange references
setPageNumber but doesn't include it in the dependency array; update the
dependency array of useCallback for handleTypeChange to include setPageNumber
(and setScriptType if not already present) so the callback stays correct and
stable. Locate handleTypeChange in CustomScriptListPage.tsx and add
setPageNumber to its dependencies for useCallback.
- Around line 180-183: The useCallback for handleSortByFilter omits
setPageNumber from its dependency array causing a stale closure; update the
dependency array of the handleSortByFilter hook to include setPageNumber (i.e.,
change useCallback((value: string) => { setSortBy(value); setPageNumber(0); },
[setPageNumber])) so the callback always uses the latest setter — reference the
handleSortByFilter function and the setPageNumber state setter to locate the
fix.
In `@admin-ui/plugins/scripts/components/CustomScripts/helper/auditUtils.ts`:
- Around line 46-50: The catch in the audit path swallows errors (postUserAction
called from logAuditAction) and only logs to devLogger, causing production audit
failures to be hidden; change the handler so after logging the error
(devLogger.error) you rethrow (or reject) the error so callers see the failure
(e.g., throw error or wrap with new Error including actionType/context) instead
of silently resolving; ensure this applies to the try/catch around
postUserAction (UserActionPayload) so callers can handle/report audit write
failures.
In `@admin-ui/plugins/scripts/components/CustomScripts/helper/utils.ts`:
- Around line 45-57: The normalizeProperty function returns early when it finds
description, so a property that has both description and hide loses the hide
field; update normalizeProperty to build the base object (using
PropertyInput/PropertyLike and current key/value logic) then conditionally add
description and hide properties to that object (e.g., if ('description' in p &&
p.description != null) set description = String(p.description); if ('hide' in p
&& p.hide != null) set hide = Boolean(p.hide)) and finally return the composed
object (cast to ConfigurationProperty | ModuleProperty) instead of returning
inside each conditional.
In `@admin-ui/plugins/scripts/components/CustomScripts/helper/validations.ts`:
- Around line 11-31: The validation schema rejects backend-nullable fields;
update the schema so optional fields accept nulls by adding .nullable() where
appropriate: change description from Yup.string() to Yup.string().nullable(),
make moduleProperties an array that can be null
(Yup.array().nullable().of(...)), and mark the nested value1 and value2 as
Yup.string().optional().nullable() (or Yup.string().nullable() if preferred) so
existing DTOs with nulls are accepted; leave required fields (scriptType,
programmingLanguage, level, script) unchanged.
In
`@admin-ui/plugins/scripts/components/CustomScripts/hooks/useCustomScriptApi.ts`:
- Around line 45-53: The current try/catch around dispatch(triggerWebhook(...))
only catches sync throw; ensure async failures are logged by handling the
promise returned by dispatch and by surfacing errors from the saga/thunk that
implements triggerWebhook. After calling dispatch(triggerWebhook(...)) (the
TriggerWebhookReducerPayload call), detect if the return is a Promise and attach
a .catch to log via devLogger.error (or await and catch if using async/await),
and/or update the saga/thunk that handles triggerWebhook to dispatch a failure
action that is logged so webhook execution errors are reliably captured.
In
`@admin-ui/plugins/scripts/components/CustomScripts/hooks/useMutationEffects.ts`:
- Around line 63-67: The current ternary may stringify null/undefined messages;
update the selection so we only use a non-empty string message from error
objects—e.g. keep error instanceof Error ? error.message, but for the object
branch require typeof (error as any).message === 'string' && (error as
any).message.trim() !== '' before using it; otherwise fall back to
t(errorMessage). Modify the expression around the error object check in
useMutationEffects (the ternary that uses String((error as { message: string
}).message)) to validate the message is a non-empty string before using it and
otherwise call t(errorMessage).
In
`@admin-ui/plugins/scripts/components/CustomScripts/PersonAuthenticationFields.tsx`:
- Around line 48-57: The handler handleAliasChange is typed as
ChangeEvent<HTMLInputElement> but checks for HTMLSelectElement so the type guard
never matches; change the event type to ChangeEvent<HTMLSelectElement> (or a
union that includes HTMLSelectElement) so the target instanceof
HTMLSelectElement check is valid, extract selectedOptions into values and call
formik.setFieldValue('aliases', values) as before, and update the useCallback
dependency to include the entire formik object instead of just formik
properties; refer to handleAliasChange, formik, and setFieldValue when making
these changes.
In
`@admin-ui/plugins/scripts/components/CustomScripts/styles/CustomScriptFormPage.style.ts`:
- Around line 204-210: The focus styles currently set in the '&:focus, &:active'
blocks (they remove outline/boxShadow by using OUTLINE_NONE) eliminate visible
keyboard focus indicators and must be replaced with an accessible visible focus
treatment; update the three occurrences (the shown '&:focus, &:active' block and
the similar blocks at the other two locations) to remove the OUTLINE_NONE
assignments and instead apply a clear, high-contrast focus ring such as a 2px
solid outline or a box-shadow ring using an existing theme token (e.g.,
themeColors.focusColor or a semi-opaque variant of themeColors.errorColor) while
preserving the intended backgroundColor/color/opacity, so keyboard users can see
focus without changing visual intent.
- Around line 74-83: The content style block uses a fixed horizontal padding
(CONTENT_HORIZONTAL_PADDING) causing cramped layout on small screens; update the
CustomScriptFormPage style to add a small-screen override (e.g., via a media
query or responsive breakpoint) that reduces paddingLeft and paddingRight for
the content object when viewport width is below the small breakpoint so the
container remains usable on narrow viewports; target the content style
definition and adjust padding via a responsive rule referencing
CONTENT_HORIZONTAL_PADDING (or a new SMALL_SCREEN_HORIZONTAL_PADDING constant)
to apply smaller horizontal padding on small screens.
In `@admin-ui/plugins/user-management/components/User2FADevicesModal.tsx`:
- Around line 6-7: Update the import for GluuViewDetailModal to use the project
alias like the other import: replace the old-style import "import
GluuViewDetailModal from 'Routes/Apps/Gluu/GluuViewDetailsModal'" with the
aliased form (using "@/...") so it matches applicationStyle's
"@/routes/Apps/Gluu/styles/applicationStyle" usage; locate the
GluuViewDetailModal import in User2FADevicesModal.tsx and change it to the
aliased path to keep imports consistent.
---
Outside diff comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuRemovableInputRow.tsx`:
- Around line 94-101: The clickable div in GluuRemovableInputRow currently calls
handler() for any key press; update the onKeyDown to accept the keyboard event,
check for activation keys (event.key === 'Enter' || event.key === ' ' ||
event.key === 'Spacebar' || event.keyCode === 13 || event.keyCode === 32) and
only then invoke handler(), calling event.preventDefault() for the Space key to
avoid scrolling; also ensure the div with role="button" is focusable by adding
tabIndex={0} if not already present so keyboard users can reach it.
In `@admin-ui/app/routes/Apps/Gluu/GluuTypeAheadWithAdd.tsx`:
- Around line 57-58: The dropdown corruption comes from conflating opts
(available options) with selected values: instead of pushing updatedItems into
temp and overwriting opts later, keep available options and selected values
separate. Change the logic around temp/opts/updatedItems so you create
availableOptions = opts ? [...opts] : [], then remove any selected values
(updatedItems) from availableOptions and dedupe, rather than doing
temp.push(...updatedItems) or reassigning opts to the selection; update the code
paths referencing temp, opts, and updatedItems (e.g., the temp creation, the
temp.push(...updatedItems) call, and the later overwrite of opts) to use
availableOptions and a separate selectedItems variable so options never contain
selected values and duplicates are prevented.
In `@admin-ui/plugins/admin/components/Health/components/ServiceStatusCard.tsx`:
- Around line 46-66: The badgeColors useMemo currently mixes themeColors and
hardcoded customColors causing inconsistent theming; update the switch in
badgeColors (the useMemo that checks service.status) to derive all bg/text
values from themeColors (e.g., themeColors.badges.statusInactiveBg,
themeColors.badges.statusInactive, etc.) instead of customColors so theme
toggles affect every status, and add any missing theme keys to theme
configuration if they don't exist; ensure the dependency array (service.status,
isDark, themeColors) remains accurate after the change.
In `@admin-ui/plugins/auth-server/components/Clients/ClientActiveTokens.js`:
- Line 26: myActions is defined as useMemo(() => [], []) but it's a constant
empty array, so remove the unnecessary useMemo and replace with a plain constant
(e.g., const myActions = []) or pass [] directly to the actions prop used in the
ClientActiveTokens component; update references to myActions (or the actions
prop usage) to use the plain array so the component no longer imports or uses
useMemo.
- Around line 422-424: ClientActiveTokens.propTypes currently uses PropTypes.any
for the client, which loses validation; change it to a more specific
PropTypes.shape that at minimum requires the inum property (e.g. client:
PropTypes.shape({ inum: PropTypes.string.isRequired })) and add any other
expected fields used by ClientActiveTokens to the shape so the component
receives validated props.
- Around line 350-352: The disabled prop in the ClientActiveTokens component
uses a redundant ternary; replace the expression disabled={pattern.dateAfter ===
null || pattern.dateBefore === null ? true : false} with the simpler boolean
form disabled={pattern.dateAfter === null || pattern.dateBefore === null},
referencing the pattern.dateAfter and pattern.dateBefore checks where the
disabled prop is set.
In `@admin-ui/plugins/auth-server/components/Configuration/Keys/KeysPage.tsx`:
- Around line 7-12: The imports in KeysPage.tsx mix alias-based and non-alias
route paths; update the GluuViewWrapper and GluuFormFooter imports (symbols:
GluuViewWrapper, GluuFormFooter) to use the same alias-based route style as
applicationStyle (e.g., change their import paths from "Routes/Apps/Gluu/..." to
"@/routes/Apps/Gluu/...") so all route imports are consistent across the file.
In `@admin-ui/plugins/user-management/components/User2FADevicesModal.tsx`:
- Around line 192-219: The handleRemove2Fa useCallback lists otpDevicesList in
its dependency array but only uses setOTPDevicesList with a functional updater,
so remove otpDevicesList from the dependencies to avoid unnecessary recreations;
update the dependency array for handleRemove2Fa to [userDetails,
deleteFido2Mutation, updateUserData] (keeping the other referenced symbols
intact).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (170)
admin-ui/__tests__/setup.tsadmin-ui/app/components/Accordion/AccordionBody.tsxadmin-ui/app/components/Accordion/AccordionIndicator.tsxadmin-ui/app/components/Accordion/index.tsadmin-ui/app/components/App/index.tsadmin-ui/app/components/Avatar/Avatar.tsxadmin-ui/app/components/Avatar/index.tsadmin-ui/app/components/Card/Card.tsxadmin-ui/app/components/Card/index.tsadmin-ui/app/components/CardHeader/index.tsadmin-ui/app/components/CustomInput/CustomInput.tsxadmin-ui/app/components/CustomInput/index.tsadmin-ui/app/components/Divider/Divider.tsxadmin-ui/app/components/Divider/index.tsadmin-ui/app/components/GluuButton/GluuButton.tsxadmin-ui/app/components/GluuDatePicker/GluuDatePicker.style.tsadmin-ui/app/components/GluuDetailGrid/GluuDetailGrid.style.tsadmin-ui/app/components/GluuDetailGrid/GluuDetailGrid.tsxadmin-ui/app/components/GluuDetailGrid/__tests__/GluuDetailGrid.test.tsxadmin-ui/app/components/GluuDetailGrid/index.tsadmin-ui/app/components/GluuDetailGrid/types.tsadmin-ui/app/components/GluuInlineAlert/GluuInlineAlert.style.tsadmin-ui/app/components/GluuInlineAlert/GluuInlineAlert.tsxadmin-ui/app/components/GluuInlineAlert/index.tsadmin-ui/app/components/GluuInlineAlert/types.tsadmin-ui/app/components/GluuSearchToolbar/GluuSearchToolbar.tsxadmin-ui/app/components/GluuSearchToolbar/types.tsadmin-ui/app/components/GluuSpinner/GluuSpinner.style.tsadmin-ui/app/components/GluuSpinner/GluuSpinner.tsxadmin-ui/app/components/GluuTable/GluuTable.style.tsadmin-ui/app/components/GluuTable/GluuTable.tsxadmin-ui/app/components/GluuTable/types.tsadmin-ui/app/components/Layout/LayoutContent.tsxadmin-ui/app/components/Widgets/GroupedButtons/Counter.tsxadmin-ui/app/components/Widgets/GroupedButtons/styles/Counter.style.tsadmin-ui/app/constants/ui.tsadmin-ui/app/context/theme/config.tsadmin-ui/app/i18n.tsadmin-ui/app/index.tsxadmin-ui/app/locales/en/translation.jsonadmin-ui/app/locales/es/translation.jsonadmin-ui/app/locales/fr/translation.jsonadmin-ui/app/locales/pt/translation.jsonadmin-ui/app/redux/types/index.tsadmin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsxadmin-ui/app/routes/Apps/Gluu/GluuCommitFooter.tsxadmin-ui/app/routes/Apps/Gluu/GluuFormDetailRow.tsxadmin-ui/app/routes/Apps/Gluu/GluuFormFooter.tsxadmin-ui/app/routes/Apps/Gluu/GluuInfo.tsxadmin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsxadmin-ui/app/routes/Apps/Gluu/GluuInputEditor.tsxadmin-ui/app/routes/Apps/Gluu/GluuInputRow.tsxadmin-ui/app/routes/Apps/Gluu/GluuLabel.tsxadmin-ui/app/routes/Apps/Gluu/GluuRemovableInputRow.tsxadmin-ui/app/routes/Apps/Gluu/GluuRemovableSelectRow.tsxadmin-ui/app/routes/Apps/Gluu/GluuRemovableTypeAhead.tsxadmin-ui/app/routes/Apps/Gluu/GluuScriptErrorModal.tsxadmin-ui/app/routes/Apps/Gluu/GluuTypeAheadWithAdd.tsxadmin-ui/app/routes/Apps/Gluu/GluuWebhookErrorDialog.tsxadmin-ui/app/routes/Apps/Gluu/Tests/GluuTypeAheadWithAdd.test.tsxadmin-ui/app/routes/Apps/Gluu/styles/GluuCommitDialog.style.tsadmin-ui/app/routes/Apps/Gluu/styles/GluuScriptErrorModal.style.tsadmin-ui/app/routes/Apps/Gluu/styles/applicationStyle.tsxadmin-ui/app/routes/Apps/Profile/ProfilePage.style.tsadmin-ui/app/routes/Apps/Profile/ProfilePage.tsxadmin-ui/app/routes/Dashboards/DashboardPage.style.tsadmin-ui/app/routes/Dashboards/DashboardPage.tsxadmin-ui/app/routes/Dashboards/Reports/ReportPiChartItem.tsxadmin-ui/app/routes/Dashboards/components/UserInfoItem.tsxadmin-ui/app/styles/main.scssadmin-ui/app/utils/hooks/useWebhookDialogAction.tsxadmin-ui/app/utils/queryUtils.tsadmin-ui/jest.config.tsadmin-ui/plugins/admin/__tests__/api/CustomScripts.test.tsadmin-ui/plugins/admin/__tests__/api/setup.test.tsadmin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptAddPage.test.tsxadmin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptDetailPage.test.tsxadmin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptEditPage.test.tsxadmin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptForm.test.tsxadmin-ui/plugins/admin/__tests__/components/CustomScripts/item.test.tsadmin-ui/plugins/admin/__tests__/components/CustomScripts/script.test.tsadmin-ui/plugins/admin/components/Assets/types/FormTypes.tsadmin-ui/plugins/admin/components/Audit/AuditListPage.style.tsadmin-ui/plugins/admin/components/Audit/AuditListPage.tsxadmin-ui/plugins/admin/components/CustomScripts/CustomScriptAddPage.tsxadmin-ui/plugins/admin/components/CustomScripts/CustomScriptDetailPage.tsxadmin-ui/plugins/admin/components/CustomScripts/CustomScriptEditPage.tsxadmin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsxadmin-ui/plugins/admin/components/CustomScripts/CustomScriptListPage.tsxadmin-ui/plugins/admin/components/CustomScripts/PersonAuthenticationFields.tsxadmin-ui/plugins/admin/components/CustomScripts/constants.tsadmin-ui/plugins/admin/components/CustomScripts/helper/utils.tsadmin-ui/plugins/admin/components/CustomScripts/helper/validations.tsadmin-ui/plugins/admin/components/Health/components/ServiceStatusCard.tsxadmin-ui/plugins/admin/components/Webhook/ShortcodePopover.tsxadmin-ui/plugins/admin/components/Webhook/WebhookListPage.tsxadmin-ui/plugins/admin/components/Webhook/styles/WebhookFormPage.style.tsadmin-ui/plugins/admin/components/Webhook/styles/WebhookListPage.style.tsadmin-ui/plugins/admin/helper/assets.tsadmin-ui/plugins/admin/redux/api/ScriptApi.tsadmin-ui/plugins/admin/redux/api/types/ScriptApiTypes.tsadmin-ui/plugins/admin/redux/features/customScriptSlice.tsadmin-ui/plugins/admin/redux/features/types/customScript.tsadmin-ui/plugins/admin/redux/features/types/index.tsadmin-ui/plugins/admin/redux/sagas/CustomScriptSaga.tsadmin-ui/plugins/admin/redux/sagas/WebhookSaga.tsadmin-ui/plugins/admin/redux/sagas/types/audit.tsadmin-ui/plugins/admin/redux/sagas/types/customScript.tsadmin-ui/plugins/admin/redux/sagas/types/index.tsadmin-ui/plugins/admin/redux/sagas/types/state.tsadmin-ui/plugins/auth-server/__tests__/api/AuthN.test.jsadmin-ui/plugins/auth-server/components/Agama/AgamaAliasListPage.tsxadmin-ui/plugins/auth-server/components/Agama/AgamaListPage.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsxadmin-ui/plugins/auth-server/components/AuthN/index.tsxadmin-ui/plugins/auth-server/components/Clients/ClientActiveTokens.jsadmin-ui/plugins/auth-server/components/Clients/ClientCibaParUmaPanel.jsadmin-ui/plugins/auth-server/components/Clients/ClientListPage.jsadmin-ui/plugins/auth-server/components/Clients/ClientWizardForm.jsadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ConfigApiPage.tsxadmin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.tsxadmin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsxadmin-ui/plugins/auth-server/components/Configuration/Keys/KeysPage.tsxadmin-ui/plugins/auth-server/components/Message/LockPage.jsadmin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsxadmin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsxadmin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsxadmin-ui/plugins/auth-server/components/Sessions/SessionListPage.tsxadmin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsxadmin-ui/plugins/auth-server/components/Ssa/SsaListPage.tsxadmin-ui/plugins/auth-server/plugin-metadata.jsadmin-ui/plugins/fido/components/Fido.tsxadmin-ui/plugins/jans-lock/components/JansLock.tsxadmin-ui/plugins/saml/components/SamlPage.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsxadmin-ui/plugins/schema/components/Person/AttributeAddPage.tsxadmin-ui/plugins/schema/components/Person/AttributeEditPage.tsxadmin-ui/plugins/schema/components/Person/AttributeListPage.tsxadmin-ui/plugins/schema/components/Person/AttributeViewPage.tsxadmin-ui/plugins/scim/components/ScimPage.tsxadmin-ui/plugins/scripts/components/CustomScripts/CustomScriptAddPage.tsxadmin-ui/plugins/scripts/components/CustomScripts/CustomScriptEditPage.tsxadmin-ui/plugins/scripts/components/CustomScripts/CustomScriptForm.tsxadmin-ui/plugins/scripts/components/CustomScripts/CustomScriptListPage.tsxadmin-ui/plugins/scripts/components/CustomScripts/PersonAuthenticationFields.tsxadmin-ui/plugins/scripts/components/CustomScripts/ScriptListPage.tsxadmin-ui/plugins/scripts/components/CustomScripts/constants.tsadmin-ui/plugins/scripts/components/CustomScripts/helper/auditUtils.tsadmin-ui/plugins/scripts/components/CustomScripts/helper/index.tsadmin-ui/plugins/scripts/components/CustomScripts/helper/utils.tsadmin-ui/plugins/scripts/components/CustomScripts/helper/validations.tsadmin-ui/plugins/scripts/components/CustomScripts/hooks/index.tsadmin-ui/plugins/scripts/components/CustomScripts/hooks/useCustomScriptActions.tsadmin-ui/plugins/scripts/components/CustomScripts/hooks/useCustomScriptApi.tsadmin-ui/plugins/scripts/components/CustomScripts/hooks/useMutationEffects.tsadmin-ui/plugins/scripts/components/CustomScripts/styles/CustomScriptFormPage.style.tsadmin-ui/plugins/scripts/components/CustomScripts/styles/CustomScriptListPage.style.tsadmin-ui/plugins/scripts/components/CustomScripts/types/actions.tsadmin-ui/plugins/scripts/components/CustomScripts/types/customScript.tsadmin-ui/plugins/scripts/components/CustomScripts/types/forms.tsadmin-ui/plugins/scripts/components/CustomScripts/types/index.tsadmin-ui/plugins/scripts/plugin-metadata.tsadmin-ui/plugins/services/Components/Configuration/CachePage.tsxadmin-ui/plugins/services/Components/Configuration/LdapListPage.tsxadmin-ui/plugins/services/Components/Configuration/PersistenceDetail.tsxadmin-ui/plugins/smtp-management/components/SmtpManagement/SmtpEditPage.tsxadmin-ui/plugins/user-management/components/User2FADevicesModal.tsxadmin-ui/plugins/user-management/components/UserList.tsx
💤 Files with no reviewable changes (29)
- admin-ui/app/components/Divider/index.ts
- admin-ui/plugins/admin/tests/components/CustomScripts/item.test.ts
- admin-ui/plugins/admin/components/CustomScripts/constants.ts
- admin-ui/app/redux/types/index.ts
- admin-ui/plugins/admin/components/CustomScripts/CustomScriptDetailPage.tsx
- admin-ui/plugins/admin/redux/features/types/index.ts
- admin-ui/plugins/admin/redux/sagas/types/state.ts
- admin-ui/plugins/admin/components/CustomScripts/CustomScriptEditPage.tsx
- admin-ui/app/components/CardHeader/index.ts
- admin-ui/plugins/auth-server/tests/api/AuthN.test.js
- admin-ui/plugins/admin/redux/api/ScriptApi.ts
- admin-ui/plugins/admin/redux/api/types/ScriptApiTypes.ts
- admin-ui/plugins/admin/tests/api/CustomScripts.test.ts
- admin-ui/plugins/admin/components/CustomScripts/helper/utils.ts
- admin-ui/plugins/admin/components/CustomScripts/PersonAuthenticationFields.tsx
- admin-ui/app/components/CustomInput/index.ts
- admin-ui/plugins/admin/components/CustomScripts/helper/validations.ts
- admin-ui/plugins/scripts/components/CustomScripts/types/forms.ts
- admin-ui/plugins/admin/components/CustomScripts/CustomScriptAddPage.tsx
- admin-ui/plugins/admin/components/CustomScripts/CustomScriptListPage.tsx
- admin-ui/plugins/admin/redux/sagas/types/customScript.ts
- admin-ui/app/components/App/index.ts
- admin-ui/plugins/admin/redux/features/customScriptSlice.ts
- admin-ui/plugins/admin/redux/sagas/CustomScriptSaga.ts
- admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx
- admin-ui/plugins/admin/tests/components/CustomScripts/CustomScriptDetailPage.test.tsx
- admin-ui/plugins/admin/redux/sagas/types/index.ts
- admin-ui/app/components/Card/index.ts
- admin-ui/plugins/admin/redux/features/types/customScript.ts
Comment on lines
+32
to
33
| msg.includes('not wrapped in act(') || | ||
| (msg.includes('Failed prop type') && msg.includes('timeout') && msg.includes('Fade')) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n admin-ui/__tests__/setup.tsRepository: GluuFederation/flex
Length of output: 2065
🏁 Script executed:
# Search for act() related issues or comments in the codebase
rg "act\(" admin-ui/__tests__ -B 2 -A 2 | head -50
# Check for any TODO or FIXME comments related to this
rg "act|async.*test|wrapped in act" admin-ui -i | grep -i "todo\|fixme\|hack\|workaround" | head -20
# Look for other setup files or similar patterns
find admin-ui -name "*.ts" -path "*__tests__*" -o -name "setup*" | head -20Repository: GluuFederation/flex
Length of output: 1720
🏁 Script executed:
# Check if there's a setup-tests.ts file that might have related context
cat -n admin-ui/__tests__/setup-tests.ts 2>/dev/null | head -50
# Search for any issues or discussions about act() warnings in comments
rg "act\(" -B 5 -A 5 admin-ui --type ts --type js | grep -i "issue\|problem\|warning\|suppress" | head -20
# Check git log for when this suppression was added
git log -p --all -S "not wrapped in act(" -- admin-ui/__tests__/setup.ts | head -100Repository: GluuFederation/flex
Length of output: 506
🏁 Script executed:
# Get more details from git log about when act() suppression was added
git log --all --oneline -- admin-ui/__tests__/setup.ts | head -20
# Check if there are any test failures or skipped tests related to act()
rg "skip\|xdescribe\|xit\|\.skip" admin-ui/__tests__ -B 2 -A 2 | head -40
# Search for any act() usage in actual tests to see if warnings are being actively triggered
rg "act\(" admin-ui --type ts --type js | grep -v "setup.ts\|node_modules" | head -20
# Check if there are any comments or documentation about this setup
rg "console.error\|suppress\|skip.*warning" admin-ui/__tests__ -B 2 -A 2Repository: GluuFederation/flex
Length of output: 368
🏁 Script executed:
# Search for test patterns using waitFor, act, or async operations
rg "waitFor|userEvent|fireEvent" admin-ui/__tests__ --type ts --type js | wc -l
# Check if tests are actually structured to handle async properly
rg "async\s*\(" admin-ui/__tests__ --type ts --type js | head -20
# Look for any comments in test files about act() or async issues
rg "act\|async\|Warning" admin-ui/__tests__ --type ts --type js -B 1 -A 1 | grep -E "//|/\*|\*/" | head -30
# Check if there are any recent issues or PRs discussing this
git log --all --grep="act\|async\|test" --oneline | head -20Repository: GluuFederation/flex
Length of output: 1767
🏁 Script executed:
# Look at actual test files to see async patterns and if they properly use act()
find admin-ui/__tests__ -name "*.test.ts" -o -name "*.test.js" | head -5 | xargs -I {} bash -c 'echo "=== {} ===" && head -50 {}'
# Check if tests are using React Testing Library properly
rg "render\|screen\|within" admin-ui/__tests__ --type ts --type js | wc -l
# Look for any potential unhandled async issues in tests
rg "setState\|useEffect\|useState" admin-ui/__tests__ --type ts --type js | head -10Repository: GluuFederation/flex
Length of output: 64
🏁 Script executed:
# List test files to understand the testing structure
find admin-ui -name "*.test.ts" -o -name "*.test.js" -o -name "*.test.tsx" | head -10
# Check a specific test file to see the pattern
find admin-ui -name "*.test.ts" -type f | head -1 | xargs cat | head -60
# Search for React Query or async test patterns
rg "useQuery|useMutation|waitFor" admin-ui --type ts --type js | head -15Repository: GluuFederation/flex
Length of output: 4274
🏁 Script executed:
# Look at a component test to see async patterns
cat -n admin-ui/app/routes/Apps/Gluu/Tests/GluuTooltip.test.tsx
# Search for act() calls or warnings about act() in test output comments
rg "act\(" admin-ui --type ts --type js -B 3 -A 3 | head -40
# Check the React version to understand the context
rg "react.*version\|\"react\":" admin-ui/package.jsonRepository: GluuFederation/flex
Length of output: 2564
🏁 Script executed:
# Check if there's any evidence of why this suppression was needed
# Look for any test-related issues or discussions
git log --all -p -- admin-ui/__tests__/setup.ts | grep -A 20 -B 20 "act(" | head -100
# Search for any test failures or issues with act() warnings in recent commits
git log --all --oneline --grep="test\|act\|warning" | head -30
# Check if tests are properly wrapped with act() or waitFor
rg "waitFor\|userEvent\|fireEvent" admin-ui/__tests__ admin-ui/app --type ts --type tsx | wc -lRepository: GluuFederation/flex
Length of output: 3860
🏁 Script executed:
# Check if there are any test files that specifically deal with async operations
rg "userEvent\|fireEvent\|waitFor" admin-ui --type tsx --type ts | wc -l
# Look for any evidence that act() warnings were being triggered before suppression
# Check if there are any comments or TODOs about this
rg "act\(\)|Warning.*act|act.*warning" admin-ui -i -B 2 -A 2
# Examine the specific commit that added this suppression
git log --all --format="%H %s" -- admin-ui/__tests__/setup.ts | head -5 | while read hash subject; do
echo "=== $subject ==="
git show --stat $hash | head -20
doneRepository: GluuFederation/flex
Length of output: 2993
🏁 Script executed:
# Check if there are any test configuration or jest setup improvements in recent history
git log --all --oneline -20 | grep -i "test\|jest\|setup"
# See if there's a PR or branch info that might explain the act() suppression
git log --all --format="%H %s %b" -- admin-ui/__tests__/setup.ts | grep -i "act\|async\|warning" -A 5 -B 5 | head -50
# Check if the test suite has any skipped or xfailing tests related to act
rg "skip\|xdescribe\|xit" admin-ui/__tests__ -l | xargs -I {} bash -c 'echo "=== {} ===" && grep -A 2 -B 2 "skip\|xdescribe\|xit" {}'Repository: GluuFederation/flex
Length of output: 140
Remove the global act() warning suppression.
Suppressing React's "not wrapped in act()" warnings masks real async test defects. Tests should be fixed to properly handle state updates with waitFor(), userEvent, or other React Testing Library patterns rather than hiding the warning.
Suggested fix
- msg.includes('not wrapped in act(') ||📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| msg.includes('not wrapped in act(') || | |
| (msg.includes('Failed prop type') && msg.includes('timeout') && msg.includes('Fade')) | |
| (msg.includes('Failed prop type') && msg.includes('timeout') && msg.includes('Fade')) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/__tests__/setup.ts` around lines 32 - 33, The current global
suppression in admin-ui/__tests__/setup.ts filters out React's "not wrapped in
act()" warnings (the msg.includes('not wrapped in act(') branch) which hides
real async test issues; remove that suppression and update the tests producing
the warnings to properly await state updates using React Testing Library
utilities (e.g., waitFor, findBy*, or userEvent async flows) or wrap the
specific updates in act() where appropriate so state changes are awaited rather
than globally silencing the error.
Comment on lines
+6
to
+12
| type AccordionComponent = typeof AccordionBase & { | ||
| Body: typeof AccordionBody | ||
| Header: typeof AccordionHeader | ||
| Indicator: typeof AccordionIndicator | ||
| } | ||
|
|
||
| const Accordion = AccordionBase as AccordionComponent |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer the shared AccordionComponent type to avoid type drift.
This file redefines a type that already exists in admin-ui/app/components/Accordion/Accordion.d.ts (Line 25-29). Reusing the shared type keeps the public API contract in one place.
♻️ Proposed refactor
import { Accordion as AccordionBase } from './Accordion'
import { AccordionBody } from './AccordionBody'
import { AccordionHeader } from './AccordionHeader'
import { AccordionIndicator } from './AccordionIndicator'
+import type { AccordionComponent } from './Accordion.d'
-type AccordionComponent = typeof AccordionBase & {
- Body: typeof AccordionBody
- Header: typeof AccordionHeader
- Indicator: typeof AccordionIndicator
-}
-
const Accordion = AccordionBase as AccordionComponent
Accordion.Body = AccordionBody
Accordion.Header = AccordionHeader
Accordion.Indicator = AccordionIndicator📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type AccordionComponent = typeof AccordionBase & { | |
| Body: typeof AccordionBody | |
| Header: typeof AccordionHeader | |
| Indicator: typeof AccordionIndicator | |
| } | |
| const Accordion = AccordionBase as AccordionComponent | |
| import { Accordion as AccordionBase } from './Accordion' | |
| import { AccordionBody } from './AccordionBody' | |
| import { AccordionHeader } from './AccordionHeader' | |
| import { AccordionIndicator } from './AccordionIndicator' | |
| import type { AccordionComponent } from './Accordion.d' | |
| const Accordion = AccordionBase as AccordionComponent | |
| Accordion.Body = AccordionBody | |
| Accordion.Header = AccordionHeader | |
| Accordion.Indicator = AccordionIndicator |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/app/components/Accordion/index.ts` around lines 6 - 12, The local
redefinition of AccordionComponent should be removed and the shared
AccordionComponent type reused: import the existing AccordionComponent type and
then cast AccordionBase using that imported type (keep references to
AccordionBase, AccordionBody, AccordionHeader, AccordionIndicator), instead of
declaring a new type locally; delete the local type alias and replace "const
Accordion = AccordionBase as AccordionComponent" with a cast to the
imported/shared AccordionComponent to ensure the public API type is consistent.
Comment on lines
+13
to
+21
| return ( | ||
| <RSCustomInput | ||
| data-testid={name} | ||
| className={inputClass} | ||
| label={label} | ||
| name={name} | ||
| {...otherProps} | ||
| /> | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider handling undefined name for data-testid.
When name is undefined, the data-testid attribute will also be undefined, which could affect test selectors that depend on this attribute. Consider providing a fallback or omitting the attribute when name is not provided.
💡 Proposed fix
return (
<RSCustomInput
- data-testid={name}
+ data-testid={name ?? undefined}
className={inputClass}
label={label}
name={name}
{...otherProps}
/>
)Or conditionally spread the attribute:
return (
<RSCustomInput
{...(name && { 'data-testid': name })}
className={inputClass}
label={label}
name={name}
{...otherProps}
/>
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return ( | |
| <RSCustomInput | |
| data-testid={name} | |
| className={inputClass} | |
| label={label} | |
| name={name} | |
| {...otherProps} | |
| /> | |
| ) | |
| return ( | |
| <RSCustomInput | |
| data-testid={name ?? undefined} | |
| className={inputClass} | |
| label={label} | |
| name={name} | |
| {...otherProps} | |
| /> | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/app/components/CustomInput/CustomInput.tsx` around lines 13 - 21,
The data-testid prop on RSCustomInput can be undefined when name is undefined;
update CustomInput.tsx to avoid rendering data-testid when name is falsy by
conditionally including that attribute (e.g., only spread {'data-testid': name}
when name is truthy) so RSCustomInput receives data-testid only for valid names;
keep the rest of the props (className=inputClass, label, name, ...otherProps)
unchanged.
Comment on lines
+16
to
+36
| it('renders all field labels and values', () => { | ||
| render(<GluuDetailGrid fields={mockFields} />, { wrapper: Wrapper }) | ||
| expect(screen.getByText(/Inum/i)).toBeInTheDocument() | ||
| expect(screen.getByText(/Name/i)).toBeInTheDocument() | ||
| expect(screen.getByText(/Script Type/i)).toBeInTheDocument() | ||
| expect(screen.getByText('09A0-93D7')).toBeInTheDocument() | ||
| expect(screen.getByText('smpp')).toBeInTheDocument() | ||
| expect(screen.getByText('person_authentication')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('renders with defaultDocCategory', () => { | ||
| render(<GluuDetailGrid fields={mockFields} defaultDocCategory="SCRIPT" />, { | ||
| wrapper: Wrapper, | ||
| }) | ||
| expect(screen.getByText('smpp')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('renders with labelStyle', () => { | ||
| render(<GluuDetailGrid fields={mockFields} labelStyle={{ color: 'red' }} />, { wrapper: Wrapper }) | ||
| expect(screen.getByText(/Inum/i)).toBeInTheDocument() | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider wrapping tests in a describe block and expanding coverage.
The tests provide basic validation but could be improved:
- Wrap tests in a
describe('GluuDetailGrid', () => { ... })block for better organization and reporting. - Consider adding tests for:
- Empty
fieldsarray handling - The
layoutandclassNameprops - Edge cases with missing/undefined field values
- Empty
♻️ Suggested structure
+describe('GluuDetailGrid', () => {
-it('renders all field labels and values', () => {
+ it('renders all field labels and values', () => {
render(<GluuDetailGrid fields={mockFields} />, { wrapper: Wrapper })
// ... assertions
-})
+ })
-it('renders with defaultDocCategory', () => {
+ it('renders with defaultDocCategory', () => {
// ...
-})
+ })
-it('renders with labelStyle', () => {
+ it('renders with labelStyle', () => {
// ...
+ })
+
+ it('handles empty fields array', () => {
+ render(<GluuDetailGrid fields={[]} />, { wrapper: Wrapper })
+ // Assert graceful empty state
+ })
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/app/components/GluuDetailGrid/__tests__/GluuDetailGrid.test.tsx`
around lines 16 - 36, Wrap the existing tests in a describe('GluuDetailGrid', ()
=> { ... }) block and add unit tests in the same file that cover: rendering when
fields=[] (expect no labels/values or a placeholder), passing different layout
values to the GluuDetailGrid component (verify DOM layout/class changes),
providing a className prop (assert it appears on the root element), and handling
missing/undefined field values in mockFields (assert no crashes and appropriate
fallback UI). Use the existing mockFields and Wrapper to build these tests and
reference the GluuDetailGrid component and its props (fields,
defaultDocCategory, labelStyle, layout, className) when locating where to add
the new test cases.
Comment on lines
+1
to
+2
| export { default } from './GluuInlineAlert' | ||
| export type { GluuInlineAlertProps, GluuInlineAlertSeverity } from './types' |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a named export for consistency with GluuDetailGrid.
GluuDetailGrid/index.ts exports the component as a named export (export { default as GluuDetailGrid }), while this file uses a default re-export (export { default }). This creates an import style inconsistency:
import { GluuDetailGrid } from '@/components/GluuDetailGrid'import GluuInlineAlert from '@/components/GluuInlineAlert'
Consider aligning the export pattern for consistency.
♻️ Suggested change
-export { default } from './GluuInlineAlert'
+export { default as GluuInlineAlert } from './GluuInlineAlert'
export type { GluuInlineAlertProps, GluuInlineAlertSeverity } from './types'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export { default } from './GluuInlineAlert' | |
| export type { GluuInlineAlertProps, GluuInlineAlertSeverity } from './types' | |
| export { default as GluuInlineAlert } from './GluuInlineAlert' | |
| export type { GluuInlineAlertProps, GluuInlineAlertSeverity } from './types' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/app/components/GluuInlineAlert/index.ts` around lines 1 - 2, The
current file re-exports the component as a default export which causes
inconsistent import styles vs GluuDetailGrid; change the re-export to a named
export so consumers can import { GluuInlineAlert } consistently — update the
component export to re-export the default under the name GluuInlineAlert (leave
the type re-exports like GluuInlineAlertProps and GluuInlineAlertSeverity
intact) so imports mirror GluuDetailGrid's pattern.
Comment on lines
63
to
67
| error instanceof Error | ||
| ? error.message | ||
| : error && typeof error === 'object' && 'message' in error | ||
| ? String((error as { message: unknown }).message) | ||
| ? String((error as { message: string }).message) | ||
| : t(errorMessage) |
Contributor
There was a problem hiding this comment.
Avoid showing "undefined"/"null" in error toasts.
Line 66 currently stringifies any message value, which can surface "undefined"/"null" instead of the translated fallback.
Suggested fix
- const errorMsg =
- error instanceof Error
- ? error.message
- : error && typeof error === 'object' && 'message' in error
- ? String((error as { message: string }).message)
- : t(errorMessage)
+ const rawMessage =
+ error instanceof Error
+ ? error.message
+ : error && typeof error === 'object' && 'message' in error
+ ? (error as { message?: unknown }).message
+ : undefined
+
+ const errorMsg =
+ typeof rawMessage === 'string' && rawMessage.trim().length > 0
+ ? rawMessage
+ : t(errorMessage)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@admin-ui/plugins/scripts/components/CustomScripts/hooks/useMutationEffects.ts`
around lines 63 - 67, The current ternary may stringify null/undefined messages;
update the selection so we only use a non-empty string message from error
objects—e.g. keep error instanceof Error ? error.message, but for the object
branch require typeof (error as any).message === 'string' && (error as
any).message.trim() !== '' before using it; otherwise fall back to
t(errorMessage). Modify the expression around the error object check in
useMutationEffects (the ternary that uses String((error as { message: string
}).message)) to validate the message is a non-empty string before using it and
otherwise call t(errorMessage).
Comment on lines
+48
to
+57
| const handleAliasChange = useCallback( | ||
| (e: ChangeEvent<HTMLInputElement>) => { | ||
| const target = e.target | ||
| if (target instanceof HTMLSelectElement) { | ||
| const values = Array.from(target.selectedOptions).map((o) => o.value) | ||
| formik.setFieldValue('aliases', values) | ||
| } | ||
| }, | ||
| [formik], | ||
| ) |
Contributor
There was a problem hiding this comment.
Type guard in handleAliasChange may never match.
The event parameter is typed as ChangeEvent<HTMLInputElement>, but the type guard checks for HTMLSelectElement. Since the <Input type="select"> component renders a <select> element, the event target will be HTMLSelectElement, but the TypeScript type doesn't reflect this.
Proposed fix
const handleAliasChange = useCallback(
- (e: ChangeEvent<HTMLInputElement>) => {
- const target = e.target
- if (target instanceof HTMLSelectElement) {
- const values = Array.from(target.selectedOptions).map((o) => o.value)
- formik.setFieldValue('aliases', values)
- }
+ (e: ChangeEvent<HTMLSelectElement>) => {
+ const values = Array.from(e.target.selectedOptions).map((o) => o.value)
+ formik.setFieldValue('aliases', values)
},
[formik],
)Based on learnings: "In React components using Formik, keeping the entire formik object in the dependency array is the recommended pattern."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleAliasChange = useCallback( | |
| (e: ChangeEvent<HTMLInputElement>) => { | |
| const target = e.target | |
| if (target instanceof HTMLSelectElement) { | |
| const values = Array.from(target.selectedOptions).map((o) => o.value) | |
| formik.setFieldValue('aliases', values) | |
| } | |
| }, | |
| [formik], | |
| ) | |
| const handleAliasChange = useCallback( | |
| (e: ChangeEvent<HTMLSelectElement>) => { | |
| const values = Array.from(e.target.selectedOptions).map((o) => o.value) | |
| formik.setFieldValue('aliases', values) | |
| }, | |
| [formik], | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@admin-ui/plugins/scripts/components/CustomScripts/PersonAuthenticationFields.tsx`
around lines 48 - 57, The handler handleAliasChange is typed as
ChangeEvent<HTMLInputElement> but checks for HTMLSelectElement so the type guard
never matches; change the event type to ChangeEvent<HTMLSelectElement> (or a
union that includes HTMLSelectElement) so the target instanceof
HTMLSelectElement check is valid, extract selectedOptions into values and call
formik.setFieldValue('aliases', values) as before, and update the useCallback
dependency to include the entire formik object instead of just formik
properties; refer to handleAliasChange, formik, and setFieldValue when making
these changes.
Comment on lines
+74
to
+83
| content: { | ||
| paddingTop: SPACING.PAGE, | ||
| paddingLeft: CONTENT_HORIZONTAL_PADDING, | ||
| paddingRight: CONTENT_HORIZONTAL_PADDING, | ||
| paddingBottom: SPACING.CONTENT_PADDING, | ||
| width: WIDTH_FULL, | ||
| boxSizing: BOX_SIZING_BORDER, | ||
| display: DISPLAY_FLEX, | ||
| flexDirection: FLEX_DIRECTION_COLUMN, | ||
| gap: CONTENT_GAP, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add small-screen horizontal padding override for responsive layout.
The content container uses fixed 52px horizontal padding, which is tight for narrow viewports and works against the PR’s responsiveness goal.
Suggested fix
content: {
paddingTop: SPACING.PAGE,
paddingLeft: CONTENT_HORIZONTAL_PADDING,
paddingRight: CONTENT_HORIZONTAL_PADDING,
paddingBottom: SPACING.CONTENT_PADDING,
width: WIDTH_FULL,
boxSizing: BOX_SIZING_BORDER,
display: DISPLAY_FLEX,
flexDirection: FLEX_DIRECTION_COLUMN,
gap: CONTENT_GAP,
+ [theme.breakpoints.down('sm')]: {
+ paddingLeft: SPACING.PAGE,
+ paddingRight: SPACING.PAGE,
+ },
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@admin-ui/plugins/scripts/components/CustomScripts/styles/CustomScriptFormPage.style.ts`
around lines 74 - 83, The content style block uses a fixed horizontal padding
(CONTENT_HORIZONTAL_PADDING) causing cramped layout on small screens; update the
CustomScriptFormPage style to add a small-screen override (e.g., via a media
query or responsive breakpoint) that reduces paddingLeft and paddingRight for
the content object when viewport width is below the small breakpoint so the
container remains usable on narrow viewports; target the content style
definition and adjust padding via a responsive rule referencing
CONTENT_HORIZONTAL_PADDING (or a new SMALL_SCREEN_HORIZONTAL_PADDING constant)
to apply smaller horizontal padding on small screens.
Comment on lines
+204
to
+210
| '&:focus, &:active': { | ||
| opacity: 1, | ||
| backgroundColor: themeColors.errorColor, | ||
| color: customColors.white, | ||
| outline: OUTLINE_NONE, | ||
| boxShadow: OUTLINE_NONE, | ||
| }, |
Contributor
There was a problem hiding this comment.
Do not remove visible keyboard focus indicators.
Lines 204-210, Line 304-311, and Line 397-403 remove focus outlines/box shadows without replacement. This breaks keyboard navigability visibility and is an accessibility blocker.
Suggested fix
@@
'&:focus, &:active': {
opacity: 1,
backgroundColor: themeColors.errorColor,
color: customColors.white,
- outline: OUTLINE_NONE,
- boxShadow: OUTLINE_NONE,
+ outline: `2px solid ${theme.palette.primary.main}`,
+ outlineOffset: 1,
},
@@
'& input:focus, & input:active, & select:focus, & select:active, & .custom-select:focus, & .custom-select:active':
{
backgroundColor: `${formInputBg} !important`,
color: `${themeColors.fontColor} !important`,
border: `1px solid ${inputBorderColor} !important`,
- outline: OUTLINE_NONE,
- boxShadow: OUTLINE_NONE,
+ outline: `2px solid ${theme.palette.primary.main}`,
+ outlineOffset: 1,
},
@@
'&:focus, &:active': {
backgroundColor: `${headersInputBg} !important`,
color: themeColors.fontColor,
border: `1px solid ${headersBorderColor} !important`,
- outline: OUTLINE_NONE,
- boxShadow: OUTLINE_NONE,
+ outline: `2px solid ${theme.palette.primary.main}`,
+ outlineOffset: 1,
},Also applies to: 304-311, 397-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@admin-ui/plugins/scripts/components/CustomScripts/styles/CustomScriptFormPage.style.ts`
around lines 204 - 210, The focus styles currently set in the '&:focus,
&:active' blocks (they remove outline/boxShadow by using OUTLINE_NONE) eliminate
visible keyboard focus indicators and must be replaced with an accessible
visible focus treatment; update the three occurrences (the shown '&:focus,
&:active' block and the similar blocks at the other two locations) to remove the
OUTLINE_NONE assignments and instead apply a clear, high-contrast focus ring
such as a 2px solid outline or a box-shadow ring using an existing theme token
(e.g., themeColors.focusColor or a semi-opaque variant of
themeColors.errorColor) while preserving the intended
backgroundColor/color/opacity, so keyboard users can see focus without changing
visual intent.
Comment on lines
6
to
+7
| import GluuViewDetailModal from 'Routes/Apps/Gluu/GluuViewDetailsModal' | ||
| import applicationStyle from 'Routes/Apps/Gluu/styles/applicationstyle' | ||
| import applicationStyle from '@/routes/Apps/Gluu/styles/applicationStyle' |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider updating line 6 to match the new import style.
Line 6 still uses the old import style without the @/ alias, while line 7 now uses @/routes. For consistency across the codebase, consider updating line 6:
📁 Suggested consistency fix
-import GluuViewDetailModal from 'Routes/Apps/Gluu/GluuViewDetailsModal'
+import GluuViewDetailModal from '@/routes/Apps/Gluu/GluuViewDetailsModal'
import applicationStyle from '@/routes/Apps/Gluu/styles/applicationStyle'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import GluuViewDetailModal from 'Routes/Apps/Gluu/GluuViewDetailsModal' | |
| import applicationStyle from 'Routes/Apps/Gluu/styles/applicationstyle' | |
| import applicationStyle from '@/routes/Apps/Gluu/styles/applicationStyle' | |
| import GluuViewDetailModal from '@/routes/Apps/Gluu/GluuViewDetailsModal' | |
| import applicationStyle from '@/routes/Apps/Gluu/styles/applicationStyle' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/plugins/user-management/components/User2FADevicesModal.tsx` around
lines 6 - 7, Update the import for GluuViewDetailModal to use the project alias
like the other import: replace the old-style import "import GluuViewDetailModal
from 'Routes/Apps/Gluu/GluuViewDetailsModal'" with the aliased form (using
"@/...") so it matches applicationStyle's
"@/routes/Apps/Gluu/styles/applicationStyle" usage; locate the
GluuViewDetailModal import in User2FADevicesModal.tsx and change it to the
aliased path to keep imports consistent.
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
2 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 Scripts module as per Figma with partial TS migration (#2631)
Summary
This PR revamps the Scripts module to align with the latest Figma designs as part of the ongoing Admin UI redesign initiative. Additionally, a few smaller JavaScript components within this module have been migrated to TypeScript to improve type safety and maintainability.
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: #2631
Summary by CodeRabbit
New Features
Component Improvements
UI/UX Updates