Skip to content

Normalize Math Brain birth data payload #293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
DHCross wants to merge 2 commits into
base: main
Choose a base branch
from
+66 −23
Draft

Normalize Math Brain birth data payload #293

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29d6a33
Normalize birth data payload before Math Brain requests
DHCross Nov 10, 2025
05cd7c8
Update page.tsx
DHCross Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 66 additions & 23 deletions app/math-brain/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,66 @@ const toFiniteNumber = (value: unknown): number => {
return Number.NaN;
};

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;
}
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
}
}
// Warn if an unexpected type is passed
console.warn('Unexpected value type in toOptionalNumber', { value, type: typeof value });

Copilot uses AI. Check for mistakes.
return null;
};

type NormalizeSubjectOptions = {
allowUnknownTime?: boolean;
};

const normalizeSubjectPayload = (subject: Subject, options: NormalizeSubjectOptions = {}) => {
const allowUnknownTime = options.allowUnknownTime ?? false;
const normalized: Record<string, any> = {
Comment on lines +246 to +248
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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 = {

Copilot uses AI. Check for mistakes.
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',
Comment on lines +248 to +254
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copilot uses AI. Check for mistakes.
};

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;
Comment on lines +274 to +275
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
}

Copilot uses AI. Check for mistakes.
}
Comment on lines +273 to +276
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 all

This 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.

Suggested change
} 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

Copilot uses AI. Check for mistakes.

const latitude = toOptionalNumber(subject.latitude);
if (latitude != null) normalized.latitude = latitude;
const longitude = toOptionalNumber(subject.longitude);
if (longitude != null) normalized.longitude = longitude;

return normalized;
};
Comment on lines +246 to +284
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

  1. The frontend sets numeric fields to null when empty/invalid, but the backend expects certain fields to always be present
  2. The frontend omits fields entirely when they're null, while the backend may expect explicit fields
  3. 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.

Copilot uses AI. Check for mistakes.
Comment on lines +246 to +284
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +226 to +284
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.

const POETIC_BRAIN_ENABLED = (() => {
const raw = process.env.NEXT_PUBLIC_ENABLE_POETIC_BRAIN;
if (typeof raw !== 'string') return true;
Expand Down Expand Up @@ -4035,21 +4095,14 @@ export default function MathBrainPage() {
if (chunks && chunks.length > 1) {
setToast(`Processing ${daysDiff} days in ${chunks.length} chunks...`);
}
const allowUnknownA = timeUnknown && timePolicy !== 'user_provided';
const allowUnknownB = timeUnknownB && timePolicy !== 'user_provided';
Comment on lines +4098 to +4099
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }),
Suggested change
const allowUnknownA = timeUnknown && timePolicy !== 'user_provided';
const allowUnknownB = timeUnknownB && timePolicy !== 'user_provided';

Copilot uses AI. Check for mistakes.

// Build unified request payload
const payload: Record<string, any> = {
mode,
personA: {
...personA,
nation: personA.nation || "US",
year: Number(personA.year),
month: Number(personA.month),
day: Number(personA.day),
hour: Number(personA.hour),
minute: Number(personA.minute),
latitude: Number(personA.latitude),
longitude: Number(personA.longitude),
},
time_policy: timeUnknown ? timePolicy : 'user_provided',
personA: normalizeSubjectPayload(personA, { allowUnknownTime: allowUnknownA }),
time_policy: allowUnknownA ? timePolicy : 'user_provided',
report_type: reportContractType,
presentation_style: 'conversational',
context: {
Expand Down Expand Up @@ -4091,17 +4144,7 @@ export default function MathBrainPage() {

// Add Person B and relationship context for relational modes
if (RELATIONAL_MODES.includes(mode) && includePersonB) {
payload.personB = {
...personB,
nation: personB.nation || "US",
year: Number(personB.year),
month: Number(personB.month),
day: Number(personB.day),
hour: Number(personB.hour),
minute: Number(personB.minute),
latitude: Number(personB.latitude),
longitude: Number(personB.longitude),
};
payload.personB = normalizeSubjectPayload(personB, { allowUnknownTime: allowUnknownB });
payload.relationship_context = {
type: relationshipType,
intimacy_tier: relationshipType === 'PARTNER' ? relationshipTier : undefined,
Expand Down
Loading