Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
!/modules/web-worker-manager

/plugins/**
!/plugins/analyze
!/plugins/analyze/**
!/plugins/annotations
!/plugins/custom-pages
!/plugins/empaia
Expand Down
24 changes: 24 additions & 0 deletions plugins/analyze/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@

Analyze plugin
Copy link
Member

Choose a reason for hiding this comment

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

Documentation? Awesome!

========================

Purpose
-------
Adds an "Analyze" tab to the AppBar with two actions: a "Run Recent →" anchor that opens a right-side recent-jobs panel, and "Create New App" which creates floating window with new app form.

Files
-----
- `analyzeDropdown.mjs` - registers the tab and wires dropdown items.
- `newAppForm.mjs` - the floating form used by "Create New App".

How to use
----------
- Provide recent jobs by passing `params.recentJobs` or saving `recentJobs` via plugin options.
- Handle job clicks by implementing `onJobClick({ index, label })` on the plugin instance.
- Provide `params.onCreate` to receive form submission data from `NewAppForm`.

Implementation notes
--------------------
- UI behaviors (menu, positioning, hover) are implemented in `SidePanel` (`setMenu`, `showNear`, `scheduleHide`, `cancelHide`) — reuse it for other flyouts.
- `SidePanel.hide()` currently removes the element; consider switching to `display:none` if you need faster show/hide cycles.

459 changes: 459 additions & 0 deletions plugins/analyze/analyzeDropdown.mjs

Large diffs are not rendered by default.

10 changes: 10 additions & 0 deletions plugins/analyze/include.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"id": "analyze",
"name": "Analyze dropdown in Main Menu",
"author": "Filip Vrubel",
"version": "1.0.0",
"description": "Plugin for creating and running jobs",
"icon": null,
"includes" : ["newAppForm.mjs", "analyzeDropdown.mjs"],
Copy link
Member

Choose a reason for hiding this comment

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

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.

"permaLoad": true
}
194 changes: 194 additions & 0 deletions plugins/analyze/newAppForm.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
import van from "../../ui/vanjs.mjs";
Copy link
Member

Choose a reason for hiding this comment

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

van is simplify available as a global, e.g. van or globalThis.van

import { BaseComponent } from "../../ui/classes/baseComponent.mjs";
import { FloatingWindow } from "../../ui/classes/components/floatingWindow.mjs";
Copy link
Member

Choose a reason for hiding this comment

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

This import isn’t correct. Unfortunatelly, our core system is still vanilla-js-first, so also the UI is compiled and available in the system through UI. You should not import the UI here like this, since it will force double-import of items. Instead, access the elements using UI -> UI.BaseComponent. If you want to get explicit you can use globalThis.UI...


const { div, input, textarea, span, button, label, h3 } = van.tags;

class NewAppForm extends BaseComponent {
constructor(options = {}) {
options = super(options).options;
this.id = options.id || 'new-app';
Copy link
Member

Choose a reason for hiding this comment

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

Not good ID. You are not alone. Prefer IDs that are more complex, at best, namespaced, e.g. by your plugin ID.

this.onSubmit = options.onSubmit || (() => {});
this.values = options.values || {};
this._el = null;
this._fields = {};
this._floatingWindow = null;
}

_row(labelText, fieldNode) {
return div({ class: 'mb-2' },
label({ class: 'block mb-1 text-xs font-medium' }, labelText),
fieldNode
);
}

create() {
if (this._el) return this._el;

// use explicit inline sizing so styles apply even if utility classes are missing
const smallInputStyle = 'height:30px;padding:4px 6px;font-size:12px;line-height:1.1;box-sizing:border-box;';
const smallTextareaBase = 'padding:6px 6px;font-size:12px;line-height:1.2;box-sizing:border-box;resize:vertical;';
const schemaEl = input({ type: 'text', id: this.id + '-schema', class: 'input w-full', style: smallInputStyle, value: this.values.schema || '' });
const nameEl = input({ type: 'text', id: this.id + '-name', class: 'input w-full', style: smallInputStyle, value: this.values.name || '' });
const nsEl = input({ type: 'text', id: this.id + '-namespace', class: 'input w-full', style: smallInputStyle, value: this.values.namespace || '' });
const descEl = textarea({ id: this.id + '-description', class: 'textarea w-full', style: smallTextareaBase + 'height:64px;', rows: 4 }, this.values.description || '');
const inputsEl = textarea({ id: this.id + '-inputs', class: 'textarea w-full', style: smallTextareaBase + 'height:48px;', rows: 3 }, this.values.inputs || '');
const outputsEl = textarea({ id: this.id + '-outputs', class: 'textarea w-full', style: smallTextareaBase + 'height:48px;', rows: 3 }, this.values.outputs || '');
const jobEl = input({ type: 'text', id: this.id + '-joburl', class: 'input w-full', style: smallInputStyle, value: this.values.jobUrl || '' });

const btnEdit = button({ class: 'btn btn-secondary btn-sm mr-2', type: 'button', onclick: (ev) => this._onEdit() }, 'Edit EAD');
const btnCreate = button({ class: 'btn btn-primary btn-sm', type: 'button', onclick: (ev) => this._onCreate() }, 'Create');

// remove inner title and close button to rely on the FloatingWindow header
// increase top padding so the FloatingWindow title has breathing room
const form = div({ class: 'p-4 bg-base-200 border border-base-300 rounded-md max-w-full relative', style: 'max-width:420px;width:100%;' },
this._row('Schema:', schemaEl),
this._row('Name:', nameEl),
this._row('Namespace:', nsEl),
this._row('Description:', descEl),
this._row('Inputs:', inputsEl),
this._row('Outputs:', outputsEl),
this._row('Job URL:', jobEl),
div({ class: 'mt-4 flex gap-2 justify-end' }, btnEdit, btnCreate)
);

this._fields = {
schema: schemaEl,
name: nameEl,
namespace: nsEl,
description: descEl,
inputs: inputsEl,
outputs: outputsEl,
jobUrl: jobEl
};

// make the form fill available height inside a FloatingWindow and layout vertically
try {
this._el = form;
// allow flex layout to let textareas expand/shrink with window
this._el.style.display = 'flex';
this._el.style.flexDirection = 'column';
this._el.style.height = '100%';

// ensure textareas grow and shrink with the container (min-height:0 prevents overflow)
const makeFlexTA = (el) => {
if (!el) return;
el.style.flex = '1 1 auto';
el.style.minHeight = '0';
// keep existing padding/box sizing
};
makeFlexTA(this._fields.description);
makeFlexTA(this._fields.inputs);
makeFlexTA(this._fields.outputs);

// push buttons to bottom
const btnContainer = this._el.querySelector('.mt-4');
if (btnContainer) btnContainer.style.marginTop = 'auto';
} catch (e) {
// non-fatal: fall back to previous behavior
this._el = form;
}

return this._el;
}

/**
* Close helper: prefer closing a floating window if we opened one;
* otherwise close the parent Dialog via Dialogs.closeWindow if present,
* fallback to removing the element from DOM (legacy behavior).
*/
_close() {
try {
if (this._floatingWindow) {
try { this._floatingWindow.close(); } catch(_) {}
this._floatingWindow = null;
return;
}
// if inside a Dialog created via Dialogs.showCustom, find the dialog root
let root = this._el && this._el.closest ? this._el.closest('[data-dialog="true"]') : null;
if (root && root.id && window.USER_INTERFACE && USER_INTERFACE.Dialogs && typeof USER_INTERFACE.Dialogs.closeWindow === 'function') {
try { USER_INTERFACE.Dialogs.closeWindow(root.id); return; } catch (_) {}
}
// fallback: traditional removal of the attached wrapper
const el = this._el;
if (!el) return;
const wrapper = el.parentNode;
if (wrapper && wrapper.parentNode) {
wrapper.parentNode.removeChild(wrapper);
} else if (el.parentNode) {
el.parentNode.removeChild(el);
}
} catch (e) { console.error('NewAppForm _close error', e); }
}

