🐛 Fix false 'screenshot uploaded' message when upload fails#4226
🐛 Fix false 'screenshot uploaded' message when upload fails#4226clubanderson merged 1 commit intomainfrom
Conversation
…4220) The frontend always showed "Screenshot uploaded and attached to the issue" based on local state (screenshots.length > 0), regardless of whether the backend actually uploaded them to GitHub. When the GitHub token lacks Contents write permission, the upload silently fails with a 404 and the issue is created without screenshots — but the UI claims success. Backend: return screenshots_uploaded/screenshots_failed counts in the create response; add a hint log when 404 suggests missing token permission. Frontend: use the backend response to show accurate status — green for success, yellow warning when uploads fail with guidance to check token permissions. 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. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
There was a problem hiding this comment.
Pull request overview
This PR addresses misleading UI feedback around screenshot attachments by propagating backend screenshot upload outcomes to the frontend, so the UI reflects what actually happened during GitHub issue creation.
Changes:
- Backend: Track screenshot upload successes/failures during issue creation and return those counts in the create response.
- Frontend: Use
screenshots_uploaded/screenshots_failedfrom the create response to render success/warning screenshot status banners in both feedback submission modals. - Types: Extend the
FeatureRequestinterface to include optional screenshot upload count fields (present on create responses).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| web/src/hooks/useFeatureRequests.ts | Extends FeatureRequest typing with optional screenshot upload result counts. |
| web/src/components/feedback/FeedbackModal.tsx | Uses backend-provided counts to show accurate screenshot upload status banners after submit. |
| web/src/components/feedback/FeatureRequestModal.tsx | Uses backend-provided counts to show accurate screenshot upload status banners after submit. |
| pkg/api/handlers/feedback.go | Tracks screenshot upload results during issue creation and returns counts in the create response; logs a hint on likely permission-related 404s. |
| if strings.Contains(err.Error(), "404") { | ||
| log.Printf("[Feedback] Hint: a 404 from the GitHub Contents API usually means the token lacks 'Contents: Read and write' permission. "+ | ||
| "Ensure FEEDBACK_GITHUB_TOKEN has this scope for %s/%s.", repoOwner, repoName) |
There was a problem hiding this comment.
The 404 detection is currently based on strings.Contains(err.Error(), "404"), which is brittle (it can match unrelated messages) and ties behavior to string formatting. Prefer checking for a specific sentinel (e.g. prefix like "GitHub Contents API returned 404") or returning a typed error/status code from uploadScreenshotToGitHub.
Also, the hint mentions FEEDBACK_GITHUB_TOKEN, but getEffectiveToken() can source the token from runtime settings; consider wording this as “configured feedback GitHub token” to avoid misleading operators.
| if strings.Contains(err.Error(), "404") { | |
| log.Printf("[Feedback] Hint: a 404 from the GitHub Contents API usually means the token lacks 'Contents: Read and write' permission. "+ | |
| "Ensure FEEDBACK_GITHUB_TOKEN has this scope for %s/%s.", repoOwner, repoName) | |
| if strings.HasPrefix(err.Error(), "GitHub Contents API returned 404") { | |
| log.Printf("[Feedback] Hint: a 404 from the GitHub Contents API usually means the token lacks 'Contents: Read and write' permission. "+ | |
| "Ensure the configured feedback GitHub token has this scope for %s/%s.", repoOwner, repoName) |
| // Return the request with screenshot upload status so the frontend can | ||
| // display an accurate message instead of always claiming success. | ||
| type createResponse struct { | ||
| *models.FeatureRequest | ||
| ScreenshotsUploaded int `json:"screenshots_uploaded"` | ||
| ScreenshotsFailed int `json:"screenshots_failed"` | ||
| } | ||
| return c.Status(fiber.StatusCreated).JSON(createResponse{ | ||
| FeatureRequest: request, | ||
| ScreenshotsUploaded: ssResult.Uploaded, | ||
| ScreenshotsFailed: ssResult.Failed, | ||
| }) |
There was a problem hiding this comment.
CreateFeatureRequest now returns additional fields (screenshots_uploaded, screenshots_failed) via a new response wrapper. There are existing handler tests in pkg/api/handlers/feedback_test.go, but none appear to assert this response shape/behavior. Please add a unit test that exercises a successful create response and verifies these fields are present and correctly reflect upload outcomes (including partial failure).
| console.debug('[Screenshot] Submit succeeded:', result.github_issue_url) | ||
| if (hasScreenshots) emitScreenshotUploadSuccess(screenshotDataURIs.length) | ||
|
|
||
| emitFeedbackSubmitted(type) | ||
|
|
||
| // Award coins based on type | ||
| const action = type === 'bug' ? 'bug_report' : 'feature_suggestion' | ||
| awardCoins(action as 'bug_report' | 'feature_suggestion', { title, type }) | ||
|
|
||
| // Clear draft on successful submit | ||
| localStorage.removeItem(DRAFT_KEY) | ||
| setSuccess({ issueUrl: result.github_issue_url }) | ||
| setSuccess({ | ||
| issueUrl: result.github_issue_url, | ||
| screenshotsUploaded: result.screenshots_uploaded, | ||
| screenshotsFailed: result.screenshots_failed, | ||
| }) |
There was a problem hiding this comment.
emitScreenshotUploadSuccess(screenshotDataURIs.length) still fires on any successful request submission, even if the backend reports that 0 screenshots uploaded (or partial failures). Since you now have result.screenshots_uploaded / result.screenshots_failed, please base analytics on those counts (emit success only when uploaded > 0, and emit a failed event when failed > 0) so telemetry matches the actual upload result.
| {screenshots.length > 0 && success && (success.screenshotsFailed ?? 0) > 0 && (success.screenshotsUploaded ?? 0) === 0 && ( | ||
| <div className="mb-4 p-3 rounded-lg bg-yellow-500/10 border border-yellow-500/20"> | ||
| <p className="text-xs text-yellow-400 font-medium"> | ||
| Screenshot could not be uploaded. The issue was created without it. Check that the GitHub token has "Contents: Read and write" permission. |
There was a problem hiding this comment.
The “all failed” warning message is always singular (“Screenshot could not be uploaded”) even when multiple screenshots were attached. Please pluralize based on screenshots.length (or success.screenshotsFailed) and consider aligning the guidance with the PR description (fine-grained PAT needs “Contents: Read and write”, classic PAT typically needs repo/public_repo) or use a more generally correct permission hint.
| Screenshot could not be uploaded. The issue was created without it. Check that the GitHub token has "Contents: Read and write" permission. | |
| {(success.screenshotsFailed ?? 0) === 1 | |
| ? 'Screenshot could not be uploaded. The issue was created without it.' | |
| : 'Screenshots could not be uploaded. The issue was created without them.'}{' '} | |
| Check that the GitHub token has sufficient repository permissions (for example, fine-grained tokens usually need "Contents: Read and write"; classic tokens typically need "repo" or "public_repo" scope). |
| {screenshots.length > 0 && success && (success.screenshotsFailed ?? 0) > 0 && (success.screenshotsUploaded ?? 0) === 0 && ( | ||
| <div className="mt-4 p-3 rounded-lg bg-yellow-500/10 border border-yellow-500/20"> | ||
| <p className="text-xs text-yellow-400 font-medium"> | ||
| Screenshot could not be uploaded. The issue was created without it. Check that the GitHub token has "Contents: Read and write" permission. |
There was a problem hiding this comment.
The “all failed” warning message is always singular (“Screenshot could not be uploaded”) even when multiple screenshots were attached. Please pluralize based on screenshots.length / success.screenshotsFailed, and consider aligning the permission guidance with both fine-grained (“Contents: Read and write”) and classic PAT scopes (repo/public_repo) or wording it generically so it’s not misleading.
| Screenshot could not be uploaded. The issue was created without it. Check that the GitHub token has "Contents: Read and write" permission. | |
| {(success.screenshotsFailed ?? 0) === 1 | |
| ? 'Screenshot could not be uploaded. The issue was created without it. Check that your GitHub token has permissions to read and write repository contents (for example, "Contents: Read and write" or classic scopes like "repo" / "public_repo").' | |
| : 'Screenshots could not be uploaded. The issue was created without them. Check that your GitHub token has permissions to read and write repository contents (for example, "Contents: Read and write" or classic scopes like "repo" / "public_repo").'} |
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 3 code suggestion(s) and 2 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
Fixes #4220
createGitHubIssueInReponow returnsscreenshots_uploadedandscreenshots_failedcounts in the create response. When upload fails with 404, logs a hint that the GitHub token likely needsContents: Read and writepermission.FeatureRequestModalandFeedbackModalnow use the backend response to display accurate screenshot status — green banner for success, yellow warning when uploads fail (with guidance to check token permissions). Previously, the UI always showed "Screenshot uploaded" based on local state regardless of backend result.Root cause: The GitHub Contents API returns 404 when the token lacks
Contents: Read and writepermission (fine-grained PATs) orrepo/public_reposcope (classic PATs). The Issues API works with justIssues: Read and write, so issue creation succeeds while screenshot upload silently fails.Test plan