Skip to content

Conversation

@yaugenst-flex
Copy link
Collaborator

@yaugenst-flex yaugenst-flex commented Oct 28, 2025

Greptile Overview

Updated On: 2025-10-28 16:24:10 UTC

Greptile Summary

Removes the deprecated API key migration functionality that converted legacy auth.json (email/password) to API key authentication. The PR cleans up unused migration code while preserving the separate config file migration feature that moves files from ~/.tidy3d to the canonical XDG config directory.

Key changes:

  • Deleted tidy3d/web/cli/migrate.py module and its tests
  • Removed tidy3d migrate command from root CLI (now only tidy3d config migrate exists for file migration)
  • Removed automatic migration call on module import in tidy3d/web/__init__.py
  • Refactored _run_config_migration() helper function in app.py to handle edge cases
  • Updated resolve_config_directory() in loader.py to prefer canonical directory over legacy
  • Removed unused CREDENTIAL_FILE constant

Confidence Score: 3/5

  • PR is mostly safe but contains one data loss risk in the config migration logic
  • Code removal is clean and thorough with proper test updates. However, the _run_config_migration() function has a logic flaw where --delete-legacy can delete the legacy config directory even when the canonical directory exists but is empty or corrupted, potentially causing data loss
  • Review tidy3d/web/cli/app.py:155-168 for the data loss issue in the migration logic

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/web/cli/migrate.py 5/5 deleted unused API key migration module - clean removal
tidy3d/web/cli/app.py 3/5 removed API key migration logic and refactored config migration helper - has potential data loss issue when delete_legacy=True
tidy3d/config/loader.py 5/5 reordered directory resolution to prefer canonical over legacy - improves config directory handling

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI
    participant Loader
    participant ConfigDir

    Note over User,ConfigDir: Before PR: API Key Migration Flow
    User->>CLI: tidy3d migrate
    CLI->>CLI: Check auth.json exists
    CLI->>CLI: Authenticate with email/password
    CLI->>CLI: Fetch/create API key
    CLI->>CLI: Save to config.toml
    CLI->>CLI: Backup auth.json to auth.json.bak

    Note over User,ConfigDir: After PR: Simplified Flow
    User->>CLI: import tidy3d
    Note over CLI: No automatic migration
    CLI->>Loader: resolve_config_directory()
    Loader->>Loader: Check TIDY3D_BASE_DIR override
    alt Canonical dir writable
        Loader->>ConfigDir: Use canonical (~/.config/tidy3d)
        alt Legacy exists
            Loader-->>User: Warning: legacy ignored, run migrate
        end
    else Legacy exists
        Loader->>ConfigDir: Use legacy (~/.tidy3d)
        Loader-->>User: Warning: consider migration
    else Neither writable
        Loader->>ConfigDir: Use temp directory
    end

    Note over User,ConfigDir: Manual Config Migration
    User->>CLI: tidy3d config migrate --delete-legacy
    CLI->>CLI: Check legacy dir exists
    CLI->>CLI: migrate_legacy_config()
    alt FileExistsError & delete_legacy
        CLI->>CLI: Delete legacy directory
        CLI-->>User: Skipped copy, deleted legacy
    else FileExistsError
        CLI-->>User: Error: use --overwrite
    else Success
        CLI->>ConfigDir: Copy files to canonical
        alt delete_legacy
            CLI->>CLI: Remove legacy directory
        end
        CLI-->>User: Migration complete
    end
Loading

@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/config-fixes branch 2 times, most recently from a67a4a4 to 0ddd570 Compare October 28, 2025 16:05
@yaugenst-flex yaugenst-flex changed the title chore(cli): remove unusued api key migration chore(cli): remove unusued api key migration (FXC-3298) Oct 28, 2025
@yaugenst-flex yaugenst-flex self-assigned this Oct 28, 2025
@yaugenst-flex yaugenst-flex marked this pull request as ready for review October 28, 2025 16:13
@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/config-fixes branch from 0ddd570 to ff5063d Compare October 28, 2025 16:18
@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/config-fixes branch from ff5063d to 0fec847 Compare October 28, 2025 16:20
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Works, I no longer see the message.

@github-actions
Copy link
Contributor

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/config/loader.py (83.3%): Missing lines 263
  • tidy3d/web/cli/app.py (47.1%): Missing lines 155-159,163-164,168,205

Summary

  • Total: 23 lines
  • Missing: 10 lines
  • Coverage: 56%

tidy3d/config/loader.py

  259     canonical_dir = canonical_config_directory()
  260     if _is_writable(canonical_dir.parent):
  261         legacy_dir = legacy_config_directory()
  262         if legacy_dir.exists():
! 263             log.warning(
  264                 f"Using canonical configuration directory at '{canonical_dir}'. "
  265                 "Found legacy directory at '~/.tidy3d', which will be ignored. "
  266                 "Remove it manually or run 'tidy3d config migrate --delete-legacy' to clean up.",
  267                 log_once=True,

tidy3d/web/cli/app.py

  151     canonical_dir = canonical_config_directory()
  152     try:
  153         destination = migrate_legacy_config(overwrite=overwrite, remove_legacy=delete_legacy)
  154     except FileExistsError:
! 155         if delete_legacy:
! 156             try:
! 157                 shutil.rmtree(legacy_dir)
! 158             except OSError as exc:
! 159                 click.echo(
  160                     f"Destination '{canonical_dir}' already exists and the legacy directory "
  161                     f"could not be removed. Error: {exc}"
  162                 )
! 163                 return
! 164             click.echo(
  165                 f"Destination '{canonical_dir}' already exists. "
  166                 "Skipped copying legacy files and removed the legacy '~/.tidy3d' directory."
  167             )
! 168             return
  169         click.echo(
  170             f"Destination '{canonical_dir}' already exists. "
  171             "Use '--overwrite' to replace the existing files."
  172         )

  201 )
  202 def config_migrate(overwrite: bool, delete_legacy: bool) -> None:
  203     """Copy configuration files from '~/.tidy3d' to the canonical location."""
  204 
! 205     _run_config_migration(overwrite, delete_legacy)
  206 
  207 
  208 @click.group()
  209 def config_group():

@yaugenst-flex yaugenst-flex added this pull request to the merge queue Oct 28, 2025
auto-merge was automatically disabled October 28, 2025 17:34

Pull Request is not mergeable

Merged via the queue into develop with commit 0ed42f3 Oct 28, 2025
67 of 97 checks passed
@yaugenst-flex yaugenst-flex deleted the yaugenst-flex/config-fixes branch October 28, 2025 18:00
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