-
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -33,9 +33,12 @@ 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")} | ||
| {prevContextTokens?.toLocaleString() ?? "0"} → {newContextTokens?.toLocaleString() ?? "0"}{" "} | ||
| {t("tokens")} | ||
| </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)} | ||
|
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. 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. |
||
| </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,175 @@ | ||
| // npx vitest run src/components/chat/__tests__/ContextCondenseRow.spec.tsx | ||
|
|
||
| import React from "react" | ||
| import { render, fireEvent } from "@/utils/test-utils" | ||
| import { ContextCondenseRow, CondensingContextRow, CondenseContextErrorRow } from "../ContextCondenseRow" | ||
|
|
||
| // Mock i18n | ||
| vi.mock("react-i18next", () => ({ | ||
| useTranslation: () => ({ | ||
| t: (key: string) => key, | ||
| }), | ||
| })) | ||
|
|
||
| // Mock VSCode components | ||
| vi.mock("@vscode/webview-ui-toolkit/react", () => ({ | ||
| VSCodeBadge: function MockVSCodeBadge({ children, className }: { children: React.ReactNode; className?: string }) { | ||
| return <span className={className}>{children}</span> | ||
| }, | ||
| })) | ||
|
|
||
| // Mock Markdown component | ||
| vi.mock("../Markdown", () => ({ | ||
| Markdown: function MockMarkdown({ markdown }: { markdown: string }) { | ||
| return <div data-testid="markdown">{markdown}</div> | ||
| }, | ||
| })) | ||
|
|
||
| // Mock ProgressIndicator component | ||
| vi.mock("../ProgressIndicator", () => ({ | ||
| ProgressIndicator: function MockProgressIndicator() { | ||
| return <div data-testid="progress-indicator">Loading...</div> | ||
| }, | ||
| })) | ||
|
|
||
| describe("ContextCondenseRow", () => { | ||
| it("renders with valid token values", () => { | ||
| const { getByText } = render( | ||
| <ContextCondenseRow | ||
| cost={1.5} | ||
| prevContextTokens={1000} | ||
| newContextTokens={500} | ||
| summary="Context condensed successfully" | ||
| />, | ||
| ) | ||
|
|
||
| expect(getByText("chat:contextCondense.title")).toBeInTheDocument() | ||
| expect(getByText(/1,000 → 500 tokens/)).toBeInTheDocument() | ||
| expect(getByText("$1.50")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("handles null token values gracefully", () => { | ||
| const { getByText } = render( | ||
| <ContextCondenseRow | ||
| cost={0} | ||
| prevContextTokens={null as any} | ||
| newContextTokens={null as any} | ||
| summary="Context condensed" | ||
| />, | ||
| ) | ||
|
|
||
| expect(getByText("chat:contextCondense.title")).toBeInTheDocument() | ||
| // Should display "0" for null values | ||
| expect(getByText(/0 → 0 tokens/)).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("handles undefined token values gracefully", () => { | ||
| const { getByText } = render( | ||
| <ContextCondenseRow | ||
| cost={undefined as any} | ||
| prevContextTokens={undefined as any} | ||
| newContextTokens={undefined as any} | ||
| summary="Context condensed" | ||
| />, | ||
| ) | ||
|
|
||
| expect(getByText("chat:contextCondense.title")).toBeInTheDocument() | ||
| // Should display "0" for undefined values | ||
| expect(getByText(/0 → 0 tokens/)).toBeInTheDocument() | ||
| expect(getByText("$0.00")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("handles mixed null and valid token values", () => { | ||
| const { getByText } = render( | ||
| <ContextCondenseRow | ||
| cost={2.5} | ||
| prevContextTokens={2000} | ||
| newContextTokens={null as any} | ||
| summary="Context condensed" | ||
| />, | ||
| ) | ||
|
|
||
| expect(getByText("chat:contextCondense.title")).toBeInTheDocument() | ||
| // Should display "2,000 → 0 tokens" for mixed values | ||
| expect(getByText(/2,000 → 0 tokens/)).toBeInTheDocument() | ||
| expect(getByText("$2.50")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("expands and collapses when clicked", () => { | ||
| const { getByText, queryByTestId } = render( | ||
| <ContextCondenseRow | ||
| cost={1.5} | ||
| prevContextTokens={1000} | ||
| newContextTokens={500} | ||
| summary="Context condensed successfully" | ||
| />, | ||
| ) | ||
|
|
||
| // Initially collapsed | ||
| expect(queryByTestId("markdown")).not.toBeInTheDocument() | ||
|
|
||
| // Click to expand | ||
| const header = getByText("chat:contextCondense.title").parentElement?.parentElement | ||
| fireEvent.click(header!) | ||
|
|
||
| // Should show summary | ||
| expect(queryByTestId("markdown")).toBeInTheDocument() | ||
| expect(getByText("Context condensed successfully")).toBeInTheDocument() | ||
|
|
||
| // Click to collapse | ||
| fireEvent.click(header!) | ||
|
|
||
| // Should hide summary | ||
| expect(queryByTestId("markdown")).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("hides badge when cost is 0", () => { | ||
| const { container } = render( | ||
| <ContextCondenseRow cost={0} prevContextTokens={1000} newContextTokens={500} summary="Context condensed" />, | ||
| ) | ||
|
|
||
| const badge = container.querySelector(".opacity-0") | ||
| expect(badge).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("shows badge when cost is greater than 0", () => { | ||
| const { container } = render( | ||
| <ContextCondenseRow | ||
| cost={1.5} | ||
| prevContextTokens={1000} | ||
| newContextTokens={500} | ||
| summary="Context condensed" | ||
| />, | ||
| ) | ||
|
|
||
| const badge = container.querySelector(".opacity-100") | ||
| expect(badge).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
||
| describe("CondensingContextRow", () => { | ||
| it("renders with progress indicator", () => { | ||
| const { getByText, getByTestId } = render(<CondensingContextRow />) | ||
|
|
||
| expect(getByTestId("progress-indicator")).toBeInTheDocument() | ||
| expect(getByText("chat:contextCondense.condensing")).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
|
||
| describe("CondenseContextErrorRow", () => { | ||
| it("renders with error text", () => { | ||
| const errorText = "Failed to condense context: API error" | ||
| const { getByText } = render(<CondenseContextErrorRow errorText={errorText} />) | ||
|
|
||
| expect(getByText("chat:contextCondense.errorHeader")).toBeInTheDocument() | ||
| expect(getByText(errorText)).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("renders without error text", () => { | ||
| const { getByText, queryByText } = render(<CondenseContextErrorRow />) | ||
|
|
||
| expect(getByText("chat:contextCondense.errorHeader")).toBeInTheDocument() | ||
| // Should not show any error text when not provided | ||
| expect(queryByText(/Failed/)).not.toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
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. 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? |
||
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?