Conversation
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
📝 WalkthroughWalkthroughThis pull request refactors the dynamic simulation configuration flow by removing automatic parameter passing through callbacks, introducing mapping-based model filtering, and adopting type-safe solver selection via an updated Changes
🚥 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. 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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
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: 2
🧹 Nitpick comments (1)
src/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-parameters.tsx (1)
175-175: Unnecessary type assertion.If the form is properly typed,
newParams.solver.solvershould already be of typeSolverType. Theas SolverTypecast may be masking a typing issue in the form schema.♻️ Suggested fix
-[Solver.SOLVER]: newParams.solver.solver as SolverType, +[Solver.SOLVER]: newParams.solver.solver,If this causes a type error, the form schema definition should be updated to properly type the solver field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-parameters.tsx` at line 175, The assignment using the unnecessary type assertion "as SolverType" on newParams.solver.solver (mapped to Solver.SOLVER) masks a typing problem; remove the cast and ensure the form schema/types declare the solver field as SolverType (or update the type of newParams/newParams.solver) so newParams.solver.solver is already SolverType at compile time; adjust the form field type definition or the interface that defines newParams (and any related form schema/type for the solver field) instead of keeping the runtime cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/dialogs/parameters/dynamicsimulation/curve-parameters.tsx`:
- Around line 179-186: The CurveSelectorDialog is passing
mapping={getValues(mappingPath)} which may be an empty string and ModelFilter
uses nullish coalescing (??) so empty strings reach
fetchDynamicSimulationModels; update the value passed to CurveSelectorDialog (or
inside ModelFilter) to normalize falsy/empty-string mappings to undefined/null
before calling fetchDynamicSimulationModels (i.e., coerce getValues(mappingPath)
with a check such as mappingValue => mappingValue ? mappingValue : undefined) so
fetchDynamicSimulationModels receives a clear "no mapping" sentinel; search for
CurveSelectorDialog, getValues(mappingPath), ModelFilter, and
fetchDynamicSimulationModels to apply this normalization.
In `@src/components/dialogs/parameters/dynamicsimulation/solver-parameters.tsx`:
- Around line 61-66: The TabPanel children use hardcoded indices ([0] and [1])
which can write form data to the wrong solver; compute the actual index of
selectedSolver (e.g., using solvers.findIndex(s => s === selectedSolver) with a
safe default) and use that index in the form path passed to IdaSolverParameters
and SimplifiedSolverParameters instead of the literal 0/1 so the path becomes
${path}.${Solver.SOLVERS}[selectedSolverIndex]; update references to
selectedSolver, Solver.SOLVERS, IdaSolverParameters, and
SimplifiedSolverParameters accordingly.
---
Nitpick comments:
In
`@src/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-parameters.tsx`:
- Line 175: The assignment using the unnecessary type assertion "as SolverType"
on newParams.solver.solver (mapped to Solver.SOLVER) masks a typing problem;
remove the cast and ensure the form schema/types declare the solver field as
SolverType (or update the type of newParams/newParams.solver) so
newParams.solver.solver is already SolverType at compile time; adjust the form
field type definition or the interface that defines newParams (and any related
form schema/type for the solver field) instead of keeping the runtime cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f6717a2-b985-4e39-9972-0e85f6061a79
📒 Files selected for processing (11)
src/components/dialogs/dynamicsimulation/dynamic-simulation-parameters-selector.tsxsrc/components/dialogs/parameters/dynamicsimulation/curve-parameters.tsxsrc/components/dialogs/parameters/dynamicsimulation/curve/dialog/curve-selector-dialog.tsxsrc/components/dialogs/parameters/dynamicsimulation/curve/dialog/curve-selector.tsxsrc/components/dialogs/parameters/dynamicsimulation/curve/dialog/model-filter.tsxsrc/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-parameters.tsxsrc/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-utils.tssrc/components/dialogs/parameters/dynamicsimulation/solver-parameters.tsxsrc/components/run-button-container.jsxsrc/services/study/dynamic-simulation.tssrc/services/study/dynamic-simulation.type.ts
| {open && ( | ||
| <CurveSelectorDialog | ||
| open={open} | ||
| onClose={handleClose} | ||
| onSave={handleAppend} | ||
| mapping={getValues(mappingPath)} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Potential issue: Empty string may not be handled as expected.
When the form initializes, mapping defaults to an empty string (''). The nullish coalescing operator (??) in ModelFilter passes empty strings through to fetchDynamicSimulationModels rather than converting them to null. If the backend API distinguishes between empty string and null for "no mapping selected", this could cause unexpected behavior.
Consider explicitly converting empty/falsy values to undefined:
<CurveSelectorDialog
open={open}
onClose={handleClose}
onSave={handleAppend}
- mapping={getValues(mappingPath)}
+ mapping={getValues(mappingPath) || undefined}
/>📝 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.
| {open && ( | |
| <CurveSelectorDialog | |
| open={open} | |
| onClose={handleClose} | |
| onSave={handleAppend} | |
| mapping={getValues(mappingPath)} | |
| /> | |
| )} | |
| {open && ( | |
| <CurveSelectorDialog | |
| open={open} | |
| onClose={handleClose} | |
| onSave={handleAppend} | |
| mapping={getValues(mappingPath) || undefined} | |
| /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/dialogs/parameters/dynamicsimulation/curve-parameters.tsx`
around lines 179 - 186, The CurveSelectorDialog is passing
mapping={getValues(mappingPath)} which may be an empty string and ModelFilter
uses nullish coalescing (??) so empty strings reach
fetchDynamicSimulationModels; update the value passed to CurveSelectorDialog (or
inside ModelFilter) to normalize falsy/empty-string mappings to undefined/null
before calling fetchDynamicSimulationModels (i.e., coerce getValues(mappingPath)
with a check such as mappingValue => mappingValue ? mappingValue : undefined) so
fetchDynamicSimulationModels receives a clear "no mapping" sentinel; search for
CurveSelectorDialog, getValues(mappingPath), ModelFilter, and
fetchDynamicSimulationModels to apply this normalization.
| <TabPanel value={selectedSolver?.type} index={SolverType.IDA}> | ||
| <IdaSolverParameters path={`${path}.${Solver.SOLVERS}[0]`} /> | ||
| </TabPanel> | ||
| <TabPanel value={selectedSolver?.type} index={SolverTypeInfos.SIM}> | ||
| <TabPanel value={selectedSolver?.type} index={SolverType.SIM}> | ||
| <SimplifiedSolverParameters path={`${path}.${Solver.SOLVERS}[1]`} /> | ||
| </TabPanel> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for solver array construction or ordering guarantees
rg -n -C3 'solvers' --type=ts --glob '**/*dynamic-simulation*' | head -100Repository: gridsuite/gridstudy-app
Length of output: 7662
🏁 Script executed:
cat -n src/components/dialogs/parameters/dynamicsimulation/solver-parameters.tsx | head -100Repository: gridsuite/gridstudy-app
Length of output: 3170
🏁 Script executed:
# Look for how solvers are fetched/initialized from backend
rg -n "getSolverParameters|fetchSolver|parameters.*solvers" --type=ts --glob '**/*dynamic-simulation*' | head -50Repository: gridsuite/gridstudy-app
Length of output: 528
🏁 Script executed:
# Check SolverInfos type definition and how it's structured
rg -n "SolverInfos\|type SolverInfos\|interface SolverInfos" --type=ts | head -30Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
rg -A 5 "type SolverInfos|interface SolverInfos" --type=tsRepository: gridsuite/gridstudy-app
Length of output: 453
🏁 Script executed:
# Also check how selectedSolver is used in the code
rg -n "selectedSolver" src/components/dialogs/parameters/dynamicsimulation/solver-parameters.tsxRepository: gridsuite/gridstudy-app
Length of output: 268
🏁 Script executed:
# Check the IdaSolverParameters and SimplifiedSolverParameters to understand what they expect
head -50 src/components/dialogs/parameters/dynamicsimulation/solver/ida-solver-parameters.tsxRepository: gridsuite/gridstudy-app
Length of output: 1425
🏁 Script executed:
# Search for backend API calls that return solvers
rg -n "getDynamicSimulationParameters|getSolverParameters" --type=ts -A 5 | head -50Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
# Check dynamic-simulation service to see how solvers are fetched
rg -n "solvers" src/services/study/dynamic-simulation.ts | head -30Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
# Look at the initial data structure and how solvers array is set up
rg -n "getSolverInfos\|solvers.*=" --type=ts | head -40Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
# Look at the complete type definitions
rg -A 15 "type IdaSolverInfos|type SimSolverInfos" --type=tsRepository: gridsuite/gridstudy-app
Length of output: 1902
🏁 Script executed:
# Check the form schema and how solvers are validated
rg -n "solverFormSchema\|SolverInfos" src/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-utils.ts -A 10 | head -60Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
# Search for where solvers array is actually constructed or initialized
rg -n "solvers.*\[\|new.*solvers\|solvers.*push\|solvers.*concat" --type=ts | head -40Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
# Check for any test files or comments about solver ordering
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx | xargs grep -l "solvers\|SolverType" 2>/dev/null | head -5Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
# Look at how the form handles solver updates to understand the data flow
rg -B 5 -A 15 "newParams.solver.solvers" src/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-parameters.tsxRepository: gridsuite/gridstudy-app
Length of output: 888
🏁 Script executed:
# Check if there's a backend configuration or constant that defines solver order
rg -n "SolverType\." src/services/study/dynamic-simulation.type.ts -B 2 -A 2 | head -40Repository: gridsuite/gridstudy-app
Length of output: 318
🏁 Script executed:
# Look at the enum definition for SolverType
rg -B 5 -A 10 "enum SolverType" --type=tsRepository: gridsuite/gridstudy-app
Length of output: 1185
🏁 Script executed:
# Check if the component receives solvers in a guaranteed order from parent
rg -B 10 -A 5 "SolverParameters.*solvers\|solver.*solvers" src/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-parameters.tsx | head -60Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
# Look at the complete parent component to see how SolverParameters is used
sed -n '250,290p' src/components/dialogs/parameters/dynamicsimulation/dynamic-simulation-parameters.tsxRepository: gridsuite/gridstudy-app
Length of output: 2025
🏁 Script executed:
# Verify that selectedSolver has all needed properties and confirm the bug
rg -A 20 "const selectedSolver = useMemo" src/components/dialogs/parameters/dynamicsimulation/solver-parameters.tsxRepository: gridsuite/gridstudy-app
Length of output: 667
Use the actual index of the selected solver instead of hardcoded indices.
The current implementation has a logic mismatch: selectedSolver is correctly identified by type matching (via .find()), and the TabPanel visibility is correctly based on selectedSolver?.type. However, the form paths passed to child components ([0] and [1]) assume IDA is always at index 0 and SIM is always at index 1. If the backend returns solvers in a different order, the correct parameter panel would display but form updates would target the wrong solver, causing data to be written to the wrong solver type.
Since selectedSolver is already available, find its actual index and use it in the paths:
const selectedSolverIndex = solvers?.findIndex(s => s === selectedSolver) ?? 0;Then pass this index to the child components instead of hardcoding [0] and [1].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/dialogs/parameters/dynamicsimulation/solver-parameters.tsx`
around lines 61 - 66, The TabPanel children use hardcoded indices ([0] and [1])
which can write form data to the wrong solver; compute the actual index of
selectedSolver (e.g., using solvers.findIndex(s => s === selectedSolver) with a
safe default) and use that index in the form path passed to IdaSolverParameters
and SimplifiedSolverParameters instead of the literal 0/1 so the path becomes
${path}.${Solver.SOLVERS}[selectedSolverIndex]; update references to
selectedSolver, Solver.SOLVERS, IdaSolverParameters, and
SimplifiedSolverParameters accordingly.
1 participant
PR Summary