199/72 canvas/elation/athena contribution plumbing #3788
Merged
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
added 2 commits
May 1, 2025 11:13
WalkthroughThis change set refactors and modularizes the EHR bundle/resource diff job infrastructure, removing legacy Canvas-specific resource diff logic and generalizing the workflow to support multiple EHRs. It introduces new utility modules for bundle and job management, updates API routes and handlers, and replaces direct Lambda invocations with SQS-driven job orchestration. Several files are deleted or replaced, type definitions are updated, and error handling and retry logic are improved throughout the codebase. Changes
Sequence Diagram(s)Generalized Resource Diff Bundle Job FlowsequenceDiagram
participant API as API Route
participant JobStarter as startCreateResourceDiffBundlesJob
participant JobDB as PatientJob Store
participant BundleRefresher as refreshEhrBundles
participant SQS as SQS Queue
participant Lambda as Lambda Worker
participant S3 as S3 Storage
API->>JobStarter: POST /internal/ehr/:ehrId/patient/:id/resource/diff (start job)
JobStarter->>JobDB: Create patient job (type: resource-diff)
JobStarter->>BundleRefresher: For each supported resource type, refreshEhrBundles(...)
BundleRefresher->>SQS: Send refresh job message
SQS->>Lambda: Trigger Lambda for refresh
Lambda->>S3: Create/replace empty diff bundles (Metriport-only, EHR-only)
Lambda->>SQS: Send compute diff job message
SQS->>Lambda: Trigger Lambda for compute
Lambda->>S3: Fetch resources, compute diff, update bundles
Lambda->>JobDB: Update job entry status (successful/failed)
Fetching Resource Diff Job PayloadsequenceDiagram
participant API as API Route
participant JobDB as PatientJob Store
participant BundleFetcher as fetchBundlePreSignedUrls
API->>JobDB: Fetch patient job by jobId
alt job.status == "completed"
API->>BundleFetcher: fetchBundlePreSignedUrls(...)
BundleFetcher->>API: Return pre-signed URLs
API->>API: Compose and return job payload with URLs
else
API->>API: Return job payload (no URLs)
end
Refreshing EHR BundlessequenceDiagram
participant API as API Route
participant BundleRefresher as refreshEhrBundles
participant SQS as SQS Queue
participant Lambda as Lambda Worker
participant S3 as S3 Storage
API->>BundleRefresher: POST /internal/ehr/:ehrId/patient/:id/resource/refresh
BundleRefresher->>SQS: Send refresh job message
SQS->>Lambda: Trigger Lambda for refresh
Lambda->>S3: Refresh EHR bundle for resource type
Lambda->>JobDB: Update job entry status
JWT Token Management (Generalized)sequenceDiagram
participant API as API Route
participant JWTUtils as checkJwtToken/saveJwtToken
participant TokenStore as JWT Token Store
API->>JWTUtils: checkJwtToken({token, source})
JWTUtils->>TokenStore: getJwtToken({token, source})
TokenStore-->>JWTUtils: Token info
JWTUtils-->>API: {active, expired?}
API->>JWTUtils: saveJwtToken({token, source, exp, data})
JWTUtils->>TokenStore: findOrCreateJwtToken(...)
TokenStore-->>JWTUtils: (persisted)
JWTUtils-->>API: (done)
SQS-Based Resource Diff ComputationsequenceDiagram
participant JobStarter as startCreateResourceDiffBundlesJob
participant SQS as SQS Queue
participant Lambda as Lambda Worker
participant S3 as S3 Storage
JobStarter->>SQS: Send compute diff job message (per resource type)
SQS->>Lambda: Trigger Lambda for compute
Lambda->>S3: Fetch resources, compute diff, update bundles
Lambda->>JobDB: Update job entry status
These diagrams represent the new modular, EHR-agnostic resource diff and bundle management workflows, reflecting the new control flow and infrastructure introduced by this change set. Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 🪧 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 (
|
added 11 commits
May 5, 2025 10:40
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
thomasyopes
commented
May 5, 2025
There was a problem hiding this comment.
moved into /shared/utils/jwt-token.ts
thomasyopes
commented
May 5, 2025
There was a problem hiding this comment.
JWT stuff moved into /shared/utils/jwt-token.ts
Appointments stuff moved into /shared/utils/appointment.ts
External ID stuff moved into /shared/utils/external-id.ts
Client stuff moved into /shared/utils/client.ts
added 5 commits
May 5, 2025 13:24
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
… 199-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
thomasyopes
commented
May 6, 2025
| @@ -56,7 +55,6 @@ export async function recreateConsolidated({ | |||
| createResourceDiffBundles({ | |||
| cxId: patient.cxId, | |||
| patientId: patient.id, | |||
| direction: ResourceDiffDirection.METRIPORT_ONLY, | |||
There was a problem hiding this comment.
The resource diff job now computes both directions at once.
added 2 commits
May 6, 2025 11:57
thomasyopes
commented
May 6, 2025
| @@ -45,7 +48,3 @@ export async function createCanvasClient( | |||
| getClient: CanvasApi.create, | |||
| }); | |||
| } | |||
|
|
|||
| export function getCreateCanvasResourceDiffBundlesJobType(direction: ResourceDiffDirection) { | |||
There was a problem hiding this comment.
Moved to jobs utils file
added 4 commits
May 6, 2025 12:52
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
thomasyopes
commented
May 6, 2025
There was a problem hiding this comment.
Taken from old shared file.
… 199-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/api/src/routes/internal/ehr/__tests__/middleware.test.ts (3)
4-7: Mock objects could be improved with Jest spy functions.While the dummy mocks work for basic testing, consider using Jest's
jest.fn()for thenextfunction to verify it's called in the success case and not called in the error case.-const dummyNext: NextFunction = function () { - return; -}; +const dummyNext: NextFunction = jest.fn();
9-19: Good test for valid ehrId case, but could verify next() is called.The test correctly verifies that ehrId is set in query, but doesn't verify that
next()is called. This would ensure the middleware is properly continuing the request chain.processEhrId(incomingRequest, dummyResp, dummyNext); expect(incomingRequest.query.ehrId).toBe("athenahealth"); + expect(dummyNext).toHaveBeenCalled();
1-29: Consider adding more test cases for edge conditions.The current tests cover basic valid and invalid cases, but consider adding tests for:
- Missing ehrId parameter
- Other valid EHR sources (not just "athenahealth")
- Edge cases like empty string or null
This would provide more comprehensive test coverage for the middleware.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
packages/api/src/routes/internal/ehr/__tests__/middleware.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and A3D4 avoid sharing state across different functions, objects, and systems - 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
packages/api/src/routes/internal/ehr/__tests__/middleware.test.ts
🧬 Code Graph Analysis (1)
packages/api/src/routes/internal/ehr/__tests__/middleware.test.ts (1)
packages/api/src/routes/internal/ehr/middleware.ts (1)
processEhrId(6-11)
🔇 Additional comments (2)
packages/api/src/routes/internal/ehr/__tests__/middleware.test.ts (2)
1-3: LGTM: Proper imports for Express middleware test.The imports correctly include the necessary Express types and the middleware function being tested.
20-28: Good test for invalid ehrId case.The test properly verifies that an error is thrown with the correct message when an invalid ehrId is provided.
added 2 commits
May 15, 2025 05:55
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/lambdas/src/ehr-compute-resource-diff-bundles.ts (2)
31-31: Use prefixedLog instead of console.log for consistencyThe codebase generally avoids using
console.logdirectly, especially in packages other than utils, infra, and shared. Consider using theprefixedLoghelper that's already defined and used elsewhere in this file.- console.log(`Running with unparsed body: ${message.body}`); + log(`Running with unparsed body: ${message.body}`);
19-21: Consider validating waitTimeInMillis similar to maxAttemptsWhile the team indicated that environment variables are explicitly set, for consistency consider adding validation for
waitTimeInMillissimilar to what could be done formaxAttemptsto prevent potential NaN issues if the configuration changes.const waitTimeInMillisRaw = getEnvOrFail("WAIT_TIME_IN_MILLIS"); -const waitTimeInMillis = parseInt(waitTimeInMillisRaw); +const waitTimeInMillis = parseInt(waitTimeInMillisRaw, 10); +if (Number.isNaN(waitTimeInMillis) || waitTimeInMillis <= 0) { + throw new MetriportError( + "`WAIT_TIME_IN_MILLIS` must be a positive integer", + undefined, + { waitTimeInMillisRaw } + ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (3)
packages/lambdas/src/ehr-compute-resource-diff-bundles.ts(4 hunks)packages/lambdas/src/ehr-refresh-ehr-bundles.ts(4 hunks)packages/lambdas/src/shared/ehr.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/lambdas/src/shared/ehr.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/lambdas/src/ehr-refresh-ehr-bundles.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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...
**/*.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
packages/lambdas/src/ehr-compute-resource-diff-bundles.ts
🧠 Learnings (1)
packages/lambdas/src/ehr-compute-resource-diff-bundles.ts (1)
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`.
🔇 Additional comments (4)
packages/lambdas/src/ehr-compute-resource-diff-bundles.ts (4)
40-42: Code handles receive count appropriatelyThis is a good implementation of the retry logic using the SQS message's
ApproximateReceiveCountattribute to determine when to report errors. As noted in your previous feedback, this value is controlled by AWS and is guaranteed to be a valid numeric value.
36-37: Improved log prefixThe simplified prefixed log addresses the previous feedback about overusing prefixed logs, making the logs more readable while still providing necessary context.
45-45: Updated method call with reportError flagThe updated method call to
computeResourceDiffBundleswith the additionalreportErrorflag properly implements the new error reporting mechanism based on retry attempts.
59-59: Shared schema usageUsing the imported shared schema instead of a local one is a good improvement that aligns with the PR objectives of centralizing shared logic.
added 6 commits
May 15, 2025 06:08
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-199 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
… 199-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
leite08
approved these changes
May 15, 2025
added 2 commits
May 15, 2025 15:45
… 199-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
… 199-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
3 tasks
This was referenced May 22, 2025
This was referenced May 30, 2025
This was referenced Jun 6, 2025
This was referenced Jun 17, 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.
Issues:
Dependencies
Description
Testing
Release Plan
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Documentation