Skip to content

fix(hotkeyBindings): consolidate Escape hotkey behavior for contour drawing tools#6104

Open
jbocce wants to merge 7 commits into
OHIF:masterfrom
jbocce:fix/OHIF-2640-contour-cancellation
Open

fix(hotkeyBindings): consolidate Escape hotkey behavior for contour drawing tools#6104
jbocce wants to merge 7 commits into
OHIF:masterfrom
jbocce:fix/OHIF-2640-contour-cancellation

Conversation

@jbocce

@jbocce jbocce commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Context

Fixes #6079

Pressing Esc while drawing a contour (Spline, Livewire, PlanarFreehand) did not cancel the in-progress annotation. Two separate commands were bound to the same esc key — cancelMeasurement and rejectPreview — but Mousetrap keeps only one handler per key, so the second binding silently shadowed the first and the cancel effectively did nothing.

Changes & Results

  • Added a single generic cancelActiveOperation command that orchestrates both single-purpose commands (rejectPreview + cancelMeasurement). Each is a no-op
    when its state isn't active, so running both is safe and order-independent.
  • Added a cancelMeasurement command that calls cornerstone's cancelActiveManipulations on the active viewport, cancelling any in-progress annotation
    drawing.
  • Bound esc to the new cancelActiveOperation command in hotkeyBindings.ts and removed the conflicting duplicate esc binding.
  • Added data-cy to the tool-settings select control and its options so the Spline Type dropdown is testable.
  • Extended RightPanelPageObject with contour-segmentation helpers (addSegmentation, and the splineContour/livewireContour/freehandContour tools incl.
    spline-type switching).
  • Added migration-guide note.

Result: a single Esc press now reliably discards whatever is in progress — a segmentation preview or an annotation being drawn.

Testing

Added Playwright E2E coverage asserting Esc cancels an in-progress drawing for each affected tool:

  • Measurement ROIs: Spline.spec.ts, Livewire.spec.ts
  • Contour segmentation: CatmullRomSplineContourSegmentation.spec.ts, LinearSplineContourSegmentation.spec.ts, BSplineContourSegmentation.spec.ts,
    LivewireContourSegmentation.spec.ts

Manual check:

  1. Open a study in Segmentation mode and add a Contour segmentation.
  2. Activate a contour tool (Spline / Livewire / Freehand) and place a few points.
  3. Press Esc → the in-progress contour is discarded; the tool stays active and can draw again.
  4. Repeat with a measurement Spline/Livewire ROI in viewer mode.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

System:
OS: Windows 11 10.0.26200
CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12700H
Memory: 8.83 GB / 31.68 GB
Binaries:
Node: 24.15.0 - C:\Users\joebo\AppData\Local\fnm_multishells\50148_1782198425052\node.EXE
Yarn: 1.22.22 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
npm: 11.12.1 - C:\Users\joebo\AppData\Local\fnm_multishells\50148_1782198425052\npm.CMD
pnpm: 11.5.2 - C:\Users\joebo\AppData\Local\pnpm\bin\pnpm.CMD
bun: 1.2.23 - C:\Users\joebo.bun\bin\bun.EXE
Browsers:
Chrome: 149.0.7827.115
Edge: Chromium (149.0.4022.80)
Internet Explorer: 11.0.26100.8115

Summary by CodeRabbit

  • New Features
    • Pressing Escape now triggers a single consolidated Cancel action to stop any in-progress annotation/segmentation work and discard active previews.
  • Bug Fixes
    • Resolved default hotkey shadowing so Escape consistently cancels active interactions (including spline, livewire, and freehand contour workflows).
  • Documentation
    • Updated the migration guide to describe the consolidated Escape behavior from earlier versions.
  • Tests
    • Added Playwright coverage for Escape cancellation across multiple spline variants and livewire contour/annotation flows.

@netlify

netlify Bot commented Jun 24, 2026

Copy link
Copy Markdown

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 97fc385
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6a3cc07eb9d76d0008e795e3
😎 Deploy Preview https://deploy-preview-6104--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 396381eb-5bfd-437b-b8a6-4219ee11866b

📥 Commits

Reviewing files that changed from the base of the PR and between 4c27a16 and 96c10f7.

📒 Files selected for processing (1)
  • extensions/cornerstone/src/initMeasurementService.ts

📝 Walkthrough

Walkthrough

Adds cancelMeasurement and cancelActiveOperation, consolidates the Escape hotkey, extends contour test helpers, adds Escape cancellation coverage for contour tools, and updates the migration guide.

Changes

Escape cancellation for contour interactions

