cleanup(renderer): drop dead code, dedupe helpers, skip wasted work, fix garbled comments (#7322)

Quality pass on renderer/shared PRs merged 2026-07-03: - WorktreeTitleInlineRename: skip the truncation measure + ResizeObserver in `wrapTitle` mode, where wrapped titles never truncate — it could only churn unused state (#7307). - editor slice: reuse the `removeEditorStateForReplacedPreview` helper this PR added instead of a hand-rolled copy of the same six-field eviction (drops ~50 lines) (#6476). - useFileExplorerTree: extract `readWorktreeDirectory` so the connectionId/settings assembly for `readRuntimeDirectory` lives in one place, not three (#6321). - comment-markdown-github-attachment-media: extract a shared `AttachmentFallbackLink` for the image/video error-fallback link (#6759). - repository-icon-github: fold the two near-identical live resolvers into one parameterized `resolveRepositoryIdentityLive`; trim a 3-line comment to 2 (#6507). - resource-usage-open-slices: delete the `shouldReadPopoverSlices` identity wrapper and inline `open` at the four call sites (#7275). - BrowserPane: drop the pointerEvents assignment already applied inside `ensureBrowserPageWebview` for the reused-webview path (#6958). - github slice: fix two garbled "…a commit main confirmed…" comments (#7277). - runtime-file-client: trim the binary-file fallback comment to its whys (#6606). - composer-branch-selection: drop the inline comment that restated the JSDoc (#6748). - TabBarQuickCommandsButton: correct the stale "+ Command" comment (button shows no +). No behavior change (the editor-helper reuse is behavior-equivalent, only more conservative on an edge case); typecheck, oxlint, react-doctor, oxfmt, and touched unit suites all pass.
2026-09-22 08:02:28 +00:00 · 2026-07-03 22:31:56 -07:00
parent 529c06ff62
commit 3fdeec7c2f
11 changed files with 97 additions and 141 deletions
@@ -3626,7 +3626,8 @@ function BrowserPagePane({
    let needsInitialDefaultZoom = ensuredWebview.created

    if (!ensuredWebview.created) {
-      webview.style.pointerEvents = inputLockedRef.current ? 'none' : 'auto'
+      // pointerEvents is already applied inside ensureBrowserPageWebview for the
+      // reused-webview path, so it isn't repeated here.
      syncNavigationState(webview)
      // Why: seed the ref with the store URL so the URL sync effect does not
      // force-navigate an already-mounted webview that is on the right page.
@@ -64,6 +64,23 @@ function entriesToTreeNodes(
  })
 }

+async function readWorktreeDirectory(
+  activeWorktreeId: string | null | undefined,
+  worktreePath: string | null,
+  dirPath: string
+): Promise<DirEntry[]> {
+  const connectionId = getConnectionId(activeWorktreeId ?? null) ?? undefined
+  return readRuntimeDirectory(
+    {
+      settings: getRightSidebarWorktreeRuntimeSettings(activeWorktreeId),
+      worktreeId: activeWorktreeId,
+      worktreePath,
+      connectionId
+    },
+    dirPath
+  )
+}
+
 export async function refreshFileExplorerExpandedDirs({
  dirs,
  worktreePath,
@@ -180,16 +197,7 @@ export function useFileExplorerTree(
        }
      }))
      try {
-        const connectionId = getConnectionId(activeWorktreeId ?? null) ?? undefined
-        const entries = await readRuntimeDirectory(
-          {
-            settings: getRightSidebarWorktreeRuntimeSettings(activeWorktreeId),
-            worktreeId: activeWorktreeId,
-            worktreePath,
-            connectionId
-          },
-          dirPath
-        )
+        const entries = await readWorktreeDirectory(activeWorktreeId, worktreePath, dirPath)
        if (!dirLoadTrackerRef.current.isCurrent(loadToken)) {
          return false
        }
@@ -278,18 +286,7 @@ export function useFileExplorerTree(
      worktreePath,
      dirLoadTracker: dirLoadTrackerRef.current,
      setDirCache,
-      readDirectory: async (dirPath) => {
-        const connectionId = getConnectionId(activeWorktreeId ?? null) ?? undefined
-        return readRuntimeDirectory(
-          {
-            settings: getRightSidebarWorktreeRuntimeSettings(activeWorktreeId),
-            worktreeId: activeWorktreeId,
-            worktreePath,
-            connectionId
-          },
-          dirPath
-        )
-      }
+      readDirectory: (dirPath) => readWorktreeDirectory(activeWorktreeId, worktreePath, dirPath)
    })
  }, [activeWorktreeId, expanded, loadDir, worktreePath])

@@ -12,32 +12,41 @@ export type RepositoryGitHubAvatarResolution = {
  upstream: GitHubRepositoryIdentity | null
 }

