Skip to content

Fix instruction browser review findings - #21

Merged
hbmartin merged 3 commits into
mainfrom
pr20-review-fixes
Aug 2, 2026
Merged

Fix instruction browser review findings#21
hbmartin merged 3 commits into
mainfrom
pr20-review-fixes

Conversation

@hbmartin

@hbmartin hbmartin commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to #20 addressing all four code-review findings:

  • Error loads exit Instructions mode_show_error now resets the view mode, syncs the mode select, and re-syncs tab visibility, so a failed load restores the whole-model controls instead of stranding empty section/step dropdowns.
  • Step labels cached and guarded — per-section occurrence counts are computed once per section selection instead of re-expanding every prior step on each render (one expansion per step change instead of ~seven). The labels are validated against the cumulative occurrence count before use; if pyldraw3's concatenation invariant ever drifts, the Pieces table falls back to blank step labels instead of dying on the strict=True zip.
  • Directive cells render losslessly — the Data and Raw LDraw columns are wrapped in Text(...) so bracketed content (JSON arrays, [tag]-style comment text) displays literally instead of going through Rich markup parsing.
  • _instruction_selection fallback syncs state — the defensive fallback to step 1 now updates _selected_instruction_step to match what it returns.

New tests cover the two gaps flagged in review: [/] clamping at section bounds, and loading a broken file while in Instructions mode.

Validation

  • uv run pytest (77 passed)
  • uv run ruff check . / uv run ruff format --check src tests
  • uv run ty check src tests
  • uv run pyrefly check src tests
  • No snapshot changes — rendered output for all snapshotted views is identical

🤖 Generated with Claude Code


Summary by cubic

Fixes the instruction browser: error loads now fully reset to whole-model mode, directive cells render literally, and step labels are stabilized with cached counts to avoid crashes and extra work. Also reuses precomputed occurrences in stats to cut recomputation.

  • Bug Fixes

    • On load errors, fully exit Instructions mode: reset to whole-model key, clear submodel selector, sync mode select, and hide instruction tabs.
    • Render directive Data/Raw cells with Text so bracketed content displays literally.
    • Stabilize step labels; guard against cumulative occurrence drift and sync _selected_instruction_step in the fallback.
  • Refactors

    • Cache per-section step occurrence counts; reuse across renders and clear on rebuild/error.
    • Pass counts into InstructionDetails.show_step and reuse precomputed model occurrences in the stats panel to avoid extra expansions.
    • Use typed query_one(expect_type=...) lookups.

Written for commit a5d3e3f. Summary will update on new commits.

Review in cubic

- Reset to whole-model mode when a load fails so the error state never
  strands the instructions UI with empty selectors
- Cache per-section step occurrence counts and guard the step-label
  length against the cumulative expansion, replacing the per-render
  quadratic recomputation and the crash-on-drift strict zip
- Pass occurrence counts into InstructionDetails instead of re-expanding
  submodels twice more per render
- Wrap directive Data/Raw cells in Text so bracketed content renders
  literally instead of being parsed as Rich markup
- Sync the selected step number in the _instruction_selection fallback
- Cover step-key clamping and error loads from instructions mode

Co-Authored-By: Claude Fable 5 <[email protected]>
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: df8d12c2-1e82-4a73-9a78-a7bb31ffe949

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.26%. Comparing base (5f31a3e) to head (321a02b).

Files with missing lines Patch % Lines
src/pyldraw3_tui/screens/model.py 86.36% 3 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           pyldraw-updates      #21      +/-   ##
===================================================
+ Coverage            87.16%   87.26%   +0.10%     
===================================================
  Files                   30       30              
  Lines                 1441     1461      +20     
===================================================
+ Hits                  1256     1275      +19     
- Misses                 185      186       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Reuse precomputed model occurrences for statistics rendering, use named Textual query type arguments, and annotate the new instruction snapshot test.

Model: GPT-5
Thread: 019fc2f8-b490-78e2-9b0b-5b3c9e9c95d8
Reset the selected whole-model key and remove options from the visible submodel selector whenever opening a model fails. Cover the complete error-state reset in the pilot test.

Model: GPT-5
Thread: 019fc31e-3c39-7a52-9dc1-e7787f276f5e
@hbmartin
hbmartin changed the base branch from pyldraw-updates to main August 2, 2026 15:57
@hbmartin
hbmartin merged commit 2c7d5e7 into main Aug 2, 2026
8 checks passed
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.

1 participant