Add management tabs for diagnostics and related pages - #1538
KillariDev wants to merge 10 commits into
Conversation
- Route settings, website access, simulation stack, and diagnostics through a unified management view - Add diagnostics storage, copy/clear actions, and tabbed navigation
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CI Agent Review
The changeset consolidates four standalone extension pages into a single tabbed management page and adds a diagnostics viewer. Reviewers identified one behavioral regression in the virtual scroller and several architectural concerns about the new management-page routing model. No exploitable security vulnerabilities were found.
| const observer = new ResizeObserver(([entry]) => { | ||
| if (entry === undefined || !isPositiveFinite(entry.contentRect.height)) return | ||
| itemHeight.value = entry.contentRect.height | ||
| }) |
There was a problem hiding this comment.
Behavioral regression: the new ResizeObserver measurement assigns itemHeight unconditionally, whereas the previous implementation only ever grew the measured height (itemHeight.value > itemRef.current.clientHeight guard). Because itemHeight feeds maxItems, scrollAreaHeight, scrollOffset, and the scrollTop-synchronizing effect, a row-height shrink mid-scroll recomputes the whole virtual window and re-clamps the scroll index, causing the viewport to jump and potentially land on a different row than the user was reading. The address book now embedded in the ManagementView renders heterogeneous row heights (active-address rows with an extra checkbox/line, ERC20 'Decimals:' lines, NFT 'Protocol:' lines), so this can manifest in practice. Consider keeping the grow-only floor behavior or otherwise stabilizing the scroll position when the measured height shrinks.
| const managementPages: readonly ManagementPage[] = ['websites', 'address-book', 'simulation-stack', 'diagnostics', 'settings'] | ||
|
|
||
| export function getManagementPageFromHash(hash: string): ManagementPage { | ||
| if (hash.startsWith('#origin:')) return 'websites' |
There was a problem hiding this comment.
Architecture: management tab routing is coupled to the hosted pages' private URL-hash namespaces. The '#origin:' prefix is duplicated as a string literal here even though it already exists as the module-private, unexported URL_HASH_PREFIX in app/ts/components/pages/WebsiteAccess.tsx. Because window.location.hash is simultaneously the tab identity and each panel's state, every tab-bar click fires hashchange in all mounted panels; WebsiteAccessProvider unconditionally rebuilds selectedDomain from the hash, so switching tabs silently wipes the currently selected website that was previously deep-linkable via '#origin:...'. The two literal copies can also drift independently (renaming URL_HASH_PREFIX would break tab selection with no compiler error). Consider exporting a shared constant or otherwise decoupling the router from panel hash conventions.
| import type { TransactionOrMessageIdentifier } from '../types/interceptor-messages.js' | ||
| import { getSimulationStackTargetElementIdFromHash, getSimulationStackTargetHash } from './simulationStackTargets.js' | ||
|
|
||
| export type ManagementPage = 'websites' | 'address-book' | 'simulation-stack' | 'diagnostics' | 'settings' |
There was a problem hiding this comment.
Architecture: adding a management tab requires lockstep edits across the ManagementPage union, managementPages array, createMountedManagementPages, the switch in mountManagementPage, getManagementPageFromOpenRequest/getManagementTabTarget, getManagementPageFromNavigationKey, and the five near-identical
| export function getManagementTabTarget(method: ManagementOpenRequest): ManagementTabTarget { | ||
| const page = getManagementPageFromOpenRequest(method) | ||
| return { | ||
| tabName: 'settingsView', |
There was a problem hiding this comment.
Architecture: rerouting the popup open commands leaves a parallel legacy page model alive. openNewTab and the idsOfOpenedTabs registry still model addressBook, websiteAccess, and simulationStack as independent top-level tabs, and scripts/generate-extension-pages.mts still ships those standalone pages with their own entry renderers. A user with an already-open legacy addressBook tab who clicks 'Address Book' in the popup gets a second settingsView tab instead of reusing the existing one, because idsOfOpenedTabs.addressBook is never consulted by the new command. The hardcoded tabName: 'settingsView' also bakes the hosting page name into a generic-sounding utility; deleting the legacy pages later will require coordinated cleanup of openNewTab's ExtensionTabName, getHtmlFile, and the opened-tab registry that this diff does not prepare for.
…flicts # Conflicts: # app/ts/background/popupMessageHandlers.ts
|
/review |
There was a problem hiding this comment.
CI Agent Review
The changeset consolidates the standalone extension pages into a shared tabbed management page and reworks the error-diagnostics pipeline. Two structural concerns were raised by the architecture review. First, the new management router (app/ts/utils/managementPages.ts) hard-copies the website-access sub-page's hash grammar ('#origin:') instead of delegating to the sub-page's own constant, and silently falls back to the Websites tab for any unrecognized hash, so mistyped or future deep-link hashes will be silently misrouted to the wrong tab. Second, a parallel routing taxonomy (ManagementPage/ManagementTabTarget) is layered on top of the existing per-tab routing model (ExtensionTabName/idsOfOpenedTabs) without retiring the legacy one: the popup 'open X' commands now always open settingsView, yet openNewTab still accepts all four tab names and idsOfOpenedTabs still persists four tab-id keys, leaving the simulation stack reachable via both a legacy standalone tab and the new settingsView#simulation-stack tab, with the command-to-tab mapping split across multiple modules rather than a single source of truth.
| const managementPages: readonly ManagementPage[] = ['websites', 'address-book', 'simulation-stack', 'diagnostics', 'settings'] | ||
|
|
||
| export function getManagementPageFromHash(hash: string): ManagementPage { | ||
| if (hash.startsWith('#origin:')) return 'websites' |
There was a problem hiding this comment.
Architectural concern: getManagementPageFromHash hard-copies the website-access sub-page's hash grammar ('#origin:') instead of delegating to the sub-page's own URL_HASH_PREFIX constant in WebsiteAccess.tsx. This duplicates the source of truth in a second module; if the two diverge, deep links will silently activate the wrong tab. Route the sub-page's hash-grammar predicate into the router (as is done for the simulation stack via getSimulationStackTargetElementIdFromHash) rather than embedding a literal copy.
| if (hash.startsWith('#origin:')) return 'websites' | ||
| if (getSimulationStackTargetElementIdFromHash(hash) !== undefined) return 'simulation-stack' | ||
| const hashPage = hash.startsWith('#') ? hash.slice(1) : hash | ||
| return managementPages.find((page) => page === hashPage) ?? 'websites' |
There was a problem hiding this comment.
Architectural concern: any unrecognized hash silently falls through to the 'websites' tab (?? 'websites'). Typos, future hash formats, and new sub-page prefixes will be silently misrouted instead of surfacing a failure. Consider failing loudly or routing unknown hashes to a neutral page.
| const handleHashChange = () => { | ||
| const hash = window.location.hash | ||
| const domainInHash = hash.slice(URL_HASH_PREFIX.length) | ||
| const domainInHash = hash.startsWith(URL_HASH_PREFIX) ? hash.slice(URL_HASH_PREFIX.length) : '' |
There was a problem hiding this comment.
Architectural concern: this guard now tolerates hashes that do not start with URL_HASH_PREFIX, which is required for the shared management router, but the router re-encodes this prefix as a literal '#origin:' in managementPages.ts. The hash grammar is now duplicated across two modules with no shared source of truth, so the prefix contract can silently diverge.
| } | ||
| } | ||
|
|
||
| export function getManagementTabTarget(method: ManagementOpenRequest): ManagementTabTarget { |
There was a problem hiding this comment.
Architectural concern: a second, parallel routing taxonomy (ManagementPage/ManagementTabTarget) is layered on top of the existing per-tab routing model. openNewTab still accepts all four ExtensionTabName values and idsOfOpenedTabs still persists four tab-id keys, yet the popup 'open X' commands now only ever produce 'settingsView'. The mapping is baked into this module rather than living in the protocol module that already lists these commands, and the legacy standalone pages (e.g. simulationStack) remain reachable, so the same feature can live in two different tabs depending on entry path. Either retire the legacy per-feature tab tracking or centralize the mapping in a single table.
|
/review |
There was a problem hiding this comment.
CI Agent Review
This changeset consolidates the standalone settings/address-book/website-access/simulation-stack pages into a hash-routed management hub and adds a Diagnostics page. Four issues were raised across reviewers: (1) making rawError a required codec field silently wipes previously stored diagnostics on upgrade, because legacy records missing that key fail the strict array parse and the catch path in getInterceptorErrorDiagnostics() removes the entire stored history; (2) the Diagnostics tab only loads data on mount while panels stay mounted, so it shows stale data on every visit after the first; (3) the hub creates bidirectional coupling with the embedded pages — the hub router must parse each page's internal hash formats, pages write hub tab hashes, and hub CSS reaches into page internals; (4) visited pages are kept mounted with active runtime listeners, so hidden tabs keep processing background broadcasts and issuing queries.
| severity: InterceptorErrorSeverity, | ||
| message: funtypes.String, | ||
| cause: funtypes.Union(funtypes.String, funtypes.Undefined), | ||
| rawError: funtypes.Union(funtypes.String, funtypes.Undefined), |
There was a problem hiding this comment.
Making rawError a required field destroys existing stored diagnostics on upgrade. Records written by the previously shipped version do not contain the rawError key; because browserStorageLocalGet parses the entire interceptorErrorDiagnostics array through the InterceptorErrorDiagnostic codec at once, the first legacy record missing rawError throws, and the catch block in getInterceptorErrorDiagnostics() (app/ts/background/storageVariables.ts) calls browserStorageLocalRemove, deleting the whole stored history rather than just the malformed records. The same applies to latestUnexpectedError via the UnexpectedErrorOccured codec (app/ts/types/interceptor-reply-messages.ts:23). Add a migration/compat read path (or parse records individually) so existing users do not silently lose their recorded error history on upgrade.
| await clipboardCopy(formatDiagnosticsForClipboard(diagnostics.peek())) | ||
| } | ||
|
|
||
| useEffect(() => { void waitForLoad(loadDiagnostics) }, []) |
There was a problem hiding this comment.
Diagnostics are fetched only in this mount effect, but ManagementView keeps mounted panels alive after first selection (mountManagementPage never unmounts). Switching away from and back to the Diagnostics tab does not remount the component, so this effect does not re-run, no popup_requestDiagnostics is sent, and the panel keeps showing the snapshot from the first visit — newly recorded errors are only visible after a manual Refresh.
|
|
||
| export function getManagementPageFromHash(hash: string): ManagementPage | undefined { | ||
| if (hash === '' || hash === '#') return 'home' | ||
| if (isWebsiteOriginHash(hash)) return 'websites' |
There was a problem hiding this comment.
The hub router now has to parse each embedded page's internal hash scheme (website-origin hashes here, simulation-stack target hashes on the next line), while the embedded pages in turn depend on the hub's routing — clearSelectedWebsite in WebsiteAccess.tsx:105 now writes getManagementPageHash('websites') — and the hub CSS reaches into page internals (.management-panel .address-book-content, .management-panel .simulation-stack-page > .simulation-stack-page-header, .management-panel .access-details). This bidirectional coupling means any change to an embedded page's hash format, layout, or scroll behavior must be coordinated with the hub router and hub CSS; pages can no longer evolve independently of the hub.
|
|
||
| function activatePage(page: ManagementPage | undefined) { | ||
| batch(() => { | ||
| if (page !== undefined) mountedPages.value = mountManagementPage(mountedPages.peek(), page) |
There was a problem hiding this comment.
Mounting a page on first visit and never unmounting it means hidden tab panels keep their browser.runtime.onMessage listeners active and keep issuing background queries and side effects while the user is on another tab (AddressBook reacts to popup_addressBookEntriesChanged/popup_settingsUpdated and issues popup_getAddressBookData; WebsiteAccessView reacts to popup_websiteAccess_changed and issues popup_retrieveWebsiteAccess). Previously each page ran in its own document, so only the visible page's listeners were active. Consider unmounting inactive panels or gating page effects/listeners on visibility to preserve the pages' original single-active-document assumptions.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The management-tab consolidation introduces a concrete UI bug: the selected tab clipped on the right of the tab bar is never scrolled into view (tabBounds.right - tabBounds.right == 0 instead of tabBounds.right - listBounds.right). It also raises structural concerns: management page identity is registered across multiple hand-synchronized files, location.hash routing is split between the shell and the embedded panels (with listHash and a magic '#' sentinel leaking host concerns into panel APIs), and the settingsView storage key now permanently represents the entire management hub. No security findings were reported by the Security or Defender agents.
| const tabBounds = tab.getBoundingClientRect() | ||
| const listBounds = tabList.getBoundingClientRect() | ||
| if (tabBounds.left < listBounds.left) tabList.scrollLeft -= listBounds.left - tabBounds.left | ||
| else if (tabBounds.right > listBounds.right) tabList.scrollLeft += tabBounds.right - listBounds.right |
There was a problem hiding this comment.
Logic bug in revealSelectedTab: the right-overflow branch computes tabBounds.right - tabBounds.right (== 0) instead of tabBounds.right - listBounds.right, so scrollLeft is never adjusted when the active tab is clipped on the right. With overflow-x: auto on .management-tabs (especially under the ≤520px layout where tabs get their own row), opening the management page directly on #settings/#diagnostics or navigating via the tablist arrow keys leaves the active/focused tab outside the scrollable viewport. Should be tabList.scrollLeft += tabBounds.right - listBounds.right.
| import type { TransactionOrMessageIdentifier } from '../types/interceptor-messages.js' | ||
| import { getSimulationStackTargetHash } from './simulationStackTargets.js' | ||
|
|
||
| export type ManagementPage = 'home' | 'websites' | 'address-book' | 'simulation-stack' | 'diagnostics' | 'settings' |
There was a problem hiding this comment.
Architecture: management page identity is scattered across at least four lockstep locations — the ManagementPage union/managementPages array (lines 4–6), the getManagementPageFromHash prefix chain, the getManagementPageFromOpenRequest switch, and the hand-written <section role='tabpanel'> blocks in ManagementView.tsx that are structurally identical but not derived from managementPages/managementSectionDetails. Adding a page requires coordinated edits to all of them (as demonstrated by diagnostics being added in this very changeset). The panels should be rendered from the managementPages data so a new page is one data entry plus a single render case.
| export type ManagementOpenRequest = 'popup_openManagement' | 'popup_openWebsiteAccess' | 'popup_openAddressBook' | 'popup_openSettings' | ||
| export const managementPages: readonly ManagementPage[] = ['home', 'websites', 'address-book', 'simulation-stack', 'diagnostics', 'settings'] | ||
|
|
||
| export function getManagementPageFromHash(hash: string): ManagementPage | undefined { |
There was a problem hiding this comment.
Architecture: location.hash is now a multi-owner protocol. getManagementPageFromHash hardcodes hash grammar owned by websiteAccessHash.ts (#websites?, legacy #origin:) and simulationStackTargets.ts (#simulation-stack?, legacy #simulation-stack-target=), and panel components leak host-routing concerns into their public API (WebsiteAccessView's listHash prop, websiteAccess.ts passing a routing-irrelevant '#' sentinel, simulationStackTargets changing its output format to fit the router namespace). Changing either side's grammar silently breaks routing/deep-linking on the other, with only hand-written cross-module tests enforcing consistency. Consider a single hash-routing owner shared by both the shell and the still-standalone pages.
| write: async (idsOfOpenedTabs) => { await browserStorageLocalSet({ idsOfOpenedTabs }) }, | ||
| getDefault: () => ({ settingsView: undefined, addressBook: undefined, websiteAccess: undefined, simulationStack: undefined }), | ||
| }) | ||
| // Keep the stored key so existing settings tabs remain discoverable after upgrade. |
There was a problem hiding this comment.
Naming debt: getManagementTabId/setManagementTabId now expose the whole management hub but persist under the idsOfOpenedTabs.settingsView key, and IdsOfOpenedTabs is reduced to only { settingsView }, so the stored key no longer matches the concept. Keeping the legacy key is justified for migration, but the permanent mismatch means future maintainers must remember 'settingsView' in storage means the management tab. An explicit constant or a renamed managementTabId key with a read-time fallback would keep the storage contract aligned with the concept.
|
/review |
This branch has not been deployed
1 participant
Summary
Screenshots
Captured from the built Chrome extension. The Diagnostics tab keeps its neutral logo even when sample error, warning, and info records are present. Images are linked from an immutable asset-only commit and are not part of this PR's source diff.
Popup entry
Home
Home at narrow width
Websites
Address Book
Diagnostics
Expanded raw error details
Diagnostics at narrow width
Settings
Testing
bun run test— 1,506 passedbun run setup-chromebun run typecheckbun run lintbun run test:chrome-communication— passed after merging the latest content-script changes