fix: Stabilize diagrams and live render#65
Conversation
Map diagram endpoints explicitly and wait for layout stabilization so arrows render against final coordinates and remain correctly positioned in print output
Ensure @LiVe templates render through the browser path automatically and raw MDX is transformed before React in dev mode. Prevents incorrect static rendering and React parse failures.
WalkthroughThe CLI adds shared module storage, skip-module rendering, remote rendering, CSS and Mermaid processing, timing telemetry, low-priority subprocess execution, and refactored server rendering. Diagram layout readiness now waits for stable measurements, while packaging, deployment, documentation, and Markdown examples are updated. ChangesRendering and module pipeline
Diagram layout
Packaging and deployment
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)DockerfileTraceback (most recent call last): Taskfile.ymlTraceback (most recent call last): chart/templates/ingress.yamlTraceback (most recent call last):
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
src/components/diagram/Diagram.tsx (1)
42-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a ref for
onSettledto avoid theeslint-disabledirective.You can maintain a stable reference to the latest
onSettledcallback using a ref, identical to how you handledupdateArrows. This makes the effect fully compliant with the rules of hooks and allows you to remove theeslint-disable-next-linedirective without triggering unwanted re-runs.♻️ Proposed refactor
const updateArrows = useXarrow(); const updateRef = useRef(updateArrows); updateRef.current = updateArrows; + + const onSettledRef = useRef(onSettled); + onSettledRef.current = onSettled; useEffect(() => { let raf = 0; let stable = 0; let frames = 0; let last = ''; const tick = () => { const el = rootRef.current; if (!el) return; frames += 1; const r = el.getBoundingClientRect(); const sig = `${r.x},${r.y},${r.width},${r.height}`; if (sig === last) { stable += 1; } else { stable = 0; last = sig; } updateRef.current(); if (stable >= STABLE_FRAMES || frames >= MAX_FRAMES) { - onSettled(); + onSettledRef.current(); return; } raf = requestAnimationFrame(tick); }; raf = requestAnimationFrame(tick); return () => cancelAnimationFrame(raf); - // eslint-disable-next-line react-hooks/exhaustive-deps }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/diagram/Diagram.tsx` around lines 42 - 73, Use a ref for the latest onSettled callback, following the existing updateRef pattern, and invoke that ref inside the updateArrows effect. Add the ref update outside the effect, include the stable ref in the effect dependencies as appropriate, and remove the eslint-disable directive while preserving the effect’s current execution behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/utils/live-template.ts`:
- Around line 1-9: Update isLiveTemplate to read only the first 4096 bytes from
the template file via a file descriptor and fixed-size buffer, rather than
loading the entire file with readFileSync. Preserve the existing UTF-8 decoding,
line splitting, first non-empty line selection, and `@live` detection behavior,
and ensure the descriptor is closed after reading.
---
Nitpick comments:
In `@src/components/diagram/Diagram.tsx`:
- Around line 42-73: Use a ref for the latest onSettled callback, following the
existing updateRef pattern, and invoke that ref inside the updateArrows effect.
Add the ref update outside the effect, include the stable ref in the effect
dependencies as appropriate, and remove the eslint-disable directive while
preserving the effect’s current execution behavior.
🪄 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: 51e07edc-0d7e-43d5-97a7-e4e69a55fa13
📒 Files selected for processing (9)
cli/src/builders/facet-directory.tscli/src/generators/html.tscli/src/server/routes.tscli/src/utils/live-template.test.tscli/src/utils/live-template.tscli/test/fixtures/live-template.tsxcli/test/fixtures/standard-template.tsxsrc/components/diagram/Arrow.tsxsrc/components/diagram/Diagram.tsx
| import { readFileSync } from 'node:fs'; | ||
|
|
||
| export function isLiveTemplate(templatePath: string): boolean { | ||
| const firstLine = readFileSync(templatePath, 'utf-8') | ||
| .slice(0, 4096) | ||
| .split(/\r?\n/) | ||
| .find((line) => line.trim() !== ''); | ||
| return /^\s*\/\/\s*@live\b/.test(firstLine ?? ''); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Read only the required bytes to improve performance.
Using readFileSync loads the entire file into memory before slicing it. For large templates or MDX documents, this creates an unnecessary memory allocation spike. You can read just the first 4096 bytes using a file descriptor to improve efficiency and scalability while maintaining the exact same behavior.
⚡ Proposed fix using a fixed-size buffer
-import { readFileSync } from 'node:fs';
+import { openSync, readSync, closeSync } from 'node:fs';
export function isLiveTemplate(templatePath: string): boolean {
- const firstLine = readFileSync(templatePath, 'utf-8')
- .slice(0, 4096)
- .split(/\r?\n/)
- .find((line) => line.trim() !== '');
- return /^\s*\/\/\s*`@live`\b/.test(firstLine ?? '');
+ const fd = openSync(templatePath, 'r');
+ try {
+ const buffer = Buffer.alloc(4096);
+ const bytesRead = readSync(fd, buffer, 0, 4096, 0);
+ const firstLine = buffer.toString('utf-8', 0, bytesRead)
+ .split(/\r?\n/)
+ .find((line) => line.trim() !== '');
+ return /^\s*\/\/\s*`@live`\b/.test(firstLine ?? '');
+ } finally {
+ closeSync(fd);
+ }
}📝 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.
| import { readFileSync } from 'node:fs'; | |
| export function isLiveTemplate(templatePath: string): boolean { | |
| const firstLine = readFileSync(templatePath, 'utf-8') | |
| .slice(0, 4096) | |
| .split(/\r?\n/) | |
| .find((line) => line.trim() !== ''); | |
| return /^\s*\/\/\s*@live\b/.test(firstLine ?? ''); | |
| } | |
| import { openSync, readSync, closeSync } from 'node:fs'; | |
| export function isLiveTemplate(templatePath: string): boolean { | |
| const fd = openSync(templatePath, 'r'); | |
| try { | |
| const buffer = Buffer.alloc(4096); | |
| const bytesRead = readSync(fd, buffer, 0, 4096, 0); | |
| const firstLine = buffer.toString('utf-8', 0, bytesRead) | |
| .split(/\r?\n/) | |
| .find((line) => line.trim() !== ''); | |
| return /^\s*\/\/\s*`@live`\b/.test(firstLine ?? ''); | |
| } finally { | |
| closeSync(fd); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/utils/live-template.ts` around lines 1 - 9, Update isLiveTemplate to
read only the first 4096 bytes from the template file via a file descriptor and
fixed-size buffer, rather than loading the entire file with readFileSync.
Preserve the existing UTF-8 decoding, line splitting, first non-empty line
selection, and `@live` detection behavior, and ensure the descriptor is closed
after reading.
Add doctor coverage for the shared Facet module store and Tailwind package/plugin layout. This surfaces missing or incompatible dependencies and supports repair through `facet doctor --skip-modules --fix`.
Add immutable shared module stores, persistent build-key digests, Tailwind v3/v4 CSS handling, filtered Markdown transforms, and low-priority subprocess execution to reduce repeated install, scan, and render costs. Add skip-modules support and improve cache invalidation, diagnostics, and loader cleanup. BREAKING CHANGE: Replace the positional computeTemplateBuildKey API and remove FacetDirectory.needsInstall and isInstallBroken; callers must use the new options-based and module-preparation APIs.
Add Facet server rendering for HTML/PDF jobs while preserving local data loading and validation, with explicit failures instead of local fallback. Introduce version-pinned shared modules, CSS post-processing controls, render timing diagnostics, and expanded API coverage for live/PDF options. BREAKING CHANGE: Require Node.js >=20.19; shared-module server mode rejects request-specific dependencies and local-only render options.
Make browser-backed diagrams render consistently in HTML/PDF output and expand the playground with reusable datasheet, diagram, Markdown, and MDX examples. Keep shared playground links reproducible through URL-synced toolbar state and surface server timings in the log dialog.
Deprioritize shell, SSR, and PDF browser work to reduce contention with interactive workloads. Extend benchmark profiling across stdout and stderr, and support numeric logger verbosity levels.\n\nBREAKING CHANGE: PersistentLoaderRequest replaces the verbose option with verbosity
Build and install the CLI from a versioned npm package with symlinked development installs and linked-version reporting. Add shared module-cache warmup, bundled runtime assets, and repeatable Kubernetes lab deployment with optional ingress.\nBREAKING CHANGE: The npm CLI now requires Node.js >=20.19 instead of >=18.
Expand the kitchen-sink report to exercise GFM, raw HTML, GitHub alerts, collapsible details, footnotes, and Mermaid diagrams. Add the rendering dependencies and remote HTML/PDF CLI integration coverage.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| export function wrapFacetStylesInLayer(css: string): string { | ||
| const imports: string[] = []; | ||
| let remaining = css; | ||
| const leadingImport = /^((?:\s|\/\*[\s\S]*?\*\/)*@import\s+(?:"[^"]*"|'[^']*'|url\([^)]*\))[^;]*;)\s*/; |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
cli/src/utils/pdf-generator.ts (1)
558-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal variable
processshadows the Node.js globalprocess.Works today since only
.pid/.kill()are used, but shadowing the global inside this function is a footgun for future edits that needprocess.envor other global APIs here.♻️ Proposed rename
const browser = await puppeteer.launch(buildBrowserLaunchOptions({ chromePath })); - const process = browser.process(); - if (process?.pid !== undefined) { - applySpawnedProcessPriority({ pid: process.pid, kill: () => process.kill() }); + const browserProcess = browser.process(); + if (browserProcess?.pid !== undefined) { + applySpawnedProcessPriority({ pid: browserProcess.pid, kill: () => browserProcess.kill() }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/utils/pdf-generator.ts` around lines 558 - 569, Rename the local variable `process` in `launchBrowser` to a non-conflicting name such as `browserProcess`, and update its `.pid` and `.kill()` references accordingly while preserving the existing priority handling.cli/src/utils/server-render.ts (1)
93-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on remote render requests.
checkedFetch/fetchcalls to the Facet server have noAbortController/timeout. If the remote server hangs or the connection stalls, the CLI process blocks indefinitely with no way to recover short of a manual kill, even though the server enforces its ownrenderTimeout.Also applies to: 139-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/utils/server-render.ts` around lines 93 - 102, Update checkedFetch to enforce a finite timeout for every remote Facet request by creating an AbortController, scheduling its abort, and passing its signal into fetch; ensure the timer is cleared after completion and preserve the existing error wrapping and serverError handling. Apply the same timeout behavior to the additional fetch calls in the affected render flow.cli/src/bundler/module-store.ts (1)
411-422: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd native binary sentinel checks for dependencies installed with
--ignore-scripts.
installModules()always runs pnpm with--ignore-scripts, butSENTINEL_DEPENDENCIESonly checks React/Vite/Facet packages.lightningcssand Tailwind 4’s native paths depend on platform optional binaries (@tailwindcss/oxide-*,lightningcss-*), and the lockfile does not include build/postinstall scripts to run. Widening this sentinel check to covertailwindcss,@tailwindcss/vite,@tailwindcss/oxide, andlightningcsswould fail broken installs before Vite/Tailwind build time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/bundler/module-store.ts` around lines 411 - 422, Update verifySentinels and its SENTINEL_DEPENDENCIES configuration to include tailwindcss, `@tailwindcss/vite`, `@tailwindcss/oxide`, lightningcss, and their platform-specific optional binary packages. Ensure installations performed by installModules with --ignore-scripts fail during sentinel validation when any required native dependency is missing, while preserving the existing Facet version validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/scripts/benchmark-render.mjs`:
- Around line 41-47: Update the profile extraction map in the benchmark script
to recognize telemetry only when each line starts with the “[FACET_PROFILE] ”
marker. Replace the current substring search while preserving the existing
payload slicing, filtering, and JSON parsing flow.
In `@cli/src/bundler/module-store.ts`:
- Around line 340-372: Update maxEntries initialization in pruneModuleStores to
handle a non-numeric FACET_MODULE_STORE_ENTRIES value before using it for
slicing. Preserve the minimum limit of one, fall back to the existing default
entry count when parsing yields NaN, and ensure stores.slice receives a finite
non-negative limit so invalid configuration cannot prune every store.
In `@cli/src/generators/pdf.ts`:
- Around line 32-34: Update the relative template path calculation near
consumerRoot and templatePath to use basename(templatePath) instead of string
replacement with a hardcoded slash, ensuring the returned fragment path is
cross-platform while preserving the existing resolved template path handling.
In `@cli/src/loaders/css.ts`:
- Around line 41-64: Update the Tailwind v4 setup around the content/css/entry
file writes so each scratch path is added to scratch immediately after its
corresponding write succeeds, rather than pushing all three paths only after
setup completes. Preserve cleanup through the existing try/finally flow and
ensure partially completed setup still tracks every file already written.
In `@cli/src/utils/remote-resolver.ts`:
- Around line 192-208: Update pruneRemoteCache to validate
FACET_REMOTE_CACHE_ENTRIES before calculating maxEntries: use the parsed value
only when it is a finite positive integer, otherwise fall back to the documented
default of 20. Preserve the minimum effective limit of one and ensure invalid
input cannot cause entries.slice to start at zero and remove every non-keepDir
entry.
In `@cli/src/utils/server-render.ts`:
- Around line 14-17: Update the archiveExcludes configuration used by
renderWithServer to exclude common secret files before the consumerRoot archive
is uploaded, including .env* files, .npmrc, and credential/key material such as
*.pem and *.key. Preserve the existing build and VCS exclusions while ensuring
these patterns are applied by the tar operation.
In `@cli/test/remote-rendering.test.ts`:
- Around line 23-25: Update the remote-rendering test suite’s beforeAll setup to
capture the existing FACET_PACKAGE_PATH and FACET_LOW_PRIORITY values, then
restore those original values in an afterAll cleanup hook, including removing
variables that were previously unset.
In `@Dockerfile`:
- Around line 138-142: Restore /workspace as the final WORKDIR after the warmup
commands; update the WORKDIR directive currently setting /app so the image
retains the declared writable workspace for relative template and output paths.
In `@examples/kitchen-sink/MarkdownReport.md`:
- Line 224: Update the Mermaid rendering sentence in MarkdownReport so it reads
“renders these fences inline via its existing Chromium rendering pipeline,”
removing the erroneous period and correcting the wording.
In `@src/styles.css`:
- Around line 221-227: Update the deprecated print-break declaration in the
surrounding table-wrapper print styles to use the modern page-break equivalent,
preserving the existing automatic inside-break behavior so Stylelint passes.
In `@Taskfile.yml`:
- Around line 177-193: Update the lab Helm upgrade command in the deployment
task to replace --rollback-on-failure with the Helm 3-compatible --atomic flag,
preserving the existing wait, timeout, and rollout status behavior.
---
Nitpick comments:
In `@cli/src/bundler/module-store.ts`:
- Around line 411-422: Update verifySentinels and its SENTINEL_DEPENDENCIES
configuration to include tailwindcss, `@tailwindcss/vite`, `@tailwindcss/oxide`,
lightningcss, and their platform-specific optional binary packages. Ensure
installations performed by installModules with --ignore-scripts fail during
sentinel validation when any required native dependency is missing, while
preserving the existing Facet version validation.
In `@cli/src/utils/pdf-generator.ts`:
- Around line 558-569: Rename the local variable `process` in `launchBrowser` to
a non-conflicting name such as `browserProcess`, and update its `.pid` and
`.kill()` references accordingly while preserving the existing priority
handling.
In `@cli/src/utils/server-render.ts`:
- Around line 93-102: Update checkedFetch to enforce a finite timeout for every
remote Facet request by creating an AbortController, scheduling its abort, and
passing its signal into fetch; ensure the timer is cleared after completion and
preserve the existing error wrapping and serverError handling. Apply the same
timeout behavior to the additional fetch calls in the affected render flow.
🪄 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 Plus
Run ID: 4a31b3d6-1f7a-4a47-82e1-6359cdd53347
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (87)
.dockerignore.gitignoreDockerfileMakefileREADME.mdTaskfile.ymlchart/templates/ingress.yamlchart/values.lab.yamlchart/values.yamlcli/npm/facet-cli/README.mdcli/npm/facet-cli/package.jsoncli/package.jsoncli/scripts/benchmark-render.mjscli/scripts/build-sea.cjscli/scripts/pack-npm-cli.cjscli/scripts/pack-npm-cli.test.tscli/scripts/task-install.test.tscli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/builders/remark-config.test.tscli/src/builders/remark-config.tscli/src/bundler/build-cache.test.tscli/src/bundler/build-cache.tscli/src/bundler/live-snapshot.tscli/src/bundler/module-store.test.tscli/src/bundler/module-store.tscli/src/bundler/ssr-pool.tscli/src/bundler/vite-builder.tscli/src/bundler/vite-server.tscli/src/cli.tscli/src/commands/doctor.test.tscli/src/commands/doctor.tscli/src/generators/html.tscli/src/generators/pdf.tscli/src/loaders/css.tscli/src/loaders/ssr.tscli/src/server/archive.tscli/src/server/config.test.tscli/src/server/config.tscli/src/server/playground-controls-script.tscli/src/server/playground-examples-script.tscli/src/server/playground-html.test.tscli/src/server/playground-html.tscli/src/server/preview.tscli/src/server/render-pipeline.tscli/src/server/render-stream.tscli/src/server/request.test.tscli/src/server/request.tscli/src/server/routes.tscli/src/server/template-workspaces.tscli/src/types.tscli/src/utils/assets.test.tscli/src/utils/assets.tscli/src/utils/browser-html.test.tscli/src/utils/browser-html.tscli/src/utils/browser-readiness.tscli/src/utils/live-template.test.tscli/src/utils/live-template.tscli/src/utils/logger.test.tscli/src/utils/logger.tscli/src/utils/pdf-generator-priority.test.tscli/src/utils/pdf-generator.tscli/src/utils/pdf-security-timings.test.tscli/src/utils/pdf-security.tscli/src/utils/performance.test.tscli/src/utils/performance.tscli/src/utils/remote-resolver.tscli/src/utils/server-render.test.tscli/src/utils/server-render.tscli/src/utils/shell.tscli/src/utils/subprocess-priority.test.tscli/src/utils/subprocess-priority.tscli/src/utils/tailwind-v4.test.tscli/src/utils/tailwind.test.tscli/src/utils/tailwind.tscli/src/utils/template-source.tscli/src/version.test.tscli/src/version.tscli/test/fixtures/report-priority.cjscli/test/markdown-styles.test.tscli/test/remote-rendering.test.tscli/test/render-api.test.tsexamples/kitchen-sink/MarkdownReport.mdopenapi.yamlpackage.jsonsrc/styles.csstailwind.config.js
| const profiles = `${stdout}\n${stderr}`.split('\n') | ||
| .map((line) => { | ||
| const start = line.indexOf('[FACET_PROFILE] '); | ||
| return start < 0 ? undefined : line.slice(start + '[FACET_PROFILE] '.length); | ||
| }) | ||
| .filter(Boolean) | ||
| .map((payload) => JSON.parse(payload)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Anchor profile markers at the start of the line.
Line 43 treats any output containing [FACET_PROFILE] as telemetry. A template log such as warning: [FACET_PROFILE] ... can make JSON.parse throw and fail the benchmark. Use line.startsWith('[FACET_PROFILE] ').
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/scripts/benchmark-render.mjs` around lines 41 - 47, Update the profile
extraction map in the benchmark script to recognize telemetry only when each
line starts with the “[FACET_PROFILE] ” marker. Replace the current substring
search while preserving the existing payload slicing, filtering, and JSON
parsing flow.
| function pruneModuleStores(cacheRoot: string, keepRoot: string, logger: Logger): void { | ||
| const modulesRoot = join(cacheRoot, 'modules'); | ||
| const maxEntries = Math.max(1, parseInt(process.env['FACET_MODULE_STORE_ENTRIES'] ?? '4', 10)); | ||
| const stores: Array<{ path: string; mtimeMs: number }> = []; | ||
| let versionDirs: string[]; | ||
| try { | ||
| versionDirs = readdirSync(modulesRoot, { withFileTypes: true }) | ||
| .filter((entry) => entry.isDirectory()) | ||
| .map((entry) => join(modulesRoot, entry.name)); | ||
| for (const versionDir of versionDirs) { | ||
| for (const entry of readdirSync(versionDir, { withFileTypes: true })) { | ||
| if (!entry.isDirectory() || entry.name.includes('.tmp-')) continue; | ||
| const path = join(versionDir, entry.name); | ||
| if (path === keepRoot || existsSync(`${path}.lock`)) continue; | ||
| stores.push({ path, mtimeMs: statSync(path).mtimeMs }); | ||
| } | ||
| } | ||
| } catch { | ||
| return; | ||
| } | ||
| stores.sort((a, b) => b.mtimeMs - a.mtimeMs); | ||
| for (const stale of stores.slice(Math.max(0, maxEntries - 1))) { | ||
| try { | ||
| rmSync(stale.path, { recursive: true, force: true }); | ||
| logger.debug(`Pruned stale module store ${stale.path}`); | ||
| } catch { /* another process may be racing the same prune */ } | ||
| } | ||
| for (const versionDir of versionDirs) { | ||
| try { | ||
| if (readdirSync(versionDir).length === 0) rmSync(versionDir, { recursive: true, force: true }); | ||
| } catch { /* best effort */ } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Invalid FACET_MODULE_STORE_ENTRIES value causes pruning of every other store.
Math.max(1, parseInt(...)) returns NaN when the env var is set to a non-numeric string (parseInt returns NaN, and Math.max propagates NaN). Math.max(0, NaN - 1) is then also NaN, and Array.prototype.slice(NaN) is spec'd to behave as slice(0) — so instead of trimming to the configured limit, all non-keepRoot stores across every version/platform get rmSync'd, including stores other concurrent Facet processes may currently be using.
🛠️ Proposed fix
- const maxEntries = Math.max(1, parseInt(process.env['FACET_MODULE_STORE_ENTRIES'] ?? '4', 10));
+ const parsedEntries = parseInt(process.env['FACET_MODULE_STORE_ENTRIES'] ?? '4', 10);
+ const maxEntries = Math.max(1, Number.isFinite(parsedEntries) ? parsedEntries : 4);📝 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 pruneModuleStores(cacheRoot: string, keepRoot: string, logger: Logger): void { | |
| const modulesRoot = join(cacheRoot, 'modules'); | |
| const maxEntries = Math.max(1, parseInt(process.env['FACET_MODULE_STORE_ENTRIES'] ?? '4', 10)); | |
| const stores: Array<{ path: string; mtimeMs: number }> = []; | |
| let versionDirs: string[]; | |
| try { | |
| versionDirs = readdirSync(modulesRoot, { withFileTypes: true }) | |
| .filter((entry) => entry.isDirectory()) | |
| .map((entry) => join(modulesRoot, entry.name)); | |
| for (const versionDir of versionDirs) { | |
| for (const entry of readdirSync(versionDir, { withFileTypes: true })) { | |
| if (!entry.isDirectory() || entry.name.includes('.tmp-')) continue; | |
| const path = join(versionDir, entry.name); | |
| if (path === keepRoot || existsSync(`${path}.lock`)) continue; | |
| stores.push({ path, mtimeMs: statSync(path).mtimeMs }); | |
| } | |
| } | |
| } catch { | |
| return; | |
| } | |
| stores.sort((a, b) => b.mtimeMs - a.mtimeMs); | |
| for (const stale of stores.slice(Math.max(0, maxEntries - 1))) { | |
| try { | |
| rmSync(stale.path, { recursive: true, force: true }); | |
| logger.debug(`Pruned stale module store ${stale.path}`); | |
| } catch { /* another process may be racing the same prune */ } | |
| } | |
| for (const versionDir of versionDirs) { | |
| try { | |
| if (readdirSync(versionDir).length === 0) rmSync(versionDir, { recursive: true, force: true }); | |
| } catch { /* best effort */ } | |
| } | |
| } | |
| function pruneModuleStores(cacheRoot: string, keepRoot: string, logger: Logger): void { | |
| const modulesRoot = join(cacheRoot, 'modules'); | |
| const parsedEntries = parseInt(process.env['FACET_MODULE_STORE_ENTRIES'] ?? '4', 10); | |
| const maxEntries = Math.max(1, Number.isFinite(parsedEntries) ? parsedEntries : 4); | |
| const stores: Array<{ path: string; mtimeMs: number }> = []; | |
| let versionDirs: string[]; | |
| try { | |
| versionDirs = readdirSync(modulesRoot, { withFileTypes: true }) | |
| .filter((entry) => entry.isDirectory()) | |
| .map((entry) => join(modulesRoot, entry.name)); | |
| for (const versionDir of versionDirs) { | |
| for (const entry of readdirSync(versionDir, { withFileTypes: true })) { | |
| if (!entry.isDirectory() || entry.name.includes('.tmp-')) continue; | |
| const path = join(versionDir, entry.name); | |
| if (path === keepRoot || existsSync(`${path}.lock`)) continue; | |
| stores.push({ path, mtimeMs: statSync(path).mtimeMs }); | |
| } | |
| } | |
| } catch { | |
| return; | |
| } | |
| stores.sort((a, b) => b.mtimeMs - a.mtimeMs); | |
| for (const stale of stores.slice(Math.max(0, maxEntries - 1))) { | |
| try { | |
| rmSync(stale.path, { recursive: true, force: true }); | |
| logger.debug(`Pruned stale module store ${stale.path}`); | |
| } catch { /* another process may be racing the same prune */ } | |
| } | |
| for (const versionDir of versionDirs) { | |
| try { | |
| if (readdirSync(versionDir).length === 0) rmSync(versionDir, { recursive: true, force: true }); | |
| } catch { /* best effort */ } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/bundler/module-store.ts` around lines 340 - 372, Update maxEntries
initialization in pruneModuleStores to handle a non-numeric
FACET_MODULE_STORE_ENTRIES value before using it for slicing. Preserve the
minimum limit of one, fall back to the existing default entry count when parsing
yields NaN, and ensure stores.slice receives a finite non-negative limit so
invalid configuration cannot prune every store.
| const consumerRoot = dirname(resolve(filePath)); | ||
| const templatePath = resolve(filePath); | ||
| const relPath = templatePath.replace(consumerRoot + '/', ''); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm basename is imported in pdf.ts and inspect the header/footer path handling.
fd -t f 'pdf.ts' cli/src/generators --exec rg -nP "from 'path'|basename|relPath|buildAndExtractFragment" {}Repository: flanksource/facet
Length of output: 764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and call sites.
sed -n '1,140p' cli/src/generators/pdf.ts
# Run deterministic Node path-library probe for POSIX and Windows-style inputs.
node - <<'JS'
const pathPosix = require('path').posix;
function relPathWithSlashing(filePath, Sep) {
const consumerRoot = pathPosix.dirname(filePath).replaceAll('/', Sep);
const templatePath = filePath.replaceAll('/', Sep);
const relPath = templatePath.replace(consumerRoot + Sep, '');
const joined = pathPosix.join(consumerRoot, relPath);
return { consumerRoot, relPath, joined };
}
console.log('posix:', relPathWithSlashing('/foo/bar/baz.pdf', '/'));
console.log('win32-like:', relPathWithSlashing('/foo/bar/baz.pdf', '\\'));
console.log('basename:', pathPosix.basename('/foo/bar/baz.pdf'));
JSRepository: flanksource/facet
Length of output: 4863
Use a cross-platform relative path for the template fragment.
templatePath.replace(consumerRoot + '/', '') only works when consumerRoot uses /, so header/footer templates built on other platforms can keep an absolute templatePath and produce an invalid joined template path. Since consumerRoot === dirname(resolve(filePath)), use basename(templatePath) for the returned relative path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/generators/pdf.ts` around lines 32 - 34, Update the relative template
path calculation near consumerRoot and templatePath to use
basename(templatePath) instead of string replacement with a hardcoded slash,
ensuring the returned fragment path is cross-platform while preserving the
existing resolved template path handling.
| if (tailwindMajor === 4) { | ||
| let pluginPath: string; | ||
| try { | ||
| pluginPath = facetRequire.resolve('@tailwindcss/vite'); | ||
| } catch { | ||
| throw new Error('Tailwind CSS v4 requires @tailwindcss/vite in the consumer project or Facet build dependencies'); | ||
| } | ||
| const tailwindModule = await import(pathToFileURL(pluginPath).href); | ||
| plugins.push(tailwindModule.default()); | ||
| // Point the generated v4 sheet at this build's own rendered-content file so | ||
| // concurrent builds with different class sets don't overwrite each other. | ||
| const contentName = `rendered-content-${uid}.html`; | ||
| const cssName = `post-process-v4-${uid}.css`; | ||
| writeFileSync(join(facetRoot, contentName), options.content ?? '<div></div>', 'utf-8'); | ||
| writeFileSync( | ||
| join(facetRoot, cssName), | ||
| readFileSync(join(facetRoot, 'post-process-v4.css'), 'utf-8') | ||
| .replace('@source "./rendered-content.html";', `@source "./${contentName}";`), | ||
| 'utf-8', | ||
| ); | ||
| entry = join(facetRoot, `post-process-v4-${uid}.entry.ts`); | ||
| writeFileSync(entry, `import './${cssName}';\n`, 'utf-8'); | ||
| scratch.push(join(facetRoot, contentName), join(facetRoot, cssName), entry); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Scratch files can leak if setup fails before all three writes succeed.
scratch.push(...) for the v4 branch only runs once all three writes succeed (line 63), but the try/finally cleanup only wraps the build() call (starting line 66). If readFileSync/writeFileSync fails partway through (e.g. post-process-v4.css missing or a transient I/O error), already-written files like the content file are never tracked and never removed.
🐛 Proposed fix: track each scratch file as soon as it's written
writeFileSync(join(facetRoot, contentName), options.content ?? '<div></div>', 'utf-8');
+ scratch.push(join(facetRoot, contentName));
writeFileSync(
join(facetRoot, cssName),
readFileSync(join(facetRoot, 'post-process-v4.css'), 'utf-8')
.replace('`@source` "./rendered-content.html";', `@source "./${contentName}";`),
'utf-8',
);
+ scratch.push(join(facetRoot, cssName));
entry = join(facetRoot, `post-process-v4-${uid}.entry.ts`);
writeFileSync(entry, `import './${cssName}';\n`, 'utf-8');
- scratch.push(join(facetRoot, contentName), join(facetRoot, cssName), entry);
+ scratch.push(entry);📝 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.
| if (tailwindMajor === 4) { | |
| let pluginPath: string; | |
| try { | |
| pluginPath = facetRequire.resolve('@tailwindcss/vite'); | |
| } catch { | |
| throw new Error('Tailwind CSS v4 requires @tailwindcss/vite in the consumer project or Facet build dependencies'); | |
| } | |
| const tailwindModule = await import(pathToFileURL(pluginPath).href); | |
| plugins.push(tailwindModule.default()); | |
| // Point the generated v4 sheet at this build's own rendered-content file so | |
| // concurrent builds with different class sets don't overwrite each other. | |
| const contentName = `rendered-content-${uid}.html`; | |
| const cssName = `post-process-v4-${uid}.css`; | |
| writeFileSync(join(facetRoot, contentName), options.content ?? '<div></div>', 'utf-8'); | |
| writeFileSync( | |
| join(facetRoot, cssName), | |
| readFileSync(join(facetRoot, 'post-process-v4.css'), 'utf-8') | |
| .replace('@source "./rendered-content.html";', `@source "./${contentName}";`), | |
| 'utf-8', | |
| ); | |
| entry = join(facetRoot, `post-process-v4-${uid}.entry.ts`); | |
| writeFileSync(entry, `import './${cssName}';\n`, 'utf-8'); | |
| scratch.push(join(facetRoot, contentName), join(facetRoot, cssName), entry); | |
| } | |
| if (tailwindMajor === 4) { | |
| let pluginPath: string; | |
| try { | |
| pluginPath = facetRequire.resolve('`@tailwindcss/vite`'); | |
| } catch { | |
| throw new Error('Tailwind CSS v4 requires `@tailwindcss/vite` in the consumer project or Facet build dependencies'); | |
| } | |
| const tailwindModule = await import(pathToFileURL(pluginPath).href); | |
| plugins.push(tailwindModule.default()); | |
| // Point the generated v4 sheet at this build's own rendered-content file so | |
| // concurrent builds with different class sets don't overwrite each other. | |
| const contentName = `rendered-content-${uid}.html`; | |
| const cssName = `post-process-v4-${uid}.css`; | |
| writeFileSync(join(facetRoot, contentName), options.content ?? '<div></div>', 'utf-8'); | |
| scratch.push(join(facetRoot, contentName)); | |
| writeFileSync( | |
| join(facetRoot, cssName), | |
| readFileSync(join(facetRoot, 'post-process-v4.css'), 'utf-8') | |
| .replace('`@source` "./rendered-content.html";', `@source "./${contentName}";`), | |
| 'utf-8', | |
| ); | |
| scratch.push(join(facetRoot, cssName)); | |
| entry = join(facetRoot, `post-process-v4-${uid}.entry.ts`); | |
| writeFileSync(entry, `import './${cssName}';\n`, 'utf-8'); | |
| scratch.push(entry); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/loaders/css.ts` around lines 41 - 64, Update the Tailwind v4 setup
around the content/css/entry file writes so each scratch path is added to
scratch immediately after its corresponding write succeeds, rather than pushing
all three paths only after setup completes. Preserve cleanup through the
existing try/finally flow and ensure partially completed setup still tracks
every file already written.
| function pruneRemoteCache(baseDir: string, keepDir: string): void { | ||
| const maxEntries = Math.max(1, parseInt(process.env['FACET_REMOTE_CACHE_ENTRIES'] ?? '20', 10)); | ||
| let entries: Array<{ path: string; mtimeMs: number }>; | ||
| try { | ||
| entries = readdirSync(baseDir, { withFileTypes: true }) | ||
| .filter((entry) => entry.isDirectory()) | ||
| .map((entry) => join(baseDir, entry.name)) | ||
| .filter((path) => path !== keepDir && existsSync(join(path, 'facet-cache.json'))) | ||
| .map((path) => ({ path, mtimeMs: statSync(join(path, 'facet-cache.json')).mtimeMs })); | ||
| } catch { | ||
| return; | ||
| } | ||
| entries.sort((a, b) => b.mtimeMs - a.mtimeMs); | ||
| for (const stale of entries.slice(Math.max(0, maxEntries - 1))) { | ||
| try { rmSync(stale.path, { recursive: true, force: true }); } catch { /* best effort */ } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalid FACET_REMOTE_CACHE_ENTRIES silently wipes the entire remote cache.
parseInt on a non-numeric env value yields NaN; Math.max(1, NaN) is still NaN, and entries.slice(NaN) is coerced to slice(0), which returns every non-keepDir entry as stale. A typo'd or malformed env var deletes the whole cache instead of falling back to the documented default of 20.
🐛 Proposed fix: fall back to the default on invalid input
- const maxEntries = Math.max(1, parseInt(process.env['FACET_REMOTE_CACHE_ENTRIES'] ?? '20', 10));
+ const parsedMaxEntries = parseInt(process.env['FACET_REMOTE_CACHE_ENTRIES'] ?? '20', 10);
+ const maxEntries = Math.max(1, Number.isNaN(parsedMaxEntries) ? 20 : parsedMaxEntries);📝 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 pruneRemoteCache(baseDir: string, keepDir: string): void { | |
| const maxEntries = Math.max(1, parseInt(process.env['FACET_REMOTE_CACHE_ENTRIES'] ?? '20', 10)); | |
| let entries: Array<{ path: string; mtimeMs: number }>; | |
| try { | |
| entries = readdirSync(baseDir, { withFileTypes: true }) | |
| .filter((entry) => entry.isDirectory()) | |
| .map((entry) => join(baseDir, entry.name)) | |
| .filter((path) => path !== keepDir && existsSync(join(path, 'facet-cache.json'))) | |
| .map((path) => ({ path, mtimeMs: statSync(join(path, 'facet-cache.json')).mtimeMs })); | |
| } catch { | |
| return; | |
| } | |
| entries.sort((a, b) => b.mtimeMs - a.mtimeMs); | |
| for (const stale of entries.slice(Math.max(0, maxEntries - 1))) { | |
| try { rmSync(stale.path, { recursive: true, force: true }); } catch { /* best effort */ } | |
| } | |
| } | |
| function pruneRemoteCache(baseDir: string, keepDir: string): void { | |
| const parsedMaxEntries = parseInt(process.env['FACET_REMOTE_CACHE_ENTRIES'] ?? '20', 10); | |
| const maxEntries = Math.max(1, Number.isNaN(parsedMaxEntries) ? 20 : parsedMaxEntries); | |
| let entries: Array<{ path: string; mtimeMs: number }>; | |
| try { | |
| entries = readdirSync(baseDir, { withFileTypes: true }) | |
| .filter((entry) => entry.isDirectory()) | |
| .map((entry) => join(baseDir, entry.name)) | |
| .filter((path) => path !== keepDir && existsSync(join(path, 'facet-cache.json'))) | |
| .map((path) => ({ path, mtimeMs: statSync(join(path, 'facet-cache.json')).mtimeMs })); | |
| } catch { | |
| return; | |
| } | |
| entries.sort((a, b) => b.mtimeMs - a.mtimeMs); | |
| for (const stale of entries.slice(Math.max(0, maxEntries - 1))) { | |
| try { rmSync(stale.path, { recursive: true, force: true }); } catch { /* best effort */ } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/utils/remote-resolver.ts` around lines 192 - 208, Update
pruneRemoteCache to validate FACET_REMOTE_CACHE_ENTRIES before calculating
maxEntries: use the parsed value only when it is a finite positive integer,
otherwise fall back to the documented default of 20. Preserve the minimum
effective limit of one and ensure invalid input cannot cause entries.slice to
start at zero and remove every non-keepDir entry.
| beforeAll(async () => { | ||
| process.env['FACET_PACKAGE_PATH'] = repositoryRoot; | ||
| process.env['FACET_LOW_PRIORITY'] = '0'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the process environment after this suite.
Lines 24-25 overwrite global environment state without restoring prior values, so later tests in the worker can inherit this fixture’s package path and priority settings.
Proposed fix
+ const previousFacetPackagePath = process.env['FACET_PACKAGE_PATH'];
+ const previousLowPriority = process.env['FACET_LOW_PRIORITY'];
+
beforeAll(async () => {
process.env['FACET_PACKAGE_PATH'] = repositoryRoot;
process.env['FACET_LOW_PRIORITY'] = '0';
@@
afterAll(async () => {
await server?.stop();
await rm(workspace, { recursive: true, force: true });
+ if (previousFacetPackagePath === undefined) delete process.env['FACET_PACKAGE_PATH'];
+ else process.env['FACET_PACKAGE_PATH'] = previousFacetPackagePath;
+ if (previousLowPriority === undefined) delete process.env['FACET_LOW_PRIORITY'];
+ else process.env['FACET_LOW_PRIORITY'] = previousLowPriority;
}, 15000);📝 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.
| beforeAll(async () => { | |
| process.env['FACET_PACKAGE_PATH'] = repositoryRoot; | |
| process.env['FACET_LOW_PRIORITY'] = '0'; | |
| const previousFacetPackagePath = process.env['FACET_PACKAGE_PATH']; | |
| const previousLowPriority = process.env['FACET_LOW_PRIORITY']; | |
| beforeAll(async () => { | |
| process.env['FACET_PACKAGE_PATH'] = repositoryRoot; | |
| process.env['FACET_LOW_PRIORITY'] = '0'; | |
| afterAll(async () => { | |
| await server?.stop(); | |
| await rm(workspace, { recursive: true, force: true }); | |
| if (previousFacetPackagePath === undefined) delete process.env['FACET_PACKAGE_PATH']; | |
| else process.env['FACET_PACKAGE_PATH'] = previousFacetPackagePath; | |
| if (previousLowPriority === undefined) delete process.env['FACET_LOW_PRIORITY']; | |
| else process.env['FACET_LOW_PRIORITY'] = previousLowPriority; | |
| }, 15000); |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/test/remote-rendering.test.ts` around lines 23 - 25, Update the
remote-rendering test suite’s beforeAll setup to capture the existing
FACET_PACKAGE_PATH and FACET_LOW_PRIORITY values, then restore those original
values in an afterAll cleanup hook, including removing variables that were
previously unset.
| WORKDIR /app/examples | ||
| RUN mkdir -p /app/.tmp | ||
| RUN FACET_PACKAGE_PATH=/app/facet.tgz facet --skip-modules html FacetReport.tsx --data simple-data.json --output /app/.tmp/warmup.html | ||
| RUN rm -f /app/.tmp/warmup.html | ||
| WORKDIR /app |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore /workspace after the warmup.
Line 142 makes /app the final runtime working directory, overriding the declared /workspace default. Relative template and output paths in container invocations will resolve under /app instead of the intended writable workspace.
Proposed fix
-WORKDIR /app
+WORKDIR /workspace📝 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.
| WORKDIR /app/examples | |
| RUN mkdir -p /app/.tmp | |
| RUN FACET_PACKAGE_PATH=/app/facet.tgz facet --skip-modules html FacetReport.tsx --data simple-data.json --output /app/.tmp/warmup.html | |
| RUN rm -f /app/.tmp/warmup.html | |
| WORKDIR /app | |
| WORKDIR /app/examples | |
| RUN mkdir -p /app/.tmp | |
| RUN FACET_PACKAGE_PATH=/app/facet.tgz facet --skip-modules html FacetReport.tsx --data simple-data.json --output /app/.tmp/warmup.html | |
| RUN rm -f /app/.tmp/warmup.html | |
| WORKDIR /workspace |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` around lines 138 - 142, Restore /workspace as the final WORKDIR
after the warmup commands; update the WORKDIR directive currently setting /app
so the image retains the declared writable workspace for relative template and
output paths.
|
|
||
| ## Mermaid Diagrams | ||
|
|
||
| GitHub-compatible Mermaid diagrams use a fenced code block with the `mermaid` language identifier. Facet renders these fences to inline. its existing Chromium rendering pipeline: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Mermaid rendering sentence.
Line 224 says “renders these fences to inline. its”; use “renders these fences inline via its existing Chromium rendering pipeline.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/kitchen-sink/MarkdownReport.md` at line 224, Update the Mermaid
rendering sentence in MarkdownReport so it reads “renders these fences inline
via its existing Chromium rendering pipeline,” removing the erroneous period and
correcting the wording.
| /* margin-top: 8mm; */ | ||
| page-break-inside: auto; | ||
| } | ||
|
|
||
| /* Ensure table wrappers also respect page margins */ | ||
| .overflow-x-auto { | ||
| margin-top: 8mm !important; | ||
| margin-top: 8mm; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the deprecated print-break property.
Line 222 triggers Stylelint’s property-no-deprecated error; use the modern equivalent so stylesheet linting passes.
Proposed fix
- page-break-inside: auto;
+ break-inside: auto;📝 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.
| /* margin-top: 8mm; */ | |
| page-break-inside: auto; | |
| } | |
| /* Ensure table wrappers also respect page margins */ | |
| .overflow-x-auto { | |
| margin-top: 8mm !important; | |
| margin-top: 8mm; | |
| /* margin-top: 8mm; */ | |
| break-inside: auto; | |
| } | |
| /* Ensure table wrappers also respect page margins */ | |
| .overflow-x-auto { | |
| margin-top: 8mm; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 222-222: Expected "page-break-inside" to be "break-inside" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/styles.css` around lines 221 - 227, Update the deprecated print-break
declaration in the surrounding table-wrapper print styles to use the modern
page-break equivalent, preserving the existing automatic inside-break behavior
so Stylelint passes.
Source: Linters/SAST tools
| - >- | ||
| helm upgrade --install {{.APP_NAME}} {{.CHART_DIR}} | ||
| --kube-context {{.LAB_CONTEXT}} | ||
| --namespace {{.LAB_NAMESPACE}} | ||
| --create-namespace | ||
| --values {{.CHART_DIR}}/values.lab.yaml | ||
| --set-string image.repository={{.LAB_REGISTRY}}/{{.APP_NAME}} | ||
| --set-string image.tag={{.DEPLOY_VERSION}} | ||
| --set-string image.pullPolicy=Always | ||
| --rollback-on-failure | ||
| --wait | ||
| --timeout 10m | ||
| - >- | ||
| kubectl --context {{.LAB_CONTEXT}} | ||
| rollout status deployment/{{.APP_NAME}} | ||
| --namespace {{.LAB_NAMESPACE}} | ||
| --timeout 5m |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
minimum Helm version that introduced --rollback-on-failure flag
💡 Result:
The --rollback-on-failure flag was introduced in Helm 4.0.0 as a replacement for the deprecated --atomic flag [1][2][3]. This change was implemented to provide clearer terminology for the functionality, which automatically reverts a failed installation or upgrade [1]. While the functionality remains similar, the new flag is the standard in Helm 4 [2][3].
Citations:
- 1: Rename 'atomic' -> 'rollback-on-failure' helm/helm#13629
- 2: https://helm.sh/docs/overview/
- 3: https://helm.sh/blog/helm-v3-end-of-life/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Taskfile references to helm image/version/context =="
rg -n "helm|Helm|helmfile|kubepi|LAB_|CHART_DIR|kube-context|kubectl" Taskfile.yml .github 2>/dev/null || true
echo
echo "== Helm references in tracked files =="
rg -n "helm (upgrade|install|up|version)|Helm|helmfile|helm3|helm4" -g '!vendor/**' -g '!dist/**' -g '!build/**' . || trueRepository: flanksource/facet
Length of output: 4594
🌐 Web query:
helm --rollback-on-failure flag available Helm v3 version
💡 Result:
The --rollback-on-failure flag is not available in Helm v3. It was introduced in Helm 4 as a replacement for the --atomic flag [1][2]. In Helm v3, the equivalent functionality is provided by the --atomic flag [3][2][4]. When --atomic is used with helm upgrade or helm install, Helm will automatically roll back the release to the previous successful state if the operation fails [2][4]. In Helm 4, the --atomic flag is deprecated in favor of --rollback-on-failure [1][3][2]. While --atomic remains available in some contexts for backward compatibility in early Helm 4 versions, it has been removed in others (such as the install command) [2]. Summary of flag usage: - Helm v3: Use --atomic. - Helm v4: Use --rollback-on-failure (replaces --atomic) [1][3][2].
Citations:
- 1: Rename 'atomic' -> 'rollback-on-failure' helm/helm#13629
- 2: helm install with --atomic no longer works in Helm 4 helm/helm#31900
- 3: https://github.com/helm/helm/blob/827a960e/pkg/cmd/upgrade.go
- 4: https://github.com/helm/helm-www/blob/main/versioned_docs/version-3/helm/helm_upgrade.md
Use --atomic for Helm v3-compatible lab installs.
This repository is documented as supporting Helm 3.0+, and all CI Helm setup uses azure/setup-helm@v4 without pinning a Helm 4.x version. helm upgrade --install with --rollback-on-failure will fail on Helm v3; use --atomic instead unless the target lab Helm binary is guaranteed to be Helm 4+.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Taskfile.yml` around lines 177 - 193, Update the lab Helm upgrade command in
the deployment task to replace --rollback-on-failure with the Helm 3-compatible
--atomic flag, preserving the existing wait, timeout, and rollout status
behavior.
2 participants
What
Why
Summary by CodeRabbit
New Features
//@live`` directive.--skip-modules/shared modules mode to reuse immutable facet modules and reduce setup work.Bug Fixes
Tests