-export async function resolveRepositoryUpstreamLive(
+function resolveRepositoryIdentityLive(
  runtimeTarget: RuntimeTarget,
-  repo: Repo
+  repo: Repo,
+  method: 'github.repoUpstream' | 'github.repoSlug',
+  localCall: (args: {
+    repoPath: string
+    repoId: string
+  }) => Promise<GitHubRepositoryIdentity | null>
 ): Promise<GitHubRepositoryIdentity | null> {
  return runtimeTarget.kind === 'environment'
-    ? await callRuntimeRpc<GitHubRepositoryIdentity | null>(
+    ? callRuntimeRpc<GitHubRepositoryIdentity | null>(
        runtimeTarget,
-        'github.repoUpstream',
+        method,
        { repo: repo.id },
        { timeoutMs: 30_000 }
      )
-    : await window.api.gh.repoUpstream({ repoPath: repo.path, repoId: repo.id })
+    : localCall({ repoPath: repo.path, repoId: repo.id })
 }

-async function resolveRepositorySlugLive(
+export function resolveRepositoryUpstreamLive(
  runtimeTarget: RuntimeTarget,
  repo: Repo
 ): Promise<GitHubRepositoryIdentity | null> {
-  return runtimeTarget.kind === 'environment'
-    ? await callRuntimeRpc<GitHubRepositoryIdentity | null>(
-        runtimeTarget,
-        'github.repoSlug',
-        { repo: repo.id },
-        { timeoutMs: 30_000 }
-      )
-    : await window.api.gh.repoSlug({ repoPath: repo.path, repoId: repo.id })
+  return resolveRepositoryIdentityLive(runtimeTarget, repo, 'github.repoUpstream', (args) =>
+    window.api.gh.repoUpstream(args)
+  )
+}
+
+function resolveRepositorySlugLive(
+  runtimeTarget: RuntimeTarget,
+  repo: Repo
+): Promise<GitHubRepositoryIdentity | null> {
+  return resolveRepositoryIdentityLive(runtimeTarget, repo, 'github.repoSlug', (args) =>
+    window.api.gh.repoSlug(args)
+  )
 }

 export async function resolveRepositoryGitHubAvatar(
@@ -52,9 +61,8 @@ export async function resolveRepositoryGitHubAvatar(
  if (upstream) {
    return { repoIcon: githubAvatarIcon(upstream), upstream }
  }
-  // Why: a null live upstream is ambiguous (offline/unauthed vs genuinely not a
-  // fork). Don't downgrade a known fork identity to the origin slug — keep the
-  // last-known parent avatar so a transient failure can't clobber fork identity.
+  // Why: a null live upstream is ambiguous (offline/unauthed vs. not-a-fork). Keep
+  // the last-known parent avatar so a transient failure can't clobber fork identity.
  if (repo.upstream) {
    return { repoIcon: githubAvatarIcon(repo.upstream), upstream: repo.upstream }
  }
@@ -89,7 +89,9 @@ export function WorktreeTitleInlineRename({
      // root owns that stale-write guard without a mount-only Effect.
      mountedRef.current = node !== null
      titleElementRef.current = node
-      if (!node || editingRef.current) {
+      // Why: wrapped titles render in full and never truncate, so skip the measure +
+      // ResizeObserver entirely — for that mode it could only churn unused state.
+      if (!node || editingRef.current || wrapTitle) {
        measureTitleTruncated(null)
        return
      }
@@ -109,7 +111,7 @@ export function WorktreeTitleInlineRename({
      observer.observe(node)
      titleResizeObserverRef.current = observer
    },
-    [measureTitleTruncated]
+    [measureTitleTruncated, wrapTitle]
  )

  const titleElementKey = `${displayName}:${showUnreadEmphasis ? 'unread' : 'read'}`
@@ -28,6 +28,28 @@ export function isGitHubUserAttachmentVideoLink(
  return isGitHubUserAttachmentUrl(href) && isBareAutolink(children, href)
 }

+// Shared fallback link for attachments that can't render inline (see the image
+// note below on why load failures drop to a session-scoped link).
+function AttachmentFallbackLink({
+  href,
+  children
+}: {
+  href: string
+  children: React.ReactNode
+}): React.ReactElement {
+  return (
+    <a
+      href={href}
+      target="_blank"
+      rel="noreferrer"
+      className="break-all text-primary underline underline-offset-2 hover:text-primary/80"
+      onClick={(e) => e.stopPropagation()}
+    >
+      {children}
+    </a>
+  )
+}
+
 export function GitHubUserAttachmentVideo({
  href,
  children
@@ -38,17 +60,7 @@ export function GitHubUserAttachmentVideo({
  const [failed, setFailed] = React.useState(false)

  if (failed) {
-    return (
-      <a
-        href={href}
-        target="_blank"
-        rel="noreferrer"
-        className="break-all text-primary underline underline-offset-2 hover:text-primary/80"
-        onClick={(e) => e.stopPropagation()}
-      >
-        {children}
-      </a>
-    )
+    return <AttachmentFallbackLink href={href}>{children}</AttachmentFallbackLink>
  }

  return (
@@ -82,17 +94,7 @@ export function GitHubUserAttachmentImage({
  // user's GitHub session cookies, so wrap in a top-level link (opening the
  // URL where that session exists) and drop to a text link on load error.
  if (failed) {
-    return (
-      <a
-        href={src}
-        target="_blank"
-        rel="noreferrer"
-        className="break-all text-primary underline underline-offset-2 hover:text-primary/80"
-        onClick={(e) => e.stopPropagation()}
-      >
-        {label}
-      </a>
-    )
+    return <AttachmentFallbackLink href={src}>{label}</AttachmentFallbackLink>
  }

  return (
@@ -6,36 +6,30 @@ const EMPTY_RUNTIME_PANE_TITLES_BY_TAB_ID: AppState['runtimePaneTitlesByTabId']
 const EMPTY_REPOS: AppState['repos'] = []
 const EMPTY_WORKTREES: ReturnType<typeof getAllWorktreesFromState> = []

-function shouldReadPopoverSlices(open: boolean): boolean {
-  return open
-}
-
 export function getResourceUsageTabsByWorktree(
  state: Pick<AppState, 'tabsByWorktree'>,
  open: boolean
 ): AppState['tabsByWorktree'] {
-  return shouldReadPopoverSlices(open) ? state.tabsByWorktree : EMPTY_TABS_BY_WORKTREE
+  return open ? state.tabsByWorktree : EMPTY_TABS_BY_WORKTREE
 }

 export function getResourceUsageRuntimePaneTitlesByTabId(
  state: Pick<AppState, 'runtimePaneTitlesByTabId'>,
  open: boolean
 ): AppState['runtimePaneTitlesByTabId'] {
-  return shouldReadPopoverSlices(open)
-    ? state.runtimePaneTitlesByTabId
-    : EMPTY_RUNTIME_PANE_TITLES_BY_TAB_ID
+  return open ? state.runtimePaneTitlesByTabId : EMPTY_RUNTIME_PANE_TITLES_BY_TAB_ID
 }

 export function getResourceUsageRepos(
  state: Pick<AppState, 'repos'>,
  open: boolean
 ): AppState['repos'] {
-  return shouldReadPopoverSlices(open) ? state.repos : EMPTY_REPOS
+  return open ? state.repos : EMPTY_REPOS
 }

 export function getResourceUsageAllWorktrees(
  state: Pick<AppState, 'worktreesByRepo'>,
  open: boolean
 ): ReturnType<typeof getAllWorktreesFromState> {
-  return shouldReadPopoverSlices(open) ? getAllWorktreesFromState(state) : EMPTY_WORKTREES
+  return open ? getAllWorktreesFromState(state) : EMPTY_WORKTREES
 }
@@ -136,7 +136,7 @@ export function TabBarQuickCommandsButton({
    return null
  }

-  // Empty state: single "+ Command" button that opens the dialog directly.
+  // Empty state: single button that opens the dialog directly.
  if (!hasAnyCommands) {
    return (
      <>
@@ -166,11 +166,9 @@ export async function readRuntimeFileContent({
      { timeoutMs: 15_000 }
    )
  } catch (err) {
-    // Why: files.read (readMobileFile) rejects binary paths — PDFs/images/etc —
-    // with the exact runtime error code 'binary_file'. Fetch the previewable
-    // base64 payload via files.readPreview so the editor renders them like the
-    // local/SSH paths do, keeping the server as the single binary-detection
-    // authority. Match the typed RPC error so an unrelated failure can't spoof it.
+    // Why: files.read rejects binary paths with the typed 'binary_file' error; fall
+    // back to the base64 preview RPC so PDFs/images render like local/SSH paths.
+    // Match the exact typed error so an unrelated failure can't spoof the fallback.
    if (err instanceof RuntimeRpcCallError && err.message === 'binary_file') {
      return callRuntimeRpc<RuntimeFilePreviewResult>(
        target,
@@ -1699,61 +1699,17 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (s
        const existingPreviewIdx = s.openFiles.findIndex((f) => f.id === replaceablePreviewId)
        if (existingPreviewIdx !== -1) {
          const replacedPreview = s.openFiles[existingPreviewIdx]
-          const nextEditorDrafts =
-            replacedPreview.id === id
-              ? s.editorDrafts
-              : Object.fromEntries(
-                  Object.entries(s.editorDrafts).filter(([fileId]) => fileId !== replacedPreview.id)
-                )
-          const nextMarkdownViewMode =
-            replacedPreview.id === id
-              ? s.markdownViewMode
-              : Object.fromEntries(
-                  Object.entries(s.markdownViewMode).filter(
-                    ([fileId]) => fileId !== replacedPreview.id
-                  )
-                )
-          const nextEditorViewMode =
-            replacedPreview.id === id
-              ? s.editorViewMode
-              : Object.fromEntries(
-                  Object.entries(s.editorViewMode).filter(
-                    ([fileId]) => fileId !== replacedPreview.id
-                  )
-                )
-          const markdownVisibilityKeys = new Set([replacedPreview.id])
-          if (replacedPreview.markdownPreviewSourceFileId) {
-            markdownVisibilityKeys.add(replacedPreview.markdownPreviewSourceFileId)
-          }
-          const visibilityKeysToRemove = [...markdownVisibilityKeys].filter(
-            (key) =>
-              !s.openFiles.some(
-                (file, index) =>
-                  index !== existingPreviewIdx &&
-                  (file.id === key || file.markdownPreviewSourceFileId === key)
-              )
-          )
-          const nextMarkdownFrontmatterVisible =
-            replacedPreview.id === id || visibilityKeysToRemove.length === 0
-              ? s.markdownFrontmatterVisible
-              : removeMarkdownVisibilityKeys(s.markdownFrontmatterVisible, visibilityKeysToRemove)
-          const nextMarkdownTableOfContentsVisible =
-            replacedPreview.id === id || visibilityKeysToRemove.length === 0
-              ? s.markdownTableOfContentsVisible
-              : removeMarkdownVisibilityKeys(
-                  s.markdownTableOfContentsVisible,
-                  visibilityKeysToRemove
-                )
-          // Why: editorCursorLine entries accumulate per file; clean up the
-          // evicted preview's entry so it does not leak across tab replacements.
-          const nextEditorCursorLine =
-            replacedPreview.id === id
-              ? s.editorCursorLine
-              : Object.fromEntries(
-                  Object.entries(s.editorCursorLine).filter(
-                    ([fileId]) => fileId !== replacedPreview.id
-                  )
-                )
+          // Why: reuse the shared eviction helper (as the four other preview-
+          // replacement paths do) so per-file cursor/draft/visibility cleanup stays
+          // defined in one place instead of a hand-rolled copy that drifts.
+          const {
+            editorDrafts: nextEditorDrafts,
+            editorCursorLine: nextEditorCursorLine,
+            markdownViewMode: nextMarkdownViewMode,
+            editorViewMode: nextEditorViewMode,
+            markdownFrontmatterVisible: nextMarkdownFrontmatterVisible,
+            markdownTableOfContentsVisible: nextMarkdownTableOfContentsVisible
+          } = removeEditorStateForReplacedPreview(s, replacedPreview, id)
          // Replace in-place to preserve tab position
          newFiles = s.openFiles.map((f, i) =>
            i === existingPreviewIdx
@@ -1038,9 +1038,9 @@ function buildPRRefreshCandidate(
    true
  )
  const cachedFallbackPRNumber = cachedPR?.number ?? null
-  // Why: a merged PR stays a valid fallback when the worktree sits on its head
-  // or on a commit main confirmed as part of the PR; anything else means the
-  // branch moved on and the number must not resurrect the old merged PR.
+  // Why: a merged PR stays a valid fallback when the worktree sits on its head or
+  // on a commit confirmed to be part of the PR; anything else means the branch
+  // moved on and the number must not resurrect the old merged PR.
  const cachedMergedPRMovedPastHead =
    worktree.linkedPR == null &&
    cachedPR?.state === 'merged' &&
@@ -1272,7 +1272,7 @@ function shouldPreserveExistingPRForFallbackMiss(args: {
  const worktreeHead = worktree?.head
  // Why: merged branch PRs are only safe to keep when cached PR metadata still
  // matches the commit this stored worktree is actually on — exactly, or via a
-  // head main confirmed to be part of the merged PR.
+  // head confirmed to be part of the merged PR.
  const preservesMergedPRForCurrentHead =
    args.nextPR === null &&
    args.linkedPRNumber == null &&
@@ -117,8 +117,6 @@ export function resolveComposerBranchNameOverrideForCreate(args: {
  createBranchFromWorkspaceName?: boolean
 }): string | undefined {
  if (!args.branchNameOverride) {
-    // Why: branch mode keeps slash-containing git branch names while the
-    // workspace folder name may still be sanitized separately.
    return args.createBranchFromWorkspaceName && args.workspaceName.includes('/')
      ? args.workspaceName
      : undefined