Skip to content

Conversation

@bergjaak
Copy link
Contributor

@bergjaak bergjaak commented Apr 10, 2025

Here is a skim integration into Q CLI. This will make it a lot more efficient to use slash commands, including adding and removing files from the /context.

Here is a video of how this looks in action:

light_mode_skim.mov
  • Add Ctrl+K keybinding to open skim (rust version of fzf) command selector
  • Implement file selection for context add commands
  • Implement context file selection for context rm commands

🤖 Assisted by Amazon Q Developer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

- Add Ctrl+K keybinding to open fzf command selector
- Implement file selection for context add commands
- Implement context file selection for context rm commands

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
@bergjaak bergjaak requested a review from a team April 10, 2025 23:17
@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 23.52941% with 169 lines in your changes missing coverage. Please review.

Project coverage is 13.97%. Comparing base (06c2e21) to head (5390f9f).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/q_cli/src/cli/chat/skim_integration.rs 22.11% 162 Missing ⚠️
crates/q_cli/src/cli/chat/input_source.rs 25.00% 4 Missing and 2 partials ⚠️
crates/q_cli/src/cli/chat/mod.rs 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1181      +/-   ##
==========================================
+ Coverage   13.96%   13.97%   +0.01%     
==========================================
  Files        2365     2366       +1     
  Lines      204869   205089     +220     
  Branches   185233   185453     +220     
==========================================
+ Hits        28618    28670      +52     
- Misses     174846   175010     +164     
- Partials     1405     1409       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bergjaak
Copy link
Contributor Author

I will replace the list of hardcoded strings with something (vibecoded btw)

I will also replace fzf with a Skim dependency

bergjaak added 22 commits April 11, 2025 01:16
Replace the fzf integration with skim for command selection in the Amazon Q CLI.
This change:
- Adds skim as a dependency
- Creates a new skim_integration module to replace fzf_integration
- Updates the input_source to use skim instead of fzf
- Maintains the same functionality and UI experience

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Remove the fzf_integration.rs file as it has been completely replaced by the skim_integration.rs module.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Update the integration plan document to reflect the completed implementation using skim instead of fzf.
The document now serves as both a plan and implementation record.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Fix issue where skim's output persists on the screen after command selection.
Added no_clear(false) option to all skim instances to ensure the terminal
is properly cleared after selection.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Add crossterm dependency and implement a clear_screen_after_skim function
to properly clear the terminal after skim selection. This fixes the issue
where skim's output persists on the screen after command selection.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Implement a solution that uses crossterm's alternate screen functionality
to ensure skim's output doesn't persist in the terminal scrollback buffer.
This completely removes the skim interface from terminal history after
selection, providing a cleaner user experience.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
…im_integration

Simplified the CommandInfo struct by removing the unused description field and updated related functions to work with just the command field.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Since we removed the description field from CommandInfo, we can now directly use the output string without splitting it to extract just the command part.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Created helper functions to centralize skim execution and screen management:
- Added run_skim_with_options to handle alternate screen and skim execution
- Added extract_selections to standardize parsing skim results
- Removed redundant enter_alternate_screen and leave_alternate_screen functions
- Fixed lifetime parameter warning in SkimOptions

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
…ch COMMANDS array

This test ensures that all hardcoded command strings used in the select_command
function are properly defined in the COMMANDS array from prompt.rs, preventing
potential inconsistencies between the two.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
}

/// Get all available commands with their descriptions
pub fn get_available_commands() -> Vec<CommandInfo> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo. another branch is adding a CommandManager, that will help here. Do you want a get_available_commands() call that returns command + description?

(Non-blocking - as an FYI for the reviewers)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be great!

This change makes the tool selection in skim_integration.rs more maintainable by
loading tool names directly from the tool_index.json file instead of hardcoding
them. Added a fallback to hardcoded list in case the file can't be loaded.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
.prompt(Some(prompt))
.reverse(true)
.multi(multi)
.color(Some("fg:252,bg:234,hl:67,fg+:252,bg+:235,hl+:81"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out cli/theme.rs for color theme class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, not sure how to do this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - we probably shouldn't be hardcoding colors as this will change depending on the user's terminal theme.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-04-11 at 5 19 35 PM

This is how it looks on light mode - I'd call this blocking. Prefer if we can at least get everything printed as dark mode

Btw cli/theme.rs seems unrelated for this task, that's more tied to the autocomplete feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to just remove the color options and now it defaults to the color theme of the user's terminal (or whatever the correct terminology is there, not sure). As evidence, please see the updated video that I posted in the PR description as evidence

FigTool::ToolSpecification(t) => self.tool_permissions.is_trusted(&t.name),
});

// Update the context manager in the input source for skim integration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't have this here, this should really be occurring when we create the InputSource

match skim_integration::select_command(self.context_manager.as_deref()) {
Ok(Some(command)) => {
// Return a command to replace the current line with the selected command
Some(Cmd::Replace(Movement::WholeBuffer, Some(command)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to be sure to set the cursor after the select command content, otherwise you have to ctrl+e to go to the end of the line and type the args

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also couldn't figure this out... I changed it to insert but that seems to only have the correct behavior for vi mode. And it doesn't seem possible to chain 2 commands together (but I could be wrong)

.prompt(Some(prompt))
.reverse(true)
.multi(multi)
.color(Some("fg:252,bg:234,hl:67,fg+:252,bg+:235,hl+:81"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-04-11 at 5 19 35 PM

This is how it looks on light mode - I'd call this blocking. Prefer if we can at least get everything printed as dark mode

Btw cli/theme.rs seems unrelated for this task, that's more tied to the autocomplete feature.

Profile(&'static str),
}

impl CommandType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess it's fine for now

@bergjaak bergjaak requested a review from brandonskiser April 14, 2025 16:00
@GoodluckH GoodluckH merged commit 51f8a2f into aws:main Apr 14, 2025
10 checks passed
@bergjaak bergjaak deleted the feature/fzf-integration branch April 14, 2025 19:01
jsamuel1 pushed a commit that referenced this pull request Apr 15, 2025
#1181)

* feat: Add fzf integration for command selection and context management

- Add Ctrl+K keybinding to open fzf command selector
- Implement file selection for context add commands
- Implement context file selection for context rm commands

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* feat(cli): Replace fzf with skim for command selection

Replace the fzf integration with skim for command selection in the Amazon Q CLI.
This change:
- Adds skim as a dependency
- Creates a new skim_integration module to replace fzf_integration
- Updates the input_source to use skim instead of fzf
- Maintains the same functionality and UI experience

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* refactor(cli): Remove fzf_integration.rs file

Remove the fzf_integration.rs file as it has been completely replaced by the skim_integration.rs module.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* chore: commit lock

* docs: Update integration plan to reflect skim implementation

Update the integration plan document to reflect the completed implementation using skim instead of fzf.
The document now serves as both a plan and implementation record.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* fix(cli): Clear skim output after command selection

Fix issue where skim's output persists on the screen after command selection.
Added no_clear(false) option to all skim instances to ensure the terminal
is properly cleared after selection.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* fix(cli): Use crossterm to clear screen after skim selection

Add crossterm dependency and implement a clear_screen_after_skim function
to properly clear the terminal after skim selection. This fixes the issue
where skim's output persists on the screen after command selection.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* fix(cli): Use alternate screen for skim to prevent output persistence

Implement a solution that uses crossterm's alternate screen functionality
to ensure skim's output doesn't persist in the terminal scrollback buffer.
This completely removes the skim interface from terminal history after
selection, providing a cleaner user experience.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* chore: vibe coding

* chore: vibe coding

* chore: vibe coding

* chore: vibe coding

* chore: remove termion

* chore: remove useless tests

* refactor(cli): Remove unused description field from CommandInfo in skim_integration

Simplified the CommandInfo struct by removing the unused description field and updated related functions to work with just the command field.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* chore: remove useless description

* refactor(cli): Simplify skim output parsing in launch_skim_selector

Since we removed the description field from CommandInfo, we can now directly use the output string without splitting it to extract just the command part.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* chore: remove integration-plan

* refactor(cli): Reduce duplication in skim_integration.rs

Created helper functions to centralize skim execution and screen management:
- Added run_skim_with_options to handle alternate screen and skim execution
- Added extract_selections to standardize parsing skim results
- Removed redundant enter_alternate_screen and leave_alternate_screen functions
- Fixed lifetime parameter warning in SkimOptions

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* fix: dont touch that file

* fix: dont touch that file

* test: Add unit test to verify command strings in skim_integration match COMMANDS array

This test ensures that all hardcoded command strings used in the select_command
function are properly defined in the COMMANDS array from prompt.rs, preventing
potential inconsistencies between the two.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* fix: dont repeat stuff

* feat(skim): Load tool names from tool_index.json instead of hardcoding

This change makes the tool selection in skim_integration.rs more maintainable by
loading tool names directly from the tool_index.json file instead of hardcoding
them. Added a fallback to hardcoded list in case the file can't be loaded.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* fix: dont hardcode tool names

* feat(skim): Load tool names from load_tools function

This change makes the tool selection in skim_integration.rs more maintainable by
reusing the existing load_tools function from the chat module instead of
implementing a separate function to parse the tool_index.json file.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* chore: make load_tools pub

* refactor(skim): Create reusable functions for skim options

This change improves code maintainability by:
1. Adding a create_skim_options function for consistent styling
2. Adding run_skim_with_options to handle alternate screen management
3. Adding extract_selections to standardize result processing
4. Updating all skim calls to use these helper functions

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* fix(skim): Use tool name field instead of key for tool selection

This change ensures that the tool selection dialog shows the correct tool names
by extracting the 'name' field from each tool spec instead of using the HashMap
keys. This matches the expected behavior when trusting/untrusting tools.

🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)

* chore: lint

* linter

* fix: pipe the find input so that program doesn't crash if there are too many files

* chore: make skim take up full screen

* feat: context manager integration

* feat: context manager integration that works

* chore: lints

* chore: lints

* chore: lints

* chore: lints

* fix: make light mode usable

* chore: cleaning up

* chore: cleaning up

* chore: cleaning up

* chore: add instructions

* chore: cleaning up

* fix: rust fmt
<em>/quit</em> <black!>Quit the application</black!>
<cyan!>Use Ctrl(^) + j to provide multi-line prompts.</cyan!>
<cyan!>Use Ctrl(^) + k to fuzzily search commands and context (use tab to select multiple files).</cyan!>
Copy link

@ANorwell ANorwell Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overriding the existing rustyline (and every other shell environment default) binding for ctrl-K of kill to end of line. I think another binding is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants