Skip to content

feat: add model detail view (#129)#158

Open
Yegorov wants to merge 3 commits into
cardmagic:masterfrom
Yegorov:129
Open

feat: add model detail view (#129)#158
Yegorov wants to merge 3 commits into
cardmagic:masterfrom
Yegorov:129

Conversation

@Yegorov

@Yegorov Yegorov commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Closes #129

Comment thread test/cli/registry_commands_test.rb
Comment thread test/cli/registry_commands_test.rb
Comment thread test/cli/registry_commands_test.rb
@greptile-apps

greptile-apps Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a model detail view to both classifier models <name> and classifier models --local <name>, introducing a new detect_registry_and_model helper that parses the @registry:model argument format.

  • Remote detail view: when a model name (or @registry:model spec) is passed, list_remote_models fetches the index and renders a structured detail card instead of the table row.
  • Local detail view: list_local_models similarly accepts an optional model name argument and renders type, categories, version, and size for a single cached model.
  • New tests: four tests cover the remote/local detail paths including the not-found and custom-registry cases, though none exercise a model entry missing the categories key.

Confidence Score: 3/5

The new detail view paths will crash at runtime for any model whose JSON lacks a categories key — a realistic scenario for older cached models or third-party registries.

Both list_remote_models and list_local_models call .map / .keys directly on info['categories'] with no nil guard. Any model entry that omits that field triggers a NoMethodError rather than a graceful message, making the detail view unreliable against real-world data.

The nil-categories guard fixes belong in lib/classifier/cli.rb at lines 395 and 455. The test fixtures always include categories, so the crash path is untested.

Important Files Changed

Filename Overview
lib/classifier/cli.rb Adds model detail view to both remote and local listing commands; introduces detect_registry_and_model helper. Two NoMethodError crash paths exist when a model's JSON lacks a categories key.
test/cli/registry_commands_test.rb Adds four new tests covering remote detail view, not-found path, custom-registry detail, and local detail view. All tests use fixtures that always include categories, so the nil-categories crash path is not exercised.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["classifier models [arg]"] --> B{--local flag?}
    B -- no --> C[list_remote_models]
    B -- yes --> D[list_local_models]

    C --> E[detect_registry_and_model]
    E --> F[parse_registry + DEFAULT_REGISTRY]
    F --> G[fetch_registry_index]
    G --> H{model arg present?}
    H -- yes --> I[find model in index]
    I --> J{found?}
    J -- no --> K[output: No model found]
    J -- yes --> L[render detail card]
    H -- no --> M[render table rows for all models]

    D --> N{model_arg present?}
    N -- yes --> O[find in local models list]
    O --> P{found?}
    P -- no --> Q[output: No local model found]
    P -- yes --> R[load_model_info + render detail card]
    N -- no --> S[render table rows for all local models]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["classifier models [arg]"] --> B{--local flag?}
    B -- no --> C[list_remote_models]
    B -- yes --> D[list_local_models]

    C --> E[detect_registry_and_model]
    E --> F[parse_registry + DEFAULT_REGISTRY]
    F --> G[fetch_registry_index]
    G --> H{model arg present?}
    H -- yes --> I[find model in index]
    I --> J{found?}
    J -- no --> K[output: No model found]
    J -- yes --> L[render detail card]
    H -- no --> M[render table rows for all models]

    D --> N{model_arg present?}
    N -- yes --> O[find in local models list]
    O --> P{found?}
    P -- no --> Q[output: No local model found]
    P -- yes --> R[load_model_info + render detail card]
    N -- no --> S[render table rows for all local models]
Loading
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 4
lib/classifier/cli.rb:395
**`NoMethodError` crash when `categories` is missing**`info['categories']` returns `nil` for any model entry that omits the key, so calling `.map` on it raises `NoMethodError`. The `|| ''` at the end is applied to the result of `join`, not the array, so it cannot rescue a nil receiver.

```suggestion
        categories = (info['categories'] || []).map(&:downcase).join(', ')
```

### Issue 2 of 4
lib/classifier/cli.rb:455
**`NoMethodError` crash when `categories` is missing in local model**`info['categories']` can be `nil` if the stored model file doesn't include that key (e.g. models saved before this field was standard). Calling `.keys` on `nil` will raise `NoMethodError`. There is no fallback default here at all, unlike the remote path.

```suggestion
        categories = (info['categories'] || {}).keys.map(&:downcase).join(', ')
```

### Issue 3 of 4
lib/classifier/cli.rb:385-411
**Nested conditional instead of early return** — both `list_remote_models` and `list_local_models` wrap the default list-rendering path in an `else` branch. Per the repo's style guide, the single-model branch should early-return so the loop is the unconditional tail, with no nesting required. The same pattern appears in `list_local_models` at the equivalent block (lines ~445–471).

In this review, suggest simpl... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

### Issue 4 of 4
lib/classifier/cli.rb:847-849
**Redundant comment restates the method name** — "Detect registry and model" adds no information beyond what the method name already says. Per the repo's De-AI style guide, comments that restate the code should be removed. The `@rbs` annotation below it is a keeper.

```suggestion
    # @rbs (String?) -> [String?, String?]
    def detect_registry_and_model(arg)
```

In this review, suggest simpl... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

Reviews (1): Last reviewed commit: "feat: add model detail view (#129)" | Re-trigger Greptile

Comment thread lib/classifier/cli.rb Outdated
Comment thread lib/classifier/cli.rb Outdated
Comment thread lib/classifier/cli.rb
Comment thread lib/classifier/cli.rb Outdated
@Yegorov

Yegorov commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@greptile-apps Tell me how to correct the comments (Metrics/AbcSize: Assignment Branch Condition size for test_models_local_model_detail_view is too high. [17.12/15]) from houndci-bot?

@greptile-apps

greptile-apps Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

The ABC size of 17.12 comes from the method having too many branches (method calls). Let me count: File.join ×3, FileUtils.mkdir_p, File.write ×2, run_cli, result[:output] ×4, result[:exit_code], result[:error], plus assertions = ~17 branches.

The cleanest fix is to extract the repeated file-setup into a helper (used by both test_models_local_model_detail_view and test_models_local_model_detail_view_if_not_found), and cache result[:output] in a local:

def create_local_model_fixtures(models_dir)
  FileUtils.mkdir_p(models_dir)
  File.write(File.join(models_dir, 'spam-filter.json'), @model_json)
  File.write(File.join(models_dir, 'sentiment.json'), @model_json)
end

def test_models_local_model_detail_view
  create_local_model_fixtures(File.join(@cache_dir, 'models'))

  result = run_cli('models', '--local', 'spam-filter')
  output = result[:output]

  assert_equal 0, result[:exit_code]
  assert_match('Name: spam-filter', output)
  assert_match('Type: bayes', output)
  assert_match('Categories: spam, ham', output)
  assert_empty result[:error]
end

This reduces the method's branch count from ~17 to ~8 (ABC ≈ 9), well under the 15 limit. Apply the same pattern to test_models_local_model_detail_view_if_not_found since it has identical setup.

@Yegorov Yegorov requested a review from houndci-bot June 18, 2026 12:44
@Yegorov

Yegorov commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@cardmagic can you take a look, please?

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.

CLI: Add model detail view

2 participants