_serialize() {
const f = this._fields;
const getVal = (el) => (el && el.value !== undefined) ? el.value : (el && el.textContent) || '';
return {
schema: getVal(f.schema),
name: getVal(f.name),
namespace: getVal(f.namespace),
description: getVal(f.description),
inputs: getVal(f.inputs),
outputs: getVal(f.outputs),
jobUrl: getVal(f.jobUrl),
};
}

_onEdit() {
}
Comment on lines +138 to +139
Copy link

@coderabbitai coderabbitai bot Jan 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

@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(_) {}
}
Comment on lines +141 to +147
Copy link

@coderabbitai coderabbitai bot Jan 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Member

Choose a reason for hiding this comment

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

I dont understand, it closes the window if it is defined - its just a sanity check.

Copy link

Choose a reason for hiding this comment

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

@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:

  1. FloatingWindow (via showFloating()) — ✅ current code handles this
  2. Dialog (via Dialogs.showCustom or similar) — ❌ current code doesn't close this
  3. 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.closeWindow if 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.

} catch (e) {
console.error('NewAppForm onSubmit error', e);
}
}

/**
* Show this form inside a FloatingWindow. Returns the FloatingWindow instance.
* Options may include width/height/title.
*/
showFloating(opts = {}) {
try {
if (this._floatingWindow) return this._floatingWindow;
const id = this.id + '-window';
// compute centered start position when possible
const width = opts.width || 420;
// increase default height so the form fits comfortably; still allows scrolling
const height = opts.height || 520;
const startLeft = (typeof window !== 'undefined') ? Math.max(8, Math.round((window.innerWidth - width) / 2)) : (opts.startLeft || 64);
const startTop = (typeof window !== 'undefined') ? Math.max(8, Math.round((window.innerHeight - height) / 2)) : (opts.startTop || 64);
const w = new FloatingWindow({ id, title: opts.title || 'New App', width, height, startLeft, startTop, onClose: () => { this._floatingWindow = null; } }, );
// attach window to body so it is visible
w.attachTo(document.body);
// wrap the form in a scrollable card-body so FloatingWindow keeps expected layout
const wrapper = document.createElement('div');
wrapper.className = 'card-body p-3 gap-2 overflow-auto';
wrapper.style.height = '100%';
wrapper.appendChild(this.create());
// set the body to our wrapper node
w.setBody(wrapper);
this._floatingWindow = w;
return w;
} catch (e) { console.error('NewAppForm showFloating error', e); }
return null;
}

