-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Full rewrite #1
Full rewrite #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to ef134f7 in 49 seconds
More details
- Looked at
276
lines of code in10
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. lib/dev_toolbar/middleware.rb:10
- Draft comment:
The middleware checks forRails.env.development?
andheaders["Content-Type"]&.include?("text/html")
to determine if it should inject the toolbar. This is a good practice to ensure that the toolbar is only added in development mode and for HTML responses. However, it's important to note that modifying the response body directly can lead to issues if the response is streamed or frozen. Consider using a more robust HTML parsing and modification library if issues arise. - Reason this comment was not posted:
Confidence changes required:33%
The middleware checks forRails.env.development?
andheaders["Content-Type"]&.include?("text/html")
to determine if it should inject the toolbar. This is a good practice to ensure that the toolbar is only added in development mode and for HTML responses. However, it's important to note that modifying the response body directly can lead to issues if the response is streamed or frozen. Consider using a more robust HTML parsing and modification library if issues arise.
Workflow ID: wflow_YoOO1BdkfJhao7UG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on 82cdcc3 in 1 minute and 59 seconds
More details
- Looked at
26
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_U4vqsMP3ueetDQSF
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
def call(env) | ||
status, headers, response = @app.call(env) | ||
|
||
if Rails.env.development? && headers["Content-Type"]&.include?("text/html") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifying the response body directly can lead to issues with streaming responses or responses that are already gzipped. Consider handling different response types more robustly, possibly by using a more sophisticated approach to modify the response only if it's a complete HTML document and not encoded in a way that would break by simple string substitution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing this error when I have no homepage and also after I generate a homepage
Oops— nevermind that! I didn't read the gem instructions close enough 😅
The instructions from the README appear to work so I don't see any issues with your approach. Are there notes somewhere about what the requirements for this rewrite are?
@jelaniwoods, see the issue board on this repo. I think I'm tracking any additional features we want to add, but if you see any issues / wishlist features that are completely impossible with this approach then let me know.. probably hard to tell at this point, so we can cross those bridges when we reach them and merge this as is then work on the smaller bits. |
Related to #2
Given the scale of this re-write, I elected to create a new fresh
bundle gem dev_toolbar
repository as a first step and replace the old repo (https://github.com/firstdraft/dev_toolbar-v1) with this one.On this PR, I've rewritten the entire gem. The instructions to install and test the gem in a Rails app are in the README, so follow along there.
To test I recommend: Generate a new app from https://github.com/appdev-projects/rails-7-template, then scaffold some tables and then following the steps in the README for setup of the Gem.
The CSS is still not great looking, and I'm not 100% sure of my solution via middleware. But it seems to work, and I think improvements (listed on this open issue: #2) can be made on additional smaller PRs once this is merged.
After merging this, I suppose I should get this gem up on RubyGems. I note that is already there (https://rubygems.org/gems/dev_toolbar) but connected with the old version on the other repository.. I'm not sure how to handle this and if maybe I should have just opened this PR into that older repo.
Full disclosure; I wrote this gem with a bunch of help from GP-TA (see my thread). The AI led me down some bad paths along the way and I had to do a lot of back-and-forth to arrive at what it looks like now.
Summary:
This PR introduces a complete rewrite of the
dev_toolbar
gem, adding significant functionality and integration features for Rails applications.Key points:
dev_toolbar
gem.Configuration
,Middleware
, andRailtie
.Generated with ❤️ by ellipsis.dev
Summary:
This PR introduces a complete rewrite of the
dev_toolbar
gem, focusing on a new middleware component that injects a customizable development toolbar into HTML responses in Rails applications, along with other significant updates.Key points:
dev_toolbar
gem.Middleware
class to inject a development toolbar into HTML responses.Configuration
,Middleware
, andRailtie
.Generated with ❤️ by ellipsis.dev