Skip to content

Conversation

@samuelstranges
Copy link

Adds integration with external editor via the $EDITOR environment variable to create and edit flashcards

- Implement external editor integration for card creation/editing
- Add editor configuration support via $EDITOR env variable
- Create temporary markdown files to prevent invalid cards being added
- Update README with example usage
@DavidMiserak DavidMiserak requested a review from Copilot August 4, 2025 12:44
@DavidMiserak DavidMiserak added the enhancement New feature or request label Aug 4, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds editor integration functionality to enable users to create and edit flashcards using their preferred external editor via the $EDITOR environment variable.

  • Adds keyboard shortcuts 'a' and 'e' for adding and editing cards during study sessions
  • Implements editor process launching and response handling with temporary file management
  • Extends the data store with functionality to add new cards to decks

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
internal/data/editor.go New module implementing editor process launching, temporary file creation, and card template generation
internal/data/editor_test.go Comprehensive test suite for editor functionality including shell editor detection and temporary file operations
internal/data/store.go Adds AddCardToDeck method to support adding new cards to existing decks
internal/ui/study_screen.go Integrates editor functionality with new keybindings and response handling during study sessions
internal/ui/study_screen_test.go Test cases for editor response handling in study screen context
README.md Updates keyboard shortcuts documentation to include new 'a' and 'e' commands

Comment on lines +373 to +377
t.Fatal("Couldn't create temp file")
}
defer os.Remove(tempFile.Name())
if _, err := tempFile.WriteString(mockValidCardContent); err != nil {
t.Fatalf("Couldnt write to temp file")
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

The error message contains a spelling error: "Couldn't" should be "Couldn't" or "Could not".

Suggested change
t.Fatal("Couldn't create temp file")
}
defer os.Remove(tempFile.Name())
if _, err := tempFile.WriteString(mockValidCardContent); err != nil {
t.Fatalf("Couldnt write to temp file")
t.Fatal("Could not create temp file")
}
defer os.Remove(tempFile.Name())
if _, err := tempFile.WriteString(mockValidCardContent); err != nil {
t.Fatalf("Could not write to temp file")

Copilot uses AI. Check for mistakes.
Copy link
Owner

Choose a reason for hiding this comment

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

(Optional)

You can setup pre-commits that will catch spelling errors before you commit: make pre-commit-setup.

Except from Makefile:

.PHONY: pre-commit-setup
pre-commit-setup:
        @echo "Setting up pre-commit hooks..."
        @echo "consider running <pre-commit autoupdate> to get the latest versions"
        pre-commit install
        pre-commit install --install-hooks
        pre-commit run --all-files

}
defer os.Remove(tempFile.Name())
if _, err := tempFile.WriteString(mockValidCardContent); err != nil {
t.Fatalf("Couldnt write to temp file")
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

The error message contains a spelling error: "Couldnt" should be "Couldn't" or "Could not".

Suggested change
t.Fatalf("Couldnt write to temp file")
t.Fatalf("Couldn't write to temp file")

Copilot uses AI. Check for mistakes.
Copy link
Owner

Choose a reason for hiding this comment

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

(Optional)

You can setup pre-commits that will catch spelling errors before you commit: make pre-commit-setup.

Except from Makefile:

.PHONY: pre-commit-setup
pre-commit-setup:
        @echo "Setting up pre-commit hooks..."
        @echo "consider running <pre-commit autoupdate> to get the latest versions"
        pre-commit install
        pre-commit install --install-hooks
        pre-commit run --all-files

// Create Fake File
tempDir, err := os.MkdirTemp("", "card-test")
if err != nil {
t.Fatalf("Cant cretae temp dir: %v", err)
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

The error message contains spelling errors: "Cant" should be "Can't" or "Cannot", and "cretae" should be "create".

Copilot uses AI. Check for mistakes.
case data.EditorResponse:
err := s.handleEditorResponse(msg)
if err != nil {
fmt.Printf("editor error: %v", err)
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

Error handling uses fmt.Printf which outputs to stdout and doesn't provide feedback to the user in the TUI. Consider using a proper error display mechanism within the UI framework.

Copilot uses AI. Check for mistakes.
}

func (s *StudyScreen) handleEditorResponse(msg data.EditorResponse) error {
defer os.Remove(msg.FileName) // All cases should cleanup temp file
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

The defer statement will always execute, even when there are early returns due to errors. This could lead to attempting to remove a file that may not exist or may have already been removed, potentially causing silent failures.

Suggested change
defer os.Remove(msg.FileName) // All cases should cleanup temp file
defer func() {
if err := os.Remove(msg.FileName); err != nil {
fmt.Printf("failed to remove temp file %s: %v\n", msg.FileName, err)
}
}() // All cases should cleanup temp file

Copilot uses AI. Check for mistakes.
editor, err := getShellEditor()

if err != nil {
return nil // can't launch
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

When getShellEditor fails, LaunchEditor returns nil without any error indication to the caller, making it difficult to distinguish between successful completion and failure.

Suggested change
return nil // can't launch
return func() tea.Msg {
return EditorResponse{
FileName: file,
ExitCode: err,
IsEdit: isEdit,
CardID: cardID,
}
}

Copilot uses AI. Check for mistakes.
@DavidMiserak DavidMiserak self-requested a review August 4, 2025 12:47
@DavidMiserak
Copy link
Owner

Is is possible to get a demo via AsciiNema?

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants