From 664ef2892be599d16cbb0aa9d2e4c93d1f294ed1 Mon Sep 17 00:00:00 2001 From: openhands Date: Sat, 14 Feb 2026 09:34:10 +0000 Subject: [PATCH] fix: address PR bot review comments Critical fixes: - Fix duplicated isPolling/pollLastTouched in events route (missing closing brace) - Add missing path import to realtime.ts (path.basename was used without import) - Fix error.message leak in sessions and beads/read routes (security) - Add missing NextResponse import to activity route - Fix diffDependencies to use composite key (type:target) for accurate tracking Code quality: - Fix beadCounts computation in kanban-controls (was counting epic's own deps, not child issues) - Replace require('path') with ES module imports throughout Tests: 13/15 passing (2 contract tests remain brittle) Co-authored-by: openhands --- src/app/api/activity/route.ts | 6 +++-- src/app/api/beads/read/route.ts | 5 +++-- src/app/api/events/route.ts | 4 ++++ src/app/api/sessions/route.ts | 9 ++++---- src/components/kanban/kanban-controls.tsx | 27 ++++++++++++++++++----- src/components/kanban/kanban-page.tsx | 1 + src/lib/realtime.ts | 1 + src/lib/snapshot-differ.ts | 21 ++++++++++-------- 8 files changed, 51 insertions(+), 23 deletions(-) diff --git a/src/app/api/activity/route.ts b/src/app/api/activity/route.ts index 5b618f8..99531b5 100644 --- a/src/app/api/activity/route.ts +++ b/src/app/api/activity/route.ts @@ -1,9 +1,11 @@ +import { NextResponse } from 'next/server'; +import path from 'node:path'; import { activityEventBus } from '../../../lib/realtime'; function isValidProjectRoot(root: string): boolean { try { - const resolved = require('path').resolve(root); - return require('path').isAbsolute(resolved); + const resolved = path.resolve(root); + return path.isAbsolute(resolved); } catch { return false; } diff --git a/src/app/api/beads/read/route.ts b/src/app/api/beads/read/route.ts index 98ba521..db71509 100644 --- a/src/app/api/beads/read/route.ts +++ b/src/app/api/beads/read/route.ts @@ -27,12 +27,13 @@ export async function GET(request: Request): Promise { const issues = await readIssuesFromDisk({ projectRoot, preferBd: true }); return NextResponse.json({ ok: true, issues }); } catch (error) { + console.error('[API/BeadsRead] Failed to read issues:', error); return NextResponse.json( { ok: false, error: { - classification: 'unknown', - message: error instanceof Error ? error.message : 'Failed to read issues.', + classification: 'internal_error', + message: 'An internal error occurred while reading issues.', }, }, { status: 500 }, diff --git a/src/app/api/events/route.ts b/src/app/api/events/route.ts index 7d64b08..48e59db 100644 --- a/src/app/api/events/route.ts +++ b/src/app/api/events/route.ts @@ -96,6 +96,10 @@ export async function GET(request: Request): Promise { lastTouchedVersion = nextVersion; write(toSseFrame(issuesEventBus.emit(projectRoot, lastTouchedPath, 'changed'))); } + } finally { + isPolling = false; + } + }; const touchedPoll = setInterval(() => { void pollLastTouched(); diff --git a/src/app/api/sessions/route.ts b/src/app/api/sessions/route.ts index 49c192e..bdbc083 100644 --- a/src/app/api/sessions/route.ts +++ b/src/app/api/sessions/route.ts @@ -1,4 +1,5 @@ import { NextResponse } from 'next/server'; +import path from 'node:path'; import { readIssuesFromDisk } from '../../../lib/read-issues'; import { activityEventBus } from '../../../lib/realtime'; import { buildSessionTaskFeed, getCommunicationSummary } from '../../../lib/agent-sessions'; @@ -7,8 +8,8 @@ function isValidProjectRoot(root: string): boolean { // Basic validation: path should not contain traversal patterns // and should resolve to an absolute path try { - const resolved = require('path').resolve(root); - return require('path').isAbsolute(resolved); + const resolved = path.resolve(root); + return path.isAbsolute(resolved); } catch { return false; } @@ -42,8 +43,8 @@ export async function GET(request: Request): Promise { { ok: false, error: { - classification: 'unknown', - message: error instanceof Error ? error.message : 'Failed to load session feed.', + classification: 'internal_error', + message: 'An internal error occurred while loading the session feed.', }, }, { status: 500 }, diff --git a/src/components/kanban/kanban-controls.tsx b/src/components/kanban/kanban-controls.tsx index a0bf0b7..9bb77df 100644 --- a/src/components/kanban/kanban-controls.tsx +++ b/src/components/kanban/kanban-controls.tsx @@ -1,5 +1,6 @@ 'use client'; +import { useMemo } from 'react'; import { motion } from 'framer-motion'; import type { KanbanFilterOptions, KanbanStats } from '../../lib/kanban'; @@ -12,6 +13,7 @@ interface KanbanControlsProps { filters: KanbanFilterOptions; stats: KanbanStats; epics: BeadIssue[]; + issues: BeadIssue[]; onFiltersChange: (filters: KanbanFilterOptions) => void; onNextActionable: () => void; nextActionableFeedback?: string | null; @@ -21,6 +23,7 @@ export function KanbanControls({ filters, stats, epics, + issues, onFiltersChange, onNextActionable, nextActionableFeedback = null, @@ -29,12 +32,24 @@ export function KanbanControls({ 'ui-field rounded-xl px-3 py-2.5 text-sm outline-none transition'; // Build bead counts map for EpicChipStrip - const beadCounts = new Map(); - for (const epic of epics) { - // Count non-epic issues that belong to this epic - const count = epic.dependencies?.filter(d => d.type === 'parent' && d.target === epic.id).length ?? 0; - beadCounts.set(epic.id, count); - } + // Count non-epic issues that have this epic as their parent + const beadCounts = useMemo(() => { + const counts = new Map(); + for (const epic of epics) { + let count = 0; + for (const issue of issues) { + if (issue.issue_type === 'epic') continue; + const parentDep = issue.dependencies.find(d => d.type === 'parent'); + const inferredParent = issue.id.includes('.') ? issue.id.split('.')[0] : null; + const parentEpicId = parentDep?.target ?? inferredParent; + if (parentEpicId === epic.id) { + count++; + } + } + counts.set(epic.id, count); + } + return counts; + }, [epics, issues]); return (
diff --git a/src/components/kanban/kanban-page.tsx b/src/components/kanban/kanban-page.tsx index c209373..8923ace 100644 --- a/src/components/kanban/kanban-page.tsx +++ b/src/components/kanban/kanban-page.tsx @@ -230,6 +230,7 @@ export function KanbanPage({ filters={filters} stats={stats} epics={localIssues.filter((issue) => issue.issue_type === 'epic')} + issues={localIssues} onFiltersChange={setFilters} onNextActionable={handleNextActionable} nextActionableFeedback={nextActionableFeedback} diff --git a/src/lib/realtime.ts b/src/lib/realtime.ts index 92332fe..1637631 100644 --- a/src/lib/realtime.ts +++ b/src/lib/realtime.ts @@ -1,3 +1,4 @@ +import path from 'node:path'; import { canonicalizeWindowsPath, windowsPathKey } from './pathing'; import type { ActivityEvent } from './activity'; diff --git a/src/lib/snapshot-differ.ts b/src/lib/snapshot-differ.ts index d3244d2..358567f 100644 --- a/src/lib/snapshot-differ.ts +++ b/src/lib/snapshot-differ.ts @@ -78,7 +78,7 @@ export function diffSnapshots( // 5. Collection Changes (Dependencies) diffDependencies(prev.dependencies, curr.dependencies).forEach(kindAndTarget => { - events.push(createEvent(kindAndTarget.kind, curr, now, { to: kindAndTarget.target })); + events.push(createEvent(kindAndTarget.kind, curr, now, { to: kindAndTarget.target, field: kindAndTarget.type })); }); }); @@ -119,25 +119,28 @@ function areArraysEqual(a: string[], b: string[]): boolean { /** * Detects added and removed dependencies. + * Uses composite key `${type}:${target}` to detect type changes as well. */ function diffDependencies( prev: BeadDependency[], curr: BeadDependency[] -): { kind: 'dependency_added' | 'dependency_removed', target: string }[] { - const changes: { kind: 'dependency_added' | 'dependency_removed', target: string }[] = []; +): { kind: 'dependency_added' | 'dependency_removed', target: string, type: string }[] { + const changes: { kind: 'dependency_added' | 'dependency_removed', target: string, type: string }[] = []; - const prevTargets = new Set(prev.map(d => d.target)); - const currTargets = new Set(curr.map(d => d.target)); + const prevKeys = new Set(prev.map(d => `${d.type}:${d.target}`)); + const currKeys = new Set(curr.map(d => `${d.type}:${d.target}`)); curr.forEach(d => { - if (!prevTargets.has(d.target)) { - changes.push({ kind: 'dependency_added', target: d.target }); + const key = `${d.type}:${d.target}`; + if (!prevKeys.has(key)) { + changes.push({ kind: 'dependency_added', target: d.target, type: d.type }); } }); prev.forEach(d => { - if (!currTargets.has(d.target)) { - changes.push({ kind: 'dependency_removed', target: d.target }); + const key = `${d.type}:${d.target}`; + if (!currKeys.has(key)) { + changes.push({ kind: 'dependency_removed', target: d.target, type: d.type }); } });