Skip to content

Define model display order once on ModelId; derive sort and UI index from it #14

Description

@BumpyClock

Problem

Model display order is encoded in four independent locations. Adding or reordering a model requires synchronized edits across all four, and existing tests mostly validate that the duplication stayed in sync rather than guarding behavior.

1. crates/shared/src/settings.rs:113–120ModelId::ALL (catalog order):

pub const ALL: &[Self] = &[
    Self::ParakeetV3,
    Self::NemotronSpeechStreamingEn06b,
    Self::WhisperSmall,
    Self::WhisperMedium,
    Self::WhisperTurbo,
    Self::WhisperLarge,
];

2. crates/core/src/transcription/model/mod.rs:105–109list_models sort key (service layer):

list.sort_by_key(|model| match model.id {
    ModelId::ParakeetV3 => (0_u8, model.size_mb),
    ModelId::NemotronSpeechStreamingEn06b => (1_u8, model.size_mb),
    _ => (2_u8, model.size_mb),
});

3. crates/app/src/ui/settings/sections/models/mod.rs:223–232model_id_index (UI element IDs):

const fn model_id_index(id: ModelId) -> usize {
    match id {
        ModelId::ParakeetV3 => 0,
        ModelId::NemotronSpeechStreamingEn06b => 1,
        ModelId::WhisperSmall => 2,
        ModelId::WhisperMedium => 3,
        ModelId::WhisperTurbo => 4,
        ModelId::WhisperLarge => 5,
    }
}

4. docs/design/unified-model-selection.md:394 — design-doc sort sentence:

Keep existing: Parakeet first, then Whisper models sorted by size ascending.

(The doc predates Nemotron and does not mention it — a silent staleness.)

All four must agree on variant count and relative order. Today they happen to agree; the risk is that the next model addition touches ModelId::ALL and catalog.rs but misses list_models or model_id_index, causing wrong UI ordering or a panicking element-ID collision that is only caught at runtime.

Why it matters

  • Synchronized-edit footgun. Every new model variant is a four-file change with no compile-time enforcement. The compiler does not warn if a new variant falls into the _ => arm of the sort or gets an auto-assigned usize in a separate match arm.
  • Silent staleness. model_id_index and the sort key are const fn / inline — no warning if they go out of sync with ALL. The design doc already drifted (no mention of Nemotron at line 394).
  • Tests verify alignment, not correctness. list_models_order_parakeet_nemotron_then_whisper (tests.rs:16–42) and model_id_index_covers_all_variants_uniquely (mod.rs:248–254) are green only because all four copies currently agree. They would not catch a new variant added to ALL but missed in list_models.

Proposed change

  1. Add ModelId::display_order() -> usize (or reuse the positional index of ALL) in crates/shared/src/settings.rs. This becomes the single authoritative rank for a variant.
  2. Rewrite list_models sort to call model.id.display_order() instead of the hard-coded match on bucket integers (mod.rs:105–109).
  3. Rewrite model_id_index to call ModelId::ALL.iter().position(|v| *v == id).unwrap() (or delegate to display_order()), removing the redundant match (mod.rs:223–232). Because model_id_index is const fn, a display_order() -> usize helper on ModelId that does the position search suffices (or promote to a non-const helper if needed by the GPUI element-ID type).
  4. Update docs/design/unified-model-selection.md:394 to mention the ordering rule now lives in ModelId::ALL.
  5. Trim or convert alignment-only tests. list_models_keeps_parakeet_first and list_models_order_parakeet_nemotron_then_whisper can be collapsed to a single property test asserting list_models() output order matches ModelId::ALL order. model_id_index_covers_all_variants_uniquely can be replaced by a doc-comment assertion or kept as a compile-time sanity check — but should not be the only guard.

No behavior changes for existing models.

Recommendation / triage

DO — define order once on ModelId (e.g. a display_order() method or reuse ALL) and derive list_models + model_id_index from it. Moderate effort; removes a real synchronized-edit footgun for future models. The Nemotron addition is recent evidence this footgun is live: nemotron-support-implementation-plan.md:184 explicitly called out "Adjust list sort if needed so Parakeet then Nemotron then Whisper" as a checklist item — meaning the author knew it was a separate place to update.

Acceptance criteria

  • ModelId::display_order() (or equivalent) defined once in crates/shared/src/settings.rs; returns a stable usize rank derived from ALL position.
  • list_models sort in crates/core/src/transcription/model/mod.rs delegates to display_order() — no free-standing match on bucket integers.
  • model_id_index in crates/app/src/ui/settings/sections/models/mod.rs delegates to the same helper — no free-standing match.
  • Adding a new variant to ModelId::ALL automatically propagates correct order to both list_models and model_id_index with no other edits required.
  • docs/design/unified-model-selection.md:394 updated to reference ModelId::ALL as the ordering authority.
  • Behavior unchanged: list_models() output order is identical to today's (Parakeet → Nemotron → Whisper by size). Verified by the surviving ordering test.
  • Tests that survive: one ordering test asserting list_models() output matches ModelId::ALL order; model_id_index_covers_all_variants_uniquely may be kept or replaced by a static assertion. The redundant list_models_keeps_parakeet_first test may be removed.
  • cargo test green across ansible-shared, ansible-core, and ansible-app.

Traceability

  • clawpatch finding ID: fnd_sig-feat-service-60d65539ae-581a_828c66acc2
  • Revalidate command: clawpatch revalidate --finding fnd_sig-feat-service-60d65539ae-581a_828c66acc2 --reasoning-effort high
  • No directly coupled issue. Related context: docs/design/nemotron-support-implementation-plan.md:156,184 (Nemotron ordering checklist items that triggered discovery of the duplication).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    deslopifyFound by clawpatch deslopify reviewtech-debtCode quality / maintainability cleanup

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions