feat(md): Think/Research mode auto-save, hyperlinks & skill integration

[lichao-sun]

Summary

• Auto-save Think/Research results as _XX.md files (01–99) in the same directory as the source file, with hyperlinks automatically inserted on the selected text
• Hover tooltip on .md hyperlinks with Open / Delete actions; Delete removes both the annotation line and the linked file
• Deep Research mode now loads the inno-deep-research skill automatically for structured, citation-rich reports

Details

Auto-save & Hyperlink insertion

• Inline text: wraps selected text as [text](file.md) — smart markdown-aware matching handles **bold**, `code`, CJK/punctuation boundaries
• Tables & block elements: inserts > 📎 [summary](file.md) annotation after the block, with best-match scoring across all table blocks
• Annotation label is auto-extracted as a one-line summary from the generated content (not the question)

UX improvements

• Preserve scroll position when opening/closing linked files
• Keep Think Mode popup state alive while viewing linked files (no unmount)
• Allow clipboard copy (Cmd+C) of selected text before popup interaction

Deep Research skill

• Research mode system prompt loaded from skills/inno-deep-research/SKILL.md at server startup
• Fallback to built-in prompt if skill file is missing

Test plan

• Select inline text in .md → Think Mode → verify _01.md created and hyperlink inserted
• Select text with bold/code formatting → verify hyperlink wraps correctly
• Select a table → Think Mode → verify > 📎 [summary](file.md) appears after the correct table
• Hover hyperlink → verify Open/Delete tooltip
• Delete → confirm → verify annotation line and linked file both removed
• Open linked file → Close → verify scroll position restored
• Start Think Mode → open a hyperlink mid-query → close → verify popup still active
• Select text → Cmd+C → verify clipboard works before interacting with popup
• Use Deep Research mode → verify structured output with citations

🤖 Generated with Claude Code

[Zhang-Henry]

Code Review: PR #166

Security Issues

1. Path traversal via projectPath — server/routes/quick-qa.js:95

projectPath from the request body is passed directly as cwd to the query() SDK call with no validation. An attacker could set it to any directory on the filesystem:

const { selectedText, question, projectPath, mode = 'fast' } = req.body;
// ...
options: { cwd: projectPath || process.cwd(), ... }

Should validate that projectPath is a descendant of the user's allowed project directories.

2. Race condition on process.env — server/routes/quick-qa.js:124-180

The code mutates process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT per-request and restores it in finally. If two concurrent requests arrive, they race on this global state — one request's finally can restore a stale value while the other is still in-flight.

const prevStreamTimeout = process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT;
process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT = '300000';
// ... async work ...
finally {
  // Another request may have already changed this
  process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT = prevStreamTimeout;
}

3. Markdown injection in linkLabel — MarkdownSelectionPopup.jsx:1143-1151

The auto-generated linkLabel is inserted directly into markdown as [${linkLabel}](...). If the AI-generated content contains ] or ) characters, it can break the link syntax or inject arbitrary markdown.

const annotation = `\n> 📎 [${linkLabel}](${newFileName})\n`;

Should sanitize linkLabel to escape [, ], (, ).

---

Bugs / Correctness

4. Stale closure in handleDeleteLink — CodeEditor.jsx:495-538

handleDeleteLink captures content in its closure. If the user deletes two links quickly, the second delete operates on the stale content from before the first delete completed, potentially overwriting the first change:

const handleDeleteLink = useCallback(async (href, linkText) => {
  // 'content' is captured at closure creation time
  let updatedContent = content.replace(annotationPattern, '\n');
  // ...
  setContent(updatedContent); // may overwrite the first delete's result
}, [file.projectName, file.path, resolvedPath, content]);

Fix: Use a functional state updater setContent(prev => ...) and perform the regex replacements on prev instead of the captured content.

5. findNextSuffix doesn't check the filesystem — MarkdownSelectionPopup.jsx:803-815

Only scans the current markdown content for existing _XX.md patterns. If a _01.md file exists on disk but isn't linked from the current content, a collision occurs and the save silently overwrites the existing file.

