Re-org Nework to allow user-defined layout and network name: front-end #205
Conversation
WalkthroughAdds debounced, server-validated parameter validation and network-name validation utilities; extends Parameter to accept Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ToolForm
participant Tool
participant API
participant Store
User->>ToolForm: change field
ToolForm->>ToolForm: handleFieldChange (compute affected params)
ToolForm->>Tool: onRefetchNeeded(formValues, affectedParams)
Tool->>API: fetch updated parameter metadata
API-->>Tool: return metadata
Tool->>Store: updateParameterMetadata(updatedMetadata)
Store-->>ToolForm: parameters updated
ToolForm->>Parameter: re-render affected Parameters
Parameter-->>User: updated options/validation state
sequenceDiagram
participant User
participant Parameter
participant Validator as useParameterValidation
participant API
participant Form
User->>Parameter: edit validated field
Parameter->>Validator: trigger validation (debounced)
Validator->>API: POST /api/tools/{toolName}/validate-field
alt valid
API-->>Validator: success
Validator->>Form: mark field valid
else invalid / error
API-->>Validator: error
Validator->>Form: mark field invalid + message
end
Parameter-->>User: display validation state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
src/features/jobs/components/Jobs/JobInfoModal.jsx (1)
42-86: Consider using strict equality for job ID comparison.On line 64, the comparison
data.jobid == job.iduses loose equality. If job IDs can be either strings or numbers, this could lead to unexpected matches.Apply this diff to use strict equality:
const message_appender = (data) => { - if (data.jobid == job.id) { + if (data.jobid === job.id) { setMessage((prevMessage) => prevMessage.concat(data.message)); } };src/features/map/components/Map/Layers/Selectors/Choice.jsx (1)
87-96: Consider removing or conditionalizing the console log.The debug log is useful during development but may clutter production console output. Consider using a debug flag or removing it before merging.
Also fix the Prettier formatting:
- console.log(`[Choice] ${parameterName}: Using default value:`, defaultValue); + console.log( + `[Choice] ${parameterName}: Using default value:`, + defaultValue, + );src/utils/validation.js (1)
9-19: Consider using lodash.debounce or react-use's useDebounce.While this implementation is correct, using a well-tested library reduces potential bugs and provides additional features (like
leading/trailingoptions).If keeping the custom implementation, consider returning the timeout ID for external cleanup:
export const debounce = (func, wait) => { let timeout; return function executedFunction(...args) { const later = () => { clearTimeout(timeout); func(...args); }; clearTimeout(timeout); timeout = setTimeout(later, wait); + return timeout; }; };However, since this is a simple use case and the custom implementation works, keeping it is also acceptable.
src/components/Parameter.jsx (1)
69-233: Replace polling withForm.useWatchTwo different
setIntervalloops poll the form every 100 ms just to detect field changes. Ant Design already exposesForm.useWatch/Form.Itemdependencies, which gives you instant updates without timers, less CPU churn, and fewer renders. Switching to the built-in watchers will simplify the code and avoid perpetual timers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
BACKEND_API_VALIDATION.md(1 hunks)BACKEND_EXPLORATION_INDEX.md(1 hunks)BACKEND_VALIDATION_EXAMPLES.md(1 hunks)NETWORK_NAME_VALIDATION_IMPLEMENTATION.md(1 hunks)src/components/Parameter.jsx(4 hunks)src/features/jobs/components/Jobs/JobInfoModal.jsx(1 hunks)src/features/map/components/Map/Layers/Selectors/Choice.jsx(2 hunks)src/features/project/stores/projectStore.jsx(1 hunks)src/features/tools/components/Tools/Tool.jsx(1 hunks)src/features/tools/components/Tools/ToolForm.jsx(2 hunks)src/utils/validation.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/features/tools/components/Tools/Tool.jsx (3)
src/features/input-editor/components/InputEditor/Table.jsx (1)
script(192-192)src/utils/validation.js (1)
params(56-61)src/components/Parameter.jsx (1)
form(482-482)
src/components/Parameter.jsx (2)
src/features/project/stores/projectStore.jsx (4)
response(14-14)apiClient(21-23)apiClient(37-39)apiClient(50-50)src/utils/validation.js (2)
validateNetworkNameChars(26-37)validateNetworkNameChars(26-37)
src/features/tools/components/Tools/ToolForm.jsx (1)
src/components/Parameter.jsx (2)
Parameter(480-867)form(482-482)
src/features/map/components/Map/Layers/Selectors/Choice.jsx (1)
src/components/Parameter.jsx (4)
choices(57-57)options(141-144)options(641-644)options(683-686)
🪛 ESLint
src/features/project/stores/projectStore.jsx
[error] 55-55: Replace error?.response?.status·===·400·&&·error?.response?.data?.detail·===·'Project·root·not·defined' with ⏎······error?.response?.status·===·400·&&⏎······error?.response?.data?.detail·===·'Project·root·not·defined'⏎····
(prettier/prettier)
[error] 56-56: Replace 'Project·root·not·configured.·Please·set·the·project·root·path·in·settings.' with ⏎········'Project·root·not·configured.·Please·set·the·project·root·path·in·settings.',⏎······
(prettier/prettier)
src/components/Parameter.jsx
[error] 73-73: Replace 'Network·type·changed·from',·networkType,·'to',·currentNetworkType with ⏎··········'Network·type·changed·from',⏎··········networkType,⏎··········'to',⏎··········currentNetworkType,⏎········
(prettier/prettier)
[error] 121-121: Replace 'Current·value·not·in·new·choices,·resetting·to:',·valueToSet with ⏎··········'Current·value·not·in·new·choices,·resetting·to:',⏎··········valueToSet,⏎········
(prettier/prettier)
[error] 128-128: Replace defaultValue·&&·currentValue·!==·defaultValue·&&·!hasFetchedRef.current with ⏎········defaultValue·&&⏎········currentValue·!==·defaultValue·&&⏎········!hasFetchedRef.current⏎······
(prettier/prettier)
[error] 130-130: Replace 'Initial·load:·backend·suggests·more·recent·network,·updating·from',·currentValue,·'to',·defaultValue with ⏎··········'Initial·load:·backend·suggests·more·recent·network,·updating·from',⏎··········currentValue,⏎··········'to',⏎··········defaultValue,⏎········
(prettier/prettier)
[error] 147-147: Insert ⏎···
(prettier/prettier)
[error] 148-148: Replace ···· with ······
(prettier/prettier)
[error] 149-149: Replace No·network·layouts·found·for·{networkType·||·'this·network·type'}.·Run with ··No·network·layouts·found·for·{networkType·||·'this·network·type'}.·Run⏎·······
(prettier/prettier)
[error] 150-150: Insert ··
(prettier/prettier)
[error] 151-151: Insert ··
(prettier/prettier)
[error] 182-182: Replace "Loading..." with 'Loading...'
(prettier/prettier)
[error] 184-184: Insert ··
(prettier/prettier)
[error] 185-185: Replace :·"Select·a·network·layout" with ··:·'Select·a·network·layout'
(prettier/prettier)
src/features/tools/components/Tools/ToolForm.jsx
[error] 10-10: Replace ·form,·parameters,·categoricalParameters,·script,·onMount· with ⏎··form,⏎··parameters,⏎··categoricalParameters,⏎··script,⏎··onMount,⏎
(prettier/prettier)
src/features/map/components/Map/Layers/Selectors/Choice.jsx
[error] 91-91: Replace ``[Choice]·${parameterName}:·Using·default·value:,·defaultValue with `⏎········`[Choice]·${parameterName}:·Using·default·value:`,⏎········defaultValue,⏎······`
(prettier/prettier)
🪛 LanguageTool
BACKEND_EXPLORATION_INDEX.md
[grammar] ~75-~75: Use a hyphen to join words.
Context: ...Usage patterns Best for: Copy-paste ready examples and implementation patter...
(QB_NEW_EN_HYPHEN)
BACKEND_VALIDATION_EXAMPLES.md
[style] ~561-~561: In American English, abbreviations like “etc.” require a period.
Context: ...ropdowns for choices, text for strings, etc) - Show validation constraints to users...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.18.1)
BACKEND_API_VALIDATION.md
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
433-433: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
452-452: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
NETWORK_NAME_VALIDATION_IMPLEMENTATION.md
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
BACKEND_EXPLORATION_INDEX.md
17-17: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
40-40: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
110-110: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
130-130: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
src/features/jobs/components/Jobs/JobInfoModal.jsx (1)
95-95: LGTM! Modern Ant Design Modal API.The
destroyOnHiddenprop correctly destroys modal content when hidden, which is the modern API replacing the olderdestroyOnClose. This ensures fresh state on each modal open and works properly with the socket cleanup logic in the useEffect hook.src/features/map/components/Map/Layers/Selectors/Choice.jsx (1)
98-103: LGTM: Consistent handling of new backend response format.The options computation and guard condition correctly adapt to the new
{choices: [...], default: ...}structure while maintaining proper null checks.src/features/tools/components/Tools/Tool.jsx (1)
173-185: Good UX improvement: clearing network-name after job creation.This prevents accidental duplicate runs for the network-layout tool. The comment correctly notes that the component will detect the change and clear validation state, so explicit
validateFields()is not needed.One minor consideration: if the user quickly submits again before the promise resolves, they could create a duplicate job. This is likely acceptable given the debounced validation and backend safeguards, but consider disabling the Run button while
createJobis in progress if this becomes an issue.src/features/tools/components/Tools/ToolForm.jsx (1)
17-25: LGTM: Consistent context propagation to Parameter components.The
allParametersandtoolNameprops are correctly passed to all Parameter instances, enabling context-aware validation for network layout parameters.Also applies to: 34-41
NETWORK_NAME_VALIDATION_IMPLEMENTATION.md (1)
1-368: Excellent comprehensive documentation of the validation feature.This document provides clear explanations of the implementation, validation flow, testing approach, and known limitations. The inclusion of future enhancements shows thoughtful consideration of potential improvements.
src/utils/validation.js (2)
26-37: LGTM: Clear and effective character validation.The validation correctly checks for invalid filesystem characters and returns a user-friendly error message. The Promise-based interface maintains consistency with the async
validateNetworkNameCollisionfunction.
47-75: Side effect concern is already documented and mitigated.The save-config side effect is confirmed and documented in NETWORK_NAME_VALIDATION_IMPLEMENTATION.md under "Known Limitations." The implementation includes a 500ms debounce that prevents multiple rapid saves—only one backend call occurs after the user stops typing (verified in test case 5). The future improvement to create a dedicated
/api/parameters/validateendpoint is already noted in the documentation as a roadmap item.No additional action required.
BACKEND_API_VALIDATION.md
Outdated
| ``` | ||
| /Users/zshi/Documents/GitHub/CityEnergyAnalyst/cea/interfaces/dashboard/ | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Fix markdown linting: Add language specifiers to fenced code blocks.
The markdown linter flags several code blocks without language specifiers. This improves syntax highlighting and accessibility.
Apply these fixes:
At line 13:
-```
+```text
/Users/zshi/Documents/GitHub/CityEnergyAnalyst/cea/interfaces/dashboard/
At line 433:
```diff
-```
+```text
/Users/zshi/Documents/GitHub/CityEnergyAnalyst/
...
At line 452:
```diff
-```
+```text
/Users/zshi/Documents/GitHub/CityEnergyAnalyst-GUI/src/
...
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In BACKEND_API_VALIDATION.md around lines 13 to 16 (and similarly at lines ~433
and ~452), the fenced code blocks lack language specifiers; update each opening
fence from totext for the blocks at line 13 (the
/Users/zshi/.../cea/interfaces/dashboard/ path), at line 433 (the
/Users/zshi/Documents/GitHub/CityEnergyAnalyst/ snippet), and at line 452 (the
/Users/zshi/Documents/GitHub/CityEnergyAnalyst-GUI/src/ snippet) so the blocks
read text and close with to satisfy the markdown linter.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
BACKEND_EXPLORATION_INDEX.md
Outdated
| ``` | ||
| CityEnergyAnalyst/ | ||
| ├── cea/ | ||
| │ ├── config.py # Parameter classes and validation | ||
| │ ├── interfaces/dashboard/ | ||
| │ │ ├── app.py # FastAPI setup, error handlers | ||
| │ │ ├── api/ | ||
| │ │ │ ├── databases.py # Database validation | ||
| │ │ │ ├── geometry.py # Geometry/typology validation | ||
| │ │ │ ├── tools.py # Tool parameter validation | ||
| │ │ │ ├── inputs.py # Input data validation | ||
| │ │ │ ├── dashboards.py # Parameter handling | ||
| │ │ │ └── utils.py # deconstruct_parameters() | ||
| │ │ └── dependencies.py # Dependency injection | ||
| │ └── datamanagement/ | ||
| │ └── databases_verification.py # Geometry/typology verification | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Fix markdown linting: Add language specifiers to fenced code blocks.
Similar to other documentation files, add language specifiers for better syntax highlighting.
At line 110:
-```
+```text
CityEnergyAnalyst/
...
At line 130:
```diff
-```
+```text
CityEnergyAnalyst-GUI/src/
...
Also applies to: 130-144
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
110-110: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In BACKEND_EXPLORATION_INDEX.md around lines 110-127 (and also 130-144), the
fenced code blocks are missing language specifiers; update each opening
triple-backtick to include an appropriate language token (e.g., text, bash
or ```console) for the directory/tree snippets to enable proper syntax
highlighting, and scan the rest of the file for any other fenced blocks and add
consistent language specifiers as well.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| ``` | ||
| User types in field | ||
| ↓ | ||
| Immediate character validation (synchronous) | ||
| ├─ Invalid chars → Red X with error message | ||
| └─ Valid chars → Continue | ||
| ↓ | ||
| Wait 500ms (debounce) | ||
| ↓ | ||
| Show "validating..." spinner | ||
| ↓ | ||
| Call backend API | ||
| ├─ Success → Green checkmark ✓ | ||
| └─ Error → Red X with collision message | ||
| ``` |
There was a problem hiding this comment.
Fix markdown linting: Add language specifier to flow diagram.
The flow diagram should have a language specifier for proper rendering.
-```
+```text
User types in field
↓
...
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In NETWORK_NAME_VALIDATION_IMPLEMENTATION.md around lines 93 to 107 the flow
diagram fenced block uses a plain code fence; update the opening fence to include a language specifier (e.g.,text) so the diagram renders correctly
with markdown linters and renderers, keeping the block contents and closing
fence unchanged.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
src/components/Parameter.jsx
Outdated
| const fetchChoices = async () => { | ||
| if (!toolName) { | ||
| console.warn('toolName is not provided, cannot fetch choices'); | ||
| return; | ||
| } | ||
|
|
||
| setLoading(true); | ||
| try { | ||
| const formValues = form.getFieldsValue(); | ||
| console.log('Fetching choices for', name, 'with formValues:', formValues); | ||
|
|
||
| const response = await apiClient.post( | ||
| `/api/tools/${toolName}/parameter-choices`, | ||
| { | ||
| parameter_name: name, | ||
| form_values: formValues, | ||
| }, | ||
| ); | ||
| const newChoices = response.data.choices || []; | ||
| const defaultValue = response.data.default; // Backend suggests the most recent network | ||
| console.log('Received new choices:', newChoices); | ||
| console.log('Received default value:', defaultValue); | ||
| setChoices(newChoices); | ||
|
|
||
| const currentValue = form.getFieldValue(name); | ||
|
|
||
| // If current value is not in new choices, use default (or first choice as fallback) | ||
| if (currentValue && !newChoices.includes(currentValue)) { | ||
| const valueToSet = defaultValue || newChoices[0] || ''; | ||
| console.log('Current value not in new choices, resetting to:', valueToSet); | ||
| form.setFieldsValue({ [name]: valueToSet }); | ||
| } else if (!currentValue && newChoices.length > 0) { | ||
| // No current value - use default from backend (most recent network) | ||
| const valueToSet = defaultValue || newChoices[0]; | ||
| console.log('No current value, setting to default:', valueToSet); | ||
| form.setFieldsValue({ [name]: valueToSet }); | ||
| } else if (defaultValue && currentValue !== defaultValue && !hasFetchedRef.current) { | ||
| // On initial load, if backend suggests a different default (more recent network), use it | ||
| console.log('Initial load: backend suggests more recent network, updating from', currentValue, 'to', defaultValue); | ||
| form.setFieldsValue({ [name]: defaultValue }); | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to fetch parameter choices:', error); | ||
| // Don't clear choices on error - keep the old ones | ||
| } finally { | ||
| setLoading(false); | ||
| } |
There was a problem hiding this comment.
Avoid overwriting choices with stale responses
fetchChoices applies whatever result finishes last. If a user flips “network-type” quickly, the slower response for the previous type can win the race, overwrite choices, and even reset the field to a layout that doesn’t belong to the currently selected network type. Track the in-flight request (or the network type you fetched for) and bail out when it no longer matches before mutating state.
A minimal guard could look like:
- const fetchChoices = async () => {
+ const fetchInFlightForRef = useRef(null);
+ const fetchChoices = async () => {
@@
- try {
+ try {
+ const requestedNetworkType = form.getFieldValue('network-type');
+ const fetchToken = Symbol(requestedNetworkType);
+ fetchInFlightForRef.current = fetchToken;
const formValues = form.getFieldsValue();
@@
- const response = await apiClient.post(
+ const response = await apiClient.post(
`/api/tools/${toolName}/parameter-choices`,
{
parameter_name: name,
form_values: formValues,
},
);
+ if (fetchInFlightForRef.current !== fetchToken) {
+ return;
+ }
const newChoices = response.data.choices || [];…applying the same guard before each setChoices / form.setFieldsValue call.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 121-121: Replace 'Current·value·not·in·new·choices,·resetting·to:',·valueToSet with ⏎··········'Current·value·not·in·new·choices,·resetting·to:',⏎··········valueToSet,⏎········
(prettier/prettier)
[error] 128-128: Replace defaultValue·&&·currentValue·!==·defaultValue·&&·!hasFetchedRef.current with ⏎········defaultValue·&&⏎········currentValue·!==·defaultValue·&&⏎········!hasFetchedRef.current⏎······
(prettier/prettier)
[error] 130-130: Replace 'Initial·load:·backend·suggests·more·recent·network,·updating·from',·currentValue,·'to',·defaultValue with ⏎··········'Initial·load:·backend·suggests·more·recent·network,·updating·from',⏎··········currentValue,⏎··········'to',⏎··········defaultValue,⏎········
(prettier/prettier)
🤖 Prompt for AI Agents
In src/components/Parameter.jsx around lines 92 to 138, fetchChoices can suffer
from race conditions where a slower response from a previous network-type
overwrites state; fix by tracking the key you fetched for (e.g., capture the
current toolName or network-type and/or increment a local requestId before the
async call), then before any state mutation (setChoices, form.setFieldsValue,
setLoading) verify the response still matches the latest request (or requestId)
and bail out if it doesn’t; apply this guard before every
setChoices/form.setFieldsValue call so only the latest response can update the
UI.
src/components/Parameter.jsx
Outdated
| const validateWithBackend = useCallback( | ||
| (value) => { | ||
| console.log('validateWithBackend called with value:', value); | ||
|
|
||
| // Clear any pending validation | ||
| if (validationTimeoutRef.current) { | ||
| clearTimeout(validationTimeoutRef.current); | ||
| console.log('Cleared previous timeout'); | ||
| } | ||
|
|
||
| // Blank/empty is valid (will auto-generate timestamp) | ||
| if (!value || !value.trim()) { | ||
| console.log('Value is blank, skipping validation'); | ||
| setValidationState({ status: '', message: '' }); | ||
| return; | ||
| } | ||
|
|
||
| console.log('Setting timeout for validation...'); | ||
|
|
||
| // Don't show "validating" state immediately - wait for debounce | ||
| // This prevents the annoying flicker on every keystroke | ||
|
|
||
| // Debounce backend validation by 1000ms (longer delay) | ||
| validationTimeoutRef.current = setTimeout(async () => { | ||
| console.log('Timeout fired! Starting validation...'); | ||
| // Now show validating state | ||
| setValidationState({ status: 'validating', message: '' }); | ||
|
|
||
| try { | ||
| // Get scenario from allParameters (not in form) | ||
| const scenario = scenarioValue; | ||
|
|
||
| // Get network-type from form | ||
| const formValues = form.getFieldsValue(); | ||
| const networkType = formValues['network-type']; | ||
|
|
||
| console.log('Form values:', formValues); | ||
| console.log('scenario:', scenario); | ||
| console.log('network-type:', networkType); | ||
|
|
||
| // Skip backend validation if dependencies aren't set | ||
| if (!scenario || !networkType) { | ||
| console.log( | ||
| 'Skipping validation - missing scenario or network-type', | ||
| ); | ||
| setValidationState({ status: '', message: '' }); | ||
| return; | ||
| } | ||
|
|
||
| // Check if network folder exists | ||
| // Format: {scenario}/outputs/data/thermal-network/{DC|DH}/{network-name}/ | ||
| const trimmedValue = value.trim(); | ||
| const networkPath = `${scenario}/outputs/data/thermal-network/${networkType}/${trimmedValue}`; | ||
|
|
||
| console.log('Checking network path:', networkPath); | ||
|
|
||
| try { | ||
| // Check if the network folder exists using /api/contents | ||
| await apiClient.get('/api/contents', { | ||
| params: { | ||
| content_path: networkPath, | ||
| content_type: 'directory', | ||
| }, | ||
| }); | ||
|
|
||
| // If we got here (no error), the folder exists | ||
| console.log('Network folder exists!'); | ||
|
|
||
| // Check if it contains network files (edges.shp or nodes.shp) | ||
| try { | ||
| const edgesPath = `${networkPath}/edges.shp`; | ||
| const nodesPath = `${networkPath}/nodes.shp`; | ||
|
|
||
| // Try to check for edges.shp | ||
| let hasEdges = false; | ||
| let hasNodes = false; | ||
|
|
||
| try { | ||
| await apiClient.get('/api/contents', { | ||
| params: { content_path: edgesPath, content_type: 'file' }, | ||
| }); | ||
| hasEdges = true; | ||
| } catch (e) { | ||
| // edges.shp doesn't exist | ||
| } | ||
|
|
||
| try { | ||
| await apiClient.get('/api/contents', { | ||
| params: { content_path: nodesPath, content_type: 'file' }, | ||
| }); | ||
| hasNodes = true; | ||
| } catch (e) { | ||
| // nodes.shp doesn't exist | ||
| } | ||
|
|
||
| if (hasEdges || hasNodes) { | ||
| // Network exists with actual network files | ||
| setValidationState({ | ||
| status: 'error', | ||
| message: `Network '${trimmedValue}' already exists for ${networkType}. Choose a different name or delete the existing folder.`, | ||
| }); | ||
| return; | ||
| } | ||
| } catch (fileCheckError) { | ||
| console.log('Error checking network files:', fileCheckError); | ||
| } | ||
| } catch (folderCheckError) { | ||
| // Folder doesn't exist - that's good! | ||
| console.log( | ||
| 'Network folder does not exist (good):', | ||
| folderCheckError.response?.status, | ||
| ); | ||
| } | ||
|
|
||
| // Validation passed - no collision detected | ||
| setValidationState({ status: 'success', message: '' }); | ||
| } catch (error) { | ||
| console.error('Validation error:', error); | ||
|
|
||
| // Extract error message from backend | ||
| const errorMessage = | ||
| error?.response?.data?.message || | ||
| error?.response?.data?.error || | ||
| error?.response?.data || | ||
| error?.message || | ||
| 'Validation failed'; | ||
|
|
||
| setValidationState({ | ||
| status: 'error', | ||
| message: String(errorMessage), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Prevent stale backend validation responses
validateWithBackend updates validationState unconditionally when the request resolves. If the user types quickly, a slow response for the previous value can finish after a later request and still flip the field into an error state (the effect immediately re-runs form.validateFields, so the UI shows a blocking error for the wrong input). Guard the setter with a request token or by comparing against the current field value before mutating state.
Apply a fix along these lines:
+ const validationRequestIdRef = useRef(0);
const validateWithBackend = useCallback(
(value) => {
- // Clear any pending validation
+ const requestId = ++validationRequestIdRef.current;
+ // Clear any pending validation
if (validationTimeoutRef.current) {
clearTimeout(validationTimeoutRef.current);
console.log('Cleared previous timeout');
}
@@
- validationTimeoutRef.current = setTimeout(async () => {
+ validationTimeoutRef.current = setTimeout(async () => {
console.log('Timeout fired! Starting validation...');
// Now show validating state
setValidationState({ status: 'validating', message: '' });
@@
- // Check if network folder exists
+ // Check if network folder exists
// Format: {scenario}/outputs/data/thermal-network/{DC|DH}/{network-name}/
const trimmedValue = value.trim();
+ if (form.getFieldValue(name)?.trim() !== trimmedValue) {
+ return;
+ }
const networkPath = `${scenario}/outputs/data/thermal-network/${networkType}/${trimmedValue}`;
@@
- setValidationState({ status: 'success', message: '' });
+ if (requestId === validationRequestIdRef.current) {
+ setValidationState({ status: 'success', message: '' });
+ }
} catch (error) {
console.error('Validation error:', error);
@@
- setValidationState({
- status: 'error',
- message: String(errorMessage),
- });
+ if (requestId === validationRequestIdRef.current) {
+ setValidationState({
+ status: 'error',
+ message: String(errorMessage),
+ });
+ }
}
}, 1000); // Increased to 1 second
},
- [form, scenarioValue],
+ [form, name, scenarioValue],
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Parameter.jsx around lines 274-405, the backend validation can
race: a slow response for an earlier value can update validationState after a
newer value was entered. Fix by adding a per-request validation token (or
capture the trimmedValue) when scheduling the timeout, store it in a ref (e.g.
validationRequestRef.current = token/value) before making API calls, and inside
every place you call setValidationState after async work check that the current
ref/token still matches the value for which the request was issued; if it
doesn't match, bail out and do not call setValidationState. Also ensure you
update the token when clearing/starting new timeouts so stale responses are
ignored.
| // Backend can return either array (legacy) or {choices: [...], default: "..."} (new) | ||
| if (Array.isArray(data)) { | ||
| setChoices({ choices: data, default: data[0] }); | ||
| } else { | ||
| setChoices(data); | ||
| } |
There was a problem hiding this comment.
Potential edge case: empty array response.
The backward compatibility logic converts legacy array responses to the new format, defaulting to data[0]. However, if the backend returns an empty array, this would set default: undefined.
Consider adding a guard:
// Backend can return either array (legacy) or {choices: [...], default: "..."} (new)
if (Array.isArray(data)) {
- setChoices({ choices: data, default: data[0] });
+ setChoices({ choices: data, default: data.length > 0 ? data[0] : null });
} else {
setChoices(data);
}📝 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.
| // Backend can return either array (legacy) or {choices: [...], default: "..."} (new) | |
| if (Array.isArray(data)) { | |
| setChoices({ choices: data, default: data[0] }); | |
| } else { | |
| setChoices(data); | |
| } | |
| // Backend can return either array (legacy) or {choices: [...], default: "..."} (new) | |
| if (Array.isArray(data)) { | |
| setChoices({ choices: data, default: data.length > 0 ? data[0] : null }); | |
| } else { | |
| setChoices(data); | |
| } |
🤖 Prompt for AI Agents
In src/features/map/components/Map/Layers/Selectors/Choice.jsx around lines 72
to 77, the compatibility branch converts a legacy array to {choices, default}
but uses data[0] without guarding against an empty array; update the logic to
detect an empty array and set a safe default (e.g., null or an empty string) and
ensure choices is still set to the array (possibly empty) so the component can
handle no-options gracefully.
Deleted BACKEND_API_VALIDATION.md, BACKEND_EXPLORATION_INDEX.md, BACKEND_VALIDATION_EXAMPLES.md, and NETWORK_NAME_VALIDATION_IMPLEMENTATION.md from the repository. These files contained documentation and implementation notes for backend parameter validation and are no longer needed in the codebase.
Consolidates backend validation and error handling for parameter fields, removing specialized subcomponents for network layout choice and name parameters. All validation, including async backend checks, is now handled within the main Parameter component, simplifying the code and improving maintainability.
Introduces success feedback and improved error handling for the NetworkLayoutNameParameter input. The field now displays validation status and feedback based on backend validation results, enhancing user experience and clarity.
Extracted parameter validation logic into a reusable useParameterValidation hook for improved code organization and clarity. Updated Parameter component to use the new hook and simplified validation handling.
Removes redundant placeholder and onChange logic from the Input component in Parameter, and simplifies the custom validator function. This streamlines input handling and ensures validation is triggered only on blur, reducing unnecessary state updates.
Removed the unused fieldError prop from useParameterValidation and Parameter components. Error handling now relies solely on blurError within the custom hook.
Refactored the validation to better handle nullable and non-nullable cases. The placeholder now reflects whether the field is nullable, and redundant onChange logic was removed for clarity.
Simplifies and centralizes parameter validation by removing local state and blur-based validation logic. Validation is now handled directly via async validator functions in Ant Design Form rules, improving maintainability and reducing complexity. Updates form fields to use the new validation approach and cleans up related props and handlers.
Enabled the 'hasFeedback' prop on the FormField for the network name input to provide visual validation feedback to users.
Replaces the onMount prop in ToolForm with a useEffect in Tool to check for missing inputs after fetching parameters. Refactors related hooks to use useCallback for better memoization and removes redundant code, improving maintainability and clarity.
The Form.Item now includes the 'dependencies' prop, set to 'parameter.depends_on' or an empty array. This ensures the form item revalidates when its dependencies change.
Implements logic to refetch and update parameter metadata when dependent form fields change. Adds a new updateParameterMetadata action to the tools store, updates Tool and ToolForm components to support on-the-fly parameter metadata updates, and ensures form values and parameter definitions stay in sync when dependencies are triggered.
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)
src/features/tools/components/Tools/Tool.jsx (1)
225-233: Drop the unusedsaveToolParamsselectorESLint is failing (
no-unused-vars) becausesaveToolParamsis read from the store but never used in this module. Please remove the selector (or put it to use) so the lint gate passes.- const saveToolParams = useToolsStore((state) => state.saveToolParams);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/Parameter.jsx(5 hunks)src/features/map/components/Map/Layers/Selectors/Choice.jsx(2 hunks)src/features/project/stores/projectStore.jsx(1 hunks)src/features/tools/components/Tools/Tool.jsx(9 hunks)src/features/tools/components/Tools/ToolForm.jsx(4 hunks)src/features/tools/stores/toolsStore.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/map/components/Map/Layers/Selectors/Choice.jsx
- src/features/project/stores/projectStore.jsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/features/tools/components/Tools/ToolForm.jsx (2)
src/features/tools/components/Tools/Tool.jsx (2)
categoricalParameters(237-243)form(118-118)src/components/Parameter.jsx (2)
form(80-80)Parameter(77-456)
src/components/Parameter.jsx (4)
src/features/tools/components/Tools/Tool.jsx (2)
form(118-118)error(26-26)src/features/building-editor/components/building-editor.jsx (1)
form(113-113)src/features/project/stores/projectStore.jsx (4)
response(14-14)apiClient(21-23)apiClient(37-39)apiClient(50-50)src/utils/validation.js (1)
errorMessage(67-71)
src/features/tools/components/Tools/Tool.jsx (1)
src/features/tools/stores/toolsStore.js (1)
useToolsStore(4-169)
🪛 ESLint
src/features/tools/components/Tools/Tool.jsx
[error] 229-229: 'saveToolParams' is assigned a value but never used.
(no-unused-vars)
| const dependencyMap = {}; | ||
| allParams.forEach((param) => { | ||
| if (param.depends_on && Array.isArray(param.depends_on)) { | ||
| param.depends_on.forEach((depName) => { | ||
| if (!dependencyMap[depName]) { | ||
| dependencyMap[depName] = []; | ||
| } | ||
| dependencyMap[depName].push(param.name); | ||
| }); | ||
| } | ||
| // Backward compatibility: also check triggers_refetch | ||
| if (param.triggers_refetch) { | ||
| if (!dependencyMap[param.name]) { | ||
| dependencyMap[param.name] = []; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Check if any changed field has dependents | ||
| for (const changedField of changedFields) { | ||
| const fieldName = changedField.name[0]; | ||
| const newValue = changedField.value; | ||
| const oldValue = watchedValues[fieldName]; | ||
|
|
||
| // Skip refetch on initial load (when oldValue is undefined) | ||
| const isInitialLoad = oldValue === undefined; | ||
|
|
||
| // Always update watched values when value changes | ||
| if (newValue !== oldValue) { | ||
| setWatchedValues((prev) => ({ ...prev, [fieldName]: newValue })); | ||
| } | ||
|
|
||
| // Check if this field has dependents (via depends_on or triggers_refetch) | ||
| const hasDependents = dependencyMap[fieldName]?.length > 0; | ||
|
|
||
| // Only trigger refetch if not initial load AND value changed AND has dependents | ||
| if (!isInitialLoad && newValue !== oldValue && hasDependents) { | ||
| console.log( | ||
| `Parameter ${fieldName} changed, refetching form (dependents: ${dependencyMap[fieldName].join(', ')})`, | ||
| ); | ||
|
|
||
| // Trigger refetch with current form values, changed param, and affected params | ||
| const formValues = form.getFieldsValue(); | ||
| onRefetchNeeded?.( | ||
| { ...formValues, [fieldName]: newValue }, | ||
| fieldName, | ||
| dependencyMap[fieldName], | ||
| ); | ||
| break; // Only need one refetch |
There was a problem hiding this comment.
Restore triggers_refetch behavior
When a parameter relies on triggers_refetch, dependencyMap[fieldName] is initialized to an empty array. Later, hasDependents is computed with dependencyMap[fieldName]?.length > 0, so any triggers_refetch parameter now returns false and the refetch path never runs. That regresses the legacy behavior you’re trying to preserve. Treat the mere presence of an entry as a signal to refetch (e.g., track a boolean flag or store null) instead of requiring a non-empty dependents array. Otherwise network metadata won’t refresh when those knobs change. Recommend adjusting hasDependents to check for dependencyMap[fieldName] !== undefined and default affectedParams to an empty array when you dispatch.
- const hasDependents = dependencyMap[fieldName]?.length > 0;
+ const dependents = dependencyMap[fieldName];
+ const hasDependents = dependents !== undefined;
...
- onRefetchNeeded?.(
- { ...formValues, [fieldName]: newValue },
- fieldName,
- dependencyMap[fieldName],
- );
+ onRefetchNeeded?.(
+ { ...formValues, [fieldName]: newValue },
+ fieldName,
+ Array.isArray(dependents) ? dependents : [],
+ );🤖 Prompt for AI Agents
In src/features/tools/components/Tools/ToolForm.jsx around lines 29-77,
parameters with triggers_refetch are added to dependencyMap as an empty array
which later makes hasDependents false (because it checks length>0), preventing
the refetch path from running; change the dependent check to treat any presence
in dependencyMap as a signal by using dependencyMap[fieldName] !== undefined for
hasDependents, and when calling onRefetchNeeded pass dependencyMap[fieldName] ||
[] (default to an empty array) as the affected params so legacy triggers_refetch
behavior is restored.
| updateParameterMetadata: (updatedMetadata) => { | ||
| set((state) => { | ||
| const currentParams = state.toolParams.params; | ||
| const newParameters = [...(currentParams.parameters || [])]; | ||
| const newCategoricalParameters = { | ||
| ...(currentParams.categoricalParameters || {}), | ||
| }; | ||
|
|
||
| // Update parameters | ||
| Object.keys(updatedMetadata).forEach((paramName) => { | ||
| const metadata = updatedMetadata[paramName]; | ||
|
|
||
| // Find in regular parameters | ||
| const paramIndex = newParameters.findIndex((p) => p.name === paramName); | ||
| if (paramIndex >= 0) { | ||
| newParameters[paramIndex] = { | ||
| ...newParameters[paramIndex], | ||
| ...metadata, | ||
| }; | ||
| } | ||
|
|
||
| // Find in categorical parameters | ||
| Object.keys(newCategoricalParameters).forEach((category) => { | ||
| const catParamIndex = newCategoricalParameters[category].findIndex( | ||
| (p) => p.name === paramName, | ||
| ); | ||
| if (catParamIndex >= 0) { | ||
| newCategoricalParameters[category][catParamIndex] = { | ||
| ...newCategoricalParameters[category][catParamIndex], | ||
| ...metadata, | ||
| }; | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| return { | ||
| toolParams: { | ||
| ...state.toolParams, | ||
| params: { | ||
| ...currentParams, | ||
| parameters: newParameters, | ||
| categoricalParameters: newCategoricalParameters, | ||
| }, |
There was a problem hiding this comment.
Avoid mutating existing categorical arrays in-place
newCategoricalParameters[category] still references the store’s original array, so assigning into [...] = { … } mutates the current state before Zustand can publish the update. This can cause selectors to observe mid-update state and breaks the immutability rules that Zustand relies on for predictable reactivity. Clone each category array before writing back (e.g., spread into a new array) and assign into that copy instead of the existing array.(zustand.docs.pmnd.rs)
const newCategoricalParameters = {
...(currentParams.categoricalParameters || {}),
};
Object.keys(newCategoricalParameters).forEach((category) => {
- const catParamIndex = newCategoricalParameters[category].findIndex(
+ const categoryParams = [
+ ...(newCategoricalParameters[category] || []),
+ ];
+ const catParamIndex = categoryParams.findIndex(
(p) => p.name === paramName,
);
if (catParamIndex >= 0) {
- newCategoricalParameters[category][catParamIndex] = {
- ...newCategoricalParameters[category][catParamIndex],
- ...metadata,
- };
+ categoryParams[catParamIndex] = {
+ ...categoryParams[catParamIndex],
+ ...metadata,
+ };
+ newCategoricalParameters[category] = categoryParams;
}
});🤖 Prompt for AI Agents
In src/features/tools/stores/toolsStore.js around lines 103 to 145, the code
mutates categorical parameter arrays in-place by directly assigning into
newCategoricalParameters[category][index], which still references the original
arrays; to fix, when iterating categories clone each category array (e.g., const
cloned = [...newCategoricalParameters[category]]), perform updates on that
cloned array, then assign the cloned array back to
newCategoricalParameters[category] so no original array object is mutated and
Zustand can detect the update immutably.
Adds an isRefetching state to display the loading spinner while parameter metadata is being refetched, improving user feedback during asynchronous operations.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/tools/components/Tools/Tool.jsx(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/tools/components/Tools/Tool.jsx (3)
src/features/tools/stores/toolsStore.js (1)
useToolsStore(4-169)src/components/Parameter.jsx (1)
form(80-80)src/utils/validation.js (1)
params(56-61)
🔇 Additional comments (4)
src/features/tools/components/Tools/Tool.jsx (4)
28-41: LGTM! Good refactor to accept parameters externally.The modification to accept
parametersas an argument enables better control flow from the parent component and aligns with the new refetch workflow.
131-174: LGTM! Proper memoization and complete dependency array.The addition of
useCallbackmemoization and the inclusion ofcategoricalParametersin the dependency array are correct improvements.
276-281: LGTM! Proper cleanup when description changes.Resetting scroll-related state when the description changes prevents stale UI state and ensures the header visibility behaves correctly with new content.
392-395: LGTM! Good UX to show loading during refetch.Including
isRefetchingin the spinner state provides clear feedback to users when parameter metadata is being updated.
| const runScript = async () => { | ||
| const params = await getForm(); | ||
|
|
||
| return createJob(script, params).catch((err) => { | ||
| if (err?.response?.status === 401) handleLogin(); | ||
| else console.error(`Error creating job: ${err}`); | ||
| }); | ||
| return createJob(script, params) | ||
| .then((result) => { | ||
| // Clear network-name field after successful job creation to prevent duplicate runs | ||
| if (script === 'network-layout' && params?.['network-name']) { | ||
| form.setFieldsValue({ 'network-name': '' }); | ||
| // Don't call validateFields - the component will detect the change and clear validation state | ||
| } | ||
| return result; | ||
| }) | ||
| .catch((err) => { | ||
| if (err?.response?.status === 401) handleLogin(); | ||
| else console.error(`Error creating job: ${err}`); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Script-specific logic in generic component reduces maintainability.
Hard-coding network-layout script behavior directly in the generic Tool component creates tight coupling. As more tools require post-submission field manipulation, this pattern will lead to a growing list of special cases.
Consider extracting this to a configuration-driven approach where tool metadata can specify post-submission field actions, or use a callback mechanism that tools can opt into.
Apply this pattern to make it configurable:
- return createJob(script, params)
- .then((result) => {
- // Clear network-name field after successful job creation to prevent duplicate runs
- if (script === 'network-layout' && params?.['network-name']) {
- form.setFieldsValue({ 'network-name': '' });
- // Don't call validateFields - the component will detect the change and clear validation state
- }
- return result;
- })
+ return createJob(script, params)
+ .then((result) => {
+ // Allow tools to define post-submission field clearing via metadata
+ const clearFields = params?.['_clearOnSubmit'] || [];
+ if (clearFields.length > 0) {
+ const fieldsToClear = {};
+ clearFields.forEach(field => { fieldsToClear[field] = ''; });
+ form.setFieldsValue(fieldsToClear);
+ }
+ return result;
+ })Then configure this in the tool's parameter metadata on the backend.
Committable suggestion skipped: line range outside the PR's diff.
| const fetchParams = async () => { | ||
| if (script) await fetchToolParams(script); | ||
| else resetToolParams(); | ||
|
|
||
| // Reset form fields to ensure they are in sync with the fetched parameters | ||
| form.resetFields(); | ||
|
|
||
| // Check for missing inputs after fetching parameters | ||
| const params = await getForm(); | ||
| if (params) checkMissingInputs(params); | ||
| }; | ||
|
|
||
| // FIXME: Run check missing inputs when form validation passes | ||
| useEffect(() => { | ||
| const fetchParams = async () => { | ||
| if (script) await fetchToolParams(script); | ||
| else resetToolParams(); | ||
|
|
||
| // Reset form fields to ensure they are in sync with the fetched parameters | ||
| form.resetFields(); | ||
| }; | ||
|
|
||
| fetchParams(); | ||
|
|
||
| // Reset header visibility when the component mounts | ||
| setHeaderVisible(true); | ||
| lastScrollPositionRef.current = 0; | ||
| descriptionHeightRef.current = 'auto'; | ||
| }, [script, fetchToolParams, resetToolParams, form]); |
There was a problem hiding this comment.
Fix incomplete dependency array and address FIXME.
The useEffect on line 324 has an incomplete dependency array. The fetchParams function references getForm and checkMissingInputs, but they're missing from the dependencies, which could cause stale closures.
Additionally, the FIXME comment suggests this implementation may not be final.
To fix the dependency array issue, wrap fetchParams in useCallback:
- const fetchParams = async () => {
+ const fetchParams = useCallback(async () => {
if (script) await fetchToolParams(script);
else resetToolParams();
// Reset form fields to ensure they are in sync with the fetched parameters
form.resetFields();
// Check for missing inputs after fetching parameters
const params = await getForm();
if (params) checkMissingInputs(params);
- };
+ }, [script, fetchToolParams, resetToolParams, form, getForm, checkMissingInputs]);
- // FIXME: Run check missing inputs when form validation passes
useEffect(() => {
fetchParams();
- }, [script, fetchToolParams, resetToolParams, form]);
+ }, [fetchParams]);Also clarify or resolve the FIXME comment about running checks when form validation passes.
🤖 Prompt for AI Agents
In src/features/tools/components/Tools/Tool.jsx around lines 311 to 326,
fetchParams closes over getForm and checkMissingInputs but the useEffect
dependency array omits them and a FIXME asks that missing-input checks run only
after form validation; fix this by extracting fetchParams into a useCallback
whose dependency array includes script, fetchToolParams, resetToolParams, form,
getForm and checkMissingInputs, and inside that callback reset or load params,
then run form.validateFields() (or await form.validateFields()) before calling
getForm and then checkMissingInputs so the missing-input check only runs after
validation; finally update the useEffect to call the memoized fetchParams and
remove the FIXME.
| const handleRefetch = useCallback( | ||
| async (formValues, changedParam, affectedParams) => { | ||
| try { | ||
| setIsRefetching(true); | ||
| console.log( | ||
| `[handleRefetch] Refetching metadata - changed: ${changedParam}, affected: ${affectedParams?.join(', ')}`, | ||
| ); | ||
|
|
||
| // Call API to get updated parameter metadata | ||
| const response = await apiClient.post( | ||
| `/api/tools/${script}/parameter-metadata`, | ||
| { | ||
| form_values: formValues, | ||
| affected_parameters: affectedParams, | ||
| }, | ||
| ); | ||
|
|
||
| const { parameters } = response.data; | ||
| console.log( | ||
| `[handleRefetch] Received metadata for ${Object.keys(parameters).length} parameters`, | ||
| ); | ||
|
|
||
| // Update parameter definitions in store | ||
| updateParameterMetadata(parameters); | ||
|
|
||
| // Update form values for affected parameters if value changed | ||
| Object.keys(parameters).forEach((paramName) => { | ||
| const metadata = parameters[paramName]; | ||
| if (metadata.value !== undefined) { | ||
| const currentValue = form.getFieldValue(paramName); | ||
| if (currentValue !== metadata.value) { | ||
| console.log( | ||
| `[handleRefetch] Updating ${paramName} value: ${currentValue} -> ${metadata.value}`, | ||
| ); | ||
| form.setFieldValue(paramName, metadata.value); | ||
| } | ||
| } | ||
| }); | ||
| } catch (err) { | ||
| console.error('Error refetching parameter metadata:', err); | ||
| } finally { | ||
| setIsRefetching(false); | ||
| } | ||
| }, | ||
| [script, form, updateParameterMetadata], | ||
| ); |
There was a problem hiding this comment.
Remove console.log statements and improve error handling.
The refetch logic is sound, but there are production code quality issues:
- Multiple
console.logstatements (lines 332-334, 346-348, 359-361) should be removed or replaced with proper logging utilities before merging to production. - Error handling only logs to console without user feedback. Users won't know if the metadata refetch failed.
Apply this diff to remove debug logs and add user feedback:
try {
setIsRefetching(true);
- console.log(
- `[handleRefetch] Refetching metadata - changed: ${changedParam}, affected: ${affectedParams?.join(', ')}`,
- );
// Call API to get updated parameter metadata
const response = await apiClient.post(
`/api/tools/${script}/parameter-metadata`,
{
form_values: formValues,
affected_parameters: affectedParams,
},
);
const { parameters } = response.data;
- console.log(
- `[handleRefetch] Received metadata for ${Object.keys(parameters).length} parameters`,
- );
// Update parameter definitions in store
updateParameterMetadata(parameters);
// Update form values for affected parameters if value changed
Object.keys(parameters).forEach((paramName) => {
const metadata = parameters[paramName];
if (metadata.value !== undefined) {
const currentValue = form.getFieldValue(paramName);
if (currentValue !== metadata.value) {
- console.log(
- `[handleRefetch] Updating ${paramName} value: ${currentValue} -> ${metadata.value}`,
- );
form.setFieldValue(paramName, metadata.value);
}
}
});
} catch (err) {
console.error('Error refetching parameter metadata:', err);
+ // TODO: Show error message to user via notification or alert
} finally {📝 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.
| const handleRefetch = useCallback( | |
| async (formValues, changedParam, affectedParams) => { | |
| try { | |
| setIsRefetching(true); | |
| console.log( | |
| `[handleRefetch] Refetching metadata - changed: ${changedParam}, affected: ${affectedParams?.join(', ')}`, | |
| ); | |
| // Call API to get updated parameter metadata | |
| const response = await apiClient.post( | |
| `/api/tools/${script}/parameter-metadata`, | |
| { | |
| form_values: formValues, | |
| affected_parameters: affectedParams, | |
| }, | |
| ); | |
| const { parameters } = response.data; | |
| console.log( | |
| `[handleRefetch] Received metadata for ${Object.keys(parameters).length} parameters`, | |
| ); | |
| // Update parameter definitions in store | |
| updateParameterMetadata(parameters); | |
| // Update form values for affected parameters if value changed | |
| Object.keys(parameters).forEach((paramName) => { | |
| const metadata = parameters[paramName]; | |
| if (metadata.value !== undefined) { | |
| const currentValue = form.getFieldValue(paramName); | |
| if (currentValue !== metadata.value) { | |
| console.log( | |
| `[handleRefetch] Updating ${paramName} value: ${currentValue} -> ${metadata.value}`, | |
| ); | |
| form.setFieldValue(paramName, metadata.value); | |
| } | |
| } | |
| }); | |
| } catch (err) { | |
| console.error('Error refetching parameter metadata:', err); | |
| } finally { | |
| setIsRefetching(false); | |
| } | |
| }, | |
| [script, form, updateParameterMetadata], | |
| ); | |
| const handleRefetch = useCallback( | |
| async (formValues, changedParam, affectedParams) => { | |
| try { | |
| setIsRefetching(true); | |
| // Call API to get updated parameter metadata | |
| const response = await apiClient.post( | |
| `/api/tools/${script}/parameter-metadata`, | |
| { | |
| form_values: formValues, | |
| affected_parameters: affectedParams, | |
| }, | |
| ); | |
| const { parameters } = response.data; | |
| // Update parameter definitions in store | |
| updateParameterMetadata(parameters); | |
| // Update form values for affected parameters if value changed | |
| Object.keys(parameters).forEach((paramName) => { | |
| const metadata = parameters[paramName]; | |
| if (metadata.value !== undefined) { | |
| const currentValue = form.getFieldValue(paramName); | |
| if (currentValue !== metadata.value) { | |
| form.setFieldValue(paramName, metadata.value); | |
| } | |
| } | |
| }); | |
| } catch (err) { | |
| console.error('Error refetching parameter metadata:', err); | |
| // TODO: Show error message to user via notification or alert | |
| } finally { | |
| setIsRefetching(false); | |
| } | |
| }, | |
| [script, form, updateParameterMetadata], | |
| ); |
🤖 Prompt for AI Agents
In src/features/tools/components/Tools/Tool.jsx around lines 328-373, remove the
three console.log debug statements and replace them with proper logging and user
feedback: use the app's logging utility (e.g., logger.error/info) instead of
console.log for internal logs (or remove the informational logs entirely), and
in the catch block show a user-facing error (e.g., call the app's
notification/toast API such as notification.error or message.error with a
concise message like "Failed to refresh parameter metadata") while still logging
the error details to the logger; ensure setIsRefetching(false) remains in
finally and keep updateParameterMetadata/form.setFieldValue behavior unchanged.
After refetching parameter metadata, the code now re-checks for missing inputs using the latest form parameters. This ensures that any changes in parameter dependencies are properly handled.
Introduced dashed line rendering for PathLayer using PathStyleExtension. Lines are now dashed unless the 'peak_mass_flow' property is present, improving visual distinction of features.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/map/components/Map/Map.jsx(2 hunks)
🔇 Additional comments (1)
src/features/map/components/Map/Map.jsx (1)
10-10: LGTM!The
PathStyleExtensionimport is correctly added to support the dash styling functionality for thermal network edges.
Updated Map and MapTooltip components to safely handle cases where 'peak_mass_flow' may be null or undefined. This prevents potential runtime errors and ensures consistent tooltip and layer rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/features/map/components/Map/Map.jsx (1)
223-223: LGTM! Safer property access with sensible default.The optional chaining with nullish coalescing (
?.['peak_mass_flow'] ?? 0) safely handles missing properties and defaults to zero for width calculation, ensuring edges without flow data still render with minimum width.Optionally, for defensive coding and consistency with MapTooltip.jsx (line 181), consider explicit Number conversion:
getLineWidth: (f) => normalizeLineWidth( - f.properties?.['peak_mass_flow'] ?? 0, + Number(f.properties?.['peak_mass_flow'] ?? 0), min, max, 1, 7 * scale, ),However, since GeoJSON properties are typically numeric and the current code works, this is purely defensive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/map/components/Map/Map.jsx(2 hunks)src/features/map/components/Map/MapTooltip.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/map/components/Map/MapTooltip.jsx (1)
src/features/building-editor/components/building-editor.jsx (1)
properties(45-45)
🔇 Additional comments (3)
src/features/map/components/Map/MapTooltip.jsx (1)
179-182: LGTM! Zero values now handled correctly.The explicit null check (
!= null) correctly treats zero as a valid value, while still catching null/undefined. This ensures edges with zero peak mass flow display "0 kg/s" instead of "N/A".src/features/map/components/Map/Map.jsx (2)
10-10: LGTM! Import required for dash functionality.The
PathStyleExtensionimport is correctly added to support the dashed-line rendering introduced on lines 230-234.
230-234: LGTM! Dash logic correctly handles zero values.The explicit null check (
!= null) ensures edges with zero peak mass flow render as solid lines ([0, 0]), while edges lacking flow data render as dashed ([8, 4]). This correctly distinguishes present data (including zero) from missing data.This addresses the concern raised in the previous review about truthy checks incorrectly treating zero as a missing value.
Plant nodes are now displayed using a bright yellow star icon via an IconLayer, rendered above other node types for better visibility. Node layers are split by type (PLANT, CONSUMER, NONE) for improved rendering order, and tooltips now show a more descriptive title based on node type.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/features/map/components/Map/Map.jsx (2)
6-16: Imports for deck.gl IconLayer / PathStyleExtension and star icon look correctThe new imports are consistent with their usage below (IconLayer for plant nodes and PathStyleExtension for dashed edges). The
// eslint-disable-next-line import/no-unresolvedaround thestar-fill.svg?urlimport is a reasonable workaround if the bundler handles theassets/alias and?urlloader; longer term you may want to teach ESLint about that alias to avoid suppressing the rule globally for this line.
250-357: Ensure robust node typing and clarify use ofscalefor node sizesThe new node partitioning and layering (NONE → CONSUMER → PLANT IconLayer) is a good approach for controlling z-order and styling. A couple of small details are worth tightening up:
Fallbacks for unknown/missing node
type
nodeFillColor,nodeLineColor, andnodeRadiusonly handle'NONE' | 'CONSUMER' | 'PLANT'. Any feature with an unexpected or missingtypefalls into the implicitundefinedbranch, which leads toundefinedcolors/radii being passed to deck.gl.It’s safer to provide explicit defaults:
const nodeFillColor = (type) => { if (type === 'NONE') { return edgeColour; } else if (type === 'CONSUMER') { return hexToRgb(colours?.nodes?.consumer) ?? [255, 255, 255]; } else if (type === 'PLANT') { return hexToRgb(colours?.nodes?.plant) ?? [255, 255, 255]; } // Fallback for unexpected types return edgeColour; }; const nodeLineColor = (type) => { if (type === 'PLANT') { return [255, 255, 255]; } // Default for NONE / CONSUMER / unknown return edgeColour; }; const nodeRadius = (type) => { if (type === 'NONE') return 1; if (type === 'CONSUMER') return 3; if (type === 'PLANT') return 5; // Fallback return 2; };Consistency between
scaleand node radius / icon size
nodeRadiusignoresscale, but the GeoJson node layers declareupdateTriggers: { getPointRadius: [scale] }, which suggests size is meant to respond toscale.- For the plant
IconLayer,getSizeis a constant (10 * scale) and you also setupdateTriggers.getSize: [scale]. Since the layer is recreated wheneverscalechanges (via theuseMemodeps), the explicit updateTrigger is redundant.Consider either:
- Incorporating
scaleintonodeRadius(e.g., multiply the base radii byscale) so the updateTriggers reflect actual behavior; or- Dropping
scalefrom the nodeupdateTriggers(and possibly from the IconLayer’supdateTriggers) if you prefer fixed screen-space sizing.These tweaks will make behavior clearer and avoid edge cases where nodes silently disappear due to undefined styling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/icons/star-fill.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
src/features/map/components/Map/Map.jsx(3 hunks)src/features/map/components/Map/MapTooltip.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/map/components/Map/MapTooltip.jsx
| getLineWidth: (f) => | ||
| normalizeLineWidth( | ||
| f.properties['peak_mass_flow'], | ||
| f.properties?.['peak_mass_flow'] ?? 0, | ||
| min, | ||
| max, | ||
| 1, | ||
| 7 * scale, | ||
| ), | ||
| getLineColor: edgeColour, | ||
| getDashArray: (f) => | ||
| f.properties?.['peak_mass_flow'] != null ? [0, 0] : [8, 4], | ||
| dashJustified: true, | ||
| dashGapPickable: true, | ||
| extensions: [new PathStyleExtension({ dash: true })], | ||
| updateTriggers: { |
There was a problem hiding this comment.
🧩 Analysis chain
Guard against invalid/missing peak_mass_flow when computing line width
Right now getLineWidth passes f.properties?.['peak_mass_flow'] ?? 0 into normalizeLineWidth. If min > 0 (which is likely when you have only positive flows), edges with missing/invalid peak_mass_flow will use 0, which is outside [min, max]. Given the current normalizeLineWidth implementation, this can produce widths smaller than minWidth or even effectively invisible lines, which is probably not intended for “no data” edges.
You can keep the current behavior for valid values while falling back to a safe minimum width for missing/invalid ones by guarding the accessor:
- getLineWidth: (f) =>
- normalizeLineWidth(
- f.properties?.['peak_mass_flow'] ?? 0,
- min,
- max,
- 1,
- 7 * scale,
- ),
+ getLineWidth: (f) => {
+ const raw = f.properties?.['peak_mass_flow'];
+ const value = raw != null ? Number(raw) : null;
+
+ // Fall back to minimum width when value is missing/invalid
+ if (value == null || Number.isNaN(value)) {
+ return 1;
+ }
+
+ return normalizeLineWidth(value, min, max, 1, 7 * scale);
+ },The dashed logic (getDashArray using a strict null/undefined check) correctly treats 0 as a present value, addressing the earlier zero-handling concern.
I'll help verify this review comment. Let me gather information about the code structure and the normalizeLineWidth function.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the normalizeLineWidth function definition and understand its behavior
rg -n "normalizeLineWidth" --type=jsx --type=js -B2 -A10
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look at the broader context of the Map.jsx file around the flagged lines
cat src/features/map/components/Map/Map.jsx | head -280 | tail -100Length of output: 3309
🏁 Script executed:
#!/bin/bash
# Find where min and max are computed/set for this specific context
rg -n "min\|max" src/features/map/components/Map/Map.jsx -B3 -A3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are other uses of getLineWidth or normalizeLineWidth in the codebase
rg -n "getLineWidth|normalizeLineWidth" --type=jsx --type=js -C2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for utility/helper files that might contain normalizeLineWidth
fd -e js -e jsx -e ts -e tsx | xargs rg -l "normalizeLineWidth" 2>/dev/null | head -20Length of output: 39
I need to search more carefully for the function definitions. Let me adjust the approach:
---
🏁 Script executed:
```shell
#!/bin/bash
# Look at the beginning of Map.jsx for imports
head -50 src/features/map/components/Map/Map.jsx
Length of output: 1492
🏁 Script executed:
#!/bin/bash
# Search for normalizeLineWidth definition without file type restriction
rg -n "normalizeLineWidth" -B2 -A10Length of output: 2037
🏁 Script executed:
#!/bin/bash
# Search for getPropertyRange definition
rg -n "getPropertyRange" -B2 -A15Length of output: 2724
🏁 Script executed:
#!/bin/bash
# Look for all imports and exports in Map.jsx
grep -n "import\|export\|normalizeLineWidth" src/features/map/components/Map/Map.jsx | head -30Length of output: 1200
🏁 Script executed:
#!/bin/bash
# Get the full context of the Map.jsx file - look at top section
head -100 src/features/map/components/Map/Map.jsxLength of output: 2997
Guard against invalid/missing peak_mass_flow when computing line width
The issue is confirmed. When peak_mass_flow is missing, the code defaults to 0 via ?? 0. Since getPropertyRange filters out null/NaN values (keeping only valid numerics), if all actual flow values are positive (min > 0), passing 0 to normalizeLineWidth violates its expected range assumption and produces out-of-range line widths via the formula 1 + ((0 - min) / (max - min)) * (7*scale - 1), resulting in values below minWidth or effectively invisible lines.
The suggested fix is appropriate: return a safe minimum width (1) for missing/invalid values instead of normalizing with out-of-range input.
- getLineWidth: (f) =>
- normalizeLineWidth(
- f.properties?.['peak_mass_flow'] ?? 0,
- min,
- max,
- 1,
- 7 * scale,
- ),
+ getLineWidth: (f) => {
+ const raw = f.properties?.['peak_mass_flow'];
+ const value = raw != null ? Number(raw) : null;
+
+ // Fall back to minimum width when value is missing/invalid
+ if (value == null || Number.isNaN(value)) {
+ return 1;
+ }
+
+ return normalizeLineWidth(value, min, max, 1, 7 * scale);
+ },📝 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.
| getLineWidth: (f) => | |
| normalizeLineWidth( | |
| f.properties['peak_mass_flow'], | |
| f.properties?.['peak_mass_flow'] ?? 0, | |
| min, | |
| max, | |
| 1, | |
| 7 * scale, | |
| ), | |
| getLineColor: edgeColour, | |
| getDashArray: (f) => | |
| f.properties?.['peak_mass_flow'] != null ? [0, 0] : [8, 4], | |
| dashJustified: true, | |
| dashGapPickable: true, | |
| extensions: [new PathStyleExtension({ dash: true })], | |
| updateTriggers: { | |
| getLineWidth: (f) => { | |
| const raw = f.properties?.['peak_mass_flow']; | |
| const value = raw != null ? Number(raw) : null; | |
| // Fall back to minimum width when value is missing/invalid | |
| if (value == null || Number.isNaN(value)) { | |
| return 1; | |
| } | |
| return normalizeLineWidth(value, min, max, 1, 7 * scale); | |
| }, | |
| getLineColor: edgeColour, | |
| getDashArray: (f) => | |
| f.properties?.['peak_mass_flow'] != null ? [0, 0] : [8, 4], | |
| dashJustified: true, | |
| dashGapPickable: true, | |
| extensions: [new PathStyleExtension({ dash: true })], | |
| updateTriggers: { |
🤖 Prompt for AI Agents
In src/features/map/components/Map/Map.jsx around lines 224 to 238, the current
logic passes 0 when peak_mass_flow is missing which breaks normalizeLineWidth
(producing out-of-range widths); change the getLineWidth handler to detect
missing/invalid values (null/undefined/NaN) and immediately return a safe
minimum width (e.g., 1) instead of calling normalizeLineWidth with 0, otherwise
pass the actual numeric peak_mass_flow into normalizeLineWidth as before.
Removed the star-fill.svg icon and added triangle-fill.svg. Updated Map.jsx to use the new triangle icon for plant nodes in the IconLayer, ensuring visual consistency and clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/features/map/components/Map/Map.jsx (1)
224-231: Guard against invalid/missingpeak_mass_flowwhen computing line widthThe issue flagged in the previous review remains unaddressed. Line 226 still defaults to
0whenpeak_mass_flowis missing, which can produce out-of-range line widths if all valid flow values are positive (min > 0). This causes edges with missing data to render with widths below the intended minimum or become effectively invisible.Apply the previously suggested fix:
- getLineWidth: (f) => - normalizeLineWidth( - f.properties?.['peak_mass_flow'] ?? 0, - min, - max, - 1, - 7 * scale, - ), + getLineWidth: (f) => { + const raw = f.properties?.['peak_mass_flow']; + const value = raw != null ? Number(raw) : null; + + // Fall back to minimum width when value is missing/invalid + if (value == null || Number.isNaN(value)) { + return 1; + } + + return normalizeLineWidth(value, min, max, 1, 7 * scale); + },
🧹 Nitpick comments (1)
src/features/map/components/Map/Map.jsx (1)
270-290: LGTM!The NONE nodes layer is correctly configured. The layer properly handles empty node arrays and uses appropriate styling functions.
Consider adding
updateTriggersfor color-related getters if thecoloursconfiguration can change at runtime:updateTriggers: { getPointRadius: [scale], + getFillColor: [mapLayers[name]?.properties?.colours], + getLineColor: [mapLayers[name]?.properties?.colours], },This ensures the layer re-renders when color configuration changes. If colors are static per network load, the current implementation is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/icons/triangle-fill.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
src/features/map/components/Map/Map.jsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/map/components/Map/Map.jsx (1)
src/features/map/hooks/map-layers.js (1)
mapLayers(111-111)
🔇 Additional comments (5)
src/features/map/components/Map/Map.jsx (5)
6-6: LGTM!The new imports are correctly added and used in the refactored node rendering logic. The
?urlsuffix for the SVG asset import is the proper Vite convention for static asset URLs.Also applies to: 11-11, 15-16
233-237: LGTM!The dash styling implementation is correct. The
getDashArrayproperly handles zero values with an explicit null check, and the dash-related properties (dashJustified,dashGapPickable) are appropriately configured for thePathStyleExtension.
250-267: LGTM!The node partitioning logic is well-implemented. Using
reduceto categorize nodes by type is clean and efficient, and the optional chaining with sensible defaults ensures robustness when node data is missing.
293-313: LGTM!The CONSUMER nodes layer follows the same pattern as the NONE nodes layer, maintaining consistency. The implementation is correct and will render consumer nodes above the NONE nodes as intended.
317-357: Verify the hardcoded PLANT node colorThe IconLayer for PLANT nodes uses a hardcoded bright yellow color (line 319) instead of using
colours?.nodes?.plantfrom the network configuration (as defined in thenodeFillColorfunction at lines 192-200). While the comment indicates this is for "high visibility," this creates an inconsistency where the color configuration is ignored for PLANT nodes.Please confirm:
- Is the hardcoded yellow color intentional for all networks, or should PLANT nodes respect the configured
colours.nodes.plantvalue?- If intentional, consider documenting this behavior clearly, as it differs from CONSUMER and NONE node styling.
If PLANT nodes should use the configured color, apply this diff:
- // Use bright yellow for high visibility and to complement blue/red edges - const plantColor = [255, 209, 29, 255]; // Bright yellow + const plantColor = nodeFillColor('PLANT');Otherwise, the IconLayer configuration is well-implemented with appropriate settings for anchor, masking, billboarding, and sizing.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements