From de48a270e9815466e9a4577e2ef19d677227b8dd Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Sat, 12 Sep 2026 08:33:45 +1000 Subject: [PATCH 1/6] DOCS: plugin import behavior cleanup --- docs/NOTIFICATIONS.md | 2 +- docs/PLUGINS_DEV.md | 2 +- docs/PLUGINS_DEV_DATA_CONTRACT.md | 66 +------------------------------ docs/PLUGINS_IMPORT_BEHAVIOR.md | 66 +++++++++++++++++++++++++++++++ mkdocs.yml | 1 + 5 files changed, 71 insertions(+), 66 deletions(-) create mode 100644 docs/PLUGINS_IMPORT_BEHAVIOR.md diff --git a/docs/NOTIFICATIONS.md b/docs/NOTIFICATIONS.md index 620fc328..fadad936 100755 --- a/docs/NOTIFICATIONS.md +++ b/docs/NOTIFICATIONS.md @@ -42,7 +42,7 @@ Click the **Read more in the docs.** Link at the top of each plugin to get more ### Plugin-level per-row overrides -A plugin author can also mark individual rows it reports as `quiet` via the `scanNotificationMode` data column, independent of any user-facing setting above - e.g. a bulk inventory import that shouldn't spam notifications for known-offline devices. This is a plugin-authoring concept, not something configured in the UI - see [Data contract](https://docs.netalertx.com/PLUGINS_DEV_DATA_CONTRACT#import-behavior-columns) for the full behavior (when it applies, and how it combines with the **Alert Events**/**Alert Down** device settings above when multiple plugins report the same device). +A plugin author can also mark individual rows it reports as `quiet` via the `scanNotificationMode` data column, independent of any user-facing setting above - e.g. a bulk inventory import that shouldn't spam notifications for known-offline devices. This is a plugin-authoring concept, not something configured in the UI - see [Plugin Import Behavior](https://docs.netalertx.com/PLUGINS_IMPORT_BEHAVIOR) for the full behavior (when it applies, and how it combines with the **Alert Events**/**Alert Down** device settings above when multiple plugins report the same device). ## Global settings ⚙ diff --git a/docs/PLUGINS_DEV.md b/docs/PLUGINS_DEV.md index 4c7f573f..373b50c9 100755 --- a/docs/PLUGINS_DEV.md +++ b/docs/PLUGINS_DEV.md @@ -311,7 +311,7 @@ To always map a static value (not read from plugin output): ### Import Behavior Columns (`scanCreatesDevice`, `scanNotificationMode`, `scanPresence`) -Three optional columns on `CurrentScan` control what happens once a row reaches it — see the [Data contract](PLUGINS_DEV_DATA_CONTRACT.md#import-behavior-columns) for the full contract (allowed values, defaults, downstream effects). All three default to today's behavior if never mapped, so existing plugins need no changes. +Three optional columns on `CurrentScan` control what happens once a row reaches it — see [Plugin Import Behavior](PLUGINS_IMPORT_BEHAVIOR.md) for the full contract (allowed values, defaults, downstream effects). All three default to today's behavior if never mapped, so existing plugins need no changes. Most plugins map a single static value for the whole import via `mapped_to_column_data` — e.g. an enrichment-only plugin that should never originate a new device: diff --git a/docs/PLUGINS_DEV_DATA_CONTRACT.md b/docs/PLUGINS_DEV_DATA_CONTRACT.md index 50420b45..be36efda 100644 --- a/docs/PLUGINS_DEV_DATA_CONTRACT.md +++ b/docs/PLUGINS_DEV_DATA_CONTRACT.md @@ -159,70 +159,7 @@ As the documentation might become outdated, it's good practice to check the late ### Import Behavior Columns -Three optional `CurrentScan` columns, all independent of each other, control what happens once a row reaches the table. - -| Column | Type | Default | Meaning | -|---|---|---|---| -| `scanCreatesDevice` | boolean | `1` | Whether this row can originate a *new* `Devices` entry. `0` lets an enrich-only plugin (e.g. a hostname resolver) update an already-existing device's fields without ever being able to create one. | -| `scanNotificationMode` | text (`normal` \| `quiet`) | `normal` | Whether this row's notifications are suppressed. `quiet` always suppresses the outbound email/push; whether the `Events` row itself still gets written depends on the event. **Live** (per-cycle aggregate, reclassifying a row changes future events): `New Device`, `Connected`, `Down Reconnected`, `IP Changed` — audit trail always written. `New Device` isn't gated on `scanPresence = 1` like the other three (see flowcharts below). **Frozen** (`devAlertDown`/`devAlertEvents` seeded at device creation, reclassifying later has no retroactive effect): `Device Down`, `Disconnected` — not symmetric. `Disconnected` always writes its `Events` row (`evePendingAlertEmail = 0` when quiet). `Device Down` writes **no row at all** when `devAlertDown = 0`. | -| `scanPresence` | boolean | `1` | Whether this row asserts the device is *currently online*. `0` means "identity/inventory data, no presence claim" — not "offline". A reservation, a lease record, or a static IPAM entry are typical `0` cases. | - -**Missing vs. invalid values — these behave differently, not interchangeably:** - -| Column | Column never mapped (missing) | Mapped but sent an unexpected value (invalid) | -|---|---|---| -| `scanCreatesDevice` | `1` (schema `DEFAULT`) | `CHECK (scanCreatesDevice IN (0, 1))` — anything else fails the `INSERT` outright, it does not silently fall back to `1` | -| `scanNotificationMode` | `normal` (schema `DEFAULT`) | No `CHECK` constraint — any string other than the literal `'quiet'` is treated as `normal`, since the SQL only special-cases that exact value | -| `scanPresence` | `1` (schema `DEFAULT`) | `CHECK (scanPresence IN (0, 1))` — same as `scanCreatesDevice`, invalid values fail the `INSERT`, they don't default | - -**Multiple plugins reporting the same MAC in the same scan cycle** (the normal case, not an edge case — see the `scan-pipeline` skill) resolve per column, not uniformly: `scanCreatesDevice` and `scanPresence` are most-permissive-wins (any row saying `1` wins), while `scanNotificationMode` is most-*restrictive*-wins (any row saying `quiet` suppresses the notification, even if a sibling row says `normal`) — erring toward under-notifying rather than spamming. - -**Combination matrix** — not every combination is meaningful for every plugin; pick the one that matches what your plugin actually knows: - -| `scanCreatesDevice` | `scanPresence` | Meaning | -|---|---|---| -| 1 | 1 | Normal discovery (the default) | -| 1 | 0 | Inventory/identity import — create the device, but don't claim it's online right now | -| 0 | 1 | Presence-confirming enrichment — never originate a device, but assert presence for one that exists | -| 0 | 0 | Silent enrichment — never originate a device, no presence claim either | - -`scanNotificationMode` is orthogonal to both of the above and can be combined with any row in the table (e.g. inventory import + quiet, for a fully silent bulk import of known-offline devices). - -**Decision: does this row create a device?** - -```mermaid -flowchart TD - A[Row reaches CurrentScan] --> B{scanMac blank or
null-equivalent?} - B -- yes --> Z[Never creates a device] - B -- no --> C{Any row this cycle for this
MAC has scanCreatesDevice = 1?} - C -- no, all say 0 --> Y[No device created
enrich-only] - C -- yes, at least one --> D{Devices row already
exists for this MAC?} - D -- yes --> E[No-op - existing device untouched
by this check] - D -- no --> F[New Devices row created
+ New Device event] -``` - -**Decision: is this event's notification suppressed?** - -```mermaid -flowchart TD - A[Event about to fire] --> B{Fired from a row that exists in
CurrentScan this cycle? New Device /
Connected / Down Reconnected / IP Changed} - B -- yes --> C{Live aggregate: any CurrentScan row
for this MAC says
scanNotificationMode = quiet?} - C -- yes --> S[Suppressed
evePendingAlertEmail = 0] - C -- no --> N[Notified
evePendingAlertEmail = 1] - B -- no, fired from row ABSENCE
Device Down / Disconnected --> D{Frozen device setting:
devAlertDown / devAlertEvents,
seeded at creation time} - D -- off --> S - D -- on --> N -``` - -**Worked scenarios:** - -| Scenario | `scanCreatesDevice` | `scanPresence` | `scanNotificationMode` | `scanMac` | Outcome | -|---|---|---|---|---|---| -| Normal discovery (default plugin behavior) | `1` (default) | `1` (default) | `normal` (default) | real MAC | Device created if new, notified normally, presence tracked live. | -| Enrich-only plugin (e.g. a hostname resolver) | `0` | `1` (default) | `normal` (default) | real MAC | Never originates a device; still updates an existing device's fields via `FIELD_SPECS`. If another plugin reports the same MAC with `scanCreatesDevice = 1`, the device still gets created (most-permissive-wins) — this plugin's `0` doesn't block it. | -| Bulk inventory import of known-offline devices | `1` | `0` | `quiet` | real MAC | Creates devices without claiming they're online, and without a wave of "New Device" notifications for a large batch import. | -| Presence-confirming enrichment (e.g. a DHCP lease scanner) | `0` | `1` | `normal` | real MAC | Confirms an *existing* device is online without ever being the plugin that creates it. | -| Row with no usable device identity (e.g. an object with no routable MAC available) | `0` | irrelevant | irrelevant | blank / null-equivalent | Never creates a device — but not for symmetric reasons. The blank-MAC guard blocks the whole aggregated group by its shared `scanMac` value, regardless of any individual row's `scanCreatesDevice` (even a stray `1` from an unrelated plugin sharing the same blank `scanMac` can't override it). Setting `scanCreatesDevice = 0` here is still correct practice, but on its own is only this row's vote — most-permissive-wins means a sibling row for the same `scanMac` asserting `1` would still win. The blank-MAC guard is what actually guarantees safety regardless of what other contributors do. | +Three optional `CurrentScan` columns — `scanCreatesDevice`, `scanNotificationMode`, `scanPresence` — control whether a row can create a device, whether it counts as a live presence signal, and whether its notifications are suppressed. Only relevant if your plugin maps to `mapped_to_table: "CurrentScan"`; all three default to today's behavior if never mapped. See **[Plugin Import Behavior](PLUGINS_IMPORT_BEHAVIOR.md)** for the full contract — value tables, precedence rules, decision flowcharts, and worked scenarios. ## Examples @@ -350,6 +287,7 @@ tail -f /tmp/log/app.log | grep -i "YOURPREFIX\|Plugins_Objects" ## See Also +- [Plugin Import Behavior](PLUGINS_IMPORT_BEHAVIOR.md) - `scanCreatesDevice`/`scanNotificationMode`/`scanPresence`, for plugins mapping to `CurrentScan` - [Plugin Settings System](PLUGINS_DEV_SETTINGS.md) - How to accept user input - [Data Sources](PLUGINS_DEV_DATASOURCES.md) - Different data source types - [Debugging Plugins](DEBUG_PLUGINS.md) - Troubleshooting plugin issues diff --git a/docs/PLUGINS_IMPORT_BEHAVIOR.md b/docs/PLUGINS_IMPORT_BEHAVIOR.md new file mode 100644 index 00000000..d186e3b8 --- /dev/null +++ b/docs/PLUGINS_IMPORT_BEHAVIOR.md @@ -0,0 +1,66 @@ +# Plugin Import Behavior + +Three optional `CurrentScan` columns, all independent of each other, control what happens once a row your plugin reports reaches the `CurrentScan` table: whether it can create a device, whether it counts as a live presence signal, and whether its notifications are suppressed. This only matters if your plugin maps to `mapped_to_table: "CurrentScan"` — see the [Data contract](PLUGINS_DEV_DATA_CONTRACT.md) for the base column spec these three sit alongside. + +| Column | Type | Default | Meaning | +|---|---|---|---| +| `scanCreatesDevice` | boolean | `1` | Whether this row can originate a *new* `Devices` entry. `0` lets an enrich-only plugin (e.g. a hostname resolver) update an already-existing device's fields without ever being able to create one. | +| `scanNotificationMode` | text (`normal` \| `quiet`) | `normal` | Whether this row's notifications are suppressed. `quiet` always suppresses the outbound email/push; whether the `Events` row itself still gets written depends on the event. **Live** (per-cycle aggregate, reclassifying a row changes future events): `New Device`, `Connected`, `Down Reconnected`, `IP Changed` — audit trail always written. `New Device` isn't gated on `scanPresence = 1` like the other three (see flowcharts below). **Frozen** (`devAlertDown`/`devAlertEvents` seeded at device creation, reclassifying later has no retroactive effect): `Device Down`, `Disconnected` — not symmetric. `Disconnected` always writes its `Events` row (`evePendingAlertEmail = 0` when quiet). `Device Down` writes **no row at all** when `devAlertDown = 0`. | +| `scanPresence` | boolean | `1` | Whether this row asserts the device is *currently online*. `0` means "identity/inventory data, no presence claim" — not "offline". A reservation, a lease record, or a static IPAM entry are typical `0` cases. | + +**Missing vs. invalid values — these behave differently, not interchangeably:** + +| Column | Column never mapped (missing) | Mapped but sent an unexpected value (invalid) | +|---|---|---| +| `scanCreatesDevice` | `1` (schema `DEFAULT`) | `CHECK (scanCreatesDevice IN (0, 1))` — anything else fails the `INSERT` outright, it does not silently fall back to `1` | +| `scanNotificationMode` | `normal` (schema `DEFAULT`) | No `CHECK` constraint — any string other than the literal `'quiet'` is treated as `normal`, since the SQL only special-cases that exact value | +| `scanPresence` | `1` (schema `DEFAULT`) | `CHECK (scanPresence IN (0, 1))` — same as `scanCreatesDevice`, invalid values fail the `INSERT`, they don't default | + +**Multiple plugins reporting the same MAC in the same scan cycle** (the normal case, not an edge case — see the `scan-pipeline` skill) resolve per column, not uniformly: `scanCreatesDevice` and `scanPresence` are most-permissive-wins (any row saying `1` wins), while `scanNotificationMode` is most-*restrictive*-wins (any row saying `quiet` suppresses the notification, even if a sibling row says `normal`) — erring toward under-notifying rather than spamming. + +**Combination matrix** — not every combination is meaningful for every plugin; pick the one that matches what your plugin actually knows: + +| `scanCreatesDevice` | `scanPresence` | Meaning | +|---|---|---| +| 1 | 1 | Normal discovery (the default) | +| 1 | 0 | Inventory/identity import — create the device, but don't claim it's online right now | +| 0 | 1 | Presence-confirming enrichment — never originate a device, but assert presence for one that exists | +| 0 | 0 | Silent enrichment — never originate a device, no presence claim either | + +`scanNotificationMode` is orthogonal to both of the above and can be combined with any row in the table (e.g. inventory import + quiet, for a fully silent bulk import of known-offline devices). + +**Decision: does this row create a device?** + +```mermaid +flowchart TD + A[Row reaches CurrentScan] --> B{scanMac blank or
null-equivalent?} + B -- yes --> Z[Never creates a device] + B -- no --> C{Any row this cycle for this
MAC has scanCreatesDevice = 1?} + C -- no, all say 0 --> Y[No device created
enrich-only] + C -- yes, at least one --> D{Devices row already
exists for this MAC?} + D -- yes --> E[No-op - existing device untouched
by this check] + D -- no --> F[New Devices row created
+ New Device event] +``` + +**Decision: is this event's notification suppressed?** + +```mermaid +flowchart TD + A[Event about to fire] --> B{Fired from a row that exists in
CurrentScan this cycle? New Device /
Connected / Down Reconnected / IP Changed} + B -- yes --> C{Live aggregate: any CurrentScan row
for this MAC says
scanNotificationMode = quiet?} + C -- yes --> S[Suppressed
evePendingAlertEmail = 0] + C -- no --> N[Notified
evePendingAlertEmail = 1] + B -- no, fired from row ABSENCE
Device Down / Disconnected --> D{Frozen device setting:
devAlertDown / devAlertEvents,
seeded at creation time} + D -- off --> S + D -- on --> N +``` + +**Worked scenarios:** + +| Scenario | `scanCreatesDevice` | `scanPresence` | `scanNotificationMode` | `scanMac` | Outcome | +|---|---|---|---|---|---| +| Normal discovery (default plugin behavior) | `1` (default) | `1` (default) | `normal` (default) | real MAC | Device created if new, notified normally, presence tracked live. | +| Enrich-only plugin (e.g. a hostname resolver) | `0` | `1` (default) | `normal` (default) | real MAC | Never originates a device; still updates an existing device's fields via `FIELD_SPECS`. If another plugin reports the same MAC with `scanCreatesDevice = 1`, the device still gets created (most-permissive-wins) — this plugin's `0` doesn't block it. | +| Bulk inventory import of known-offline devices | `1` | `0` | `quiet` | real MAC | Creates devices without claiming they're online, and without a wave of "New Device" notifications for a large batch import. | +| Presence-confirming enrichment (e.g. a DHCP lease scanner) | `0` | `1` | `normal` | real MAC | Confirms an *existing* device is online without ever being the plugin that creates it. | +| Row with no usable device identity (e.g. an object with no routable MAC available) | `0` | irrelevant | irrelevant | blank / null-equivalent | Never creates a device — but not for symmetric reasons. The blank-MAC guard blocks the whole aggregated group by its shared `scanMac` value, regardless of any individual row's `scanCreatesDevice` (even a stray `1` from an unrelated plugin sharing the same blank `scanMac` can't override it). Setting `scanCreatesDevice = 0` here is still correct practice, but on its own is only this row's vote — most-permissive-wins means a sibling row for the same `scanMac` asserting `1` would still win. The blank-MAC guard is what actually guarantees safety regardless of what other contributors do. | diff --git a/mkdocs.yml b/mkdocs.yml index e5da6cdc..3c1f9832 100755 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -110,6 +110,7 @@ nav: - Overview: PLUGINS_DEV.md - Quick start: PLUGINS_DEV_QUICK_START.md - Data contract: PLUGINS_DEV_DATA_CONTRACT.md + - Import behavior: PLUGINS_IMPORT_BEHAVIOR.md - Settings system: PLUGINS_DEV_SETTINGS.md - Data sources: PLUGINS_DEV_DATASOURCES.md - UI components: PLUGINS_DEV_UI_COMPONENTS.md From cff3ddfc38e5e1ec1834132a3fe563fdb63f9ebe Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Sat, 12 Sep 2026 08:53:16 +1000 Subject: [PATCH 2/6] DOCS: plugin import behavior cleanup --- .claude/skills/plugin-development/SKILL.md | 2 +- .claude/skills/scan-pipeline/SKILL.md | 3 ++- .gemini/skills/plugin-development/plugin-skill.md | 2 +- .gemini/skills/scan-pipeline/SKILL.md | 3 ++- .github/skills/plugin-run-development/SKILL.md | 2 +- .github/skills/scan-pipeline/SKILL.md | 3 ++- 6 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.claude/skills/plugin-development/SKILL.md b/.claude/skills/plugin-development/SKILL.md index fad051fa..772a2717 100644 --- a/.claude/skills/plugin-development/SKILL.md +++ b/.claude/skills/plugin-development/SKILL.md @@ -33,7 +33,7 @@ server/plugins// - `_CMD`: script path. - `_RUN_TIMEOUT`: timeout in seconds — **enforced by the core plugin runner as the whole script's kill-timeout** (`server/plugin.py` passes it straight to `subprocess(..., timeout=...)`). Not a safe per-HTTP-call timeout — don't reuse it for individual network calls in a loop, or one slow call can burn the whole budget and get the process killed before it writes its result file. Two correct alternatives: `config.json`'s `"timeoutMultiplier": true` on a `params[]` entry for a config-declared, known-length loop (see `arp_scan`); `plugin_helper.per_item_timeout()` for a runtime-variable-length loop (see the `_publisher_*` plugins). - `_WATCH`: columns to watch for changes. -- `_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_DEV_DATA_CONTRACT.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns. +- `_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_IMPORT_BEHAVIOR.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns. ## Data Contract diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index 9781183a..de2ee977 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -47,12 +47,13 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Four real gotchas (not hypothetical — all surfaced live during a design review) +## Five real gotchas (not hypothetical — all surfaced live during a design review) 1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. 4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. +5. **A blank/null-equivalent `scanMac` could create a phantom `Devices` row until this was checked directly.** `create_new_devices()`'s two creation-path queries used whatever `scanMac` a `scanCreatesDevice = 1` row supplied, with no check that it was non-empty — and `scanCreatesDevice` defaults to `1`, so *any* plugin reporting a row with no real MAC available, without explicitly setting `scanCreatesDevice = 0` itself, would have created a `devMac = ''` device. Once that phantom row existed, every other blank-MAC row from every other plugin across every cycle would silently write presence/timestamp/field updates onto it — a real bug, not a hypothetical, surfaced by a plugin author's own design question rather than by inspection. Both creation queries now filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` as a backstop (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) — this doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows, it's what keeps a MAC-less row inert even when some *other* plugin forgets to. If you add a third creation-adjacent query here, check it against blank `scanMac` too, the same way the existing two now are. **Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. **`Events`, `Sessions`, and `Notifications`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. diff --git a/.gemini/skills/plugin-development/plugin-skill.md b/.gemini/skills/plugin-development/plugin-skill.md index 9f5a8061..3293f6ae 100644 --- a/.gemini/skills/plugin-development/plugin-skill.md +++ b/.gemini/skills/plugin-development/plugin-skill.md @@ -43,7 +43,7 @@ server/plugins// - `_CMD`: script path - `_RUN_TIMEOUT`: timeout in seconds — **this is enforced by the core plugin runner as the whole script's kill-timeout** (`server/plugin.py` passes it straight to `subprocess(..., timeout=...)`). It is not a safe per-HTTP-call timeout — don't reuse it for individual network calls in a loop, or one slow call can burn the whole budget and get the process killed before it writes its result file. - `_WATCH`: columns to watch for changes -- `_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_DEV_DATA_CONTRACT.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns. +- `_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_IMPORT_BEHAVIOR.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns. ## Data Contract diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index a2bab0f9..066a16b5 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -47,12 +47,13 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Four real gotchas (not hypothetical — all surfaced live during a design review) +## Five real gotchas (not hypothetical — all surfaced live during a design review) 1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. 4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. +5. **A blank/null-equivalent `scanMac` could create a phantom `Devices` row until this was checked directly.** `create_new_devices()`'s two creation-path queries used whatever `scanMac` a `scanCreatesDevice = 1` row supplied, with no check that it was non-empty — and `scanCreatesDevice` defaults to `1`, so *any* plugin reporting a row with no real MAC available, without explicitly setting `scanCreatesDevice = 0` itself, would have created a `devMac = ''` device. Once that phantom row existed, every other blank-MAC row from every other plugin across every cycle would silently write presence/timestamp/field updates onto it — a real bug, not a hypothetical, surfaced by a plugin author's own design question rather than by inspection. Both creation queries now filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` as a backstop (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) — this doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows, it's what keeps a MAC-less row inert even when some *other* plugin forgets to. If you add a third creation-adjacent query here, check it against blank `scanMac` too, the same way the existing two now are. **Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. **`Events`, `Sessions`, and `Notifications`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. diff --git a/.github/skills/plugin-run-development/SKILL.md b/.github/skills/plugin-run-development/SKILL.md index 95933b40..79100e74 100644 --- a/.github/skills/plugin-run-development/SKILL.md +++ b/.github/skills/plugin-run-development/SKILL.md @@ -44,7 +44,7 @@ server/plugins// - `_CMD`: script path - `_RUN_TIMEOUT`: timeout in seconds — **this is enforced by the core plugin runner as the whole script's kill-timeout** (`server/plugin.py` passes it straight to `subprocess(..., timeout=...)`). It is not a safe per-HTTP-call timeout — don't reuse it for individual network calls in a loop, or one slow call can burn the whole budget and get the process killed before it writes its result file. - `_WATCH`: columns to watch for changes -- `_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_DEV_DATA_CONTRACT.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns. +- `_IMPORT_ON`: optional — gates whether this run's rows get promoted into `CurrentScan` (only relevant if `mapped_to_table: "CurrentScan"`). See `docs/PLUGINS_IMPORT_BEHAVIOR.md` for the related per-row `scanCreatesDevice`/`scanNotificationMode`/`scanPresence` columns. ## Data Contract diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index e8ba8d75..7b7e733c 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -47,12 +47,13 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Four real gotchas (not hypothetical — all surfaced live during a design review) +## Five real gotchas (not hypothetical — all surfaced live during a design review) 1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. 4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. +5. **A blank/null-equivalent `scanMac` could create a phantom `Devices` row until this was checked directly.** `create_new_devices()`'s two creation-path queries used whatever `scanMac` a `scanCreatesDevice = 1` row supplied, with no check that it was non-empty — and `scanCreatesDevice` defaults to `1`, so *any* plugin reporting a row with no real MAC available, without explicitly setting `scanCreatesDevice = 0` itself, would have created a `devMac = ''` device. Once that phantom row existed, every other blank-MAC row from every other plugin across every cycle would silently write presence/timestamp/field updates onto it — a real bug, not a hypothetical, surfaced by a plugin author's own design question rather than by inspection. Both creation queries now filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` as a backstop (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) — this doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows, it's what keeps a MAC-less row inert even when some *other* plugin forgets to. If you add a third creation-adjacent query here, check it against blank `scanMac` too, the same way the existing two now are. **Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. **`Events`, `Sessions`, and `Notifications`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. From f52de6cbdca4e482354111d6abcb3f5ae99f09ea Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Mon, 14 Sep 2026 08:29:02 +1000 Subject: [PATCH 3/6] FE+BE: performance improvements --- .claude/skills/ux-design-patterns/SKILL.md | 35 ++++++++++ .gemini/internal-docs/PRDs/completed/.gitkeep | 0 .gemini/internal-docs/PRDs/to_review/.gitkeep | 0 .gemini/internal-docs/research/.gitkeep | 0 .gemini/internal-docs/research_old/.gitkeep | 0 .gemini/skills/skills-index/SKILL.md | 1 + .gemini/skills/ux-design-patterns/SKILL.md | 35 ++++++++++ .github/skills/code-standards/SKILL.md | 29 ++++++++ .github/skills/ux-design-patterns/SKILL.md | 35 ++++++++++ .gitignore | 8 +++ CLAUDE.md | 2 + front/js/network-tree.js | 46 +++++++++---- front/php/templates/language/en_us.json | 2 + front/presence.php | 52 +++++++++++++- scripts/check_skill_pairs.py | 1 + server/api_server/api_server_start.py | 16 ++++- server/api_server/graphql_endpoint.py | 7 +- server/api_server/openapi/schemas.py | 2 + server/helper.py | 15 ++-- server/models/device_instance.py | 15 ++-- test/api_endpoints/test_devices_endpoints.py | 47 +++++++++++++ test/api_endpoints/test_graphq_endpoints.py | 43 +++++++++++- test/server/test_helper.py | 68 +++++++++++++++++++ 23 files changed, 428 insertions(+), 31 deletions(-) create mode 100644 .claude/skills/ux-design-patterns/SKILL.md create mode 100644 .gemini/internal-docs/PRDs/completed/.gitkeep create mode 100644 .gemini/internal-docs/PRDs/to_review/.gitkeep create mode 100644 .gemini/internal-docs/research/.gitkeep create mode 100644 .gemini/internal-docs/research_old/.gitkeep create mode 100644 .gemini/skills/ux-design-patterns/SKILL.md create mode 100644 .github/skills/ux-design-patterns/SKILL.md create mode 100644 test/server/test_helper.py diff --git a/.claude/skills/ux-design-patterns/SKILL.md b/.claude/skills/ux-design-patterns/SKILL.md new file mode 100644 index 00000000..355691b6 --- /dev/null +++ b/.claude/skills/ux-design-patterns/SKILL.md @@ -0,0 +1,35 @@ +--- +name: ux-design-patterns +description: Read before adding or changing any front/ UI element - a control, layout, button, or interaction pattern. Covers the don't-invent-new-UX-without-a-PRD rule and the priority order for design tradeoffs (existing behavior > intuitiveness > information density > usability > utility > uniqueness > industry practices > generic UI). +--- + +# UX / Frontend Design Patterns + +## Core principle: reuse before inventing + +Don't introduce new UX behavior or visual patterns unless a PRD explicitly calls for it. Before building any new UI element, search the existing frontend for a pattern that already solves this exact need, and reuse its markup/CSS/behavior instead of inventing a new one. + +Real, recent example: a presence-page Prev/Next pager was first built with custom `