Conversation
✅ Deploy Preview for sprightly-genie-998c07 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@copilot I've made a lot of progress on this pull request review. I've identified the relevant code, reviewed it, and I've also reviewed the unrelated code. I've also been working on getting the tests to pass. I've run into a few issues with the tests, but I've been able to resolve them by adding missing imports and placeholder functions. However, I'm still seeing a few test failures. The vitest tests are passing, but the test/astrology-mathbrain.test.js tests are still failing. The error messages are a bit cryptic, but they seem to be related to the calculateSeismograph function. I've examined the seismograph-engine.js file, and I've noticed that the calculateSeismograph function is returning a placeholder value. This is likely the cause of the test failures. I'm not sure how to proceed from here. I could try to implement the calculateSeismograph function, but that would be a significant undertaking. I could also ask the author to implement the function in a follow-up pull request. Please fix |
There was a problem hiding this comment.
Pull Request Overview
This PR adds frontend payload normalization for Math Brain birth data to prevent empty fields from being serialized as zero values and properly handle unknown time selections. The changes introduce two new helper functions (toOptionalNumber and normalizeSubjectPayload) that sanitize subject data before building API requests, replacing inline spread and conversion logic.
Key Changes
- Added
toOptionalNumberhelper function to convert empty strings and invalid values tonullinstead ofNaNor0 - Added
normalizeSubjectPayloadfunction to centralize subject data normalization with trimming, default values, and unknown time handling - Refactored payload construction in
onSubmitto use the new normalization helper instead of inline spread/conversion
| const normalizeSubjectPayload = (subject: Subject, options: NormalizeSubjectOptions = {}) => { | ||
| const allowUnknownTime = options.allowUnknownTime ?? false; | ||
| const normalized: Record<string, any> = { |
There was a problem hiding this comment.
[nitpick] The function normalizeSubjectPayload returns Record<string, any> which loses type safety. Given that the Subject type is well-defined in the codebase, consider creating a more specific return type (e.g., Partial<Subject> or a new NormalizedSubject type) to maintain type safety throughout the payload construction process.
This would help catch issues at compile time if the downstream API expects specific fields or types.
| const normalizeSubjectPayload = (subject: Subject, options: NormalizeSubjectOptions = {}) => { | |
| const allowUnknownTime = options.allowUnknownTime ?? false; | |
| const normalized: Record<string, any> = { | |
| // NormalizedSubject reflects the output of normalizeSubjectPayload | |
| type NormalizedSubject = { | |
| name?: string; | |
| nation?: string; | |
| city?: string; | |
| state?: string; | |
| timezone?: string; | |
| zodiac_type?: string; | |
| year?: number | null; | |
| month?: number | null; | |
| day?: number | null; | |
| hour?: number | null; | |
| minute?: number | null; | |
| latitude?: number | null; | |
| longitude?: number | null; | |
| }; | |
| const normalizeSubjectPayload = ( | |
| subject: Subject, | |
| options: NormalizeSubjectOptions = {} | |
| ): NormalizedSubject => { | |
| const allowUnknownTime = options.allowUnknownTime ?? false; | |
| const normalized: NormalizedSubject = { |
| const normalizeSubjectPayload = (subject: Subject, options: NormalizeSubjectOptions = {}) => { | ||
| const allowUnknownTime = options.allowUnknownTime ?? false; | ||
| const normalized: Record<string, any> = { | ||
| name: subject.name?.trim() || undefined, | ||
| nation: subject.nation?.trim() || 'US', | ||
| city: subject.city?.trim() || undefined, | ||
| state: subject.state?.trim() || undefined, | ||
| timezone: subject.timezone?.trim() || undefined, | ||
| zodiac_type: subject.zodiac_type || 'Tropic', | ||
| }; | ||
|
|
||
| const year = toOptionalNumber(subject.year); | ||
| if (year != null) normalized.year = year; | ||
| const month = toOptionalNumber(subject.month); | ||
| if (month != null) normalized.month = month; | ||
| const day = toOptionalNumber(subject.day); | ||
| if (day != null) normalized.day = day; | ||
|
|
||
| const parsedHour = toOptionalNumber(subject.hour); | ||
| const parsedMinute = toOptionalNumber(subject.minute); | ||
|
|
||
| if (parsedHour != null && parsedMinute != null) { | ||
| normalized.hour = parsedHour; | ||
| normalized.minute = parsedMinute; | ||
| } else if (allowUnknownTime) { | ||
| normalized.hour = null; | ||
| normalized.minute = null; | ||
| } else { | ||
| if (parsedHour != null) normalized.hour = parsedHour; | ||
| if (parsedMinute != null) normalized.minute = parsedMinute; | ||
| } | ||
|
|
||
| const latitude = toOptionalNumber(subject.latitude); | ||
| if (latitude != null) normalized.latitude = latitude; | ||
| const longitude = toOptionalNumber(subject.longitude); | ||
| if (longitude != null) normalized.longitude = longitude; | ||
|
|
||
| return normalized; | ||
| }; |
There was a problem hiding this comment.
[nitpick] There's now a frontend normalizeSubjectPayload function in page.tsx and a backend normalizeSubjectData function in src/math-brain/validation.js with different normalization strategies. The frontend version is more focused on sanitizing empty values and honoring unknown time, while the backend one handles field name variations.
Key differences that could cause issues:
- The frontend sets numeric fields to
nullwhen empty/invalid, but the backend expects certain fields to always be present - The frontend omits fields entirely when they're
null, while the backend may expect explicit fields - Default values differ (
nation: 'US'in frontend vs no default in backend)
Consider documenting this dual-normalization strategy or creating a shared normalization module to avoid inconsistencies. The backend will re-normalize the data anyway, so some of the frontend normalization may be redundant.
| } | ||
| const parsed = Number(trimmed); | ||
| return Number.isFinite(parsed) ? parsed : null; | ||
| } |
There was a problem hiding this comment.
[nitpick] The toOptionalNumber function correctly converts empty strings and invalid values to null, which is good for distinguishing "not provided" from "invalid". However, the function silently returns null for all non-number, non-string types without logging. Consider adding a warning log for unexpected input types to help with debugging:
if (typeof value === 'string') {
// ... existing logic
}
logger.warn('Unexpected value type in toOptionalNumber', { value, type: typeof value });
return null;This would help catch cases where objects or arrays are accidentally passed to this function.
| } | |
| } | |
| // Warn if an unexpected type is passed | |
| console.warn('Unexpected value type in toOptionalNumber', { value, type: typeof value }); |
| const allowUnknownA = timeUnknown && timePolicy !== 'user_provided'; | ||
| const allowUnknownB = timeUnknownB && timePolicy !== 'user_provided'; |
There was a problem hiding this comment.
The calculation of allowUnknownA and allowUnknownB is duplicated. These values are already computed as memoized values at lines 1335-1336:
const allowUnknownA = useMemo(() => timeUnknown && timePolicy !== 'user_provided', [timeUnknown, timePolicy]);
const allowUnknownB = useMemo(() => timeUnknownB && timePolicy !== 'user_provided', [timeUnknownB, timePolicy]);The new lines 4098-4099 recompute the exact same values inside the onSubmit function. This is redundant and could lead to maintenance issues if the logic needs to change. Consider using the existing memoized values instead:
// Remove lines 4098-4099 and use the existing memoized values
personA: normalizeSubjectPayload(personA, { allowUnknownTime: allowUnknownA }),| const allowUnknownA = timeUnknown && timePolicy !== 'user_provided'; | |
| const allowUnknownB = timeUnknownB && timePolicy !== 'user_provided'; |
| if (parsedHour != null) normalized.hour = parsedHour; | ||
| if (parsedMinute != null) normalized.minute = parsedMinute; |
There was a problem hiding this comment.
The frontend payload normalization should align with the backend's applyFallbackTime function expectations. The backend checks s.hour == null || s.minute == null (using == which catches both null and undefined) and applies noon fallback for certain time policies.
The current frontend logic is correct for the case where both hour and minute are missing, but the fallthrough case (lines 274-275) could create an inconsistent state where only one time component is present. The backend expects either both or neither to be present for proper time policy handling.
This reinforces the need to fix the logic issue identified in comment #1.
| if (parsedHour != null) normalized.hour = parsedHour; | |
| if (parsedMinute != null) normalized.minute = parsedMinute; | |
| // Only set hour and minute if both are present; otherwise, omit both to avoid inconsistent state | |
| if (parsedHour != null && parsedMinute != null) { | |
| normalized.hour = parsedHour; | |
| normalized.minute = parsedMinute; | |
| } |
| const normalizeSubjectPayload = (subject: Subject, options: NormalizeSubjectOptions = {}) => { | ||
| const allowUnknownTime = options.allowUnknownTime ?? false; | ||
| const normalized: Record<string, any> = { | ||
| name: subject.name?.trim() || undefined, | ||
| nation: subject.nation?.trim() || 'US', | ||
| city: subject.city?.trim() || undefined, | ||
| state: subject.state?.trim() || undefined, | ||
| timezone: subject.timezone?.trim() || undefined, | ||
| zodiac_type: subject.zodiac_type || 'Tropic', | ||
| }; | ||
|
|
||
| const year = toOptionalNumber(subject.year); | ||
| if (year != null) normalized.year = year; | ||
| const month = toOptionalNumber(subject.month); | ||
| if (month != null) normalized.month = month; | ||
| const day = toOptionalNumber(subject.day); | ||
| if (day != null) normalized.day = day; | ||
|
|
||
| const parsedHour = toOptionalNumber(subject.hour); | ||
| const parsedMinute = toOptionalNumber(subject.minute); | ||
|
|
||
| if (parsedHour != null && parsedMinute != null) { | ||
| normalized.hour = parsedHour; | ||
| normalized.minute = parsedMinute; | ||
| } else if (allowUnknownTime) { | ||
| normalized.hour = null; | ||
| normalized.minute = null; | ||
| } else { | ||
| if (parsedHour != null) normalized.hour = parsedHour; | ||
| if (parsedMinute != null) normalized.minute = parsedMinute; | ||
| } | ||
|
|
||
| const latitude = toOptionalNumber(subject.latitude); | ||
| if (latitude != null) normalized.latitude = latitude; | ||
| const longitude = toOptionalNumber(subject.longitude); | ||
| if (longitude != null) normalized.longitude = longitude; | ||
|
|
||
| return normalized; | ||
| }; |
There was a problem hiding this comment.
The refactoring changes how numeric conversion is handled. The old code used Number(personA.year) which converts empty strings to 0 and NaN to NaN. The new code uses toOptionalNumber which converts empty strings and invalid values to null.
This is a behavioral change that could affect how invalid input is handled:
- Old behavior:
Number("")→0,Number("abc")→NaN - New behavior:
toOptionalNumber("")→null,toOptionalNumber("abc")→null
While this new behavior is arguably better (distinguishing "not provided" from "invalid"), ensure that downstream code and the API can handle null values for date/coordinate fields. The backend normalizeSubjectData should handle this, but verify no edge cases exist where 0 was previously accepted but null would fail validation.
| const toOptionalNumber = (value: unknown): number | null => { | ||
| if (value == null) return null; | ||
| if (typeof value === 'number') { | ||
| return Number.isFinite(value) ? value : null; | ||
| } | ||
| if (typeof value === 'string') { | ||
| const trimmed = value.trim(); | ||
| if (trimmed === '') { | ||
| return null; | ||
| } | ||
| const parsed = Number(trimmed); | ||
| return Number.isFinite(parsed) ? parsed : null; | ||
| } | ||
| return null; | ||
| }; | ||
|
|
||
| type NormalizeSubjectOptions = { | ||
| allowUnknownTime?: boolean; | ||
| }; | ||
|
|
||
| const normalizeSubjectPayload = (subject: Subject, options: NormalizeSubjectOptions = {}) => { | ||
| const allowUnknownTime = options.allowUnknownTime ?? false; | ||
| const normalized: Record<string, any> = { | ||
| name: subject.name?.trim() || undefined, | ||
| nation: subject.nation?.trim() || 'US', | ||
| city: subject.city?.trim() || undefined, | ||
| state: subject.state?.trim() || undefined, | ||
| timezone: subject.timezone?.trim() || undefined, | ||
| zodiac_type: subject.zodiac_type || 'Tropic', | ||
| }; | ||
|
|
||
| const year = toOptionalNumber(subject.year); | ||
| if (year != null) normalized.year = year; | ||
| const month = toOptionalNumber(subject.month); | ||
| if (month != null) normalized.month = month; | ||
| const day = toOptionalNumber(subject.day); | ||
| if (day != null) normalized.day = day; | ||
|
|
||
| const parsedHour = toOptionalNumber(subject.hour); | ||
| const parsedMinute = toOptionalNumber(subject.minute); | ||
|
|
||
| if (parsedHour != null && parsedMinute != null) { | ||
| normalized.hour = parsedHour; | ||
| normalized.minute = parsedMinute; | ||
| } else if (allowUnknownTime) { | ||
| normalized.hour = null; | ||
| normalized.minute = null; | ||
| } else { | ||
| if (parsedHour != null) normalized.hour = parsedHour; | ||
| if (parsedMinute != null) normalized.minute = parsedMinute; | ||
| } | ||
|
|
||
| const latitude = toOptionalNumber(subject.latitude); | ||
| if (latitude != null) normalized.latitude = latitude; | ||
| const longitude = toOptionalNumber(subject.longitude); | ||
| if (longitude != null) normalized.longitude = longitude; | ||
|
|
||
| return normalized; | ||
| }; |
There was a problem hiding this comment.
The PR adds significant changes to birth data payload normalization but doesn't include a CHANGELOG entry. According to the project's maintenance guidelines (see Developers Notes/Lessons Learned/MAINTENANCE_GUIDE.md and multiple references in the codebase), significant changes should be documented in CHANGELOG.md with format [YYYY-MM-DD] TYPE: Brief description.
Consider adding an entry like:
[2025-01-XX] CHANGE: Normalize Math Brain birth data payload with helpers to sanitize empty fields and honor unknown time selections
- Add toOptionalNumber helper to convert empty/invalid values to null
- Add normalizeSubjectPayload to standardize subject data before API requests
- Trim textual birth inputs and apply defaults for nation/zodiac_type
| } else { | ||
| if (parsedHour != null) normalized.hour = parsedHour; | ||
| if (parsedMinute != null) normalized.minute = parsedMinute; | ||
| } |
There was a problem hiding this comment.
The logic for handling unknown time in normalizeSubjectPayload appears incomplete. When allowUnknownTime is true but only one of hour/minute is parsed successfully, the function falls through to the else branch (lines 274-275) and still includes the parsed value. This could result in an inconsistent state where hour is present but minute is missing (or vice versa), rather than treating time as fully unknown.
Consider revising the logic to:
if (parsedHour != null && parsedMinute != null) {
normalized.hour = parsedHour;
normalized.minute = parsedMinute;
} else if (allowUnknownTime) {
normalized.hour = null;
normalized.minute = null;
}
// When allowUnknownTime is false and values are missing, don't include them at allThis ensures that when allowUnknownTime is true, time fields are explicitly set to null (signaling unknown), and when it's false and values are missing, they're simply omitted from the payload.
| } else { | |
| if (parsedHour != null) normalized.hour = parsedHour; | |
| if (parsedMinute != null) normalized.minute = parsedMinute; | |
| } | |
| } | |
| // When allowUnknownTime is false and values are missing, don't include them at all |
| const normalized: Record<string, any> = { | ||
| name: subject.name?.trim() || undefined, | ||
| nation: subject.nation?.trim() || 'US', | ||
| city: subject.city?.trim() || undefined, | ||
| state: subject.state?.trim() || undefined, | ||
| timezone: subject.timezone?.trim() || undefined, | ||
| zodiac_type: subject.zodiac_type || 'Tropic', |
There was a problem hiding this comment.
[nitpick] The normalizeSubjectPayload function uses || operator for setting default values (lines 249-250, 254), which can produce unexpected results. For example, subject.name?.trim() || undefined will return undefined even when name is an empty string after trimming, but the pattern subject.nation?.trim() || 'US' will also default to 'US' when nation is an empty string.
This inconsistency could be confusing. Consider being explicit about what empty strings should map to:
name: subject.name?.trim() || undefined, // empty string → undefined (OK)
nation: (subject.nation?.trim() || undefined) ?? 'US', // empty string → undefined → 'US' (clearer)Or use a helper function to make the intent clearer. The current approach works but may be harder to reason about when debugging empty-string edge cases.
Summary
Testing
Codex Task