Skip to content

Commit 50af5ef

Browse files
maggie-li-ydclaude
andauthored
Add defensive guards and refactor in PA (#2923)
* fix(pa): share constraint-value normalizer and add defensive guards Prerequisite refactor split out of maggie-li-yd/2872-webmcp-poc (PR A1). No functional change to existing UI flows. - Extract value normalization out of useDashboardFlow into utils/applyConstraintValue.ts so the wizard flow and the (upcoming) WebMCP patch applier share one normalizer. Reject objects that are neither a {from,to} date range nor a numeric value instead of String()-ing them into a broken '[object Object]' filter. - StackBarChart: null-guard filterCard?.name so a category referencing a no-longer-chartable filter card can't crash the whole chart render. - BaseTagInput: coerce displayValue to String() so a numeric constraint value doesn't throw in .replace(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * address code review * add guard for date range constraints * add unit test for applyConstraintValue --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c7c8f6e commit 50af5ef

5 files changed

Lines changed: 335 additions & 88 deletions

File tree

plugins/ui/apps/vue-mri-ui-lib/src/components/StackBarChart.vue

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -645,8 +645,12 @@ export default {
645645
}
646646
} else {
647647
const filterCard = this.getChartableFilterCardByInstanceId(filterCardPath.join('.'))
648-
if (!category.name || !category.name.startsWith(filterCard.name + ' - ')) {
649-
category.name = `${filterCard.name} - ${category.name}`
648+
// Guard: a category can reference a filter card that is no longer chartable
649+
// (e.g. after an inconsistent bookmark load). Skip the prefix rather than
650+
// dereferencing undefined and crashing the whole chart render.
651+
const filterCardName = filterCard?.name
652+
if (filterCardName && (!category.name || !category.name.startsWith(filterCardName + ' - '))) {
653+
category.name = category.name ? `${filterCardName} - ${category.name}` : filterCardName
650654
}
651655
}
652656
}

plugins/ui/apps/vue-mri-ui-lib/src/composables/useDashboardFlow.ts

Lines changed: 2 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,9 @@ import {
33
getFieldAttrKey,
44
getFieldFilterCardPathForField,
55
getWizardFlow,
6-
parseNumericInput,
76
validateRequiredFields,
87
isConditionField,
98
type MissingRequiredField,
10-
type NumericFilterValue,
119
type WizardDefinition,
1210
type WizardFieldDefinition,
1311
type BookmarkBooleanContainer,
@@ -16,6 +14,7 @@ import {
1614
import { constraintContainsExpression, type Constraint } from '../services/dashboardFlowService'
1715
import BinaryToString from '../utils/BinaryToString'
1816
import { useNotificationStore } from '../stores/notifications'
17+
import { applyConstraintValue as applyConstraintValueShared } from '../utils/applyConstraintValue'
1918

2019
export interface WizardFieldValue {
2120
value: string | number | boolean | object
@@ -935,89 +934,7 @@ export function useDashboardFlow(
935934
operator = '=',
936935
displayValue?: string
937936
): Promise<any> {
938-
const constraintType = constraint.props.type
939-
if (constraintType === 'num') {
940-
let parsedValues: NumericFilterValue[] = []
941-
if (typeof rawInput === 'string') {
942-
parsedValues = parseNumericInput(rawInput)
943-
if (
944-
operator &&
945-
operator !== '=' &&
946-
/^-?\d+(?:\.\d+)?$/.test(rawInput.trim()) &&
947-
parsedValues.length === 1 &&
948-
parsedValues[0].op === '='
949-
) {
950-
parsedValues[0].op = operator
951-
}
952-
} else if (typeof rawInput === 'number') {
953-
parsedValues = [{ op: operator || '=', value: rawInput }]
954-
} else if (rawInput !== null && typeof rawInput !== 'undefined') {
955-
const numericValue = Number(rawInput)
956-
if (!Number.isNaN(numericValue)) {
957-
parsedValues = [{ op: operator || '=', value: numericValue }]
958-
}
959-
}
960-
if (!parsedValues.length) {
961-
console.error('[DashboardFlow] Invalid numeric value:', { rawInput, constraint })
962-
return Promise.reject(new Error(`Invalid numeric value for ${constraint.props.name || constraint.id}`))
963-
}
964-
return dispatch('updateConstraintValue', {
965-
constraintId: constraint.id,
966-
value: parsedValues,
967-
})
968-
}
969-
if (rawInput && typeof rawInput === 'object' && 'from' in rawInput && 'to' in rawInput) {
970-
const fromYear = rawInput.from
971-
const toYear = rawInput.to
972-
if (!fromYear && !toYear) {
973-
return Promise.reject(new Error(`Missing year value for ${constraint.props.name || constraint.id}`))
974-
}
975-
const fromDate = fromYear ? new Date(`${fromYear}-01-01`) : new Date(`${toYear}-01-01`)
976-
const toDate = toYear ? new Date(`${toYear}-12-31`) : new Date(`${fromYear}-12-31`)
977-
return dispatch('updateDateConstraintValue', {
978-
constraintId: constraint.id,
979-
fromDateValue: fromDate,
980-
toDateValue: toDate,
981-
isUTC: false,
982-
})
983-
}
984-
if (constraintType === 'time' || constraintType === 'datetime') {
985-
const fromDateRaw = rawInput?.from || rawInput?.value || rawInput
986-
const toDateRaw = rawInput?.to || rawInput?.value || rawInput
987-
if (!fromDateRaw && !toDateRaw) {
988-
return Promise.reject(new Error(`Missing date value for ${constraint.props.name || constraint.id}`))
989-
}
990-
const fromDate = new Date(fromDateRaw || toDateRaw)
991-
const toDate = new Date(toDateRaw || fromDateRaw)
992-
return dispatch('updateDateConstraintValue', {
993-
constraintId: constraint.id,
994-
fromDateValue: fromDate,
995-
toDateValue: toDate,
996-
isUTC: false,
997-
})
998-
}
999-
const rawValue = rawInput?.value ?? rawInput
1000-
if (rawValue === null || typeof rawValue === 'undefined' || String(rawValue).trim() === '') {
1001-
if (constraintType === 'text' || constraintType === 'conceptSet') {
1002-
return dispatch('updateConstraintValue', {
1003-
constraintId: constraint.id,
1004-
value: [],
1005-
})
1006-
}
1007-
return Promise.reject(new Error(`Missing value for ${constraint.props.name || constraint.id}`))
1008-
}
1009-
const finalDisplayValue = displayValue || rawInput?.displayName || String(rawValue)
1010-
return dispatch('updateConstraintValue', {
1011-
constraintId: constraint.id,
1012-
value: [
1013-
{
1014-
value: String(rawValue),
1015-
score: 1,
1016-
display_value: finalDisplayValue,
1017-
text: finalDisplayValue,
1018-
},
1019-
],
1020-
})
937+
return applyConstraintValueShared(dispatch, constraint, rawInput, operator, displayValue)
1021938
}
1022939

1023940
async function handleRequiredFiltersSubmit(

plugins/ui/apps/vue-mri-ui-lib/src/lib/ui/BaseTagInput.vue

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,9 @@ export default {
340340
},
341341
formatValues(values) {
342342
return values.map(elem => {
343-
const displayValue = elem.text || elem.value
343+
// Coerce to string: a constraint value may arrive numeric (e.g. a mis-shaped
344+
// value from a programmatic edit), and .replace would throw on a number.
345+
const displayValue = String(elem.text ?? elem.value ?? '')
344346
elem.display_value = displayValue.replace('</b>', '').replace('<b>', '')
345347
return elem
346348
})
Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
1+
import { describe, it, expect, vi, beforeEach } from 'vitest'
2+
import { applyConstraintValue } from '../applyConstraintValue'
3+
4+
describe('applyConstraintValue', () => {
5+
let dispatch: ReturnType<typeof vi.fn>
6+
7+
beforeEach(() => {
8+
dispatch = vi.fn().mockResolvedValue(undefined)
9+
})
10+
11+
const constraint = (overrides: Record<string, any> = {}, props: Record<string, any> = {}) => ({
12+
id: 'constraint-1',
13+
props: { type: 'text', name: 'Age', ...props },
14+
...overrides,
15+
})
16+
17+
describe('numeric constraints', () => {
18+
it('parses an operator-prefixed string', async () => {
19+
await applyConstraintValue(dispatch, constraint({}, { type: 'num' }), '>=65')
20+
expect(dispatch).toHaveBeenCalledWith('updateConstraintValue', {
21+
constraintId: 'constraint-1',
22+
value: [{ op: '>=', value: 65 }],
23+
})
24+
})
25+
26+
it('overrides the default = op for a plain numeric string', async () => {
27+
await applyConstraintValue(dispatch, constraint({}, { type: 'num' }), '50', '>')
28+
expect(dispatch).toHaveBeenCalledWith('updateConstraintValue', {
29+
constraintId: 'constraint-1',
30+
value: [{ op: '>', value: 50 }],
31+
})
32+
})
33+
34+
it('does not override the op when the string already carries operators', async () => {
35+
await applyConstraintValue(dispatch, constraint({}, { type: 'num' }), '>50,<=70', '>=')
36+
expect(dispatch).toHaveBeenCalledWith('updateConstraintValue', {
37+
constraintId: 'constraint-1',
38+
value: [
39+
{ op: '>', value: 50 },
40+
{ op: '<=', value: 70 },
41+
],
42+
})
43+
})
44+
45+
it('accepts a numeric input with an explicit operator', async () => {
46+
await applyConstraintValue(dispatch, constraint({}, { type: 'num' }), 42, '<')
47+
expect(dispatch).toHaveBeenCalledWith('updateConstraintValue', {
48+
constraintId: 'constraint-1',
49+
value: [{ op: '<', value: 42 }],
50+
})
51+
})
52+
53+
it('coerces a non-string, non-number numeric value', async () => {
54+
await applyConstraintValue(dispatch, constraint({}, { type: 'num' }), { valueOf: () => 7 } as any)
55+
expect(dispatch).toHaveBeenCalledWith('updateConstraintValue', {
56+
constraintId: 'constraint-1',
57+
value: [{ op: '=', value: 7 }],
58+
})
59+
})
60+
61+
it('rejects an unparseable numeric value', async () => {
62+
await expect(
63+
applyConstraintValue(dispatch, constraint({}, { type: 'num' }), 'abc')
64+
).rejects.toThrow('Invalid numeric value for Age')
65+
expect(dispatch).not.toHaveBeenCalled()
66+
})
67+
})
68+
69+
describe('date/year range ({ from, to }) values', () => {
70+
it('converts a full year range into a date-constraint dispatch', async () => {
71+
await applyConstraintValue(dispatch, constraint({}, { type: 'text' }), { from: '2000', to: '2010' })
72+
const [action, payload] = dispatch.mock.calls[0]
73+
expect(action).toBe('updateDateConstraintValue')
74+
expect(payload.constraintId).toBe('constraint-1')
75+
expect((payload.fromDateValue as Date).toISOString()).toContain('2000-01-01')
76+
expect((payload.toDateValue as Date).toISOString()).toContain('2010-12-31')
77+
expect(payload.isUTC).toBe(false)
78+
})
79+
80+
it('falls back to the "to" year when "from" is missing', async () => {
81+
await applyConstraintValue(dispatch, constraint(), { from: '', to: '2015' })
82+
const payload = dispatch.mock.calls[0][1]
83+
expect((payload.fromDateValue as Date).toISOString()).toContain('2015-01-01')
84+
expect((payload.toDateValue as Date).toISOString()).toContain('2015-12-31')
85+
})
86+
87+
it('falls back to the "from" year when "to" is missing', async () => {
88+
await applyConstraintValue(dispatch, constraint(), { from: '2015', to: '' })
89+
const payload = dispatch.mock.calls[0][1]
90+
expect((payload.fromDateValue as Date).toISOString()).toContain('2015-01-01')
91+
expect((payload.toDateValue as Date).toISOString()).toContain('2015-12-31')
92+
})
93+
94+
it('rejects when neither year is present', async () => {
95+
await expect(applyConstraintValue(dispatch, constraint(), { from: '', to: '' })).rejects.toThrow(
96+
'Missing year value for Age'
97+
)
98+
expect(dispatch).not.toHaveBeenCalled()
99+
})
100+
101+
// Regression: a full-date { from, to } range (the shape the WebMCP cohort-patch
102+
// applier sends for `date`-type constraints) must be parsed as-is, NOT collapsed
103+
// into a Jan-1..Dec-31 year span. Before the fix this hit the year-range logic and
104+
// produced `new Date('2020-06-01-01-01')` → Invalid Date.
105+
it('parses a full-date range as-is instead of treating it as a year span', async () => {
106+
await applyConstraintValue(dispatch, constraint({}, { type: 'datetime' }), {
107+
from: '2020-06-01',
108+
to: '2020-06-30',
109+
})
110+
const [action, payload] = dispatch.mock.calls[0]
111+
expect(action).toBe('updateDateConstraintValue')
112+
expect((payload.fromDateValue as Date).toISOString()).toContain('2020-06-01')
113+
expect((payload.toDateValue as Date).toISOString()).toContain('2020-06-30')
114+
})
115+
116+
it('handles a full-date range regardless of constraint type', async () => {
117+
await applyConstraintValue(dispatch, constraint({}, { type: 'time' }), {
118+
from: '2019-01-15',
119+
to: '2019-12-20',
120+
})
121+
const payload = dispatch.mock.calls[0][1]
122+
expect((payload.fromDateValue as Date).toISOString()).toContain('2019-01-15')
123+
expect((payload.toDateValue as Date).toISOString()).toContain('2019-12-20')
124+
})
125+
126+
it('falls back to the populated bound for a single-sided full-date range', async () => {
127+
await applyConstraintValue(dispatch, constraint({}, { type: 'datetime' }), {
128+
from: '2021-03-10',
129+
to: '',
130+
})
131+
const payload = dispatch.mock.calls[0][1]
132+
expect((payload.fromDateValue as Date).toISOString()).toContain('2021-03-10')
133+
expect((payload.toDateValue as Date).toISOString()).toContain('2021-03-10')
134+
})
135+
})
136+
137+
describe('time / datetime constraints', () => {
138+
// NOTE: a { from, to } object never reaches this branch — the range branch above
139+
// intercepts any object carrying both keys (handling both bare-year and full-date
140+
// ranges). This branch only handles scalar / { value } dates.
141+
it('handles a { value } payload', async () => {
142+
await applyConstraintValue(dispatch, constraint({}, { type: 'datetime' }), { value: '2021-03-15' })
143+
const payload = dispatch.mock.calls[0][1]
144+
expect((payload.fromDateValue as Date).toISOString()).toContain('2021-03-15')
145+
expect((payload.toDateValue as Date).toISOString()).toContain('2021-03-15')
146+
})
147+
148+
it('handles a bare scalar date string', async () => {
149+
await applyConstraintValue(dispatch, constraint({}, { type: 'datetime' }), '2022-11-20')
150+
const payload = dispatch.mock.calls[0][1]
151+
expect((payload.fromDateValue as Date).toISOString()).toContain('2022-11-20')
152+
})
153+
154+
it('rejects when no date value is present', async () => {
155+
await expect(
156+
applyConstraintValue(dispatch, constraint({}, { type: 'time' }), '')
157+
).rejects.toThrow('Missing date value for Age')
158+
expect(dispatch).not.toHaveBeenCalled()
159+
})
160+
})
161+
162+
describe('scalar (default) constraints', () => {
163+
it('dispatches a scalar string value with derived display value', async () => {
164+
await applyConstraintValue(dispatch, constraint(), 'male')
165+
expect(dispatch).toHaveBeenCalledWith('updateConstraintValue', {
166+
constraintId: 'constraint-1',
167+
value: [{ value: 'male', score: 1, display_value: 'male', text: 'male' }],
168+
})
169+
})
170+
171+
it('prefers an explicit displayValue argument', async () => {
172+
await applyConstraintValue(dispatch, constraint(), 'M', '=', 'Male')
173+
expect(dispatch).toHaveBeenCalledWith('updateConstraintValue', {
174+
constraintId: 'constraint-1',
175+
value: [{ value: 'M', score: 1, display_value: 'Male', text: 'Male' }],
176+
})
177+
})
178+
179+
it('unwraps a { value, displayName } object', async () => {
180+
await applyConstraintValue(dispatch, constraint(), { value: 'M', displayName: 'Male' })
181+
expect(dispatch).toHaveBeenCalledWith('updateConstraintValue', {
182+
constraintId: 'constraint-1',
183+
value: [{ value: 'M', score: 1, display_value: 'Male', text: 'Male' }],
184+
})
185+
})
186+
187+
it.each([[null], [undefined], [''], [' ']])('rejects a missing value (%p)', async input => {
188+
await expect(applyConstraintValue(dispatch, constraint(), input)).rejects.toThrow('Missing value for Age')
189+
expect(dispatch).not.toHaveBeenCalled()
190+
})
191+
192+
it('rejects an unsupported object value instead of stringifying it', async () => {
193+
await expect(applyConstraintValue(dispatch, constraint(), { foo: 'bar' })).rejects.toThrow(
194+
'Unsupported object value for Age'
195+
)
196+
expect(dispatch).not.toHaveBeenCalled()
197+
})
198+
})
199+
200+
it('falls back to the constraint id in error messages when name is absent', async () => {
201+
await expect(
202+
applyConstraintValue(dispatch, constraint({}, { type: 'num', name: undefined }), 'abc')
203+
).rejects.toThrow('Invalid numeric value for constraint-1')
204+
})
205+
})

0 commit comments

Comments
 (0)