Repository navigation
UI Consistency: 1 issue found
apps/web/src/components/settings/ThemeSearchSection.tsx
The new onKeyDown Enter handler on the search input (lines 282-284) has no in-flight guard, unlike the submit button it replaces (disabled={... || isSearching || installingId !== null}). Because runSearch aborts the previous request, repeated/held Enter presses cancel and restart the pending search, and typing + Enter inside the 350 ms debounce window issues two Open VSX requests for the same key (lastSearchKeyRef is recorded only on success, so the debounced effect does not dedupe an Enter-started request). Suggested fix posted inline: gate on isSearching and add the repo's existing IME composition guard.
No other findings this round.
Details
Note
Your check run agent prompt is: .macroscope/check-run-agents/ui-consistency.md
More information about how Check Run Agents work can be found in our Help Center.
Scope reviewed
Changes since the last reviewed commit: ThemeSearchSection.tsx only β the immediate clear/abort effect for an emptied query (lines 163-171) and the Enter-to-search handler on InputGroupInput (lines 282-284).
Checks performed
- Confirmed
InputGroupInputforwardsonKeyDownto the sharedInputprimitive (no prop shadowing), so the handler composes correctly with the primitive. - Compared against other search surfaces using
InputGroup/Inputwithtype="search"(pullRequest/PullRequestListFilters.tsx,files/FileBrowserPanel.tsx,settings/KeybindingsSettings.tsx,settings/SettingsSidebarNav.tsx,Sidebar.tsx): the addon spinner/search-icon swap and keyboard handling pattern match the house style; the missing composition guard does not (Sidebar.tsx:1008,Sidebar.tsx:2344,chat/ChatHeader.tsx:216). - Verified there is no
<form>ancestor inThemeImportDialog, so no default submit behavior is being suppressed or relied on. - Traced the interaction between the Enter path,
runSearch's abort-then-refetch, and the debounced effect'slastSearchKeyRef/prevSearchKeyRefdedupe to confirm the duplicate-request and restart behavior. - Re-checked the immediate clear effect for query emptied mid-flight: it aborts, clears results/error, and stops the spinner, and it defers while
installingId !== null, matching the existing!debouncedQuerybranch. No new finding.
Not re-reported
The hand-rolled action row in ThemeImportDialog.tsx (line 539) was flagged in an earlier run; the author added an explanatory comment documenting the section-scoped intent, so it is treated as an intentional exception rather than re-flagged.
π Agent activity β 1 tool call
π I want to check what's changed in the PR since the last run, comparing commit ccfed94 to 2b8bb6d by examining the diff between them. The latest commit seems to address the Enter-to-retry issue along with aborting immediately when cleared. I want to diff that commit against head to confirm. I'm adding an effect to clear results when the query is empty, plus making Enter trigger the search on the input field. Let me review the full current file to check everything fits together. I'm checking whether adding an Enter-to-search handler on the input works correctly without a form wrapper, and whether it could misfire while a search is already in progress. Looking at the new effect that clears results when the query becomes empty β it correctly aborts in-flight searches and resets state, though it does re-run harmlessly on mount and dialog open since React bails out on identical state updates. But I'm spotting a real bug: the effect doesn't check
open, and more importantly, if a user clears the query while an install is running, the effect skips clearing becauseinstallingIdis set. The problem is once the install finishes andinstallingIdresets to null, the effect fires and wipes out the results and the error state β including any install failure message that was being carefully preserved elsewhere in the design. Actually, checking the pre-existing debounced-query logic, this same clearing of error/results on empty query already happened before this change, so it's not a new regression β just consistent with prior behavior. I should shift focus to UI consistency instead: whetheronKeyDownpasses throughInputGroupInputproperly, whether there's an established pattern elsewhere for Enter-key handling in search inputs, and how the nativetype="search"clear button interacts with this. Good, onKeyDown is forwarded properly. I want to check if there's a shared pattern elsewhere in the repo for search inputs handling Enter, like the extension marketplace search or command palette, so I'll grep for search input types and Enter key handling to see if there's a consistent approach to follow. I'm checking that this search input's keyboard behavior stays consistent with other inputs, and since it's inside a dialog without a form, pressing Enter won't accidentally trigger the "Add theme" button. Tracing through the Enter-to-retry path: an empty query short-circuits, but a retry after an error clears the error state and re-issues the search correctly. I'm verifying the debounce effect's dependencies don't change from this manual trigger, so it won't re-run and interfere with the in-flight request β the search key guard should hold steady. Now I'm looking at the clearing effect that fires when installingId resets to null with an empty query, which seems fine on its own. There's some overlap with the debounced-clear branch elsewhere, but that duplication looks intentional β one path clears immediately, the other waits for debounce β matching the stated goal of aborting right away when the query is cleared. I'm weighing whether this counts as a real consistency issue or just acceptable redundancy, then shifting to check how native search inputs behave on Enter, since Chrome fires its own "search" event that could interact with this handler. I also notice the Enter handler never calls preventDefault, so I want to check whether the search input sits inside a form element anywhere in the dialog hierarchy β there was a form in ThemeSearchSection before it got removed, so I should look at ThemeImportDialog to confirm there's no surrounding form that could trigger a submit. With no form ancestor, Enter has no default action to prevent. On the footer row issue, the author already addressed my earlier comment about the hand-rolled footer instead of DialogFooter, so I shouldn't re-flag that β but I still want to check whether the actions remain inside the ScrollArea, causing the primary action to scroll away in the mobile sheet. Since the underlying scroll/pinning issue persists despite the author's comment, repeatedly flagging it would just be noise, so I'll let it go. Instead I'm focusing on what's new: the Enter key handler and the immediate-clear effect, checking them against the debounced-search flow for race conditions β specifically whether pressing Enter mid-typing could fire a search that conflicts with the debounce effect settling on the same key before the Enter search completes. That's a minor duplicate-request concern, but the bigger issue is that the Enter handler lacks theisSearchingguard the old submit button had. Since keydown auto-repeats while a key is held, mashing or holding Enter would fire repeated abort/refire cycles, spamming the Open VSX API and preventing the search from ever completing β a real regression compared to the disabled-button behavior it replaced, and worth flagging. I should also consider guarding againstevent.repeatand IME composition (isComposing), since Enter during composition shouldn't trigger a search on partial input. Let me check how other search inputs in the codebase, like SettingsSidebarNav'shandleSearchKeyDown, handle these cases to see if there's an existing convention to follow. I notice the codebase guards Enter handlers with isComposing/keyCode 229 checks in text-entry surfaces like Sidebar and ChatHeader, likely for rename inputs where Enter commits. I should follow this same convention in the new Enter handler to properly ignore IME composition events. The IME concern itself is minor β an extra premature search request isn't really harmful since debounce would fire anyway. The stronger issue is key-repeat: holding Enter down would continuously abort and refire the search, so results never settle while held. Since the previous submit button was disabled during isSearching, this is a genuine behavior regression from replacing it with Enter-to-search, making it a legitimate finding worth flagging as a new comment on these changed lines. There's also a race with the debounce timer itself: typing a query and pressing Enter before the 350ms debounce settles triggers a search via Enter, then the debounce effect fires afterward since lastSearchKeyRef hasn't updated yet, causing a second redundant request that aborts the first. This is actually more likely to happen than key-mashing, since pressing Enter right after typing is the natural way users confirm a search. I consider guarding Enter with a check that no search is currently in flight, but that only fixes the Enter-side duplicate β the debounce effect can still fire its own duplicate afterward since it isn't guarded by the same flag. A truly clean fix would need the debounce effect itself to skip when a request for that key is already in flight, rather than patching only the Enter handler. I verify the isSearching guard doesn't break retrying a failed search, since isSearching resets to false on failure, and check that event.repeat combined with isSearching prevents held-Enter from spamming requests. Given this is a real, reproducible issue, I'll leave one comment on the relevant lines recommending the in-flight-key tracking approach so both paths dedupe properly. I also want to add a composition guard to the Enter handler, since the repo has an existing convention for that on text inputs, to avoid triggering a search mid-IME-composition. Writing the suggested keydown handler... Checking the new effect's behavior when a user clears the search field entirely β results, errors, and spinner all reset correctly, and whitespace-only input is treated the same as empty since it's trimmed. This matches the intended behavior, so I'll leave just the one comment about the state handling when findings exist versus when everything is clear. Since the finding is moderate β duplicate or aborted requests triggered on Enter β I'll mark this as a failure per the guidance on consistency risks. Now I need to pin down the exact diff line for the review comment: the onKeyDown additions land around line 283 in the new file, so I'll anchor the comment with start_line 282 through line 284 on the RIGHT side, which should let the suggestion block properly replace those lines.
1. modify_pr β One finding on the new Enter-to-search handler in ThemeSearchSection. Everything else in this round (immediate clear/abort when the query is emptied) looks consistent with the existing search surfaces.
Agent Credits: 155 credits