refactor: Migrate all templates to createFileSync() pattern#62
Conversation
✅ Deploy Preview for agent-native-fw ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder has reviewed your changes and found 3 potential issues.
Review Details
Code Review Summary — PR #62
This PR migrates all 8 templates (analytics, brand, calendar, content, imagegen, mail, slides, videos) to the createFileSync() pattern established in PR #57. Each template gets an async createAppServer(), a diagnostic endpoint, SSE extra-emitters, graceful shutdown, and conflict notification. The approach is architecturally sound and the documentation updates are thorough and accurate.
Risk: Standard (🟡) — Core server infrastructure across 8 templates; no auth/payment changes.
Key Findings
🔴 HIGH:
- Route collision in calendar — The new
/api/eventsSSE route shadows the existingGET /api/eventsevents-list endpoint (present but not visible in the diff). In H3, the later registration wins, solistEventsis replaced by the SSE stream, breaking the calendar's event list UI even when sync is disabled.
🟡 MEDIUM:
- SIGTERM handler in factory —
process.on("SIGTERM", ...)is registered insidecreateAppServer(). This fires beforecreateProductionServer()'s own shutdown handler (server.close(...)), causing abrupt exit instead of graceful connection draining. Additionally, repeated calls (tests, dev hot-reload) accumulate listeners up to Node'sMaxListenersExceededWarninglimit. - Missing
.catch()on node-build.ts — All 8 templates now use.then()without.catch(). IfcreateFileSync()rejects (e.g. an unhandled exception), Node emits an unhandled rejection and the server silently never starts. - Conflict notification overwrites silently —
fs.writeFileSync("application-state/sync-conflict.json", ...)unconditionally replaces any existing conflict. Two rapid conflicts lose the first; aconflict-resolvedevent for conflict A can delete the file while it describes conflict B.
Positives
- ✅ Opt-in design (
FILE_SYNC_ENABLED=true) ensures zero behavioral change without explicit opt-in - ✅ Content template correctly uses
contentRoot: "./content"instead of./data - ✅ Mail template correctly excludes
google-auth.jsonfrom sync patterns - ✅ AGENTS.md docs are comprehensive and consistent across all templates
Found by running 4 parallel code-review agents with randomized file ordering.
Code review by Builder.io
| ); | ||
|
|
||
| // Graceful shutdown | ||
| process.on("SIGTERM", async () => { |
There was a problem hiding this comment.
🟡 SIGTERM handler inside createAppServer() bypasses graceful shutdown and accumulates on repeated calls
The handler calls process.exit(0) before createProductionServer()'s own SIGTERM listener can call server.close(), dropping in-flight requests. Additionally, since process.on() appends a new listener on every createAppServer() call (dev hot-reload, tests), listeners accumulate causing MaxListenersExceededWarning. Move signal handling to node-build.ts or return a { app, shutdown } tuple so the caller owns lifecycle management. Affects all 8 templates.
How did I do? React with 👍 or 👎 to help me improve.
| import { createProductionServer } from "@agent-native/core/server"; | ||
| import { createAppServer } from "./index.js"; | ||
| createProductionServer(createAppServer()); | ||
| createAppServer().then((app) => createProductionServer(app)); |
There was a problem hiding this comment.
🟡 Missing .catch() on async startup — unhandled rejection if createFileSync() throws
createAppServer().then((app) => createProductionServer(app)) has no .catch(). If createAppServer() rejects (e.g. an exception inside createFileSync()), Node.js terminates with an unhandled rejection and no useful diagnostic, and the already-created file watcher is never closed. Add .catch((err) => { console.error('[startup] Fatal:', err); process.exit(1); }). Affects all 8 node-build.ts files.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Builder has reviewed your changes and found 2 potential issues.
Review Details
Code Review Summary — PR #62 (Update 2)
This review covers the full PR diff including the new packages/core/src/server/sse.ts change (widening extraEmitters from EventEmitter to an EventLike interface). The sse.ts change is correct — createSSEHandler uses .off() for cleanup and the sync emitter implements both on and off. No issues there.
Risk: Standard (🟡) — Core infrastructure across 8 templates + a core package change.
Previously reported issues (calendar route collision, SIGTERM handler, missing .catch()) remain open and unfixed.
New Findings
🔴 HIGH:
createFileSyncignorescontentRootfor config path —loadSyncConfig()has a hardcoded fallback tocontent/sync-config.json. SincecreateFileSyncnever derivessyncConfigPathfromcontentRoot, every template usingcontentRoot: "./data"silently gets empty sync patterns. Sync is effectively disabled for 7 of 8 templates (brand, mail, videos, calendar, analytics, slides, imagegen) even whenFILE_SYNC_ENABLED=true. Only thecontenttemplate accidentally works because./contentmatches the hardcoded default. Found independently by 2 of 4 agents.
🟡 MEDIUM:
- Conflict notification file is last-write-wins and prematurely deleted — Multiple simultaneous conflicts overwrite each other in
sync-conflict.json. Worse: anyconflict-resolvedevent unconditionally deletes the file, even if it resolved a different conflict and others are still outstanding. Found by 3 of 4 agents.
Found by running 4 parallel code-review agents with randomized file ordering.
Code review by Builder.io
| try { | ||
| if (event.type === "conflict-needs-llm") { | ||
| fs.mkdirSync("application-state", { recursive: true }); | ||
| fs.writeFileSync( |
There was a problem hiding this comment.
🟡 Conflict notification overwrites concurrent conflicts and prematurely deletes on unrelated resolves
The single sync-conflict.json file is always overwritten on conflict-needs-llm, so a second concurrent conflict silently drops the first. Worse, any conflict-resolved event unconditionally deletes the file even if it resolved a different file — leaving other active conflicts invisible to the agent. Fix: key conflict files per path (e.g. sync-conflict-${safeId}.json) or check syncResult.fileSync.conflictCount === 0 before deleting. Same pattern copied across all 8 templates.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Builder has reviewed your changes and found 1 potential issue.
Review Details
Code Review Summary — PR #62 (Update 3)
This update fixes the previously reported calendar route collision by renaming the SSE endpoint from /api/events to /api/sse. However, the client-side SSE connection was not updated to match.
Risk: Standard (🟡)
Previously reported issues (SIGTERM handler, missing .catch(), createFileSync syncConfigPath bug, conflict notification overwrite) remain open and unfixed.
New Finding
🔴 HIGH:
- Calendar client still connects to
/api/eventsfor SSE — The server was renamed to/api/ssebutuseFileWatcherincalendar/client/App.tsxhas noeventsUrloverride and defaults to/api/events. TheEventSourceopens against the calendar CRUD endpoint (which returns JSON), so real-time file-sync updates are completely broken for the calendar template. Fix: addeventsUrl: "/api/sse"to theuseFileWatchercall incalendar/client/App.tsx.
Found by running 1 of 4 code-review agents (others aborted due to concurrent PR update).
Code review by Builder.io
| // SSE uses /api/sse to avoid collision with /api/events (calendar CRUD) | ||
| router.get( | ||
| "/api/sse", | ||
| createSSEHandler(watcher, { extraEmitters, contentRoot: "./data" }), |
There was a problem hiding this comment.
🔴 SSE renamed to /api/sse but calendar client still connects to /api/events — real-time updates broken
The server registers SSE at /api/sse to avoid collision with the calendar CRUD /api/events route, but calendar/client/App.tsx calls useFileWatcher with no eventsUrl override. The hook defaults to /api/events, so the EventSource connects to the JSON CRUD endpoint instead of the SSE stream — real-time updates are silently broken. Fix: add eventsUrl: "/api/sse" to the useFileWatcher call in App.tsx.
How did I do? React with 👍 or 👎 to help me improve.
|
Resolved comments:
|
Wire createFileSync() into analytics, brand, calendar, content, imagegen, mail, slides, and videos templates — matching the pattern established in the default template (PR #57). Per template: - Make createAppServer() async - Update node-build.ts to .then() pattern - Add createFileSync() wiring with diagnostic endpoint, SSE extraEmitters, graceful shutdown, and conflict notification - Add/update AGENTS.md with standard File Sync section - Create sync-config.json where missing (brand, mail, videos) Templates that had no SSE (analytics, calendar, slides, videos) now get createFileWatcher + createSSEHandler. Content template correctly uses contentRoot: "./content". Mail template syncs ./data only (not ./application-state). Slides keeps existing /api/decks/events alongside new /api/events. Dead Firestore/initFileSync docs replaced in calendar and slides AGENTS.md. Sync remains opt-in — apps work identically without FILE_SYNC_ENABLED.
… compatibility The SSEHandlerOptions type required concrete EventEmitter but createFileSync returns TypedEventEmitter which wraps EventEmitter without extending it. Define an EventLike interface with just on/off signatures (the only methods the SSE handler uses) so both types work.
gopinav
force-pushed
the
feat/migrate-templates-file-sync
branch
from
4c83f76 to
638e3c3
Compare
There was a problem hiding this comment.
Builder has reviewed your changes and found 1 potential issue.
Review Details
Code Review Summary — PR #62 (Update 4)
This update fixes the createFileSync syncConfigPath bug (now correctly derives the path from contentRoot). Good fix. Previously reported issues (SIGTERM handler, missing .catch(), conflict overwrite, calendar client /api/sse mismatch) remain open.
Risk: Standard (🟡)
New Finding
🔴 HIGH:
- Slides client never connects to
/api/events— The slides template adds a new/api/eventsSSE endpoint with thesyncResult.sseEmitterwired through it, but the slides client (DeckContext.tsx) hardcodesnew EventSource("/api/decks/events")and never subscribes to/api/events. Sync events and conflict notifications are written to disk but never propagate to the browser. The newcreateFileWatcher("./data")andsyncResult.sseEmitteron/api/eventshave zero subscribers. Fix: wiresyncResult.sseEmitterinto the existing/api/decks/eventshandler, or add a secondEventSource("/api/events")subscription inDeckContext.tsx.
Found by 1 of 4 code-review agents (others aborted due to concurrent PR update).
Code review by Builder.io
| @@ -133,5 +162,36 @@ export function createAppServer() { | |||
| }), | |||
| ); | |||
|
|
|||
There was a problem hiding this comment.
🔴 Slides client uses /api/decks/events but sync SSE is on /api/events — sync events never reach the browser
The slides client (DeckContext.tsx) hardcodes new EventSource("/api/decks/events") and never subscribes to the new /api/events endpoint. All sync events and conflict notifications written through syncResult.sseEmitter are silently dropped. Fix: wire syncResult.sseEmitter into the existing /api/decks/events handler, or add a useFileWatcher call with eventsUrl: "/api/events" on the client.
How did I do? React with 👍 or 👎 to help me improve.
1 participant
Summary
createFileSync()wiring matching the default template pattern from PR feat: Improve File Sync DX — hardening, factory, Convex adapter, docs #57createAppServer(),.then()node-build, diagnostic endpoint (/api/file-sync/status), SSE extraEmitters, graceful shutdown, and conflict notificationcreateFileWatcher+createSSEHandlerinitFileSync()docs in calendar and slides AGENTS.md replaced with correctcreateFileSync()documentationsync-config.jsonfiles created for brand, mail, and videos templatescontentRoot: "./content"(not./data)./dataonly —./application-stateis ephemeral and excluded/api/decks/eventsalongside new/api/eventsfor file syncTesting
pnpm buildpasses — all 13 workspace packages build successfullyFILE_SYNC_ENABLED=true— no behavioral change without it