Copilot commented on code in PR #14071:
URL: https://github.com/apache/gravitino/pull/14071#discussion_r4335024673
##########
web-v2/web/src/app/compliance/tags/CreateTagDialog.js:
##########
@@ -99,6 +101,7 @@ export default function CreateTagDialog({ ...props }) {
const submitData = {
name: values.name.trim(),
comment: values.comment,
+ allowedValues: values.valueMode === 'any' ? null : values.valueMode
=== 'none' ? [] : values.allowedValues,
Review Comment:
This mapping makes `valueMode: 'none'` and `valueMode: 'allowed'` with an
empty selection both submit `allowedValues: []`, which is ambiguous and can’t
be distinguished server-side. To avoid incorrect behavior, enforce that
`allowedValues` must be non-empty when `valueMode === 'allowed'` (and/or map an
empty list to a distinct representation).
##########
web-v2/web/src/app/compliance/tags/CreateTagDialog.js:
##########
@@ -160,6 +163,51 @@ export default function CreateTagDialog({ ...props }) {
<Form.Item name='comment' label='Comment'>
<TextArea />
</Form.Item>
+ {editTag ? (
+ <Form.Item label='Values' extra='Set when the tag was created.'>
+ {cacheData?.allowedValues == null ? (
+ 'Any value'
+ ) : cacheData.allowedValues.length === 0 ? (
+ 'No value'
+ ) : (
+ <Space wrap>
+ {cacheData.allowedValues.map(value => (
+ <Tag className='mr-0' key={value}>{value}</Tag>
+ ))}
Review Comment:
Using `key={value}` will produce duplicate React keys if `allowedValues`
contains repeated strings, which can cause incorrect rendering updates. To make
rendering stable, ensure `allowedValues` is unique before rendering and/or use
a key that is guaranteed unique (e.g., include the index if duplicates are
allowed).
##########
web-v2/web/src/app/compliance/tags/CreateTagDialog.js:
##########
@@ -160,6 +163,51 @@ export default function CreateTagDialog({ ...props }) {
<Form.Item name='comment' label='Comment'>
<TextArea />
</Form.Item>
+ {editTag ? (
+ <Form.Item label='Values' extra='Set when the tag was created.'>
+ {cacheData?.allowedValues == null ? (
+ 'Any value'
+ ) : cacheData.allowedValues.length === 0 ? (
+ 'No value'
+ ) : (
+ <Space wrap>
+ {cacheData.allowedValues.map(value => (
+ <Tag className='mr-0' key={value}>{value}</Tag>
+ ))}
+ </Space>
+ )}
+ </Form.Item>
+ ) : (
+ <>
+ <Form.Item label='Values' className={values?.valueMode ===
'allowed' ? 'mb-2' : undefined}>
+ <Form.Item name='valueMode' noStyle>
+ <Radio.Group
+ options={[
+ { label: 'Any value', value: 'any' },
+ { label: 'No value', value: 'none' },
+ { label: 'Only these values', value: 'allowed' }
+ ]}
+ />
+ </Form.Item>
+ </Form.Item>
+ {values?.valueMode === 'allowed' && (
+ <Form.Item
+ name='allowedValues'
+ extra='Press Enter to add each value. Case-sensitive, 256
characters maximum.'
+ rules={[
+ {
+ validator: (_, allowedValues) =>
+ allowedValues?.every(value => value.length <= 256)
+ ? Promise.resolve()
+ : Promise.reject(new Error('Each value must be at
most 256 characters'))
+ }
+ ]}
+ >
+ <Select mode='tags' placeholder='Type a value and press
Enter' />
+ </Form.Item>
Review Comment:
The validator only checks max length; it currently allows
empty/whitespace-only entries and duplicates. That can lead to confusing UX and
(given the edit-mode rendering) potential key collisions and inconsistent
backend data. Consider extending validation to: (1) trim and reject empty
values, (2) enforce uniqueness, and (3) (if required by semantics) require at
least one value when `valueMode === 'allowed'`.
##########
web-v2/web/src/app/compliance/tags/CreateTagDialog.js:
##########
@@ -160,6 +163,51 @@ export default function CreateTagDialog({ ...props }) {
<Form.Item name='comment' label='Comment'>
<TextArea />
</Form.Item>
+ {editTag ? (
+ <Form.Item label='Values' extra='Set when the tag was created.'>
Review Comment:
The helper text is a bit vague about the constraint. Consider clarifying
that values are immutable after creation (e.g., 'Values can’t be changed after
the tag is created.') so users understand why it is read-only in edit mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
