From fb15d647c6a030bcbb8aa68c367995b2ad063de7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 12 Jul 2026 17:11:08 -0700 Subject: [PATCH] refactor: remove state-only React effects (#8437) --- .../pr-sidebar/PRCommentsSection.test.ts | 131 ++++++++++++++++++ .../pr-sidebar/PRCommentsSection.tsx | 16 ++- .../files/MobileFileMarkdownPreview.test.ts | 122 ++++++++++++++++ .../src/files/MobileFileMarkdownPreview.tsx | 12 +- .../sidebar/LinearAgentSkillSetupPrompt.tsx | 11 +- .../TabBarCreateEntry.keyboard.test.tsx | 20 +++ .../components/tab-bar/TabBarCreateEntry.tsx | 10 +- .../CloseTerminalDialog.test.tsx | 28 ++++ .../terminal-pane/CloseTerminalDialog.tsx | 10 +- .../terminal-pane/PinnedTabCloseDialog.tsx | 10 +- 10 files changed, 347 insertions(+), 23 deletions(-) create mode 100644 mobile/src/components/pr-sidebar/PRCommentsSection.test.ts create mode 100644 mobile/src/files/MobileFileMarkdownPreview.test.ts diff --git a/mobile/src/components/pr-sidebar/PRCommentsSection.test.ts b/mobile/src/components/pr-sidebar/PRCommentsSection.test.ts new file mode 100644 index 000000000..d174f455c --- /dev/null +++ b/mobile/src/components/pr-sidebar/PRCommentsSection.test.ts @@ -0,0 +1,131 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { GitHubWorkItemDetails, PRComment } from '../../../../src/shared/types' +import { PRCommentsSection } from './PRCommentsSection' + +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + Pressable: 'Pressable', + Text: 'Text', + View: 'View' +})) + +vi.mock('lucide-react-native', () => ({ + ChevronDown: 'ChevronDown', + ChevronRight: 'ChevronRight' +})) + +vi.mock('../../session/pr-comment-actions', () => ({ + canAddRootComment: () => false +})) + +vi.mock('../../session/mobile-pr-sidebar-state', () => ({ + isPrSidebarDetailsPlaceholder: () => false +})) + +vi.mock('./PRSection', () => ({ PRSection: 'PRSection' })) +vi.mock('./CommentMarkdown', () => ({ CommentMarkdown: 'CommentMarkdown' })) +vi.mock('./PRCommentCard', () => ({ PRCommentCard: 'PRCommentCard' })) +vi.mock('./PRCommentComposer', () => ({ PRCommentComposer: 'PRCommentComposer' })) +vi.mock('./pr-comments-styles', () => ({ prCommentsStyles: {} })) +vi.mock('./mobile-pr-sidebar-styles', () => ({ mobilePrSidebarStyles: {} })) +vi.mock('../../theme/mobile-theme', () => ({ colors: { textSecondary: '#999' } })) + +function suppressReactTestRendererDeprecationWarning(): () => void { + const originalConsoleError = console.error + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation((...args) => { + const firstArg = args[0] + if (typeof firstArg === 'string' && firstArg.includes('react-test-renderer is deprecated')) { + return + } + originalConsoleError(...args) + }) + return () => consoleErrorSpy.mockRestore() +} + +function comment(id: number): PRComment { + return { + id, + author: 'octocat', + authorAvatarUrl: '', + body: '', + createdAt: '', + url: '' + } +} + +function detailsWithComments(count: number): GitHubWorkItemDetails { + return { + item: { id: 'pr:1', type: 'pr' }, + body: '', + comments: Array.from({ length: count }, (_, index) => comment(index + 1)) + } as GitHubWorkItemDetails +} + +async function renderComments(details: GitHubWorkItemDetails): Promise { + let renderer: ReactTestRenderer | null = null + const restoreConsoleError = suppressReactTestRendererDeprecationWarning() + try { + await act(async () => { + renderer = create(createElement(PRCommentsSection, { details, prState: 'open' })) + }) + } finally { + restoreConsoleError() + } + if (!renderer) { + throw new Error('PRCommentsSection did not render') + } + return renderer +} + +function audienceTabs(renderer: ReactTestRenderer) { + return renderer.root + .findAllByType('Pressable') + .filter((node) => node.props.accessibilityState !== undefined) +} + +function showMoreButton(renderer: ReactTestRenderer) { + const button = renderer.root + .findAllByType('Pressable') + .find((node) => node.props.accessibilityState === undefined) + if (!button) { + throw new Error('Show more button not found') + } + return button +} + +async function press(node: { props: { onPress: () => void } }): Promise { + await act(async () => { + node.props.onPress() + }) +} + +describe('PRCommentsSection', () => { + let renderer: ReactTestRenderer | null = null + + afterEach(() => { + renderer?.unmount() + renderer = null + vi.restoreAllMocks() + }) + + it('resets pagination only when the user chooses a different audience filter', async () => { + renderer = await renderComments(detailsWithComments(25)) + expect(renderer.root.findAllByType('PRCommentCard')).toHaveLength(12) + + await press(showMoreButton(renderer)) + expect(renderer.root.findAllByType('PRCommentCard')).toHaveLength(24) + + // The second tab is Humans; moving from All to Humans resets the page limit. + await press(audienceTabs(renderer)[1]) + expect(renderer.root.findAllByType('PRCommentCard')).toHaveLength(12) + + await press(showMoreButton(renderer)) + expect(renderer.root.findAllByType('PRCommentCard')).toHaveLength(24) + + // Retapping the active tab used to leave the page size alone; retain that behavior. + await press(audienceTabs(renderer)[1]) + expect(renderer.root.findAllByType('PRCommentCard')).toHaveLength(24) + }) +}) diff --git a/mobile/src/components/pr-sidebar/PRCommentsSection.tsx b/mobile/src/components/pr-sidebar/PRCommentsSection.tsx index 6fe7bf731..bb4733076 100644 --- a/mobile/src/components/pr-sidebar/PRCommentsSection.tsx +++ b/mobile/src/components/pr-sidebar/PRCommentsSection.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState } from 'react' +import { useMemo, useState } from 'react' import { ActivityIndicator, Pressable, Text, View } from 'react-native' import { ChevronDown, ChevronRight } from 'lucide-react-native' import type { GitHubWorkItemDetails, PRState } from '../../../../src/shared/types' @@ -102,11 +102,17 @@ export function PRCommentsSection({ ) const groups = useMemo(() => groupPRComments(visible), [visible]) - // Bounded render window; reset to the first page whenever the filtered set changes. + // Bounded render window; reset to the first page when the user selects another filter. const [limit, setLimit] = useState(COMMENT_PAGE) - useEffect(() => { + const selectFilter = (nextFilter: PRCommentAudienceFilter): void => { + if (nextFilter === filter) { + return + } + // Why: paging belongs to the filter-tab event, so reset it in the same batch + // instead of briefly rendering the new filter with the previous page size. setLimit(COMMENT_PAGE) - }, [filter]) + setFilter(nextFilter) + } const shownGroups = groups.slice(0, limit) const remaining = groups.length - shownGroups.length @@ -154,7 +160,7 @@ export function PRCommentsSection({ setFilter(tab.value)} + onPress={() => selectFilter(tab.value)} accessibilityRole="button" accessibilityState={{ selected: active }} > diff --git a/mobile/src/files/MobileFileMarkdownPreview.test.ts b/mobile/src/files/MobileFileMarkdownPreview.test.ts new file mode 100644 index 000000000..2afdb0b2c --- /dev/null +++ b/mobile/src/files/MobileFileMarkdownPreview.test.ts @@ -0,0 +1,122 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { MobileFileMarkdownPreview } from './MobileFileMarkdownPreview' + +vi.mock('react-native', () => ({ + Pressable: 'Pressable', + ScrollView: 'ScrollView', + View: 'View' +})) + +vi.mock('lucide-react-native', () => ({ + Code: 'Code', + Pencil: 'Pencil' +})) + +vi.mock('../components/MobileMarkdown', () => ({ + MobileMarkdown: 'MobileMarkdown' +})) + +vi.mock('./MobileFilePreviewSourceText', () => ({ + MobileFilePreviewSourceText: 'MobileFilePreviewSourceText', + MobileFilePreviewTruncatedNote: 'MobileFilePreviewTruncatedNote' +})) + +vi.mock('../theme/mobile-theme', () => ({ + colors: { textPrimary: '#fff', textSecondary: '#999' } +})) + +vi.mock('./mobile-file-preview-styles', () => ({ + filePreviewStyles: {} +})) + +type PreviewProps = Parameters[0] + +function suppressReactTestRendererDeprecationWarning(): () => void { + const originalConsoleError = console.error + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation((...args) => { + const firstArg = args[0] + if (typeof firstArg === 'string' && firstArg.includes('react-test-renderer is deprecated')) { + return + } + originalConsoleError(...args) + }) + return () => consoleErrorSpy.mockRestore() +} + +async function renderPreview(props: PreviewProps): Promise { + let renderer: ReactTestRenderer | null = null + const restoreConsoleError = suppressReactTestRendererDeprecationWarning() + try { + await act(async () => { + renderer = create(createElement(MobileFileMarkdownPreview, props)) + }) + } finally { + restoreConsoleError() + } + if (!renderer) { + throw new Error('MobileFileMarkdownPreview did not render') + } + return renderer +} + +async function updatePreview(renderer: ReactTestRenderer, props: PreviewProps): Promise { + await act(async () => { + renderer.update(createElement(MobileFileMarkdownPreview, props)) + }) +} + +function modeToggle(renderer: ReactTestRenderer, label: string) { + const toggle = renderer.root + .findAllByType('Pressable') + .find((node) => node.props.accessibilityLabel === label) + if (!toggle) { + throw new Error(`Missing ${label} toggle`) + } + return toggle +} + +async function selectMode(renderer: ReactTestRenderer, label: string): Promise { + await act(async () => { + modeToggle(renderer, label).props.onPress() + }) +} + +function isSelected(renderer: ReactTestRenderer, label: string): boolean { + return modeToggle(renderer, label).props.accessibilityState.selected === true +} + +describe('MobileFileMarkdownPreview', () => { + let renderer: ReactTestRenderer | null = null + + afterEach(() => { + renderer?.unmount() + renderer = null + vi.restoreAllMocks() + }) + + it('resets the selected mode for a new file or line target without remounting the preview', async () => { + const baseProps: PreviewProps = { + relativePath: 'notes/first.md', + content: '# First', + truncated: false, + byteLength: 7 + } + renderer = await renderPreview(baseProps) + + expect(isSelected(renderer, 'View rendered Markdown preview')).toBe(true) + await selectMode(renderer, 'View Markdown source') + expect(isSelected(renderer, 'View Markdown source')).toBe(true) + + // Content updates alone preserve the user's explicitly selected mode. + await updatePreview(renderer, { ...baseProps, content: '# First updated' }) + expect(isSelected(renderer, 'View Markdown source')).toBe(true) + + await updatePreview(renderer, { ...baseProps, relativePath: 'notes/second.md' }) + expect(isSelected(renderer, 'View rendered Markdown preview')).toBe(true) + + await updatePreview(renderer, { ...baseProps, relativePath: 'notes/second.md', initialLine: 8 }) + expect(isSelected(renderer, 'View Markdown source')).toBe(true) + }) +}) diff --git a/mobile/src/files/MobileFileMarkdownPreview.tsx b/mobile/src/files/MobileFileMarkdownPreview.tsx index 0923cf1f9..9d53af3fe 100644 --- a/mobile/src/files/MobileFileMarkdownPreview.tsx +++ b/mobile/src/files/MobileFileMarkdownPreview.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react' +import { useState } from 'react' import { Pressable, ScrollView, View } from 'react-native' import { Code, Pencil } from 'lucide-react-native' import { MobileMarkdown } from '../components/MobileMarkdown' @@ -25,9 +25,15 @@ export function MobileFileMarkdownPreview({ initialLine }: Props) { const [mode, setMode] = useState<'preview' | 'source'>(() => (initialLine ? 'source' : 'preview')) - useEffect(() => { + const [previousRelativePath, setPreviousRelativePath] = useState(relativePath) + const [previousInitialLine, setPreviousInitialLine] = useState(initialLine) + // Why: opening a different file or line target must switch modes before paint, + // never briefly retain the prior file's manually selected mode. + if (relativePath !== previousRelativePath || initialLine !== previousInitialLine) { + setPreviousRelativePath(relativePath) + setPreviousInitialLine(initialLine) setMode(initialLine ? 'source' : 'preview') - }, [initialLine, relativePath]) + } const previewSelected = mode === 'preview' const sourceSelected = mode === 'source' diff --git a/src/renderer/src/components/sidebar/LinearAgentSkillSetupPrompt.tsx b/src/renderer/src/components/sidebar/LinearAgentSkillSetupPrompt.tsx index d9c01e182..e6157339c 100644 --- a/src/renderer/src/components/sidebar/LinearAgentSkillSetupPrompt.tsx +++ b/src/renderer/src/components/sidebar/LinearAgentSkillSetupPrompt.tsx @@ -105,6 +105,13 @@ export function LinearAgentSkillSetupPrompt({ const [localDismissed, setLocalDismissed] = useState(() => readLocalDismissed(localDismissStorageKey) ) + const [previousDismissStorageKey, setPreviousDismissStorageKey] = useState(localDismissStorageKey) + // Why: dismissal is scoped to the selected runtime, so read the new key before + // paint rather than briefly showing the previous runtime's prompt state. + if (localDismissStorageKey !== previousDismissStorageKey) { + setPreviousDismissStorageKey(localDismissStorageKey) + setLocalDismissed(readLocalDismissed(localDismissStorageKey)) + } const skill = useInstalledAgentSkillNames(LINEAR_AGENT_SKILL_NAMES, { enabled: linked, discoveryTarget: skillDiscoveryTarget, @@ -127,10 +134,6 @@ export function LinearAgentSkillSetupPrompt({ settings, agentRuntime ) - useEffect(() => { - setLocalDismissed(readLocalDismissed(localDismissStorageKey)) - }, [localDismissStorageKey]) - const writeCliStatusIfCurrent = useCallback( (requestIdentity: string, requestGeneration: number, write: () => void): void => { if ( diff --git a/src/renderer/src/components/tab-bar/TabBarCreateEntry.keyboard.test.tsx b/src/renderer/src/components/tab-bar/TabBarCreateEntry.keyboard.test.tsx index ffd81e96c..437e2c2b1 100644 --- a/src/renderer/src/components/tab-bar/TabBarCreateEntry.keyboard.test.tsx +++ b/src/renderer/src/components/tab-bar/TabBarCreateEntry.keyboard.test.tsx @@ -88,6 +88,26 @@ afterEach(() => { }) describe('TabBarCreateEntry keyboard navigation', () => { + it('publishes the query from the typing event without an extra effect commit', () => { + const onQueryChange = vi.fn() + mount( + + ) + + expect(onQueryChange).not.toHaveBeenCalled() + + setQuery('src/app.ts') + + expect(onQueryChange).toHaveBeenCalledTimes(1) + expect(onQueryChange).toHaveBeenCalledWith('src/app.ts') + }) + it('intercepts ArrowDown on a single-option list so it does not leak (guards >0 vs >1)', () => { entryOptionsMock.options = [fileOption('src/only-match.ts')] const onOpenEntry = vi.fn().mockResolvedValue(undefined) diff --git a/src/renderer/src/components/tab-bar/TabBarCreateEntry.tsx b/src/renderer/src/components/tab-bar/TabBarCreateEntry.tsx index 619e906df..dd2772f36 100644 --- a/src/renderer/src/components/tab-bar/TabBarCreateEntry.tsx +++ b/src/renderer/src/components/tab-bar/TabBarCreateEntry.tsx @@ -117,10 +117,6 @@ export default function TabBarCreateEntry({ [agentOptions, query] ) - useEffect(() => { - onQueryChange?.(query) - }, [onQueryChange, query]) - if (selectedIndexQuery !== query) { setSelectedIndexQuery(query) if (selectedIndex !== 0) { @@ -249,7 +245,11 @@ export default function TabBarCreateEntry({ ref={inputRef} value={query} onChange={(event) => { - setQuery(event.target.value) + const nextQuery = event.target.value + // Why: the parent query only changes in response to typing, so publish + // it in this event rather than a later effect after the render commits. + setQuery(nextQuery) + onQueryChange?.(nextQuery) setError(null) }} disabled={disabled} diff --git a/src/renderer/src/components/terminal-pane/CloseTerminalDialog.test.tsx b/src/renderer/src/components/terminal-pane/CloseTerminalDialog.test.tsx index bd18107ac..d2b8efd3e 100644 --- a/src/renderer/src/components/terminal-pane/CloseTerminalDialog.test.tsx +++ b/src/renderer/src/components/terminal-pane/CloseTerminalDialog.test.tsx @@ -88,4 +88,32 @@ describe('CloseTerminalDialog', () => { expect(onConfirm).toHaveBeenCalledWith(true) }) + + it('resets the skip preference when the dialog closes and reopens', async () => { + const onConfirm = vi.fn() + const onCancel = vi.fn() + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + mountedRoots.push(root) + const render = async (open: boolean): Promise => { + await act(async () => { + root.render() + }) + } + + await render(true) + const checkbox = document.body.querySelector('[role="checkbox"]') + await act(async () => { + checkbox?.click() + }) + expect(checkbox?.getAttribute('aria-checked')).toBe('true') + + await render(false) + await render(true) + + expect(document.body.querySelector('[role="checkbox"]')?.getAttribute('aria-checked')).toBe( + 'false' + ) + }) }) diff --git a/src/renderer/src/components/terminal-pane/CloseTerminalDialog.tsx b/src/renderer/src/components/terminal-pane/CloseTerminalDialog.tsx index a1f5c371e..771d5e15e 100644 --- a/src/renderer/src/components/terminal-pane/CloseTerminalDialog.tsx +++ b/src/renderer/src/components/terminal-pane/CloseTerminalDialog.tsx @@ -1,4 +1,4 @@ -import { useEffect, useId, useState } from 'react' +import { useId, useState } from 'react' import { Dialog, DialogContent, @@ -27,12 +27,16 @@ export default function CloseTerminalDialog({ }): React.JSX.Element { const checkboxId = useId() const [dontAskAgain, setDontAskAgain] = useState(false) + const [previousOpen, setPreviousOpen] = useState(open) - useEffect(() => { + // Why: each reopen represents a fresh confirmation, so clear the old choice + // during render rather than briefly painting it while the dialog opens. + if (open !== previousOpen) { + setPreviousOpen(open) if (open) { setDontAskAgain(false) } - }, [open]) + } const isAgent = copyKind === 'agent' diff --git a/src/renderer/src/components/terminal-pane/PinnedTabCloseDialog.tsx b/src/renderer/src/components/terminal-pane/PinnedTabCloseDialog.tsx index d8b9ea646..9416ba0d3 100644 --- a/src/renderer/src/components/terminal-pane/PinnedTabCloseDialog.tsx +++ b/src/renderer/src/components/terminal-pane/PinnedTabCloseDialog.tsx @@ -1,4 +1,4 @@ -import { useEffect, useId, useState } from 'react' +import { useId, useState } from 'react' import { Dialog, DialogContent, @@ -23,14 +23,18 @@ export default function PinnedTabCloseDialog(): React.JSX.Element { const dismissPinnedTabClose = useAppStore((state) => state.dismissPinnedTabClose) const updateSettings = useAppStore((state) => state.updateSettings) const [dontAskAgain, setDontAskAgain] = useState(false) + const [previousRequest, setPreviousRequest] = useState(request) const tabLabel = request?.tabLabel.trim() - useEffect(() => { + // Why: a new store request is a new confirmation, so reset its checkbox before + // paint while keeping a cancelled request's state inert until the next open. + if (request !== previousRequest) { + setPreviousRequest(request) if (request !== null) { setDontAskAgain(false) } - }, [request]) + } const handleConfirm = (): void => { if (dontAskAgain) {