fix: persistence/UI audit findings (F3/F10/F12)#76
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR clamps extrapolation output segments to the surviving row count after dropping degenerate rows, adds dual-language warning messages for skipped/failed extrapolation rows, restores fitting log-axis checkbox state during workspace reload, documents mode-switch result preservation, and adjusts an example workspace timeout test. ChangesExtrapolation Segment Clamping and Warnings
Workspace Log-Axis Restore and Mode Switch Documentation
Example Workspace Timeout Adjustment
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
F3 (shared/extrapolation_engine.py, datalab_core/extrapolation.py) — rows that fail to extrapolate were dropped silently (the skip only printed under verbose, never appended to warnings) and the payload's segments still referenced the original row count. Always append a bilingual warning on a dropped row, and clamp segments to the surviving row count (_clamp_segments) so indices never reference a discarded row. F10 (app_desktop/window.py) — switching modes did not clear the previous mode's result text / CSV export buffer / plot, so stale wrong-mode data could be shown or exported. Clear it on mode change, guarded against workspace-restore and an in-flight worker. F12 (app_desktop/workspace_controller.py) — fitting log_axes (log-x/log-y) was captured on save but never restored, so a reloaded workspace re-ran fits with linear axes. Restore it in _restore_workspace_contents (add a _set_checked_if helper). TDD: dropped-row warning + segment clamp; mode-switch clears result; log_axes round-trip. Neighborhood regression green (148 tests).
…upe warning
Review follow-ups on the F3 dropped-row / segment-clamp fix:
- _clamp_segments caught (TypeError, ValueError, IndexError) but not KeyError,
so a mapping-shaped segment entry ({"start": 0}) would crash run_extrapolation
on seg[0]. Add KeyError to the skip set and cover it with a direct unit test
(RED without the fix: KeyError: 0).
- The two "too few values" drop-row branches emitted the identical bilingual
warning inline; consolidate into one local helper so the dual-language string
is defined once and the two guards stay in sync.
ruff clean; 159 tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…10 vs #70) The first F10 pass cleared the previous mode's result inside _on_mode_change, but that defeated release-gate contract #70 (test_run_validation_error_preserves_previous_valid_result_overview): switching mode then hitting a validation error left "No results" because the result was already wiped at switch time, so there was nothing to preserve. Reconcile by moving the clear off the switch: a successful run in the new mode replaces the result (so stale wrong-mode output is never shown/exported once you actually compute — F10's real intent, satisfied via the _on_calc_finished -> _show_*_results path), while a failed or validation-errored run returns early and keeps the last good result (#70). Rewrite F10's own test to assert the new timing (switch preserves; a successful run replaces). Both previously-conflicting tests pass; 221 neighborhood tests pass; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yilibinbin
force-pushed
the
fix/audit-r2-persistence-ui
branch
from
a8a2fc7 to
8b04e6e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app_desktop/workspace_controller.py`:
- Around line 1968-1973: The `log_axes` restore logic in `restore_workspace`
leaves stale checkbox state because `_set_checked_if` is only called when the
saved dict contains a value, so missing `x`/`y` entries never clear the reused
`window` state. Update the `log_axes` handling to explicitly set both
`log_x_checkbox` and `log_y_checkbox` every time `restore_workspace` runs,
treating absent or non-dict `fitting.get("log_axes")` as false for both. This
should align the restored UI with saved workspace data when
`open_workspace`/`open_workspace_path` reuses the same window instance.
- Around line 128-133: Remove the duplicated _set_checked_if helper from
workspace_controller so there is only one definition in the module; the later
copy near the WorkspaceController methods is shadowing the earlier top-level
helper and should be deleted. Keep the shared implementation in the original
_set_checked_if function and update any local calls to use that existing helper
rather than redefining it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1638951a-6927-4a3e-bff2-95cde8751e55
📒 Files selected for processing (7)
app_desktop/window.pyapp_desktop/workspace_controller.pydatalab_core/extrapolation.pyshared/extrapolation_engine.pytests/test_datalab_core_extrapolation.pytests/test_desktop_mode_stack.pytests/test_workspace_controller.py
| def _set_checked_if(window: Any, attr: str, value: Any) -> None: | ||
| widget = getattr(window, attr, None) | ||
| if widget is not None and value is not None and hasattr(widget, "setChecked"): | ||
| widget.setChecked(bool(value)) | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether _set_checked_if is defined more than once in this file.
rg -n "^def _set_checked_if" app_desktop/workspace_controller.pyRepository: yilibinbin/DataLab
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=app_desktop/workspace_controller.py
echo "== around first definition =="
sed -n '120,145p' "$file"
echo
echo "== around second definition =="
sed -n '730,750p' "$file"
echo
echo "== exact matches =="
rg -n "^def _set_checked_if" "$file"Repository: yilibinbin/DataLab
Length of output: 2244
Remove the duplicate _set_checked_if helper app_desktop/workspace_controller.py:739-742 repeats the earlier definition at :128-133, so the later copy shadows the first and adds dead code.
🤖 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/workspace_controller.py` around lines 128 - 133, Remove the
duplicated _set_checked_if helper from workspace_controller so there is only one
definition in the module; the later copy near the WorkspaceController methods is
shadowing the earlier top-level helper and should be deleted. Keep the shared
implementation in the original _set_checked_if function and update any local
calls to use that existing helper rather than redefining it.
| # log_axes (log-x / log-y plot axis selection) is captured on save but was | ||
| # never restored, so a reloaded workspace re-ran fits with linear axes (F12). | ||
| log_axes = fitting.get("log_axes") | ||
| if isinstance(log_axes, dict): | ||
| _set_checked_if(window, "log_x_checkbox", log_axes.get("x")) | ||
| _set_checked_if(window, "log_y_checkbox", log_axes.get("y")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing/partial log_axes leaves stale checkbox state on reused window.
When fitting.get("log_axes") is missing entirely (older workspace format) or only has one of x/y, _set_checked_if is skipped for the absent value(s) rather than explicitly unchecking. Since open_workspace/open_workspace_path reuse the same self window instance across successive opens (see _open_workspace_from_path in app_desktop/window.py, which calls restore_workspace(self, ...)), opening a legacy workspace after one with log-x/log-y checked will leave the checkboxes in their previous, now-stale, checked state instead of reflecting the (absent) saved config.
🛠️ Proposed fix to always set an explicit value
- log_axes = fitting.get("log_axes")
- if isinstance(log_axes, dict):
- _set_checked_if(window, "log_x_checkbox", log_axes.get("x"))
- _set_checked_if(window, "log_y_checkbox", log_axes.get("y"))
+ log_axes = fitting.get("log_axes") if isinstance(fitting.get("log_axes"), dict) else {}
+ _set_checked_if(window, "log_x_checkbox", bool(log_axes.get("x", False)))
+ _set_checked_if(window, "log_y_checkbox", bool(log_axes.get("y", False)))📝 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.
| # log_axes (log-x / log-y plot axis selection) is captured on save but was | |
| # never restored, so a reloaded workspace re-ran fits with linear axes (F12). | |
| log_axes = fitting.get("log_axes") | |
| if isinstance(log_axes, dict): | |
| _set_checked_if(window, "log_x_checkbox", log_axes.get("x")) | |
| _set_checked_if(window, "log_y_checkbox", log_axes.get("y")) | |
| # log_axes (log-x / log-y plot axis selection) is captured on save but was | |
| # never restored, so a reloaded workspace re-ran fits with linear axes (F12). | |
| log_axes = fitting.get("log_axes") if isinstance(fitting.get("log_axes"), dict) else {} | |
| _set_checked_if(window, "log_x_checkbox", bool(log_axes.get("x", False))) | |
| _set_checked_if(window, "log_y_checkbox", bool(log_axes.get("y", False))) |
🤖 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/workspace_controller.py` around lines 1968 - 1973, The `log_axes`
restore logic in `restore_workspace` leaves stale checkbox state because
`_set_checked_if` is only called when the saved dict contains a value, so
missing `x`/`y` entries never clear the reused `window` state. Update the
`log_axes` handling to explicitly set both `log_x_checkbox` and `log_y_checkbox`
every time `restore_workspace` runs, treating absent or non-dict
`fitting.get("log_axes")` as false for both. This should align the restored UI
with saved workspace data when `open_workspace`/`open_workspace_path` reuses the
same window instance.
The quantum-defect-implicit example runs a per-point inner root-find per seed variant (~103s locally). Shared GitHub CI runners are ~2-3x slower and run the suite concurrently (a full run measured 27m), so the 300s waitUntil budget intermittently timed out on this one case while the other 3599 tests passed — an environment-dependent timeout of a heavy pre-existing test, not a logic regression (verified: the test passes locally identically to main, and this branch's changes don't touch the implicit path). Raise the implicit budget to 600s; a genuine stall still trips it, just later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…finish The real blocker was not the test-level waitUntil but the implicit example's shipped 300s self-consistent-fit timeout: on slow shared CI runners (~2-3x slower, suite run concurrently) the fit needs >300s and self-aborts with "Self-consistent fit exceeded 300s and was stopped" -> _workbench_result_state == "failed", so the test's `assert state != "failed"` failed (raising the test-level budget only let the test wait long enough to observe that internal abort). Disable the internal cap for the implicit case in the test (implicit_timeout_spin = 0, "no auto timeout"); the qtbot.waitUntil budget still bounds the run, so a genuine hang is still caught. Verified: implicit test passes locally in ~99s with the cap disabled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Audit R2 — PR4/5: persistence / UI (F3/F10/F12)
F3 — dropped extrapolation rows vanished silently
Drop-row paths now emit bilingual "第 N 行…" warnings via
_append_option_warning. Payload segments are clamped to the surviving row count (_clamp_segments, hardened to skip mapping-shaped/malformed segment entries incl.KeyError).F10 — stale cross-mode result
A successful run in a new mode replaces the previous result (so stale wrong-mode output is never shown/exported once you compute), while a failed/validation-errored run preserves the last good result (honors release-gate contract #70). Clearing happens on successful run, not at mode-switch.
F12 — log-axes not restored
Workspace restore now reapplies
log_x/log_yplot-axis selection.Test plan
ruffclean.🤖 Generated with Claude Code
Summary by CodeRabbit