BE+FE: Plugins -> Sources DevDetails tab

.claude/skills/plugin-development/SKILL.md

@@ -61,6 +61,12 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext
 | `on_new_device` | When new device detected |
 | `on_notification` | When notification triggered |
 
+## `all_plugins` Lifecycle - Cache Hot-Path Derivations by Identity
+
+`all_plugins` (every plugin's parsed `config.json`) is loaded once, in `initialise.py`'s `get_plugins_configs()` call, and reused as the same list object everywhere after that until an actual config reload replaces it. `update_api()` (`server/api.py`), which receives `all_plugins` as a parameter, runs far more often than that - on every scan cycle and every plugin completion (`server/__main__.py`, `server/plugin.py`), not just on reload.
+
+Any function deriving something from `all_plugins` that gets called from a hot path like `update_api()` should cache its result, not recompute on every call - the input is almost always the same object even though the call site fires constantly. `server/db/plugin_field_views.py`'s `get_all_device_field_views()` is the pattern: retain a reference to the last-seen `all_plugins` list and compare it to the current one with `is`, recomputing only when a genuinely new object (an actual reload) appears. Don't key the cache by `id(all_plugins)` alone - once the old list is garbage collected, Python can reuse that id for an unrelated object, which would wrongly serve a stale cached result for what is actually a new reload.
+
 ## Plugin Formats
 
 | Format | Purpose | Phase |

.claude/skills/prd-writing/SKILL.md

@@ -53,11 +53,22 @@ Both are plausible, well-written, and wrong. Reading the code first catches both
 - **Docs/skills to update** — anywhere this needs to be reflected outside the code itself (external docs, paired skill files, template files new authors copy from).
 - **Tests** — organized by mechanism, each case naming the real function/query it exercises and the concrete assertion (step 7), plus a manual verification checklist for anything that can't be unit-tested (including an `EXPLAIN QUERY PLAN` check at realistic scale if the Performance impact section found a genuine risk).
 - **(Optional) Execution plan** — phased, referencing the same file/function names used above rather than restating the design in vaguer terms.
+- **Implementation checklist** — added to the PRD file itself when implementation actually starts (not at draft time), three boxes matching the "Implementing an approved PRD" section below: plan written before any code; test written and confirmed red against the pre-fix code before the fix was applied; no PRD filename/path/word left in committed code. Check each box only as it's actually done, not preemptively - the point is a forcing function visible in the file you're editing, not a formality.
 
 ## Before starting: check for an existing architecture-reference skill
 
 If a skill already documents the subsystem the feature touches, load it before researching from scratch — don't re-derive call graphs or mechanism details that are already written down. If the feature touches a subsystem with no such skill, and understanding it required significant re-derivation from raw code, that's a signal to write one afterward so the next PRD in that area doesn't start from zero.
 
+## Implementing an approved PRD
+
+Add the Implementation checklist (see Structure above) to the PRD file as the first edit, before touching any other code - an unchecked, visible checklist in the file you're actively working in is the forcing function; a rule read once at skill-load time is not.
+
+Before writing a single line of implementation code, write an explicit plan (via `EnterPlanMode` or an equivalent written plan) covering which files change, in what order, and which test(s) get written first. Understanding the PRD's design is not the same as planning its implementation - in practice these have been conflated, going straight from "I've read and understood this PRD" into code edits without ever writing either the plan or the test required below.
+
+Then follow step 7 above literally, not just in spirit, at implementation time too: write the test, run it against the pre-fix code, confirm it fails for the right reason (a real behavioral mismatch, or a legitimate missing-interface error for a brand-new contract - not a setup bug), and only then write the fix. Writing the test after the implementation and asserting it "would have failed" is not equivalent - it's an unverified claim. If you catch yourself having implemented first, the correct recovery is to actually revert the implementation, confirm the test fails red against the reverted code, and only then reapply the fix - not to just add tests afterward and call it done.
+
+Never reference a PRD's filename, path, or the word "PRD" in committed code, comments, docstrings, or commit messages - `PRDs/` is gitignored and local-only, so a reader of the committed code later has no way to find what it's pointing at, and the reference rots the moment the PRD file is deleted or moved. Explain the *why* self-containedly in the code itself (a design PRD's rationale restated in the comment, not a pointer to go read the PRD) instead.
+
 ## Where to save
 
 `.gemini/internal-docs/PRDs/<kebab-case-name>.md`, unless the user specifies otherwise. Mark the status line (`**Status:** Draft — pending review`) so it's clear this hasn't been approved yet, and keep the author line accurate about who made the calls (a design discussion with an assistant is not sole assistant authorship).

.claude/skills/scan-pipeline/SKILL.md

@@ -59,6 +59,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat
 8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one.
 
 7. **`CurrentScan.scanMac`/`Events.eveMac` are permanently MAC-keyed, independent of the devGUID-as-PK migration.** See `database-patterns`' "Device Identity" section - `devMac` is today's actual schema PK, `devGUID` is the intended long-term identity, but a plugin can only ever report a MAC from network discovery, never an app-internal `devGUID`. Don't design around these tables ever becoming devGUID-keyed.
+9. **`insert_events()`'s `IP Changed` query evaluates every distinct present address for a MAC individually, not one `MIN()`-reduced candidate.** A device can have more than one simultaneously-valid, already-known address in the same family (e.g. two live IPv6 addresses); testing only the `MIN()`'d one against `devPrimaryIPv4`/`devPrimaryIPv6`/`devLastIP` can pick the one address that happens not to be sitting in the single-valued slot, firing a false event even though nothing actually changed. The fix makes "known" additive: an address counts as known if it matches the three-slot check *or* if it's been reported (and isn't `missing-in-last-scan`) by an eligible plugin in `Plugins_Objects`, via `_known_plugin_ip_addresses_sql()`'s use of `plugin_field_views.get_plugin_columns_for_field(all_plugins, "ip")` - the same plugin-to-column mapping the Field View tab uses. `all_plugins` is optional on both `insert_events()` and `process_scan()` (default `None`); when it's not eligible for any plugin (or a caller hasn't passed it), the additive half is skipped entirely and the query reduces to exactly the three-slot check. Addresses that fail `known()` are aggregated into one `Events` row per MAC per cycle (today's shape), listing every new address in `eveAdditionalInfo` rather than firing one row per address.
 
 ## When to read this vs. other docs/skills
 

.claude/skills/skill-hygiene/SKILL.md

@@ -57,4 +57,4 @@ This does not apply to PRDs (`.gemini/internal-docs/PRDs/`) — those keep their
 
 ## Keep the three trees in sync
 
-Most skills exist as three near-identical copies (`.claude/skills/<name>/SKILL.md`, `.gemini/skills/<name>/SKILL.md`, `.github/skills/<name>/SKILL.md` — see `.gemini/skills/skills-index/SKILL.md` for the pairing map). When a hygiene fix changes a skill's body, apply the same fix to all paired copies so they stay identical (frontmatter `name`/`description` may differ per tree's own convention; the body should not). `scripts/check_skill_pairs.py`'s `GROUPS` list only flags when some-but-not-all paired files changed in a diff — it doesn't check the bodies actually match, so a manual diff after editing is still worth it.
+Most skills exist as three near-identical copies (`.claude/skills/<name>/SKILL.md`, `.gemini/skills/<name>/SKILL.md`, `.github/skills/<name>/SKILL.md` - see `.gemini/skills/skills-index/SKILL.md` for the pairing map). When a hygiene fix changes a skill's body, apply the same fix to all paired copies so they stay identical (frontmatter `name`/`description` may differ per tree's own convention; the body should not). `scripts/check_skill_pairs.py` checks both: it flags a diff that touched some but not all of a group's paired files, and - for the groups the skills index documents as covering identical content - it also diffs the bodies directly and flags when they've actually drifted apart. The CI step calling it is non-blocking, so a manual diff after editing is still worth doing yourself.

.claude/skills/ux-design-patterns/SKILL.md

@@ -11,6 +11,30 @@ Don't introduce new UX behavior or visual patterns unless a PRD explicitly calls
 
 Real, recent example: a presence-page Prev/Next pager was first built with custom `<button class="btn btn-xs">` elements floated in a `.box-header`. The DataTables pagination pattern (`dataTables_wrapper` / `dataTables_paginate` / `ul.pagination` / `li.paginate_button.previous|next`) already existed elsewhere in the app and does the exact same job. The custom version looked visually broken in the actual UI and had to be reimplemented using the existing pattern once that was caught in manual testing - reusing it also picked up dark-mode theming (`front/css/dark-patch.css`'s `.pagination li > a` / `.disabled` rules) for free, which the hand-rolled version didn't have. Grep first: e.g. `grep -rn "pagination\|paginate_button" front/` before adding a "previous/next" control of your own; the same applies to modals, filter inputs, badges, tooltips, tables - anything that already has an established shape somewhere in `front/`.
 
+A second real example: a new device-details tab's status display was hand-written as `${icon} ${label}` plain text, and its field-group markup was approximated from memory (`<div class="form-group">` / plain `<div class="col-sm-8">`) instead of read from the actual existing tab it was meant to match. Both were wrong in ways that only showed up once rendered: the real status chip everywhere else in the app (`devices-table.js`, `network-tabs.js`, `network-api.js`) is `<span class="badge ${cssClass}">${iconHtml} ${label}</span>` via the shared `getStatusBadgeParts()`, not bare text; the real field-group markup needs `col-xs-12` on both the `.form-group` and its value `<div>` (plus `.input-group` on the value div), which a plain `grep` for the class names would have shown immediately. Approximating a reference pattern "from memory" instead of reading its actual current markup is the same mistake as not searching at all - read the real template/function that produces the reference UI, don't reconstruct it from a general impression of what it probably looks like.
+
+A third real example, and the one to remember when the new thing isn't markup but behavior: Field View's per-field tables were first built as raw `<table><tbody>` HTML-string concatenation, when `pluginsCore.php`'s own `buildDT()` - a few hundred lines away, same feature area - already builds near-identical plugin-data tables: same DataTables paging/searching/ordering/lengthMenu options, same `createdCell` + `getFormControl()` cell-rendering approach. The fix was extracting the shared options into `getStandardDataTableOptions()` (`front/js/plugin_column_render.js`) and reusing the same cell-rendering pattern, rather than two parallel table implementations answering the same "render a plugin-sourced table" need. Duplication risk isn't only in markup/CSS - a JS initialization config or rendering callback can be just as duplicated. Before writing a new `.DataTable({...})` call (or any other substantial JS behavior-config object), grep for existing instances of it in the same subsystem first: `grep -rn '\.DataTable(' front/`.
+
+## Loading skeletons and spinnerTarget
+
+Two non-obvious mechanisms govern how a per-tab loading skeleton behaves, and skipping either produces a skeleton that visually escapes its own tab or gets stuck showing forever.
+
+**Positioning:** `resolveSpinnerTarget()` (`front/js/common.js`) looks for `.spinnerTarget` among the active tab-pane's *direct children only* (`activePane.children(".spinnerTarget:visible")`), falling back to the pane itself if none is found. A skeleton needs its own `#panX` id added to `app.css`'s positioning-anchor rule (`#panDetails, #panSessions, #panPresence, #panEvents, #appEvents, #panSessionInfo { position: relative; }`) and must be a direct child of that pane - no wrapper `<div>` in between. Introducing an extra wrapper div (even just to group the skeleton with its content) breaks the direct-child lookup and the pane's own `position: relative`, and the skeleton escapes to overlay the whole page instead of staying inside its tab. Match the existing pattern exactly: `front/deviceDetailsPresence.php`'s `<?php require '.../skel_device_details_tab_presence.php'; ?>` as the very first line, no wrapper, `#panPresence` already in the anchor list.
+
+**Lifecycle:** a per-tab polling updater (`setTimeout(updater, 200)`) that shows a skeleton while data loads and hides it once rendered needs to track *what it's waiting for*, not just whether it has ever rendered once. Track the mac (or whatever identity the content depends on) and re-render when it changes - `currentMac !== lastMac`, not just a one-time boolean flag - or switching to a different device while the tab stays active leaves stale data on screen with no re-render. Use `.finally(hideSkeleton)` on the render promise, not `.then(hideSkeleton)` - if the render throws or its fetch rejects, `.then()` never runs and the skeleton is stuck visible forever; `.finally()` always runs regardless of outcome. The render call itself must also sit inside a `try`/`catch` (or be deferred into an async callback, as below) - the updater's own `setTimeout(updater, 200)` call is what keeps the polling loop alive, and if it sits *after* an inline render call that throws synchronously, the throw skips it and the loop dies permanently, leaving the skeleton stuck with no further retries ever.
+
+**Show/hide symmetry:** `showSpinner()`/`hideSpinner()` (`front/js/common.js`) are a pair and must resolve their target the same way. `showSpinner()` calls `resolveSpinnerTarget()`; `hideSpinner()` must too, not an independent lookup (e.g. grabbing `$(".spinnerTarget").last()` in raw document order) - two different targeting strategies on the two halves of one show/hide pair means the one feature whose tab happens to sit earliest in the page's markup gets the wrong element locked in when the overlay animates closed. If you add a new global or shared show/hide pair, check that both halves call the identical resolver.
+
+**Readiness gating:** a render function invoked from a per-tab polling updater that issues its own network call (GraphQL/REST) must be gated the same way an existing equivalent is - e.g. wrapped in `callAfterAppInitialized()` (`front/js/app-init.js`), which waits for both `isAppInitialized()` and the GraphQL server to be confirmed running before calling back. A render path added without this gate can fire its request at the earliest possible point in the page lifecycle (most likely when its tab loads as the *default* tab on page load, before the server is confirmed up) and fail. If the underlying request is wrapped in a cache keyed by id (e.g. a per-mac promise cache) that caches unconditionally, that one early failure poisons the cache entry permanently - nothing will ever retry it, even once the server comes up moments later. Any such cache must evict its own entry on rejection so a later poll tick gets a fresh attempt.
+
+## Fetch once, filter client-side
+
+When a page needs two or more related views over the same underlying data (e.g. a "currently valid" list and a "stale/historical" list derived from the same rows), issue one request and split the result client-side rather than issuing a second request with a different filter. The data doesn't change between the two views, so a second round-trip only adds latency and server load for no new information. Example: Session Info's "Known IPs" and "IP History" chip lists (`front/deviceDetailsSessionInfo.php`) both come from one `getSourcesFieldData()` call - the response is partitioned into the two lists by `entry.status` in JS, not fetched twice with two different status filters. This is also why `getSourcesFieldData()`/`getFieldViewDefinitions()` are themselves cached per-mac (`front/deviceDetails.php`) - so Field View and Session Info, which need the same underlying rows for different purposes, share one fetch instead of each issuing its own.
+
+## Don't omit information to simplify a view
+
+A UI must not be made worse by omitting information - if data exists and is relevant, default to showing it (with a status/staleness indicator if needed) rather than hiding it outright to make a view look cleaner. Example: Session Info's "Known IPs" chip list originally filtered out any plugin entry with `status === 'missing-in-last-scan'`, which produced a confusing "sometimes shows No data" bug even when a plugin had reported the IP recently - the fix was a separate "IP History" list for those entries (mirroring Field View's own "Not Reported" status badge for the same status, rather than hiding it), not filtering them out of existence. Reserve actual omission for genuinely irrelevant data (e.g. a field the user has no permission to see), not for data that's merely uncertain, pending, or outdated.
+
 ## Priority order for design decisions
 
 When several options are all locally reasonable, resolve the choice in this order - highest wins on conflict: