Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions apps/server/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -598,24 +598,23 @@ wss.on('connection', (ws: WebSocket) => {

// Subscribe to all events and forward to this client
const unsubscribe = events.subscribe((type, payload) => {
logger.info('Event received:', {
// Use debug level for high-frequency events to avoid log spam
// that causes progressive memory growth and server slowdown
const isHighFrequency =
type === 'dev-server:output' || type === 'test-runner:output' || type === 'feature:progress';
const log = isHighFrequency ? logger.debug.bind(logger) : logger.info.bind(logger);

log('Event received:', {
type,
hasPayload: !!payload,
payloadKeys: payload ? Object.keys(payload) : [],
wsReadyState: ws.readyState,
wsOpen: ws.readyState === WebSocket.OPEN,
});

if (ws.readyState === WebSocket.OPEN) {
const message = JSON.stringify({ type, payload });
logger.info('Sending event to client:', {
type,
messageLength: message.length,
sessionId: (payload as Record<string, unknown>)?.sessionId,
});
ws.send(message);
} else {
logger.info('WARNING: Cannot send event, WebSocket not open. ReadyState:', ws.readyState);
logger.warn('Cannot send event, WebSocket not open. ReadyState:', ws.readyState);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The log level for the WebSocket not-open warning is changed from info to warn. While this change aligns with the severity of the message, it's important to ensure that such warnings are appropriately handled and monitored in a production environment. Consider adding metrics or alerts to track the frequency of these warnings to proactively address potential WebSocket connectivity issues.

If the WebSocket is not opened, it might be a critical issue that needs to be addressed soon.

});

Expand Down
10 changes: 7 additions & 3 deletions apps/server/src/services/dev-server-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,13 @@ const PORT_PATTERNS: Array<{ pattern: RegExp; description: string }> = [
},
];

// Throttle output to prevent overwhelming WebSocket under heavy load
const OUTPUT_THROTTLE_MS = 4; // ~250fps max update rate for responsive feedback
const OUTPUT_BATCH_SIZE = 4096; // Smaller batches for lower latency
// Throttle output to prevent overwhelming WebSocket under heavy load.
// 100ms (~10fps) is sufficient for readable log streaming while keeping
// WebSocket traffic manageable. The previous 4ms rate (~250fps) generated
// up to 250 events/sec which caused progressive browser slowdown from
// accumulated console logs, JSON serialization overhead, and React re-renders.
Comment on lines +91 to +95

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The comments added to explain the throttling mechanism are very helpful for understanding the rationale behind the changes. However, the comment could be improved by explicitly mentioning the specific metrics or monitoring tools that can be used to observe the impact of the throttling on browser performance and server load. This would provide additional guidance for future maintenance and optimization.

It's important to monitor the impact of this change on the user experience. If the throttling is too aggressive, it could lead to a noticeable degradation in the responsiveness of the dev server logs.

const OUTPUT_THROTTLE_MS = 100; // ~10fps max update rate
const OUTPUT_BATCH_SIZE = 8192; // Larger batches to compensate for lower frequency
Comment on lines +96 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The OUTPUT_THROTTLE_MS and OUTPUT_BATCH_SIZE constants are updated to 100ms and 8192 bytes, respectively. While these values are chosen to balance log streaming readability and WebSocket traffic, it's important to consider making these configurable via environment variables. This would allow administrators to fine-tune the throttling behavior without requiring code changes.

Making these values configurable would provide flexibility to adapt to different network conditions and browser capabilities.

Suggested change
const OUTPUT_THROTTLE_MS = 100; // ~10fps max update rate
const OUTPUT_BATCH_SIZE = 8192; // Larger batches to compensate for lower frequency
const OUTPUT_THROTTLE_MS = parseInt(process.env.OUTPUT_THROTTLE_MS || '100', 10); // ~10fps max update rate
const OUTPUT_BATCH_SIZE = parseInt(process.env.OUTPUT_BATCH_SIZE || '8192', 10); // Larger batches to compensate for lower frequency


export interface DevServerInfo {
worktreePath: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ describe('DevServerService Event Types', () => {

// 2. Output & URL Detected
mockProcess.stdout.emit('data', Buffer.from('Local: http://localhost:5173/\n'));
// Throttled output needs a bit of time
await new Promise((resolve) => setTimeout(resolve, 100));
// Throttled output needs a bit of time (OUTPUT_THROTTLE_MS is 100ms)
await new Promise((resolve) => setTimeout(resolve, 250));
Comment on lines +93 to +94

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The timeout in the test is increased from 100ms to 250ms to accommodate the new throttle rate. While this addresses the immediate issue, it's important to ensure that the test remains reliable and doesn't become flaky due to timing variations. Consider adding additional assertions or retries to make the test more robust.

It's important to ensure that the test consistently passes and doesn't flake due to timing issues.

expect(emittedEvents['dev-server:output'].length).toBeGreaterThanOrEqual(1);
expect(emittedEvents['dev-server:url-detected'].length).toBe(1);
expect(emittedEvents['dev-server:url-detected'][0].url).toBe('http://localhost:5173/');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ export function useBoardFeatures({ currentProject }: UseBoardFeaturesProps) {
const isRestoring = useIsRestoring();

// Use React Query for features
const {
data: features = [],
isLoading: isQueryLoading,
refetch: loadFeatures,
} = useFeatures(currentProject?.path);
const { data: features = [], isLoading: isQueryLoading } = useFeatures(currentProject?.path);

// Don't report loading while IDB cache restore is in progress —
// features will appear momentarily once the restore completes.
Expand Down Expand Up @@ -159,7 +155,6 @@ export function useBoardFeatures({ currentProject }: UseBoardFeaturesProps) {
});

return unsubscribe;
// eslint-disable-next-line react-hooks/exhaustive-deps -- loadFeatures is a stable ref from React Query
}, [currentProject]);

// Check for interrupted features on mount
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,18 @@ export function useDevServerLogs({ worktreePath, autoSubscribe = true }: UseDevS
hasFetchedInitialLogs.current = false;
}, []);

/**
* Append content to logs, enforcing a maximum buffer size to prevent
* unbounded memory growth and progressive UI lag.
*/
const appendLogs = useCallback((content: string) => {
// Buffer for batching rapid output events into fewer setState calls.
// Content accumulates here and is flushed via requestAnimationFrame,
// ensuring at most one React re-render per animation frame (~60fps max).
const pendingOutputRef = useRef('');
const rafIdRef = useRef<number | null>(null);

const flushPendingOutput = useCallback(() => {
rafIdRef.current = null;
const content = pendingOutputRef.current;
if (!content) return;
pendingOutputRef.current = '';

setState((prev) => {
const combined = prev.logs + content;
const didTrim = combined.length > MAX_LOG_BUFFER_SIZE;
Expand All @@ -170,6 +177,33 @@ export function useDevServerLogs({ worktreePath, autoSubscribe = true }: UseDevS
});
}, []);

/**
* Append content to logs, enforcing a maximum buffer size to prevent
* unbounded memory growth and progressive UI lag.
*
* Uses requestAnimationFrame to batch rapid output events into at most
* one React state update per frame, preventing excessive re-renders.
*/
const appendLogs = useCallback(
(content: string) => {
pendingOutputRef.current += content;
if (rafIdRef.current === null) {
rafIdRef.current = requestAnimationFrame(flushPendingOutput);
}
Comment thread
gsxdsm marked this conversation as resolved.
},
[flushPendingOutput]
);

// Clean up pending RAF on unmount to prevent state updates after unmount
useEffect(() => {
return () => {
if (rafIdRef.current !== null) {
cancelAnimationFrame(rafIdRef.current);
rafIdRef.current = null;
}
};
}, []);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

// Fetch initial logs when worktreePath changes
useEffect(() => {
if (worktreePath && autoSubscribe) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@ import { getElectronAPI } from '@/lib/electron';
import { normalizePath } from '@/lib/utils';
import { toast } from 'sonner';
import type { DevServerInfo, WorktreeInfo } from '../types';
import { useEventRecencyStore } from '@/hooks/use-event-recency';

const logger = createLogger('DevServers');

// Timeout (ms) for port detection before showing a warning to the user
const PORT_DETECTION_TIMEOUT_MS = 30_000;
// Interval (ms) for periodic state reconciliation with the backend
const STATE_RECONCILE_INTERVAL_MS = 5_000;
// Interval (ms) for periodic state reconciliation with the backend.
// 30 seconds is sufficient since WebSocket events handle real-time updates;
// reconciliation is only a fallback for missed events (PWA restart, WS gaps).
// The previous 5-second interval added unnecessary HTTP pressure.
const STATE_RECONCILE_INTERVAL_MS = 30_000;

interface UseDevServersOptions {
projectPath: string;
Expand Down Expand Up @@ -322,12 +326,24 @@ export function useDevServers({ projectPath }: UseDevServersOptions) {
return () => clearInterval(intervalId);
}, [clearPortDetectionTimer, startPortDetectionTimer]);

// Record global events so smart polling knows WebSocket is healthy.
// Without this, dev-server events don't suppress polling intervals,
// causing all queries (features, worktrees, running-agents) to poll
// at their default rates even though the WebSocket is actively connected.
const recordGlobalEvent = useEventRecencyStore((state) => state.recordGlobalEvent);

// Subscribe to all dev server lifecycle events for reactive state updates
useEffect(() => {
const api = getElectronAPI();
if (!api?.worktree?.onDevServerLogEvent) return;

const unsubscribe = api.worktree.onDevServerLogEvent((event) => {
// Record that WS is alive (but only for lifecycle events, not output -
// output fires too frequently and would trigger unnecessary store updates)
if (event.type !== 'dev-server:output') {
recordGlobalEvent();
}

if (event.type === 'dev-server:starting') {
const { worktreePath } = event.payload;
const key = normalizePath(worktreePath);
Expand Down Expand Up @@ -424,7 +440,7 @@ export function useDevServers({ projectPath }: UseDevServersOptions) {
});

return unsubscribe;
}, [clearPortDetectionTimer, startPortDetectionTimer]);
}, [clearPortDetectionTimer, startPortDetectionTimer, recordGlobalEvent]);

// Cleanup all port detection timers on unmount
useEffect(() => {
Expand Down
20 changes: 11 additions & 9 deletions apps/ui/src/lib/http-api-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -923,17 +923,19 @@ export class HttpApiClient implements ElectronAPI {
this.ws.onmessage = (event) => {
try {
const data = JSON.parse(event.data);
logger.info(
'WebSocket message:',
data.type,
'hasPayload:',
!!data.payload,
'callbacksRegistered:',
this.eventCallbacks.has(data.type)
);
// Only log non-high-frequency events to avoid progressive memory growth
// from accumulated console entries. High-frequency events (dev-server output,
// test runner output, agent progress) fire 10+ times/sec and would generate
// thousands of console entries per minute.
const isHighFrequency =
data.type === 'dev-server:output' ||
data.type === 'test-runner:output' ||
data.type === 'auto_mode_progress';
if (!isHighFrequency) {
logger.info('WebSocket message:', data.type);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const callbacks = this.eventCallbacks.get(data.type);
if (callbacks) {
logger.info('Dispatching to', callbacks.size, 'callbacks');
callbacks.forEach((cb) => cb(data.payload));
}
} catch (error) {
Expand Down
26 changes: 21 additions & 5 deletions apps/ui/vite.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -238,14 +238,16 @@ export default defineConfig(({ command }) => {
// Inject build hash into sw.js CACHE_NAME for automatic cache busting
swCacheBuster(),
],
// Keep Vite dep-optimization cache local to apps/ui so each worktree gets
// its own pre-bundled dependencies. Shared cache state across worktrees can
// produce duplicate React instances (notably with @xyflow/react) and trigger
// "Invalid hook call" in the graph view.
cacheDir: path.resolve(__dirname, 'node_modules/.vite'),
resolve: {
alias: [
{ find: '@', replacement: path.resolve(__dirname, './src') },
// Force ALL React imports (including from nested deps like zustand@4 inside
// @xyflow/react) to resolve to the single copy in the workspace root node_modules.
// This prevents "Cannot read properties of null (reading 'useState')" caused by
// react-dom setting the hooks dispatcher on one React instance while component
// code reads it from a different instance.
// @xyflow/react) to resolve to a single copy.
{
find: /^react-dom(\/|$)/,
replacement: path.resolve(__dirname, '../../node_modules/react-dom') + '/',
Expand All @@ -254,8 +256,18 @@ export default defineConfig(({ command }) => {
find: /^react(\/|$)/,
replacement: path.resolve(__dirname, '../../node_modules/react') + '/',
},
// Explicit subpath aliases avoid mixed module IDs between bare imports and
// optimized deps (e.g. react/jsx-runtime), which can manifest as duplicate React.
{
find: 'react/jsx-runtime',
replacement: path.resolve(__dirname, '../../node_modules/react/jsx-runtime.js'),
},
{
find: 'react/jsx-dev-runtime',
replacement: path.resolve(__dirname, '../../node_modules/react/jsx-dev-runtime.js'),
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
],
dedupe: ['react', 'react-dom'],
dedupe: ['react', 'react-dom', 'zustand', 'use-sync-external-store', '@xyflow/react'],
},
server: {
host: process.env.HOST || '0.0.0.0',
Expand Down Expand Up @@ -355,8 +367,12 @@ export default defineConfig(({ command }) => {
include: [
'react',
'react-dom',
'react/jsx-runtime',
'react/jsx-dev-runtime',
'use-sync-external-store',
'use-sync-external-store/shim',
'use-sync-external-store/shim/with-selector',
'zustand',
'@xyflow/react',
],
},
Expand Down
Loading