attachTo(parent) {
const target = (typeof parent === 'string') ? document.getElementById(parent) : parent;
if (!target) throw new Error('attachTo: parent not found');
target.appendChild(this.create());
}

getValues() {
return this._serialize();
}
}

export { NewAppForm };
74 changes: 68 additions & 6 deletions ui/classes/baseComponent.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -96,37 +96,67 @@ export class BaseComponent {
}

/**
*
* @param {*} element - The element to attach the component to
* @return {BaseComponent} builder pattern
*/
attachTo(element) {
this.refreshClassState();
this.refreshPropertiesState();

if (element instanceof BaseComponent) {
// 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 {
Comment on lines +106 to 118
Copy link

@coderabbitai coderabbitai bot Jan 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate the baseComponent.mjs file
find . -name "baseComponent.mjs" -type f

Repository: 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.mjs

Repository: RationAI/xopat

Length of output: 584


🏁 Script executed:

# Check for _children property usage and initialization
rg "_children" ui/classes/baseComponent.mjs

Repository: RationAI/xopat

Length of output: 767


🏁 Script executed:

# Look for test files related to attachTo
find . -name "*.test.*" -o -name "*.spec.*" | head -20

Repository: 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 -20

Repository: 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.mjs

Repository: 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 -5

Repository: 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 2

Repository: 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 -40

Repository: 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.

Suggested change
// 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.

Copy link
Member

Choose a reason for hiding this comment

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

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..

Copy link

Choose a reason for hiding this comment

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

🧩 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) extend BaseComponent
  • All attachTo() calls use proper BaseComponent instances created with new
  • No plain objects with {create, id} properties being passed to attachTo() anywhere in the codebase

The isComponentLike abstraction appears to be premature/unused, which creates unnecessary complexity. You have two options:

  1. Remove the abstraction (recommended if truly unused):

    • Revert to simple instanceof BaseComponent check
    • Simpler, clearer code matching your existing patterns
  2. If keeping it (for future extensibility), you must guard _children access 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.

const mount = typeof element === "string"
// Resolve mount target from id/string or direct reference
let mount = typeof element === "string"
? document.getElementById(element)
: element;

// If we got a jQuery/Cash wrapper, unwrap to the first DOM node
Copy link
Member

Choose a reason for hiding this comment

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

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.

// (supports libraries exposing .jquery flag or .get/.[0])
if (mount && (mount.jquery || typeof mount.get === "function" || Array.isArray(mount))) {
const candidate = typeof mount.get === "function" ? mount.get(0) : (Array.isArray(mount) ? mount[0] : mount[0]);
if (candidate) mount = candidate;
}

if (!mount) {
console.error(`Element ${element} not found`);
van.add(element, this.create());
} else {
try {
van.add(element, this.create());
} catch (_) { /* noop: element may be invalid for van.add */ }
} else if (typeof mount.append === "function") {
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();
Copy link
Member

Choose a reason for hiding this comment

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

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.

if (mount.appendChild) mount.appendChild(created);
else {
try { mount.innerHTML += created.outerHTML || String(created); } catch (_) {}
}
} else {
// Last resort: try jQuery-style append if available or log a clearer error
try { mount.append(this.create()); }
catch (e) {
console.error("Failed to attach component: unsupported mount target", mount);
console.error(e);
}
}
}
return this;
}

