Conversation
- Extract board-view into organized subfolders following new pattern: - components/: kanban-card, kanban-column - dialogs/: all dialog and modal components (8 files) - hooks/: all board-specific hooks (10 files) - shared/: reusable components between dialogs (model-selector, etc.) - Rename all files to kebab-case convention - Add barrel exports (index.ts) for clean imports - Add docs/folder-pattern.md documenting the folder structure - Reduce board-view.tsx from ~3600 lines to ~490 lines 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Shironex
added
Refactor
Summary of ChangesHello @Shironex, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant architectural improvement by refactoring the core Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and much-needed refactoring of the massive board-view.tsx component. Breaking it down into a well-structured folder with components, hooks, and dialogs greatly improves maintainability and readability. The addition of folder-pattern.md is a fantastic step towards standardizing the codebase. My review includes a critical security concern and several suggestions for improving efficiency, correctness, and consistency in the new hooks.
| "allow": [ | ||
| "Bash(dir \"C:\\Users\\Ben\\Desktop\\appdev\\git\\automaker\\apps\\app\\public\")" | ||
| "Bash(dir \"C:\\Users\\Ben\\Desktop\\appdev\\git\\automaker\\apps\\app\\public\")", | ||
| "Bash(find:*)" |
There was a problem hiding this comment.
The permission Bash(find:*) is overly permissive and poses a security risk. It allows the find command to run on any directory on the system, which could be exploited. Please scope this permission to only the necessary directories or remove it if it's not essential.
| if ( | ||
| e.key === "/" && | ||
| !(e.target instanceof HTMLInputElement) && | ||
| !(e.target instanceof HTMLTextAreaElement) | ||
| ) { |
There was a problem hiding this comment.
The check for focused input elements is not comprehensive. It misses other input-like elements, such as those with contenteditable="true". This could lead to the search bar being focused unexpectedly when the user is typing in another part of the UI. A more robust check should be used to prevent this.
| if ( | |
| e.key === "/" && | |
| !(e.target instanceof HTMLInputElement) && | |
| !(e.target instanceof HTMLTextAreaElement) | |
| ) { | |
| if ( | |
| e.key === "/" && | |
| !(e.target instanceof HTMLInputElement) && | |
| !(e.target instanceof HTMLTextAreaElement) && | |
| !(e.target as HTMLElement).isContentEditable | |
| ) { |
| if (feature.imagePaths && feature.imagePaths.length > 0) { | ||
| try { | ||
| const api = getElectronAPI(); | ||
| for (const imagePathObj of feature.imagePaths) { | ||
| try { | ||
| await api.deleteFile(imagePathObj.path); | ||
| console.log(`[Board] Deleted image: ${imagePathObj.path}`); | ||
| } catch (error) { | ||
| console.error(`[Board] Failed to delete image ${imagePathObj.path}:`, error); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`[Board] Error deleting images for feature ${featureId}:`, error); | ||
| } | ||
| } |
There was a problem hiding this comment.
The image deletion logic uses await inside a for...of loop, which processes deletions sequentially. For better performance, especially with multiple images, these deletions can be parallelized using Promise.all.
if (feature.imagePaths && feature.imagePaths.length > 0) {
try {
const api = getElectronAPI();
const deletePromises = feature.imagePaths.map((imagePathObj) =>
api.deleteFile(imagePathObj.path).catch((error) => {
console.error(
`[Board] Failed to delete image ${imagePathObj.path}:`,
error
);
})
);
await Promise.all(deletePromises);
} catch (error) {
console.error(
`[Board] Error setting up image deletion for feature ${featureId}:`,
error
);
}
}| const handleDeleteAllVerified = useCallback(async () => { | ||
| const verifiedFeatures = features.filter((f) => f.status === "verified"); | ||
|
|
||
| for (const feature of verifiedFeatures) { | ||
| const isRunning = runningAutoTasks.includes(feature.id); | ||
| if (isRunning) { | ||
| try { | ||
| await autoMode.stopFeature(feature.id); | ||
| } catch (error) { | ||
| console.error( | ||
| "[Board] Error stopping feature before delete:", | ||
| error | ||
| ); | ||
| } | ||
| } | ||
| removeFeature(feature.id); | ||
| persistFeatureDelete(feature.id); | ||
| } | ||
|
|
||
| toast.success("All verified features deleted", { | ||
| description: `Deleted ${verifiedFeatures.length} feature(s).`, | ||
| }); | ||
| }, [features, runningAutoTasks, autoMode, removeFeature, persistFeatureDelete]); |
There was a problem hiding this comment.
The handleDeleteAllVerified function processes feature deletions sequentially inside a loop. This can be slow if there are many features to delete. The operations can be parallelized for better performance by first stopping all running features and then deleting them.
const handleDeleteAllVerified = useCallback(async () => {
const verifiedFeatures = features.filter((f) => f.status === "verified");
if (verifiedFeatures.length === 0) return;
const featuresToStop = verifiedFeatures.filter((feature) =>
runningAutoTasks.includes(feature.id)
);
if (featuresToStop.length > 0) {
await Promise.all(
featuresToStop.map((feature) =>
autoMode.stopFeature(feature.id).catch((error) => {
console.error(
`[Board] Error stopping feature ${feature.id} before delete:`,
error
);
})
)
);
}
const deletePromises = verifiedFeatures.map((feature) => {
removeFeature(feature.id);
return persistFeatureDelete(feature.id);
});
await Promise.all(deletePromises);
toast.success("All verified features deleted", {
description: `Deleted ${verifiedFeatures.length} feature(s).`,
});
}, [features, runningAutoTasks, autoMode, removeFeature, persistFeatureDelete]);| const featuresWithIds = result.features.map( | ||
| (f: any, index: number) => ({ | ||
| ...f, | ||
| id: f.id || `feature-${index}-${Date.now()}`, |
There was a problem hiding this comment.
The fallback ID generation feature-${index}-${Date.now()} might not be sufficiently unique. If multiple features are loaded in the same millisecond, they could get the same ID, leading to issues with React keys and state management. Using Math.random() would provide better uniqueness.
| id: f.id || `feature-${index}-${Date.now()}`, | |
| id: f.id || `feature-${Date.now()}-${Math.random().toString(36).substring(2, 9)}`, |
| if (!muteDoneSound) { | ||
| const audio = new Audio("/sounds/ding.mp3"); | ||
| audio | ||
| .play() | ||
| .catch((err) => console.warn("Could not play ding sound:", err)); | ||
| } |
There was a problem hiding this comment.
A new Audio object is created every time a feature completion event is handled. This is inefficient. The Audio object should be created once and reused. Consider creating it once inside the useEffect but outside the event handler.
| description: `Manually verified: ${draggedFeature.description.slice( | ||
| 0, | ||
| 50 | ||
| )}${draggedFeature.description.length > 50 ? "..." : ""}`, |
There was a problem hiding this comment.
This file manually truncates description strings for toast messages. A new utility function truncateDescription has been added in apps/app/src/lib/utils.ts for this purpose. Using this utility will ensure consistency across the application. This applies to multiple toast messages in this file.
You'll need to import it: import { truncateDescription } from "@/lib/utils";
description: `Manually verified: ${truncateDescription(draggedFeature.description)}`,
Shironex
added
Ready-To-Merge
2 participants
folder-pattern.mdto standardize the project structure