refactor: remove state-only React effects (#8437)

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<ReactTestRenderer> {
+  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<void> {
+  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)
+  })
+})

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({
                         <Pressable
                           key={tab.value}
                           style={[styles.audienceTab, active && styles.audienceTabActive]}
-                          onPress={() => setFilter(tab.value)}
+                          onPress={() => selectFilter(tab.value)}
                           accessibilityRole="button"
                           accessibilityState={{ selected: active }}
                         >

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<typeof MobileFileMarkdownPreview>[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<ReactTestRenderer> {
+  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<void> {
+  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<void> {
+  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)
+  })
+})

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'
 

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 (

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(
+      <TabBarCreateEntry
+        worktreeId="wt"
+        groupId="g"
+        menuOpen
+        onOpenEntry={vi.fn()}
+        onQueryChange={onQueryChange}
+      />
+    )
+
+    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)

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}

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<void> => {
+      await act(async () => {
+        root.render(<CloseTerminalDialog open={open} onCancel={onCancel} onConfirm={onConfirm} />)
+      })
+    }
+
+    await render(true)
+    const checkbox = document.body.querySelector<HTMLButtonElement>('[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'
+    )
+  })
 })

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'
 

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) {