🐛 Fix screenshot upload failure and overlapping info tooltips#4134
🐛 Fix screenshot upload failure and overlapping info tooltips#4134clubanderson merged 1 commit intomainfrom
Conversation
Fix #4118: Screenshot uploads caused form submission to fail silently due to: - Fiber default body limit (4 MB) too small for base64-encoded images - GitHub API upload timeout (10s) too short for large screenshots - Frontend API timeout (15s) insufficient for server-side GitHub uploads - Base64 padding validation failure when browsers omit trailing '=' Fix #4120: Info icon showed both a native browser tooltip ("Card information") and a custom portal tooltip simultaneously, causing overlapping text. Removed the native title attribute since the custom tooltip already provides the card description. Closes #4118 Closes #4120 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
|
[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. |
There was a problem hiding this comment.
Pull request overview
Fixes feedback submission failures when screenshots are attached by increasing request/body limits and timeouts, and resolves overlapping “info” tooltips by relying solely on the custom tooltip.
Changes:
- Add a dedicated frontend timeout for feedback submissions that include screenshot uploads
- Increase backend request body limit and GitHub upload timeout, and normalize base64 padding for screenshots
- Remove the native
titletooltip from the info icon to prevent duplicated tooltip UI
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/lib/constants/network.ts | Adds FEEDBACK_UPLOAD_TIMEOUT_MS for longer-running screenshot submissions |
| web/src/hooks/useFeatureRequests.ts | Extends createRequest to accept request options (e.g., timeout) |
| web/src/components/feedback/FeedbackModal.tsx | Uses extended timeout only when screenshots are present |
| web/src/components/feedback/FeatureRequestModal.tsx | Uses extended timeout only when screenshots are present |
| web/src/components/cards/CardWrapper.tsx | Removes native tooltip to avoid overlap with custom tooltip |
| pkg/api/server.go | Increases Fiber BodyLimit to allow larger screenshot payloads |
| pkg/api/handlers/feedback.go | Normalizes base64 padding and extends GitHub upload timeout for screenshots |
| const createRequest = useCallback(async (input: CreateFeatureRequestInput, options?: { timeout?: number }) => { | ||
| try { | ||
| setIsSubmitting(true) | ||
| const { data } = await api.post<FeatureRequest>('/api/feedback/requests', input) | ||
| const { data } = await api.post<FeatureRequest>('/api/feedback/requests', input, options) |
There was a problem hiding this comment.
The new options parameter is typed as { timeout?: number } but is passed directly to api.post(...) as the request config. If api is an Axios-like client (or a custom wrapper that supports more config fields), this type is overly restrictive and can lead to unsafe/incorrect usage (callers can’t pass other supported config, and the shape may not match what api.post expects). Prefer typing options to the actual config type used by api.post (e.g., AxiosRequestConfig or your project’s ApiRequestOptions) and/or narrowing it at the call site instead of widening the hook API.
| onMouseEnter={() => setIsVisible(true)} | ||
| onMouseLeave={() => setIsVisible(false)} | ||
| className="p-0.5 rounded text-muted-foreground/50 hover:text-muted-foreground transition-colors" | ||
| aria-label={t('cardWrapper.cardInfo')} | ||
| title={t('cardWrapper.cardInfo')} | ||
| > |
There was a problem hiding this comment.
With the native title removed, the tooltip content is now only discoverable via mouse hover handlers (onMouseEnter/Leave). Keyboard users won’t trigger the custom tooltip, and screen readers will only get the button name (aria-label) without the descriptive tooltip text. Consider also toggling visibility on onFocus/onBlur and associating the tooltip content via aria-describedby (or ensuring the tooltip implementation itself handles focus/ARIA) so the information is accessible without relying on the removed native tooltip.
| // feedbackBodyLimit allows base64-encoded screenshot uploads (up to ~20 MB) | ||
| const feedbackBodyLimit = 20 * 1024 * 1024 | ||
| app := fiber.New(fiber.Config{ | ||
| ErrorHandler: customErrorHandler, | ||
| ReadBufferSize: 16384, | ||
| WriteBufferSize: 16384, | ||
| BodyLimit: feedbackBodyLimit, |
There was a problem hiding this comment.
Setting BodyLimit on the global Fiber app raises the max body size for all endpoints, which can increase memory pressure and amplify DoS risk from large request bodies unrelated to feedback. If only feedback endpoints need the larger limit, prefer applying a body-limit middleware scoped to the feedback routes (e.g., Fiber bodylimit middleware on a route group) while keeping a smaller global default, and/or pair this with rate limiting on the feedback submission endpoint.
| // Use a longer timeout for screenshot uploads (large base64 payloads) | ||
| uploadClient := &http.Client{Timeout: screenshotUploadTimeout} | ||
| resp, err := uploadClient.Do(req) |
There was a problem hiding this comment.
Creating a new http.Client here bypasses any custom configuration on h.httpClient (custom Transport, proxies, tracing, keep-alive tuning) and makes timeout behavior inconsistent across GitHub calls. Prefer keeping a single configured client and using a per-request timeout via context.WithTimeout (and req = req.WithContext(ctx) / http.NewRequestWithContext), or if a separate client is required, initialize it once (e.g., on the handler struct) reusing the same Transport as h.httpClient and only changing the timeout.
|
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. |
- 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>
- 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>
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 0 code suggestion(s) and 4 general comment(s). Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
=.titleattribute since the custom tooltip already provides the card description.Changes
FEEDBACK_UPLOAD_TIMEOUT_MS) used when screenshots are attached, removed duplicatetitleattribute fromInfoTooltipbuttonTest plan
cd web && npx tsc --noEmitgo build ./...Closes #4118
Closes #4120