From 36e93abd42bf524af580af4efd9c5df377bf83de Mon Sep 17 00:00:00 2001 From: iliya Date: Sun, 15 Mar 2026 19:27:59 +0200 Subject: [PATCH] feat: enhance review approval process and notification handling - Added a `suppressTaskComment` flag to the `approveReview` function, allowing users to approve reviews without adding a comment to the task. - Updated the `notifyNewInboxMessages` function to include additional debug logging for better tracking of inbox notifications. - Adjusted notification settings in `ConfigManager` to enable notifications for lead inbox and change the status change behavior. - Enhanced the `NotificationManager` to improve error handling and logging during notification display. - Refactored `TeamDataService` to utilize the new `suppressTaskComment` feature during review approvals. - Updated tests to validate the new approval process and notification behaviors. --- agent-teams-controller/src/internal/review.js | 15 ++-- src/main/index.ts | 15 +++- .../services/infrastructure/ConfigManager.ts | 4 +- .../infrastructure/NotificationManager.ts | 90 ++++++++++++------- src/main/services/team/TeamDataService.ts | 2 +- .../team/dialogs/TaskDetailDialog.tsx | 11 ++- .../components/team/members/MemberLogsTab.tsx | 51 +++++++---- .../components/ui/auto-resize-textarea.tsx | 14 +++ src/renderer/hooks/useViewportCommentRead.ts | 16 ++-- src/renderer/hooks/useViewportObserver.ts | 35 +++++--- .../services/team/TeamDataService.test.ts | 2 +- 11 files changed, 172 insertions(+), 83 deletions(-) diff --git a/agent-teams-controller/src/internal/review.js b/agent-teams-controller/src/internal/review.js index 65cac819..19fd468b 100644 --- a/agent-teams-controller/src/internal/review.js +++ b/agent-teams-controller/src/internal/review.js @@ -109,6 +109,7 @@ function approveReview(context, taskId, flags = {}) { const from = typeof flags.from === 'string' && flags.from.trim() ? flags.from.trim() : 'team-lead'; const note = typeof flags.note === 'string' && flags.note.trim() ? flags.note.trim() : 'Approved'; + const suppressTaskComment = flags.suppressTaskComment === true; const leadSessionId = resolveLeadSessionId(context, flags); const prevReviewState = getCurrentReviewState(task); @@ -127,12 +128,14 @@ function approveReview(context, taskId, flags = {}) { return t; }); - tasks.addTaskComment(context, task.id, { - text: note, - from, - type: 'review_approved', - notifyOwner: false, - }); + if (!suppressTaskComment) { + tasks.addTaskComment(context, task.id, { + text: note, + from, + type: 'review_approved', + notifyOwner: false, + }); + } if ((flags.notify === true || flags['notify-owner'] === true) && task.owner) { messages.sendMessage(context, { diff --git a/src/main/index.ts b/src/main/index.ts index bd1b3ff6..7f14611e 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -183,6 +183,7 @@ function extractNotificationContent(text: string): { summary: string; body: stri } async function notifyNewInboxMessages(teamName: string, detail: string): Promise { + logger.debug(`[inbox-notify] called: team=${teamName} detail=${detail}`); const config = configManager.getConfig(); // Skip orphaned team directories without config.json (e.g., "default"). @@ -191,6 +192,7 @@ async function notifyNewInboxMessages(teamName: string, detail: string): Promise // correct team name via sentMessages.json, so inbox notifications from orphaned dirs // would be duplicates with a wrong team name. if (!existsSync(join(getTeamsBasePath(), teamName, 'config.json'))) { + logger.debug(`[inbox-notify] skipped: no config.json for team=${teamName}`); return; // No config.json → orphaned team dir, skip notification } @@ -221,6 +223,7 @@ async function notifyNewInboxMessages(teamName: string, detail: string): Promise if (isFirstLoad) { // First load — seed count, don't notify for pre-existing messages + logger.debug(`[inbox-notify] first load for ${key}: seeding count=${messages.length}`); inboxMessageCounts.set(key, messages.length); return; } @@ -234,6 +237,10 @@ async function notifyNewInboxMessages(teamName: string, detail: string): Promise const newMessages = messages.slice(0, messages.length - prevCount); inboxMessageCounts.set(key, messages.length); + logger.debug( + `[inbox-notify] ${key}: prevCount=${prevCount} newCount=${messages.length} newMessages=${newMessages.length} suppressToast=${String(suppressToast)}` + ); + const teamDisplayName = await resolveTeamDisplayName(teamName); for (let i = 0; i < newMessages.length; i++) { @@ -729,9 +736,11 @@ function initializeServices(): void { // Fire-and-forget: initializeServices() is sync, cannot await. // Safe because TeamBackupService.initialized flag blocks all backup/restore // operations until initialize() completes internally (restore → prune → set flag). - void teamBackupService.initialize().catch((error: unknown) => - logger.warn(`[Init] TeamBackupService init failed: ${String(error)}`) - ); + void teamBackupService + .initialize() + .catch((error: unknown) => + logger.warn(`[Init] TeamBackupService init failed: ${String(error)}`) + ); // Cross-team communication service const crossTeamConfigReader = new TeamConfigReader(); diff --git a/src/main/services/infrastructure/ConfigManager.ts b/src/main/services/infrastructure/ConfigManager.ts index 4600013d..aa656456 100644 --- a/src/main/services/infrastructure/ConfigManager.ts +++ b/src/main/services/infrastructure/ConfigManager.ts @@ -259,12 +259,12 @@ const DEFAULT_CONFIG: AppConfig = { snoozedUntil: null, snoozeMinutes: 30, includeSubagentErrors: false, - notifyOnLeadInbox: false, + notifyOnLeadInbox: true, notifyOnUserInbox: true, notifyOnClarifications: true, notifyOnStatusChange: true, notifyOnTaskComments: true, - statusChangeOnlySolo: true, + statusChangeOnlySolo: false, statusChangeStatuses: ['in_progress', 'completed'], triggers: DEFAULT_TRIGGERS, }, diff --git a/src/main/services/infrastructure/NotificationManager.ts b/src/main/services/infrastructure/NotificationManager.ts index 51f3444b..58a5d2ed 100644 --- a/src/main/services/infrastructure/NotificationManager.ts +++ b/src/main/services/infrastructure/NotificationManager.ts @@ -421,41 +421,56 @@ export class NotificationManager extends EventEmitter { stored: StoredNotification, payload: TeamNotificationPayload ): void { - if (!this.isNativeNotificationSupported()) return; + if (!this.isNativeNotificationSupported()) { + logger.warn('[team-toast] native notifications not supported — skipping'); + return; + } - const config = this.configManager.getConfig(); - const isMac = process.platform === 'darwin'; - const truncatedBody = stripMarkdown(payload.body).slice(0, 300); - const iconPath = isMac ? undefined : getAppIconPath(); - const notification = new Notification({ - title: payload.teamDisplayName, - ...(isMac ? { subtitle: payload.summary } : {}), - body: !isMac && payload.summary ? `${payload.summary}\n${truncatedBody}` : truncatedBody, - sound: config.notifications.soundEnabled ? 'default' : undefined, - ...(iconPath ? { icon: iconPath } : {}), - }); + try { + const config = this.configManager.getConfig(); + const isMac = process.platform === 'darwin'; + const truncatedBody = stripMarkdown(payload.body).slice(0, 300); + const iconPath = isMac ? undefined : getAppIconPath(); - // Hold a strong reference to prevent GC from collecting the notification - this.activeNotifications.add(notification); - const cleanup = (): void => { - this.activeNotifications.delete(notification); - }; + logger.debug( + `[team-toast] creating: title="${payload.teamDisplayName}" summary="${payload.summary ?? ''}" bodyLen=${truncatedBody.length}` + ); - notification.on('click', () => { - this.handleNativeNotificationClick(stored); - cleanup(); - }); - notification.on('close', cleanup); + const notification = new Notification({ + title: payload.teamDisplayName, + ...(isMac ? { subtitle: payload.summary } : {}), + body: !isMac && payload.summary ? `${payload.summary}\n${truncatedBody}` : truncatedBody, + sound: config.notifications.soundEnabled ? 'default' : undefined, + ...(iconPath ? { icon: iconPath } : {}), + }); - notification.on('show', () => { - logger.debug(`[notification] shown: "${payload.teamDisplayName}" — ${payload.summary ?? ''}`); - }); - notification.on('failed', (_, error) => { - logger.warn(`[notification] failed: ${error}`); - cleanup(); - }); + // Hold a strong reference to prevent GC from collecting the notification + this.activeNotifications.add(notification); + const cleanup = (): void => { + this.activeNotifications.delete(notification); + }; - notification.show(); + notification.on('click', () => { + this.handleNativeNotificationClick(stored); + cleanup(); + }); + notification.on('close', cleanup); + + notification.on('show', () => { + logger.debug( + `[team-toast] OS confirmed show: "${payload.teamDisplayName}" — ${payload.summary ?? ''}` + ); + }); + notification.on('failed', (_, error) => { + logger.warn(`[team-toast] OS failed: ${String(error)}`); + cleanup(); + }); + + notification.show(); + logger.debug('[team-toast] notification.show() called'); + } catch (error) { + logger.error(`[team-toast] exception in showTeamNativeNotification: ${String(error)}`); + } } /** @@ -653,10 +668,21 @@ export class NotificationManager extends EventEmitter { async addTeamNotification(payload: TeamNotificationPayload): Promise { const error = buildDetectedErrorFromTeam(payload); const stored = await this.storeNotification(error); - if (!stored) return null; + if (!stored) { + logger.debug( + `[team-notification] skipped (dedup): type=${payload.teamEventType} key=${payload.dedupeKey}` + ); + return null; + } // Team-specific toast policy: enabled/snoozed + suppressToast + dedupeKey throttle only - if (!payload.suppressToast && this.areNotificationsEnabled() && !this.isToastThrottled(error)) { + const enabled = this.areNotificationsEnabled(); + const throttled = this.isToastThrottled(error); + const shouldShow = !payload.suppressToast && enabled && !throttled; + logger.debug( + `[team-notification] toast decision: type=${payload.teamEventType} suppressToast=${String(payload.suppressToast ?? false)} enabled=${String(enabled)} throttled=${String(throttled)} → show=${String(shouldShow)}` + ); + if (shouldShow) { this.showTeamNativeNotification(stored, payload); } diff --git a/src/main/services/team/TeamDataService.ts b/src/main/services/team/TeamDataService.ts index ec73534e..5b6b7519 100644 --- a/src/main/services/team/TeamDataService.ts +++ b/src/main/services/team/TeamDataService.ts @@ -1891,7 +1891,7 @@ export class TeamDataService { const { leadSessionId } = await this.resolveLeadRuntimeContext(teamName); controller.review.approveReview(taskId, { from: 'user', - note: 'Approved from kanban', + suppressTaskComment: true, 'notify-owner': true, ...(leadSessionId ? { leadSessionId } : {}), }); diff --git a/src/renderer/components/team/dialogs/TaskDetailDialog.tsx b/src/renderer/components/team/dialogs/TaskDetailDialog.tsx index 8650bc60..c745aff5 100644 --- a/src/renderer/components/team/dialogs/TaskDetailDialog.tsx +++ b/src/renderer/components/team/dialogs/TaskDetailDialog.tsx @@ -224,8 +224,11 @@ export const TaskDetailDialog = ({ lightboxOpenRef.current = isOpen; }, []); - // Ref for the scrollable DialogContent — needed as IO root for viewport-based read tracking. - const dialogContentRef = useRef(null); + // Callback-ref + useState for the scrollable DialogContent — needed as IO root + // for viewport-based read tracking. Using useState (not useRef) ensures that + // useViewportObserver recreates the IntersectionObserver when the portal mounts + // and the DOM element becomes available. + const [dialogContentEl, setDialogContentEl] = useState(null); const handleReply = useCallback( (author: string, text: string) => { if (currentTask) setReplyTo({ taskId: currentTask.id, author, text }); @@ -269,7 +272,7 @@ export const TaskDetailDialog = ({ const { registerComment, flush: flushCommentRead } = useViewportCommentRead({ teamName, taskId: currentTask?.id ?? '', - scrollContainerRef: dialogContentRef, + scrollContainer: dialogContentEl, }); const handleClose = useCallback(() => { @@ -525,7 +528,7 @@ export const TaskDetailDialog = ({ }} > { if (lightboxOpenRef.current) e.preventDefault(); diff --git a/src/renderer/components/team/members/MemberLogsTab.tsx b/src/renderer/components/team/members/MemberLogsTab.tsx index 2825c77c..f9aee3f6 100644 --- a/src/renderer/components/team/members/MemberLogsTab.tsx +++ b/src/renderer/components/team/members/MemberLogsTab.tsx @@ -65,10 +65,6 @@ function filterChunksByWorkIntervals( return cs <= end && ce >= i.startMs; }); }); - // DEBUG - console.log( - `[filterChunks] intervals=${parsed.length} chunks=${chunks.length}→${filtered.length}` - ); return filtered; } @@ -111,12 +107,6 @@ export const MemberLogsTab = ({ showLeadPreview = false, onPreviewOnlineChange, }: MemberLogsTabProps): React.JSX.Element => { - // DEBUG: verify workIntervals reach this component - if (taskId && taskWorkIntervals) { - console.log( - `[MemberLogsTab] taskId=${taskId} workIntervals=${JSON.stringify(taskWorkIntervals)}` - ); - } const MIN_REFRESH_VISIBLE_MS = 250; const intervalsKey = useMemo( () => (taskWorkIntervals ? JSON.stringify(taskWorkIntervals) : ''), @@ -198,21 +188,52 @@ export const MemberLogsTab = ({ if (!Number.isFinite(startMs)) return Number.NaN; const durationMs = Number.isFinite(log.durationMs) ? Math.max(0, log.durationMs) : 0; const endMs = startMs + durationMs; - // Keep actively-updating logs at the top even if duration lags slightly. return log.isOngoing ? Math.max(endMs, nowMs) : endMs; }; - const withIndex = logs.map((log, index) => ({ log, index })); + // When viewing a task with workIntervals, sort by overlap (most relevant first). + // Fallback to endMs (most recent activity) when no intervals available. + const getOverlapMs = (log: MemberLogSummary): number => { + if (!taskWorkIntervals || taskWorkIntervals.length === 0) return 0; + const logStartMs = new Date(log.startTime).getTime(); + if (!Number.isFinite(logStartMs)) return 0; + const logDurationMs = Number.isFinite(log.durationMs) ? Math.max(0, log.durationMs) : 0; + const logEndMs = log.isOngoing ? nowMs : logStartMs + logDurationMs; + + let totalOverlap = 0; + for (const interval of taskWorkIntervals) { + const intStart = Date.parse(interval.startedAt); + if (!Number.isFinite(intStart)) continue; + const intEnd = + typeof interval.completedAt === 'string' ? Date.parse(interval.completedAt) : nowMs; + if (!Number.isFinite(intEnd)) continue; + const overlapStart = Math.max(logStartMs, intStart); + const overlapEnd = Math.min(logEndMs, intEnd); + if (overlapEnd > overlapStart) totalOverlap += overlapEnd - overlapStart; + } + return totalOverlap; + }; + + const withIndex = logs.map((log, index) => ({ + log, + index, + overlap: getOverlapMs(log), + lastActivity: getLastActivityMs(log), + })); + withIndex.sort((a, b) => { - const aTime = getLastActivityMs(a.log); - const bTime = getLastActivityMs(b.log); + // Primary: overlap with task workIntervals (more overlap = higher) + if (a.overlap !== b.overlap) return b.overlap - a.overlap; + // Secondary: last activity (most recent first) + const aTime = a.lastActivity; + const bTime = b.lastActivity; if (Number.isFinite(aTime) && Number.isFinite(bTime) && aTime !== bTime) return bTime - aTime; if (Number.isFinite(aTime) && !Number.isFinite(bTime)) return -1; if (!Number.isFinite(aTime) && Number.isFinite(bTime)) return 1; return a.index - b.index; }); return withIndex.map((x) => x.log); - }, [logs]); + }, [logs, taskWorkIntervals]); const shouldShowPreview = useMemo(() => { return taskId != null && (showSubagentPreview || showLeadPreview); diff --git a/src/renderer/components/ui/auto-resize-textarea.tsx b/src/renderer/components/ui/auto-resize-textarea.tsx index 58f57bf6..479cc0df 100644 --- a/src/renderer/components/ui/auto-resize-textarea.tsx +++ b/src/renderer/components/ui/auto-resize-textarea.tsx @@ -40,12 +40,26 @@ const AutoResizeTextarea = React.forwardRef maxHeight ? 'auto' : 'hidden'; + + // Restore scrollTop after height adjustment, then scroll to caret + // if cursor is at the end (common after paste). + if (scrollHeight > maxHeight) { + if (textarea.selectionStart === textarea.value.length) { + textarea.scrollTop = textarea.scrollHeight; + } else { + textarea.scrollTop = savedScrollTop; + } + } }, [minRows, maxRows]); React.useEffect(() => { diff --git a/src/renderer/hooks/useViewportCommentRead.ts b/src/renderer/hooks/useViewportCommentRead.ts index 95e8d8f0..c2bc464a 100644 --- a/src/renderer/hooks/useViewportCommentRead.ts +++ b/src/renderer/hooks/useViewportCommentRead.ts @@ -4,17 +4,17 @@ import { markCommentsRead } from '@renderer/services/commentReadStorage'; import { useViewportObserver } from './useViewportObserver'; -import type { RefObject } from 'react'; - interface UseViewportCommentReadOptions { teamName: string; taskId: string; /** - * Scrollable ancestor element (e.g. DialogContent) used as IO root. - * Required for portalled Dialogs where the default viewport root - * would not detect intersections correctly. + * Scrollable ancestor DOM element (e.g. DialogContent) used as IO root. + * Pass the actual element (not a RefObject) so that the observer is + * recreated when the portal mounts. Use useState + callback ref: + * const [rootEl, setRootEl] = useState(null); + * */ - scrollContainerRef: RefObject; + scrollContainer: HTMLElement | null; } /** @@ -34,7 +34,7 @@ interface UseViewportCommentReadOptions { export function useViewportCommentRead({ teamName, taskId, - scrollContainerRef, + scrollContainer, }: UseViewportCommentReadOptions): { /** Ref callback factory. Call with the comment's unique ID. */ registerComment: (commentId: string) => (el: HTMLElement | null) => void; @@ -78,7 +78,7 @@ export function useViewportCommentRead({ ); const { registerElement } = useViewportObserver({ - rootRef: scrollContainerRef, + root: scrollContainer, threshold: 0.1, onVisibleChange: handleVisibleChange, }); diff --git a/src/renderer/hooks/useViewportObserver.ts b/src/renderer/hooks/useViewportObserver.ts index f9cfd9ed..cd21ec4d 100644 --- a/src/renderer/hooks/useViewportObserver.ts +++ b/src/renderer/hooks/useViewportObserver.ts @@ -1,17 +1,20 @@ import { useCallback, useEffect, useRef } from 'react'; -import type { RefObject } from 'react'; - /** Data attribute name used to store arbitrary string data on observed elements. */ const DATA_ATTR = 'data-viewport-value'; interface UseViewportObserverOptions { /** - * Scrollable ancestor element used as IntersectionObserver root. - * Required for elements inside Dialog portals where the default - * document viewport root would not detect intersections correctly. + * Scrollable ancestor DOM element used as IntersectionObserver root. + * Pass the actual element (not a RefObject) so that the hook can + * react to the element becoming available (e.g. after a Dialog portal mounts). + * + * Use a callback-ref + useState pattern in the consumer: + * const [rootEl, setRootEl] = useState(null); + * + * useViewportObserver({ root: rootEl, ... }) */ - rootRef?: RefObject; + root?: HTMLElement | null; /** Visibility ratio threshold (0..1). Default: 0.1 (10% visible). */ threshold?: number; /** @@ -26,17 +29,21 @@ interface UseViewportObserverOptions { * scrollable container using IntersectionObserver. * * Usage: - * 1. Call the hook with a root ref and a callback. + * 1. Call the hook with a root element and a callback. * 2. Attach `registerElement(value)` as a ref callback on each element. * `value` is an arbitrary string stored in a data attribute for identification. * 3. The callback fires with the list of currently visible values whenever * the intersection state changes. * + * Important: pass the root as a plain DOM element (not a RefObject) so the + * hook can recreate the observer when the element becomes available. + * Use `useState` + callback ref in the consumer for this. + * * The hook manages a single IntersectionObserver instance and handles * element registration/deregistration automatically. */ export function useViewportObserver({ - rootRef, + root, threshold = 0.1, onVisibleChange, }: UseViewportObserverOptions): { @@ -50,9 +57,15 @@ export function useViewportObserver({ const visibleValuesRef = useRef>(new Set()); const elementsByValue = useRef>(new Map()); - // Create / recreate observer when root or threshold changes. + // Create / recreate observer when root element or threshold changes. + // root is a plain DOM element (not a RefObject), so when the consumer + // updates state (e.g. Dialog portal mounts), this effect re-runs and + // creates an IO with the correct root. useEffect(() => { - const root = rootRef?.current ?? null; + // When root is not yet available (e.g. portal not mounted), skip + // creating the observer — it would default to document viewport + // and produce false positives for all visible elements. + if (!root) return; const observer = new IntersectionObserver( (entries) => { @@ -94,7 +107,7 @@ export function useViewportObserver({ observerRef.current = null; visibleValuesRef.current.clear(); }; - }, [rootRef, threshold]); + }, [root, threshold]); const registerElement = useCallback((value: string) => { return (el: HTMLElement | null) => { diff --git a/test/main/services/team/TeamDataService.test.ts b/test/main/services/team/TeamDataService.test.ts index 9bb94d0e..cc1e45ed 100644 --- a/test/main/services/team/TeamDataService.test.ts +++ b/test/main/services/team/TeamDataService.test.ts @@ -566,7 +566,7 @@ describe('TeamDataService', () => { }); expect(approveReviewMock).toHaveBeenCalledWith('task-1', { from: 'user', - note: 'Approved from kanban', + suppressTaskComment: true, 'notify-owner': true, leadSessionId: 'lead-2', });