From 30b4e74924822713e6aefaf851f2781c4032ba13 Mon Sep 17 00:00:00 2001 From: iliya Date: Fri, 6 Mar 2026 13:00:08 +0200 Subject: [PATCH] feat: enhance file path validation and member stats computation - Refactored isValidFilePath function to improve validation logic, including trimming whitespace and stripping trailing punctuation. - Updated MemberStatsComputer to filter out invalid file paths from perFileStats, ensuring only valid paths are included in the final stats. - Added unit tests for isValidFilePath to cover various edge cases, enhancing reliability of file path handling. - Improved overall code readability and maintainability in MemberStatsComputer. --- src/main/services/team/MemberStatsComputer.ts | 32 +++++-- src/main/services/team/TeamDataService.ts | 41 +++++--- .../services/team/TeamProvisioningService.ts | 95 +++---------------- .../components/layout/SortableTab.tsx | 4 +- .../team/activity/LeadThoughtsGroup.tsx | 51 +++++++--- .../attachments/AttachmentPreviewList.tsx | 15 ++- .../team/messages/MessageComposer.tsx | 2 + src/renderer/hooks/useAttachments.ts | 6 ++ src/renderer/index.css | 16 ++++ test/main/ipc/teams.test.ts | 2 - .../services/team/MemberStatsComputer.test.ts | 45 ++++++++- 11 files changed, 185 insertions(+), 124 deletions(-) diff --git a/src/main/services/team/MemberStatsComputer.ts b/src/main/services/team/MemberStatsComputer.ts index 6443436b..738cd80e 100644 --- a/src/main/services/team/MemberStatsComputer.ts +++ b/src/main/services/team/MemberStatsComputer.ts @@ -9,8 +9,12 @@ import type { FileLineStats, MemberFullStats } from '@shared/types'; const logger = createLogger('Service:MemberStatsComputer'); -function isValidFilePath(value: string): boolean { - return value.length > 0 && value !== 'null' && value !== 'undefined' && value !== 'None'; +const TRAILING_PUNCT = /[;.,]+$/; +const INVALID_NAMES = new Set(['null', 'undefined', 'None', 'false', 'true', '']); + +export function isValidFilePath(value: string): boolean { + const cleaned = value.trim().replace(TRAILING_PUNCT, ''); + return cleaned.length > 1 && !INVALID_NAMES.has(cleaned) && cleaned.includes('/'); } const CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes @@ -73,11 +77,19 @@ export class MemberStatsComputer { .filter(isValidFilePath) .sort((a, b) => a.localeCompare(b)); + // Also filter perFileStats keys to exclude invalid paths + const filteredFileStats: Record = {}; + for (const [fp, fls] of Object.entries(perFileStats)) { + if (isValidFilePath(fp)) { + filteredFileStats[fp] = fls; + } + } + const stats: MemberFullStats = { linesAdded, linesRemoved, filesTouched: validFiles, - fileStats: perFileStats, + fileStats: filteredFileStats, toolUsage, inputTokens, outputTokens, @@ -121,18 +133,24 @@ export class MemberStatsComputer { // Track last known content per file for accurate Write/NotebookEdit diffs const fileLastContent = new Map(); + const cleanPath = (fp: string): string => fp.trim().replace(TRAILING_PUNCT, ''); + const trackFile = (fp: string): void => { - if (typeof fp === 'string' && isValidFilePath(fp)) filesTouchedSet.add(fp); + if (typeof fp === 'string') { + const cleaned = cleanPath(fp); + if (isValidFilePath(cleaned)) filesTouchedSet.add(cleaned); + } }; const addFileLines = (fp: string, added: number, removed: number): void => { - if (!isValidFilePath(fp)) return; - const existing = perFileStats[fp]; + const cleaned = cleanPath(fp); + if (!isValidFilePath(cleaned)) return; + const existing = perFileStats[cleaned]; if (existing) { existing.added += added; existing.removed += removed; } else { - perFileStats[fp] = { added, removed }; + perFileStats[cleaned] = { added, removed }; } }; diff --git a/src/main/services/team/TeamDataService.ts b/src/main/services/team/TeamDataService.ts index 7f9bea48..cd31e560 100644 --- a/src/main/services/team/TeamDataService.ts +++ b/src/main/services/team/TeamDataService.ts @@ -9,7 +9,11 @@ import { } from '@main/utils/pathDecoder'; import { isProcessAlive } from '@main/utils/processHealth'; import { killProcessByPid } from '@main/utils/processKill'; -import { AGENT_BLOCK_CLOSE, AGENT_BLOCK_OPEN } from '@shared/constants/agentBlocks'; +import { + AGENT_BLOCK_CLOSE, + AGENT_BLOCK_OPEN, + stripAgentBlocks, +} from '@shared/constants/agentBlocks'; import { getMemberColor } from '@shared/constants/memberColors'; import { createLogger } from '@shared/utils/logger'; import { parseNumericSuffixName } from '@shared/utils/teamMemberName'; @@ -60,7 +64,7 @@ import type { const logger = createLogger('Service:TeamDataService'); const MIN_TEXT_LENGTH = 30; -const MAX_LEAD_TEXTS = 50; +const MAX_LEAD_TEXTS = 150; const PROCESS_HEALTH_INTERVAL_MS = 2_000; const MAX_PROCESSES_FILE_BYTES = 2 * 1024 * 1024; const TASK_MAP_YIELD_EVERY = 250; @@ -1472,20 +1476,31 @@ export class TeamDataService { const timestamp = typeof msg.timestamp === 'string' ? msg.timestamp : new Date().toISOString(); + const textParts: string[] = []; for (const block of content as Record[]) { if (block.type !== 'text' || typeof block.text !== 'string') continue; - const text = block.text.trim(); - if (text.length < MIN_TEXT_LENGTH) continue; - textsReversed.push({ - from: leadName, - text, - timestamp, - read: true, - source: 'lead_session', - leadSessionId: config.leadSessionId, - }); - if (textsReversed.length >= MAX_LEAD_TEXTS) break; + textParts.push(block.text); } + if (textParts.length === 0) continue; + + const combined = stripAgentBlocks(textParts.join('\n')).trim(); + if (combined.length < MIN_TEXT_LENGTH) continue; + + // Stable messageId: timestamp + text prefix (survives tail-scan range changes) + const textPrefix = combined + .slice(0, 50) + .replace(/[^\p{L}\p{N}]/gu, '') + .slice(0, 20); + + textsReversed.push({ + from: leadName, + text: combined, + timestamp, + read: true, + source: 'lead_session', + leadSessionId: config.leadSessionId, + messageId: `lead-session-${timestamp}-${textPrefix}`, + }); if (textsReversed.length >= MAX_LEAD_TEXTS) break; } diff --git a/src/main/services/team/TeamProvisioningService.ts b/src/main/services/team/TeamProvisioningService.ts index 1156b83a..77cf5cfc 100644 --- a/src/main/services/team/TeamProvisioningService.ts +++ b/src/main/services/team/TeamProvisioningService.ts @@ -161,15 +161,8 @@ interface ProvisioningRun { rejectOnce: (error: string) => void; timeoutHandle: NodeJS.Timeout; } | null; - /** - * Accumulates assistant text for direct user→lead messages (no relay capture active). - * Flushed to liveLeadProcessMessages on result.success. - */ - directReplyParts: string[]; - /** Monotonic counter for stream-json turns (incremented on result). */ - leadTurnSeq: number; - /** Stable timestamp used for the current aggregated lead turn message. */ - leadTurnMessageTimestamp: string | null; + /** Monotonic counter for individual lead assistant messages. */ + leadMsgSeq: number; /** Throttle timestamp for emitting inbox refresh events for lead text. */ lastLeadTextEmitMs: number; /** @@ -1750,9 +1743,7 @@ export class TeamProvisioningService { isLaunch: false, fsPhase: 'waiting_config', leadRelayCapture: null, - directReplyParts: [], - leadTurnSeq: 0, - leadTurnMessageTimestamp: null, + leadMsgSeq: 0, lastLeadTextEmitMs: 0, silentUserDmForward: null, silentUserDmForwardClearHandle: null, @@ -2051,9 +2042,7 @@ export class TeamProvisioningService { isLaunch: true, fsPhase: 'waiting_members', leadRelayCapture: null, - directReplyParts: [], - leadTurnSeq: 0, - leadTurnMessageTimestamp: null, + leadMsgSeq: 0, lastLeadTextEmitMs: 0, silentUserDmForward: null, silentUserDmForwardClearHandle: null, @@ -2491,8 +2480,6 @@ export class TeamProvisioningService { }, }; run.leadRelayCapture = capture; - // Clear any direct reply parts — relay capture takes priority - run.directReplyParts = []; }); try { @@ -2921,27 +2908,21 @@ export class TeamProvisioningService { }, capture.idleMs); } } else if (run.provisioningComplete) { - // Accumulate assistant text for a single "live lead turn" message in Messages. - // If the same assistant message includes SendMessage(to:"user"), prefer the captured - // SendMessage and avoid duplicating it as a separate lead text entry. + // Push each assistant text block as a separate live message (per-message pattern). + // When the same assistant message includes SendMessage(to:"user"), skip text — + // captureSendMessageToUser() handles it separately. if (!run.silentUserDmForward && !hasSendMessageToUser) { - run.directReplyParts.push(text); - const raw = run.directReplyParts.join('\n'); - const cleanText = stripAgentBlocks(raw).trim(); + const cleanText = stripAgentBlocks(text).trim(); if (cleanText.length >= TeamProvisioningService.LEAD_TEXT_MIN_LENGTH) { + run.leadMsgSeq += 1; const leadName = run.request.members.find((m) => m.role?.toLowerCase().includes('lead'))?.name || 'team-lead'; - if (!run.leadTurnMessageTimestamp) { - run.leadTurnMessageTimestamp = nowIso(); - } - // Update timestamp on each text block so the live indicator stays fresh - const currentTimestamp = nowIso(); - const messageId = `lead-turn-${run.runId}-${run.leadTurnSeq}`; + const messageId = `lead-turn-${run.runId}-${run.leadMsgSeq}`; const leadMsg: InboxMessage = { from: leadName, text: cleanText, - timestamp: currentTimestamp, + timestamp: nowIso(), read: true, summary: cleanText.length > 60 ? cleanText.slice(0, 57) + '...' : cleanText, messageId, @@ -2962,13 +2943,6 @@ export class TeamProvisioningService { }); } } - } else if (hasSendMessageToUser) { - run.directReplyParts = []; - run.leadTurnMessageTimestamp = null; - this.removeLiveLeadProcessMessage( - run.teamName, - `lead-turn-${run.runId}-${run.leadTurnSeq}` - ); } } } @@ -3110,47 +3084,6 @@ export class TeamProvisioningService { const capture = run.leadRelayCapture; const combined = capture.textParts.join('\n').trim(); capture.resolveOnce(combined); - } else if (run.provisioningComplete && run.directReplyParts.length > 0) { - // Finalize the current live lead turn message (single messageId per turn). - const rawReply = run.directReplyParts.join('\n').trim(); - run.directReplyParts = []; - const leadName = - run.request.members.find((m) => m.role?.toLowerCase().includes('lead'))?.name || - 'team-lead'; - const replyText = stripAgentBlocks(rawReply).trim(); - const finalText = - replyText.length > 0 - ? replyText - : rawReply.length > 0 - ? '(Message received and processed)' - : ''; - if (finalText.length > 0) { - if (!run.leadTurnMessageTimestamp) { - run.leadTurnMessageTimestamp = nowIso(); - } - const messageId = `lead-turn-${run.runId}-${run.leadTurnSeq}`; - const replyMsg: InboxMessage = { - from: leadName, - text: finalText, - timestamp: run.leadTurnMessageTimestamp, - read: true, - summary: finalText.length > 60 ? finalText.slice(0, 57) + '...' : finalText, - messageId, - source: 'lead_process', - }; - this.pushLiveLeadProcessMessage(run.teamName, replyMsg); - this.teamChangeEmitter?.({ - type: 'inbox', - teamName: run.teamName, - detail: 'lead-turn-final', - }); - } - } - // Turn boundary: advance lead turn sequence. - if (run.provisioningComplete) { - run.leadTurnSeq += 1; - run.leadTurnMessageTimestamp = null; - run.directReplyParts = []; } // Clear silent relay flag after any successful turn. run.silentUserDmForward = null; @@ -3168,12 +3101,6 @@ export class TeamProvisioningService { if (run.leadRelayCapture) { run.leadRelayCapture.rejectOnce(errorMsg); } - // Turn boundary: advance lead turn sequence. - if (run.provisioningComplete) { - run.leadTurnSeq += 1; - run.leadTurnMessageTimestamp = null; - run.directReplyParts = []; - } // Clear silent relay flag after any errored turn. run.silentUserDmForward = null; if (run.silentUserDmForwardClearHandle) { diff --git a/src/renderer/components/layout/SortableTab.tsx b/src/renderer/components/layout/SortableTab.tsx index bf366be6..7ee062cf 100644 --- a/src/renderer/components/layout/SortableTab.tsx +++ b/src/renderer/components/layout/SortableTab.tsx @@ -125,7 +125,7 @@ export const SortableTab = ({ role="tab" tabIndex={0} aria-selected={isActive} - className="group flex min-w-0 max-w-[200px] shrink-0 cursor-grab items-center gap-2 rounded-md px-3 py-1.5" + className="group flex min-w-0 cursor-grab items-center gap-2 rounded-md px-3 py-1.5" style={style} onClick={(e) => onTabClick(tab.id, e)} onMouseDown={(e) => onMouseDown(tab.id, e)} @@ -188,7 +188,7 @@ export const DragOverlayTab = ({ tab }: { tab: Tab }): React.JSX.Element => { return (
- {formatTime(oldest.timestamp)}–{formatTime(newest.timestamp)} + {formatTime(oldest.timestamp) === formatTime(newest.timestamp) + ? formatTime(oldest.timestamp) + : `${formatTime(oldest.timestamp)}–${formatTime(newest.timestamp)}`}
{/* Scrollable body — fixed height, always visible */}
{chronologicalThoughts.map((thought, idx) => ( -
- - {formatTimeWithSec(thought.timestamp)} - -
- +
+ {idx > 0 && ( +
+
+ + {formatTimeWithSec(thought.timestamp)} + +
+
+ )} +
+
+ +
))} diff --git a/src/renderer/components/team/attachments/AttachmentPreviewList.tsx b/src/renderer/components/team/attachments/AttachmentPreviewList.tsx index cb86dec8..362fe301 100644 --- a/src/renderer/components/team/attachments/AttachmentPreviewList.tsx +++ b/src/renderer/components/team/attachments/AttachmentPreviewList.tsx @@ -1,4 +1,4 @@ -import { AlertCircle } from 'lucide-react'; +import { AlertCircle, X } from 'lucide-react'; import { AttachmentPreviewItem } from './AttachmentPreviewItem'; @@ -8,6 +8,7 @@ interface AttachmentPreviewListProps { attachments: AttachmentPayload[]; onRemove: (id: string) => void; error?: string | null; + onDismissError?: () => void; /** When true, previews are overlaid with a disabled indicator (recipient doesn't support attachments). */ disabled?: boolean; /** Hint text shown when disabled and attachments are present. */ @@ -18,6 +19,7 @@ export const AttachmentPreviewList = ({ attachments, onRemove, error, + onDismissError, disabled, disabledHint, }: AttachmentPreviewListProps): React.JSX.Element | null => { @@ -49,7 +51,16 @@ export const AttachmentPreviewList = ({ {error ? (
-

{error}

+

{error}

+ {onDismissError ? ( + + ) : null}
) : null}
diff --git a/src/renderer/components/team/messages/MessageComposer.tsx b/src/renderer/components/team/messages/MessageComposer.tsx index 89563741..94259ef8 100644 --- a/src/renderer/components/team/messages/MessageComposer.tsx +++ b/src/renderer/components/team/messages/MessageComposer.tsx @@ -128,6 +128,7 @@ export const MessageComposer = ({ addFiles, removeAttachment, clearAttachments, + clearError: clearAttachmentError, handlePaste, handleDrop, } = useAttachments({ persistenceKey: `compose:${teamName}:attachments` }); @@ -408,6 +409,7 @@ export const MessageComposer = ({ attachments={attachments} onRemove={removeAttachment} error={attachmentError} + onDismissError={clearAttachmentError} disabled={attachmentsBlocked} disabledHint="Image attachments are only supported when sending to the team lead while the team is online. Remove attachments or switch recipient." /> diff --git a/src/renderer/hooks/useAttachments.ts b/src/renderer/hooks/useAttachments.ts index 6c2154c6..ca17ffb3 100644 --- a/src/renderer/hooks/useAttachments.ts +++ b/src/renderer/hooks/useAttachments.ts @@ -23,6 +23,7 @@ interface UseAttachmentsReturn { addFiles: (files: FileList | File[]) => Promise; removeAttachment: (id: string) => void; clearAttachments: () => void; + clearError: () => void; handlePaste: (event: React.ClipboardEvent) => void; handleDrop: (event: React.DragEvent) => void; } @@ -219,6 +220,10 @@ export function useAttachments(options?: UseAttachmentsOptions): UseAttachmentsR [schedulePersist] ); + const clearError = useCallback(() => { + setError(null); + }, []); + const clearAttachments = useCallback(() => { if (timerRef.current != null) { clearTimeout(timerRef.current); @@ -280,6 +285,7 @@ export function useAttachments(options?: UseAttachmentsOptions): UseAttachmentsR addFiles, removeAttachment, clearAttachments, + clearError, handlePaste, handleDrop, }; diff --git a/src/renderer/index.css b/src/renderer/index.css index 7506603d..fec49dd6 100644 --- a/src/renderer/index.css +++ b/src/renderer/index.css @@ -642,6 +642,22 @@ body { animation: chat-message-enter 350ms ease-out both; } +@keyframes thought-expand { + from { + max-height: 0; + opacity: 0; + overflow: hidden; + } + to { + max-height: 500px; + opacity: 1; + } +} + +.thought-expand-in { + animation: thought-expand 350ms ease-out both; +} + .skeleton-card { animation: skeleton-fade-in 0.4s ease-out both; position: relative; diff --git a/test/main/ipc/teams.test.ts b/test/main/ipc/teams.test.ts index 522dd469..959bc7cf 100644 --- a/test/main/ipc/teams.test.ts +++ b/test/main/ipc/teams.test.ts @@ -274,7 +274,6 @@ describe('ipc teams handlers', () => { messages: [ { from: 'team-lead', - to: 'user', text: 'Hello there', timestamp: '2026-02-23T10:00:00.000Z', read: true, @@ -287,7 +286,6 @@ describe('ipc teams handlers', () => { provisioningService.getLiveLeadProcessMessages.mockReturnValueOnce([ { from: 'team-lead', - to: 'user', text: 'Hello there', timestamp: '2026-02-23T10:00:01.000Z', read: true, diff --git a/test/main/services/team/MemberStatsComputer.test.ts b/test/main/services/team/MemberStatsComputer.test.ts index 5c62acf7..058db67d 100644 --- a/test/main/services/team/MemberStatsComputer.test.ts +++ b/test/main/services/team/MemberStatsComputer.test.ts @@ -1,6 +1,49 @@ import { describe, expect, it } from 'vitest'; -import { estimateBashLinesChanged } from '../../../../src/main/services/team/MemberStatsComputer'; +import { + estimateBashLinesChanged, + isValidFilePath, +} from '../../../../src/main/services/team/MemberStatsComputer'; + +describe('isValidFilePath', () => { + it('rejects null-like values', () => { + expect(isValidFilePath('null')).toBe(false); + expect(isValidFilePath('null;')).toBe(false); + expect(isValidFilePath('null;,')).toBe(false); + expect(isValidFilePath('undefined')).toBe(false); + expect(isValidFilePath('None')).toBe(false); + expect(isValidFilePath('false')).toBe(false); + expect(isValidFilePath('true')).toBe(false); + expect(isValidFilePath('')).toBe(false); + }); + + it('rejects paths without slash', () => { + expect(isValidFilePath('somefile')).toBe(false); + expect(isValidFilePath('a')).toBe(false); + }); + + it('rejects very short paths', () => { + expect(isValidFilePath('/')).toBe(false); + }); + + it('accepts valid file paths', () => { + expect(isValidFilePath('/tmp/file.txt')).toBe(true); + expect(isValidFilePath('/Users/dev/project/src/main.ts')).toBe(true); + expect(isValidFilePath('./src/index.ts')).toBe(true); + expect(isValidFilePath('src/utils/helper.ts')).toBe(true); + }); + + it('strips trailing punctuation before validation', () => { + expect(isValidFilePath('/tmp/file.txt;')).toBe(true); + expect(isValidFilePath('/tmp/file.txt,')).toBe(true); + expect(isValidFilePath('/tmp/file.txt.')).toBe(true); + }); + + it('handles whitespace', () => { + expect(isValidFilePath(' /tmp/file.txt ')).toBe(true); + expect(isValidFilePath(' null ')).toBe(false); + }); +}); describe('estimateBashLinesChanged', () => { it('returns zero for simple non-writing commands', () => {