feat(canvas): timeout + background endpoints #3723
Conversation
Ref: ENG-47 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
WalkthroughThis update modifies the existing POST route for refreshing cached Canvas bundles by changing the endpoint path to include the Canvas patient ID as a path parameter. It adds two new API endpoints to the Canvas patient router for managing asynchronous resource diff jobs: a POST endpoint to initiate a resource diff job and a GET endpoint to retrieve the status and results of a job. Both endpoints validate the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Router
participant Job Service
Client->>API Router: POST /internal/ehr/canvas/patient/:id/resource/diff (with cxId, practiceId, direction)
API Router->>API Router: Validate direction param
API Router->>Job Service: createResourceDiffBundlesJob(params)
Job Service-->>API Router: jobId
API Router-->>Client: { jobId }
Client->>API Router: GET /internal/ehr/canvas/patient/:id/resource/diff/:jobId (with cxId, practiceId, direction)
API Router->>API Router: Validate direction param
API Router->>Job Service: getResourceDiffBundlesJobPayload(params, jobId)
Job Service-->>API Router: job payload, bundle URLs
API Router-->>Client: job payload, bundle URLs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/api/src/routes/internal/ehr/canvas/patient.ts (1)
90-158: Consider extracting shared validation logicThere's duplicated validation code between the POST and GET endpoints for the direction parameter. Consider extracting this into a middleware or utility function to follow the DRY principle.
+ const validateResourceDiffDirection = (req: Request): void => { + const direction = getFromQueryOrFail("direction", req); + if (!isResourceDiffDirection(direction)) { + throw new BadRequestError("Invalid direction", undefined, { + direction, + }); + } + }; router.post( "/resource-diff", requestLogger, asyncHandler(async (req: Request, res: Response) => { const cxId = getUUIDFrom("query", req, "cxId").orFail(); const canvasPatientId = getFromQueryOrFail("patientId", req); const canvasPracticeId = getFromQueryOrFail("practiceId", req); - const direction = getFromQueryOrFail("direction", req); - if (!isResourceDiffDirection(direction)) { - throw new BadRequestError("Invalid direction", undefined, { - direction, - }); - } + validateResourceDiffDirection(req); + const direction = getFromQueryOrFail("direction", req); const jobId = await createResourceDiffBundlesJob({ cxId, canvasPatientId, canvasPracticeId, direction, }); return res.status(httpStatus.OK).json(jobId); }) );Apply the same pattern to the GET endpoint as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/api/src/routes/internal/ehr/canvas/patient.ts(2 hunks)packages/infra/lib/ehr-nested-stack.ts(2 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 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/infra/lib/ehr-nested-stack.tspackages/api/src/routes/internal/ehr/canvas/patient.ts
🧠 Learnings (1)
packages/infra/lib/ehr-nested-stack.ts (1)
Learnt from: leite08
PR: metriport/metriport#3709
File: packages/core/src/external/ehr/bundle/create-resource-diff-bundles/steps/start/ehr-start-resource-diff-bundles-local.ts:18-24
Timestamp: 2025-04-23T21:39:58.667Z
Learning: The instance of `EhrStartResourceDiffBundlesLocal` is created inside the Lambda handler function, ensuring a fresh instance on each invocation with no state leakage between invocations.
🧬 Code Graph Analysis (1)
packages/api/src/routes/internal/ehr/canvas/patient.ts (6)
packages/api/src/routes/helpers/request-logger.ts (1)
requestLogger(10-63)packages/api/src/routes/util.ts (2)
asyncHandler(10-28)getFromQueryOrFail(61-65)packages/shared/src/interface/external/ehr/resource-diff.ts (1)
isResourceDiffDirection(7-9)packages/shared/src/index.ts (1)
BadRequestError(39-39)packages/api/src/external/ehr/canvas/job/create-resource-diff-bundles/start-job.ts (1)
createResourceDiffBundlesJob(29-72)packages/api/src/external/ehr/canvas/job/create-resource-diff-bundles/get-job-payload.ts (1)
getResourceDiffBundlesJobPayload(37-56)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/infra/lib/ehr-nested-stack.ts (2)
71-71: Lambda timeout increased for startResourceDiffBundles functionThe timeout has been increased from 1 minute to 5 minutes beyond the wait time. This change is appropriate to accommodate the potentially longer execution time needed for resource difference calculations, especially when the server needs to download data from Canvas.
119-119: Lambda timeout increased for refreshEhrBundles functionThe timeout has been increased from 1 minute to 5 minutes beyond the wait time. This change aligns with the other timeout increase and is necessary to handle the longer processing times required for refreshing EHR bundles from Canvas.
packages/api/src/routes/internal/ehr/canvas/patient.ts (3)
4-4: New imports added for resource diff functionalityThe imports are appropriate for the new functionality being added. They include the necessary validation function and job-related commands.
Also applies to: 12-13
90-122: Well-structured POST endpoint for initiating resource diff jobsThe endpoint follows best practices with proper parameter validation, error handling, and asynchronous processing. The direction parameter is appropriately validated using
isResourceDiffDirection.The job is started asynchronously, which is important for potentially long-running operations, and the request returns quickly with a job ID for later status checking.
124-158: Well-structured GET endpoint for retrieving resource diff job statusThe endpoint properly validates all required parameters including the job ID and retrieves the job payload. It follows the same pattern as the POST endpoint for direction validation.
The response includes not only the job status but also pre-signed URLs for the bundles if the job has completed, which provides a good user experience by not requiring additional API calls to access the results.
… 1040-patch-canvas-diff Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
| @@ -84,6 +87,76 @@ router.post( | |||
| }) | |||
| ); | |||
|
|
|||
| /** | |||
| * POST /internal/ehr/canvas/patient/resource-diff | |||
There was a problem hiding this comment.
I thought it would be /patient/:id/resource/diff, no?
Note two things there:
- /patient/:id/...
- .../resource/diff
Ref: ENG-47 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-47 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/api/src/external/ehr/canvas/job/create-resource-diff-bundles/start-job.ts (1)
36-40: Good validation addition to enforce direction constraint upfront.Early validation of the
directionparameter ensures better error handling by failing fast when an unsupported direction is provided. This is a cleaner approach than handling it later in the code.Consider updating the JSDoc comment at lines 19-28 to explicitly mention that currently only
METRIPORT_ONLYdirection is supported. This would make the API contract clearer to other developers./** * Starts the resource diff job to produce the resource type bundles containing * the resources in Metriport that are not in Canvas, or vice versa. * * @param cxId - The cxId of the patient. * @param canvasPracticeId - The canvas practice id of the patient. * @param canvasPatientId - The canvas patient id of the patient. * @param direction - The direction of the resource diff bundles to create. + * Currently only supports ResourceDiffDirection.METRIPORT_ONLY. * @param requestId - The request id of the job. (optional, defaults to a new UUID) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
packages/api/src/external/ehr/canvas/job/create-resource-diff-bundles/start-job.ts(2 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 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.string 8000 ify()- 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/external/ehr/canvas/job/create-resource-diff-bundles/start-job.ts
🔇 Additional comments (1)
packages/api/src/external/ehr/canvas/job/create-resource-diff-bundles/start-job.ts (1)
60-71: Well-structured async handler implementation with proper error handling.The code now directly creates the handler and invokes the appropriate method, which is cleaner and more maintainable than conditional logic. The use of
.catch(processAsyncError(...))follows the project guidelines for handling asynchronous errors properly.The function now returns the job ID immediately while processing continues asynchronously, which aligns with the PR's objective of supporting longer-running operations. This approach allows the API to respond quickly while the resource-intensive processing happens in the background.
Ref: ENG-47
Ref: #1040
Issues:
Description
Testing
Release Plan
Summary by CodeRabbit
New Features
Chores