feat: Rework Lab Result Add and Table view functionaity#898
Open
robf17 wants to merge 7 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 11 minutes and 59 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds panel-aware lab results, textual lab test result support, a component-defaults lookup endpoint, panel libraries/templates, a new panel creation dialog, updated lab result and component views, and matching locale/test updates across backend and frontend code. ChangesLab panels and textual results
Estimated code review effort🎯 5 (Critical) | ⏱️ ~95 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/api/v1/endpoints/lab_test_component.py (2)
724-727:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
textualbefore the numeric statistics fallback.A textual component is neither qualitative nor quantitative; today it falls into
_calculate_quantitative_statistics, which returnsresult_type="quantitative"and treats missing numeric values as abnormal.Proposed fix
if result_type == "qualitative": return _calculate_qualitative_statistics(components, count) + if result_type == "textual": + normal_count = sum(1 for c in components if c.status == "normal") + return LabTestComponentTrendStatistics( + count=count, + latest=None, + average=None, + min=None, + max=None, + std_dev=None, + trend_direction="stable", + time_in_range_percent=( + round((normal_count / count * 100), 1) if count > 0 else None + ), + normal_count=normal_count, + abnormal_count=count - normal_count, + result_type="textual", + ) + return _calculate_quantitative_statistics(components, count)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/endpoints/lab_test_component.py` around lines 724 - 727, The code currently only explicitly handles qualitative result types, causing textual components to incorrectly fall through to _calculate_quantitative_statistics which treats them as numeric data. Add a conditional check for result_type == "textual" before the final return statement that calls _calculate_quantitative_statistics, and route textual components to an appropriate handler (likely _calculate_textual_statistics or similar) that correctly processes text-based components instead of attempting numeric calculations on them.
531-557:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
textual_valuethrough every manual component adapter. The new entry UI captures and submits textual results, but these manual adapters drop the field, so saved textual reports can come back blank in detail, patient component, trend, and stack drill-down views.
app/api/v1/endpoints/lab_test_component.py#L531-L557: passtextual_value=comp.textual_valueintoLabTestComponentForStack.app/api/v1/endpoints/lab_test_component.py#L125-L155: include"textual_value": db_component.textual_valuein the single-component response dict.app/api/v1/endpoints/lab_test_component.py#L940-L957: includetextual_value=component.textual_valueinLabTestComponentTrendDataPoint.frontend/src/pages/medical/LabResults.jsx#L395-L414: includetextual_value: comp.textual_value ?? nullwhen building component pseudo-results for stack drill-down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/endpoints/lab_test_component.py` around lines 531 - 557, The textual_value field captured in the new entry UI is being dropped across multiple component adapters, causing saved textual results to appear blank in various views. Fix this by adding the textual_value field to all manual component adapters: In app/api/v1/endpoints/lab_test_component.py at lines 531-557, add textual_value=comp.textual_value to the LabTestComponentForStack initialization. In app/api/v1/endpoints/lab_test_component.py at lines 125-155, include "textual_value": db_component.textual_value in the single-component response dictionary. In app/api/v1/endpoints/lab_test_component.py at lines 940-957, add textual_value=component.textual_value to the LabTestComponentTrendDataPoint initialization. In frontend/src/pages/medical/LabResults.jsx at lines 395-414, include textual_value: comp.textual_value ?? null when constructing component pseudo-results for stack drill-down.shared/data/test_library.json (1)
2-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBump the catalog metadata for the new
result_typeschema.The file now carries a new field on every test entry, but the top-level version/date still describe the previous catalog. Consumers using these metadata for cache invalidation or generated-data validation can miss the schema change.
Proposed fix
- "version": "1.8.0", - "lastUpdated": "2026-05-23", + "version": "1.9.0", + "lastUpdated": "2026-06-15",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/data/test_library.json` around lines 2 - 3, The catalog metadata in test_library.json needs to be updated to reflect the addition of the new result_type field to test entries. Update the "version" field to a higher version number (increment it appropriately based on your versioning scheme) and update the "lastUpdated" field with the current date to signal to consumers that the schema has changed and cache invalidation or data validation may be necessary.frontend/src/components/medical/labresults/TestComponentTemplates.tsx (1)
261-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCustom-entry filter incorrectly requires
unitfor non-quantitative rows.At Line 261–264, custom-entry submission enforces
component.unit.trim() !== ''unconditionally. That rejects valid qualitative/textual rows (where unit is legitimately empty), causing rows to be dropped before bulk submit.Suggested fix
if (selectedTemplate?.id === 'custom_entry') { - return ( - component.test_name.trim() !== '' && component.unit.trim() !== '' - ); + return component.test_name.trim() !== '' && ( + component.result_type !== 'quantitative' || + component.unit.trim() !== '' + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/TestComponentTemplates.tsx` around lines 261 - 264, The custom-entry validation filter at the selectedTemplate?.id === 'custom_entry' condition unconditionally requires both test_name and unit to be non-empty, but this incorrectly rejects valid qualitative or textual rows where unit is legitimately empty. Modify the validation logic so that the unit field is only required for quantitative rows. Check the row type or component properties to determine if it is a quantitative entry before enforcing the unit requirement; qualitative/textual rows should only require a non-empty test_name.frontend/src/components/medical/labresults/LabResultFormWrapper.jsx (1)
342-350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-add the
hematologyoption to preserve existing category values.Removing
hematologyfrom selectable categories can strand existing records with that value and make edits silently inconsistent.Suggested fix
const categoryOptions = [ { value: 'blood work', label: t('labresults:category.bloodWork') }, { value: 'imaging', label: t('labresults:category.imaging') }, { value: 'pathology', label: t('labresults:category.pathology') }, { value: 'microbiology', label: t('labresults:category.microbiology') }, { value: 'chemistry', label: t('labresults:category.chemistry') }, + { value: 'hematology', label: t('labresults:category.hematology') }, { value: 'hepatology', label: t('labresults:category.hepatology') },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/LabResultFormWrapper.jsx` around lines 342 - 350, The categoryOptions array in LabResultFormWrapper.jsx is missing the hematology option that was previously present. Add back the hematology option to the categoryOptions array with its corresponding translation key to ensure existing lab result records with hematology category values remain selectable and editable, preventing data inconsistency issues.frontend/src/components/shared/MedicalPageActions.jsx (1)
77-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
rightChildrenin the early-return guard.Line [77]-Line [82] returns
nullwithout consideringrightChildren, so a validrightChildren-only usage never renders.Suggested fix
if ( !primaryVisible && visibleSecondaryActions.length === 0 && !shouldShowViewToggle && - !children + !children && + !rightChildren ) { return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/shared/MedicalPageActions.jsx` around lines 77 - 82, The early-return guard condition in MedicalPageActions.jsx that checks multiple flags and returns null does not account for rightChildren. This causes the component to return null even when rightChildren is provided and should be rendered. Add rightChildren to the early-return condition by including !rightChildren in the existing guard statement alongside the checks for !primaryVisible, visibleSecondaryActions.length === 0, !shouldShowViewToggle, and !children. This ensures the component only returns null when all renderable content is absent, including rightChildren.
🟡 Minor comments (8)
frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx-145-145 (1)
145-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t silently no-op when
currentPatientis missing.Line 145 returns without any user feedback; the action appears broken. Set an explicit error message before returning.
Proposed fix
- if (!currentPatient?.id) return; + if (!currentPatient?.id) { + setError(t('medical:labResults.addPanel.createError')); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx` at line 145, The guard clause checking for currentPatient?.id in TestPanelCreateDialog silently returns without providing any feedback to the user, making the action appear broken. Before the return statement in this guard clause, set an explicit error message (such as using a notification/toast or error state) to inform the user that the patient information is missing and the action cannot proceed.frontend/src/components/medical/labresults/LabResultViewModal.jsx-275-277 (1)
275-277:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a namespaced key for
notCompletedto avoid fallback English.Line 277 and Line 421 use
t('labels.notCompleted', 'Not completed'); this key is not namespaced like nearby calls and can bypass locale translations.Suggested patch
- : t('labels.notCompleted', 'Not completed')} + : t('common:labels.notCompleted', 'Not completed')}Also applies to: 419-421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/LabResultViewModal.jsx` around lines 275 - 277, The translation key t('labels.notCompleted', 'Not completed') used at lines 275-277 and lines 419-421 in LabResultViewModal.jsx is not properly namespaced, which can cause the locale translations to be bypassed in favor of the English fallback. Update both occurrences of this key to use proper namespacing that matches the pattern of nearby translation calls in the file, ensuring consistent namespace formatting across both locations.frontend/public/locales/th/medical.json-825-833 (1)
825-833:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNew Thai locale entries are still in English.
These keys will render mixed-language UI in the Thai experience and should be translated before release.
Also applies to: 836-837
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/public/locales/th/medical.json` around lines 825 - 833, The Thai locale file contains English text for several test panel-related entries that should be translated to Thai. In the medical.json file, translate the English values for the keys title, panelName, panelNamePlaceholder, panelNameDescription, createButton, creating, createError, and testResultRequired from their current English text to Thai equivalents. The tagsPlaceholder key is already translated correctly. Ensure all translations maintain consistency with the existing Thai translations in the file and provide contextually appropriate Thai translations for each field to deliver a fully localized experience for Thai users.frontend/src/components/medical/labresults/LabResultsComponentTable.tsx-329-336 (1)
329-336:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winImprove keyboard/screen-reader accessibility for filter/category toggles.
These controls are mouse-first right now (missing labels on icon-only actions and no keyboard handler/focus semantics on category row toggle).
♿ Suggested fix
<ActionIcon variant={filtersExpanded ? 'filled' : 'light'} color={hasActiveFilters ? 'blue' : 'gray'} size="lg" onClick={() => setFiltersExpanded(prev => !prev)} + aria-label={t('common:filters.title', 'Filters & Search')} > @@ <ActionIcon variant="subtle" color="gray" onClick={() => setFiltersExpanded(prev => !prev)} style={{ flexShrink: 0 }} + aria-label={ + filtersExpanded + ? t('common:buttons.collapse', 'Collapse filters') + : t('common:buttons.expand', 'Expand filters') + } > @@ <Table.Tr style={{ cursor: 'pointer', backgroundColor: `var(--mantine-color-${catColor}-0)` }} onClick={() => handleToggleCategory(category)} + onKeyDown={e => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleToggleCategory(category); + } + }} + tabIndex={0} + role="button" + aria-expanded={!isCategoryCollapsed} data-testid={`category-row-${category}`} >Also applies to: 358-365, 489-493
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/LabResultsComponentTable.tsx` around lines 329 - 336, The ActionIcon components used as filter and category toggles lack proper accessibility support for keyboard navigation and screen readers. In LabResultsComponentTable.tsx at lines 329-336, 358-365, and 489-493, add an aria-label attribute to each icon-only ActionIcon button to provide descriptive text for screen readers. For the category row toggle at lines 358-365, additionally ensure the parent element has appropriate keyboard handling semantics (role and onKeyDown handler if needed) to support keyboard navigation. Ensure all icon-only toggle controls follow accessibility best practices by including clear, descriptive labels that explain the button's purpose.frontend/public/locales/es/labresults.json-336-336 (1)
336-336:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the Spanish locale fully translated for category labels.
"Imágenes - Radiología & Scans"mixes English and Spanish in one UI label.Suggested fix
- "imaging": "Imágenes - Radiología & Scans" + "imaging": "Imágenes - Radiología y escaneos"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/public/locales/es/labresults.json` at line 336, The "imaging" key in the Spanish locale file contains a mixed-language translation with English "Scans" alongside Spanish text. Replace the English word "Scans" with its proper Spanish translation to ensure the entire label is in Spanish, maintaining consistency across the locale file.frontend/src/components/medical/labresults/TestComponentTrendsPanel.tsx-252-257 (1)
252-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTextual CSV export still includes numeric-only summary rows.
isTextualupdates headers/rows, but summary generation still falls through toLatest/Average/Min/Max/Std Dev, which are not meaningful for textual trends.Suggested fix
- ...(isQualitative && trendData.statistics.qualitative_summary - ? Object.entries(trendData.statistics.qualitative_summary).map( - ([val, cnt]) => [val, String(cnt)] - ) - : [ - ['Latest', trendData.statistics.latest?.toFixed(2) || 'N/A'], - ['Average', trendData.statistics.average?.toFixed(2) || 'N/A'], - ['Min', trendData.statistics.min?.toFixed(2) || 'N/A'], - ['Max', trendData.statistics.max?.toFixed(2) || 'N/A'], - ['Std Dev', trendData.statistics.std_dev?.toFixed(2) || 'N/A'], - ]), + ...(isTextual + ? [['Textual Results', 'Numeric statistics are not available']] + : isQualitative && trendData.statistics.qualitative_summary + ? Object.entries(trendData.statistics.qualitative_summary).map( + ([val, cnt]) => [val, String(cnt)] + ) + : [ + ['Latest', trendData.statistics.latest?.toFixed(2) || 'N/A'], + ['Average', trendData.statistics.average?.toFixed(2) || 'N/A'], + ['Min', trendData.statistics.min?.toFixed(2) || 'N/A'], + ['Max', trendData.statistics.max?.toFixed(2) || 'N/A'], + ['Std Dev', trendData.statistics.std_dev?.toFixed(2) || 'N/A'], + ]),Also applies to: 307-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/TestComponentTrendsPanel.tsx` around lines 252 - 257, The isTextual variable correctly updates the headers for textual trends around lines 252-257, but the summary row generation logic (around lines 307-317) still falls through to creating numeric-only summary statistics like Latest/Average/Min/Max/Std Dev, which are meaningless for textual data. Add a conditional check for isTextual in the summary generation section to skip numeric summary calculations when the trend data is textual, ensuring that textual CSV exports do not include inappropriate numeric summary rows.frontend/public/locales/ru/medical.json-824-837 (1)
824-837:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRussian locale contains untranslated English strings in new panel flow.
The new
labResults.addPanel.*andlabResults.buttons.addLabResultsvalues are in English, which will surface mixed-language UI forru.Suggested fix
"addPanel": { - "title": "Add New Test Panel", - "panelName": "Panel Name", - "panelNamePlaceholder": "e.g. CBC Panel, Annual Bloodwork", - "panelNameDescription": "A name for this group of tests", - "createButton": "Create", - "creating": "Creating panel...", - "createError": "Failed to create test panel", - "testResultRequired": "At least one test result is required", + "title": "Добавить новую панель тестов", + "panelName": "Название панели", + "panelNamePlaceholder": "например, Панель ОАК, Ежегодные анализы", + "panelNameDescription": "Название для этой группы тестов", + "createButton": "Создать", + "creating": "Создание панели...", + "createError": "Не удалось создать панель тестов", + "testResultRequired": "Требуется как минимум один результат теста", "tagsPlaceholder": "Добавьте теги для удобной организации и поиска этой записи позже" }, "buttons": { - "addLabResults": "Add Lab Results" + "addLabResults": "Добавить результаты анализов" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/public/locales/ru/medical.json` around lines 824 - 837, The Russian locale file (frontend/public/locales/ru/medical.json) contains English strings that must be translated to Russian. Specifically, in the addPanel object, translate all the English values: title (Add New Test Panel), panelName (Panel Name), panelNamePlaceholder (e.g. CBC Panel, Annual Bloodwork), panelNameDescription (A name for this group of tests), createButton (Create), creating (Creating panel...), createError (Failed to create test panel), and testResultRequired (At least one test result is required). Additionally, translate the addLabResults button label in the buttons section. Ensure all translations are accurate and maintain consistency with the existing Russian translations in the file, then remove the English strings and replace them with their Russian equivalents.frontend/public/locales/pl/medical.json-825-832 (1)
825-832:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate new
labResults.addPanel/buttonsstrings to Polish.Line 825-832 and Line 836 introduce English UI text in the Polish locale file, causing mixed-language UX in production.
💡 Suggested locale fix
- "title": "Add New Test Panel", - "panelName": "Panel Name", - "panelNamePlaceholder": "e.g. CBC Panel, Annual Bloodwork", - "panelNameDescription": "A name for this group of tests", - "createButton": "Create", - "creating": "Creating panel...", - "createError": "Failed to create test panel", - "testResultRequired": "At least one test result is required", + "title": "Dodaj nowy panel badań", + "panelName": "Nazwa panelu", + "panelNamePlaceholder": "np. Panel morfologii, Roczne badania krwi", + "panelNameDescription": "Nazwa dla tej grupy badań", + "createButton": "Utwórz", + "creating": "Tworzenie panelu...", + "createError": "Nie udało się utworzyć panelu badań", + "testResultRequired": "Wymagany jest co najmniej jeden wynik badania", @@ - "addLabResults": "Add Lab Results" + "addLabResults": "Dodaj wyniki laboratoryjne"Also applies to: 836-836
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/public/locales/pl/medical.json` around lines 825 - 832, The Polish locale file contains English text for new labResults.addPanel strings ("Add New Test Panel", "panelName", "panelNamePlaceholder", "panelNameDescription", "createButton", "creating", "createError", "testResultRequired") at lines 825-832 and an additional string at line 836, which creates a mixed-language user experience. Translate each of these English string values to Polish while keeping the JSON key names unchanged. Ensure all new UI text introduced in this section has proper Polish translations to maintain consistent localization.
🧹 Nitpick comments (4)
frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx (1)
271-273: ⚡ Quick winUse a real fallback for unmapped panel categories.
Line 272 falls back to
'', but this path should degrade to a valid form category to avoid emptytest_categorywhen library data evolves.Proposed fix
- const category = panel - ? (PANEL_CATEGORY_TO_FORM_CATEGORY[panel.category] ?? '') - : ''; + const category = panel + ? (PANEL_CATEGORY_TO_FORM_CATEGORY[panel.category] ?? 'blood work') + : '';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx` around lines 271 - 273, The code mapping panel.category to a form category using PANEL_CATEGORY_TO_FORM_CATEGORY currently falls back to an empty string when the mapping is not found. Replace this empty string fallback with a valid default form category value to ensure the test_category field has a sensible value even when new panel categories are added to the library data that aren't yet in the PANEL_CATEGORY_TO_FORM_CATEGORY mapping. Identify an appropriate default form category from your application's valid category options and use that instead of the empty string fallback in the ternary expression.frontend/src/components/medical/labresults/__tests__/TestPanelCreateDialog.test.tsx (1)
220-457: ⚡ Quick winAdd a regression test for create-success + bulk-failure sequencing.
Current suite does not cover the case where
createLabResultsucceeds butcreateBulkForLabResultfails. Add this to prevent regressions around duplicate panel creation behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/__tests__/TestPanelCreateDialog.test.tsx` around lines 220 - 457, Add a new test case to cover the scenario where mockCreateLabResult succeeds but mockCreateBulkForLabResult fails. This test should mock createLabResult to resolve with a valid lab result, mock createBulkForLabResult to reject with an error (such as a network error or API failure), fill in the panel name and at least one test component, click Create Panel, and then verify that an error alert is displayed and onCreateSuccess is not called. This prevents regressions where a panel is created successfully in the database but fails to associate its component tests, leaving an orphaned panel.frontend/src/components/medical/labresults/__tests__/LabResultsComponentTable.test.tsx (1)
343-361: ⚡ Quick win
filters by facilitydoes not test filtering behavior yet.This test currently verifies only the initial unfiltered state, so it won’t catch a broken facility filter implementation.
Proposed test completion
it('filters by facility', () => { @@ - // Both visible initially + // Both visible initially expect(screen.getByTestId('summary-row-glucose')).toBeInTheDocument(); expect(screen.getByTestId('summary-row-hemoglobin')).toBeInTheDocument(); + + // Apply facility filter (adjust selector to the real control label/testid) + fireEvent.change(screen.getByPlaceholderText('Facility'), { target: { value: 'Lab A' } }); + + expect(screen.getByTestId('summary-row-glucose')).toBeInTheDocument(); + expect(screen.queryByTestId('summary-row-hemoglobin')).not.toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/__tests__/LabResultsComponentTable.test.tsx` around lines 343 - 361, The `filters by facility` test currently only verifies the initial unfiltered state where both the glucose and hemoglobin components are visible, but does not actually exercise the facility filtering functionality. Complete this test by adding steps that apply a facility filter (such as selecting "Lab A" or "Lab B" through the component's filter UI or state), then verify that only the component matching the selected facility remains visible in the document while the other is removed or hidden. This will ensure the facility filter implementation is properly validated.frontend/src/components/medical/labresults/__tests__/TestComponentCatalog.test.tsx (1)
245-268: ⚡ Quick winAdd regression tests for the new non-search filters.
The suite now covers search filtering, but not the newly added date/practitioner/facility filters. Adding at least one test per filter path would protect the new catalog logic from regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/__tests__/TestComponentCatalog.test.tsx` around lines 245 - 268, The test suite in TestComponentCatalog.test.tsx currently has tests for search filtering but is missing regression tests for the newly added non-search filters (date, practitioner, and facility filters). Add at least one test for each of these three filter types to verify that when each filter is applied independently, the component correctly filters the catalog results and displays only the matching items. These tests should follow the same pattern as the existing search filter tests, using fireEvent to interact with the filter controls and asserting the filtered card results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/v1/endpoints/lab_test_component.py`:
- Around line 650-677: The current lookup order prioritizes test_name over
test_code, which causes incorrect results when both are provided, as it may
match a historical component with the same name but different code. Reverse the
lookup order to check test_code first (the more specific identifier) before
falling back to test_name. Specifically, refactor the condition blocks so that
the test_code query using sqlfunc operations executes first and only falls back
to the get_by_patient_and_test_name call if no component was found by code. This
ensures that when callers provide both test_name and test_code, the exact
test_code match takes precedence, preventing mismatches with wrong
unit/reference ranges.
- Around line 685-692: The endpoint returning TestComponentDefaults is missing
audit logging for PHI access. Add a log_data_access call before the return
statement to properly audit access to patient test metadata, consistent with how
nearby patient component read endpoints handle audit logging. This ensures PHI
access is properly recorded for compliance purposes.
In `@app/schemas/lab_test_component.py`:
- Around line 447-463: The validate_result_type_fields method in
LabTestComponent only validates constraints for qualitative and textual result
types, but omits validation for quantitative transitions. Add a validation block
after the existing qualitative/textual checks that enforces the quantitative
constraint: when rt equals "quantitative", ensure that self.value is not None,
raising a ValueError with a descriptive message if the numeric value is empty,
since quantitative tests require a numeric value to be logically consistent.
In
`@frontend/src/components/medical/labresults/__tests__/LabResultsComponentTable.test.tsx`:
- Around line 22-27: The vi.mock() paths in the test file are using incorrect
relative paths that do not point to the actual module locations, causing the
component to use real modules instead of mocks. Fix the three mock declarations
by adjusting their paths: change the useDateFormat mock path from
../../../hooks/useDateFormat to ../../../../hooks/useDateFormat, change the
DateInput mock path from ../../adapters/DateInput to
../../../adapters/DateInput, and change the logger mock path from
../../../services/logger to ../../../../services/logger. These corrected paths
will ensure that the vi.mock() calls properly intercept and isolate the actual
modules used by the component under test.
In `@frontend/src/components/medical/labresults/LabResultsComponentTable.tsx`:
- Around line 88-90: The toDateStr function has two issues affecting date
filtering: toISOString() converts dates to UTC, causing timezone shifts when
comparing against local date filters, and null/undated records are not being
properly excluded when date filters are active. Fix the toDateStr function to
use local date representation (such as constructing the date string in
YYYY-MM-DD format using local date components instead of toISOString()) to
preserve timezone context. Additionally, review the filter logic around lines
192-195 that uses this function to ensure that records with null dates are
explicitly excluded when date range filters are active, preventing undated
records from leaking through the filter.
In `@frontend/src/components/medical/labresults/TestComponentBulkEntry.tsx`:
- Around line 818-823: When setting component.result_type to 'textual' at line
818 in the standardized test assignment block, you must also ensure that the
textual_value field is properly populated or initialized. The validation logic
at lines 967 and 1150 requires textual_value to exist for textual result types,
and without it, rows will be silently filtered out during submit. Ensure that
whenever result_type is set to 'textual' in this flow, the corresponding
textual_value is also set appropriately. Additionally, check the validation
checks at lines 963-970 and 1146-1153 to confirm they are consistent with the
initialization approach and that no other code paths have the same issue where
textual_value is missing when result_type is textual.
In `@frontend/src/components/medical/labresults/TestComponentCatalog.tsx`:
- Around line 323-330: The ActionIcon components used for filter toggle controls
are icon-only and lack accessible labels, making them inaccessible to screen
readers. Add an aria-label attribute to both ActionIcon instances to provide an
accessible name that describes the button's function. In the
TestComponentCatalog.tsx file at lines 323-330 (the anchor location with the
IconFilter component showing the filter toggle button) and at lines 352-359 (the
sibling location with the same pattern), add aria-label="Toggle filters" or
similar descriptive text to the ActionIcon element to ensure screen reader users
understand the button's purpose.
- Around line 206-208: The date filtering logic in the filter condition near
getComponentDate(c) does not exclude components that have no date when date
filters are active. Currently, when fromStr or toStr filters are set but dateStr
is falsy (meaning no date exists), these components still pass through the
filter because the conditions only evaluate when dateStr exists. Add a check
that returns false (filters out the component) if either a fromStr or toStr
filter is active but dateStr is missing, ensuring that date-filtered results
only include components with actual dates to compare against.
- Around line 103-106: In the toDateStr function, replace the current
implementation that uses toISOString() with a local-timezone-aware date
formatting approach. Instead of calling d.toISOString().slice(0, 10) which
converts to UTC and causes off-by-one filtering for non-UTC users, use the local
date component getter methods (getFullYear, getMonth, getDate) to build the date
string in YYYY-MM-DD format based on the user's local timezone. This ensures the
returned date string corresponds to the actual date selected in the date picker
rather than the UTC-shifted date.
In `@frontend/src/components/medical/labresults/TestComponentDisplay.tsx`:
- Around line 317-319: The condition at the reference range display block in
TestComponentDisplay.tsx checks strictly for component.result_type ===
'quantitative', which excludes legacy numeric records where result_type is not
populated. Modify the conditional check to include cases where result_type is
either 'quantitative' or undefined/missing, so that both new quantitative
results and older unpopulated records can display reference ranges. This allows
legacy components to remain eligible for range display while maintaining support
for the newer typed records.
In `@frontend/src/components/medical/labresults/TestComponentEditModal.tsx`:
- Around line 297-299: The isSubmitDisabled validation in the submit button
logic is insufficient because it only checks if test_name has content, but does
not validate that the payload is complete based on the result type. The
validation needs to be result-type-aware by checking that required fields are
populated according to whether the test is qualitative or textual. Update the
isSubmitDisabled logic to include additional checks that verify the appropriate
result fields (such as result_value for qualitative tests or result_text for
textual tests) are filled in alongside test_name, and apply the same
comprehensive validation logic at lines 518-564 where similar submit/save
operations occur.
In `@frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx`:
- Around line 179-223: The bulk component creation in the
labTestComponentApi.createBulkForLabResult call is wrapped in the same try-catch
as the initial lab result creation. If the bulk creation fails with an
exception, it propagates to the catch block which shows a full operation failure
even though the panel was already successfully created at line 179. Refactor the
error handling so that bulk component failures do not prevent the success path:
move the bulk component creation and its error handling outside of the main
try-catch block (or into its own nested try-catch), ensuring that the success
logging, form reset, and onCreateSuccess callback are always executed when the
lab result itself is created successfully. The partial failure logging for
bulkResult.errors should remain, but exceptions from bulk creation should be
caught separately and logged without triggering the full operation failure
message.
In `@frontend/src/constants/panelTemplateMap.ts`:
- Around line 3-14: The TestTemplate interface currently lacks per-test category
support, causing all tests to inherit the single template-level category. To fix
this, add an optional category field to the test object within the tests array
in the TestTemplate interface, then update the row generation logic at both
affected locations (around Line 5 and the range 198-205) to use the per-test
category when available, falling back to template.category if the individual
test category is not provided. This preserves backward compatibility while
enabling per-test category fidelity.
In `@frontend/src/pages/medical/LabResults.jsx`:
- Around line 1052-1064: The LabResultsComponentTable component rendering block
dereferences currentPatient.id without first verifying that currentPatient
exists, which will cause a crash when the table layout is active but the patient
context is not ready. Add a guard condition before the tableLayout return
statement to check that currentPatient exists (similar to how the catalog branch
handles missing patient context), and only render the LabResultsComponentTable
if the patient is available. If currentPatient is not ready, return a loading
state or placeholder instead.
---
Outside diff comments:
In `@app/api/v1/endpoints/lab_test_component.py`:
- Around line 724-727: The code currently only explicitly handles qualitative
result types, causing textual components to incorrectly fall through to
_calculate_quantitative_statistics which treats them as numeric data. Add a
conditional check for result_type == "textual" before the final return statement
that calls _calculate_quantitative_statistics, and route textual components to
an appropriate handler (likely _calculate_textual_statistics or similar) that
correctly processes text-based components instead of attempting numeric
calculations on them.
- Around line 531-557: The textual_value field captured in the new entry UI is
being dropped across multiple component adapters, causing saved textual results
to appear blank in various views. Fix this by adding the textual_value field to
all manual component adapters: In app/api/v1/endpoints/lab_test_component.py at
lines 531-557, add textual_value=comp.textual_value to the
LabTestComponentForStack initialization. In
app/api/v1/endpoints/lab_test_component.py at lines 125-155, include
"textual_value": db_component.textual_value in the single-component response
dictionary. In app/api/v1/endpoints/lab_test_component.py at lines 940-957, add
textual_value=component.textual_value to the LabTestComponentTrendDataPoint
initialization. In frontend/src/pages/medical/LabResults.jsx at lines 395-414,
include textual_value: comp.textual_value ?? null when constructing component
pseudo-results for stack drill-down.
In `@frontend/src/components/medical/labresults/LabResultFormWrapper.jsx`:
- Around line 342-350: The categoryOptions array in LabResultFormWrapper.jsx is
missing the hematology option that was previously present. Add back the
hematology option to the categoryOptions array with its corresponding
translation key to ensure existing lab result records with hematology category
values remain selectable and editable, preventing data inconsistency issues.
In `@frontend/src/components/medical/labresults/TestComponentTemplates.tsx`:
- Around line 261-264: The custom-entry validation filter at the
selectedTemplate?.id === 'custom_entry' condition unconditionally requires both
test_name and unit to be non-empty, but this incorrectly rejects valid
qualitative or textual rows where unit is legitimately empty. Modify the
validation logic so that the unit field is only required for quantitative rows.
Check the row type or component properties to determine if it is a quantitative
entry before enforcing the unit requirement; qualitative/textual rows should
only require a non-empty test_name.
In `@frontend/src/components/shared/MedicalPageActions.jsx`:
- Around line 77-82: The early-return guard condition in MedicalPageActions.jsx
that checks multiple flags and returns null does not account for rightChildren.
This causes the component to return null even when rightChildren is provided and
should be rendered. Add rightChildren to the early-return condition by including
!rightChildren in the existing guard statement alongside the checks for
!primaryVisible, visibleSecondaryActions.length === 0, !shouldShowViewToggle,
and !children. This ensures the component only returns null when all renderable
content is absent, including rightChildren.
In `@shared/data/test_library.json`:
- Around line 2-3: The catalog metadata in test_library.json needs to be updated
to reflect the addition of the new result_type field to test entries. Update the
"version" field to a higher version number (increment it appropriately based on
your versioning scheme) and update the "lastUpdated" field with the current date
to signal to consumers that the schema has changed and cache invalidation or
data validation may be necessary.
---
Minor comments:
In `@frontend/public/locales/es/labresults.json`:
- Line 336: The "imaging" key in the Spanish locale file contains a
mixed-language translation with English "Scans" alongside Spanish text. Replace
the English word "Scans" with its proper Spanish translation to ensure the
entire label is in Spanish, maintaining consistency across the locale file.
In `@frontend/public/locales/pl/medical.json`:
- Around line 825-832: The Polish locale file contains English text for new
labResults.addPanel strings ("Add New Test Panel", "panelName",
"panelNamePlaceholder", "panelNameDescription", "createButton", "creating",
"createError", "testResultRequired") at lines 825-832 and an additional string
at line 836, which creates a mixed-language user experience. Translate each of
these English string values to Polish while keeping the JSON key names
unchanged. Ensure all new UI text introduced in this section has proper Polish
translations to maintain consistent localization.
In `@frontend/public/locales/ru/medical.json`:
- Around line 824-837: The Russian locale file
(frontend/public/locales/ru/medical.json) contains English strings that must be
translated to Russian. Specifically, in the addPanel object, translate all the
English values: title (Add New Test Panel), panelName (Panel Name),
panelNamePlaceholder (e.g. CBC Panel, Annual Bloodwork), panelNameDescription (A
name for this group of tests), createButton (Create), creating (Creating
panel...), createError (Failed to create test panel), and testResultRequired (At
least one test result is required). Additionally, translate the addLabResults
button label in the buttons section. Ensure all translations are accurate and
maintain consistency with the existing Russian translations in the file, then
remove the English strings and replace them with their Russian equivalents.
In `@frontend/public/locales/th/medical.json`:
- Around line 825-833: The Thai locale file contains English text for several
test panel-related entries that should be translated to Thai. In the
medical.json file, translate the English values for the keys title, panelName,
panelNamePlaceholder, panelNameDescription, createButton, creating, createError,
and testResultRequired from their current English text to Thai equivalents. The
tagsPlaceholder key is already translated correctly. Ensure all translations
maintain consistency with the existing Thai translations in the file and provide
contextually appropriate Thai translations for each field to deliver a fully
localized experience for Thai users.
In `@frontend/src/components/medical/labresults/LabResultsComponentTable.tsx`:
- Around line 329-336: The ActionIcon components used as filter and category
toggles lack proper accessibility support for keyboard navigation and screen
readers. In LabResultsComponentTable.tsx at lines 329-336, 358-365, and 489-493,
add an aria-label attribute to each icon-only ActionIcon button to provide
descriptive text for screen readers. For the category row toggle at lines
358-365, additionally ensure the parent element has appropriate keyboard
handling semantics (role and onKeyDown handler if needed) to support keyboard
navigation. Ensure all icon-only toggle controls follow accessibility best
practices by including clear, descriptive labels that explain the button's
purpose.
In `@frontend/src/components/medical/labresults/LabResultViewModal.jsx`:
- Around line 275-277: The translation key t('labels.notCompleted', 'Not
completed') used at lines 275-277 and lines 419-421 in LabResultViewModal.jsx is
not properly namespaced, which can cause the locale translations to be bypassed
in favor of the English fallback. Update both occurrences of this key to use
proper namespacing that matches the pattern of nearby translation calls in the
file, ensuring consistent namespace formatting across both locations.
In `@frontend/src/components/medical/labresults/TestComponentTrendsPanel.tsx`:
- Around line 252-257: The isTextual variable correctly updates the headers for
textual trends around lines 252-257, but the summary row generation logic
(around lines 307-317) still falls through to creating numeric-only summary
statistics like Latest/Average/Min/Max/Std Dev, which are meaningless for
textual data. Add a conditional check for isTextual in the summary generation
section to skip numeric summary calculations when the trend data is textual,
ensuring that textual CSV exports do not include inappropriate numeric summary
rows.
In `@frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx`:
- Line 145: The guard clause checking for currentPatient?.id in
TestPanelCreateDialog silently returns without providing any feedback to the
user, making the action appear broken. Before the return statement in this guard
clause, set an explicit error message (such as using a notification/toast or
error state) to inform the user that the patient information is missing and the
action cannot proceed.
---
Nitpick comments:
In
`@frontend/src/components/medical/labresults/__tests__/LabResultsComponentTable.test.tsx`:
- Around line 343-361: The `filters by facility` test currently only verifies
the initial unfiltered state where both the glucose and hemoglobin components
are visible, but does not actually exercise the facility filtering
functionality. Complete this test by adding steps that apply a facility filter
(such as selecting "Lab A" or "Lab B" through the component's filter UI or
state), then verify that only the component matching the selected facility
remains visible in the document while the other is removed or hidden. This will
ensure the facility filter implementation is properly validated.
In
`@frontend/src/components/medical/labresults/__tests__/TestComponentCatalog.test.tsx`:
- Around line 245-268: The test suite in TestComponentCatalog.test.tsx currently
has tests for search filtering but is missing regression tests for the newly
added non-search filters (date, practitioner, and facility filters). Add at
least one test for each of these three filter types to verify that when each
filter is applied independently, the component correctly filters the catalog
results and displays only the matching items. These tests should follow the same
pattern as the existing search filter tests, using fireEvent to interact with
the filter controls and asserting the filtered card results.
In
`@frontend/src/components/medical/labresults/__tests__/TestPanelCreateDialog.test.tsx`:
- Around line 220-457: Add a new test case to cover the scenario where
mockCreateLabResult succeeds but mockCreateBulkForLabResult fails. This test
should mock createLabResult to resolve with a valid lab result, mock
createBulkForLabResult to reject with an error (such as a network error or API
failure), fill in the panel name and at least one test component, click Create
Panel, and then verify that an error alert is displayed and onCreateSuccess is
not called. This prevents regressions where a panel is created successfully in
the database but fails to associate its component tests, leaving an orphaned
panel.
In `@frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx`:
- Around line 271-273: The code mapping panel.category to a form category using
PANEL_CATEGORY_TO_FORM_CATEGORY currently falls back to an empty string when the
mapping is not found. Replace this empty string fallback with a valid default
form category value to ensure the test_category field has a sensible value even
when new panel categories are added to the library data that aren't yet in the
PANEL_CATEGORY_TO_FORM_CATEGORY mapping. Identify an appropriate default form
category from your application's valid category options and use that instead of
the empty string fallback in the ternary expression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 008d4b2f-1bdc-4e5e-ada0-f2a066fd5ec5
📒 Files selected for processing (81)
alembic/migrations/versions/20260613_1000_c5d6e7f8a9b0_add_is_panel_to_lab_results.pyalembic/migrations/versions/20260614_1000_d4e5f6a7b8c9_add_textual_value_to_lab_test_components.pyapp/api/v1/endpoints/lab_result.pyapp/api/v1/endpoints/lab_test_component.pyapp/core/constants.pyapp/models/labs.pyapp/schemas/lab_result.pyapp/schemas/lab_test_component.pyfrontend/public/locales/de/common.jsonfrontend/public/locales/de/labresults.jsonfrontend/public/locales/de/medical.jsonfrontend/public/locales/en/common.jsonfrontend/public/locales/en/labresults.jsonfrontend/public/locales/en/medical.jsonfrontend/public/locales/es/common.jsonfrontend/public/locales/es/labresults.jsonfrontend/public/locales/es/medical.jsonfrontend/public/locales/fr/common.jsonfrontend/public/locales/fr/labresults.jsonfrontend/public/locales/fr/medical.jsonfrontend/public/locales/it/common.jsonfrontend/public/locales/it/labresults.jsonfrontend/public/locales/it/medical.jsonfrontend/public/locales/nl/common.jsonfrontend/public/locales/nl/labresults.jsonfrontend/public/locales/nl/medical.jsonfrontend/public/locales/pl/common.jsonfrontend/public/locales/pl/labresults.jsonfrontend/public/locales/pl/medical.jsonfrontend/public/locales/pt/common.jsonfrontend/public/locales/pt/labresults.jsonfrontend/public/locales/pt/medical.jsonfrontend/public/locales/ru/common.jsonfrontend/public/locales/ru/labresults.jsonfrontend/public/locales/ru/medical.jsonfrontend/public/locales/sv/common.jsonfrontend/public/locales/sv/labresults.jsonfrontend/public/locales/sv/medical.jsonfrontend/public/locales/th/common.jsonfrontend/public/locales/th/labresults.jsonfrontend/public/locales/th/medical.jsonfrontend/public/locales/zh/common.jsonfrontend/public/locales/zh/labresults.jsonfrontend/public/locales/zh/medical.jsonfrontend/src/components/medical/MantineLabResultForm.jsxfrontend/src/components/medical/labresults/InlineTestComponentEntry.tsxfrontend/src/components/medical/labresults/LabResultCard.jsxfrontend/src/components/medical/labresults/LabResultFormWrapper.jsxfrontend/src/components/medical/labresults/LabResultStackCard.tsxfrontend/src/components/medical/labresults/LabResultStackPanel.tsxfrontend/src/components/medical/labresults/LabResultViewModal.jsxfrontend/src/components/medical/labresults/LabResultsComponentTable.tsxfrontend/src/components/medical/labresults/TestComponentBulkEntry.tsxfrontend/src/components/medical/labresults/TestComponentCatalog.tsxfrontend/src/components/medical/labresults/TestComponentDisplay.tsxfrontend/src/components/medical/labresults/TestComponentEditModal.tsxfrontend/src/components/medical/labresults/TestComponentStats.tsxfrontend/src/components/medical/labresults/TestComponentTemplates.tsxfrontend/src/components/medical/labresults/TestComponentTrendChart.tsxfrontend/src/components/medical/labresults/TestComponentTrendTable.tsxfrontend/src/components/medical/labresults/TestComponentTrendsPanel.tsxfrontend/src/components/medical/labresults/TestComponentsTab.tsxfrontend/src/components/medical/labresults/TestPanelCreateDialog.tsxfrontend/src/components/medical/labresults/__tests__/LabResultsComponentTable.test.tsxfrontend/src/components/medical/labresults/__tests__/TestComponentCatalog.test.tsxfrontend/src/components/medical/labresults/__tests__/TestPanelCreateDialog.test.tsxfrontend/src/components/shared/MedicalPageActions.jsxfrontend/src/components/shared/ViewToggle.jsxfrontend/src/constants/labCategories.tsfrontend/src/constants/panelLibrary.tsfrontend/src/constants/panelTemplateMap.test.tsfrontend/src/constants/panelTemplateMap.tsfrontend/src/pages/medical/LabResults.jsxfrontend/src/pages/medical/__tests__/LabResults.integration.test.jsxfrontend/src/services/api/labTestComponentApi.tsfrontend/src/utils/labTestComponentUtils.tsfrontend/src/utils/medicalPageConfigs/labResults.jsshared/data/panel_library.jsonshared/data/test_library.jsontests/api/test_lab_test_component_defaults.pytests/unit/test_lab_test_component_schema.py
💤 Files with no reviewable changes (2)
- frontend/src/utils/medicalPageConfigs/labResults.js
- frontend/src/components/medical/MantineLabResultForm.jsx
robf17
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/api/v1/endpoints/lab_test_component.py (2)
641-677:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize lookup inputs before querying defaults.
On Line 652 and Line 675, whitespace-only
test_code/test_namestill trigger lookups. A" "test_codeis normalized to""inside the SQL compare and can match blank-coded historical rows, returning unrelated defaults instead of properly falling back.Suggested fix
def get_component_defaults( @@ - if not test_name and not test_code: + normalized_name = test_name.strip() if test_name else "" + normalized_code = test_code.strip().upper() if test_code else "" + + if not normalized_name and not normalized_code: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail="At least one of test_name or test_code is required", ) @@ - if test_code: + if normalized_code: from sqlalchemy import func as sqlfunc from app.models.models import LabTestComponent as LTCModel from app.models.labs import LabResult as LRModel @@ LRModel.patient_id == patient_id, sqlfunc.upper(sqlfunc.trim(LTCModel.test_code)) - == test_code.strip().upper(), + == normalized_code, ) @@ - if component is None and test_name: + if component is None and normalized_name: results = lab_test_component.get_by_patient_and_test_name( - db, patient_id=patient_id, test_name=test_name.strip(), limit=1 + db, patient_id=patient_id, test_name=normalized_name, limit=1 )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/endpoints/lab_test_component.py` around lines 641 - 677, The inputs test_code and test_name are not normalized before being checked and used in database queries. Whitespace-only strings like " " remain truthy and trigger lookups, but get trimmed to empty strings inside SQL queries, potentially matching blank-coded historical rows. Normalize both test_code and test_name by stripping whitespace at the start of the function (before the validation check on line 641), then use these normalized values throughout the function. This ensures whitespace-only inputs become empty strings and fail the truthiness checks, properly falling back to alternative lookups instead of querying with blank values.
736-750:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTextual trend summaries currently degrade to
unknown.On Line 736,
textualis routed into qualitative stats, but Line 749 only readsqualitative_value. Textual rows then collapse into"unknown"buckets inqualitative_summary, which makes the trend summary inaccurate.Suggested fix
def _calculate_qualitative_statistics( @@ qualitative_summary: dict = {} for c in components: - qv = getattr(c, "qualitative_value", None) or "unknown" + qv = getattr(c, "qualitative_value", None) + if not qv and (getattr(c, "result_type", None) == "textual"): + qv = getattr(c, "textual_value", None) + qv = qv or "unknown" qualitative_summary[qv] = qualitative_summary.get(qv, 0) + 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/endpoints/lab_test_component.py` around lines 736 - 750, The _calculate_qualitative_statistics function handles both qualitative and textual types (routed from line 736) but only reads the qualitative_value attribute. For textual components that lack a qualitative_value attribute, the code defaults to "unknown", causing all textual data to collapse into "unknown" buckets and degrade trend accuracy. Update the function to check the component attributes and read the appropriate value field - use qualitative_value for qualitative components and textual_value for textual components when populating the qualitative_summary dictionary.frontend/src/components/medical/labresults/TestComponentEditModal.tsx (1)
385-410:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
statuswhen result type changes to prevent stale persisted state.On Line 386, switching result types clears value fields but leaves
statusuntouched. A prior qualitative status (normal/abnormal) can be submitted with textual/quantitative payloads and saved incorrectly.Suggested fix
onChange={value => { setFormData(prev => ({ ...prev, result_type: value as 'quantitative' | 'qualitative' | 'textual', + status: undefined, // Clear incompatible fields when switching result type ...(value === 'qualitative' ? {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/medical/labresults/TestComponentEditModal.tsx` around lines 385 - 410, The onChange handler for result type changes in the setFormData call does not reset the status field when switching between qualitative, textual, and quantitative types. This allows stale status values (like 'normal' or 'abnormal' from a previous qualitative result) to persist and be saved with incompatible payload types. Add status: undefined to each of the three clearing branches (when value === 'qualitative', when value === 'textual', and the default quantitative branch) to ensure the status is always reset alongside the other incompatible fields when the result type changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/api/v1/endpoints/lab_test_component.py`:
- Around line 641-677: The inputs test_code and test_name are not normalized
before being checked and used in database queries. Whitespace-only strings like
" " remain truthy and trigger lookups, but get trimmed to empty strings inside
SQL queries, potentially matching blank-coded historical rows. Normalize both
test_code and test_name by stripping whitespace at the start of the function
(before the validation check on line 641), then use these normalized values
throughout the function. This ensures whitespace-only inputs become empty
strings and fail the truthiness checks, properly falling back to alternative
lookups instead of querying with blank values.
- Around line 736-750: The _calculate_qualitative_statistics function handles
both qualitative and textual types (routed from line 736) but only reads the
qualitative_value attribute. For textual components that lack a
qualitative_value attribute, the code defaults to "unknown", causing all textual
data to collapse into "unknown" buckets and degrade trend accuracy. Update the
function to check the component attributes and read the appropriate value field
- use qualitative_value for qualitative components and textual_value for textual
components when populating the qualitative_summary dictionary.
In `@frontend/src/components/medical/labresults/TestComponentEditModal.tsx`:
- Around line 385-410: The onChange handler for result type changes in the
setFormData call does not reset the status field when switching between
qualitative, textual, and quantitative types. This allows stale status values
(like 'normal' or 'abnormal' from a previous qualitative result) to persist and
be saved with incompatible payload types. Add status: undefined to each of the
three clearing branches (when value === 'qualitative', when value === 'textual',
and the default quantitative branch) to ensure the status is always reset
alongside the other incompatible fields when the result type changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bede5495-7743-4fc0-8345-2edf699437db
📒 Files selected for processing (20)
app/api/v1/endpoints/lab_test_component.pyfrontend/public/locales/es/labresults.jsonfrontend/public/locales/pl/medical.jsonfrontend/public/locales/ru/medical.jsonfrontend/public/locales/th/medical.jsonfrontend/src/components/medical/labresults/LabResultFormWrapper.jsxfrontend/src/components/medical/labresults/LabResultViewModal.jsxfrontend/src/components/medical/labresults/LabResultsComponentTable.tsxfrontend/src/components/medical/labresults/TestComponentBulkEntry.tsxfrontend/src/components/medical/labresults/TestComponentCatalog.tsxfrontend/src/components/medical/labresults/TestComponentDisplay.tsxfrontend/src/components/medical/labresults/TestComponentEditModal.tsxfrontend/src/components/medical/labresults/TestComponentTemplates.tsxfrontend/src/components/medical/labresults/TestComponentTrendsPanel.tsxfrontend/src/components/medical/labresults/TestPanelCreateDialog.tsxfrontend/src/components/medical/labresults/__tests__/LabResultsComponentTable.test.tsxfrontend/src/components/shared/MedicalPageActions.jsxfrontend/src/constants/panelTemplateMap.test.tsfrontend/src/constants/panelTemplateMap.tsfrontend/src/pages/medical/LabResults.jsx
✅ Files skipped from review due to trivial changes (1)
- frontend/public/locales/th/medical.json
🚧 Files skipped from review as they are similar to previous changes (16)
- frontend/public/locales/pl/medical.json
- frontend/src/components/medical/labresults/TestComponentTrendsPanel.tsx
- frontend/src/constants/panelTemplateMap.ts
- frontend/src/components/shared/MedicalPageActions.jsx
- frontend/src/components/medical/labresults/TestComponentDisplay.tsx
- frontend/src/constants/panelTemplateMap.test.ts
- frontend/src/components/medical/labresults/TestComponentTemplates.tsx
- frontend/src/components/medical/labresults/LabResultViewModal.jsx
- frontend/src/components/medical/labresults/TestComponentBulkEntry.tsx
- frontend/src/components/medical/labresults/LabResultsComponentTable.tsx
- frontend/public/locales/es/labresults.json
- frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx
- frontend/src/components/medical/labresults/LabResultFormWrapper.jsx
- frontend/src/components/medical/labresults/TestComponentCatalog.tsx
- frontend/public/locales/ru/medical.json
- frontend/src/pages/medical/LabResults.jsx
Owner
Code reviewFound 3 issues:
MediKeep/app/schemas/lab_test_component.py Lines 250 to 268 in 27b4f6a (same removal in the Update validator: MediKeep/app/schemas/lab_test_component.py Lines 446 to 465 in 27b4f6a
MediKeep/frontend/src/components/medical/labresults/TestPanelCreateDialog.tsx Lines 188 to 212 in 27b4f6a
MediKeep/app/schemas/lab_test_component.py Lines 221 to 232 in 27b4f6a |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/schemas/lab_test_component.py (1)
486-503:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftClear incompatible persisted fields when switching to qualitative/textual.
Because this update schema is used with partial
exclude_unsetsemantics,self.value is Nonealso means “field omitted”. A request like{"result_type": "qualitative", "qualitative_value": "positive"}passes here but does not clear an existing numericvalue/reference range in the database, leaving a mixed quantitative/qualitative row. Require explicit null clears in the transition payload, or perform the cleanup in the update handler with the existing DB object.Possible schema-level guard
elif rt in ("qualitative", "textual"): + if "result_type" in self.model_fields_set: + if "value" not in self.model_fields_set or self.value is not None: + raise ValueError( + f"When switching to {rt}, numeric value must be explicitly cleared" + ) + for field_name in ("ref_range_min", "ref_range_max"): + if ( + field_name not in self.model_fields_set + or getattr(self, field_name) is not None + ): + raise ValueError( + f"When switching to {rt}, {field_name} must be explicitly cleared" + ) if self.value is not None: raise ValueError(f"Numeric value must be empty for {rt} tests")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/schemas/lab_test_component.py` around lines 486 - 503, The validation for switching to qualitative or textual result types does not properly ensure that incompatible numeric fields (value, ref_range_min, ref_range_max) are actually cleared from the database. Because the schema uses partial exclude_unset semantics, a None value means "field omitted" not "explicitly cleared". To fix this in the qualitative and textual validation branches around the ref_range_min/ref_range_max checks and the textual validation block, either require explicit null values for incompatible fields in the request payload (by checking if they are explicitly set to None rather than just checking if they are None), or move the cleanup logic to the update handler where you have access to both the existing database object and the incoming update data so you can perform the actual field clearing operations at the database level.
🧹 Nitpick comments (1)
tests/unit/test_lab_test_component_schema.py (1)
397-411: ⚡ Quick winAdd the sibling response-read regression for
textual_value.The schema now intentionally normalizes-only for
LabTestComponentResponse.textual_value, but this test block only pins theref_range_textcase. Add an over-limit textual response test so the new read-tolerance contract cannot regress.Suggested test
def test_response_tolerates_over_limit_ref_range_text(self): """Regression `#894`: Response must serialize ref_range_text longer than the input limit. Otherwise legacy/over-limit records crash the response with ResponseValidationError and become unviewable and unfixable in-app.""" @@ ) assert resp.ref_range_text == long_text + + def test_response_tolerates_over_limit_textual_value(self): + """Response must serialize textual_value longer than the input limit.""" + long_text = "x" * 5001 + resp = LabTestComponentResponse( + id=8, + test_name="Radiology Report", + lab_result_id=1, + result_type="textual", + value=None, + unit=None, + textual_value=long_text, + ) + assert resp.textual_value == long_text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_lab_test_component_schema.py` around lines 397 - 411, Add a new regression test method similar to test_response_tolerates_over_limit_ref_range_text that verifies LabTestComponentResponse can serialize textual_value that exceeds the MAX_TEXTUAL_VALUE_LENGTH limit without crashing. Create a test that instantiates LabTestComponentResponse with a textual_value set to a string longer than the schema limit (using LAB_TEST_COMPONENT_LIMITS for the threshold), and assert that the response's textual_value field equals the original long string, ensuring the read-tolerance contract for this field is protected from future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/schemas/lab_test_component.py`:
- Around line 486-503: The validation for switching to qualitative or textual
result types does not properly ensure that incompatible numeric fields (value,
ref_range_min, ref_range_max) are actually cleared from the database. Because
the schema uses partial exclude_unset semantics, a None value means "field
omitted" not "explicitly cleared". To fix this in the qualitative and textual
validation branches around the ref_range_min/ref_range_max checks and the
textual validation block, either require explicit null values for incompatible
fields in the request payload (by checking if they are explicitly set to None
rather than just checking if they are None), or move the cleanup logic to the
update handler where you have access to both the existing database object and
the incoming update data so you can perform the actual field clearing operations
at the database level.
---
Nitpick comments:
In `@tests/unit/test_lab_test_component_schema.py`:
- Around line 397-411: Add a new regression test method similar to
test_response_tolerates_over_limit_ref_range_text that verifies
LabTestComponentResponse can serialize textual_value that exceeds the
MAX_TEXTUAL_VALUE_LENGTH limit without crashing. Create a test that instantiates
LabTestComponentResponse with a textual_value set to a string longer than the
schema limit (using LAB_TEST_COMPONENT_LIMITS for the threshold), and assert
that the response's textual_value field equals the original long string,
ensuring the read-tolerance contract for this field is protected from future
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08346941-5f17-4303-803c-57ae9340b5a3
📒 Files selected for processing (8)
app/core/constants.pyapp/schemas/lab_result.pyapp/schemas/lab_test_component.pyfrontend/src/components/medical/labresults/TestComponentBulkEntry.tsxfrontend/src/components/medical/labresults/TestComponentEditModal.tsxfrontend/src/components/medical/labresults/__tests__/TestPanelCreateDialog.test.tsxfrontend/src/utils/labTestComponentUtils.tstests/unit/test_lab_test_component_schema.py
🚧 Files skipped from review as they are similar to previous changes (6)
- app/schemas/lab_result.py
- app/core/constants.py
- frontend/src/components/medical/labresults/TestComponentBulkEntry.tsx
- frontend/src/utils/labTestComponentUtils.ts
- frontend/src/components/medical/labresults/tests/TestPanelCreateDialog.test.tsx
- frontend/src/components/medical/labresults/TestComponentEditModal.tsx
1 task
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Refactors two base lab result workflows: (1) the add lab result flow now uses test panels + tests for all lab results. Singleton test results are deprecated, although existing singleton tests can still be viewed, edited and deleted. (2) the table view for lab results is replaced by a new
LabResultsComponentTablethat shows all test components in a filterable, grouped flat table with collapsible rows and trend chart access. Addsis_panelandtextual_valuefields to the backend models, atextualresult type, and a shared panel/test library for autocomplete.Related issue
N/A
Type of change
Changes
Frontend
LabResultFormWrapper— conditionally renders a "Test Results" tab (viaTestComponentsTab) for panel-type results instead of the single-result "Results & Status" tab; hides irrelevant fields (test code, test type) for panelsTestPanelCreateDialog(new) — modal for creating a lab result panel with metadata + inline test component rows in one step; usespanelLibraryautocomplete andpanelTemplateMapto pre-populate standard test rowsLabResultsComponentTable(new) — flat table of all lab test components across results, grouped by test name with collapsible rows; supports filtering by search, category, practitioner, facility, and date range; links to trend chartsLabResultspage — addsTestPanelCreateDialog, integratesLabResultsComponentTableas acomponentsview mode, coerces legacytable/cardsview modes topanels, computes roll-up worst status per panelpanelLibrary.ts/panelTemplateMap.ts(new) — shared constants for panel autocomplete options and default test component templatesmedical.jsonfiles with new translation keysBackend
LabResultmodel — addsis_panelboolean column (migration included)LabTestComponentmodel — addstextual_valuecolumn andtextualresult type with cross-field validation (migration included)lab_test_componentendpoint — minor additions to support new fieldsTesting
LabResultsComponentTable.test.tsx(filter, grouping, action callbacks)TestPanelCreateDialog.test.tsx(form submission, panel autocomplete, component rows)tests/api/test_lab_test_component_defaults.py(API-level defaults for new fields)TestComponentCatalog.test.tsxandLabResults.integration.test.jsxfor changed interfacesChecklist
npm run lintandnpm run buildpasspytest) if backend code changedconsole.logleft in the difft()translationsSummary by CodeRabbit
New Features
Localization Updates