Manage mapping via directory server#186
Conversation
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR switches mapping operations to ID-based flow, adds explore API configuration, expands the new-mapping dialog with directory and description inputs, replaces the rename dialog, and updates containers, navigation, and localized messages. ChangesMapping ID flow and dialog updates
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/components/2-molecules/rhf/dialogs/rename-mapping/rename-mapping-dialog-utils.ts (1)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the DIRECTORY field requirement more clearly.
The
FieldConstants.DIRECTORYfield is defined in the schema but never rendered in the form UI. The comment indicates it's "not used" but required for internaluseUniqueNameValidationbehavior. This creates implicit coupling between the form schema and commons-ui library internals that may confuse future maintainers.📝 Suggested documentation improvement
Consider expanding the comment to explain why this field must be present:
[FieldConstants.DIRECTORY]: yup .string() - .notRequired() /* not used (use activeDirectory instead) but should be coherent to empty data */, + .notRequired() /* Required in schema for UniqueNameInput validation behavior, but not rendered in UI. Parent directory is passed via activeDirectory prop instead. */,🤖 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/components/2-molecules/rhf/dialogs/rename-mapping/rename-mapping-dialog-utils.ts` around lines 14 - 19, Expand the comment for the FieldConstants.DIRECTORY field in the renameMappingDialogSchema to provide clearer documentation about why this field must be present in the schema despite not being rendered in the UI form. The comment should explain that this field is required internally by the useUniqueNameValidation function from the commons-ui library and must remain synchronized with the expected data structure, even though it appears unused in the form rendering. This will help future maintainers understand the implicit coupling between the form schema and the commons-ui library internals.
🤖 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/components/2-molecules/rhf/dialogs/new-mapping/NewMappingDialog.tsx`:
- Around line 7-43: The NewMappingDialog.tsx file imports UUID from the
Node.js-only module node:crypto on line 26, which is semantically incorrect for
browser environments. Replace the UUID import from node:crypto with a proper
UUID type from a web-compatible source such as a UUID library, or define it
locally as a type alias (e.g., type UUID = string) if appropriate for your
codebase. Ensure the NewMappingDialogProps type definition continues to work
correctly with the updated UUID type.
In `@src/components/2-molecules/rhf/dialogs/new-mapping/NewMappingForm.tsx`:
- Around line 8-33: Remove the import statement that brings in UUID from the
node:crypto module, as this violates the browser/Node.js boundary. Replace the
UUID type with a local type definition or use the string type directly
throughout the NewMappingForm component, aligning with the patterns used
elsewhere in the codebase where UUID is represented as a string type.
In
`@src/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingDialog.tsx`:
- Line 8: The RenameMappingDialog component is importing the UUID type from
node:crypto, which is a Node.js module unavailable in browser environments and
will cause build or runtime errors. Remove the import from node:crypto and
replace it with either a type-only import defining UUID as a string type, or use
a UUID type exported from commons-ui if one is available. Ensure the UUID type
definition works in a browser/React context.
- Around line 69-74: The fetchDirectoryElementPath promise in the useEffect hook
has no error handling, which means setInitialized(true) is never called if the
fetch fails, leaving the form unrendered with no user feedback. Add a .catch()
handler (or use try-catch with async-await) to the fetchDirectoryElementPath
promise chain that ensures setInitialized(true) is called even when the fetch
fails, and include appropriate error handling or feedback to inform the user
when the operation fails.
- Line 71: The uniqueness validation for root-level mappings is skipped because
when path.length equals 1, the setParentDirectory call results in undefined
being passed as activeDirectory to UniqueNameInput. The useUniqueNameValidation
hook then skips validation entirely due to its condition checking if directory
exists. Fix this by either providing a fallback directory identifier (such as a
root-level constant) when activeDirectory would be undefined in the
setParentDirectory call, or by modifying the useUniqueNameValidation hook to
explicitly handle and validate root-level mappings even when the directory
parameter is undefined, ensuring duplicate names cannot be created at the root
level.
In `@src/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingForm.tsx`:
- Line 10: The import statement is importing UUID from node:crypto, which is a
Node.js module unavailable in browser environments where this React component
(RenameMappingForm) executes. Replace this import by either removing it and
defining UUID as a string type locally in the file, using a type-only import
syntax if needed, or checking if the commons-ui package provides a UUID type
that can be imported instead.
In `@src/rest/mappingsAPI.js`:
- Around line 166-170: In the createMapping function, add conditional guards
before appending the optional parameters description and parentDirectoryUuid to
urlSearchParams. Check if description is defined before calling
urlSearchParams.append for description, and similarly check if
parentDirectoryUuid is defined before appending it. This will prevent
URLSearchParams from coercing undefined values to the string "undefined" when
these optional parameters are not provided by the caller.
---
Nitpick comments:
In
`@src/components/2-molecules/rhf/dialogs/rename-mapping/rename-mapping-dialog-utils.ts`:
- Around line 14-19: Expand the comment for the FieldConstants.DIRECTORY field
in the renameMappingDialogSchema to provide clearer documentation about why this
field must be present in the schema despite not being rendered in the UI form.
The comment should explain that this field is required internally by the
useUniqueNameValidation function from the commons-ui library and must remain
synchronized with the expected data structure, even though it appears unused in
the form rendering. This will help future maintainers understand the implicit
coupling between the form schema and the commons-ui library internals.
🪄 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: d072e64a-dc0c-4440-b110-0cd3ae7cee49
📒 Files selected for processing (14)
.env.env.developmentsrc/components/2-molecules/NavigationMenu.jsxsrc/components/2-molecules/RenameDialog.jsxsrc/components/2-molecules/rhf/dialogs/new-mapping/NewMappingDialog.tsxsrc/components/2-molecules/rhf/dialogs/new-mapping/NewMappingForm.tsxsrc/components/2-molecules/rhf/dialogs/new-mapping/new-mapping-dialog-utils.tssrc/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingDialog.tsxsrc/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingForm.tsxsrc/components/2-molecules/rhf/dialogs/rename-mapping/rename-mapping-dialog-utils.tssrc/containers/MappingContainer.jsxsrc/containers/MenuContainer.jsxsrc/redux/slices/Mapping.jssrc/rest/mappingsAPI.js
💤 Files with no reviewable changes (1)
- src/components/2-molecules/RenameDialog.jsx
| import { | ||
| CustomMuiDialog, | ||
| DIRECTORY_ITEM, | ||
| DIRECTORY_ITEM_ID, | ||
| FieldConstants, | ||
| isObjectEmpty, | ||
| Nullable, | ||
| } from '@gridsuite/commons-ui'; | ||
| import { FieldValues, useForm } from 'react-hook-form'; | ||
| import { FILE_SELECTOR, MAPPING_NAME, newMappingDialogEmpty, newMappingDialogSchema } from './new-mapping-dialog-utils'; | ||
| import { | ||
| FILE_SELECTOR, | ||
| MAPPING_NAME, | ||
| newMappingDialogEmpty, | ||
| NewMappingDialogForm, | ||
| newMappingDialogSchema, | ||
| } from './new-mapping-dialog-utils'; | ||
| import { yupResolver } from '@hookform/resolvers/yup'; | ||
| import NewMappingForm from './NewMappingForm'; | ||
| import { OperationType } from '../../../../../utils/types'; | ||
| import { UUID } from 'node:crypto'; | ||
|
|
||
| type NewMappingDialogProps = { | ||
| onClose: () => void; | ||
| open: boolean; | ||
| onSubmit: (params: { operationType: OperationType; file: File; name: string }) => void; | ||
| onSubmit: (params: { | ||
| operationType: OperationType; | ||
| file: File; | ||
| name: string; | ||
| description: string; | ||
| parentDirectoryUuid: UUID; | ||
| }) => void; | ||
| }; | ||
|
|
||
| function NewMappingDialog({ onClose, open, onSubmit }: NewMappingDialogProps) { | ||
| const formMethods = useForm({ | ||
| const formMethods = useForm<Nullable<NewMappingDialogForm>>({ | ||
| defaultValues: newMappingDialogEmpty, | ||
| resolver: yupResolver(newMappingDialogSchema), | ||
| resolver: yupResolver<Nullable<NewMappingDialogForm>>(newMappingDialogSchema), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if node:crypto UUID is used only as a type across new-mapping dialog files
rg -nP "from ['\"]node:crypto['\"]" src/components/2-molecules/rhf/dialogs/new-mapping/
rg -nP '\bUUID\b' src/components/2-molecules/rhf/dialogs/new-mapping/ -A2 -B2Repository: gridsuite/griddyna-app
Length of output: 3337
Replace node:crypto UUID import with a web-compatible UUID type.
Line 26 imports UUID from node:crypto, a Node.js-only module. Browser code should not import from Node.js standard library modules. While the current usage is type-only (preventing runtime errors), this import is semantically incorrect for browser environments.
Use a proper UUID type from a web-compatible source (e.g., from a UUID library, or define as type UUID = string if appropriate for your use case).
🤖 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/components/2-molecules/rhf/dialogs/new-mapping/NewMappingDialog.tsx`
around lines 7 - 43, The NewMappingDialog.tsx file imports UUID from the
Node.js-only module node:crypto on line 26, which is semantically incorrect for
browser environments. Replace the UUID import from node:crypto with a proper
UUID type from a web-compatible source such as a UUID library, or define it
locally as a type alias (e.g., type UUID = string) if appropriate for your
codebase. Ensure the NewMappingDialogProps type definition continues to work
correctly with the updated UUID type.
| import { | ||
| DescriptionField, | ||
| DIRECTORY_ITEM, | ||
| DIRECTORY_ITEM_ID, | ||
| DirectoryItemInput, | ||
| DirectoryItemSchema, | ||
| ElementType, | ||
| FieldConstants, | ||
| RadioInput, | ||
| UniqueNameInput, | ||
| } from '@gridsuite/commons-ui'; | ||
| import { FILE_SELECTOR, MAPPING_NAME } from './new-mapping-dialog-utils'; | ||
| import FileInputSelector from '../../inputs/FileInputSelector'; | ||
| import { usePrefilledName } from '../../hooks/usePrefilledName'; | ||
| import { useWatch } from 'react-hook-form'; | ||
| import { OperationType } from '../../../../../utils/types'; | ||
| import { useIntl } from 'react-intl'; | ||
| import { UUID } from 'node:crypto'; | ||
|
|
||
| const ADD_MAPPING_OPTIONS = [ | ||
| { id: OperationType.NEW, label: 'emptyMapping' }, | ||
| { id: OperationType.IMPORT, label: 'importMapping' }, | ||
| ]; | ||
|
|
||
| function NewMappingForm() { | ||
| const intl = useIntl(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify UUID from node:crypto is used only as a type, not a runtime value
rg -nP '\bUUID\b' src/components/2-molecules/rhf/dialogs/new-mapping/NewMappingForm.tsxRepository: gridsuite/griddyna-app
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Check full file content to understand context better
cat -n src/components/2-molecules/rhf/dialogs/new-mapping/NewMappingForm.tsx | head -80Repository: gridsuite/griddyna-app
Length of output: 3844
🏁 Script executed:
#!/bin/bash
# Search for other UUID type definitions or imports in the codebase
rg -t tsx -t ts 'UUID' --no-heading -B 1 | head -50Repository: gridsuite/griddyna-app
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Check if `@gridsuite/commons-ui` exports UUID
fd -e d.ts -e ts node_modules/@gridsuite/commons-ui 2>/dev/null | xargs grep -l "UUID" 2>/dev/null | head -5Repository: gridsuite/griddyna-app
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Search for UUID usage patterns in other files - fixed command
rg 'UUID' src/ --include '*.ts' --include '*.tsx' -B 2 -A 2 | head -60Repository: gridsuite/griddyna-app
Length of output: 602
🏁 Script executed:
#!/bin/bash
# Check if there's a local type definition or if UUID should come from commons-ui
rg 'DIRECTORY_ITEM_ID' src/ --include '*.ts' --include '*.tsx' -B 1 -A 1 | head -40Repository: gridsuite/griddyna-app
Length of output: 602
🏁 Script executed:
#!/bin/bash
# Search for UUID usage - correct rg syntax
rg 'UUID' src/ -g '*.tsx' -g '*.ts' -B 2 -A 2 | head -80Repository: gridsuite/griddyna-app
Length of output: 6767
🏁 Script executed:
#!/bin/bash
# Check what DIRECTORY_ITEM_ID is used for and its type
rg 'DIRECTORY_ITEM_ID' src/ -g '*.tsx' -g '*.ts' -B 1 -A 1 | head -60Repository: gridsuite/griddyna-app
Length of output: 1534
🏁 Script executed:
#!/bin/bash
# Check the actual type definition in commons-ui package
find node_modules/@gridsuite/commons-ui -name '*.d.ts' -o -name 'index.ts' 2>/dev/null | head -3 | xargs grep -l 'DIRECTORY_ITEM_ID\|UUID' 2>/dev/nullRepository: gridsuite/griddyna-app
Length of output: 164
Remove the node:crypto import and use a local type definition or string type for UUID.
Line 25 imports UUID from node:crypto, a Node.js-only module. While TypeScript erases type annotations at runtime and prevents immediate errors, this import violates the browser/Node.js boundary and indicates the type should be defined locally or represented as string.
The same pattern exists in multiple files (NewMappingDialog.tsx, RenameMappingForm.tsx, RenameMappingDialog.tsx). Consider replacing with a local UUID type alias or using string directly, as done elsewhere in the codebase.
🤖 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/components/2-molecules/rhf/dialogs/new-mapping/NewMappingForm.tsx` around
lines 8 - 33, Remove the import statement that brings in UUID from the
node:crypto module, as this violates the browser/Node.js boundary. Replace the
UUID type with a local type definition or use the string type directly
throughout the NewMappingForm component, aligning with the patterns used
elsewhere in the codebase where UUID is represented as a string type.
| useEffect(() => { | ||
| fetchDirectoryElementPath(mappingId).then((path: ElementAttributes[]) => { | ||
| setParentDirectory(path[path.length - 2]?.elementUuid); | ||
| setInitialized(true); | ||
| }); | ||
| }, [mappingId]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add error handling for fetchDirectoryElementPath.
The fetchDirectoryElementPath call has no error handling. If the fetch fails or rejects, setInitialized(true) is never called, and the form never renders (line 94 conditionally renders based on initialized). Users would see an empty dialog with no feedback or error message.
🛡️ Proposed fix
Add error handling and feedback:
useEffect(() => {
fetchDirectoryElementPath(mappingId).then((path: ElementAttributes[]) => {
setParentDirectory(path[path.length - 2]?.elementUuid);
setInitialized(true);
+ }).catch((error) => {
+ console.error('Failed to fetch parent directory:', error);
+ // Could also show a snackbar error or close the dialog
+ setInitialized(true); // Initialize anyway to show the form
});
}, [mappingId]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| fetchDirectoryElementPath(mappingId).then((path: ElementAttributes[]) => { | |
| setParentDirectory(path[path.length - 2]?.elementUuid); | |
| setInitialized(true); | |
| }); | |
| }, [mappingId]); | |
| useEffect(() => { | |
| fetchDirectoryElementPath(mappingId).then((path: ElementAttributes[]) => { | |
| setParentDirectory(path[path.length - 2]?.elementUuid); | |
| setInitialized(true); | |
| }).catch((error) => { | |
| console.error('Failed to fetch parent directory:', error); | |
| // Could also show a snackbar error or close the dialog | |
| setInitialized(true); // Initialize anyway to show the form | |
| }); | |
| }, [mappingId]); |
🤖 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/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingDialog.tsx`
around lines 69 - 74, The fetchDirectoryElementPath promise in the useEffect
hook has no error handling, which means setInitialized(true) is never called if
the fetch fails, leaving the form unrendered with no user feedback. Add a
.catch() handler (or use try-catch with async-await) to the
fetchDirectoryElementPath promise chain that ensures setInitialized(true) is
called even when the fetch fails, and include appropriate error handling or
feedback to inform the user when the operation fails.
| const [parentDirectory, setParentDirectory] = useState<UUID | undefined>(); | ||
| useEffect(() => { | ||
| fetchDirectoryElementPath(mappingId).then((path: ElementAttributes[]) => { | ||
| setParentDirectory(path[path.length - 2]?.elementUuid); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for UniqueNameInput usage with undefined or optional activeDirectory
rg -A5 -B5 "activeDirectory.*undefined|activeDirectory\?" node_modules/@gridsuite/commons-ui --type ts --type tsx 2>/dev/null || \
rg -A5 -B5 "activeDirectory.*undefined|activeDirectory\?" --type ts --type tsxRepository: gridsuite/griddyna-app
Length of output: 188
🏁 Script executed:
# First, find the file and understand its structure
fd -type f -name "RenameMappingDialog.tsx" | head -20Repository: gridsuite/griddyna-app
Length of output: 331
🏁 Script executed:
# Once we have the file, let's examine the context around line 71
if [ -f "src/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingDialog.tsx" ]; then
sed -n '50,90p' "src/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingDialog.tsx"
fiRepository: gridsuite/griddyna-app
Length of output: 1534
🏁 Script executed:
# Search for UniqueNameInput component definition and usage
rg "UniqueNameInput" --type-list | head -5
rg "UniqueNameInput" -A 3 -B 3 | head -50Repository: gridsuite/griddyna-app
Length of output: 550
🏁 Script executed:
# Find more of the RenameMappingDialog.tsx file to see the full return statement
wc -l "src/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingDialog.tsx"
sed -n '71,150p' "src/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingDialog.tsx"Repository: gridsuite/griddyna-app
Length of output: 1060
🏁 Script executed:
# Search for UniqueNameInput component definition
fd -name "*UniqueNameInput*" 2>/dev/nullRepository: gridsuite/griddyna-app
Length of output: 160
🏁 Script executed:
# Search for how UniqueNameInput is used (activeDirectory prop)
rg "UniqueNameInput" -A 5 -B 2Repository: gridsuite/griddyna-app
Length of output: 160
🏁 Script executed:
# Find RenameMappingForm
fd -name "*RenameMappingForm*"Repository: gridsuite/griddyna-app
Length of output: 391
🏁 Script executed:
# Search for where RenameMappingForm and UniqueNameInput are located
rg "RenameMappingForm|UniqueNameInput" -lRepository: gridsuite/griddyna-app
Length of output: 160
🏁 Script executed:
# Look in the same directory for related files
ls -la "src/components/2-molecules/rhf/dialogs/rename-mapping/"Repository: gridsuite/griddyna-app
Length of output: 496
🏁 Script executed:
# Read RenameMappingForm.tsx to see how parentDirectory is used
cat -n "src/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingForm.tsx"Repository: gridsuite/griddyna-app
Length of output: 1479
🏁 Script executed:
# Search for UniqueNameInput in the entire codebase
grep -r "UniqueNameInput" --include="*.tsx" --include="*.ts" .Repository: gridsuite/griddyna-app
Length of output: 1223
🏁 Script executed:
# Check UniqueNameInput prop definition in commons-ui
cat -n "node_modules/@gridsuite/commons-ui/dist/components/ui/reactHookForm/text/UniqueNameInput.d.ts"Repository: gridsuite/griddyna-app
Length of output: 1254
🏁 Script executed:
# Also check the actual implementation if available
find node_modules/@gridsuite/commons-ui -name "UniqueNameInput*" -type fRepository: gridsuite/griddyna-app
Length of output: 346
🏁 Script executed:
# Check the actual implementation of UniqueNameInput
cat -n "node_modules/@gridsuite/commons-ui/dist/components/ui/reactHookForm/text/UniqueNameInput.js" | head -100Repository: gridsuite/griddyna-app
Length of output: 2621
🏁 Script executed:
# Search for useUniqueNameValidation implementation
find node_modules/@gridsuite/commons-ui -name "*unique-name-validation*" -type fRepository: gridsuite/griddyna-app
Length of output: 674
🏁 Script executed:
# Check if useUniqueNameValidation handles undefined activeDirectory
grep -r "useUniqueNameValidation\|activeDirectory" node_modules/@gridsuite/commons-ui/dist/hooks --include="*.js" | head -30Repository: gridsuite/griddyna-app
Length of output: 602
🏁 Script executed:
# Get full implementation of useUniqueNameValidation
cat -n "node_modules/@gridsuite/commons-ui/dist/hooks/use-unique-name-validation.js"Repository: gridsuite/griddyna-app
Length of output: 4498
🏁 Script executed:
# Check the rename-mapping-dialog-utils to understand form structure
cat -n "src/components/2-molecules/rhf/dialogs/rename-mapping/rename-mapping-dialog-utils.ts"Repository: gridsuite/griddyna-app
Length of output: 1397
🏁 Script executed:
# Check if there's a DIRECTORY field in the form or if selectedDirectory is available
grep -r "FieldConstants.DIRECTORY\|DIRECTORY" "src/components/2-molecules/rhf/dialogs/rename-mapping/" -A 2 -B 2Repository: gridsuite/griddyna-app
Length of output: 1503
🏁 Script executed:
# Compare with NewMappingForm to see how it handles directory
cat -n "src/components/2-molecules/rhf/dialogs/new-mapping/NewMappingForm.tsx"Repository: gridsuite/griddyna-app
Length of output: 3905
Fix uniqueness validation for root-level mappings.
The code intentionally allows parentDirectory to be undefined for root-level mappings (when path.length = 1), passing it as activeDirectory to UniqueNameInput. However, the validation hook useUniqueNameValidation relies on a directory being set (line 39: if (nameValue !== currentName && directory)), and since the form initializes FieldConstants.DIRECTORY to undefined with a comment stating it's "not used (use activeDirectory instead)", uniqueness validation is completely skipped for root-level mappings. This allows duplicate mapping names at the root level.
Either provide a fallback directory when activeDirectory is undefined, or explicitly handle root-level validation to prevent name duplication.
🤖 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/components/2-molecules/rhf/dialogs/rename-mapping/RenameMappingDialog.tsx`
at line 71, The uniqueness validation for root-level mappings is skipped because
when path.length equals 1, the setParentDirectory call results in undefined
being passed as activeDirectory to UniqueNameInput. The useUniqueNameValidation
hook then skips validation entirely due to its condition checking if directory
exists. Fix this by either providing a fallback directory identifier (such as a
root-level constant) when activeDirectory would be undefined in the
setParentDirectory call, or by modifying the useUniqueNameValidation hook to
explicitly handle and validate root-level mappings even when the directory
parameter is undefined, ensuring duplicate names cannot be created at the root
level.
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
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/redux/slices/Mapping.js (2)
805-809: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
controlledParameterswhen deleting the active mapping.Line 805 clears the active editor state for
rulesandautomata, but leavescontrolledParametersfrom the deleted mapping behind.🐛 Proposed fix
if (id === state.activeMapping) { state.rules = []; state.automata = []; + state.controlledParameters = false; state.activeMapping = undefined; }🤖 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/redux/slices/Mapping.js` around lines 805 - 809, The active mapping delete/reset branch in the Mapping slice clears rules, automata, and activeMapping, but leaves controlledParameters from the removed mapping behind. Update the same state reset logic in this active-mapping check to also clear controlledParameters alongside the other editor state fields so deleting the active mapping fully resets the editor.
440-484: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
updateMappingbefore dereferencing mapping data.If this thunk is dispatched with no active mapping or with a stale non-active
id,mappingId,rules, orautomatacan beundefined;rules.map/automata.mapwill throw before the rejected reducer can handle it cleanly.🐛 Proposed guard
-export const updateMapping = createAsyncThunk('mappings/update', async (id, { getState }) => { +export const updateMapping = createAsyncThunk('mappings/update', async (id, { getState, rejectWithValue }) => { const state = getState(); const token = state?.user.user?.id_token; const mappingId = id ?? state?.mappings.activeMapping; + if (!mappingId) { + return rejectWithValue('No mapping selected'); + } + + const storedMapping = + id && id !== state?.mappings.activeMapping + ? state?.mappings.mappings.find((mapping) => mapping.id === id) + : undefined; + if (id && id !== state?.mappings.activeMapping && !storedMapping) { + return rejectWithValue(`Mapping ${id} not found`); + } + const rules = id && id !== state?.mappings.activeMapping - ? state?.mappings.mappings.find((mapping) => mapping.id === id)?.rules + ? storedMapping?.rules : state?.mappings.rules; + if (!Array.isArray(rules)) { + return rejectWithValue(`Mapping ${mappingId} has no rules`); + }🤖 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/redux/slices/Mapping.js` around lines 440 - 484, `updateMapping` can dereference missing mapping state and throw when `rules` or `automata` is undefined, especially with no active mapping or a stale `id`. Add an early guard in `updateMapping` after reading state and resolving the target mapping (`mappingId`) so the thunk exits or rejects cleanly when the mapping, its rules, or its automata are missing, before calling `rules.map` or `automata.map`.
🤖 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/redux/slices/Mapping.js`:
- Around line 805-809: The active mapping delete/reset branch in the Mapping
slice clears rules, automata, and activeMapping, but leaves controlledParameters
from the removed mapping behind. Update the same state reset logic in this
active-mapping check to also clear controlledParameters alongside the other
editor state fields so deleting the active mapping fully resets the editor.
- Around line 440-484: `updateMapping` can dereference missing mapping state and
throw when `rules` or `automata` is undefined, especially with no active mapping
or a stale `id`. Add an early guard in `updateMapping` after reading state and
resolving the target mapping (`mappingId`) so the thunk exits or rejects cleanly
when the mapping, its rules, or its automata are missing, before calling
`rules.map` or `automata.map`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3736f30a-1c91-4444-850c-b53673b46eb3
📒 Files selected for processing (4)
.env.developmentsrc/containers/MappingContainer.jsxsrc/redux/slices/Mapping.jssrc/rest/mappingsAPI.js
✅ Files skipped from review due to trivial changes (1)
- .env.development
🚧 Files skipped from review as they are similar to previous changes (2)
- src/containers/MappingContainer.jsx
- src/rest/mappingsAPI.js
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/2-molecules/NavigationMenu.jsx`:
- Around line 89-91: The IconButton in NavigationMenu renders only MoreVertIcon,
so it needs an accessible name for assistive tech. Update the IconButton used in
the row actions to include a localized aria-label that reflects the mapping
item, ideally using item.name, and add the corresponding mappingActions message
to the locale files so the label is translated consistently.
🪄 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: 680f260c-5c75-4fd8-a0ef-3064dc41fdf8
📒 Files selected for processing (5)
src/components/1-atoms/buttons/AddButton.jsxsrc/components/2-molecules/NavigationMenu.jsxsrc/components/2-molecules/NavigationMenuStyles.jssrc/translations/en.jsonsrc/translations/fr.json
✅ Files skipped from review due to trivial changes (3)
- src/components/1-atoms/buttons/AddButton.jsx
- src/translations/fr.json
- src/translations/en.json
| <IconButton edge="end" id={item.id} onClick={setMenu}> | ||
| <MoreVertIcon /> | ||
| </IconButton> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add an accessible name to the icon-only menu button.
Line 89 renders only MoreVertIcon, so assistive tech users get an unlabeled action button for each mapping row. Add a localized aria-label, ideally including item.name.
♿ Proposed fix
- <IconButton edge="end" id={item.id} onClick={setMenu}>
+ <IconButton
+ edge="end"
+ id={item.id}
+ onClick={setMenu}
+ aria-label={intl.formatMessage(
+ { id: 'mappingActions' },
+ { mappingName: item.name }
+ )}
+ >
<MoreVertIcon />
</IconButton>Also add the mappingActions message to the locale files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <IconButton edge="end" id={item.id} onClick={setMenu}> | |
| <MoreVertIcon /> | |
| </IconButton> | |
| <IconButton | |
| edge="end" | |
| id={item.id} | |
| onClick={setMenu} | |
| aria-label={intl.formatMessage( | |
| { id: 'mappingActions' }, | |
| { mappingName: item.name } | |
| )} | |
| > | |
| <MoreVertIcon /> | |
| </IconButton> |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 89-89: A list component should have a key to prevent re-rendering
Context:
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
🤖 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/components/2-molecules/NavigationMenu.jsx` around lines 89 - 91, The
IconButton in NavigationMenu renders only MoreVertIcon, so it needs an
accessible name for assistive tech. Update the IconButton used in the row
actions to include a localized aria-label that reflects the mapping item,
ideally using item.name, and add the corresponding mappingActions message to the
locale files so the label is translated consistently.
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
|
2 participants
PR Summary