Skip to content

fix: Phase 6 review follow-up — shared util, GET side-effect, dead branch, test gap#13

Open
StatPan wants to merge 1 commit intomainfrom
fix/phase6-review-followup
Open

fix: Phase 6 review follow-up — shared util, GET side-effect, dead branch, test gap#13
StatPan wants to merge 1 commit intomainfrom
fix/phase6-review-followup

Conversation

@StatPan
Copy link
Copy Markdown
Owner

@StatPan StatPan commented Apr 19, 2026

Summary

Follow-up fixes from Phase 6 code review (PR #12).

  • Extract projectGroupFromDirectory to shared util (src/server/utils/projectGroup.ts) — removes 3x duplication between agent.ts and tree.ts on the server side
  • Remove DB write side-effect from GET /api/agent/treesetCanvasNodeProject was called on every GET request; removed entirely; auto-assignment remains in GET /api/tree (tree.ts)
  • Fix dead branch in SSE banner stylebackground: sseStatus === 'disconnected' ? '#1c1917' : '#1c1917' collapsed to background: '#1c1917'
  • Add needs-answer status override testagent.test.ts was missing coverage for the question.askedneeds-answer path

Test plan

  • 89/89 tests pass
  • grep setCanvasNodeProject src/server/routes/agent.ts → 0 matches
  • src/server/utils/projectGroup.ts created and imported in agent.ts + tree.ts

…effect, fix dead branch, add needs-answer test
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the development workflow documentation in CLAUDE.md, refactors the projectGroupFromDirectory function into a shared utility, and removes project auto-creation logic from the agent tree API to prevent side effects during GET requests. Additionally, it simplifies a redundant style property in the AgentCanvas component and adds a test case for session status overrides. Feedback suggests removing an unused import in agent.ts and optimizing the new utility function by hoisting the bucketPrefixes set to a constant to avoid repeated allocations.

import { getAllCanvasNodes, getAllProjects, getAllSessionRelations, getAllTaskInvocations, getForkRelationMap } from '../db/index.js'
import { opencodeAdapter } from '../opencode/index.js'
import { getPendingPermissions, getPendingQuestions } from '../sse/broadcaster.js'
import { projectGroupFromDirectory } from '../utils/projectGroup.js'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The import projectGroupFromDirectory is unused in this file. The logic that previously required it (project auto-creation) was removed in this PR to eliminate side effects from the GET request. This import should be removed to keep the code clean.

Comment on lines +1 to +12
export function projectGroupFromDirectory(directory: string): string {
const marker = '/workspace/'
const index = directory.indexOf(marker)
const normalized = index >= 0 ? directory.slice(index + marker.length) : directory.replace(/^\/+/, '')
const parts = normalized.split('/').filter(Boolean)
if (parts.length === 0) return 'workspace'
const bucketPrefixes = new Set(['apps', 'research', 'pypi_lib', 'libs', 'infra', 'skills', 'mcps', 'anal-repo'])
if (parts.length >= 2 && bucketPrefixes.has(parts[0])) {
return `${parts[0]}/${parts[1]}`
}
return parts[0]
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The bucketPrefixes Set is currently allocated on every call to projectGroupFromDirectory. Since this function is called within loops (e.g., in tree.ts), moving this set to a constant outside the function scope will avoid unnecessary allocations and improve performance.

Suggested change
export function projectGroupFromDirectory(directory: string): string {
const marker = '/workspace/'
const index = directory.indexOf(marker)
const normalized = index >= 0 ? directory.slice(index + marker.length) : directory.replace(/^\/+/, '')
const parts = normalized.split('/').filter(Boolean)
if (parts.length === 0) return 'workspace'
const bucketPrefixes = new Set(['apps', 'research', 'pypi_lib', 'libs', 'infra', 'skills', 'mcps', 'anal-repo'])
if (parts.length >= 2 && bucketPrefixes.has(parts[0])) {
return `${parts[0]}/${parts[1]}`
}
return parts[0]
}
const BUCKET_PREFIXES = new Set(['apps', 'research', 'pypi_lib', 'libs', 'infra', 'skills', 'mcps', 'anal-repo'])
export function projectGroupFromDirectory(directory: string): string {
const marker = '/workspace/'
const index = directory.indexOf(marker)
const normalized = index >= 0 ? directory.slice(index + marker.length) : directory.replace(/^\/+/, '')
const parts = normalized.split('/').filter(Boolean)
if (parts.length === 0) return 'workspace'
if (parts.length >= 2 && BUCKET_PREFIXES.has(parts[0])) {
return `${parts[0]}/${parts[1]}`
}
return parts[0]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant