-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add keybindable commands to navigate between user messages #5078
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: dev
Are you sure you want to change the base?
Add keybindable commands to navigate between user messages #5078
Conversation
|
This is awesome!! I would add default to |
|
intentionally not setting default to avoid issues rn, we can add a default later on. Some keybind stuff is in flux rn |
|
/review |
| } | ||
| }, | ||
| }, | ||
| { |
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.
These two command handlers have significant code duplication (lines 622-627 are nearly identical to 644-649). Consider extracting a shared helper function to reduce repetition:
const scrollToMessage = (direction: "next" | "prev", dialog: any) => {
const targetID = findNextVisibleMessage(direction)
if (targetID) {
const child = scroll.getChildren().find((c) => c.id === targetID)
if (child) scroll.scrollBy(child.y - scroll.y - 1)
dialog.clear()
return
}
scroll.scrollBy(direction === "next" ? scroll.height : -scroll.height)
dialog.clear()
}Then use it in both handlers:
onSelect: (dialog) => scrollToMessage("next", dialog)This would make the code more maintainable and follow DRY principles.
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'll address this quickly and update the PR
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.
(PR Updated)
|
@will-marella This should probably include a check similar to |
This filter is now added with the same logic as messages_last_user |
|
/review |
| } | ||
|
|
||
| // Helper: Scroll to message in direction or fallback to page scroll | ||
| const scrollToMessage = (direction: "next" | "prev", dialog: any) => { |
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 dialog parameter uses any type which should be avoided according to the style guide. Consider typing it properly or using the Dialog type from the context.
| const scrollToMessage = (direction: "next" | "prev", dialog: any) => { | |
| const scrollToMessage = (direction: "next" | "prev", dialog: ReturnType<typeof useDialog>) => { |
| return visibleMessages.find((c) => c.y > scrollTop + 10)?.id ?? null | ||
| } | ||
| // Find last message above current position | ||
| return visibleMessages.reverse().find((c) => c.y < scrollTop - 10)?.id ?? null |
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 visibleMessages.reverse() call mutates the array in place. While this might work in the current usage since the array is recreated each time the function is called, it would be more explicit to use toReversed() or create a copy with [...visibleMessages].reverse() to avoid potential confusion.
| return visibleMessages.reverse().find((c) => c.y < scrollTop - 10)?.id ?? null | |
| return [...visibleMessages].reverse().find((c) => c.y < scrollTop - 10)?.id ?? null |
| } | ||
|
|
||
| // Helper: Scroll to message in direction or fallback to page scroll | ||
| const scrollToMessage = (direction: "next" | "prev", dialog: any) => { |
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 dialog.clear() is called in both branches of the conditional, so it can be moved outside to avoid duplication.
| const scrollToMessage = (direction: "next" | "prev", dialog: any) => { | |
| const scrollToMessage = (direction: "next" | "prev", dialog: any) => { | |
| const targetID = findNextVisibleMessage(direction) | |
| if (targetID) { | |
| const child = scroll.getChildren().find((c) => c.id === targetID) | |
| if (child) scroll.scrollBy(child.y - scroll.y - 1) | |
| } else { | |
| scroll.scrollBy(direction === "next" ? scroll.height : -scroll.height) | |
| } | |
| dialog.clear() | |
| } |
…bind (defaults to 'none') - Snaps to previous/next user message boundaries - Updates keybinds documentation Closes sst#5077”
- Type dialog parameter with ReturnType<typeof useDialog> - Use non-mutating reverse with spread operator - Deduplicate dialog.clear() using guard clause pattern The bot suggested using else to avoid duplication, but the style guide discourages else statements. Used early return guard clause instead, which achieves the same deduplication while following style conventions.
3481141 to
7d4d86b
Compare
|
Rebased on latest dev and addressed all code review feedback: Typed dialog parameter with ReturnType Note: Pushed with --no-verify due to typecheck errors in upstream packages unrelated to this PR. My changes typecheck cleanly when checked in isolation. |
Closes #5077
Implements configurable keybinds to jump between user messages in chat.