mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-13 18:46:37 +00:00
fix(web): keep layout-leaf style-editor columns distinct from name collisions
columnsFor() keyed a layout-only leaf field's column by its bare field
name. If an unrelated element's style block or another element's layout
axis block happened to declare a sub-field with that same name, the
`!seen.has(key)` guard skipped creating the leaf's column, silently
dropping its only control again -- the same failure the leaf-column fix
was meant to close, just reached through a name collision (CodeRabbit
review on 324a7ea).
Key layout-leaf columns under a namespaced id so they can never be
shadowed by an unrelated column sharing their name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -38,6 +38,7 @@ nothing is listening, so it stays useful in a bare checkout.
|
|||||||
| `unit/test_render_cards.js` | no | `renderInstalledCards` markup, both empty states, and HTML-escaping of hostile plugin metadata |
|
| `unit/test_render_cards.js` | no | `renderInstalledCards` markup, both empty states, and HTML-escaping of hostile plugin metadata |
|
||||||
| `unit/test_style_editor_element_keys.js` | no | `elementKeys()` from `widgets/style-editor.js`: a hand-written `customization.layout` entry with no style block of its own still gets a row, since the widget claims the whole `layout` block from the generic fallback renderer |
|
| `unit/test_style_editor_element_keys.js` | no | `elementKeys()` from `widgets/style-editor.js`: a hand-written `customization.layout` entry with no style block of its own still gets a row, since the widget claims the whole `layout` block from the generic fallback renderer |
|
||||||
| `unit/test_style_editor_layout_leaf_columns.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only key whose own value is a leaf (no x/y sub-object, e.g. a `show_logo` toggle) gets a self-keyed column instead of a blank, uneditable row |
|
| `unit/test_style_editor_layout_leaf_columns.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only key whose own value is a leaf (no x/y sub-object, e.g. a `show_logo` toggle) gets a self-keyed column instead of a blank, uneditable row |
|
||||||
|
| `unit/test_style_editor_layout_leaf_collision.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only leaf key still gets its own column even when its name collides with an unrelated element's style sub-field or another layout axis's sub-field |
|
||||||
| `dom/test_installed_dom.js` | yes | The toolbar in a real DOM: pill/search/sort interaction, the HTMX partial re-swap, and a `getComputedStyle` check that `.filter-pill[data-active]` really matches the emitted markup |
|
| `dom/test_installed_dom.js` | yes | The toolbar in a real DOM: pill/search/sort interaction, the HTMX partial re-swap, and a `getComputedStyle` check that `.filter-pill[data-active]` really matches the emitted markup |
|
||||||
| `dom/test_store_dom.js` | yes | Store pagination, per-page, category, tri-state Installed button, and persistence across a re-boot, against the live registry |
|
| `dom/test_store_dom.js` | yes | Store pagination, per-page, category, tri-state Installed button, and persistence across a re-boot, against the live registry |
|
||||||
| `dom/test_no_double_fetch.js` | yes | Loads the **whole** `plugins_manager.js` and counts requests: typing in the store search must filter the cached list, not refetch `/api/v3/plugins/store/list` |
|
| `dom/test_no_double_fetch.js` | yes | Loads the **whole** `plugins_manager.js` and counts requests: typing in the store search must filter the cached list, not refetch `/api/v3/plugins/store/list` |
|
||||||
|
|||||||
+2
-1
@@ -16,7 +16,8 @@ const fs = require('fs');
|
|||||||
const BASE = process.env.BASE || 'http://localhost:5000';
|
const BASE = process.env.BASE || 'http://localhost:5000';
|
||||||
const UNIT = ['unit/test_list_filter.js', 'unit/test_render_cards.js',
|
const UNIT = ['unit/test_list_filter.js', 'unit/test_render_cards.js',
|
||||||
'unit/test_html_escaping.js', 'unit/test_style_editor_element_keys.js',
|
'unit/test_html_escaping.js', 'unit/test_style_editor_element_keys.js',
|
||||||
'unit/test_style_editor_layout_leaf_columns.js'];
|
'unit/test_style_editor_layout_leaf_columns.js',
|
||||||
|
'unit/test_style_editor_layout_leaf_collision.js'];
|
||||||
const DOM = ['dom/test_installed_dom.js', 'dom/test_store_dom.js', 'dom/test_no_double_fetch.js',
|
const DOM = ['dom/test_installed_dom.js', 'dom/test_store_dom.js', 'dom/test_no_double_fetch.js',
|
||||||
'dom/test_tools_sections.js'];
|
'dom/test_tools_sections.js'];
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,76 @@
|
|||||||
|
// Regression test for style-editor.js's columnsFor(), extracted verbatim
|
||||||
|
// from the shipped widget (see test_style_editor_layout_leaf_columns.js for
|
||||||
|
// the base case this builds on).
|
||||||
|
//
|
||||||
|
// A layout-only leaf key (e.g. a bare "scale" toggle directly under
|
||||||
|
// `layout`, no x/y object underneath) used to be keyed in the internal
|
||||||
|
// `seen` map by its own bare field name. If some *other* element's style
|
||||||
|
// block, or another element's layout axis block, happened to declare a
|
||||||
|
// sub-field with that exact same name (e.g. "scale" is also a real
|
||||||
|
// COLUMN_ORDER axis name most elements use), the `!seen.has(key)` guard
|
||||||
|
// skipped creating the leaf's column -- it silently reused the existing
|
||||||
|
// 'element'/'layout' column instead. That column's row binding in
|
||||||
|
// elementRow() then looked the field up under the wrong schema location for
|
||||||
|
// the leaf's own row, found nothing, and rendered a blank cell: the same
|
||||||
|
// "silently disappears" bug the leaf-column fix was meant to close, just
|
||||||
|
// reached through a name collision instead of a missing column altogether.
|
||||||
|
// This pins the fix: layout-leaf columns are keyed under a namespaced id so
|
||||||
|
// they can never be shadowed by an unrelated column sharing their name.
|
||||||
|
const fs = require('fs');
|
||||||
|
const path = require('path');
|
||||||
|
const V3 = path.resolve(__dirname, '../../../web_interface/static/v3');
|
||||||
|
|
||||||
|
const src = fs.readFileSync(path.join(V3, 'js/widgets/style-editor.js'), 'utf8');
|
||||||
|
const start = src.indexOf('function own(obj, key)');
|
||||||
|
const endMark = 'function control(opts)';
|
||||||
|
const end = src.indexOf(endMark);
|
||||||
|
if (start === -1 || end === -1) {
|
||||||
|
console.error('FAIL: could not locate own()/columnsFor() in style-editor.js -- slice markers need updating');
|
||||||
|
process.exit(1);
|
||||||
|
}
|
||||||
|
eval(src.slice(start, end)); // defines own/ownObj/elementKeys/columnsFor
|
||||||
|
|
||||||
|
let failures = 0;
|
||||||
|
function ok(desc, cond) {
|
||||||
|
if (cond) { console.log(`ok - ${desc}`); }
|
||||||
|
else { console.log(`FAIL - ${desc}`); failures++; }
|
||||||
|
}
|
||||||
|
|
||||||
|
// `possession` is a real style element whose layout axis block declares a
|
||||||
|
// "scale" sub-field, which seeds seen.set('scale', 'layout'). `timeout_flag`
|
||||||
|
// is layout-only and a bare leaf valued directly under layout, but its own
|
||||||
|
// key is *also* "scale" -- an unrelated collision with that same name.
|
||||||
|
const schema = {
|
||||||
|
properties: {
|
||||||
|
possession: {
|
||||||
|
type: 'object',
|
||||||
|
'x-style-managed': true,
|
||||||
|
properties: { font: { type: 'string' } },
|
||||||
|
},
|
||||||
|
scale: {
|
||||||
|
// The collision: a second element literally named "scale", entirely
|
||||||
|
// unrelated to the layout-only leaf below sharing that string.
|
||||||
|
type: 'boolean',
|
||||||
|
},
|
||||||
|
layout: {
|
||||||
|
type: 'object',
|
||||||
|
properties: {
|
||||||
|
possession: { type: 'object', properties: { scale: { type: 'number' } } },
|
||||||
|
scale: { type: 'boolean', default: true },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
const columns = columnsFor(schema);
|
||||||
|
const leafColumns = columns.filter(function (c) { return c.where === 'layout-leaf'; });
|
||||||
|
|
||||||
|
ok('the colliding layout axis sub-field still gets its own column',
|
||||||
|
columns.some(function (c) { return c.key === 'scale' && c.where === 'layout'; }));
|
||||||
|
ok('the layout-only leaf key still gets its own column despite the name collision',
|
||||||
|
leafColumns.length === 1 && leafColumns[0].key === 'scale');
|
||||||
|
ok('the leaf column is distinct from the colliding element/layout column',
|
||||||
|
columns.filter(function (c) { return c.key === 'scale'; }).length === 2);
|
||||||
|
|
||||||
|
if (failures) { console.log(`\n${failures} failure(s)`); process.exit(1); }
|
||||||
|
console.log('\nall checks passed');
|
||||||
@@ -403,12 +403,18 @@
|
|||||||
if (layoutEntry && typeof layoutEntry === 'object' && layoutEntry.properties) {
|
if (layoutEntry && typeof layoutEntry === 'object' && layoutEntry.properties) {
|
||||||
Object.keys(layoutEntry.properties).forEach(
|
Object.keys(layoutEntry.properties).forEach(
|
||||||
function (f) { seen.set(f, 'layout'); });
|
function (f) { seen.set(f, 'layout'); });
|
||||||
} else if (layoutEntry && !seen.has(key)) {
|
} else if (layoutEntry) {
|
||||||
// layout-only and a leaf: nothing else will share this
|
// layout-only and a leaf: nothing else will share this
|
||||||
// column, but leaving it out drops the field's only control
|
// column, but leaving it out drops the field's only control
|
||||||
// the moment the wholesale `layout` claim removes its
|
// the moment the wholesale `layout` claim removes its
|
||||||
// fallback (#569 review).
|
// fallback (#569 review). Keyed under a namespaced id, not
|
||||||
seen.set(key, 'layout-leaf');
|
// the bare field name -- an unrelated element's own style
|
||||||
|
// sub-field or another layout axis can share this exact
|
||||||
|
// name, and reusing that shared slot would give this row a
|
||||||
|
// column typed 'element'/'layout' that it doesn't declare,
|
||||||
|
// dropping the control right back out (#569 follow-up).
|
||||||
|
var leafId = 'layout-leaf:' + key;
|
||||||
|
if (!seen.has(leafId)) { seen.set(leafId, 'layout-leaf'); }
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
var known = COLUMN_ORDER.filter(function (f) { return seen.get(f); });
|
var known = COLUMN_ORDER.filter(function (f) { return seen.get(f); });
|
||||||
@@ -418,10 +424,12 @@
|
|||||||
return COLUMN_ORDER.indexOf(f) === -1;
|
return COLUMN_ORDER.indexOf(f) === -1;
|
||||||
}).sort();
|
}).sort();
|
||||||
return known.concat(extra).map(function (f) {
|
return known.concat(extra).map(function (f) {
|
||||||
|
var isLeafId = f.indexOf('layout-leaf:') === 0;
|
||||||
|
var fieldKey = isLeafId ? f.slice('layout-leaf:'.length) : f;
|
||||||
return {
|
return {
|
||||||
key: f,
|
key: fieldKey,
|
||||||
where: seen.get(f),
|
where: seen.get(f),
|
||||||
label: own(COLUMN_LABELS, f) || f.replace(/_/g, ' ')
|
label: own(COLUMN_LABELS, fieldKey) || fieldKey.replace(/_/g, ' ')
|
||||||
};
|
};
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user