refactor(desktop): single-source result-kind CSV spec + reviewed Batch 10 plan#80
Merged
Conversation
…dedup) The (kind -> CSV base headers + export filename) mapping was hardcoded 2-4x per kind — once in each _show_*_results mixin (first render) and again in _refresh_display_format (reformat). Changing a header meant editing both or the reformat path would silently emit stale headers. Introduce app_desktop/result_csv_spec.py (a leaf module, no app_desktop imports, so window.py and the mixins it composes can both import it without a cycle) with result_csv_headers(kind)/result_csv_filename(kind). Route the extrapolation and statistics first-render paths and the extrapolation/statistics/fit_single reformat branches through it. The statistics dynamic *_unit column extension and error/snapshot dynamic-header paths are unchanged. This is the narrow, low-risk dedup — NOT the larger _refresh_display_format polymorphic refactor, which was consciously declined (result kinds are not added frequently and the 11 branches diverge too much for a clean abstraction). New test asserts the spec values, that headers are a fresh mutable copy, and (via getsource) that neither path re-hardcodes the literals — RED when a literal is re-inlined. ruff clean; 122 desktop tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rial review Staged plan (no code) for decomposing the 3197-line ExtrapolationWindow god-class via the proven window_fitting_mixin.py shim pattern. Revised per external adversarial review (Codex/gpt-5.5) which flagged real gaps: - Stage 2 (extrapolation mixin) NO-GO as written: it's a cross-family run controller (run_calculation dispatches stats/fitting/root; owns unit collectors), not extrapolation-only. Re-scope + park. - Stage 1 CONDITIONAL: _on_stats_mode_change lives in window.py, not the stats mixin — keep it out of the split. - Stage 0 expanded: MRO/provider-order snapshots, shim-override checks (the fitting shim is NOT empty — it overrides _on_fit_finished), no-__init__ checks, and enumerate direct-import sites of internal helpers (tests import them). - Stage 4 (Protocol facade) NO-GO for Batch 10: 182 owner.* names; do as narrow per-view typing spikes later. Revised order: 0 -> 3 (latex, cleanest) -> 1 (stats, small PRs) -> 2 (re-scoped). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new ChangesCSV Spec Centralization
Estimated code review effort: 2 (Simple) | ~15 minutes Window Decomposition Plan Document
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant Window as ExtrapolationWindow / Mixin
participant Spec as result_csv_spec
Window->>Spec: result_csv_headers(kind)
Spec-->>Window: fresh header list
Window->>Spec: result_csv_filename(kind)
Spec-->>Window: suggested filename
Window->>Window: build CSV rows and enable export
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…l review) Second external review (Antigravity/Gemini) verdict REJECT — agreed with all Codex findings and added three verified hazards: - Qt MRO severance: QMainWindow is the leftmost base (window.py:467); PySide6 C++ wrappers don't cooperatively super(), so a split mixin overriding a Qt event handler (closeEvent/resizeEvent/__init__) is silently ignored. Stage 0 must forbid Qt-event overrides in mixins. - Precedence reversal: splitting a monolith bottom->top and composing Shim(A_top, B, C_bottom) reverses shadowing (A shadows C under left-to-right MRO). Stage 0 must prohibit duplicate method names across siblings. - Stage 3: _TectonicInstallWorker (:55) and _LatexCompileWorker (:135) are defined inside window_latex_pdf_mixin.py while all other workers live in workers_qt.py — extract them for consistency. All re-verified against the code. Plan remains a plan (no code executed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app_desktop/window.py (1)
2932-2945: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReformat path drops the unit-column / multi-column header extension that the first-render paths still apply.
result_csv_headers("statistics")only returns the base 4 columns. Butwindow_statistics_mixin._display_statistics_resultextends these withvalue_unit/uncertainty_unitwhen present, and_display_statistics_batchesadditionally prepends"column"for multi-value-column runs plus the same unit extension. This reformat code drops both extensions, so ifcsv_rowscontain a"column"key (multi-column stats) orvalue_unit/uncertainty_unitkeys (units-enabled stats),self._csv_headerswon't match the row keys._export_csv_data'scsv.DictWriteruses the defaultextrasaction="raise", so exporting CSV after a reformat (e.g. toggling display digits/scientific notation) on these result kinds will raise and surface an "Export failed" dialog.This is exactly the first-render/reformat divergence the new
result_csv_specmodule (and its accompanying test) is meant to eliminate — but the migration missed porting over the extension logic, only the base header/filename literals.🐛 Proposed fix to restore the extension logic
elif kind == "statistics_single": text, csv_rows = self._format_statistics_display(**payload) self._set_result_text(text) if csv_rows: - self._set_csv_data(csv_rows, result_csv_headers("statistics"), suggestion=result_csv_filename("statistics")) + headers = result_csv_headers("statistics") + if any("value_unit" in row or "uncertainty_unit" in row for row in csv_rows): + headers.extend(["value_unit", "uncertainty_unit"]) + self._set_csv_data(csv_rows, headers, suggestion=result_csv_filename("statistics")) else: self._reset_csv_data() elif kind == "statistics_batches": text, csv_rows = self._format_statistics_batches_display(**payload) self._set_result_text(text) if csv_rows: - self._set_csv_data(csv_rows, result_csv_headers("statistics"), suggestion=result_csv_filename("statistics")) + multi_column = any("column" in row for row in csv_rows) + headers = (["column"] + result_csv_headers("statistics")) if multi_column else result_csv_headers("statistics") + if any("value_unit" in row or "uncertainty_unit" in row for row in csv_rows): + headers.extend(["value_unit", "uncertainty_unit"]) + self._set_csv_data(csv_rows, headers, suggestion=result_csv_filename("statistics")) else: self._reset_csv_data()Consider also adding a regression test alongside
test_reformat_and_first_render_read_the_same_specthat exercises the multi-column / units-enabled cases, since the current test only checks the filename-literal string, not header/row-key consistency.🤖 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 `@app_desktop/window.py` around lines 2932 - 2945, Reformat handling for statistics results is using only the base CSV headers, which breaks multi-column and units-enabled exports. Update the `statistics_single` and `statistics_batches` branches in `window.py` to use the same `result_csv_spec`/header-extension logic as the first-render paths in `window_statistics_mixin._display_statistics_result` and `_display_statistics_batches`, so `column`, `value_unit`, and `uncertainty_unit` are included when present. Make sure the headers passed to `_set_csv_data` always match the keys in `csv_rows`, and add a regression test covering reformat after multi-column and units-enabled statistics output.
🧹 Nitpick comments (1)
tests/test_result_csv_spec.py (1)
35-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a behavioral test in addition to the source-text check.
The current test only proves that a filename literal isn't hardcoded in the reformat path's source — it can't catch a case where the reformat path calls the accessor but drops surrounding logic (e.g. header extension for
value_unit/uncertainty_unit, or the multi-column"column"prefix), which is exactly what happened inwindow.py'sstatistics_single/statistics_batchesbranches (see review comment there). A follow-up test that drives_refresh_display_formatend-to-end with a multi-column or units-enabled statistics payload and assertsself._csv_headersmatches the keys inself._csv_rowswould close this gap.🤖 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_result_csv_spec.py` around lines 35 - 60, The current test only checks source text in _refresh_display_format and can miss behavioral regressions where result_csv_filename() is used but CSV header construction is still wrong. Add an end-to-end assertion for ExtrapolationWindow._refresh_display_format that feeds a statistics payload exercising the statistics_single/statistics_batches path, then verifies self._csv_headers matches the keys actually produced in self._csv_rows, including the multi-column "column" prefix and any value_unit/uncertainty_unit header suffix handling. Keep the existing source checks in test_reformat_and_first_render_read_the_same_spec, but extend the test to validate the real output shape from _refresh_display_format rather than only inspecting window_mod source.
🤖 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.
Outside diff comments:
In `@app_desktop/window.py`:
- Around line 2932-2945: Reformat handling for statistics results is using only
the base CSV headers, which breaks multi-column and units-enabled exports.
Update the `statistics_single` and `statistics_batches` branches in `window.py`
to use the same `result_csv_spec`/header-extension logic as the first-render
paths in `window_statistics_mixin._display_statistics_result` and
`_display_statistics_batches`, so `column`, `value_unit`, and `uncertainty_unit`
are included when present. Make sure the headers passed to `_set_csv_data`
always match the keys in `csv_rows`, and add a regression test covering reformat
after multi-column and units-enabled statistics output.
---
Nitpick comments:
In `@tests/test_result_csv_spec.py`:
- Around line 35-60: The current test only checks source text in
_refresh_display_format and can miss behavioral regressions where
result_csv_filename() is used but CSV header construction is still wrong. Add an
end-to-end assertion for ExtrapolationWindow._refresh_display_format that feeds
a statistics payload exercising the statistics_single/statistics_batches path,
then verifies self._csv_headers matches the keys actually produced in
self._csv_rows, including the multi-column "column" prefix and any
value_unit/uncertainty_unit header suffix handling. Keep the existing source
checks in test_reformat_and_first_render_read_the_same_spec, but extend the test
to validate the real output shape from _refresh_display_format rather than only
inspecting window_mod source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: edc1d011-916f-4539-b732-a5979bebb1d4
📒 Files selected for processing (6)
app_desktop/result_csv_spec.pyapp_desktop/window.pyapp_desktop/window_extrapolation_mixin.pyapp_desktop/window_statistics_mixin.pydocs/BATCH10_WINDOW_DECOMPOSITION_PLAN.mdtests/test_result_csv_spec.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Result-kind CSV dedup + reviewed Batch 10 plan
Two related follow-ups from the R3 review's DECIDE + Batch 10 items.
(c) CSV-spec dedup — the small, low-risk fix
The
(kind → CSV base headers + export filename)mapping was hardcoded 2–4× perkind — once in each
_show_*_resultsmixin (first render) and again in_refresh_display_format(reformat). A header change had to be made in both orthe reformat path would silently emit stale headers.
app_desktop/result_csv_spec.py(result_csv_headers(kind)/result_csv_filename(kind)), imported bywindow.pyand the mixins itcomposes (no circular dep).
extrapolation/statistics/fit_single reformat branches through it. Dynamic
header paths (error
output_unit, statistics*_unit, snapshot kinds) unchanged.getsource) thatneither path re-hardcodes the literals — RED when a literal is re-inlined.
Deliberately NOT done: the larger
_refresh_display_formatpolymorphicrefactor — declined because result kinds aren't added frequently and the 11
branches diverge too much for a clean abstraction (surgical-changes principle).
(b) Batch 10 decomposition plan — revised after adversarial review
docs/BATCH10_WINDOW_DECOMPOSITION_PLAN.md(plan only, no code). Reviewed byCodex (gpt-5.5); every finding independently re-verified against the code:
run_calculationdispatches statistics (
:278/390) and fitting (:472); it's a cross-familyrun controller, not extrapolation-only. Re-scope + park.
_on_stats_mode_changeis inwindow.py:1329, notthe stats mixin; keep it out of the split.
overrides
_on_fit_finished,window_fitting_mixin.py:59), no-__init__checks, and enumerate direct internal-helper imports (tests import
_statistics_raw_table_preserving_cellsfrom the mixin,test_desktop_statistics_ui.py:706).owner.*names.(Gemini CLI was deauthorized this session — "migrate to Antigravity" — so the
second external model was unavailable; the primary model's independent
re-verification stands in.)
Test plan
ruffclean; no circular import.result-workflows) pass.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation