💭 I'm examining the PR diff to understand what changes were made. I need to narrow my focus to just the apps/web/src directory since this PR is massive. Let me check what's changed since the last review at commit e4775dc—there were 2 comments posted then, but the current head is e6e6dd4—so I should look at the diff for that specific path to catch any new issues or things that haven't been flagged yet. I'm checking what's changed since the last commit at e4775dc, where two issues were flagged: a Badge override in ComputerHistorySettings.tsx and a RowTitle duplication in ComputerUseMoreBrowsers.tsx. Looking at the current diff, it seems the Badge styling issue in ComputerHistorySettings.tsx is still present. The prior commit was force-pushed, so the diff is entirely new, but I need to avoid duplicating the two existing comments that still describe the current code. Let me look through the new UI code to identify other issues, particularly checking whether SettingsRow in settingsLayout.tsx supports the children, status, and resetAction props that ComputerUseMoreBrowsers is passing to it. I should also verify the available Button variants like ghost-muted and icon-micro, plus check the Badge component. I need to confirm that SettingIconAction properly imports ComponentPropsWithoutRef<typeof Button> and then examine the settingsLayout.tsx file to validate these component APIs. I'm checking how other settings pages handle consistency — ComputerHistorySettings uses searchableSetting which returns an object with id and title, and there's a minor inconsistency where RowTitle wraps content in an inline-flex span without much purpose. I'm starting to identify some key UI consistency patterns to address. Now I need to verify whether the color tokens like bg-success and bg-warning used in ExtensionStatus are actually defined in the CSS, since missing tokens would make those status indicators invisible — that's a concrete bug I should search for. Looking at the Collapsible component setup with CollapsibleTrigger using a render prop for the Button, and checking what the ui/collapsible exports along with the CollapsiblePanel's id prop. I'm noticing a potential conflict where ComputerUseMoreBrowsers manually sets aria-controls on the trigger, but Base UI's Collapsible already generates this automatically — so the manual attribute might override what Base UI intended. I've already noted the Badge issue. Now I'm checking the Button component to verify whether the icon-micro and ghost-muted variants exist, and to confirm the size-5 claim from my earlier comment. I'm noticing that SettingIconAction inherits Button's props, which means consumers could pass size or variant, but since those are spread after the defaults, caller values would override them. The className is destructured separately before the spread, so that's handled correctly. Looking at the other components, PrivacyExclusionList uses the Input and Button primitives straightforwardly. The SettingIconAction inside Badge passes type="button" explicitly, though Button already defaults to that. Accessibility-wise, having a button inside a span (which Badge renders) is fine. For ComputerHistorySettings, the searchableSetting spread for "computer-history-enabled" provides the id and title correctly. Now I need to check how searchableSetting is actually implemented. I'm noticing a mismatch in how the searchable settings are mapping their IDs to target IDs—some items like "computer-history-privacy" have matching IDs and targetIds, but others like "computer-history-enabled" have a targetId that points to a different ID ("computer-history"), which could cause lookup issues. I see the targetId pattern is working, but there's a conflict with the computer-history-privacy item—the spread operator passes in the catalog title "Computer History privacy", but then the explicit title="Privacy" attribute overrides it, which breaks the single source of truth principle. The same thing happens with computer-history-timeline getting overridden to just "History", while computer-history-enabled keeps its catalog title "Enable Computer History". This inconsistency between the search index and what actually renders is worth investigating—let me search for other instances where searchableSetting() is used with title overrides to see if this is a pattern across the settings pages. The id={searchableSetting(...).id} pattern with a separate title is already established in SourceControlSettings, so using spread with a title override isn't a significant finding. Now I'm looking for actual UI-consistency issues in the changed web code, and a strong candidate is the ComputerUseSettings line with its inline notice styling—the repo has an Alert primitive that might be more appropriate here. I'm also checking whether other settings pages use Alert for similar notices or if they reconstruct it with raw divs like this one does. The ComputerHistorySettings intro paragraphs use bare <p> tags with muted-foreground styling, which I need to verify against existing patterns. The timeline list uses a raw <ul> with custom row styling instead of SettingsRow, which is acceptable for a list. The button state management looks fine—busy is only set during clear operations. But there's a consistency gap: the "Permissions" SettingsSection in ComputerUseSettings is missing an id, which means search items won't work for it. Looking at the status indicators and icons, the ExtensionStatus dot uses bg-success/bg-warning but ComputerHistorySettings doesn't have one—not a real issue. The ComputerUseMoreBrowsers row uses EdgeIcon as the title icon, which is semantic. However, I'm spotting an actual consistency problem: the icon sizes vary across sibling rows in the same section—AgentCursorIcon is 24px while ChromeIcon and EdgeIcon are 20px, creating a visual inconsistency. Now I'm checking the Collapsible primitive to see if CollapsiblePanel supports the necessary attributes, and looking at how ComputerUseMoreBrowsers nests SettingsRow inside Collapsible—the padding adjustments look correct. I need to examine the hooks being used. The settings hooks show different patterns: ComputerHistorySettings passes a partial object while ComputerUseSettings spreads the full object, but that's not a UI concern. I'm also verifying the Tailwind class ordering across the codebase—there doesn't appear to be any index.css changes in this PR, and the class order suggests the repo might be using a Tailwind plugin for formatting. Looking for a concrete violation now, I'm checking whether the "remote environment" notice pattern reuses a shared Alert component elsewhere in the settings, and examining whether the Delete buttons in the timeline rows should use a more explicit destructive variant. I'm noticing that the "remote environment" notice in ComputerUseSettings is just a plain div with an icon and text, missing the accessibility role that the Alert primitive provides. The Alert component has a proper structure with an icon slot, rounded corners, border, and padding, while this div is deliberately minimal without those styling touches. I should check if Alert is being used elsewhere in the settings to understand the pattern better. The Alert component isn't heavily used in settings, so using a raw notice div might not be a clear departure from the page's conventions—probably not worth flagging. Let me search for how other settings pages handle desktop-only notices to see what the standard approach is. I'm noticing an inconsistency in how the Switch controls are disabled in ComputerHistorySettings. The "Mirror to Codex" switch remains interactive when not on desktop, while the Enabled and Paused switches are properly disabled in that state. Even though the mirror switch's patch operation still persists settings to the server, it's odd that it's enabled within a section labeled "recording runs in the desktop app" where the other controls are disabled. I need to verify that the Input component supports the onValueChange prop, and check whether it needs explicit width styling when used inside a flex container in the PrivacyExclusionList form. Looking at the sizing alignment between the Input and Button components — the Input defaults to h-8.5 (34px) while a size="sm" Button is h-8 (32px), which is a minor height mismatch. I'm checking whether the common pattern in the repo pairs them with matching sizes, likely using size="sm" on the Input as well. Actually, I'm getting into implementation details here. Let me refocus on the strongest findings from my earlier comments and move on to examining the ComputerUseSettings and ComputerUseMoreBrowsers components more thoroughly. I'm thinking through how the CollapsiblePanel's overflow-hidden and height animation interact with the child element's margin-top. Since the panel has overflow hidden, margin collapsing is prevented, which means the child's margin-top gets included in the scrollHeight measurement that Base UI uses to set the panel's height variable. The bigger issue is the aria-controls mismatch: Base UI's Collapsible generates a panelId and passes it to the Trigger for aria-controls, but when an explicit id is set on the Panel, it overrides that generated id. This means the Trigger's aria-controls attribute points to a panelId that no longer exists in the DOM, breaking the accessibility relationship even though the author manually set aria-controls on the trigger. The manual override doesn't actually break anything since both approaches end up working, so it's not a real regression—just redundant. Let me check the actual ComputerUseSettings component to see if there are any genuine issues there. I need to verify that the text-destructive token actually exists in the codebase since I'm seeing text-destructive-foreground used in button variants and other destructive-related classes like border-destructive and bg-destructive. Let me search through the repo to check if text-destructive is used elsewhere or if the convention is consistently text-destructive-foreground like what I see in the alert component. I'm checking whether text-destructive is actually a problem—it's not, it's widely used. Since I already have two unresolved findings on this PR, the check will fail anyway, so I need to decide if posting additional comments is worthwhile. I'm looking for maybe one or two more strong findings to flag, and the RowTitle duplication I already caught seems worth keeping. The Badge override was already flagged too. The plain string wrapping in RowTitle is minor and arguably covered by my second comment. But the Mirror-to-Codex switch missing disabled={!onDesktop} while its siblings have it—that's a concrete inconsistency worth calling out, even though the setting does persist to the server the same way the other switches do. Timeline buttons aren't disabled off-desktop either, but they'd never render there anyway since items only populate on desktop. The Input/Button height mismatch in the add-entry form is real though—Input defaults to 34/30px while Button size="sm" is 32/28px, so with flex and default alignment the button sits 2px short. Adding items-center to the flex container would fix that alignment issue cleanly. The 2px misalignment is borderline enough that I'd rather not include it. But there's a real keyboard accessibility issue in the PrivacyExclusionList "Add" flow — after committing, focus gets lost to the body instead of staying with the user, which breaks keyboard navigation. That's worth flagging since the check emphasizes preserving keyboard behavior. There's also a potential event propagation problem where pressing Escape inside the input doesn't stop the event from bubbling up to the dialog handler. The issue is that Base UI's escape handling happens at the document level via floating-ui's useDismiss, so when the input's onKeyDown handler calls setAdding(false) without stopping propagation, the native event continues bubbling up and triggers the dialog's close handler too. The fix is to call stopPropagation() and preventDefault() on the Escape key event. Actually, I need to verify that stopPropagation() on the React synthetic event actually prevents the document-level listener from firing — React attaches listeners at the root container, so calling stopPropagation() there stops the event from bubbling further up to document, which means the dialog's escape handler won't fire. That should work. Let me check the Dialog component to confirm Escape actually closes it. The dialog uses the standard Base UI component with Escape to close, and the DialogPopup has a Cancel button in the footer so that's handled. The PrivacyExclusionList form is nested inside ComputerHistoryPrivacyDialog without an outer form wrapper, which means Enter will submit the inner form as expected. I should check if there are other examples in the repo of this Escape-inside-dialog pattern with stopPropagation. I'm noticing the new code is missing the standard repo pattern for handling Escape in inline editing — it should be calling event.preventDefault() and event.stopPropagation() like the examples in SettingsSidebarNav, ModelPickerContent, and SettingsPanels show. Without these, pressing Escape will both cancel the draft and close the privacy dialog, losing both exclusion lists. I should also check routes/settings.ts for context. There's a second issue: the Mirror to Codex switch is missing the disabled={!onDesktop} attribute that keeps it consistent with other controls in that section. I'll flag both of these. Before I post though, I need to verify whether my previous findings are still visible on the PR — the force push may have made them outdated, but the comments themselves should persist. I'm checking that the two UI Consistency comments are still open and noting that the PR head has moved to a newer commit, though my context references an earlier one. I'll post my review against the earlier commit since the tool handles line mapping automatically on the latest diff. I need to check the current PR head at ad0a0ea and verify the ComputerHistorySettings.tsx file to get the correct line numbers for my review, since the scope is at an earlier commit. The file's line numbers shifted slightly, so I need to check the current state at that commit to make sure my comments reference the right sections. The RowTitle duplication has been resolved with the shared SettingRowTitle component, so I should verify whether the Badge issue was also addressed in that commit. Now I'm posting the findings against the current head. The first issue is in ComputerHistorySettings.tsx around line 97-100 where the Escape key handler doesn't call stopPropagation, which causes the entire privacy dialog to close instead of just the nested component. The second issue is on line 351 where the mirrorToCodex Switch is missing the disabled={!onDesktop} prop to prevent interaction when not on desktop. Writing the second suggestion...

1. modify_pr — UI consistency review of the changed web UI (apps/web/src/**). The two findings from the previous run are resolved on this head: RowTitle is now the shared SettingRowTitle in settingsLayout.tsx, and the exclusion chips no longer override Badge's geometry. Two new interaction-consistency issues in ComputerHistorySettings.tsx are noted inline.