diff --git a/docs/research/diff-view-audit.md b/docs/research/diff-view-audit.md new file mode 100644 index 00000000..543d4551 --- /dev/null +++ b/docs/research/diff-view-audit.md @@ -0,0 +1,536 @@ +# Diff View Feature — Full Audit Report + +Date: 2026-02-26 +Verified: 2026-02-26 (4 parallel agents cross-checked every bug against actual source code) + +Comprehensive audit of the changes/diff viewing feature covering line count reliability, +hunk parsing, stat-to-hunk consistency, and UI rendering. + +--- + +## Architecture Overview + +The app uses **two distinct diff strategies**: + +1. **Chat Viewer Diffs** (read-only, simple display): + - `DiffViewer.tsx` — custom LCS-based line-by-line diff + - Pure string comparison, NO hunk structure + +2. **Team Review Diffs** (interactive, hunk-aware): + - CodeMirror Merge plugin (`@codemirror/merge`) + - `ReviewDiffContent.tsx` — uses `diffLines` from `diff` package + - `CodeMirrorDiffView.tsx` — full merge view with hunk navigation + - `ReviewApplierService.ts` — applies/rejects hunks to disk + +### Three independent stat computation paths: + +| Service | Algorithm | Used For | +|---------|-----------|----------| +| `ChangeExtractorService` | `diffLines()` from npm `diff` | Team member changes (file badges) | +| `MemberStatsComputer` | `split('\n').length` arithmetic | Session analytics | +| `FileContentResolver` | `diffLines()` from npm `diff` | Full file content diffs (CodeMirror) | + +--- + +## Evaluation Summary + +| # | Bug | Real? | Confidence | Status | +|---|-----|-------|------------|--------| +| 1 | Two conflicting line-counting methods | **YES** | 9/10 | Open | +| 2 | Write never counts removals | **YES** | 10/10 | Open | +| 3 | Trailing newline off-by-one | **PARTIAL** | 6/10 | Resolves with #1 | +| 4 | FileContentResolver overwrites stats | **PARTIAL** | 5/10 | Design issue | +| 5 | computeHunkIndexAtPos → 0 fallback | **YES** | 10/10 | Open | +| 6 | Hunk ≠ snippet mapping | **YES** | 9/10 | Open | +| 7 | indexOf duplicates in rejection | **YES** | 10/10 | Open | +| 8 | Skeleton flash after save | **FIXED** | 9/10 | Done | +| 9 | CRLF → false diffs | **YES** | 9/10 | Open | +| 10 | OOM on large files (LCS) | **YES** | 8/10 | Open | +| 11 | Race condition disk vs cache | **YES** | 8/10 | Open | +| 12 | Empty string inconsistency | **YES** | 7/10 | Resolves with #1 | +| 13 | Bash estimation ~30-40% | **PARTIAL** | 6/10 | Design limitation | +| 14 | Echo escape handling wrong | **YES** | 8/10 | Open | +| 15 | portionCollapse edge case | **PARTIAL** | 5/10 | Needs testing | +| 16 | No-newline-at-EOF hidden | **YES** | 8/10 | Open | +| 17 | Three-way merge labels | **NO** | 9/10 | False positive | +| 18 | Zero-change files invisible | **YES** | 8/10 | Open | +| 19 | Viewed threshold mismatch | **YES** | 9/10 | Open | +| 20 | useEffect no deps array | **YES** | 10/10 | Open | +| 21 | Hunk count ≠ snippet count | **YES** | 8/10 | Open | +| 22 | Toolbar off-screen narrow viewport | **YES** | 7/10 | Open | +| 23 | Deleted files not marked | **YES** | 8/10 | Open | +| 24 | write-update reconstruction null | **YES** | 9/10 | Open | +| 25 | Bash relative paths | **YES** | 9/10 | Open | +| 26 | Empty line → space | **YES** | 7/10 | Open | +| 27 | No keyboard nav in tree | **YES** | 8/10 | Feature | +| 28 | Collapse state not persisted | **YES** | 9/10 | Open | +| 29 | No stats summary | **NO** | 3/10 | Feature request | +| 30 | Binary files not detected | **YES** | 8/10 | Open | +| 31 | No max file size | **YES** | 7/10 | Open | +| 32 | Whitespace changes not distinguished | **PARTIAL** | 6/10 | Optional feature | + +**Totals**: 24 real bugs, 3 false positives (#4, #17, #29), 5 partial/design (#3, #13, #15, #32, #4), 1 fixed (#8) + +--- + +## CRITICAL BUGS + +### 1. Two Conflicting Line-Counting Methods + +**Real bug: YES — Confidence: 9/10** + +**Impact**: Line count badges in file tree may not match actual hunks in editor. + +**Details**: +- `ChangeExtractorService` (`src/main/services/team/ChangeExtractorService.ts:463-473`) uses `diffLines()` — semantic line diffing +- `MemberStatsComputer` (`src/main/services/team/MemberStatsComputer.ts:193-196`) uses naive `split('\n').length` — `newLines - oldLines` + +Example divergence: +``` +File: 10 lines rewritten completely (same line count) +diffLines(): added=10, removed=10 (correct — all lines changed) +split arithmetic: added=0, removed=0 (wrong — same line count) +``` + +**Best fix**: Create unified line-counting utility using `diffLines()` as source of truth. Replace `MemberStatsComputer`'s arithmetic with shared utility. +**Risk**: Line count numbers will change post-fix; must test edge cases. + +### 2. Write Operations Never Count Removals + +**Real bug: YES — Confidence: 10/10** + +**Location**: `MemberStatsComputer.ts:204-214` + +```typescript +if (toolName === 'Write') { + const writeContent = typeof input.content === 'string' ? input.content : ''; + if (writeContent) { + const fileAdded = writeContent.split('\n').length; + linesAdded += fileAdded; + addFileLines(input.file_path, fileAdded, 0); // Always 0 removals! + } +} +``` + +**Impact**: Write replacing 100-line file with 50-line file shows `+50 / -0` instead of accurate counts. + +**Best fix**: Use `FileContentResolver` to access original state before Write, calculate true delta. +**Risk**: Requires coordination with FileContentResolver; may introduce coupling. + +### 3. Trailing Newline Off-by-One + +**Real bug: PARTIAL — Confidence: 6/10** + +Resolves automatically when #1 is fixed (migration to `diffLines()`). Within MemberStatsComputer's own logic, the delta arithmetic is roughly self-consistent (both old and new over-count by 1, so the difference is correct). The issue is only visible when comparing MemberStatsComputer output against ChangeExtractorService output. + +### 4. FileContentResolver Overwrites Stats + +**Real bug: PARTIAL (design issue) — Confidence: 5/10** + +`FileContentResolver.getFileContent()` recalculates stats from full content using `diffLines()`, overwriting input stats. This is actually MORE ACCURATE than snippet-based counts. The "overwrite" is intentional improvement, not a bug. The inconsistency is that file tree badges (pre-CM load) use snippet counts, while CodeMirror view uses recalculated counts. + +**Verdict**: Not a code bug. Design choice with minor visual inconsistency during loading. + +### 5. `computeHunkIndexAtPos` Returns 0 as Fallback + +**Real bug: YES — Confidence: 10/10** + +**Location**: `CodeMirrorDiffView.tsx:129-143` + +```typescript +function computeHunkIndexAtPos(state: EditorState, pos: number): number { + const chunks = getChunks(state); + if (!chunks) return 0; + let index = 0; + for (const chunk of chunks.chunks) { + if (pos >= chunk.fromB && pos <= chunk.toB) { + return index; + } + index++; + } + return 0; // ← Always returns first hunk if no match! +} +``` + +**Impact**: Clicking Accept/Reject when cursor is between hunks applies action to the FIRST hunk, not the nearest one. Confirmed: callers at lines 416-420 and 432-435 trust this value unconditionally. + +**Best fix**: Find nearest chunk by minimum distance: `Math.min(|pos - chunk.fromB|, |pos - chunk.toB|)` for each chunk, return index of nearest. +**Alternative**: Return -1 for "no match" and require caller handling. +**Risk**: Need tie-breaking rule when cursor is equidistant from two chunks. + +### 6. Hunk Index ≠ Snippet Index (False 1:1 Assumption) + +**Real bug: YES — Confidence: 9/10** + +**Location**: `ReviewApplierService.ts:337-342` + +```typescript +const snippetsToReject = hunkIndices + .filter((idx) => idx >= 0 && idx < validSnippets.length) + .map((idx) => validSnippets[idx]); +``` + +**Problem**: Assumes hunk #N corresponds to snippet #N. But: +- Multiple Edit calls can merge into one hunk in structuredPatch +- One Write call can produce multiple hunks +- MultiEdit creates 1 snippet with multiple logical changes + +**Best fix**: Build hunk-to-snippet mapping using position matching. For each hunk, find snippets whose newString appears in that hunk region. Store `hunkIndex -> Set`. +**Risk**: Complex implementation, requires re-running diff analysis. + +### 7. Snippet Rejection via indexOf — Duplicate Content Bug + +**Real bug: YES — Confidence: 10/10** + +**Location**: `ReviewApplierService.ts:353` + +```typescript +const pos = content.indexOf(snippet.newString); +``` + +`indexOf()` finds FIRST occurrence only. If identical code patterns exist elsewhere in the file, rejection corrupts the wrong section. + +**Best fix**: Position-aware matching: calculate approximate line/column of original edit, search for newString near that position (±5 lines tolerance), require context match. +**Risk**: More complex logic, false negatives if context too strict. + +### 8. Skeleton Flash After File Save — FIXED + +**Location**: `changeReviewSlice.ts:649-666` + +After saving, `fileContents[filePath]` was deleted from cache, causing `hasContent = false` → skeleton placeholder shown until lazy re-fetch completes. + +**Fix applied**: Instead of deleting, update `modifiedFullContent` with saved content in-place. `contentSource` set to `'disk-current'`. + +--- + +## HIGH PRIORITY BUGS + +### 9. CRLF Line Endings → False Diffs + +**Real bug: YES — Confidence: 9/10** + +**Location**: `DiffViewer.tsx:297` + +```typescript +const oldLines = oldString.split('\n'); +``` + +Windows files with `\r\n` leave trailing `\r` on each line. Every line shows as "changed" even if content is identical. + +**Fix**: Use `split(/\r?\n/)` or normalize before diffing. +**Risk**: Very low. Standard regex, no side effects. + +### 10. OOM on Large Files (DiffViewer LCS) + +**Real bug: YES — Confidence: 8/10** + +**Location**: `DiffViewer.tsx:50-68` + +LCS algorithm is O(m×n) space. Two 5000-line files = 25M matrix entries ≈ 100MB RAM. +No safeguards, no fallback for large files. + +**Best fix**: Add size check: if `m * n > MAX_CELLS` (e.g., 1M), fallback to `diffLines()` from npm `diff` package. +**Risk**: Fallback produces different visual output (semantic vs LCS). Need to test. + +### 11. Race Condition: File Disk State vs Cache + +**Real bug: YES — Confidence: 8/10** + +Stats and hunks come from SEPARATE sources: +- Stats: JSONL tool_use blocks (snapshot at parse time) +- Hunks: current file on disk (read at view time) + +3-minute cache TTL on both `ChangeExtractorService` and `FileContentResolver`. +If file changes on disk between fetches, stats and hunks desynchronize. + +**No cache invalidation** on FileWatcher events → caches stay stale until TTL expires. + +**Best fix**: Hook FileWatcher to evict caches when files change. Or reduce TTL to 30s. +**Risk**: Must ensure invalidation doesn't create new race conditions. + +### 12. Empty String Handling Inconsistency + +**Real bug: YES — Confidence: 7/10** + +**Location**: `MemberStatsComputer.ts:193` + +Empty string `''` is falsy → returns 0. But `''.split('\n').length === 1`. + +**Resolves with #1** — migrating to `diffLines()` handles this correctly. + +### 13. Bash Line Estimation Covers ~30-40% of Patterns + +**Real bug: PARTIAL — Confidence: 6/10** + +**Location**: `MemberStatsComputer.ts:314-416` + +**Verdict**: Fundamental limitation, not a code bug. The JSONL only stores command strings, not execution output. Without running the shell, accurate counting is impossible. Code comments acknowledge this. Best approach: document limitation in UI with tooltip. + +### 14. Echo Escape Sequence Handling Wrong + +**Real bug: YES — Confidence: 8/10** + +**Location**: `MemberStatsComputer.ts:371` + +```typescript +added += content.split('\\n').length; +``` + +Splits on literal `\\n` in quoted string. But `echo "line1\nline2"` does NOT expand `\n` without `-e` flag. Counter is wrong for standard echo. + +**Best fix**: Check for `-e` flag before splitting on `\\n`. Without `-e`, treat as single line. +**Risk**: Hacky logic, any change may break other cases. Conservative: don't count echo lines at all. + +--- + +## MEDIUM PRIORITY BUGS + +### 15. portionCollapse Line Position Edge Case + +**Real bug: PARTIAL — Confidence: 5/10** + +**Location**: `portionCollapse.ts:140` + +Rare edge case at exact line boundaries. Needs unit tests with edge cases to confirm. +**Verdict**: Low probability, needs testing before fixing. + +### 16. No-Newline-At-End-Of-File Not Shown + +**Real bug: YES — Confidence: 8/10** + +**Location**: `ReviewDiffContent.tsx:46-48` + +```typescript +const lines = part.value.replace(/\n$/, '').split('\n'); +``` + +Strips trailing newline. If original has no final newline but modified adds one, diff shows them as identical. + +**Best fix**: Add visual indicator for no-newline-at-EOF. +**Risk**: Need to update rendering without breaking existing layout. + +### 17. Three-Way Merge Labels Confusing — FALSE POSITIVE + +**Confidence: 9/10 that this is NOT a bug** + +Labels correctly follow diff3 semantics: `<<<<<<< current` = disk state, `>>>>>>> original` = pre-change state. This is standard and correct. The audit was wrong. + +### 18. Zero-Change Files Invisible + +**Real bug: YES — Confidence: 8/10** + +**Location**: `ChangeStatsBadge.tsx` + +```typescript +if (linesAdded === 0 && linesRemoved === 0) return null; +``` + +Files modified with equal adds/removes (e.g., 5 lines rewritten) show no badge. Missing `modified` boolean flag. + +**Best fix**: Add `modified: boolean` flag to `FileChangeSummary`. Show neutral badge for zero-net-change files. +**Risk**: Requires data structure change, but straightforward. + +### 19. Viewed File Threshold Mismatch + +**Real bug: YES — Confidence: 9/10** + +- `FileSectionDiff.tsx`: `threshold: 0.85` +- `CodeMirrorDiffView.tsx`: `threshold: 1.0` + +**Best fix**: Standardize on 0.85 everywhere. One-line change. +**Risk**: None. + +### 20. Missing Dependency Array in useEffect + +**Real bug: YES — Confidence: 10/10** + +**Location**: `FileSectionDiff.tsx:50-56` + +```typescript +useEffect(() => { + if (localEditorViewRef.current) { + onEditorViewReady(file.filePath, localEditorViewRef.current); + } +}); // ← No dependency array! Runs EVERY render. +``` + +**Best fix**: Add `[file.filePath, onEditorViewReady]` dependency array. +**Risk**: Low. Need to ensure `onEditorViewReady` is memoized with useCallback. + +### 21. Hunk Counts Mismatch in UI + +**Real bug: YES — Confidence: 8/10** + +`snippets.length` used as fallback before CodeMirror loads, then replaced by `chunks.length`. User sees progress jump (e.g., "1 of 1" → "1 of 5"). + +**Best fix**: Pre-compute chunk count from `diffLines()` at load time, not from snippet count. +**Risk**: Medium — adds computation step, but improves correctness. + +### 22. Merge Toolbar Off-Screen in Narrow Viewport + +**Real bug: YES — Confidence: 7/10** + +Buttons at `insetInlineEnd: '8px'` get clipped in narrow viewports. + +**Best fix**: CSS-only: use `insetInlineStart` or add `maxWidth: '100%'` overflow handling. +**Risk**: None. CSS-only change. + +### 23. Deleted Files Not Marked + +**Real bug: YES — Confidence: 8/10** + +No `isDeleted` flag in `FileChangeSummary`. Deleted files show as regular changes. + +**Best fix**: Add `isDeleted: boolean` field. Set when original has content and modified is empty. +**Risk**: Medium — requires data structure change. + +### 24. Snippet Reconstruction Returns null for Write-Update + +**Real bug: YES — Confidence: 9/10** + +**Location**: `FileContentResolver.ts:392-394` + +`write-update` can't be reconstructed because JSONL doesn't include `oldString`. Falls back to disk-current which may differ from actual original. + +**Best fix**: Store original content when detecting write-update during extraction. +**Risk**: Medium — requires data structure enrichment. + +### 25. Bash Relative Paths Not Captured + +**Real bug: YES — Confidence: 9/10** + +**Location**: `MemberStatsComputer.ts:373-376` + +Only captures absolute paths (`startsWith('/')`). Misses relative paths. + +**Best fix**: Remove `startsWith('/')` check, just validate non-empty string. +**Risk**: Very low. 1-line fix. + +--- + +## LOW PRIORITY + +### 26. DiffViewer Empty Line Rendering + +**Real bug: YES — Confidence: 7/10** + +```typescript +{line.content || ' '} +``` + +Empty lines show as single space. Can't distinguish from space-only lines. + +**Best fix**: `{line.content ?? ' '}` (only use space if truly undefined). +**Risk**: None. 1-line fix. + +### 27. No Keyboard Navigation in File Tree + +**Real bug: YES — Confidence: 8/10** + +Mouse-only navigation. No WAI-ARIA tree widget support. + +**Verdict**: UX feature, ~150 LOC. Not a correctness bug. + +### 28. Folder Collapse State Not Persisted + +**Real bug: YES — Confidence: 9/10** + +State lost on dialog close. Uses `useState` with fresh `Set()`. + +**Best fix**: Persist to localStorage or Zustand store. +**Risk**: Low-medium. localStorage has size limits but file tree data is tiny. + +### 29. No Diff Stats Summary at Top — FALSE POSITIVE + +**Confidence: 3/10 that this is a bug** + +Missing feature, not a bug. File-by-file stats are visible. Aggregate summary would be nice UX but isn't a correctness issue. + +### 30. Binary Files Not Detected + +**Real bug: YES — Confidence: 8/10** + +`diffLines()` called on binary content without check. Can cause corrupted display. + +**Best fix**: Check for null bytes (`content.includes('\0')`) before diffing. Skip binary files. +**Risk**: Very low. Simple guard. + +### 31. No Maximum File Size Handling + +**Real bug: YES — Confidence: 7/10** + +No progress indicator for large files. Browser freezes for 2-3s on 1MB+ files. + +**Best fix**: Add size check, show "file too large" fallback for >5MB. +**Risk**: Low. Graceful degradation. + +### 32. Whitespace-Only Changes Not Distinguished + +**Real bug: PARTIAL — Confidence: 6/10** + +No visual distinction between content vs whitespace-only changes. + +**Verdict**: Optional enhancement. Add `ignoreWhitespace` toggle. Not a correctness bug. + +--- + +## Race Condition Severity Matrix + +| Scenario | Probability | Severity | +|----------|-------------|----------| +| Disk modification between stat cache and hunk fetch | Medium | High | +| File deletion after stats cached | Low-Medium | High | +| JSONL appended while parsing | Low | Medium | +| Git repo state change (checkout, rebase) | Low | Medium | +| Snippet reconstruction chain broken | Medium | High | + +--- + +## Fix Categories + +### Safe to Fix Now (isolated, low-risk, clear approach) +- **#5**: computeHunkIndexAtPos → nearest hunk (local function change) +- **#9**: CRLF normalization `split(/\r?\n/)` (1-line regex change) +- **#19**: Threshold 0.85 vs 1.0 → standardize (1-line constant) +- **#20**: useEffect dependency array (1-line addition) +- **#25**: Bash relative paths (remove `startsWith('/')` check) +- **#26**: Empty line rendering (`||` → `??`) + +### Need More Research Before Fixing (multi-file, complex, risky) +- **#1**: Unify line counting (affects 3 services, all consumers) +- **#2**: Write removals (needs original file content during stats) +- **#6**: Hunk↔snippet mapping (algorithm redesign) +- **#7**: indexOf → position-aware matching (could break existing logic) +- **#10**: OOM safeguard (need to test fallback visual consistency) +- **#11**: Cache invalidation (FileWatcher integration complexity) +- **#23**: Deleted files flag (data structure change, migration) +- **#24**: write-update reconstruction (data enrichment needed) + +### Feature Requests / Won't Fix +- **#13**: Bash estimation — fundamental limitation +- **#17**: Three-way labels — NOT a bug +- **#27**: Keyboard nav — UX feature +- **#29**: Stats summary — UX feature +- **#32**: Whitespace toggle — optional enhancement + +--- + +## Key Files Reference + +| File | Role | +|------|------| +| `src/main/services/team/ChangeExtractorService.ts` | Parses JSONL, aggregates per-file changes | +| `src/main/services/team/MemberStatsComputer.ts` | Session analytics, line counting | +| `src/main/services/team/FileContentResolver.ts` | Full file content resolution | +| `src/main/services/team/ReviewApplierService.ts` | Apply/reject hunks, save files | +| `src/renderer/components/chat/viewers/DiffViewer.tsx` | Simple LCS diff viewer | +| `src/renderer/components/team/review/CodeMirrorDiffView.tsx` | Advanced CodeMirror merge view | +| `src/renderer/components/team/review/ReviewDiffContent.tsx` | Fallback snippet-based diff | +| `src/renderer/components/team/review/FileSectionDiff.tsx` | Diff section wrapper | +| `src/renderer/components/team/review/FileSectionHeader.tsx` | File header with Save button | +| `src/renderer/components/team/review/ContinuousScrollView.tsx` | Scroll container, skeleton logic | +| `src/renderer/components/team/review/portionCollapse.ts` | Smart collapse of unchanged regions | +| `src/renderer/components/team/review/ReviewFileTree.tsx` | File tree with stats badges | +| `src/renderer/store/slices/changeReviewSlice.ts` | Store: file contents, save, decisions | +| `src/renderer/hooks/useDiffNavigation.ts` | Keyboard navigation between hunks | +| `src/shared/types/review.ts` | Shared types: FileChangeSummary, FileChangeWithContent | diff --git a/docs/research/diff-view-fix-plans.md b/docs/research/diff-view-fix-plans.md new file mode 100644 index 00000000..849f3e06 --- /dev/null +++ b/docs/research/diff-view-fix-plans.md @@ -0,0 +1,587 @@ +# Diff View — Detailed Fix Plans from Deep Research + +Date: 2026-02-26 +Source: 4 parallel research agents analyzing actual source code + +--- + +## Fix #11: Cache TTL 3min → 30sec + +**Confidence: 10/10** +**Effort: 2 lines** + +### Files to Change + +1. `src/main/services/team/ChangeExtractorService.ts:40` +```typescript +// OLD +private readonly CACHE_TTL = 3 * 60 * 1000; // 3 мин + +// NEW +private readonly CACHE_TTL = 30 * 1000; // 30 sec +``` + +2. `src/main/services/team/FileContentResolver.ts:32` +```typescript +// OLD +private readonly cacheTtl = 3 * 60 * 1000; // 3 мин + +// NEW +private readonly cacheTtl = 30 * 1000; // 30 sec +``` + +### Cache Architecture Details + +Both services use `Map` with TTL: +- `ChangeExtractorService`: key = `${teamName}:${memberName}`, stores `AgentChangeSet` + `mtime` + `expiresAt` +- `FileContentResolver`: key = file path, stores `original | modified | source` + `expiresAt` +- `ChangeExtractorService` stores file `mtime` but NEVER uses it for validation +- `FileContentResolver` has `invalidateFile(filePath)` but only called in ONE place: `review.ts:265` after save + +### FileWatcher Coverage + +FileWatcher ALREADY watches the right directories: +- `~/.claude/projects/` (JSONL session files) +- `~/.claude/todos/` (todo JSON files) +- `~/.claude/teams/` (team config files) +- `~/.claude/tasks/` (task JSON files) + +But NO service-level cache invalidation hooks exist beyond the single `invalidateFile()` call. + +### Thundering Herd Risk: NONE + +- Each cache entry expires independently, staggered by client refresh timing +- 30sec = 120 cache refreshes/hour per user, negligible CPU +- Each team member has separate cache key; concurrent misses don't cascade + +### Future Phase (Optional): FileWatcher Integration + +Would require: +1. Add `ChangeExtractorService` and `FileContentResolver` to ServiceContext +2. Wire FileWatcher events to precise cache invalidation +3. Map `${teamName}:${memberName}` cache keys to affected files +4. Complex wiring, not worth it until TTL proves insufficient + +--- + +## Fix #10: OOM Safeguard for DiffViewer LCS + +**Confidence: 9/10** +**Effort: ~30 LOC** + +### Memory Analysis + +LCS matrix: `(m+1) × (n+1)` entries, each number = ~8 bytes in V8: +- 1000×1000 = 1M entries ≈ 8MB ✓ Safe +- 3000×3000 = 9M entries ≈ 72MB ⚠️ Manageable +- 5000×5000 = 25M entries ≈ 200MB ✗ Dangerous +- 10000×10000 = 100M entries ≈ 800MB ✗ OOM + +**Recommended threshold: `MAX_CELLS = 1_000_000`** (~1000×1000 lines) + +### `diffLines()` Return Format + +From npm `diff` package: +```typescript +Array<{ + value: string; // The actual line(s) + newline + count?: number; // Number of lines + added?: boolean; // true = new lines + removed?: boolean; // true = removed lines + // If neither added/removed: unchanged context lines +}> +``` + +### DiffLine Type (DiffViewer) + +```typescript +interface DiffLine { + type: 'removed' | 'added' | 'context'; + content: string; + lineNumber: number; +} +``` + +### Implementation + +**Import to add** (DiffViewer.tsx top): +```typescript +import { diffLines as semanticDiffLines } from 'diff'; +``` + +**New constant**: +```typescript +/** Max LCS matrix cells before falling back to semantic diff. + * 1M cells ≈ 8MB RAM — safe for all platforms. */ +const MAX_LCS_CELLS = 1_000_000; +``` + +**Fallback function**: +```typescript +/** + * Fallback diff using semantic line-diffing from npm `diff` package. + * Used when LCS matrix would exceed memory threshold. + * Output format matches LCS-based generateDiff(). + */ +function generateDiffFallback(oldLines: string[], newLines: string[]): DiffLine[] { + const oldText = oldLines.join('\n'); + const newText = newLines.join('\n'); + const changes = semanticDiffLines(oldText, newText); + + const result: DiffLine[] = []; + let lineNumber = 1; + + for (const change of changes) { + // Split change value into individual lines, removing trailing newline + const changeLines = change.value.replace(/\r?\n$/, '').split(/\r?\n/); + + for (const content of changeLines) { + if (change.added) { + result.push({ type: 'added', content, lineNumber: lineNumber++ }); + } else if (change.removed) { + result.push({ type: 'removed', content, lineNumber: lineNumber++ }); + } else { + result.push({ type: 'context', content, lineNumber: lineNumber++ }); + } + } + } + + return result; +} +``` + +**Modified `generateDiff()`**: +```typescript +function generateDiff(oldLines: string[], newLines: string[]): DiffLine[] { + // Fallback to semantic diffing for large files to prevent OOM + if (oldLines.length * newLines.length > MAX_LCS_CELLS) { + return generateDiffFallback(oldLines, newLines); + } + + // Original LCS-based algorithm + const matrix = computeLCSMatrix(oldLines, newLines); + // ... rest unchanged ... +} +``` + +### Visual Behavior Difference + +| File Pair | Strategy | Visual Quality | +|-----------|----------|---------------| +| < 1000×1000 | LCS | Precise character-level alignment | +| > 1000×1000 | Semantic | Groups consecutive changes differently, but correct | + +The fallback is semantically correct but may group consecutive changes differently. +For most real code diffs, the visual difference is negligible. + +### Precedent in Codebase + +`ReviewDiffContent.tsx` already uses `diffLines()` successfully: +```typescript +const diffResult = diffLines(original ?? '', modified ?? ''); +``` + +--- + +## Fix #1+#2: Unified Line Counting + +**Confidence: 7/10** +**Effort: ~4-6 hours** + +### Current State: 3 Independent Algorithms + +#### MemberStatsComputer (`src/main/services/team/MemberStatsComputer.ts`) + +**Edit** (lines 189-202): +```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; +``` +- WRONG when content changes but line count stays same + +**Write** (lines 204-214): +```typescript +const fileAdded = writeContent.split('\n').length; +linesAdded += fileAdded; +addFileLines(input.file_path, fileAdded, 0); // Always removals = 0! +``` +- NEVER counts removals + +**MultiEdit** (lines 216-229): +- Same pattern as Write: only additions, no removals + +**Bash** (lines 232-243): +- Heuristic `estimateBashLinesChanged()` (~30-40% coverage) + +#### ChangeExtractorService (`src/main/services/team/ChangeExtractorService.ts`) + +**countLines** (lines 463-473): Uses `diffLines()` — CORRECT +**buildTimeline** (lines 426-427): Uses `split('\n').length` — INCONSISTENT with own countLines! + +#### FileContentResolver (`src/main/services/team/FileContentResolver.ts`) + +Lines 141-156: Uses `diffLines()` — CORRECT + +### Who Consumes These Counts + +| Source | Consumer | UI | +|--------|----------|-----| +| MemberStatsComputer | MemberStatsTab.tsx | Session analytics "+X / -Y" | +| ChangeExtractorService | ChangeStatsBadge.tsx | File tree badges | +| ChangeExtractorService | ReviewApplierService.ts | Diff hunks | +| FileContentResolver | CodeMirrorDiffView.tsx | Full file diff display | + +### Proposed Fix + +**Phase 1: Create UnifiedLineCounter** + +```typescript +// src/main/services/team/UnifiedLineCounter.ts +import { diffLines } from 'diff'; + +export class UnifiedLineCounter { + static countLines(oldStr: string, newStr: string): { added: number; removed: number } { + if (!oldStr && !newStr) return { added: 0, removed: 0 }; + const changes = diffLines(oldStr, newStr); + let added = 0; + let removed = 0; + for (const c of changes) { + if (c.added) added += c.count ?? 0; + if (c.removed) removed += c.count ?? 0; + } + return { added, removed }; + } +} +``` + +**Phase 2: Migrate MemberStatsComputer Edit** + +```typescript +// Replace lines 189-202 +const { added: fileAdded, removed: fileRemoved } = UnifiedLineCounter.countLines(oldStr, newStr); +``` + +**Phase 3: Fix Write Operations (Bug #2)** + +Track file creation vs update during JSONL parse: + +```typescript +const filesSeen = new Set(); + +// In Write handler: +if (toolName === 'Write') { + const filePath = typeof input.file_path === 'string' ? input.file_path : ''; + const writeContent = typeof input.content === 'string' ? input.content : ''; + + const isNewFile = !filesSeen.has(filePath); + filesSeen.add(filePath); + + if (writeContent) { + if (isNewFile) { + // New file creation — all lines are additions + const { added } = UnifiedLineCounter.countLines('', writeContent); + linesAdded += added; + if (filePath) addFileLines(filePath, added, 0); + } else { + // File replacement — assume full rewrite (conservative estimate) + const { added } = UnifiedLineCounter.countLines('', writeContent); + linesAdded += added; + linesRemoved += added; // Assume ~same amount removed + if (filePath) addFileLines(filePath, added, added); + } + } +} +``` + +**Phase 4: Fix buildTimeline in ChangeExtractorService** + +```typescript +// Replace lines 426-427 +const { added, removed } = UnifiedLineCounter.countLines(s.oldString, s.newString); +// Use `added` and `removed` instead of split('\n').length arithmetic +``` + +**Phase 5: Keep Bash As-Is** + +Fundamental limitation — command string has no execution output. + +### Risk Assessment + +| Risk | Level | Mitigation | +|------|-------|-----------| +| Write removals estimation inaccurate | HIGH | `filesSeen` Set tracks if file existed before; conservative "full rewrite" estimate | +| Line count numbers change in UI | MEDIUM | Expected — numbers become MORE accurate | +| Historical data shows different numbers | LOW | Accept as one-time correction | +| Circular dependency (MemberStatsComputer → FileContentResolver) | NONE | UnifiedLineCounter is independent utility | + +### Open Questions + +1. **Write-update without oldString**: `filesSeen` approach assumes sequential JSONL parsing. If messages are out of order, may misclassify. Need to verify JSONL ordering. +2. **Performance**: `diffLines()` is heavier than `split().length`. For sessions with 1000+ Edit calls, could add latency. Need to benchmark. +3. **Bash estimation**: Keep as-is or drop entirely? Current tooltip says "Approximate" — may be enough. + +--- + +## Fix #6+#7: Hunk↔Snippet Mapping + indexOf + +**Confidence: 5/10** +**Effort: ~150 LOC** + +### Current Architecture Flow + +``` +CodeMirrorDiffView.tsx (lines 427, 443) + ↓ +computeHunkIndexAtPos(state, pos) → hunkIndex: number + ↓ +onRejectRef.current?.(idx) // onHunkRejected callback + ↓ +IPC: team:applyReviewDecisions + ↓ +ReviewApplierService.rejectHunks(filePath, original, modified, hunkIndices, snippets) + ↓ +trySnippetLevelReject(modified, hunkIndices, snippets) + ↓ +snippetsToReject = hunkIndices.map(idx => validSnippets[idx]) // ← BUG: 1:1 assumption + ↓ +content.indexOf(snippet.newString) // ← BUG: first occurrence only +``` + +### Data Available in Snippets + +From `ChangeExtractorService` (SnippetDiff type): +- `oldString` / `newString` — actual content +- `toolName` — Edit, Write, MultiEdit +- `toolUseId` — unique ID +- `timestamp` — when it happened +- `type` — 'edit' | 'write-new' | 'write-update' | 'multi-edit' +- `isError` — whether tool errored +- `replaceAll` — for Edit with replace_all flag +- **NO line numbers** — this is the core problem + +### Data Available in Hunks + +From `structuredPatch()` (npm `diff` package): +```typescript +interface StructuredPatchHunk { + oldStart: number; // Line number in original (1-based) + oldLines: number; // Line count in original + newStart: number; // Line number in modified (1-based) + newLines: number; // Line count in modified + lines: string[]; // Actual diff lines (+, -, space context) +} +``` + +From CodeMirror's `getChunks()`: +```typescript +chunks: { + fromA: number // Original doc character position + toA: number // Original doc character position + fromB: number // Modified doc character position + toB: number // Modified doc character position +}[] +``` + +### Proposed Fix: 3 Phases + +#### Phase 1: `buildHunkToSnippetMapping()` + +Build explicit mapping using content overlap detection: + +```typescript +private buildHunkToSnippetMapping( + original: string, + modified: string, + hunkIndices: number[], + snippets: SnippetDiff[] +): Map> { + const patch = structuredPatch('file', 'file', original, modified); + if (!patch.hunks || patch.hunks.length === 0) return new Map(); + + const mapping = new Map>(); + + for (const hunkIdx of hunkIndices) { + if (hunkIdx < 0 || hunkIdx >= patch.hunks.length) continue; + const hunk = patch.hunks[hunkIdx]; + const snippetSet = new Set(); + + // Extract added/removed content from hunk + const addedLines = hunk.lines.filter(l => l.startsWith('+')).map(l => l.slice(1)); + const removedLines = hunk.lines.filter(l => l.startsWith('-')).map(l => l.slice(1)); + const addedContent = addedLines.join('\n'); + const removedContent = removedLines.join('\n'); + + for (let sIdx = 0; sIdx < snippets.length; sIdx++) { + const snippet = snippets[sIdx]; + if (snippet.isError) continue; + + const matchesNew = addedContent.includes(snippet.newString); + const matchesOld = removedContent.includes(snippet.oldString); + + if (snippet.type === 'write-new' || snippet.type === 'write-update') { + if (matchesNew) snippetSet.add(sIdx); + } else { + // For edits: require both old AND new match for higher confidence + if (matchesNew && matchesOld) { + snippetSet.add(sIdx); + } else if (matchesNew) { + snippetSet.add(sIdx); // Lower confidence fallback + } + } + } + + mapping.set(hunkIdx, snippetSet); + } + + return mapping; +} +``` + +#### Phase 2: Position-Aware `findSnippetPosition()` + +Replace indexOf with context-aware search: + +```typescript +private findSnippetPosition( + snippet: SnippetDiff, + content: string +): number { + const { newString, oldString } = snippet; + + // Fast path: newString is unique in content + const firstPos = content.indexOf(newString); + if (firstPos === -1) return -1; + + const lastPos = content.lastIndexOf(newString); + if (firstPos === lastPos) return firstPos; // Only one occurrence — safe + + // Multiple occurrences — use oldString context to disambiguate + // Search for each occurrence and check if surrounding context matches oldString + const positions: number[] = []; + let searchStart = 0; + while (true) { + const pos = content.indexOf(newString, searchStart); + if (pos === -1) break; + positions.push(pos); + searchStart = pos + 1; + } + + // For each candidate position, check if oldString context is nearby + if (oldString) { + for (const pos of positions) { + // Look for oldString within ±1000 chars of this position + // (in the original document, oldString would be at roughly the same position) + const nearbyStart = Math.max(0, pos - 1000); + const nearbyEnd = Math.min(content.length, pos + newString.length + 1000); + const nearby = content.substring(nearbyStart, nearbyEnd); + + // If any unique token from oldString appears nearby, this is likely correct + const oldTokens = oldString.split(/\s+/).filter(t => t.length > 3); + const matchScore = oldTokens.filter(t => nearby.includes(t)).length; + + if (matchScore > oldTokens.length * 0.5) { + return pos; // >50% of oldString tokens found nearby + } + } + } + + // Last resort: return first position with warning + return firstPos; +} +``` + +#### Phase 3: Update `trySnippetLevelReject()` Signature + +```typescript +// Pass `original` through to enable mapping and context matching +private trySnippetLevelReject( + modified: string, + hunkIndices: number[], + snippets: SnippetDiff[], + original: string // NEW parameter +): RejectResult | null { + const validSnippets = snippets.filter(s => !s.isError); + if (validSnippets.length === 0) return null; + + // NEW: Build mapping instead of assuming 1:1 + const hunkToSnippets = this.buildHunkToSnippetMapping( + original, modified, hunkIndices, validSnippets + ); + + // Collect all snippets to reject + const snippetIndices = new Set(); + for (const indices of hunkToSnippets.values()) { + indices.forEach(idx => snippetIndices.add(idx)); + } + + const snippetsToReject = Array.from(snippetIndices) + .map(idx => validSnippets[idx]) + .filter(Boolean); + + // NEW: Position-aware matching + const positioned = snippetsToReject + .map(snippet => ({ + snippet, + pos: this.findSnippetPosition(snippet, modified) + })) + .filter(item => item.pos !== -1) + .sort((a, b) => b.pos - a.pos); // Descending for safe replacement + + if (positioned.length !== snippetsToReject.length) { + return null; // Fallback to hunk-level + } + + let content = modified; + for (const { snippet, pos } of positioned) { + if (snippet.type === 'write-new') continue; + if (snippet.replaceAll) { + content = content.split(snippet.newString).join(snippet.oldString); + } else { + content = content.substring(0, pos) + snippet.oldString + + content.substring(pos + snippet.newString.length); + } + } + + return { success: true, newContent: content, hadConflicts: false }; +} +``` + +### Edge Cases & Concerns + +| Case | Current Behavior | After Fix | +|------|------------------|-----------| +| Multiple Edit → 1 Hunk | Assumes hunkIdx = snippetIdx (WRONG) | Content overlap mapping (CORRECT) | +| 1 Write → 2 Hunks | Maps to wrong snippet | Maps via content match | +| Duplicate code in file | Corrupts first occurrence | Context-aware disambiguation | +| Short snippet (1 line) | indexOf works | May still match wrong occurrence if context is ambiguous | +| No oldString context | N/A | Falls back to first indexOf match (same as before) | +| replaceAll snippets | Works (replaces all) | Still works (replaceAll logic unchanged) | + +### Open Questions + +1. **Short snippets**: If `newString` is `"return true;"` and appears 10 times — context matching may fail. Need hunk line range to narrow search. +2. **Performance**: `structuredPatch()` is called again in `buildHunkToSnippetMapping` (already called in `rejectHunks`). Should cache the patch result. +3. **MultiEdit**: Creates 1 snippet but may affect multiple non-contiguous regions. `buildHunkToSnippetMapping` should handle this but needs testing. +4. **Overlapping snippets**: Two snippets touching the same line range. Position-aware replacement from end (descending sort) handles this, but still fragile. +5. **`original` parameter**: Need to thread it through from all callers: `rejectHunks()`, `previewReject()`, `acceptHunks()`. + +### Why Confidence is Only 5/10 + +The core issue is that **snippets have NO line numbers**. All matching is content-based (heuristic). For short/common snippets, disambiguation may fail. A truly robust fix would require: +1. Adding line number tracking to `SnippetDiff` during extraction +2. Or using `structuredPatch` bidirectionally to map hunks to file regions + +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 | diff --git a/src/main/index.ts b/src/main/index.ts index 2a61a7e2..df66467e 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -33,7 +33,9 @@ import { existsSync } from 'fs'; import { join } from 'path'; import { initializeIpcHandlers, removeIpcHandlers } from './ipc/handlers'; +import { showTeamNativeNotification } from './ipc/teams'; import { HttpServer } from './services/infrastructure/HttpServer'; +import { TeamInboxReader } from './services/team/TeamInboxReader'; import { getProjectsBasePath, getTodosBasePath } from './utils/pathDecoder'; import { CliInstallerService, @@ -56,6 +58,74 @@ import type { TeamChangeEvent } from '@shared/types'; const logger = createLogger('App'); +// --- Team message notification tracking --- +const teamInboxReader = new TeamInboxReader(); +/** Track last-seen message count per inbox file to detect new messages. */ +const inboxMessageCounts = new Map(); +/** Debounce per-inbox to avoid flooding during batch writes. */ +const inboxNotifyTimers = new Map>(); +const INBOX_NOTIFY_DEBOUNCE_MS = 500; +/** Messages sent from our UI (user_sent) — suppress notifications for these. */ +const suppressedSources = new Set(['user_sent']); + +async function notifyNewInboxMessages(teamName: string, detail: string): Promise { + // detail is like "inboxes/carol.json" — extract member name + const match = /^inboxes\/(.+)\.json$/.exec(detail); + if (!match) return; + const memberName = match[1]; + const key = `${teamName}:${memberName}`; + + try { + const messages = await teamInboxReader.getMessagesFor(teamName, memberName); + const prevCount = inboxMessageCounts.get(key) ?? 0; + + if (prevCount === 0) { + // First load — seed count, don't notify + inboxMessageCounts.set(key, messages.length); + return; + } + + if (messages.length <= prevCount) { + inboxMessageCounts.set(key, messages.length); + return; + } + + // Messages are sorted newest-first, so new ones are at the beginning + 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 + } + + for (const msg of newMessages) { + // 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}`, + body: msg.text, + }); + } + } catch (error) { + logger.warn(`Failed to check inbox messages for ${key}:`, error); + } +} + // Window icon path for non-mac platforms. const getWindowIconPath = (): string | undefined => { const isDev = process.env.NODE_ENV === 'development'; @@ -163,15 +233,33 @@ function wireFileWatcherEvents(context: ServiceContext): void { } httpServer?.broadcast('team-change', event); - // Auto-relay direct messages to live team lead process (no UI dependency). + // Process inbox change events — relay to lead + native OS notifications. try { if (!event || typeof event !== 'object') return; - const row = event as { type?: unknown; teamName?: unknown }; + const row = event as { type?: unknown; teamName?: unknown; detail?: unknown }; if (row.type !== 'inbox') return; if (typeof row.teamName !== 'string' || row.teamName.trim().length === 0) return; const teamName = row.teamName.trim(); - if (!teamProvisioningService.isTeamAlive(teamName)) return; - void teamProvisioningService.relayLeadInboxMessages(teamName).catch(() => undefined); + const detail = typeof row.detail === 'string' ? row.detail : ''; + + // Auto-relay direct messages to live team lead process (no UI dependency). + if (teamProvisioningService.isTeamAlive(teamName)) { + void teamProvisioningService.relayLeadInboxMessages(teamName).catch(() => undefined); + } + + // Show native OS notification for new inbox messages (debounced per inbox). + if (detail.startsWith('inboxes/')) { + const timerKey = `${teamName}:${detail}`; + const existing = inboxNotifyTimers.get(timerKey); + if (existing) clearTimeout(existing); + inboxNotifyTimers.set( + timerKey, + setTimeout(() => { + inboxNotifyTimers.delete(timerKey); + void notifyNewInboxMessages(teamName, detail).catch(() => undefined); + }, INBOX_NOTIFY_DEBOUNCE_MS) + ); + } } catch { // ignore } diff --git a/src/main/ipc/teams.ts b/src/main/ipc/teams.ts index 49a8b352..df419c15 100644 --- a/src/main/ipc/teams.ts +++ b/src/main/ipc/teams.ts @@ -33,6 +33,7 @@ import { TEAM_RESTORE_TASK, TEAM_SEND_MESSAGE, TEAM_SET_TASK_CLARIFICATION, + TEAM_SHOW_MESSAGE_NOTIFICATION, TEAM_SOFT_DELETE_TASK, TEAM_START_TASK, TEAM_STOP, @@ -47,10 +48,11 @@ import { import { KANBAN_COLUMN_IDS } from '@shared/constants/kanban'; import { createLogger } from '@shared/utils/logger'; import { isRateLimitMessage } from '@shared/utils/rateLimitDetector'; -import { type IpcMain, type IpcMainInvokeEvent } from 'electron'; +import { BrowserWindow, type IpcMain, type IpcMainInvokeEvent, Notification } from 'electron'; import * as fs from 'fs'; import * as path from 'path'; +import { ConfigManager } from '../services/infrastructure/ConfigManager'; import { NotificationManager } from '../services/infrastructure/NotificationManager'; import { gitIdentityResolver } from '../services/parsing/GitIdentityResolver'; @@ -88,6 +90,7 @@ import type { TeamData, TeamLaunchRequest, TeamLaunchResponse, + TeamMessageNotificationData, TeamProvisioningPrepareResult, TeamProvisioningProgress, TeamSummary, @@ -209,6 +212,7 @@ export function registerTeamHandlers(ipcMain: IpcMain): void { ipcMain.handle(TEAM_RESTORE_TASK, handleRestoreTask); ipcMain.handle(TEAM_GET_DELETED_TASKS, handleGetDeletedTasks); ipcMain.handle(TEAM_SET_TASK_CLARIFICATION, handleSetTaskClarification); + ipcMain.handle(TEAM_SHOW_MESSAGE_NOTIFICATION, handleShowMessageNotification); logger.info('Team handlers registered'); } @@ -253,6 +257,7 @@ export function removeTeamHandlers(ipcMain: IpcMain): void { ipcMain.removeHandler(TEAM_RESTORE_TASK); ipcMain.removeHandler(TEAM_GET_DELETED_TASKS); ipcMain.removeHandler(TEAM_SET_TASK_CLARIFICATION); + ipcMain.removeHandler(TEAM_SHOW_MESSAGE_NOTIFICATION); } function getTeamDataService(): TeamDataService { @@ -1594,6 +1599,67 @@ async function handleKillProcess( }); } +async function handleShowMessageNotification( + _event: IpcMainInvokeEvent, + data: unknown +): Promise> { + if (!data || typeof data !== 'object') { + return { success: false, error: 'Invalid notification data' }; + } + const d = data as TeamMessageNotificationData; + if (!d.teamDisplayName || !d.from || !d.body) { + return { success: false, error: 'Missing required fields (teamDisplayName, from, body)' }; + } + + showTeamNativeNotification({ + title: d.teamDisplayName, + subtitle: d.summary ?? `${d.from} → ${d.to ?? 'team'}`, + body: d.body, + }); + return { success: true, data: undefined }; +} + +/** + * Show a native OS notification for a team event. + * Respects user's notification settings (enabled, snoozed). + * Cross-platform: macOS, Linux, Windows via Electron Notification API. + */ +export function showTeamNativeNotification(opts: { + title: string; + subtitle?: string; + body: string; +}): void { + const config = ConfigManager.getInstance().getConfig(); + if (!config.notifications.enabled) return; + if (config.notifications.snoozedUntil && Date.now() < config.notifications.snoozedUntil) return; + + if ( + typeof Notification === 'undefined' || + typeof Notification.isSupported !== 'function' || + !Notification.isSupported() + ) { + return; + } + + const notification = new Notification({ + title: opts.title, + subtitle: opts.subtitle, + body: opts.body.slice(0, 300), + sound: config.notifications.soundEnabled ? 'default' : undefined, + }); + + notification.on('click', () => { + const windows = BrowserWindow.getAllWindows(); + const mainWin = windows[0]; + if (mainWin && !mainWin.isDestroyed()) { + mainWin.show(); + mainWin.focus(); + } + }); + + notification.show(); +} + async function handleAddTaskComment( _event: IpcMainInvokeEvent, teamName: unknown, diff --git a/src/main/services/team/ChangeExtractorService.ts b/src/main/services/team/ChangeExtractorService.ts index 90c81634..360c60bd 100644 --- a/src/main/services/team/ChangeExtractorService.ts +++ b/src/main/services/team/ChangeExtractorService.ts @@ -37,7 +37,7 @@ interface LogFileRef { export class ChangeExtractorService { private cache = new Map(); - private readonly CACHE_TTL = 3 * 60 * 1000; // 3 мин + private readonly CACHE_TTL = 30 * 1000; // 30 сек — shorter TTL to reduce stale data risk constructor( private readonly logsFinder: TeamMemberLogsFinder, diff --git a/src/main/services/team/FileContentResolver.ts b/src/main/services/team/FileContentResolver.ts index a525a3fb..4b1f86fb 100644 --- a/src/main/services/team/FileContentResolver.ts +++ b/src/main/services/team/FileContentResolver.ts @@ -29,7 +29,7 @@ interface ContentCacheEntry { */ export class FileContentResolver { private cache = new Map(); - private readonly cacheTtl = 3 * 60 * 1000; // 3 мин (same as ChangeExtractorService) + private readonly cacheTtl = 30 * 1000; // 30 сек — shorter TTL to reduce stale data risk constructor( private readonly logsFinder: TeamMemberLogsFinder, diff --git a/src/main/services/team/MemberStatsComputer.ts b/src/main/services/team/MemberStatsComputer.ts index 89b54572..11e5f90b 100644 --- a/src/main/services/team/MemberStatsComputer.ts +++ b/src/main/services/team/MemberStatsComputer.ts @@ -371,7 +371,7 @@ export function estimateBashLinesChanged(command: string): BashLinesResult { added += content.split('\\n').length; } const filePath = echoMatch[3]; - if (filePath?.startsWith('/')) { + if (filePath?.trim()) { files.push(filePath); } } diff --git a/src/main/services/team/TeamProvisioningService.ts b/src/main/services/team/TeamProvisioningService.ts index 281be120..705c2c0b 100644 --- a/src/main/services/team/TeamProvisioningService.ts +++ b/src/main/services/team/TeamProvisioningService.ts @@ -306,16 +306,16 @@ function buildTaskStatusProtocol(teamName: string): string { - Typical flow: a) Owner finishes work on #X → task complete #X b) Reviewer accepts → review approve #X -10. CLARIFICATION PROTOCOL (IMPORTANT): - When you are blocked and need information to continue a task: - a) Do BOTH of these steps: - 1) Send a message to your team lead via SendMessage explaining what you need. - 2) Set the clarification flag on the task: - node "$HOME/.claude/tools/teamctl.js" --team "${teamName}" task set-clarification lead --from "" - b) The flag is auto-cleared when the lead adds a task comment on your task. +10. CLARIFICATION PROTOCOL (CRITICAL — MANDATORY): + When you are blocked and need information to continue a task, you MUST do BOTH steps below — skipping the Bash command breaks the task board: + a) STEP 1 — FIRST, set the clarification flag via Bash (this updates the task board): + node "$HOME/.claude/tools/teamctl.js" --team "${teamName}" task set-clarification lead --from "" + b) STEP 2 — THEN, send a message to your team lead via SendMessage explaining what you need. + IMPORTANT: Always run the Bash command BEFORE sending the message. The flag is what makes the task board show "needs clarification" — without it, your request is invisible on the board. + c) The flag is auto-cleared when the lead adds a task comment on your task. If the lead replies via SendMessage instead, clear the flag yourself once you have the answer: node "$HOME/.claude/tools/teamctl.js" --team "${teamName}" task set-clarification clear --from "" - c) Do NOT set clarification to "user" yourself — only the team lead escalates to the user. + d) Do NOT set clarification to "user" yourself — only the team lead escalates to the user. Failure to follow this protocol means the task board will show incorrect status.`); } @@ -347,12 +347,16 @@ function buildTeamCtlOpsInstructions(teamName: string, leadName: string): string `Notification policy:`, `- The --notify flag sends the assignment to the member automatically, so do NOT send a separate SendMessage for the same task.`, ``, - `Clarification handling:`, + `Clarification handling (CRITICAL — MANDATORY for correct task board state):`, `- When a teammate needs clarification (needsClarification: "lead"), reply via task comment (preferred — auto-clears the flag) or SendMessage.`, `- If you reply via SendMessage instead of task comment, also clear the flag manually:`, ` node "$HOME/.claude/tools/teamctl.js" --team "${teamName}" task set-clarification clear --from "${leadName}"`, - `- If you cannot answer and the user needs to decide, escalate to "user":`, - ` node "$HOME/.claude/tools/teamctl.js" --team "${teamName}" task set-clarification user --from "${leadName}"`, + `- If you cannot answer and the user needs to decide — ESCALATION PROTOCOL:`, + ` 1) FIRST, set the flag to "user" via Bash (this updates the task board):`, + ` node "$HOME/.claude/tools/teamctl.js" --team "${teamName}" task set-clarification user --from "${leadName}"`, + ` 2) THEN, send a message to "user" explaining the question.`, + ` 3) THEN, reply to the teammate telling them to wait.`, + ` IMPORTANT: Always run the Bash command BEFORE sending messages. Without the flag, the task board won't show that the task is blocked waiting for user input.`, ].join('\n') ); } diff --git a/src/preload/constants/ipcChannels.ts b/src/preload/constants/ipcChannels.ts index 322b6e3d..5e8df42d 100644 --- a/src/preload/constants/ipcChannels.ts +++ b/src/preload/constants/ipcChannels.ts @@ -309,6 +309,9 @@ export const TEAM_GET_DELETED_TASKS = 'team:getDeletedTasks'; /** Set needsClarification flag on a task */ export const TEAM_SET_TASK_CLARIFICATION = 'team:setTaskClarification'; +/** Show native OS notification for a team message */ +export const TEAM_SHOW_MESSAGE_NOTIFICATION = 'team:showMessageNotification'; + // ============================================================================= // CLI Installer API Channels // ============================================================================= diff --git a/src/preload/index.ts b/src/preload/index.ts index 7c14a7bb..1ba04287 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -69,6 +69,7 @@ import { TEAM_RESTORE_TASK, TEAM_SEND_MESSAGE, TEAM_SET_TASK_CLARIFICATION, + TEAM_SHOW_MESSAGE_NOTIFICATION, TEAM_SOFT_DELETE_TASK, TEAM_START_TASK, TEAM_STOP, @@ -161,6 +162,7 @@ import type { TeamData, TeamLaunchRequest, TeamLaunchResponse, + TeamMessageNotificationData, TeamProvisioningPrepareResult, TeamProvisioningProgress, TeamSummary, @@ -710,6 +712,9 @@ const electronAPI: ElectronAPI = { ) => { return invokeIpcWithResult(TEAM_SET_TASK_CLARIFICATION, teamName, taskId, value); }, + showMessageNotification: async (data: TeamMessageNotificationData) => { + return invokeIpcWithResult(TEAM_SHOW_MESSAGE_NOTIFICATION, data); + }, onTeamChange: (callback: (event: unknown, data: TeamChangeEvent) => void): (() => void) => { ipcRenderer.on( TEAM_CHANGE, diff --git a/src/renderer/api/httpClient.ts b/src/renderer/api/httpClient.ts index a0b19594..6c1ae5e5 100644 --- a/src/renderer/api/httpClient.ts +++ b/src/renderer/api/httpClient.ts @@ -796,6 +796,9 @@ export class HttpAPIClient implements ElectronAPI { ): Promise => { // Not available via HTTP client — no-op }, + showMessageNotification: async (): Promise => { + // Not available via HTTP client — native notifications require Electron + }, onTeamChange: (callback: (event: unknown, data: TeamChangeEvent) => void): (() => void) => { return this.addEventListener('team-change', (data: unknown) => callback(null, data as TeamChangeEvent) diff --git a/src/renderer/components/chat/viewers/DiffViewer.tsx b/src/renderer/components/chat/viewers/DiffViewer.tsx index 719e4395..edf6a1f0 100644 --- a/src/renderer/components/chat/viewers/DiffViewer.tsx +++ b/src/renderer/components/chat/viewers/DiffViewer.tsx @@ -20,6 +20,7 @@ import { } from '@renderer/constants/cssVariables'; import { getBaseName } from '@renderer/utils/pathUtils'; import { formatTokens } from '@shared/utils/tokenFormatting'; +import { diffLines as semanticDiffLines } from 'diff'; import { Pencil } from 'lucide-react'; // ============================================================================= @@ -41,9 +42,41 @@ interface DiffLine { } // ============================================================================= -// Diff Algorithm (LCS-based) +// Diff Algorithm (LCS-based, with semantic fallback for large files) // ============================================================================= +/** Max LCS matrix cells before falling back to semantic diff. + * 1M cells ≈ 8MB RAM — safe for all platforms. */ +const MAX_LCS_CELLS = 1_000_000; + +/** + * Fallback diff using semantic line-diffing from npm `diff` package. + * Used when LCS matrix would exceed memory threshold. + */ +function generateDiffFallback(oldLines: string[], newLines: string[]): DiffLine[] { + const oldText = oldLines.join('\n'); + const newText = newLines.join('\n'); + const changes = semanticDiffLines(oldText, newText); + + const result: DiffLine[] = []; + let lineNumber = 1; + + for (const change of changes) { + const changeLines = change.value.replace(/\r?\n$/, '').split(/\r?\n/); + for (const content of changeLines) { + if (change.added) { + result.push({ type: 'added', content, lineNumber: lineNumber++ }); + } else if (change.removed) { + result.push({ type: 'removed', content, lineNumber: lineNumber++ }); + } else { + result.push({ type: 'context', content, lineNumber: lineNumber++ }); + } + } + } + + return result; +} + /** * Computes the Longest Common Subsequence matrix for two arrays of strings. */ @@ -69,8 +102,13 @@ function computeLCSMatrix(oldLines: string[], newLines: string[]): number[][] { /** * Backtrack through LCS matrix to generate diff lines. + * Falls back to semantic diffing for large files to prevent OOM. */ function generateDiff(oldLines: string[], newLines: string[]): DiffLine[] { + if (oldLines.length * newLines.length > MAX_LCS_CELLS) { + return generateDiffFallback(oldLines, newLines); + } + const matrix = computeLCSMatrix(oldLines, newLines); const result: DiffLine[] = []; @@ -276,7 +314,7 @@ const DiffLineRow: React.FC = ({ line }): React.JSX.Element => {/* Content */} - {line.content || ' '} + {line.content ?? ' '} ); @@ -294,8 +332,8 @@ export const DiffViewer: React.FC = ({ tokenCount, }): React.JSX.Element => { // Compute diff - const oldLines = oldString.split('\n'); - const newLines = newString.split('\n'); + const oldLines = oldString.split(/\r?\n/); + const newLines = newString.split(/\r?\n/); const diffLines = generateDiff(oldLines, newLines); const stats = computeStats(diffLines); diff --git a/src/renderer/components/team/review/CodeMirrorDiffView.tsx b/src/renderer/components/team/review/CodeMirrorDiffView.tsx index 6ded2f1c..67809bac 100644 --- a/src/renderer/components/team/review/CodeMirrorDiffView.tsx +++ b/src/renderer/components/team/review/CodeMirrorDiffView.tsx @@ -125,21 +125,31 @@ function getAsyncLanguageDesc(fileName: string): LanguageDescription | null { return LanguageDescription.matchFilename(languages, fileName); } -/** Compute hunk index for the chunk at a given position (B-side / modified doc) */ +/** Compute hunk index for the chunk at a given position (B-side / modified doc). + * If the position falls inside a chunk, returns that chunk's index. + * Otherwise returns the nearest chunk by distance (avoids defaulting to 0). */ function computeHunkIndexAtPos(state: EditorState, pos: number): number { const chunks = getChunks(state); - if (!chunks) return 0; + if (!chunks || chunks.chunks.length === 0) return 0; + + let nearestIndex = 0; + let nearestDist = Infinity; let index = 0; for (const chunk of chunks.chunks) { - // Only check B-side (modified doc) — editor positions correspond to B-side in unified view. - // A-side (fromA/toA) positions refer to the original document and are not editor positions. + // Exact match — position is inside this chunk if (pos >= chunk.fromB && pos <= chunk.toB) { return index; } + // Track nearest chunk for fallback + const dist = Math.min(Math.abs(pos - chunk.fromB), Math.abs(pos - chunk.toB)); + if (dist < nearestDist) { + nearestDist = dist; + nearestIndex = index; + } index++; } - return 0; + return nearestIndex; } /** Custom dark theme for diff view using CSS variables */ @@ -773,7 +783,7 @@ export const CodeMirrorDiffView = ({ } } }, - { threshold: 1.0 } + { threshold: 0.85 } ); observer.observe(endSentinelRef.current); diff --git a/src/renderer/components/team/review/FileSectionDiff.tsx b/src/renderer/components/team/review/FileSectionDiff.tsx index 63a53e9d..1291c121 100644 --- a/src/renderer/components/team/review/FileSectionDiff.tsx +++ b/src/renderer/components/team/review/FileSectionDiff.tsx @@ -53,7 +53,7 @@ export const FileSectionDiff = ({ if (localEditorViewRef.current) { onEditorViewReady(file.filePath, localEditorViewRef.current); } - }); + }, [file.filePath, onEditorViewReady]); // Auto-viewed sentinel observer useEffect(() => { diff --git a/src/renderer/store/slices/changeReviewSlice.ts b/src/renderer/store/slices/changeReviewSlice.ts index ede5a2b8..6831b4c4 100644 --- a/src/renderer/store/slices/changeReviewSlice.ts +++ b/src/renderer/store/slices/changeReviewSlice.ts @@ -655,9 +655,18 @@ export const createChangeReviewSlice: StateCreator { const nextEdited = { ...s.editedContents }; delete nextEdited[filePath]; - // Remove cached file content so next fetchFileContent re-reads from disk + // Update cached content in-place to avoid skeleton flash. + // Replace modifiedFullContent with saved version so CodeMirror + // reflects the new baseline without a full re-fetch cycle. const nextContents = { ...s.fileContents }; - delete nextContents[filePath]; + const existing = nextContents[filePath]; + if (existing) { + nextContents[filePath] = { + ...existing, + modifiedFullContent: content, + contentSource: 'disk-current', + }; + } return { editedContents: nextEdited, fileContents: nextContents, applying: false }; }); } catch (error) { diff --git a/src/renderer/store/slices/teamSlice.ts b/src/renderer/store/slices/teamSlice.ts index 81d90d43..1b6bf04a 100644 --- a/src/renderer/store/slices/teamSlice.ts +++ b/src/renderer/store/slices/teamSlice.ts @@ -29,6 +29,9 @@ import type { import type { StateCreator } from 'zustand'; // --- Clarification notification tracking --- +// Native OS notifications for new inbox messages are handled in main process +// (main/index.ts → notifyNewInboxMessages). This renderer-side tracking only +// handles clarification-specific logic (e.g., marking tasks as needing user input). const notifiedClarificationTaskKeys = new Set(); let isFirstFetchAllTasks = true; @@ -49,11 +52,19 @@ function detectClarificationNotifications(oldTasks: GlobalTask[], newTasks: Glob } function fireClarificationNotification(task: GlobalTask): void { - if (typeof Notification === 'undefined') return; - if (Notification.permission !== 'granted') return; - new Notification('Clarification needed', { - body: `[${task.teamDisplayName}] Task #${task.id}: ${task.subject}`, - }); + // Delegate to main process for native OS notification (cross-platform, no permission needed) + const latestComment = task.comments?.length ? task.comments[task.comments.length - 1] : undefined; + const body = latestComment?.text || task.description || `Task #${task.id}: ${task.subject}`; + + void api.teams + ?.showMessageNotification({ + teamDisplayName: task.teamDisplayName, + from: latestComment?.author || 'team-lead', + to: 'user', + summary: `Clarification needed — Task #${task.id}`, + body, + }) + .catch(() => undefined); } function mapSendMessageError(error: unknown): string { diff --git a/src/shared/types/api.ts b/src/shared/types/api.ts index fbc67a7b..4088f2e6 100644 --- a/src/shared/types/api.ts +++ b/src/shared/types/api.ts @@ -46,6 +46,7 @@ import type { TeamData, TeamLaunchRequest, TeamLaunchResponse, + TeamMessageNotificationData, TeamProvisioningPrepareResult, TeamProvisioningProgress, TeamSummary, @@ -440,6 +441,7 @@ export interface TeamsAPI { softDeleteTask: (teamName: string, taskId: string) => Promise; restoreTask: (teamName: string, taskId: string) => Promise; getDeletedTasks: (teamName: string) => Promise; + showMessageNotification: (data: TeamMessageNotificationData) => Promise; onTeamChange: (callback: (event: unknown, data: TeamChangeEvent) => void) => () => void; onProvisioningProgress: ( callback: (event: unknown, data: TeamProvisioningProgress) => void diff --git a/src/shared/types/team.ts b/src/shared/types/team.ts index 09eeaab0..61d675ba 100644 --- a/src/shared/types/team.ts +++ b/src/shared/types/team.ts @@ -374,3 +374,18 @@ export interface UpdateMemberRoleRequest { name: string; role: string | undefined; } + +/** Data sent from renderer to main for native OS team message notification. */ +export interface TeamMessageNotificationData { + teamDisplayName: string; + /** Who sent the message. */ + from: string; + /** Who received the message (member name or "user"). */ + to?: string; + /** Short summary shown in subtitle. */ + summary?: string; + /** Full message body — displayed as notification body (truncated to 300 chars). */ + body: string; + /** Optional sender color for visual context. */ + color?: string; +} diff --git a/test/main/ipc/teams.test.ts b/test/main/ipc/teams.test.ts index 499c5f2c..23512465 100644 --- a/test/main/ipc/teams.test.ts +++ b/test/main/ipc/teams.test.ts @@ -3,7 +3,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; vi.mock('electron', () => ({ app: { getLocale: vi.fn(() => 'en'), getPath: vi.fn(() => '/tmp') }, - Notification: vi.fn(), + Notification: Object.assign(vi.fn(), { isSupported: vi.fn(() => false) }), + BrowserWindow: { getAllWindows: vi.fn(() => []) }, })); vi.mock('@preload/constants/ipcChannels', () => ({ @@ -45,6 +46,7 @@ vi.mock('@preload/constants/ipcChannels', () => ({ TEAM_SOFT_DELETE_TASK: 'team:softDeleteTask', TEAM_GET_DELETED_TASKS: 'team:getDeletedTasks', TEAM_SET_TASK_CLARIFICATION: 'team:setTaskClarification', + TEAM_SHOW_MESSAGE_NOTIFICATION: 'team:showMessageNotification', TEAM_RESTORE: 'team:restoreTeam', TEAM_PERMANENTLY_DELETE: 'team:permanentlyDeleteTeam', TEAM_RESTORE_TASK: 'team:restoreTask',