Import dissemination block task now works with french file#1813
Conversation
WalkthroughAdds language-variant handling for the DBUID tag by introducing a list of acceptable names and using it to locate the DBUID element instead of direct name matching. Adds a guard that throws if no DBUID element is found, reads the DBUID value from the chosen element's first child, replaces some console.log calls with console.error for GML parse errors, and changes stream error rejection to include the error object. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transition-backend/src/services/importers/ZonesImporter.ts (1)
122-130:⚠️ Potential issue | 🔴 CriticalRemove
throw errorfrom un-awaitedqueue.add()callbacks to prevent unhandled promise rejections.Both
queue.add()calls are fire-and-forget (not awaited or handled with.catch()). When the async callback throws, it creates an unhandled promise rejection that can crash the process in Node.js 15+. SincereadStream.destroy(error as Error)already propagates the error via the stream's'error'event, the rethrow is redundant—just remove it.Fix
queue.add(async () => { try { await zonesQueries.addZonesAndConvertedGeography(currentBatch); } catch (error) { console.error(`Error adding zones batch to db: ${error}`); readStream.destroy(error as Error); - throw error; } });Also applies to: 146–154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/importers/ZonesImporter.ts` around lines 122 - 130, In ZonesImporter.ts, remove the redundant "throw error" from the async callbacks passed to queue.add (the ones that call zonesQueries.addZonesAndConvertedGeography(currentBatch) and the second similar block) so they don't create unhandled promise rejections; instead keep the existing console.error and call readStream.destroy(error as Error) to propagate the failure via the stream's 'error' event (you can optionally log additional context), but do not rethrow from the queue.add callback.
🤖 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-backend/src/services/importers/ZonesImporter.ts`:
- Line 58: The current one-liner uses an unsafe cast "(... as XmlNode)" when
calling fme.children.find(...), which can be undefined; update the code in
ZonesImporter (where dbuid is computed using dbuidPredicate and fme.children) to
first assign the result of fme.children.find(...) to a properly typed variable
(e.g., const dbuidNode: XmlNode | undefined), check for undefined and throw or
log a clear error mentioning the missing dbuid tag (including context such as
the FME node id or name), and only then access dbuidNode.children[0].value to
set dbuid; this preserves type-safety and avoids the runtime TypeError.
- Around line 163-167: The stream 'error' handler in ZonesImporter.ts currently
logs with console.log, calls reject() with no argument, and then throws the
error; change it to log with console.error (matching other error sites), call
reject(err) so the caller receives the error, and remove the subsequent throw
err to avoid uncaught exceptions from the EventEmitter handler (i.e., update the
readStream.on('error', (err) => { ... }) handler to console.error(...),
reject(err) and no throw).
- Around line 137-141: The catch block inside the parser.on listener (in
ZonesImporter.ts) must not rethrow the caught error because throwing inside the
sync listener can bubble out of parser.parse() and become an uncaught exception;
instead remove the `throw error;` and rely on `readStream.destroy(error as
Error)` to propagate the error to the stream's 'error' handler (or alternatively
call the enclosing promise's reject if present). Update the catch in the parser
listener to log the error, call `readStream.destroy(error as Error)`, and then
exit the handler without rethrowing.
---
Outside diff comments:
In `@packages/transition-backend/src/services/importers/ZonesImporter.ts`:
- Around line 122-130: In ZonesImporter.ts, remove the redundant "throw error"
from the async callbacks passed to queue.add (the ones that call
zonesQueries.addZonesAndConvertedGeography(currentBatch) and the second similar
block) so they don't create unhandled promise rejections; instead keep the
existing console.error and call readStream.destroy(error as Error) to propagate
the failure via the stream's 'error' event (you can optionally log additional
context), but do not rethrow from the queue.add callback.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/transition-backend/src/services/importers/ZonesImporter.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). (5)
- GitHub Check: pr-build-check
- 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)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-backend/src/services/importers/ZonesImporter.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-backend/src/services/importers/ZonesImporter.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1710
File: packages/chaire-lib-backend/src/models/db/zones.db.queries.ts:206-215
Timestamp: 2026-01-20T22:00:37.310Z
Learning: In packages/chaire-lib-backend/src/models/db/zones.db.queries.ts, the ST_MAKEVALID function is used without ST_CollectionExtract wrapping when converting zone boundaries to geography. The current census/zone data being imported does not produce GeometryCollections, so the potential casting issue is deferred for future consideration if different data sources are used.
📚 Learning: 2026-01-20T22:00:37.310Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1710
File: packages/chaire-lib-backend/src/models/db/zones.db.queries.ts:206-215
Timestamp: 2026-01-20T22:00:37.310Z
Learning: In packages/chaire-lib-backend/src/models/db/zones.db.queries.ts, the ST_MAKEVALID function is used without ST_CollectionExtract wrapping when converting zone boundaries to geography. The current census/zone data being imported does not produce GeometryCollections, so the potential casting issue is deferred for future consideration if different data sources are used.
Applied to files:
packages/transition-backend/src/services/importers/ZonesImporter.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-backend/src/services/importers/ZonesImporter.ts
🔇 Additional comments (1)
packages/transition-backend/src/services/importers/ZonesImporter.ts (1)
196-199: LGTM — clean and well-documented fix for the French GML tag.The predicate neatly centralises both tag variants and the comment clearly explains the rationale.
packages/transition-backend/src/services/importers/ZonesImporter.ts
Outdated
Show resolved
Hide resolved
GabrielBruno24
force-pushed
the
fixFrenchImportTask
branch
from
2b76760 to
470dcd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-backend/src/services/importers/ZonesImporter.ts`:
- Around line 58-63: The code assumes dbuidElement.children[0].value always
exists which can raise a TypeError; in ZonesImporter.ts, after obtaining
dbuidElement (the result of dbuidPredicate), validate that dbuidElement.children
is non-empty and that the first child has a defined value (or find the first
child with a value) before assigning dbuid, and if absent throw a clear Error
(e.g., "DBUID element has no text child") so failures are explicit and
debuggable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/transition-backend/src/services/importers/ZonesImporter.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-backend/src/services/importers/ZonesImporter.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-backend/src/services/importers/ZonesImporter.ts
🧠 Learnings (2)
📚 Learning: 2026-01-20T22:00:37.310Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1710
File: packages/chaire-lib-backend/src/models/db/zones.db.queries.ts:206-215
Timestamp: 2026-01-20T22:00:37.310Z
Learning: In packages/chaire-lib-backend/src/models/db/zones.db.queries.ts, the ST_MAKEVALID function is used without ST_CollectionExtract wrapping when converting zone boundaries to geography. The current census/zone data being imported does not produce GeometryCollections, so the potential casting issue is deferred for future consideration if different data sources are used.
Applied to files:
packages/transition-backend/src/services/importers/ZonesImporter.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/services/importers/ZonesImporter.ts
🔇 Additional comments (3)
packages/transition-backend/src/services/importers/ZonesImporter.ts (3)
141-144: Good: error logging is now consistent in the parser catch.
165-167: Good: stream errors now reject with the real error.
197-201: Nice: predicate helper cleanly supports French/English tags.
packages/transition-backend/src/services/importers/ZonesImporter.ts
Outdated
Show resolved
Hide resolved
packages/transition-backend/src/services/importers/ZonesImporter.ts
Outdated
Show resolved
Hide resolved
Due to one of the tags being translated, the import would fail with the french version of the file. This also changes error handling so that the task will fail properly is an error happens during the import. Fix: chairemobilite#1780
GabrielBruno24
force-pushed
the
fixFrenchImportTask
branch
from
470dcd1 to
1a66fd9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/transition-backend/src/services/importers/ZonesImporter.ts (1)
62-67:⚠️ Potential issue | 🟡 MinorPlease guard missing DBUID text before reading
children[0].value.Line 67 can still throw a generic
TypeErrorif the DBUID node exists but has no text child. A small guard keeps failures explicit and easier to debug.Suggested fix
- const dbuid = dbuidElement.children[0].value; + const dbuidChild = dbuidElement.children[0]; + if (dbuidChild === undefined || dbuidChild.value === undefined) { + throw new Error('DBUID element has no text child in zone feature.'); + } + const dbuid = dbuidChild.value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/importers/ZonesImporter.ts` around lines 62 - 67, The code in ZonesImporter.ts assumes dbuidElement.children[0].value exists which can throw a TypeError if the DBUID node has no text child; update the extraction after locating dbuidElement by verifying a text child exists (e.g., find a child with a value or check children.length and children[0].value) and throw a clear Error like "DBUID element has no text child" if not present; keep the checks around the variables dbuidElement, dbuidElement.children, and the resolved dbuid to make failures explicit and easier to debug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/transition-backend/src/services/importers/ZonesImporter.ts`:
- Around line 62-67: The code in ZonesImporter.ts assumes
dbuidElement.children[0].value exists which can throw a TypeError if the DBUID
node has no text child; update the extraction after locating dbuidElement by
verifying a text child exists (e.g., find a child with a value or check
children.length and children[0].value) and throw a clear Error like "DBUID
element has no text child" if not present; keep the checks around the variables
dbuidElement, dbuidElement.children, and the resolved dbuid to make failures
explicit and easier to debug.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/transition-backend/src/services/importers/ZonesImporter.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). (3)
- GitHub Check: build-and-test (24.x)
- GitHub Check: pr-build-check
- GitHub Check: build-and-run-UI-tests (24.x)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-backend/src/services/importers/ZonesImporter.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-backend/src/services/importers/ZonesImporter.ts
🧠 Learnings (2)
📚 Learning: 2026-01-20T22:00:37.310Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1710
File: packages/chaire-lib-backend/src/models/db/zones.db.queries.ts:206-215
Timestamp: 2026-01-20T22:00:37.310Z
Learning: In packages/chaire-lib-backend/src/models/db/zones.db.queries.ts, the ST_MAKEVALID function is used without ST_CollectionExtract wrapping when converting zone boundaries to geography. The current census/zone data being imported does not produce GeometryCollections, so the potential casting issue is deferred for future consideration if different data sources are used.
Applied to files:
packages/transition-backend/src/services/importers/ZonesImporter.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/services/importers/ZonesImporter.ts
🔇 Additional comments (3)
packages/transition-backend/src/services/importers/ZonesImporter.ts (3)
16-19: Nice bilingual tag handling.Line 18 cleanly centralizes accepted DBUID tags and makes future variants easy to extend.
146-147: Good parser error path.Line 147 correctly destroys the stream after logging, so the task now fails through the promise rejection flow.
170-171: Stream error handling is now correct.Line 171 preserving the original error in
reject(err)is a solid fix for debugging and task failure reporting.
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
J'imagine que tu as bien testé l'import complet dans les 2 langues et que tout fonctionne?
greenscientist
left a comment
greenscientist
left a comment
There was a problem hiding this comment.
Est-ce qu'il y a un impact sur le temps d'importation du fichier ou bien c'est négligeable?
Yes |
3 participants
Due to one of the tags being translated, the import would fail with the french version of the file. This also changes error handling so that the task will fail properly is an error happens during the import.
This increases the task length by about 10 seconds, for a total ~300 seconds.
Fix: #1780
Summary by CodeRabbit
New Features
Bug Fixes