Conversation
|
@VaidehiSakalkar |
There was a problem hiding this comment.
Pull request overview
This pull request implements a "Safety Shield" feature that checks for drug interactions using the RxNorm API. The feature allows users to verify if a new medication will interact with their existing medications before adding it.
Changes:
- Added drug interaction checking service that integrates with RxNorm API with 24-hour caching
- Created new API endpoint
/api/safety-shield/checkfor interaction validation - Fixed .env file path in emailService and updated code formatting in index.js
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/services/interactionService.js | New service for RxNorm API integration with caching for drug name resolution and interaction checking |
| server/src/routes/safetyShieldRoutes.js | New route definition with authentication middleware for safety shield endpoint |
| server/src/api/safetyShieldController.js | New controller handling drug interaction validation against user's existing medications |
| server/src/services/emailService.js | Fixed .env file path (removed one directory level) |
| server/src/index.js | Added safety shield route registration and code formatting improvements |
| server/package.json | Added nodemon as dev dependency |
| server/package-lock.json | Updated lock file with nodemon and dependency changes |
| server/.gitignore | Modified .env ignore pattern (introduces security issue) |
Files not reviewed (1)
- server/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const existingMeds = await Medication.find({ userId, status: { $ne: 'deleted' } }, 'name'); | ||
| const existingNames = existingMeds.map(m => m.name); |
There was a problem hiding this comment.
The controller queries the Medication model for a 'name' field, but the Medication model schema uses 'pillName' instead of 'name'. This will result in the query retrieving empty or undefined values, causing the drug interaction check to fail silently.
| const existingMeds = await Medication.find({ userId, status: { $ne: 'deleted' } }, 'name'); | |
| const existingNames = existingMeds.map(m => m.name); | |
| const existingMeds = await Medication.find({ userId, status: { $ne: 'deleted' } }, 'pillName'); | |
| const existingNames = existingMeds.map(m => m.pillName); |
| "pdf-parse": "^1.1.1", | ||
| "socket.io": "^4.8.1", | ||
| "syllable": "^5.0.1" | ||
| }, |
There was a problem hiding this comment.
The 'axios' package is missing from dependencies but is imported and used throughout the interactionService. While it exists as a peer dependency in package-lock.json, it should be explicitly added to the dependencies in package.json to ensure it's properly installed.
|
|
||
| const existingMeds = await Medication.find({ userId, status: { $ne: 'deleted' } }, 'name'); | ||
| const existingNames = existingMeds.map(m => m.name); | ||
| const existingRxCuis = await Promise.all(existingNames.map(name => getRxCui(name))); |
There was a problem hiding this comment.
The parallel resolution of RxCUIs for existing medications using Promise.all could lead to rate limiting or performance issues if a user has many medications. Consider implementing batch processing with rate limiting or adding a maximum limit on the number of medications checked at once.
| const existingRxCuis = await Promise.all(existingNames.map(name => getRxCui(name))); | |
| const BATCH_SIZE = 10; | |
| const existingRxCuis = []; | |
| for (let i = 0; i < existingNames.length; i += BATCH_SIZE) { | |
| const batch = existingNames.slice(i, i + BATCH_SIZE); | |
| const batchRxCuis = await Promise.all(batch.map(name => getRxCui(name))); | |
| existingRxCuis.push(...batchRxCuis); | |
| } |
| env files | ||
| env |
There was a problem hiding this comment.
The .gitignore file has been modified to no longer properly ignore .env files. The line "env files" (without the dot and hash) will not match the ".env" file pattern. This could lead to accidentally committing sensitive environment variables to version control.
| env files | |
| env | |
| # env files | |
| .env |
| if (!drugName) return res.status(400).json({ success: false, message: 'drugName required' }); | ||
|
|
||
| // Resolve RxCUI for the new drug | ||
| const newRxCui = await getRxCui(drugName); |
There was a problem hiding this comment.
No input validation is performed on the drugName parameter beyond checking if it exists. Consider adding validation for minimum/maximum length, allowed characters, and sanitization to prevent potential injection attacks or malformed API requests.
| if (!drugName) return res.status(400).json({ success: false, message: 'drugName required' }); | |
| // Resolve RxCUI for the new drug | |
| const newRxCui = await getRxCui(drugName); | |
| // Basic validation and sanitization of drugName | |
| const rawDrugName = typeof drugName === 'string' ? drugName.trim() : ''; | |
| // Enforce minimum and maximum length | |
| const MIN_DRUG_NAME_LENGTH = 1; | |
| const MAX_DRUG_NAME_LENGTH = 100; | |
| // Allow letters, numbers, spaces, and common punctuation used in drug names | |
| const ALLOWED_DRUG_NAME_REGEX = /^[a-zA-Z0-9\s\-,'().]+$/; | |
| if ( | |
| rawDrugName.length < MIN_DRUG_NAME_LENGTH || | |
| rawDrugName.length > MAX_DRUG_NAME_LENGTH || | |
| !ALLOWED_DRUG_NAME_REGEX.test(rawDrugName) | |
| ) { | |
| return res.status(400).json({ | |
| success: false, | |
| message: 'Invalid drugName format' | |
| }); | |
| } | |
| const sanitizedDrugName = rawDrugName; | |
| // Resolve RxCUI for the new drug | |
| const newRxCui = await getRxCui(sanitizedDrugName); |
| // src/api/safetyShieldController.js | ||
| import { getRxCui, checkInteractions } from '../services/interactionService.js'; | ||
| import Medication from '../models/medicineModel.js'; // existing model, we only read | ||
|
|
||
| /** | ||
| * POST /api/safety-shield/check | ||
| * Body: { drugName: string } | ||
| * Returns: { safe: boolean, interactions: [] } | ||
| */ | ||
| export const checkSafety = async (req, res) => { | ||
| try { | ||
| const { drugName } = req.body; | ||
| if (!drugName) return res.status(400).json({ success: false, message: 'drugName required' }); | ||
|
|
||
| // Resolve RxCUI for the new drug | ||
| const newRxCui = await getRxCui(drugName); | ||
| if (!newRxCui) { | ||
| return res.status(404).json({ success: false, message: 'Unable to resolve drug name to RxCUI' }); | ||
| } | ||
|
|
||
| // Fetch user's existing medications (assumes auth middleware sets req.user.id) | ||
| const userId = req.user?.id; | ||
| if (!userId) return res.status(401).json({ success: false, message: 'Unauthenticated' }); | ||
|
|
||
| const existingMeds = await Medication.find({ userId, status: { $ne: 'deleted' } }, 'name'); | ||
| const existingNames = existingMeds.map(m => m.name); | ||
| const existingRxCuis = await Promise.all(existingNames.map(name => getRxCui(name))); | ||
| const filteredRxCuis = existingRxCuis.filter(Boolean); | ||
|
|
||
| const interactions = await checkInteractions(newRxCui, filteredRxCuis); | ||
| if (interactions.length === 0) { | ||
| return res.json({ success: true, safe: true, interactions: [] }); | ||
| } | ||
| return res.json({ success: true, safe: false, interactions }); | ||
| } catch (err) { | ||
| console.error('SafetyShield error:', err); | ||
| res.status(500).json({ success: false, message: 'Server error' }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The new Safety Shield feature lacks test coverage. Given that the repository has comprehensive testing for other features (as seen in test/readability.test.js and test/translation.test.js), tests should be added for the interactionService and safetyShieldController to ensure reliability and prevent regressions.
| } catch (err) { | ||
| console.error('RxNav name lookup error:', err.message); | ||
| return null; |
There was a problem hiding this comment.
The error handling returns an empty array on failure, which could mask API errors or network issues. The caller has no way to distinguish between "no interactions found" and "an error occurred while checking". Consider throwing errors or returning a more structured response that includes success/error status.
| const interactions = interactionTypeGroup.map(it => ({ | ||
| drugA: it.interactionPair?.[0]?.interactionConcept?.[0]?.sourceConceptItem?.name, | ||
| drugB: it.interactionPair?.[0]?.interactionConcept?.[1]?.sourceConceptItem?.name, | ||
| severity: it.severity, | ||
| description: it.interactionPair?.[0]?.description | ||
| })); |
There was a problem hiding this comment.
The response format is inconsistent with interaction data access. The interactionPair array is accessed with hardcoded index [0], which assumes there's always exactly one interaction pair. If the API returns multiple pairs or none, this will either miss data or cause errors. Consider iterating over all interaction pairs or documenting this assumption.
| const interactions = interactionTypeGroup.map(it => ({ | |
| drugA: it.interactionPair?.[0]?.interactionConcept?.[0]?.sourceConceptItem?.name, | |
| drugB: it.interactionPair?.[0]?.interactionConcept?.[1]?.sourceConceptItem?.name, | |
| severity: it.severity, | |
| description: it.interactionPair?.[0]?.description | |
| })); | |
| const interactions = interactionTypeGroup.flatMap(it => { | |
| const pairs = it.interactionPair || []; | |
| return pairs.map(pair => ({ | |
| drugA: pair.interactionConcept?.[0]?.sourceConceptItem?.name, | |
| drugB: pair.interactionConcept?.[1]?.sourceConceptItem?.name, | |
| severity: pair.severity ?? it.severity, | |
| description: pair.description | |
| })); | |
| }); |
|
|
||
|
|
||
| app.use("/api/agents",agentsRoutes) | ||
| app.use("/api/agents", agentsRoutes) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (92% of all statements in the enclosing script have an explicit semicolon).
JainamDedhia
left a comment
There was a problem hiding this comment.
Thanks for working on the Safety Shield feature 👍
I can’t approve this yet due to a few blocking issues:
Medication.find(..., 'name')uses a field that doesn’t exist in the schema (pillNameis used instead)..envhandling /.gitignoreneeds to be fixed to avoid committing env files.axiosis used but not listed inserver/package.json.- Interaction parsing assumes
interactionPair[0]always exists, which may break for other API responses.
Please fix these first, then I’ll re-review.
Safety Shield has been added as described and is working