fix: readOnly for PersonLookup and OrganisationLookup#4181
fix: readOnly for PersonLookup and OrganisationLookup#4181walldenfilippa wants to merge 4 commits intomainfrom
Conversation
walldenfilippa
added
kind/feature-request
📝 WalkthroughWalkthroughOrganisationLookupComponent and PersonLookupComponent are updated to respect a ChangesRead-Only State Support in Lookup Components
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. 🔧 Microsoft Presidio Analyzer (2.2.362)src/layout/OrganisationLookup/OrganisationLookupComponent.tsxMicrosoft Presidio Analyzer failed to scan this file 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 |
walldenfilippa
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/layout/PersonLookup/PersonLookupComponent.tsx (1)
235-239:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnter-key handlers fire unconditionally in read-only mode, producing spurious validation errors.
Same root cause as in
OrganisationLookupComponent: bothonKeyDownhandlers callhandleSubmit()without checkingreadOnly. WithreadOnly=true, pressing Enter on either input triggershandleValidateSsn('')/handleValidateName(''), sets error state, and renders error UI beneath read-only fields.🐛 Proposed fix (apply to both handlers)
- onKeyDown={async (ev) => { - if (ev.key === 'Enter') { - await handleSubmit(); - } - }} + onKeyDown={async (ev) => { + if (ev.key === 'Enter' && !readOnly) { + await handleSubmit(); + } + }}Also applies to: 289-293
🤖 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/layout/PersonLookup/PersonLookupComponent.tsx` around lines 235 - 239, The onKeyDown handlers in PersonLookupComponent are calling handleSubmit() unconditionally, causing validation to run in read-only mode; update both onKeyDown handlers (the ones that currently call handleSubmit()) to first check the component's readOnly prop/state and only call await handleSubmit() when readOnly is falsy, so Enter presses in read-only mode are ignored and do not trigger handleValidateSsn/handleValidateName or set error state.src/layout/OrganisationLookup/OrganisationLookupComponent.tsx (1)
165-169:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnter-key handler fires in read-only mode and can produce spurious validation errors.
When
readOnlyistrue, theNumericInputis still focusable and firesonKeyDown. Pressing Enter callshandleSubmit()→handleValidateOrgnr(tempOrgNr). SincetempOrgNris''(the user cannot type into a read-only field), validation fails andsetOrgNrErrorsis set, rendering error UI below what should be a clean read-only input.🐛 Proposed fix
- onKeyDown={async (ev) => { - if (ev.key === 'Enter') { - await handleSubmit(); - } - }} + onKeyDown={async (ev) => { + if (ev.key === 'Enter' && !readOnly) { + await handleSubmit(); + } + }}🤖 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/layout/OrganisationLookup/OrganisationLookupComponent.tsx` around lines 165 - 169, The Enter-key handler on the NumericInput should not trigger validation when the component is read-only; update the onKeyDown callback to first check the readOnly prop (or the input's readOnly state) and return early if true, so it only calls handleSubmit() when readOnly is false. Specifically, modify the onKeyDown in OrganisationLookupComponent to guard before invoking handleSubmit (which calls handleValidateOrgnr using tempOrgNr) to prevent setOrgNrErrors from running for read-only inputs.
🧹 Nitpick comments (1)
src/layout/PersonLookup/PersonLookupComponent.tsx (1)
229-229: 💤 Low value
isFetchingis absent from thereadOnlycondition — inconsistent withOrganisationLookupComponent.
OrganisationLookupComponentusesreadOnly={hasSuccessfullyFetched || isFetching || readOnly}, locking the input during the in-flight API call. BothPersonLookupinputs omitisFetching, leaving the SSN and surname fields editable while a lookup is in progress.♻️ Proposed change
- readOnly={hasSuccessfullyFetched || readOnly} + readOnly={hasSuccessfullyFetched || isFetching || readOnly}Apply the same diff to both the
NumericInput(line 229) and theInput(line 283).Also applies to: 283-283
🤖 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/layout/PersonLookup/PersonLookupComponent.tsx` at line 229, Update the readOnly prop on PersonLookupComponent's inputs to match OrganisationLookupComponent: include isFetching in the condition so the fields are locked while the lookup is in-flight. Specifically, change the NumericInput (currently using readOnly={hasSuccessfullyFetched || readOnly}) and the Input (similarly missing isFetching) to use readOnly={hasSuccessfullyFetched || isFetching || readOnly} so both SSN and surname become non-editable during loading; reference the NumericInput and Input elements and the variables hasSuccessfullyFetched, isFetching, and readOnly when making the edits.
🤖 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.
Outside diff comments:
In `@src/layout/OrganisationLookup/OrganisationLookupComponent.tsx`:
- Around line 165-169: The Enter-key handler on the NumericInput should not
trigger validation when the component is read-only; update the onKeyDown
callback to first check the readOnly prop (or the input's readOnly state) and
return early if true, so it only calls handleSubmit() when readOnly is false.
Specifically, modify the onKeyDown in OrganisationLookupComponent to guard
before invoking handleSubmit (which calls handleValidateOrgnr using tempOrgNr)
to prevent setOrgNrErrors from running for read-only inputs.
In `@src/layout/PersonLookup/PersonLookupComponent.tsx`:
- Around line 235-239: The onKeyDown handlers in PersonLookupComponent are
calling handleSubmit() unconditionally, causing validation to run in read-only
mode; update both onKeyDown handlers (the ones that currently call
handleSubmit()) to first check the component's readOnly prop/state and only call
await handleSubmit() when readOnly is falsy, so Enter presses in read-only mode
are ignored and do not trigger handleValidateSsn/handleValidateName or set error
state.
---
Nitpick comments:
In `@src/layout/PersonLookup/PersonLookupComponent.tsx`:
- Line 229: Update the readOnly prop on PersonLookupComponent's inputs to match
OrganisationLookupComponent: include isFetching in the condition so the fields
are locked while the lookup is in-flight. Specifically, change the NumericInput
(currently using readOnly={hasSuccessfullyFetched || readOnly}) and the Input
(similarly missing isFetching) to use readOnly={hasSuccessfullyFetched ||
isFetching || readOnly} so both SSN and surname become non-editable during
loading; reference the NumericInput and Input elements and the variables
hasSuccessfullyFetched, isFetching, and readOnly when making the edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d21c5182-b2aa-4709-a1a0-95c90b0f1bec
📒 Files selected for processing (2)
src/layout/OrganisationLookup/OrganisationLookupComponent.tsxsrc/layout/PersonLookup/PersonLookupComponent.tsx
walldenfilippa
added
kind/product-feature
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/layout/OrganisationLookup/OrganisationLookupComponent.tsx`:
- Around line 166-167: The submit and clear handlers (handleSubmit and
handleClear) still perform state mutations (e.g., calling setValue) even when
the component is read-only; add an explicit readOnly guard at the start of both
functions to return early if readOnly is true so mutations cannot occur,
mirroring the render/key gating already present (also update any helper
functions they call to respect readOnly if needed and ensure the Enter key
handler remains unchanged).
In `@src/layout/PersonLookup/PersonLookupComponent.tsx`:
- Around line 236-237: The Enter-key branch currently checks readOnly before
calling handleSubmit, but the mutation handlers themselves must defensively
refuse to mutate state; add an early guard at the start of handleSubmit and
handleClear (and any other mutation functions that call setValue) that returns
immediately if readOnly is true to prevent async/stale paths from performing
setValue or other writes; reference the handleSubmit and handleClear functions
and any places where setValue is invoked and ensure they check the readOnly flag
before mutating.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90ad5b5d-84de-4b4e-8d9a-040cac832a27
📒 Files selected for processing (2)
src/layout/OrganisationLookup/OrganisationLookupComponent.tsxsrc/layout/PersonLookup/PersonLookupComponent.tsx
| if (ev.key === 'Enter' && !readOnly) { | ||
| await handleSubmit(); |
There was a problem hiding this comment.
Mirror read-only enforcement inside submit/clear handlers.
The new render/key gating is correct, but handleSubmit/handleClear still allow setValue writes if called while read-only. Add explicit guards in those handlers so read-only is enforced at the mutation boundary too.
Suggested patch
@@
async function handleSubmit() {
+ if (readOnly) {
+ return;
+ }
const isValid = handleValidateOrgnr(tempOrgNr);
@@
const { data } = await performLookup();
+ if (readOnly) {
+ return;
+ }
if (data?.org) {
@@
function handleClear() {
+ if (readOnly) {
+ return;
+ }
setValue('organisation_lookup_orgnr', '');Also applies to: 180-200
🤖 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/layout/OrganisationLookup/OrganisationLookupComponent.tsx` around lines
166 - 167, The submit and clear handlers (handleSubmit and handleClear) still
perform state mutations (e.g., calling setValue) even when the component is
read-only; add an explicit readOnly guard at the start of both functions to
return early if readOnly is true so mutations cannot occur, mirroring the
render/key gating already present (also update any helper functions they call to
respect readOnly if needed and ensure the Enter key handler remains unchanged).
| if (ev.key === 'Enter' && !readOnly) { | ||
| await handleSubmit(); |
There was a problem hiding this comment.
Add hard read-only guards in mutation handlers, not only in UI triggers.
Good UI gating, but handleSubmit/handleClear can still write with setValue if invoked while readOnly is true (including async/stale paths). Add defensive guards inside those handlers to enforce no data mutation in read-only mode.
Suggested patch
@@
async function handleSubmit() {
+ if (readOnly) {
+ return;
+ }
const isNameValid = handleValidateName(tempName);
@@
const { data } = await performLookup();
+ if (readOnly) {
+ return;
+ }
if (data?.person) {
@@
function handleClear() {
+ if (readOnly) {
+ return;
+ }
if (dataModelBindings.person_lookup_ssn) {
setValue('person_lookup_ssn', '');Also applies to: 290-291, 308-328
🤖 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/layout/PersonLookup/PersonLookupComponent.tsx` around lines 236 - 237,
The Enter-key branch currently checks readOnly before calling handleSubmit, but
the mutation handlers themselves must defensively refuse to mutate state; add an
early guard at the start of handleSubmit and handleClear (and any other mutation
functions that call setValue) that returns immediately if readOnly is true to
prevent async/stale paths from performing setValue or other writes; reference
the handleSubmit and handleClear functions and any places where setValue is
invoked and ensure they check the readOnly flag before mutating.
|
1 participant
Description
FIxes readOnly for PersonLookup and OrganisationLookup
lookUpReadOnly.mp4
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit