Austinamoruso/testpanel #30
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 user interface by integrating a new, interactive Jest testing tab. This new feature allows users to initiate Jest test runs directly from the UI, view real-time test output, and quickly navigate to and open failed test files in their configured IDE. Concurrently, the PR refines the application's keybinding system to support Jest-specific actions and cleans up outdated configuration documentation, improving overall usability and maintainability.
Highlights
- New Jest Testing UI: A dedicated Jest tab has been added to the application's UI, allowing users to run tests, view live output, and interact with test results.
- Enhanced Test Interaction: New keybindings (
nfor next failure,pfor previous failure,enterto open in IDE, andrto rerun tests) are introduced, providing direct control over Jest test workflows within the new tab. - Streamlined Test Execution: The underlying Jest test command has been simplified, and test execution is now managed directly by the new Jest pane, providing live output streaming and automatic opening of failed test files in the IDE.
- Documentation Cleanup: Outdated configuration documentation related to IDE and diff tool commands has been removed from
CLAUDE.md, improving the clarity and accuracy of the project's setup instructions.
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 a new Jest testing tab, allowing users to run tests and see live output. The changes include a new JestPane UI component, keybindings for test execution, and integration into the main application loop. My review focuses on improving correctness, maintainability, and robustness of the new testing functionality. Key feedback includes fixing a critical bug in the test command, addressing missing functionality for test navigation, removing dead code, and improving code quality by reducing duplication and eliminating magic numbers.
| outputChan <- string(testOutput) | ||
|
|
||
| // Run Jest without JSON for live output | ||
| cmd := exec.Command("yarn", "tester") |
There was a problem hiding this comment.
The command yarn tester is hardcoded here, but there is no tester script in web/package.json. The test script is jest. This will cause the test run to fail. It seems like it should be yarn test instead. Using the correct script ensures that the tests are executed as intended.
| cmd := exec.Command("yarn", "tester") | |
| cmd := exec.Command("yarn", "test") |
app/app.go
Outdated
| for !m.tabbedWindow.IsInJestTab() { | ||
| m.tabbedWindow.Toggle() | ||
| } |
There was a problem hiding this comment.
This for loop to switch to the Jest tab by repeatedly calling Toggle() is inefficient and brittle. If the order or number of tabs changes, this could break or become even less efficient.
It would be better to add a method to TabbedWindow to switch directly to a specific tab, for example SetTab(tabID int). A more direct approach would enhance the code's robustness and maintainability.
ui/jest.go
Outdated
| func (j *JestPane) parseJestJSON(state *JestInstanceState, data []byte) { | ||
| var result struct { | ||
| TestResults []struct { | ||
| Name string `json:"name"` | ||
| Status string `json:"status"` | ||
| AssertionResults []struct { | ||
| Title string `json:"title"` | ||
| Status string `json:"status"` | ||
| FailureMessages []string `json:"failureMessages"` | ||
| Location struct { | ||
| Line int `json:"line"` | ||
| } `json:"location"` | ||
| } `json:"assertionResults"` | ||
| } `json:"testResults"` | ||
| } | ||
|
|
||
| if err := json.Unmarshal(data, &result); err != nil { | ||
| return | ||
| } | ||
|
|
||
| state.testResults = []TestResult{} | ||
| state.failedFiles = []string{} | ||
| failedSet := make(map[string]bool) | ||
|
|
||
| for _, testFile := range result.TestResults { | ||
| for _, assertion := range testFile.AssertionResults { | ||
| tr := TestResult{ | ||
| FilePath: testFile.Name, | ||
| TestName: assertion.Title, | ||
| Status: assertion.Status, | ||
| Line: assertion.Location.Line, | ||
| } | ||
|
|
||
| if assertion.Status == "failed" && len(assertion.FailureMessages) > 0 { | ||
| tr.ErrorOutput = strings.Join(assertion.FailureMessages, "\n") | ||
| if !failedSet[testFile.Name] { | ||
| state.failedFiles = append(state.failedFiles, testFile.Name) | ||
| failedSet[testFile.Name] = true | ||
| } | ||
| } | ||
|
|
||
| state.testResults = append(state.testResults, tr) | ||
| } | ||
| } | ||
|
|
||
| if len(state.failedFiles) > 0 && state.currentIndex == -1 { | ||
| state.currentIndex = j.findFirstFailureIndex(state) | ||
| } | ||
| } | ||
|
|
||
| func (j *JestPane) parseJestOutput(state *JestInstanceState, output string) { | ||
| // Fallback parser for non-JSON output | ||
| lines := strings.Split(output, "\n") | ||
| currentFile := "" | ||
| fileRegex := regexp.MustCompile(`(?:PASS|FAIL)\s+(.+\.(?:js|jsx|ts|tsx))`) | ||
| testRegex := regexp.MustCompile(`\s*([✓✗])\s+(.+)`) | ||
|
|
||
| state.testResults = []TestResult{} | ||
| state.failedFiles = []string{} | ||
| failedSet := make(map[string]bool) | ||
|
|
||
| for _, line := range lines { | ||
| if matches := fileRegex.FindStringSubmatch(line); matches != nil { | ||
| currentFile = matches[1] | ||
| } else if matches := testRegex.FindStringSubmatch(line); matches != nil && currentFile != "" { | ||
| status := "passed" | ||
| if matches[1] == "✗" { | ||
| status = "failed" | ||
| if !failedSet[currentFile] { | ||
| state.failedFiles = append(state.failedFiles, currentFile) | ||
| failedSet[currentFile] = true | ||
| } | ||
| } | ||
|
|
||
| state.testResults = append(state.testResults, TestResult{ | ||
| FilePath: currentFile, | ||
| TestName: matches[2], | ||
| Status: status, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| if len(state.failedFiles) > 0 && state.currentIndex == -1 { | ||
| state.currentIndex = j.findFirstFailureIndex(state) | ||
| } | ||
| } |
There was a problem hiding this comment.
The functions parseJestJSON and parseJestOutput appear to be dead code. They are not called from anywhere in the new test execution flow, which relies on real-time parsing of the raw output stream in runJestWithStream. If they are not intended to be used, they should be removed to keep the codebase clean. Removing dead code reduces complexity and improves the overall quality of the codebase.
ui/jest.go
Outdated
| func (j *JestPane) NextFailure() { | ||
| // Navigation disabled when showing raw output | ||
| return | ||
| } | ||
|
|
||
| func (j *JestPane) PreviousFailure() { | ||
| // Navigation disabled when showing raw output | ||
| return | ||
| } |
There was a problem hiding this comment.
The functions NextFailure and PreviousFailure are currently empty stubs. The PR description mentions functionality for navigating test failures, and keybindings for this (n and p) have been added. This seems like a core piece of functionality is missing. The comment "Navigation disabled when showing raw output" suggests this is intentional for now, but it's a significant feature gap that should be addressed. Implementing these navigation features would greatly enhance the user experience.
ui/jest.go
Outdated
| func getIDECommand(workingDir string) string { | ||
| // Check CLAUDE.md configuration in the working directory | ||
| claudeMdPath := filepath.Join(workingDir, "CLAUDE.md") | ||
| if data, err := os.ReadFile(claudeMdPath); err == nil { | ||
| scanner := bufio.NewScanner(strings.NewReader(string(data))) | ||
| inConfig := false | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if strings.TrimSpace(line) == "[claude-squad]" { | ||
| inConfig = true | ||
| continue | ||
| } | ||
| if inConfig && strings.HasPrefix(line, "ide_command:") { | ||
| return strings.TrimSpace(strings.TrimPrefix(line, "ide_command:")) | ||
| } | ||
| if inConfig && strings.TrimSpace(line) == "" { | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check .claude-squad/config.json in the working directory | ||
| localConfigPath := filepath.Join(workingDir, ".claude-squad", "config.json") | ||
| if data, err := os.ReadFile(localConfigPath); err == nil { | ||
| var config map[string]string | ||
| if err := json.Unmarshal(data, &config); err == nil { | ||
| if cmd, ok := config["ide_command"]; ok { | ||
| return cmd | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check global config | ||
| homeDir, _ := os.UserHomeDir() | ||
| globalConfigPath := filepath.Join(homeDir, ".claude-squad", "config.json") | ||
| if data, err := os.ReadFile(globalConfigPath); err == nil { | ||
| var config map[string]string | ||
| if err := json.Unmarshal(data, &config); err == nil { | ||
| if cmd, ok := config["default_ide_command"]; ok { | ||
| return cmd | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Default to code | ||
| return "code" | ||
| } |
There was a problem hiding this comment.
This getIDECommand function duplicates logic that already exists in config.GetEffectiveIdeCommand. To improve maintainability and avoid code duplication, this function should be removed and config.GetEffectiveIdeCommand should be used instead. You'll need to pass the appConfig to the JestPane or relevant methods. Reusing existing logic promotes consistency and reduces the risk of errors.
ui/jest.go
Outdated
| outputChan <- fmt.Sprintf("Running command: yarn tester") | ||
| outputChan <- fmt.Sprintf("Working directory: %s", workDir) | ||
| outputChan <- fmt.Sprintf("Instance worktree path: %s", instance.Path) | ||
| outputChan <- "Starting test run...\n" |
There was a problem hiding this comment.
These lines send debugging information directly to the UI via the outputChan. This information is useful for debugging but should not be part of the user-facing output. Consider using the application's logger (log.InfoLog) for this kind of information instead. Using the logger ensures that debugging information is separate from the user interface.
| go func() { | ||
| defer wg.Done() | ||
| scanner := bufio.NewScanner(stdout) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| outputChan <- line | ||
| allOutput.WriteString(line + "\n") | ||
|
|
||
| // Look for test failures in real-time | ||
| if strings.HasPrefix(strings.TrimSpace(line), "FAIL ") { | ||
| // Extract file path from FAIL line | ||
| // Format: "FAIL src/pages/individualDashboard/component.test.js" | ||
| trimmedLine := strings.TrimSpace(line) | ||
| if len(trimmedLine) > 5 { // "FAIL " is 5 characters | ||
| filePath := strings.TrimSpace(trimmedLine[5:]) | ||
| // Remove any trailing whitespace or test duration info | ||
| if idx := strings.IndexAny(filePath, " \t("); idx > 0 { | ||
| filePath = filePath[:idx] | ||
| } | ||
|
|
||
| // Check if it's a valid test file | ||
| if strings.HasSuffix(filePath, ".js") || strings.HasSuffix(filePath, ".jsx") || | ||
| strings.HasSuffix(filePath, ".ts") || strings.HasSuffix(filePath, ".tsx") || | ||
| strings.HasSuffix(filePath, ".test.js") || strings.HasSuffix(filePath, ".spec.js") { | ||
|
|
||
| absPath := filePath | ||
| if !filepath.IsAbs(filePath) { | ||
| absPath = filepath.Join(workDir, filePath) | ||
| } | ||
|
|
||
| // Avoid duplicates | ||
| alreadyAdded := false | ||
| for _, existing := range failedFiles { | ||
| if existing == absPath { | ||
| alreadyAdded = true | ||
| break | ||
| } | ||
| } | ||
| if !alreadyAdded { | ||
| failedFiles = append(failedFiles, absPath) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if err := scanner.Err(); err != nil { | ||
| outputChan <- fmt.Sprintf("Error reading stdout: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| // Read stderr in goroutine | ||
| go func() { | ||
| defer wg.Done() | ||
| scanner := bufio.NewScanner(stderr) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| outputChan <- line | ||
| allOutput.WriteString(line + "\n") | ||
|
|
||
| // Also check stderr for FAIL lines (Jest might output to stderr) | ||
| if strings.HasPrefix(strings.TrimSpace(line), "FAIL ") { | ||
| // Extract file path from FAIL line | ||
| // Format: "FAIL src/pages/individualDashboard/component.test.js" | ||
| trimmedLine := strings.TrimSpace(line) | ||
| if len(trimmedLine) > 5 { // "FAIL " is 5 characters | ||
| filePath := strings.TrimSpace(trimmedLine[5:]) | ||
| // Remove any trailing whitespace or test duration info | ||
| if idx := strings.IndexAny(filePath, " \t("); idx > 0 { | ||
| filePath = filePath[:idx] | ||
| } | ||
|
|
||
| // Check if it's a valid test file | ||
| if strings.HasSuffix(filePath, ".js") || strings.HasSuffix(filePath, ".jsx") || | ||
| strings.HasSuffix(filePath, ".ts") || strings.HasSuffix(filePath, ".tsx") || | ||
| strings.HasSuffix(filePath, ".test.js") || strings.HasSuffix(filePath, ".spec.js") { | ||
|
|
||
| absPath := filePath | ||
| if !filepath.IsAbs(filePath) { | ||
| absPath = filepath.Join(workDir, filePath) | ||
| } | ||
|
|
||
| // Avoid duplicates | ||
| alreadyAdded := false | ||
| for _, existing := range failedFiles { | ||
| if existing == absPath { | ||
| alreadyAdded = true | ||
| break | ||
| } | ||
| } | ||
| if !alreadyAdded { | ||
| failedFiles = append(failedFiles, absPath) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if err := scanner.Err(); err != nil { | ||
| outputChan <- fmt.Sprintf("Error reading stderr: %v", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The logic to parse FAIL lines and extract file paths is duplicated in the goroutines for reading stdout and stderr. This could be refactored into a helper function to reduce code duplication and improve maintainability. The helper function could take a line string and return the filePath if it's a failure line, or an empty string otherwise. Refactoring this logic into a helper function promotes code reuse and simplifies maintenance.
ui/jest.go
Outdated
| _ = filepath.Walk(searchDir, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return nil // Continue walking | ||
| } | ||
| if info.Name() == "package.json" { | ||
| foundDir = filepath.Dir(path) | ||
| return io.EOF // Stop walking on first package.json found | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
The error returned by filepath.Walk is ignored here (_ = filepath.Walk(...)). Also, inside the walk function, if an error occurs, it's ignored and the walk continues (return nil). This could hide filesystem issues or other problems during the search for package.json. The error should be handled or at least logged. Proper error handling ensures that potential issues are identified and addressed.
ui/tabbed_window.go
Outdated
|
|
||
| // IsInJestTab returns true if the Jest tab is currently active | ||
| func (w *TabbedWindow) IsInJestTab() bool { | ||
| return w.activeTab == 3 |
There was a problem hiding this comment.
A magic number 3 is used here to check for the Jest tab. The constant JestTab is defined for this purpose and should be used instead to improve readability and maintainability. Using named constants instead of magic numbers makes the code easier to understand and modify.
| return w.activeTab == 3 | |
| return w.activeTab == JestTab |
ui/tabbed_window.go
Outdated
| case 3: | ||
| content = w.jest.String() |
There was a problem hiding this comment.
A magic number 3 is used here in the switch statement. The constant JestTab should be used instead to avoid magic numbers and improve code clarity. Replacing magic numbers with named constants improves code readability and reduces the likelihood of errors.
| case 3: | |
| content = w.jest.String() | |
| case JestTab: | |
| content = w.jest.String() |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Jest testing tab, which is a great addition to the application's capabilities. The refactoring of the test execution logic into the ui package is a good design choice that improves modularity.
I've found a few critical issues in the new ui/jest.go file related to the test execution command and race conditions that need to be addressed. I've also included a high-severity comment about ensuring the UI correctly reflects test failure states. The other changes in app/app.go, keys/keys.go, and ui/tabbed_window.go look solid and improve the codebase, especially with the removal of magic numbers.
Once these issues are resolved, this will be a strong contribution.
| var allOutput strings.Builder | ||
| failedFiles := []string{} | ||
|
|
||
| // Create wait group for both readers | ||
| var wg sync.WaitGroup | ||
| wg.Add(2) | ||
|
|
||
| // Read stdout in goroutine | ||
| go func() { | ||
| defer wg.Done() | ||
| scanner := bufio.NewScanner(stdout) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| outputChan <- line | ||
| allOutput.WriteString(line + "\n") | ||
|
|
||
| // Look for test failures in real-time | ||
| if failedFile := parseFailedTestFile(line, workDir); failedFile != "" { | ||
| // Avoid duplicates | ||
| alreadyAdded := false | ||
| for _, existing := range failedFiles { | ||
| if existing == failedFile { | ||
| alreadyAdded = true | ||
| break | ||
| } | ||
| } | ||
| if !alreadyAdded { | ||
| failedFiles = append(failedFiles, failedFile) | ||
| } | ||
| } | ||
| } | ||
| if err := scanner.Err(); err != nil { | ||
| outputChan <- fmt.Sprintf("Error reading stdout: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| // Read stderr in goroutine | ||
| go func() { | ||
| defer wg.Done() | ||
| scanner := bufio.NewScanner(stderr) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| outputChan <- line | ||
| allOutput.WriteString(line + "\n") | ||
|
|
||
| // Also check stderr for FAIL lines (Jest might output to stderr) | ||
| if failedFile := parseFailedTestFile(line, workDir); failedFile != "" { | ||
| // Avoid duplicates | ||
| alreadyAdded := false | ||
| for _, existing := range failedFiles { | ||
| if existing == failedFile { | ||
| alreadyAdded = true | ||
| break | ||
| } | ||
| } | ||
| if !alreadyAdded { | ||
| failedFiles = append(failedFiles, failedFile) | ||
| } | ||
| } | ||
| } | ||
| if err := scanner.Err(); err != nil { | ||
| outputChan <- fmt.Sprintf("Error reading stderr: %v", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
There is a race condition here. The failedFiles slice is being modified by two separate goroutines (one for stdout, one for stderr) without any synchronization, which can lead to data corruption or panics. Also, the logic for checking for and adding new failed files is duplicated in both goroutines.
To fix this, use a mutex to protect access to failedFiles and extract the duplicate logic into a shared helper function.
var allOutput strings.Builder
var failedFilesMu sync.Mutex
failedFiles := []string{}
addFailedFile := func(file string) {
failedFilesMu.Lock()
defer failedFilesMu.Unlock()
for _, f := range failedFiles {
if f == file {
return // Already added
}
}
failedFiles = append(failedFiles, file)
}
// Create wait group for both readers
var wg sync.WaitGroup
wg.Add(2)
// Read stdout in goroutine
go func() {
defer wg.Done()
scanner := bufio.NewScanner(stdout)
for scanner.Scan() {
line := scanner.Text()
outputChan <- line
allOutput.WriteString(line + "\n")
// Look for test failures in real-time
if failedFile := parseFailedTestFile(line, workDir); failedFile != "" {
addFailedFile(failedFile)
}
}
if err := scanner.Err(); err != nil {
outputChan <- fmt.Sprintf("Error reading stdout: %v", err)
}
}()
// Read stderr in goroutine
go func() {
defer wg.Done()
scanner := bufio.NewScanner(stderr)
for scanner.Scan() {
line := scanner.Text()
outputChan <- line
allOutput.WriteString(line + "\n")
// Also check stderr for FAIL lines (Jest might output to stderr)
if failedFile := parseFailedTestFile(line, workDir); failedFile != "" {
addFailedFile(failedFile)
}
}
if err := scanner.Err(); err != nil {
outputChan <- fmt.Sprintf("Error reading stderr: %v", err)
}
}()| j.mu.Lock() | ||
| state.running = false | ||
| state.cmd = nil | ||
| // Keep the liveOutput so it persists after tests complete | ||
| j.mu.Unlock() |
There was a problem hiding this comment.
The state.failedFiles slice is not being updated with the results from the test run. This means the UI will not correctly display the status message indicating the number of failed tests (e.g., "❌ 2 test(s) failed"). Update state.failedFiles with the failedFiles collected during the test run.
j.mu.Lock()
state.running = false
state.failedFiles = failedFiles
state.cmd = nil
// Keep the liveOutput so it persists after tests complete
j.mu.Unlock()
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a Jest testing tab, which is a great addition. The implementation moves test-running logic into a new JestPane UI component, which is a good separation of concerns. My review focuses on the new ui/jest.go file, where I've identified a critical issue with the test command, a bug in the fallback logic, and some opportunities to improve robustness and maintainability by using structured JSON output and removing magic numbers.
|
|
||
|
|
||
| // Run Jest without JSON for live output | ||
| cmd := exec.Command("yarn", "tester") |
There was a problem hiding this comment.
The command yarn tester is used to run Jest tests, but the tester script is not defined in web/package.json. The correct script name appears to be test. This will cause test execution to fail.
| cmd := exec.Command("yarn", "tester") | |
| cmd := exec.Command("yarn", "test") |
| // If we didn't get any output, try running with CombinedOutput as fallback | ||
| if allOutput.Len() == 0 { | ||
| outputChan <- "\nNo output captured from pipes, trying alternative method..." | ||
| fallbackCmd := exec.Command("yarn", "tester") |
There was a problem hiding this comment.
The fallback command also uses the incorrect yarn tester script. This should be yarn test to match the primary test command.
| fallbackCmd := exec.Command("yarn", "tester") | |
| fallbackCmd := exec.Command("yarn", "test") |
| func parseFailedTestFile(line string, workDir string) string { | ||
| // Check if line starts with "FAIL " | ||
| if !strings.HasPrefix(strings.TrimSpace(line), "FAIL ") { | ||
| return "" | ||
| } | ||
|
|
||
| // Extract file path from FAIL line | ||
| // Format: "FAIL src/pages/individualDashboard/component.test.js" | ||
| trimmedLine := strings.TrimSpace(line) | ||
| if len(trimmedLine) <= 5 { // "FAIL " is 5 characters | ||
| return "" | ||
| } | ||
|
|
||
| filePath := strings.TrimSpace(trimmedLine[5:]) | ||
| // Remove any trailing whitespace or test duration info | ||
| if idx := strings.IndexAny(filePath, " \t("); idx > 0 { | ||
| filePath = filePath[:idx] | ||
| } | ||
|
|
||
| // Check if it's a valid test file | ||
| if !strings.HasSuffix(filePath, ".js") && !strings.HasSuffix(filePath, ".jsx") && | ||
| !strings.HasSuffix(filePath, ".ts") && !strings.HasSuffix(filePath, ".tsx") && | ||
| !strings.HasSuffix(filePath, ".test.js") && !strings.HasSuffix(filePath, ".spec.js") { | ||
| return "" | ||
| } | ||
|
|
||
| // Convert to absolute path if needed | ||
| absPath := filePath | ||
| if !filepath.IsAbs(filePath) { | ||
| absPath = filepath.Join(workDir, filePath) | ||
| } | ||
|
|
||
| return absPath | ||
| } |
There was a problem hiding this comment.
This function parses unstructured string output from Jest to find failed test files. This approach is fragile and may break if Jest's output format changes in a future version.
The previous implementation in app/app.go used Jest's --json output, which provides a structured and more reliable way to determine test results. Please consider re-introducing JSON-based parsing for improved robustness. You could still stream the raw output for the live view, but use the generated JSON file for a definitive analysis of test failures after the run completes.
ui/jest.go
Outdated
| if fallbackErr != nil { | ||
| outputChan <- fmt.Sprintf("\nFallback command error: %v", fallbackErr) | ||
| } | ||
| outputChan <- string(fallbackOutput) |
There was a problem hiding this comment.
The fallback mechanism sends the entire test output as a single string to the UI. This bypasses the real-time parsing for failed files that is done for the primary pipe-based output. As a result, if the fallback is triggered, features like autoOpenFailedTests will not work because the failedFiles slice will be empty.
The fallback output should be split into lines and parsed for failures, similar to how the piped output is handled.
// Parse fallback output for failed files and send to UI
lines := strings.Split(string(fallbackOutput), "\n")
for _, line := range lines {
if failedFile := parseFailedTestFile(line, workDir); failedFile != "" {
addFailedFile(failedFile)
}
outputChan <- line
}
ui/jest.go
Outdated
| } | ||
|
|
||
| // Open up to 5 failed test files | ||
| maxFiles := 5 |
There was a problem hiding this comment.
The maximum number of failed test files to open is hardcoded as 5. This "magic number" should be extracted into a package-level constant (e.g., const maxAutoOpenFailedFiles = 5) to improve readability and make it easier to configure in the future. A similar issue exists on line 510 with the 100 * time.Millisecond delay.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Jest testing tab, which is a valuable addition for improving the development workflow. The implementation is well-structured, with a new JestPane component that cleanly encapsulates the test running logic. The code correctly handles streaming test output and the convenience of auto-opening failed files in the IDE is a great touch.
My review focuses on improving the robustness of the test execution and parsing, simplifying some of the UI rendering logic, and ensuring the feature is complete as described. I've identified a critical issue with the test command that will prevent tests from running, and a high-severity issue regarding the reliability of test result parsing. I've also pointed out that some of the advertised keybindings are not yet implemented.
Overall, this is a solid contribution that will be even better with these changes.
|
|
||
|
|
||
| // Run Jest without JSON for live output | ||
| cmd := exec.Command("yarn", "tester") |
There was a problem hiding this comment.
The command yarn tester will fail because the package.json file for the web project does not define a tester script. It does, however, contain a test script which executes jest.
Please correct the script name to test.
cmd := exec.Command("yarn", "test")|
|
||
|
|
||
| // Run Jest without JSON for live output | ||
| cmd := exec.Command("yarn", "tester") |
There was a problem hiding this comment.
This implementation parses raw text output from Jest to find failed tests, which can be brittle. This is a regression from the previous implementation which used Jest's more reliable --json output format.
Parsing structured JSON is much more robust than parsing human-readable text. I strongly recommend modifying the test command to output JSON and updating the parsing logic accordingly. For example, you could change the command to yarn test --json and parse the JSON objects streamed to stdout.
| if m.tabbedWindow.IsInJestTab() { | ||
| switch msg.String() { | ||
| case "r": | ||
| m.tabbedWindow.JestRerunTests() | ||
| return m, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The key handling for the Jest tab appears to be incomplete. The pull request description mentions keybindings for navigating failures (n and p) and opening failures in the IDE (enter), but only the rerun key (r) is handled here.
To fully implement the described feature, you'll need to add cases for the other keys. This will likely require adding corresponding methods to JestPane and TabbedWindow for navigation and opening files.
| if state != nil && state.running { | ||
| content = j.viewport.View() | ||
| } else { | ||
| // Render content as plain text when not running | ||
| rawContent := j.formatContent() | ||
| // Calculate available height for content | ||
| availableHeight := j.height - 4 // header + status + help + spacing | ||
|
|
||
| if availableHeight > 0 { | ||
| lines := strings.Split(rawContent, "\n") | ||
|
|
||
| // Apply scrolling offset manually | ||
| scrollOffset := j.viewport.YOffset | ||
| if scrollOffset < 0 { | ||
| scrollOffset = 0 | ||
| } | ||
|
|
||
| visibleLines := []string{} | ||
| for i := scrollOffset; i < len(lines) && i < scrollOffset+availableHeight; i++ { | ||
| visibleLines = append(visibleLines, lines[i]) | ||
| } | ||
|
|
||
| // Pad with empty lines if needed | ||
| for len(visibleLines) < availableHeight { | ||
| visibleLines = append(visibleLines, "") | ||
| } | ||
|
|
||
| content = strings.Join(visibleLines, "\n") | ||
| } else { | ||
| content = rawContent | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic here for rendering the viewport content when tests are not running is unnecessarily complex. It manually calculates visible lines and re-implements the scrolling behavior that the viewport bubble already provides.
You can simplify this significantly by always using j.viewport.View() to render the content. Your ScrollUp and ScrollDown methods already update the viewport's state, so you can let the viewport component handle the rendering of the scrolled content.
// When tests are running, the viewport auto-scrolls.
// When not running, manual scrolling is handled by ScrollUp/ScrollDown updating the viewport's YOffset.
// In both cases, viewport.View() correctly renders the visible portion.
content = j.viewport.View()| if !strings.HasSuffix(filePath, ".js") && !strings.HasSuffix(filePath, ".jsx") && | ||
| !strings.HasSuffix(filePath, ".ts") && !strings.HasSuffix(filePath, ".tsx") && | ||
| !strings.HasSuffix(filePath, ".test.js") && !strings.HasSuffix(filePath, ".spec.js") { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Checking for specific file suffixes to validate a test file path can be brittle if other test file naming conventions are used. A more robust approach is to check if the file actually exists on the filesystem. This was done in the previous implementation and would be a good addition here to prevent false positives from the text parsing.
// Convert to absolute path if needed
absPath := filePath
if !filepath.IsAbs(filePath) {
absPath = filepath.Join(workDir, filePath)
}
// Check if the file exists to ensure we've parsed a valid path.
if _, err := os.Stat(absPath); err != nil {
return ""
}
1 participant
This pull request introduces a new Jest testing tab to the application's UI and removes outdated configuration documentation. The Jest tab provides functionality for running tests, navigating test failures, and integrating with the IDE. Key changes include updates to the
TabbedWindowcomponent, new Jest-specific keybindings, and modifications to the test execution logic.Jest Tab Integration:
JestTabto theTabbedWindowcomponent, including methods for updating the Jest pane, navigating test failures, rerunning tests, and opening failures in the IDE (ui/tabbed_window.go). [1] [2] [3] [4] [5] [6] [7]homemodel to handle Jest-specific keybindings and integrate Jest functionality, including switching to the Jest tab and updating it with test results (app/app.go). [1] [2] [3] [4]Keybindings:
nandp), rerunning tests (r), and opening failures in the IDE (enter) in the Jest tab (keys/keys.go). [1] [2]Test Execution:
app/app.go).Documentation:
CLAUDE.mdto streamline the documentation.