-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,11 @@ export const ContextCondenseRow = ({ cost, prevContextTokens, newContextTokens, | |||||||||||
| const { t } = useTranslation() | ||||||||||||
| const [isExpanded, setIsExpanded] = useState(false) | ||||||||||||
|
|
||||||||||||
| // Handle null/undefined token values to prevent crashes | ||||||||||||
| const prevTokens = prevContextTokens ?? 0 | ||||||||||||
| const newTokens = newContextTokens ?? 0 | ||||||||||||
| const displayCost = cost ?? 0 | ||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Suggested change
|
||||||||||||
|
|
||||||||||||
| return ( | ||||||||||||
| <div className="mb-2"> | ||||||||||||
| <div | ||||||||||||
|
|
@@ -33,9 +38,11 @@ export const ContextCondenseRow = ({ cost, prevContextTokens, newContextTokens, | |||||||||||
| <span className="codicon codicon-compress text-blue-400" /> | ||||||||||||
| <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")} | ||||||||||||
| {prevTokens.toLocaleString()} → {newTokens.toLocaleString()} {t("tokens")} | ||||||||||||
| </span> | ||||||||||||
| <VSCodeBadge className={cost > 0 ? "opacity-100" : "opacity-0"}>${cost.toFixed(2)}</VSCodeBadge> | ||||||||||||
| <VSCodeBadge className={displayCost > 0 ? "opacity-100" : "opacity-0"}> | ||||||||||||
| ${displayCost.toFixed(2)} | ||||||||||||
| </VSCodeBadge> | ||||||||||||
| </div> | ||||||||||||
| <span className={`codicon codicon-chevron-${isExpanded ? "up" : "down"}`}></span> | ||||||||||||
| </div> | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,282 @@ | ||||||||||||||||||||||||||||
| import { render, fireEvent, screen } from "@/utils/test-utils" | ||||||||||||||||||||||||||||
| import { ContextCondenseRow, CondensingContextRow, CondenseContextErrorRow } from "../ContextCondenseRow" | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Mock the translation hook | ||||||||||||||||||||||||||||
| vi.mock("react-i18next", () => ({ | ||||||||||||||||||||||||||||
| useTranslation: () => ({ | ||||||||||||||||||||||||||||
| t: (key: string) => { | ||||||||||||||||||||||||||||
| const translations: Record<string, string> = { | ||||||||||||||||||||||||||||
| "chat:contextCondense.title": "Context Condensed", | ||||||||||||||||||||||||||||
| "chat:contextCondense.condensing": "Condensing context...", | ||||||||||||||||||||||||||||
| "chat:contextCondense.errorHeader": "Context condensation failed", | ||||||||||||||||||||||||||||
| tokens: "tokens", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return translations[key] || key | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||
| initReactI18next: { | ||||||||||||||||||||||||||||
| type: "3rdParty", | ||||||||||||||||||||||||||||
| init: () => {}, | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| describe("ContextCondenseRow", () => { | ||||||||||||||||||||||||||||
| describe("with valid data", () => { | ||||||||||||||||||||||||||||
| const defaultProps = { | ||||||||||||||||||||||||||||
| cost: 0.05, | ||||||||||||||||||||||||||||
| prevContextTokens: 1000, | ||||||||||||||||||||||||||||
| newContextTokens: 500, | ||||||||||||||||||||||||||||
| summary: "Context has been condensed successfully", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should render without crashing", () => { | ||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...defaultProps} />) | ||||||||||||||||||||||||||||
| expect(container).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should display token counts correctly", () => { | ||||||||||||||||||||||||||||
| render(<ContextCondenseRow {...defaultProps} />) | ||||||||||||||||||||||||||||
| // The component should display "1,000 → 500 tokens" | ||||||||||||||||||||||||||||
| expect(screen.getByText(/1,000/)).toBeInTheDocument() | ||||||||||||||||||||||||||||
| expect(screen.getByText(/500/)).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should display cost when greater than 0", () => { | ||||||||||||||||||||||||||||
| render(<ContextCondenseRow {...defaultProps} />) | ||||||||||||||||||||||||||||
| expect(screen.getByText("$0.05")).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should hide cost badge when cost is 0", () => { | ||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...defaultProps} cost={0} />) | ||||||||||||||||||||||||||||
| const badge = container.querySelector("vscode-badge") | ||||||||||||||||||||||||||||
| expect(badge).toHaveClass("opacity-0") | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should expand and show summary when clicked", () => { | ||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...defaultProps} />) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Summary should not be visible initially | ||||||||||||||||||||||||||||
| expect(screen.queryByText(defaultProps.summary)).not.toBeInTheDocument() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Click to expand - find the clickable div | ||||||||||||||||||||||||||||
| const expandButton = container.querySelector(".cursor-pointer") | ||||||||||||||||||||||||||||
| fireEvent.click(expandButton!) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Summary should now be visible | ||||||||||||||||||||||||||||
| expect(screen.getByText(defaultProps.summary)).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should toggle chevron icon when expanded/collapsed", () => { | ||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...defaultProps} />) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Initially should show chevron-down | ||||||||||||||||||||||||||||
| expect(container.querySelector(".codicon-chevron-down")).toBeInTheDocument() | ||||||||||||||||||||||||||||
| expect(container.querySelector(".codicon-chevron-up")).not.toBeInTheDocument() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Click to expand | ||||||||||||||||||||||||||||
| const expandButton = container.querySelector(".cursor-pointer") | ||||||||||||||||||||||||||||
| fireEvent.click(expandButton!) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Should now show chevron-up | ||||||||||||||||||||||||||||
| expect(container.querySelector(".codicon-chevron-up")).toBeInTheDocument() | ||||||||||||||||||||||||||||
| expect(container.querySelector(".codicon-chevron-down")).not.toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| describe("with null/undefined values", () => { | ||||||||||||||||||||||||||||
| it("should handle null prevContextTokens without crashing", () => { | ||||||||||||||||||||||||||||
| const props = { | ||||||||||||||||||||||||||||
| cost: 0.05, | ||||||||||||||||||||||||||||
| prevContextTokens: null as any, | ||||||||||||||||||||||||||||
| newContextTokens: 500, | ||||||||||||||||||||||||||||
| summary: "Context condensed", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...props} />) | ||||||||||||||||||||||||||||
| expect(container).toBeInTheDocument() | ||||||||||||||||||||||||||||
| // Should display 0 instead of null | ||||||||||||||||||||||||||||
| expect(screen.getByText(/0 →/)).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should handle null newContextTokens without crashing", () => { | ||||||||||||||||||||||||||||
| const props = { | ||||||||||||||||||||||||||||
| cost: 0.05, | ||||||||||||||||||||||||||||
| prevContextTokens: 1000, | ||||||||||||||||||||||||||||
| newContextTokens: null as any, | ||||||||||||||||||||||||||||
| summary: "Context condensed", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...props} />) | ||||||||||||||||||||||||||||
| expect(container).toBeInTheDocument() | ||||||||||||||||||||||||||||
| // Should display 0 instead of null | ||||||||||||||||||||||||||||
| expect(screen.getByText(/→ 0/)).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should handle both tokens being null without crashing", () => { | ||||||||||||||||||||||||||||
| const props = { | ||||||||||||||||||||||||||||
| cost: 0.05, | ||||||||||||||||||||||||||||
| prevContextTokens: null as any, | ||||||||||||||||||||||||||||
| newContextTokens: null as any, | ||||||||||||||||||||||||||||
| summary: "Context condensed", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...props} />) | ||||||||||||||||||||||||||||
| expect(container).toBeInTheDocument() | ||||||||||||||||||||||||||||
| // Should display "0 → 0 tokens" | ||||||||||||||||||||||||||||
| expect(screen.getByText(/0 → 0/)).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should handle undefined prevContextTokens without crashing", () => { | ||||||||||||||||||||||||||||
| const props = { | ||||||||||||||||||||||||||||
| cost: 0.05, | ||||||||||||||||||||||||||||
| prevContextTokens: undefined as any, | ||||||||||||||||||||||||||||
| newContextTokens: 500, | ||||||||||||||||||||||||||||
| summary: "Context condensed", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...props} />) | ||||||||||||||||||||||||||||
| expect(container).toBeInTheDocument() | ||||||||||||||||||||||||||||
| // Should display 0 instead of undefined | ||||||||||||||||||||||||||||
| expect(screen.getByText(/0 →/)).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should handle undefined newContextTokens without crashing", () => { | ||||||||||||||||||||||||||||
| const props = { | ||||||||||||||||||||||||||||
| cost: 0.05, | ||||||||||||||||||||||||||||
| prevContextTokens: 1000, | ||||||||||||||||||||||||||||
| newContextTokens: undefined as any, | ||||||||||||||||||||||||||||
| summary: "Context condensed", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...props} />) | ||||||||||||||||||||||||||||
| expect(container).toBeInTheDocument() | ||||||||||||||||||||||||||||
| // Should display 0 instead of undefined | ||||||||||||||||||||||||||||
| expect(screen.getByText(/→ 0/)).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should handle null cost without crashing", () => { | ||||||||||||||||||||||||||||
| const props = { | ||||||||||||||||||||||||||||
| cost: null as any, | ||||||||||||||||||||||||||||
| prevContextTokens: 1000, | ||||||||||||||||||||||||||||
| newContextTokens: 500, | ||||||||||||||||||||||||||||
| summary: "Context condensed", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...props} />) | ||||||||||||||||||||||||||||
| expect(container).toBeInTheDocument() | ||||||||||||||||||||||||||||
| // Should display $0.00 for null cost | ||||||||||||||||||||||||||||
| expect(screen.getByText("$0.00")).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| it("should handle undefined cost without crashing", () => { | ||||||||||||||||||||||||||||
| const props = { | ||||||||||||||||||||||||||||
| cost: undefined as any, | ||||||||||||||||||||||||||||
| prevContextTokens: 1000, | ||||||||||||||||||||||||||||
| newContextTokens: 500, | ||||||||||||||||||||||||||||
| summary: "Context condensed", | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { container } = render(<ContextCondenseRow {...props} />) | ||||||||||||||||||||||||||||
| expect(container).toBeInTheDocument() | ||||||||||||||||||||||||||||
| // Should display $0.00 for undefined cost | ||||||||||||||||||||||||||||
| expect(screen.getByText("$0.00")).toBeInTheDocument() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| }) | |
| 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 | |
| }) |
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.tsdefines them as required fields. This creates a type safety issue.Is this intentional? The schema has:
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.