src/commands/create-prd.tsx (1)

305-343: Consider extracting duplicated cleanup logic.

The cleanup logic in handleComplete (lines 310-318) and handleCancel (lines 330-338) is identical. Consider extracting it to a helper function to reduce duplication.

♻️ Suggested refactor

+  const cleanupImages = async () => {
+    try {
+      await performExitCleanup({ cwd });
+    } catch (err) {
+      console.error(
+        "Warning: Image cleanup failed:",
+        err instanceof Error ? err.message : String(err),
+      );
+    }
+  };
+
   const handleComplete = async (result: PrdCreationResult) => {
     root.unmount();
     renderer.destroy();

-    // Clean up any attached images from this session
-    try {
-      await performExitCleanup({ cwd });
-    } catch (err) {
-      // Don't let cleanup errors prevent normal exit
-      console.error(
-        "Warning: Image cleanup failed:",
-        err instanceof Error ? err.message : String(err),
-      );
-    }
+    await cleanupImages();

     console.log("");
     console.log(`PRD workflow complete: ${result.prdPath}`);
     resolvePromise(result);
   };

   const handleCancel = async () => {
     root.unmount();
     renderer.destroy();

-    // Clean up any attached images from this session
-    try {
-      await performExitCleanup({ cwd });
-    } catch (err) {
-      // Don't let cleanup errors prevent normal exit
-      console.error(
-        "Warning: Image cleanup failed:",
-        err instanceof Error ? err.message : String(err),
-      );
-    }
+    await cleanupImages();

     console.log("");
     console.log("PRD creation cancelled.");
     resolvePromise(null);
   };

src/tui/utils/image-attachment.test.ts (1)

92-173: Consider extracting the PNG buffer constant for reuse.

The valid PNG buffer bytes are repeated across multiple tests with slight variations. Consider extracting a minimal valid PNG constant at the top of the describe block for better maintainability.

