-
Notifications
You must be signed in to change notification settings - Fork 1
Improve inspect output for IRB, Rails console, and ERB #47
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
base: main
Are you sure you want to change the base?
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.
Important
Looks good to me! 👍
Reviewed everything up to 117c9b6 in 2 minutes and 13 seconds. Click for details.
- Reviewed
208lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai-chat.rb:1
- Draft comment:
Good: The addition ofrequire_relative "ai/message"ensures that the Message class is loaded before it’s used in Chat. Verify the load order and dependency consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. lib/ai/amazing_print.rb:31
- Draft comment:
Informat_ai_chat, the code now maps overchat.inspectable_attributesand removes the previous truncation logic for message contents. If long messages might overwhelm the output, consider adding notes or reintroducing optional truncation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This comment is speculative - it says "If long messages might overwhelm the output, consider..." which is a conditional suggestion. The rules explicitly state "Do NOT make speculative comments, such as 'If X, then Y is an issue'. Only comment if it is definitely an issue." The comment doesn't identify a definite bug or problem, it's just raising a potential concern about what "might" happen. Additionally, the truncation logic was intentionally removed in this refactor, and the new implementation delegates toinspectable_attributes, which suggests the Chat class itself now handles what should be inspectable. Without evidence that this is causing a real problem, this is just a speculative concern. Perhaps the removal of truncation is actually a bug and will cause real issues with output being overwhelmed. The original code had explicit truncation for a reason, and removing it without replacement could be problematic. Maybe this is a legitimate concern that should be addressed. While truncation was removed, the refactor delegates tochat.inspectable_attributes, which suggests the Chat class now controls what gets displayed. This is likely intentional design - moving the responsibility to the Chat class itself. Without evidence of an actual problem (like a failing test or known issue), this remains speculative. This comment is speculative and conditional ("If long messages might...consider"), which violates the rule against speculative comments. The truncation was intentionally removed as part of a refactor that delegates toinspectable_attributes. Without evidence of a definite problem, this comment should be deleted.
3. lib/ai/chat.rb:87
- Draft comment:
Refactored message creation now usesMessage[...]instead of a hash literal. This is great for consistent custom display, but please verify thatMessage[]correctly returns an instance of Message (and not a plain Hash) to preserve your custom inspect behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the behavior of a function, which violates the rule against asking for confirmation or verification of behavior. It does not provide a specific suggestion or point out a clear issue.
4. lib/ai/chat.rb:233
- Draft comment:
The newinspectable_attributesmethod neatly orders and filters attributes for display. Consider adding inline documentation to clarify the rationale behind which attributes are always shown, conditionally shown, or omitted. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. lib/ai/chat.rb:265
- Draft comment:
Using AmazingPrint::Inspector in bothinspectandto_htmlis a neat enhancement. Consider if any fallback might be useful when AmazingPrint isn’t available, though the library already attempts to conditionally load it. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. lib/ai/message.rb:4
- Draft comment:
Defining Message as a subclass of Hash with custom inspect/to_html is a solid approach. Ensure that usingMessage[...](inherited from Hash[]) always returns an instance of Message so that the custom inspect behavior is preserved. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure a specific behavior is maintained, which violates the rule against asking the author to ensure behavior. It doesn't provide a specific suggestion or point out a specific issue with the code.
Workflow ID: wflow_66CIwSt0IjlCPhYa
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed d546688 in 44 seconds. Click for details.
- Reviewed
31lines of code in2files - Skipped
1files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. ai-chat.gemspec:5
- Draft comment:
Version bump to 0.4.1 looks good. Ensure that the changelog reflects this update. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. lib/ai/chat.rb:266
- Draft comment:
Refactoring inspect/to_html to use theaihelper is concise. Confirm that theaimethod from AmazingPrint is available in all contexts (e.g., in non-TTY or HTML environments) to avoid potential runtime errors. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_cprLOz6uDXz54VuV
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 10c7844 in 1 minute and 40 seconds. Click for details.
- Reviewed
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/chat.rb:270
- Draft comment:
Removing the custom 'pretty_print' method may break compatibility with libraries using Ruby’s PrettyPrint protocol (e.g., when callingpp chat). Consider retaining a minimalpretty_print(q)that delegates topretty_inspect(or wrapsinspect) to ensure consistent output in both IRB and test environments. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a speculative comment about potential breakage. The comment says "may break compatibility" and uses conditional language. According to the rules, I should NOT make speculative comments like "If X, then Y is an issue" - I should only comment if it's definitely an issue. The comment doesn't provide evidence that this will actually break anything, just that it "may" break. Additionally, the author clearly made this change intentionally, removing the complex pretty_print implementation in favor of a simpler pretty_inspect. If this were going to cause actual problems, it would likely be caught in testing. The comment is asking the author to "consider retaining" something they deliberately removed, which is second-guessing their decision without strong evidence. However, this could be a legitimate concern about Ruby's standard library behavior. Theppmethod does rely on the PrettyPrint protocol, and removingpretty_printcould change behavior. But the author may have intentionally wanted to simplify this, andpretty_inspectis also part of Ruby's pretty printing infrastructure. Without evidence that this actually breaks something, this is still speculative. While it's true thatppuses the PrettyPrint protocol, the comment is still speculative ("may break") and doesn't demonstrate actual breakage. The author clearly intended to simplify this code by removing the complexpretty_printimplementation. If this causes issues, they would be caught in testing. The comment is essentially asking the author to reconsider their deliberate change without strong evidence of a problem. This comment should be deleted. It's speculative ("may break compatibility"), doesn't provide evidence of actual breakage, and questions a deliberate design decision by the author. According to the rules, speculative comments should not be kept unless there's strong evidence of a definite issue.
Workflow ID: wflow_DoBL5dkuLAj6oIwt
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed a7cd738 in 47 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/message.rb:9
- Draft comment:
The new pretty_inspect method simply appends a newline to the inspect result. Consider checking if inspect might already include a trailing newline (to avoid duplicate newlines) and adding a brief comment clarifying its purpose for future maintainers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_R9puWQFKFzL58GkH
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
f85ee53 to
b18b3e4
Compare
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.
Important
Looks good to me! 👍
Reviewed f85ee53 in 2 minutes and 13 seconds. Click for details.
- Reviewed
784lines of code in8files - Skipped
2files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/chat.rb:246
- Draft comment:
The inspectable_attributes method uses msg.except(:response), but Hash#except is provided by ActiveSupport. Consider requiring 'active_support/core_ext/hash/except' or providing a fallback to ensure compatibility in non-Rails environments. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Looking at the full file context, I don't see a definition of theMessageclass within this file. The code usesMessage[...]syntax which could be a Hash literal or a custom class constructor. The comment assumes these are plain Hashes and thatexceptcomes from ActiveSupport. However, I cannot see the definition ofMessagein this file. The diff shows that the code was refactored to useMessage[...]instead of plain hash literals{...}. This is a significant change that suggestsMessagemight be a custom class defined elsewhere. Without seeing the Message class definition, I cannot definitively say whetherexceptis available or not. The comment might be incorrect if Message is a custom class that implementsexcept. I'm assuming that Message is just a Hash, but the refactoring in the diff from plain hash literals toMessage[...]suggests it might be a custom class. If Message is defined elsewhere in the codebase and implementsexcept, then this comment would be incorrect. While I cannot see the Message class definition in this file, the fact that the code was specifically refactored to useMessage[...]instead of plain hashes suggests intentional design. If the author introduced this pattern and is usingexcept, they likely ensured Message supports it. Without cross-file context showing Message is just a Hash alias, I should assume the author knows what they're doing. The comment makes an assumption about Message being a plain Hash without evidence. The refactoring to useMessage[...]suggests it's likely a custom class that may implementexcept. Without seeing the Message definition or evidence that this is actually a problem, this comment should be deleted.
2. lib/ai/chat.rb:86
- Draft comment:
The add method has two branches for constructing messages (simple vs multimodal) with similar key assignments. Consider refactoring or extracting a helper to reduce duplication and improve clarity. - Reason this comment was not posted:
Comment was on unchanged code.
3. ai-chat.gemspec:24
- Draft comment:
Base64 is a Ruby standard library. Consider removing it as a runtime dependency to avoid unnecessary dependency declarations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_f2Okl0gThWUCO61p
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed b18b3e4 in 1 minute and 43 seconds. Click for details.
- Reviewed
785lines of code in8files - Skipped
2files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. ai-chat.gemspec:24
- Draft comment:
Consider removing the runtime dependency on 'base64' and 'ostruct' since these are part of Ruby's standard library. If intentional, please document the rationale. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. lib/ai/chat.rb:86
- Draft comment:
The updated #add method now returns an AI::Message instance instead of a plain Hash, which improves consistency with generate! and the other helper methods. Good job! - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change in the code without suggesting any action or asking for clarification. It doesn't align with the rules for useful comments.
3. lib/ai/chat.rb:234
- Draft comment:
The 'inspectable_attributes' method is well structured, selectively including attributes and ordering them logically. Consider adding inline comments on the ordering rationale for future maintainers. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. lib/ai/chat.rb:268
- Draft comment:
The addition of the 'to_html' method for Chat is a useful improvement for ERB rendering. It appropriately marks the output as html_safe if possible. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it praises the addition of a method without providing any actionable feedback or suggestions for improvement. It does not align with the rules for useful comments.
5. lib/ai/message.rb:5
- Draft comment:
The custom inspect, pretty_inspect, pretty_print, and to_html methods in Message enhance output consistency across IRB, Rails console, and ERB. Looks clear and effective. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the methods enhance output consistency, which does not align with the rules for useful comments.
6. spec/unit/chat_spec.rb:8
- Draft comment:
The specs for AI::Chat are comprehensive and effectively cover the new behaviors (e.g., returning AI::Message, inspectable attributes, and HTML output). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it praises the comprehensiveness of the specs without providing any actionable feedback or suggestions for improvement. It does not align with the rules for useful comments.
7. spec/unit/message_spec.rb:5
- Draft comment:
The tests for AI::Message confirm that it behaves as a Hash and that the custom formatting methods work correctly. This is thorough and helps ensure reliability across output formats. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, praising the thoroughness of the tests. It does not provide any actionable feedback or suggestions for improvement.
Workflow ID: wflow_NHyC49NynaxWUfDS
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
b18b3e4 to
c437ec1
Compare
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.
Important
Looks good to me! 👍
Reviewed c437ec1 in 2 minutes and 6 seconds. Click for details.
- Reviewed
1282lines of code in13files - Skipped
2files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. examples/13_conversation_features_comprehensive.rb:117
- Draft comment:
The example calls 'chat.display_items' but no such method is defined in AI::Chat. Consider adding an alias or implementing this method (e.g., aliasing it to 'inspect' or 'pretty_inspect'). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is making an assumption thatdisplay_itemsdoesn't exist in theAI::Chatclass. However, I can't see the actual class definition in this diff - only the example file. The PR author deliberately changed fromverbosetodisplay_items, which strongly suggests they also updated the actual class implementation. This would be a cross-file issue that I can't verify from this diff alone. According to the rules, I should "Ignore cross-file issues. Only think about the file you are reviewing." Also, if this method truly didn't exist, it would be caught when running the example, so this falls under "Do NOT comment on anything that would be obviously caught by the build." Maybe the method really doesn't exist and the PR author forgot to add it? The comment could be catching a legitimate bug where the example was updated but the implementation wasn't. Even if that's true, this would be caught immediately when running the example (a NoMethodError would be raised). The rules explicitly state not to comment on things that would be obviously caught by the build. Additionally, this is a cross-file issue - I'd need to see the AI::Chat class definition to verify, which violates the rule about ignoring cross-file issues. This comment should be deleted. It's speculating about whether a method exists in a different file (cross-file issue), and if the method truly doesn't exist, it would be immediately caught when running the example. There's no strong evidence from this diff alone that the comment is correct.
2. lib/ai/chat.rb:200
- Draft comment:
In the get_items proxy branch, the transformation block uses 'hash.key(value) == :type' which doesn’t reliably detect when a key is :type. Since transform_values! only receives the value (not the key), consider iterating over key-value pairs (e.g., using each_with_object) to properly convert values for key :type. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_DK9GsIOVKFSc26w9
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
c437ec1 to
8e9b26a
Compare
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.
Important
Looks good to me! 👍
Reviewed 8e9b26a in 2 minutes and 5 seconds. Click for details.
- Reviewed
1295lines of code in14files - Skipped
2files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/chat.rb:202
- Draft comment:
The transformation inside the proxy branch of get_items is problematic. Usinghash.transform_values! do |value| if hash.key(value) == :type ...does not correctly check if the key is :type. Consider iterating over key/value pairs (e.g. using each_pair or transform_keys) to properly convert values for the :type key. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. examples/13_conversation_features_comprehensive.rb:53
- Draft comment:
The example still useschat.itemsinstead of the renamedchat.get_items, which conflicts with the breaking change. Update these calls for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. lib/ai/chat.rb:233
- Draft comment:
The use ofmsg.except(:response)in building inspectable attributes assumes availability of ActiveSupport's Hash#except. If running in a plain Ruby environment this may cause issues. Consider providing a fallback implementation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50%Hash#exceptwas added to Ruby core in version 3.0 (released December 2020). This means it's been part of standard Ruby for over 4 years now. The comment assumes this is an ActiveSupport-only method, which is outdated information. Unless this codebase needs to support Ruby versions older than 3.0 (which would be quite unusual in 2024), the comment is incorrect. The suggestion to userejectinstead is unnecessary and would make the code less readable. The comment is about changed code (theinspectable_attributesmethod is new in the diff), so it does relate to the changes. I don't know what Ruby version this project targets. If it needs to support Ruby 2.x for some reason, the comment would be valid. Also, I cannot see if there's a Gemfile or version specification that would clarify this. While I can't see the Ruby version requirement, it's extremely unlikely that a modern project in 2024 would target Ruby versions older than 3.0. Ruby 2.7 reached end-of-life in March 2023. The comment makes an incorrect assumption thatHash#exceptrequires ActiveSupport, when it's been part of Ruby core for years. Without strong evidence that this project needs to support ancient Ruby versions, this comment should be deleted. The comment is based on outdated information.Hash#excepthas been part of Ruby core since version 3.0 (December 2020) and doesn't require ActiveSupport. Unless there's clear evidence this project supports Ruby 2.x (which would be unusual), this comment is incorrect and should be deleted.
Workflow ID: wflow_Ox2IvNoB7N8dbyLQ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
8e9b26a to
0161d91
Compare
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.
Important
Looks good to me! 👍
Reviewed 0161d91 in 2 minutes and 30 seconds. Click for details.
- Reviewed
1358lines of code in15files - Skipped
2files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/chat.rb:191
- Draft comment:
In the proxy branch of get_items, the mapping over response_hash[:data] is executed if the key exists, but then create_deep_struct(response_hash) is always the final returned value. Please clarify if this is intentional or if the transformed data should be returned instead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. lib/ai/chat.rb:232
- Draft comment:
The use of Hash#except in inspectable_attributes may fail in pure Ruby since it's an ActiveSupport method. Consider adding a dependency or providing a fallback implementation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%Hash#exceptwas added to Ruby core in Ruby 3.0, so it's been available in standard Ruby for several years now. The comment is factually incorrect if the project targets Ruby 3.0+. Even if the project targets older Ruby versions, this would be caught immediately by tests or runtime errors, making it an obvious issue that doesn't need a review comment. Additionally, I cannot see theMessageclass definition to confirm these are even Hashes. The comment is speculative and likely incorrect. I cannot see what Ruby version this project targets, and I cannot see the definition of theMessageclass to confirm whether these objects are Hashes. Perhaps the project does target Ruby 2.x whereexceptis not available, or perhapsMessageobjects don't respond toexceptat all. Even if the project targets Ruby 2.x or ifMessageobjects don't respond toexcept, this would be immediately caught by any test suite or runtime execution. This is not a subtle bug that needs to be caught in code review. The comment also makes an incorrect claim thatexceptis an ActiveSupport method, when it's been in Ruby core since 3.0. Without strong evidence that this is actually a problem (like seeing a Ruby version constraint in the diff), I should delete this comment. The comment makes a factually incorrect claim aboutHash#exceptbeing ActiveSupport-only (it's been in Ruby core since 3.0), and even if there were a compatibility issue, it would be immediately caught by tests or runtime. There's no strong evidence this is a real problem, and the rules say not to make speculative comments.
3. lib/ai/chat.rb:636
- Draft comment:
The API key validation in validate_api_key relies on the prefix 'sk-proj'. This check might be brittle if the official key format changes. Consider a more robust solution or updating documentation. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
4. lib/ai/chat.rb:397
- Draft comment:
In prepare_messages_for_api, if last_response_id is not found, the entire messages array is returned. Verify that this behavior correctly handles edge cases and does not mistakenly resend previous messages. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
5. lib/ai/chat.rb:170
- Draft comment:
When setting schema via a String, parsing errors (e.g. JSON::ParserError) might occur. Consider adding explicit error handling to provide a clearer message to users. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
6. spec/unit/chat_spec.rb:111
- Draft comment:
The tests for inspectable_attributes assume a specific order of attributes (e.g. @conversation_id followed by @last_response_id). Ensure this order is intentional and documented, as future refactoring might change it. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
Workflow ID: wflow_NWsOXca4E2lR6yKt
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 7381ef3 in 39 seconds. Click for details.
- Reviewed
11lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai-chat.rb:2
- Draft comment:
The new HTML_PRE_STYLE update improves text wrapping for demo outputs. Consider using 'overflow-wrap: break-word' in addition to or instead of 'word-wrap', as the latter is deprecated in modern CSS. Also, verify that removing 'overflow-x: auto' doesn't negatively impact the display of long lines. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment suggests a specific code improvement by recommending the use ofoverflow-wrap: break-wordinstead ofword-wrap, which is deprecated. This is a valid suggestion as it aligns with modern CSS practices. However, the second part of the comment asks the author to verify the impact of removingoverflow-x: auto, which violates the rule against asking the author to verify or ensure behavior. The first part of the comment is useful, but the second part should be removed.
Workflow ID: wflow_IGqgy1wMmCxBgcRC
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
2036b22 to
23d7e4f
Compare
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.
Important
Looks good to me! 👍
Reviewed 23d7e4f in 1 minute and 17 seconds. Click for details.
- Reviewed
1361lines of code in15files - Skipped
2files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/chat.rb:202
- Draft comment:
Bug: In the transformation block for proxy responses in get_items, the code useshash.key(value) == :typewhich is incorrect. It seems intended to check if the current key is :type. Consider iterating with each_pair and checking the key directly (e.g.|k, v| k == :type ? v.to_sym : v). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. lib/ai/chat.rb:636
- Draft comment:
Note: The API key validation in validate_api_key relies on checking if the key starts with 'sk-proj'. This string prefix check may be brittle. Consider a more robust or configurable method for distinguishing official from unofficial API keys. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. lib/ai/chat.rb:194
- Draft comment:
Clarification: In the proxy branch of get_items, after mutating the response_hash, the code unconditionally calls create_deep_struct(response_hash) as the return value. Ensure that the intended transformed values are preserved and that create_deep_struct handles the mutated hash appropriately. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is asking the author to ensure that the intended transformed values are preserved and that a function handles the mutated hash appropriately. This falls under asking the author to ensure behavior, which is not allowed. The comment does not provide a specific suggestion or point out a clear issue, making it more of a request for confirmation rather than a constructive suggestion.
4. lib/ai/amazing_print.rb:6
- Draft comment:
Reminder: The monkey patch in AmazingPrint (lines 7-16) is intended as a temporary fix. Be sure to remove it once the upstream PR is merged. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_RCq3vZGgnD7ola1c
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
- Add AI::Message class (Hash subclass) with custom display - Add AI::Items class (SimpleDelegator) wrapping API response - Rename items to get_items to indicate API call - Enable reasoning summaries when reasoning_effort is set - Add to_html methods with dark terminal-style background - TTY-aware colorization (disabled when piped/redirected) - Add examples/16_get_items.rb demonstrating item inspection
2add4b6 to
3c4a734
Compare
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.
Important
Looks good to me! 👍
Reviewed 3c4a734 in 1 minute and 1 seconds. Click for details.
- Reviewed
1352lines of code in15files - Skipped
2files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/chat.rb:202
- Draft comment:
The transformation logic in get_items (proxy branch) uses 'hash.key(value) == :type', which does not reliably check for the :type key. This approach may lead to unintended behavior. Consider iterating over each key-value pair (for example, using each_pair or transforming only when the key equals :type) to ensure that the :type field is correctly converted to a symbol. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. lib/ai/amazing_print.rb:7
- Draft comment:
The custom formatting relies on AmazingPrint. Consider documenting this dependency or providing a fallback behavior if AmazingPrint is unavailable, so users are aware that inspect output improvements depend on it. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_zcpifRtFNRR513Um
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
bpurinton
left a comment
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 tested everything here and also created a small Rails app to test the views there. Everything looks good with the formatting.
| when :reasoning | ||
| puts "Reasoning: #{item.summary.first.text}" | ||
| # Reasoning summaries show the model's chain-of-thought | ||
| if item.summary&.first |
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.
This is maybe the most advanced syntax in the README that I think appdev 1 students wouldn't understand. Do we want to make an effort to refactor this to be more appdev 1 approachable?
maybe something like
if item.summary.length > 0There 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.
Good catch. A lot of stuff has accrued in the README and it's kind of a mess right now. I'll make this change but overall I think I want to make a thorough reference lesson in First Draft with runnable code blocks, and link to that from the README.
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.
That's a good point. If we do that, we should also probably avoid the case / when :symbol syntax and write the README with e.g.
if item.type == "message"
# ... etc.| module AI | ||
| class Message < Hash | ||
| def inspect | ||
| ai(plain: !$stdout.tty?, index: false) |
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.
The fact that AI and ai are unrelated confused me for a bit! 😄 I forgot it's amazing print's method.
The ergonomics of demo'ing the gem could be better. This patch attempts to make IRB/console output as beginner-friendly/demo-friendly as possible, as well as adding a
to_htmlmethod to make visualizing in ERB better.Summary
This PR improves how
AI::Chat, messages, and conversation items display in IRB, Rails console, and ERB templates.Key Changes
AI::Messageclass: Messages are nowAI::Messageinstances (a Hash subclass) with custominspectandto_htmlmethods for readable outputAI::Itemsclass: New wrapper usingSimpleDelegatorthat provides nice display while delegating all methods to the underlying API responseitems→get_items: Clearer naming to indicate the method makes an API callreasoning_effortis set, the API now returns chain-of-thought summaries showing the model's reasoning processto_htmlmethods: All display objects support rendering in ERB with a dark terminal-style backgroundBreaking Changes
chat.itemsis nowchat.get_itemschat.verbose; instead,chat.get_itemsformats itself more nicely.AI::Itemswrapper instead of raw OpenStruct (all methods still work via delegation)Test Plan
bundle exec rspec spec/unit/- 37 unit tests for display functionalitychat = AI::Chat.new; chat.user("hi"); chat.generate!; chatchat.get_itemswithreasoning_effort = "high"to see reasoning summarieschat.to_htmlandchat.get_items.to_htmlin an ERB templatebundle exec ruby examples/16_get_items.rbto see the full demoExample ERB:
Important
Enhances AI chat message and item inspection in IRB, Rails console, and ERB, introducing
AI::MessageandAI::Itemsclasses with improved display methods.AI::Messageclass now represents messages with custominspectandto_htmlmethods for better readability.AI::Itemsclass wraps conversation items with display methods, delegating other methods to the API response.chat.itemsrenamed tochat.get_itemsto indicate API call, returningAI::Itemswrapper.reasoning_effortis set, showing model's reasoning process.to_htmlmethods added for rendering in ERB with terminal-style background.chat.itemsis nowchat.get_items.chat.verbose;chat.get_itemsnow provides formatted output.AI::Itemsinstead of raw OpenStruct, with method delegation.examples/16_get_items.rbto demonstrate conversation item inspection.CHANGELOG.mdandREADME.mdto reflect changes.ai-chat.gemspec.This description was created by
for 3c4a734. You can customize this summary. It will automatically update as commits are pushed.