-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle null/undefined token values in ContextCondenseRow to prevent UI crash #6916
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.
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
Conversation
…ent UI crash - Added null/undefined checks for prevContextTokens, newContextTokens, and cost - Default to 0 when values are null or undefined - Added comprehensive test coverage for edge cases - Fixes #6914
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| const [isExpanded, setIsExpanded] = useState(false) | ||
|
|
||
| // Handle null/undefined token values to prevent crashes | ||
| const prevTokens = prevContextTokens ?? 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice we're treating these values as optional here, but the Zod schema in packages/types/src/message.ts defines them as required fields. This creates a type safety issue.
Is this intentional? The schema has:
export const contextCondenseSchema = z.object({
cost: z.number(),
prevContextTokens: z.number(),
newContextTokens: z.number(),
summary: z.string(),
})If these values can indeed be null/undefined in practice, we should update the schema to use .number().optional() to maintain type consistency across the codebase.
| // Handle null/undefined token values to prevent crashes | ||
| const prevTokens = prevContextTokens ?? 0 | ||
| const newTokens = newContextTokens ?? 0 | ||
| const displayCost = cost ?? 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While defaulting to 0 prevents the crash, could this be misleading to users? When data is unavailable, showing "0 tokens" might imply no tokens were used, when actually the data is missing.
Would it be clearer to display "--" or "N/A" when values are null/undefined?
| const displayCost = cost ?? 0 | |
| const prevTokens = prevContextTokens ?? 0 | |
| const newTokens = newContextTokens ?? 0 | |
| const displayCost = cost ?? 0 | |
| const hasValidData = prevContextTokens != null && newContextTokens != null |
| // Should display $0.00 for undefined cost | ||
| expect(screen.getByText("$0.00")).toBeInTheDocument() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test coverage for null/undefined cases! One edge case to consider: what happens with NaN values? They would also fail with .toLocaleString(). Might be worth adding a test case:
| }) | |
| it("should handle NaN values without crashing", () => { | |
| const props = { | |
| cost: NaN, | |
| prevContextTokens: NaN, | |
| newContextTokens: NaN, | |
| summary: "NaN test", | |
| } | |
| const { container } = render(<ContextCondenseRow {...props} />) | |
| expect(container).toBeInTheDocument() | |
| // Should handle NaN gracefully | |
| }) |
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
This PR fixes a UI crash that occurs when the ContextCondenseRow component receives null or undefined values for token counts. The error "Cannot read properties of null (reading 'toLocaleString')" was causing the entire UI to crash, as reported in issue #6914.
Problem
The
ContextCondenseRowcomponent was attempting to call.toLocaleString()onprevContextTokensandnewContextTokenswithout checking if these values were null or undefined first. This caused a runtime error that crashed the UI.Solution
prevContextTokens,newContextTokens, andcostvaluesTesting
ContextCondenseRowcomponentRelated Issues
Fixes #6914
Checklist
Important
Fixes UI crash in
ContextCondenseRowby handling null/undefined token values and adds comprehensive tests for edge cases.ContextCondenseRowby handling null/undefinedprevContextTokens,newContextTokens, andcost..toLocaleString()errors.ContextCondenseRow.spec.tsxfor null/undefined handling, valid data, and edge cases like negative and large numbers.This description was created by
for 22a873d. You can customize this summary. It will automatically update as commits are pushed.