-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle null/undefined token values in ContextCondenseRow (#6914) #6915
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-safe operators for prevContextTokens, newContextTokens, and cost - Fallback to "0" for null/undefined token values - 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.
| <span className="font-bold text-vscode-foreground">{t("chat:contextCondense.title")}</span> | ||
| <span className="text-vscode-descriptionForeground text-sm"> | ||
| {prevContextTokens.toLocaleString()} → {newContextTokens.toLocaleString()} {t("tokens")} | ||
| {prevContextTokens?.toLocaleString() ?? "0"} → {newContextTokens?.toLocaleString() ?? "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 there's a type mismatch here. The ContextCondense type from packages/types/src/message.ts defines these fields as required numbers, but we're now treating them as potentially null/undefined. Should we update the type definition to make these fields optional, or should we ensure the data provider always sends valid numbers?
| </span> | ||
| <VSCodeBadge className={cost > 0 ? "opacity-100" : "opacity-0"}>${cost.toFixed(2)}</VSCodeBadge> | ||
| <VSCodeBadge className={cost > 0 ? "opacity-100" : "opacity-0"}> | ||
| ${(cost ?? 0).toFixed(2)} |
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.
Is it intentional that when cost is null/undefined, we hide the badge (opacity-0) but still display "$0.00"? This seems correct for consistency, but just wanted to confirm this is the desired behavior.
| // Should not show any error text when not provided | ||
| expect(queryByText(/Failed/)).not.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! Consider adding a test case for negative cost values to ensure we handle all edge cases properly. What should happen if cost is -1?
|
Duplicate of #6913 |
This PR fixes a UI crash that occurs when the ContextCondenseRow component receives null or undefined values for token counts.
Problem
The UI was crashing with the error:
TypeError: Cannot read properties of null (reading 'toLocaleString')whenprevContextTokensornewContextTokenswere null/undefined.Solution
?.) forprevContextTokens,newContextTokens, andcostTesting
Fixes #6914
Important
Fixes UI crash in
ContextCondenseRowby handling null/undefined token values with null-safe operators and fallback defaults, adding comprehensive test coverage.ContextCondenseRowby handling null/undefinedprevContextTokens,newContextTokens, andcost.?.) and defaults to "0" for null/undefined values.ContextCondenseRow.spec.tsxwith 10 test cases for edge cases.This description was created by
for 294d34a. You can customize this summary. It will automatically update as commits are pushed.