Skip to content

misc improvement suggestions in init flow UX #545

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Saga4
Copy link
Contributor

@Saga4 Saga4 commented Jul 14, 2025

When user already have a CF configuration:
image

Giving option to select nested directory on test file:
image

PR Type

Enhancement


Description

  • Add interactive directory browser functionality

  • Implement display_current_config for existing settings

  • Update reconfiguration prompt UX in init flow

  • Use browser for tests directory selection


Changes diagram

flowchart LR
  A["should_modify_pyproject_toml"] -- "not reconfigure" --> B["display_current_config"]
  A -- "reconfigure" --> E["collect_setup_info"]
  E -- "browse for tests" --> C["browse_and_select_directory"]
  C -- "returns path" --> D["tests_root"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
cmd_init.py
Add directory browser and config display                                 

codeflash/cli_cmds/cmd_init.py

  • Add browse_and_select_directory function
  • Add display_current_config function
  • Update should_modify_pyproject_toml UX
  • Replace tests path prompt with browser
  • +178/-11

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Undefined Variable

    config_file_path is not defined in should_modify_pyproject_toml, causing a NameError when display_current_config is called.

        display_current_config(config, config_file_path)
    
    return should_reconfigure
    Incompatible Type Annotation

    The signature Path | None requires Python 3.10+. If older versions are supported, use Optional[Path] from typing.

    def browse_and_select_directory(start_dir: Path, message: str, title: str) -> Path | None:
    Unbounded Loop

    browse_and_select_directory uses an unbounded while True. Invalid inputs (e.g., bad custom paths) could trap the user in the loop; consider retry limits or a clear exit option.

    def browse_and_select_directory(start_dir: Path, message: str, title: str) -> Path | None:
        """Interactive directory browser with tab-completion-like functionality."""
        current_dir = start_dir
    
        while True:
            # Get all subdirectories in current directory
            try:
                subdirs = []
                for item in current_dir.iterdir():
                    if item.is_dir() and not item.name.startswith('.'):
                        subdirs.append(item.name)
                subdirs.sort()
            except PermissionError:
                console.print(f"❌ Permission denied accessing {current_dir}")
                return None
    
            # Build choices list
            choices = []
    
            # Add parent directory option if not at start_dir
            if current_dir != start_dir and current_dir.parent != current_dir:
                choices.append(("📁 .. (parent directory)", ".."))
    
            # Add current directory selection option
            choices.append(("✅ Select this directory", "SELECT"))
    
            # Add subdirectories
            for subdir in subdirs:
                choices.append((f"📂 {subdir}/", subdir))
    
            # Add option to type custom path
            choices.append(("⌨️  Type custom path", "CUSTOM"))
            choices.append(("❌ Cancel", "CANCEL"))
    
            # Show current directory in panel
            browse_panel = Panel(
                Text(f"📍 Current directory: {current_dir}\n\n{message}", style="cyan"),
                title=title,
                border_style="bright_blue",
            )
            console.print(browse_panel)
            console.print()
    
            # Prompt for selection
            questions = [
                inquirer.List(
                    "selection",
                    message="Navigate or select directory:",
                    choices=choices,
                    carousel=True,
                )
            ]
    
            answers = inquirer.prompt(questions, theme=CodeflashTheme())
            if not answers:
                return None
    
            selection = answers["selection"]
    
            if selection == "SELECT":
                return current_dir
            elif selection == "CANCEL":
                return None
            elif selection == "..":
                current_dir = current_dir.parent
            elif selection == "CUSTOM":
                # Allow custom path input
                custom_panel = Panel(
                    Text("Enter a directory path (relative to current directory or absolute):", style="yellow"),
                    title="🔧 Custom Path",
                    border_style="bright_yellow",
                )
                console.print(custom_panel)
                console.print()
    
                custom_path = click.prompt("Directory path", type=str).strip()
                if custom_path:
                    try:
                        if Path(custom_path).is_absolute():
                            new_dir = Path(custom_path)
                        else:
                            new_dir = current_dir / custom_path
    
                        new_dir = new_dir.resolve()
                        if new_dir.exists() and new_dir.is_dir():
                            current_dir = new_dir
                        else:
                            console.print(f"❌ Directory does not exist: {new_dir}")
                            click.pause()
                    except Exception as e:
                        console.print(f"❌ Invalid path: {e}")
                        click.pause()
            else:
                # Navigate to subdirectory
                try:
                    new_dir = current_dir / selection
                    if new_dir.exists() and new_dir.is_dir():
                        current_dir = new_dir
                    else:
                        console.print(f"❌ Directory not accessible: {new_dir}")
                        click.pause()
                except Exception as e:
                    console.print(f"❌ Error accessing directory: {e}")
                    click.pause()

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Catch generic filesystem errors

    Broaden the exception handling to include other filesystem errors like OSError and
    show the error message. This prevents uncaught exceptions if iterdir fails for
    reasons other than permissions.

    codeflash/cli_cmds/cmd_init.py [192-200]

     try:
         subdirs = []
         for item in current_dir.iterdir():
             if item.is_dir() and not item.name.startswith('.'):
                 subdirs.append(item.name)
         subdirs.sort()
    -except PermissionError:
    -    console.print(f"❌ Permission denied accessing {current_dir}")
    +except (PermissionError, OSError) as e:
    +    console.print(f"❌ Error accessing directory {current_dir}: {e}")
         return None
    Suggestion importance[1-10]: 6

    __

    Why: Expanding to catch OSError prevents uncaught filesystem errors during directory iteration and surfaces the actual error message.

    Low
    Prevent KeyError on selection

    Guard against a missing "selection" key in the answers dict to avoid a potential
    KeyError. Use dict.get and explicitly handle the None case before proceeding.

    codeflash/cli_cmds/cmd_init.py [239-243]

     answers = inquirer.prompt(questions, theme=CodeflashTheme())
     if not answers:
         return None
    -    
    -selection = answers["selection"]
     
    +selection = answers.get("selection")
    +if selection is None:
    +    return None
    +
    Suggestion importance[1-10]: 5

    __

    Why: Using answers.get("selection") guards against a missing key and avoids a potential KeyError, improving robustness.

    Low
    General
    Use explicit Choice objects

    Wrap each choice in an explicit inquirer.Choice to ensure the display name and value
    map correctly and avoid ambiguous tuple handling. This makes the list of choices
    more robust and clear for the prompt engine.

    codeflash/cli_cmds/cmd_init.py [204-218]

     choices = []
     if current_dir != start_dir and current_dir.parent != current_dir:
    -    choices.append(("📁 .. (parent directory)", ".."))
    -choices.append(("✅ Select this directory", "SELECT"))
    +    choices.append(inquirer.Choice("📁 .. (parent directory)", ".."))
    +choices.append(inquirer.Choice("✅ Select this directory", "SELECT"))
     for subdir in subdirs:
    -    choices.append((f"📂 {subdir}/", subdir))
    -choices.append(("⌨️  Type custom path", "CUSTOM"))
    -choices.append(("❌ Cancel", "CANCEL"))
    +    choices.append(inquirer.Choice(f"📂 {subdir}/", subdir))
    +choices.append(inquirer.Choice("⌨️  Type custom path", "CUSTOM"))
    +choices.append(inquirer.Choice("❌ Cancel", "CANCEL"))
    Suggestion importance[1-10]: 4

    __

    Why: Wrapping tuples in inquirer.Choice can improve clarity but is optional since tuple choices already work and requires an extra import.

    Low

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

    Successfully merging this pull request may close these issues.

    1 participant