Add validator option for useStrongFrom and use it in few places #1651
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a Validator type and a Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
138-176:submit()returns a stale/undefined value instead of the latest outputInside
submit, youawaitthesubmitorchain, but then returnsubmitOutput, which is a React state value:await props .submitor(submissionData) .then((output) => { setSubmitOutput(output); if (props.onSubmitSuccess) { props.onSubmitSuccess(output); } }) .catch(/* ... */); return submitOutput;Because
setSubmitOutputis async,submitOutputstill holds the previous value when you return, so callers ofawait strongForm.submit()don't reliably get the latest output (undefined on first call, stale on subsequent calls).You can keep the existing error handling while returning the fresh output:
- setSubmitting(true); - try { - await props - .submitor(submissionData) - .then((output) => { - setSubmitOutput(output); - if (props.onSubmitSuccess) { - props.onSubmitSuccess(output); - } - }) - .catch((error) => { + setSubmitting(true); + try { + const output = await props + .submitor(submissionData) + .then((output) => { + setSubmitOutput(output); + if (props.onSubmitSuccess) { + props.onSubmitSuccess(output); + } + return output; + }) + .catch((error) => { if (error instanceof RequestBodyParseError) { setSubmitError( new Error( "Some of the field values are invalid" ) as SubmissionError ); setInputErrors(error.error.formErrors as InputErrors); } else if (error instanceof RequestQueryParamsParseError) { @@ } else if (error instanceof ParseError) { setSubmitError( new Error( "Request succeeded, but the response was invalid" ) as SubmissionError ); setInputErrors(error.error.formErrors as InputErrors); throw error; } else { throw error; } - }); - return submitOutput; + }); + return output;This preserves the existing error mapping and callbacks but makes
submit()'s return value reliable.
🧹 Nitpick comments (2)
apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx (1)
140-155: Service account form wiring looks good; consider simplifying submit hook‑upUsing
validators: { nickname: { required: true } }plusstrongForm.isReadyin both theonSubmithandler and the disabled state gives a consistent required-field check and avoids the old ad‑hoc validation. Functionally solid.You can simplify by letting the button submit the form instead of calling
strongForm.submitdirectly:- <form - className="service-accounts__form" - onSubmit={(e) => { - e.preventDefault(); - if (strongForm.isReady) { - strongForm.submit(); - } - }} - > + <form + className="service-accounts__form" + onSubmit={(e) => { + e.preventDefault(); + if (strongForm.isReady) { + strongForm.submit(); + } + }} + > @@ - {serviceAccountAdded ? null : ( - <Modal.Footer> - <NewButton onClick={strongForm.submit} disabled={!strongForm.isReady}> - Add Service Account - </NewButton> - </Modal.Footer> - )} + {serviceAccountAdded ? null : ( + <Modal.Footer> + <NewButton type="submit" disabled={!strongForm.isReady}> + Add Service Account + </NewButton> + </Modal.Footer> + )}This keeps all submission logic in the form submit handler and avoids duplicate pathways.
Also applies to: 200-208, 235-238
apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
8-10: Validator/isReady design is fine; tighten dependencies and typing for future‑proofingThe
Validatorshape +validatorsprop andisReadymemo are straightforward and easy to extend. A couple of small robustness improvements:
- Include validators in the
useMemodeps
isReadyuses bothprops.inputsandprops.validators, but onlyprops.inputsis in the dependency array. If someone ever passes dynamic validators,isReadycould become stale.- const isReady = useMemo(() => { + const isReady = useMemo(() => { if (!props.validators) return true; @@ - return true; - }, [props.inputs]); + return true; + }, [props.inputs, props.validators]);
- Improve key typing inside the validator loop (optional)
Using
for (const key in props.validators)gives you astringkey andprops.inputs[key]is only loosely typed. You could make this a bit safer:const isReady = useMemo(() => { if (!props.validators) return true; (Object.keys(props.validators) as (keyof Inputs)[]).forEach((key) => { const validator = props.validators?.[key]; const value = props.inputs[key]; if (validator?.required) { if (typeof value === "string" && value.trim() === "") return false; if (value === undefined || value === null) return false; } }); return true; }, [props.inputs, props.validators]);Not mandatory today, but it helps if more validators are added later.
Also applies to: 20-28, 61-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cyberstorm-remix/app/settings/teams/Teams.tsx(2 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx(2 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx(3 hunks)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1651 +/- ##
==========================================
- Coverage 10.82% 10.81% -0.01%
==========================================
Files 315 315
Lines 22684 22704 +20
Branches 463 463
==========================================
Hits 2455 2455
- Misses 20229 20249 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Oksamies
force-pushed
the
12-03-add_validator_option_for_usestrongfrom_and_use_it_in_few_places
branch
from
2471673 to
2843cbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx (1)
132-155: Validator wiring andisReadygating look solid
validators: { nickname: { required: true } }plus checkingstrongForm.isReadyin both the formonSubmitand the footer buttondisabledstate gives you a consistent, centralized readiness check and avoids firingsubmitorfor obviously invalid input. This integrates cleanly with the newuseStrongFormAPI and keeps the Enter‑to‑submit path aligned with the button click path.Only small nit (optional): if your
requiredvalidator does not already trim, and you care about rejecting whitespace‑only nicknames, consider either normalizingformInputs.nicknameon change or configuring the validator to trim, instead of relying ondata.nickname.trim()solely insubmitor.Also applies to: 200-208, 235-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cyberstorm-remix/app/settings/teams/Teams.tsx(2 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx(2 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx(3 hunks)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts
- apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (1)
apps/cyberstorm-remix/app/settings/teams/Teams.tsx (1)
157-184: Create Team form now correctly uses centralized validationAdding
validators: { name: { required: true } }and wiring the Create button asdisabled={!strongForm.isReady}while still callingstrongForm.submit().then(() => setOpen(false))gives you a clean, unified readiness check and restores proper submission behavior. This is consistent with the newuseStrongFormpattern used elsewhere.No further issues from my side here.
Also applies to: 225-235
No description provided.