LFX Mentorship: Feat: Week 2-3 - Implement TokenRatePolicy form view#575
LFX Mentorship: Feat: Week 2-3 - Implement TokenRatePolicy form view#575justin212407 wants to merge 1 commit into
Conversation
Signed-off-by: justin212407 <charlesjustin2124@gmail.com>
|
Thanks for the contribution! The linked issue(s) are closed. Please link an open, triaged issue and reopen this PR. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a form/YAML toggle create-and-edit page for TokenRateLimitPolicy resources, a new actions hook exposing edit/delete/labels/annotations actions and a "form edit" navigation action, corresponding console extension route/action wiring, an exposed-module registration, and related translation entries. ChangesTokenRateLimitPolicy Form Editing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ActionsMenu as useTokenRateLimitPolicyActions
participant CreatePage as KuadrantTokenRateLimitPolicyCreatePage
participant K8sAPI as Kubernetes API
User->>ActionsMenu: Select "Edit via form"
ActionsMenu->>CreatePage: Navigate to /tokenratelimitpolicy/name/:name/edit
CreatePage->>K8sAPI: useK8sWatchResource(TokenRateLimitPolicy)
K8sAPI-->>CreatePage: Existing resource data
CreatePage->>CreatePage: Populate form state (name, gateway, limits)
alt Form view
User->>CreatePage: Edit fields / add limit
CreatePage->>CreatePage: Update yamlInput to stay in sync
else YAML view
User->>CreatePage: Edit YAML
CreatePage->>CreatePage: Parse YAML, update form state
end
User->>CreatePage: Save
CreatePage->>K8sAPI: Submit updated TokenRateLimitPolicy resource
K8sAPI-->>CreatePage: Confirmation / error
CreatePage-->>User: Redirect or show result
Possibly related issues
Suggested reviewers: Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
There was a problem hiding this comment.
Nice work, left a few comments below 👍
Also one additional comment - The CRD spec shows rates is an array, supporting multiple rate windows per limit (like "1m" and "1h"). The modal currently only supports adding one rate per limit. We should update this to support multiple rates. See docs: https://docs.kuadrant.io/dev/kuadrant-operator/doc/overviews/token-rate-limiting/#multiple-time-windows
| const handleSaveLimit = () => { | ||
| if (newLimitName && newLimitValue !== '' && newLimitWindow) { | ||
| setLimits((prevLimits) => ({ | ||
| ...prevLimits, | ||
| [newLimitName]: { rates: [{ limit: Number(newLimitValue), window: newLimitWindow }] }, | ||
| })); | ||
| setIsAddLimitOpen(false); | ||
| } | ||
| }; | ||
|
|
||
| const isAddLimitSaveDisabled = !newLimitName || newLimitValue === '' || !newLimitWindow; |
There was a problem hiding this comment.
When a user adds a limit with the same name twice (e.g., "global"), the second one silently overwrites the first without warning. This could be confusing since they won't know why their first configuration disappeared.
Adding duplicate detection would prevent this, example:
const isDuplicateName = !!limits[newLimitName];
const isAddLimitSaveDisabled =
!newLimitName ||
newLimitValue === '' ||
!newLimitWindow ||
isDuplicateName;And show validation feedback:
<FormHelperText>
<HelperText>
<HelperTextItem variant={isDuplicateName ? 'error' : 'default'}>
{isDuplicateName
? t('A limit with this name already exists')
: t('Unique identifier for this rate limit')}
</HelperTextItem>
</HelperText>
</FormHelperText>| <FormGroup label={t('Window')} isRequired fieldId="new-limit-window"> | ||
| <TextInput | ||
| isRequired | ||
| type="text" | ||
| id="new-limit-window" | ||
| value={newLimitWindow} | ||
| onChange={(_event, value) => setNewLimitWindow(value)} | ||
| placeholder="e.g. 1h, 60s, 1440m" | ||
| /> | ||
| <FormHelperText> | ||
| <HelperText> | ||
| <HelperTextItem> | ||
| {t('Time window for the rate limit (e.g. 1h, 60s, 1440m)')} | ||
| </HelperTextItem> | ||
| </HelperText> | ||
| </FormHelperText> |
There was a problem hiding this comment.
Window field accepts any string right now, so users can enter invalid formats like "1 hour" or "60" and only discover the error when the K8s API rejects it with a regex validation message.
The window field must follow Gateway API Duration format with pattern ^([0-9]{1,5}(h|m|s|ms)){1,4}$.
Valid formats: "1h", "60s", "500ms", "1h30m", "2h15m30s"
Invalid formats: "1 hour" (spaces), "1d" (days not supported), "1000000h" (max 5 digits per unit)
Adding client-side validation would catch these early with clear feedback instead of the API error when trying to create, example:
const windowPattern = /^([0-9]{1,5}(h|m|s|ms)){1,4}$/;
const isValidWindow = !newLimitWindow || windowPattern.test(newLimitWindow);
<TextInput
validated={isValidWindow ? 'default' : 'error'}
/>
<FormHelperText>
<HelperText>
<HelperTextItem variant={isValidWindow ? 'default' : 'error'}>
{isValidWindow
? t('Time window for the rate limit (e.g. 1h, 60s, 1440m)')
: t('Format must be like: 1h, 60s, 500ms, or 1h30m')}
</HelperTextItem>
</HelperText>
</FormHelperText>| <FormGroup label={t('Limit')} isRequired fieldId="new-limit-value"> | ||
| <TextInput | ||
| isRequired | ||
| type="number" | ||
| id="new-limit-value" | ||
| value={newLimitValue} | ||
| onChange={(_event, value) => setNewLimitValue(value === '' ? '' : Number(value))} | ||
| placeholder={t('Limit value')} | ||
| /> |
There was a problem hiding this comment.
The limit input field has a small clickable area where only the left portion where the placeholder text appears is responsive. The rest of the input box doesn't accept clicks/focus, which can create a confusing UX.
|
FYI this PR will require a rebase once #457 is merged |
I will make the changes and rebase before pushing the next commit. |
2 participants
Description
Adds a form-based creation/editing view to
KuadrantTokenRateLimitPolicyCreatePage.tsx, which previously only rendered a static YAML editor with a hardcoded example resource. Users can now create and editTokenRateLimitPolicyresources through a guided form interface without needing to know the CRD spec structure, while retaining the option to switch to a YAML view at any point.This is the second deliverable in the Add form views for policy creation pages (#378) epic, following the same five-pillar architecture established in
KuadrantDNSPolicyCreatePage.tsxand first implemented forOIDCPolicyin #545.Type of change
[feat]New featureChanges made
Replaced the static YAML-only editor in
KuadrantTokenRateLimitPolicyCreatePage.tsxwith a complete form view following the five-pillar architecture fromKuadrantDNSPolicyCreatePage.tsx.Added a Form/YAML toggle (PatternFly Radio buttons), with both views backed by the same underlying state.
Implemented
createTokenRateLimitPolicy()to generate the complete Kubernetes resource from form state. This object is shared by bothResourceYAMLEditorandKuadrantCreateUpdate.Implemented bidirectional YAML synchronization:
useEffectupdates YAML whenever form fields change.handleYAMLChange()parses edited YAML back into form state usingjs-yaml.Added edit mode hydration:
useK8sWatchResourceto load the existing resource.formDisabled.Built an inline limits UI using:
LabelandLabelGroupfor displaying configured limits.Modalfor adding new limits with Limit Name, Limit Value, and Window fields.Wired
KuadrantCreateUpdatefor both create and update flows withpolicyType="tokenratelimit".Added
src/components/tokenratelimitpolicy/useTokenRateLimitPolicyActions.tsx, providing:Added the edit route:
pointing to
KuadrantTokenRateLimitPolicyCreatePageinconsole-extensions.json.Registered
useTokenRateLimitPolicyActionsas aconsole.action/resource-providerforkuadrant.io/v1alpha1/TokenRateLimitPolicy.Exported
useTokenRateLimitPolicyActionsfrompackage.jsonviaexposedModules.Added 7 new i18n keys to
locales/en/plugin__kuadrant-console-plugin.jsonin alphabetical order.Test plan
yarn lintpasses (no changes after running)yarn buildpassesyarn i18npasses (no changes after running; commit updated locale files if needed)Manual testing
Navigate to:
Verify the form renders with:
Leave Policy Name empty and verify the Save button is disabled.
Enter a policy name and select a gateway.
Verify Save becomes enabled.
Click Add Limit.
Verify the modal opens with:
Leave all modal fields empty.
Verify Add Limit is disabled.
Fill all three fields.
Verify Add Limit becomes enabled.
Save the limit.
Verify:
Remove a limit using the × button.
Verify it disappears.
Switch to YAML View.
Verify the generated YAML contains:
Edit the YAML.
Switch back to Form View and verify the form reflects the updated values.
Submit the form.
Verify the user is redirected to the TokenRateLimitPolicy list page.
Open the kebab menu for an existing TokenRateLimitPolicy.
Verify the following actions are available:
Click Edit.
Verify:
Click Cancel.
Verify the browser navigates back.
Screenshots
Screencast.From.2026-06-25.00-53-32.mp4
Review guidance
Suggested review order
src/components/KuadrantTokenRateLimitPolicyCreatePage.tsxMain implementation.
Compare against
KuadrantDNSPolicyCreatePage.tsxfor five-pillar architecture consistency.Key areas:
TokenRate,TokenLimitConfig, andTokenLimitMapinterfacescreateTokenRateLimitPolicy()factorysrc/components/tokenratelimitpolicy/useTokenRateLimitPolicyActions.tsxNew resource action hook following the same implementation pattern as
useOIDCPolicyActions.tsx.console-extensions.jsonReview:
package.jsonVerify
useTokenRateLimitPolicyActionshas been added toexposedModules.locales/en/plugin__kuadrant-console-plugin.jsonConfirm the seven new translation keys are alphabetically ordered.
Why a local modal?
The existing
AddLimitModaluses:whereas
TokenRateLimitPolicyexpects:Rather than modifying a shared component used elsewhere, this PR introduces a self-contained modal that matches the TokenRateLimitPolicy schema, keeping the change isolated and avoiding regressions for existing
RateLimitPolicyfunctionality.Checklist
t()and have been added tolocales/en/plugin__kuadrant-console-plugin.jsonkuadrant-(no bare.pf-*or.co-*selectors and no hex colors)console.logstatements remainSigned-off-byline (git commit -s)Summary by CodeRabbit
New Features
Documentation