refactor: improve state management and accessibility in components#123
refactor: improve state management and accessibility in components#123
Conversation
- Updated `LoadingIndicator` to use `useReducer` for better state handling. - Refactored `OnboardingGuide` to utilize `useReducer` for image loading state. - Enhanced accessibility by adding `role` and `tabIndex` attributes to various interactive elements across components. - Simplified state updates in `ChatHistoryModal` and `LoginWithOverleaf` by consolidating state management. - Improved performance in `CodeBlock` by using `useMemo` for highlighted code. - General code cleanup and consistency improvements across multiple components.
There was a problem hiding this comment.
Pull request overview
This pull request refactors state management and enhances accessibility across multiple React components in the webapp. The changes focus on improving code quality through better state handling patterns (useReducer), performance optimizations (useMemo, Promise.all), and comprehensive accessibility improvements (role attributes, keyboard navigation).
Changes:
- Converted
LoadingIndicatorandOnboardingGuideto useuseReducerfor more predictable state transitions - Added accessibility attributes (
role="button",tabIndex,onKeyDown) to 20+ interactive elements across the codebase - Consolidated state updates in
ChatHistoryModaland optimizedSelectionStorewith batched updates - Improved performance with
useMemoinCodeBlockandPromise.allinLoginWithOverleaf - Enhanced code consistency by simplifying imports, optimizing useState initializers, and refactoring helper functions to components
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 29 comments.
Show a summary per file
| File | Description |
|---|---|
| webapp/_webapp/src/views/settings/setting-text-input.tsx | Removed autoFocus attributes from input fields |
| webapp/_webapp/src/views/settings/sections/user-developer-tools.tsx | Optimized useState with function initializer |
| webapp/_webapp/src/views/settings/sections/ui-settings.tsx | Moved line wrap logic to onChange handler with DOM manipulation |
| webapp/_webapp/src/views/settings/sections/footer.tsx | Added keyboard navigation (role, tabIndex, onKeyDown) for version click feature |
| webapp/_webapp/src/views/prompts/user-instructions.tsx | Simplified import path by removing "/index" |
| webapp/_webapp/src/views/prompts/project-instructions.tsx | Simplified import path by removing "/index" |
| webapp/_webapp/src/views/office/app.tsx | Refactored displayMode sync to render-time logic and consolidated initialization |
| webapp/_webapp/src/views/login/login-with-overleaf.tsx | Added keyboard navigation and optimized Promise execution with Promise.all |
| webapp/_webapp/src/views/login/index.tsx | Added keyboard navigation for Advanced Options button |
| webapp/_webapp/src/views/login/advanced-settings.tsx | Optimized useState with function initializer |
| webapp/_webapp/src/views/embed-sidebar.tsx | Added accessibility attributes to resize handle |
| webapp/_webapp/src/views/devtools/index.tsx | Connected label to input with htmlFor and id attributes |
| webapp/_webapp/src/views/chat/header/chat-history-modal.tsx | Consolidated edit state management and moved refetch to modal open handler |
| webapp/_webapp/src/views/chat/header/chat-button.tsx | Added keyboard navigation support |
| webapp/_webapp/src/views/chat/footer/toolbar/selection.tsx | Refactored to useReducer for coordinated state updates |
| webapp/_webapp/src/views/chat/body/index.tsx | Added keyboard navigation for debug reload button |
| webapp/_webapp/src/stores/selection-store.ts | Added setLastSelection method to batch related state updates |
| webapp/_webapp/src/main.tsx | Refactored to use batched setLastSelection and removed duplicate line wrap logic |
| webapp/_webapp/src/components/text-patches.tsx | Improved diff key generation using character offset |
| webapp/_webapp/src/components/tabs.tsx | Added accessibility attributes to resize handle |
| webapp/_webapp/src/components/onboarding-guide.tsx | Converted to useReducer for image loading state and fixed key generation |
| webapp/_webapp/src/components/message-entry-container/tools/xtramcp/*.tsx | Added keyboard navigation to all collapsible tool cards |
| webapp/_webapp/src/components/message-entry-container/tools/tools.tsx | Simplified import path |
| webapp/_webapp/src/components/message-entry-container/tools/paper-score-comment/filter-controls.tsx | Added keyboard navigation |
| webapp/_webapp/src/components/message-entry-container/tools/general.tsx | Added keyboard navigation |
| webapp/_webapp/src/components/message-entry-container/assistant.tsx | Added keyboard handler and changed key prop (problematic) |
| webapp/_webapp/src/components/loading-indicator.tsx | Refactored to useReducer for cleaner state transitions |
| webapp/_webapp/src/components/code-block.tsx | Optimized with useMemo to prevent unnecessary re-highlighting |
Comments suppressed due to low confidence (2)
webapp/_webapp/src/views/chat/footer/toolbar/selection.tsx:57
- The wrapper functions
setSelectedIdxandsetIsKeyboardNavigationare created during every render but are included in useEffect dependencies. This will cause the useEffect to re-run on every render. Consider usinguseCallbackto memoize these functions or includingdispatchSelectiondirectly in the dependency array and dispatching actions inline.
const setSelectedIdx = (idx: number) => dispatchSelection({ type: "SET_IDX", idx });
const setIsKeyboardNavigation = (value: boolean) => dispatchSelection({ type: "SET_KEYBOARD_NAV", value });
const itemCount = items?.length ?? 0;
useEffect(() => {
if (initialValue !== undefined) {
const idx = items.findIndex((item) => item.value === initialValue);
if (idx !== -1) {
setSelectedIdx(idx);
return;
}
}
setSelectedIdx(0);
}, [itemCount, initialValue, items]);
webapp/_webapp/src/components/message-entry-container/tools/xtramcp/review-paper.tsx:43
- Using
section(the string content) as a React key can cause issues if the same section name appears multiple times in the array, violating React's requirement for unique keys. Consider using the index or a combination of index and section name to ensure unique keys.
{sections.map((section, index) => (
<span key={section}>
<code className="text-xs px-1.5 py-0.5 rounded text-gray-700 dark:text-default-200 bg-gray-100 dark:!bg-default-200">
{section}
</code>
{index < sections.length - 1 && ", "}
</span>
))}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| role="button" | ||
| tabIndex={0} | ||
| onClick={() => setIsMetadataCollapsed(!isMetadataCollapsed)} | ||
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { setIsMetadataCollapsed(!isMetadataCollapsed); } }} |
There was a problem hiding this comment.
The Space key handler should call e.preventDefault() to prevent the default scrolling behavior. When a user presses Space on a focusable element, the browser will scroll the page by default, which is likely not the intended behavior here.
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { setIsMetadataCollapsed(!isMetadataCollapsed); } }} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| setIsMetadataCollapsed(!isMetadataCollapsed); | |
| } | |
| }} |
| @@ -36,6 +38,25 @@ export const SettingsFooter = () => { | |||
| }, 1500); | |||
| setVersionClickTimeout(timeout); | |||
| }} | |||
| onKeyDown={(e) => { | |||
| if (e.key === 'Enter' || e.key === ' ') { | |||
| setVersionClickCount((prev: number) => { | |||
| const next = prev + 1; | |||
| if (next >= 5) { | |||
| setEnableUserDeveloperTools(!enableUserDeveloperTools); | |||
| return 0; | |||
| } | |||
| return next; | |||
| }); | |||
| if (versionClickTimeout) { | |||
| clearTimeout(versionClickTimeout); | |||
| } | |||
| const timeout = setTimeout(() => { | |||
| setVersionClickCount(0); | |||
| }, 1500); | |||
| setVersionClickTimeout(timeout); | |||
| } | |||
| }} | |||
There was a problem hiding this comment.
There is significant code duplication between the onClick and onKeyDown handlers. Consider extracting the logic into a shared function to improve maintainability and reduce the risk of inconsistencies if the logic needs to be updated in the future.
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| setVersionClickCount((prev: number) => { | ||
| const next = prev + 1; | ||
| if (next >= 5) { | ||
| setEnableUserDeveloperTools(!enableUserDeveloperTools); | ||
| return 0; | ||
| } | ||
| return next; | ||
| }); | ||
| if (versionClickTimeout) { | ||
| clearTimeout(versionClickTimeout); | ||
| } | ||
| const timeout = setTimeout(() => { | ||
| setVersionClickCount(0); | ||
| }, 1500); | ||
| setVersionClickTimeout(timeout); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The Space key handler should call e.preventDefault() to prevent the default scrolling behavior. When a user presses Space on a focusable element, the browser will scroll the page by default, which is likely not the intended behavior here.
| @@ -131,6 +133,25 @@ export const ChatBody = ({ conversation }: ChatBodyProps) => { | |||
| }, 3000); | |||
| } | |||
| }} | |||
| onKeyDown={async (e) => { | |||
| if (e.key === 'Enter' || e.key === ' ') { | |||
| try { | |||
| const response = await getConversation({ conversationId: conversation?.id ?? "" }); | |||
| if (!response.conversation) { | |||
| throw new Error(`Failed to load conversation ${conversation?.id ?? "unknown"}`); | |||
| } | |||
| setCurrentConversation(response.conversation); | |||
| useStreamingStateMachine.getState().reset(); | |||
| setReloadSuccess(ReloadStatus.Success); | |||
| } catch { | |||
| setReloadSuccess(ReloadStatus.Failed); | |||
| } finally { | |||
| setTimeout(() => { | |||
| setReloadSuccess(ReloadStatus.Default); | |||
| }, 3000); | |||
| } | |||
| } | |||
| }} | |||
There was a problem hiding this comment.
There is significant code duplication between the onClick and onKeyDown handlers. Consider extracting the logic into a shared function to improve maintainability and reduce the risk of inconsistencies if the logic needs to be updated in the future.
| onKeyDown={async (e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| try { | ||
| const response = await getConversation({ conversationId: conversation?.id ?? "" }); | ||
| if (!response.conversation) { | ||
| throw new Error(`Failed to load conversation ${conversation?.id ?? "unknown"}`); | ||
| } | ||
| setCurrentConversation(response.conversation); | ||
| useStreamingStateMachine.getState().reset(); | ||
| setReloadSuccess(ReloadStatus.Success); | ||
| } catch { | ||
| setReloadSuccess(ReloadStatus.Failed); | ||
| } finally { | ||
| setTimeout(() => { | ||
| setReloadSuccess(ReloadStatus.Default); | ||
| }, 3000); | ||
| } | ||
| } | ||
| }} |
There was a problem hiding this comment.
The Space key handler should call e.preventDefault() to prevent the default scrolling behavior. When a user presses Space on a focusable element, the browser will scroll the page by default, which is likely not the intended behavior here.
| role="button" | ||
| tabIndex={0} | ||
| onClick={() => setIsMetadataCollapsed(!isMetadataCollapsed)} | ||
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { setIsMetadataCollapsed(!isMetadataCollapsed); } }} |
There was a problem hiding this comment.
The Space key handler should call e.preventDefault() to prevent the default scrolling behavior. When a user presses Space on a focusable element, the browser will scroll the page by default, which is likely not the intended behavior here.
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { setIsMetadataCollapsed(!isMetadataCollapsed); } }} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| if (e.key === ' ') { | |
| e.preventDefault(); | |
| } | |
| setIsMetadataCollapsed(!isMetadataCollapsed); | |
| } | |
| }} |
| {parsedMessage.paperDebuggerContent.map((content) => ( | ||
| <TextPatches key={content} attachment={prevAttachment}> |
There was a problem hiding this comment.
Using the content string itself as a React key is problematic because duplicate content will have the same key, violating React's uniqueness requirement for keys. This can cause rendering issues if the array contains duplicate strings. Consider using the index or a combination of index and content hash to ensure unique keys.
| {parsedMessage.paperDebuggerContent.map((content) => ( | |
| <TextPatches key={content} attachment={prevAttachment}> | |
| {parsedMessage.paperDebuggerContent.map((content, index) => ( | |
| <TextPatches key={`${index}-${content.slice(0, 20)}`} attachment={prevAttachment}> |
| role="button" | ||
| tabIndex={0} | ||
| onClick={onImageClick} | ||
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { onImageClick(); } }} |
There was a problem hiding this comment.
The Space key handler should call e.preventDefault() to prevent the default scrolling behavior. When a user presses Space on a focusable element, the browser will scroll the page by default, which is likely not the intended behavior here.
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { onImageClick(); } }} | |
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { if (e.key === ' ') { e.preventDefault(); } onImageClick(); } }} |
| role="button" | ||
| tabIndex={0} | ||
| onClick={() => setIsMetadataCollapsed(!isMetadataCollapsed)} | ||
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { setIsMetadataCollapsed(!isMetadataCollapsed); } }} |
There was a problem hiding this comment.
The Space key handler should call e.preventDefault() to prevent the default scrolling behavior. When a user presses Space on a focusable element, the browser will scroll the page by default, which is likely not the intended behavior here.
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { setIsMetadataCollapsed(!isMetadataCollapsed); } }} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| setIsMetadataCollapsed(!isMetadataCollapsed); | |
| } | |
| }} |
| role="button" | ||
| tabIndex={0} | ||
| onClick={() => setIsMetadataCollapsed(!isMetadataCollapsed)} | ||
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { setIsMetadataCollapsed(!isMetadataCollapsed); } }} |
There was a problem hiding this comment.
The Space key handler should call e.preventDefault() to prevent the default scrolling behavior. When a user presses Space on a focusable element, the browser will scroll the page by default, which is likely not the intended behavior here.
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { setIsMetadataCollapsed(!isMetadataCollapsed); } }} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter') { | |
| setIsMetadataCollapsed(!isMetadataCollapsed); | |
| } else if (e.key === ' ') { | |
| e.preventDefault(); | |
| setIsMetadataCollapsed(!isMetadataCollapsed); | |
| } | |
| }} |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| editor.classList.add("cm-lineWrapping"); | ||
| }); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Line wrap disable setting no longer applied on startup
High Severity
The disableLineWrap feature no longer works on app startup. The old useEffect in main.tsx that watched disableLineWrap and applied DOM changes (removing/adding cm-lineWrapping class) on mount and on value change was removed. The new code in ui-settings.tsx only calls onElementAppeared inside the onSelectChange handler, which fires only when the user actively toggles the setting. Users who previously enabled "Disable line wrap" will see the toggle shown as enabled in settings, but the DOM manipulation won't actually take effect until they toggle it off and on again.
Additional Locations (1)
| }, 3000); | ||
| } | ||
| } | ||
| }} |
There was a problem hiding this comment.
Duplicated async handler logic in onKeyDown and onClick
Low Severity
The onKeyDown handler duplicates the entire async onClick handler logic (API call, error handling, state updates, timeout). The complex try/catch/finally block with getConversation, setCurrentConversation, streaming state reset, and reload status management is copy-pasted. Extracting the shared logic into a single function and calling it from both handlers would avoid maintenance risk where a future fix to one handler is missed in the other.
| setVersionClickCount(0); | ||
| }, 1500); | ||
| setVersionClickTimeout(timeout); | ||
| } |
There was a problem hiding this comment.
Duplicated click handler logic in onKeyDown callback
Low Severity
The onKeyDown handler is an exact copy of the onClick handler logic for the version click counter (state increment, developer tools toggle at 5 clicks, timeout reset). This duplication risks divergence if the behavior is later changed in only one handler. The shared logic could be extracted into a single function called by both handlers.
| {parsedMessage.paperDebuggerContent.map((content, index) => ( | ||
| <TextPatches key={index} attachment={prevAttachment}> | ||
| {parsedMessage.paperDebuggerContent.map((content) => ( | ||
| <TextPatches key={content} attachment={prevAttachment}> |
There was a problem hiding this comment.
Content string as React key risks duplicate keys
Medium Severity
Using content as a React key in the paperDebuggerContent.map() call can produce duplicate keys if two <PaperDebugger> blocks in the same message have identical content. The previous index-based key was imperfect but guaranteed uniqueness. Duplicate keys cause React to skip rendering of the second element and produce incorrect DOM updates.
| googleAnalytics.fireEvent(user?.id, `select_${normalizeName(item.title)}`, {}); | ||
| onSelect?.(item); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Enter key triggers onSelect twice in Selection component
Medium Severity
The newly added item-level onKeyDown handler (which calls onSelect) conflicts with the existing global handleKeyDown listener on window (which also calls onSelect on Enter). When a user tabs to a focusable item and presses Enter, both handlers fire — the React synthetic event on the item fires first, then the native event reaches window. This causes onSelect to be called twice, potentially selecting the item twice or selecting two different items if selectedIdx doesn't match the focused element.
LoadingIndicatorto useuseReducerfor better state handling.OnboardingGuideto utilizeuseReducerfor image loading state.roleandtabIndexattributes to various interactive elements across components.ChatHistoryModalandLoginWithOverleafby consolidating state management.CodeBlockby usinguseMemofor highlighted code.Note
Medium Risk
Mostly UI refactors and a11y improvements, but it touches global keyboard/selection handling and moves some store updates to render-time, which could cause subtle interaction regressions.
Overview
Refactors component state management by replacing several
useState/useEffectflows withuseReducer/useMemo(e.g.,LoadingIndicator, onboarding image loading, promptSelection), and consolidating selection updates viauseSelectionStore.setLastSelection.Improves keyboard accessibility across many clickable non-button elements by adding
role,tabIndex, andEnter/Spacehandlers (tool card headers, copy actions, filters, login/advanced options toggles, resize handles, etc.).Behavior tweaks/cleanup: conversation history now refetches on modal open, Overleaf login loads settings/prompts in parallel, line-wrap toggling is triggered from
UISettingsinstead ofMain, list rendering uses more stable React keys, andsetting-text-inputno longer auto-focuses inputs.Written by Cursor Bugbot for commit a2cfa5d. This will update automatically on new commits. Configure here.