-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: a bunch of smaller enhancements #30
base: master
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.
❌ Changes requested. Reviewed everything up to 6ddeb73 in 2 minutes and 18 seconds
More details
- Looked at
607
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. src/components/ConversationContent.tsx:58
- Draft comment:
The focus flag reset via a 100ms timeout works but could be fragile; consider more deterministic focus management. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. src/utils/markdownUtils.ts:78
- Draft comment:
The check for transforming thinking tags (skipping content wrapped in backticks) is a bit naive; it may miss inline code cases. - 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. src/components/ChatInput.tsx:92
- Draft comment:
Both the local keydown handler (lines 92-100) and the global document listener (lines 57-69) trigger onInterrupt for Escape. Consider calling e.stopPropagation() in handleKeyDown to avoid duplicate calls if the event bubbles. - 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.
4. src/utils/markdownUtils.ts:77
- Draft comment:
The early return in transformThinkingTags for content starting and ending with backticks (lines 78-80) may be too broad and could skip transformation for inline code segments. Consider refining this check. - 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.
5. src/utils/markdownUtils.ts:99
- Draft comment:
Extracting the language tag from the code block's class (using split(' ')[1]) might be brittle if no language class is present. Consider a more robust fallback mechanism. - 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_21qHAQv2LcDbSgGG
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.
}, [autoFocus, isReadOnly, api.isConnected]); | ||
|
||
// Global keyboard shortcut for interrupting generation with Escape key | ||
useEffect(() => { |
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.
Consider if duplicate Escape handling (global listener vs. Textarea
keydown) might trigger onInterrupt
twice.
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.
Maybe only have the global listener, so that user can escape once to unfocus textarea and once again to interrupt?
Not sure how these handlers work together right now.
if (!code) return null; | ||
|
||
const handleCopy = () => { | ||
navigator.clipboard.writeText(code); |
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.
Consider adding error handling for navigator.clipboard.writeText
in handleCopy
to gracefully handle clipboard API failures.
navigator.clipboard.writeText(code); | |
try { navigator.clipboard.writeText(code); } catch (err) { console.error('Failed to copy text:', err); } |
CodeDisplay
component with code highlighting, line numbers, copy button (idea was to use it for rendered chat-markdown, but not so easy)hljs.getLanguage(lang)
, but we should!