VERIFIED: Issues Actually Introduced by PR #1500:
- Snapshot JSON parsing complexity (notes_controller.rb:38-80) - new code
- JSON parsing with bare rescue (notes_controller.rb:50-56) - silent failures
- get_value() falsy handling behavior (debatable if bug - intentionally returns falsy values)
VERIFIED: Fixed Within PR #1500:
- ✅ N+1 queries for staff roles (introduced commit 99aec49922, fixed commit 1b878d6c55)
- ✅ Lambda duplication (introduced commit 99aec49922, fixed commit a5819fec1d)
VERIFIED: Not Issues:
- ✅ XSS in formatter service (ERB::Util.html_escape present from day 1)
- ✅ Soft-delete regression (deleted_at commented out in PREVIOUS PR, #1500 only removed comment)
VERIFIED: Pre-Existing Issues (Not from PR #1500):
- Controller lambdas with XSS risks (existed before PR #1500)
- SQL injection whitelist missing
- N+1 queries in controller (patients_controller.rb:8290, 10679)
- Missing database indexes
- Authorization gaps (TODO comments)
- Input validation gaps
- Controller god object (11,323 lines)
- 130+ bare rescue statements
- Add error logging to JSON parsing (notes_controller.rb:50-56) instead of silent rescue
- Simplify snapshot parsing logic if possible
- Review if falsy value handling is intentional or needs fixing
- Document expected behavior for false/0/nil values
- Audit controller lambdas (patients_controller.rb) for XSS
- Add ERB::Util.html_escape to controller lambdas if missing
- Verify all 22+
.html_safeusages have proper sanitization
- Add ALLOWED_SORT_COLUMNS whitelist (adverse_event_line_list_service.rb:159-175)
- Validate column/direction parameters before SQL interpolation
- Add permission check to generate_rca_note (patients_controller.rb:7130)
- Review other endpoints for missing authorization
- Fix RCA records N+1 (patients_controller.rb:8290) - add eager loading
- Fix CA records N+1 (patients_controller.rb:10679) - add eager loading
- Create migration for pt_outcome, h_outcome indexes
- Add composite indexes for adverse_reaction_status_histories
- Add deleted_at index for soft-delete queries
- Optimize O(n²) queries in line_list_service
- Consider materialized views or cached columns
- Replace STR_TO_DATE with proper datetime columns
- Add model validations for pt_outcome, h_outcome (adverse_reaction.rb)
- Add length validations for cause_of_death, other_details
- Add date format validation
- Fix 130+ bare rescue statements - add specific exceptions
- Add proper error logging throughout
- Remove silent failures
- Add nil-safety for deleted records (notes_controller.rb:38-80)
- Handle cases where adverse_reaction is deleted but notes exist
- Extract AdverseEventsController from PatientsController
- Extract RcaNotesController from PatientsController
- Extract AdverseEventExportsController from PatientsController
- Target: Reduce PatientsController from 11,323 lines to <500
- Add controller tests for PDF generation with snapshots
- Add integration tests for snapshot functionality
- Add tests for deleted record scenarios
- Test false/0 value handling in snapshots
- Add query result caching for frequently-accessed lookups
- Cache role lookups, event type lookups
- Add Content-Security-Policy headers
Multi-agent review found mostly pre-existing issues, not regressions from PR #1500.
PR #1500 actually FIXED issues:
- N+1 queries (introduced early in PR, fixed before merge)
- Lambda duplication (introduced early in PR, fixed before merge)
False positives from review:
- Soft-delete regression (pre-existing, not from #1500)
- XSS in formatter service (proper escaping exists)
- Many "critical" issues existed before PR #1500
Actual new code in PR #1500:
- AdverseEventNoteFormatterService (876 lines, well-tested)
- Snapshot support in notes_controller (47 lines)
- Database normalization (outcome columns vs string parsing)