Layer / File(s) Summary
Cancellation commands
extensions/cornerstone/src/commandsModule.ts, extensions/cornerstone/src/initMeasurementService.ts
Adds cancelMeasurement to cancel active manipulations on the enabled viewport element, adds cancelActiveOperation to run rejectPreview before cancellation, and routes measurement removal through the new command.
Escape hotkey binding
platform/core/src/defaults/hotkeyBindings.ts
Replaces the separate Escape bindings for cancelMeasurement and rejectPreview with a single cancelActiveOperation binding labeled Cancel.
Tool settings and contour page-object helpers
platform/ui-next/src/components/OHIFToolSettings/ToolSettings.tsx, tests/pages/RightPanelPageObject.ts
Adds data-cy hooks to the tool settings select and items, and extends the contour segmentation page object with segmentation creation plus spline, livewire, and freehand contour helpers.
Escape cancellation Playwright coverage
tests/Spline.spec.ts, tests/Livewire.spec.ts, tests/BSplineContourSegmentation.spec.ts, tests/CatmullRomSplineContourSegmentation.spec.ts, tests/LinearSplineContourSegmentation.spec.ts, tests/LivewireContourSegmentation.spec.ts
Adds Playwright coverage for Escape cancellation of in-progress spline, livewire, B-spline, Catmull-Rom, and linear contour interactions, plus the existing spline and livewire annotation cases.
Migration guide for Escape hotkey changes
platform/docs/docs/migration-guide/3p12-to-3p13/escape-cancel-hotkey.md
Documents the 3.12 shadowing issue, the 3.13 cancelActiveOperation binding, migration steps for custom hotkey bindings, and the reason for consolidating Escape handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 Hop, hop — the Escape key found one true path,
No more shadowed bindings doing quiet math.
Contours vanish, previews bow, then back to play,
A tidy rabbit hop makes room for a calmer day.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, semantic-release styled, and accurately summarizes the Escape hotkey consolidation.
Description check ✅ Passed The PR description matches the template with context, changes, testing, checklist, and a linked issue reference.
Linked Issues check ✅ Passed The Escape hotkey now routes through cancelActiveOperation/cancelMeasurement, matching the contour-cancel behavior requested in #6079.
Out of Scope Changes check ✅ Passed The docs, page-object, data-cy, and tests all support the hotkey fix and related E2E coverage, so no clear out-of-scope changes stand out.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@jbocce jbocce marked this pull request as draft June 24, 2026 07:13

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@extensions/cornerstone/src/commandsModule.ts`:
- Around line 2055-2057: The cancel flow currently lets a preview failure from
rejectPreview stop cancelMeasurement from running, so update
cancelActiveOperation to guarantee measurement cancellation even if preview
rejection throws. Adjust the preview handling path in _handlePreviewAction so it
does not escape past the per-tool try/catch, or wrap the rejectPreview call
locally so cancelMeasurement still executes for the active annotation when
Escape is pressed.
🪄 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: 471220d7-52a7-4901-a89e-f8b762d3b00d

📥 Commits

Reviewing files that changed from the base of the PR and between 20632f9 and e98ff11.

📒 Files selected for processing (11)
  • extensions/cornerstone/src/commandsModule.ts
  • platform/core/src/defaults/hotkeyBindings.ts
  • platform/docs/docs/migration-guide/3p12-to-3p13/escape-cancel-hotkey.md
  • platform/ui-next/src/components/OHIFToolSettings/ToolSettings.tsx
  • tests/BSplineContourSegmentation.spec.ts
  • tests/CatmullRomSplineContourSegmentation.spec.ts
  • tests/LinearSplineContourSegmentation.spec.ts
  • tests/Livewire.spec.ts
  • tests/LivewireContourSegmentation.spec.ts
  • tests/Spline.spec.ts
  • tests/pages/RightPanelPageObject.ts

Comment thread extensions/cornerstone/src/commandsModule.ts Outdated
@jbocce jbocce requested review from sedghi and wayfarer3130 June 24, 2026 08:21
@jbocce jbocce marked this pull request as ready for review June 24, 2026 08:22

@wayfarer3130 wayfarer3130 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extensions/cornerstone/src/commandsModule.ts:307 — cancelMeasurement duplicates existing cancel logic (reuse). The body (const element = _getActiveViewportEnabledElement()?.viewport?.element; if (element) cancelActiveManipulations(element);) is identical to initMeasurementService.ts:534-536. Two copies of the same "cancel active manipulations on the active viewport" snippet will drift independently; consider a shared helper. (Cost: duplicated logic, not a crash.)

@jbocce

jbocce commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

extensions/cornerstone/src/commandsModule.ts:307 — cancelMeasurement duplicates existing cancel logic (reuse). The body (const element = _getActiveViewportEnabledElement()?.viewport?.element; if (element) cancelActiveManipulations(element);) is identical to initMeasurementService.ts:534-536. Two copies of the same "cancel active manipulations on the active viewport" snippet will drift independently; consider a shared helper. (Cost: duplicated logic, not a crash.)

Great catch. Resolved!

@jbocce jbocce requested a review from wayfarer3130 June 25, 2026 06:14
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.

[Bug] Contour cancellation

2 participants