Refactor PreIssueAccessTokenActionConfigForm to use useReducer#10484
Refactor PreIssueAccessTokenActionConfigForm to use useReducer#10484canbo206 wants to merge 4 commits into
Conversation
…enActionConfigForm
…enActionConfigForm
|
|
📝 WalkthroughWalkthrough
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@features/admin.actions.v1/components/pre-issue-access-token-action-config-form.tsx`:
- Around line 85-98: The reducer state in
pre-issue-access-token-action-config-form is currently typed too narrowly for
values that can be absent. Update FormStateInterface so authenticationType and
rule can be null, widen FormActionInterface payloads for SET_AUTHENTICATION_TYPE
and SET_RULE to accept null, and adjust the RuleConfigForm setRule prop/type to
allow null as well. Ensure the reducer and any related state initialization or
action handling in this component use the updated nullable types consistently.
- Line 220: The pre-issue-access-token-action-config-form component has stray
extra braces that break parsing, so clean up the malformed conditional and file
terminator in the main form component. In `PreIssueAccessTokenActionConfigForm`,
change the `if (formState.isAuthenticationUpdateFormState || isCreateFormState)`
block to use a single opening brace, and remove the extra trailing `};` at the
end of the file so the component is syntactically valid.
- Around line 210-216: Initialize payloadRule to null before the
formState.isHasRule branching in pre-issue-access-token-action-config-form.tsx
so the create flow with no selected rule is explicitly treated as absent. Update
the logic around the payloadRule assignment and the later payloadRule !== null
check to use a consistent null sentinel, keeping the existing formState.rule and
isCreateFormState handling intact.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 34c94822-4ce3-4981-baf3-77ea35adc41a
📒 Files selected for processing (1)
features/admin.actions.v1/components/pre-issue-access-token-action-config-form.tsx
| if (formState.isHasRule) { | ||
| payloadRule = formState.rule; | ||
| } else { | ||
| if (!isCreateFormState && !rule) { | ||
| if (!isCreateFormState && !formState.rule) { | ||
| payloadRule = {}; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Initialize payloadRule to null before branching. In the create flow with no selected rule, it can stay undefined, so the later payloadRule !== null check still treats it as present.
🤖 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
`@features/admin.actions.v1/components/pre-issue-access-token-action-config-form.tsx`
around lines 210 - 216, Initialize payloadRule to null before the
formState.isHasRule branching in pre-issue-access-token-action-config-form.tsx
so the create flow with no selected rule is explicitly treated as absent. Update
the logic around the payloadRule assignment and the later payloadRule !== null
check to use a consistent null sentinel, keeping the existing formState.rule and
isCreateFormState handling intact.
|
Hi @canbo206, Thank you for the contribution. Let's update the pr title and description. Also please address the coderabbit bot comments as well. Thanks in advance! |
|
Hi @pavinduLakshan thanks for the feedback! I have addressed all three
|
| import { AxiosError } from "axios"; | ||
| import React, { FunctionComponent, ReactElement, useEffect, useState } from "react"; | ||
| import { useTranslation } from "react-i18next"; | ||
| import React, { FunctionComponent, ReactElement, useEffect, useReducer } from "react";import { useTranslation } from "react-i18next"; |
There was a problem hiding this comment.
| import React, { FunctionComponent, ReactElement, useEffect, useReducer } from "react";import { useTranslation } from "react-i18next"; | |
| import React, { FunctionComponent, ReactElement, useEffect, useReducer } from "react"; | |
| import { useTranslation } from "react-i18next"; |
| isAuthenticationUpdateFormState: boolean; | ||
| authenticationType: AuthenticationType | null; | ||
| isSubmitting: boolean; | ||
| isHasRule: boolean; | ||
| rule: RuleWithoutIdInterface | null; |
There was a problem hiding this comment.
lets add doc comments for interface members
| Partial<ActionConfigFormPropertyInterface> => { | ||
|
|
||
| // Call the utility validate function | ||
| return validateActionCommonFields(values, { | ||
| authenticationType: formState.authenticationType, | ||
| isAuthenticationUpdateFormState: formState.isAuthenticationUpdateFormState, | ||
| isCreateFormState: isCreateFormState | ||
| }); |
There was a problem hiding this comment.
removing indentation will cause lint failures
| const [ isHasRule, setIsHasRule ] = useState<boolean>(false); | ||
| const [ rule, setRule ] = useState<RuleWithoutIdInterface>(null); | ||
|
|
||
| const [ formState, dispatch ] = useReducer<React.Reducer<FormStateInterface, FormActionInterface>>( |
There was a problem hiding this comment.
variable name formState is too generic imo
Hi @canbo206, kind reminder on this. PS: Also the issue link is not properly mentioned in the PR description. Let's fix that too. |
|
The linked issue reports over 200 occurrences, but this pr only fixes one. is this intentional? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10484 +/- ##
==========================================
+ Coverage 72.62% 72.85% +0.22%
==========================================
Files 469 470 +1
Lines 70633 71228 +595
Branches 240 240
==========================================
+ Hits 51300 51892 +592
- Misses 19222 19225 +3
Partials 111 111 🚀 New features to boost your workflow:
|
canbo206
changed the title
Hi @pavinduLakshan I started with one file to get the pattern and get initial feedback before scaling. I'm happy to fix additional files in this PR or open separate PRs for the remaining occurrences. Whichever one you prefer. |
Great, and thanks. Let's open separate prs for the remaining occurrences as that'd make it much easier to manage for both of us. |
3 participants
Purpose
This PR addresses the react-doctor/prefer-useReducer lint warning in
PreIssueAccessTokenActionConfigForm. The component had 5 separate useStatecalls managing related form state (
isAuthenticationUpdateFormState,authenticationType,isSubmitting,isHasRule, andrule). These have beenrefactored into a single useReducer hook with explicit TypeScript interfaces
for state and actions, improving readability and maintainability.
Related Issues
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.