Fix obscure errors by introducing FULL_TEXT_ERRORS #1695
Conversation
WalkthroughThe changes introduce a new environment-variable–controlled feature, Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant PR as PrerenderError
participant Utl as Utils
participant Log as Logger
App->>PR: An error occurs during prerendering
PR->>PR: Format error with Rainbow styling
PR->>PR: Check FULL_TEXT_ERRORS env variable
alt FULL_TEXT_ERRORS = true
PR->>PR: Use full error backtrace
else
PR->>PR: Trim backtrace to first 15 lines with hidden notice
end
PR->>Log: Log the formatted error message
App->>Utl: Call smart_trim(s, length)
Utl->>Utl: Check FULL_TEXT_ERRORS env variable
alt FULL_TEXT_ERRORS = true
Utl->>App: Return original string
else
Utl->>App: Return trimmed string
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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 (
|
![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 (2)
lib/react_on_rails/prerender_error.rb (1)
60-66: Consider enhancing the error message for better debugging.While the conditional logic is good, consider adding more context to help users debug issues faster:
- Include the environment (development/production)
- Add timestamp for error occurrence
backtrace = if ENV["FULL_TEXT_ERRORS"] == "true" err.backtrace.join("\n") else "#{err.backtrace.first(15).join("\n")}\n" + - Rainbow("The rest of the backtrace is hidden. " \ - "To see the full backtrace, set FULL_TEXT_ERRORS=true.").red + Rainbow("[#{Time.now}][#{Rails.env}] The rest of the backtrace is hidden. " \ + "To see the full backtrace, set FULL_TEXT_ERRORS=true.").red endlib/react_on_rails/utils.rb (1)
11-13: Consider extracting the environment variable check to a method.The
FULL_TEXT_ERRORScheck is repeated. Consider extracting it to a method for better maintainability:module ReactOnRails module Utils + def self.full_text_errors? + ENV["FULL_TEXT_ERRORS"] == "true" + end + TRUNCATION_FILLER = "\n... TRUNCATED #{ Rainbow('To see the full output, set FULL_TEXT_ERRORS=true.').red } ...\n".freeze def self.smart_trim(str, max_length = 1000) str = str.to_s - return str if ENV["FULL_TEXT_ERRORS"] == "true" + return str if full_text_errors?Also applies to: 191-191
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(2 hunks)lib/react_on_rails/prerender_error.rb(2 hunks)lib/react_on_rails/utils.rb(2 hunks)spec/react_on_rails/prender_error_spec.rb(1 hunks)spec/react_on_rails/utils_spec.rb(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
22-22: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (4)
spec/react_on_rails/prender_error_spec.rb (1)
49-72: Well-structured test coverage for the new FULL_TEXT_ERRORS feature!The test suite effectively covers both scenarios (enabled/disabled) with proper environment variable management.
lib/react_on_rails/prerender_error.rb (1)
3-3: LGTM! Good choice using Rainbow for colored output.The Rainbow gem is a good choice for improving error message visibility.
spec/react_on_rails/utils_spec.rb (1)
315-354: Excellent test coverage for smart_trim!The test suite thoroughly covers:
- Both FULL_TEXT_ERRORS scenarios
- Edge cases with different string lengths
- Hash handling
- Environment variable cleanup
CHANGELOG.md (1)
21-23: Document FULL_TEXT_ERRORS Feature in ChangelogThe changelog entry clearly records the fix for obscure errors by introducing the
FULL_TEXT_ERRORSfeature, and it correctly references PR 1695 by Romex91. Please verify that this entry is placed in the appropriate category (here, under "Fixed") according to the project's changelog conventions.Additionally, the bullet list item is indented by two spaces. According to markdown style guidelines (MD007), unordered list markers are expected to have no leading indentation. Consider removing these extra spaces to ensure consistency with the project's markdown style guidelines.
[approved, nitpick]
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
22-22: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
21-23: Markdown Lint: Adjust Unordered List Indentation
The markdown linter reports that the bullet list item is indented with 2 spaces (MD007) whereas the convention in this file is to have no indentation. To align with the expected style, consider removing the extra leading spaces from the bullet. For example, change:- - Fix obscure errors by introducing FULL_TEXT_ERRORS [PR 1695](https://github.com/shakacode/react_on_rails/pull/1695) by [Romex91](https://github.com/Romex91). + - Fix obscure errors by introducing FULL_TEXT_ERRORS [PR 1695](https://github.com/shakacode/react_on_rails/pull/1695) by [Romex91](https://github.com/Romex91).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
22-22: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
22-22: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (1)
CHANGELOG.md (1)
21-23: New Changelog Entry for FULL_TEXT_ERRORS
The new entry clearly documents the fix for obscure errors by introducing theFULL_TEXT_ERRORSfeature (as described in PR 1695 by Romex91). The entry is concise and consistent with the previous changelog entries.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
22-22: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/react_on_rails/prerender_error.rb (2)
56-56: Consider using err.message instead of err.inspect.Using
err.inspectmight expose internal error details that aren't relevant to users. Consider usingerr.messagefor a cleaner error message.- #{err.inspect} + #{err.message}
60-66: LGTM: Clear backtrace truncation with user guidance.The implementation effectively handles backtrace truncation and provides clear instructions for viewing the full backtrace.
However, consider extracting the magic number
15to a constant for better maintainability.+ BACKTRACE_LINES_TO_SHOW = 15 + def calc_message(component_name, console_messages, err, js_code, props) # ... - "#{err.backtrace.first(15).join("\n")}\n" + + "#{err.backtrace.first(BACKTRACE_LINES_TO_SHOW).join("\n")}\n" +lib/react_on_rails/utils.rb (1)
188-190: Consider caching the environment variable check.The
full_text_errors_enabled?method is called frequently but reads from ENV each time. Consider caching the result.def self.full_text_errors_enabled? - ENV["FULL_TEXT_ERRORS"] == "true" + return @full_text_errors if defined?(@full_text_errors) + @full_text_errors = ENV["FULL_TEXT_ERRORS"] == "true" end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/prerender_error.rb(2 hunks)lib/react_on_rails/utils.rb(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (3)
lib/react_on_rails/prerender_error.rb (1)
3-3: LGTM: Rainbow gem dependency added for colored output.The Rainbow gem is appropriately required for enhancing error message visibility with color.
lib/react_on_rails/utils.rb (2)
11-13: LGTM: Clear truncation message with color.The truncation message effectively communicates how to see the full output.
195-195: LGTM: Early return for full text output.The early return when full text errors are enabled is clean and efficient.
| backtrace = if Utils.full_text_errors_enabled? | ||
| err.backtrace.join("\n") | ||
| else | ||
| "#{err.backtrace.first(15).join("\n")}\n" + |
There was a problem hiding this comment.
Why do we need this at all? And why first 15 instead of the normal Rails backtrace cleaning?
There was a problem hiding this comment.
Do you suggest removing the ENV variable and using Rails.backtrace_cleaner?
There was a problem hiding this comment.
We can leave the variable and only use Rails.backtrace_cleaner in the else case.
There was a problem hiding this comment.
The alternative is just to always show the full stack trace, but I don't know why we'd ever want first N in particular unless N is 1 or 2.
Summary
Troubleshooting 500 errors is challenging as important parts of the error messages are hidden.
This PR adds the following message to truncated error texts:
Pull Request checklist
- [ ] Update documentation- No need, the new feature is self-documentedThis change is
Summary by CodeRabbit
New Features
FULL_TEXT_ERRORSenvironment variable.Bug Fixes