diff --git a/docs/research/diff-view-fix-plans.md b/docs/research/diff-view-fix-plans.md index 849f3e06..861c100a 100644 --- a/docs/research/diff-view-fix-plans.md +++ b/docs/research/diff-view-fix-plans.md @@ -1,7 +1,8 @@ # Diff View — Detailed Fix Plans from Deep Research Date: 2026-02-26 -Source: 4 parallel research agents analyzing actual source code +Source: 4 parallel research agents + 3 deep research agents (Round 2) +Last updated: 2026-02-26 (Round 2 deep research — 3 agents, 280k+ tokens total) --- @@ -185,9 +186,20 @@ const diffResult = diffLines(original ?? '', modified ?? ''); ## Fix #1+#2: Unified Line Counting -**Confidence: 7/10** +**Confidence: 7/10 → UPGRADED to 9.5/10 (after deep research)** **Effort: ~4-6 hours** +### Deep Research Upgrade (2026-02-26) + +Deep research agent (76.1k tokens, 12 tool uses) found a significantly more reliable approach: + +1. **UnifiedLineCounter** — единая утилита на `diffLines()` для ALL counting paths +2. **filesSeen tracking** — Set для отслеживания Write (новый файл vs перезапись) +3. **File-history backups** — использование `~/.claude/file-history/` для получения оригинального контента при Write-update (вместо приблизительной оценки "assume ~same amount removed") +4. **buildTimeline fix** — ChangeExtractorService.buildTimeline тоже должен использовать `diffLines()` вместо `split('\n').length` + +**Ключевое улучшение**: вместо `linesRemoved += added` (assume ~same) для Write-update, агент обнаружил что можно получить оригинальный контент через тот же FileContentResolver/file-history pipeline и сделать точный diff. + ### Current State: 3 Independent Algorithms #### MemberStatsComputer (`src/main/services/team/MemberStatsComputer.ts`) @@ -326,9 +338,23 @@ Fundamental limitation — command string has no execution output. ## Fix #6+#7: Hunk↔Snippet Mapping + indexOf -**Confidence: 5/10** +**Confidence: 5/10 → UPGRADED to 9/10 (after deep research)** **Effort: ~150 LOC** +### Deep Research Upgrade (2026-02-26) + +Deep research agent (83.8k tokens, 24 tool uses) found a significantly more reliable approach: + +1. **HunkSnippetMatcher** — отдельный класс для маппинга с fallback chain: + - Level 1: `contextHash` — хеш ±3 строк контекста вокруг edit site, вычисляется при извлечении snippet в ChangeExtractorService + - Level 2: `structuredPatch()` content overlap — hunk added/removed lines vs snippet newString/oldString + - Level 3: `indexOf` с disambiguation через oldString proximity scoring +2. **contextHash на SnippetDiff** — новое поле, вычисляется один раз при создании snippet, используется для быстрого matching без повторного парсинга +3. **Position-aware rejection** — вместо `content.indexOf(snippet.newString)` (первое вхождение), ищет ВСЕ вхождения и выбирает ближайшее к hunk position +4. **Fallback chain** — если contextHash не матчит → content overlap → indexOf, каждый уровень менее точный но покрывает больше кейсов + +**Ключевое улучшение**: добавление `contextHash` поля в SnippetDiff при извлечении (в ChangeExtractorService) даёт O(1) matching вместо O(n×m) content scanning. + ### Current Architecture Flow ``` @@ -579,9 +605,210 @@ Both are larger architectural changes that go beyond the current fix scope. ## Summary: Implementation Priority -| Fix | Confidence | Effort | Do When | -|-----|-----------|--------|---------| -| #11 Cache TTL | 10/10 | 2 lines | NOW | -| #10 OOM safeguard | 9/10 | ~30 LOC | NOW | -| #1+#2 Line counting | 7/10 | ~4-6h | After tests written | -| #6+#7 Hunk mapping | 5/10 | ~150 LOC | After architectural review | +| Fix | Confidence | Effort | Status | +|-----|-----------|--------|--------| +| #11 Cache TTL | 10/10 | 2 lines | DONE (commit d97a757) | +| #10 OOM safeguard | 9/10 | ~30 LOC | DONE (commit d97a757) | +| #1+#2 Line counting | 9.5/10 (was 7) | ~4-6h | Deep research done, pending implementation | +| #6+#7 Hunk mapping | 9/10 (was 5) | ~150 LOC | Deep research done, pending implementation | + +### Also Fixed in d97a757 +- #5: computeHunkIndexAtPos → nearest hunk (CodeMirrorDiffView.tsx) +- #8: Skeleton flash after save (changeReviewSlice.ts) +- #9: CRLF normalization (DiffViewer.tsx) +- #19: threshold 1.0 → 0.85 (CodeMirrorDiffView.tsx) +- #20: useEffect dependency array (FileSectionDiff.tsx) +- #25: bash relative paths (MemberStatsComputer.ts) +- #26: empty line ?? fix (DiffViewer.tsx) + +--- + +# Round 2: Deep Research (3 parallel agents, 280k+ tokens) + +## Agent 1: UnifiedLineCounter — Exact Line Numbers & Code Paths + +### 6 точек с неправильным подсчётом строк + +| # | Файл | Метод | Строки | Алгоритм | Проблема | Влияет на | +|---|------|-------|--------|----------|----------|-----------| +| 1 | MemberStatsComputer.ts | parseFile (Edit) | 193-196 | `split('\n').length` diff | Не считает реальные diff операции | MemberFullStats → Team stats | +| 2 | MemberStatsComputer.ts | parseFile (Write) | 208 | `split('\n').length` абсолют | `removed` всегда 0 | MemberFullStats → Team stats | +| 3 | MemberStatsComputer.ts | parseFile (NotebookEdit) | 220 | `split('\n').length` абсолют | Аналогично Write | MemberFullStats → Team stats | +| 4 | ChangeExtractorService.ts | buildTimeline() | 426-427 | `split('\n').length` diff | Не использует собственный `countLines()`! | FileEditTimeline → UI | +| 5 | ChangeExtractorService.ts | generateEditSummary() | 445, 449-450 | `split('\n').length` | Дублирует логику подсчёта | FileEditEvent.summary | +| 6 | ChangeExtractorService.ts | aggregateByFile() | 391 | `countLines()` → `diffLines()` | **ПРАВИЛЬНО** | FileChangeSummary | + +### Точные строки для замены + +**MemberStatsComputer.ts:193-196 (Edit):** +```typescript +// ТЕКУЩЕЕ (НЕПРАВИЛЬНО): +const oldLines = oldStr ? oldStr.split('\n').length : 0; +const newLines = newStr ? newStr.split('\n').length : 0; +const fileAdded = newLines > oldLines ? newLines - oldLines : 0; +const fileRemoved = oldLines > newLines ? oldLines - newLines : 0; +// ЗАМЕНА: const { added: fileAdded, removed: fileRemoved } = UnifiedLineCounter.countLines(oldStr, newStr); +``` + +**MemberStatsComputer.ts:208 (Write):** +```typescript +// ТЕКУЩЕЕ (НЕПРАВИЛЬНО): +const fileAdded = writeContent.split('\n').length; +linesAdded += fileAdded; +addFileLines(input.file_path, fileAdded, 0); // removed всегда 0! +// ЗАМЕНА: использовать diffLines('', writeContent) для write-new, отслеживать через filesSeen +``` + +**MemberStatsComputer.ts:220 (NotebookEdit):** +```typescript +// ТЕКУЩЕЕ (НЕПРАВИЛЬНО): +const fileAdded = src.split('\n').length; +// ЗАМЕНА: аналогично Write +``` + +**ChangeExtractorService.ts:426-427 (buildTimeline):** +```typescript +// ТЕКУЩЕЕ (НЕПРАВИЛЬНО): +linesAdded: Math.max(0, s.newString.split('\n').length - s.oldString.split('\n').length), +linesRemoved: Math.max(0, s.oldString.split('\n').length - s.newString.split('\n').length), +// ЗАМЕНА: const { added, removed } = this.countLines(s.oldString, s.newString); +``` + +**ChangeExtractorService.ts:445,449-450 (generateEditSummary):** +```typescript +// ТЕКУЩЕЕ (НЕПРАВИЛЬНО): +const lines = snippet.oldString.split('\n').length; +const added = snippet.newString.split('\n').length; +const removed = snippet.oldString.split('\n').length; +// ЗАМЕНА: использовать this.countLines() +``` + +### Критичные находки +- `diffLines` НЕ импортирован в MemberStatsComputer — нужно добавить +- `seenFiles` в ChangeExtractorService (строка 207-265) определяет write-new vs write-update, но НЕ учитывает файлы существовавшие ДО сессии +- JSONL парсится строго последовательно — filesSeen паттерн безопасен +- Performance: `diffLines()` для типичных snippets (<50 строк) = микросекунды, no risk +- **НЕТ ТЕСТОВ** для countLines, buildTimeline, generateEditSummary! + +--- + +## Agent 2: HunkSnippetMatcher — Exact Bug Chain + +### Полная цепочка бага (от UI до backend) + +``` +1. UI: CodeMirrorDiffView.tsx → computeHunkIndexAtPos(state, pos) → chunk index +2. UI: FileSectionDiff.tsx:123-124 → onHunkRejected(file.filePath, idx) +3. Store: changeReviewSlice.ts:610 → + for (let i = 0; i < file.snippets.length; i++) { + hunkDecs[i] = hunkDecisions[`${filePath}:${i}`] ?? 'pending'; + } + // ← СТРОИТ hunkDecs по SNIPPET INDICES, но hunk indices != snippet indices! +4. IPC: review.ts:191-203 → handleRejectHunks(teamName, filePath, original, modified, hunkIndices, snippets) +5. Backend: ReviewApplierService.ts:71 → trySnippetLevelReject(modified, hunkIndices, snippets) +6. Bug #1: строка 340-342 → hunkIndices.map(idx => validSnippets[idx]) // 1:1 ASSUMPTION +7. Bug #2: строка 353 → content.indexOf(snippet.newString) // ПЕРВОЕ ВХОЖДЕНИЕ +8. Fallback: строка 87 → tryHunkLevelReject() (structuredPatch + inverse) +``` + +### SnippetDiff — полное определение (shared/types/review.ts:1-12) + +```typescript +export interface SnippetDiff { + toolUseId: string; + filePath: string; + toolName: 'Edit' | 'Write' | 'MultiEdit'; + type: 'edit' | 'write-new' | 'write-update' | 'multi-edit'; + oldString: string; // ← ДО изменения (пусто для write-new) + newString: string; // ← ПОСЛЕ изменения + replaceAll: boolean; + timestamp: string; + isError: boolean; + // НЕТ: contextHash, lineNumber, position — core problem! +} +``` + +### Как создаются snippets (ChangeExtractorService.ts:176-308) + +- **Edit** (строки 239-258): берёт `input.old_string`, `input.new_string` из JSONL. НЕТ доступа к полному файлу. +- **Write** (строки 259-277): `newString = input.content` (весь файл!). 1 Write → 1 snippet → N hunks при patch. +- **MultiEdit** (строки 278-302): `for (const edit of edits)` → N snippets с ОДНИМ toolUseId. + +### Edge cases + +| Case | Текущее поведение | Опасность | +|------|-------------------|-----------| +| `snippet.newString === ""` (deletion) | `indexOf("") = 0` | Всегда находит позицию 0! | +| `newString` встречается 5+ раз | `indexOf` = первое | Неправильная позиция | +| `replaceAll = true` | `content.split(new).join(old)` | OK, но конфликт с другими snippets | +| MultiEdit | N snippets с одним toolUseId | Могут слиться в 1 hunk | +| 1 Write → N hunks | hunkIdx > snippets.length | Out of bounds! | + +### Proposed HunkSnippetMatcher Architecture + +```typescript +class HunkSnippetMatcher { + // Fallback chain (от самого точного к менее точному): + // 1. contextHash match (если добавлено к SnippetDiff) + // 2. structuredPatch content overlap (hunk lines vs snippet strings) + // 3. indexOf с disambiguation через oldString proximity + + matchAll(original, modified, hunks, snippets): Map + // Один hunk может соответствовать нескольким snippets! + // Один snippet может создать несколько hunks (Write)! +} +``` + +--- + +## Agent 3: Integration Points & Conflicts + +### 3 критичных конфликта между #1+#2 и #6+#7 + +#### КОНФЛИКТ #1: countLines() в ChangeExtractorService +- #1+#2 исправляет countLines() (строки 463-473) +- #6+#7 зависит от результатов countLines() в aggregateByFile() +- **Решение:** Реализовать #1+#2 ПЕРВЫМ + +#### КОНФЛИКТ #2: FileContentResolver переопределяет numbers +- FileContentResolver.getFileContent() (строки 144-156) ПЕРЕСЧИТЫВАЕТ linesAdded/linesRemoved из full content +- Может перезаписать значения из ChangeExtractorService +- **Решение:** Это OK — FileContentResolver использует более точный метод (full content diff) + +#### КОНФЛИКТ #3: Порядок snippets +- Если #1+#2 изменит фильтрацию/порядок snippets → hunk indices в #6+#7 сломаются +- **Решение:** #1+#2 НЕ меняет порядок snippets, только алгоритм подсчёта + +### Порядок реализации: #1+#2 ПЕРВЫМ, потом #6+#7 + +### Карта зависимостей SnippetDiff (13 файлов): + +``` +Создание: + ChangeExtractorService.parseJSONLFile() [строки 177-308] + ↓ +Агрегация: + ChangeExtractorService.aggregateByFile() [строки 368-415] + ↓ +IPC передача (5 каналов): + REVIEW_GET_AGENT_CHANGES, REVIEW_GET_TASK_CHANGES, + REVIEW_GET_FILE_CONTENT, REVIEW_REJECT_HUNKS, REVIEW_PREVIEW_REJECT + ↓ +Потребители: + FileContentResolver.resolveFileContent() — реконструкция + ReviewApplierService.trySnippetLevelReject() — reject/accept + ReviewDiffContent → SnippetDiffView — UI рендеринг + ChangeStatsBadge — отображает +/- +``` + +### Если добавить contextHash к SnippetDiff: +- shared/types/review.ts — ОБЯЗАТЕЛЬНО (тип) +- ChangeExtractorService.ts — ОБЯЗАТЕЛЬНО (вычисление при создании) +- Остальные 11 файлов — НЕ ТРЕБУЮТ изменений (optional field, JSON-safe) +- Preload/IPC — не требуют изменений (JSON сериализация OK) + +### Тестовая инфраструктура: +- MemberStatsComputer.test.ts — ЕСТЬ (75 строк, только Bash эвристика) +- ChangeExtractorService.test.ts — **НЕ СУЩЕСТВУЕТ!** +- ReviewApplierService.test.ts — **НЕ СУЩЕСТВУЕТ!** +- **Нужно создать оба перед реализацией** diff --git a/src/main/index.ts b/src/main/index.ts index df66467e..2cd60d61 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -68,6 +68,20 @@ const INBOX_NOTIFY_DEBOUNCE_MS = 500; /** Messages sent from our UI (user_sent) — suppress notifications for these. */ const suppressedSources = new Set(['user_sent']); +/** Resolve human-friendly team display name, falling back to raw teamName. */ +async function resolveTeamDisplayName(teamName: string): Promise { + try { + if (teamDataService) { + const summary = await teamDataService.listTeams(); + const team = summary.find((t) => t.teamName === teamName); + if (team?.displayName) return team.displayName; + } + } catch { + // fallback + } + return teamName; +} + async function notifyNewInboxMessages(teamName: string, detail: string): Promise { // detail is like "inboxes/carol.json" — extract member name const match = /^inboxes\/(.+)\.json$/.exec(detail); @@ -94,30 +108,20 @@ async function notifyNewInboxMessages(teamName: string, detail: string): Promise const newMessages = messages.slice(0, messages.length - prevCount); inboxMessageCounts.set(key, messages.length); - // Resolve team display name - let teamDisplayName = teamName; - try { - const service = teamDataService; - if (service) { - const summary = await service.listTeams(); - const team = summary.find((t) => t.teamName === teamName); - if (team?.displayName) teamDisplayName = team.displayName; - } - } catch { - // use teamName as fallback - } + const teamDisplayName = await resolveTeamDisplayName(teamName); for (const msg of newMessages) { + // Only notify for messages addressed to the human user + if (msg.to !== 'user') continue; // Skip messages sent from our own UI if (msg.source && suppressedSources.has(msg.source)) continue; const fromLabel = msg.from || 'Unknown'; - const toLabel = msg.to || memberName; const summary = msg.summary || msg.text.slice(0, 60); showTeamNativeNotification({ - title: `${teamDisplayName}`, - subtitle: `${fromLabel} → ${toLabel}: ${summary}`, + title: teamDisplayName, + subtitle: `${fromLabel}: ${summary}`, body: msg.text, }); } @@ -260,6 +264,27 @@ function wireFileWatcherEvents(context: ServiceContext): void { }, INBOX_NOTIFY_DEBOUNCE_MS) ); } + + // Show native OS notification for live lead process replies. + // These don't go through inbox files — they're held in-memory by TeamProvisioningService. + if (detail === 'lead-process-reply' || detail === 'lead-direct-reply') { + const messages = teamProvisioningService.getLiveLeadProcessMessages(teamName); + const latest = messages.length > 0 ? messages[messages.length - 1] : undefined; + // Only notify for messages addressed to the human user + if (latest?.to === 'user') { + const fromLabel = latest.from || 'team-lead'; + const summary = latest.summary || latest.text.slice(0, 60); + void resolveTeamDisplayName(teamName) + .then((displayName) => { + showTeamNativeNotification({ + title: displayName, + subtitle: `${fromLabel}: ${summary}`, + body: latest.text, + }); + }) + .catch(() => undefined); + } + } } catch { // ignore }