feat: show 4xx status codes as warning (orange) in Functions and Sites - #2351
Conversation
ConsoleProject ID: Sites (2)
Note You can use Avatars API to generate QR code for any text or URLs. |
WalkthroughIntroduces
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/sheet.svelte (2)
102-106: Remove redundant optional chaining in guarded blockInside
{#if selectedLog},selectedLogis non-null. Drop?.for consistency.- type={selectedLog?.responseStatusCode >= 500 + type={selectedLog.responseStatusCode >= 500
102-106: Handle status code 0 consistently (neutral/no type) or confirm it can't occur for SitesFunctions executions views treat
0as neutral (typeundefined). Sites views currently map it to'success'. Align or confirm the product difference is intentional.- : 'success'} /> + : selectedLog.responseStatusCode === 0 + ? undefined + : 'success'} />src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.svelte (1)
99-105: De-duplicate status→badge mapping via a small helperThis ternary appears across 4 files. Centralize to avoid drift.
Add once (e.g.,
src/lib/helpers/httpStatus.ts):export type BadgeType = 'success' | 'warning' | 'error' | undefined; export const statusCodeToBadgeType = (code?: number): BadgeType => code == null || code === 0 ? undefined : code >= 500 ? 'error' : code >= 400 ? 'warning' : 'success';Then apply here:
- type={log.responseStatusCode >= 500 - ? 'error' - : log.responseStatusCode >= 400 - ? 'warning' - : log.responseStatusCode === 0 - ? undefined - : 'success'} + type={statusCodeToBadgeType(log.responseStatusCode)}And import:
import { statusCodeToBadgeType } from '$lib/helpers/httpStatus';src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.svelte (2)
90-94: Add 0→neutral case for parity with Functions, or confirm 0 never appears for SitesWithout this,
0shows as'success'here but neutral elsewhere.- : log.responseStatusCode >= 400 - ? 'warning' - : 'success'} + : log.responseStatusCode >= 400 + ? 'warning' + : log.responseStatusCode === 0 + ? undefined + : 'success'}
90-94: Prefer shared helper to keep mappings consistentIf you add
statusCodeToBadgeType, replace the ternary here too:- type={log.responseStatusCode >= 500 ? 'error' : log.responseStatusCode >= 400 ? 'warning' : 'success'} + type={statusCodeToBadgeType(log.responseStatusCode)}src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/sheet.svelte (1)
101-107: Drop optional chaining; the block already guardsselectedLogKeeps the condition set consistent and slightly cleaner.
- type={selectedLog?.responseStatusCode >= 500 + type={selectedLog.responseStatusCode >= 500
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/sheet.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/sheet.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.svelte(1 hunks)
🔇 Additional comments (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.svelte (2)
99-105: LGTM: Correct 5xx→error, 4xx→warning, 0→neutral mappingMatches the PR intent and improves UX differentiation.
99-105: Confirm Badge.type supports 'warning' and undefinedVerified — Pink's Tag/Badge supports a "warning" state and the
typeprop is optional (omitting it renders a neutral badge).src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/sheet.svelte (1)
101-107: LGTM: Mapping matches table view and PR intent5xx→error, 4xx→warning, 0→neutral, else→success looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/lib/helpers/httpStatus.ts (3)
1-6: Clarify JSDoc with explicit ranges and special case.Document exact mapping (5xx → error, 4xx → warning, 1xx–3xx → success, 0/invalid → undefined) for quicker comprehension and future consistency.
/** - * determines the badge color based on HTTP status code + * Determine badge type from an HTTP status code. + * Mapping: + * - 5xx → 'error' + * - 4xx → 'warning' + * - 1xx–3xx → 'success' + * - 0, negative, or non‑finite → undefined (no type) * * @param statusCode - * @returns badge color + * @returns 'error' | 'warning' | 'success' | undefined */
7-9: Optional: Reuse a shared type for badge “type”.If Pink Svelte exposes a badge prop type, import it; otherwise declare a local alias to avoid repeating unions.
+export type BadgeType = 'error' | 'warning' | 'success'; export function getBadgeTypeFromStatusCode( - statusCode: number -): 'error' | 'warning' | 'success' | undefined { + statusCode: number +): BadgeType | undefined {
7-23: Add light tests for mapping.Cover representative codes (204, 301, 404, 418, 500, 0, -1, NaN).
+// Suggested file: src/lib/helpers/httpStatus.spec.ts +import { describe, it, expect } from 'vitest'; +import { getBadgeTypeFromStatusCode } from './httpStatus'; + +describe('getBadgeTypeFromStatusCode', () => { + it('maps 5xx to error', () => expect(getBadgeTypeFromStatusCode(500)).toBe('error')); + it('maps 4xx to warning', () => { + expect(getBadgeTypeFromStatusCode(404)).toBe('warning'); + expect(getBadgeTypeFromStatusCode(418)).toBe('warning'); + }); + it('maps 1xx–3xx to success', () => { + expect(getBadgeTypeFromStatusCode(204)).toBe('success'); + expect(getBadgeTypeFromStatusCode(301)).toBe('success'); + }); + it('returns undefined for 0/invalid', () => { + expect(getBadgeTypeFromStatusCode(0)).toBeUndefined(); + expect(getBadgeTypeFromStatusCode(-1)).toBeUndefined(); + expect(getBadgeTypeFromStatusCode(Number.NaN as unknown as number)).toBeUndefined(); + }); +});src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.svelte (1)
88-93: Avoid passingundefinedprop to Badge when status is 0.If
getBadgeTypeFromStatusCode(...)returnsundefined, Svelte may still pass an explicittype={undefined}which can override the component’s default. Prefer omitting the prop in that case.- <Badge - variant="secondary" - type={getBadgeTypeFromStatusCode(log.responseStatusCode)} - content={log.responseStatusCode.toString()} /> + {#if getBadgeTypeFromStatusCode(log.responseStatusCode) === undefined} + <Badge + variant="secondary" + content={log.responseStatusCode.toString()} /> + {:else} + <Badge + variant="secondary" + type={getBadgeTypeFromStatusCode(log.responseStatusCode)} + content={log.responseStatusCode.toString()} /> + {/if}To confirm behavior, please verify in UI that a log with
responseStatusCode = 0renders a neutral (non‑orange/non‑red) badge. If it doesn’t, apply the diff above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/helpers/httpStatus.ts(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/sheet.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/sheet.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/sheet.svelte
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/sheet.svelte
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (2)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.svelte (2)
15-15: Good centralization.Importing the helper here keeps badge logic consistent across views.
15-15: Verified — helper migrated; Badge supports "warning"
- No legacy inline mappings found (no matches for ">= 400 ? 'error' : 'success'").
- getBadgeTypeFromStatusCode is present at src/lib/helpers/httpStatus.ts and is used in the Sites and Functions views (e.g. src/routes/.../sites/.../logs/table.svelte & sheet.svelte; src/routes/.../functions/.../executions/table.svelte & sheet.svelte).
- Pink Svelte Badge supports type="warning" (node_modules/.../pink-svelte/dist/Badge.svelte defines warning-primary/secondary) and the repo already uses type="warning" in several places (auth/updateStatus.svelte, storage/bucket +page.svelte, functions/.../domains/recordsCard.svelte, org members +page.svelte, domains tables).
| export function getBadgeTypeFromStatusCode( | ||
| statusCode: number | ||
| ): 'error' | 'warning' | 'success' | undefined { | ||
| if (statusCode >= 500) { | ||
| return 'error'; | ||
| } | ||
|
|
||
| if (statusCode >= 400) { | ||
| return 'warning'; | ||
| } | ||
|
|
||
| if (statusCode === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return 'success'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden mapping for invalid/edge values; simplify 0-check.
Guard against non-finite and negative status codes to avoid accidentally treating them as “success.” This also lets you drop the separate === 0 branch.
export function getBadgeTypeFromStatusCode(
statusCode: number
): 'error' | 'warning' | 'success' | undefined {
- if (statusCode >= 500) {
+ // Treat non-finite or non-positive (0, negatives) as “no status”
+ if (!Number.isFinite(statusCode) || statusCode <= 0) {
+ return undefined;
+ }
+
+ if (statusCode >= 500) {
return 'error';
}
- if (statusCode >= 400) {
+ if (statusCode >= 400) {
return 'warning';
}
- if (statusCode === 0) {
- return undefined;
- }
-
return 'success';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getBadgeTypeFromStatusCode( | |
| statusCode: number | |
| ): 'error' | 'warning' | 'success' | undefined { | |
| if (statusCode >= 500) { | |
| return 'error'; | |
| } | |
| if (statusCode >= 400) { | |
| return 'warning'; | |
| } | |
| if (statusCode === 0) { | |
| return undefined; | |
| } | |
| return 'success'; | |
| } | |
| export function getBadgeTypeFromStatusCode( | |
| statusCode: number | |
| ): 'error' | 'warning' | 'success' | undefined { | |
| // Treat non-finite or non-positive (0, negatives) as “no status” | |
| if (!Number.isFinite(statusCode) || statusCode <= 0) { | |
| return undefined; | |
| } | |
| if (statusCode >= 500) { | |
| return 'error'; | |
| } | |
| if (statusCode >= 400) { | |
| return 'warning'; | |
| } | |
| return 'success'; | |
| } |
🤖 Prompt for AI Agents
In src/lib/helpers/httpStatus.ts around lines 7 to 23, the function currently
treats non-finite and negative status codes as "success" and keeps a separate
check for statusCode === 0; change the guard so any non-finite or non-positive
value returns undefined, then apply the usual >=500 => 'error', >=400 =>
'warning', else 'success'. Use Number.isFinite(statusCode) and check statusCode
> 0 to validate the input and remove the dedicated 0 branch.
2 participants
What does this PR do?
Update status code badge coloring so client errors (4xx) are orange and server errors (5xx) remain red across Functions Executions and Sites Logs (tables and sheets).
Test Plan
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
yes
Summary by CodeRabbit