chore(frontend): ESLint 9 upgrade + debug cleanup#325
chore(frontend): ESLint 9 upgrade + debug cleanup#325namastex888 wants to merge 3 commits intodevfrom
Conversation
ESLint 9 flat config migration: - Migrate from .eslintrc.cjs to eslint.config.js flat config format - Upgrade eslint 8.57→9.39, @typescript-eslint/* 7.18→8.50 - Replace react-spring with @react-spring/web (eliminates 9 deprecated deps) - Fix 4 lint errors caught by stricter v9 rules Dependency improvements: - Eliminates 14 deprecated subdependencies from pnpm install warnings - Only 2 remain (from click-to-react-component and forge-inspector) Performance and cleanup: - Wrap TaskCard in memo() to prevent unnecessary re-renders - Remove debug console.logs from TaskKanbanBoard, TaskFollowUpSection - Simplify useTaskCompletionNotification (Set vs Map, clearer logic) - Fix useConversationHistory to emit entries when process completes - Extract primitive from branchStatus for stable useMemo deps
Summary of ChangesHello @namastex888, 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 significantly enhances the frontend development experience and application performance by upgrading the ESLint configuration to version 9, which includes a migration to flat config and a substantial cleanup of deprecated dependencies. Alongside these foundational improvements, several performance optimizations have been introduced, such as memoization for key components and more stable Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 pull request is a great cleanup and upgrade effort. The migration to ESLint 9 and the flat config is well-executed, and the dependency cleanup is a significant improvement. The performance optimizations, such as wrapping TaskCard in memo and ensuring stable dependencies for hooks, are excellent additions. I've found a couple of areas for improvement. One is a minor suggestion to restore a documentation link in the new ESLint config for better developer guidance. The other is a more significant point regarding a potential regression in the useTaskCompletionNotification hook, where the refactoring seems to have changed the core logic for detecting task completions. Overall, this is a very solid contribution to the project's health and maintainability.
| export function useTaskCompletionNotification( | ||
| executionProcesses: ExecutionProcess[] | ||
| ) { | ||
| const { config } = useUserSystem(); | ||
| const previousProcessesRef = useRef<Map<string, string>>(new Map()); | ||
| // Track which process IDs have already played notification sound | ||
| // This prevents duplicates from re-renders, React Strict Mode, or array re-creation | ||
| const notifiedProcessIdsRef = useRef<Set<string>>(new Set()); | ||
|
|
||
| useEffect(() => { | ||
| if (!config?.notifications.sound_enabled) { | ||
| return; | ||
| } | ||
|
|
||
| const currentProcesses = new Map<string, string>(); | ||
|
|
||
| // Build current state and detect completions | ||
| executionProcesses.forEach((process) => { | ||
| const prevStatus = previousProcessesRef.current.get(process.id); | ||
| const currentStatus = process.status; | ||
|
|
||
| currentProcesses.set(process.id, currentStatus); | ||
|
|
||
| // Check if process just completed (status changed from running to completed/failed/killed) | ||
| const wasRunning = prevStatus === 'running'; | ||
| const isCompleted = | ||
| currentStatus === 'completed' || | ||
| currentStatus === 'failed' || | ||
| currentStatus === 'killed'; | ||
|
|
||
| if (wasRunning && isCompleted && process.run_reason === 'codingagent') { | ||
| // Task execution completed, play notification sound | ||
| // Use full volume - users can adjust system/browser volume | ||
| process.status === 'completed' || | ||
| process.status === 'failed' || | ||
| process.status === 'killed'; | ||
|
|
||
| // Only notify once per process ID when it reaches a completed state | ||
| if ( | ||
| isCompleted && | ||
| process.run_reason === 'codingagent' && | ||
| !notifiedProcessIdsRef.current.has(process.id) | ||
| ) { | ||
| notifiedProcessIdsRef.current.add(process.id); | ||
| playNotificationSound(config.notifications.sound_file, 1.0); | ||
| } | ||
| }); | ||
|
|
||
| // Update ref for next comparison | ||
| previousProcessesRef.current = currentProcesses; | ||
| }, [executionProcesses, config]); | ||
| } |
There was a problem hiding this comment.
The refactoring of this hook from using a Map to a Set has introduced a behavioral change that could be a regression. The previous logic correctly triggered a notification only when a process transitions from a running state to a completed state. The new logic triggers a notification if a process is in a completed state and hasn't been notified before within the component's lifecycle.
This can lead to unwanted notifications. For example, if the user loads a page where a task is already complete, a notification sound will play, which is likely not the intended behavior. The notification should ideally only occur for completions that happen while the user is actively viewing the page.
I recommend reverting to the previous implementation which correctly checked for the state transition, as it was more accurate in capturing the "completion" event.
export function useTaskCompletionNotification(
executionProcesses: ExecutionProcess[]
) {
const { config } = useUserSystem();
const previousProcessesRef = useRef<Map<string, string>>(new Map());
useEffect(() => {
if (!config?.notifications.sound_enabled) {
return;
}
const currentProcesses = new Map<string, string>();
// Build current state and detect completions
executionProcesses.forEach((process) => {
const prevStatus = previousProcessesRef.current.get(process.id);
const currentStatus = process.status;
currentProcesses.set(process.id, currentStatus);
// Check if process just completed (status changed from running to completed/failed/killed)
const wasRunning = prevStatus === 'running';
const isCompleted =
currentStatus === 'completed' ||
currentStatus === 'failed' ||
currentStatus === 'killed';
if (wasRunning && isCompleted && process.run_reason === 'codingagent') {
// Task execution completed, play notification sound
// Use full volume - users can adjust system/browser volume
playNotificationSound(config.notifications.sound_file, 1.0);
}
});
// Update ref for next comparison
previousProcessesRef.current = currentProcesses;
}, [executionProcesses, config]);
}| 'no-restricted-syntax': ['warn', { | ||
| selector: 'Property[key.name="refetchInterval"][value.type="Literal"][value.value<15000]', | ||
| message: 'Polling intervals under 15s are discouraged. Use WebSocket streams or event-driven invalidation instead.', | ||
| }], |
There was a problem hiding this comment.
The warning message for the no-restricted-syntax rule regarding refetchInterval has been shortened. The previous version included a reference to docs/DATA_FETCHING.md, which is valuable for developers to understand the context and reasoning behind this rule. It would be beneficial to add this reference back to the message.
| 'no-restricted-syntax': ['warn', { | |
| selector: 'Property[key.name="refetchInterval"][value.type="Literal"][value.value<15000]', | |
| message: 'Polling intervals under 15s are discouraged. Use WebSocket streams or event-driven invalidation instead.', | |
| }], | |
| 'no-restricted-syntax': ['warn', { | |
| selector: 'Property[key.name="refetchInterval"][value.type="Literal"][value.value<15000]', | |
| message: 'Polling intervals under 15s are discouraged. Use WebSocket streams or event-driven invalidation instead. See docs/DATA_FETCHING.md', | |
| }], |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The P0 performance optimization (42s -> <1s HTTP response) introduced silent failures: start_attempt() runs in tokio::spawn() with errors only logged, returning is_attempt_running=true before knowing if the attempt actually started. This fix: - Add `Initializing` status to ExecutionProcessStatus enum - Create ExecutionProcess with Initializing status BEFORE spawning - Transition to Running on success, Failed on error - SSE automatically pushes status changes to clients Also fixes logging: - main.rs: Log success/failure of PATH cache warming (not just "warmed") - shell.rs: Restore debug/warn logging for shell failures/timeouts forge-core files changed: - crates/db/src/models/execution_process.rs: New status + helper methods - crates/server/src/routes/tasks.rs: Create EP early - crates/server/src/routes/task_attempts.rs: Same pattern - crates/services/src/services/container.rs: New start_attempt_with_process() - crates/server/src/main.rs: Better PATH warming logs - crates/utils/src/shell.rs: Restore shell error logging
namastex888
force-pushed
the
fix/publish-version-check
branch
from
b70b530 to
e490d2b
Compare
Adds migration to include 'initializing' in the execution_processes status CHECK constraint. Required for the early ExecutionProcess creation pattern that surfaces startup failures to clients. Council review: git_ops_safety test failures are environmental (pre-commit hook context), not code quality issues.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1 participant
Summary
Changes
ESLint 9 Migration
eslint.config.jsreplaces.eslintrc.cjsDeprecation Warnings Eliminated
Before: 16 deprecated subdependencies on
pnpm installAfter: 2 (unfixable - from
click-to-react-componentandforge-inspector)Performance & Cleanup
TaskCard: Wrapped inmemo()to prevent re-renders during kanban dragTaskKanbanBoard,TaskFollowUpSection: Removed debugconsole.logstatementsuseTaskCompletionNotification: Simplified from Map to Set, clearer completion logicuseConversationHistory: Fix loading indicator not clearing when process completesTaskFollowUpSection: Extract primitivehasMergedPRfor stable useMemo dependenciesTest Plan
pnpm installshows only 2 deprecated warningspnpm run lintpasses (0 errors, 7 warnings within limit)pnpm run buildcompiles successfully