feat(ss): swithc to fifo stuff #4031
Conversation
Ref: ENG-306 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
|
Caution Review failedThe pull request is closed. WalkthroughThe changes refactor several Lambda handler functions to process AWS SQS events instead of direct typed parameters. Each handler now extracts and validates a single message from the SQS event using Zod schemas before invoking the core business logic. Utility functions for message parsing and schema validation are introduced, and additional context is provided for tracing and observability. Changes
Sequence Diagram(s)sequenceDiagram
participant SQS as AWS SQS
participant Lambda as Lambda Handler
participant Utils as Utility Functions
participant Zod as Zod Schema
participant Core as Core Logic
SQS->>Lambda: Send SQSEvent
Lambda->>Utils: getSingleMessageOrFail(event)
Utils-->>Lambda: SQS Message
Lambda->>Utils: parseBody(schema, message.body)
Utils->>Zod: schema.parse(JSON.parse(body))
Zod-->>Utils: Validated Data
Utils-->>Lambda: Typed Data
Lambda->>Core: Invoke business logic with typed data
Core-->>Lambda: (Result/void)
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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
packages/lambdas/src/surescripts/send-patient-request.ts (1)
29-33: Schema should bestrict()to reject unexpected keysAdding
.strict()(or.passthrough()explicitly) documents the contract and prevents silent acceptance of unknown fields that could mask upstream issues.-const surescriptsPatientRequestSchema = z.object({ +const surescriptsPatientRequestSchema = z + .object({ patientId: z.string(), cxId: z.string(), facilityId: z.string(), -}); + }) + .strict();packages/lambdas/src/surescripts/send-batch-request.ts (1)
29-33: Use clearer, validated array schema
z.string().array()works but is less idiomatic and doesn’t ensure the list isn’t empty. Preferz.array(z.string()).min(1)for readability and stronger validation.-const surescriptsBatchRequestSchema = z.object({ - patientIds: z.string().array(), +const surescriptsBatchRequestSchema = z.object({ + patientIds: z.array(z.string()).min(1), cxId: z.string(), facilityId: z.string(), }).strict();packages/lambdas/src/surescripts/receive-response.ts (2)
15-17: Trim oversized context sent to Sentry
capture.setExtra({ event, context: lambdaName });serialises the full SQS event, which can exceed Sentry’s 100 kB limit and increase cost/noise.
Consider logging only metadata (messageId, body length, queueArn, etc.) or attaching the raw body under abreadcrumbs/attachmentinstead.
16-18: Redundant null-check aftergetSingleMessageOrFail
getSingleMessageOrFailalready throws when no message is found, so theif (!message) return;branch is dead code.- const message = getSingleMessageOrFail(event.Records, lambdaName); - if (!message) return; + const message = getSingleMessageOrFail(event.Records, lambdaName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/lambdas/src/shared/sqs.ts(2 hunks)packages/lambdas/src/surescripts/receive-response.ts(1 hunks)packages/lambdas/src/surescripts/receive-verification.ts(1 hunks)packages/lambdas/src/surescripts/send-batch-request.ts(1 hunks)packages/lambdas/src/surescripts/send-patient-request.ts(1 hunks)packages/lambdas/src/surescripts/verify-request-in-history.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 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/shared/sqs.tspackages/lambdas/src/surescripts/send-patient-request.tspackages/lambdas/src/surescripts/receive-verification.tspackages/lambdas/src/surescripts/receive-response.tspackages/lambdas/src/surescripts/send-batch-request.tspackages/lambdas/src/surescripts/verify-request-in-history.ts
🧬 Code Graph Analysis (3)
packages/lambdas/src/shared/sqs.ts (1)
packages/shared/src/index.ts (1)
MetriportError(41-41)
packages/lambdas/src/surescripts/send-patient-request.ts (4)
packages/lambdas/src/shared/sqs.ts (2)
getSingleMessageOrFail(63-84)parseBody(86-95)packages/core/src/external/surescripts/types.ts (1)
SurescriptsPatientRequest(36-38)packages/lambdas/src/surescripts/shared.ts (1)
makeSurescriptsClient(6-15)packages/core/src/external/surescripts/command/send-patient-request/send-patient-request-direct.ts (1)
SurescriptsSendPatientRequestHandlerDirect(7-17)
packages/lambdas/src/surescripts/send-batch-request.ts (7)
packages/lambdas/src/surescripts/receive-verification.ts (1)
handler(14-23)packages/lambdas/src/surescripts/receive-response.ts (1)
handler(14-23)packages/lambdas/src/surescripts/send-patient-request.ts (1)
handler(15-27)packages/lambdas/src/shared/sqs.ts (2)
getSingleMessageOrFail(63-84)parseBody(86-95)packages/core/src/external/surescripts/types.ts (1)
SurescriptsBatchRequest(44-46)packages/lambdas/src/surescripts/shared.ts (1)
makeSurescriptsClient(6-15)packages/core/src/external/surescripts/command/send-batch-request/send-batch-request-direct.ts (1)
SurescriptsSendBatchRequestHandlerDirect(7-17)
⏰ 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/lambdas/src/shared/sqs.ts (1)
86-95: 🛠️ Refactor suggestionGuard against invalid JSON before it takes down the Lambda
JSON.parsewill throw aSyntaxErrorthat bypasses the unifiedMetriportErrorflow, causing the whole invocation to fail without context. Wrap the parse in atry / catchand re-throw aMetriportErrorso downstream monitoring sees a consistent error type.- const bodyAsJson = JSON.parse(bodyString); + let bodyAsJson: unknown; + try { + bodyAsJson = JSON.parse(bodyString); + } catch (err) { + throw new MetriportError("Body is not valid JSON", err as Error); + }⛔ Skipped due to learnings
Learnt from: thomasyopes PR: metriport/metriport#3608 File: packages/lambdas/src/ehr-compute-resource-diff-bundles.ts:58-62 Timestamp: 2025-04-23T19:00:49.707Z Learning: For Lambda functions in the metriport codebase, the team prefers to let the default Lambda error handling catch JSON parsing errors rather than adding explicit try/catch blocks.
| export const handler = capture.wrapHandler(async (event: SQSEvent) => { | ||
| capture.setExtra({ event, context: lambdaName }); | ||
| const message = getSingleMessageOrFail(event.Records, lambdaName); |
There was a problem hiding this comment.
Avoid shipping PHI to Sentry/Datadog – don’t attach the raw event
capture.setExtra({ event, context: lambdaName }) serialises the full SQS event, including the message body that contains patientId, potentially leaking regulated data to external observability tooling. Emit only non-sensitive identifiers (e.g., messageId, receiptHandle) instead.
- capture.setExtra({ event, context: lambdaName });
+ capture.setExtra({
+ context: lambdaName,
+ messageIds: event.Records.map((r) => r.messageId),
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const handler = capture.wrapHandler(async (event: SQSEvent) => { | |
| capture.setExtra({ event, context: lambdaName }); | |
| const message = getSingleMessageOrFail(event.Records, lambdaName); | |
| export const handler = capture.wrapHandler(async (event: SQSEvent) => { | |
| capture.setExtra({ | |
| context: lambdaName, | |
| messageIds: event.Records.map((r) => r.messageId), | |
| }); | |
| const message = getSingleMessageOrFail(event.Records, lambdaName); |
🤖 Prompt for AI Agents
In packages/lambdas/src/surescripts/send-patient-request.ts around lines 15 to
17, avoid attaching the full SQS event to capture.setExtra as it includes
sensitive patient information. Instead, extract and pass only non-sensitive
identifiers like messageId and receiptHandle from the event records to
capture.setExtra to prevent leaking PHI to external monitoring tools.
| export const handler = capture.wrapHandler(async (event: SQSEvent) => { | ||
| capture.setExtra({ event, context: lambdaName }); | ||
| const message = getSingleMessageOrFail(event.Records, lambdaName); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential PHI leakage – raw event is captured
Same concern as other handlers: the event body contains transmissionId that may link to patient data. Limit the payload you forward to capture.
🤖 Prompt for AI Agents
In packages/lambdas/src/surescripts/receive-verification.ts around lines 14 to
16, the raw event is being passed to capture.setExtra, which may expose
sensitive patient-related data like transmissionId. Modify the code to extract
only non-sensitive, necessary fields from the event before passing them to
capture.setExtra, avoiding forwarding the entire event object to prevent
potential PHI leakage.
| export const handler = capture.wrapHandler(async (event: SQSEvent) => { | ||
| capture.setExtra({ event, context: lambdaName }); | ||
| const message = getSingleMessageOrFail(event.Records, lambdaName); |
There was a problem hiding this comment.
Restrict captured context to non-sensitive metadata
Forwarding the full SQS event risks exposing patient identifiers. Trim the data to the minimal fields required for debugging.
🤖 Prompt for AI Agents
In packages/lambdas/src/surescripts/verify-request-in-history.ts around lines 14
to 16, the current code forwards the entire SQS event to capture.setExtra, which
may expose sensitive patient identifiers. Modify the code to extract and forward
only the minimal necessary metadata fields from the event for debugging
purposes, avoiding any sensitive or personally identifiable information.
| export const handler = capture.wrapHandler(async (event: SQSEvent) => { | ||
| capture.setExtra({ event, context: lambdaName }); | ||
| const message = getSingleMessageOrFail(event.Records, lambdaName); |
There was a problem hiding this comment.
Sanitise event before attaching to telemetry
As with the other lambdas, avoid pushing the raw SQS event (contains patient IDs) to external logging/monitoring systems.
🤖 Prompt for AI Agents
In packages/lambdas/src/surescripts/send-batch-request.ts around lines 15 to 17,
the raw SQS event containing sensitive patient IDs is being attached directly to
telemetry. To fix this, sanitize the event data by removing or masking patient
IDs before passing it to capture.setExtra. Ensure only non-sensitive information
is included in the telemetry to protect patient privacy.
Ref: ENG-306 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
… eng-306-patch Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-306
Ref: #1040
Issues:
Description
Release Plan
Summary by CodeRabbit
New Features
Refactor