got-feedBack/feedBack
1
0
Fork 0
mirror of https://github.com/got-feedBack/feedBack.git synced 2026-08-11 03:09:57 +00:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

fix(panes): review fixes — stranded elements, duplicate listener, class clobber

Browse Source 
Three real findings from CodeRabbit on #928, all in current code.

1. closePane() adopted the element out of the pane window ONLY when its
   original home was still connected. If the panel never had a parent (a
   plugin that builds it lazily and hands it straight over) or its container
   was torn down while the pane was out (a screen change), the whole block was
   skipped — leaving the element inside a window we then close, which strips
   every listener in its subtree. That is exactly the "comes home dead" failure
   this ordering exists to prevent; the guard just moved it from the common
   path to the rare one, where it is far harder to spot.

   Adopting and re-homing are two different jobs and only one of them is
   allowed to fail. Adopt UNCONDITIONALLY — that is what rescues the element —
   and insert only when there is somewhere to insert it. With no home the
   element ends up owned by this document but not in it: detached, intact,
   listeners alive, ready for the plugin to re-insert.

2. The pane window's `beforeunload` handler was registered TWICE, comment block
   and all — a bad scripted edit on my part. Harmless (the handler is
   idempotent via panes.isOpen) but dead duplicate code. Also fixed the stale
   comment further down that still claimed there was no beforeunload listener
   at all.

3. _copyStyles ASSIGNED className on the pane document's <html> and <body>
   instead of merging. pane.html sets `class="fb-pane-window"` on <html>, and
   panes.css hangs the pane window's own chrome off exactly that — so copying
   the app's classes over it silently took the pane window's own layout with
   them. Merge both class lists, and append the interface-scale inline style
   rather than replacing the attribute.

Also guarded the docs' integration example behind a feedBack.panes check: the
doc says the API is optional, and then showed an example that would throw on a
host without it.

Not applicable (reviewed against e5cbea2, the branch's first commit, before the
rebuild): the prototype-pollution findings in pane-bridge.js and pane-mirror.js,
and the `panes[]` manifest validation in plugins/__init__.py. All three files are
gone — 188bdaa deleted the entire cross-realm bridge, mirrorGlobal, and manifest
layer when panes switched to moving the real DOM node.

Signed-off-by: topkoa <topkoa@gmail.com>
This commit is contained in:
topkoa
2026-07-12 19:04:10 -04:00
parent a7348052ae
commit 859b0036e5
3 changed files with 58 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
static/panes/pane-manager.js
+31 -8
View File
@@ -168,17 +168,40 @@
// Adopt first, while the pane window is still alive, and the node moves out
// of a living document into a living document, which is the only case the
// DOM actually guarantees.
const home = entry.home;
if (home && home.parent && home.parent.isConnected) {
try {
// adoptNode, not appendChild: the node's owner is currently the pane
// window's document, and adopting is what transfers ownership back.
const node = document.adoptNode(entry.el);
// ADOPT UNCONDITIONALLY, INSERT CONDITIONALLY. The rescue and the
// re-homing are two different jobs, and only one of them is allowed to
// fail.
//
// Adopting is what saves the element: it transfers ownership away from the
// pane window's document, so that document can be destroyed without taking
// the listeners with it. Do that FIRST, and always — even when there is
// nowhere to put the element afterwards.
//
// Re-homing can legitimately be impossible: the panel may never have had a
// parent (a plugin that builds it lazily and hands it straight to us), or
// its container may have been torn down while the pane was out (a screen
// change). Gating the adopt on a reachable home would mean that in exactly
// those cases we leave the element inside a window we are about to close —
// which is the "comes home dead" failure this whole ordering exists to
// prevent. It just moves it from the common path to the rare one, where it
// is far harder to spot.
//
// With no home, the element ends up owned by this document but not in it:
// detached, intact, listeners alive, and ready for the plugin to re-insert
// whenever it rebuilds its UI.
try {
// adoptNode, not appendChild: the node's owner is currently the pane
// window's document, and adopting is what transfers ownership back.
const node = document.adoptNode(entry.el);
const home = entry.home;
if (home && home.parent && home.parent.isConnected) {
if (home.next && home.next.parentNode === home.parent) home.parent.insertBefore(node, home.next);
else home.parent.appendChild(node);
} catch (e) {
console.error('[panes] could not return', id, 'to its home', e);
} else {
console.warn('[panes]', id, 'has no home to return to — the element is detached but intact');
}
} catch (e) {
console.error('[panes] could not bring', id, 'back out of its pane window', e);
}
const host = hosts.get(entry.hostId);
static/panes/pane-window-host.js
+15 -28
View File
@@ -61,10 +61,19 @@
// Carry the theme/scale hooks the app hangs on <html> and <body>. v3 keys
// off these for its colour tokens and interface scale, and a panel that
// lands without them renders in the wrong palette at the wrong size.
//
// MERGE, don't assign: pane.html sets `class="fb-pane-window"` on <html>,
// and panes.css hangs the pane window's own chrome off it. Overwriting the
// class list would take that with it and the window would lose its own
// layout — the app's classes and the pane document's are both wanted.
try {
doc.documentElement.className = document.documentElement.className;
doc.documentElement.setAttribute('style', document.documentElement.getAttribute('style') || '');
doc.body.className = document.body.className;
document.documentElement.classList.forEach((c) => doc.documentElement.classList.add(c));
document.body.classList.forEach((c) => doc.body.classList.add(c));
// The inline style on <html> carries the interface-scale custom property
// (--fb-scale). Merge it in rather than replacing the attribute, for the
// same reason.
const scale = document.documentElement.style.cssText;
if (scale) doc.documentElement.style.cssText += ';' + scale;
} catch (e) { /* non-fatal */ }
}
@@ -168,26 +177,6 @@
if (panes.isOpen(spec.id)) panes.close(spec.id);
});
// THE ELEMENT MUST LEAVE BEFORE THE DOCUMENT DIES.
//
// When the user closes a pane window, its document is torn down — and the
// panel is inside it. The node itself survives (we hold a reference) and
// comes home looking perfect: right markup, right classes, right size. But
// it comes home DEAD: every event listener in the subtree is gone with the
// document that hosted them. A panel that renders and does nothing.
//
// The `closed` poll cannot save us: by the time `w.closed` is true, the
// document is already gone. `beforeunload` fires while it is still alive, so
// this is the last moment we can get the element out — and panes.close()
// adopts it back into the main document synchronously.
//
// We attach it HERE, not when the window was opened: back then the window
// still held its throwaway about:blank document, and a listener registered
// on that is discarded when /pane replaces it.
w.addEventListener('beforeunload', () => {
if (panes.isOpen(spec.id)) panes.close(spec.id);
});
// Measure LATE. The pane window has not laid out yet at this point (it is
// still being created and shown), so anything read now reports 0x0 whether
// or not there is a real problem.
@@ -245,11 +234,9 @@
panes.close(spec.id); // never strand the element in a dead window
});
// Note there is no 'beforeunload' listener on the popup. A listener added
// now would be attached to its throwaway about:blank window and thrown away
// with it when /pane loads. The `closed` poll above is what notices the user
// shutting a pane window — and it has to be, since a crashed renderer never
// gets to say goodbye either.
// The pane window's 'beforeunload' listener is registered in _adopt(), NOT
// here: a listener added now would attach to the window's throwaway
// about:blank document and be discarded when /pane replaces it.
}
function unplace(id, el) {