chore: properly change fetch remote css map spinner#3522
Conversation
WalkthroughAdded descriptive messages to spinner notifications in the preprocess step for remote CSS map retrieval, covering success and error cases. No control flow, data handling, or exported declarations were changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/preprocess/preprocess.go (1)
54-64: Close HTTP response body and check status code (resource leak, misreported errors).
cssMapResp.Bodyisn’t closed; repeated runs can leak FDs. Also, a non-200 response is treated as JSON-malformed.- cssMapResp, err := http.Get(cssMapURL) - if err != nil { - return err - } else { - err := json.NewDecoder(cssMapResp.Body).Decode(cssTranslationMap) - if err != nil { - utils.PrintWarning("Remote CSS map JSON malformed.") - return err - } - } + resp, err := http.Get(cssMapURL) + if err != nil { + return err + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("GET %s: %s", cssMapURL, resp.Status) + } + if err := json.NewDecoder(resp.Body).Decode(cssTranslationMap); err != nil { + utils.PrintWarning("Remote CSS map JSON malformed.") + return err + }
🧹 Nitpick comments (3)
src/preprocess/preprocess.go (3)
91-91: Make the warning message precise (this failure is tag fetch, not CSS map fetch).The first failure is fetching the latest tag. The message suggests CSS map fetch failed, which can mislead when the subsequent remote fetch might still succeed.
- fetchSpinner.Warning("Failed to fetch remote CSS map") + fetchSpinner.Warning("Failed to fetch latest tag; will try remote CSS map")
95-101: Surface error details and add tag context to success.Helps debugging and confirms which tag/main was used.
- if readRemoteCssMap(tag, &cssTranslationMap) != nil { - fetchSpinner.Warning("Failed to fetch remote CSS map") - utils.PrintInfo("Using local CSS map instead") - readLocalCssMap(&cssTranslationMap) - } else { - fetchSpinner.Success("Fetched remote CSS map") - } + if err := readRemoteCssMap(tag, &cssTranslationMap); err != nil { + fetchSpinner.Warning(fmt.Sprintf("Failed to fetch remote CSS map (%v)", err)) + utils.PrintInfo("Using local CSS map instead") + readLocalCssMap(&cssTranslationMap) + } else { + fetchSpinner.Success(fmt.Sprintf("Fetched remote CSS map (%s)", tag)) + }
88-88: Optional: don’t ignore spinner start error.Capture/log it to aid diagnostics if the spinner backend fails.
- fetchSpinner, _ := utils.Spinner.Start("Fetching remote CSS map") + fetchSpinner, spinnerErr := utils.Spinner.Start("Fetching remote CSS map") + if spinnerErr != nil { + utils.PrintWarning(fmt.Sprintf("Spinner start failed: %v", spinnerErr)) + }
2 participants
I forgot about this one
Summary by CodeRabbit