fix: 'New' badge flagged the entire existing device population
Deployed the new-device feature, checked the live API response instead of assuming the earlier unit test covered it, and found 65/71 devices marked isNew: true. The notification path was correctly bootstrap-safe (verified separately), but the UI's isNew check just tested "first_ever_seen within 24h" with no bootstrap awareness -- and bootstrap timestamps are, correctly, "now", so the whole existing population qualified on day one. Added seen_macs.is_bootstrap, set on the seeding call and excluded from isNew. Migration backfills is_bootstrap=1 for any seen_macs rows that already existed before this column did (the already-deployed CT122 instance) -- verified locally against both a fresh DB and a simulated pre-migration table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -58,10 +58,16 @@ export function openDb(path: string): Database.Database {
|
||||
-- Deliberately separate from the devices table (keyed by IP, which churns
|
||||
-- on DHCP renewal) -- keying "have we ever seen this MAC" by IP would
|
||||
-- generate a false new-device alert every time an existing device's
|
||||
-- lease just happened to change.
|
||||
-- lease just happened to change. is_bootstrap marks rows seeded by the
|
||||
-- very first (empty-table) call -- without it, the "New" UI badge (which
|
||||
-- just checks "seen within 24h") would flag the entire pre-existing
|
||||
-- device population as new on the day this feature is first deployed,
|
||||
-- since bootstrap timestamps are, correctly, "now". Caught by actually
|
||||
-- checking the deployed API response, not just the notification-side unit test.
|
||||
CREATE TABLE IF NOT EXISTS seen_macs (
|
||||
mac TEXT PRIMARY KEY,
|
||||
first_seen TEXT NOT NULL DEFAULT (datetime('now'))
|
||||
first_seen TEXT NOT NULL DEFAULT (datetime('now')),
|
||||
is_bootstrap INTEGER NOT NULL DEFAULT 0
|
||||
);
|
||||
`);
|
||||
// Migration for the devices table pre-dating mdns_hostname (CREATE TABLE IF
|
||||
@@ -73,6 +79,19 @@ export function openDb(path: string): Database.Database {
|
||||
(c) => c.name === "mdns_hostname"
|
||||
);
|
||||
if (!hasMdnsColumn) db.exec(`ALTER TABLE devices ADD COLUMN mdns_hostname TEXT`);
|
||||
|
||||
// Same situation for seen_macs.is_bootstrap. Any rows that already existed
|
||||
// before this column was added were, by definition, from a table that had
|
||||
// no bootstrap tracking at all -- backfill them as bootstrap rows (the
|
||||
// true history) so they don't show up as "new" in the UI once this
|
||||
// migration lands on an already-deployed instance.
|
||||
const hasBootstrapColumn = (db.pragma("table_info(seen_macs)") as { name: string }[]).some(
|
||||
(c) => c.name === "is_bootstrap"
|
||||
);
|
||||
if (!hasBootstrapColumn) {
|
||||
db.exec(`ALTER TABLE seen_macs ADD COLUMN is_bootstrap INTEGER NOT NULL DEFAULT 0`);
|
||||
db.exec(`UPDATE seen_macs SET is_bootstrap = 1`);
|
||||
}
|
||||
return db;
|
||||
}
|
||||
|
||||
@@ -196,6 +215,7 @@ export interface DeviceRow {
|
||||
first_seen: string;
|
||||
last_seen: string;
|
||||
first_ever_seen: string | null;
|
||||
is_bootstrap: number | null;
|
||||
}
|
||||
|
||||
// Devices not seen in the last 24h (unplugged, moved, DHCP lease expired) are
|
||||
@@ -210,7 +230,7 @@ export function getRecentDevices(db: Database.Database, sinceHours = 24): Device
|
||||
return db
|
||||
.prepare(
|
||||
`SELECT d.ip, d.mac, d.known_name, d.mdns_hostname, l.label AS manual_label,
|
||||
d.first_seen, d.last_seen, s.first_seen AS first_ever_seen
|
||||
d.first_seen, d.last_seen, s.first_seen AS first_ever_seen, s.is_bootstrap
|
||||
FROM devices d
|
||||
LEFT JOIN device_labels l ON l.mac = d.mac
|
||||
LEFT JOIN seen_macs s ON s.mac = d.mac
|
||||
@@ -250,9 +270,11 @@ export function recordSeenMacs(db: Database.Database, macs: string[]): string[]
|
||||
);
|
||||
const newMacs = macs.filter((mac) => !alreadySeen.has(mac));
|
||||
|
||||
const insert = db.prepare(`INSERT OR IGNORE INTO seen_macs (mac) VALUES (@mac)`);
|
||||
const insert = db.prepare(
|
||||
`INSERT OR IGNORE INTO seen_macs (mac, is_bootstrap) VALUES (@mac, @isBootstrap)`
|
||||
);
|
||||
const tx = db.transaction((items: string[]) => {
|
||||
for (const mac of items) insert.run({ mac });
|
||||
for (const mac of items) insert.run({ mac, isBootstrap: isBootstrap ? 1 : 0 });
|
||||
});
|
||||
tx(macs);
|
||||
|
||||
|
||||
@@ -37,6 +37,11 @@ function isRecent(isoTimestamp: string | null, hours: number): boolean {
|
||||
|
||||
function toApiDevice(r: DeviceRow) {
|
||||
const name = r.manual_label ?? r.known_name ?? null;
|
||||
// is_bootstrap excludes devices that were only ever "new" because this
|
||||
// feature had just been turned on, not because they actually just
|
||||
// appeared -- otherwise the whole pre-existing device population shows as
|
||||
// "new" for 24h after every fresh deploy of this feature.
|
||||
const isNew = r.is_bootstrap === 0 && isRecent(r.first_ever_seen, NEW_DEVICE_WINDOW_HOURS);
|
||||
return {
|
||||
ip: r.ip,
|
||||
mac: r.mac,
|
||||
@@ -44,7 +49,7 @@ function toApiDevice(r: DeviceRow) {
|
||||
known: r.manual_label !== null || r.known_name !== null,
|
||||
vendor: getVendor(r.mac),
|
||||
mdnsHostname: r.mdns_hostname,
|
||||
isNew: isRecent(r.first_ever_seen, NEW_DEVICE_WINDOW_HOURS),
|
||||
isNew,
|
||||
firstSeen: r.first_seen,
|
||||
lastSeen: r.last_seen,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user