Focus files section #287
Focus files section #287
Conversation
This comment has been minimized.
This comment has been minimized.
Gkrumbach07
force-pushed
the
focus-files-section
branch
from
407b71f to
ac6522e
Compare
PR Review: Focus Files SectionOverviewThis PR adds a Results section to the session detail page that displays workflow output files based on glob patterns defined in ambient.json. The implementation includes backend endpoints, frontend UI with tabbed file viewing, and proper React Query integration. StrengthsBackend Implementation
Frontend Implementation
Issues and RecommendationsCritical Issues1. Ignored error in filepath.Rel (content.go:689) Code Quality Issues2. Missing RBAC enforcement in ContentWorkflowResults (content.go:656) 3. Potential duplicate results (content.go:678-687) 4. Frontend: Potential memory leak with polling (use-workflows.ts:43) 5. Frontend: Missing loading state (page.tsx:976-1044) Minor Issues6. Inconsistent response type (sessions.go:1387) 7. Frontend: Magic number for tab ID (page.tsx:1005) 8. Tailwind config Security ReviewNo security issues identified. Path traversal protection is properly implemented using os.DirFS, namespace isolation is maintained, and no user input is used in glob patterns. Performance Considerations
SummaryOverall Assessment: Approve with Minor Changes This is a well-implemented feature that follows the project established patterns. The main concern is the ignored error in filepath.Rel which should be addressed. Other improvements are recommended but can be addressed in follow-up PRs. Great work on this feature! The tabbed interface and markdown rendering are particularly well done. |
- Implemented new backend endpoints for retrieving workflow results: - `GET /content/workflow-results` for fetching results based on session. - `GET /api/projects/:projectName/agentic-sessions/:sessionName/workflow/results` for project-specific results. - Updated frontend to display workflow results in a tabbed interface, allowing users to view output files and their statuses. - Enhanced the `useWorkflowResults` hook for fetching results with polling every 5 seconds. These changes improve the user experience by providing access to workflow output files directly within the session interface.
…flow - Added the `doublestar` package to support recursive glob pattern matching in the `ContentWorkflowResults` function. - Refactored file matching logic to utilize `findMatchingFiles`, improving the accuracy of file retrieval based on specified patterns. - Updated `go.mod` and `go.sum` to include the new dependency. These changes enhance the functionality of the content workflow by allowing for more flexible file matching capabilities.
- Added sorting of result display names and matches in the `ContentWorkflowResults` function to ensure consistent order of files. - Implemented auto-refresh for open tabs in the session detail page, allowing for real-time updates of workflow results. - Improved UI structure and formatting for better readability and user experience in the session detail page. These changes enhance the functionality and usability of the content workflow and session management interface.
- Introduced a constant `MaxResultFileSize` to limit the size of result files to 10MB, preventing potential memory issues. - Implemented file size checks before reading files in the `ContentWorkflowResults` function, ensuring that files exceeding the limit are handled gracefully with appropriate error messages. These changes enhance the robustness of the content workflow by preventing excessive memory usage when processing large files.
Gkrumbach07
force-pushed
the
focus-files-section
branch
from
4d95443 to
9c50f85
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Check file size before reading | ||
| fileInfo, statErr := os.Stat(matchedPath) | ||
| if statErr != nil { |
There was a problem hiding this comment.
🔴 Critical: Error handling issue
The filepath.Rel error is silently ignored with relPath, _ := filepath.Rel(...). This could lead to incorrect paths if the workspace base and matched path are on different volumes or have issues.
Recommendation:
relPath, err := filepath.Rel(workspaceBase, matchedPath)
if err != nil {
result.Error = fmt.Sprintf("Failed to compute relative path: %v", err)
results = append(results, result)
continue
}
| } | ||
|
|
||
| // findMatchingFiles finds files matching a glob pattern with ** support for recursive matching | ||
| func findMatchingFiles(baseDir, pattern string) []string { |
There was a problem hiding this comment.
🟡 Major: Security & Resource Management
The doublestar.Glob implementation has potential issues:
- Directory Traversal Risk: No validation that matched paths stay within
baseDir - Resource Exhaustion: No limit on number of matches (e.g.,
**/*could match thousands of files) - Error Logging Only: Errors are logged but empty slice is returned - caller cannot distinguish between "no matches" and "glob failed"
Recommendations:
func findMatchingFiles(baseDir, pattern string) ([]string, error) {
// Limit glob matches to prevent resource exhaustion
const maxMatches = 100
fsys := os.DirFS(baseDir)
matches, err := doublestar.Glob(fsys, pattern)
if err != nil {
return nil, fmt.Errorf("glob pattern error: %w", err)
}
if len(matches) > maxMatches {
return nil, fmt.Errorf("too many matches (%d), maximum is %d", len(matches), maxMatches)
}
// Validate paths stay within baseDir
var absolutePaths []string
for _, match := range matches {
absPath := filepath.Join(baseDir, match)
cleanPath := filepath.Clean(absPath)
// Ensure path is under baseDir (prevent traversal)
if !strings.HasPrefix(cleanPath, filepath.Clean(baseDir)) {
log.Printf("SECURITY: path traversal attempt blocked: %s", match)
continue
}
absolutePaths = append(absolutePaths, cleanPath)
}
return absolutePaths, nil
}Then update caller to handle errors properly.
| log.Printf("GetWorkflowResults: project=%s session=%s endpoint=%s", project, sessionName, endpoint) | ||
|
|
||
| // Create and send request to content pod | ||
| req, _ := http.NewRequestWithContext(c.Request.Context(), http.MethodGet, u, nil) |
There was a problem hiding this comment.
🟡 Major: Error ignored
http.NewRequestWithContext can return an error (though rarely), but it's being silently ignored with req, _ := ....
Fix:
req, err := http.NewRequestWithContext(c.Request.Context(), http.MethodGet, u, nil)
if err != nil {
log.Printf("GetWorkflowResults: failed to create request: %v", err)
c.JSON(http.StatusInternalServerError, gin.H{"error": "Internal error"})
return
}
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| b, _ := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
🔵 Minor: Ignored error
io.ReadAll error is ignored. While the response body has already been checked, it's better practice to handle this:
b, err := io.ReadAll(resp.Body)
if err != nil {
log.Printf("GetWorkflowResults: failed to read response: %v", err)
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to read results"})
return
}
c.Data(resp.StatusCode, "application/json", b)
| queryKey: workflowKeys.results(projectName, sessionName), | ||
| queryFn: () => workflowsApi.getWorkflowResults(projectName, sessionName), | ||
| enabled: !!projectName && !!sessionName, | ||
| refetchInterval: 5000, // Poll every 5 seconds |
There was a problem hiding this comment.
🔴 Critical: Aggressive polling causing performance issues
5-second polling interval is very aggressive and can cause:
- Unnecessary backend load (720 requests/hour per active session)
- Battery drain on mobile devices
- Network bandwidth waste
Recommendations:
- Use a more reasonable interval:
refetchInterval: 30000(30 seconds) - Better yet, implement conditional polling:
- Only poll when workflow is active AND session is running
- Stop polling when session completes
- Use exponential backoff
export function useWorkflowResults(
projectName: string,
sessionName: string,
sessionPhase?: string // Pass from parent
) {
// Only poll if session is active
const shouldPoll = sessionPhase === 'Running' || sessionPhase === 'Pending';
return useQuery({
queryKey: workflowKeys.results(projectName, sessionName),
queryFn: () => workflowsApi.getWorkflowResults(projectName, sessionName),
enabled: !!projectName && !!sessionName,
refetchInterval: shouldPoll ? 10000 : false, // 10s when active, off when complete
});
}
| }, [session, ootbWorkflows, workflowManagement]); | ||
|
|
||
| // Sync open tabs with latest workflow results (auto-refresh tab content) | ||
| useEffect(() => { |
There was a problem hiding this comment.
🟡 Major: Inefficient useEffect - excessive re-renders
This effect runs on every workflowResults change (every 5 seconds due to polling), causing:
- Full tabs array reconstruction
- Unnecessary re-renders even when content hasn't changed
- React reconciliation overhead
Issues:
- No dependency on
openTabsdespite using it in closure - No memoization of comparison logic
- String comparison for potentially large content
Fix:
// Use useMemo to compute updated tabs only when needed
const tabsWithLatestContent = useMemo(() => {
if (!workflowResults?.results) return openTabs;
return openTabs.map(tab => {
if (tab.id === 'chat') return tab;
const matchingResult = workflowResults.results.find(r =>
r.exists && r.path === tab.path
);
// Only create new object if content actually changed
if (matchingResult?.content && matchingResult.content !== tab.content) {
return { ...tab, content: matchingResult.content };
}
return tab; // Return same reference if unchanged
});
}, [workflowResults?.results, openTabs]);
// Update state only if tabs actually changed
useEffect(() => {
if (tabsWithLatestContent !== openTabs) {
setOpenTabs(tabsWithLatestContent);
}
}, [tabsWithLatestContent]);
| )} | ||
| onClick={() => { | ||
| if (result.exists && result.content) { | ||
| const tabId = `result-${idx}`; |
There was a problem hiding this comment.
🔵 Minor: Duplicate tab prevention incomplete
The check if (!openTabs.find(t => t.id === tabId)) prevents duplicate tab IDs but has issues:
- If user clicks same result multiple times rapidly, race condition can add duplicates
- Tab ID is based on array index (
result-${idx}) which can change if results reorder - No limit on number of open tabs (memory leak potential)
Recommendations:
onClick={() => {
if (result.exists && result.content) {
// Use path as stable ID instead of index
const tabId = `result-${result.path.replace(/[^a-zA-Z0-9]/g, '-')}`;
const existingTab = openTabs.find(t => t.id === tabId);
if (existingTab) {
// Tab already open, just switch to it
setActiveTab(tabId);
} else {
// Limit to 10 tabs
if (openTabs.length >= 10) {
errorToast("Maximum 10 tabs open. Close a tab first.");
return;
}
setOpenTabs([...openTabs, {
id: tabId,
name: result.displayName,
path: result.path,
content: result.content
}]);
setActiveTab(tabId);
}
}
}}
| <p className="text-sm text-muted-foreground">{tab.path}</p> | ||
| </div> | ||
|
|
||
| {isMarkdown ? ( |
There was a problem hiding this comment.
🔵 Minor: Hardcoded Tailwind prose classes should use CSS or @tailwindcss/typography plugin
While @tailwindcss/typography was added as a dependency, the implementation uses custom inline Tailwind classes instead of leveraging the plugin's prose classes properly. This creates:
- Maintenance burden (lots of inline styling)
- Inconsistent styling vs typography plugin defaults
- Bundle size increase from unused typography plugin
Options:
Option A: Use typography plugin properly
<article className="prose prose-slate max-w-none dark:prose-invert">
<ReactMarkdown remarkPlugins={[remarkGfm]}>
{tab.content}
</ReactMarkdown>
</article>Option B: Remove unused dependency
If custom styling is preferred, remove @tailwindcss/typography from package.json to reduce bundle size.
| border-radius: 4px; | ||
| } | ||
|
|
||
| *::-webkit-scrollbar-thumb:hover { |
There was a problem hiding this comment.
🔵 Minor: Checkbox styling could use existing Shadcn components
While the custom checkbox styling works, consider using Shadcn's Checkbox component for consistency:
// In ReactMarkdown components
input: ({checked}) => (
<Checkbox checked={checked} disabled className="mr-2" />
)This ensures:
- Consistent styling with rest of app
- Proper accessibility attributes
- Theme support out of the box
Claude Code ReviewSummaryPR #287 introduces a "Focus files section" feature that adds workflow result file viewing capabilities with a tabbed interface. The implementation adds backend endpoints for fetching workflow results via glob patterns, integrates the doublestar library for pattern matching, and creates a tabbed UI in the frontend to display markdown and text files from workflow outputs. Overall Assessment: The feature is functional and well-structured, but has several critical issues that should be addressed before merge, particularly around performance (aggressive polling), security (path traversal risk), and error handling. Issues by Severity🚫 Blocker IssuesNone - no blocking issues found. 🔴 Critical Issues1. Aggressive polling causing performance problems (use-workflows.ts:43)
2. Error handling silently ignored (content.go:704)
🟡 Major Issues3. Security & Resource Management in glob implementation (content.go:743)
4. Inefficient React useEffect causing excessive re-renders (page.tsx:322)
5. Hardcoded light mode CSS breaks dark mode (globals.css:122)
6. Multiple error ignoring instances (sessions.go:1372, 1386)
🔵 Minor Issues7. Duplicate tab prevention incomplete (page.tsx:1032)
8. Unused @tailwindcss/typography dependency (page.tsx:1230, package.json)
9. Custom checkbox styling vs Shadcn components (globals.css:144)
Positive Highlights✅ Good architecture: Clean separation between backend (content.go), API layer (routes.go, sessions.go), and frontend (React Query) ✅ Type safety: Proper TypeScript types defined for all API responses (workflows.ts) ✅ Consistent patterns: Follows repository's established patterns for API handlers and React Query hooks ✅ User experience: Tabbed interface is intuitive; markdown rendering with GFM support is well-implemented ✅ Robust file handling: 10MB file size limit prevents memory issues; proper sorting for consistent ordering ✅ React Query usage: Correctly uses queries with appropriate cache keys and staleTime settings RecommendationsPriority 1 (Must fix before merge):
Priority 2 (Should fix before merge): Priority 3 (Nice to have): Testing Suggestions:
Code quality score: 7/10 - Good feature implementation with solid structure, but needs security hardening, performance optimization, and error handling improvements before production deployment. |
- Introduced a new constant `MaxGlobMatches` to limit the number of matched files, preventing resource exhaustion during file searches. - Updated `findMatchingFiles` to return an error for invalid base directories and enforce security checks against directory traversal. - Enhanced the session detail page to limit the number of open tabs, ensuring efficient memory usage and improved user experience. - Implemented memoization for workflow results to optimize tab content updates, reducing unnecessary re-renders. These changes improve the robustness and performance of the content workflow and session management interface.
Claude Code ReviewSummaryThis PR implements a significant simplification of the AgenticSession CRD and associated code, focusing on streamlining the repository configuration structure and removing unused fields. The changes include comprehensive documentation of field usage analysis and refactoring plans. The PR represents important technical debt reduction but has several critical issues that must be addressed before merging. Overall Assessment: The intent and analysis are excellent, but the implementation is incomplete and breaking. The operator does not support the new CRD format, causing a critical mismatch between the schema and runtime behavior. Issues by SeverityBlocker Issues1. Operator-CRD Format Mismatch (BREAKING CHANGE)Location: components/operator/internal/handlers/sessions.go:329-345 vs components/manifests/base/crds/agenticsessions-crd.yaml:21-35 Problem: The CRD now defines repos with simple url, branch structure, but the operator still reads the OLD format:
Impact:
Fix Required: Update operator to read spec.repos[] array format as documented in CRD_SIMPLIFICATION_SUMMARY.md (lines 65-74). 2. Backend Type MismatchLocation: components/backend/handlers/sessions.go:112-132, components/backend/types/session.go:24-33 Problem: Backend simplified parseSpec to read SimpleRepo format but removed critical fields:
Impact:
Fix Required: Either keep Output field in SimpleRepo type, OR update frontend to remove output configuration AND update all repo workflows 3. Go Lint FailuresLocation: CI check lint-backend and lint-operator both FAILED Problem: Code does not pass golangci-lint checks, violating CLAUDE.md requirements. Fix Required: Run linting locally and fix all issues before pushing. Critical Issues4. Documentation Files Committed to Repo RootLocation: Root directory: 6 large analysis documents (2490 lines total) Problem: COMPREHENSIVE_FIELD_USAGE_ANALYSIS.md, CRD_API_FIELD_COMPARISON.md, CRD_SIMPLIFICATION_SUMMARY.md, FIELD_USAGE_ANALYSIS.md, REFACTORING_PLAN_SUMMARY.md, REMOVAL_GUIDE.md Issues:
Fix Required: Move essential findings to docs/architecture/crd-simplification.md, remove redundant analysis documents 5. Incomplete ImplementationProblem: PR changes CRD schema but doesn't update all consumers Not Updated:
Fix Required: This should be an atomic change - all components must be updated together or maintain backward compatibility. 6. Missing Migration StrategyProblem: No migration path for existing sessions with old input/output format. Fix Required:
Positive HighlightsExcellent Analysis and DocumentationThe field usage analysis documents are exceptional:
This level of analysis is rare and valuable for technical debt reduction. Security Improvements
Backend SimplificationRemoving BotAccount and ResourceOverrides reduces complexity and removes unused features (good YAGNI principle). Final RecommendationDO NOT MERGE in current state. The analysis and intent are excellent, but the implementation is incomplete and will break production. The operator-CRD mismatch is a critical runtime failure that will prevent all new sessions from starting. Suggested Path Forward:
Alternatively, split into phases:
Great work on the analysis! The execution just needs to catch up to the planning. |
Gkrumbach07
force-pushed
the
focus-files-section
branch
from
e1b71fe to
9798183
Compare
Claude Code ReviewSummaryThis PR implements a comprehensive workflow results feature that allows users to view workflow output files in a tabbed interface. The implementation spans backend endpoints for retrieving workflow results with glob pattern matching, and frontend UI enhancements with auto-refreshing tabs. Overall, this is a well-structured feature with good security considerations, but there are several critical issues that should be addressed before merge. Issues by SeverityBlocker Issues1. Error Silencing in Critical Path (content.go:717) 2. User Token Not Validated (sessions.go:1355) Critical Issues3. State Update Closure Bug (page.tsx:1075-1076) 4. Missing Authentication on Content Endpoint (routes.go:23) 5. No Stale Time Configuration (use-workflows.ts:43) Major Issues6. Memory Leak Risk 7. Component Size Violation (page.tsx: 1416 lines) 8. Missing Type Safety (page.tsx:82) 9. No Tests Added Minor Issues
Positive Highlights✅ Excellent Security: Directory traversal prevention, file size validation, MaxGlobMatches limit RecommendationsPriority 1 (Before Merge):
Priority 2 (Before Next Release):
Overall Assessment: Solid feature with good architecture and security awareness. Main concerns are two blocker issues around error handling and authentication that violate repository standards. Once fixed, this PR will be ready to merge. Estimated Rework: 2-3 hours to address Priority 1 issues. |
- Replaced the unified session repository mapping with a simplified format, reducing complexity in the session spec. - Removed unnecessary fields from the session status, focusing on essential information (phase, message, is_error). - Updated frontend components to align with the new repository structure, enhancing clarity and usability. - Eliminated deprecated fields and logic related to input/output repositories, streamlining the session management process. These changes improve the maintainability and performance of the session handling system.
Claude Code ReviewSummaryThis PR introduces a "Results" section that displays workflow output files to users, enabling them to view generated artifacts (spec.md, plan.md, tasks.md, etc.) directly in the UI via clickable file cards that open in tabs. The implementation includes:
Overall Assessment: Good implementation with solid security considerations, but has several issues requiring attention before merge. Issues by Severity🔴 Critical Issues1. Unvalidated Type Assertions in Backend (CLAUDE.md violation)
if spec, ok := item.Object["spec"].(map[string]interface{}); ok {
if workflow, ok := spec["activeWorkflow"].(map[string]interface{}); ok {
if gitURL, ok := workflow["gitUrl"].(string); ok && gitURL != "" {
// ...
}
}
}
2. Error Swallowing in Relative Path Calculation
relPath, _ := filepath.Rel(workspaceBase, matchedPath)
3. Missing Input Validation on Query Parameters
sessionName := c.Query("session")
if sessionName == "" || strings.Contains(sessionName, "..") {
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid session parameter"})
return
}🟡 Major Issues4. Frontend Type Safety Violations (DESIGN_GUIDELINES.md)
code: ({inline, children}: {inline?: boolean; children?: React.ReactNode}) =>
5. Hardcoded Polling Interval
refetchInterval: 5000, // Poll every 5 seconds
6. Potential Memory Issue with Large Result Files
7. Inconsistent Error Handling in ContentWorkflowResults
if len(ambientConfig.Results) == 0 {
c.JSON(http.StatusOK, gin.H{"results": []ResultFile{}})
return
}
🔵 Minor Issues8. Missing Documentation for New Dependency
9. Inconsistent Logging Levels
10. Frontend Markdown Styling Not Using Shadcn Theme
h1: ({children}) => <h1 className="text-3xl font-bold mt-8 mb-4 text-foreground border-b pb-2">{children}</h1>,11. Magic Numbers Without Constants
12. Tailwind Typography Plugin Added But Not Configured
13. Test Coverage Gap
Positive Highlights✅ Excellent Security Implementation
✅ Good User Experience Design
✅ Follows React Query Best Practices
✅ Code Organization
✅ Accessibility
RecommendationsPriority 1 (Must Fix Before Merge)
Priority 2 (Should Fix Before Merge)
Priority 3 (Nice to Have)
General
Recommendation: Request changes for Critical and Major issues, then approve once addressed. |
|
Tracked in Jira: https://issues.redhat.com/browse/RHOAIENG-39125 |
2 participants
No description provided.