fix: add a link to a github issue when themes are missing #37
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe recent updates focus on improving error handling within an API. Error messages have been clarified, and HTTP status codes adjusted to better reflect the nature of the errors. A specific case for a missing theme has been addressed, providing users with a helpful link to report or request themes on GitHub. Changes
TipsChat with CodeRabbit Bot (
|
|
@levino approval on this one is kinda of urgent |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- apps/registry/pages/api/[payload].js (2 hunks)
- apps/registry/pages/api/formatters/template.js (1 hunks)
Additional comments: 3
apps/registry/pages/api/[payload].js (1)
- 54-58: The error message correction from "not supported formatted" to "not supported formatter" improves clarity and is consistent with the summary.
apps/registry/pages/api/formatters/template.js (2)
52-54: The error handling for a missing theme renderer is correctly implemented by throwing an error with the message 'theme-missing'. This aligns with the summary provided.
58-58: The return structure of the
formatfunction remains consistent with the previous version, as indicated by the summary.
apps/registry/pages/api/[payload].js
Outdated
| if (e.message === 'theme-missing') { | ||
| return res | ||
| .status(200) | ||
| .send( | ||
| failMessage( | ||
| 'This theme is currently unsupported. Please visit this Github issue to request it https://github.com/jsonresume/jsonresume.org/issues/36 (unfortunately we have recently (11/2023) disabled a bunch of legacy themes due to critical flaws in them, please request if you would like them back.)' | ||
| ) | ||
| ); |
There was a problem hiding this comment.
The addition of a custom error message for the "theme-missing" error is a good enhancement for user feedback. However, the HTTP status code 200 is not appropriate for an error condition. Consider using a 4xx status code to indicate client-side errors.
- .status(200)
+ .status(404) // or another appropriate 4xx error codeCommittable 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.
| if (e.message === 'theme-missing') { | |
| return res | |
| .status(200) | |
| .send( | |
| failMessage( | |
| 'This theme is currently unsupported. Please visit this Github issue to request it https://github.com/jsonresume/jsonresume.org/issues/36 (unfortunately we have recently (11/2023) disabled a bunch of legacy themes due to critical flaws in them, please request if you would like them back.)' | |
| ) | |
| ); | |
| if (e.message === 'theme-missing') { | |
| return res | |
| .status(404) // or another appropriate 4xx error code | |
| .send( | |
| failMessage( | |
| 'This theme is currently unsupported. Please visit this Github issue to request it https://github.com/jsonresume/jsonresume.org/issues/36 (unfortunately we have recently (11/2023) disabled a bunch of legacy themes due to critical flaws in them, please request if you would like them back.)' | |
| ) | |
| ); |
apps/registry/pages/api/[payload].js
Outdated
| return res | ||
| .status(200) | ||
| .send( |
There was a problem hiding this comment.
The HTTP status code for the generic error message should also reflect an error condition. Consider using a 4xx or 5xx status code to indicate client or server errors, respectively.
- .status(200)
+ .status(500) // or another appropriate error codeCommittable 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.
| return res | |
| .status(200) | |
| .send( | |
| return res | |
| .status(500) // or another appropriate error code | |
| .send( |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- apps/registry/pages/api/[payload].js (4 hunks)
Additional comments: 4
apps/registry/pages/api/[payload].js (4)
56-57: The error message has been correctly updated to 'not supported formatter', which is more accurate and aligns with the summary.
123-123: The HTTP status code for the error message 'Cannot fetch gist, no idea why' has been correctly updated to 400, indicating a bad request, which is more appropriate than the previous 200 status code.
135-135: The HTTP status code for the error message 'Validation failed' has been correctly updated to 400, which is the correct status code for validation errors.
176-181: The new conditional block for the 'theme-missing' error has been added, providing a detailed message with a link to a Github issue for theme requests. This is a helpful addition for users encountering this error.
|
Please see #38 (comment) If you get impatient, let me know and I will approve anyhow. |
|
@thomasdavis You can now "override" the branch protection rules and merge as you please. There should be a checkbox with a red warning text just above the "merge button". This is so you do not become blocked if I am absent etc. I suggest to still only use it when it is really necessary ("emergency"). |
|
I would also add a test to try to open the site with a non-working theme and see that it returns an error code and check that the message contains expected strings (like "github issue" or something). |
|
@levino good feedback. just merging for now so i can collect the errors of the themes that are missing. but will add the test, and move to a theme request template later today |
Fix #38
Summary by CodeRabbit
Bug Fixes
Documentation