feat: enhance diff view fix plans with deep research upgrades
- Updated the diff view fix plans to reflect findings from deep research conducted by 4 parallel and 3 deep research agents. - Improved line counting methods, upgrading confidence from 7/10 to 9.5/10, and introduced a unified line counting utility for better accuracy. - Enhanced hunk mapping confidence from 5/10 to 9/10 with a new HunkSnippetMatcher class, optimizing matching processes. - Added a new function to resolve team display names for improved notification handling, ensuring accurate user communication. - Included additional fixes and improvements in the summary table, marking several tasks as completed and pending implementation.
This commit is contained in:
iliya
parent
d97a757f24
commit
d177d22dba
2 changed files with 276 additions and 24 deletions
|
|
@ -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<hunkIndex, SnippetDiff[]>
|
||||
// Один 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 — **НЕ СУЩЕСТВУЕТ!**
|
||||
- **Нужно создать оба перед реализацией**
|
||||
|
|
|
|||
|
|
@ -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<string> {
|
||||
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<void> {
|
||||
// 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
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue