Austinamoruso/shifttab #36
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @austinamorusoyardstick, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the application's navigation capabilities by introducing a Shift+Tab keybinding. This new feature allows users to cycle through open tabs in reverse, providing a more complete and intuitive tab management experience alongside the existing forward tab switching.
Highlights
- New Keybinding for Reverse Tab Switching: I've identified that this PR introduces
Shift+Tabas a new keybinding, enabling users to navigate through tabs in reverse order, complementing the existingTabkey for forward navigation. - Core Logic for Reverse Tab Navigation: A new
ToggleReversemethod has been implemented within theTabbedWindowclass, which contains the core logic for cycling through tabs backwards, ensuring correct index wrapping. - Keybinding Configuration and Help Text Updates: The necessary keybinding configurations across
keys/keys.gohave been updated to registerShift+Tab, map it to the new functionality, and provide appropriate help text for user guidance.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces Shift+Tab functionality for reverse tab navigation, which is a nice usability improvement. The implementation is straightforward and correct. I've added a couple of suggestions to improve maintainability and robustness: one to prevent a potential panic in ToggleReverse and another to refactor duplicated code in the key press handler. Overall, great work!
app/app.go
Outdated
| case keys.KeyShiftTab: | ||
| m.tabbedWindow.ToggleReverse() | ||
| m.menu.SetInDiffTab(m.tabbedWindow.IsInDiffTab()) | ||
| return m, m.instanceChanged() |
There was a problem hiding this comment.
The logic inside this case block is almost identical to the one for keys.KeyTab (lines 885-887). This introduces code duplication. To improve maintainability, consider refactoring this into a shared helper function.
func (m *home) handleTabSwitch(reverse bool) (tea.Model, tea.Cmd) {
if reverse {
m.tabbedWindow.ToggleReverse()
} else {
m.tabbedWindow.Toggle()
}
m.menu.SetInDiffTab(m.tabbedWindow.IsInDiffTab())
return m, m.instanceChanged()
}
case keys.KeyTab:
return m.handleTabSwitch(false)
case keys.KeyShiftTab:
return m.handleTabSwitch(true)| func (w *TabbedWindow) ToggleReverse() { | ||
| w.activeTab = (w.activeTab - 1 + len(w.tabs)) % len(w.tabs) | ||
| } |
There was a problem hiding this comment.
For robustness, it's good practice to guard against a potential division by zero if w.tabs were ever an empty slice. While it's currently initialized with a fixed size, adding a check will make the code more resilient to future changes. The existing Toggle() method has the same potential issue and would benefit from a similar guard.
if len(w.tabs) == 0 {
return
}
w.activeTab = (w.activeTab - 1 + len(w.tabs)) % len(w.tabs)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully introduces the Shift+Tab keybinding for reverse tab navigation. The changes are well-structured, with updates to keybindings, application logic, and UI components. The new handleTabSwitch function in app/app.go is a good refactoring that centralizes tab switching logic.
I've included one suggestion to further improve maintainability in ui/tabbed_window.go by reducing code duplication between the Toggle and ToggleReverse functions.
Overall, this is a solid contribution that enhances user experience.
| func (w *TabbedWindow) Toggle() { | ||
| if len(w.tabs) == 0 { | ||
| return | ||
| } | ||
| w.activeTab = (w.activeTab + 1) % len(w.tabs) | ||
| } | ||
|
|
||
| // ToggleReverse cycles through tabs in reverse order | ||
| func (w *TabbedWindow) ToggleReverse() { | ||
| if len(w.tabs) == 0 { | ||
| return | ||
| } | ||
| w.activeTab = (w.activeTab - 1 + len(w.tabs)) % len(w.tabs) | ||
| } |
There was a problem hiding this comment.
While the implementation is correct, there's an opportunity to reduce code duplication between Toggle() and ToggleReverse(). You can extract the common logic into a private helper method. This improves maintainability by centralizing the tab cycling logic.
func (w *TabbedWindow) Toggle() {
w.cycleTabs(1)
}
// ToggleReverse cycles through tabs in reverse order
func (w *TabbedWindow) ToggleReverse() {
w.cycleTabs(-1)
}
// cycleTabs handles cycling through tabs in a given direction.
func (w *TabbedWindow) cycleTabs(direction int) {
if len(w.tabs) == 0 {
return
}
numTabs := len(w.tabs)
w.activeTab = (w.activeTab + direction + numTabs) % numTabs
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully introduces reverse tab-switching with Shift+Tab. The implementation is clean, with good refactoring in app/app.go and ui/tabbed_window.go to support both forward and reverse cycling. The new logic in cycleTabs is robust and correctly handles edge cases like an empty tab list and negative direction for reverse cycling. My suggestions focus on improving UI consistency for the new keybinding's help text.
| ), | ||
| KeyShiftTab: key.NewBinding( | ||
| key.WithKeys("shift+tab"), | ||
| key.WithHelp("shift+tab", "switch tab (reverse)"), |
There was a problem hiding this comment.
For UI consistency in the help menu, consider using the tab symbol ⇥ for the key display. The application uses symbols for other special keys like arrows (↑, ↓, ←, →) and enter (↵), and using a symbol for tab would align with this convention.
| key.WithHelp("shift+tab", "switch tab (reverse)"), | |
| key.WithHelp("shift+⇥", "switch tab (reverse)"), |
| // Actions | ||
| {Command: "enter", Keys: []string{"enter", "o"}, Help: "↵/o"}, | ||
| {Command: "tab", Keys: []string{"tab"}, Help: "tab"}, | ||
| {Command: "shift_tab", Keys: []string{"shift+tab"}, Help: "shift+tab"}, |
There was a problem hiding this comment.
For UI consistency in the keybinding editor, consider using the tab symbol ⇥ for the help text. This would align with the use of symbols for other special keys like arrows and enter in the keybinding configuration.
| {Command: "shift_tab", Keys: []string{"shift+tab"}, Help: "shift+tab"}, | |
| {Command: "shift_tab", Keys: []string{"shift+tab"}, Help: "shift+⇥"}, |
austinamorusoyardstick
force-pushed
the
austinamoruso/shifttab
branch
from
4ed1c2e to
10db04b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to switch tabs in reverse using Shift+Tab. The implementation is well-done, with a good refactoring in app/app.go to handle both forward and reverse tab switching, and a robust implementation in ui/tabbed_window.go that also prevents a potential panic when no tabs are present. I've added one comment regarding the visibility of the new keybinding in the UI.
| KeyPRReview | ||
|
|
||
| KeyTab // Tab is a special keybinding for switching between panes. | ||
| KeyShiftTab // ShiftTab is a special keybinding for switching between panes in reverse. |
There was a problem hiding this comment.
While the keybinding is implemented correctly, it's not visible in the help menu, which makes it undiscoverable to users. Please add keys.KeyShiftTab to the menu options in ui/menu.go to ensure users are aware of this new feature.
1 participant
This pull request introduces a new feature that allows users to switch between tabs in reverse order using the
Shift+Tabkeybinding. The changes include updates to keybinding configurations, help text, and the implementation of the reverse tab-switching functionality.Keybinding Updates:
KeyShiftTab, for reverse tab switching inkeys/keys.go. This includes updates toGlobalKeyStringsMap,GlobalkeyBindings,DefaultKeyBindings,getCommandToKeyNameMap, andgetHelpText. ([[1]](https://github.com/austinamorusoyardstick/claude-squad/pull/36/files#diff-afc783802cd8fd9fdcbfcd151232a4edeea3c7e7620b1633239feb287d9ee3fbR26),[[2]](https://github.com/austinamorusoyardstick/claude-squad/pull/36/files#diff-afc783802cd8fd9fdcbfcd151232a4edeea3c7e7620b1633239feb287d9ee3fbR99),[[3]](https://github.com/austinamorusoyardstick/claude-squad/pull/36/files#diff-afc783802cd8fd9fdcbfcd151232a4edeea3c7e7620b1633239feb287d9ee3fbR228-R231),[[4]](https://github.com/austinamorusoyardstick/claude-squad/pull/36/files#diff-afc783802cd8fd9fdcbfcd151232a4edeea3c7e7620b1633239feb287d9ee3fbR373),[[5]](https://github.com/austinamorusoyardstick/claude-squad/pull/36/files#diff-afc783802cd8fd9fdcbfcd151232a4edeea3c7e7620b1633239feb287d9ee3fbR524),[[6]](https://github.com/austinamorusoyardstick/claude-squad/pull/36/files#diff-afc783802cd8fd9fdcbfcd151232a4edeea3c7e7620b1633239feb287d9ee3fbR582))Reverse Tab-Switching Implementation:
ToggleReversemethod in theTabbedWindowclass to cycle through tabs in reverse order. ([ui/tabbed_window.goR113-R117](https://github.com/austinamorusoyardstick/claude-squad/pull/36/files#diff-6252955ebc5793fedbe5f4277cf22867afa4025846142ff6fe1d65377c711860R113-R117))ToggleReversemethod into thehandleKeyPressfunction inapp/app.goto handle theShift+Tabkeybinding. ([app/app.goR888-R891](https://github.com/austinamorusoyardstick/claude-squad/pull/36/files#diff-0f1d2976054440336a576d47a44a37b80cdf6701dd9113012bce0e3c425819b7R888-R891))