🐛 Address Copilot review comments from PRs #4129, #4130, #4134#4147
🐛 Address Copilot review comments from PRs #4129, #4130, #4134#4147clubanderson merged 1 commit intomainfrom
Conversation
- Include ClusterRole name in error messages for easier debugging (client.go) - Add machine-readable errorCode to sync endpoint 501 response (console_persistence.go) - Fix test hygiene: check json.Marshal/io.ReadAll errors, close response bodies, explicitly discard hash.Write returns (feedback_test.go) - Add onFocus/onBlur and aria-describedby to InfoTooltip for keyboard and screen-reader accessibility (CardWrapper.tsx) - Reuse h.httpClient with context.WithTimeout instead of creating a new http.Client per screenshot upload (feedback.go) - Scope elevated 20 MB body limit to feedback creation route only via per-route middleware guard (server.go) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
kubestellar-prow
bot
added
the
size/L
There was a problem hiding this comment.
Pull request overview
Addresses actionable Copilot review comments from prior PRs by improving accessibility, operational debug-ability, API error contracts, HTTP client usage, request body-size protections, and Go test hygiene.
Changes:
- Improves tooltip accessibility (keyboard + screen reader support) and adds stable tooltip identifiers.
- Enhances API/server behavior: clearer K8s error messages, machine-readable 501 error codes, HTTP client reuse with timeouts, and body-size limiting logic.
- Hardens tests by asserting on JSON marshaling/read errors and ensuring response bodies are closed.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/components/cards/CardWrapper.tsx | Adds focus/blur handling and ARIA wiring for InfoTooltip. |
| pkg/k8s/client.go | Includes ClusterRole name in relevant error messages. |
| pkg/api/server.go | Adds API body-size guard logic and updates comments/config around body limits. |
| pkg/api/handlers/feedback_test.go | Improves test robustness via helper functions and proper body closing. |
| pkg/api/handlers/feedback.go | Reuses handler HTTP client with per-request timeout via request context. |
| pkg/api/handlers/console_persistence.go | Expands 501 response with machine-readable errorCode. |
| const [position, setPosition] = useState<{ top: number; left: number } | null>(null) | ||
| const triggerRef = useRef<HTMLButtonElement>(null) | ||
| const tooltipRef = useRef<HTMLDivElement>(null) | ||
| const tooltipId = useRef(`info-tooltip-${Math.random().toString(36).slice(2, 9)}`).current |
There was a problem hiding this comment.
Two related accessibility issues: (1) generating tooltipId via Math.random() can cause unstable IDs across renders (and can lead to hydration mismatches if this component is ever SSR-rendered); prefer React 18's useId() or a deterministic ID source. (2) aria-describedby is only set when isVisible is true, and the tooltip content is only mounted when visible; on keyboard focus, assistive tech may query the description before state updates mount the tooltip. A more robust pattern is to keep a stable aria-describedby pointing to an element that remains in the DOM (visually hidden when inactive) and toggle visibility via CSS/aria-hidden.
| aria-describedby={isVisible ? tooltipId : undefined} | ||
| > | ||
| <Info className="w-3.5 h-3.5" /> | ||
| </button> | ||
| {isVisible && position && createPortal( | ||
| <div | ||
| ref={tooltipRef} | ||
| id={tooltipId} | ||
| role="tooltip" | ||
| className="fixed z-[100] max-w-xs px-3 py-2.5 text-xs leading-relaxed rounded-lg bg-background border border-border text-foreground shadow-xl animate-fade-in" |
There was a problem hiding this comment.
Two related accessibility issues: (1) generating tooltipId via Math.random() can cause unstable IDs across renders (and can lead to hydration mismatches if this component is ever SSR-rendered); prefer React 18's useId() or a deterministic ID source. (2) aria-describedby is only set when isVisible is true, and the tooltip content is only mounted when visible; on keyboard focus, assistive tech may query the description before state updates mount the tooltip. A more robust pattern is to keep a stable aria-describedby pointing to an element that remains in the DOM (visually hidden when inactive) and toggle visibility via CSS/aria-hidden.
| aria-describedby={isVisible ? tooltipId : undefined} | |
| > | |
| <Info className="w-3.5 h-3.5" /> | |
| </button> | |
| {isVisible && position && createPortal( | |
| <div | |
| ref={tooltipRef} | |
| id={tooltipId} | |
| role="tooltip" | |
| className="fixed z-[100] max-w-xs px-3 py-2.5 text-xs leading-relaxed rounded-lg bg-background border border-border text-foreground shadow-xl animate-fade-in" | |
| aria-describedby={tooltipId} | |
| > | |
| <Info className="w-3.5 h-3.5" /> | |
| </button> | |
| {position && createPortal( | |
| <div | |
| ref={tooltipRef} | |
| id={tooltipId} | |
| role="tooltip" | |
| className={cn( | |
| "fixed z-[100] max-w-xs px-3 py-2.5 text-xs leading-relaxed rounded-lg bg-background border border-border text-foreground shadow-xl animate-fade-in", | |
| !isVisible && "opacity-0 pointer-events-none" | |
| )} |
| {isVisible && position && createPortal( | ||
| <div | ||
| ref={tooltipRef} | ||
| id={tooltipId} |
There was a problem hiding this comment.
Two related accessibility issues: (1) generating tooltipId via Math.random() can cause unstable IDs across renders (and can lead to hydration mismatches if this component is ever SSR-rendered); prefer React 18's useId() or a deterministic ID source. (2) aria-describedby is only set when isVisible is true, and the tooltip content is only mounted when visible; on keyboard focus, assistive tech may query the description before state updates mount the tooltip. A more robust pattern is to keep a stable aria-describedby pointing to an element that remains in the DOM (visually hidden when inactive) and toggle visibility via CSS/aria-hidden.
| // feedbackBodyLimit is elevated globally to 20 MB to support base64-encoded | ||
| // screenshot uploads in the POST /api/feedback/requests endpoint. Non-feedback | ||
| // POST routes are protected by a tighter per-route body-size middleware | ||
| // (apiDefaultBodyLimit) to limit memory pressure from oversized requests. | ||
| const feedbackBodyLimit = 20 * 1024 * 1024 // 20 MB — base64 screenshot uploads |
There was a problem hiding this comment.
The per-route bodyGuard currently calls c.Body() to measure size. With a global BodyLimit of 20MB, this still allows up to ~20MB to be read/buffered into memory for any /api request before being rejected at 1MB, which undermines the stated goal of limiting memory pressure (and can worsen DoS behavior under load). Consider enforcing the smaller limit before buffering by checking Content-Length (when present) and/or restructuring to keep Fiber's global BodyLimit at 1MB and only allow 20MB on the feedback creation endpoint (e.g., via mounting a sub-app with a higher BodyLimit for /api/feedback/requests). This also appears inconsistent with the PR description claim of keeping a 1MB default for all other API routes while the app config sets 20MB globally.
| ErrorHandler: customErrorHandler, | ||
| ReadBufferSize: 16384, | ||
| WriteBufferSize: 16384, | ||
| BodyLimit: feedbackBodyLimit, |
There was a problem hiding this comment.
The per-route bodyGuard currently calls c.Body() to measure size. With a global BodyLimit of 20MB, this still allows up to ~20MB to be read/buffered into memory for any /api request before being rejected at 1MB, which undermines the stated goal of limiting memory pressure (and can worsen DoS behavior under load). Consider enforcing the smaller limit before buffering by checking Content-Length (when present) and/or restructuring to keep Fiber's global BodyLimit at 1MB and only allow 20MB on the feedback creation endpoint (e.g., via mounting a sub-app with a higher BodyLimit for /api/feedback/requests). This also appears inconsistent with the PR description claim of keeping a 1MB default for all other API routes while the app config sets 20MB globally.
| // Body-size guard: enforce a 1 MB limit on all API routes except the | ||
| // feedback creation endpoint which accepts large base64 screenshot payloads | ||
| // (up to the global 20 MB Fiber BodyLimit). | ||
| const apiDefaultBodyLimit = 1 * 1024 * 1024 // 1 MB — sufficient for JSON API requests | ||
| bodyGuard := func(c *fiber.Ctx) error { | ||
| if c.Method() == fiber.MethodPost && c.Path() == "/api/feedback/requests" { | ||
| // Allow the elevated feedbackBodyLimit (checked by Fiber global config) | ||
| return c.Next() | ||
| } | ||
| if len(c.Body()) > apiDefaultBodyLimit { | ||
| return fiber.ErrRequestEntityTooLarge | ||
| } | ||
| return c.Next() | ||
| } | ||
| api := s.app.Group("/api", apiLimiter, bodyGuard, middleware.JWTAuth(s.config.JWTSecret)) |
There was a problem hiding this comment.
The per-route bodyGuard currently calls c.Body() to measure size. With a global BodyLimit of 20MB, this still allows up to ~20MB to be read/buffered into memory for any /api request before being rejected at 1MB, which undermines the stated goal of limiting memory pressure (and can worsen DoS behavior under load). Consider enforcing the smaller limit before buffering by checking Content-Length (when present) and/or restructuring to keep Fiber's global BodyLimit at 1MB and only allow 20MB on the feedback creation endpoint (e.g., via mounting a sub-app with a higher BodyLimit for /api/feedback/requests). This also appears inconsistent with the PR description claim of keeping a 1MB default for all other API routes while the app config sets 20MB globally.
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 1 code suggestion(s) and 5 general comment(s). @copilot Please apply all of the following code review suggestions:
Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
Addresses all actionable Copilot review comments from three recently merged PRs:
PR #4129 — GPU health & persistence:
gpuHealthClusterRole) in error messages for easier operational debuggingerrorCode: "SYNC_NOT_IMPLEMENTED"to the 501 sync endpoint response so clients can handle it without string matchingPR #4130 — Feedback test hygiene:
payload, _ := json.Marshal(...)withrequire.NoErrorvia arequireMarshalJSONhelper to catch unexpected marshal failuresbody, _ := io.ReadAll(resp.Body)withrequire.NoErrorvia areadBodyhelper that also closes the bodydefer resp.Body.Close()to all tests that read response bodiesmac.Write()return values (_, _ =) to satisfy strict lintersPR #4134 — Accessibility, HTTP client reuse, body limit scoping:
onFocus/onBlurhandlers andaria-describedbytoInfoTooltipso keyboard users and screen readers can access tooltip contenthttp.Clientcreation inuploadScreenshotToGitHubwithcontext.WithTimeout+h.httpClientto reuse Transport (connection pooling, proxy settings)Test plan
go build ./...passesgo test ./pkg/api/handlers/... -run "TestFeedback|TestWebhook"— all 13 tests passnpx tsc --noEmitpasses (frontend)