Add semantic instruction browser - #20
Conversation
Model: GPT-5 Thread: 019fc2b0-5c19-75d0-bb00-4f97e318e787
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
📝 WalkthroughWalkthroughThis change adds instruction-mode model viewing to Sequence Diagram(s)sequenceDiagram
participant Operator
participant ModelView
participant InstructionDocument
participant DisplayWidgets
Operator->>ModelView: Select Instructions and a section-local step
ModelView->>InstructionDocument: Resolve cumulative step state
InstructionDocument-->>ModelView: Occurrences, BOM data, directives, and details
ModelView->>DisplayWidgets: Update pieces, statistics, BOM, directives, and details
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Model: GPT-5 Thread: 019fc2b0-5c19-75d0-bb00-4f97e318e787
Model: GPT-5 Thread: 019fc2b0-5c19-75d0-bb00-4f97e318e787
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 86.09% 87.16% +1.07%
==========================================
Files 28 30 +2
Lines 1215 1441 +226
==========================================
+ Hits 1046 1256 +210
- Misses 169 185 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pyldraw3_tui/widgets/directives_table.py (1)
1-55: 📐 Maintainability & Code Quality | 🔴 CriticalFix the Ruff CPY001 pipeline failure before merge.
The pipeline logs report
uv run ruff check .failing with RuffCPY001— missing copyright notices in 21-22 Python files, including new files like this one. This blocks CI (exit code 1). Add the required copyright header to this file (and the other new/changed files) or adjust the Ruffflake8-copyrightconfiguration to match project convention before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pyldraw3_tui/widgets/directives_table.py` around lines 1 - 55, Add the project-standard copyright header to the module containing DirectivesTable, and apply the same header consistently to the other new or changed Python files reported by Ruff CPY001. Preserve the existing flake8-copyright configuration unless the repository convention requires updating it, and ensure uv run ruff check . passes.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pyldraw3_tui/app.py`:
- Around line 204-205: Update the query_one calls in the model-tab focus logic
in src/pyldraw3_tui/app.py (lines 204-205) to pass PieceTable using the named
expect_type argument. Apply the same change in tests/test_snapshots.py (lines
91-92), passing Select and TabbedContent via expect_type; no other behavior
should change.
In `@src/pyldraw3_tui/screens/model.py`:
- Around line 261-275: Update _show_error to reset _view_mode to the default
model mode, reset `#view-mode-select` to the matching value, and call
_sync_mode_ui() after clearing the model state. Mirror the existing reset
behavior from load_model’s success path so the instructions CSS class and
section/step selectors are removed when loading fails.
In `@src/pyldraw3_tui/widgets/instruction_details.py`:
- Line 1: Add the project-required copyright header at the top of
instruction_details.py, before the existing module docstring, using the
repository’s established Ruff-compatible copyright notice format. Leave the
module content unchanged.
In `@src/pyldraw3_tui/widgets/stats_panel.py`:
- Around line 57-61: The whole-model render computes occurrences twice. In
src/pyldraw3_tui/widgets/stats_panel.py#L57-L61, update StatsPanel.show_model to
accept precomputed Sequence[ModelOccurrence] data or remove it in favor of
show_occurrences; in src/pyldraw3_tui/screens/model.py#L297-L317, update
ModelView._render_whole_model to pass its existing occurrences to
StatsPanel.show_occurrences with self._parts and steps=len(steps), preserving
the existing _render_instruction_step behavior.
In `@tests/test_snapshots.py`:
- Around line 86-94: Add type annotations to the
test_snapshot_model_instructions function parameters make_app and
instructions_mpd using the appropriate existing fixture types, and annotate the
function’s return type as None. Keep the test behavior unchanged.
---
Outside diff comments:
In `@src/pyldraw3_tui/widgets/directives_table.py`:
- Around line 1-55: Add the project-standard copyright header to the module
containing DirectivesTable, and apply the same header consistently to the other
new or changed Python files reported by Ruff CPY001. Preserve the existing
flake8-copyright configuration unless the repository convention requires
updating it, and ensure uv run ruff check . passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 879325f1-51ba-4746-8d3b-43a4f2e34f0f
⛔ Files ignored due to path filters (5)
tests/__snapshots__/model_bom.svgis excluded by!**/*.svgtests/__snapshots__/model_instructions.svgis excluded by!**/*.svgtests/__snapshots__/model_issues.svgis excluded by!**/*.svgtests/__snapshots__/model_pieces.svgis excluded by!**/*.svguv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
README.mdpyproject.tomlsrc/pyldraw3_tui/app.pysrc/pyldraw3_tui/commands.pysrc/pyldraw3_tui/screens/model.pysrc/pyldraw3_tui/widgets/bom_table.pysrc/pyldraw3_tui/widgets/directives_table.pysrc/pyldraw3_tui/widgets/instruction_details.pysrc/pyldraw3_tui/widgets/issues_table.pysrc/pyldraw3_tui/widgets/piece_table.pysrc/pyldraw3_tui/widgets/stats_panel.pytests/conftest.pytests/fixtures/models/instructions.mpdtests/test_pilot_model.pytests/test_snapshots.py
| if tab == "model": | ||
| self.query_one("#piece-table", PieceTable).focus() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Pass query_one expected types by name.
Textual names the type-selector parameter expect_type. (textual.textualize.io)
src/pyldraw3_tui/app.py#L204-L205: useexpect_type=PieceTable.tests/test_snapshots.py#L91-L92: useexpect_type=Selectandexpect_type=TabbedContent.
As per coding guidelines, “Prefer named arguments when calling a method with more than one argument”.
📍 Affects 2 files
src/pyldraw3_tui/app.py#L204-L205(this comment)tests/test_snapshots.py#L91-L92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pyldraw3_tui/app.py` around lines 204 - 205, Update the query_one calls
in the model-tab focus logic in src/pyldraw3_tui/app.py (lines 204-205) to pass
PieceTable using the named expect_type argument. Apply the same change in
tests/test_snapshots.py (lines 91-92), passing Select and TabbedContent via
expect_type; no other behavior should change.
Source: Coding guidelines
| def _show_error(self, message: str) -> None: | ||
| self._model = None | ||
| self._instruction_document = None | ||
| self._selected_instruction_section = None | ||
| self._selected_instruction_step = 1 | ||
| self.add_class("errored") | ||
| self.query_one("#model-error", Static).update(f"[bold red]Error:[/] {message}") | ||
| self.query_one("#model-title", Static).update("No model open") | ||
| self.query_one("#piece-table", PieceTable).set_occurrences([], self._parts) | ||
| self.query_one("#stats-panel", StatsPanel).update("Model has no pieces.") | ||
| self.query_one("#instruction-details", InstructionDetails).update("") | ||
| self.query_one("#pli-table", BomTable).set_rows([], self._parts) | ||
| self.query_one("#bom-table", BomTable).set_rows([], self._parts) | ||
| self.query_one("#directives-table", DirectivesTable).set_directives([]) | ||
| self._clear_instruction_selectors() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the view mode when a model load fails.
_show_error clears the model and instruction state, but it does not reset self._view_mode, the #view-mode-select value, or call self._sync_mode_ui(). If the user was in instructions mode when a subsequent load fails, the widget keeps the instructions CSS class and the mode selector still shows "Instructions" and the section/step selectors, even though there is no model data. Reset the view mode the same way load_model's success path does.
🐛 Proposed fix to reset the view mode on error
def _show_error(self, message: str) -> None:
self._model = None
self._instruction_document = None
self._selected_instruction_section = None
self._selected_instruction_step = 1
+ self._view_mode = MODEL_MODE
self.add_class("errored")
self.query_one("`#model-error`", Static).update(f"[bold red]Error:[/] {message}")
self.query_one("`#model-title`", Static).update("No model open")
self.query_one("`#piece-table`", PieceTable).set_occurrences([], self._parts)
self.query_one("`#stats-panel`", StatsPanel).update("Model has no pieces.")
self.query_one("`#instruction-details`", InstructionDetails).update("")
self.query_one("`#pli-table`", BomTable).set_rows([], self._parts)
self.query_one("`#bom-table`", BomTable).set_rows([], self._parts)
self.query_one("`#directives-table`", DirectivesTable).set_directives([])
self._clear_instruction_selectors()
+ mode_select = self.query_one("`#view-mode-select`", Select)
+ with mode_select.prevent(Select.Changed):
+ mode_select.value = MODEL_MODE
+ self._sync_mode_ui()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _show_error(self, message: str) -> None: | |
| self._model = None | |
| self._instruction_document = None | |
| self._selected_instruction_section = None | |
| self._selected_instruction_step = 1 | |
| self.add_class("errored") | |
| self.query_one("#model-error", Static).update(f"[bold red]Error:[/] {message}") | |
| self.query_one("#model-title", Static).update("No model open") | |
| self.query_one("#piece-table", PieceTable).set_occurrences([], self._parts) | |
| self.query_one("#stats-panel", StatsPanel).update("Model has no pieces.") | |
| self.query_one("#instruction-details", InstructionDetails).update("") | |
| self.query_one("#pli-table", BomTable).set_rows([], self._parts) | |
| self.query_one("#bom-table", BomTable).set_rows([], self._parts) | |
| self.query_one("#directives-table", DirectivesTable).set_directives([]) | |
| self._clear_instruction_selectors() | |
| def _show_error(self, message: str) -> None: | |
| self._model = None | |
| self._instruction_document = None | |
| self._selected_instruction_section = None | |
| self._selected_instruction_step = 1 | |
| self._view_mode = MODEL_MODE | |
| self.add_class("errored") | |
| self.query_one("`#model-error`", Static).update(f"[bold red]Error:[/] {message}") | |
| self.query_one("`#model-title`", Static).update("No model open") | |
| self.query_one("`#piece-table`", PieceTable).set_occurrences([], self._parts) | |
| self.query_one("`#stats-panel`", StatsPanel).update("Model has no pieces.") | |
| self.query_one("`#instruction-details`", InstructionDetails).update("") | |
| self.query_one("`#pli-table`", BomTable).set_rows([], self._parts) | |
| self.query_one("`#bom-table`", BomTable).set_rows([], self._parts) | |
| self.query_one("`#directives-table`", DirectivesTable).set_directives([]) | |
| self._clear_instruction_selectors() | |
| mode_select = self.query_one("`#view-mode-select`", Select) | |
| with mode_select.prevent(Select.Changed): | |
| mode_select.value = MODEL_MODE | |
| self._sync_mode_ui() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pyldraw3_tui/screens/model.py` around lines 261 - 275, Update _show_error
to reset _view_mode to the default model mode, reset `#view-mode-select` to the
matching value, and call _sync_mode_ui() after clearing the model state. Mirror
the existing reset behavior from load_model’s success path so the instructions
CSS class and section/step selectors are removed when loading fails.
| @@ -0,0 +1,85 @@ | |||
| """Semantic state for a selected instruction step.""" | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required copyright notice.
Ruff reports CPY001 for package files. The lint job cannot pass until this file has the required header.
As per coding guidelines, use “Ruff formatter and linter style conventions”.
🧰 Tools
🪛 GitHub Actions: Lint and Test / 2_lint-test (3.13).txt
[error] 1-1: Ruff CPY001: Missing copyright notices in 21 Python files under this package. Command 'uv run ruff check .' reported 46 total errors.
🪛 GitHub Actions: Lint and Test / lint-test (3.13)
[error] 1-1: Ruff CPY001: 22 Python files under this directory are missing copyright notices at the top of the file. Command 'uv run ruff check .' failed with exit code 1.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pyldraw3_tui/widgets/instruction_details.py` at line 1, Add the
project-required copyright header at the top of instruction_details.py, before
the existing module docstring, using the repository’s established
Ruff-compatible copyright notice format. Leave the module content unchanged.
Sources: Coding guidelines, Pipeline failures
| self.show_occurrences( | ||
| tuple(model.iter_occurrences()), | ||
| parts, | ||
| steps=steps, | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid computing model occurrences twice per whole-model render. StatsPanel.show_model re-traverses the model via its own model.iter_occurrences() call, even though ModelView._render_whole_model already computed an equivalent occurrences list moments earlier. include_steps only changes whether occurrence.step is populated, not occurrence identity or count, so the second traversal is pure duplicated work on every whole-model render (submodel switch, catalog-ready, mode toggle).
src/pyldraw3_tui/widgets/stats_panel.py#L57-L61: changeshow_modelto accept a pre-computedSequence[ModelOccurrence](or drop it and let callers useshow_occurrencesdirectly, as_render_instruction_stepalready does).src/pyldraw3_tui/screens/model.py#L297-L317: update_render_whole_modelto callself.query_one("#stats-panel", StatsPanel).show_occurrences(occurrences, self._parts, steps=len(steps))with the already-computedoccurrences, instead of callingshow_model(model, self._parts, steps=len(steps)).
📍 Affects 2 files
src/pyldraw3_tui/widgets/stats_panel.py#L57-L61(this comment)src/pyldraw3_tui/screens/model.py#L297-L317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pyldraw3_tui/widgets/stats_panel.py` around lines 57 - 61, The
whole-model render computes occurrences twice. In
src/pyldraw3_tui/widgets/stats_panel.py#L57-L61, update StatsPanel.show_model to
accept precomputed Sequence[ModelOccurrence] data or remove it in favor of
show_occurrences; in src/pyldraw3_tui/screens/model.py#L297-L317, update
ModelView._render_whole_model to pass its existing occurrences to
StatsPanel.show_occurrences with self._parts and steps=len(steps), preserving
the existing _render_instruction_step behavior.
| async def test_snapshot_model_instructions(make_app, instructions_mpd): | ||
| app = make_app(model_path=instructions_mpd) | ||
| async with app.run_test(size=SIZE) as pilot: | ||
| await wait_for_catalog(app, pilot) | ||
| await pilot.press("i") | ||
| app.query_one("#instruction-step-select", Select).value = 3 | ||
| app.query_one("#model-tabs", TabbedContent).active = "tab-summary" | ||
| await pilot.pause() | ||
| _check(app, "model_instructions") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add type hints to the new test.
Annotate make_app, instructions_mpd, and the None return value.
As per coding guidelines, “Always use or add type hints”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_snapshots.py` around lines 86 - 94, Add type annotations to the
test_snapshot_model_instructions function parameters make_app and
instructions_mpd using the appropriate existing fixture types, and annotate the
function’s return type as None. Keep the test behavior unchanged.
Source: Coding guidelines
1 participant
Summary
User impact
Users can inspect renderer-neutral building instructions without changing existing whole-model browsing behavior. Embedded
.datdependencies remain inventory items, and unsupported directives remain raw and lossless.Validation
uv run pytest(75 passed)uv run ruff check .uv run ruff format --check src testsuv run ty check src testsuv run pyrefly check src testsSummary by cubic
Add a semantic instruction browser to the Model tab so you can navigate section-local steps and view renderer-neutral instruction state (rotation, camera, LPub PLI/BOM, callouts, directives). Diagnostics now combine LDraw and instruction issues, and the app updates to
0.3.0withpyldraw3>=1.5.0.New Features
Dependencies
pyldraw3>=1.5.0.0.3.0.CPY001(copyright rule).Written for commit 5f31a3e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes