Create a new BaseObject class to replace GenericObject and let TransitRouting extend it #1508
Conversation
WalkthroughAdds an abstract BaseObject (attribute storage, clone/merge, get/set, validate, errors) and unit tests. Refactors TransitRouting to extend BaseObject, replaces ObjectWithHistory, introduces _prepareAttributes and _validate, changes constructor to a single attributes parameter, and switches internal accesses to this.attributes with mergeAttributes/set/get usage. Updates TransitRouting tests. Frontend changes: ScenarioComparisonPanel and TransitRoutingForm removed the boolean constructor arg, use set/mergeAttributes and stricter TransitRoutingAttributes key typing, and replace dynamic field IDs with static IDs. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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/transitRouting/TransitRouting.ts (1)
116-130: Bug: destination feature id mismatch (id is 1, properties.id is 2).Set the feature id to 2 for destination to stay consistent.
- id: 1, + id: 2,Also consider extracting a small helper to build these point features to avoid future mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/chaire-lib-common/src/utils/objects/BaseObject.ts(1 hunks)packages/chaire-lib-common/src/utils/objects/__tests__/BaseObject.test.ts(1 hunks)packages/transition-common/src/services/transitRouting/TransitRouting.ts(6 hunks)packages/transition-common/src/services/transitRouting/__tests__/TransitRouting.test.ts(2 hunks)packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx(8 hunks)packages/transition-frontend/src/components/forms/transitRouting/TransitRoutingForm.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/transition-common/src/services/transitRouting/__tests__/TransitRouting.test.ts (1)
packages/transition-common/src/services/transitRouting/TransitRouting.ts (1)
TransitRouting(44-240)
packages/transition-frontend/src/components/forms/transitRouting/TransitRoutingForm.tsx (1)
packages/transition-common/src/services/transitRouting/TransitRouting.ts (2)
TransitRouting(44-240)TransitRoutingAttributes(40-40)
packages/transition-common/src/services/transitRouting/TransitRouting.ts (3)
packages/chaire-lib-common/src/services/routing/types.ts (1)
TripRoutingQueryAttributes(61-69)packages/chaire-lib-common/src/utils/objects/GenericObject.ts (4)
attributes(67-69)isValid(63-65)errors(72-74)errors(77-79)packages/transition-common/src/services/transitRouting/TransitRoutingQueryAttributes.ts (1)
validateTrQueryAttributes(54-66)
packages/chaire-lib-common/src/utils/objects/__tests__/BaseObject.test.ts (1)
packages/chaire-lib-common/src/utils/objects/BaseObject.ts (1)
attributes(37-39)
packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx (2)
packages/transition-common/src/services/transitRouting/TransitRouting.ts (2)
TransitRouting(44-240)TransitRoutingAttributes(40-40)packages/chaire-lib-frontend/src/components/forms/ChangeEventsForm.tsx (1)
onFormFieldChange(40-50)
⏰ 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). (3)
- GitHub Check: build-and-test (24.x)
- GitHub Check: pr-build-check
- GitHub Check: build-and-run-UI-tests (24.x)
🔇 Additional comments (7)
packages/transition-frontend/src/components/forms/transitRouting/TransitRoutingForm.tsx (1)
327-331: Static field IDs: good changeStatic IDs improve predictability and avoid unnecessary re-renders tied to dynamic IDs. Looks good.
Also applies to: 353-359, 364-383, 394-399
packages/transition-common/src/services/transitRouting/__tests__/TransitRouting.test.ts (2)
40-41: Constructor update LGTMUsing the new constructor signature without the boolean is correct.
203-209: mergeAttributes usage LGTMSwitching from setAttributes to mergeAttributes matches the BaseObject API.
packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx (1)
8-9: Confirm uuid dependency and bundled types: All relevant package.json files declareuuid(^11.1.0) and none include@types/uuid; v11 ships its own TypeScript definitions.packages/transition-common/src/services/transitRouting/TransitRouting.ts (3)
44-47: No second-arg constructor calls remaining Verified nonew TransitRouting(..., true|false)usages.
60-96: Time blank checks: ensure_isBlank(0)returns false. Midnight (0) must not be considered blank; verify this inchaire-lib-common.
205-222: Usethis.setinstead of mutatingsavedForBatchin place
Build a new array and callthis.set('savedForBatch', …)to trigger change tracking:- if (this.attributes.savedForBatch.length >= MAX_BATCH_ELEMENTS) { - this.attributes.savedForBatch.splice(0, this.attributes.savedForBatch.length - MAX_BATCH_ELEMENTS + 1); - } - this.attributes.savedForBatch.push(element); + const current = this.attributes.savedForBatch; + const trimmed = + current.length >= MAX_BATCH_ELEMENTS + ? current.slice(current.length - MAX_BATCH_ELEMENTS + 1) + : current.slice(); + this.set('savedForBatch', [...trimmed, element]);(Note:
routingPortisn’t part ofTransitRoutingQueryand need not be added to the uniqueness check.)
| test('should validate object correctly', () => { | ||
| const invalidObj = new TestObject({ name: 'invalid', shortname: 'TS' }); | ||
| expect(invalidObj.validate()).toBe(false); | ||
| expect(invalidObj.getErrors()).toContain('Name is invalid'); | ||
| expect(invalidObj.isValid).toBe(false); | ||
| expect(invalidObj.getErrors()).toEqual(['Name is invalid']); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rename duplicate test title for clarity
Both tests in this describe block share the same name. Please rename the second to reflect the invalid case (e.g., “should invalidate object when name is invalid”).
🤖 Prompt for AI Agents
packages/chaire-lib-common/src/utils/objects/__tests__/BaseObject.test.ts lines
69-76: the second test in this describe block duplicates the first test title;
rename the test title to reflect the invalid-case scenario (for example "should
invalidate object when name is invalid") so test names are unique and
descriptive; update only the string passed to test(...) and keep assertions
intact.
| get<K extends keyof T>(path: K, defaultValue: T[K] | undefined = undefined): T[K] | undefined { | ||
| const value = this._attributes[path] ? this._attributes[path] : _get(this._attributes, path); | ||
| if (defaultValue === undefined && value !== undefined) { | ||
| return value; | ||
| } | ||
| return value === null || value === undefined || value === '' ? defaultValue : value; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
get() treats empty string as “blank”
Returning default when value === '' may be surprising for some fields. If intentional, add a short comment; otherwise consider only null/undefined.
🤖 Prompt for AI Agents
In packages/chaire-lib-common/src/utils/objects/BaseObject.ts around lines 76 to
83, the get() method currently treats the empty string ('') as a blank and
returns the default; change this so empty string is treated as a valid value by
removing the '' check from the final conditional (i.e., only consider null or
undefined as missing), or if empty string should indeed be treated as blank, add
a short inline comment explaining that intentional behavior; update tests if any
rely on the previous behavior.
| export type TransitRoutingQuery = { | ||
| routingName?: string; | ||
| departureTimeSecondsSinceMidnight?: number; | ||
| arrivalTimeSecondsSinceMidnight?: number; | ||
| originGeojson: GeoJSON.Feature<GeoJSON.Point>; | ||
| destinationGeojson: GeoJSON.Feature<GeoJSON.Point>; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add optional routingId to align with frontend and aid dedupe.
Frontend adds a v4 routingId; allow it here to avoid excess-property errors and to use as a stable key.
export type TransitRoutingQuery = {
routingName?: string;
+ routingId?: string; // optional unique identifier for UI/batch entries
departureTimeSecondsSinceMidnight?: number;
arrivalTimeSecondsSinceMidnight?: number;
originGeojson: GeoJSON.Feature<GeoJSON.Point>;
destinationGeojson: GeoJSON.Feature<GeoJSON.Point>;
};Based on learnings
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type TransitRoutingQuery = { | |
| routingName?: string; | |
| departureTimeSecondsSinceMidnight?: number; | |
| arrivalTimeSecondsSinceMidnight?: number; | |
| originGeojson: GeoJSON.Feature<GeoJSON.Point>; | |
| destinationGeojson: GeoJSON.Feature<GeoJSON.Point>; | |
| } | |
| }; | |
| export type TransitRoutingQuery = { | |
| routingName?: string; | |
| routingId?: string; // optional unique identifier for UI/batch entries | |
| departureTimeSecondsSinceMidnight?: number; | |
| arrivalTimeSecondsSinceMidnight?: number; | |
| originGeojson: GeoJSON.Feature<GeoJSON.Point>; | |
| destinationGeojson: GeoJSON.Feature<GeoJSON.Point>; | |
| }; |
🤖 Prompt for AI Agents
In packages/transition-common/src/services/transitRouting/TransitRouting.ts
around lines 18 to 24, the TransitRoutingQuery type is missing an optional
routingId which the frontend supplies; add routingId?: string to the type
definition so consumers won't get excess-property errors and can use the
frontend v4 routingId as a stable key. Ensure the new field is optional and
documented briefly in the type comment or nearby typedefs.
packages/transition-common/src/services/transitRouting/TransitRouting.ts
Show resolved
Hide resolved
| this.attributes.savedForBatch.splice(0, this.attributes.savedForBatch.length); | ||
| this.updateRoutingPrefs(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reset batch immutably for consistent change tracking.
- this.attributes.savedForBatch.splice(0, this.attributes.savedForBatch.length);
+ this.set('savedForBatch', []);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/transition-common/src/services/transitRouting/TransitRouting.ts
around lines 226 to 227, the code mutates this.attributes.savedForBatch in-place
via splice which can prevent consistent change tracking; replace the splice with
an immutable reset by assigning a new empty array to
this.attributes.savedForBatch (e.g., this.attributes.savedForBatch = []), then
call this.updateRoutingPrefs() as before so downstream observers and change
detection see the new reference.
| // FIXME Try to have separate functions for alternate scenarios, or manage them separately, or send a specific object here is only one needs update | ||
| const onValueChange = ( | ||
| path: string, | ||
| path: keyof TransitRoutingAttributes | 'alternateScenario1Id' | 'alternateScenario2Id', | ||
| newValue: { value: any; valid?: boolean } = { value: null, valid: true }, | ||
| resetResultsFlag = true | ||
| ) => { | ||
| onFormFieldChange(path, newValue); | ||
| if (newValue.valid || newValue.valid === undefined) { | ||
| if ( | ||
| newValue.valid || | ||
| (newValue.valid === undefined && path !== 'alternateScenario1Id' && path !== 'alternateScenario2Id') | ||
| ) { | ||
| const updatedObject = routingObj; | ||
| updatedObject.set(path, newValue.value); | ||
| updatedObject.set(path as keyof TransitRoutingAttributes, newValue.value); | ||
| if (typeof updatedObject.validate === 'function') { |
There was a problem hiding this comment.
Avoid setting unknown keys on the routing object
Strengthen the guard so alternateScenario* never calls set, regardless of valid.
- if (
- newValue.valid ||
- (newValue.valid === undefined && path !== 'alternateScenario1Id' && path !== 'alternateScenario2Id')
- ) {
- const updatedObject = routingObj;
- updatedObject.set(path as keyof TransitRoutingAttributes, newValue.value);
+ const isAltScenario = path === 'alternateScenario1Id' || path === 'alternateScenario2Id';
+ if (!isAltScenario && (newValue.valid || newValue.valid === undefined)) {
+ const updatedObject = routingObj;
+ updatedObject.set(path as keyof TransitRoutingAttributes, newValue.value);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // FIXME Try to have separate functions for alternate scenarios, or manage them separately, or send a specific object here is only one needs update | |
| const onValueChange = ( | |
| path: string, | |
| path: keyof TransitRoutingAttributes | 'alternateScenario1Id' | 'alternateScenario2Id', | |
| newValue: { value: any; valid?: boolean } = { value: null, valid: true }, | |
| resetResultsFlag = true | |
| ) => { | |
| onFormFieldChange(path, newValue); | |
| if (newValue.valid || newValue.valid === undefined) { | |
| if ( | |
| newValue.valid || | |
| (newValue.valid === undefined && path !== 'alternateScenario1Id' && path !== 'alternateScenario2Id') | |
| ) { | |
| const updatedObject = routingObj; | |
| updatedObject.set(path, newValue.value); | |
| updatedObject.set(path as keyof TransitRoutingAttributes, newValue.value); | |
| if (typeof updatedObject.validate === 'function') { | |
| // FIXME Try to have separate functions for alternate scenarios, or manage them separately, or send a specific object here if only one needs update | |
| const onValueChange = ( | |
| path: keyof TransitRoutingAttributes | 'alternateScenario1Id' | 'alternateScenario2Id', | |
| newValue: { value: any; valid?: boolean } = { value: null, valid: true }, | |
| resetResultsFlag = true | |
| ) => { | |
| onFormFieldChange(path, newValue); | |
| const isAltScenario = | |
| path === 'alternateScenario1Id' || path === 'alternateScenario2Id'; | |
| if (!isAltScenario && (newValue.valid || newValue.valid === undefined)) { | |
| const updatedObject = routingObj; | |
| updatedObject.set(path as keyof TransitRoutingAttributes, newValue.value); | |
| if (typeof updatedObject.validate === 'function') { | |
| // …rest of validation logic | |
| } | |
| } | |
| // …rest of onValueChange | |
| }; |
🤖 Prompt for AI Agents
In
packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx
around lines 90–103, the current guard still allows calling routingObj.set when
path is 'alternateScenario1Id' or 'alternateScenario2Id' (if valid is true);
prevent setting unknown keys on the routing object by adding an explicit check
so that if path === 'alternateScenario1Id' || path === 'alternateScenario2Id' we
skip calling updatedObject.set entirely (handle those alternate IDs with their
own dedicated update logic or leave onFormFieldChange to manage them), while
leaving the existing validation and validate() call intact for real
TransitRoutingAttributes keys only.
packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx
Outdated
Show resolved
Hide resolved
| if (_isBlank(routingObj.get('withAlternatives'))) { | ||
| routingObj.attributes.withAlternatives = false; | ||
| routingObj.set('withAlternatives', false); | ||
| } |
There was a problem hiding this comment.
Sync local state when defaulting withAlternatives
Mirror the default into formValues to keep UI consistent.
- if (_isBlank(routingObj.get('withAlternatives'))) {
- routingObj.set('withAlternatives', false);
- }
+ if (_isBlank(routingObj.get('withAlternatives'))) {
+ routingObj.set('withAlternatives', false);
+ setFormValues((prev) => ({ ...prev, withAlternatives: false }));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (_isBlank(routingObj.get('withAlternatives'))) { | |
| routingObj.attributes.withAlternatives = false; | |
| routingObj.set('withAlternatives', false); | |
| } | |
| if (_isBlank(routingObj.get('withAlternatives'))) { | |
| routingObj.set('withAlternatives', false); | |
| setFormValues((prev) => ({ ...prev, withAlternatives: false })); | |
| } |
🤖 Prompt for AI Agents
packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx
around lines 333-335: when you default routingObj.withAlternatives to false,
also mirror that default into the component's local form state so the UI stays
consistent; after routingObj.set('withAlternatives', false) update the
formValues state (e.g. via the appropriate setter such as setFormValues(prev =>
({ ...prev, withAlternatives: false })) or the form API you use) so both
routingObj and formValues reflect the same false value.
| if (_isBlank(transitRouting.get('withAlternatives'))) { | ||
| transitRouting.attributes.withAlternatives = false; | ||
| transitRouting.set('withAlternatives', false); | ||
| } | ||
|
|
There was a problem hiding this comment.
Sync local state when defaulting withAlternatives
You set the model default but not the form state, so the radio may render unset. Update formValues too.
- if (_isBlank(transitRouting.get('withAlternatives'))) {
- transitRouting.set('withAlternatives', false);
- }
+ if (_isBlank(transitRouting.get('withAlternatives'))) {
+ transitRouting.set('withAlternatives', false);
+ setFormValues((prev) => ({ ...prev, withAlternatives: false }));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (_isBlank(transitRouting.get('withAlternatives'))) { | |
| transitRouting.attributes.withAlternatives = false; | |
| transitRouting.set('withAlternatives', false); | |
| } | |
| if (_isBlank(transitRouting.get('withAlternatives'))) { | |
| transitRouting.set('withAlternatives', false); | |
| setFormValues((prev) => ({ ...prev, withAlternatives: false })); | |
| } |
🤖 Prompt for AI Agents
In
packages/transition-frontend/src/components/forms/transitRouting/TransitRoutingForm.tsx
around lines 303 to 306, you default the model field withAlternatives to false
but do not update the form state, causing the radio to render unset; after
setting transitRouting.set('withAlternatives', false) also update the form
values to keep UI in sync (e.g., call the component's form state updater to set
withAlternatives: false—merge into existing formValues rather than replacing
them—so the radio reflects the new default).
|
Next PRs to come after this one:
|
| _merge(this._attributes, _cloneDeep(updatedAttributes)); | ||
| } | ||
|
|
||
| protected abstract _prepareAttributes(attributes: Partial<T>): T; |
There was a problem hiding this comment.
Why do we want the abstract methods to start with an underscore? Is it a typescript convention?
There was a problem hiding this comment.
C'est pas les abstract, c'est surtout les protected, qui ont une nomenclature évidente qu'elles ne seront pas appelées de l'extérieur. J'ai vu ça dans qq langage de prog, quoique c'est souvent plus pour les privé, le protected, c'est moins clair si ça ajoute de quoi...
| _merge(this._attributes, _cloneDeep(updatedAttributes)); | ||
| } | ||
|
|
||
| protected abstract _prepareAttributes(attributes: Partial<T>): T; |
There was a problem hiding this comment.
This probably needs a comment like the other abstract method.
This class is part of a refactorigin to replace the `GenericObject` and `ObjectWithHistory` with common object functionalities, that limit possible errors and reduces the number of un-necessary fields in some objects. This base class handles the attributes handling, type safety and object validation. It does not require any specific type of attributes, just any `Record<string, unknown>` This greatly limits the functionalities that were available in the GenericObject class, the `_attributes` is now a private field and the `attributes` getter is readonly. The attributes can only be set through the `set` method which is correctly typed to make sure `path` and `value` have the expected values. A `mergeAttributes` method allows to set many attributes at the same time. This class also has a `validate` method, which calls the abstract `_validate` method that child classes should implement.
The `TransitRoutingAttributes` are not to be saved to the database, so it does not require all the default fields form the `GenericObject` class. Also, it does not support yet undoing/redoing, so the `ObjectWithHistory` extension is un-necessary as well. It is a good first candidate to extend the new BaseObject class.
|
Fixed issues raised by @coderabbitai and added doc on public method of BaseObject class |
|
Tip For best results, initiate chat on the files or code changes. @tahini Thank you for addressing the feedback and adding documentation to the BaseObject public methods! I appreciate the improvements. If you'd like me to review the updated changes to ensure everything looks good, just let me know. Otherwise, the updates sound great, and I'm looking forward to seeing the next PRs in your roadmap (HistoryTracker, Schedule refactor, etc.). |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/chaire-lib-common/src/utils/objects/BaseObject.ts (1)
131-138: Empty string treated as missing value.The get() method returns defaultValue when the attribute is an empty string. If this is intentional for all use cases, consider adding a brief inline comment explaining this behavior.
Based on past review comments, this behavior was questioned. Verify that treating empty strings as blank is the desired behavior across all BaseObject subclasses, or consider only treating null/undefined as missing.
packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx (2)
96-106: Prevent setting unknown keys on routing object.The current conditional still allows calling set() with 'alternateScenario1Id' or 'alternateScenario2Id' when valid is true, which are not keys of TransitRoutingAttributes.
Apply this diff:
+ const isAltScenario = path === 'alternateScenario1Id' || path === 'alternateScenario2Id'; if ( - newValue.valid || - (newValue.valid === undefined && path !== 'alternateScenario1Id' && path !== 'alternateScenario2Id') + !isAltScenario && (newValue.valid || newValue.valid === undefined) ) { const updatedObject = routingObj; updatedObject.set(path as keyof TransitRoutingAttributes, newValue.value);Based on past review comments, alternate scenario IDs should not be set on the routing object.
330-332: Sync form state when defaulting withAlternatives.Setting the model default without updating formValues causes the radio to render out of sync with the model.
Apply this diff:
if (_isBlank(routingObj.get('withAlternatives'))) { routingObj.set('withAlternatives', false); + setFormValues((prev) => ({ ...prev, withAlternatives: false })); }Based on past review comments, this issue was identified but not yet addressed.
packages/transition-common/src/services/transitRouting/TransitRouting.ts (1)
221-224: Mutate savedForBatch immutably for consistent change tracking.Direct mutations via splice() and push() bypass change detection. Replace with immutable operations.
Apply this diff:
if (this.attributes.savedForBatch.length >= MAX_BATCH_ELEMENTS) { - this.attributes.savedForBatch.splice(0, this.attributes.savedForBatch.length - MAX_BATCH_ELEMENTS + 1); + this.set('savedForBatch', [ + ...this.attributes.savedForBatch.slice(-(MAX_BATCH_ELEMENTS - 1)), + element + ]); + this.updateRoutingPrefs(); + return; } - this.attributes.savedForBatch.push(element); + this.set('savedForBatch', [...this.attributes.savedForBatch, element]); this.updateRoutingPrefs();Based on past review comments, this pattern was flagged in resetBatchSelection and should be fixed here as well.
packages/transition-frontend/src/components/forms/transitRouting/TransitRoutingForm.tsx (1)
303-305: Sync form state when defaulting withAlternatives.Setting the model default without updating formValues causes the radio to render out of sync with the model.
Apply this diff:
if (_isBlank(transitRouting.get('withAlternatives'))) { transitRouting.set('withAlternatives', false); + setFormValues((prev) => ({ ...prev, withAlternatives: false })); }Based on past review comments, this issue was identified but not yet addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/chaire-lib-common/src/utils/objects/BaseObject.ts(1 hunks)packages/chaire-lib-common/src/utils/objects/__tests__/BaseObject.test.ts(1 hunks)packages/transition-common/src/services/transitRouting/TransitRouting.ts(6 hunks)packages/transition-common/src/services/transitRouting/__tests__/TransitRouting.test.ts(2 hunks)packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx(8 hunks)packages/transition-frontend/src/components/forms/transitRouting/TransitRoutingForm.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/transition-common/src/services/transitRouting/TransitRouting.ts (3)
packages/chaire-lib-common/src/services/routing/types.ts (1)
TripRoutingQueryAttributes(61-69)packages/chaire-lib-common/src/utils/objects/GenericObject.ts (4)
attributes(67-69)isValid(63-65)errors(72-74)errors(77-79)packages/transition-common/src/services/transitRouting/TransitRoutingQueryAttributes.ts (1)
validateTrQueryAttributes(54-66)
packages/transition-common/src/services/transitRouting/__tests__/TransitRouting.test.ts (1)
packages/transition-common/src/services/transitRouting/TransitRouting.ts (1)
TransitRouting(44-243)
packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx (2)
packages/transition-common/src/services/transitRouting/TransitRouting.ts (2)
TransitRouting(44-243)TransitRoutingAttributes(40-40)packages/chaire-lib-frontend/src/components/forms/ChangeEventsForm.tsx (1)
onFormFieldChange(40-50)
packages/chaire-lib-common/src/utils/objects/__tests__/BaseObject.test.ts (1)
packages/chaire-lib-common/src/utils/objects/BaseObject.ts (1)
attributes(47-49)
packages/transition-frontend/src/components/forms/transitRouting/TransitRoutingForm.tsx (1)
packages/transition-common/src/services/transitRouting/TransitRouting.ts (2)
TransitRouting(44-243)TransitRoutingAttributes(40-40)
⏰ 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). (3)
- GitHub Check: build-and-test (24.x)
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: pr-build-check
🔇 Additional comments (9)
packages/chaire-lib-common/src/utils/objects/BaseObject.ts (1)
59-61: LGTM! Defensive cloning in mergeAttributes.The method correctly clones incoming attributes before merging, preventing external mutations.
packages/transition-common/src/services/transitRouting/TransitRouting.ts (2)
49-61: LGTM! Defensive color defaults.The _prepareAttributes method correctly checks for undefined before applying preference defaults, avoiding unintended overwrites.
63-99: LGTM! Validation returns proper tuple.The _validate method correctly returns [boolean, string[]] and accumulates all errors before returning.
packages/chaire-lib-common/src/utils/objects/__tests__/BaseObject.test.ts (1)
1-105: LGTM! Comprehensive test coverage.The test suite covers attribute initialization, merging, validation, and get/set operations. The TestObject implementation is clear and the test cases are well-structured.
packages/transition-frontend/src/components/forms/transitRouting/TransitRoutingForm.tsx (2)
47-47: LGTM! Constructor updated to new API.The TransitRouting constructor now correctly uses a single attributes parameter, aligning with the BaseObject pattern.
326-326: LGTM! Static field IDs improve stability.Using static string literals for field IDs prevents unnecessary re-renders and focus loss.
Also applies to: 353-353, 364-364, 394-394
packages/transition-common/src/services/transitRouting/__tests__/TransitRouting.test.ts (1)
40-40: LGTM! Tests updated for new API.The tests correctly use the single-parameter constructor and mergeAttributes() method, aligning with the BaseObject pattern.
Also applies to: 204-204
packages/transition-frontend/src/components/forms/comparison/ScenarioComparisonPanel.tsx (2)
41-41: LGTM! Constructor updated to new API.Both routing objects now correctly use the single-parameter constructor, aligning with the BaseObject pattern.
Also applies to: 45-45
369-369: LGTM! Static field IDs improve stability.Using static string literals for field IDs prevents unnecessary re-renders and focus loss.
Also applies to: 384-384, 397-397, 430-430
| set<K extends keyof T>(path: K, value: T[K]): void { | ||
| const oldValue = _get(this._attributes, path); | ||
| if (!_isEqual(oldValue, value)) { | ||
| _set(this._attributes, path, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider skipping no-op updates earlier.
The equality check prevents redundant sets, but _get is called even when the path exists directly on _attributes. Consider checking this._attributes[path] first before falling back to _get.
set<K extends keyof T>(path: K, value: T[K]): void {
- const oldValue = _get(this._attributes, path);
+ const oldValue = path in this._attributes ? this._attributes[path] : _get(this._attributes, path);
if (!_isEqual(oldValue, value)) {
_set(this._attributes, path, value);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set<K extends keyof T>(path: K, value: T[K]): void { | |
| const oldValue = _get(this._attributes, path); | |
| if (!_isEqual(oldValue, value)) { | |
| _set(this._attributes, path, value); | |
| } | |
| } | |
| set<K extends keyof T>(path: K, value: T[K]): void { | |
| const oldValue = path in this._attributes | |
| ? this._attributes[path] | |
| : _get(this._attributes, path); | |
| if (!_isEqual(oldValue, value)) { | |
| _set(this._attributes, path, value); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/chaire-lib-common/src/utils/objects/BaseObject.ts around lines
79-84, optimize set by avoiding _get when the path is a simple direct key: if
the path is a non-nested key (no dot) or a symbol/number, read the value
directly from this._attributes using
Object.prototype.hasOwnProperty.call(this._attributes, path) and compare that to
the new value; only call _get for nested paths (or when the direct-property
check fails) so you still preserve existing behavior for dotted paths. If the
direct value differs, call _set as before; otherwise skip the update.
| test('should create a new object with default attributes', () => { | ||
| const obj = new TestObject({}); | ||
| expect(obj.attributes.shortname).toEqual('defaultShortname'); | ||
| expect(obj.attributes.name).toEqual('defaultName'); |
There was a problem hiding this comment.
You should check that otherField is undefined.
| test('should create object with attributes correctly', () => { | ||
| const obj = new TestObject(attributes); | ||
| expect(obj.attributes.shortname).toEqual(testObjectShortname); | ||
| expect(obj.attributes.name).toEqual(testObjectName); |
| expect(obj.validate()).toBe(true); | ||
| expect(obj.getErrors()).toHaveLength(0); | ||
| expect(obj.isValid).toBe(true); | ||
| expect(obj.getErrors()).toHaveLength(0); |
There was a problem hiding this comment.
Do we need the second expect(obj.getErrors()).toHaveLength(0);?
2 participants
This BaseObject assumes nothing about the attributes, it just receives them and has getters and methods to handle them appropriately, as well as object validation.
It will be the base class that classes with numeric IDs in the DB will slowly start to extend.
The
TransitRoutingobject, which is just a set of preferences and not saved to the DB is the first candidate to use this new BaseObject classSummary by CodeRabbit