feat: added terminal title status output #438
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e95270a in 1 minute and 36 seconds
More details
- Looked at
286lines of code in4files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. gptme/chat.py:46
- Draft comment:
The terminal_title_state context manager resets the title to the default (using _make_title) on exit. If nested title changes are expected, consider preserving and restoring the previous state. - 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%
- The comment points out a potential issue with nested context manager usage
- However, I cannot see the implementation of set_terminal_state and reset_terminal_title
- Without seeing those implementations, I cannot verify if this is actually an issue
- The comment is speculative ("if nested title changes are expected...")
- The current code looks reasonable for the simple use cases shown in the diff
I may be too quick to dismiss this - nested context managers are a real pattern and this could be a legitimate architectural concern.
While nested context managers are important, this comment is speculative and we don't have enough context to verify if it's a real issue. The code works for its current usage patterns.
Delete the comment because it is speculative ("if nested title changes are expected") and we cannot verify if it's a real issue without seeing the terminal utility implementations.
2. gptme/tools/__init__.py:15
- Draft comment:
The tool_execution_state context manager is clean. Ensure that the tool_name passed is concise, as it directly appears in the terminal title. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
3. gptme/util/ask_execute.py:27
- Draft comment:
The confirmation_title_state context manager is used to set a specific title during prompts. Its usage improves UX, and the implementation looks sound. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. gptme/util/terminal.py:34
- Draft comment:
In _set_raw_title, using print() for emitting ANSI sequences works in most cases, but consider sys.stdout.write for finer control in non-interactive scenarios. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
5. gptme/util/ask_execute.py:180
- Draft comment:
The return expression does not handle the default=False case. When default is false, it evaluates to an empty list, so even a 'y' input will return false. Consider using something like: return answer in (['y', 'yes'] if not default else ['y', 'yes', '']). - 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.
6. gptme/util/ask_execute.py:177
- Draft comment:
The help option uses a recursive call to ask_execute, which may lead to deep recursion if help is requested multiple times. Consider refactoring to an iterative loop. - 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.
7. gptme/chat.py:92
- Draft comment:
The use of the terminal_title_state context manager for initialization sets a temporary title that resets right after init. Confirm this is intended; if a persistent title is desired, the context scope might need to be adjusted. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. gptme/tools/__init__.py:131
- Draft comment:
Ensure that 'tooluse.tool' is a string as expected by the tool_execution_state context manager. If it's an object, consider using its name or another identifier. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_63BjSTZ6Xw3g7TqG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #438 +/- ##
==========================================
+ Coverage 65.46% 67.15% +1.68%
==========================================
Files 70 71 +1
Lines 6226 6266 +40
==========================================
+ Hits 4076 4208 +132
+ Misses 2150 2058 -92
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ErikBjare
force-pushed
the
dev/terminal-status
branch
from
e95270a to
d37e8e7
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d37e8e7 in 30 seconds
More details
- Looked at
290lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. gptme/chat.py:70
- Draft comment:
Using 'set_current_conv_name(logdir.name)' immediately sets the conversation name; ensure that logdir.name is sanitized if it might contain unexpected characters. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. gptme/chat.py:268
- Draft comment:
Good use of the terminal_state_title context manager around the reply generation. Verify that exceptions inside yield still result in a proper terminal title reset. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
3. gptme/util/terminal.py:57
- Draft comment:
The reset_terminal_title() uses _make_title() without a state, which reuses _current_conv_name. Ensure this is the intended behavior for title reset after temporary state changes. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
4. gptme/chat.py:70
- Draft comment:
Good addition: setting the terminal title with the conversation name improves context. Consider validating logdir.name to avoid unwanted characters in terminal titles. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. gptme/chat.py:276
- Draft comment:
Wrapping the call to reply() in terminal_state_title provides clear status feedback. Ensure that any exceptions within reply() are handled properly within this context. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
6. gptme/chat.py:298
- Draft comment:
Using terminal_state_title in prompt_user to indicate input waiting state is a nice UX improvement. The placement of clear_interruptible() outside the context seems appropriate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. gptme/tools/__init__.py:116
- Draft comment:
Wrapping tool execution in terminal_state_tool contextualizes tool runs. Verify that tooluse.tool is reliably a valid string and consider sanitizing if necessary. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. gptme/util/ask_execute.py:126
- Draft comment:
Using the confirmation_title_state context manager to update the terminal title during confirmation prompt enhances clarity. This is a clean implementation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. gptme/util/terminal.py:67
- Draft comment:
The new terminal manipulation utilities are clear. Consider using sys.stdout.write instead of print when setting the terminal title to avoid any unexpected formatting, though the current approach is acceptable. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_FeJ4UpwsokFsU1ve
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ErikBjare
force-pushed
the
dev/terminal-status
branch
from
d37e8e7 to
63f3e6b
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 63f3e6b in 27 seconds
More details
- Looked at
282lines of code in4files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. gptme/chat.py:276
- Draft comment:
Using the terminal_state_title context manager to wrap the reply generation is clear and neat. Confirm that the title reset is acceptable in non-interactive environments. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. gptme/chat.py:298
- Draft comment:
The prompt_user function wraps the waiting-for-input phase with terminal_state_title appropriately. This is a clean integration; ensure that title resets are handled correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. gptme/tools/__init__.py:116
- Draft comment:
Wrapping tool execution within terminal_state_tool provides clear context in the terminal title. This looks well integrated. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. gptme/util/ask_execute.py:115
- Draft comment:
The terminal_state_confirm context manager is correctly applied to the user prompt session. This helps indicate waiting state; ensure consistency with other terminal contexts. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. gptme/util/terminal.py:11
- Draft comment:
The new terminal manipulation utilities are straightforward. Verify that using ANSI escape sequences via print is consistent across your supported terminal emulators. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. gptme/chat.py:70
- Draft comment:
Nice addition: setting the terminal title with the conversation name (using logdir.name) enhances UX. Ensure the conversation name is always a valid string for the title. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. gptme/chat.py:268
- Draft comment:
Good use of the terminal_state_title context manager during response generation to update terminal title. This temporary UI feedback is a useful addition. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. gptme/chat.py:298
- Draft comment:
Wrapping the prompt input loop with terminal_state_title helps indicate waiting state. This improves user experience during user input. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. gptme/tools/__init__.py:118
- Draft comment:
Using terminal_state_tool around tool execution is effective. Just confirm that 'tooluse.tool' reliably yields a string identifier for proper terminal title display. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. gptme/util/ask_execute.py:115
- Draft comment:
Good integration: the terminal_state_confirm context manager clearly indicates when the system is waiting for user confirmation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. gptme/util/terminal.py:10
- Draft comment:
The new terminal utility module with context managers for updating terminal titles is well implemented. Consider adding comments about potential fallback options for non-ANSI terminals if needed in the future. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
Workflow ID: wflow_MX7Mz5gNIAOcLTZb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ErikBjare
force-pushed
the
dev/terminal-status
branch
from
63f3e6b to
a02bee9
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a02bee9 in 40 seconds
More details
- Looked at
262lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. gptme/tools/__init__.py:118
- Draft comment:
Use an f-string for dynamic interpolation. Change "🛠️ running {tooluse.tool}" to f"🛠️ running {tooluse.tool}". - Reason this comment was not posted:
Comment looked like it was already resolved.
2. gptme/chat.py:268
- Draft comment:
Good use of context manager for terminal state during response generation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. gptme/chat.py:298
- Draft comment:
Nice addition of terminal state when waiting for user input. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. gptme/util/ask_execute.py:115
- Draft comment:
Appropriate wrapping of the session prompt with terminal_state_title for confirmation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. gptme/util/terminal.py:10
- Draft comment:
New terminal utility file looks good; consider adding tests for escape sequences if possible. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. gptme/tools/__init__.py:118
- Draft comment:
Use an f-string here to interpolate the tool name. Change "🛠️ running {tooluse.tool}" to f"🛠️ running {tooluse.tool}". - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_rhr1eDh5qeWPrPKi
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Adds terminal title updates to reflect current status during message generation, tool execution, and user input in
gptmeapplication.terminal_state_title()inchat.py,tools/__init__.py, andask_execute.py.chat()inchat.py.terminal_state_title(),set_terminal_state(), andreset_terminal_title()inutil/terminal.pyfor managing terminal titles.step()inchat.pywithterminal_state_title("🤔 generating").prompt_user()inchat.pywithterminal_state_title("⌨️ waiting for input").execute_msg()intools/__init__.pywithterminal_state_title("🛠️ running {tooluse.tool}").ask_execute()inask_execute.pywithterminal_state_title("❓ waiting for confirmation").This description was created by
for a02bee9. It will automatically update as commits are pushed.