networkDesign: replace SimulationAlgorithmDescriptor with UserDefinedConfigSchema#1848
Open
tahini wants to merge 3 commits into
Open
networkDesign: replace SimulationAlgorithmDescriptor with UserDefinedConfigSchema#1848tahini wants to merge 3 commits into
SimulationAlgorithmDescriptor with UserDefinedConfigSchema#1848tahini wants to merge 3 commits into
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
WalkthroughThe PR introduces a new user-defined config schema system and migrates simulation/algorithm descriptors from a SimulationAlgorithmDescriptor API to UserDefinedConfigSchema (renaming getOptions→getFields and validateOptions→validateFields). It moves GA/transit i18n keys into a transit:networkDesign namespace, updates algorithm/method implementations and tests, adds utils for defaulting/validation of schema-driven fields, and implements a frontend UserDefinedConfigEditComponent with related widget and SimulationEdit changes. Numerous tests were added/updated to exercise the new schema and UI behavior. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts (1)
58-84: 🧹 Nitpick | 🔵 TrivialPrefer
test.eachfor this validation matrix.Bundling several expectations into each test makes failures harder to localize.
As per coding guidelines, "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts` around lines 58 - 84, Replace the grouped assertions in the "sampleRatio validation" suite with parametric tests using test.each to make failures localizable: for the positive case use descriptor.getFields().sampleRatio.validate and table rows like [0.5, true], [1, true], [0.1, true]; for non-positive use rows [0, false], [-0.1, false], [-1, false]; and for >1 use rows [1.1, false], [2, false], [10, false]; ensure each test calls the same validate function (obtained via descriptor.getFields().sampleRatio.validate) and asserts the expected boolean per row.packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.ts (1)
52-60: 🧹 Nitpick | 🔵 TrivialPrefer
test.eachfor the ratio validation matrix.This is easier to maintain and debug as a table-driven test.
As per coding guidelines, "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.ts` around lines 52 - 60, Replace the multiple expect assertions with a parametric table-driven test using test.each: build an array of [input, expected] cases and call test.each to iterate them, invoking the existing validate function (from descriptor.getFields() -> options.sampleRatio.validate) for each case and asserting the expected boolean; update the test named 'should validate sampleRatio correctly' to use this table (including cases 0.5->true, 1->true, 0->false, -0.1->false, 1.1->false) so failures are reported per row.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@locales/fr/transit.json`:
- Around line 870-872: The French translation strings
NumberOfGenerationsInvalid, KeepGenerationsInvalid, and KeepCandidatesInvalid
are stricter than the validator in EvolutionaryAlgorithm.ts which allows 0;
update these messages to reflect "greater than or equal to 0" (e.g., "doit être
supérieur ou égal à 0" / "doivent être supérieurs ou égaux à 0") so they match
the actual validation for numberOfGenerations, keepGenerations, and
keepCandidates.
In `@packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts`:
- Around line 39-40: The call descriptor.choices({}) discards the current field
values; update it to pass the current configuration object used in this scope
(replace {} with the existing config/values object that holds previously entered
defaults) so dependent choice builders receive the full field object—locate the
descriptor.choices call and replace the empty object with the appropriate
current config variable (the same value used elsewhere in this module for form
state) to ensure choices are generated correctly.
- Around line 20-21: The getPromptForDescriptor function currently accepts any
UserDefinedConfigFieldDescriptor but only handles 'integer'/'number', 'boolean',
and 'select', causing unsupported kinds to fall through to input() and produce
invalid shapes; update getPromptForDescriptor to explicitly handle each
descriptor.kind (at minimum: 'multiselect', 'nested', 'seconds', 'percentage',
'csvFile') by mapping them to the correct prompt builder or validation/shape (or
throw a clear error for kinds the CLI does not support), and include
options.t/options.name where needed; ensure the switch over descriptor.kind is
exhaustive (or adds a default that throws with descriptor.kind and options.name)
so malformed schema inputs are detected instead of silently using input().
- Line 69: The file still calls descriptor.getOptions() after getDescriptor()
now returns UserDefinedConfigSchema; replace that call with
descriptor.getFields() to complete the getOptions → getFields rename (search for
descriptor.getOptions in AlgorithmEdition.ts and update it), and ensure any
local variables or types previously named getOptions-related are adjusted to use
getFields (e.g., the existing const algoOptions = descriptor.getFields() should
be the single source); run type-check to confirm getDescriptor(),
descriptor.getFields(), and any usages of algoOptions match the new
UserDefinedConfigSchema API.
In
`@packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts`:
- Around line 101-127: The three config entries numberOfGenerations,
keepGenerations, and keepCandidates should use the same 'integer' type as
populationSizeMin and validate integers; change their type from 'number' to
'integer' and update their validate functions to check Number.isInteger(value)
&& value >= 0 (keeping the current defaults), so the fields enforce whole-number
counts consistently with populationSizeMin.
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.ts`:
- Around line 91-93: The tests call odTripFitnessFunction.choices() and
fitnessFunction.choices() with await even though these functions are synchronous
and return arrays; remove the await and treat them as direct returns. In the
test using descriptor.getFields(), update the lines that assign choices from
options.odTripFitnessFunction.choices() and options.fitnessFunction.choices() to
call them without await so the variables receive the returned arrays directly,
and run assertions unchanged.
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.ts`:
- Line 34: The parameter named object in the choices arrow function is unused
and triggers the linter; rename it to _object (i.e., change choices: (object:
Record<string, unknown>) => to choices: (_object: Record<string, unknown>) =>)
so it is recognized as intentionally unused, and update any local references if
they exist; the change should be made in the choices definition inside
OdTripSimulationMethod.ts.
In
`@packages/transition-common/src/services/simulation/__tests__/Simulation.test.ts`:
- Around line 169-172: The test currently compares
Object.keys(simulation1.attributes.data.algorithmConfiguration?.config ||
{}).length to stubAlgorithm.getFields().length but getDefaultFieldsFromSchema
only returns fields that have explicit defaults, and
SimulationAlgorithmDescriptorStub's three fields have no defaults, so the config
is empty; update the assertion to compare against the actual defaults produced
(e.g., use Object.keys(getDefaultFieldsFromSchema({}, new
SimulationAlgorithmDescriptorStub()) as any).length) or change the stub to
provide defaults on its fields, and keep references to
getDefaultFieldsFromSchema, SimulationAlgorithmDescriptorStub, Simulation, and
simulation1.attributes.data.algorithmConfiguration.config to locate the change.
In
`@packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts`:
- Around line 438-446: The test never exercises the multiselect branch because
the schema defines multiselectField but initialOptions uses selectField; update
the test so initialOptions provides multiselectField (e.g., initialOptions: {
multiselectField: ['choice1'] }) so validateFieldsWithSchema actually validates
the multiselectField against the choices in mockSchema; keep the rest of the
test (description, expected) unchanged and ensure the field name matches the
schema's getFields entry.
- Around line 70-74: The tests share a single validateMock and queue
mockReturnValueOnce values at module init, making test order-dependent; instead,
reinitialize validateMock before each test (inside beforeEach assign
validateMock = jest.fn()) and then set per-test return values
(mockReturnValueOnce) inside each test or the test.each body so each case gets a
fresh mock; update occurrences that reference validateMock (the validateMock
variable and the beforeEach block and test.each table usages) across the file so
no shared queued returns persist between tests.
In `@packages/transition-common/src/utils/userDefinedConfig/index.ts`:
- Around line 187-227: The code lacks default primitive type validation when
fieldDef has no custom validate: update the branch handling "else if
(fieldDef.type === 'percentage')" to instead perform built-in type checks for
types 'number', 'integer', 'seconds', 'percentage', 'string', and 'boolean' (and
keep the existing percentage range check and select/multiselect logic).
Concretely, inside the else (i.e. when 'validate' is absent) ensure for
number/seconds/percentage the value is typeof 'number' (and for 'integer' also
Number.isInteger(value)), for 'string' ensure typeof 'string', and for 'boolean'
ensure typeof 'boolean'; if any check fails set valid = false and push
fieldDef.i18nName + 'Invalid' to errors; retain the existing percentage 0..1
range check and the select/multiselect choices validation using
fieldDef.choices(fields as Record<string, unknown>).
- Around line 176-182: The nested validation always recurses using an empty
object which causes required children to fail when the parent is optional;
change the logic in the block handling fieldDef.type === 'nested' so that you
only call validateFieldsWithSchema((value as Record<string, unknown>) || {},
nestedDescriptor) when the parent value is present (not null/undefined) — if
value is undefined and fieldDef.required is falsy, skip child validation
entirely; keep using nestedDescriptor and validateFieldsWithSchema when value
exists to preserve recursive validation.
- Around line 96-97: The union type for UserDefinedConfigFieldDescriptor uses
NestedFieldDescriptor<any>, which weakens type safety; update the declaration of
UserDefinedConfigFieldDescriptor to use NestedFieldDescriptor<Record<string,
unknown>> instead of any so the shared API boundary remains strictly typed and
consistent with coding guidelines; locate the type alias
UserDefinedConfigFieldDescriptor and replace the NestedFieldDescriptor<any>
reference accordingly (keeping other parts: UserDefinedConfigFieldBase and
UserDefinedConfigFieldByType unchanged).
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx`:
- Around line 290-295: The onValueChange handler ignores the per-widget
readiness flag (newValue.valid) and only uses schema validation; update
onValueChange to combine newValue.valid with the schema result from
validateFieldsWithSchema(props.configSchema) so that props.onUpdate receives a
final validity that is false if either the widget reports invalid
(newValue.valid===false) or the schema check fails; ensure you still call
setErrors(errors) with schema errors and reference the handler name
onValueChange, the widget CsvFileOptionComponent, the validator
validateFieldsWithSchema, and the update call props.onUpdate when making the
change.
- Around line 283-288: The effect in React.useEffect calls
getDefaultFieldsFromSchema and validateFieldsWithSchema but only forwards the
boolean valid to props.onUpdate, so initial validation errors are dropped;
capture the errors returned by validateFieldsWithSchema and persist them by
passing them into the update path (e.g., include the errors argument when
calling props.onUpdate or call the component's error setter if one exists) so
that initial invalid defaultFields show validation feedback; update the call
site in OptionsDescriptorWidgets.tsx (React.useEffect) to forward the errors
alongside defaultFields and valid.
- Around line 67-76: The InputSelect and other child editor components are not
receiving the parent disabled state, allowing edits when the form is read-only;
update each child instantiation (e.g., the InputSelect in this block and the
similar usages at the other blocks referenced) to forward the parent's disabled
flag (props.disabled or the relevant read-only prop) as a prop (e.g.,
disabled={props.disabled}) so the select and nested schema editors respect the
form's read-only/disabled state; apply the same change to the other occurrences
mentioned (lines 90-100 and 250-259) and any nested editor components so they
all receive and honor the disabled prop.
- Around line 80-94: MultiSelectOptionComponent is passing raw i18n keys to
InputMultiselect via choices; translate them before rendering by using i18next
(e.g., import/useTranslation and call t) and mapping
option.choices(props.completeObject) to a new array of { ...choice, label:
t(choice.label) } (or t(choice) if choices are simple keys) before passing to
InputMultiselect so labels are localized; keep the component name
(MultiSelectOptionComponent), the call to option.choices(props.completeObject),
and the prop passed to InputMultiselect (choices) as the places to change.
- Around line 37-42: The exported editor component has collapsed the generic T
to any; restore the generic by declaring the component with a generic parameter
and using it in all prop types: keep UserDefinedConfigEditComponentProps<T> and
change the exported component signature to a generic component (e.g., export
const UserDefinedConfigEditor = <T,>(props:
UserDefinedConfigEditComponentProps<T>) => { ... }) and update any internal
callbacks/props to use value: T and onUpdate: (parameters: Partial<T>, isValid:
boolean) => void (replace Partial<any> and value: any). Also ensure any other
uses inside the file that reference the editor or
UserDefinedConfigEditComponentProps are updated to propagate the generic T
rather than using any.
---
Outside diff comments:
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts`:
- Around line 58-84: Replace the grouped assertions in the "sampleRatio
validation" suite with parametric tests using test.each to make failures
localizable: for the positive case use
descriptor.getFields().sampleRatio.validate and table rows like [0.5, true], [1,
true], [0.1, true]; for non-positive use rows [0, false], [-0.1, false], [-1,
false]; and for >1 use rows [1.1, false], [2, false], [10, false]; ensure each
test calls the same validate function (obtained via
descriptor.getFields().sampleRatio.validate) and asserts the expected boolean
per row.
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.ts`:
- Around line 52-60: Replace the multiple expect assertions with a parametric
table-driven test using test.each: build an array of [input, expected] cases and
call test.each to iterate them, invoking the existing validate function (from
descriptor.getFields() -> options.sampleRatio.validate) for each case and
asserting the expected boolean; update the test named 'should validate
sampleRatio correctly' to use this table (including cases 0.5->true, 1->true,
0->false, -0.1->false, 1.1->false) so failures are reported per row.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2939b09e-1905-4f48-bf26-a32c38db5564
⛔ Files ignored due to path filters (1)
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/__snapshots__/OptionsDescriptorWidgets.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
locales/en/transit.jsonlocales/fr/transit.jsonpackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/TransitNetworkDesignAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-frontend/src/components/forms/simulation/SimulationEdit.tsxpackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
💤 Files with no reviewable changes (2)
- packages/transition-frontend/src/components/forms/simulation/SimulationEdit.tsx
- packages/transition-common/src/services/networkDesign/transit/TransitNetworkDesignAlgorithm.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr-build-check
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxpackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Files:
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxpackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts
packages/transition-frontend/src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
React components should use Redux for state management, i18next for translations, and SCSS for styling
Files:
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts
**/*.json
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Translation files in
locales/directory should organize strings by namespace (auth.json, transit.json, main.json, etc.) with support for both English (en/) and French (fr/) languages
Files:
locales/fr/transit.jsonlocales/en/transit.json
🧠 Learnings (16)
📚 Learning: 2025-12-17T22:46:52.336Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1666
File: packages/chaire-lib-frontend/package.json:60-60
Timestamp: 2025-12-17T22:46:52.336Z
Learning: In packages/chaire-lib-frontend/package.json, the `testing-library/dom` devDependency is required for tests to pass even though it's not explicitly imported. It was previously provided as a transitive dependency through `react-select-event`, but after removing `react-select-event`, it must be explicitly declared.
Applied to files:
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
📚 Learning: 2026-01-21T14:59:52.530Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1592
File: packages/transition-frontend/src/components/map/CircleSpinnerExtension.tsx:37-44
Timestamp: 2026-01-21T14:59:52.530Z
Learning: In TSX files under packages/transition-frontend/src that currently rely on deck.gl's this.context.animationProps.time for animation timing, prefer window.performance.now() (or performance.now()) to measure elapsed time. Replace time-based calculations tied to deck.gl with direct high-resolution timestamps to improve accuracy and decouple from the rendering library. Ensure animations use delta = now - lastTime, and update lastTime accordingly for smooth, frame-rate-independent timing.
Applied to files:
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx
📚 Learning: 2026-02-24T00:36:44.483Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/__tests__/propertyRegistry.db.test.ts:253-258
Timestamp: 2026-02-24T00:36:44.483Z
Learning: In test files, it is acceptable to use 'any' types for error handling and test scenarios. Reserve strict TypeScript typing for production code; in tests, prioritize pragmatic, readable test cases, and use 'any' only where it clearly improves test clarity or reduces boilerplate.
Applied to files:
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxpackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts
📚 Learning: 2025-11-04T19:35:10.045Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1554
File: packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts:23-36
Timestamp: 2025-11-04T19:35:10.045Z
Learning: In the OdTripSimulationMethod (packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts), the team plans to move away from the datasource path and use batch calculations instead. The current non-functional dataSourceId selector will be reworked as part of this architectural change.
Applied to files:
packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: When modifying transit data models that use Cap'n Proto caching, update the Cap'n Proto schema, regenerate TypeScript types, and regenerate the cache
Applied to files:
packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.ts
📚 Learning: 2026-03-02T21:57:21.005Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1830
File: packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts:114-122
Timestamp: 2026-03-02T21:57:21.005Z
Learning: In test files for database query modules, ensure mocked functions match the async/sync nature of the production implementations. If a function returns a Promise in production (async), use an async mock (e.g., jest.fn().mockImplementation(async (...args) => { ... })). If a function is synchronous (e.g., returns a stream or uses direct values), use a synchronous mock. This helps prevent false positives/negatives due to mismatched mock behavior.
Applied to files:
packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.test.{ts,tsx} : Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Applied to files:
packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts
📚 Learning: 2026-02-06T18:45:02.744Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1767
File: packages/transition-backend/src/services/accessibilityMap/TransitAccessibilityMapCalculator.ts:106-107
Timestamp: 2026-02-06T18:45:02.744Z
Learning: In the transition codebase, the pattern `dataSourceId: attributes.dataSourceId || undefined` is intentionally used to convert empty strings to undefined, ensuring optional string fields are normalized (empty string becomes undefined rather than remaining as an empty string).
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.ts
📚 Learning: 2025-11-25T21:28:28.491Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1604
File: packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts:74-79
Timestamp: 2025-11-25T21:28:28.491Z
Learning: In packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts, the maxBirdDistanceMeters, maxNetworkDistanceMeters, and maxTravelTimeSeconds parameters should not accept 0 or negative values, as they are semantically invalid for distance and time thresholds. Explicit validation should be preferred over relying on falsy coercion.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts
📚 Learning: 2026-02-06T19:06:19.438Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1767
File: packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx:177-178
Timestamp: 2026-02-06T19:06:19.438Z
Learning: In the transition accessibility map API responses (packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx), the populationData property is always present as an object with the structure { population: number | null; dataSourceAreaRatio: number }. Even when population calculation is disabled or the data source is missing, populationData is returned with population set to null rather than populationData being undefined. Therefore, accessing properties1.populationData.population without optional chaining is safe.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts
📚 Learning: 2025-09-25T17:08:56.828Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:105-107
Timestamp: 2025-09-25T17:08:56.828Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred changing the wrapBatchAccessMap input file validation from `hasInputFile()` to `getFilePath('input')` (actual file existence check), indicating it will be addressed in an upcoming file access refactor.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/**/*.tsx : React components should use Redux for state management, i18next for translations, and SCSS for styling
Applied to files:
packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.json : Translation files in `locales/` directory should organize strings by namespace (auth.json, transit.json, main.json, etc.) with support for both English (en/) and French (fr/) languages
Applied to files:
locales/fr/transit.jsonlocales/en/transit.json
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/routers/*.tsx : Application routing should be defined in `TransitionRouter.tsx` using React Router with feature-based route organization
Applied to files:
packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.{ts,tsx} : Use i18next and the `t()` function for all user-facing strings to support internationalization
Applied to files:
packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx
🪛 GitHub Check: code-lint
packages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.ts
[failure] 34-34:
'object' is defined but never used. Allowed unused args must match /^_/u
🔇 Additional comments (9)
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts (1)
50-53: Thechoices()method signature in the descriptor is() => { ... }(zero parameters), so calling it without arguments is correct and matches the implementation. No changes needed at line 52.> Likely an incorrect or invalid review comment.locales/en/transit.json (1)
845-900: LGTM!The new
networkDesigntranslation block is well-organized with all required keys for the evolutionary algorithm configuration. The three validation error keys referenced inEvolutionaryAlgorithm.ts(PopulationSizeMinGreaterThanMax,CandidatesToKeepGreaterThanPopulation,GenerationsToKeepGreaterThanGenerations) are all present. Help texts are comprehensive.packages/transition-common/src/services/networkDesign/transit/algorithm/index.ts (1)
8-8: LGTM!Clean type migration from
SimulationAlgorithmDescriptortoUserDefinedConfigSchema. The generic typing is properly preserved throughout the file.Also applies to: 41-45, 53-57
packages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts (1)
17-21: LGTM!Tests properly updated to use the new
getFields()andvalidateFields()methods. Good use of parametric tests for option groups, and the i18n key updates are consistent with the translation file.Also applies to: 23-46, 171-177
packages/transition-common/src/services/simulation/Simulation.ts (3)
28-29: LGTM!The migration to
getFields()andgetDefaultFieldsFromSchema()is clean. The FIXME comments appropriately document the temporary type casting needed during the transition to jobs-based architecture.Also applies to: 83-83, 130-138
150-155: Type mismatch acknowledged.The FIXME correctly notes that
validateFields()returnsTranslatableMessage[]whilethis._errorsexpectsstring[]. Since this class is being replaced by network design jobs, the cast is acceptable.
188-191: LGTM!Return type updated to
UserDefinedConfigSchema<any>. Theanygeneric is pragmatic here since the method returns descriptors for multiple algorithm types.packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts (2)
32-34: LGTM!Clean implementation of
UserDefinedConfigSchema. All field descriptors are well-defined with appropriate i18n keys, types, validators, and defaults.Also applies to: 37-128
130-163: LGTM!Cross-field validation logic is correct. The three validation rules properly check:
populationSizeMin≤populationSizeMaxkeepCandidates≤populationSizeMinkeepGenerations≤numberOfGenerations
Comment on lines
+37
to
+42
| type UserDefinedConfigEditComponentProps<T extends Record<string, unknown>> = { | ||
| configSchema: UserDefinedConfigSchema<T>; | ||
| value: T; | ||
| disabled?: boolean; | ||
| onUpdate: (parameters: Partial<T>, isValid: boolean) => void; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx | head -100Repository: chairemobilite/transition
Length of output: 4604
🏁 Script executed:
cat -n packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx | sed -n '240,280p'Repository: chairemobilite/transition
Length of output: 1847
🏁 Script executed:
cd packages/transition-frontend/src/components/forms/userDefinedConfig && grep -r "UserDefinedConfigEditComponent" --include="*.tsx" --include="*.ts" | head -20Repository: chairemobilite/transition
Length of output: 924
🏁 Script executed:
cat -n packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx | sed -n '1,50p'Repository: chairemobilite/transition
Length of output: 1777
🏁 Script executed:
cat -n packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx | sed -n '50,120p'Repository: chairemobilite/transition
Length of output: 2833
🏁 Script executed:
grep -n "as.*Config\|as.*any" packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxRepository: chairemobilite/transition
Length of output: 181
Keep the exported editor generic instead of collapsing it to any.
The exported component erases the generic type T to any (lines 275–276), forcing unsafe casts at call sites (as TestConfig in tests) and losing the type safety the schema API should provide. Keep the component generic without hardcoding any.
Also applies to: line 254 (Partial<any>), line 50 (value: any in callback).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx`
around lines 37 - 42, The exported editor component has collapsed the generic T
to any; restore the generic by declaring the component with a generic parameter
and using it in all prop types: keep UserDefinedConfigEditComponentProps<T> and
change the exported component signature to a generic component (e.g., export
const UserDefinedConfigEditor = <T,>(props:
UserDefinedConfigEditComponentProps<T>) => { ... }) and update any internal
callbacks/props to use value: T and onUpdate: (parameters: Partial<T>, isValid:
boolean) => void (replace Partial<any> and value: any). Also ensure any other
uses inside the file that reference the editor or
UserDefinedConfigEditComponentProps are updated to propagate the generic T
rather than using any.
tahini
force-pushed
the
userDefinedOptionDescriptor
branch
from
bae3c79 to
ab46e30
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.ts (1)
53-59:⚠️ Potential issue | 🟠 Major
validateFieldscurrently accepts invalid configurations.Right now it always returns
valid: true, so invalid values can bypass validation when coming from non-UI paths.Suggested fix
- validateFields = (_fields: Partial<AccessibilityMapSimulationOptions>): { valid: boolean; errors: string[] } => { - const valid = true; - const errors: string[] = []; - - // TODO Actually validate something - - return { valid, errors }; - }; + validateFields = (fields: Partial<AccessibilityMapSimulationOptions>): { valid: boolean; errors: string[] } => { + const errors: string[] = []; + + if (fields.sampleRatio !== undefined && !(fields.sampleRatio > 0 && fields.sampleRatio <= 1)) { + errors.push('transit:networkDesign.simulationMethods.accessibilityMap.errors.InvalidSampleRatio'); + } + + if (fields.dataSourceId !== undefined && fields.dataSourceId.trim().length === 0) { + errors.push('transit:networkDesign.simulationMethods.accessibilityMap.errors.DataSourceRequired'); + } + + return { valid: errors.length === 0, errors }; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.ts` around lines 53 - 59, The validateFields method in AccessibilityMapSimulationMethod currently always returns valid: true; update AccessibilityMapSimulationMethod.validateFields to actually inspect the incoming Partial<AccessibilityMapSimulationOptions> (_fields), check required option keys, types and value constraints (e.g., required properties present, numeric fields are numbers and within allowed ranges/positive, enum/string fields match allowed values, and objects/arrays have expected shapes), accumulate descriptive error messages into errors[], and return { valid: errors.length === 0, errors } so invalid configs are rejected when coming from non-UI paths.
♻️ Duplicate comments (7)
packages/transition-common/src/utils/userDefinedConfig/index.ts (1)
197-219:⚠️ Potential issue | 🟠 MajorTighten the numeric base checks before custom/range validation.
integerandsecondscurrently accept any number before the custom validator runs, so1.5can pass if that validator returnstrue. The defaultpercentagepath has the inverse bug: non-numbers becomeNaN, and Line 217 then accepts them because both range comparisons are false.🔢 Suggested fix
if ('validate' in fieldDef && fieldDef.validate) { let isValid: TranslatableMessage | boolean = true; - if ( - fieldDef.type === 'integer' || - fieldDef.type === 'number' || - fieldDef.type === 'seconds' || - fieldDef.type === 'percentage' - ) { - isValid = typeof value === 'number' ? fieldDef.validate(value) : false; + if (fieldDef.type === 'integer' || fieldDef.type === 'seconds') { + isValid = Number.isInteger(value) ? fieldDef.validate(value) : false; + } else if (fieldDef.type === 'number' || fieldDef.type === 'percentage') { + isValid = Number.isFinite(value) ? fieldDef.validate(value) : false; } else if (fieldDef.type === 'string') { isValid = typeof value === 'string' ? fieldDef.validate(value) : false; } if (isValid !== true) { valid = false; @@ } else if (fieldDef.type === 'percentage') { // By default, if there is no validate function, percentage type must be between 0 and 1 const numberValue = typeof value === 'number' ? value : NaN; - if (numberValue < 0 || numberValue > 1) { + if (!Number.isFinite(numberValue) || numberValue < 0 || numberValue > 1) { setFieldInvalid(fieldDef, 'Invalid'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/utils/userDefinedConfig/index.ts` around lines 197 - 219, The numeric-type validation currently allows non-integers and NaN to slip through; tighten checks in the block that handles fieldDef.validate and the fallback percentage branch: for field types 'integer' and 'seconds' require typeof value === 'number' && Number.isInteger(value) before calling fieldDef.validate (for 'number' allow typeof value === 'number' && !Number.isNaN(value)), and keep the existing isValid flow; in the else-if for fieldDef.type === 'percentage' ensure value is a real number (typeof value === 'number' && !Number.isNaN(numberValue)) before comparing range 0..1 and call setFieldInvalid(fieldDef, 'Invalid') when the check fails or the value is out of range. Reference: fieldDef, fieldDef.validate, isValid, numberValue, setFieldInvalid.packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts (1)
70-74:⚠️ Potential issue | 🟠 MajorBuild a fresh
validateMockper case.Line 73 resets the same mock that was already mutated by the
mockReturnValueOnce(...)calls embedded in thetest.eachtable. The invalid/custom-error rows therefore lose their per-row behavior before they run, so this suite is not exercising the branches it claims to cover.🧪 Suggested fix
- const validateMock = jest.fn(); + let validateMock: jest.Mock; beforeEach(() => { - validateMock.mockReset().mockReturnValue(true); + validateMock = jest.fn().mockReturnValue(true); }); - schema: { + setupValidateMock: () => validateMock.mockReturnValueOnce(false), + schemaFactory: () => ({ ...mockSchema, getFields: () => ({ - integerField: { type: 'integer' as const, i18nName: 'IntegerField', validate: validateMock.mockReturnValueOnce(false) } + integerField: { type: 'integer' as const, i18nName: 'IntegerField', validate: validateMock } }) - }, + }), ... - }])('field specific validations: $description', ({ initialOptions, expected, schema, expectedValidateValue }) => { + }])('field specific validations: $description', ({ initialOptions, expected, schemaFactory, setupValidateMock, expectedValidateValue }) => { + setupValidateMock?.(); + const schema = schemaFactory(); const result = validateFieldsWithSchema(initialOptions, schema);Also applies to: 183-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts` around lines 70 - 74, The tests reset a single shared validateMock (const validateMock) in beforeEach which wipes out the mockReturnValueOnce setups defined in the test.each table; create a fresh jest.fn() per test case instead: change validateMock to be recreated inside each test (or make it a let and assign a new jest.fn() in beforeEach before any per-row mockReturnValueOnce calls run), and ensure any per-row mockReturnValueOnce calls are applied to that newly created validateMock inside the test body (or via test parameters) so each row keeps its intended behavior; refer to validateMock and the test.each blocks when making the change.packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts (1)
101-106:⚠️ Potential issue | 🟠 Major
numberOfGenerationsshould be integer-constrained.This value is a discrete count; allowing decimals can produce surprising generation-loop behavior.
Suggested fix
numberOfGenerations: { i18nName: 'transit:networkDesign.evolutionaryAlgorithm.NumberOfGenerations', i18nHelp: 'transit:networkDesign.evolutionaryAlgorithm.help.NumberOfGenerations', - type: 'number' as const, - validate: (value: number) => value > 0, + type: 'integer' as const, + validate: (value: number) => Number.isInteger(value) && value > 0, default: 100 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts` around lines 101 - 106, The numberOfGenerations option currently allows non-integer numbers; change its validation so it only accepts positive integers (e.g., replace validate: (value: number) => value > 0 with a check that enforces integer-ness such as Number.isInteger(value) && value > 0), ensuring the field in EvolutionaryAlgorithm.ts named numberOfGenerations keeps the same default (100) but rejects decimals.packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts (2)
139-143:⚠️ Potential issue | 🔴 CriticalThe rename is still incomplete in this loop (
getOptionsremains).This path still calls
descriptor.getOptions(), which breaks theUserDefinedConfigSchemamigration and can fail at runtime/type-check time.Suggested fix
- const methodOptions = descriptor.getOptions(); + const methodOptions = descriptor.getFields();#!/bin/bash # Verify stale API usage remains in this file rg -nP '\bgetOptions\s*\(' packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts rg -nP '\bgetFields\s*\(' packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts` around lines 139 - 143, The loop still calls descriptor.getOptions(), which is the stale API and breaks the UserDefinedConfigSchema migration; locate the code using methods[methodId].getDescriptor() and replace descriptor.getOptions() with the new API (e.g., descriptor.getFields()) and update any downstream variable (methodOptions) usage to match the new return shape so the rest of the loop consumes the new fields format instead of the old options format.
39-40:⚠️ Potential issue | 🟠 MajorDon’t call
choiceswith{}here.Using an empty object drops current form state, so dependent select options can be incorrect.
Suggested fix
-const getPromptForDescriptor = async ( +const getPromptForDescriptor = async ( descriptor: UserDefinedConfigFieldDescriptor, - options: { t: TFunction; name: string; default?: unknown } + options: { t: TFunction; name: string; default?: unknown; currentValues: Record<string, unknown> } ): Promise<unknown> => { @@ - const choices = descriptor.choices({}); + const choices = descriptor.choices(options.currentValues);- const value = await getPromptForDescriptor(algoOptions[optionKey], { + const value = await getPromptForDescriptor(algoOptions[optionKey], { t: options.t, name: optionKey, - default: simulation.attributes.data.algorithmConfiguration?.config[optionKey] + default: simulation.attributes.data.algorithmConfiguration?.config[optionKey], + currentValues: { ...(simulation.attributes.data.algorithmConfiguration?.config || {}), ...answers } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts` around lines 39 - 40, Do not invoke descriptor.choices with an empty object (which drops current form state); instead either assign the function reference (e.g., const choicesFn = descriptor.choices) and call it later with the real form state, or call descriptor.choices(currentFormState) using the actual form-state variable (e.g., currentValues, formState, or the value passed into your surrounding function) so the select options reflect current dependent values; update the usage of choices accordingly.packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx (2)
50-51:⚠️ Potential issue | 🟠 MajorRestore generic typing end-to-end instead of erasing to
any.
anyis still used in the editor callback path and exported component signature, which removes schema-driven type guarantees for callers.Suggested typed refactor
-type FieldComponentProps = { +type FieldComponentProps<T extends Record<string, unknown> = Record<string, unknown>> = { fieldDescriptor: UserDefinedConfigFieldDescriptor; value?: unknown; fieldKey: string; completeObject: Record<string, unknown>; disabled?: boolean; - onValueChange: (path: string, newValue: { value: any; valid?: boolean }) => void; + onValueChange: (path: string, newValue: { value: unknown; valid?: boolean }) => void; }; @@ -export const UserDefinedConfigEditComponent: React.FunctionComponent<UserDefinedConfigEditComponentProps<any>> = ( - props: UserDefinedConfigEditComponentProps<any> -) => { +export const UserDefinedConfigEditComponent = <T extends Record<string, unknown>>( + props: UserDefinedConfigEditComponentProps<T> +) => { @@ - const onValueChange = (path: string, newValue: { value: any; valid?: boolean }): void => { + const onValueChange = (path: string, newValue: { value: unknown; valid?: boolean }): void => {Also applies to: 260-265, 281-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx` around lines 50 - 51, The onValueChange callback and exported component signatures currently use `any`, losing type-safety; change these to a generic type parameter (e.g., <T> or <ValueT>) and propagate that generic through the relevant types and props: update the onValueChange signature from (path: string, newValue: { value: any; valid?: boolean }) => void to use the generic ({ value: T; valid?: boolean }), make the component/interface (OptionsDescriptorWidgets) generic (OptionsDescriptorWidgets<T>) and apply the same generic to any WidgetProps, internal handlers, and child component props referenced in the file (including the places flagged around lines 260-265 and 281-283) so callers can supply the concrete type without erasing to any. Ensure all usages inside the file and exported types import/forward the generic parameter rather than using any.
297-303:⚠️ Potential issue | 🟠 MajorCombine child-widget validity with schema validity before publishing updates.
Line 302 currently ignores
newValue.valid, so widget-level invalid states can be reported as valid.Suggested fix
- const { valid, errors } = validateFieldsWithSchema(updatedParameters, props.configSchema); - - props.onUpdate(updatedParameters, valid); - setErrors(errors); + const { valid, errors } = validateFieldsWithSchema(updatedParameters, props.configSchema); + const isWidgetValid = newValue.valid ?? true; + props.onUpdate(updatedParameters, valid && isWidgetValid); + setErrors(errors);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx` around lines 297 - 303, onValueChange currently ignores the child widget's newValue.valid when calling props.onUpdate, allowing an invalid child widget to be treated as valid; update the handler to compute an overall validity by combining the child's validity (treat undefined as true, treat explicit false as invalid via newValue.valid) with the schema validation result from validateFieldsWithSchema(props.configSchema) and pass that combined boolean to props.onUpdate (continue to update updatedParameters and call setErrors(errors) as before). Ensure you reference onValueChange, newValue.valid, validateFieldsWithSchema, props.onUpdate, setErrors, props.value and props.configSchema when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@locales/en/transit.json`:
- Around line 851-864: Update the GA label/messages for pluralization: change
the value of the "NumberOfRandoms" key from "Number of random per generation" to
"Number of randoms per generation" and also update the "NumberOfRandomsInvalid"
value to "Number of randoms per generation must be greater or equal to 0" so
both the label and its validation message use the correct plural form; locate
and edit the "NumberOfRandoms" and "NumberOfRandomsInvalid" entries in the JSON.
In `@locales/fr/transit.json`:
- Line 890: Update the French string value for the key
"GenerationsToKeepGreaterThanGenerations" in locales/fr/transit.json to correct
grammar by changing the text to "Le nombre de générations à conserver est
supérieur au nombre de générations". Ensure you only modify the string value for
the "GenerationsToKeepGreaterThanGenerations" key.
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts`:
- Around line 50-53: The test calls options.dataSourceId.choices() with no
arguments but the method expects an object; update the test to call
options.dataSourceId.choices({}) so it matches the choices(object) signature
(locate the call via descriptor.getFields() and options.dataSourceId.choices).
In `@packages/transition-common/src/services/simulation/Simulation.ts`:
- Around line 28-29: The current Simulation.validate implementation inspects
only explicit field-level validate callbacks and then calls the descriptor
validator, which bypasses required flags, select/multiselect choice checks,
nested schema validation, and primitive checks; replace that manual logic with
the existing validateFieldsWithSchema helper: call
validateFieldsWithSchema(EvolutionaryAlgorithmOptions or the algorithm config
schema and the config object) and use its returned error map to determine
whether to add the "main:InvalidFormFields" banner and to populate field-level
errors, ensuring required, choices, nested schemas, and primitives are validated
consistently; update any references in Simulation.validate (and the surrounding
block that previously inspected descriptor.validate) to use the helper result
instead of duplicating validation logic.
In `@packages/transition-common/src/utils/userDefinedConfig/index.ts`:
- Around line 134-140: The loop over fieldDefinitions currently always assigns a
nested object into fields for fieldDef.type === 'nested' even when the caller
didn't provide the parent and getDefaultFieldsFromSchema returns an empty {}; to
avoid turning optional nested fields into present ones (which causes
validateFieldsWithSchema to recurse and fail), only set (fields as any)[key] =
nestedDefaults when either an existingNestedValue was provided or nestedDefaults
is non-empty; otherwise skip assigning the key. Locate this logic in the block
handling nestedDescriptor/getDefaultFieldsFromSchema and add a guard that checks
existingNestedValue !== undefined || Object.keys(nestedDefaults).length > 0
before writing to fields.
In
`@packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx`:
- Line 53: TransitNetworkDesignAlgorithmComponent must avoid passing undefined
into UserDefinedConfigEditComponent because that component reads fields from
props.value; update the prop so it always receives an object (e.g. change
value={props.algorithmConfig?.config} to value={props.algorithmConfig?.config ??
{}}) or otherwise ensure props.algorithmConfig is initialized before rendering;
reference the component name TransitNetworkDesignAlgorithmComponent, the prop
algorithmConfig and its config field, and the child
UserDefinedConfigEditComponent (which accesses props.value) when making the
change.
- Line 52: The React key currently uses _toInteger(props.algorithmConfig?.type)
which coerces the algorithm type to NaN and produces unstable keys; update the
key in TransitNetworkDesignAlgorithmComponent (the element using
key={`algorithmConfigOptions${_toInteger(props.algorithmConfig?.type)}`}) to use
the raw algorithm type instead (e.g., use props.algorithmConfig?.type or
String(props.algorithmConfig?.type)) and remove the _toInteger call so the key
changes stably when algorithmConfig.type changes.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx`:
- Around line 110-141: The tests "renders all widget types with defaults" and
"renders with provided values" rely only on snapshots; add explicit behavior
assertions to make regressions easier to debug: in each test interact with the
rendered UserDefinedConfigEditComponent (e.g., find inputs for stringField,
booleanField, selectField, multiselectField, and nestedField using mockSchema
labels or test ids), simulate user changes and assert that the onUpdate mock is
called with the expected updated value shape, and add assertions that
validity/required state propagates (e.g., trigger invalid input for a required
field and assert the component reports invalid state or does not call onUpdate).
Target functions/identifiers: UserDefinedConfigEditComponent, mockSchema,
onUpdate, and the field keys like
stringField/booleanField/selectField/multiselectField/nestedField when adding
these assertions.
---
Outside diff comments:
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.ts`:
- Around line 53-59: The validateFields method in
AccessibilityMapSimulationMethod currently always returns valid: true; update
AccessibilityMapSimulationMethod.validateFields to actually inspect the incoming
Partial<AccessibilityMapSimulationOptions> (_fields), check required option
keys, types and value constraints (e.g., required properties present, numeric
fields are numbers and within allowed ranges/positive, enum/string fields match
allowed values, and objects/arrays have expected shapes), accumulate descriptive
error messages into errors[], and return { valid: errors.length === 0, errors }
so invalid configs are rejected when coming from non-UI paths.
---
Duplicate comments:
In `@packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts`:
- Around line 139-143: The loop still calls descriptor.getOptions(), which is
the stale API and breaks the UserDefinedConfigSchema migration; locate the code
using methods[methodId].getDescriptor() and replace descriptor.getOptions() with
the new API (e.g., descriptor.getFields()) and update any downstream variable
(methodOptions) usage to match the new return shape so the rest of the loop
consumes the new fields format instead of the old options format.
- Around line 39-40: Do not invoke descriptor.choices with an empty object
(which drops current form state); instead either assign the function reference
(e.g., const choicesFn = descriptor.choices) and call it later with the real
form state, or call descriptor.choices(currentFormState) using the actual
form-state variable (e.g., currentValues, formState, or the value passed into
your surrounding function) so the select options reflect current dependent
values; update the usage of choices accordingly.
In
`@packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts`:
- Around line 101-106: The numberOfGenerations option currently allows
non-integer numbers; change its validation so it only accepts positive integers
(e.g., replace validate: (value: number) => value > 0 with a check that enforces
integer-ness such as Number.isInteger(value) && value > 0), ensuring the field
in EvolutionaryAlgorithm.ts named numberOfGenerations keeps the same default
(100) but rejects decimals.
In
`@packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts`:
- Around line 70-74: The tests reset a single shared validateMock (const
validateMock) in beforeEach which wipes out the mockReturnValueOnce setups
defined in the test.each table; create a fresh jest.fn() per test case instead:
change validateMock to be recreated inside each test (or make it a let and
assign a new jest.fn() in beforeEach before any per-row mockReturnValueOnce
calls run), and ensure any per-row mockReturnValueOnce calls are applied to that
newly created validateMock inside the test body (or via test parameters) so each
row keeps its intended behavior; refer to validateMock and the test.each blocks
when making the change.
In `@packages/transition-common/src/utils/userDefinedConfig/index.ts`:
- Around line 197-219: The numeric-type validation currently allows non-integers
and NaN to slip through; tighten checks in the block that handles
fieldDef.validate and the fallback percentage branch: for field types 'integer'
and 'seconds' require typeof value === 'number' && Number.isInteger(value)
before calling fieldDef.validate (for 'number' allow typeof value === 'number'
&& !Number.isNaN(value)), and keep the existing isValid flow; in the else-if for
fieldDef.type === 'percentage' ensure value is a real number (typeof value ===
'number' && !Number.isNaN(numberValue)) before comparing range 0..1 and call
setFieldInvalid(fieldDef, 'Invalid') when the check fails or the value is out of
range. Reference: fieldDef, fieldDef.validate, isValid, numberValue,
setFieldInvalid.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx`:
- Around line 50-51: The onValueChange callback and exported component
signatures currently use `any`, losing type-safety; change these to a generic
type parameter (e.g., <T> or <ValueT>) and propagate that generic through the
relevant types and props: update the onValueChange signature from (path: string,
newValue: { value: any; valid?: boolean }) => void to use the generic ({ value:
T; valid?: boolean }), make the component/interface (OptionsDescriptorWidgets)
generic (OptionsDescriptorWidgets<T>) and apply the same generic to any
WidgetProps, internal handlers, and child component props referenced in the file
(including the places flagged around lines 260-265 and 281-283) so callers can
supply the concrete type without erasing to any. Ensure all usages inside the
file and exported types import/forward the generic parameter rather than using
any.
- Around line 297-303: onValueChange currently ignores the child widget's
newValue.valid when calling props.onUpdate, allowing an invalid child widget to
be treated as valid; update the handler to compute an overall validity by
combining the child's validity (treat undefined as true, treat explicit false as
invalid via newValue.valid) with the schema validation result from
validateFieldsWithSchema(props.configSchema) and pass that combined boolean to
props.onUpdate (continue to update updatedParameters and call setErrors(errors)
as before). Ensure you reference onValueChange, newValue.valid,
validateFieldsWithSchema, props.onUpdate, setErrors, props.value and
props.configSchema when making the change.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1c286c8d-b56f-4bff-8d11-2e5e0cdc3a8f
⛔ Files ignored due to path filters (1)
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/__snapshots__/OptionsDescriptorWidgets.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
locales/en/transit.jsonlocales/fr/transit.jsonpackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/TransitNetworkDesignAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-frontend/src/components/forms/simulation/SimulationEdit.tsxpackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
💤 Files with no reviewable changes (2)
- packages/transition-common/src/services/networkDesign/transit/TransitNetworkDesignAlgorithm.ts
- packages/transition-frontend/src/components/forms/simulation/SimulationEdit.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-sequential (24.x)
- GitHub Check: build-and-test (24.x)
- GitHub Check: code-lint
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: pr-build-check
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxpackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxpackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts
packages/transition-frontend/src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
React components should use Redux for state management, i18next for translations, and SCSS for styling
Files:
packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
**/*.json
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Translation files in
locales/directory should organize strings by namespace (auth.json, transit.json, main.json, etc.) with support for both English (en/) and French (fr/) languages
Files:
locales/en/transit.jsonlocales/fr/transit.json
🧠 Learnings (26)
📓 Common learnings
Learnt from: tahini
Repo: chairemobilite/transition PR: 1554
File: packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts:23-36
Timestamp: 2025-11-04T19:35:10.045Z
Learning: In the OdTripSimulationMethod (packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts), the team plans to move away from the datasource path and use batch calculations instead. The current non-functional dataSourceId selector will be reworked as part of this architectural change.
📚 Learning: 2025-11-04T19:35:10.045Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1554
File: packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts:23-36
Timestamp: 2025-11-04T19:35:10.045Z
Learning: In the OdTripSimulationMethod (packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts), the team plans to move away from the datasource path and use batch calculations instead. The current non-functional dataSourceId selector will be reworked as part of this architectural change.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.ts
📚 Learning: 2026-03-02T21:57:21.005Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1830
File: packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts:114-122
Timestamp: 2026-03-02T21:57:21.005Z
Learning: In test files for database query modules, ensure mocked functions match the async/sync nature of the production implementations. If a function returns a Promise in production (async), use an async mock (e.g., jest.fn().mockImplementation(async (...args) => { ... })). If a function is synchronous (e.g., returns a stream or uses direct values), use a synchronous mock. This helps prevent false positives/negatives due to mismatched mock behavior.
Applied to files:
packages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.ts
📚 Learning: 2025-09-25T17:08:56.828Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:105-107
Timestamp: 2025-09-25T17:08:56.828Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred changing the wrapBatchAccessMap input file validation from `hasInputFile()` to `getFilePath('input')` (actual file existence check), indicating it will be addressed in an upcoming file access refactor.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx
📚 Learning: 2026-02-06T19:06:19.438Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1767
File: packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx:177-178
Timestamp: 2026-02-06T19:06:19.438Z
Learning: In the transition accessibility map API responses (packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx), the populationData property is always present as an object with the structure { population: number | null; dataSourceAreaRatio: number }. Even when population calculation is disabled or the data source is missing, populationData is returned with population set to null rather than populationData being undefined. Therefore, accessing properties1.populationData.population without optional chaining is safe.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.ts
📚 Learning: 2025-11-25T21:28:28.491Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1604
File: packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts:74-79
Timestamp: 2025-11-25T21:28:28.491Z
Learning: In packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts, the maxBirdDistanceMeters, maxNetworkDistanceMeters, and maxTravelTimeSeconds parameters should not accept 0 or negative values, as they are semantically invalid for distance and time thresholds. Explicit validation should be preferred over relying on falsy coercion.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/utils/userDefinedConfig/index.tslocales/fr/transit.jsonpackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts
📚 Learning: 2026-02-24T00:36:44.483Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/__tests__/propertyRegistry.db.test.ts:253-258
Timestamp: 2026-02-24T00:36:44.483Z
Learning: In test files, it is acceptable to use 'any' types for error handling and test scenarios. Reserve strict TypeScript typing for production code; in tests, prioritize pragmatic, readable test cases, and use 'any' only where it clearly improves test clarity or reduces boilerplate.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsxpackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: When modifying transit data models that use Cap'n Proto caching, update the Cap'n Proto schema, regenerate TypeScript types, and regenerate the cache
Applied to files:
packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/**/*.tsx : React components should use Redux for state management, i18next for translations, and SCSS for styling
Applied to files:
packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/routers/*.tsx : Application routing should be defined in `TransitionRouter.tsx` using React Router with feature-based route organization
Applied to files:
packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx
📚 Learning: 2026-01-21T14:59:52.530Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1592
File: packages/transition-frontend/src/components/map/CircleSpinnerExtension.tsx:37-44
Timestamp: 2026-01-21T14:59:52.530Z
Learning: In TSX files under packages/transition-frontend/src that currently rely on deck.gl's this.context.animationProps.time for animation timing, prefer window.performance.now() (or performance.now()) to measure elapsed time. Replace time-based calculations tied to deck.gl with direct high-resolution timestamps to improve accuracy and decouple from the rendering library. Ensure animations use delta = now - lastTime, and update lastTime accordingly for smooth, frame-rate-independent timing.
Applied to files:
packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.test.{ts,tsx} : Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Applied to files:
packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts
📚 Learning: 2025-09-25T17:09:26.332Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:85-91
Timestamp: 2025-09-25T17:09:26.332Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred improving input file validation to check actual file existence on disk (using getFilePath('input') instead of just hasInputFile() metadata check), indicating it should be addressed when file access is refactored soon.
Applied to files:
packages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx
📚 Learning: 2025-11-18T19:37:07.564Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1588
File: packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts:11-23
Timestamp: 2025-11-18T19:37:07.564Z
Learning: In the chairemobilite/transition repository TypeScript code, greenscientist prefers that reviews highlight when interfaces could be replaced with type aliases for stylistic consistency, especially when the structure is just a data shape.
Applied to files:
packages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts
📚 Learning: 2025-09-25T16:40:55.314Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/executableJob/ExecutableJob.ts:152-158
Timestamp: 2025-09-25T16:40:55.314Z
Learning: In packages/transition-backend/src/services/executableJob/ExecutableJob.ts, greenscientist deferred improving the hasFile() method to check actual file existence on disk rather than just metadata, indicating it should be addressed in a later PR.
Applied to files:
packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts
📚 Learning: 2026-01-28T18:01:07.845Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1736
File: packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts:54-63
Timestamp: 2026-01-28T18:01:07.845Z
Learning: In packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts, the headway thresholds in getHeadwayThresholdSeconds() are intentionally hard-coded constants rather than configurable parameters. These values should be based on international consensus to allow consistent comparison of transit service quality across different cities. The TODO comments indicate that these values need more research and community discussion before being finalized.
Applied to files:
packages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts
📚 Learning: 2026-01-31T22:01:53.681Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1736
File: packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts:0-0
Timestamp: 2026-01-31T22:01:53.681Z
Learning: In packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts, the inVehicleTravelTimeWeightForLoadFactor can intentionally be < 1, which allows the weighted travel time to be less than actual travel time. This models scenarios where passengers perceive travel time as shorter, such as when they can do productive work on a train. The formula `weight * (loadFactor + 1)` is correct and should not be changed to `1 + weight * loadFactor`.
Applied to files:
packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts
📚 Learning: 2026-01-28T18:00:30.001Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1736
File: packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts:34-47
Timestamp: 2026-01-28T18:00:30.001Z
Learning: In packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts, the getHeadways method intentionally returns undefined if any leg is missing headway data (rather than handling missing headways per-leg) because applying headway penalties only to some legs could bias the calibration. This all-or-nothing approach ensures consistent treatment of incomplete data.
Applied to files:
packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts
📚 Learning: 2026-02-06T18:45:02.744Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1767
File: packages/transition-backend/src/services/accessibilityMap/TransitAccessibilityMapCalculator.ts:106-107
Timestamp: 2026-02-06T18:45:02.744Z
Learning: In the transition codebase, the pattern `dataSourceId: attributes.dataSourceId || undefined` is intentionally used to convert empty strings to undefined, ensuring optional string fields are normalized (empty string becomes undefined rather than remaining as an empty string).
Applied to files:
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.ts : Use TypeScript strictly and avoid `any` types - maintain type safety throughout the application
Applied to files:
packages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx
📚 Learning: 2026-02-24T00:36:50.541Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/__tests__/propertyRegistry.db.test.ts:253-258
Timestamp: 2026-02-24T00:36:50.541Z
Learning: In test files (*.test.ts, *.spec.ts), `any` types are acceptable for error handling and test scenarios, even though strict TypeScript typing is preferred in production code.
Applied to files:
packages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.{ts,tsx} : Use i18next and the `t()` function for all user-facing strings to support internationalization
Applied to files:
packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.json : Translation files in `locales/` directory should organize strings by namespace (auth.json, transit.json, main.json, etc.) with support for both English (en/) and French (fr/) languages
Applied to files:
locales/fr/transit.json
📚 Learning: 2025-12-17T22:46:52.336Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1666
File: packages/chaire-lib-frontend/package.json:60-60
Timestamp: 2025-12-17T22:46:52.336Z
Learning: In packages/chaire-lib-frontend/package.json, the `testing-library/dom` devDependency is required for tests to pass even though it's not explicitly imported. It was previously provided as a transitive dependency through `react-select-event`, but after removing `react-select-event`, it must be explicitly declared.
Applied to files:
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
📚 Learning: 2026-02-24T00:40:13.916Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/propertyRegistry.db.queries.ts:35-36
Timestamp: 2026-02-24T00:40:13.916Z
Learning: The project is migrating from UUID primary keys to integer primary keys, one class at a time. Integer IDs (id: number) are acceptable and preferred for entities like PropertyRegistry in packages/chaire-lib-backend/src/models/db/*.ts and related files.
Applied to files:
packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts
🔇 Additional comments (8)
packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.ts (1)
29-47: Nice schema migration in the stub.The
UserDefinedConfigSchemaadoption andgetFields/validateFieldsrename look consistent here.packages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts (1)
23-305: Good update to the new descriptor API in tests.The switch to
getFields/validateFieldsand the new i18n namespace is coherent across this suite.packages/transition-backend/src/services/simulation/methods/SimulationMethod.ts (1)
9-9: Good contract update.The
SimulationMethodFactorydescriptor type migration toUserDefinedConfigSchema<TOptionsType>is clear and consistent.Also applies to: 39-39
packages/transition-common/src/services/simulation/__tests__/Simulation.test.ts (1)
13-13: Looks aligned with the schema-based defaults flow.The test setup now uses
getDefaultFieldsFromSchemaand the newgetFieldsAPI consistently.Also applies to: 169-172
locales/en/transit.json (1)
845-900: Good namespace migration for GA/network design translations.The new
networkDesign.evolutionaryAlgorithmandnetworkDesign.simulationMethods.accessibilityMapblocks align with the new schema-based i18n keys used by the algorithm descriptors.locales/fr/transit.json (1)
845-900: FR translation structure matches the new networkDesign namespace.The newly added French
networkDesignblock is consistent with the English migration and the schema-driven key layout.packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx (1)
289-295: Nice fix: initial validation errors are now persisted.Setting
setErrors(errors)during initialization ensures invalid defaults show immediate feedback.packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx (1)
25-107: Good breadth for widget rendering coverage.The mocked schema exercises string/number/time/select/multiselect/csv/nested branches, which is solid baseline coverage for this new editor.
Also applies to: 109-142
Comment on lines
+110
to
+141
| test('renders all widget types with defaults', () => { | ||
| const onUpdate = jest.fn(); | ||
| const { container } = render( | ||
| <UserDefinedConfigEditComponent configSchema={mockSchema} value={{} as TestConfig} onUpdate={onUpdate} /> | ||
| ); | ||
| expect(container).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| test('renders with provided values', () => { | ||
| const onUpdate = jest.fn(); | ||
| const { container } = render( | ||
| <UserDefinedConfigEditComponent | ||
| configSchema={mockSchema} | ||
| value={{ | ||
| stringField: 'custom', | ||
| booleanField: false, | ||
| integerField: 10, | ||
| numberField: 2.71, | ||
| percentageField: 0.75, | ||
| secondsField: 60, | ||
| minutesField: 180, | ||
| hoursField: 3600, | ||
| selectField: 'b', | ||
| multiselectField: ['a', 'b'], | ||
| csvField: undefined, | ||
| nestedField: { nestedString: 'inner', nestedNumber: 9 } | ||
| }} | ||
| onUpdate={onUpdate} | ||
| /> | ||
| ); | ||
| expect(container).toMatchSnapshot(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding behavior assertions alongside snapshots.
A few explicit checks (for example onUpdate calls and validity propagation) would make regressions easier to debug than snapshot diffs alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx`
around lines 110 - 141, The tests "renders all widget types with defaults" and
"renders with provided values" rely only on snapshots; add explicit behavior
assertions to make regressions easier to debug: in each test interact with the
rendered UserDefinedConfigEditComponent (e.g., find inputs for stringField,
booleanField, selectField, multiselectField, and nestedField using mockSchema
labels or test ids), simulate user changes and assert that the onUpdate mock is
called with the expected updated value shape, and add assertions that
validity/required state propagates (e.g., trigger invalid input for a required
field and assert the component reports invalid state or does not call onUpdate).
Target functions/identifiers: UserDefinedConfigEditComponent, mockSchema,
onUpdate, and the field keys like
stringField/booleanField/selectField/multiselectField/nestedField when adding
these assertions.
greenscientist
left a comment
greenscientist
left a comment
Collaborator
There was a problem hiding this comment.
At an high level, I fail to understand the scope of UserDefinedConfigFieldDescriptor.
We need either something more in the commit messages or a longer description in the class doc.
I'll wait to review the code until I have more clarity on this.
tahini
force-pushed
the
userDefinedOptionDescriptor
branch
from
ab46e30 to
08ae728
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts (1)
142-142:⚠️ Potential issue | 🔴 CriticalIncomplete migration:
getOptions()should begetFields().Line 142 still calls
descriptor.getOptions(), butgetDescriptor()now returnsUserDefinedConfigSchemawhich only hasgetFields(). This will fail to compile/run.Suggested fix
- const methodOptions = descriptor.getOptions(); + const methodOptions = descriptor.getFields();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts` at line 142, The code still calls descriptor.getOptions() which no longer exists because getDescriptor() now returns a UserDefinedConfigSchema; replace the call with descriptor.getFields() and update any downstream usage of methodOptions (the variable assigned from getOptions()) to work with the fields shape returned by getFields() (rename methodOptions if helpful and adapt access patterns where used in AlgorithmEdition and any referenced methods).packages/transition-common/src/services/simulation/Simulation.ts (1)
79-87:⚠️ Potential issue | 🔴 CriticalInitialize
algoConfig.configbefore indexing it.
validate()already handlesconfigbeing absent, but_prepareAttributes()dereferencesalgoConfig.config[key]unconditionally here. A simulation withalgorithmConfiguration.typeset and noconfigwill throw before defaults can be applied.Suggested fix
if (algoConfig && algoConfig.type) { const descriptor = getAlgorithmDescriptor(algoConfig.type); if (descriptor !== undefined) { + algoConfig.config ??= {}; const options = descriptor.getFields(); Object.keys(options).forEach((key) => { if (algoConfig.config[key] === undefined) { algoConfig.config[key] = undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/simulation/Simulation.ts` around lines 79 - 87, In _prepareAttributes(), before accessing algoConfig.config, ensure algoConfig.config is initialized when algorithmConfiguration.type is present (e.g., if (!algoConfig.config) algoConfig.config = {}), then proceed to call getAlgorithmDescriptor(algoConfig.type) and iterate descriptor.getFields(); this prevents dereferencing undefined when algorithmConfiguration.type exists but config is missing while still allowing validate() to handle absent configs elsewhere.packages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.ts (1)
51-82:⚠️ Potential issue | 🟠 MajorRequire or default the fitness selectors.
With the current schema,
{ sampleRatio: 1 }validates successfully:odTripFitnessFunctionandfitnessFunctionare neitherrequirednor defaulted, andvalidateFieldsalways returnsvalid. That lets the editor emit a config that does not satisfyOdTripSimulationOptions.Possible fix
odTripFitnessFunction: { i18nName: 'transit:simulation:fitness:odTripFitnessFunction', type: 'select' as const, + required: true, choices: () => [ { label: 'transit:simulation:fitness:travelTimeCost', @@ fitnessFunction: { i18nName: 'transit:simulation:fitness:fitnessFunction', type: 'select' as const, + required: true, choices: () => [ { label: 'transit:simulation:fitness:hourlyUserPlusOperatingCosts',Also applies to: 85-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.ts` around lines 51 - 82, The schema leaves odTripFitnessFunction and fitnessFunction optional so validateFields allows configs like { sampleRatio: 1 } that do not satisfy OdTripSimulationOptions; update the schema entries for odTripFitnessFunction and fitnessFunction to either be required or provide sensible defaults so the editor always emits a valid config (e.g., mark fields as required or add default values), and ensure validateFields uses those defaults when validating; locate the entries named odTripFitnessFunction and fitnessFunction in OdTripSimulationMethod and adjust their schema definitions accordingly (also apply the same change for the duplicate block at lines 85-91).
♻️ Duplicate comments (10)
packages/transition-common/src/services/simulation/Simulation.ts (1)
130-155:⚠️ Potential issue | 🟠 MajorUse schema validation end-to-end here.
This block only runs explicit field
validatecallbacks and the descriptor's cross-field validator. It skipsrequired, primitive type checks, select/multiselect membership, and nested schemas, so invalid algorithm configs can still passSimulation.validate(). Please callvalidateFieldsWithSchema(algoConfig.config, algorithmDescriptor)here and derive the generic banner from that result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/simulation/Simulation.ts` around lines 130 - 155, The current manual checks using algorithmDescriptor.getFields and per-field validate plus validateFields miss schema rules (required, types, enums, nested); replace that validation block to call validateFieldsWithSchema(algoConfig.config, algorithmDescriptor), use its returned valid/errors to set this._isValid and to push generic banner error into this._errors (instead of the ad-hoc erroneousFields logic), and still merge any descriptor-level errors (algorithmDescriptor.validateFields) if needed; keep getDefaultFieldsFromSchema(...) assignment for missing config but route all validation through validateFieldsWithSchema and derive the top-level banner/error array from its result.packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts (1)
70-74:⚠️ Potential issue | 🟠 MajorBuild a fresh validator mock per case.
Several rows below configure
mockReturnValueOnce(...)while thetest.eachtable is created, butbeforeEach(mockReset())clears that queue before the test runs. The invalid/custom-error cases then exercise the defaulttruepath instead of the row-specific behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts` around lines 70 - 74, The test currently reuses a single validateMock and calls validateMock.mockReset() in beforeEach which clears any mockReturnValueOnce() setups created in the test.each rows, causing row-specific cases to fall back to the default true; fix by creating a fresh validator mock per test case instead of resetting the shared one—move the mock construction into beforeEach (e.g., reassign validateMock = jest.fn() or create a new local mock in the test body) so that mockReturnValueOnce(...) configured in the test.each rows is preserved for that test; reference validateMock, beforeEach, mockReturnValueOnce, and the test.each table when making the change.packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts (1)
101-106:⚠️ Potential issue | 🟡 MinorUse an integer field for
numberOfGenerations.This is a count, but the schema still declares it as
number, so values like12.5pass field validation. Please make it anintegerlike the other generation and candidate counters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts` around lines 101 - 106, The numberOfGenerations field is declared as a generic number so non-integer values (e.g. 12.5) pass validation; change its schema to use an integer field and tighten validation: set type to 'integer' (like the other counters) and update the validate function to ensure the value is an integer and > 0 (e.g. use Number.isInteger(value) && value > 0), leaving the default at 100; modify the declaration for numberOfGenerations in EvolutionaryAlgorithm.ts accordingly.packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx (1)
110-141: 🧹 Nitpick | 🔵 TrivialAdd behavior checks beside these snapshots.
These two cases only assert snapshots, so regressions in
onUpdatepayloads and validity propagation will be hard to catch. Please drive a few widgets and assert the emitted config and invalid-state behavior directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx` around lines 110 - 141, Extend the two snapshot tests in OptionsDescriptorWidgets.test.tsx to also simulate user interactions on key widgets of UserDefinedConfigEditComponent and assert the emitted onUpdate payload and validity propagation: use the existing onUpdate mock to verify specific updates when changing stringField, booleanField, selectField, multiselectField, csvField and nestedField (e.g., type new values and toggle checkbox/select options) and assert onUpdate receives the expected partial/full config objects; additionally, simulate invalid inputs for numeric fields (integerField, numberField, percentageField, secondsField) and assert the component reports invalid state (via onUpdate payload or exposed validity flag) and that validity clears when corrected. Ensure tests reference UserDefinedConfigEditComponent and the exact field names used in the test value object so the assertions remain robust if other parts of the file change.locales/en/transit.json (1)
856-869:⚠️ Potential issue | 🟡 MinorFix the plural wording for
NumberOfRandoms.Both strings still say "random" singular. Please use "randoms" in the label and the invalid message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locales/en/transit.json` around lines 856 - 869, Update the pluralization for the "NumberOfRandoms" keys: change the label value for NumberOfRandoms to use "randoms" (e.g., "Number of randoms per generation") and update NumberOfRandomsInvalid to also use "randoms" (e.g., "Number of randoms per generation must be greater or equal to 0"); modify the values for the keys NumberOfRandoms and NumberOfRandomsInvalid accordingly.packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx (1)
51-53:⚠️ Potential issue | 🟠 MajorUse the raw algorithm type for this React key.
algorithmConfig.typeis the schema identity here. Coercing it with_toIntegercan collapse distinct algorithm ids to the same key, so the editor does not reliably remount when the selected algorithm changes.Possible fix
- key={`algorithmConfigOptions${_toInteger(props.algorithmConfig?.type)}`} + key={`algorithmConfigOptions${props.algorithmConfig?.type ?? 'unknown'}`}#!/bin/bash set -euo pipefail echo "=== Current key derivation ===" sed -n '50,54p' packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx echo echo "=== Algorithm identifier definitions/usages ===" rg -n -C2 --type=ts 'AlgorithmConfiguration|getAllAlgorithmTypes|getAlgorithmDescriptor|evolutionaryAlgorithm' \ packages/transition-common/src/services/networkDesign/transit/algorithm echo echo "=== _toInteger implementation/re-export ===" fd -HI 'LodashExtensions.*' . -x rg -n '_toInteger|toInteger' {}Expected result: the algorithm ids are string-like identifiers, while
_toIntegeris numeric coercion, so the key should use the raw type instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx` around lines 51 - 53, The React key for UserDefinedConfigEditComponent is currently derived using _toInteger(props.algorithmConfig?.type), which can collapse distinct algorithm identifiers; change the key to use the raw algorithm type value (e.g., props.algorithmConfig?.type or a stable stringified form) so the component remounts correctly when the selected algorithm changes—update the key expression in TransitNetworkDesignAlgorithmComponent (the UserDefinedConfigEditComponent key) to reference the raw algorithmConfig.type instead of coercing via _toInteger.packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx (2)
297-303:⚠️ Potential issue | 🟠 MajorHonor
newValue.validwhen bubbling updates.Nested widgets already report readiness. This handler discards that flag and only trusts schema validation, so incomplete CSV mappings can still be reported as valid.
Possible fix
const onValueChange = (path: string, newValue: { value: any; valid?: boolean }): void => { const updatedParameters = { ...props.value, [path]: newValue.value }; // FIXME Should we validate and update only if valid from the parameters is not `false`? const { valid, errors } = validateFieldsWithSchema(updatedParameters, props.configSchema); - - props.onUpdate(updatedParameters, valid); + props.onUpdate(updatedParameters, newValue.valid !== false && valid); setErrors(errors); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx` around lines 297 - 303, The onValueChange handler currently ignores the incoming widget readiness flag (newValue.valid) and uses only validateFieldsWithSchema result; change it so that if newValue.valid === false you force the overall validity to false when calling props.onUpdate (i.e., compute overallValid = newValue.valid === false ? false : valid), continue to update the parameter value and setErrors(errors) as before, and pass overallValid into props.onUpdate; update references in onValueChange, validateFieldsWithSchema, props.onUpdate, and setErrors accordingly.
306-313:⚠️ Potential issue | 🟠 MajorNormalize missing config objects before rendering fields.
This component indexes
props.valueduring render, but callers can still provide no config object yet. That crashes before the mount effect applies defaults.Possible fix
export const UserDefinedConfigEditComponent: React.FunctionComponent<UserDefinedConfigEditComponentProps<any>> = ( props: UserDefinedConfigEditComponentProps<any> ) => { const { t } = useTranslation(['transit', 'main']); + const currentValue = props.value ?? {}; const fieldDescriptors = React.useMemo(() => props.configSchema.getFields(), [props.configSchema]); @@ - const defaultFields = getDefaultFieldsFromSchema(props.value, props.configSchema); + const defaultFields = getDefaultFieldsFromSchema(currentValue, props.configSchema); @@ - const updatedParameters = { ...props.value, [path]: newValue.value }; + const updatedParameters = { ...currentValue, [path]: newValue.value }; @@ value={props.value[fieldName]} - completeObject={props.value} + value={currentValue[fieldName]} + completeObject={currentValue} disabled={props.disabled} fieldDescriptor={field} onValueChange={onValueChange}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx` around lines 306 - 313, OptionsDescriptorWidgets currently indexes props.value during render (in fieldWidgets mapping and FieldComponent props), causing a crash if callers pass no config object yet; ensure props.value is normalized before any render access (e.g. set a local normalizedValue = props.value || {} or call the existing normalize/defaulting helper) and use normalizedValue for value and completeObject passed into FieldComponent and any other reads so the mount effect can later apply defaults safely.packages/transition-common/src/utils/userDefinedConfig/index.ts (1)
135-142:⚠️ Potential issue | 🟠 MajorDo not materialize absent optional nested objects.
This always writes
{}for nested fields. That turns an omitted optional section into a present one, and later validation recurses into its children as if the user had provided it.Possible fix
if (fieldDef.type === 'nested') { // Handle nested options recursively const nestedDescriptor = fieldDef.schema; const existingNestedValue = fields[key as keyof T] as Record<string, unknown> | undefined; const nestedDefaults = getDefaultFieldsFromSchema(existingNestedValue || {}, nestedDescriptor); - (fields as any)[key] = nestedDefaults; + if (existingNestedValue !== undefined || Object.keys(nestedDefaults).length > 0) { + (fields as Record<string, unknown>)[key] = nestedDefaults; + } } else if (fields[key as keyof T] === undefined && 'default' in fieldDef && fieldDef.default !== undefined) { (fields as any)[key] = fieldDef.default; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/utils/userDefinedConfig/index.ts` around lines 135 - 142, The code currently always materializes nested fields by creating an empty object for type === 'nested', which turns omitted optional sections into present ones; change the logic in getDefaultFieldsFromSchema handling so that you only recurse and assign nested defaults when the existing value is present (i.e., fields[key as keyof T] !== undefined / existingNestedValue !== undefined) — keep the lookup of nestedDescriptor and call to getDefaultFieldsFromSchema only when existingNestedValue is provided, otherwise leave fields[key] undefined so absent optional nested objects are not materialized.locales/fr/transit.json (1)
895-895:⚠️ Potential issue | 🟡 MinorFix the French grammar here.
The sentence is missing
de, and the last noun should be plural.Possible fix
- "GenerationsToKeepGreaterThanGenerations": "Le nombre générations à conserver est supérieur au nombre de génération" + "GenerationsToKeepGreaterThanGenerations": "Le nombre de générations à conserver est supérieur au nombre de générations"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locales/fr/transit.json` at line 895, Update the value for the key "GenerationsToKeepGreaterThanGenerations" to correct French grammar: insert the missing "de" and make the final noun plural so the string reads "Le nombre de générations à conserver est supérieur au nombre de générations." Replace the current value with this corrected sentence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/transition-common/src/utils/userDefinedConfig/index.ts`:
- Around line 197-205: The integer/seconds type check wrongly only enforces
Number.isInteger when no custom validator is present, so custom validators for
'integer' and 'seconds' can accept non-integers; update the branches that handle
fieldDef.validate (the first block checking 'validate' in fieldDef and the
similar block later) to always apply an integer check when fieldDef.type ===
'integer' or 'seconds' in addition to calling fieldDef.validate(value) — i.e.,
for these types first confirm typeof value === 'number' and
Number.isInteger(value) then run fieldDef.validate(value) (or return false if
number/integer checks fail), ensuring both the numeric/integer constraints and
the custom validator are enforced.
---
Outside diff comments:
In `@packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts`:
- Line 142: The code still calls descriptor.getOptions() which no longer exists
because getDescriptor() now returns a UserDefinedConfigSchema; replace the call
with descriptor.getFields() and update any downstream usage of methodOptions
(the variable assigned from getOptions()) to work with the fields shape returned
by getFields() (rename methodOptions if helpful and adapt access patterns where
used in AlgorithmEdition and any referenced methods).
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.ts`:
- Around line 51-82: The schema leaves odTripFitnessFunction and fitnessFunction
optional so validateFields allows configs like { sampleRatio: 1 } that do not
satisfy OdTripSimulationOptions; update the schema entries for
odTripFitnessFunction and fitnessFunction to either be required or provide
sensible defaults so the editor always emits a valid config (e.g., mark fields
as required or add default values), and ensure validateFields uses those
defaults when validating; locate the entries named odTripFitnessFunction and
fitnessFunction in OdTripSimulationMethod and adjust their schema definitions
accordingly (also apply the same change for the duplicate block at lines 85-91).
In `@packages/transition-common/src/services/simulation/Simulation.ts`:
- Around line 79-87: In _prepareAttributes(), before accessing
algoConfig.config, ensure algoConfig.config is initialized when
algorithmConfiguration.type is present (e.g., if (!algoConfig.config)
algoConfig.config = {}), then proceed to call
getAlgorithmDescriptor(algoConfig.type) and iterate descriptor.getFields(); this
prevents dereferencing undefined when algorithmConfiguration.type exists but
config is missing while still allowing validate() to handle absent configs
elsewhere.
---
Duplicate comments:
In `@locales/en/transit.json`:
- Around line 856-869: Update the pluralization for the "NumberOfRandoms" keys:
change the label value for NumberOfRandoms to use "randoms" (e.g., "Number of
randoms per generation") and update NumberOfRandomsInvalid to also use "randoms"
(e.g., "Number of randoms per generation must be greater or equal to 0"); modify
the values for the keys NumberOfRandoms and NumberOfRandomsInvalid accordingly.
In `@locales/fr/transit.json`:
- Line 895: Update the value for the key
"GenerationsToKeepGreaterThanGenerations" to correct French grammar: insert the
missing "de" and make the final noun plural so the string reads "Le nombre de
générations à conserver est supérieur au nombre de générations." Replace the
current value with this corrected sentence.
In
`@packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts`:
- Around line 101-106: The numberOfGenerations field is declared as a generic
number so non-integer values (e.g. 12.5) pass validation; change its schema to
use an integer field and tighten validation: set type to 'integer' (like the
other counters) and update the validate function to ensure the value is an
integer and > 0 (e.g. use Number.isInteger(value) && value > 0), leaving the
default at 100; modify the declaration for numberOfGenerations in
EvolutionaryAlgorithm.ts accordingly.
In `@packages/transition-common/src/services/simulation/Simulation.ts`:
- Around line 130-155: The current manual checks using
algorithmDescriptor.getFields and per-field validate plus validateFields miss
schema rules (required, types, enums, nested); replace that validation block to
call validateFieldsWithSchema(algoConfig.config, algorithmDescriptor), use its
returned valid/errors to set this._isValid and to push generic banner error into
this._errors (instead of the ad-hoc erroneousFields logic), and still merge any
descriptor-level errors (algorithmDescriptor.validateFields) if needed; keep
getDefaultFieldsFromSchema(...) assignment for missing config but route all
validation through validateFieldsWithSchema and derive the top-level
banner/error array from its result.
In
`@packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts`:
- Around line 70-74: The test currently reuses a single validateMock and calls
validateMock.mockReset() in beforeEach which clears any mockReturnValueOnce()
setups created in the test.each rows, causing row-specific cases to fall back to
the default true; fix by creating a fresh validator mock per test case instead
of resetting the shared one—move the mock construction into beforeEach (e.g.,
reassign validateMock = jest.fn() or create a new local mock in the test body)
so that mockReturnValueOnce(...) configured in the test.each rows is preserved
for that test; reference validateMock, beforeEach, mockReturnValueOnce, and the
test.each table when making the change.
In `@packages/transition-common/src/utils/userDefinedConfig/index.ts`:
- Around line 135-142: The code currently always materializes nested fields by
creating an empty object for type === 'nested', which turns omitted optional
sections into present ones; change the logic in getDefaultFieldsFromSchema
handling so that you only recurse and assign nested defaults when the existing
value is present (i.e., fields[key as keyof T] !== undefined /
existingNestedValue !== undefined) — keep the lookup of nestedDescriptor and
call to getDefaultFieldsFromSchema only when existingNestedValue is provided,
otherwise leave fields[key] undefined so absent optional nested objects are not
materialized.
In
`@packages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsx`:
- Around line 51-53: The React key for UserDefinedConfigEditComponent is
currently derived using _toInteger(props.algorithmConfig?.type), which can
collapse distinct algorithm identifiers; change the key to use the raw algorithm
type value (e.g., props.algorithmConfig?.type or a stable stringified form) so
the component remounts correctly when the selected algorithm changes—update the
key expression in TransitNetworkDesignAlgorithmComponent (the
UserDefinedConfigEditComponent key) to reference the raw algorithmConfig.type
instead of coercing via _toInteger.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx`:
- Around line 110-141: Extend the two snapshot tests in
OptionsDescriptorWidgets.test.tsx to also simulate user interactions on key
widgets of UserDefinedConfigEditComponent and assert the emitted onUpdate
payload and validity propagation: use the existing onUpdate mock to verify
specific updates when changing stringField, booleanField, selectField,
multiselectField, csvField and nestedField (e.g., type new values and toggle
checkbox/select options) and assert onUpdate receives the expected partial/full
config objects; additionally, simulate invalid inputs for numeric fields
(integerField, numberField, percentageField, secondsField) and assert the
component reports invalid state (via onUpdate payload or exposed validity flag)
and that validity clears when corrected. Ensure tests reference
UserDefinedConfigEditComponent and the exact field names used in the test value
object so the assertions remain robust if other parts of the file change.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx`:
- Around line 297-303: The onValueChange handler currently ignores the incoming
widget readiness flag (newValue.valid) and uses only validateFieldsWithSchema
result; change it so that if newValue.valid === false you force the overall
validity to false when calling props.onUpdate (i.e., compute overallValid =
newValue.valid === false ? false : valid), continue to update the parameter
value and setErrors(errors) as before, and pass overallValid into
props.onUpdate; update references in onValueChange, validateFieldsWithSchema,
props.onUpdate, and setErrors accordingly.
- Around line 306-313: OptionsDescriptorWidgets currently indexes props.value
during render (in fieldWidgets mapping and FieldComponent props), causing a
crash if callers pass no config object yet; ensure props.value is normalized
before any render access (e.g. set a local normalizedValue = props.value || {}
or call the existing normalize/defaulting helper) and use normalizedValue for
value and completeObject passed into FieldComponent and any other reads so the
mount effect can later apply defaults safely.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 596da1f9-d155-40bd-8e3c-62bc58a0b152
⛔ Files ignored due to path filters (1)
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/__snapshots__/OptionsDescriptorWidgets.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
locales/en/transit.jsonlocales/fr/transit.jsonpackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/TransitNetworkDesignAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-frontend/src/components/forms/simulation/SimulationEdit.tsxpackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
💤 Files with no reviewable changes (2)
- packages/transition-frontend/src/components/forms/simulation/SimulationEdit.tsx
- packages/transition-common/src/services/networkDesign/transit/TransitNetworkDesignAlgorithm.ts
tahini
force-pushed
the
userDefinedOptionDescriptor
branch
from
08ae728 to
bfa008f
Compare
Collaborator
Author
|
I update the doc for the interface and type, to explain more how to use it. I also updated the commit message with information from the issue where we mentioned the other packages that were considered and why they were not used. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.ts (1)
39-47:⚠️ Potential issue | 🟡 MinorType mismatch:
errorsshould beTranslatableMessage[].Same issue as in the common package stub. The
errorsarray type should match the interface contract.Suggested fix
+import { TranslatableMessage } from 'chaire-lib-common/lib/utils/TranslatableMessage'; + validateFields = (fields: Partial<AlgorithmStubOptions>) => { let valid = true; - let errors: string[] = []; + let errors: TranslatableMessage[] = []; if (fields.stringOption === undefined) { valid = false; errors.push('StringOptionMandatory'); } return { valid, errors }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.ts` around lines 39 - 47, In TransitNetworkDesignAlgorithmDescriptorStub, the validateFields method currently declares errors as string[] but the interface expects TranslatableMessage[]; update validateFields (in the TransitNetworkDesignAlgorithmDescriptorStub class) to type errors as TranslatableMessage[] and push appropriate TranslatableMessage values (not plain strings) when adding 'StringOptionMandatory' (or use the existing TranslatableMessage factory/enum used elsewhere), and ensure the returned object still matches the expected { valid, errors } shape for AlgorithmStubOptions validation.packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.ts (1)
39-47:⚠️ Potential issue | 🟡 MinorType mismatch:
errorsshould beTranslatableMessage[].The
validateFieldsmethod returnserrors: string[], butUserDefinedConfigSchemainterface expectserrors?: TranslatableMessage[]. Whilestringis assignable toTranslatableMessage, explicitly typing it correctly improves clarity and ensures consistency with the interface contract.Suggested fix
+import { TranslatableMessage } from 'chaire-lib-common/lib/utils/TranslatableMessage'; + // ... in validateFields method validateFields = (fields: Partial<AlgorithmStubOptions>) => { let valid = true; - let errors: string[] = []; + let errors: TranslatableMessage[] = []; if (fields.stringOption === undefined) { valid = false; errors.push('StringOptionMandatory'); } return { valid, errors }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.ts` around lines 39 - 47, The validateFields method returns errors as string[] but the UserDefinedConfigSchema expects TranslatableMessage[]; update validateFields (in TransitNetworkDesignAlgorithmStub) so errors is typed as TranslatableMessage[] (and the function return type reflects UserDefinedConfigSchema or an appropriate typed object), convert or cast pushed error identifiers (e.g., 'StringOptionMandatory') to TranslatableMessage when calling errors.push, and ensure AlgorithmStubOptions usage remains unchanged so the returned value matches the expected interface.packages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts (1)
104-153: 🧹 Nitpick | 🔵 TrivialUse
test.eachfor these validator matrices.The
forEachloops collapse many cases into a few tests, so one failure will not say which field or range regressed.As per coding guidelines,
**/*.test.{ts,tsx}: Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts` around lines 104 - 153, Replace the forEach-based grouped assertions with parametric tests using test.each so each validator+value combination runs as its own test row; specifically, for the positiveValidators and probabilityValidators arrays (and numberOfRandoms), pull options via descriptor.getFields(), then create test.each tables that iterate [fieldName, value, expectedBoolean] and call (options[fieldName as keyof typeof options] as any).validate!(value) to assert the expected result, ensuring failures report the exact field and value (use identifiers like 'populationSizeMin', 'numberOfRandoms', 'crossoverProbability', etc.).packages/transition-common/src/services/simulation/Simulation.ts (1)
83-87:⚠️ Potential issue | 🟠 MajorBackfill missing algorithm defaults instead of writing
undefined.When an existing simulation loads with a partial
algorithmConfiguration.config, this loop leaves every new field unset. Defaulted fields likekeepGenerationsandkeepCandidateswill stayundefined, andvalidate()only repairs the config when the whole object is missing.🛠️ Suggested fix
- const options = descriptor.getFields(); - Object.keys(options).forEach((key) => { - if (algoConfig.config[key] === undefined) { - algoConfig.config[key] = undefined; - } - }); + algoConfig.config = getDefaultFieldsFromSchema( + algoConfig.config ?? {}, + descriptor + ) as AlgorithmConfiguration['config'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/simulation/Simulation.ts` around lines 83 - 87, The loop currently sets new fields to undefined; instead, when iterating descriptor.getFields() for each key that is missing in algoConfig.config, look up the field's declared default and assign that value (e.g., read options[key].defaultValue or options[key].default or a descriptor-provided default getter) and set algoConfig.config[key] to that default; if no default exists, omit the property (or set to null) but do not explicitly set undefined. Update the loop around descriptor.getFields() and algoConfig.config to use the field descriptor's default value lookup so defaults like keepGenerations/keepCandidates are backfilled, and ensure validate() still runs afterwards if needed.packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.ts (1)
34-43:⚠️ Potential issue | 🟡 Minor
choicesfunction signature doesn't match interface.The
UserDefinedConfigFieldByTypeinterface requireschoices: (object: Record<string, unknown>) => {...}, but this declareschoices: () => {...}. Add the required parameter.Suggested fix
- choices: () => { + choices: (_object: Record<string, unknown>) => { // FIXME Still using data source queries. When this code was in the🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.ts` around lines 34 - 43, The choices declaration in AccessibilityMapSimulationMethod.ts has the wrong signature: change choices: () => {...} to match the UserDefinedConfigFieldByType signature, e.g. choices: (context: Record<string, unknown>) => {...}; update the function parameter name (context or params) so the compiler sees the required parameter and keep the current body (returning dataSources.map(...)) — if the parameter is unused, accept it but do not remove it; this occurs on the choices method inside the AccessibilityMapSimulationMethod class/object.
♻️ Duplicate comments (11)
packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts (1)
50-56:⚠️ Potential issue | 🟡 MinorCall
choiceswith an object argument.Line 52 calls
choices()without arguments, but the method signature expects an object parameter. Pass an empty object to match the schema contract.Suggested fix
test('should return empty choices for dataSourceId (FIXME case)', () => { const options = descriptor.getFields(); - const choices = options.dataSourceId.choices(); + const choices = options.dataSourceId.choices({}); expect(Array.isArray(choices)).toBe(true); expect(choices).toHaveLength(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts` around lines 50 - 56, The test calls options.dataSourceId.choices() with no arguments but the choices method expects an object parameter; update the test to call descriptor.getFields() then invoke options.dataSourceId.choices({}) (i.e., pass an empty object) so the call matches the method signature used by choices in the codebase (refer to descriptor.getFields and options.dataSourceId.choices).packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts (1)
70-74:⚠️ Potential issue | 🟡 MinorShared mock with queued returns can cause order-dependent tests.
The
validateMockis shared, andmockReturnValueOnceis called when thetest.eachtable is constructed (at module load).mockReset()inbeforeEachclears call history but the one-shot returns are already queued during table creation. This can make tests order-dependent if run individually vs. together.Consider initializing the mock per-test or using
mockReturnValueinside each test body.Also applies to: 183-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts` around lines 70 - 74, The shared validateMock uses mockReturnValueOnce at module load which queues one-shot returns and makes tests order-dependent; to fix, create/reset the mock per test (move const validateMock = jest.fn() into beforeEach or reassign it there) and set return values inside each test body (use validateMock.mockReturnValue(...) or call mockReturnValueOnce inside the test) so test.each row construction no longer captures queued returns; update usages of validateMock in the tests referenced (the test.each table and the tests around the validateMock usage) to use per-test initialization instead of module-level queued returns.packages/transition-common/src/utils/userDefinedConfig/index.ts (1)
251-267:⚠️ Potential issue | 🟠 MajorEnforce integer check even when custom validator passes.
For
integerandsecondstypes, if a custom validator exists and returnstrue, noNumber.isIntegercheck runs. A custom rule likevalue > 0will accept1.5.Suggested fix
if (isValid !== true) { valid = false; // If the return value is a string, it is a custom error message, otherwise use the default message based on the field name and "Invalid" errors.push(typeof isValid === 'boolean' ? fieldDef.i18nName + 'Invalid' : isValid); + } else if ( + (fieldDef.type === 'integer' || fieldDef.type === 'seconds') && + !Number.isInteger(value) + ) { + setFieldInvalid(fieldDef, 'Invalid'); } } else if (fieldDef.type === 'percentage') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/utils/userDefinedConfig/index.ts` around lines 251 - 267, The validator currently trusts a custom validator for 'integer' and 'seconds' types and thus allows non-integers; update the validation logic in the block handling fieldDef.validate (the code using fieldDef.type === 'integer' || 'seconds' etc.) so that after computing isValid = fieldDef.validate(value) for number types you also enforce Number.isInteger(value) for types 'integer' and 'seconds' (i.e., set isValid = false or an appropriate i18n error if the custom validator passed but Number.isInteger(value) is false). Use the same error message construction (fieldDef.i18nName + 'Invalid' when isValid is boolean) and ensure the integer check runs regardless of the custom validator result for the 'integer' and 'seconds' cases.packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts (1)
65-69:⚠️ Potential issue | 🔴 CriticalFinish the
getFieldsrename in the simulation-method path.This file now migrates descriptors to
getFields(), butgetSimulationFunctions()still callsdescriptor.getOptions()on Line 142. OncegetDescriptor()returnsUserDefinedConfigSchema, that branch stops type-checking.🛠️ Suggested fix
- const methodOptions = descriptor.getOptions(); + const methodOptions = descriptor.getFields();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts` around lines 65 - 69, The getFields rename is incomplete: update the call in getSimulationFunctions() that currently uses descriptor.getOptions() to use descriptor.getFields() so the code path remains type-safe once getDescriptor() returns UserDefinedConfigSchema; locate getSimulationFunctions(), replace descriptor.getOptions() with descriptor.getFields(), and ensure any variable names or expected types that assumed "options" are adjusted to the "fields" shape returned by getFields() (e.g., where algoOptions is used).locales/en/transit.json (1)
856-869:⚠️ Potential issue | 🟡 MinorUse the plural form for
NumberOfRandoms.Both strings still read “Number of random per generation”. Please keep the label and validation message aligned with the plural noun.
✏️ Suggested fix
- "NumberOfRandoms": "Number of random per generation", + "NumberOfRandoms": "Number of randoms per generation", @@ - "NumberOfRandomsInvalid": "Number of random per generation must be greater or equal to 0", + "NumberOfRandomsInvalid": "Number of randoms per generation must be greater or equal to 0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locales/en/transit.json` around lines 856 - 869, Update the two localization entries for NumberOfRandoms and NumberOfRandomsInvalid so they use the plural noun "randoms"; specifically change the label for "NumberOfRandoms" from "Number of random per generation" to "Number of randoms per generation" and change "NumberOfRandomsInvalid" from "Number of random per generation must be greater or equal to 0" to "Number of randoms per generation must be greater or equal to 0" so the UI label and its validation message are consistent.packages/transition-common/src/services/simulation/Simulation.ts (1)
130-155:⚠️ Potential issue | 🟠 MajorUse the schema helper here instead of re-implementing partial validation.
This block only runs explicit field
validatecallbacks beforedescriptor.validateFields(), so required flags, select/multiselect membership, nested schemas, and primitive checks can still slip throughSimulation.validate().🛠️ Suggested fix
- const options = algorithmDescriptor.getFields(); if (algoConfig.config === undefined) { // FIXME Movign away from this class, to use jobs // instead, so not worth doing a proper refactor, but // the casting may not really give the complete type. algoConfig.config = getDefaultFieldsFromSchema( @@ - const erroneousFields = Object.keys(options).filter( - (option) => - algoConfig.config[option] !== undefined && - (options[option] as any).validate !== undefined && - !(options[option] as any).validate(algoConfig.config[option]) - ); - if (erroneousFields.length > 0) { - this._isValid = false; - this._errors.push('main:InvalidFormFields'); - } - const { valid: algoValid, errors: algoErrors } = algorithmDescriptor.validateFields( - algoConfig.config - ); + const { valid: algoValid, errors: algoErrors } = validateFieldsWithSchema( + algoConfig.config, + algorithmDescriptor + ); + if (!algoValid) { + this._errors.push('main:InvalidFormFields'); + }Add
validateFieldsWithSchemato the existinguserDefinedConfigimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/simulation/Simulation.ts` around lines 130 - 155, Replace the ad-hoc per-field validate loop with the shared schema helper: import validateFieldsWithSchema from the existing userDefinedConfig helper, call validateFieldsWithSchema(algoConfig.config, algorithmDescriptor) instead of computing erroneousFields, and merge its returned { valid, errors } into this._isValid and this._errors (preserving the subsequent algorithmDescriptor.validateFields result if still needed). Update the code that sets algoConfig.config default and then call validateFieldsWithSchema before/alongside algorithmDescriptor.validateFields, removing the manual Object.keys(options).filter(...) check.packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx (1)
109-141: 🧹 Nitpick | 🔵 TrivialAdd behavior assertions beside these snapshots.
Both tests still pass if
onUpdateor validity propagation regress, so snapshot diffs alone will not tell us what broke.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx` around lines 109 - 141, Add explicit behavior assertions to the UserDefinedConfigEditComponent tests: after rendering the component (using UserDefinedConfigEditComponent, mockSchema, TestConfig and the onUpdate mock), simulate user interactions on a few representative inputs (e.g., update stringField text, toggle booleanField, change selectField and multiselectField) and assert that onUpdate was called with the expected updated values; also add assertions that validity propagation occurs (e.g., when entering an invalid value for integerField or leaving a required field empty, assert onUpdate is called with a payload or callback indicating valid: false, and when corrected it reports valid: true). Ensure you reference the onUpdate mock in these assertions so regressions in update/validity propagation fail the tests.packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts (1)
101-107: 🧹 Nitpick | 🔵 TrivialConsider using 'integer' type for
numberOfGenerations.
keepGenerationsandkeepCandidateswere updated to 'integer', butnumberOfGenerationsremains 'number'. For consistency with other generation/count fields, consider changing it to 'integer'.Suggested fix
numberOfGenerations: { i18nName: 'transit:networkDesign.evolutionaryAlgorithm.NumberOfGenerations', i18nHelp: 'transit:networkDesign.evolutionaryAlgorithm.help.NumberOfGenerations', - type: 'number' as const, + type: 'integer' as const, validate: (value: number) => value > 0, default: 100 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts` around lines 101 - 107, The numberOfGenerations option in EvolutionaryAlgorithm.ts should use the same 'integer' type as keepGenerations and keepCandidates; change its type from 'number' to 'integer' (i.e., type: 'integer' as const) and tighten validation to only accept positive integers (e.g., use Number.isInteger(value) && value > 0) while keeping the default at 100 so the field is consistent with other count fields.locales/fr/transit.json (1)
892-896:⚠️ Potential issue | 🟡 MinorMinor grammar issue in French error message.
Line 895 should read "Le nombre de générations à conserver est supérieur au nombre de générations".
Suggested fix
- "GenerationsToKeepGreaterThanGenerations": "Le nombre générations à conserver est supérieur au nombre de génération" + "GenerationsToKeepGreaterThanGenerations": "Le nombre de générations à conserver est supérieur au nombre de générations"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locales/fr/transit.json` around lines 892 - 896, Update the French translation value for the key "GenerationsToKeepGreaterThanGenerations" in locales/fr/transit.json to correct the grammar: add the missing "de" and pluralize the final word so the string becomes "Le nombre de générations à conserver est supérieur au nombre de générations". Locate the "errors" object and replace the current value for "GenerationsToKeepGreaterThanGenerations" accordingly.packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx (2)
281-282: 🧹 Nitpick | 🔵 TrivialGeneric type
Tcollapsed toanyin the exported component.The exported
UserDefinedConfigEditComponenterases the generic, forcing unsafe casts at call sites. Consider keeping the generic signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx` around lines 281 - 282, The exported component currently collapses its generic to any; update the declaration of UserDefinedConfigEditComponent to preserve the generic type parameter T so callers can provide strong types. Concretely, change the exported symbol UserDefinedConfigEditComponent to be generic (e.g. declare it as UserDefinedConfigEditComponent<T>) and accept props of type UserDefinedConfigEditComponentProps<T>, and ensure the React.FunctionComponent/React.FC typing also reflects the generic parameter; adjust any internal type assertions that were compensating for the previous any to use T instead.
297-304:⚠️ Potential issue | 🟠 MajorWidget-level validity (
newValue.valid) is ignored.The FIXME on line 299 acknowledges this. Widgets like
CsvFileOptionComponentcompute readiness viavalid, butonValueChangeonly trusts schema validation. This could reportisValid=truefor incomplete CSV mappings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx` around lines 297 - 304, The onValueChange handler currently ignores the widget-level newValue.valid and always revalidates the whole parameter set via validateFieldsWithSchema; update onValueChange to respect newValue.valid by treating the overall validity as false when newValue.valid === false (e.g., compute updatedParameters as before but if newValue.valid === false call props.onUpdate(updatedParameters, false) and setErrors to include or preserve the widget-level error state) otherwise continue with validateFieldsWithSchema to compute valid/errors; refer to onValueChange, CsvFileOptionComponent (which sets newValue.valid), validateFieldsWithSchema, props.onUpdate and setErrors to implement this short-circuit so incomplete widget states (like CSV mappings) correctly mark the form invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts`:
- Around line 32-37: The caller still calls the removed method
descriptor.getOptions(); update that usage to call the renamed method
descriptor.getFields() instead. Locate the call site in AlgorithmEdition (e.g.,
where descriptor.getOptions() is invoked in the task handling logic) and replace
it with descriptor.getFields(), ensuring any downstream handling expects the
fields shape returned by EvolutionaryAlgorithmDescriptor.getFields; adjust
variable names or destructuring if necessary to match the new return structure.
In `@packages/transition-common/src/utils/userDefinedConfig/index.ts`:
- Around line 188-197: The loop over fieldDefinitions assigns nestedDefaults to
(fields as any)[key]; replace the any cast with a typed intermediate like
existingParent: Record<string, unknown> and assign into fields via (fields as
unknown as Record<string, unknown>)[key] or set a local Record<string, unknown>
before merging to preserve type safety, and update the logic around
nestedDefaults: when fieldDef.type === 'nested', compute nestedDefaults via
getDefaultFieldsFromSchema(existingNestedValue || {}, nestedDescriptor) but only
assign it into fields if existingNestedValue !== undefined or
Object.keys(nestedDefaults).length > 0 to avoid creating empty parent objects
when the parent field was absent.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx`:
- Around line 115-118: currentMapping created by useMemo uses
CsvFileAndFieldMapper(option.mappingDescriptors, props.value) but only
props.value is listed as a dependency; add option.mappingDescriptors (or the
specific stable identifier like option.id if descriptors are immutable) to the
dependency array so the memo recomputes when mappingDescriptors change; update
the useMemo that defines currentMapping to include option.mappingDescriptors
alongside props.value.
---
Outside diff comments:
In
`@packages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.ts`:
- Around line 39-47: In TransitNetworkDesignAlgorithmDescriptorStub, the
validateFields method currently declares errors as string[] but the interface
expects TranslatableMessage[]; update validateFields (in the
TransitNetworkDesignAlgorithmDescriptorStub class) to type errors as
TranslatableMessage[] and push appropriate TranslatableMessage values (not plain
strings) when adding 'StringOptionMandatory' (or use the existing
TranslatableMessage factory/enum used elsewhere), and ensure the returned object
still matches the expected { valid, errors } shape for AlgorithmStubOptions
validation.
In
`@packages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.ts`:
- Around line 104-153: Replace the forEach-based grouped assertions with
parametric tests using test.each so each validator+value combination runs as its
own test row; specifically, for the positiveValidators and probabilityValidators
arrays (and numberOfRandoms), pull options via descriptor.getFields(), then
create test.each tables that iterate [fieldName, value, expectedBoolean] and
call (options[fieldName as keyof typeof options] as any).validate!(value) to
assert the expected result, ensuring failures report the exact field and value
(use identifiers like 'populationSizeMin', 'numberOfRandoms',
'crossoverProbability', etc.).
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.ts`:
- Around line 34-43: The choices declaration in
AccessibilityMapSimulationMethod.ts has the wrong signature: change choices: ()
=> {...} to match the UserDefinedConfigFieldByType signature, e.g. choices:
(context: Record<string, unknown>) => {...}; update the function parameter name
(context or params) so the compiler sees the required parameter and keep the
current body (returning dataSources.map(...)) — if the parameter is unused,
accept it but do not remove it; this occurs on the choices method inside the
AccessibilityMapSimulationMethod class/object.
In
`@packages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.ts`:
- Around line 39-47: The validateFields method returns errors as string[] but
the UserDefinedConfigSchema expects TranslatableMessage[]; update validateFields
(in TransitNetworkDesignAlgorithmStub) so errors is typed as
TranslatableMessage[] (and the function return type reflects
UserDefinedConfigSchema or an appropriate typed object), convert or cast pushed
error identifiers (e.g., 'StringOptionMandatory') to TranslatableMessage when
calling errors.push, and ensure AlgorithmStubOptions usage remains unchanged so
the returned value matches the expected interface.
In `@packages/transition-common/src/services/simulation/Simulation.ts`:
- Around line 83-87: The loop currently sets new fields to undefined; instead,
when iterating descriptor.getFields() for each key that is missing in
algoConfig.config, look up the field's declared default and assign that value
(e.g., read options[key].defaultValue or options[key].default or a
descriptor-provided default getter) and set algoConfig.config[key] to that
default; if no default exists, omit the property (or set to null) but do not
explicitly set undefined. Update the loop around descriptor.getFields() and
algoConfig.config to use the field descriptor's default value lookup so defaults
like keepGenerations/keepCandidates are backfilled, and ensure validate() still
runs afterwards if needed.
---
Duplicate comments:
In `@locales/en/transit.json`:
- Around line 856-869: Update the two localization entries for NumberOfRandoms
and NumberOfRandomsInvalid so they use the plural noun "randoms"; specifically
change the label for "NumberOfRandoms" from "Number of random per generation" to
"Number of randoms per generation" and change "NumberOfRandomsInvalid" from
"Number of random per generation must be greater or equal to 0" to "Number of
randoms per generation must be greater or equal to 0" so the UI label and its
validation message are consistent.
In `@locales/fr/transit.json`:
- Around line 892-896: Update the French translation value for the key
"GenerationsToKeepGreaterThanGenerations" in locales/fr/transit.json to correct
the grammar: add the missing "de" and pluralize the final word so the string
becomes "Le nombre de générations à conserver est supérieur au nombre de
générations". Locate the "errors" object and replace the current value for
"GenerationsToKeepGreaterThanGenerations" accordingly.
In `@packages/transition-backend/src/tasks/simulation/AlgorithmEdition.ts`:
- Around line 65-69: The getFields rename is incomplete: update the call in
getSimulationFunctions() that currently uses descriptor.getOptions() to use
descriptor.getFields() so the code path remains type-safe once getDescriptor()
returns UserDefinedConfigSchema; locate getSimulationFunctions(), replace
descriptor.getOptions() with descriptor.getFields(), and ensure any variable
names or expected types that assumed "options" are adjusted to the "fields"
shape returned by getFields() (e.g., where algoOptions is used).
In
`@packages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.ts`:
- Around line 101-107: The numberOfGenerations option in
EvolutionaryAlgorithm.ts should use the same 'integer' type as keepGenerations
and keepCandidates; change its type from 'number' to 'integer' (i.e., type:
'integer' as const) and tighten validation to only accept positive integers
(e.g., use Number.isInteger(value) && value > 0) while keeping the default at
100 so the field is consistent with other count fields.
In
`@packages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.ts`:
- Around line 50-56: The test calls options.dataSourceId.choices() with no
arguments but the choices method expects an object parameter; update the test to
call descriptor.getFields() then invoke options.dataSourceId.choices({}) (i.e.,
pass an empty object) so the call matches the method signature used by choices
in the codebase (refer to descriptor.getFields and
options.dataSourceId.choices).
In `@packages/transition-common/src/services/simulation/Simulation.ts`:
- Around line 130-155: Replace the ad-hoc per-field validate loop with the
shared schema helper: import validateFieldsWithSchema from the existing
userDefinedConfig helper, call validateFieldsWithSchema(algoConfig.config,
algorithmDescriptor) instead of computing erroneousFields, and merge its
returned { valid, errors } into this._isValid and this._errors (preserving the
subsequent algorithmDescriptor.validateFields result if still needed). Update
the code that sets algoConfig.config default and then call
validateFieldsWithSchema before/alongside algorithmDescriptor.validateFields,
removing the manual Object.keys(options).filter(...) check.
In
`@packages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.ts`:
- Around line 70-74: The shared validateMock uses mockReturnValueOnce at module
load which queues one-shot returns and makes tests order-dependent; to fix,
create/reset the mock per test (move const validateMock = jest.fn() into
beforeEach or reassign it there) and set return values inside each test body
(use validateMock.mockReturnValue(...) or call mockReturnValueOnce inside the
test) so test.each row construction no longer captures queued returns; update
usages of validateMock in the tests referenced (the test.each table and the
tests around the validateMock usage) to use per-test initialization instead of
module-level queued returns.
In `@packages/transition-common/src/utils/userDefinedConfig/index.ts`:
- Around line 251-267: The validator currently trusts a custom validator for
'integer' and 'seconds' types and thus allows non-integers; update the
validation logic in the block handling fieldDef.validate (the code using
fieldDef.type === 'integer' || 'seconds' etc.) so that after computing isValid =
fieldDef.validate(value) for number types you also enforce
Number.isInteger(value) for types 'integer' and 'seconds' (i.e., set isValid =
false or an appropriate i18n error if the custom validator passed but
Number.isInteger(value) is false). Use the same error message construction
(fieldDef.i18nName + 'Invalid' when isValid is boolean) and ensure the integer
check runs regardless of the custom validator result for the 'integer' and
'seconds' cases.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx`:
- Around line 109-141: Add explicit behavior assertions to the
UserDefinedConfigEditComponent tests: after rendering the component (using
UserDefinedConfigEditComponent, mockSchema, TestConfig and the onUpdate mock),
simulate user interactions on a few representative inputs (e.g., update
stringField text, toggle booleanField, change selectField and multiselectField)
and assert that onUpdate was called with the expected updated values; also add
assertions that validity propagation occurs (e.g., when entering an invalid
value for integerField or leaving a required field empty, assert onUpdate is
called with a payload or callback indicating valid: false, and when corrected it
reports valid: true). Ensure you reference the onUpdate mock in these assertions
so regressions in update/validity propagation fail the tests.
In
`@packages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsx`:
- Around line 281-282: The exported component currently collapses its generic to
any; update the declaration of UserDefinedConfigEditComponent to preserve the
generic type parameter T so callers can provide strong types. Concretely, change
the exported symbol UserDefinedConfigEditComponent to be generic (e.g. declare
it as UserDefinedConfigEditComponent<T>) and accept props of type
UserDefinedConfigEditComponentProps<T>, and ensure the
React.FunctionComponent/React.FC typing also reflects the generic parameter;
adjust any internal type assertions that were compensating for the previous any
to use T instead.
- Around line 297-304: The onValueChange handler currently ignores the
widget-level newValue.valid and always revalidates the whole parameter set via
validateFieldsWithSchema; update onValueChange to respect newValue.valid by
treating the overall validity as false when newValue.valid === false (e.g.,
compute updatedParameters as before but if newValue.valid === false call
props.onUpdate(updatedParameters, false) and setErrors to include or preserve
the widget-level error state) otherwise continue with validateFieldsWithSchema
to compute valid/errors; refer to onValueChange, CsvFileOptionComponent (which
sets newValue.valid), validateFieldsWithSchema, props.onUpdate and setErrors to
implement this short-circuit so incomplete widget states (like CSV mappings)
correctly mark the form invalid.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 98de486d-34ad-4d78-9662-4f6efe4ec8ec
⛔ Files ignored due to path filters (1)
packages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/__snapshots__/OptionsDescriptorWidgets.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
locales/en/transit.jsonlocales/fr/transit.jsonpackages/transition-backend/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmDescriptorStub.tspackages/transition-backend/src/services/simulation/methods/SimulationMethod.tspackages/transition-backend/src/tasks/simulation/AlgorithmEdition.tspackages/transition-common/src/services/networkDesign/transit/TransitNetworkDesignAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/algorithm/EvolutionaryAlgorithm.tspackages/transition-common/src/services/networkDesign/transit/algorithm/__tests__/EvolutionaryAlgorithm.test.tspackages/transition-common/src/services/networkDesign/transit/algorithm/index.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/AccessibilityMapSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/OdTripSimulationMethod.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/AccessibilityMapSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/__tests__/OdTripSimulationMethod.test.tspackages/transition-common/src/services/networkDesign/transit/simulationMethod/index.tspackages/transition-common/src/services/simulation/Simulation.tspackages/transition-common/src/services/simulation/__tests__/Simulation.test.tspackages/transition-common/src/services/simulation/__tests__/TransitNetworkDesignAlgorithmStub.tspackages/transition-common/src/utils/userDefinedConfig/__tests__/index.test.tspackages/transition-common/src/utils/userDefinedConfig/index.tspackages/transition-frontend/src/components/forms/simulation/SimulationEdit.tsxpackages/transition-frontend/src/components/forms/simulation/widgets/TransitNetworkDesignAlgorithmComponent.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/OptionsDescriptorWidgets.tsxpackages/transition-frontend/src/components/forms/userDefinedConfig/__tests__/OptionsDescriptorWidgets.test.tsx
💤 Files with no reviewable changes (2)
- packages/transition-frontend/src/components/forms/simulation/SimulationEdit.tsx
- packages/transition-common/src/services/networkDesign/transit/TransitNetworkDesignAlgorithm.ts
kaligrafy
requested changes
Apr 2, 2026
Collaborator
|
Pas tout reviewé, il y a beaucoup de stuff! |
tahini
force-pushed
the
userDefinedOptionDescriptor
branch
2 times, most recently
from
c0bd37d to
28c89ea
Compare
…criptor` part of chairemobilite#1580 The `SimulationAlgorithmDescriptor` is a very specific type for simulation algorithm, but it may be used to describe any user-defined set of configuration options, for network design or anything else. This adds the new types for field description and interface for schema, with all the field types required to describe the network design options required. The `UserDefinedConfigSchema` interface describes the name and options required by the config. It simplifies the definition and entry of the configuration data by allowing to define the schema of the configuration option types that have builtin functionalities, like type validation, translation management, and default value generation. Using this interface gives access to default validation functions for each option and for the entire object, as well as a way to get default values. Forms or any other entry mechanism can be dynamically generated from the schema. Note on other nodejs packages: A few nodejs packages have been considered to manage user-defined json schema, like `zod` that Transition already uses for object's json import, but is limited in possible types and is not extensible (for example, only integer and floating points are supported as number types, while we want specific treatment for percentages, seconds (entered as minutes, seconds, or hours, etc.), we also want to support csv file and field mapping entry in our configs, which are not supported by any of these packages). There's also many packages around json-schema support (https://json-schema.org/docs), like `react-jsonschema-form` and `@rjsf/core` for dynamic form generation, but they are not flexible enough for our needs.
part of chairemobilite#1580 This adds the React components to enter the various user defined field descriptor types from the UI, calling the appropriate validation functions and producing the valid objects for the component's consumer.
…nedConfigSchema` completes chairemobilite#1580 Remove the previous `SimulationAlgorithmDescriptor` interface, to be replaced with the new `UserDefinedConfigSchema`. Rename the classes' methods to match the new interface (`getOptions` becomes `getFields`, `validateOptions` becomes `validateFields`). Update a few translation strings to use the new `networkDesign:` namespace instead of `simulation`.
tahini
force-pushed
the
userDefinedOptionDescriptor
branch
from
28c89ea to
4b53fa1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #1580
The
SimulationAlgorithmDescriptoris a very specific type forsimulation algorithm, but it may be used to describe any user-defined
set of configuration options, for network design or anything else.
This adds the new types, with all the field types required to describe
the network design options required.
Adds the React components to enter the various user defined field
descriptor types from the UI, calling the appropriate validation
functions and producing the valid objects for the component's consumer.
Remove the previous
SimulationAlgorithmDescriptorinterface, to bereplaced with the new
UserDefinedConfigSchema.Rename the classes' methods to match the new interface (
getOptionsbecomes
getFields,validateOptionsbecomesvalidateFields).Update a few translation strings to use the new
networkDesign:namespace instead of
simulation.Summary by CodeRabbit
New Features
Improvements
Tests