ENG-534 fix broken tcm encounter pagination #4098
Merged
+460
−370
Conversation
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
WalkthroughThe changes refactor TCM encounter querying to use raw SQL queries instead of Sequelize ORM methods, introducing new types, DTO transformation logic, and updated pagination sorting. The API route now returns transformed DTOs, and related tests are updated or added to validate the new raw SQL logic, pagination, and DTO mapping. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Route
participant CommandLayer
participant Sequelize
participant Database
Client->>API_Route: GET /medical/tcm-encounter
API_Route->>CommandLayer: getTcmEncounters({cxId, after, pagination})
CommandLayer->>Sequelize: query(raw SQL with join, filters, pagination)
Sequelize->>Database: Execute SQL
Database-->>Sequelize: Result rows (with embedded patient data)
Sequelize-->>CommandLayer: Query result
CommandLayer->>API_Route: Mapped results (with patientData separated)
API_Route->>API_Route: Map results to DTOs
API_Route-->>Client: Paginated DTO response
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERR_SSL_WRONG_VERSION_NUMBER ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
e215e77 to
2356325
Compare
metriport/metriport-internal#1040 Ref: ENG-534 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-534 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
e090c5c to
a6fdde7
Compare
RamilGaripov
previously approved these changes
Jun 26, 2025
There was a problem hiding this comment.
for unit tests, we usually use dynamic values on patientId / cxId using faker.string.uuid()
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (2)
70-101: Raw SQL implementation is secure and well-structured.The query properly uses parameterized replacements to prevent SQL injection. The conditional fragments for pagination are cleanly implemented.
Consider adding error handling for query execution failures:
- return sequelize.query<TcmEncounterQueryResult>(queryString, { + try { + return await sequelize.query<TcmEncounterQueryResult>(queryString, { replacements: { cxId, ...(toItem ? { toItem } : {}), ...(fromItem ? { fromItem } : {}), ...(count ? { count } : { count: defaultPageSize }), ...{ afterDate: after ? buildDayjs(after).toDate() : DEFAULT_FILTER_DATE }, }, type: QueryTypes.SELECT, }); + } catch (error) { + throw new Error(`Failed to query TCM encounters: ${error.message}`, { cause: error }); + }
70-127: Consider documenting the rationale for raw SQL approach.While the raw SQL implementation is well-executed, it represents a significant departure from the ORM pattern. Given the PR objective mentions this fixes a pagination bug, consider:
- Adding a comment explaining why raw SQL was necessary here
- Documenting any Sequelize limitations that prevented the ORM approach
- Ensuring this pattern doesn't proliferate unnecessarily to other parts of the codebase
This will help future maintainers understand the architectural decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts(2 hunks)packages/api/src/routes/medical/__tests__/tcm-encounter.test.ts(8 hunks)packages/api/src/shared/sql.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column ...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
📄 Source: CodeRabbit Inference Engine (.cursorrules)
List of files the instruction was applied to:
packages/api/src/shared/sql.tspackages/api/src/routes/medical/__tests__/tcm-encounter.test.tspackages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try ...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/api/src/shared/sql.tspackages/api/src/routes/medical/__tests__/tcm-encounter.test.tspackages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: lucasdellabella
PR: metriport/metriport#3588
File: packages/infra/lib/hl7-notification-stack/vpn.ts:11-15
Timestamp: 2025-04-02T22:25:52.792Z
Learning: User lucasdellabella prefers to handle certain code consistency issues (like mismatches between interface extension and class inheritance) in separate PRs rather than immediately fixing them in the current release PR.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/__tests__/hl7v2-roster-generator.test.ts:190-191
Timestamp: 2025-05-27T15:49:16.573Z
Learning: The user lucasdellabella prefers to keep changes focused on the PR's scope and avoid making ancillary performance improvements (like replacing delete operators with undefined assignments) that are not directly related to the main objectives of the current PR.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-03-05T18:43:30.389Z
Learning: For backmerge PRs at metriport/metriport, only verify two things: (1) that the source branch is `master` and destination branch is `develop`, and (2) that there's a link to at least one PR (usually a "patch" PR) in the description. No need for detailed review comments or updates to the PR description unless there's an issue with these criteria.
Learnt from: thomasyopes
PR: metriport/metriport#3883
File: packages/core/src/external/ehr/shared.ts:414-428
Timestamp: 2025-05-28T19:49:26.364Z
Learning: In the Metriport codebase, thomasyopes prefers to address potential stack overflow issues in pagination functions reactively (when they actually occur) rather than proactively refactoring recursive implementations to iterative ones.
Learnt from: leite08
PR: metriport/metriport#3624
File: packages/api/src/command/medical/patient/get-patient.ts:90-97
Timestamp: 2025-04-07T17:32:22.935Z
Learning: When dealing with raw SQL queries in Sequelize that return complex data structures (especially with joins and JSON aggregation), using `any` type assertions may be necessary since TypeScript cannot infer the types. Using constants for column aliases helps minimize maintenance issues in these cases.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc. The migration files properly define all FK constraints, NOT NULL requirements, and cascade behaviors.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-28T22:58:19.669Z
Learning: For Sequelize models in this codebase, field constraints (such as `allowNull: false`) and field mappings (such as `field: "column_name"`) are defined in the migrations, not in the model definitions themselves.
Learnt from: leite08
PR: metriport/metriport#3412
File: packages/api/src/models/medical/patient-import.ts:49-83
Timestamp: 2025-04-24T02:24:47.642Z
Learning: For the Metriport codebase, database constraints like `allowNull: false`, default values, and enum validations are defined in the database migration files, not in the Sequelize model definition files.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/command/medical/cohort/patient-cohort/patient-cohort.ts:0-0
Timestamp: 2025-06-20T03:15:43.544Z
Learning: In Node.js applications using Sequelize ORM, the `bulkCreate` method executes as a single SQL command even for large batches (confirmed for 1500+ records), providing database-level atomicity without needing explicit transaction wrappers for simple bulk insertions.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/command/medical/patient/patient-import/update-tracking.ts:70-84
Timestamp: 2025-04-28T23:01:39.688Z
Learning: In the Metriport codebase, it's acceptable to include a `where` clause when calling Sequelize's instance update method (like `job.update(jobToUpdate, { where: { cxId, id: jobId } })`), even though Sequelize silently ignores this clause when called on an instance rather than the model static method.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/cohort.ts:12-26
Timestamp: 2025-06-17T23:06:20.383Z
Learning: In the Metriport codebase, Sequelize models can work without explicit field mappings from camelCase to snake_case (e.g., cxId attribute can map to cx_id column without specifying field: "cx_id"). The user confirmed this works in their configuration.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/cohort.ts:0-0
Timestamp: 2025-06-17T23:10:31.779Z
Learning: In the Metriport codebase, foreign key constraints including cascade behaviors are defined in Sequelize migrations rather than being duplicated in the model definitions. The model files may have commented out constraint definitions that serve as documentation but the actual constraints are enforced at the database level through migrations.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:63-63
Timestamp: 2025-05-27T16:10:53.952Z
Learning: User lucasdellabella is okay with using JSON.stringify in log statements in TypeScript files, even though the coding guidelines suggest avoiding multi-line logs with JSON.stringify. They prefer to keep such logging as is when it serves the purpose effectively.
Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.
Learnt from: lucasdellabella
PR: metriport/metriport#3596
File: packages/core/src/util/base64-scrambler.ts:40-48
Timestamp: 2025-04-03T15:46:16.447Z
Learning: Lucas prefers to keep non-null assertions in the Base64Scrambler class rather than using null coalescing and additional conditionals.
packages/api/src/shared/sql.ts (2)
Learnt from: leite08
PR: metriport/metriport#3924
File: packages/utils/src/open-search/ingest-patients.ts:23-25
Timestamp: 2025-05-30T14:18:31.953Z
Learning: In this TypeScript codebase, global constants can use SCREAMING_SNAKE_CASE (all caps) naming convention, even though the general guideline is camelCase for constants, variables, and functions. This is an accepted exception for global constants.
Learnt from: leite08
PR: metriport/metriport#3857
File: packages/core/src/external/opensearch/paginate.ts:0-0
Timestamp: 2025-05-27T23:52:23.553Z
Learning: In the Metriport codebase, for OpenSearch pagination implementations, unlimited pagination is preferred for data processing pipelines. Users need to be able to load as many pages as needed without artificial iteration limits, relying on OpenSearch's natural termination conditions.
packages/api/src/routes/medical/__tests__/tcm-encounter.test.ts (16)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc. The migration files properly define all FK constraints, NOT NULL requirements, and cascade behaviors.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: lucasdellabella
PR: metriport/metriport#3907
File: packages/core/src/external/fhir/adt-encounters.ts:118-143
Timestamp: 2025-05-28T02:32:27.527Z
Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/routes/medical/patient.ts:541-543
Timestamp: 2025-06-18T21:05:22.256Z
Learning: In packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortIdSchema are acceptable and don't need to be moved to separate schema files when the user prefers to keep them inline.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: lucasdellabella
PR: metriport/metriport#4059
File: packages/api/src/command/medical/tcm-encounter/create-sample-tcm-encounter.ts:6-12
Timestamp: 2025-06-19T20:30:28.388Z
Learning: In sample data generation functions like createSampleTcmEncounters, it's preferred to keep facility names and similar test data inline within the function rather than extracting them as constants, as they are only used locally and don't need to be shared.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-28T22:58:19.669Z
Learning: For Sequelize models in this codebase, field constraints (such as `allowNull: false`) and field mappings (such as `field: "column_name"`) are defined in the migrations, not in the model definitions themselves.
Learnt from: leite08
PR: metriport/metriport#3953
File: packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts:28-30
Timestamp: 2025-06-03T21:02:23.374Z
Learning: The `makePatient()` function returns `PatientWithId` type which requires the `id` field to be present, ensuring `patient.id` is never undefined.
Learnt from: leite08
PR: metriport/metriport#3648
File: packages/utils/src/shared/patient-create.ts:35-53
Timestamp: 2025-04-10T17:30:30.368Z
Learning: The function `mergePatients` in the patient-create.ts file is intentionally designed to take all properties from the first patient (p1) and only update address and contact fields, as the system supports duplicated patients specifically to allow multiple addresses across different records.
Learnt from: leite08
PR: metriport/metriport#3942
File: packages/core/src/external/fhir/__tests__/location.ts:6-9
Timestamp: 2025-06-01T13:48:26.600Z
Learning: In TypeScript test code, don't flag deconstructed object parameters as needing simplification or refactoring. Test helper functions with nested optional object parameters (like `param?: { patient?: PatientWithId; location?: Partial<Location>; }`) are acceptable and should not be suggested for flattening.
Learnt from: thomasyopes
PR: metriport/metriport#3997
File: packages/api/src/routes/internal/medical/patient-job.ts:28-29
Timestamp: 2025-06-13T20:24:16.668Z
Learning: In packages/api/src/routes/internal/medical/patient-job.ts, mounting patientJobsRouter at the root path before other route handlers is intentional behavior, even though it could potentially consume requests before later handlers.
Learnt from: thomasyopes
PR: metriport/metriport#3960
File: packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts:11-19
Timestamp: 2025-06-05T16:28:33.728Z
Learning: In the EhrGetAppointmentsDirect class in packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts, type casting `as T[]` for the handler return value is considered safe and doesn't require additional runtime validation by the user thomasyopes.
Learnt from: leite08
PR: metriport/metriport#3624
File: packages/api/src/command/medical/patient/get-patient.ts:90-97
Timestamp: 2025-04-07T17:32:22.935Z
Learning: When dealing with raw SQL queries in Sequelize that return complex data structures (especially with joins and JSON aggregation), using `any` type assertions may be necessary since TypeScript cannot infer the types. Using constants for column aliases helps minimize maintenance issues in these cases.
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (16)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: leite08
PR: metriport/metriport#3624
File: packages/api/src/command/medical/patient/get-patient.ts:90-97
Timestamp: 2025-04-07T17:32:22.935Z
Learning: When dealing with raw SQL queries in Sequelize that return complex data structures (especially with joins and JSON aggregation), using `any` type assertions may be necessary since TypeScript cannot infer the types. Using constants for column aliases helps minimize maintenance issues in these cases.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: thomasyopes
PR: metriport/metriport#3960
File: packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts:11-19
Timestamp: 2025-06-05T16:28:33.728Z
Learning: In the EhrGetAppointmentsDirect class in packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts, type casting `as T[]` for the handler return value is considered safe and doesn't require additional runtime validation by the user thomasyopes.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/routes/medical/patient.ts:541-543
Timestamp: 2025-06-18T21:05:22.256Z
Learning: In packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortIdSchema are acceptable and don't need to be moved to separate schema files when the user prefers to keep them inline.
Learnt from: lucasdellabella
PR: metriport/metriport#3907
File: packages/core/src/external/fhir/adt-encounters.ts:118-143
Timestamp: 2025-05-28T02:32:27.527Z
Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.
Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/command/medical/patient/patient-import/update-tracking.ts:70-84
Timestamp: 2025-04-28T23:01:39.688Z
Learning: In the Metriport codebase, it's acceptable to include a `where` clause when calling Sequelize's instance update method (like `job.update(jobToUpdate, { where: { cxId, id: jobId } })`), even though Sequelize silently ignores this clause when called on an instance rather than the model static method.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc. The migration files properly define all FK constraints, NOT NULL requirements, and cascade behaviors.
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/core/src/external/surescripts/client.ts:0-0
Timestamp: 2025-05-30T01:09:55.013Z
Learning: In packages/core/src/external/surescripts/client.ts, the Surescripts date formatting should use local time according to their documentation ("message-date-must-be-local"), which means buildDayjs() cannot be used since it defaults to UTC. The specific use case for transmission date formatting requires dayjs to be used directly to preserve local time behavior.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:23-25
Timestamp: 2025-06-18T08:03:09.215Z
Learning: In the Metriport codebase, composite unique constraints are defined in migrations only, not at the Sequelize model level. Models use individual field constraints (unique: true) at the model level, but composite constraints are handled via migrations using uniqueKeys in createTable options or addConstraint. Models consistently use `...BaseModel.modelOptions(sequelize)` and `tableName: ModelName.NAME` without defining indexes arrays.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-28T22:58:19.669Z
Learning: For Sequelize models in this codebase, field constraints (such as `allowNull: false`) and field mappings (such as `field: "column_name"`) are defined in the migrations, not in the model definitions themselves.
Learnt from: leite08
PR: metriport/metriport#3412
File: packages/api/src/models/medical/patient-import.ts:49-83
Timestamp: 2025-04-24T02:24:47.642Z
Learning: For the Metriport codebase, database constraints like `allowNull: false`, default values, and enum validations are defined in the database migration files, not in the Sequelize model definition files.
🧬 Code Graph Analysis (1)
packages/api/src/routes/medical/__tests__/tcm-encounter.test.ts (2)
packages/api/src/models/medical/tcm-encounter.ts (1)
TcmEncounterModel(7-87)packages/api/src/models/medical/patient.ts (1)
PatientModel(8-35)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/api/src/shared/sql.ts (1)
3-3: LGTM!Exporting
defaultPageSizemakes it accessible for the raw SQL implementation inget-tcm-encounters.ts.packages/api/src/routes/medical/__tests__/tcm-encounter.test.ts (2)
17-32: Test setup correctly adapted for raw SQL queries.The mock setup appropriately configures the sequelize instance and table names needed for the raw SQL implementation.
150-189: Test cases properly validate raw SQL implementation.The test updates correctly verify:
- SQL query structure with JOIN and WHERE clauses
- Parameterized replacements for security
- Consistent default date filter (2020-01-01)
- Patient data extraction from query results
Also applies to: 191-231, 233-285, 287-341
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (2)
25-36: Type definitions properly support raw SQL results.The
TcmEncounterQueryResulttype provides good type safety for the raw query results, and the extendedTcmEncounterForDisplayfields align with the patient data extraction logic.
38-61: Main function implementation looks good.The patient data extraction properly handles nested structures with appropriate null safety using optional chaining and nullish coalescing.
| if (!sequelize) throw new Error("Sequelize not found"); | ||
| const { toItem, fromItem, count } = pagination ?? {}; | ||
|
|
||
| const queryString = ` |
There was a problem hiding this comment.
i thought we're trying to use sequelize methods more, but maybe this is fine.. I actually do find these easier to follow, but maybe it's just from inexperience with Op methods
There was a problem hiding this comment.
specifically for pagination our current pattern is to try and write the stuff ourselves because of how we do pagination.
metriport/metriport-internal#1040 Ref: ENG-534 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (1)
62-62: Count parsing follows established codebase patterns.Based on previous discussions, this parsing approach is consistent with existing codebase patterns for handling database count results.
🧹 Nitpick comments (1)
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (1)
74-76: Add defensive null check for better error handling.The error handling for missing Sequelize instance is good, but consider making the error message more descriptive for debugging.
- if (!sequelize) throw new Error("Sequelize not found"); + if (!sequelize) throw new Error("Sequelize instance not available on TcmEncounterModel");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts(1 hunks)packages/api/src/routes/medical/tcm-encounter.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column ...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
📄 Source: CodeRabbit Inference Engine (.cursorrules)
List of files the instruction was applied to:
packages/api/src/routes/medical/tcm-encounter.tspackages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try ...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/api/src/routes/medical/tcm-encounter.tspackages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: lucasdellabella
PR: metriport/metriport#3588
File: packages/infra/lib/hl7-notification-stack/vpn.ts:11-15
Timestamp: 2025-04-02T22:25:52.792Z
Learning: User lucasdellabella prefers to handle certain code consistency issues (like mismatches between interface extension and class inheritance) in separate PRs rather than immediately fixing them in the current release PR.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/__tests__/hl7v2-roster-generator.test.ts:190-191
Timestamp: 2025-05-27T15:49:16.573Z
Learning: The user lucasdellabella prefers to keep changes focused on the PR's scope and avoid making ancillary performance improvements (like replacing delete operators with undefined assignments) that are not directly related to the main objectives of the current PR.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-03-05T18:43:30.389Z
Learning: For backmerge PRs at metriport/metriport, only verify two things: (1) that the source branch is `master` and destination branch is `develop`, and (2) that there's a link to at least one PR (usually a "patch" PR) in the description. No need for detailed review comments or updates to the PR description unless there's an issue with these criteria.
Learnt from: thomasyopes
PR: metriport/metriport#3883
File: packages/core/src/external/ehr/shared.ts:414-428
Timestamp: 2025-05-28T19:49:26.364Z
Learning: In the Metriport codebase, thomasyopes prefers to address potential stack overflow issues in pagination functions reactively (when they actually occur) rather than proactively refactoring recursive implementations to iterative ones.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: leite08
PR: metriport/metriport#3624
File: packages/api/src/command/medical/patient/get-patient.ts:90-97
Timestamp: 2025-04-07T17:32:22.935Z
Learning: When dealing with raw SQL queries in Sequelize that return complex data structures (especially with joins and JSON aggregation), using `any` type assertions may be necessary since TypeScript cannot infer the types. Using constants for column aliases helps minimize maintenance issues in these cases.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc. The migration files properly define all FK constraints, NOT NULL requirements, and cascade behaviors.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-28T22:58:19.669Z
Learning: For Sequelize models in this codebase, field constraints (such as `allowNull: false`) and field mappings (such as `field: "column_name"`) are defined in the migrations, not in the model definitions themselves.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/command/medical/cohort/patient-cohort/patient-cohort.ts:0-0
Timestamp: 2025-06-20T03:15:43.544Z
Learning: In Node.js applications using Sequelize ORM, the `bulkCreate` method executes as a single SQL command even for large batches (confirmed for 1500+ records), providing database-level atomicity without needing explicit transaction wrappers for simple bulk insertions.
Learnt from: leite08
PR: metriport/metriport#3412
File: packages/api/src/models/medical/patient-import.ts:49-83
Timestamp: 2025-04-24T02:24:47.642Z
Learning: For the Metriport codebase, database constraints like `allowNull: false`, default values, and enum validations are defined in the database migration files, not in the Sequelize model definition files.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/cohort.ts:0-0
Timestamp: 2025-06-17T23:10:31.779Z
Learning: In the Metriport codebase, foreign key constraints including cascade behaviors are defined in Sequelize migrations rather than being duplicated in the model definitions. The model files may have commented out constraint definitions that serve as documentation but the actual constraints are enforced at the database level through migrations.
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts:126-126
Timestamp: 2025-06-26T17:42:32.635Z
Learning: User lucasdellabella prefers to maintain existing patterns for database result parsing (like using parseInt(result[0].count.toString())) over making isolated improvements that would deviate from established codebase patterns, prioritizing consistency over individual optimizations.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:63-63
Timestamp: 2025-05-27T16:10:53.952Z
Learning: User lucasdellabella is okay with using JSON.stringify in log statements in TypeScript files, even though the coding guidelines suggest avoiding multi-line logs with JSON.stringify. They prefer to keep such logging as is when it serves the purpose effectively.
Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.
Learnt from: lucasdellabella
PR: metriport/metriport#3596
File: packages/core/src/util/base64-scrambler.ts:40-48
Timestamp: 2025-04-03T15:46:16.447Z
Learning: Lucas prefers to keep non-null assertions in the Base64Scrambler class rather than using null coalescing and additional conditionals.
packages/api/src/routes/medical/tcm-encounter.ts (12)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: lucasdellabella
PR: metriport/metriport#3907
File: packages/core/src/external/fhir/adt-encounters.ts:118-143
Timestamp: 2025-05-28T02:32:27.527Z
Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.
Learnt from: thomasyopes
PR: metriport/metriport#3960
File: packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts:11-19
Timestamp: 2025-06-05T16:28:33.728Z
Learning: In the EhrGetAppointmentsDirect class in packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts, type casting `as T[]` for the handler return value is considered safe and doesn't require additional runtime validation by the user thomasyopes.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/routes/medical/patient.ts:541-543
Timestamp: 2025-06-18T21:05:22.256Z
Learning: In packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortIdSchema are acceptable and don't need to be moved to separate schema files when the user prefers to keep them inline.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
Learnt from: thomasyopes
PR: metriport/metriport#3997
File: packages/api/src/routes/internal/medical/patient-job.ts:28-29
Timestamp: 2025-06-13T20:24:16.668Z
Learning: In packages/api/src/routes/internal/medical/patient-job.ts, mounting patientJobsRouter at the root path before other route handlers is intentional behavior, even though it could potentially consume requests before later handlers.
Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.
Learnt from: leite08
PR: metriport/metriport#3940
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:82-86
Timestamp: 2025-05-31T21:58:28.502Z
Learning: In packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts, the mutable array operations using push() on resourcesMutable and hydratedMutable (lines 82-86 and 100-103) have been explicitly accepted as exceptions to the immutability guidelines after previous discussion.
Learnt from: RamilGaripov
PR: metriport/metriport#3997
File: packages/api/src/routes/internal/ehr/job.ts:33-34
Timestamp: 2025-06-17T15:01:20.015Z
Learning: In the EHR route handlers under packages/api/src/routes/internal/ehr/, the ehrId parameter is moved from req.params to req.query by the processEhrId middleware before reaching the route handlers, so handlers should use getFromQueryOrFail("ehrId", req) not getFrom("params").orFail("ehrId", req).
Learnt from: leite08
PR: metriport/metriport#3463
File: packages/api/src/routes/internal/medical/patient-settings.ts:31-49
Timestamp: 2025-03-19T01:11:01.406Z
Learning: In the Metriport codebase, Zod schema validation errors in routes are intentionally allowed to propagate to the central error handler (packages/api/src/routes/helpers/default-error-handler.ts) rather than being caught with try/catch blocks in individual route handlers. The error handler automatically processes ZodErrors and returns a 400 Bad Request response with formatted validation details.
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (30)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: leite08
PR: metriport/metriport#3624
File: packages/api/src/command/medical/patient/get-patient.ts:90-97
Timestamp: 2025-04-07T17:32:22.935Z
Learning: When dealing with raw SQL queries in Sequelize that return complex data structures (especially with joins and JSON aggregation), using `any` type assertions may be necessary since TypeScript cannot infer the types. Using constants for column aliases helps minimize maintenance issues in these cases.
Learnt from: thomasyopes
PR: metriport/metriport#3960
File: packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts:11-19
Timestamp: 2025-06-05T16:28:33.728Z
Learning: In the EhrGetAppointmentsDirect class in packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts, type casting `as T[]` for the handler return value is considered safe and doesn't require additional runtime validation by the user thomasyopes.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc. The migration files properly define all FK constraints, NOT NULL requirements, and cascade behaviors.
Learnt from: lucasdellabella
PR: metriport/metriport#3907
File: packages/core/src/external/fhir/adt-encounters.ts:118-143
Timestamp: 2025-05-28T02:32:27.527Z
Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/routes/medical/patient.ts:541-543
Timestamp: 2025-06-18T21:05:22.256Z
Learning: In packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortIdSchema are acceptable and don't need to be moved to separate schema files when the user prefers to keep them inline.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts:126-126
Timestamp: 2025-06-26T17:42:32.635Z
Learning: User lucasdellabella prefers to maintain existing patterns for database result parsing (like using parseInt(result[0].count.toString())) over making isolated improvements that would deviate from established codebase patterns, prioritizing consistency over individual optimizations.
Learnt from: thomasyopes
PR: metriport/metriport#3788
File: packages/lambdas/src/ehr-compute-resource-diff-bundles.ts:42-43
Timestamp: 2025-05-09T18:47:30.626Z
Learning: The AWS SQS message attribute `ApproximateReceiveCount` is controlled by AWS and always contains a valid numeric value, so additional validation isn't required when parsing it with `parseInt`.
Learnt from: keshavsaharia
PR: metriport/metriport#4045
File: packages/shared/src/interface/external/surescripts/plan-code.ts:1-3
Timestamp: 2025-06-18T17:22:24.597Z
Learning: In packages/shared/src/interface/external/surescripts/plan-code.ts, the getPlanCodeName function should return undefined for invalid plan codes rather than throwing errors, as per user preference for graceful error handling.
Learnt from: leite08
PR: metriport/metriport#3463
File: packages/api/src/command/medical/patient/settings/create-patient-settings.ts:35-35
Timestamp: 2025-03-19T13:47:03.604Z
Learning: In the Metriport codebase, avoid defining redundant properties or variables where one can be directly derived from another (e.g., having both an array and a separate count property when the count can be determined using array.length).
Learnt from: leite08
PR: metriport/metriport#3463
File: packages/api/src/command/medical/patient/settings/create-patient-settings.ts:131-131
Timestamp: 2025-03-19T13:46:21.984Z
Learning: In the Metriport codebase, defensive programming practices should be used for conditional checks, particularly:
1. Using `>=` instead of `===` when comparing against maximum values or limits (e.g., `attempt >= MAX_BATCH_RETRIES` instead of `attempt === MAX_BATCH_RETRIES`)
2. Using `==` instead of `===` when checking against `undefined` to also catch `null` values
3. Using boundary checks when accessing array elements or counting items
Learnt from: leite08
PR: metriport/metriport#3857
File: packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts:67-82
Timestamp: 2025-05-28T02:51:35.779Z
Learning: In the search-lexical.ts file, the user prefers to bubble up JSON parsing errors rather than catching and logging them. When processing FHIR resources from OpenSearch results, errors should be thrown and allowed to propagate up the call stack instead of being caught and silently ignored.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:142-151
Timestamp: 2025-05-27T16:10:48.223Z
Learning: In packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts, the user prefers to let axios errors bubble up naturally rather than adding try-catch blocks that re-throw with MetriportError wrappers, especially when the calling code already has retry mechanisms in place via simpleExecuteWithRetries.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/command/medical/patient/patient-import/update-tracking.ts:70-84
Timestamp: 2025-04-28T23:01:39.688Z
Learning: In the Metriport codebase, it's acceptable to include a `where` clause when calling Sequelize's instance update method (like `job.update(jobToUpdate, { where: { cxId, id: jobId } })`), even though Sequelize silently ignores this clause when called on an instance rather than the model static method.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/command/medical/cohort/patient-cohort/patient-cohort.ts:0-0
Timestamp: 2025-06-20T03:15:43.544Z
Learning: In Node.js applications using Sequelize ORM, the `bulkCreate` method executes as a single SQL command even for large batches (confirmed for 1500+ records), providing database-level atomicity without needing explicit transaction wrappers for simple bulk insertions.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/cohort.ts:12-26
Timestamp: 2025-06-17T23:06:20.383Z
Learning: In the Metriport codebase, Sequelize models can work without explicit field mappings from camelCase to snake_case (e.g., cxId attribute can map to cx_id column without specifying field: "cx_id"). The user confirmed this works in their configuration.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/cohort.ts:0-0
Timestamp: 2025-06-17T23:10:31.779Z
Learning: In the Metriport codebase, foreign key constraints including cascade behaviors are defined in Sequelize migrations rather than being duplicated in the model definitions. The model files may have commented out constraint definitions that serve as documentation but the actual constraints are enforced at the database level through migrations.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-28T22:58:19.669Z
Learning: For Sequelize models in this codebase, field constraints (such as `allowNull: false`) and field mappings (such as `field: "column_name"`) are defined in the migrations, not in the model definitions themselves.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/__tests__/hl7v2-roster-generator.test.ts:190-191
Timestamp: 2025-05-27T15:49:16.573Z
Learning: The user lucasdellabella prefers to keep changes focused on the PR's scope and avoid making ancillary performance improvements (like replacing delete operators with undefined assignments) that are not directly related to the main objectives of the current PR.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:63-63
Timestamp: 2025-05-27T16:10:53.952Z
Learning: User lucasdellabella is okay with using JSON.stringify in log statements in TypeScript files, even though the coding guidelines suggest avoiding multi-line logs with JSON.stringify. They prefer to keep such logging as is when it serves the purpose effectively.
Learnt from: lucasdellabella
PR: metriport/metriport#3588
File: packages/infra/lib/hl7-notification-stack/vpn.ts:11-15
Timestamp: 2025-04-02T22:25:52.792Z
Learning: User lucasdellabella prefers to handle certain code consistency issues (like mismatches between interface extension and class inheritance) in separate PRs rather than immediately fixing them in the current release PR.
Learnt from: leite08
PR: metriport/metriport#3940
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:82-86
Timestamp: 2025-05-31T21:58:28.502Z
Learning: In packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts, the mutable array operations using push() on resourcesMutable and hydratedMutable (lines 82-86 and 100-103) have been explicitly accepted as exceptions to the immutability guidelines after previous discussion.
Learnt from: leite08
PR: metriport/metriport#3869
File: packages/core/src/external/fhir/export/string/shared/deny.ts:17-20
Timestamp: 2025-05-21T16:30:39.421Z
Learning: In the Metriport codebase, even when parameters are currently unused in a function implementation (as indicated by eslint-disable comments), the team prefers to keep the original parameter names without underscore prefixes to maintain API compatibility for clients, as the implementation details might change in the future.
Learnt from: lucasdellabella
PR: metriport/metriport#3596
File: packages/core/src/util/base64-scrambler.ts:30-35
Timestamp: 2025-04-03T15:46:22.893Z
Learning: In the metriport codebase, non-null assertions with eslint-disable comments are acceptable in certain scenarios like array indexing when the developer is confident the values will never be null/undefined.
Learnt from: thomasyopes
PR: metriport/metriport#3788
File: packages/api/src/external/ehr/shared/command/create-resource-diff-bundles.ts:0-0
Timestamp: 2025-05-08T19:16:10.258Z
Learning: Avoid using Promise.all for parallel execution of functions that access the cxMapping table, as this may lead to database locking issues. Serial execution (using for loops with await) is preferred for such operations even if it might be slower for large datasets.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:23-25
Timestamp: 2025-06-18T08:03:09.215Z
Learning: In the Metriport codebase, composite unique constraints are defined in migrations only, not at the Sequelize model level. Models use individual field constraints (unique: true) at the model level, but composite constraints are handled via migrations using uniqueKeys in createTable options or addConstraint. Models consistently use `...BaseModel.modelOptions(sequelize)` and `tableName: ModelName.NAME` without defining indexes arrays.
🧬 Code Graph Analysis (1)
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (4)
packages/api/src/models/medical/tcm-encounter.ts (1)
TcmEncounterModel(7-87)packages/api/src/models/medical/patient.ts (1)
PatientModel(8-35)packages/api-sdk/src/medical/client/metriport.ts (1)
Pagination(81-85)packages/shared/src/common/date.ts (1)
buildDayjs(93-95)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/api/src/routes/medical/tcm-encounter.ts (1)
78-78: Verify this breaking change won't impact API consumers.The response format is changing from a full pagination object to a simple array of DTOs. This removes pagination metadata from the response, which could break existing API consumers expecting the standard pagination structure.
#!/bin/bash # Description: Search for consumers of this TCM encounter endpoint to verify impact # Expected: Find usage patterns and confirm they can handle array response # Search for API calls to this endpoint rg -A 5 -B 5 "tcm.?encounter" --type ts --type js # Search for references to TCM encounter API responses rg -A 10 "getTcmEncounters|tcm.*encounter.*list" --type ts --type jspackages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (2)
12-18: Type definitions look good for raw SQL query results.The type definitions properly extend the base models to include the joined patient data, which aligns with the SQL query structure that joins
tcm_encounterandpatienttables.
32-44: ```shell
#!/bin/bashLocate TcmEncounterModel definition with surrounding context
rg "class TcmEncounterModel" -n -C5 .
Locate PatientModel definition with surrounding context
rg "class PatientModel" -n -C5 .
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
Outdated
Show resolved
Hide resolved
metriport/metriport-internal#1040 Ref: ENG-534 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column ...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
📄 Source: CodeRabbit Inference Engine (.cursorrules)
List of files the instruction was applied to:
packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try ...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: lucasdellabella
PR: metriport/metriport#3588
File: packages/infra/lib/hl7-notification-stack/vpn.ts:11-15
Timestamp: 2025-04-02T22:25:52.792Z
Learning: User lucasdellabella prefers to handle certain code consistency issues (like mismatches between interface extension and class inheritance) in separate PRs rather than immediately fixing them in the current release PR.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/__tests__/hl7v2-roster-generator.test.ts:190-191
Timestamp: 2025-05-27T15:49:16.573Z
Learning: The user lucasdellabella prefers to keep changes focused on the PR's scope and avoid making ancillary performance improvements (like replacing delete operators with undefined assignments) that are not directly related to the main objectives of the current PR.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-03-05T18:43:30.389Z
Learning: For backmerge PRs at metriport/metriport, only verify two things: (1) that the source branch is `master` and destination branch is `develop`, and (2) that there's a link to at least one PR (usually a "patch" PR) in the description. No need for detailed review comments or updates to the PR description unless there's an issue with these criteria.
Learnt from: thomasyopes
PR: metriport/metriport#3883
File: packages/core/src/external/ehr/shared.ts:414-428
Timestamp: 2025-05-28T19:49:26.364Z
Learning: In the Metriport codebase, thomasyopes prefers to address potential stack overflow issues in pagination functions reactively (when they actually occur) rather than proactively refactoring recursive implementations to iterative ones.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: leite08
PR: metriport/metriport#3624
File: packages/api/src/command/medical/patient/get-patient.ts:90-97
Timestamp: 2025-04-07T17:32:22.935Z
Learning: When dealing with raw SQL queries in Sequelize that return complex data structures (especially with joins and JSON aggregation), using `any` type assertions may be necessary since TypeScript cannot infer the types. Using constants for column aliases helps minimize maintenance issues in these cases.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc. The migration files properly define all FK constraints, NOT NULL requirements, and cascade behaviors.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-28T22:58:19.669Z
Learning: For Sequelize models in this codebase, field constraints (such as `allowNull: false`) and field mappings (such as `field: "column_name"`) are defined in the migrations, not in the model definitions themselves.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/command/medical/cohort/patient-cohort/patient-cohort.ts:0-0
Timestamp: 2025-06-20T03:15:43.544Z
Learning: In Node.js applications using Sequelize ORM, the `bulkCreate` method executes as a single SQL command even for large batches (confirmed for 1500+ records), providing database-level atomicity without needing explicit transaction wrappers for simple bulk insertions.
Learnt from: leite08
PR: metriport/metriport#3412
File: packages/api/src/models/medical/patient-import.ts:49-83
Timestamp: 2025-04-24T02:24:47.642Z
Learning: For the Metriport codebase, database constraints like `allowNull: false`, default values, and enum validations are defined in the database migration files, not in the Sequelize model definition files.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/cohort.ts:0-0
Timestamp: 2025-06-17T23:10:31.779Z
Learning: In the Metriport codebase, foreign key constraints including cascade behaviors are defined in Sequelize migrations rather than being duplicated in the model definitions. The model files may have commented out constraint definitions that serve as documentation but the actual constraints are enforced at the database level through migrations.
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts:126-126
Timestamp: 2025-06-26T17:42:32.635Z
Learning: User lucasdellabella prefers to maintain existing patterns for database result parsing (like using parseInt(result[0].count.toString())) over making isolated improvements that would deviate from established codebase patterns, prioritizing consistency over individual optimizations.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:63-63
Timestamp: 2025-05-27T16:10:53.952Z
Learning: User lucasdellabella is okay with using JSON.stringify in log statements in TypeScript files, even though the coding guidelines suggest avoiding multi-line logs with JSON.stringify. They prefer to keep such logging as is when it serves the purpose effectively.
Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.
Learnt from: lucasdellabella
PR: metriport/metriport#3596
File: packages/core/src/util/base64-scrambler.ts:40-48
Timestamp: 2025-04-03T15:46:16.447Z
Learning: Lucas prefers to keep non-null assertions in the Base64Scrambler class rather than using null coalescing and additional conditionals.
packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts (10)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/routes/medical/patient.ts:541-543
Timestamp: 2025-06-18T21:05:22.256Z
Learning: In packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortIdSchema are acceptable and don't need to be moved to separate schema files when the user prefers to keep them inline.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: lucasdellabella
PR: metriport/metriport#3907
File: packages/core/src/external/fhir/adt-encounters.ts:118-143
Timestamp: 2025-05-28T02:32:27.527Z
Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.
Learnt from: leite08
PR: metriport/metriport#3648
File: packages/utils/src/shared/patient-create.ts:35-53
Timestamp: 2025-04-10T17:30:30.368Z
Learning: The function `mergePatients` in the patient-create.ts file is intentionally designed to take all properties from the first patient (p1) and only update address and contact fields, as the system supports duplicated patients specifically to allow multiple addresses across different records.
Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.
Learnt from: leite08
PR: metriport/metriport#3942
File: packages/core/src/external/fhir/__tests__/location.ts:6-9
Timestamp: 2025-06-01T13:48:26.600Z
Learning: In TypeScript test code, don't flag deconstructed object parameters as needing simplification or refactoring. Test helper functions with nested optional object parameters (like `param?: { patient?: PatientWithId; location?: Partial<Location>; }`) are acceptable and should not be suggested for flattening.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: thomasyopes
PR: metriport/metriport#3960
File: packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts:11-19
Timestamp: 2025-06-05T16:28:33.728Z
Learning: In the EhrGetAppointmentsDirect class in packages/core/src/external/ehr/command/get-appointments/ehr-get-appointments-direct.ts, type casting `as T[]` for the handler return value is considered safe and doesn't require additional runtime validation by the user thomasyopes.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
🧬 Code Graph Analysis (1)
packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts (1)
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (1)
TcmEncounterQueryData(12-14)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts (2)
1-1: LGTM - Clean import structure.The import is correctly structured and imports the required type from the appropriate command layer.
3-8: LGTM - Well-designed DTO type.The DTO effectively flattens the nested
patientDatastructure into individual fields, which will simplify the API response format. The use ofOmitand intersection types is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/api/src/command/medical/tcm-encounter/__tests__/tcm-encounters-pagination.test.ts (2)
19-24: Consider extracting mock setup to improve maintainability.The
anytype assertions are unavoidable for mocking but consider extracting this setup to a helper function to reduce repetition and improve maintainability if this pattern is used across multiple test files.+const setupTcmEncounterMocks = (mockSequelize: any) => { + (TcmEncounterModel as any).sequelize = mockSequelize; + (TcmEncounterModel as any).tableName = "tcm_encounter"; + (PatientModel as any).tableName = "patient"; +}; + beforeEach(() => { jest.clearAllMocks(); - - // Setup model mocks - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (TcmEncounterModel as any).sequelize = mockSequelize; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (TcmEncounterModel as any).tableName = "tcm_encounter"; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (PatientModel as any).tableName = "patient"; + setupTcmEncounterMocks(mockSequelize); });
28-50: Consider adding parameter validation test.While the current test verifies SQL query structure, consider adding assertions to verify that the same parameter values (replacements) are passed to both queries to ensure complete consistency.
it("are both called with the same key arguments", async () => { const cxId = faker.string.uuid(); const afterDate = faker.date.past().toISOString(); const pagination: Pagination = { count: 10 }; mockSequelize.query.mockResolvedValueOnce([]); await getTcmEncounters({ cxId, pagination, after: afterDate }); mockSequelize.query.mockResolvedValueOnce([{ count: 0 }]); await getTcmEncountersCount({ cxId, after: afterDate }); const [dataQuery, dataOptions] = mockSequelize.query.mock.calls[0]; expect(dataQuery).toContain("tcm_encounter.cx_id = :cxId"); expect(dataQuery).toContain("tcm_encounter.admit_time > :afterDate"); expect(dataQuery).toContain("GROUP BY tcm_encounter.id"); expect(dataQuery).toContain("ORDER BY tcm_encounter.id DESC"); expect(dataQuery).toContain("LIMIT :count"); const [countQuery, countOptions] = mockSequelize.query.mock.calls[1]; expect(countQuery).toContain("tcm_encounter.cx_id = :cxId"); expect(countQuery).toContain("tcm_encounter.admit_time > :afterDate"); + + // Verify consistent parameter values + expect(dataOptions.replacements.cxId).toBe(countOptions.replacements.cxId); + expect(dataOptions.replacements.afterDate).toEqual(countOptions.replacements.afterDate); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/api/src/command/medical/tcm-encounter/__tests__/tcm-encounters-pagination.test.ts(1 hunks)packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts(1 hunks)packages/api/src/routes/medical/__tests__/tcm-encounter.test.ts(4 hunks)packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts(1 hunks)packages/api/src/routes/medical/tcm-encounter.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/api/src/routes/medical/tcm-encounter.ts
- packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts
- packages/api/src/routes/medical/tests/tcm-encounter.test.ts
- packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column ...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
📄 Source: CodeRabbit Inference Engine (.cursorrules)
List of files the instruction was applied to:
packages/api/src/command/medical/tcm-encounter/__tests__/tcm-encounters-pagination.test.ts
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try ...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/api/src/command/medical/tcm-encounter/__tests__/tcm-encounters-pagination.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts:88-93
Timestamp: 2025-06-27T01:50:29.077Z
Learning: The pagination logic in packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts uses conditional sort order (ASC when toItem is present for forward pagination, DESC otherwise for backward pagination) and inclusive bounds (>= and <=) which is intentional for proper bidirectional pagination functionality. Changing to always use ASC order or exclusive bounds would break this pagination pattern.
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: lucasdellabella
PR: metriport/metriport#3588
File: packages/infra/lib/hl7-notification-stack/vpn.ts:11-15
Timestamp: 2025-04-02T22:25:52.792Z
Learning: User lucasdellabella prefers to handle certain code consistency issues (like mismatches between interface extension and class inheritance) in separate PRs rather than immediately fixing them in the current release PR.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/__tests__/hl7v2-roster-generator.test.ts:190-191
Timestamp: 2025-05-27T15:49:16.573Z
Learning: The user lucasdellabella prefers to keep changes focused on the PR's scope and avoid making ancillary performance improvements (like replacing delete operators with undefined assignments) that are not directly related to the main objectives of the current PR.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-03-05T18:43:30.389Z
Learning: For backmerge PRs at metriport/metriport, only verify two things: (1) that the source branch is `master` and destination branch is `develop`, and (2) that there's a link to at least one PR (usually a "patch" PR) in the description. No need for detailed review comments or updates to the PR description unless there's an issue with these criteria.
Learnt from: thomasyopes
PR: metriport/metriport#3883
File: packages/core/src/external/ehr/shared.ts:414-428
Timestamp: 2025-05-28T19:49:26.364Z
Learning: In the Metriport codebase, thomasyopes prefers to address potential stack overflow issues in pagination functions reactively (when they actually occur) rather than proactively refactoring recursive implementations to iterative ones.
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts:10-22
Timestamp: 2025-06-27T01:50:14.201Z
Learning: In packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts, the patient fields patientData.firstName, patientData.lastName, and patientData.dob are guaranteed to be non-nullable values, so defensive null/undefined checks are not needed when accessing these fields.
Learnt from: leite08
PR: metriport/metriport#3624
File: packages/api/src/command/medical/patient/get-patient.ts:90-97
Timestamp: 2025-04-07T17:32:22.935Z
Learning: When dealing with raw SQL queries in Sequelize that return complex data structures (especially with joins and JSON aggregation), using `any` type assertions may be necessary since TypeScript cannot infer the types. Using constants for column aliases helps minimize maintenance issues in these cases.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc. The migration files properly define all FK constraints, NOT NULL requirements, and cascade behaviors.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-28T22:58:19.669Z
Learning: For Sequelize models in this codebase, field constraints (such as `allowNull: false`) and field mappings (such as `field: "column_name"`) are defined in the migrations, not in the model definitions themselves.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/command/medical/cohort/patient-cohort/patient-cohort.ts:0-0
Timestamp: 2025-06-20T03:15:43.544Z
Learning: In Node.js applications using Sequelize ORM, the `bulkCreate` method executes as a single SQL command even for large batches (confirmed for 1500+ records), providing database-level atomicity without needing explicit transaction wrappers for simple bulk insertions.
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts:126-126
Timestamp: 2025-06-26T17:42:32.635Z
Learning: User lucasdellabella prefers to maintain existing patterns for database result parsing (like using parseInt(result[0].count.toString())) over making isolated improvements that would deviate from established codebase patterns, prioritizing consistency over individual optimizations.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:63-63
Timestamp: 2025-05-27T16:10:53.952Z
Learning: User lucasdellabella is okay with using JSON.stringify in log statements in TypeScript files, even though the coding guidelines suggest avoiding multi-line logs with JSON.stringify. They prefer to keep such logging as is when it serves the purpose effectively.
Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.
Learnt from: lucasdellabella
PR: metriport/metriport#3596
File: packages/core/src/util/base64-scrambler.ts:40-48
Timestamp: 2025-04-03T15:46:16.447Z
Learning: Lucas prefers to keep non-null assertions in the Base64Scrambler class rather than using null coalescing and additional conditionals.
packages/api/src/command/medical/tcm-encounter/__tests__/tcm-encounters-pagination.test.ts (8)
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts:88-93
Timestamp: 2025-06-27T01:50:29.077Z
Learning: The pagination logic in packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts uses conditional sort order (ASC when toItem is present for forward pagination, DESC otherwise for backward pagination) and inclusive bounds (>= and <=) which is intentional for proper bidirectional pagination functionality. Changing to always use ASC order or exclusive bounds would break this pagination pattern.
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts:10-22
Timestamp: 2025-06-27T01:50:14.201Z
Learning: In packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts, the patient fields patientData.firstName, patientData.lastName, and patientData.dob are guaranteed to be non-nullable values, so defensive null/undefined checks are not needed when accessing these fields.
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/routes/medical/patient.ts:541-543
Timestamp: 2025-06-18T21:05:22.256Z
Learning: In packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortIdSchema are acceptable and don't need to be moved to separate schema files when the user prefers to keep them inline.
Learnt from: lucasdellabella
PR: metriport/metriport#3907
File: packages/core/src/external/fhir/adt-encounters.ts:118-143
Timestamp: 2025-05-28T02:32:27.527Z
Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
🧬 Code Graph Analysis (1)
packages/api/src/command/medical/tcm-encounter/__tests__/tcm-encounters-pagination.test.ts (4)
packages/api/src/models/medical/tcm-encounter.ts (1)
TcmEncounterModel(7-87)packages/api/src/models/medical/patient.ts (1)
PatientModel(8-35)packages/api-sdk/src/medical/client/metriport.ts (1)
Pagination(81-85)packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts (2)
getTcmEncounters(23-73)getTcmEncountersCount(75-106)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/api/src/command/medical/tcm-encounter/__tests__/tcm-encounters-pagination.test.ts (1)
39-49: Excellent test coverage for pagination consistency.This test effectively verifies that both
getTcmEncountersandgetTcmEncountersCountuse consistent filtering criteria, which is crucial for accurate pagination. The assertions properly check for the key SQL clauses that ensure both queries filter by the same conditions.
metriport/metriport-internal#1040 Ref: ENG-534 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-534 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
f9d788d to
19905ac
Compare
metriport/metriport-internal#1040 Ref: ENG-534 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/api/src/routes/medical/__tests__/fixtures.ts (1)
20-38: Consider improving type safety and data consistency.The function works well but has a few areas for improvement:
- Type safety: Using
Record<string, unknown>is quite permissive and could allow invalid properties to be passed- Data consistency: The patient ID appears in multiple places and could become inconsistent if overridden
Consider this more type-safe approach:
export function makePatient( - overrides: { patientData?: Partial<ReturnType<typeof makePatientData>> } & Record< - string, - unknown - > = {} + overrides: { + id?: string; + cxId?: string; + patientData?: Partial<ReturnType<typeof makePatientData>>; + } = {} ) { const patientData = makePatientData(overrides.patientData); + const id = overrides.id ?? "patient-123"; + const cxId = overrides.cxId ?? "cx-123"; return { - id: "patient-123", - cxId: "cx-123", + id, + cxId, data: patientData, dataValues: { - id: "patient-123", - cxId: "cx-123", + id, + cxId, data: patientData, }, - ...overrides, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/api/src/routes/medical/__tests__/fixtures.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column ...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
📄 Source: CodeRabbit Inference Engine (.cursorrules)
List of files the instruction was applied to:
packages/api/src/routes/medical/__tests__/fixtures.ts
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try ...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/api/src/routes/medical/__tests__/fixtures.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts:88-93
Timestamp: 2025-06-27T01:50:29.077Z
Learning: The pagination logic in packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts uses conditional sort order (ASC when toItem is present for forward pagination, DESC otherwise for backward pagination) and inclusive bounds (>= and <=) which is intentional for proper bidirectional pagination functionality. Changing to always use ASC order or exclusive bounds would break this pagination pattern.
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: lucasdellabella
PR: metriport/metriport#3588
File: packages/infra/lib/hl7-notification-stack/vpn.ts:11-15
Timestamp: 2025-04-02T22:25:52.792Z
Learning: User lucasdellabella prefers to handle certain code consistency issues (like mismatches between interface extension and class inheritance) in separate PRs rather than immediately fixing them in the current release PR.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/__tests__/hl7v2-roster-generator.test.ts:190-191
Timestamp: 2025-05-27T15:49:16.573Z
Learning: The user lucasdellabella prefers to keep changes focused on the PR's scope and avoid making ancillary performance improvements (like replacing delete operators with undefined assignments) that are not directly related to the main objectives of the current PR.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-03-05T18:43:30.389Z
Learning: For backmerge PRs at metriport/metriport, only verify two things: (1) that the source branch is `master` and destination branch is `develop`, and (2) that there's a link to at least one PR (usually a "patch" PR) in the description. No need for detailed review comments or updates to the PR description unless there's an issue with these criteria.
Learnt from: thomasyopes
PR: metriport/metriport#3883
File: packages/core/src/external/ehr/shared.ts:414-428
Timestamp: 2025-05-28T19:49:26.364Z
Learning: In the Metriport codebase, thomasyopes prefers to address potential stack overflow issues in pagination functions reactively (when they actually occur) rather than proactively refactoring recursive implementations to iterative ones.
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts:10-22
Timestamp: 2025-06-27T01:50:14.201Z
Learning: In packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts, the patient fields patientData.firstName, patientData.lastName, and patientData.dob are guaranteed to be non-nullable values, so defensive null/undefined checks are not needed when accessing these fields.
Learnt from: leite08
PR: metriport/metriport#3624
File: packages/api/src/command/medical/patient/get-patient.ts:90-97
Timestamp: 2025-04-07T17:32:22.935Z
Learning: When dealing with raw SQL queries in Sequelize that return complex data structures (especially with joins and JSON aggregation), using `any` type assertions may be necessary since TypeScript cannot infer the types. Using constants for column aliases helps minimize maintenance issues in these cases.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc. The migration files properly define all FK constraints, NOT NULL requirements, and cascade behaviors.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/models/medical/patient-cohort.ts:15-21
Timestamp: 2025-06-18T08:03:50.674Z
Learning: In the Metriport codebase, Sequelize models typically don't define `allowNull: false` or `references` metadata for foreign key columns. These constraints are handled exclusively in migration files, keeping models simpler and avoiding duplication. This is a consistent pattern across recent models like CohortModel, CoverageEnhancementModel, DocRefMappingModel, etc.
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-28T22:58:19.669Z
Learning: For Sequelize models in this codebase, field constraints (such as `allowNull: false`) and field mappings (such as `field: "column_name"`) are defined in the migrations, not in the model definitions themselves.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/command/medical/cohort/patient-cohort/patient-cohort.ts:0-0
Timestamp: 2025-06-20T03:15:43.544Z
Learning: In Node.js applications using Sequelize ORM, the `bulkCreate` method executes as a single SQL command even for large batches (confirmed for 1500+ records), providing database-level atomicity without needing explicit transaction wrappers for simple bulk insertions.
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts:126-126
Timestamp: 2025-06-26T17:42:32.635Z
Learning: User lucasdellabella prefers to maintain existing patterns for database result parsing (like using parseInt(result[0].count.toString())) over making isolated improvements that would deviate from established codebase patterns, prioritizing consistency over individual optimizations.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:63-63
Timestamp: 2025-05-27T16:10:53.952Z
Learning: User lucasdellabella is okay with using JSON.stringify in log statements in TypeScript files, even though the coding guidelines suggest avoiding multi-line logs with JSON.stringify. They prefer to keep such logging as is when it serves the purpose effectively.
Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.
Learnt from: lucasdellabella
PR: metriport/metriport#3596
File: packages/core/src/util/base64-scrambler.ts:40-48
Timestamp: 2025-04-03T15:46:16.447Z
Learning: Lucas prefers to keep non-null assertions in the Base64Scrambler class rather than using null coalescing and additional conditionals.
packages/api/src/routes/medical/__tests__/fixtures.ts (13)
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts:10-22
Timestamp: 2025-06-27T01:50:14.201Z
Learning: In packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts, the patient fields patientData.firstName, patientData.lastName, and patientData.dob are guaranteed to be non-nullable values, so defensive null/undefined checks are not needed when accessing these fields.
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/routes/medical/patient.ts:541-543
Timestamp: 2025-06-18T21:05:22.256Z
Learning: In packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortIdSchema are acceptable and don't need to be moved to separate schema files when the user prefers to keep them inline.
Learnt from: leite08
PR: metriport/metriport#3648
File: packages/utils/src/shared/patient-create.ts:35-53
Timestamp: 2025-04-10T17:30:30.368Z
Learning: The function `mergePatients` in the patient-create.ts file is intentionally designed to take all properties from the first patient (p1) and only update address and contact fields, as the system supports duplicated patients specifically to allow multiple addresses across different records.
Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.
Learnt from: leite08
PR: metriport/metriport#3942
File: packages/core/src/external/fhir/__tests__/location.ts:6-9
Timestamp: 2025-06-01T13:48:26.600Z
Learning: In TypeScript test code, don't flag deconstructed object parameters as needing simplification or refactoring. Test helper functions with nested optional object parameters (like `param?: { patient?: PatientWithId; location?: Partial<Location>; }`) are acceptable and should not be suggested for flattening.
Learnt from: lucasdellabella
PR: metriport/metriport#4059
File: packages/api/src/command/medical/tcm-encounter/create-sample-tcm-encounter.ts:6-12
Timestamp: 2025-06-19T20:30:28.388Z
Learning: In sample data generation functions like createSampleTcmEncounters, it's preferred to keep facility names and similar test data inline within the function rather than extracting them as constants, as they are only used locally and don't need to be shared.
Learnt from: leite08
PR: metriport/metriport#3953
File: packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts:28-30
Timestamp: 2025-06-03T21:02:23.374Z
Learning: The `makePatient()` function returns `PatientWithId` type which requires the `id` field to be present, ensuring `patient.id` is never undefined.
Learnt from: thomasyopes
PR: metriport/metriport#3997
File: packages/api/src/routes/internal/medical/patient-job.ts:28-29
Timestamp: 2025-06-13T20:24:16.668Z
Learning: In packages/api/src/routes/internal/medical/patient-job.ts, mounting patientJobsRouter at the root path before other route handlers is intentional behavior, even though it could potentially consume requests before later handlers.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: lucasdellabella
PR: metriport/metriport#3907
File: packages/core/src/external/fhir/adt-encounters.ts:118-143
Timestamp: 2025-05-28T02:32:27.527Z
Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.
Learnt from: leite08
PR: metriport/metriport#4034
File: packages/core/src/command/patient-import/patient-or-record-failed.ts:19-30
Timestamp: 2025-06-16T17:03:23.069Z
Learning: In patient import error handling functions like setPatientOrRecordFailed, the team prefers Promise.all over Promise.allSettled to ensure atomic all-or-nothing behavior - if either the job update or patient record update fails, the entire operation should fail rather than allowing partial success.
Learnt from: thomasyopes
PR: metriport/metriport#3771
File: packages/api/src/external/ehr/healthie/shared.ts:31-40
Timestamp: 2025-05-01T16:14:33.361Z
Learning: For the `createContacts` function in the Healthie integration, returning an empty contacts array is preferred over returning an array with an empty object when no contact information (email or phone) is available for a patient.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/api/src/routes/medical/__tests__/fixtures.ts (1)
1-18: LGTM! Well-structured factory function.The
makePatientDatafunction follows good practices with optional overrides and provides sensible defaults for test data generation.
leite08
reviewed
Jun 27, 2025
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
Outdated
Show resolved
Hide resolved
| mapToModel: true, | ||
| replacements: { | ||
| cxId, | ||
| ...{ afterDate: after ? buildDayjs(after).toDate() : DEFAULT_FILTER_DATE }, |
There was a problem hiding this comment.
Can we always use .toISOString instead of .toDate when working with dates as DB query params? It sets a pattern that works regardless of how we map the date column (Date or string).
There was a problem hiding this comment.
yep, this is a TIL, thanks!
There was a problem hiding this comment.
Cool. Let's also update how we use/set DEFAULT_FILTER_DATE to match it? I'd actually create it as dayjs so we can perform operations on it, and when we want to use it we just .toISOString() on it
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
Outdated
Show resolved
Hide resolved
packages/api/src/command/medical/tcm-encounter/get-tcm-encounters.ts
Outdated
Show resolved
Hide resolved
This was referenced Jul 16, 2025
Merged
This was referenced Jul 23, 2025
Closed
Merged
Merged
16 tasks
This was referenced Aug 9, 2025
Merged
This was referenced Aug 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Testing
Release Plan
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores