Skip to content

Conversation

@paxcalpt
Copy link
Contributor

@paxcalpt paxcalpt commented Jan 7, 2026

Summary

This PR adds several major improvements to TaskRepo:

🎯 TUI State Persistence

The TUI now remembers your preferences across sessions:

  • Tree vs Flat view: Automatically restores your preferred display mode
  • View selection: Returns to the last repo/project/assignee you were viewing
  • View mode: Remembers whether you were in repo/project/assignee mode

Configuration:

  • remember_tui_state: Master toggle (default: true)
  • tui_tree_view: Tree view preference
  • tui_last_view_item: Last selected view item

Bug fixes:

  • Fixed --repo flag always overwriting restored state
  • Changed tree toggle key from 'r' to 't' for better ergonomics

📝 New Commands

tsk add-link - Add URLs to tasks

tsk add-link 5 "https://github.com/org/repo/issues/123"

tsk append - Append text to task descriptions

tsk append 5 --text "Additional note from meeting"

tsk update - Batch update task fields

tsk update 5,6,7 --status in-progress --priority H

🤖 Non-Interactive Mode

Better support for automation and CI environments:

  • delete: Requires --force flag in non-interactive terminals
  • done/unarchive: Auto-confirms subtask operations
  • sync: New --non-interactive flag to skip prompts

✨ Other Improvements

  • Archive: New --all-completed flag to bulk-archive completed tasks
  • Sync: Improved git operations with better interactive prompt handling
  • Config display: Shows TUI state persistence settings

Testing

  • ✅ TUI state persistence tested across sessions
  • ✅ View restoration works correctly
  • ✅ Tree view toggle persistence verified
  • ✅ View mode switching resets saved item as expected

🤖 Generated with Claude Code

…ements

## TUI State Persistence
- Add configuration properties for remembering TUI state across sessions
  - remember_tui_state: master toggle (default: true)
  - tui_tree_view: persist tree/flat view preference
  - tui_last_view_item: persist selected repo/project/assignee
- Restore view state on TUI startup
- Save state on navigation (arrow keys, tree toggle)
- Reset saved item when switching view modes (Tab key)
- Fix bug where --repo flag always overwrote restored state
- Update config display to show TUI persistence settings
- Change tree toggle key from 'r' to 't' in help text

## New Commands
- add-link: Add URLs to task links
  - Usage: tsk add-link <task_id> <url>
  - Validates HTTP/HTTPS URLs
- append: Append text to task descriptions
  - Usage: tsk append <task_id> --text "content"
- update: Batch update task fields
  - Usage: tsk update <task_ids> [--priority/--status/--project/etc]
  - Supports: priority, status, project, tags, assignees, due date, title

## Non-Interactive Terminal Detection
- Add sys.stdin.isatty() checks across commands
- delete: require --force flag in non-interactive mode
- done: auto-confirm subtask marking in non-interactive mode
- unarchive: auto-confirm subtask operations in non-interactive mode
- sync: add --non-interactive flag to skip unexpected file prompts

## Archive Command Enhancement
- Add --all-completed flag to archive all completed tasks at once
- Usage: tsk archive --all-completed [--repo <name>]

## Sync Command Improvements
- Add run_git_verbose() for interactive git operations
- Implement SimpleSyncProgress for safer terminal output
- Better handling of prompts and credential helpers
- Improved display of unexpected files during sync

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Jan 7, 2026

Code Review - PR #11

Thank you for this comprehensive PR! This adds several valuable features to TaskRepo. Below is my detailed review covering code quality, potential issues, performance, security, and testing.


✅ Strengths

1. Well-Structured New Commands

The three new commands (add-link, append, update) follow existing patterns consistently and integrate well with the codebase.

2. TUI State Persistence

The implementation is clean with proper configuration management through properties and sensible defaults (remember_tui_state: true).

3. Non-Interactive Mode Improvements

Good safety-first approach requiring --force for destructive operations in non-interactive terminals.

4. Sync Command Refactoring

The switch from Rich's Progress to SimpleSyncProgress is a pragmatic solution to terminal freezing issues during interactive prompts.


🐛 Potential Bugs & Issues

HIGH PRIORITY

1. Type Error in update.py:128 ⚠️

task.due = parsed_date.isoformat()  # Line 128

Issue: task.due expects Optional[datetime], but you're assigning a string (isoformat() returns str).

Fix:

task.due = parsed_date  # Store datetime directly

The Task model already handles serialization to ISO format during save.


2. detect_conflicts Missing Parameter in sync.py:603 ⚠️

