feat(admin-ui): revamp Jans Lock module as per Figma#2705
Closed
faisalsiddique4400 wants to merge 15 commits intomainfrom
Closed
feat(admin-ui): revamp Jans Lock module as per Figma#2705faisalsiddique4400 wants to merge 15 commits intomainfrom
faisalsiddique4400 wants to merge 15 commits intomainfrom
Conversation
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
…ex into admin-ui-issue-2632
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
mo-auto
added
comp-admin-ui
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces comprehensive testing for Cedarling functionality, refactors the Jans-lock module with new form components and styling, migrates the schema plugin from Attribute to UserClaims-based components, adds localization strings across multiple languages, and introduces new UI components including a multi-select row component with theming support. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
📝 Coding Plan
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 |
![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 (1)
admin-ui/plugins/scripts/components/CustomScripts/styles/CustomScriptFormPage.style.ts (1)
320-331: 🧹 Nitpick | 🔵 TrivialAccessibility consideration for removed focus outlines.
The addition of
focus-visibleselectors is a good practice for distinguishing keyboard navigation from mouse clicks. However, removing outlines entirely (even with!important) can make the form difficult to navigate for keyboard users if no alternative visual focus indicator is provided.Ensure that either:
- The border style change on focus (line 324) provides sufficient visual differentiation, or
- A custom focus ring/indicator is applied elsewhere
🤖 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 320 - 331, The current focus rules ('& input:focus, & input:active, & select:focus, & select:active, & .custom-select:focus, & .custom-select:active' and '& input:focus-visible, & select:focus-visible') remove outlines entirely which removes keyboard focus affordance; update the focus-visible rule(s) so keyboard users get a clear visual indicator (e.g. keep or replace outline with a visible style such as a distinct border color/width or a subtle box-shadow focus ring) instead of `outline: none`/`boxShadow: none` — modify the selectors in CustomScriptFormPage.style (the focus/focus-visible rules) to apply that accessible focus ring (or ensure the existing border change on focus is perceptibly stronger) so focus is always visually distinguishable for keyboard navigation.
🤖 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/cedarling/__tests__/client/CedarlingClient.test.ts`:
- Around line 17-20: The test should assert idempotency by verifying the
underlying initialization helper is only invoked once: create a spy (e.g.,
jest.spyOn(CedarlingClient.prototype, '_initialize') or the actual internal
helper used by cedarlingClient.initialize) before the first call, call
cedarlingClient.initialize({}) and expect the spy to have been called once, then
call cedarlingClient.initialize({}) again and expect the spy call count to
remain unchanged (still 1) and the second call to resolve to undefined; restore
the spy after the test.
In `@admin-ui/app/cedarling/__tests__/constants/resourceScopes.test.ts`:
- Around line 40-48: The test titles claim specific scope types but only assert
counts; update the tests referencing CEDAR_RESOURCE_SCOPES.SMTP and
CEDAR_RESOURCE_SCOPES.Dashboard to either (A) rename the it(...) descriptions to
reflect what is actually asserted (e.g., "SMTP has 3 scopes" and "Dashboard has
2 scopes") or (B) strengthen the assertions to verify actual scope strings/types
match the descriptions by asserting the arrays contain the expected scope names
(use expect(...).toEqual(...) or
expect(...).toEqual(expect.arrayContaining([...])) / expect(...).toContain(...)
against the known scope strings such as the SMTP scopes and the Dashboard stat
scope).
- Around line 33-38: The test CEDAR_RESOURCE_SCOPES.Lock currently only checks
length and that some permission contains 'read' or 'lock', which doesn't verify
both read and write scopes exist; update the assertion in the test that uses
lockScopes / permissions to explicitly assert that one permission includes
'read' and another includes 'write' (or adapt to the actual 'lock' naming if
write is represented by 'lock') so the test name "Lock has read and write
scopes" matches the checked conditions.
In `@admin-ui/app/cedarling/__tests__/hooks/useCedarling.test.ts`:
- Around line 185-194: The test in useCedarling.test.ts duplicates the earlier
case by passing an empty array to authorizeHelper; either remove this duplicate
test or change it to pass an actual undefined-like value and update its
description. Specifically, in the test case referencing useCedarling() and
result.current.authorizeHelper, replace the [] argument with undefined (or null)
and assert the expected empty-array result, and update the it(...) title to
"returns empty array for undefined input" (or delete the whole it(...) block if
you prefer to remove the duplicate).
In `@admin-ui/app/cedarling/__tests__/utility/resources.test.ts`:
- Around line 46-48: The test hardcodes 27 in the assertion which duplicates the
length of expectedResources; update the test (the "contains all 27 resources"
spec) to compute the expected length dynamically by comparing
Object.keys(ADMIN_UI_RESOURCES).length to expectedResources.length (or
Object.keys(expectedResources).length) instead of the magic number 27 so the
assertion stays correct when expectedResources changes.
In `@admin-ui/app/locales/en/translation.json`:
- Around line 1143-1155: The translation keys add_attribute, delete_attribute,
edit_attribute, and view_attribute currently use inconsistent casing ("Add User
Claim" vs "Delete user claim"); update their values to sentence case to match
similar entries (e.g., "Add user claim", "Delete user claim", "Edit user claim",
"View user claim") by editing the corresponding entries in translation.json so
all four labels use lowercase 'user claim'.
In `@admin-ui/app/locales/fr/translation.json`:
- Line 774: Update the French translation for the "multi_select_hint" key to
reflect the dropdown interaction instead of typing; replace the current string
that instructs typing with a concise phrase such as "Sélectionnez plusieurs
éléments dans la liste déroulante" (or similar) in the translation.json entry
for "multi_select_hint" so the copy matches the actual multi-select dropdown
control.
In `@admin-ui/app/locales/pt/translation.json`:
- Line 766: Update the "multi_select_hint" translation to reflect the new
click-to-select dropdown behavior (no text entry) by replacing the current
instruction about "inserir" each item with a concise hint that users should
select multiple items from the dropdown (e.g., "Selecione vários itens no menu
suspenso; cada clique adiciona um item"). Locate the "multi_select_hint" key in
the translation.json and change its value to a phrase that explicitly mentions
clicking/selecting from the dropdown rather than typing/inserting.
In `@admin-ui/app/routes/Apps/Gluu/GluuMultiSelectRow.tsx`:
- Around line 165-175: The aria-label on the remove button is hard-coded in
English; update GluuMultiSelectRow.tsx to build the label via i18n so
screen-reader text follows locale: use the component's translation function
(e.g., the useTranslation hook or existing t prop) to produce the label (either
by calling t('remove', { item: getOptionLabel(val) }) or by concatenating
t('remove') + ' ' + getOptionLabel(val)), update the aria-label expression
accordingly, and ensure a corresponding translation key (like "remove" or
"remove_item") exists for all locales; keep handleRemoveChip, getOptionLabel,
and classes.chipRemove references unchanged.
- Around line 140-145: The current inline onBlur on the trigger in
GluuMultiSelectRow.tsx doesn't work because formik.handleBlur reads
event.target.name and blurs from buttons/options outside the trigger never reach
it; move blur logic into a container-level onBlur/onFocusOut handler on the
composite field wrapper (the component that renders the trigger, popup, chips
and listbox) and on blur check via event.relatedTarget or
container.contains(event.relatedTarget) (or use setTimeout+contains) to detect
when focus actually leaves the whole composite; when focus leaves call
formik.handleBlur with a synthetic event whose target/name is the field name
(same name used previously) so Formik marks touched and closes the popup; apply
the same change to the duplicate handlers around lines 187-217.
- Around line 127-157: The combobox div rendered inside GluuMultiSelectRow (the
element using triggerClasses, toggleDropdown, containerRef, name, isOpen,
disabled, and formik.handleBlur) needs ARIA connections: add a stable id to that
combobox element and set aria-labelledby to the label element id, add
aria-controls pointing to the listbox id used for the dropdown, and set
aria-invalid="true" when showError is true; also assign ids to the error and
helper text elements and include them in aria-describedby on the combobox so
screen readers announce validation/help text. Locate the error/helper renderers
(where showError is computed) and give them unique ids, then wire those ids into
aria-describedby, and ensure the listbox component uses the same id referenced
by aria-controls.
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuMultiSelectRow.style.ts`:
- Around line 24-40: selectTrigger currently removes the browser outline and
lacks a keyboard focus indicator; add a dedicated :focus-visible style for
selectTrigger (e.g., "&:focus-visible") that replaces the removed outline with
an accessible visible focus treatment such as changing borderColor and/or adding
a subtle boxShadow ring using an accessible focus color (use existing theme
token or derive from inputBorderColor/fontColor), keep outline: 'none' but
ensure transition is preserved so keyboard focus is visually clear and
consistent with selectTrigger's existing styles.
In `@admin-ui/app/routes/Apps/Gluu/types/GluuMultiSelectRow.types.ts`:
- Around line 6-9: The type alias GluuMultiSelectRowFormik references
React.FocusEvent<HTMLElement> but the file lacks a React type import; add an
explicit type import (e.g., import type React from 'react') or replace
React.FocusEvent with the DOM/React FocusEvent type by importing FocusEvent from
'react' so the GluuMultiSelectRowFormik definition compiles correctly.
In `@admin-ui/plugins/jans-lock/__tests__/components/JansLock.test.tsx`:
- Around line 105-147: Tests only assert that the component didn't crash
(document.body truthy or generic form present); update the three tests to assert
specific outputs for each branch: for the "no read permission" case, mock
useCedarling as done and expect JansLock to render the access-denied UI by
querying for a stable selector or text (e.g., an "access denied" message or a
data-testid you add to the access-denied component); for the "renders loading
state" case, mock useGetLockProperties to return isLoading: true and assert the
loader is present by querying a stable selector (e.g., the GluuLoader's text,
role="status", or a data-testid on the loader); for the "renders with empty
config" case, mock data: undefined and isLoading: false and assert the
empty-state UI is shown by checking for specific empty-state content or a
data-testid; add data-testid attributes to JansLock's access-denied, loader, and
empty-state if no stable selectors exist, and update the tests to query those
identifiers instead of document.body or a generic form.
In
`@admin-ui/plugins/jans-lock/__tests__/components/JansLockConfiguration.test.tsx`:
- Around line 174-188: The test "detects form changes after field modification"
only asserts the input value change; update it to also assert the form
dirty-state side-effect by locating the form submit/apply button associated with
JansLockConfiguration (e.g., the primary action button rendered by the
component) and asserting it transitions from disabled to enabled after changing
the input "baseDN". In the test, after fireEvent.change on input[name="baseDN"]
and the waitFor confirming the value, add an assertion that the button (found
via getByRole('button', { name: /apply|save|submit/i }) or a specific label used
in JansLockConfiguration) was initially disabled and becomes enabled, ensuring
the component's dirty-state is validated.
In
`@admin-ui/plugins/jans-lock/__tests__/components/JansLockFieldRenderer.test.tsx`:
- Around line 192-194: The test currently only checks for '.field-item'
(variable toggle) which verifies layout but not the actual control; update the
assertion to locate the toggle control itself (e.g., use
container.querySelector('input[type="checkbox"]') or screen.getByRole('switch')
to find the renderer's switch element) and assert that that element is in the
document (and optionally has the expected attributes like aria-checked) so the
JansLockFieldRenderer toggle presence is validated directly.
In `@admin-ui/plugins/jans-lock/__tests__/components/validations.test.ts`:
- Around line 35-39: Add explicit tests that pass null for optional numeric and
URI fields (not just empty strings) and assert
schema.validateAt('metricReporterInterval', { metricReporterInterval: null })
resolves to null; do the same for the other cases mentioned (the blocks around
lines 61-65, 94-97, 132-135). Ensure the Yup schema used in these tests marks
those fields as nullable() and omits required() so the schema accepts backend
DTO nulls; locate and update the schema definitions referenced by
schema.validateAt in this test file to include nullable() for the optional
numeric/URI fields.
In `@admin-ui/plugins/jans-lock/components/JansLock.tsx`:
- Around line 41-45: The useEffect in JansLock.tsx currently lists only
authorizeHelper in its dependency array; update the dependency array to include
lockScopes as well (i.e., [authorizeHelper, lockScopes]) so the hook explicitly
depends on both authorizeHelper and lockScopes; locate the useEffect block that
calls authorizeHelper(lockScopes) and add lockScopes to its dependencies to
satisfy exhaustive-deps and document the dependency.
In `@admin-ui/plugins/jans-lock/components/JansLockConfiguration.tsx`:
- Around line 100-109: The submitForm callback currently ignores its
_userMessage parameter; update submitForm and the onUpdate usage so the commit
message is forwarded for audit logging: modify submitForm(_userMessage) to pass
_userMessage along with the patchOperations to onUpdate (e.g., call
onUpdate(patchOperations, _userMessage)), update the onUpdate function/type
signature and all call sites to accept the optional message, and ensure
trimmedValuesAndPatches and patchOperations are still used as before inside
submitForm.
- Around line 95-98: The current tokenChannelOptions useMemo unsafely asserts
lockConfig?.tokenChannels as string[]; update it to defensively handle
unexpected types by checking Array.isArray(lockConfig?.tokenChannels) and
filtering for string entries before mapping. For example, in tokenChannelOptions
replace the assertion with a channels value computed as
Array.isArray(lockConfig?.tokenChannels) ? lockConfig.tokenChannels.filter(item
=> typeof item === 'string') : [] and then map to {value,label} pairs; keep the
useMemo and its dependency on lockConfig?.tokenChannels.
In `@admin-ui/plugins/jans-lock/components/JansLockFieldRenderer.tsx`:
- Around line 102-120: The JSX in JansLockFieldRenderer passes both
disabled={isDisabled} and viewOnly={isDisabled} to GluuToggleRow which may be
redundant; inspect GluuToggleRow to determine whether viewOnly and disabled have
distinct behavior and then either remove the redundant prop or pass the correct
value. If viewOnly should mirror disabled keep both, otherwise remove
viewOnly={isDisabled} from the toggle case (or set viewOnly to the intended
conditional) so only the appropriate prop controls the component.
In `@admin-ui/plugins/jans-lock/components/styles/JansLockFormPage.style.ts`:
- Around line 147-167: The focus styles for typeaheads and multi-inputs
(selectors: '.rbt .form-control', '.rbt .form-control:active',
'.rbt-input-main', '.rbt-input-main:active', '.rbt-input-multi', and
'.rbt-input-multi:focus-within') currently remove outline/box-shadow and keep
the same border color, making keyboard focus invisible; restore a visible focus
indicator by removing the rules that set outline: 'none' and boxShadow: 'none'
for these selectors and instead apply a distinct focus style (for example a
stronger border color or a visible box-shadow/outline using a focus color
variable such as themeColors.focus or an inputFocusColor) when :focus or
:focus-within is active so focused controls are clearly distinguishable for
keyboard users.
In `@admin-ui/plugins/jans-lock/helper/utils.ts`:
- Around line 145-161: The policySources array is built by conditional pushes
which shifts indices and breaks transformToFormValues that expects JSON at index
0 and ZIP at index 1; replace the push-based construction with a fixed
two-element array (first element = JSON source object, second element = ZIP
source object) using empty strings for missing authorizationToken/policyStoreUri
so positions are preserved, and keep references to policySources and
transformToFormValues consistent when updating this logic.
In `@admin-ui/plugins/jans-lock/helper/validations.ts`:
- Around line 17-25: The validation for cleanServiceInterval in
getLockValidationSchema is confusing because .nullable() is called before
.transform(emptyStringToNull) and .required(); move
.transform(emptyStringToNull) before .nullable() (i.e., apply transform first,
then call .nullable()) so empty strings are converted to null prior to
nullability checks, and then either keep .required() to reject nulls or
remove/adjust .required() to match the backend DTO if null is permitted by the
API; update the logic for the cleanServiceInterval field (and any similar
fields) accordingly.
In `@admin-ui/plugins/jans-lock/types/jans-lock-types.ts`:
- Around line 45-51: lockConfig is currently typed as Record<string, unknown>
which loses type safety; define a concrete interface (e.g., JansLockConfig) that
matches the backend response shape and use that in JansLockConfigurationProps
instead of Record<string, unknown>, then validate/narrow incoming data at the
component boundary (e.g., in the parent or a parse/deserialize function) with a
runtime check or type guard before passing to components so consumers of
JansLockConfigurationProps get proper typings for properties used in the
component.
In `@admin-ui/plugins/schema/__tests__/components/UserClaimsForm.test.tsx`:
- Line 49: The import for UserClaimsForm is declared after test fixture
declarations; move the import statement for UserClaimsForm (import
UserClaimsForm from 'Plugins/schema/components/Person/UserClaimsForm') up into
the top-level imports section alongside the other imports so all module imports
are grouped before any test fixture or setup code, keeping the same import path
and only changing its position.
In
`@admin-ui/plugins/schema/components/Person/styles/UserClaimsFormPage.style.ts`:
- Around line 42-58: The formGrid currently forces two columns and the content
wrapper keeps fixed side padding so the layout will squeeze on narrow viewports;
update the styles for content and formGrid so they respond to small screens:
change formGrid's gridTemplateColumns to a responsive pattern (e.g., use
repeat/auto-fit with a minmax breakpoint or switch to a single column under a
small-screen media query) and make content's horizontal padding adapt (reduce or
switch to smaller padding at narrow widths) so fields stack instead of
overflowing; modify the objects named content and formGrid in
UserClaimsFormPage.style.ts to implement these responsive grid and padding
adjustments.
- Around line 180-188: The focus style block for inputs in
UserClaimsFormPage.style.ts currently removes both outline and boxShadow
(selector starting with '& input:not([type="checkbox"]):focus...'), leaving no
visible focus indicator; restore an accessible focus state by removing the lines
that clear outline/boxShadow or replacing them with a visible focus rule (e.g.,
set outline: '2px solid var(--theme-focus-color)' or boxShadow: `0 0 0 3px
rgba(...)`) and ensure the border/box shadow uses a contrasting color (use
inputBorderColor or a --theme-focus-color variable) so keyboard users can see
focused fields.
In `@admin-ui/plugins/schema/components/Person/UserClaimsAddPage.tsx`:
- Around line 61-72: The add form uses a hard-coded partial object named
defaultAttribute which only populates a subset of AttributeItem and risks
missing new required fields; replace that literal with a call to the shared
factory getDefaultAttributeItem() (optionally merge/override any specific
defaults needed) so the add path uses the same full defaults as edit—update the
usage in UserClaimsAddPage (replace defaultAttribute with
getDefaultAttributeItem() or { ...getDefaultAttributeItem(), /* overrides */ })
to avoid unsafe casting and keep defaults aligned.
In `@admin-ui/plugins/schema/components/Person/UserClaimsEditPage.tsx`:
- Around line 41-52: The attribute query is currently fired unconditionally;
update the useAttribute call to only run when the user can read and we have a
valid id by passing an enabled flag or gating the hook: useAttribute(inum, {
enabled: canRead && Boolean(inum) }) (or equivalent enabled option used by your
hook), ensuring the hook invocation or options reference useAttribute, inum, and
canRead so the query won’t execute before the read check (apply the same change
to the other occurrence around lines 112-123).
In `@admin-ui/plugins/schema/components/Person/UserClaimsForm.tsx`:
- Line 488: The hardcoded isLoading={false} in UserClaimsForm should be made
dynamic: add an isLoading or isSubmitting boolean prop to the UserClaimsForm
component (update its props interface) and use that prop when rendering the
footer control (where isLoading is currently set) — e.g., pass props.isLoading
(or internal submitting state) into the FormFooter/Button isLoading prop so
footer buttons disable and show loading during submission; if you intentionally
don't need a loading state, add a concise comment near the isLoading line
explaining why it's fixed false.
In `@admin-ui/plugins/schema/components/Person/UserClaimsListPage.tsx`:
- Around line 148-160: The handleDeleteConfirm callback should ensure modal
state is updated even when deleteAttributeMutation.mutateAsync fails: wrap the
await deleteAttributeMutation.mutateAsync(...) call in a try/catch (or
try/finally) inside handleDeleteConfirm (referencing handleDeleteConfirm,
deleteAttributeMutation.mutateAsync, setModal, setItemToDelete) and move
setModal(false) and setItemToDelete(null) into a finally block (or execute them
in both success and error branches); optionally surface the error via the
mutation's error handler or rethrow after cleaning up so the UI doesn't remain
stuck open on failure.
In `@admin-ui/plugins/schema/components/Person/UserClaimsViewPage.tsx`:
- Around line 44-47: The useMemo calls for attributeScopes and canRead include
attributeResourceId in their dependency arrays even though attributeResourceId
is a module-level constant that never changes; remove attributeResourceId from
the dependencies and replace with an empty array (or only include real reactive
dependencies) so the hooks correctly reflect their stable inputs: update the
useMemo that defines attributeScopes (which references CEDAR_RESOURCE_SCOPES and
attributeResourceId) and the useMemo that computes canRead to drop
attributeResourceId from the dependency arrays.
In `@admin-ui/plugins/schema/hooks/useAttributeApi.ts`:
- Around line 82-88: The wrapper currently calls mutateAsync(...args).catch(()
=> {}) which swallows any errors from the post-write side-effect chain; change
the wrapper for mutate (and the analogous wrappers around mutateAsync at the
other spots) to return the promise from mutateAsync and handle failures by
either rethrowing or at minimum logging the error so callers see/report
failures: update the mutate implementation (and the other two wrappers) so it
does return mutateAsync(...args).catch(err => { console.error('post-write
side-effect failed', err); throw err; }) or otherwise propagate the rejection
instead of swallowing it, ensuring baseMutation and mutateAsync behavior is
preserved.
In `@admin-ui/plugins/schema/hooks/useMutationEffects.ts`:
- Around line 25-27: The hook useMutationEffects currently hard-codes navigation
to ROUTES.ATTRIBUTES_LIST on success (lines around the success handling) which
prevents callers from choosing a different destination; modify the hook
signature to accept a success destination/callback option (e.g.,
successDestination?: string | (data: any) => void or onSuccessNavigate?:
(data)=>void) alongside the existing
successMessage/errorMessage/navigateOnSuccess options, and replace the direct
call to navigate(ROUTES.ATTRIBUTES_LIST) with logic that calls the provided
destination or callback when present (falling back to navigateOnSuccess behavior
if you must preserve it); update callers to pass the desired route or callback
instead of relying on the hard-coded ROUTES.ATTRIBUTES_LIST.
---
Outside diff comments:
In
`@admin-ui/plugins/scripts/components/CustomScripts/styles/CustomScriptFormPage.style.ts`:
- Around line 320-331: The current focus rules ('& input:focus, & input:active,
& select:focus, & select:active, & .custom-select:focus, &
.custom-select:active' and '& input:focus-visible, & select:focus-visible')
remove outlines entirely which removes keyboard focus affordance; update the
focus-visible rule(s) so keyboard users get a clear visual indicator (e.g. keep
or replace outline with a visible style such as a distinct border color/width or
a subtle box-shadow focus ring) instead of `outline: none`/`boxShadow: none` —
modify the selectors in CustomScriptFormPage.style (the focus/focus-visible
rules) to apply that accessible focus ring (or ensure the existing border change
on focus is perceptibly stronger) so focus is always visually distinguishable
for keyboard navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d04bfdc-1b42-4d48-80c9-45682ace5050
📒 Files selected for processing (71)
admin-ui/app/cedarling/__tests__/client/CedarlingClient.test.tsadmin-ui/app/cedarling/__tests__/constants/resourceScopes.test.tsadmin-ui/app/cedarling/__tests__/enums/CedarlingLogType.test.tsadmin-ui/app/cedarling/__tests__/hooks/useCedarling.test.tsadmin-ui/app/cedarling/__tests__/utility/resources.test.tsadmin-ui/app/constants/ui.tsadmin-ui/app/customColors.tsadmin-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/GluuMultiSelectRow.tsxadmin-ui/app/routes/Apps/Gluu/GluuTabs.tsxadmin-ui/app/routes/Apps/Gluu/styles/GluuMultiSelectRow.style.tsadmin-ui/app/routes/Apps/Gluu/types/GluuMultiSelectRow.types.tsadmin-ui/app/styles/main.scssadmin-ui/app/utilities.tsxadmin-ui/plugins/admin/components/Assets/types/FormTypes.tsadmin-ui/plugins/jans-lock/__tests__/components/JansLock.test.tsxadmin-ui/plugins/jans-lock/__tests__/components/JansLockConfiguration.test.tsxadmin-ui/plugins/jans-lock/__tests__/components/JansLockFieldRenderer.test.tsxadmin-ui/plugins/jans-lock/__tests__/components/constants.test.tsadmin-ui/plugins/jans-lock/__tests__/components/helperConstants.test.tsadmin-ui/plugins/jans-lock/__tests__/components/utils.test.tsadmin-ui/plugins/jans-lock/__tests__/components/validations.test.tsadmin-ui/plugins/jans-lock/components/JansLock.tsxadmin-ui/plugins/jans-lock/components/JansLockConfiguration.tsxadmin-ui/plugins/jans-lock/components/JansLockFieldRenderer.tsxadmin-ui/plugins/jans-lock/components/constants.tsadmin-ui/plugins/jans-lock/components/styles/JansLockFormPage.style.tsadmin-ui/plugins/jans-lock/helper/index.tsadmin-ui/plugins/jans-lock/helper/utils.tsadmin-ui/plugins/jans-lock/helper/validations.tsadmin-ui/plugins/jans-lock/types/jans-lock-types.tsadmin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.jsadmin-ui/plugins/schema/__tests__/components/AttributeDetailPage.test.jsadmin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.jsadmin-ui/plugins/schema/__tests__/components/AttributeListPage.test.jsadmin-ui/plugins/schema/__tests__/components/UserClaimsAddPage.test.tsxadmin-ui/plugins/schema/__tests__/components/UserClaimsEditPage.test.tsxadmin-ui/plugins/schema/__tests__/components/UserClaimsForm.test.tsxadmin-ui/plugins/schema/__tests__/components/UserClaimsListPage.test.tsxadmin-ui/plugins/schema/components/Person/AttributeAddPage.tsxadmin-ui/plugins/schema/components/Person/AttributeDetailPage.tsxadmin-ui/plugins/schema/components/Person/AttributeEditPage.tsxadmin-ui/plugins/schema/components/Person/AttributeForm.tsxadmin-ui/plugins/schema/components/Person/AttributeListPage.tsxadmin-ui/plugins/schema/components/Person/AttributeViewPage.tsxadmin-ui/plugins/schema/components/Person/UserClaimsAddPage.tsxadmin-ui/plugins/schema/components/Person/UserClaimsDetailPage.tsxadmin-ui/plugins/schema/components/Person/UserClaimsEditPage.tsxadmin-ui/plugins/schema/components/Person/UserClaimsForm.tsxadmin-ui/plugins/schema/components/Person/UserClaimsListPage.tsxadmin-ui/plugins/schema/components/Person/UserClaimsViewPage.tsxadmin-ui/plugins/schema/components/Person/styles/UserClaimsFormPage.style.tsadmin-ui/plugins/schema/components/Person/styles/UserClaimsListPage.style.tsadmin-ui/plugins/schema/components/types/UserClaimsListPage.types.tsadmin-ui/plugins/schema/constants/index.tsadmin-ui/plugins/schema/hooks/index.tsadmin-ui/plugins/schema/hooks/useAttributeApi.tsadmin-ui/plugins/schema/hooks/useMutationEffects.tsadmin-ui/plugins/schema/hooks/useSchemaAuditLogger.tsadmin-ui/plugins/schema/hooks/useSchemaWebhook.tsadmin-ui/plugins/schema/plugin-metadata.tsadmin-ui/plugins/schema/utils/formHelpers.tsadmin-ui/plugins/schema/utils/validation.tsadmin-ui/plugins/scripts/components/CustomScripts/styles/CustomScriptFormPage.style.tsadmin-ui/plugins/user-management/utils/attributeTransformUtils.ts
💤 Files with no reviewable changes (10)
- admin-ui/plugins/schema/components/Person/AttributeForm.tsx
- admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx
- admin-ui/plugins/schema/tests/components/AttributeDetailPage.test.js
- admin-ui/plugins/schema/components/Person/AttributeDetailPage.tsx
- admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx
- admin-ui/plugins/schema/tests/components/AttributeEditPage.test.js
- admin-ui/plugins/schema/tests/components/AttributeListPage.test.js
- admin-ui/plugins/schema/tests/components/AttributeAddPage.test.js
- admin-ui/plugins/schema/components/Person/AttributeListPage.tsx
- admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx
| showApply={!hideButtons?.save} | ||
| applyButtonType="submit" | ||
| disableApply={!canApply} | ||
| isLoading={false} |
Contributor
There was a problem hiding this comment.
Consider making isLoading dynamic.
isLoading={false} is hardcoded. If the parent component passes a loading state (e.g., during submission), this should reflect it to disable the footer buttons and show appropriate feedback.
🔧 Suggested improvement
Consider accepting an isLoading or isSubmitting prop:
+// In props interface
+isLoading?: boolean
<GluuThemeFormFooter
// ...
- isLoading={false}
+ isLoading={props.isLoading ?? false}
/>Alternatively, if this form is only used in contexts where loading state is not needed, document this intentional choice with a comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/plugins/schema/components/Person/UserClaimsForm.tsx` at line 488,
The hardcoded isLoading={false} in UserClaimsForm should be made dynamic: add an
isLoading or isSubmitting boolean prop to the UserClaimsForm component (update
its props interface) and use that prop when rendering the footer control (where
isLoading is currently set) — e.g., pass props.isLoading (or internal submitting
state) into the FormFooter/Button isLoading prop so footer buttons disable and
show loading during submission; if you intentionally don't need a loading state,
add a concise comment near the isLoading line explaining why it's fixed false.
admin-ui/plugins/schema/components/Person/UserClaimsViewPage.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+82
to
+88
| return { | ||
| ...baseMutation, | ||
| mutate: (...args: Parameters<typeof mutateAsync>) => { | ||
| mutateAsync(...args).catch(() => {}) | ||
| }, | ||
| mutateAsync, | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t swallow failures from the post-write side-effect chain.
These wrappers still expose ...baseMutation, so callers can observe success as soon as the HTTP write finishes. The wrapper then keeps doing invalidation/webhook/audit work, but Lines 85, 135, and 182 drop any rejection from that second phase. That can produce a success toast/navigation even when the follow-up work failed. Please surface or at least log those errors instead of ignoring them.
Also applies to: 132-138, 179-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/plugins/schema/hooks/useAttributeApi.ts` around lines 82 - 88, The
wrapper currently calls mutateAsync(...args).catch(() => {}) which swallows any
errors from the post-write side-effect chain; change the wrapper for mutate (and
the analogous wrappers around mutateAsync at the other spots) to return the
promise from mutateAsync and handle failures by either rethrowing or at minimum
logging the error so callers see/report failures: update the mutate
implementation (and the other two wrappers) so it does return
mutateAsync(...args).catch(err => { console.error('post-write side-effect
failed', err); throw err; }) or otherwise propagate the rejection instead of
swallowing it, ensuring baseMutation and mutateAsync behavior is preserved.
Comment on lines
+25
to
+27
| successMessage: string | ||
| errorMessage: string | ||
| navigateOnSuccess?: boolean |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inject the success destination instead of hard-coding it.
useMutationEffects is generic and exported from the schema hooks, but Line 52 always navigates to ROUTES.ATTRIBUTES_LIST. That means any other flow can only disable navigation entirely, not choose the correct destination. Please make the target route/callback part of the hook options.
Also applies to: 51-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin-ui/plugins/schema/hooks/useMutationEffects.ts` around lines 25 - 27,
The hook useMutationEffects currently hard-codes navigation to
ROUTES.ATTRIBUTES_LIST on success (lines around the success handling) which
prevents callers from choosing a different destination; modify the hook
signature to accept a success destination/callback option (e.g.,
successDestination?: string | (data: any) => void or onSuccessNavigate?:
(data)=>void) alongside the existing
successMessage/errorMessage/navigateOnSuccess options, and replace the direct
call to navigate(ROUTES.ATTRIBUTES_LIST) with logic that calls the provided
destination or callback when present (falling back to navigateOnSuccess behavior
if you must preserve it); update callers to pass the desired route or callback
instead of relying on the hard-coded ROUTES.ATTRIBUTES_LIST.
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
|
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 Jans Lock module as per Figma (#2637)
Summary
This PR revamps the Jans Lock module in the Admin UI to align with the latest Figma designs as part of the ongoing 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: #2637
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests