From 0e83d99e0a08f03bb36006023b3cf4c4d7a5fb01 Mon Sep 17 00:00:00 2001 From: Artem Rootman <4586640+artemrootman@users.noreply.github.com> Date: Sun, 5 Apr 2026 16:59:44 +0000 Subject: [PATCH] fix: address PR review feedback (6 issues) Critical: - findLogsForTask: call worker directly instead of wrapping in wrapTeamHandler, so failures propagate to catch and trigger the main-thread fallback correctly. - TeamDataWorkerClient: scope error/exit handlers to the specific worker instance to prevent a stale worker's exit event from rejecting pending requests on a replacement worker (race condition). Major: - TeamDataWorkerClient: validate teamName and taskId before forwarding to worker thread (input sanitization). - team-data-worker: include status, since, and intervals length in cache key to prevent stale results after filter changes. - team-data-worker: move logsInFlight.delete() into .finally() so rejected lookups don't poison the in-flight map permanently. - MemberLogsTab: reset refreshCountRef and refreshing state in effect cleanup to prevent the refresh indicator from latching on permanently when the effect tears down mid-refresh (e.g. on tab switch). --- src/main/ipc/teams.ts | 9 ++++--- .../services/team/TeamDataWorkerClient.ts | 21 +++++++++++---- src/main/workers/team-data-worker.ts | 26 +++++++++++-------- .../components/team/members/MemberLogsTab.tsx | 8 ++++++ 4 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/main/ipc/teams.ts b/src/main/ipc/teams.ts index 2c201e2c..b73a00dc 100644 --- a/src/main/ipc/teams.ts +++ b/src/main/ipc/teams.ts @@ -2153,13 +2153,14 @@ async function handleGetLogsForTask( : undefined, } : undefined; - // Prefer worker thread to keep main event loop responsive + // Prefer worker thread to keep main event loop responsive. + // Call worker directly (not via wrapTeamHandler) so that failures + // propagate to the catch block and trigger the main-thread fallback. const worker = getTeamDataWorkerClient(); if (worker.isAvailable()) { try { - return await wrapTeamHandler('getLogsForTask', () => - worker.findLogsForTask(vTeam.value!, vTask.value!, opts) - ); + const result = await worker.findLogsForTask(vTeam.value!, vTask.value!, opts); + return { success: true, data: result }; } catch (workerErr) { logger.warn( `[teams:getLogsForTask] worker failed, falling back: ${workerErr instanceof Error ? workerErr.message : workerErr}` diff --git a/src/main/services/team/TeamDataWorkerClient.ts b/src/main/services/team/TeamDataWorkerClient.ts index bd585cfb..8d348542 100644 --- a/src/main/services/team/TeamDataWorkerClient.ts +++ b/src/main/services/team/TeamDataWorkerClient.ts @@ -18,6 +18,8 @@ import type { TeamDataWorkerRequest, TeamDataWorkerResponse } from './teamDataWo const logger = createLogger('Service:TeamDataWorkerClient'); const WORKER_CALL_TIMEOUT_MS = 30_000; +const SAFE_NAME_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}$/; +const SAFE_ID_RE = /^[a-zA-Z0-9][a-zA-Z0-9-]{0,63}$/; function makeId(): string { return `${Date.now()}-${crypto.randomUUID().slice(0, 12)}`; @@ -68,9 +70,10 @@ export class TeamDataWorkerClient { if (!this.workerPath) throw new Error('Worker not available'); if (this.worker) return this.worker; - this.worker = new Worker(this.workerPath); + const w = new Worker(this.workerPath); + this.worker = w; - this.worker.on('message', (msg: TeamDataWorkerResponse) => { + w.on('message', (msg: TeamDataWorkerResponse) => { const entry = this.pending.get(msg.id); if (!entry) return; this.pending.delete(msg.id); @@ -81,7 +84,11 @@ export class TeamDataWorkerClient { } }); - this.worker.on('error', (err) => { + // Scope error/exit handlers to this specific worker instance. + // Without this guard, a stale worker's exit event can reject + // pending requests that belong to a newer replacement worker. + w.on('error', (err) => { + if (this.worker !== w) return; logger.error('Worker error', err); for (const [, entry] of this.pending) { entry.reject(err instanceof Error ? err : new Error(String(err))); @@ -90,7 +97,8 @@ export class TeamDataWorkerClient { this.worker = null; }); - this.worker.on('exit', (code) => { + w.on('exit', (code) => { + if (this.worker !== w) return; if (code !== 0) logger.warn(`Worker exited with code ${code}`); for (const [, entry] of this.pending) { entry.reject(new Error(`Worker exited with code ${code}`)); @@ -99,7 +107,7 @@ export class TeamDataWorkerClient { this.worker = null; }); - return this.worker; + return w; } private call( @@ -133,6 +141,7 @@ export class TeamDataWorkerClient { } async getTeamData(teamName: string): Promise { + if (!SAFE_NAME_RE.test(teamName)) throw new Error('Invalid teamName'); return this.call('getTeamData', { teamName }) as Promise; } @@ -146,6 +155,8 @@ export class TeamDataWorkerClient { since?: string; } ): Promise { + if (!SAFE_NAME_RE.test(teamName)) throw new Error('Invalid teamName'); + if (!SAFE_ID_RE.test(taskId)) throw new Error('Invalid taskId'); return this.call('findLogsForTask', { teamName, taskId, options }) as Promise< MemberLogSummary[] >; diff --git a/src/main/workers/team-data-worker.ts b/src/main/workers/team-data-worker.ts index 2c93a78c..4b91fdf1 100644 --- a/src/main/workers/team-data-worker.ts +++ b/src/main/workers/team-data-worker.ts @@ -44,7 +44,7 @@ parentPort?.on('message', async (msg: TeamDataWorkerRequest) => { } case 'findLogsForTask': { const { teamName, taskId, options } = msg.payload; - const cacheKey = `${teamName}:${taskId}:${options?.owner ?? ''}`; + const cacheKey = `${teamName}:${taskId}:${options?.owner ?? ''}:${options?.status ?? ''}:${options?.since ?? ''}:${options?.intervals?.length ?? 0}`; // Check result cache const cached = logsResultCache.get(cacheKey); @@ -56,16 +56,20 @@ parentPort?.on('message', async (msg: TeamDataWorkerRequest) => { // Dedup concurrent calls let promise = logsInFlight.get(cacheKey) as Promise | undefined; if (!promise) { - promise = logsFinder.findLogsForTask(teamName, taskId, options).then((result) => { - logsInFlight.delete(cacheKey); - logsResultCache.set(cacheKey, { result, cachedAt: Date.now() }); - // Cap cache - if (logsResultCache.size > 100) { - const firstKey = logsResultCache.keys().next().value; - if (firstKey !== undefined) logsResultCache.delete(firstKey); - } - return result; - }); + promise = logsFinder + .findLogsForTask(teamName, taskId, options) + .then((result) => { + logsResultCache.set(cacheKey, { result, cachedAt: Date.now() }); + // Cap cache + if (logsResultCache.size > 100) { + const firstKey = logsResultCache.keys().next().value; + if (firstKey !== undefined) logsResultCache.delete(firstKey); + } + return result; + }) + .finally(() => { + logsInFlight.delete(cacheKey); + }); logsInFlight.set(cacheKey, promise); } const result = await promise; diff --git a/src/renderer/components/team/members/MemberLogsTab.tsx b/src/renderer/components/team/members/MemberLogsTab.tsx index db5be22b..430c20dc 100644 --- a/src/renderer/components/team/members/MemberLogsTab.tsx +++ b/src/renderer/components/team/members/MemberLogsTab.tsx @@ -488,6 +488,14 @@ export const MemberLogsTab = ({ return () => { cancelled = true; if (interval) clearInterval(interval); + // Reset refresh state so the indicator doesn't stay latched + // when the effect tears down mid-refresh (e.g. tab switch). + refreshCountRef.current = 0; + if (refreshHideTimeoutRef.current) { + clearTimeout(refreshHideTimeoutRef.current); + refreshHideTimeoutRef.current = null; + } + setRefreshing(false); }; // eslint-disable-next-line react-hooks/exhaustive-deps -- intervalsKey + taskSince drive refresh; deps intentionally minimal to avoid refetch loops }, [teamName, memberName, taskId, taskOwner, taskStatus, intervalsKey, taskSince, isTabActive]);