feat(roadmap): M-B foundation — detector registry + hero detector + onIdle driver (recovery of #41)#51
Conversation
hoainho
added
the
tracked-plan
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a pluggable detector registry foundation and React version adapters to handle fiber tag differences across React versions. It migrates existing detectors to this registry pattern, schedules deferred work via idle callbacks, adds a new reconciler-keys detector, and introduces a schema-validated settings storage system with a corresponding Settings UI tab. The review feedback identifies several critical issues: a protocol mismatch in the per-site override settings where full URLs are saved instead of hostnames, a logic bug in the site enablement check that falls back to deleted legacy storage after migration, a positional bug in the mixed keyed/unkeyed sibling check of the reconciler-keys detector, and duplicated React version detection logic in the inject script.
| const handleAddSiteOverride = async () => { | ||
| if (!settings || !newSiteOrigin.trim()) return; | ||
| const origin = newSiteOrigin.trim(); | ||
| const perDetectorOverrides: Record<string, { enabled: boolean }> = {}; | ||
| for (const { id } of KNOWN_DETECTORS_DEFAULTS) { | ||
| perDetectorOverrides[id] = { enabled: false }; | ||
| } | ||
| const updated: Settings = { | ||
| ...settings, | ||
| perSite: { | ||
| ...settings.perSite, | ||
| [origin]: { detectors: perDetectorOverrides }, | ||
| }, | ||
| }; | ||
| setSettings(updated); | ||
| setNewSiteOrigin(''); | ||
| await write(updated); | ||
| }; |
There was a problem hiding this comment.
The input field has type="url" and placeholder="https://app.example.com", which forces/encourages the user to enter a full URL with a protocol (e.g., https://app.example.com). However, origin is saved directly as the key in perSite, while checkIfSiteEnabled in content/index.ts looks up window.location.hostname (e.g., app.example.com). Because of the protocol mismatch, the per-site override will never match and the feature is completely broken. We should extract the hostname from the entered URL before saving it.
const handleAddSiteOverride = async () => {
if (!settings || !newSiteOrigin.trim()) return;
let hostname = newSiteOrigin.trim();
try {
const url = new URL(hostname);
hostname = url.hostname;
} catch {
// Fallback to raw input if it's not a parseable URL
}
if (!hostname) return;
const perDetectorOverrides: Record<string, { enabled: boolean }> = {};
for (const { id } of KNOWN_DETECTORS_DEFAULTS) {
perDetectorOverrides[id] = { enabled: false };
}
const updated: Settings = {
...settings,
perSite: {
...settings.perSite,
[hostname]: { detectors: perDetectorOverrides },
},
};
setSettings(updated);
setNewSiteOrigin('');
await write(updated);
};
| if (!migrationDone) { | ||
| const result = await migrate(); | ||
| migrationDone = true; | ||
| const siteOverride = result.settings.perSite[hostname]; | ||
| if (siteOverride?.detectors) { | ||
| const allDisabled = Object.values(siteOverride.detectors).every( | ||
| (d) => !d.enabled | ||
| ); | ||
| if (allDisabled) return false; | ||
| } | ||
| return true; | ||
| } | ||
| const legacyResult = await chrome.storage.local.get(STORAGE_KEY); | ||
| const disabledSites: string[] = legacyResult[STORAGE_KEY] || []; | ||
| return !disabledSites.includes(hostname); |
There was a problem hiding this comment.
Once migrationDone is set to true, subsequent calls to checkIfSiteEnabled will bypass the new settings check and fall back to reading the legacy STORAGE_KEY (react_debugger_disabled_sites). Since the legacy key is deleted during migration, legacyResult[STORAGE_KEY] will be undefined, causing the function to always return true (enabled) even if the site was disabled in the new settings. We should always use the migrated settings directly.
const result = await migrate();
const siteOverride = result.settings.perSite[hostname];
if (siteOverride?.detectors) {
const allDisabled = Object.values(siteOverride.detectors).every(
(d) => !d.enabled
);
if (allDisabled) return false;
}
return true;| let child: FiberNode | null = fiber.child; | ||
| const keyed: ListChild[] = []; | ||
| let index = 0; | ||
| let hasUnkeyed = false; | ||
| while (child !== null) { | ||
| if (child.key !== null && child.key !== undefined) { | ||
| keyed.push({ key: String(child.key), index }); | ||
| } else if (keyed.length > 0) { | ||
| // Mixed keyed + unkeyed siblings — punt; this is the legacy | ||
| // checkListKeys territory (MISSING_KEY). | ||
| hasUnkeyed = true; | ||
| } | ||
| index++; | ||
| child = child.sibling; | ||
| } | ||
| if (hasUnkeyed) return; | ||
| if (keyed.length < 2) return; |
There was a problem hiding this comment.
There is a positional bug in the mixed keyed/unkeyed sibling check. If an unkeyed sibling is at the beginning of the list (e.g., [unkeyed, keyed, keyed]), keyed.length is 0 when the unkeyed sibling is processed, so hasUnkeyed remains false and the list is incorrectly analyzed. If the unkeyed sibling is at the end, it is correctly punted. We should count the unkeyed siblings and check if both keyed and unkeyed siblings exist under the same parent to consistently punt on mixed lists.
| let child: FiberNode | null = fiber.child; | |
| const keyed: ListChild[] = []; | |
| let index = 0; | |
| let hasUnkeyed = false; | |
| while (child !== null) { | |
| if (child.key !== null && child.key !== undefined) { | |
| keyed.push({ key: String(child.key), index }); | |
| } else if (keyed.length > 0) { | |
| // Mixed keyed + unkeyed siblings — punt; this is the legacy | |
| // checkListKeys territory (MISSING_KEY). | |
| hasUnkeyed = true; | |
| } | |
| index++; | |
| child = child.sibling; | |
| } | |
| if (hasUnkeyed) return; | |
| if (keyed.length < 2) return; | |
| let child: FiberNode | null = fiber.child; | |
| const keyed: ListChild[] = []; | |
| let index = 0; | |
| let unkeyedCount = 0; | |
| while (child !== null) { | |
| if (child.key !== null && child.key !== undefined) { | |
| keyed.push({ key: String(child.key), index }); | |
| } else { | |
| unkeyedCount++; | |
| } | |
| index++; | |
| child = child.sibling; | |
| } | |
| if (unkeyedCount > 0 && keyed.length > 0) return; | |
| if (keyed.length < 2) return; | |
| import { installCleanupInterval, uninstallCleanupInterval } from './lifecycle'; | ||
| import { createRegistry } from './registry'; | ||
| import { registerAllDetectors } from './detectors'; | ||
| import { sanitizeValue as sanitizeForRegistry } from '../utils/sanitize'; |
There was a problem hiding this comment.
To avoid code duplication, we should import detectReactVersion from the ./react-adapters module instead of redefining it inside this file.
| import { installCleanupInterval, uninstallCleanupInterval } from './lifecycle'; | |
| import { createRegistry } from './registry'; | |
| import { registerAllDetectors } from './detectors'; | |
| import { sanitizeValue as sanitizeForRegistry } from '../utils/sanitize'; | |
| import { installCleanupInterval, uninstallCleanupInterval } from './lifecycle'; | |
| import { createRegistry } from './registry'; | |
| import { registerAllDetectors } from './detectors'; | |
| import { sanitizeValue as sanitizeForRegistry } from '../utils/sanitize'; | |
| import { detectReactVersion as getReactVersion } from './react-adapters'; |
| function detectReactVersion(): string { | ||
| return (window as any).React?.version || 'unknown'; | ||
| } | ||
| const hook = (window as any).__REACT_DEVTOOLS_GLOBAL_HOOK__; | ||
| // Priority 1: renderer.version — works for ESM, Next.js App Router, bundled React | ||
| // Source: https://github.com/facebook/react/blob/05ca66ad9c/packages/react-devtools-shared/src/backend/types.js#L131 | ||
| if (hook?.renderers instanceof Map && hook.renderers.size > 0) { | ||
| const renderer = hook.renderers.values().next().value; | ||
| if (typeof renderer?.version === 'string' && renderer.version) { | ||
| return renderer.version; | ||
| } | ||
| } | ||
| // Priority 2: window.React.version (fails for ESM-only bundles) | ||
| const reactVersion = (window as any).React?.version; | ||
| if (typeof reactVersion === 'string' && reactVersion) return reactVersion; | ||
| // Priority 3: reconcilerVersion | ||
| if (hook?.renderers instanceof Map && hook.renderers.size > 0) { | ||
| const renderer = hook.renderers.values().next().value; | ||
| if (typeof renderer?.reconcilerVersion === 'string' && renderer.reconcilerVersion) { | ||
| return renderer.reconcilerVersion; | ||
| } | ||
| } | ||
| return 'unknown'; | ||
| } |
There was a problem hiding this comment.
This function is duplicated in src/inject/react-adapters/index.ts. We should replace it with a call to the imported getReactVersion helper.
function detectReactVersion(): string {
return getReactVersion() || 'unknown';
}
hoainho
added
tracked-plan
…nIdle driver T1: Detector<TIssue> registry with type-safe contract (src/types/registry.ts, src/inject/registry.ts) T2: Registry dispatch wired into inject pipeline T3: React adapters r17/r18/r19/r19.2 with version sniffing T4: Settings storage layer with zod validation (src/settings/storage.ts, types.ts) T5: v0→v1 settings migration with backup (src/settings/migrate.ts) T6: Settings UI tab with toggles + thresholds (src/panel/tabs/SettingsTab.tsx) T7: Reconciler-keys hero detector (src/inject/detectors/reconciler-keys.ts) T8: Closure-leak Strategy-A bridge (src/inject/detectors/closure-leak.ts) T9: Scan-overlay + onIdle driver + getBoundingClientRect moved off hot path T10: drainAll + smoke-test coverage (src/__tests__/*.test.ts) Test pass: 219 (baseline at M-A: 141) tsc --noEmit: 0 errors Recovery branch — replays content from orphaned feat/self-roadmap-m-b-registry onto origin/main now that M-A landed via #50.
hoainho
force-pushed
the
feat/m-b-recovery
branch
from
047749d to
5d1cb4e
Compare
1 participant
Self-Roadmap H2 2026 — Milestone M-B (Detector Registry + Hero #1)
Ships the detector registry foundation + first new detector since v2.0.3 + registry migration of the two existing detectors, all per the Momus-approved OpenSpec change.
Why M-B matters
reconciler-keys): flagsMath.random()/Date.now()keys + verified cross-commit reorders on numeric-index keys. Confidence: high, defaults ON. Production-capable.getBoundingClientRect()moved from synchronousonCommitto deferredonIdlevia new registry idle driver.10 task groups, squashed into one commit
Detector<TIssue>interface (staged-write transactionality, LRU dedupe, throw isolation). 20 tests.reconciler-keys(newUNSTABLE_LIST_KEYissue type). 11 tests.closure-leakextracted to registry (Strategy A bridge pattern). 7 tests.scan-overlayextracted + registrydispatchIdledriver added.getBoundingClientRect()now runs only inonIdle. 8 tests.periodicCleanupdrains registry, M_B_SMOKE_TEST.md runbook, CHANGELOG [Unreleased] (merged with PR #47/#48 entries).Verification gates (all green)
Critical perf gate
After T9, zero synchronous
getBoundingClientRectcalls in the injectonCommitFiberRoothot path when scan is enabled. The only callsite is now insideflashRenderOverlay, invoked exclusively frombridge.paint()driven by the detector'sonIdlelifecycle.Files in this PR
CHANGELOG merge
M-B's
[Unreleased]section is merged with main's existing[Unreleased]content (PR #47 claim rule + PR #48 star check). Combined section now lists: Added (workflow + detector registry + adapters + settings storage + UI + reconciler-keys + onIdle driver), Changed (claim policy hardened + closure-leak migration + scan-overlay perf fix), Fixed (drainAll), Migration (settings v0→v1 + star-rule grandfathering).Production smoke test (cannot run in this PR)
M_B_SMOKE_TEST.md— 12 TCs covering build verification, default-policy correctness, all 3 detectors, Settings UI walkthrough, migration paths, memory growth check.Pipeline used
Each of T1-T10 ran through the SDD pipeline (implementer → spec-compliance reviewer → code-quality reviewer). 1 loop-back on T1 (anti-pattern cast fix), 1 on T5 (contract violation), all 8 others first-pass.
Notes
.opencode/plans/2026-05-31-self-roadmap-m-b.mdpreserves the task narrative + done-notes for posterity.tracked-planlabeled, so the Star Check workflow auto-exempts (maintainer + tracked-plan double exemption).