Skip to content

feat: add external provider to run Claude in separate terminal #102

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

Merged
merged 10 commits into from
Aug 8, 2025

Conversation

marcinjahn
Copy link
Contributor

I think it's pretty convenient to have Claude running in a separate window, separate from Neovim window. I think this is particularly useful on tiling window managers.

Screencast.From.2025-08-02.23-43-19.mp4

I think it's pretty convenient to have Claude running in a separate
window, separate from Neovim window. I think this is particularly useful on
tiling window managers.
Copy link
Member

@ThomasK33 ThomasK33 left a comment

Choose a reason for hiding this comment

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

That sounds like a good idea.

Could you please rebase onto main and address the review comments below?

marcinjahn and others added 6 commits August 5, 2025 19:28
Co-authored-by: Thomas Kosiewski <thoma471@googlemail.com>
Co-authored-by: Thomas Kosiewski <thoma471@googlemail.com>
Co-authored-by: Thomas Kosiewski <thoma471@googlemail.com>
@marcinjahn marcinjahn requested a review from ThomasK33 August 5, 2025 18:06
Copy link
Member

@ThomasK33 ThomasK33 left a comment

Choose a reason for hiding this comment

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

LGTM. Just a minor nit about the default in config.lua; once that's addressed, it's good to merge.

Co-authored-by: Thomas Kosiewski <thoma471@googlemail.com>
@marcinjahn
Copy link
Contributor Author

I had trouble running the tests locally, the scripts are very nix-oriented

@ThomasK33
Copy link
Member

I had trouble running the tests locally, the scripts are very nix-oriented

True. I've added a devcontainer configuration in #112 to make the setup easier.

You can launch it in a devcontainer, and then Nix should be preinstalled.

@arjunmahishi
Copy link

arjunmahishi commented Aug 8, 2025

I had trouble running the tests locally, the scripts are very nix-oriented

BTW, I was able to run the tests locally with these temporary changes to the Makefile. Of course, you'd have to install busted

diff --git a/Makefile b/Makefile
index 2c2b329..1ecb30b 100644
--- a/Makefile
+++ b/Makefile
@@ -5,7 +5,7 @@ all: format check test
 
 # Detect if we are already inside a Nix shell
 ifeq (,$(IN_NIX_SHELL))
-NIX_PREFIX := nix develop .#ci -c
+NIX_PREFIX := 
 else
 NIX_PREFIX :=
 endif
@@ -29,7 +29,7 @@ test:
 	echo "Found test files:"; \
 	echo "$$TEST_FILES"; \
 	if [ -n "$$TEST_FILES" ]; then \
-		$(NIX_PREFIX) busted --coverage -v $$TEST_FILES; \
+		$(NIX_PREFIX) busted -v $$TEST_FILES; \
 	else \
 		echo "No test files found"; \
 	fi

@fxrlv
Copy link

fxrlv commented Aug 8, 2025

out of curiosity
is it somehow related to #50?

@ThomasK33
Copy link
Member

BTW, I was able to run the tests locally with these temporary changes to the Makefile. Of course, you'd have to install busted

Cool. Please feel free to modify these to accommodate your local setup and preferences; however, we won't accept any changes that move away from Nix.

out of curiosity
is it somehow related to #50?

No.

@marcinjahn
Copy link
Contributor Author

I had trouble running the tests locally, the scripts are very nix-oriented

True. I've added a devcontainer configuration in #112 to make the setup easier.

You can launch it in a devcontainer, and then Nix should be preinstalled.

Nice, devcontainer is exactly what I needed. Pushed the fixes.

@ThomasK33 ThomasK33 added this pull request to the merge queue Aug 8, 2025
Merged via the queue into coder:main with commit 985b4b1 Aug 8, 2025
3 checks passed
@ThomasK33
Copy link
Member

Sweet, thanks @marcinjahn 👍

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.

4 participants