-
-
Notifications
You must be signed in to change notification settings - Fork 199
fix(proxy): persist Fake 200 error detail to DB/Redis and display in dashboard #814
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -131,9 +131,11 @@ export function ProviderChainPopover({ | |||||
| const hasFake200PostStreamFailure = chain.some( | ||||||
| (item) => typeof item.errorMessage === "string" && item.errorMessage.startsWith("FAKE_200_") | ||||||
| ); | ||||||
| const fake200CodeForDisplay = chain.find( | ||||||
| (item) => typeof item.errorMessage === "string" && item.errorMessage.startsWith("FAKE_200_") | ||||||
| )?.errorMessage; | ||||||
| const fake200CodeForDisplay = chain | ||||||
| .find( | ||||||
| (item) => typeof item.errorMessage === "string" && item.errorMessage.startsWith("FAKE_200_") | ||||||
| ) | ||||||
| ?.errorMessage?.split(": ")[0]; | ||||||
|
|
||||||
| // Calculate actual request count (excluding intermediate states) | ||||||
| const requestCount = chain.filter(isActualRequest).length; | ||||||
|
|
@@ -545,7 +547,7 @@ export function ProviderChainPopover({ | |||||
| {t("logs.details.fake200DetectedReason", { | ||||||
| reason: t( | ||||||
| getFake200ReasonKey( | ||||||
| item.errorMessage, | ||||||
| item.errorMessage.split(": ")[0], | ||||||
|
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. potential runtime error if The code assumes all fake 200 errors have the new format, but old errors or errors without detail will cause
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/dashboard/logs/_components/provider-chain-popover.tsx
Line: 550
Comment:
potential runtime error if `item.errorMessage` doesn't contain ": "
The code assumes all fake 200 errors have the new format, but old errors or errors without detail will cause `.split(": ")[0]` to return the full string. Line 138 handles this correctly by using optional chaining (`?.errorMessage?.split`), but this line doesn't.
```suggestion
item.errorMessage?.split(": ")[0] ?? item.errorMessage,
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||
| "logs.details.fake200Reasons" | ||||||
| ) | ||||||
| ), | ||||||
|
|
||||||
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.
[MEDIUM] [TEST-EDGE-CASE] No unit coverage for new
"FAKE_200_*: detail"parsingWhy this is a problem: New logic now relies on
errorMessage.split(": ")[0]to recover the FAKE_200 code for i18n lookups:src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:72-77src/app/[locale]/dashboard/logs/_components/provider-chain-popover.tsx:134-138and:549-552Existing UI tests only exercise the raw code format (no
": detail"suffix), so a delimiter/format regression will slip:src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx:400-414src/app/[locale]/dashboard/logs/_components/provider-chain-popover.test.tsx:274-291This conflicts with
CLAUDE.md:2. **Test Coverage** - All new features must have unit test coverage of at least 80%Suggested fix: Add/extend tests to cover the new
"CODE: detail"format.