Analyze plugin #147
Analyze plugin #147
Conversation
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
Aiosa
left a comment
Aiosa
left a comment
There was a problem hiding this comment.
Good code quality and mostly respects the viewer API. Good job! just a few fixes. Also, please, lets discuss whether SidePanel is really a valid new component, or whether it is something that the UI system already provides before mergning.
| "version": "1.0.0", | ||
| "description": "Plugin for creating and running jobs", | ||
| "icon": null, | ||
| "includes" : ["newAppForm.mjs", "analyzeDropdown.mjs"], |
There was a problem hiding this comment.
You can include only main file, import/export is handled by browser automatically. This will preload the files no matter whether they are needed or not, browser will do it dynamically. Just FYI no need to change.
| super(id); | ||
| this.params = params || {}; | ||
| // plugin-level stored recent jobs can be configured via params or saved options | ||
| this.recentJobs = this.getOption('recentJobs') || this.params.recentJobs || []; |
There was a problem hiding this comment.
Params are already read from using getOption so avoid touching params directly for this matter.
There was a problem hiding this comment.
The correct syntax is this.getOption('recentJobs', defaultVALUEHERE)
| pluginReady() { | ||
| const register = () => { | ||
|
|
||
| if (!window.USER_INTERFACE || !USER_INTERFACE.AppBar || !USER_INTERFACE.AppBar.menu) { |
There was a problem hiding this comment.
Also, user interface should be already available, if it is not, it is a bug.
| } | ||
|
|
||
| // safe translation helper: return translated value or fallback when missing | ||
| const tOr = (key, fallback) => { |
There was a problem hiding this comment.
If you need this generic 'safe' utility, we should put this function to the CORE api, not here... Why is typeof $?.t === 'function' needed? It should be always defined.
| return false; | ||
| }; | ||
| // Try immediate attach; if DOM not present yet, retry shortly | ||
| if (!attachToggle()) setTimeout(attachToggle, 50); |
There was a problem hiding this comment.
Again, DOM should be already ready at pluginReady
ui/classes/baseComponent.mjs
Outdated
| attachTo(element) { | ||
| this.refreshClassState(); | ||
| this.refreshPropertiesState(); | ||
| this.refreshClassState(); |
ui/classes/baseComponent.mjs
Outdated
| return; | ||
| } | ||
|
|
||
| if (!(mount instanceof Node) && typeof mount.append !== 'function') { |
There was a problem hiding this comment.
Duplicated code, it should be possible to get a reference to a node element to attach above, and resolve this logics just once.
| * | ||
| * @param {...any} properties - functions to set the state of the component | ||
| */ | ||
| set(...properties) { |
There was a problem hiding this comment.
Why is this method removed? Did you check that it is not used?
ui/classes/baseComponent.mjs
Outdated
| */ | ||
| _applyOptions(options, ...names) { | ||
| // internal helper: call provided option functions in the component context | ||
| try { |
| * SidePanel: small reusable fixed-position panel that plugins can show near an anchor. | ||
| * Simple API: constructor(options), attachToBody(), setBody(node|string), showAt({left,top}), hide(), remove(). | ||
| */ | ||
| class SidePanel extends BaseComponent { |
There was a problem hiding this comment.
Can you show me usage of this panel? Is it nececssary to have this menu-like component, or are there other components already that implement this logics? Form the name it sounds to me like there are.
There was a problem hiding this comment.
Yes, you are right. After reviewing the other components, it seems like Menu should be sufficient for this use case.
There was a problem hiding this comment.
So, do we keep this component, or remove it?
|
So what are we going to do with this PR? Is it used somehow? |
|
Analyze dropdown is used for selecting app via 'Apps' tab (not pushed yet). 'Run recent jobs' and 'Create new app' tabs do not have functionality as of now. |
|
there are still many issues not resolved from my last comments. Please fix all of them. Reolsve the conflicts here, and add a PR with all your logics implemented up till now so I can give a good feedback, |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR introduces an "Analyze" plugin that adds a dropdown menu to the application's AppBar with features for managing recent jobs, creating new apps, and viewing available apps. Supporting infrastructure includes a new SidePanel component, a NewAppForm UI component, and enhancements to BaseComponent to improve attachment and class management. Changes
Sequence DiagramssequenceDiagram
actor User
participant AppBar
participant Dropdown
participant SidePanel
participant Callback
User->>Dropdown: hover "Run Recent"
Dropdown->>SidePanel: showNear(anchor)
SidePanel-->>Dropdown: panel visible
Dropdown->>Dropdown: populate with recentJobs
User->>SidePanel: click job
SidePanel->>Callback: onJobClick(jobData)
Callback-->>User: job triggered
User->>SidePanel: mouseout
SidePanel->>SidePanel: scheduleHide()
SidePanel-->>Dropdown: panel hidden
sequenceDiagram
actor User
participant AppBar
participant Dropdown
participant NewAppForm
participant FloatingWindow
participant Callback
User->>Dropdown: click "Create New App"
Dropdown->>NewAppForm: instantiate form
NewAppForm->>FloatingWindow: showFloating()
FloatingWindow-->>User: form displayed
User->>NewAppForm: fill fields & click Create
NewAppForm->>NewAppForm: _serialize()
NewAppForm->>Callback: onSubmit(formData)
Callback-->>NewAppForm: success
NewAppForm->>FloatingWindow: close()
FloatingWindow-->>Dropdown: window closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@plugins/analyze/analyzeDropdown.mjs`:
- Around line 144-146: Replace the blocking, non-localized alert in the
NewAppForm onSubmit handler with a non-blocking success delegation: remove the
alert('Created new app: ...') call inside the onSubmit lambda and instead let
the caller handle UI feedback by returning success or invoking
this.params.onCreate with the created data (or emitting a success event/toast)
so success messaging is handled by the caller or a toast system; keep the
try/catch and error logging around this.params.onCreate to preserve error
handling.
- Around line 197-218: Replace the hardcoded caseIdHardcoded used when calling
window.EmpaiaStandaloneJobs.createAndRunJob with a sourced value from the plugin
options/params (e.g., this.getOption('caseId') || this.params.caseId) and remove
the constant; before calling createAndRunJob (inside the btn click handler)
validate the resolved caseId and if missing set status.textContent to an error
message and return early to avoid running with an undefined caseId; ensure you
reference the existing getOption and params member names so behavior matches
other usages in the constructor and elsewhere.
In `@plugins/analyze/newAppForm.mjs`:
- Around line 138-139: The _onEdit() method in newAppForm.mjs is currently a
no-op so the "Edit EAD" button is clickable but does nothing; either implement
the handler to perform the intended edit action (e.g., open the EAD
editor/modal, populate form state, or emit an event like this.dispatchEvent(new
CustomEvent('edit-ead', {detail: {...}}))) or explicitly disable/hide the button
until implemented (set the button's disabled attribute or conditional render) so
users can't click it; update the button's click binding to call _onEdit and
ensure any required state/methods (e.g., openEditor, populateEadData) are
present and tested.
- Around line 141-147: _onCreate currently only closes floating windows after a
successful submit, leaving dialog/embedded hosts open; modify the success branch
so that when onSubmit(data) does not return false you also call this._close()
(wrapped in a try/catch like the floating window close) to handle non-floating
hosts; reference the _onCreate method, the onSubmit call, the _floatingWindow
property, and the _close method to locate where to add the call.
In `@ui/classes/baseComponent.mjs`:
- Around line 465-468: The constructor in BaseSelectableComponent incorrectly
assigns the result of super(...) to options (super/constructor doesn't return a
value); change it to call super(options, ...args) without assignment and then
read options from this.options (e.g., set this.itemID = this.options.itemID ||
this.id). Update the constructor body in BaseSelectableComponent to stop using
the returned value from BaseComponent and instead use this.options, preserving
fallback to this.id.
- Around line 106-118: The attachTo and prependedTo flows use isComponentLike to
accept plain objects but then unguardedly access element._children (see
attachTo's element._children.push(this) and prependedTo's
element._children.unshift(this)); update both methods to only push/unshift when
element is an instance of BaseComponent and element._children is an array (same
guard used in detach: if (element instanceof BaseComponent &&
Array.isArray(element._children))). This preserves current behavior for real
BaseComponent instances while avoiding property access errors for plain
{create,id} objects.
In `@ui/classes/components/sidePanel.mjs`:
- Around line 47-58: The menu items created in setMenu currently set each
anchor's tabindex to "-1", making them unfocusable by keyboard; update setMenu
(in sidePanel.mjs) so each menu item anchor (the variable a) is
keyboard-focusable—either remove the tabindex attribute or set it to "0" and
ensure role="menuitem" remains, and if you plan roving-focus keep tabindex="-1"
only on non-active items and implement keyboard handlers to manage focus
movement; adjust the creation of a (the anchor) and any focus-init logic so
users can tab into and navigate the menu with the keyboard.
- Around line 75-104: In showNear, listeners for 'mouseenter' and 'mouseleave'
are being added every time which stacks handlers; modify showNear (or the
element creation path in create/attachToBody) to add those hover listeners only
once by guarding with a flag or element marker (e.g.,
this._hoverListenersAttached or setting an attribute like
panelEl.dataset.hoverAttached) before calling panelEl.addEventListener(...), and
set that flag/attribute after attaching so subsequent calls to showNear skip
re-attaching; keep using this.cancelHide and this.scheduleHide as the handler
functions.
♻️ Duplicate comments (2)
plugins/analyze/newAppForm.mjs (2)
1-3: Avoid importing UI/van directly to prevent duplicate framework instances.The runtime exposes UI/van globally; importing here can create parallel module instances and break
instanceofchecks. Please confirm the contract and switch to global access.🛠️ Example adjustment (align with global UI/van)
-import van from "../../ui/vanjs.mjs"; -import { BaseComponent } from "../../ui/classes/baseComponent.mjs"; -import { FloatingWindow } from "../../ui/classes/components/floatingWindow.mjs"; +const van = globalThis.van; +const { BaseComponent, FloatingWindow } = globalThis.UI;
8-11: Default id is too generic and can collide.Multiple form instances will share
new-app(andnew-app-window), causing duplicate IDs. Please namespace or randomize.🛠️ Suggested fix
- this.id = options.id || 'new-app'; + this.id = options.id || `analyze-new-app-${Math.random().toString(36).slice(2,8)}`;
🧹 Nitpick comments (1)
ui/classes/baseComponent.mjs (1)
301-310: Consider deleting class keys when toggled off.Leaving empty-string entries still produces extra tokens in
classState. Optional, but keeps the map clean.♻️ Optional cleanup
- toggleClass(key, value, on=true) { - this.classMap[key] = on ? value : ""; - this.classState.val = Object.values(this.classMap).join(" "); - } + toggleClass(key, value, on=true) { + if (on) this.classMap[key] = value; + else delete this.classMap[key]; + this.classState.val = Object.values(this.classMap).join(" "); + }
| const form = new NewAppForm({ onSubmit: (data) => { | ||
| try { if (this.params.onCreate?.(data) !== false) { alert('Created new app: ' + JSON.stringify(data)); } } | ||
| catch (err) { console.error(err); } |
There was a problem hiding this comment.
Avoid alert on successful create.
alert blocks the UI and isn’t localized. Prefer delegating success messaging to the caller or a toast system.
🛠️ Suggested fix
- try { if (this.params.onCreate?.(data) !== false) { alert('Created new app: ' + JSON.stringify(data)); } }
+ try { this.params.onCreate?.(data); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const form = new NewAppForm({ onSubmit: (data) => { | |
| try { if (this.params.onCreate?.(data) !== false) { alert('Created new app: ' + JSON.stringify(data)); } } | |
| catch (err) { console.error(err); } | |
| const form = new NewAppForm({ onSubmit: (data) => { | |
| try { this.params.onCreate?.(data); } | |
| catch (err) { console.error(err); } |
🤖 Prompt for AI Agents
In `@plugins/analyze/analyzeDropdown.mjs` around lines 144 - 146, Replace the
blocking, non-localized alert in the NewAppForm onSubmit handler with a
non-blocking success delegation: remove the alert('Created new app: ...') call
inside the onSubmit lambda and instead let the caller handle UI feedback by
returning success or invoking this.params.onCreate with the created data (or
emitting a success event/toast) so success messaging is handled by the caller or
a toast system; keep the try/catch and error logging around this.params.onCreate
to preserve error handling.
There was a problem hiding this comment.
Yes, we don't want alerts in the production. There is a Dialogs api if necessary, but if the UI shows the app, we don't need to show any dialogs.
There was a problem hiding this comment.
@Aiosa, thank you for confirming! I agree—removing the alert entirely and letting the caller handle feedback (or just showing the app in the UI) is the cleanest approach. The Dialogs API is a good fallback if explicit user feedback is ever needed.
🐰📝
✏️ Learnings added
Learnt from: Aiosa
Repo: RationAI/xopat PR: 147
File: plugins/analyze/analyzeDropdown.mjs:144-146
Timestamp: 2026-01-25T11:30:34.295Z
Learning: In the xopat codebase (JavaScript), a `Dialogs` API is available as an alternative to `alert()` for showing user messages.
Learnt from: Aiosa
Repo: RationAI/xopat PR: 147
File: plugins/analyze/analyzeDropdown.mjs:144-146
Timestamp: 2026-01-25T11:30:34.295Z
Learning: In the xopat codebase, avoid showing dialogs (or alerts) if the UI already displays the app or relevant information to the user.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
plugins/analyze/analyzeDropdown.mjs
Outdated
| const caseIdHardcoded = '87fbb59a-3183-4d36-ab22-48f4e027d1f0'; | ||
|
|
||
| // No input preloading for now; keep simple run flow | ||
|
|
||
| items.forEach((app, idx) => { | ||
| const appId = app?.id || app?.app_id; | ||
| const btn = document.createElement('button'); | ||
| btn.type = 'button'; | ||
| btn.className = 'btn btn-sm btn-primary w-full justify-start'; | ||
| btn.textContent = app?.name_short || app?.name || `App ${idx+1}`; | ||
| const status = document.createElement('div'); | ||
| status.className = 'text-xs mt-1'; | ||
| status.textContent = tOr('analyze.jobReady', 'Ready to run'); | ||
|
|
||
| // Keep UI minimal: no inputs or mode selector | ||
|
|
||
| btn.addEventListener('click', async () => { | ||
| try { | ||
| status.textContent = tOr('analyze.jobStarting', 'Starting job...'); | ||
| // Create + run job via helper (returns final job) | ||
| const res = await window.EmpaiaStandaloneJobs?.createAndRunJob?.({ appId, caseId: caseIdHardcoded }); | ||
| status.textContent = `${tOr('analyze.jobFinal', 'Final status')}: ${res?.status || 'UNKNOWN'}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n plugins/analyze/analyzeDropdown.mjs | head -250Repository: RationAI/xopat
Length of output: 17383
🏁 Script executed:
rg -n "caseId" plugins/analyze/analyzeDropdown.mjsRepository: RationAI/xopat
Length of output: 315
🏁 Script executed:
rg -n "caseId\|case" plugins/analyze/analyzeDropdown.mjs | head -20Repository: RationAI/xopat
Length of output: 40
🏁 Script executed:
rg -n "params\." plugins/analyze/analyzeDropdown.mjs | head -10Repository: RationAI/xopat
Length of output: 299
🏁 Script executed:
rg -rn "caseId" plugins/ --type mjs | head -20Repository: RationAI/xopat
Length of output: 85
🏁 Script executed:
rg -rn "caseId" plugins/Repository: RationAI/xopat
Length of output: 40
🏁 Script executed:
rg -rn "analyze" plugins/ | grep -i "param\|case\|init" | head -15Repository: RationAI/xopat
Length of output: 40
🏁 Script executed:
rg -rn "new.*analyze\|addPlugin.*analyze" --type-list | head -10Repository: RationAI/xopat
Length of output: 447
🏁 Script executed:
rg -rn "addPlugin.*analyze" .Repository: RationAI/xopat
Length of output: 127
🏁 Script executed:
rg -rn "analyze.*params\|EmpaiaStandaloneJobs" . --include="*.mjs" --include="*.js" | head -20Repository: RationAI/xopat
Length of output: 499
🏁 Script executed:
rg -rn "EmpaiaStandaloneJobs" . | head -20Repository: RationAI/xopat
Length of output: 346
🏁 Script executed:
cat -n plugins/analyze/analyzeDropdown.mjs | sed -n '1,50p'Repository: RationAI/xopat
Length of output: 2665
🏁 Script executed:
find . -name "README*" -o -name "*config*" | xargs grep -l "analyze" 2>/dev/null | head -5Repository: RationAI/xopat
Length of output: 85
🏁 Script executed:
cat plugins/analyze/README.mdRepository: RationAI/xopat
Length of output: 1070
Source caseId from plugin params instead of hardcoding.
The hardcoded caseId at line 197 is used for all job runs (line 217), forcing all apps to run against a fixed case. This prevents real workflows from using arbitrary cases. The plugin already accepts params in its constructor (line 6-8) and follows the pattern this.getOption('key') || this.params.key elsewhere (line 10), so source caseId consistently and guard when missing to prevent silent failures.
🛠️ Suggested fix
- const caseIdHardcoded = '87fbb59a-3183-4d36-ab22-48f4e027d1f0';
+ const caseId = this.getOption?.('caseId') || this.params.caseId;
...
- const res = await window.EmpaiaStandaloneJobs?.createAndRunJob?.({ appId, caseId: caseIdHardcoded });
+ if (!caseId) {
+ status.textContent = tOr('analyze.noCase', 'Select a case first');
+ return;
+ }
+ const res = await window.EmpaiaStandaloneJobs?.createAndRunJob?.({ appId, caseId });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const caseIdHardcoded = '87fbb59a-3183-4d36-ab22-48f4e027d1f0'; | |
| // No input preloading for now; keep simple run flow | |
| items.forEach((app, idx) => { | |
| const appId = app?.id || app?.app_id; | |
| const btn = document.createElement('button'); | |
| btn.type = 'button'; | |
| btn.className = 'btn btn-sm btn-primary w-full justify-start'; | |
| btn.textContent = app?.name_short || app?.name || `App ${idx+1}`; | |
| const status = document.createElement('div'); | |
| status.className = 'text-xs mt-1'; | |
| status.textContent = tOr('analyze.jobReady', 'Ready to run'); | |
| // Keep UI minimal: no inputs or mode selector | |
| btn.addEventListener('click', async () => { | |
| try { | |
| status.textContent = tOr('analyze.jobStarting', 'Starting job...'); | |
| // Create + run job via helper (returns final job) | |
| const res = await window.EmpaiaStandaloneJobs?.createAndRunJob?.({ appId, caseId: caseIdHardcoded }); | |
| status.textContent = `${tOr('analyze.jobFinal', 'Final status')}: ${res?.status || 'UNKNOWN'}`; | |
| const caseId = this.getOption?.('caseId') || this.params.caseId; | |
| // No input preloading for now; keep simple run flow | |
| items.forEach((app, idx) => { | |
| const appId = app?.id || app?.app_id; | |
| const btn = document.createElement('button'); | |
| btn.type = 'button'; | |
| btn.className = 'btn btn-sm btn-primary w-full justify-start'; | |
| btn.textContent = app?.name_short || app?.name || `App ${idx+1}`; | |
| const status = document.createElement('div'); | |
| status.className = 'text-xs mt-1'; | |
| status.textContent = tOr('analyze.jobReady', 'Ready to run'); | |
| // Keep UI minimal: no inputs or mode selector | |
| btn.addEventListener('click', async () => { | |
| try { | |
| status.textContent = tOr('analyze.jobStarting', 'Starting job...'); | |
| // Create + run job via helper (returns final job) | |
| if (!caseId) { | |
| status.textContent = tOr('analyze.noCase', 'Select a case first'); | |
| return; | |
| } | |
| const res = await window.EmpaiaStandaloneJobs?.createAndRunJob?.({ appId, caseId }); | |
| status.textContent = `${tOr('analyze.jobFinal', 'Final status')}: ${res?.status || 'UNKNOWN'}`; |
🤖 Prompt for AI Agents
In `@plugins/analyze/analyzeDropdown.mjs` around lines 197 - 218, Replace the
hardcoded caseIdHardcoded used when calling
window.EmpaiaStandaloneJobs.createAndRunJob with a sourced value from the plugin
options/params (e.g., this.getOption('caseId') || this.params.caseId) and remove
the constant; before calling createAndRunJob (inside the btn click handler)
validate the resolved caseId and if missing set status.textContent to an error
message and return early to avoid running with an undefined caseId; ensure you
reference the existing getOption and params member names so behavior matches
other usages in the constructor and elsewhere.
There was a problem hiding this comment.
Yes, the problem here is not only hardcoding a case, but hardcoding everything - this is all empaia data models const appId = app?.id || app?.app_id;... we can keep it for now like this, but please add a todo comment that explains we need to make this more flexible - what if your apps come from a different API? DICOM? HuggingFace?
There was a problem hiding this comment.
@Aiosa, you're absolutely right – the coupling extends beyond just the caseId. The entire implementation assumes Empaia's data models and API structure. I'll add a comprehensive TODO comment to document this technical debt:
// TODO: Decouple from Empaia-specific data models and API
// Current implementation is tightly coupled to EmpaiaStandaloneJobs API and Empaia data structures:
// - App ID extraction: app?.id || app?.app_id
// - Case ID handling: hardcoded Empaia case UUID
// - API calls: window.EmpaiaStandaloneJobs.getApps(), createAndRunJob()
//
// Future work should introduce abstraction layers to support multiple app sources:
// - DICOM app registries
// - HuggingFace model hub
// - Generic REST APIs
// - Custom plugin-provided adapters
//
// Consider: plugin-level adapter pattern where params.appSource defines the provider
// and params.adapter supplies the necessary methods (listApps, runJob, etc.)This can be placed just before line 197 where the hardcoded caseId appears, since that's where the Empaia-specific integration is most concentrated.
✏️ Learnings added
Learnt from: Aiosa
Repo: RationAI/xopat PR: 147
File: plugins/analyze/analyzeDropdown.mjs:0-0
Timestamp: 2026-01-25T11:33:05.489Z
Learning: In the analyze plugin (plugins/analyze/analyzeDropdown.mjs), the current Empaia-specific implementation (EmpaiaStandaloneJobs API, app ID extraction with app?.id || app?.app_id, hardcoded case IDs) is accepted as temporary technical debt. Future refactoring should add abstraction layers to support multiple app sources including DICOM, HuggingFace, and other APIs through a plugin-level adapter pattern.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| _onEdit() { | ||
| } |
There was a problem hiding this comment.
Edit EAD handler is a no‑op.
The button is visible but does nothing. Consider hiding/disable until implemented or wire a callback. Happy to help implement the handler.
🤖 Prompt for AI Agents
In `@plugins/analyze/newAppForm.mjs` around lines 138 - 139, The _onEdit() method
in newAppForm.mjs is currently a no-op so the "Edit EAD" button is clickable but
does nothing; either implement the handler to perform the intended edit action
(e.g., open the EAD editor/modal, populate form state, or emit an event like
this.dispatchEvent(new CustomEvent('edit-ead', {detail: {...}}))) or explicitly
disable/hide the button until implemented (set the button's disabled attribute
or conditional render) so users can't click it; update the button's click
binding to call _onEdit and ensure any required state/methods (e.g., openEditor,
populateEadData) are present and tested.
There was a problem hiding this comment.
It's good to leave todo comments where there is expected a work to be done. Also same issue as above - we should not really realy on EAD - its EMPAIA thing. We should support 'some config' in a more generic way, but as before-maybe let's first get EMPAIA working, and then continue generalization of the logics.
There was a problem hiding this comment.
@Aiosa, good points! Adding a TODO comment will help document the intent. For the _onEdit() method, consider adding:
_onEdit() {
// TODO: Implement EAD editor integration (EMPAIA-specific, will be generalized later)
}This approach is consistent with the current strategy of getting EMPAIA working first before adding abstraction layers for multiple app sources. The TODO makes it clear this is intentional technical debt that will be addressed during future refactoring.
🧠 Learnings used
Learnt from: Aiosa
Repo: RationAI/xopat PR: 147
File: plugins/analyze/analyzeDropdown.mjs:0-0
Timestamp: 2026-01-25T11:33:05.489Z
Learning: In the analyze plugin (plugins/analyze/analyzeDropdown.mjs), the current Empaia-specific implementation (EmpaiaStandaloneJobs API, app ID extraction with app?.id || app?.app_id, hardcoded case IDs) is accepted as temporary technical debt. Future refactoring should add abstraction layers to support multiple app sources including DICOM, HuggingFace, and other APIs through a plugin-level adapter pattern.
| _onCreate() { | ||
| const data = this._serialize(); | ||
| try { | ||
| const r = this.onSubmit(data); | ||
| if (r !== false) { | ||
| try { if (this._floatingWindow) { this._floatingWindow.close(); this._floatingWindow = null; } } catch(_) {} | ||
| } |
There was a problem hiding this comment.
Close non‑floating hosts after successful submit.
For Dialog/embedded paths, the form stays open. Calling _close() after success covers all hosts.
🛠️ Suggested fix
- const r = this.onSubmit(data);
- if (r !== false) {
- try { if (this._floatingWindow) { this._floatingWindow.close(); this._floatingWindow = null; } } catch(_) {}
- }
+ const r = this.onSubmit(data);
+ if (r !== false) {
+ this._close();
+ }🤖 Prompt for AI Agents
In `@plugins/analyze/newAppForm.mjs` around lines 141 - 147, _onCreate currently
only closes floating windows after a successful submit, leaving dialog/embedded
hosts open; modify the success branch so that when onSubmit(data) does not
return false you also call this._close() (wrapped in a try/catch like the
floating window close) to handle non-floating hosts; reference the _onCreate
method, the onSubmit call, the _floatingWindow property, and the _close method
to locate where to add the call.
There was a problem hiding this comment.
I dont understand, it closes the window if it is defined - its just a sanity check.
There was a problem hiding this comment.
@Aiosa, I understand the confusion! Let me clarify the issue:
The current code in _onCreate only handles one of the three ways this form can be hosted:
Current code:
if (r !== false) {
try { if (this._floatingWindow) { this._floatingWindow.close(); this._floatingWindow = null; } } catch(_) {}
}This only closes the form when it's shown via showFloating() (i.e., _floatingWindow is defined).
But the form can be used in three different ways:
- FloatingWindow (via
showFloating()) — ✅ current code handles this - Dialog (via
Dialogs.showCustomor similar) — ❌ current code doesn't close this - Direct DOM attachment (via
attachTo()) — ❌ current code doesn't clean this up
Looking at your _close() method (lines 99-120), it was specifically designed to handle all three cases:
- First tries to close
_floatingWindow - Then tries to close via
Dialogs.closeWindowif inside a Dialog - Falls back to removing from DOM
CodeRabbit's suggestion is to simply call _close() after successful submit, which would handle all three hosting scenarios consistently, rather than only handling the FloatingWindow case.
So it's not just a sanity check—it's about ensuring the form closes properly regardless of how it was opened. Does this clarify the suggestion?
🧠 Learnings used
Learnt from: Aiosa
Repo: RationAI/xopat PR: 147
File: plugins/analyze/analyzeDropdown.mjs:0-0
Timestamp: 2026-01-25T11:33:05.489Z
Learning: In the analyze plugin (plugins/analyze/analyzeDropdown.mjs), the current Empaia-specific implementation (EmpaiaStandaloneJobs API, app ID extraction with app?.id || app?.app_id, hardcoded case IDs) is accepted as temporary technical debt. Future refactoring should add abstraction layers to support multiple app sources including DICOM, HuggingFace, and other APIs through a plugin-level adapter pattern.
| // Accept true BaseComponent instances and component-like objects | ||
| const isComponentLike = (el) => { | ||
| return !!el && (el instanceof BaseComponent || (typeof el === "object" && typeof el.create === "function" && typeof el.id === "string")); | ||
| }; | ||
|
|
||
| if (isComponentLike(element)) { | ||
| const mount = document.getElementById(element.id); | ||
| if (mount === null) { | ||
| element._children.push(this); | ||
| } else { | ||
| mount.append(this.create()); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the baseComponent.mjs file
find . -name "baseComponent.mjs" -type fRepository: RationAI/xopat
Length of output: 172
🏁 Script executed:
# Read the relevant section with context
cat -n ui/classes/baseComponent.mjs | sed -n '90,130p'Repository: RationAI/xopat
Length of output: 1946
🏁 Script executed:
# Search for component-like object definitions and usage patterns
rg -A 5 "component-like|isComponentLike" ui/classes/baseComponent.mjsRepository: RationAI/xopat
Length of output: 584
🏁 Script executed:
# Check for _children property usage and initialization
rg "_children" ui/classes/baseComponent.mjsRepository: RationAI/xopat
Length of output: 767
🏁 Script executed:
# Look for test files related to attachTo
find . -name "*.test.*" -o -name "*.spec.*" | head -20Repository: RationAI/xopat
Length of output: 40
🏁 Script executed:
# Search for actual component-like object definitions (objects with create and id properties)
rg "create.*function|\.create\s*:" . --type-list | grep -v node_modules | head -20Repository: RationAI/xopat
Length of output: 447
🏁 Script executed:
# Look for prependedTo method to see if similar pattern exists
rg -B 5 -A 10 "prependedTo" ui/classes/baseComponent.mjsRepository: RationAI/xopat
Length of output: 595
🏁 Script executed:
# Search for test files that test attachTo with component-like objects
find . -type f \( -name "*.test.js" -o -name "*.spec.js" -o -name "test*.js" \) | xargs grep -l "attachTo" 2>/dev/null | head -5Repository: RationAI/xopat
Length of output: 40
🏁 Script executed:
# Look for other instances where _children is accessed on non-BaseComponent instances
rg "element\._children" ui/classes/baseComponent.mjs -B 2 -A 2Repository: RationAI/xopat
Length of output: 1138
🏁 Script executed:
# Check if prependedTo has similar _children.unshift issue with component-like objects
cat -n ui/classes/baseComponent.mjs | sed -n '135,160p'Repository: RationAI/xopat
Length of output: 1356
🏁 Script executed:
# Verify component-like objects are actually being used and passed around
rg "create\s*[:=].*function|create\s*\(" . --type js -B 2 -A 2 | head -40Repository: RationAI/xopat
Length of output: 4613
Guard _children access in attachTo and prependedTo for component-like objects.
Both methods access element._children without checking if the property exists. Since isComponentLike accepts plain objects with {create, id} properties (not just BaseComponent instances), and these objects may not have a _children property, this will throw when the mount element is not found in the DOM.
The detach method already demonstrates defensive awareness with if (element instanceof BaseComponent && Array.isArray(element._children)). Apply the same pattern to attachTo (line 114) and prependedTo to prevent crashes on unsupported targets.
🛠️ Suggested fix
if (isComponentLike(element)) {
const mount = document.getElementById(element.id);
if (mount === null) {
- element._children.push(this);
+ if (element instanceof BaseComponent && Array.isArray(element._children)) {
+ element._children.push(this);
+ } else {
+ console.error("Failed to attach: component-like target has no mount and no _children queue", element);
+ }
} else {
mount.append(this.create());
}Apply the same guard to prependedTo's element._children.unshift(this) call.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Accept true BaseComponent instances and component-like objects | |
| const isComponentLike = (el) => { | |
| return !!el && (el instanceof BaseComponent || (typeof el === "object" && typeof el.create === "function" && typeof el.id === "string")); | |
| }; | |
| if (isComponentLike(element)) { | |
| const mount = document.getElementById(element.id); | |
| if (mount === null) { | |
| element._children.push(this); | |
| } else { | |
| mount.append(this.create()); | |
| } | |
| } else { | |
| // Accept true BaseComponent instances and component-like objects | |
| const isComponentLike = (el) => { | |
| return !!el && (el instanceof BaseComponent || (typeof el === "object" && typeof el.create === "function" && typeof el.id === "string")); | |
| }; | |
| if (isComponentLike(element)) { | |
| const mount = document.getElementById(element.id); | |
| if (mount === null) { | |
| if (element instanceof BaseComponent && Array.isArray(element._children)) { | |
| element._children.push(this); | |
| } else { | |
| console.error("Failed to attach: component-like target has no mount and no _children queue", element); | |
| } | |
| } else { | |
| mount.append(this.create()); | |
| } | |
| } else { |
🤖 Prompt for AI Agents
In `@ui/classes/baseComponent.mjs` around lines 106 - 118, The attachTo and
prependedTo flows use isComponentLike to accept plain objects but then
unguardedly access element._children (see attachTo's
element._children.push(this) and prependedTo's element._children.unshift(this));
update both methods to only push/unshift when element is an instance of
BaseComponent and element._children is an array (same guard used in detach: if
(element instanceof BaseComponent && Array.isArray(element._children))). This
preserves current behavior for real BaseComponent instances while avoiding
property access errors for plain {create,id} objects.
There was a problem hiding this comment.
I agree, why is this check added? I don't see a scenario where it would be needed, we usually require inheritance from a BaseComponent. Maybe this would fail if we have longer inheritance chain... so checking on props explicitly might be benefical, but then we need to check _children props when we use it..
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for attachTo and prependedTo usage in the analyze plugin files
echo "=== Checking attachTo/prependedTo usage in analyze plugin ==="
rg "attachTo|prependedTo" plugins/analyze/ -n -C3
echo -e "\n=== Checking if NewAppForm or SidePanel are used as plain objects ==="
# Look for patterns where these are created without 'new' keyword or used as plain objects
rg "NewAppForm|SidePanel" plugins/analyze/ -n -C3
echo -e "\n=== Check for component-like object patterns (objects with create and id) ==="
# Look for plain objects with create and id properties
rg -U "{\s*create\s*:|{\s*id\s*:" plugins/analyze/ -n -C3
echo -e "\n=== Check class declarations for NewAppForm and SidePanel ==="
rg "class (NewAppForm|SidePanel)" plugins/analyze/ ui/classes/components/ -n
Length of output: 11912
🏁 Script executed:
#!/bin/bash
# Search for attachTo usage patterns across the entire codebase to see if plain objects are ever used
echo "=== Checking all attachTo usage patterns ==="
rg "\.attachTo\(" --type js -n -C2 | head -100
echo -e "\n=== Check if FloatingWindow extends BaseComponent ==="
rg "class FloatingWindow" --type js -n -A2
echo -e "\n=== Search for any objects created with literal syntax being passed to attachTo ==="
# Look for patterns like {create: ..., id: ...}.attachTo or variable.attachTo where variable might be a plain object
rg "{\s*(?:create|id)\s*:" --type js -n | head -30
Length of output: 72226
🏁 Script executed:
#!/bin/bash
# Find FloatingWindow class definition
echo "=== Finding FloatingWindow class ==="
rg "class FloatingWindow" --type js -n -A5
echo -e "\n=== Check if any non-BaseComponent objects are passed to attachTo in the codebase ==="
# Look for patterns where objects might be created and passed to attachTo
rg "attachTo\(" --type js -B3 | grep -E "(const|let|var|=)" | head -40
Length of output: 65999
@Aiosa, you're absolutely right to question this. Based on my search:
No usage of component-like plain objects found:
- All new components in this PR (
NewAppForm,SidePanel) extendBaseComponent - All
attachTo()calls use proper BaseComponent instances created withnew - No plain objects with
{create, id}properties being passed toattachTo()anywhere in the codebase
The isComponentLike abstraction appears to be premature/unused, which creates unnecessary complexity. You have two options:
-
Remove the abstraction (recommended if truly unused):
- Revert to simple
instanceof BaseComponentcheck - Simpler, clearer code matching your existing patterns
- Revert to simple
-
If keeping it (for future extensibility), you must guard
_childrenaccess as CodeRabbit correctly flagged:if (mount === null) { if (element instanceof BaseComponent && Array.isArray(element._children)) { element._children.push(this); } else { console.error("Failed to attach: component-like target has no mount and no _children queue", element); } }
The same guard is needed in
prependedTo().
My recommendation: Remove isComponentLike and revert to instanceof BaseComponent unless you have a specific use case for plain component-like objects that I haven't found.
🧠 Learnings used
Learnt from: Aiosa
Repo: RationAI/xopat PR: 147
File: plugins/analyze/analyzeDropdown.mjs:0-0
Timestamp: 2026-01-25T11:33:05.489Z
Learning: In the analyze plugin (plugins/analyze/analyzeDropdown.mjs), the current Empaia-specific implementation (EmpaiaStandaloneJobs API, app ID extraction with app?.id || app?.app_id, hardcoded case IDs) is accepted as temporary technical debt. Future refactoring should add abstraction layers to support multiple app sources including DICOM, HuggingFace, and other APIs through a plugin-level adapter pattern.
| constructor(options, ...args) { | ||
| options = super(options, ...args); | ||
| this.itemID = options.itemID || this.id; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "baseComponent.mjs" -type fRepository: RationAI/xopat
Length of output: 88
🏁 Script executed:
# First, let's locate and read the file
fd baseComponent.mjsRepository: RationAI/xopat
Length of output: 86
🏁 Script executed:
# Check the structure around the target lines
cat -n ui/classes/baseComponent.mjs | sed -n '460,475p'Repository: RationAI/xopat
Length of output: 562
🏁 Script executed:
# Let's see more context - the class definition and parent
cat -n ui/classes/baseComponent.mjs | head -100Repository: RationAI/xopat
Length of output: 4368
🏁 Script executed:
# Search for BaseSelectableComponent usage and instantiation
rg "BaseSelectableComponent" -A 2 -B 2Repository: RationAI/xopat
Length of output: 396
🏁 Script executed:
# Also check if there are any tests that might show the actual behavior
fd -e test.js -o -e test.mjs -o -e spec.js -o -e spec.mjs | head -20Repository: RationAI/xopat
Length of output: 174
🏁 Script executed:
# Examine the BaseComponent constructor return statement
cat -n ui/classes/baseComponent.mjs | sed -n '50,96p' | grep -E "return|constructor"Repository: RationAI/xopat
Length of output: 104
🏁 Script executed:
# Final check - search for any explicit return in BaseComponent
rg "return" ui/classes/baseComponent.mjs | head -5Repository: RationAI/xopat
Length of output: 316
Fix constructor to properly access options after super call.
In the BaseSelectableComponent constructor, options = super(options, ...args) assigns undefined to the options variable since the parent BaseComponent constructor doesn't return anything. This causes options.itemID to fail when accessed. The parent class's own documentation shows the correct pattern: call super(), then access options via this.options.
🐛 Fix constructor option handling
- constructor(options, ...args) {
- options = super(options, ...args);
- this.itemID = options.itemID || this.id;
- }
+ constructor(options, ...args) {
+ super(options, ...args);
+ const opts = this.options || {};
+ this.itemID = opts.itemID || this.id;
+ }🤖 Prompt for AI Agents
In `@ui/classes/baseComponent.mjs` around lines 465 - 468, The constructor in
BaseSelectableComponent incorrectly assigns the result of super(...) to options
(super/constructor doesn't return a value); change it to call super(options,
...args) without assignment and then read options from this.options (e.g., set
this.itemID = this.options.itemID || this.id). Update the constructor body in
BaseSelectableComponent to stop using the returned value from BaseComponent and
instead use this.options, preserving fallback to this.id.
There was a problem hiding this comment.
This fix of options = super(options, ...args).options; logics is already present in the latest v3 branch, this is just outdated.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of BaseSelectableComponent in the release/v3 branch
git fetch origin release/v3:release/v3 2>/dev/null || true
git show origin/release/v3:ui/classes/baseComponent.mjs | tail -30
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check available branches and try to access release/v3 differently
git branch -a | grep -i release
# Try to fetch and show the file from release/v3
git fetch origin release/v3 2>&1
# List recent commits on release/v3 if it exists
git log origin/release/v3 --oneline -10 2>&1 | head -20
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Show the BaseSelectableComponent from the fetched release/v3
git show release/v3:ui/classes/baseComponent.mjs | grep -A 10 "export class BaseSelectableComponent"
# Also check if there are recent commits that mention this fix
git log release/v3 --oneline --all --grep="constructor\|BaseSelectableComponent\|options" -10
Length of output: 1171
@Aiosa, you're absolutely correct! I've verified that the release/v3 branch already contains the fix:
constructor(options, ...args) {
options = super(options, ...args).options;
this.itemID = options.itemID || this.id;
}This was introduced in commit 7bfdb0e ("feat: new layout manager, UI BaseComponent now makes options optional arg").
The dev-vrubel branch needs to be rebased against release/v3 to pick up this fix along with other updates from the base branch. This would also help resolve any potential merge conflicts and ensure the PR includes the latest improvements.
🧬 Code Graph Analysis Results
[ui/classes/baseComponent.mjs, lines 260-264]
export class BaseSelectableComponent extends BaseComponent {
constructor(options, ...args) {
options = super(options, ...args);
this.itemID = options.itemID || this.id;
}
setSelected(itemID) {
throw new Error("Component must override setSelected method");
}
}
🧠 Learnings used
Learnt from: Aiosa
Repo: RationAI/xopat PR: 147
File: plugins/analyze/analyzeDropdown.mjs:0-0
Timestamp: 2026-01-25T11:33:05.489Z
Learning: In the analyze plugin (plugins/analyze/analyzeDropdown.mjs), the current Empaia-specific implementation (EmpaiaStandaloneJobs API, app ID extraction with app?.id || app?.app_id, hardcoded case IDs) is accepted as temporary technical debt. Future refactoring should add abstraction layers to support multiple app sources including DICOM, HuggingFace, and other APIs through a plugin-level adapter pattern.
| setMenu(items = [], onClick) { | ||
| const node = document.createElement('div'); | ||
| node.className = 'p-2'; | ||
| const ul = document.createElement('ul'); ul.className = 'menu bg-transparent p-0'; ul.setAttribute('role','menu'); | ||
| for (let i = 0; i < items.length; i++) { | ||
| const it = items[i]; | ||
| const label = (typeof it === 'string') ? it : (it && it.label) || String(it); | ||
| const li = document.createElement('li'); li.setAttribute('role','none'); | ||
| const a = document.createElement('a'); | ||
| a.className = 'flex items-center gap-3 rounded-md px-3 py-2 hover:bg-base-300 focus:bg-base-300'; | ||
| a.setAttribute('role','menuitem'); a.setAttribute('tabindex','-1'); | ||
| a.textContent = label; |
There was a problem hiding this comment.
Menu items are not keyboard reachable.
tabindex="-1" on <a> without roving focus means users can’t tab into the menu. This is an accessibility blocker.
🛠️ Suggested fix (make items focusable)
- a.setAttribute('role','menuitem'); a.setAttribute('tabindex','-1');
+ a.setAttribute('role','menuitem'); a.setAttribute('tabindex','0');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setMenu(items = [], onClick) { | |
| const node = document.createElement('div'); | |
| node.className = 'p-2'; | |
| const ul = document.createElement('ul'); ul.className = 'menu bg-transparent p-0'; ul.setAttribute('role','menu'); | |
| for (let i = 0; i < items.length; i++) { | |
| const it = items[i]; | |
| const label = (typeof it === 'string') ? it : (it && it.label) || String(it); | |
| const li = document.createElement('li'); li.setAttribute('role','none'); | |
| const a = document.createElement('a'); | |
| a.className = 'flex items-center gap-3 rounded-md px-3 py-2 hover:bg-base-300 focus:bg-base-300'; | |
| a.setAttribute('role','menuitem'); a.setAttribute('tabindex','-1'); | |
| a.textContent = label; | |
| setMenu(items = [], onClick) { | |
| const node = document.createElement('div'); | |
| node.className = 'p-2'; | |
| const ul = document.createElement('ul'); ul.className = 'menu bg-transparent p-0'; ul.setAttribute('role','menu'); | |
| for (let i = 0; i < items.length; i++) { | |
| const it = items[i]; | |
| const label = (typeof it === 'string') ? it : (it && it.label) || String(it); | |
| const li = document.createElement('li'); li.setAttribute('role','none'); | |
| const a = document.createElement('a'); | |
| a.className = 'flex items-center gap-3 rounded-md px-3 py-2 hover:bg-base-300 focus:bg-base-300'; | |
| a.setAttribute('role','menuitem'); a.setAttribute('tabindex','0'); | |
| a.textContent = label; |
🤖 Prompt for AI Agents
In `@ui/classes/components/sidePanel.mjs` around lines 47 - 58, The menu items
created in setMenu currently set each anchor's tabindex to "-1", making them
unfocusable by keyboard; update setMenu (in sidePanel.mjs) so each menu item
anchor (the variable a) is keyboard-focusable—either remove the tabindex
attribute or set it to "0" and ensure role="menuitem" remains, and if you plan
roving-focus keep tabindex="-1" only on non-active items and implement keyboard
handlers to manage focus movement; adjust the creation of a (the anchor) and any
focus-init logic so users can tab into and navigate the menu with the keyboard.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| showNear(anchor, opts = {}) { | ||
| try { | ||
| const el = this.create(); | ||
| this.attachToBody(); | ||
| const rect = (anchor && anchor.getBoundingClientRect) ? anchor.getBoundingClientRect() : (anchor || { left: 0, right: 0, top: 0 }); | ||
| const nudge = (opts.nudge === undefined) ? 1 : opts.nudge; | ||
| const offsetY = opts.offsetY || 0; | ||
| requestAnimationFrame(() => { | ||
| try { | ||
| const panelEl = document.getElementById(this.id); | ||
| const panelW = panelEl && panelEl.offsetWidth ? panelEl.offsetWidth : (typeof this.width === 'number' ? this.width : this.minWidth); | ||
| const panelH = panelEl && panelEl.offsetHeight ? panelEl.offsetHeight : 0; | ||
| let left = rect.right; | ||
| if (left + panelW > window.innerWidth - 8) { | ||
| left = Math.max(8, rect.left - panelW); | ||
| } else { | ||
| left = Math.max(8, left - nudge); | ||
| } | ||
| let top = Math.max(8, rect.top + offsetY); | ||
| // clamp vertically so the panel stays within viewport when possible | ||
| if (panelH && (top + panelH > window.innerHeight - 8)) { | ||
| top = Math.max(8, window.innerHeight - panelH - 8); | ||
| } | ||
| panelEl.style.left = `${left}px`; | ||
| panelEl.style.top = `${top}px`; | ||
| panelEl.style.display = ''; | ||
| // ensure hover keeps it open | ||
| panelEl.addEventListener('mouseenter', () => { this.cancelHide(); }); | ||
| panelEl.addEventListener('mouseleave', () => { this.scheduleHide(); }); | ||
| } catch (e) { /* swallow layout issues */ } |
There was a problem hiding this comment.
Avoid re‑attaching hover listeners on every showNear call.
Each call adds new listeners, which can stack and cause multiple timers. Add a one‑time guard.
🛠️ Suggested fix
- // ensure hover keeps it open
- panelEl.addEventListener('mouseenter', () => { this.cancelHide(); });
- panelEl.addEventListener('mouseleave', () => { this.scheduleHide(); });
+ // ensure hover keeps it open (bind once)
+ if (!panelEl.__sidePanelHoverBound) {
+ panelEl.addEventListener('mouseenter', () => { this.cancelHide(); });
+ panelEl.addEventListener('mouseleave', () => { this.scheduleHide(); });
+ panelEl.__sidePanelHoverBound = true;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| showNear(anchor, opts = {}) { | |
| try { | |
| const el = this.create(); | |
| this.attachToBody(); | |
| const rect = (anchor && anchor.getBoundingClientRect) ? anchor.getBoundingClientRect() : (anchor || { left: 0, right: 0, top: 0 }); | |
| const nudge = (opts.nudge === undefined) ? 1 : opts.nudge; | |
| const offsetY = opts.offsetY || 0; | |
| requestAnimationFrame(() => { | |
| try { | |
| const panelEl = document.getElementById(this.id); | |
| const panelW = panelEl && panelEl.offsetWidth ? panelEl.offsetWidth : (typeof this.width === 'number' ? this.width : this.minWidth); | |
| const panelH = panelEl && panelEl.offsetHeight ? panelEl.offsetHeight : 0; | |
| let left = rect.right; | |
| if (left + panelW > window.innerWidth - 8) { | |
| left = Math.max(8, rect.left - panelW); | |
| } else { | |
| left = Math.max(8, left - nudge); | |
| } | |
| let top = Math.max(8, rect.top + offsetY); | |
| // clamp vertically so the panel stays within viewport when possible | |
| if (panelH && (top + panelH > window.innerHeight - 8)) { | |
| top = Math.max(8, window.innerHeight - panelH - 8); | |
| } | |
| panelEl.style.left = `${left}px`; | |
| panelEl.style.top = `${top}px`; | |
| panelEl.style.display = ''; | |
| // ensure hover keeps it open | |
| panelEl.addEventListener('mouseenter', () => { this.cancelHide(); }); | |
| panelEl.addEventListener('mouseleave', () => { this.scheduleHide(); }); | |
| } catch (e) { /* swallow layout issues */ } | |
| showNear(anchor, opts = {}) { | |
| try { | |
| const el = this.create(); | |
| this.attachToBody(); | |
| const rect = (anchor && anchor.getBoundingClientRect) ? anchor.getBoundingClientRect() : (anchor || { left: 0, right: 0, top: 0 }); | |
| const nudge = (opts.nudge === undefined) ? 1 : opts.nudge; | |
| const offsetY = opts.offsetY || 0; | |
| requestAnimationFrame(() => { | |
| try { | |
| const panelEl = document.getElementById(this.id); | |
| const panelW = panelEl && panelEl.offsetWidth ? panelEl.offsetWidth : (typeof this.width === 'number' ? this.width : this.minWidth); | |
| const panelH = panelEl && panelEl.offsetHeight ? panelEl.offsetHeight : 0; | |
| let left = rect.right; | |
| if (left + panelW > window.innerWidth - 8) { | |
| left = Math.max(8, rect.left - panelW); | |
| } else { | |
| left = Math.max(8, left - nudge); | |
| } | |
| let top = Math.max(8, rect.top + offsetY); | |
| // clamp vertically so the panel stays within viewport when possible | |
| if (panelH && (top + panelH > window.innerHeight - 8)) { | |
| top = Math.max(8, window.innerHeight - panelH - 8); | |
| } | |
| panelEl.style.left = `${left}px`; | |
| panelEl.style.top = `${top}px`; | |
| panelEl.style.display = ''; | |
| // ensure hover keeps it open (bind once) | |
| if (!panelEl.__sidePanelHoverBound) { | |
| panelEl.addEventListener('mouseenter', () => { this.cancelHide(); }); | |
| panelEl.addEventListener('mouseleave', () => { this.scheduleHide(); }); | |
| panelEl.__sidePanelHoverBound = true; | |
| } | |
| } catch (e) { /* swallow layout issues */ } |
🤖 Prompt for AI Agents
In `@ui/classes/components/sidePanel.mjs` around lines 75 - 104, In showNear,
listeners for 'mouseenter' and 'mouseleave' are being added every time which
stacks handlers; modify showNear (or the element creation path in
create/attachToBody) to add those hover listeners only once by guarding with a
flag or element marker (e.g., this._hoverListenersAttached or setting an
attribute like panelEl.dataset.hoverAttached) before calling
panelEl.addEventListener(...), and set that flag/attribute after attaching so
subsequent calls to showNear skip re-attaching; keep using this.cancelHide and
this.scheduleHide as the handler functions.
|
|
||
| // safe translation helper: return translated value or fallback when missing | ||
| const tOr = (key, fallback) => { | ||
| if (typeof $?.t === 'function') { |
There was a problem hiding this comment.
Again, checking xopat API definitions is just a bloat, it will be defined. Remove this chat bloat.
| ? document.getElementById(element) | ||
| : element; | ||
|
|
||
| // If we got a jQuery/Cash wrapper, unwrap to the first DOM node |
There was a problem hiding this comment.
In general the API was not designed to work with a jquery - when you wanted to use it, you had to supply a DOM node. Is there a need to add the jquery support? I would like to keep things simple where possible.
| mount.append(this.create()); | ||
| } else if (mount.nodeType || mount instanceof Node) { | ||
| // Fallback for very old environments where append may be missing | ||
| const created = this.create(); |
There was a problem hiding this comment.
I see a lot of fixes for attachTo method, but there are other attaching logics elsewhere. This is the CORE. If you do immature changes, you change EVERY component - in case you NEED these stability improvements, make sure all of the API is CONSISTENT. what about prependedTo? etc.
Summary by CodeRabbit
Release Notes
New Features
UI Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.