Conversation
📝 WalkthroughWalkthroughThis pull request introduces support for a composer-host module bundle alongside the existing standalone build. New client files ( 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.js (1)
166-178:⚠️ Potential issue | 🟠 Major
SERVE_DIR=moduledoes not match the documented/simulations/{id}/...URLs.With the current join,
/simulations/foo/content.htmlresolves tomodule/simulations/foo/content.html. The new docs describe the module artifacts undermodule/, so composer-host requests will 404 unless the proxy strips/simulations/{id}/first. Either normalize that prefix here or document the rewrite requirement explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.js` around lines 166 - 178, The static-serving code builds filePath from pathName which leaves URLs like /simulations/{id}/... mapping to STATIC_DIR/simulations/{id}/..., causing 404 when STATIC_DIR is the module root; update the handler to detect and normalize the simulations prefix before joining (e.g. if pathName matches ^/simulations/[^/]+/ then strip the leading /simulations/{id} segment from pathName when STATIC_DIR (or SERVE_DIR) points at module), then continue with the existing path.join and the subsequent path.resolve/path.relative security checks; adjust references in this block (pathName, filePath, resolveBaseDir/resolvedFilePath, serveFile) so the normalized path is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BESPOKE-TEMPLATE.md`:
- Line 6: Update the "Required Files Structure" section in BESPOKE-TEMPLATE.md
to include the new composer module files (client/standalone.js,
client/simulation-app.js, client/content.html, client/bespoke-simulation.css) so
the list matches the template behavior; locate the "Required Files Structure"
block (references to the existing app.js and server.js entries) and add those
four filenames, and also review and correct the corresponding lines/paragraphs
mentioned around 55-69 to ensure the document and AGENTS.md conventions remain
consistent.
In `@client/content.html`:
- Line 6: The fragment text in client/content.html is out of sync with the
standalone block in client/index.html; open both files and replace the markup in
client/content.html (the placeholder line referencing simulation-app.js) with
the exact inner markup from the standalone block in client/index.html so they
are identical; ensure you preserve the reference to simulation-app.js and keep
whitespace/inline elements identical to avoid drift, then run a quick diff
between the two files to confirm they match.
In `@client/index.html`:
- Line 33: The h1 still contains the template placeholder <!-- APP_NAME -->;
replace that HTML comment placeholder with your actual application name so the
user-facing title is readable (update the <h1 class="heading-small"> element to
contain the app name text instead of the <!-- APP_NAME --> comment).
In `@client/simulation-app.js`:
- Around line 27-32: The handler in bindDemo currently adds a new click listener
every time init() runs, causing duplicate 'demo:click' emits; make the binding
idempotent by ensuring the previous listener is removed or not re-added: define
a stable handler function (e.g., inside bindDemo) and call
btn.removeEventListener('click', handler) before btn.addEventListener('click',
handler), or set a marker on the element (e.g., btn.dataset.simBound) and skip
adding if already set; keep references to the 'demo:click' emit usage and the
btn element so the same handler can be reused/removed.
In `@client/standalone.js`:
- Around line 35-36: Wrap the call to init(context) inside boot() with a
try/catch (or handle the returned promise) so any thrown or rejected errors are
caught; in the catch log the error via console.error and set a short
user-visible failure message into the mount root element (e.g.,
document.getElementById(...) or the same mount root used elsewhere) so the
standalone page doesn't remain blank; update the boot function around
init(context) to perform this error handling and rendering of the fallback
message.
- Around line 12-20: flushLogs currently removes the batch immediately with
logBuffer.splice(0) and simply logs transport errors, which loses telemetry on
non-2xx or transient failures; change flushLogs to keep a copy of the entries
(e.g. const batch = logBuffer.slice(0, N)) and only remove them from logBuffer
after a successful response (check response.ok), and on failure requeue the
batch back into logBuffer and schedule a retry with exponential backoff and a
bounded retry count; reference functions/variables flushLogs, logBuffer,
flushTimer and ensure you guard against unbounded growth by enforcing a max
buffer size and maxRetries per batch.
In `@README.md`:
- Around line 5-6: The README has inconsistent docs about PORT: the module
serving example uses IS_PRODUCTION=true SERVE_DIR=module PORT=<port> node
server.js but the later environment-variable section says production ignores
PORT; update that section to state that when running the module bundle in
production mode (IS_PRODUCTION=true) the server will honor the PORT env var for
server.js serving from SERVE_DIR, and remove or correct the sentence that says
production ignores PORT so both examples consistently document PORT usage for
module deployment.
In `@server.js`:
- Around line 91-96: The response is sent before the disk append completes;
change the append to a synchronous-handled async call (use
fs.promises.appendFile or wrap fs.appendFile in a Promise) for the block that
builds "lines" from "entries" and writes to "LOG_FILE", await that write, and
only call res.writeHead/res.end with { ok: true, count: entries.length } after
the awaited append succeeds; if the append throws or the callback returns an
error, send a 5xx response (e.g., 500) with an error payload and do not report
success.
- Around line 70-79: readBody currently buffers the entire request with no
limit; add a maxBytes constant (e.g. 1-2MB) and in the req.on('data') handler
track accumulated bytes, and if accumulated > maxBytes stop reading (remove
listeners / req.destroy()) and reject the promise with an Error-like object that
includes a statusCode 413 and a clear message (e.g. 'Payload Too Large'); update
the /api/log caller to catch this rejection and respond with HTTP 413 when the
rejected error has statusCode 413 so oversized POSTs are rejected instead of
exhausting memory.
---
Outside diff comments:
In `@server.js`:
- Around line 166-178: The static-serving code builds filePath from pathName
which leaves URLs like /simulations/{id}/... mapping to
STATIC_DIR/simulations/{id}/..., causing 404 when STATIC_DIR is the module root;
update the handler to detect and normalize the simulations prefix before joining
(e.g. if pathName matches ^/simulations/[^/]+/ then strip the leading
/simulations/{id} segment from pathName when STATIC_DIR (or SERVE_DIR) points at
module), then continue with the existing path.join and the subsequent
path.resolve/path.relative security checks; adjust references in this block
(pathName, filePath, resolveBaseDir/resolvedFilePath, serveFile) so the
normalized path is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bf33a82-73fa-4eba-842c-b1b81bd6a967
📒 Files selected for processing (13)
.gitignoreAGENTS.mdBESPOKE-TEMPLATE.mdREADME.mdclient/bespoke-simulation.cssclient/content.htmlclient/index.htmlclient/simulation-app.jsclient/standalone.jspackage.jsonserver.jsvite.config.jsvite.config.module.js
| embedded applications using the Bespoke Simulation template. Follow these | ||
| instructions exactly to ensure consistency across all applications. | ||
| NOTE: Never edit this `BESPOKE-TEMPLATE.md` file. Codebase changes should be reflected in the `AGENTS.md` file. | ||
| Keep this document aligned with template behavior; `AGENTS.md` summarizes the same conventions for contributors. |
There was a problem hiding this comment.
Update the required file list to match the new module workflow.
The new composer section introduces client/standalone.js, client/simulation-app.js, client/content.html, and client/bespoke-simulation.css, but "Required Files Structure" still only lists app.js and server.js. That leaves the document internally inconsistent for new template adopters.
Based on learnings, "Maintain consistency with the template's file structure and patterns when adding new files".
Also applies to: 55-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BESPOKE-TEMPLATE.md` at line 6, Update the "Required Files Structure" section
in BESPOKE-TEMPLATE.md to include the new composer module files
(client/standalone.js, client/simulation-app.js, client/content.html,
client/bespoke-simulation.css) so the list matches the template behavior; locate
the "Required Files Structure" block (references to the existing app.js and
server.js entries) and add those four filenames, and also review and correct the
corresponding lines/paragraphs mentioned around 55-69 to ensure the document and
AGENTS.md conventions remain consistent.
| <section class="box card bespoke-sim-intro"> | ||
| <h2 class="heading-small">Your app</h2> | ||
| <p class="body-default bespoke-sim-hint"> | ||
| Replace this markup and extend <code>simulation-app.js</code>. Keep this file in sync with the standalone block in <code>index.html</code> when you use both modes. |
There was a problem hiding this comment.
Keep the fragment text identical to client/index.html to avoid drift.
Line 6 does not match the corresponding standalone block copy, so the two files are already out of sync.
Suggested sync fix
- Replace this markup and extend <code>simulation-app.js</code>. Keep this file in sync with the standalone block in <code>index.html</code> when you use both modes.
+ Replace this block when customizing; keep in sync with <code>client/content.html</code> for composer builds.As per coding guidelines, "Keep client/content.html in sync with the same inner markup as client/index.html for build:module."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Replace this markup and extend <code>simulation-app.js</code>. Keep this file in sync with the standalone block in <code>index.html</code> when you use both modes. | |
| Replace this block when customizing; keep in sync with <code>client/content.html</code> for composer builds. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/content.html` at line 6, The fragment text in client/content.html is
out of sync with the standalone block in client/index.html; open both files and
replace the markup in client/content.html (the placeholder line referencing
simulation-app.js) with the exact inner markup from the standalone block in
client/index.html so they are identical; ensure you preserve the reference to
simulation-app.js and keep whitespace/inline elements identical to avoid drift,
then run a quick diff between the two files to confirm they match.
| <!-- Navigation Header --> | ||
| <header class="header"> | ||
| <h1 class="heading-small">APP NAME</h1> | ||
| <h1 class="heading-small"><!-- APP_NAME --></h1> |
There was a problem hiding this comment.
Replace the APP_NAME placeholder with the actual app name.
Line 33 still contains a template placeholder instead of user-facing title text.
As per coding guidelines, "Replace <!-- APP_NAME --> placeholder in client/index.html with your app name."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/index.html` at line 33, The h1 still contains the template placeholder
<!-- APP_NAME -->; replace that HTML comment placeholder with your actual
application name so the user-facing title is readable (update the <h1
class="heading-small"> element to contain the app name text instead of the <!--
APP_NAME --> comment).
| function bindDemo(root, emit) { | ||
| const btn = root.querySelector('#btn-sim-demo'); | ||
| if (!btn) return; | ||
| btn.addEventListener('click', () => { | ||
| emit('demo:click', { source: 'template' }); | ||
| }); |
There was a problem hiding this comment.
Make event binding idempotent to prevent duplicate emits on re-init.
If init() runs multiple times for the same root, Line 30 adds multiple listeners and one click can emit demo:click repeatedly.
Suggested fix
function bindDemo(root, emit) {
const btn = root.querySelector('#btn-sim-demo');
- if (!btn) return;
+ if (!btn || btn.dataset.simDemoBound === 'true') return;
+ btn.dataset.simDemoBound = 'true';
btn.addEventListener('click', () => {
emit('demo:click', { source: 'template' });
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function bindDemo(root, emit) { | |
| const btn = root.querySelector('#btn-sim-demo'); | |
| if (!btn) return; | |
| btn.addEventListener('click', () => { | |
| emit('demo:click', { source: 'template' }); | |
| }); | |
| function bindDemo(root, emit) { | |
| const btn = root.querySelector('#btn-sim-demo'); | |
| if (!btn || btn.dataset.simDemoBound === 'true') return; | |
| btn.dataset.simDemoBound = 'true'; | |
| btn.addEventListener('click', () => { | |
| emit('demo:click', { source: 'template' }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/simulation-app.js` around lines 27 - 32, The handler in bindDemo
currently adds a new click listener every time init() runs, causing duplicate
'demo:click' emits; make the binding idempotent by ensuring the previous
listener is removed or not re-added: define a stable handler function (e.g.,
inside bindDemo) and call btn.removeEventListener('click', handler) before
btn.addEventListener('click', handler), or set a marker on the element (e.g.,
btn.dataset.simBound) and skip adding if already set; keep references to the
'demo:click' emit usage and the btn element so the same handler can be
reused/removed.
| function flushLogs() { | ||
| const entries = logBuffer.splice(0); | ||
| flushTimer = null; | ||
| if (entries.length === 0) return; | ||
| fetch('/api/log', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ entries }) | ||
| }).catch(err => console.error('Failed to flush logs:', err)); |
There was a problem hiding this comment.
Requeue failed log batches instead of dropping them.
logBuffer.splice(0) removes the batch before the request succeeds, and this only logs transport failures. Any transient network issue or non-2xx response will permanently lose telemetry. Keep the batch until the server acknowledges it, then add bounded retry/backoff for failed flushes.
As per coding guidelines, "Implement retry logic for network operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/standalone.js` around lines 12 - 20, flushLogs currently removes the
batch immediately with logBuffer.splice(0) and simply logs transport errors,
which loses telemetry on non-2xx or transient failures; change flushLogs to keep
a copy of the entries (e.g. const batch = logBuffer.slice(0, N)) and only remove
them from logBuffer after a successful response (check response.ok), and on
failure requeue the batch back into logBuffer and schedule a retry with
exponential backoff and a bounded retry count; reference functions/variables
flushLogs, logBuffer, flushTimer and ensure you guard against unbounded growth
by enforcing a max buffer size and maxRetries per batch.
| function boot() { | ||
| init(context); |
There was a problem hiding this comment.
Catch init(context) failures and render a fallback.
A thrown or rejected init() leaves the standalone page blank with no user-facing signal. Wrap this call so you console.error(...) and show a short failure message in the mount root.
As per coding guidelines, "Provide meaningful error messages to users" and "Log errors to console for debugging".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/standalone.js` around lines 35 - 36, Wrap the call to init(context)
inside boot() with a try/catch (or handle the returned promise) so any thrown or
rejected errors are caught; in the catch log the error via console.error and set
a short user-visible failure message into the mount root element (e.g.,
document.getElementById(...) or the same mount root used elsewhere) so the
standalone page doesn't remain blank; update the boot function around
init(context) to perform this error handling and rendering of the fallback
message.
| Each app can run as a **normal static site** (`npm run build` → `dist/`) and as a **bundle for composer hosts** (`npm run build:module` → `module/`). Hosts fetch `/simulations/{id}/content.html`, `simulation.css`, and `simulation.js`; serve the module tree with `IS_PRODUCTION=true SERVE_DIR=module PORT=<port> node server.js`. See `AGENTS.md` and `BESPOKE-TEMPLATE.md`. | ||
|
|
There was a problem hiding this comment.
Document PORT consistently for production mode.
These lines correctly show PORT=<port> for module serving, but the environment-variable section later still says production ignores PORT. Please update that section too so the deployment instructions do not contradict each other.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 5 - 6, The README has inconsistent docs about PORT:
the module serving example uses IS_PRODUCTION=true SERVE_DIR=module PORT=<port>
node server.js but the later environment-variable section says production
ignores PORT; update that section to state that when running the module bundle
in production mode (IS_PRODUCTION=true) the server will honor the PORT env var
for server.js serving from SERVE_DIR, and remove or correct the sentence that
says production ignores PORT so both examples consistently document PORT usage
for module deployment.
| function readBody(req) { | ||
| return new Promise((resolve, reject) => { | ||
| let body = ''; | ||
| req.on('data', chunk => { body += chunk.toString(); }); | ||
| req.on('end', () => { | ||
| try { resolve(JSON.parse(body)); } | ||
| catch (e) { reject(e); } | ||
| }); | ||
| req.on('error', reject); | ||
| }); |
There was a problem hiding this comment.
Cap request bodies before buffering them in memory.
readBody() concatenates the entire POST payload with no size check. A single oversized /api/log request can exhaust memory and take the process down. Reject bodies past a small limit and return 413.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server.js` around lines 70 - 79, readBody currently buffers the entire
request with no limit; add a maxBytes constant (e.g. 1-2MB) and in the
req.on('data') handler track accumulated bytes, and if accumulated > maxBytes
stop reading (remove listeners / req.destroy()) and reject the promise with an
Error-like object that includes a statusCode 413 and a clear message (e.g.
'Payload Too Large'); update the /api/log caller to catch this rejection and
respond with HTTP 413 when the rejected error has statusCode 413 so oversized
POSTs are rejected instead of exhausting memory.
| const lines = entries.map(e => JSON.stringify(e)).join('\n') + '\n'; | ||
| fs.appendFile(LOG_FILE, lines, (err) => { | ||
| if (err) console.error('Failed to write log:', err); | ||
| }); | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify({ ok: true, count: entries.length })); |
There was a problem hiding this comment.
Return success only after the append finishes.
This sends { ok: true } before fs.appendFile() completes. Disk-full or permission errors will still return 200, so callers think telemetry was persisted when it was not. Await the append and send a 5xx on write failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server.js` around lines 91 - 96, The response is sent before the disk append
completes; change the append to a synchronous-handled async call (use
fs.promises.appendFile or wrap fs.appendFile in a Promise) for the block that
builds "lines" from "entries" and writes to "LOG_FILE", await that write, and
only call res.writeHead/res.end with { ok: true, count: entries.length } after
the awaited append succeeds; if the append throws or the callback returns an
error, send a 5xx response (e.g., 500) with an error payload and do not report
success.
1 participant
No description provided.