Open
Conversation
Contributor
WalkthroughAdded defensive value handling and entity-type inference to Autocomplete: new isRecord guard and helpers, safer key/display/outcome logic for primitives and non-records, lastLoadedKeys tracking to avoid unnecessary reloads, and propagation of effectiveEntityType through metadata/DataTableProvider paths. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Autocomplete
participant DataTableProvider
participant API
User->>Autocomplete: open dropdown / select value
Autocomplete->>Autocomplete: compute effectiveEntityType (props or infer from value)
Autocomplete->>DataTableProvider: request metadata/data (includes effectiveEntityType, lastLoadedKeys)
DataTableProvider->>API: fetch rows/metadata (guarded query params)
API-->>DataTableProvider: return rows/metadata
DataTableProvider-->>Autocomplete: deliver rows/metadata
Autocomplete->>Autocomplete: apply isRecord / isSelectOption -> build display/key/outcome
Autocomplete-->>User: render labels / return selected value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-reactjs/src/components/autocomplete/index.tsx`:
- Around line 103-107: The computed effectiveEntityType is duplicated; add
effectiveEntityType?: string | IReferenceListIdentifier | null to
IAutocompleteBaseProps, compute it once in Autocomplete (reusing the existing
useMemo there) and pass it explicitly through props (or spread) into
AutocompleteInner, then remove the useMemo that computes effectiveEntityType
inside AutocompleteInner and read props.effectiveEntityType directly; update any
call sites or tests that construct props if needed.
- Around line 68-73: The displayValueFunc implementation is noisy: replace the
redundant !Boolean(value) with the simpler !value, and remove the unnecessary
optional chaining on value?.toString() after the isRecord guard (use
value.toString() or String(value.toString() ?? '') as appropriate) so the logic
in displayValueFunc (referenced props.displayValueFunc, displayPropName,
isRecord, getValueByPropertyName) remains identical but cleaner.
- Around line 109-116: The useMemo that computes keys (function named keys)
currently only depends on props.value but calls keyValueFunc(..., allData),
which can change and cause stale closures; update the dependency array to
include keyValueFunc and allData (and if keyValueFunc originates from props,
include props.keyValueFunc) so keys is recomputed whenever keyValueFunc or
allData change, ensuring downstream checks like allExist, allExistInTable, and
keysChanged use fresh key values.
In `@shesha-reactjs/src/designer-components/autocomplete/autocomplete.tsx`:
- Around line 63-94: The branch in outcomeValueFunc builds an entity reference
for any object because isRecord currently matches arrays; update the isRecord
implementation (in object.ts) to explicitly exclude arrays (use Array.isArray
check) and then add a defensive guard inside outcomeValueFunc right before the
isRecord branch: if (Array.isArray(item)) return item; so arrays are returned
unchanged instead of producing malformed {id:undefined,...}; keep the existing
isEntityReferenceId, typeof item !== 'object' checks and then use
getValueByPropertyName for id/_displayName as before for genuine records.
- Around line 86-92: The id extraction is redundant and wrongly uses || which
treats 0 as falsy; in the block inside isRecord(...) replace "item.id ||
getValueByPropertyName(item, 'id')" with a nullish-coalescing expression such as
"getValueByPropertyName(item, 'id') ?? item.id" (or simply
"getValueByPropertyName(item, 'id')" if you prefer) so that numeric ids like 0
are preserved; update the return in that function in autocomplete.tsx (the
isRecord branch) referring to getValueByPropertyName and item.id accordingly.
In `@shesha-reactjs/src/utils/object.ts`:
- Around line 10-11: The isRecord type guard incorrectly returns true for
arrays; update isRecord to exclude arrays (use Array.isArray) so it only accepts
plain objects, e.g., change the predicate in isRecord to check typeof value ===
'object' && value !== null && !Array.isArray(value); then run/adjust callers in
autocomplete/index.tsx and designer-components/autocomplete/autocomplete.tsx
(the code paths that access .id and ._displayName) to rely on the corrected
guard or add fallback checks if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
shesha-reactjs/src/components/autocomplete/index.tsxshesha-reactjs/src/designer-components/autocomplete/autocomplete.tsxshesha-reactjs/src/utils/index.tsshesha-reactjs/src/utils/object.ts
Contributor
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 (2)
shesha-reactjs/src/components/autocomplete/index.tsx (2)
121-141: 🧹 Nitpick | 🔵 TrivialCommented-out dependencies are a stale-closure hazard and code smell.
The
useEffecton line 121 readskeyValueFunc,outcomeValueFunc,allData, andsource?.tableDatainside the callback, but the dependency array on line 141 omits them (left as a comment). If any of these change, the effect will use stale references.The commented-out deps suggest they were intentionally removed to avoid re-trigger loops, but this should be documented with a
// NOTE:explaining the trade-off. If the functions are guaranteed stable (memoized with fixed deps), the risk is low—but removing the comment avoids confusion for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/components/autocomplete/index.tsx` around lines 121 - 141, The effect in useEffect reads keyValueFunc, outcomeValueFunc, allData and source?.tableData but omits them from the dependency array, creating a stale-closure hazard; either add those dependencies (keyValueFunc, outcomeValueFunc, allData, source?.tableData) to the array so the effect reruns when they change, or if you intentionally want to avoid reruns, add a clear // NOTE: explaining the trade-off and guarantee that keyValueFunc and outcomeValueFunc are stable/memoized and source.tableData changes are handled elsewhere; ensure selected.current and getNormalizedValues(props.value) behavior remains correct with whichever approach you choose.
49-49:⚠️ Potential issue | 🟠 MajorUncomment
useAvailableConstantsData()hook and add missing import —allDatais stubbed as an empty object.The hook call is commented out on line 49 and
useAvailableConstantsDatais not imported. As a result,allDatais always{}, which breaks any expression-based callbacks (keyValueFunc, outcomeValueFunc, displayValueFunc) that reference form data or global context. Every similar component (calendar, dataList, reactTable) properly calls this hook.The commented-out dependencies on line 141 suggest an incomplete fix. Restore the hook call, add the import, and include
allDatain the dependency arrays where functions use it, as shown in other components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/components/autocomplete/index.tsx` at line 49, Restore the real data hook by replacing the stubbed const allData = {} with a call to useAvailableConstantsData() and add the missing import for useAvailableConstantsData at the top of the file; then update the dependency arrays for the callbacks that reference it (keyValueFunc, outcomeValueFunc, displayValueFunc and any similar functions) to include allData so those memoized callbacks react to changes in global/form constants. Ensure you reference the existing symbols allData, useAvailableConstantsData, keyValueFunc, outcomeValueFunc, displayValueFunc when making the changes.
♻️ Duplicate comments (1)
shesha-reactjs/src/designer-components/autocomplete/autocomplete.tsx (1)
93-93: 🧹 Nitpick | 🔵 Trivial
getValueByPropertyName(item, 'id') ?? item.idis still redundant — both access the same top-level key.The switch from
||to??fixes the falsy-id concern from the prior review, but the fallbackitem.idis functionally identical togetValueByPropertyName(item, 'id')for a plain'id'key. Simplify to just one access.♻️ Proposed fix
- id: getValueByPropertyName(item, 'id') ?? item.id, + id: item.id,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/designer-components/autocomplete/autocomplete.tsx` at line 93, Remove the redundant fallback for the id field: replace the expression using both getValueByPropertyName(item, 'id') and item.id with a single access. Locate the assignment setting id (the line containing id: getValueByPropertyName(item, 'id') ?? item.id) and simplify it to use only getValueByPropertyName(item, 'id') or only item.id depending on whether you need the property-name lookup; ensure the chosen single access preserves the original null/undefined semantics that the code expects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-reactjs/src/components/autocomplete/index.tsx`:
- Around line 39-40: The type guard isSelectOption is too permissive because it
only checks for 'data'; update it to verify the presence and expected shapes of
all required ISelectOption members (check that 'value' and 'label' keys exist
and that label is a string and data is an object/record) so the guard
confidently narrows to ISelectOption; locate and modify the isSelectOption
function and ensure it still uses isRecord(value) as the first check and then
`in` checks and lightweight typeof/object checks for 'label' and 'data'.
In `@shesha-reactjs/src/designer-components/autocomplete/autocomplete.tsx`:
- Around line 129-132: onChangeInternal currently force-casts value to object
when calling customEvent.onChange, which bypasses type safety; add a type guard
in onChangeInternal (e.g., check typeof value === 'object' && value !== null)
before calling customEvent.onChange(value) and handle non-object values by
either calling customEvent.onChange with an appropriately typed wrapper or by
calling the original onChange(value) path; alternatively, if primitives are
valid inputs, update the customEvent.onChange signature to accept string |
number | object so no cast is necessary. Ensure references: onChangeInternal,
customEvent.onChange, and onChange are the points to modify.
---
Outside diff comments:
In `@shesha-reactjs/src/components/autocomplete/index.tsx`:
- Around line 121-141: The effect in useEffect reads keyValueFunc,
outcomeValueFunc, allData and source?.tableData but omits them from the
dependency array, creating a stale-closure hazard; either add those dependencies
(keyValueFunc, outcomeValueFunc, allData, source?.tableData) to the array so the
effect reruns when they change, or if you intentionally want to avoid reruns,
add a clear // NOTE: explaining the trade-off and guarantee that keyValueFunc
and outcomeValueFunc are stable/memoized and source.tableData changes are
handled elsewhere; ensure selected.current and getNormalizedValues(props.value)
behavior remains correct with whichever approach you choose.
- Line 49: Restore the real data hook by replacing the stubbed const allData =
{} with a call to useAvailableConstantsData() and add the missing import for
useAvailableConstantsData at the top of the file; then update the dependency
arrays for the callbacks that reference it (keyValueFunc, outcomeValueFunc,
displayValueFunc and any similar functions) to include allData so those memoized
callbacks react to changes in global/form constants. Ensure you reference the
existing symbols allData, useAvailableConstantsData, keyValueFunc,
outcomeValueFunc, displayValueFunc when making the changes.
---
Duplicate comments:
In `@shesha-reactjs/src/designer-components/autocomplete/autocomplete.tsx`:
- Line 93: Remove the redundant fallback for the id field: replace the
expression using both getValueByPropertyName(item, 'id') and item.id with a
single access. Locate the assignment setting id (the line containing id:
getValueByPropertyName(item, 'id') ?? item.id) and simplify it to use only
getValueByPropertyName(item, 'id') or only item.id depending on whether you need
the property-name lookup; ensure the chosen single access preserves the original
null/undefined semantics that the code expects.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
shesha-reactjs/src/components/autocomplete/index.tsxshesha-reactjs/src/components/autocomplete/models.tsxshesha-reactjs/src/designer-components/autocomplete/autocomplete.tsxshesha-reactjs/src/utils/object.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-reactjs/src/designer-components/autocomplete/autocomplete.tsx`:
- Around line 129-134: The onChangeInternal currently only calls
customEvent.onChange when value is an object, which skips custom handling for
primitive/free-text selections; update the guard to detect AntD runtime shape by
checking for the presence of the runtime property (use "'data' in value" or
"'data' in option") instead of only typeof/object checks so customEvent.onChange
is invoked for AntD option objects while primitives still call onChange(value);
adjust the conditional(s) in onChangeInternal accordingly so both
customEvent.onChange(customValue, optionValue) and onChange(value) are called in
the correct scenarios.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
96353
Summary by CodeRabbit
New Features
Bug Fixes