/**
*
* @param {*} element - The element to prepend the component to
* @return {BaseComponent} builder pattern
*/
prependedTo(element) {
this.refreshClassState();
Expand All @@ -151,6 +181,7 @@ export class BaseComponent {
mount.prepend(this.create());
}
}
return this;
}

/**
Expand Down Expand Up @@ -267,6 +298,22 @@ export class BaseComponent {
this.classState.val = Object.values(this.classMap).join(" ");
}

/**
* Toggle the class of the component
* @param {string} key
* @param {string} value
* @param {boolean} on if true, set class
*/
toggleClass(key, value, on=true) {
this.classMap[key] = on ? value : "";
this.classState.val = Object.values(this.classMap).join(" ");
}

/**
* Set attribute property to the element
* @param {string} key attribute name
* @param {string} value
*/
setExtraProperty(key, value) {
this.propertiesMap[key] = value;
let stateMap = this.propertiesStateMap[key];
Expand Down Expand Up @@ -409,3 +456,18 @@ export class BaseComponent {
this.refreshPropertiesState();
}
}

/**
* @typedef {BaseUIOptions} SelectableUIOptions
* @property {string} [itemID] - The selection ID
*/
export class BaseSelectableComponent extends BaseComponent {
constructor(options, ...args) {
options = super(options, ...args);
this.itemID = options.itemID || this.id;
}
Comment on lines +465 to +468
Copy link

@coderabbitai coderabbitai bot Jan 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "baseComponent.mjs" -type f

Repository: RationAI/xopat

Length of output: 88


🏁 Script executed:

# First, let's locate and read the file
fd baseComponent.mjs

Repository: 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 -100

Repository: RationAI/xopat

Length of output: 4368


🏁 Script executed:

# Search for BaseSelectableComponent usage and instantiation
rg "BaseSelectableComponent" -A 2 -B 2

Repository: 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 -20

Repository: 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 -5

Repository: 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.

Copy link
Member

Choose a reason for hiding this comment

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

This fix of options = super(options, ...args).options; logics is already present in the latest v3 branch, this is just outdated.

Copy link

Choose a reason for hiding this comment

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

🧩 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.


setSelected(itemID) {
throw new Error("Component must override setSelected method");
}
}
Loading