♻️ Suggested refactor

 describe('Image Storage', () => {
   let testDir: string;
+  
+  // Minimal valid PNG (1x1 transparent pixel)
+  const VALID_PNG_HEADER = Buffer.from([
+    0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a,
+  ]);

   beforeEach(async () => {

src/tui/utils/marker-edit.ts (1)

78-94: Callback invocation order may not match visual order.

At line 81, markers are sorted by start descending for deletion (to preserve indices), but at line 91, onImageMarkerDeleted is called in the original markers array order (as found in the text, left-to-right). This inconsistency between deletion order and callback order could cause confusion if the callback has side effects that depend on order.

Consider iterating over sorted for both operations or documenting the expected behaviour.

♻️ Suggested consistency fix

       editBuffer.setText(newText);
       editBuffer.setCursorByOffset(Math.min(...markers.map((m) => m.start)));

-      for (const m of markers) onImageMarkerDeleted(m.imageNumber);
+      // Call in ascending order (visual order) for consistent UX
+      const sortedAsc = [...markers].sort((m1, m2) => m1.start - m2.start);
+      for (const m of sortedAsc) onImageMarkerDeleted(m.imageNumber);
       return;

src/tui/components/PrdChatApp.tsx (1)

519-525: Redundant variable assignment.

The comment mentions "no zero-width markers to clean" but userMessage is simply assigned the same value as rawMessage without any transformation. This appears to be leftover from a previous implementation.

♻️ Simplify by using rawMessage directly

     const rawMessage = value?.trim() ?? inputValue.trim();
     if (!rawMessage || isLoading) {
       return;
     }

-    // Use the message as-is (no zero-width markers to clean)
-    const userMessage = rawMessage;
-
     // Check for slash commands first
-    if (isSlashCommand(userMessage)) {
+    if (isSlashCommand(rawMessage)) {
       setInputValue('');
-      const result = await executeSlashCommand(userMessage, {
+      const result = await executeSlashCommand(rawMessage, {

And use rawMessage throughout the rest of the function instead of userMessage.

src/tui/hooks/useImageAttachmentWithFeedback.ts (1)

149-161: getInstallHint and getInstallHints share platform-checking logic, but serve different purposes and cannot be directly unified.

getInstallHints() in clipboard-image.ts returns a Record<string, string> with detailed, tool-specific installation instructions (including distribution-specific details like Debian/Ubuntu vs Arch), used internally by clipboard utilities. getInstallHint() in useImageAttachmentWithFeedback.ts returns a simple string for toast notifications, with more generic messages. Since getInstallHints is not exported and the return types differ, direct reuse is not straightforward. If refactoring, consider whether extracting shared platform logic into a common utility is worthwhile, though the current separation is maintainable given the different contexts and message styles.

src/tui/utils/clipboard-image.ts (1)

41-100: Consider reusing the shared command-existence helper. There is already a commandExists in src/sandbox/detect.ts with a timeout; reusing it (or extracting a shared util) would avoid divergence and reduce the chance of a hang in edge cases.

src/commands/create-prd.tsx (1)

409-447: Extract duplicated cleanup into a shared helper.

handleComplete and handleCancel contain identical unmount → cleanup → log sequences (lines 410-422 and 430-442). Extracting a small helper keeps both paths in sync and reduces the risk of them diverging over time.

♻️ Proposed refactor

+    const teardown = async () => {
+      root.unmount();
+      renderer.destroy();
+      try {
+        await performExitCleanup({ cwd });
+      } catch (err) {
+        console.error(
+          "Warning: Image cleanup failed:",
+          err instanceof Error ? err.message : String(err),
+        );
+      }
+    };
+
     const handleComplete = async (result: PrdCreationResult) => {
-      root.unmount();
-      renderer.destroy();
-
-      // Clean up any attached images from this session
-      try {
-        await performExitCleanup({ cwd });
-      } catch (err) {
-        // Don't let cleanup errors prevent normal exit
-        console.error(
-          "Warning: Image cleanup failed:",
-          err instanceof Error ? err.message : String(err),
-        );
-      }
-
+      await teardown();
       console.log("");
       console.log(`PRD workflow complete: ${result.prdPath}`);
       resolvePromise(result);
     };

     const handleCancel = async () => {
-      root.unmount();
-      renderer.destroy();
-
-      // Clean up any attached images from this session
-      try {
-        await performExitCleanup({ cwd });
-      } catch (err) {
-        // Don't let cleanup errors prevent normal exit
-        console.error(
-          "Warning: Image cleanup failed:",
-          err instanceof Error ? err.message : String(err),
-        );
-      }
-
+      await teardown();
       console.log("");
       console.log("PRD creation cancelled.");
       resolvePromise(null);
     };

tests/utils/clipboard.test.ts (1)

282-343: Good coverage for the happy path — consider adding error-handling tests for readFromClipboard.

The new read tests cover macOS, Linux (with fallback), and Windows. The write suite already tests ENOENT, stderr, other-error, and unsupported platform, but the read suite currently lacks those analogous cases. Adding at least a "all Linux tools fail" test and an unsupported-platform test would improve confidence.

src/tui/components/PrdChatApp.labels.test.ts (1)

1-4: ABOUTME header doesn't mention classifyPastePayload tests.

The file now also tests classifyPastePayload, but the header only mentions buildBeadsLabelsInstruction. As per coding guidelines, all code files should start with a file-level JSDoc comment explaining the file's purpose.

📝 Suggested update

 /**
- * ABOUTME: Tests for buildBeadsLabelsInstruction helper.
- * Verifies case-insensitive dedup and label instruction formatting.
+ * ABOUTME: Tests for buildBeadsLabelsInstruction and classifyPastePayload helpers.
+ * Verifies case-insensitive dedup, label instruction formatting, and paste payload classification.
  */

src/tui/utils/image-attachment.test.ts (1)

262-300: Static analysis ReDoS warnings are false positives here — imageNum is always a number.

The ast-grep warnings about regex-from-variable on lines 271–278 are not exploitable because imageNum is always a parsed integer (digits only), so no malicious patterns can be injected. The warning is relevant in general but can be safely ignored in this context.

However, note that the removeMarkerRemnants helper is duplicated inline for testing rather than imported from the production code. If this logic exists (or is planned) in ChatView.tsx, consider extracting it to a shared utility and testing it directly, rather than maintaining a parallel implementation in tests.

src/tui/utils/marker-edit.ts (1)

53-58: Minor: wordBoundaryBackward doesn't account for image markers as atomic units.

The word-boundary functions treat markers character-by-character (bracket, space, letters, digits), so Ctrl+W near a marker would compute a boundary mid-marker. This is mitigated by deleteRangeSafely catching overlapping markers, but the cursor would land at a non-intuitive position during the boundary calculation itself.

src/tui/components/ChatView.tsx (2)

260-316: Cursor skip-over logic looks correct but has a potential tight loop risk.

The cursor-changed handler calls editBuffer.setCursorByOffset() when the cursor lands inside a marker, which could fire another cursor-changed event. Since the cursor is moved to the marker boundary (outside the marker), the regex won't match on the subsequent event, so it won't loop infinitely. This is safe in practice, but adding a guard (e.g., a boolean flag to skip re-entrant calls) would make it more robust.

---

551-557: findMarkerAtCursor is an imported module function — unnecessary in the dependency array.

findMarkerAtCursor is a stable import, not a React state/prop/callback. Including it in the useCallback dependency array is harmless but misleading.

📝 Suggested fix

   [
     inputEnabled,
     isLoading,
     handleSubmit,
     onImageMarkerDeleted,
-    findMarkerAtCursor,
   ],

src/tui/components/PrdChatApp.tsx (1)

805-825: Clever native-paste vs. keyboard-shortcut debounce — but the 80ms window is a magic number.

The mechanism to skip the clipboard fallback when a native paste event arrives within 80ms is well-designed. Consider extracting the 80ms constant and adding a brief comment explaining why this specific duration was chosen (balances responsiveness vs. giving the terminal time to emit the paste event).

📝 Suggested improvement

+      const PASTE_EVENT_GRACE_PERIOD_MS = 80;
       if (isPasteShortcut) {
         key.preventDefault?.();
         const requestTime = Date.now();
         setTimeout(() => {
           if (lastPasteEventAtRef.current >= requestTime) {
             return;
           }
           void performClipboardPasteFallback();
-        }, 80);
+        }, PASTE_EVENT_GRACE_PERIOD_MS);
         return;
       }

src/tui/hooks/useImageAttachment.ts (1)

252-261: Redundant non-null assertions after truthy guards.

At lines 258, 323, and 357–358 you use ! (e.g. storageResult.path!, pathResult.filePath!) on values that have already been verified as truthy by preceding if checks. These assertions are safe but unnecessary — removing them keeps the code cleaner and avoids masking future bugs if the guard logic changes.

♻️ Remove redundant non-null assertions

       displayName: generateDisplayName(
         'clipboard',
-        storageResult.path!,
+        storageResult.path,
         imageNumber - 1,
       ),

       displayName: generateDisplayName(
         'base64',
-        storageResult.path!,
+        storageResult.path,
         imageNumber - 1,
       ),

       displayName: generateDisplayName(
-            pathResult.filePath!,
-            storageResult.path!,
+            pathResult.filePath,
+            storageResult.path,
             imageNumber - 1,
       ),

Also applies to: 317-326, 352-361

src/tui/utils/clipboard-image.ts (2)

47-54: commandExists lacks a timeout, unlike the version in src/sandbox/detect.ts.

The commandExists in src/sandbox/detect.ts (lines 11–44) includes a COMMAND_TIMEOUT_MS guard and kills the process if it hangs. This version has no such safeguard. If which/where hangs (e.g. due to a broken NFS mount on $PATH), it will block indefinitely. In practice this is very unlikely, but it's worth noting since the pattern already exists in the codebase.

---

281-332: AppleScript temp-file path is interpolated directly into the script string.

Line 292 embeds tempFile verbatim into the AppleScript. If tmpdir() ever returns a path containing a double-quote ("), the script will break or — theoretically — be subject to injection. With randomUUID() and standard tmpdir() values this is extremely unlikely, but a defensive escape (replacing " with \\") would eliminate the risk entirely.

src/tui/utils/image-storage.ts (1)

508-520: deleteAllStoredImages performs sequential deletes — consider parallelism for large sets.

Each deletion is awaited sequentially. For the expected use-case (≤10 images per DEFAULT_IMAGE_CONFIG.max_images_per_message), this is fine. If the limit is ever raised significantly, a Promise.allSettled approach would be faster.

src/tui/hooks/useInlineImageIndicators.ts (1)

256-262: Redundant nullish coalescing on line 259.

getIndicatorNumber always returns a number, never undefined or null, so number ?? 1 is unnecessary.

♻️ Remove redundant fallback

     (imageId: string): string => {
       const number = getIndicatorNumber(imageId);
-      return `${INDICATOR_START}[Image ${number ?? 1}]${INDICATOR_END}`;
+      return `${INDICATOR_START}[Image ${number}]${INDICATOR_END}`;
     },

src/tui/hooks/useInlineImageIndicators.ts (4)

210-221: Variable nextNumber shadows the state variable from line 182.

The local const nextNumber = number + 1 on line 213 shadows the state destructuring const [nextNumber, setNextNumber] on line 182. It works because of scoping, but it reduces clarity. Consider a distinct name like updatedNextNumber.

♻️ Suggested rename

       const number = currentNextNumber;
       const nextMap = { ...currentMap, [imageId]: number };
-      const nextNumber = number + 1;
+      const updatedNextNumber = number + 1;

       // Update refs immediately to avoid duplicate assignments on rapid calls.
       indicatorMapRef.current = nextMap;
-      nextNumberRef.current = nextNumber;
+      nextNumberRef.current = updatedNextNumber;

       // Mirror refs in state for rendering.
       setIndicatorMap(nextMap);
-      setNextNumber(nextNumber);
+      setNextNumber(updatedNextNumber);

---

256-262: Unnecessary nullish coalescing on line 259.

getIndicatorNumber always returns a number — it never returns undefined or null. The ?? 1 fallback is dead code and might mislead readers into thinking the function can return nullish values.

♻️ Suggested fix

-      return `${INDICATOR_START}[Image ${number ?? 1}]${INDICATOR_END}`;
+      return `${INDICATOR_START}[Image ${number}]${INDICATOR_END}`;

---

301-311: findImageIdByNumber reads from indicatorMap state instead of indicatorMapRef.

Other functions in this hook (e.g., getIndicatorNumber, renumberIndicators) read from refs for freshness. This function uses the state value, which could be stale within the same render cycle. Consider using indicatorMapRef.current for consistency, and removing the indicatorMap dependency.

♻️ Suggested fix

   const findImageIdByNumber = useCallback(
     (number: number): string | null => {
-      for (const [imageId, num] of Object.entries(indicatorMap)) {
+      for (const [imageId, num] of Object.entries(indicatorMapRef.current)) {
         if (num === number) {
           return imageId;
         }
       }
       return null;
     },
-    [indicatorMap],
+    [],
   );

---

413-438: removeOrphanedIndicators also reads from indicatorMap state — same staleness concern.

Same inconsistency as findImageIdByNumber: prefer indicatorMapRef.current for freshness. The static analysis ReDoS warning on lines 428-430 is a false positive since number is always an integer from the internal map.

♻️ Suggested fix

   const removeOrphanedIndicators = useCallback(
     (text: string, attachedImages: AttachedImage[]): string => {
       const attachedIds = new Set(attachedImages.map((img) => img.id));
       let result = text;

       const indicatorsToRemove: number[] = [];
-      for (const [imageId, number] of Object.entries(indicatorMap)) {
+      for (const [imageId, number] of Object.entries(indicatorMapRef.current)) {
         if (!attachedIds.has(imageId)) {
           indicatorsToRemove.push(number);
         }
       }

       for (const number of indicatorsToRemove) {
         const pattern = new RegExp(
           `${INDICATOR_START}\\[Image ${number}\\]${INDICATOR_END}`,
           'g',
         );
         result = result.replace(pattern, '');
       }

       return result;
     },
-    [indicatorMap],
+    [],
   );

src/tui/utils/clipboard-image.ts (2)

47-54: Duplicate commandExists implementation.

There is an existing commandExists in src/sandbox/detect.ts (lines 11–44) that includes timeout handling and a guard against double-resolution. This local version lacks both. Consider reusing the existing utility to reduce duplication and gain the timeout safety net.

#!/bin/bash
# Check if commandExists from detect.ts is exported and could be imported
rg -n 'export.*commandExists' --type=ts

---

281-332: AppleScript string interpolation with tempFile — low risk but worth hardening.

The tempFile path is interpolated directly into the AppleScript string on line 292. While randomUUID() produces safe characters, if tmpdir() ever returned a path containing double-quotes or backslashes, the AppleScript would break or behave unexpectedly. Consider escaping quotes in tempFile before interpolation.

🛡️ Minimal hardening

+  const safeTempFile = tempFile.replace(/"/g, '\\"');
   const script = `
     try
       set theImage to the clipboard as «class PNGf»
-      set theFile to open for access POSIX file "${tempFile}" with write permission
+      set theFile to open for access POSIX file "${safeTempFile}" with write permission

src/tui/components/PrdChatApp.tsx (4)

252-280: classifyPastePayload control-character heuristic is reasonable but the thresholds are magic numbers.

The combined check controlChars >= 4 && controlChars / Math.max(text.length, 1) > 0.05 uses two hard-coded thresholds. Consider extracting these as named constants for clarity and easier future tuning.

♻️ Extract constants

+const MIN_CONTROL_CHARS_FOR_BINARY = 4;
+const CONTROL_CHAR_RATIO_THRESHOLD = 0.05;
+
 export function classifyPastePayload(text: string): PasteClassification {
   // ...
-  if (controlChars >= 4 && controlChars / Math.max(text.length, 1) > 0.05) {
+  if (controlChars >= MIN_CONTROL_CHARS_FOR_BINARY && controlChars / Math.max(text.length, 1) > CONTROL_CHAR_RATIO_THRESHOLD) {
     return { intercept: true, suppressFallbackInsert: true };
   }

---

834-854: 80 ms race window between keyboard shortcut and native paste event.

The setTimeout on line 847 waits 80 ms to check whether a native PasteEvent arrived after the keyboard shortcut. If the native event is delayed beyond 80 ms (e.g., slow terminal, high system load), both the fallback and the native handler will fire, potentially inserting content twice. This is a known trade-off, but consider making the delay configurable or slightly larger (e.g., 150 ms) to reduce double-paste on slower environments.

---

386-408: Image config defaults are resolved inline — consider centralising.

Lines 388–393 individually fall back to DEFAULT_IMAGE_CONFIG fields. If more config fields are added later, each one needs the same pattern. A single merged config object would be DRYer:

♻️ Merge config once

-  const imagesEnabled = imageConfig?.enabled ?? DEFAULT_IMAGE_CONFIG.enabled;
-  const maxImagesPerMessage =
-    imageConfig?.max_images_per_message ??
-    DEFAULT_IMAGE_CONFIG.max_images_per_message;
-  const showPasteHints =
-    imageConfig?.show_paste_hints ?? DEFAULT_IMAGE_CONFIG.show_paste_hints;
+  const resolvedImageConfig = { ...DEFAULT_IMAGE_CONFIG, ...imageConfig };
+  const { enabled: imagesEnabled, max_images_per_message: maxImagesPerMessage, show_paste_hints: showPasteHints } = resolvedImageConfig;

---

711-719: sendMessage dependency array includes attachedImages — may cause extra re-creations.

attachedImages is an array reference that likely changes on every attach/detach. Since sendMessage only reads attachedImages.length (line 660), consider using a separate attachedImages.length ref or a count state to avoid re-creating sendMessage on every image change. This is minor since React will batch well in practice.

src/tui/components/PrdChatApp.tsx (1)

410-448: handlePrdDetected is captured in the effect closure but is not a dependency.

handlePrdDetected (line 429) is a plain async function that is re-created every render, yet the useEffect only re-runs when its listed deps change. If a prd:detected event fires after a render where outputDir or trackerOptions changed without cwd changing, the callback uses stale values.

In practice this is safe today because outputDir is a static prop default and trackerOptions is derived from cwd. However, eslint-plugin-react-hooks would flag this, and it becomes a real bug if these ever decouple.

Consider wrapping handlePrdDetected in useCallback (with its own deps) and adding it to the effect's dependency array, or moving its body inside the effect.