function findNextSuffix(existingContent, baseName) {
  const used = new Set();
  // Only scans existingContent, not the filesystem

Consider listing files in the directory via an API call, or at minimum doing a HEAD check before writing.

6. Non-atomic delete-then-save — CodeEditor.jsx:504-534

If api.deleteFile() succeeds but api.saveFile() fails, the linked .md file is deleted while the hyperlink still exists in the source document, leaving a broken link. Consider reversing the order (remove the link first, then delete the file) so the failure mode is a harmless orphan file rather than a broken link.

7. Auto-save effect doesn't handle unmount — MarkdownSelectionPopup.jsx:1087-1164

The async IIFE inside the effect calls onContentChange() and onSaveFile() without checking if the component is still mounted. If the user closes the popup during the save, these calls fire on stale/unmounted state. Add a mounted flag in the effect's cleanup.

8. findContainingBlock only handles tables — MarkdownSelectionPopup.jsx:734-795

Block-level matching only looks for table blocks (lines starting with |). Selecting text inside fenced code blocks, blockquotes, or list blocks won't produce an annotation — the hyperlink is silently dropped.

---

Design / Maintainability

9. 782-line popup component — MarkdownSelectionPopup.jsx

The file mixes utility functions (findMarkdownSpan, findContainingBlock, findNextSuffix), SSE streaming logic, and complex UI rendering in one component. Consider extracting:

• Text matching utilities to a separate module (also makes them unit-testable)
• SSE streaming to a custom hook (useQuickQA)

10. buildMarkdownComponents re-render cascade — CodeEditor.jsx:398-425

buildMarkdownComponents is wrapped in useMemo, but onDeleteLink changes whenever content changes (it's in the dependency array of handleDeleteLink). This means every keystroke or content mutation recreates all markdown components, forcing ReactMarkdown to re-render the entire document tree.

11. Inline <style> tag — MarkdownSelectionPopup.jsx:627-641, line 1195

popupStyles is injected as a <style> element on every render of the popup. Should be a CSS module or a global stylesheet entry.

---

Minor Notes

• server/routes/quick-qa.js line 33 — Comment says "opus" for research mode but MODE_CONFIG actually uses model: 'sonnet' (line 71). Misleading.
• MdLinkWithTooltip tooltip dismiss timeout is 200ms — could feel sluggish or cause tooltip to persist unexpectedly.
• The click-outside effect (line 916) doesn't include handleClose in its dependency array — could reference a stale queryId when aborting.

---

Recommendation

Items 1–4 (path traversal, env race, stale closure, markdown injection) should be addressed before merge. The rest are lower priority but worth tracking.

The feature design is solid — auto-save _XX.md with hyperlink insertion, hover tooltips with delete confirmation, and overlay navigation are well-thought-out UX patterns.

[Zhang-Henry]

Code review — blocking issues before merge

I read through the PR branch (feat/md-think-mode-enhancements @ 5e883ac). Two blocking problems and a few smaller ones:

🛑 1. Runtime crash in MarkdownSelectionPopup.jsx — Temporal Dead Zone

src/components/MarkdownSelectionPopup.jsx:

• L347: useEffect(..., [popupState, handleClose])
• L380: const handleClose = useCallback(() => { ... }, [queryId]);

The effect's dependency array reads handleClose before the const is declared. Because const bindings live in the Temporal Dead Zone until their declaration is evaluated, every render of this component will throw:

ReferenceError: Cannot access 'handleClose' before initialization

I reproduced the exact pattern in plain Node to confirm it's not a toolchain quirk:

function render() {
  const popupState = 'ready';
  const deps = [popupState, handleClose];  // ← TDZ
  const handleClose = () => {};
  return deps;
}
render();
// → ReferenceError: Cannot access 'handleClose' before initialization

vite build does not catch this because it's a runtime error, and there are no tests that actually mount MarkdownSelectionPopup, so vitest run is green as well. The component will crash the moment a user opens any .md file in preview mode — the ErrorBoundary around ChatInterface won't save this panel because the popup is rendered inside CodeEditor, not the chat tree.

Fix: move the handleClose useCallback above the click-outside useEffect (and above handleOpenResult / handleKeyDown which also depend on it). The useRef-for-latest-callback pattern would also work, but just reordering is the smaller change.

🛑 2. Merge conflict with main

gh pr view 166 reports mergeStateStatus: DIRTY. Please rebase on top of the latest main (the file-preview-reader-modes merge 298c4dd touched CodeEditor.jsx, which is the likely source of the conflict).

---

⚠️ Non-blocking, but worth addressing

a. handleDeleteLink reads a closure variable set inside a functional setState updater (src/components/CodeEditor.jsx ~L420–448):

let updatedContent = null;
setContent((prev) => {
  ...
  updatedContent = result !== prev ? result : null;
  return result;
});

if (updatedContent !== null) {
  await api.saveFile(..., updatedContent);
}

Functional updaters are not guaranteed to run synchronously during the setState call — React schedules them to run during the next reconciliation. In practice this often works in event handlers due to eager batching, but it's undefined-behavior territory and will silently break under concurrent rendering. Compute updatedContent outside the updater (or pass the old content from state and do the replacement imperatively) and then call setContent(updatedContent) with a plain value.

b. Dead projectPath validation in server/routes/quick-qa.js: validateProjectPath is thorough, but the client never sends projectPath — MarkdownSelectionPopup.jsx posts { selectedText, question, mode } only. The server always falls back to process.cwd(). Either wire the client to send file.projectName's resolved path, or drop the validator to avoid dead code. Also note the projResolved + '/' boundary check is POSIX-only; use path.sep or path.relative if Windows support matters.

c. Module-level mutation of process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT in quick-qa.js runs as an import side-effect and leaks into every other consumer of the agent SDK in the same process. Move it to your server startup config so the ordering and scope are explicit.

d. handleDeleteLink annotation regex requires a leading \n:

new RegExp(`\\n> 📎 \\[[^\\]]*?\\]\\(${escapedHref}\\)\\n?`, 'g')

If the annotation happens to be the very first line of the file (no leading newline) it won't match and the fallback inline-removal won't fire either, leaving a stale annotation. Anchor with (^|\\n) instead.

e. Auto-save suffix scan (findNextSuffix) only checks existingFiles returned by api.getFiles. If that call 404s or is rate-limited, two concurrent Think-mode runs in the same directory could both pick the same _XX.md and race-overwrite. Consider a server-side "next-available-suffix" endpoint, or retry on conflict.

---

Summary

Please fix (1) and (2) before merge — they're hard blockers. (a) is also a correctness bug that happens to work today by luck; I'd fix it in the same pass. (b)–(e) can land as follow-ups but are cheap to address now.

Happy to re-review once the TDZ fix + rebase land.