Error pages refactor#768
Conversation
WalkthroughThis pull request introduces a comprehensive overhaul of error handling and configuration management across the application. The changes focus on creating a more robust, informative error reporting mechanism that provides detailed error information in non-production environments. A new contact configuration has been added, and error templates have been consolidated into a single, more flexible template. The implementation includes enhanced error classes, improved middleware error handling, and a more structured approach to displaying technical errors. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![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
🧹 Nitpick comments (1)
config/util.js (1)
Line range hint
130-143: Consider enhancing error handling for production environmentsGiven that the PR objectives mention different error handling for production vs non-production environments, consider adding environment-specific validation logic.
export const validateConfig = (config) => { try { return v.parse(ConfigSchema, config) } catch (error) { + const isProd = process.env.NODE_ENV === 'production' || + process.env.ENVIRONMENT === 'production' + console.error('invalid config', error.message) for (const issue of error.issues) { - console.info( + console[isProd ? 'error' : 'info']( `issue under path: [${issue.path.map((elem) => elem.key).join(', ')}]` ) } throw error } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/util.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (2)
config/util.js (2)
97-102: Well-structured contact configuration schema!The implementation includes proper email validation and follows the established pattern of nested configuration objects. The trailing comma improves maintainability for future additions.
123-129: Well-documented validation function with proper type inference!The JSDoc documentation and error handling implementation are thorough and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/unit/middleware/entryIssueDetails.middleware.test.js (1)
131-131: LGTM! Enhanced error message clarity.The updated error messages now provide more specific context about missing values, which aligns well with the PR's objective of improving error handling.
Consider adding test cases for scenarios where both
issuesandresourcesare missing to ensure complete coverage of error states.Also applies to: 152-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/middleware/common.middleware.js(3 hunks)test/unit/middleware/entryIssueDetails.middleware.test.js(2 hunks)test/unit/views/organisations/lpaOverviewPage.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/unit/views/organisations/lpaOverviewPage.test.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/views/organisations/dataset-overview.test.js:3-3
Timestamp: 2024-11-12T10:13:49.419Z
Learning: Ensure to verify import paths in all relevant test files before flagging inconsistencies.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (4)
test/unit/views/organisations/lpaOverviewPage.test.js (1)
70-70: LGTM! Improved dataset validation.The enhanced checks for both existence and length of datasets provide better protection against edge cases with empty arrays.
Also applies to: 76-76
src/middleware/common.middleware.js (3)
9-9: LGTM! Added error handling utilities.The import of error handling utilities aligns with the PR's objective of standardising error handling.
101-102: LGTM! Enhanced error handling in query validation.The use of
MiddlewareErrorwith status code and context provides a more structured approach to handling validation errors.
131-131: LGTM! Standardised 404 error handling.The change to use
MiddlewareErrorfor page range errors maintains consistency with the new error handling approach.
GeorgeGoodall-GovUk
left a comment
GeorgeGoodall-GovUk
left a comment
There was a problem hiding this comment.
awesome! this is going to be super useful
3 participants
What type of PR is this? (check all applicable)
Description
This PR removes all the
errorPages/xxx.htmltemplates and replaces them with a single error template. It introduces a Error subclassMiddlewareErrorthat should be use when we need to force the page to display specific error.The error pages include stack traces and (when relevant) schema validation issues. Both are displayed only on when the environment
process.env.NODE_ENV || process.env.ENVIRONMENTis not set toproduction. Also, no errors details are shown on 404 page - it would be noise.Also: the templates for URL/file upload errors in the check tool provide
/checklink to start over (it used to be/urland/upload).Related Tickets & Documents
Note: #762 should be merged first.
QA Instructions, Screenshots, Recordings
See example URL for a 400 error page
Note: Schema Issues are only present when the error was cause by invalid schema.
Added/updated tests?
QA sign off
Summary by CodeRabbit
Release Notes
New Features
MiddlewareErrorclassBug Fixes
Documentation
Refactor
Chores
These changes improve the application's error reporting, user guidance, and overall error management strategy.