fix(opcua): align UI with design system#744
Merged
Merged
Conversation
Mirror of openplc-web fix/ui-opcua-design-system to keep shared frontend surfaces byte-identical. Updates the OPC UA address-space tree, selected variables list, certificate/security/user tabs and modals so that: - Variable tree uses Radix Chevron icons instead of glyphs that were falling back to the color emoji font. - Type badges in the variable tree and selected-variables list are neutralized to a single palette. - Delete/Remove buttons across the OPC UA tabs use the neutral box styling consistent with adjacent Edit actions. - Inline alert and validation-error containers drop the amber/red backgrounds in favour of neutral border + background, with copy carrying the severity cue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughUpdated UI/styling and layout across multiple OPC‑UA server editor components: validation/warning colors moved to neutral tones, several list/card views were refactored into table layouts, node-type icon rendering unified via a lookup, and form validation shifted to per-field inline errors. ChangesOPC‑UA Editor UI & Layout Updates
Sequence Diagram(s)(Skipped — changes are UI/layout and do not introduce new multi-component control flow.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/variable-tree.tsx:
- Around line 33-44: The ExpandIcon component renders an icon-only button
without accessible name/state; update ExpandIcon to import and use i18next
(e.g., useTranslation) and add aria-expanded={expanded} and a localized
aria-label such as aria-label={t(expanded ? 'opcua.collapse' : 'opcua.expand')}
(or similar keys) so the button exposes both state and a translated name while
keeping the existing onClick and Icon selection
(ChevronDownIcon/ChevronRightIcon).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9bff4e36-1802-41e3-8756-6f078a2c2681
📒 Files selected for processing (9)
src/frontend/components/_features/[workspace]/editor/server/opcua-server/components/certificate-modal.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/certificates-tab.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/security-profile-modal.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/selected-variables-list.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/user-modal.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/users-tab.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/variable-config-modal.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/variable-tree.tsx
Comment on lines
+33
to
+44
| const ExpandIcon = ({ expanded, onClick }: { expanded: boolean; onClick: (e: React.MouseEvent) => void }) => { | ||
| const Icon = expanded ? ChevronDownIcon : ChevronRightIcon | ||
| return ( | ||
| <button | ||
| type='button' | ||
| onClick={onClick} | ||
| className='flex h-4 w-4 items-center justify-center text-neutral-500 hover:text-neutral-700 dark:text-neutral-400 dark:hover:text-neutral-200' | ||
| > | ||
| <Icon className='h-3.5 w-3.5' /> | ||
| </button> | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Add accessible name/state to the expand button.
At Line 36, this is now an icon-only control, but it has no accessible name and no exposed expanded state. Please add aria-label + aria-expanded (with localized labels).
♿ Suggested fix
-import { useCallback, useState } from 'react'
+import { useCallback, useState } from 'react'
+import { useTranslation } from 'react-i18next'
...
const ExpandIcon = ({ expanded, onClick }: { expanded: boolean; onClick: (e: React.MouseEvent) => void }) => {
+ const { t } = useTranslation()
const Icon = expanded ? ChevronDownIcon : ChevronRightIcon
return (
<button
type='button'
onClick={onClick}
+ aria-label={expanded ? t('opcua.variableTree.collapseNode') : t('opcua.variableTree.expandNode')}
+ aria-expanded={expanded}
className='flex h-4 w-4 items-center justify-center text-neutral-500 hover:text-neutral-700 dark:text-neutral-400 dark:hover:text-neutral-200'
>
<Icon className='h-3.5 w-3.5' />
</button>
)
}As per coding guidelines "Use i18next for internationalization in frontend components".
📝 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 ExpandIcon = ({ expanded, onClick }: { expanded: boolean; onClick: (e: React.MouseEvent) => void }) => { | |
| const Icon = expanded ? ChevronDownIcon : ChevronRightIcon | |
| return ( | |
| <button | |
| type='button' | |
| onClick={onClick} | |
| className='flex h-4 w-4 items-center justify-center text-neutral-500 hover:text-neutral-700 dark:text-neutral-400 dark:hover:text-neutral-200' | |
| > | |
| <Icon className='h-3.5 w-3.5' /> | |
| </button> | |
| ) | |
| } | |
| import { useCallback, useState } from 'react' | |
| import { useTranslation } from 'react-i18next' | |
| const ExpandIcon = ({ expanded, onClick }: { expanded: boolean; onClick: (e: React.MouseEvent) => void }) => { | |
| const { t } = useTranslation() | |
| const Icon = expanded ? ChevronDownIcon : ChevronRightIcon | |
| return ( | |
| <button | |
| type='button' | |
| onClick={onClick} | |
| aria-label={expanded ? t('opcua.variableTree.collapseNode') : t('opcua.variableTree.expandNode')} | |
| aria-expanded={expanded} | |
| className='flex h-4 w-4 items-center justify-center text-neutral-500 hover:text-neutral-700 dark:text-neutral-400 dark:hover:text-neutral-200' | |
| > | |
| <Icon className='h-3.5 w-3.5' /> | |
| </button> | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/variable-tree.tsx
around lines 33 - 44, The ExpandIcon component renders an icon-only button
without accessible name/state; update ExpandIcon to import and use i18next
(e.g., useTranslation) and add aria-expanded={expanded} and a localized
aria-label such as aria-label={t(expanded ? 'opcua.collapse' : 'opcua.expand')}
(or similar keys) so the button exposes both state and a translated name while
keeping the existing onClick and Icon selection
(ChevronDownIcon/ChevronRightIcon).
Convert Security Profiles, Users, and Trusted Client Certificates lists to tables matching the S7Comm Data Blocks pattern, with the brand-blue "Add" action button and edit/delete icon buttons. In the user modal, move per-field validation errors inline beneath each input and drop the bottom validation card. On the Users tab, drop the colored role badges in favor of plain text. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/components/_features/[workspace]/editor/server/opcua-server/components/users-tab.tsx (1)
102-135:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove hardcoded UI strings to i18next translation keys.
This component contains multiple user-facing strings that should be localized: the warning message about missing password users, "Add User" button text, empty state message, table headers ("Name", "Type", "Role", "Actions"), role descriptions, and the header description. The i18next infrastructure is already configured in the project; use
useTranslation()hook to consume these strings from translation files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/users-tab.tsx around lines 102 - 135, The UI strings in users-tab.tsx (including the warning block text, the "Add User" button label, the empty-state message, table headers "Name"/"Type"/"Role"/"Actions", role descriptions and header description) should be replaced with i18next translation keys via the useTranslation() hook; import and call const { t } = useTranslation() at the top of the UsersTab component, swap each hardcoded literal passed to elements (e.g. the warning paragraph, the button label used by handleAddUser and PlusIcon button, the empty-state <p>, and the <th> headers) to t('opcua.users.<key>') or similar keys, and add corresponding keys to the translation files so the same keys are available in locales.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/certificates-tab.tsx:
- Around line 298-305: The delete button using <TrashIcon /> lacks an accessible
name; update the button element that calls handleDeleteCertificate(cert.id) to
include an aria-label (e.g., aria-label={`Delete certificate ${cert.id}`} or
include cert.name if available) so assistive tech can announce the control; keep
the existing title and visual markup but add the aria-label attribute to the
same button element to provide a clear, unique accessible name.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsx:
- Around line 137-140: The delete path currently only checks total row count and
can remove the last enabled profile; update the deletion check and any delete
handlers to consider enabled count: compute const enabledCount =
config.securityProfiles.filter(p => p.enabled).length and before allowing delete
(or enabling the delete button) disallow removing a profile when profile.enabled
&& enabledCount <= 1; apply the same check where you render the delete action
and in the actual delete handler used by the map over config.securityProfiles
(the block using isLastEnabled and the separate delete code at the later section
referenced) so deletion is prevented consistently.
- Around line 143-149: The checkbox and icon-only buttons in the
SecurityProfilesTab component lack accessible names; update the input and the
Edit/Delete icon buttons to include descriptive aria-labels that use the profile
context (e.g., for the checkbox: aria-label={`Toggle ${profile.name} enabled`}
or similar, and for the Edit/Delete buttons: aria-label={`Edit ${profile.name}
profile`} and aria-label={`Delete ${profile.name} profile`}); locate the
controls around handleToggleEnabled/profile.enabled and the icon buttons (the
Edit/Delete handlers) and add the aria-label props so screen readers announce
each control clearly.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/user-modal.tsx:
- Around line 156-183: certificateError must only represent the "please select a
certificate" validation message, not the "no trusted certs" UI banner; change
the logic so certificateError === 'Please select a certificate' when authType
=== 'certificate' and certificateId is falsy, and create a separate boolean
(e.g. hasNoTrustedCerts or missingTrustedCerts) computed from authType ===
'certificate' && availableCertificates.length === 0 &&
!existingUser?.certificateId; include that boolean in the isValid check (isValid
= ... && !missingTrustedCerts) so Save is disabled for new users when no certs
exist, and keep the JSX guard that only renders the certificateError span when
availableCertificates.length > 0 so the banner and the inline error don't
duplicate.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/users-tab.tsx:
- Around line 152-167: The Edit and Delete icon-only buttons lack reliable
accessible names; update the two button elements that call handleEditUser(user)
and handleDeleteUser(user.id) to include explicit aria-label attributes (e.g.,
aria-label={`Edit user ${user.username ?? user.id}`} and aria-label={`Delete
user ${user.username ?? user.id}`}) so screen readers get a proper name while
keeping the existing title and icons (Pencil1Icon and TrashIcon).
---
Outside diff comments:
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/users-tab.tsx:
- Around line 102-135: The UI strings in users-tab.tsx (including the warning
block text, the "Add User" button label, the empty-state message, table headers
"Name"/"Type"/"Role"/"Actions", role descriptions and header description) should
be replaced with i18next translation keys via the useTranslation() hook; import
and call const { t } = useTranslation() at the top of the UsersTab component,
swap each hardcoded literal passed to elements (e.g. the warning paragraph, the
button label used by handleAddUser and PlusIcon button, the empty-state <p>, and
the <th> headers) to t('opcua.users.<key>') or similar keys, and add
corresponding keys to the translation files so the same keys are available in
locales.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f8310d88-4e31-40cd-b58c-4fb38d283f52
📒 Files selected for processing (4)
src/frontend/components/_features/[workspace]/editor/server/opcua-server/components/certificates-tab.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/user-modal.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/users-tab.tsx
Comment on lines
+298
to
+305
| <button | ||
| type='button' | ||
| onClick={() => handleDeleteCertificate(cert.id)} | ||
| className='rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400' | ||
| title='Delete' | ||
| > | ||
| <TrashIcon className='h-4 w-4' /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Provide an accessible name for the icon-only delete control.
Please add aria-label; relying on title alone is insufficient for consistent assistive-tech support.
Proposed fix
<button
type='button'
onClick={() => handleDeleteCertificate(cert.id)}
className='rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400'
+ aria-label={`Delete trusted certificate ${cert.id}`}
title='Delete'
>
<TrashIcon className='h-4 w-4' />
</button>📝 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
| <button | |
| type='button' | |
| onClick={() => handleDeleteCertificate(cert.id)} | |
| className='rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400' | |
| title='Delete' | |
| > | |
| <TrashIcon className='h-4 w-4' /> | |
| </button> | |
| <button | |
| type='button' | |
| onClick={() => handleDeleteCertificate(cert.id)} | |
| className='rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400' | |
| aria-label={`Delete trusted certificate ${cert.id}`} | |
| title='Delete' | |
| > | |
| <TrashIcon className='h-4 w-4' /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/certificates-tab.tsx
around lines 298 - 305, The delete button using <TrashIcon /> lacks an
accessible name; update the button element that calls
handleDeleteCertificate(cert.id) to include an aria-label (e.g.,
aria-label={`Delete certificate ${cert.id}`} or include cert.name if available)
so assistive tech can announce the control; keep the existing title and visual
markup but add the aria-label attribute to the same button element to provide a
clear, unique accessible name.
Comment on lines
+137
to
+140
| {config.securityProfiles.map((profile) => { | ||
| const isLastEnabled = | ||
| profile.enabled && config.securityProfiles.filter((p) => p.enabled).length <= 1 | ||
| return ( |
Contributor
There was a problem hiding this comment.
Deleting an enabled profile can bypass the “at least one enabled” rule.
The toggle path blocks disabling the last enabled profile, but delete only checks total row count. If one profile is enabled and others are disabled, deleting the enabled one leaves zero enabled profiles.
Proposed fix
{config.securityProfiles.map((profile) => {
- const isLastEnabled =
- profile.enabled && config.securityProfiles.filter((p) => p.enabled).length <= 1
+ const enabledCount = config.securityProfiles.filter((p) => p.enabled).length
+ const isLastEnabled = profile.enabled && enabledCount <= 1
+ const disableDelete = config.securityProfiles.length <= 1 || isLastEnabled
return (
@@
<button
type='button'
onClick={() => handleDeleteProfile(profile.id)}
- disabled={config.securityProfiles.length <= 1}
+ disabled={disableDelete}
className={cn(
'rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400',
- config.securityProfiles.length <= 1 && 'cursor-not-allowed opacity-50',
+ disableDelete && 'cursor-not-allowed opacity-50',
)}
title='Delete'
>Also applies to: 179-186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsx
around lines 137 - 140, The delete path currently only checks total row count
and can remove the last enabled profile; update the deletion check and any
delete handlers to consider enabled count: compute const enabledCount =
config.securityProfiles.filter(p => p.enabled).length and before allowing delete
(or enabling the delete button) disallow removing a profile when profile.enabled
&& enabledCount <= 1; apply the same check where you render the delete action
and in the actual delete handler used by the map over config.securityProfiles
(the block using isLastEnabled and the separate delete code at the later section
referenced) so deletion is prevented consistently.
Comment on lines
+143
to
+149
| <label className='relative inline-flex cursor-pointer items-center'> | ||
| <input | ||
| type='checkbox' | ||
| checked={profile.enabled} | ||
| onChange={() => handleToggleEnabled(profile)} | ||
| className='peer sr-only' | ||
| disabled={isLastEnabled} |
Contributor
There was a problem hiding this comment.
Add ARIA labels to unnamed toggle/action controls.
The enabled checkbox has no text label, and Edit/Delete are icon-only buttons. Please add explicit aria-labels so controls are announced clearly by assistive technologies.
Proposed fix
<input
type='checkbox'
checked={profile.enabled}
onChange={() => handleToggleEnabled(profile)}
className='peer sr-only'
disabled={isLastEnabled}
+ aria-label={`Enable security profile ${profile.name}`}
/>
@@
<button
type='button'
onClick={() => handleEditProfile(profile)}
className='rounded p-1 text-neutral-500 hover:bg-neutral-100 hover:text-neutral-700 dark:text-neutral-400 dark:hover:bg-neutral-800 dark:hover:text-neutral-200'
+ aria-label={`Edit security profile ${profile.name}`}
title='Edit'
>
@@
<button
type='button'
onClick={() => handleDeleteProfile(profile.id)}
disabled={config.securityProfiles.length <= 1}
className={cn(
'rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400',
config.securityProfiles.length <= 1 && 'cursor-not-allowed opacity-50',
)}
+ aria-label={`Delete security profile ${profile.name}`}
title='Delete'
>Also applies to: 171-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsx
around lines 143 - 149, The checkbox and icon-only buttons in the
SecurityProfilesTab component lack accessible names; update the input and the
Edit/Delete icon buttons to include descriptive aria-labels that use the profile
context (e.g., for the checkbox: aria-label={`Toggle ${profile.name} enabled`}
or similar, and for the Edit/Delete buttons: aria-label={`Edit ${profile.name}
profile`} and aria-label={`Delete ${profile.name} profile`}); locate the
controls around handleToggleEnabled/profile.enabled and the icon buttons (the
Edit/Delete handlers) and add the aria-label props so screen readers announce
each control clearly.
Comment on lines
+156
to
+183
| const certificateError = useMemo<string | null>(() => { | ||
| if (authType !== 'certificate') return null | ||
| if (availableCertificates.length === 0 && !existingUser?.certificateId) { | ||
| // The certificate-auth section already renders this banner inline | ||
| // when there are no trusted certs. Skip surfacing it as a duplicate | ||
| // field error. | ||
| return null | ||
| } | ||
|
|
||
| return errors | ||
| }, [ | ||
| authType, | ||
| username, | ||
| password, | ||
| confirmPassword, | ||
| certificateId, | ||
| existingUsernames, | ||
| existingUser, | ||
| isEditing, | ||
| availableCertificates, | ||
| ]) | ||
|
|
||
| const isValid = validationErrors.length === 0 | ||
| if (!certificateId) return 'Please select a certificate' | ||
| return null | ||
| }, [authType, certificateId, availableCertificates, existingUser]) | ||
|
|
||
| // Password mismatch handling has two flavours: | ||
| // - `passwordsMismatchVisible`: drives the inline red label under the | ||
| // Confirm Password input. Only true once the user has typed something | ||
| // in confirmPassword, so the error doesn't flicker while they're still | ||
| // filling out the form. | ||
| // - `passwordsOk`: gates the Save button. Requires both fields to be | ||
| // filled AND match when a password is being set (new user, or editing | ||
| // user with non-empty password). Empty confirmPassword keeps Save | ||
| // disabled silently — no error is shown until the user types into the | ||
| // confirm field. | ||
| const isSettingPassword = authType === 'password' && (!isEditing || password.length > 0) | ||
| const passwordsMismatchVisible = | ||
| isSettingPassword && confirmPassword.length > 0 && password !== confirmPassword | ||
| const passwordsOk = !isSettingPassword || (confirmPassword.length > 0 && password === confirmPassword) | ||
|
|
||
| const isValid = !usernameError && !passwordError && !certificateError && passwordsOk |
Contributor
There was a problem hiding this comment.
certificateError returns null when no certs exist, making isValid = true and allowing a malformed user to be saved
When authType === 'certificate', availableCertificates.length === 0, and existingUser is undefined (new user):
usernameError→null(password branch skipped)passwordError→null(password branch skipped)certificateError→ hits the earlyreturn nullon lines 158-163passwordsOk→true(isSettingPasswordisfalse)isValid→true
The Save button becomes enabled; handleSave runs and persists:
{ type: 'certificate', certificateId: null, ... }The comment's intent (avoid duplicating the existing "no trusted certs" banner as a second error label) is sound UX, but the current implementation conflates the display decision with the validity decision. These need to be separated.
🐛 Proposed fix
const certificateError = useMemo<string | null>(() => {
if (authType !== 'certificate') return null
if (availableCertificates.length === 0 && !existingUser?.certificateId) {
- // The certificate-auth section already renders this banner inline
- // when there are no trusted certs. Skip surfacing it as a duplicate
- // field error.
- return null
+ // No trusted certificates exist — still an error for isValid purposes,
+ // but we return a sentinel so the render layer knows NOT to show a
+ // duplicate label (the "no trusted certs" banner already conveys this).
+ return 'NO_CERTS' // not shown as a label; just blocks Save
}
if (!certificateId) return 'Please select a certificate'
return null
}, [authType, certificateId, availableCertificates, existingUser])Then in the JSX, keep the existing guard that only renders the error span when there are available certificates (the banner already handles the empty case):
- {certificateError && (
+ {certificateError && certificateError !== 'NO_CERTS' && (
<span className='font-caption text-cp-xs font-normal text-red-500'>
{certificateError}
</span>
)}Alternatively, extract a separate hasNoCertsError boolean and keep certificateError purely for the "please select" message, then isValid uses both.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/user-modal.tsx
around lines 156 - 183, certificateError must only represent the "please select
a certificate" validation message, not the "no trusted certs" UI banner; change
the logic so certificateError === 'Please select a certificate' when authType
=== 'certificate' and certificateId is falsy, and create a separate boolean
(e.g. hasNoTrustedCerts or missingTrustedCerts) computed from authType ===
'certificate' && availableCertificates.length === 0 &&
!existingUser?.certificateId; include that boolean in the isValid check (isValid
= ... && !missingTrustedCerts) so Save is disabled for new users when no certs
exist, and keep the JSX guard that only renders the certificateError span when
availableCertificates.length > 0 so the banner and the inline error don't
duplicate.
Comment on lines
+152
to
+167
| <button | ||
| type='button' | ||
| onClick={() => handleEditUser(user)} | ||
| className='rounded p-1 text-neutral-500 hover:bg-neutral-100 hover:text-neutral-700 dark:text-neutral-400 dark:hover:bg-neutral-800 dark:hover:text-neutral-200' | ||
| title='Edit' | ||
| > | ||
| <Pencil1Icon className='h-4 w-4' /> | ||
| </button> | ||
| <button | ||
| type='button' | ||
| onClick={() => handleDeleteUser(user.id)} | ||
| className='rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400' | ||
| title='Delete' | ||
| > | ||
| <TrashIcon className='h-4 w-4' /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add explicit accessible names to icon-only action buttons.
title is not a reliable accessible name for screen readers. Please add aria-label on Edit/Delete buttons.
Proposed fix
<button
type='button'
onClick={() => handleEditUser(user)}
className='rounded p-1 text-neutral-500 hover:bg-neutral-100 hover:text-neutral-700 dark:text-neutral-400 dark:hover:bg-neutral-800 dark:hover:text-neutral-200'
+ aria-label={`Edit user ${getUserDisplayName(user)}`}
title='Edit'
>
<Pencil1Icon className='h-4 w-4' />
</button>
<button
type='button'
onClick={() => handleDeleteUser(user.id)}
className='rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400'
+ aria-label={`Delete user ${getUserDisplayName(user)}`}
title='Delete'
>
<TrashIcon className='h-4 w-4' />
</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/users-tab.tsx
around lines 152 - 167, The Edit and Delete icon-only buttons lack reliable
accessible names; update the two button elements that call handleEditUser(user)
and handleDeleteUser(user.id) to include explicit aria-label attributes (e.g.,
aria-label={`Edit user ${user.username ?? user.id}`} and aria-label={`Delete
user ${user.username ?? user.id}`}) so screen readers get a proper name while
keeping the existing title and icons (Pencil1Icon and TrashIcon).
Collapses short multi-line expressions in security-profiles-tab, user-modal and users-tab so prettier --check passes. Mirrored across both repos to keep ci-sync byte-identical. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/frontend/components/_features/[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsx (2)
142-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIcon-only controls still lack
aria-label.The toggle checkbox (line 143), the Edit button (lines 170-177), and the Delete button (lines 178-189) are all visually unlabeled.
titleattributes are not a reliable accessible name for screen readers.♿ Proposed fix
<input type='checkbox' checked={profile.enabled} onChange={() => handleToggleEnabled(profile)} className='peer sr-only' disabled={isLastEnabled} + aria-label={`Enable security profile ${profile.name}`} /><button type='button' onClick={() => handleEditProfile(profile)} className='rounded p-1 text-neutral-500 hover:bg-neutral-100 hover:text-neutral-700 dark:text-neutral-400 dark:hover:bg-neutral-800 dark:hover:text-neutral-200' title='Edit' + aria-label={`Edit security profile ${profile.name}`} ><button type='button' onClick={() => handleDeleteProfile(profile.id)} - disabled={config.securityProfiles.length <= 1} + disabled={disableDelete} className={...} title='Delete' + aria-label={`Delete security profile ${profile.name}`} >Also applies to: 170-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsx around lines 142 - 149, The icon-only interactive controls (the checkbox and the Edit/Delete buttons) lack accessible names; update the input checkbox in the profile row and the Edit/Delete button elements to include explicit aria-label attributes (not relying on title) that uniquely describe the action and target (e.g., aria-label={`Toggle enable ${profile.name}`} for the checkbox and aria-label={`Edit ${profile.name}`} / aria-label={`Delete ${profile.name}`} for the buttons). Locate the checkbox using handleToggleEnabled/profile.enabled and the Edit/Delete buttons in the same component where those handlers are invoked, and ensure disabled/visibility states remain unchanged while adding these aria-labels.
178-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete button still permits removing the last enabled profile.
isLastEnabled(line 138) is computed and used to disable the toggle, but the deletedisabledguard on line 181 only checks total count (config.securityProfiles.length <= 1). A user can still delete the sole enabled profile as long as there are ≥ 2 profiles total, leaving zero enabled profiles and violating the invariant enforced byhandleToggleEnabled.🐛 Proposed fix
{config.securityProfiles.map((profile) => { - const isLastEnabled = profile.enabled && config.securityProfiles.filter((p) => p.enabled).length <= 1 + const enabledCount = config.securityProfiles.filter((p) => p.enabled).length + const isLastEnabled = profile.enabled && enabledCount <= 1 + const disableDelete = config.securityProfiles.length <= 1 || isLastEnabled return (<button type='button' onClick={() => handleDeleteProfile(profile.id)} - disabled={config.securityProfiles.length <= 1} + disabled={disableDelete} className={cn( 'rounded p-1 text-neutral-500 hover:bg-red-100 hover:text-red-600 dark:text-neutral-400 dark:hover:bg-red-900/30 dark:hover:text-red-400', - config.securityProfiles.length <= 1 && 'cursor-not-allowed opacity-50', + disableDelete && 'cursor-not-allowed opacity-50', )} title='Delete' >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsx around lines 178 - 186, The Delete button currently only checks config.securityProfiles.length and can remove the last enabled profile; update the disabled guard on the delete button (the element using handleDeleteProfile) to also prevent deleting a profile when it is the last enabled one by using the existing isLastEnabled check for that profile (or equivalent enabled-count logic) — i.e., disable the Delete button if config.securityProfiles.length <= 1 OR isLastEnabled(profile.id) is true so deletion cannot leave zero enabled profiles (consistent with handleToggleEnabled invariant).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@src/frontend/components/_features/`[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsx:
- Around line 142-149: The icon-only interactive controls (the checkbox and the
Edit/Delete buttons) lack accessible names; update the input checkbox in the
profile row and the Edit/Delete button elements to include explicit aria-label
attributes (not relying on title) that uniquely describe the action and target
(e.g., aria-label={`Toggle enable ${profile.name}`} for the checkbox and
aria-label={`Edit ${profile.name}`} / aria-label={`Delete ${profile.name}`} for
the buttons). Locate the checkbox using handleToggleEnabled/profile.enabled and
the Edit/Delete buttons in the same component where those handlers are invoked,
and ensure disabled/visibility states remain unchanged while adding these
aria-labels.
- Around line 178-186: The Delete button currently only checks
config.securityProfiles.length and can remove the last enabled profile; update
the disabled guard on the delete button (the element using handleDeleteProfile)
to also prevent deleting a profile when it is the last enabled one by using the
existing isLastEnabled check for that profile (or equivalent enabled-count
logic) — i.e., disable the Delete button if config.securityProfiles.length <= 1
OR isLastEnabled(profile.id) is true so deletion cannot leave zero enabled
profiles (consistent with handleToggleEnabled invariant).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa875938-02db-41ee-a42f-0b86e50ba0b5
📒 Files selected for processing (3)
src/frontend/components/_features/[workspace]/editor/server/opcua-server/components/security-profiles-tab.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/user-modal.tsxsrc/frontend/components/_features/[workspace]/editor/server/opcua-server/components/users-tab.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/components/_features/[workspace]/editor/server/opcua-server/components/users-tab.tsx
- src/frontend/components/_features/[workspace]/editor/server/opcua-server/components/user-modal.tsx
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
3 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.
Summary
Editor-side mirror of openplc-web#377 (
fix/ui-opcua-design-system). Keeps the sharedfrontend/surface byte-identical with the web counterpart so the cross-repoShared Surface Synccheck can converge once the existingdevelopmentdrift backlog is drained.Mirrored changes
Updates the OPC UA address-space tree, selected-variables list, and the certificate / security-profile / user tabs and modals:
Files touched
All under
src/frontend/components/_features/[workspace]/editor/server/opcua-server/components/:certificate-modal.tsxcertificates-tab.tsxsecurity-profile-modal.tsxsecurity-profiles-tab.tsxselected-variables-list.tsxuser-modal.tsxusers-tab.tsxvariable-config-modal.tsxvariable-tree.tsxSync status
compare-surfaces.pyreports 0 OPC UA diffs).editor@developmentare pre-existing drift unrelated to this PR (ethercat, version-control, misc.) and are tracked by other mirror PRs (sync(ethercat): mirror openplc-web feat/ethercat-web-adapter #741, feat: version control #742, ...). This PR does not attempt to cover them.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Style
New Features
Refactor