Feature/public user - #158
Open
SucramRekoob wants to merge 25 commits into
Open
Conversation
…perly have the Public User gain access to experiments set to public.
SucramRekoob
added
enhancement
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Public User mode: experiments gain an ChangesPublic User Feature
Robustness & Minor Fixes
Sequence Diagram(s)sequenceDiagram
participant Launcher as Launcher
participant FS as FileSystem
participant Other as OtherUsers
participant Public as users/Public/experiments
participant JSON as recent_experiments.json
Launcher->>FS: ensure_public_user_exists()
FS-->>Launcher: Public dirs created/confirmed
Launcher->>Other: scan users/*/experiments for .nexp
loop per .nexp
Other->>Other: read .nexp JSON
alt experiment.is_public == true
Other->>Public: copy file as <owner>__<stem>.nexp
end
end
Public->>JSON: rewrite recent_experiments.json with current set
JSON-->>Launcher: recents updated
sequenceDiagram
participant User as EndUser
participant UI as MainWindow
participant Guard as ReadOnlyGuard
participant Persist as Persistence
User->>UI: Attempt Save
alt in Public mode
UI->>Guard: ensure write actions disabled
Guard-->>UI: action blocked
UI-->>User: show info dialog / no save
else
UI->>Persist: write .nexp
Persist-->>UI: file written
UI-->>User: save succeeded
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.claude/worktrees/brave-gates-32b4f5 (1)
1-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove accidental worktree metadata file from the PR.
This file appears to be local VCS/worktree artifact content (including a personal email at Line 2), not product code for the Public User feature. It should not ship in
main; please drop this file from the PR and add the test asset through a normal tracked path only if truly required.🤖 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 @.claude/worktrees/brave-gates-32b4f5 around lines 1 - 10, The PR includes an accidental worktree metadata file (.claude/worktrees/brave-gates-32b4f5) and a binary test archive (GETTING_STARTED/TEST_IMAGES.zip) that should not be committed; remove the .claude/worktrees/* entry and the added TEST_IMAGES.zip from the commit history/PR, revert or delete the file additions from the branch (use git rm --cached or amend/reset the commit), force-push or update the branch so the worktree metadata and personal email are not present, and if the test images are needed add them later under a proper tracked path and .gitattributes/.gitignore rules to prevent committing local VCS artifacts.src/ui/main_window.py (2)
1443-1466:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
_exit_experimentsaves without a Public User mode check
closeEvent(line 894) and_close_experiment(line 1413) both guard their save calls withnot self._is_public_user_mode(), but_exit_experimentdoes not. The backstop inExperimentManager.save_experimentsilently returnsFalse, so no actual write happens — but the inconsistency means any future change removing the backstop would introduce a write regression here.🛡️ Proposed fix
- if self.current_experiment_path: + if self.current_experiment_path and not self._is_public_user_mode(): try: self.manager.save_experiment(self.experiment, self.current_experiment_path) except Exception: pass🤖 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/ui/main_window.py` around lines 1443 - 1466, _exit_experiment currently saves the experiment unconditionally; mirror the other guards (closeEvent and _close_experiment) by checking self._is_public_user_mode() and skipping the save when in public user mode. Specifically, before calling self.manager.save_experiment(self.experiment, self.current_experiment_path) in _exit_experiment, add a conditional that only invokes save_experiment when not self._is_public_user_mode(); keep the existing flush/sync/capture calls as-is and maintain the try/except behavior around manager.save_experiment.
281-360:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
refresh_controlscan re-enable write actions after_apply_public_user_restrictions
_apply_public_user_restrictionsdisables_action_open_stack,_action_align_images, etc. via plainsetEnabled(False). BecauseQActionis not aQWidget,ReadOnlyGuardcannot be installed on it. Immediately after restrictions are applied,_auto_load_experiment_datacallsworkflow_manager.refresh_state(), which emitsstate_changed, which calls therefresh_controlsclosure wired in_init_workflow_bindings. For an experiment whose persistedcurrent_stepisLOAD_IMAGESorALIGN_IMAGES(possible if the owner shared it mid-workflow),refresh_controlsunconditionally calls_action_open_stack.setEnabled(True)or_action_align_images.setEnabled(True), overriding the Public-mode restriction.The same race occurs in
_reload_workbench_after_startup_choice(_sync_public_user_mode()then_auto_load_experiment_data()).Functionality is ultimately protected by the per-method guards, but the menu items appear incorrectly enabled, violating the stated UI contract for Public Users.
🛡️ Proposed fix — guard action enables in `refresh_controls`
Inside the
refresh_controlsclosure in_init_workflow_bindings, wrap the actionsetEnabledcalls with a public-mode check:# Step 1: Load Image Stack enable_load = current == WorkflowStep.LOAD_IMAGES - if self._action_open_stack is not None: - self._action_open_stack.setEnabled(enable_load) + if self._action_open_stack is not None and not self._is_public_user_mode(): + self._action_open_stack.setEnabled(enable_load) # ... # Step 4: Align Images enable_align = current == WorkflowStep.ALIGN_IMAGES - if self._action_align_images is not None: - self._action_align_images.setEnabled(enable_align) + if self._action_align_images is not None and not self._is_public_user_mode(): + self._action_align_images.setEnabled(enable_align)Also applies to: 362-453
🤖 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/ui/main_window.py` around lines 281 - 360, refresh_controls can re-enable actions disabled by _apply_public_user_restrictions because it unconditionally calls setEnabled on QActions; update refresh_controls so any setEnabled(True) for actions (_action_open_stack, _action_align_images, etc.) first checks the public-user mode and skips re-enabling when in public mode. Concretely, wrap calls like self._action_open_stack.setEnabled(enable_load) and self._action_align_images.setEnabled(enable_align) with a guard that returns False or leaves the action disabled if the window is in public mode (use the existing public-mode flag/method used by _apply_public_user_restrictions, e.g. self._public_user_mode or self._is_public_user()), and apply the same guard for other QAction enables in refresh_controls.src/ui/startup_dialog.py (1)
219-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
new_btn/load_btnenabled state is stale after a user switch
new_btnandload_btnare local variables in__init__; there is no stored reference to update them. When_open_user_account_popupswitches to the Public User and updates_public_modeon line 283, the two buttons remain visually enabled. The method-level guards in_start_new/_load_existingprevent any actual operation, but the UI is misleading.🛡️ Proposed fix
- new_btn = QPushButton("Start New Experiment") - new_btn.setProperty("class", "tab-action") - load_btn = QPushButton("Load Existing Experiment") - load_btn.setProperty("class", "tab-action") + self._new_btn = QPushButton("Start New Experiment") + self._new_btn.setProperty("class", "tab-action") + self._load_btn = QPushButton("Load Existing Experiment") + self._load_btn.setProperty("class", "tab-action") if self._public_mode: - new_btn.setEnabled(False) - new_btn.setToolTip("The Public User cannot create experiments.") - load_btn.setEnabled(False) - load_btn.setToolTip("The Public User cannot load arbitrary experiment files.") + self._new_btn.setEnabled(False) + self._new_btn.setToolTip("The Public User cannot create experiments.") + self._load_btn.setEnabled(False) + self._load_btn.setToolTip("The Public User cannot load arbitrary experiment files.") - new_btn.clicked.connect(self._start_new) - load_btn.clicked.connect(self._load_existing) + self._new_btn.clicked.connect(self._start_new) + self._load_btn.clicked.connect(self._load_existing)Then in
_open_user_account_popup, afterself._public_mode = is_public_user(...):+ self._new_btn.setEnabled(not self._public_mode) + self._load_btn.setEnabled(not self._public_mode) + if self._public_mode: + self._new_btn.setToolTip("The Public User cannot create experiments.") + self._load_btn.setToolTip("The Public User cannot load arbitrary experiment files.") + else: + self._new_btn.setToolTip("") + self._load_btn.setToolTip("")Also update
layout.addWidget(new_btn)/layout.addWidget(load_btn)to useself._new_btn/self._load_btn.Also applies to: 274-286
🤖 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/ui/startup_dialog.py` around lines 219 - 228, The buttons new_btn and load_btn are created as local variables in __init__ so their enabled/toolTip state isn't updated when _open_user_account_popup flips _public_mode; change them to instance attributes (self._new_btn and self._load_btn), update layout.addWidget calls to use these attributes, and wherever you setEnabled/setToolTip for the public mode use self._new_btn and self._load_btn; this lets _open_user_account_popup (after setting self._public_mode = is_public_user(...)) update the UI state (call setEnabled/setToolTip) to reflect the new mode and keeps the existing method-level guards in _start_new / _load_existing.
🧹 Nitpick comments (1)
src/ui/public_user_dialog.py (1)
175-227: ⚡ Quick winRemove
register_public_experimentandunregister_public_experimentor document as reservedBoth functions are listed in the module's Public API docstring but are never imported or called anywhere in the codebase. All visibility toggling uses
sync_public_experiments()directly. Shipping dead code that parallels these functions adds maintenance burden — if future callers useregister_public_experiment, they would updaterecent_experiments.jsonwithout copying the file, leaving the Public folder inconsistent.Either remove these functions or add a note that they are reserved for incremental-update optimizations in future work.
🤖 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/ui/public_user_dialog.py` around lines 175 - 227, The module contains two unused public API functions register_public_experiment and unregister_public_experiment; either delete them or explicitly mark them as reserved to avoid shipping dead/unsafe code. Remove both function definitions (and update the module docstring to stop exporting them) OR keep them but add a clear docstring/reservation comment above register_public_experiment and unregister_public_experiment stating they are reserved for future incremental-update optimizations and must not be used by callers (and consider prefixing with an underscore or adding them to __all__ with a “reserved” note). Ensure any references in the module-level Public API docstring are updated to match the chosen action so callers and maintainers aren’t misled.
🤖 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/core/experiment_manager.py`:
- Around line 345-354: The backstop that prevents writes into the Public User
directory silently falls through on any exception during path resolution; update
the try/except around Path(file_path).resolve() and
_public_experiments_dir().resolve() so that on any exception you explicitly
block the save by returning False (rather than pass), keeping the check that if
pub_dir in target.parents returns False; reference the existing symbols Path,
file_path, target, pub_dir and the _public_experiments_dir() call so the change
is applied in that exact guard block.
In `@src/main.py`:
- Around line 56-59: The code currently calls
is_public_user(user_dialog.selected_user_experiments_dir.parent.name) which
dereferences selected_user_experiments_dir (typed Path | None) and risks a None
dereference; instead use the existing string field user_dialog.selected_user:
change the check to is_public_user(user_dialog.selected_user) and keep the call
to sync_public_experiments() unchanged (look for selected_user_experiments_dir,
selected_user, is_public_user, and sync_public_experiments to locate the code).
In `@src/ui/public_user_dialog.py`:
- Around line 125-152: The current loop uses rglob("*.nexp") and builds
dest_name as f"{user_dir.name}__{nexp_file.stem}.nexp", which can collide when
the same user has multiple files with the same stem in different subfolders;
change the dest naming logic in this loop to incorporate the file's relative
sub-path (e.g., nexp_file.relative_to(exp_dir).with_suffix("").as_posix() or
join parents) into dest_name so it becomes collision-safe (for example include
the relative parent path parts between owner and stem), then use that new
dest_name when creating dest and copying via shutil.copy2; also ensure
keep_names and recent_entries use the resulting dest_name/path so no entries are
lost.
In `@src/ui/workflow.py`:
- Around line 461-464: The read-only locking misses _skip_align_button and
_align_button, allowing public users to call _on_skip_alignment_clicked which
calls complete_current_step() and mutates workflow state; update the read-only
enable path where _next_button and self._step_buttons are locked to also call
_lock(self._skip_align_button) and _lock(self._align_button), and add a guard at
the start of _on_skip_alignment_clicked (check self._read_only_enabled or
equivalent public-mode flag) to early-return when read-only to prevent calling
complete_current_step().
---
Outside diff comments:
In @.claude/worktrees/brave-gates-32b4f5:
- Around line 1-10: The PR includes an accidental worktree metadata file
(.claude/worktrees/brave-gates-32b4f5) and a binary test archive
(GETTING_STARTED/TEST_IMAGES.zip) that should not be committed; remove the
.claude/worktrees/* entry and the added TEST_IMAGES.zip from the commit
history/PR, revert or delete the file additions from the branch (use git rm
--cached or amend/reset the commit), force-push or update the branch so the
worktree metadata and personal email are not present, and if the test images are
needed add them later under a proper tracked path and .gitattributes/.gitignore
rules to prevent committing local VCS artifacts.
In `@src/ui/main_window.py`:
- Around line 1443-1466: _exit_experiment currently saves the experiment
unconditionally; mirror the other guards (closeEvent and _close_experiment) by
checking self._is_public_user_mode() and skipping the save when in public user
mode. Specifically, before calling self.manager.save_experiment(self.experiment,
self.current_experiment_path) in _exit_experiment, add a conditional that only
invokes save_experiment when not self._is_public_user_mode(); keep the existing
flush/sync/capture calls as-is and maintain the try/except behavior around
manager.save_experiment.
- Around line 281-360: refresh_controls can re-enable actions disabled by
_apply_public_user_restrictions because it unconditionally calls setEnabled on
QActions; update refresh_controls so any setEnabled(True) for actions
(_action_open_stack, _action_align_images, etc.) first checks the public-user
mode and skips re-enabling when in public mode. Concretely, wrap calls like
self._action_open_stack.setEnabled(enable_load) and
self._action_align_images.setEnabled(enable_align) with a guard that returns
False or leaves the action disabled if the window is in public mode (use the
existing public-mode flag/method used by _apply_public_user_restrictions, e.g.
self._public_user_mode or self._is_public_user()), and apply the same guard for
other QAction enables in refresh_controls.
In `@src/ui/startup_dialog.py`:
- Around line 219-228: The buttons new_btn and load_btn are created as local
variables in __init__ so their enabled/toolTip state isn't updated when
_open_user_account_popup flips _public_mode; change them to instance attributes
(self._new_btn and self._load_btn), update layout.addWidget calls to use these
attributes, and wherever you setEnabled/setToolTip for the public mode use
self._new_btn and self._load_btn; this lets _open_user_account_popup (after
setting self._public_mode = is_public_user(...)) update the UI state (call
setEnabled/setToolTip) to reflect the new mode and keeps the existing
method-level guards in _start_new / _load_existing.
---
Nitpick comments:
In `@src/ui/public_user_dialog.py`:
- Around line 175-227: The module contains two unused public API functions
register_public_experiment and unregister_public_experiment; either delete them
or explicitly mark them as reserved to avoid shipping dead/unsafe code. Remove
both function definitions (and update the module docstring to stop exporting
them) OR keep them but add a clear docstring/reservation comment above
register_public_experiment and unregister_public_experiment stating they are
reserved for future incremental-update optimizations and must not be used by
callers (and consider prefixing with an underscore or adding them to __all__
with a “reserved” note). Ensure any references in the module-level Public API
docstring are updated to match the chosen action so callers and maintainers
aren’t misled.
🪄 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
Run ID: a620f9a5-61a5-4e96-b274-db00dc1e604f
📒 Files selected for processing (9)
.claude/worktrees/brave-gates-32b4f5pyproject.tomlsrc/core/experiment_manager.pysrc/main.pysrc/ui/main_window.pysrc/ui/public_user_dialog.pysrc/ui/startup_dialog.pysrc/ui/user_selection_dialog.pysrc/ui/workflow.py
Comment on lines
+461
to
+464
| for btn in self._step_buttons.values(): | ||
| _lock(btn) | ||
| _lock(self._next_button) | ||
| self._read_only_enabled = True |
There was a problem hiding this comment.
_skip_align_button and _align_button are not locked in read-only mode
_next_button and the step buttons are locked, but _skip_align_button is omitted. _on_skip_alignment_clicked has no public-mode guard — a Public User at the ALIGN_IMAGES step can invoke complete_current_step(), mutating in-memory workflow state and advancing the stepper even though saves are blocked at the backstop.
🛡️ Proposed fix
for btn in self._step_buttons.values():
_lock(btn)
_lock(self._next_button)
+ _lock(self._align_button)
+ _lock(self._skip_align_button)
self._read_only_enabled = True🤖 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/ui/workflow.py` around lines 461 - 464, The read-only locking misses
_skip_align_button and _align_button, allowing public users to call
_on_skip_alignment_clicked which calls complete_current_step() and mutates
workflow state; update the read-only enable path where _next_button and
self._step_buttons are locked to also call _lock(self._skip_align_button) and
_lock(self._align_button), and add a guard at the start of
_on_skip_alignment_clicked (check self._read_only_enabled or equivalent
public-mode flag) to early-return when read-only to prevent calling
complete_current_step().
…ck ton of user tests to the code base to bring code coverage above 80%
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
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 (2)
src/ui/startup_dialog.py (1)
237-249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winButton enabled-state is not refreshed when the user switches profiles.
new_btnandload_btnare local variables in__init__with no instance reference;_open_user_account_popupupdatesself._public_modebut has no way to update those buttons. As a result:
- Public → Normal switch: both action buttons remain disabled (visually locked out even though
_public_modeis nowFalse).- Normal → Public switch: both buttons remain enabled. The runtime guards in
_start_new/_load_existingstill block the actions with an info box, so no functional bypass occurs — but the UX contract is broken.🐛 Proposed fix — store buttons as instance attributes and refresh on switch
In
__init__, store the buttons:- new_btn = QPushButton("Start New Experiment") - new_btn.setProperty("class", "tab-action") - load_btn = QPushButton("Load Existing Experiment") - load_btn.setProperty("class", "tab-action") + self._new_btn = QPushButton("Start New Experiment") + self._new_btn.setProperty("class", "tab-action") + self._load_btn = QPushButton("Load Existing Experiment") + self._load_btn.setProperty("class", "tab-action")Add a helper and call it:
def _apply_public_mode_ui(self) -> None: self._new_btn.setEnabled(not self._public_mode) self._load_btn.setEnabled(not self._public_mode) tip = "The Public User cannot {action}." self._new_btn.setToolTip(tip.format(action="create experiments") if self._public_mode else "") self._load_btn.setToolTip(tip.format(action="load arbitrary experiment files") if self._public_mode else "")Call it in
__init__(replacing the currentif self._public_mode:block) and at the end of_open_user_account_popupafter updatingself._public_mode.Also applies to: 300-304
🤖 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/ui/startup_dialog.py` around lines 237 - 249, The Start/Load buttons are local to __init__ so they don't update when self._public_mode changes; convert new_btn and load_btn into instance attributes (e.g., self._new_btn, self._load_btn), replace the current one-time if self._public_mode: block with a call to a new helper _apply_public_mode_ui that sets enabled state and tooltips based on self._public_mode, and call _apply_public_mode_ui both in __init__ (after creating the buttons) and at the end of _open_user_account_popup after updating self._public_mode so the UI refreshes on profile switches; reference the button symbols self._new_btn, self._load_btn and the helper name _apply_public_mode_ui when making changes.src/ui/user_selection_dialog.py (1)
472-499:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
_confirm_new_usershould explicitly blockPUBLIC_USER_NAMEas a profile name.
FileExistsErroronly fires if the public user directory already exists. On first launch (before the sync hook has created the public folder), a user could create a regular profile namedPUBLIC_USER_NAME, which would then be indistinguishable from the managed public profile and could have its deletion button shown.🛡️ Proposed fix
+from ui.public_user_dialog import PUBLIC_USER_NAME def _confirm_new_user(self) -> None: name = self._name_input.text().strip() if not name: self._name_input.setFocus() return + if name == PUBLIC_USER_NAME: + QMessageBox.warning(self, "Invalid Name", f'"{PUBLIC_USER_NAME}" is a reserved profile name.') + return if name in {".", ".."}:🤖 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/ui/user_selection_dialog.py` around lines 472 - 499, In _confirm_new_user, add an explicit check that the entered name equals the reserved PUBLIC_USER_NAME (use the PUBLIC_USER_NAME constant) and treat it as invalid: show a QMessageBox.warning (e.g., "Invalid Name" with a message that the name is reserved) and return before attempting to create the directory; keep the existing validation order so this check runs after empty/illegal-character checks but before calling _experiments_dir_for_user and mkdir to prevent creating a regular profile that collides with the managed public profile.
🧹 Nitpick comments (9)
tests/test_settings_dialog.py (1)
171-199: ⚡ Quick winNo test coverage for the cancelled-dialog (invalid
QColor) path.Every new test only patches
getColorwith a validQColor. When a user dismisses the color picker,QColorDialog.getColorreturns an invalidQColor(isValid() == False). If the production handler doesn't guard against this, it will silently store a corrupt hex string (e.g.,"#000000"fromQColor().name()). Even if the guard exists in production code, the branch is completely unexercised by the test suite.✅ Suggested additional tests (one per handler)
def test_pick_roi_color_ignores_invalid_choice(self, app): invalid = QColor() # isValid() == False with patch.object(QColorDialog, "getColor", return_value=invalid): d = SettingsDialog() original = d._roi_colors["roi_1"] d._pick_roi_color("roi_1") assert d._roi_colors["roi_1"] == original def test_pick_avg_traj_color_ignores_invalid_choice(self, app): invalid = QColor() with patch.object(QColorDialog, "getColor", return_value=invalid): d = SettingsDialog() original = d._avg_traj_color d._pick_avg_traj_color() assert d._avg_traj_color == original def test_pick_peak_color_ignores_invalid_choice(self, app): invalid = QColor() with patch.object(QColorDialog, "getColor", return_value=invalid): d = SettingsDialog() original = d._peak_marker_color d._pick_peak_color() assert d._peak_marker_color == original🤖 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_settings_dialog.py` around lines 171 - 199, Tests only cover valid QColor returns from QColorDialog.getColor; add tests that patch getColor to return an invalid QColor() (isValid() == False) and assert each handler (_pick_roi_color, _pick_avg_traj_color, _pick_avg_roi_color, _pick_peak_color, _pick_trough_color) leaves the corresponding stored value unchanged (e.g., _roi_colors["roi_1"], _avg_traj_color, _avg_traj_roi_colors["roi_1"], _peak_marker_color, _trough_marker_color). For each handler create a test that captures the original value, invokes the handler with the invalid color, and asserts the original value remains. Ensure you patch QColorDialog.getColor to return QColor() in each new test.tests/test_image_viewer_lru_cache.py (2)
24-30: ⚡ Quick win
test_lru_cache_set_replaces_existing_key_without_evictiondoesn't verify recency update on overwrite.After
c.set(0, np.array([10])), key 0 should be MRU and a subsequent capacity-exceeding insert should evict key 1, not key 0. The current test only checks both keys are present after the overwrite, so an implementation that does not promote key 0 to MRU on update would still pass.✅ Suggested extension
assert c.get(0)[0] == 10 assert c.get(1) is not None + # After overwriting key 0, it should be MRU; inserting key 2 must evict key 1. + c.set(2, np.array([20])) + assert c.get(1) is None, "key 1 should have been evicted as LRU after key 0 was overwritten" + assert c.get(0) is not None + assert c.get(2) is not None🤖 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_image_viewer_lru_cache.py` around lines 24 - 30, The test test_lru_cache_set_replaces_existing_key_without_eviction should also assert that overwriting an existing key updates its recency: after using _LRUCache.set(0, ...) to overwrite key 0, insert a new key to exceed capacity and assert that key 1 (the older key) is evicted while key 0 remains; update the assertions to use _LRUCache.get to confirm key0 is MRU (still present and has value 10) and key1 is None after the capacity-exceeding insert.
39-40: ⚡ Quick win
np.allcloseon a potentiallyNonereturn raisesTypeErrorinstead of a clean assertion failure.If the LRU implementation has a bug and
getreturnsNonefor key 1 or key 2,np.allclose(None, ...)raises aTypeError, masking the real failure. Guard with an explicitis not Noneassertion first.🛡️ Proposed fix
- assert np.allclose(c.get(1), [1.0]) - assert np.allclose(c.get(2), [2.0]) + val1 = c.get(1) + assert val1 is not None + assert np.allclose(val1, [1.0]) + val2 = c.get(2) + assert val2 is not None + assert np.allclose(val2, [2.0])🤖 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_image_viewer_lru_cache.py` around lines 39 - 40, The test uses np.allclose directly on c.get(1) and c.get(2) which will raise TypeError if get returns None; update the assertions to first assert that c.get(1) is not None and c.get(2) is not None before calling np.allclose so failures produce clear assertion errors; locate the checks around c.get(...) in the test (references: c.get) and insert explicit is not None assertions immediately before the np.allclose calls.tests/test_data_analyzer.py (3)
87-90: 💤 Low valueStub-freezing test — verify these are placeholders, not regressions.
Asserting
time_series_analysis(...) == {}andcorrelation_analysis(...) == {}will pin the empty-dict behavior in place. If these methods are intentional stubs (as the file summary suggests), that's fine for coverage; if they're meant to be implemented later, this test will mask the moment they start returning real data and turn a real implementation PR into a test failure that looks like a regression. Consider adding a# TODO: replace once <method> is implementedcomment so future readers know the assertion is intentionally locking the stub contract.🤖 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_data_analyzer.py` around lines 87 - 90, The test is intentionally asserting stub return values for time_series_analysis and correlation_analysis which will freeze those stubs; update the test to document this intent by adding a TODO comment clarifying the assertion is a temporary placeholder (e.g. "TODO: replace once time_series_analysis is implemented" and "TODO: replace once correlation_analysis is implemented") next to the two asserts in test_time_series_and_correlation_stubs so future contributors know these are deliberate stub checks rather than permanent behavior checks.
100-104: 💤 Low valueGood coverage of the degenerate-rect path; consider also exercising
roi_height=0.The
roi_width=0case is covered here. Since the legacy path likely guards on either dimension being non-positive, a symmetric case (roi_width=2, roi_height=0) — or a negative dimension — would close the obvious branch gap with one more line.🤖 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_data_analyzer.py` around lines 100 - 104, Add a symmetric test that exercises the degenerate rectangle when roi_height is zero: create an analyzer via _make_analyzer(), use frames = np.zeros((2, 4, 4), dtype=np.float32) and call analyzer.extract_roi_intensity_time_series(frames, roi=None, roi_x=2, roi_y=2, roi_width=2, roi_height=0), then assert the returned time series equals [0.0, 0.0]; name the test something like test_extract_roi_intensity_legacy_empty_rect_height_returns_zeros to mirror the existing width-zero test and cover the other branch.
72-77: 💤 Low valuePlot assertion is shallow.
assert fig1 is not None and fig2 is not Noneonly verifies a truthy return; it won't catch silent regressions like producing the wrong plot type or a figure with no axes/data. Consider asserting onfig.axes, the axes' artist counts (e.g.,len(ax.patches)for hist,len(ax.lines)for line), or the plot title/label to actually distinguish"hist"vs"line"paths.🤖 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_data_analyzer.py` around lines 72 - 77, Update the shallow assertions in test_generate_plots_hist_and_line to validate plot contents returned by analyzer.generate_plots instead of just non-None: for the hist plot (fig1) assert fig1.axes is non-empty, inspect the primary axis (e.g., ax = fig1.axes[0]) and assert len(ax.patches) or another histogram-specific artist count is > 0 (or that a distinguishing title/label matches "hist"); for the line plot (fig2) assert fig2.axes is non-empty and that len(fig2.axes[0].lines) > 0 (or check title/label matches "line") so the test verifies the correct plot type and that data was drawn.src/core/image_processor.py (1)
669-679: ⚡ Quick winUse
np.nanmeanto avoid spuriously excluding well-correlated neurons.The
errstatecorrectly suppresses the warning, butnp.meanstill propagates any NaN produced by a zero-variance trajectory through the entire row. A neuron that has high correlation with every other neuron except one constant-signal neighbour will getmean_correlations[i] = NaN, which fails> correlation_thresholdand is incorrectly excluded.♻️ Proposed fix
- mean_correlations = np.zeros(len(present_indices), dtype=np.float32) - for i in range(len(present_indices)): - other_correlations = np.delete(correlation_matrix[i], i) - mean_correlations[i] = np.mean(other_correlations) + mean_correlations = np.zeros(len(present_indices), dtype=np.float32) + for i in range(len(present_indices)): + other_correlations = np.delete(correlation_matrix[i], i) + mean_correlations[i] = np.nanmean(other_correlations)🤖 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/core/image_processor.py` around lines 669 - 679, The current loop uses np.mean on rows of correlation_matrix which produces NaN when a row contains NaNs (from zero-variance trajectories), causing correctly correlated neurons to be excluded; update the computation of mean_correlations to use np.nanmean instead of np.mean (operate on other_correlations when computing mean_correlations[i]) so NaNs are ignored, keep dtype=np.float32, and then apply the existing quality_mask[present_indices] = mean_correlations > correlation_threshold check unchanged; reference symbols: present_indices, neuron_trajectories, correlation_matrix, mean_correlations, quality_mask, correlation_threshold.tests/test_main_launcher.py (1)
36-39: ⚡ Quick winUse
PUBLIC_USER_NAMEconstant instead of hardcoded"Public"in test stubs.This keeps tests aligned with production behavior if the canonical public username changes.
Suggested patch
+from ui.public_user_dialog import PUBLIC_USER_NAME @@ - selected_user = "Public" + selected_user = PUBLIC_USER_NAMEAlso applies to: 82-85
🤖 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_main_launcher.py` around lines 36 - 39, Replace hardcoded "Public" with the canonical PUBLIC_USER_NAME constant in the test stub class _UserDlg (update selected_user = "Public" to use PUBLIC_USER_NAME) and make the same change in the other stub instance around the 82-85 area; ensure the test imports or references the PUBLIC_USER_NAME symbol used in production so tests follow the canonical public username.tests/test_roi_selection_dialog.py (1)
62-75: ⚡ Quick winStrengthen smoke-style tests with one observable assertion each.
These tests currently validate mostly “no exception” execution. Please add at least one concrete state/output assertion per test so regressions to silent no-ops are caught reliably.
Also applies to: 78-89, 139-144, 146-175
🤖 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_roi_selection_dialog.py` around lines 62 - 75, The test test_roi_dialog_other_roi_polygon_overlay currently only runs methods for side-effects; add a concrete observable assertion after calling dlg._update_overlay() (before dlg.close()) to verify visible state such as dlg.overlay is not None or that dlg.overlay contains the expected polygon/ROI (e.g., check that an overlay item count > 0 or that the ROI polygon points match the ROI instance), using the ROISelectionDialog instance and the other ROI object; apply the same pattern to the other listed tests (the ones around lines 78-89, 139-144, 146-175) to assert a meaningful property (overlay presence, shown flag, item count, or stored ROI coordinates) so each smoke test has one explicit assertion.
🤖 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/ui/startup_dialog.py`:
- Around line 54-62: The code uses Path(path).stem which fails for Windows-style
backslash paths on Linux; normalize separators before calling Path(...).stem by
converting backslashes to forward slashes (e.g. use Path(path.replace("\\",
"/")).stem) wherever Path(path).stem is used (the name fallback and the owner
extraction logic that references stem and the "__" split), so both the name =
... fallback and the owner = ... logic operate on normalized paths.
In `@tests/test_alignment_worker.py`:
- Around line 542-556: The test test_run_uint8_stack_denormalizes_to_uint8 uses
a transform mock that returns a 4x4 frame (mock_sr.transform.return_value =
np.zeros((4,4), ...)) while the input stack frames are 2x2, which can hide shape
bugs in AlignmentWorker; update the mock_sr.transform.return_value to match the
input frame shape (e.g., np.zeros((2,2), dtype=np.float64)) and add an explicit
assertion after run() that aligned.shape == stack.shape to ensure the alignment
preserves frame dimensions.
In `@tests/test_data_analyzer.py`:
- Around line 80-84: Update the test_save_results_to_experiment_appends_run test
to assert the actual run was appended, not just truthiness: after calling
analyzer.save_results_to_experiment(exp) verify exp.analysis_results["runs"] has
length 1 and that its first element matches the expected appended entry (e.g.,
the placeholder analysis dict produced by save_results_to_experiment). Use the
Experiment instance and analyzer.save_results_to_experiment call already in the
test and assert both len(exp.analysis_results["runs"]) == 1 and the contents of
exp.analysis_results["runs"][0] equal the expected dict.
In `@tests/test_public_user_feature.py`:
- Around line 333-336: The test helper _patch_open_fail_recent_write currently
only treats mode == "w" as write which misses modes like "a", "x", or modes
containing "+" (e.g., "r+","w+b"); update the condition in
_patch_open_fail_recent_write to detect any write-capable mode (e.g., if mode
contains 'w' or 'a' or 'x' or '+' / use set intersection with {'w','a','x','+'})
while keeping the existing file-name check for "recent_experiments.json", so any
open call for that file with a write-capable mode raises OSError.
In `@tests/test_workflow.py`:
- Around line 321-325: The helper _advance_workflow_to currently loops until
wm.current_step == target and can hang; modify it to bound the loop with a
max-iterations guard (e.g., max_steps or max_attempts) and fail fast if exceeded
by raising an AssertionError or using pytest.fail with a clear message. Keep use
of wm.mark_step_ready(wm.current_step) and assert wm.complete_current_step() is
True inside the loop, increment a counter each iteration, and if counter > max
then raise/pytest.fail referencing WorkflowManager/wm.current_step and the
target step to make the failure explicit.
---
Outside diff comments:
In `@src/ui/startup_dialog.py`:
- Around line 237-249: The Start/Load buttons are local to __init__ so they
don't update when self._public_mode changes; convert new_btn and load_btn into
instance attributes (e.g., self._new_btn, self._load_btn), replace the current
one-time if self._public_mode: block with a call to a new helper
_apply_public_mode_ui that sets enabled state and tooltips based on
self._public_mode, and call _apply_public_mode_ui both in __init__ (after
creating the buttons) and at the end of _open_user_account_popup after updating
self._public_mode so the UI refreshes on profile switches; reference the button
symbols self._new_btn, self._load_btn and the helper name _apply_public_mode_ui
when making changes.
In `@src/ui/user_selection_dialog.py`:
- Around line 472-499: In _confirm_new_user, add an explicit check that the
entered name equals the reserved PUBLIC_USER_NAME (use the PUBLIC_USER_NAME
constant) and treat it as invalid: show a QMessageBox.warning (e.g., "Invalid
Name" with a message that the name is reserved) and return before attempting to
create the directory; keep the existing validation order so this check runs
after empty/illegal-character checks but before calling
_experiments_dir_for_user and mkdir to prevent creating a regular profile that
collides with the managed public profile.
---
Nitpick comments:
In `@src/core/image_processor.py`:
- Around line 669-679: The current loop uses np.mean on rows of
correlation_matrix which produces NaN when a row contains NaNs (from
zero-variance trajectories), causing correctly correlated neurons to be
excluded; update the computation of mean_correlations to use np.nanmean instead
of np.mean (operate on other_correlations when computing mean_correlations[i])
so NaNs are ignored, keep dtype=np.float32, and then apply the existing
quality_mask[present_indices] = mean_correlations > correlation_threshold check
unchanged; reference symbols: present_indices, neuron_trajectories,
correlation_matrix, mean_correlations, quality_mask, correlation_threshold.
In `@tests/test_data_analyzer.py`:
- Around line 87-90: The test is intentionally asserting stub return values for
time_series_analysis and correlation_analysis which will freeze those stubs;
update the test to document this intent by adding a TODO comment clarifying the
assertion is a temporary placeholder (e.g. "TODO: replace once
time_series_analysis is implemented" and "TODO: replace once
correlation_analysis is implemented") next to the two asserts in
test_time_series_and_correlation_stubs so future contributors know these are
deliberate stub checks rather than permanent behavior checks.
- Around line 100-104: Add a symmetric test that exercises the degenerate
rectangle when roi_height is zero: create an analyzer via _make_analyzer(), use
frames = np.zeros((2, 4, 4), dtype=np.float32) and call
analyzer.extract_roi_intensity_time_series(frames, roi=None, roi_x=2, roi_y=2,
roi_width=2, roi_height=0), then assert the returned time series equals [0.0,
0.0]; name the test something like
test_extract_roi_intensity_legacy_empty_rect_height_returns_zeros to mirror the
existing width-zero test and cover the other branch.
- Around line 72-77: Update the shallow assertions in
test_generate_plots_hist_and_line to validate plot contents returned by
analyzer.generate_plots instead of just non-None: for the hist plot (fig1)
assert fig1.axes is non-empty, inspect the primary axis (e.g., ax =
fig1.axes[0]) and assert len(ax.patches) or another histogram-specific artist
count is > 0 (or that a distinguishing title/label matches "hist"); for the line
plot (fig2) assert fig2.axes is non-empty and that len(fig2.axes[0].lines) > 0
(or check title/label matches "line") so the test verifies the correct plot type
and that data was drawn.
In `@tests/test_image_viewer_lru_cache.py`:
- Around line 24-30: The test
test_lru_cache_set_replaces_existing_key_without_eviction should also assert
that overwriting an existing key updates its recency: after using
_LRUCache.set(0, ...) to overwrite key 0, insert a new key to exceed capacity
and assert that key 1 (the older key) is evicted while key 0 remains; update the
assertions to use _LRUCache.get to confirm key0 is MRU (still present and has
value 10) and key1 is None after the capacity-exceeding insert.
- Around line 39-40: The test uses np.allclose directly on c.get(1) and c.get(2)
which will raise TypeError if get returns None; update the assertions to first
assert that c.get(1) is not None and c.get(2) is not None before calling
np.allclose so failures produce clear assertion errors; locate the checks around
c.get(...) in the test (references: c.get) and insert explicit is not None
assertions immediately before the np.allclose calls.
In `@tests/test_main_launcher.py`:
- Around line 36-39: Replace hardcoded "Public" with the canonical
PUBLIC_USER_NAME constant in the test stub class _UserDlg (update selected_user
= "Public" to use PUBLIC_USER_NAME) and make the same change in the other stub
instance around the 82-85 area; ensure the test imports or references the
PUBLIC_USER_NAME symbol used in production so tests follow the canonical public
username.
In `@tests/test_roi_selection_dialog.py`:
- Around line 62-75: The test test_roi_dialog_other_roi_polygon_overlay
currently only runs methods for side-effects; add a concrete observable
assertion after calling dlg._update_overlay() (before dlg.close()) to verify
visible state such as dlg.overlay is not None or that dlg.overlay contains the
expected polygon/ROI (e.g., check that an overlay item count > 0 or that the ROI
polygon points match the ROI instance), using the ROISelectionDialog instance
and the other ROI object; apply the same pattern to the other listed tests (the
ones around lines 78-89, 139-144, 146-175) to assert a meaningful property
(overlay presence, shown flag, item count, or stored ROI coordinates) so each
smoke test has one explicit assertion.
In `@tests/test_settings_dialog.py`:
- Around line 171-199: Tests only cover valid QColor returns from
QColorDialog.getColor; add tests that patch getColor to return an invalid
QColor() (isValid() == False) and assert each handler (_pick_roi_color,
_pick_avg_traj_color, _pick_avg_roi_color, _pick_peak_color, _pick_trough_color)
leaves the corresponding stored value unchanged (e.g., _roi_colors["roi_1"],
_avg_traj_color, _avg_traj_roi_colors["roi_1"], _peak_marker_color,
_trough_marker_color). For each handler create a test that captures the original
value, invokes the handler with the invalid color, and asserts the original
value remains. Ensure you patch QColorDialog.getColor to return QColor() in each
new test.
🪄 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
Run ID: 8d5b8559-6889-42df-a8cd-dd69bb362371
📒 Files selected for processing (24)
pyproject.tomlsrc/core/image_processor.pysrc/ui/neuron_trajectory_plot.pysrc/ui/public_user_dialog.pysrc/ui/startup_dialog.pysrc/ui/user_selection_dialog.pytests/test_alignment_worker.pytests/test_circular_stats.pytests/test_data_analyzer.pytests/test_experiment_manager.pytests/test_file_handler.pytests/test_help_widgets.pytests/test_image_viewer_lru_cache.pytests/test_lomb_scargle_core.pytests/test_main_launcher.pytests/test_main_window_autoload.pytests/test_neuron_detection_widget.pytests/test_public_user_feature.pytests/test_roi_selection_dialog.pytests/test_settings_dialog.pytests/test_startup_dialog.pytests/test_styles.pytests/test_user_selection.pytests/test_workflow.py
✅ Files skipped from review due to trivial changes (1)
- src/ui/neuron_trajectory_plot.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- src/ui/public_user_dialog.py
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
| for widget, guard in self._read_only_guards: | ||
| try: | ||
| widget.removeEventFilter(guard) # type: ignore[arg-type] | ||
| except Exception: |
| try: | ||
| guard = ReadOnlyGuard.lock(w) | ||
| self._read_only_guards.append((w, guard)) | ||
| except Exception: |
| ) -> None: | ||
| """Large stacks use the process pool when not frozen (mocked executor).""" | ||
| monkeypatch.setattr(alignment_worker, "_FROZEN", False) | ||
| monkeypatch.setattr(alignment_worker, "as_completed", lambda pending: list(pending)) |
| ) -> None: | ||
| """Parallel registration supports ``reference='mean'`` (mean stack projection).""" | ||
| monkeypatch.setattr(alignment_worker, "_FROZEN", False) | ||
| monkeypatch.setattr(alignment_worker, "as_completed", lambda pending: list(pending)) |
| ) -> None: | ||
| """``shutdown(..., cancel_futures=True)`` falls back on older executor APIs.""" | ||
| monkeypatch.setattr(alignment_worker, "_FROZEN", False) | ||
| monkeypatch.setattr(alignment_worker, "as_completed", lambda pending: list(pending)) |
| monkeypatch.setattr("main.MainWindow", lambda *a, **k: mw) | ||
| monkeypatch.setattr("main.get_theme", lambda: "light") | ||
| monkeypatch.setattr("main.get_stylesheet", lambda _theme: "") | ||
| monkeypatch.setattr("main.QTimer", lambda: MagicMock()) |
| monkeypatch.setattr("main.MainWindow", lambda *a, **k: mw) | ||
| monkeypatch.setattr("main.get_theme", lambda: "light") | ||
| monkeypatch.setattr("main.get_stylesheet", lambda _theme: "") | ||
| monkeypatch.setattr("main.QTimer", lambda: MagicMock()) |
…ed and making experiments public or private.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ui/main_window.py`:
- Around line 1021-1022: The UI-only restrictions are insufficient because
non-UI persistence paths still call save_experiment; update the shared save
logic so that persistence is blocked for Public sessions by either (a) adding a
guard at the top of save_experiment() to return early when
_is_public_user_mode() is true, or (b) adding checks in each caller
(_save_workflow_progress(), _open_user_account_popup(), _exit_experiment()) to
avoid invoking save_experiment() when _is_public_user_mode() is true; modify the
implementation in main_window.py accordingly and ensure
_apply_public_user_restrictions() remains for UI tweaks but does not replace the
required persistence gating.
- Around line 363-385: Make public-user locking idempotent by changing
_apply_public_user_restrictions (and its inner _lock_widget) to skip widgets
already recorded in self._public_user_guards instead of appending duplicates;
before calling ReadOnlyGuard.lock(w) check whether any(entry[0] is w for entry
in self._public_user_guards) (or maintain a set of locked widgets) and only call
ReadOnlyGuard.lock and append (w, guard, was_enabled) when the widget is not
already present so _clear_public_user_restrictions can correctly restore
original was_enabled values.
- Around line 684-718: The _toggle_experiment_visibility method sets
self.experiment.is_public before calling manager.save_experiment but does not
revert it if the save fails; update the logic to capture the previous value
(e.g., prev = self.experiment.is_public), attempt the save, and on any Exception
set self.experiment.is_public back to prev and log the failure; only call
sync_public_experiments() when the save succeeds (do not run sync if rollback
occurred) and then call self._update_visibility_button() to reflect the final
in-memory state.
🪄 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
Run ID: 22fee184-bc29-460f-a59d-b97e3ec9f781
📒 Files selected for processing (4)
src/main.pysrc/ui/main_window.pysrc/ui/public_user_dialog.pysrc/ui/startup_dialog.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/startup_dialog.py
- src/ui/public_user_dialog.py
- src/main.py
Comment on lines
+1021
to
+1022
| # Apply read-only restrictions when running as the Public User | ||
| self._apply_public_user_restrictions() |
There was a problem hiding this comment.
UI locks alone do not make the Public session read-only.
Applying restrictions here still leaves non-UI persistence paths active. save_experiment() is still reachable from _save_workflow_progress(), _open_user_account_popup(), and _exit_experiment() without checking _is_public_user_mode(), so a Public session can still write experiment state just by loading, switching users, or using File → Exit. Please gate the shared save paths as well, not only the visible actions.
🤖 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/ui/main_window.py` around lines 1021 - 1022, The UI-only restrictions are
insufficient because non-UI persistence paths still call save_experiment; update
the shared save logic so that persistence is blocked for Public sessions by
either (a) adding a guard at the top of save_experiment() to return early when
_is_public_user_mode() is true, or (b) adding checks in each caller
(_save_workflow_progress(), _open_user_account_popup(), _exit_experiment()) to
avoid invoking save_experiment() when _is_public_user_mode() is true; modify the
implementation in main_window.py accordingly and ensure
_apply_public_user_restrictions() remains for UI tweaks but does not replace the
required persistence gating.
| try: | ||
| with open(recent_file, "w", encoding="utf-8") as f: | ||
| json.dump(data, f, indent=2) | ||
| except OSError: |
| try: | ||
| with open(recent_file, "w", encoding="utf-8") as f: | ||
| json.dump(data, f, indent=2) | ||
| except OSError: |
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
1 participant
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.
Implements the Public User feature. The Public User only has access to experiments that are labeled as public by normal users and the Public User only has read access to given experiments so it can not edit experiment data.
Summary by CodeRabbit
New Features
Behavior Changes
Tests / Assets