conflicts, _ = run_with_spinner(
    progress, spinner_task, "Checking for conflicts", 
    check_conflicts, verbose, operations_task
)

The lambda passes skip_fetch=True, but detect_conflicts may not accept this parameter. Verify the function signature in the conflict detection module.


3. Race Condition in Config Saves

Multiple TUI keybindings save config state independently:

  • src/taskrepo/tui/task_tui.py:663-668 (right arrow)
  • src/taskrepo/tui/task_tui.py:678-683 (left arrow)
  • src/taskrepo/tui/task_tui.py:703-704 (tab)
  • src/taskrepo/tui/task_tui.py:794-795 ('t' key)

Issue: Each keypress triggers a disk write (config.save()). In rapid navigation, this could cause:

  • Performance degradation
  • Potential data loss if writes overlap

Recommendation: Debounce config saves or batch them on TUI exit:

def cleanup(self):
    """Called on TUI exit."""
    if self.config.remember_tui_state:
        self.config.tui_tree_view = self.tree_view
        if self.current_view_idx == -1:
            self.config.tui_last_view_item = None
        else:
            self.config.tui_last_view_item = self.view_items[self.current_view_idx]
        self.config.save()  # Single save on exit

MEDIUM PRIORITY

4. Missing URL Validation in add_link.py:30-32

if not url.startswith(("http://", "https://")):
    click.secho("Error: URL must start with http:// or https://", fg="red", err=True)
    ctx.exit(1)

Issue: This is less strict than Task.validate_url() which checks for a valid netloc. Use the existing validation:

from taskrepo.core.task import Task

if not Task.validate_url(url):
    click.secho("Error: Invalid URL format", fg="red", err=True)
    ctx.exit(1)

5. Inconsistent Error Handling in append.py

if task.description:
    task.description = task.description.rstrip() + "\n\n" + text
else:
    task.description = text

Potential Issue: If task.description is None (not just empty string), this will fail. The Task model uses description: str = "" so it should be safe, but defensive coding is better:

current = (task.description or "").rstrip()
task.description = f"{current}\n\n{text}" if current else text

6. Detached HEAD Recovery Logic Could Be More Robust (sync.py:437-467)

The detached HEAD recovery tries to reattach automatically, which is good, but:

if current_sha == branch_sha:
    git_repo.heads[target_branch].checkout()

Concern: If checkout fails (e.g., due to local changes), the exception is caught but should_push is set to False silently. The user should be informed more clearly.

Suggestion:

try:
    git_repo.heads[target_branch].checkout()
    progress.console.print(f"  [green]✓[/green] Re-attached to '{target_branch}'")
except GitCommandError as checkout_error:
    progress.console.print(f"  [red]✗[/red] Failed to checkout {target_branch}: {checkout_error}")
    should_push = False

7. --all-completed Without Confirmation (archive.py:21-66)

The new --all-completed flag archives all completed tasks without confirmation. This could be surprising for users with many completed tasks.

Recommendation: Add a confirmation prompt unless --yes is also provided:

if all_completed and not task_ids:
    # ... find completed tasks ...
    
    if not yes:
        click.echo(f"About to archive {len(completed_ids)} completed task(s).")
        if not click.confirm("Continue?"):
            click.echo("Cancelled.")
            return
    
    task_ids = tuple(completed_ids)

🔒 Security Considerations

1. Command Injection Risk is Low

The run_git_verbose function uses subprocess.run with a list (not shell=True), which prevents injection:

result = subprocess.run(["git"] + args, cwd=repo_path, check=False)

This is secure - well done!

2. Path Traversal in delete_unexpected_files (file_validation.py:177-206)

full_path = repo_path / file_path
if full_path.is_file():
    full_path.unlink()

Low Risk: The file_path comes from git_repo.untracked_files, which should be safe. However, explicitly checking that the resolved path is within repo_path would be more defensive:

full_path = (repo_path / file_path).resolve()
if not full_path.is_relative_to(repo_path):
    click.secho(f"  ⚠️  Skipping file outside repo: {file_path}", fg="yellow")
    continue

Performance Considerations

1. Config Save Frequency (mentioned above)

Saving config on every TUI keypress is inefficient. Batch saves on exit.

2. --all-completed Loads All Tasks

all_tasks = manager.list_all_tasks(include_archived=False)

For users with hundreds of tasks across many repos, this could be slow. Consider adding progress feedback:

with click.progressbar(all_tasks, label="Scanning for completed tasks") as bar:
    completed_tasks = [task for task in bar if task.status == "completed"]

3. SimpleSyncProgress Doesn't Update Progress

The new SimpleSyncProgress.update() method doesn't print updates (line 260-261: commented out). This means users lose visual feedback during long operations. Consider printing significant updates:

def update(self, task_id, advance=None, description=None, **kwargs):
    # ... existing code ...
    if description and "completed" in description.lower():
        self.console.print(f"  {description}")  # Print completion messages

🧪 Test Coverage

Missing Tests ⚠️

Based on the file listing, there are no tests for the new commands:

  • tests/unit/test_add_link.py - missing
  • tests/unit/test_append.py - missing
  • tests/unit/test_update.py - missing

Recommended Test Cases

For add-link:

def test_add_link_valid_url():
    # Test adding a valid URL to a task

def test_add_link_duplicate():
    # Test adding the same URL twice (should skip)

def test_add_link_invalid_url():
    # Test with invalid URL (should error)

def test_add_link_task_not_found():
    # Test with non-existent task ID

For append:

def test_append_to_existing_description():
    # Test appending to task with existing description

def test_append_to_empty_description():
    # Test appending to task with no description

def test_append_preserves_existing_content():
    # Verify original content is preserved

For update:

def test_update_single_task():
    # Test updating one field on one task

def test_update_multiple_tasks():
    # Test updating multiple tasks at once

def test_update_multiple_fields():
    # Test updating priority, status, tags together

def test_update_no_options():
    # Test error when no update options provided

def test_update_due_date_parsing():
    # Test natural language date parsing

def test_update_due_date_invalid():
    # Test handling of invalid dates

For TUI state persistence:

def test_tui_state_persistence():
    # Test that view state is restored across sessions

def test_tui_state_disabled():
    # Test behavior when remember_tui_state is False

def test_tui_tree_view_toggle():
    # Test tree view preference is saved

For non-interactive mode:

def test_delete_non_interactive_without_force():
    # Should fail without --force

def test_done_non_interactive_subtasks():
    # Should auto-confirm subtasks

def test_sync_non_interactive_skip_unexpected():
    # Should skip repos with unexpected files

📝 Code Quality & Best Practices

Good Practices

  1. Docstrings: All new functions have Google-style docstrings
  2. Type hints: Used consistently (Optional[str], Tuple[str, ...])
  3. Error messages: Clear and actionable
  4. Code reuse: Leverages existing helpers (find_task_by_title_or_id, select_task_from_result)

Style Suggestions

1. Unused Import in add_link.py

import sys  # Line 3 - never used

Remove this or suppress with # noqa: F401 if planned for future use.

2. Magic Numbers in file_validation.py:106

for file_path in sorted(files)[:5]:  # Show max 5 files per pattern
    console.print(f"    - {file_path}")
if file_count > 5:
    console.print(f"    ... and {file_count - 5} more")

Extract to a named constant:

MAX_FILES_TO_DISPLAY = 5

3. Duplicate Code in TUI Key Bindings

Lines 663-668 and 678-683 in task_tui.py have identical logic for saving view state. Extract to a helper:

def _save_current_view(self):
    if self.config.remember_tui_state:
        if self.current_view_idx == -1:
            self.config.tui_last_view_item = None
        else:
            self.config.tui_last_view_item = self.view_items[self.current_view_idx]

📋 Documentation

PR Description

Excellent! Clear summary with examples and testing notes.

Missing Documentation

  • Update main README with new commands (add-link, append, update)
  • Document --all-completed flag in archive command docs
  • Document new config options (remember_tui_state, tui_tree_view, tui_last_view_item)
  • Add --non-interactive flag to sync command docs

🎯 Summary & Recommendations

Must Fix Before Merge:

  1. ✅ Fix type error in update.py:128 (datetime vs string)
  2. ✅ Verify detect_conflicts signature for skip_fetch parameter
  3. ✅ Add tests for new commands (minimum: happy path tests)

Should Fix:

  1. Use Task.validate_url() in add_link.py
  2. Add confirmation for --all-completed without --yes
  3. Batch TUI config saves on exit instead of per-keypress
  4. Remove unused import sys from add_link.py

Nice to Have:

  1. Add defensive path checking in delete_unexpected_files
  2. Extract duplicate TUI save logic to helper method
  3. Add progress feedback for --all-completed scanning

Overall Assessment

This is a solid PR with valuable features. The code follows existing patterns well and the non-interactive improvements are thoughtful. The main concerns are:

  1. The type error in update.py
  2. Missing test coverage for new functionality
  3. Frequent config saves in TUI

With the critical fixes applied and basic tests added, this will be ready to merge.

Great work on the comprehensive feature additions! 🚀

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.

2 participants