fix(ToolGroupService): Centralize tool binding persistence and provide API to add and remove persisted bindings.

[jbocce]

Context

• It started as a result of this comment in another PR: feat(ui-next): Adds toggle state for ToolButton and Crosshair example #5914 (comment)
• Previously, tool binding persistence logic was split between the user preferences customization and ToolGroupService, which duplicated storage-key resolution and made persistence behavior harder to reason about.
• Also persisting Crosshair bindings was clobbering any other persisted bindings in localStorage.
• Updating Crosshair modifier bindings could leave previous bindings active.
• Resetting the Crosshair modifier did not restore to the tool group default but the current modifier persisted for the rest of the session
• If the Crosshair tool were active by default (i.e. when entering MPR mode) and if there was a preference in localStorage different than the default, the localStorage was not applied.

Changes & Results

• Centralized tool-binding persistence in ToolGroupService by adding:
  • persistToolBindings(toolGroupId, toolName, bindings)
  • removePersistedToolBindings(toolGroupId, toolName?)
• Removed duplicated storage-key/localStorage logic from userPreferencesCustomization.tsx and routed persistence calls through toolGroupService.
• Updated Crosshairs preferences flow to:
  • persist via toolGroupService.persistToolBindings(...)
  • remove only Crosshairs persisted binding via toolGroupService.removePersistedToolBindings('mpr', 'Crosshairs')
• Added explicit runtime apply semantics in ToolGroupService:
  • applyToolBindings(toolGroupId, toolName, options?)
  • supports options.replaceExisting to reset existing active bindings before reapplying.
• Updated Crosshairs apply call to use replacement semantics:
  • applyToolBindings('mpr', 'Crosshairs', { replaceExisting: true })
• Updated ToolGroupService docs to:
  • document newly exposed public methods
  • clarify that setToolBindings updates internal service state, while applyToolBindings applies those bindings to runtime tool behavior.
• ToolGroupService._loadPersistedBindings still loads persisted bindings into the internal map (setToolBindings), but now also applies them immediately when the affected tool is already Active, via applyToolBindings(..., { replaceExisting: true }).
• ToolGroupService
  • Adds a defaultToolBindingsMap captured once during normal tool-group initialization (when bindings are provided).
  • Adds getDefaultToolBindings(toolGroupId, toolName) to read those defaults (cloned).
  • setToolBindings now stores cloned binding objects (_cloneToolBindings) to reduce accidental shared mutation.
  • Clears the defaults map on service destroy().
• userPreferencesCustomization.tsx
  • Uses getDefaultToolBindings('mpr','Crosshairs') (memoized once as defaultCrosshairBindings) to drive reset UI/runtime restore.
  • On reset: restores runtime Crosshairs bindings using defaults (setToolBindings + applyToolBindings with replace semantics), then clears persisted prefs (removePersistedToolBindings).
  • Makes modifier extraction explicit by requiring mouseButton match (passed 1 for Crosshairs).

Testing

Test A - Previous preferences removed

1. Launch OHIF in incognito mode.
2. Launch a study in MPR.
3. Activate the crosshairs tool.
4. Note that Shift+Left mouse button controls the crosshairs.
5. Open the user preferences and change the crosshairs binding to be something else (e.g. Ctrl+Left mouse button).
6. Ensure that the new binding takes effect and any previous binding is no longer present.

Test B - Reset preferences restores tool group binding

1. Launch OHIF in incognito mode.
2. Launch a study in MPR.
3. Activate the crosshairs tool.
4. Note that Shift+Left mouse button controls the crosshairs.*
5. Open the user preferences and change the crosshairs binding to be something else (e.g. Ctrl+Left mouse button).
6. Ensure that the new binding takes effect.
7. Refresh the browser.
8. The binding set on step 5 should be the one active for Crosshairs MPR
9. Open the user preferences and click reset to default.
10. Ensure that the default Shift+Left mouse button controls the crosshairs.*

• Note that if the tool group default is changed to something else, then that will be the default.

Test C - Preference binding applied when Crosshair tool is active by default

1. Set the Crosshair tool to be active by default for the MPR tool group.
2. Launch OHIF in incognito mode.
3. Open the user preferences and change the crosshairs binding to be something else (e.g. Ctrl+Left mouse button).
4. Refresh the browser.
5. Launch a study in MPR.
6. The Crosshair tool should be active and the key binding preference in step 3 should be the one that controls the crosshairs.

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: 5.63 GB / 31.68 GB
Binaries:
Node: 20.9.0 - C:\Users\joebo\AppData\Local\fnm_multishells\24236_1777488400307\node.EXE
Yarn: 1.22.22 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
npm: 10.1.0 - C:\Users\joebo\AppData\Local\fnm_multishells\24236_1777488400307\npm.CMD
bun: 1.2.23 - C:\Users\joebo.bun\bin\bun.EXE
Browsers:
Chrome: 147.0.7727.117
Edge: Chromium (146.0.3856.84)
Internet Explorer: 11.0.26100.8115

Greptile Summary

This PR centralises tool-binding persistence in ToolGroupService, replacing scattered localStorage calls in the UI layer with dedicated service methods (persistToolBindings, removePersistedToolBindings). It also captures default bindings at tool-group init time and applies persisted bindings immediately when a tool is already Active on load, closing several Crosshairs-specific regressions.

• ToolGroupService: adds persistToolBindings, removePersistedToolBindings, getDefaultToolBindings, applyToolBindings with replaceExisting semantics, and _loadPersistedBindings now applies bindings immediately for Active tools; defaultToolBindingsMap captures initial bindings once and survives the session until destroy().
• userPreferencesCustomization.tsx: removes duplicated storage-key resolution; reset path now calls setToolBindings + applyToolBindings({ replaceExisting: true }) with the captured defaults before clearing the persisted preference, ensuring the runtime is immediately consistent without requiring a page reload.
• Docs: all new public API surface is documented, including the replaceExisting flag and the semantic distinction between setToolBindings (map update) and applyToolBindings (runtime update).

Confidence Score: 5/5

Safe to merge — the refactor is well-scoped, the new API is internally consistent, and the previously reported runtime-restore gap on reset has been addressed.

All three changed files are in good shape. The persistence centralisation is correct, the default-bindings capture is guarded against re-initialisation, and the replaceExisting path correctly clears stale modifier bindings before applying new ones. No data-loss or incorrect-runtime-state paths were found.

No files require special attention.

Important Files Changed

Filename	Overview
extensions/cornerstone/src/services/ToolGroupService/ToolGroupService.ts	Centralises localStorage read/write, adds persistToolBindings/removePersistedToolBindings, captures default bindings on init, and applies persisted bindings immediately when the affected tool is already Active. Logic is correct and well-guarded.
extensions/default/src/customizations/userPreferencesCustomization.tsx	Removes duplicated localStorage key logic, routes all persistence through toolGroupService, and properly restores default runtime bindings on reset. Minor: getToolModifier and getModifierFromBindings share identical predicate logic.
platform/docs/docs/platform/services/data/ToolGroupService.md	Documents all newly exposed public methods accurately, including the replaceExisting option and the distinction between setToolBindings (map update) and applyToolBindings (runtime update).

Comments Outside Diff (1)

1. extensions/default/src/customizations/userPreferencesCustomization.tsx, line 107-117 (link)

   Reset removes persisted binding but leaves runtime binding active

   onResetHandler removes the localStorage entry via removePersistedToolBindings, but neither calls setToolBindings with the default modifier nor calls applyToolBindings to push that default back to the Cornerstone runtime. The Crosshairs tool continues operating with the custom binding for the rest of the current session — only a full page reload would restore the default. The save path performs all three steps (set map → apply runtime → persist); reset should symmetrically reverse them (set map to default → apply runtime → remove persisted).

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: extensions/default/src/customizations/userPreferencesCustomization.tsx
   Line: 107-117
   
   Comment:
   **Reset removes persisted binding but leaves runtime binding active**
   
   `onResetHandler` removes the localStorage entry via `removePersistedToolBindings`, but neither calls `setToolBindings` with the default modifier nor calls `applyToolBindings` to push that default back to the Cornerstone runtime. The Crosshairs tool continues operating with the custom binding for the rest of the current session — only a full page reload would restore the default. The save path performs all three steps (set map → apply runtime → persist); reset should symmetrically reverse them (set map to default → apply runtime → remove persisted).
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
extensions/default/src/customizations/userPreferencesCustomization.tsx:29-50
`getToolModifier` and `getModifierFromBindings` share identical matching logic (same `mouseButton`, `modifierKey != null`, `numTouchPoints == null` predicate). The former could delegate to the latter, eliminating the duplication and making the code easier to maintain if the matching logic ever needs to change.

```suggestion
function getToolModifier(
  toolGroupService: any,
  toolGroupId: string,
  toolName: string,
  mouseButton: number
): string | null {
  if (!toolGroupService) {
    return null;
  }
  return getModifierFromBindings(toolGroupService.getToolBindings(toolGroupId, toolName), mouseButton);
}
```

_{Reviews (3): Last reviewed commit: "PR feedback." | Re-trigger Greptile}

[netlify]

✅ Deploy Preview for ohif-dev canceled.

Name	Link
🔨 Latest commit	4acc463
🔍 Latest deploy log	https://app.netlify.com/projects/ohif-dev/deploys/69fa312654b720000895c7bb

[sedghi]

see greptile review too please

[jbocce]

see greptile review too please

Yes I am looking at those now and they are significant.