Skip to content

feat(gsps): add next-overlay navigation button and shortcut#32

Merged
timcogan merged 2 commits intomasterfrom
feat/gsps-next-overlay-navigation
Mar 7, 2026
Merged

feat(gsps): add next-overlay navigation button and shortcut#32
timcogan merged 2 commits intomasterfrom
feat/gsps-next-overlay-navigation

Conversation

@timcogan
Copy link
Owner

@timcogan timcogan commented Mar 7, 2026

Summary by CodeRabbit

  • New Features
    • GSPS overlay navigation: Click the new "Next GSPS (N)" button or press N to advance to the next GSPS target. Navigation cycles through targets within an image and across viewport groups, activating overlays and wrapping as needed.
  • UI
    • The "Next GSPS (N)" control is shown only when targets exist and triggers an immediate view update.
  • Tests
    • Added automated tests validating cycling through frames and across viewports.

@coderabbitai
Copy link

coderabbitai bot commented Mar 7, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cf9af917-8927-4512-a2ec-76aa831ea113

📥 Commits

Reviewing files that changed from the base of the PR and between 4285b0e and bdfafb2.

📒 Files selected for processing (1)
  • src/app.rs

📝 Walkthrough

Walkthrough

Adds GSPS navigation to the viewer: computes per-frame/viewport GSPS targets, cycles to the next target (keyboard and UI), updates frame/viewport state, schedules repaints, and includes unit tests for single and multi-viewport scenarios.

Changes

Cohort / File(s) Summary
GSPS Navigation Core
src/app.rs
New internal GspsNavigationTarget type; functions to compute GSPS target frames and navigation targets; selection logic (next_gsps_navigation_target); main action jump_to_next_gsps_overlay() that updates frame or MammoViewport index and triggers repaint; keybinding (N) and "Next GSPS (N)" UI button; added state (n_pressed, has_gsps_navigation_target) and input handling; two unit tests for cycling behavior.
Dicom Test Helpers
src/dicom.rs
Added DicomImage::test_stub_with_mono_frames(gsps_overlay, frame_count) to create test images with optional GSPS overlay and configurable mono frames; updated test_stub() to call the new helper.

Sequence Diagram

sequenceDiagram
    participant User as "User\n(running client)"
    participant Input as "Input Handler\n(key/UI)"
    participant Nav as "Navigation Logic\n(app.rs)"
    participant GSPS as "GSPS Data\n(dicom / overlays)"
    participant State as "Frame/Viewport State\n(app state)"
    participant UI as "Renderer\n(redraw)"

    User->>Input: Press N / Click Next GSPS
    Input->>Nav: jump_to_next_gsps_overlay()
    Nav->>GSPS: gsps_navigation_targets()
    GSPS-->>Nav: target list
    Nav->>Nav: next_gsps_navigation_target()
    Nav->>State: update frame index or viewport index
    State->>UI: schedule repaint
    UI->>User: render updated frame/overlay
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Poem

🐰
I hop through frames with gentle paws,
N-key bound, I follow overlay laws.
From viewport to frame I skip and cheer,
GSPS targets found — the path is clear! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: adding GSPS navigation support with a next-overlay button and keyboard shortcut (N key), which are the primary features implemented.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/gsps-next-overlay-navigation

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.

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/app.rs`:
- Around line 428-430: The Next GSPS action currently returns the same index
when GSPS is hidden (match arm Some(index) => index), causing the first press to
be a no-op; change the logic in the handler that computes target_index (using
symbols targets, current_target, self.gsps_overlay_visible) so that it always
advances to the next target (e.g., make the Some(index) case wrap to (index + 1)
% targets.len() instead of returning index), and add a regression test that
starts with the current target having GSPS hidden to assert that pressing "Next
GSPS (N)" advances to the next target and shows its overlay.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f858ca61-d21f-4027-b851-5477ab70763b

📥 Commits

Reviewing files that changed from the base of the PR and between 2a9b125 and 4285b0e.

📒 Files selected for processing (2)
  • src/app.rs
  • src/dicom.rs

@timcogan timcogan merged commit e83af48 into master Mar 7, 2026
7 checks passed
@timcogan timcogan deleted the feat/gsps-next-overlay-navigation branch March 7, 2026 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant