feat: improve sam segmentation, questionaire, recorder, sam-segmemnta…#184
Conversation
…tion, menu-pages, profiler, remove web worker
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (55)
📝 WalkthroughWalkthroughThis PR refactors the recorder from a global timeline to a per-viewer/per-recording model with named recordings, adds an overlay system (types, renderer, editor) for per-step annotations, migrates SAM segmentation from JS to TypeScript, redesigns the questionnaire plugin with new element types and an IO pipeline, adds ChangesRecorder: Per-viewer/per-recording overhaul with overlays
SAM segmentation: JS→TypeScript migration
Questionnaire plugin: new element types and designer redesign
UI infrastructure: AppBar Tools, menu-pages, profiler, custom-pages, annotations, loader
Sequence Diagram(s)sequenceDiagram
participant User
participant RecorderPlugin
participant RecorderModule
participant OverlayRenderer
participant OSDViewer
rect rgba(70, 130, 180, 0.5)
Note over User,OSDViewer: Playback with overlays
User->>RecorderPlugin: click Play
RecorderPlugin->>RecorderModule: play(viewerId?)
RecorderModule->>RecorderModule: fan-out to all viewers' active recordings
loop per viewer session
RecorderModule->>RecorderModule: _jumpAt(session, index)
RecorderModule->>OSDViewer: apply visualization / navigation state
RecorderModule->>RecorderPlugin: emit enter {viewerId, recordingId, index, step}
RecorderPlugin->>OverlayRenderer: _onEnter(step, viewerId)
OverlayRenderer->>OSDViewer: mount overlay DOM elements
end
end
rect rgba(180, 100, 50, 0.5)
Note over User,OSDViewer: Overlay editing
User->>RecorderPlugin: click overlay edit button on step
RecorderPlugin->>OverlayEditor: open()
OverlayEditor->>OverlayRenderer: previewSet(step, draftOverlays, draftAssets)
OverlayRenderer->>OSDViewer: mount preview overlay elements
User->>OverlayEditor: edit markdown / image / anchor
OverlayEditor->>OverlayRenderer: previewSet(step, updatedOverlays, draftAssets)
User->>OverlayEditor: Save
OverlayEditor->>RecorderModule: updateStep(id, mutate)
OverlayEditor->>RecorderModule: putAsset(asset) per new draft asset
OverlayEditor->>OverlayRenderer: previewClear()
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements across several modules and plugins, notably refactoring the Recorder module to support parallel multi-viewport playback and named recordings, updating the Questionnaire plugin with a capability-gated designer and ROI region capture, and migrating the Segment Anything (SAM) experimental module to TypeScript. It also adds a generic 'Tools' category to the app-bar and resolves various multi-viewport rendering and reset issues. Feedback on the changes highlights critical runtime issues in the Questionnaire plugin, including focus loss on numeric inputs due to the use of the 'input' event instead of 'change', and TypeErrors caused by calling the non-existent 'this.showInfo' method instead of 'Dialogs.show'. Additionally, a minor typo in the README was identified.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| unitSel.disabled = readOnly; | ||
| units.forEach((u) => { const o = document.createElement("option"); o.value = u; o.textContent = u; o.selected = (cur.unit ?? units[0]) === u; unitSel.append(o); }); | ||
| const push = () => setValue({ value: num.value === "" ? null : Number(num.value), unit: unitSel.value } as unknown as QuestionnaireValue); | ||
| num.addEventListener("input", push); |
There was a problem hiding this comment.
Using the "input" event listener on the numeric input causes "setValue" to be called on every keystroke. This triggers "setAnswer", which calls "renderRuntime()" and completely destroys and recreates the DOM elements of the runtime view. As a result, the user loses focus and cursor position after typing a single character, making the field virtually unusable. Changing the event listener to "change" ensures the value is committed only when the input is blurred or the user presses Enter, preserving focus during typing.
| num.addEventListener("input", push); | |
| num.addEventListener("change", push); |
| const actions = el("div", "flex flex-wrap gap-2"); | ||
| if (!readOnly) actions.append(button("Capture selected region", "btn btn-primary btn-sm", () => { | ||
| const captured = captureSelectedRegion(Array.from(this._viewerMap.values())); | ||
| if (!captured) { this.showInfo("Select a single annotation in the viewer first."); return; } |
There was a problem hiding this comment.
"this.showInfo" is not a valid method on "XOpatPlugin" or "XOpatElement" and will cause a runtime "TypeError" when triggered. Please use the standard "Dialogs.show" method to display warning/info messages to the user.
| if (!captured) { this.showInfo("Select a single annotation in the viewer first."); return; } | |
| if (!captured) { Dialogs.show("Select a single annotation in the viewer first.", 3000, Dialogs.MSG_WARN); return; } |
| setValue(captured as unknown as QuestionnaireValue); | ||
| })); | ||
| actions.append(button("Show region", "btn btn-outline btn-sm", () => { | ||
| if (!region) { this.showInfo("No region captured yet."); return; } |
There was a problem hiding this comment.
"this.showInfo" is not a valid method on "XOpatPlugin" or "XOpatElement" and will cause a runtime "TypeError" when triggered. Please use the standard "Dialogs.show" method to display warning/info messages to the user.
| if (!region) { this.showInfo("No region captured yet."); return; } | |
| if (!region) { Dialogs.show("No region captured yet.", 3000, Dialogs.MSG_WARN); return; } |
| > ⚠️ **This is xOpat v3 — an alpha release.** The older but | ||
| > stable **v2** line lives on the [`archive/v2`](https://github.com/RationAI/xopat/tree/archive/v2) | ||
| > branch. | ||
| > branch. If you experience anny issues, please, let us know. |
There was a problem hiding this comment.
There is a typo in this line: "anny" should be "any". Also, the comma after "please" is unnecessary.
| > branch. If you experience anny issues, please, let us know. | |
| > branch. If you experience any issues, please let us know. |
1 participant
…tion, menu-pages, profiler, remove web worker
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation