refactor: migrate to pnpm and enhance header component flexibility#38
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
WalkthroughThis PR migrates the project from npm to pnpm, introduces a Changespnpm Migration & Package Manager Infrastructure
CLI Infrastructure & Doctor Command
FacetDirectory & Dependency Installation
Component Library Enhancements
Documentation & Examples
Possibly Related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
…dge and Avatar Introduce 'xxs' (extra extra small) as a new named size preset for Badge, Avatar, and AvatarGroup components. Refactor Badge sizing to use a named size map with consistent mm-based measurements for container padding, icon dimensions, and gaps. Add support for numeric font sizes (in pt) that interpolate layout dimensions from named presets, enabling custom typography while maintaining consistent spacing. Update ListTable to use Badge component for tags instead of inline spans, improving consistency and maintainability. Replace gray color palette with slate throughout components for better visual hierarchy. Add resolveSizeVariant utility to safely resolve variant lookups with fallback behavior and console warnings for unknown values, preventing SSR crashes from typos. Update example documentation and package.json scripts to use pnpm instead of npm. BREAKING CHANGE: Badge size type now includes 'xxs' and number; components using hardcoded 'xs' should be reviewed for appropriate sizing.
…eader component flexibility - Replace npm with pnpm across all documentation, task definitions, and examples for improved dependency management - Add logoClass and className props to FlanksourceHeader for flexible styling and sizing - Refactor Header component to support custom className and style props with merged utility function - Remove hardcoded padding from CSS and move to component-level DEFAULT_CLASSES for better control - Update HeaderSolid example with detailed comments showing 30mm hero and 14mm compact header configurations - Add Fira Code as primary monospace font in Tailwind config - Update .gitignore to exclude .agent/ and .sandbox.yaml files These changes improve component reusability and allow per-instance header customization without CSS modifications.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cli/src/server/routes.ts (1)
497-501:⚠️ Potential issue | 🟠 MajorSilent fallback masks
pnpmmissing at runtime.Same
pnpm execconcern ascli/src/generators/html.tsLine 114 (see comment there). Additionally, the catch on Line 499–501 only emits atdebuglevel — ifpnpmisn't on PATH on the server host, every render silently falls back to the (likely incomplete) Vite CSS without operators noticing. Consider logging atwarnlevel here, or using a direct binary path as suggested in the root-cause comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/server/routes.ts` around lines 497 - 501, The catch currently swallows errors from the shell invocation of pnpm (`await $`cd ${facetRoot} && pnpm exec tailwindcss -i ${stylesInput} --content ${tempHtmlPath} -o ${outputCssPath}`.quiet()`) and only logs at debug level; change the catch to accept an error (e) and log a warning with the error details via logger.warn including which command failed and relevant variables (facetRoot, stylesInput, tempHtmlPath, outputCssPath), so missing pnpm on PATH is visible at runtime; additionally, consider using a direct binary path (or a resolved pnpm exec path) instead of relying on plain `pnpm` in the shell to avoid PATH issues..github/workflows/test.yml (1)
22-53:⚠️ Potential issue | 🟠 MajorWorkflow still uses npm despite the PR's stated pnpm migration.
The PR objective is "migrate to pnpm", but this workflow continues to:
- cache against
cli/package-lock.json(line 27), which no longer exists (repository now usescli/pnpm-lock.yaml),- run
npm installandcd cli && npm install(lines 46–47),- run
cd cli && npm run build:lib(line 53).Since the repository has migrated to pnpm (evidenced by
pnpm-lock.yamlat root and cli, and"packageManager": "pnpm@8.13.1"in package.json), this workflow will fail: the cache step will not find the referenced npm lockfile, andnpm installwill not respect the pnpm lockfiles.Switch the setup, cache, install, and build steps to pnpm (e.g.,
pnpm/action-setup,cache: "pnpm",pnpm install --frozen-lockfile,pnpm --filter cli build:lib).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 22 - 53, The workflow still uses npm-specific steps and cache keys (actions/setup-node with cache-dependency-path referencing cli/package-lock.json, and the commands npm install, cd cli && npm install, cd cli && npm run build:lib) but the repo uses pnpm/pnpm-lock.yaml; update the job to use pnpm (replace actions/setup-node step to include pnpm via pnpm/action-setup or install pnpm, change cache: "npm" to cache: "pnpm" and cache-dependency-path to refer to pnpm-lock.yaml files, replace npm install commands with pnpm install --frozen-lockfile (and run from root only or use pnpm --filter cli install where appropriate), and replace cd cli && npm run build:lib with pnpm --filter cli build:lib) so the CI respects pnpm lockfiles and caching.examples/kitchen-sink/UberKitchenSink.tsx (1)
1-40:⚠️ Potential issue | 🟡 MinorAdd
AIModelCardto the CLI runtime types infacet-types.ts.
AIModelCardis properly exported from the package's public API (src/components/index.tsx), but it is missing fromcli/src/server/facet-types.ts. Templates rendered through the CLI will lack TypeScript support for this component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/kitchen-sink/UberKitchenSink.tsx` around lines 1 - 40, The CLI runtime type definitions are missing the AIModelCard component type; update the facet runtime types by adding AIModelCard to the exported component/type list in facet-types.ts (the same union/interface where other components like Badge, StatCard, and Theme are declared), ensure its TypeScript type matches the public export signature (as in src/components/index.tsx), and export it so templates rendered through the CLI receive proper typings.cli/src/utils/pdf-multipass.ts (1)
499-566:⚠️ Potential issue | 🟡 MinorSilent skip when placeholders are present but
browserorhtmlis missing.When
headerHasPlaceholders.get(key)is true and eitherbrowserorhtmlisundefined, the placeholder branch is skipped,embeddedremainsundefined, and the header (or footer) is not drawn at all for that page. SinceembeddedHeaders.set(key, …)was also skipped at line 507 specifically because placeholders were detected, there is no pre-embedded fallback either.Both arguments are optional in the new signature, so a caller that passes only
pageSizeMap(or only one ofbrowser/html) silently loses headers/footers on pages that were supposed to receive page numbers. At minimum, log a warning when this combination occurs so the regression isn't invisible; consider falling back to the unsubstituted overlay (await embedOverlay(doc, buf)) so static portions still render.🛡️ Suggested guard
for (const [key, buf] of overlays.headers) { const has = htmlHasTokens || bufferHasPlaceholders(buf); headerHasPlaceholders.set(key, has); - if (!has) embeddedHeaders.set(key, await embedOverlay(doc, buf)); + if (!has || !browser || !html) embeddedHeaders.set(key, await embedOverlay(doc, buf)); } for (const [key, buf] of overlays.footers) { const has = htmlHasTokens || bufferHasPlaceholders(buf); footerHasPlaceholders.set(key, has); - if (!has) embeddedFooters.set(key, await embedOverlay(doc, buf)); + if (!has || !browser || !html) embeddedFooters.set(key, await embedOverlay(doc, buf)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/utils/pdf-multipass.ts` around lines 499 - 566, The code currently skips rendering headers/footers when headerHasPlaceholders.get(key) or footerHasPlaceholders.get(key) is true but browser or html are missing; update the branches in the header and footer handling (the blocks using headerHasPlaceholders/footerHasPlaceholders, embeddedHeaders/embeddedFooters, overlays.headers/overlays.footers, renderElementPdf, and embedOverlay) to detect this condition, emit a warning (so the regression is visible) and fall back to embedding the raw overlay (await embedOverlay(doc, buf)) so static parts still render; apply the same change for both the header and footer paths and ensure the fallback assigns embedded so page.drawPage still runs.
🧹 Nitpick comments (26)
README.md (1)
402-405: Redundant explicitprepublishOnlyinvocation.
prepublishOnlyis a lifecycle script that npm/pnpm run automatically beforepublish— invoking it manually duplicates the build. Either drop the manual step (just documentnpm publish/pnpm publish) or update the comment to clarify it's only for testing the prepublish step in isolation.Minor — pre-existing nit surfaced by this diff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 402 - 405, The README currently shows an explicit pnpm run prepublishOnly followed by npm publish which duplicates npm/pnpm's automatic running of the prepublishOnly lifecycle; update that snippet to either remove the manual prepublishOnly call and show just "npm publish" / "pnpm publish", or keep the manual command but change the comment to state it's only for testing the prepublishOnly step in isolation (reference the prepublishOnly lifecycle and the npm publish / pnpm publish commands in the change).package.json (1)
62-62:source-map-supportas a runtime dep — intentional for consumers of the library?
@flanksource/facetis published as a library (permain/exports). Listingsource-map-supportunderdependenciesmeans every downstream consumer of the React component library installs it, even though it's primarily useful at the Node CLI runtime. If it's only needed by the CLI, consider moving it tocli/package.json'sdependencies(and out of root) so library consumers don't pull it transitively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 62, The package.json currently lists "source-map-support" as a root dependency, which makes every consumer of the published library install it; move "source-map-support" out of the root "dependencies" and into the CLI package's dependencies instead (e.g., add it to cli/package.json dependencies and remove it from the root package.json) so only the CLI runtime pulls it; update any root code that imports it to either lazy-require from the CLI entrypoint or ensure the CLI package provides the import path.src/components/PageNo.tsx (1)
9-18: Document the placeholder semantics — bare_PG_/_TL_will render in non-PDF contexts.When this component is rendered in a browser/Storybook (i.e. anywhere outside the PDF multipass that substitutes
_PG_/_TL_), users will see literal_PG_ / _TL_text. That's surprising without a doc comment. A short JSDoc on the component noting that the output is intended to be post-processed bycli/src/utils/pdf-multipass.tswould prevent confusion.📝 Proposed JSDoc
+/** + * Renders a placeholder span whose text is later substituted with the actual + * page number / total during PDF post-processing (see cli/src/utils/pdf-multipass.ts). + * In non-PDF rendering contexts the raw `_PG_`/`_TL_` markers will be visible. + */ export default function PageNo({ format = '${page} / ${total}', className, }: PageNoProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/PageNo.tsx` around lines 9 - 18, Add a short JSDoc comment above the PageNo component explaining that the format prop uses placeholders PAGE_PLACEHOLDER (`_PG_`) and TOTAL_PLACEHOLDER (`_TL_`) which are intended to be replaced later by the PDF multipass processor (see cli/src/utils/pdf-multipass.ts), and note that in non-PDF contexts (browser/Storybook) these raw placeholders will be rendered literally; reference the component name PageNo and the symbols PAGE_PLACEHOLDER and TOTAL_PLACEHOLDER so maintainers know where to look.cli/src/utils/package-manager.test.ts (1)
59-59: Use the top-levelfs/promisesimport instead of dynamicimport().
fs/promisesis already statically imported at line 2; pulling inmkdiragain viaawait import('fs/promises')inside each test adds noise without benefit.♻️ Diff
-import { mkdtemp, rm, writeFile } from 'fs/promises'; +import { mkdir, mkdtemp, rm, writeFile } from 'fs/promises'; @@ - const nested = join(workDir, 'a', 'b'); - const { mkdir } = await import('fs/promises'); - await mkdir(nested, { recursive: true }); + const nested = join(workDir, 'a', 'b'); + await mkdir(nested, { recursive: true }); @@ - const nested = join(workDir, 'child'); - const { mkdir } = await import('fs/promises'); - await mkdir(nested); + const nested = join(workDir, 'child'); + await mkdir(nested);Also applies to: 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/utils/package-manager.test.ts` at line 59, Replace the dynamic await import of 'fs/promises' and its destructured mkdir with the top-level static import already present in this test module: remove occurrences of "const { mkdir } = await import('fs/promises')" and use the statically imported mkdir symbol directly in the tests (both places where the dynamic import is used). This keeps the code consistent with the existing import at the top and removes unnecessary dynamic imports.cli/src/utils/package-manager.ts (1)
79-79: Nit:?? rawis unreachable —String.prototype.splitalways returns a non-empty array.
raw.split('@')[0]isstring(neverundefined), so the nullish fallback is dead. Either drop it, or simplify withpkg.packageManager.split('@', 1)[0]. Functionally fine either way.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/utils/package-manager.ts` at line 79, Nit: the nullish fallback is unreachable when computing the variable name from raw.split; remove the dead fallback or simplify the split logic. Update the code that assigns name (the variable computed from raw.split) to either drop the "?? raw" fallback entirely or use a single-item split on the packageManager field (e.g., call split with a limit of 1 on pkg.packageManager) so the result is clearly a string without needing a fallback.examples/kitchen-sink/package.json (1)
12-12: Considerlink:../../for the local facet dependency.
file:../../works with pnpm but may copy the package at install time, requiring reinstallation to reflect changes insrc/. With pnpm,link:../../creates a direct symlink and provides live updates during development, which is typically better for in-repo examples.If you set up a
pnpm-workspace.yamlat the repository root,"@flanksource/facet": "workspace:*"would be another option, but that requires additional monorepo configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/kitchen-sink/package.json` at line 12, Update the local dependency spec for "@flanksource/facet" in package.json to use a pnpm symlink instead of a file copy by replacing the value "file:../../" with "link:../../" (or use "workspace:*" if you add a pnpm-workspace.yaml); edit the dependency entry for "@flanksource/facet" so installs create a live symlink to the repo source rather than copying it.src/components/Page.tsx (1)
14-24:productprop in PageProps is unused and should be removed or wired through to rendering.The
productprop is declared in the PageProps interface but never destructured or used in the Page component. However, multiple callers are already passing this prop (examples/kitchen-sink/TypographyTest.tsx, examples/kitchen-sink/UberKitchenSink.tsx, cli/src/server/playground-html.ts), suggesting it was intended to display in the page header. Either remove it from the public API to clarify the interface, or wire it through to the title/Header rendering logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Page.tsx` around lines 14 - 24, The PageProps interface declares a product prop that is never used by the Page component; either remove it from PageProps or pass it into the header rendering—preferably wire it through: update the Page component to destructure product from its props (alongside title, titleClassName, etc.) and include it in the header/title rendering logic (e.g., render product next to or below title in the Header/title block, or pass product into the existing Header component if one is used) so callers like TypographyTest and UberKitchenSink see the product string; if you choose removal, delete product from PageProps and remove usages in callers.src/components/CalloutBox.tsx (1)
60-60: Nit: helper nameresolveSizeVariantreads oddly when applied to a non-size variant.
CalloutBox.variantis a semantic kind (info/warning/success/default), not a size. The lookup logic is identical, but naming the helper around "size" creates a small cognitive mismatch with every non-size caller (CalloutBox, and any others that key on color/theme/variant). Consider renaming toresolveVariantorresolveByKeyin a follow-up; not worth blocking here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CalloutBox.tsx` at line 60, The helper name resolveSizeVariant is misleading for semantic variants like CalloutBox.variant; rename the helper (and its exported identifier) to something generic such as resolveVariant or resolveByKey and update all usages (e.g., the call in CalloutBox that currently uses resolveSizeVariant, the helper definition, any imports/exports, and tests) so callers still pass the same args (variant, variantStyles, default, 'CalloutBox') but the function name reflects generic variant lookup; ensure types/signatures remain unchanged and update any references in other components to avoid breaking imports.cli/package.json (2)
16-28:gen-versionruns twice duringpnpm run build; also consider pinning the package manager.
prebuildrunsgen-version, thenprebuild:cliruns it again beforebuild:cli. Not a correctness issue but redundant — drop one of them (typicallyprebuild:cliis sufficient sincebuild:libdoesn't need the generated file, or rely solely onprebuildand removeprebuild:cli).- Since the PR standardizes on pnpm, adding a top-level
"packageManager": "pnpm@<version>"field makes the toolchain reproducible and lets Corepack pick it up automatically — otherwise contributors runningnpm installwon't get a clear failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/package.json` around lines 16 - 28, Remove the redundant double invocation of the gen-version script by deleting either the "prebuild:cli" script or the "prebuild" script so "gen-version" runs only once during the build (keep "prebuild:cli" if you want generation only before build:cli, or keep "prebuild" to run it before the whole build chain); and add a top-level "packageManager": "pnpm@<pin-version>" field to package.json to pin pnpm (replace <pin-version> with the project’s chosen pnpm version) so Corepack and contributors use a consistent package manager.
70-84: Remove stale Jest tooling now that tests run on Bun.
testandtest:watchhave been migrated tobun test, and the test files usebun:testimports, butjest,@types/jest, andts-jestare still listed indevDependencies. They're unreachable from any script and only add install weight and supply-chain surface. Drop them unless something outside this PR still depends on them.♻️ Proposed cleanup
"devDependencies": { "@types/bun": "^1.3.0", - "@types/jest": "^29.5.0", "@types/node": "^20.11.0", "@types/node-forge": "^1.3.14", "@types/semver": "^7.5.0", "@types/react": "^18.3.12", "@types/react-dom": "^18.3.1", - "jest": "^29.7.0", "node-forge": "^1.3.3", "pdfassembler": "^0.1.0-beta.8", - "ts-jest": "^29.1.0", "tsdown": "^0.16.0", "typescript": "^5.6.3" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/package.json` around lines 70 - 84, The devDependencies still include stale Jest packages ("jest", "@types/jest", "ts-jest") even though tests were migrated to Bun; remove those three entries from the "devDependencies" object in package.json to eliminate unused install weight and supply-chain surface, and then verify no scripts or other files reference Jest symbols before committing (look for occurrences of "jest", "@types/jest", and "ts-jest" to confirm safe removal).cli/src/utils/remote-resolver.test.ts (1)
229-242: Opt-in network test depends on a third-party package; consider pinning to a more reliable fixture.
leftpad@0.0.1is fetched live from the public registry. Even though this is gated behindFACET_TEST_NPM=1, using an unmaintained third-party package as a fixture leaves the test exposed to registry/availability flakiness, unpublishes, or supply-chain incidents. A tiny first-party fixture package (or a well-known stable package likeis-number/@flanksource/*) would be a safer long-term choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/utils/remote-resolver.test.ts` around lines 229 - 242, Replace the external leftpad fixture in the itNetwork test by pointing npmRef to a small, pinned test package maintained by us (or a highly stable public package); update the npmRef object (repoUrl and ref fields) inside the itNetwork('resolveNpmRef installs a real package via pnpm' ...) test and adjust the expected templateFile and resolvedSha assertions to match the chosen fixture, then run the test to confirm resolveRemoteRef still writes the expected file under result.consumerRoot; specifically modify the npmRef declaration and the expect(...) checks that reference result.templateFile and result.resolvedSha so they align with the new fixture package and its file layout.src/stories/components/AIModelCard.stories.tsx (1)
74-74: Minor: typeMultipleCardsasStoryfor consistency.All other stories use
Story(i.e.StoryObj<typeof meta>). BareStoryObjhere loses themetalinkage and the controls/argTypes inference, which is a small consistency wart even thoughMultipleCardsonly usesrender.♻️ Proposed change
-export const MultipleCards: StoryObj = { +export const MultipleCards: Story = { render: () => (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/components/AIModelCard.stories.tsx` at line 74, The story export MultipleCards is currently typed as StoryObj which breaks linkage to meta and argTypes; change its type to Story (i.e. StoryObj<typeof meta> or the project's alias Story) to restore proper meta inference and consistency with other stories—locate the MultipleCards export in AIModelCard.stories.tsx and update its type annotation from StoryObj to Story (or StoryObj<typeof meta> if using the generic form).src/components/utils/resolveSizeVariant.ts (1)
1-31: Optional: capwarnedKeysto avoid unbounded growth in long-running processes.
warnedKeysis a module-singletonSetthat lives for the process lifetime. Under normal use this is fine (small, finite valid+invalid key space), but in long-running SSR/dev servers a misconfigured caller passing dynamically generatedsizevalues would accumulate entries indefinitely. A trivial size cap (or LRU) prevents that without changing the “warn once” UX.♻️ Possible bound
const warnedKeys = new Set<string>(); +const MAX_WARN_KEYS = 1024; @@ - if (!warnedKeys.has(key) && size != null) { + if (!warnedKeys.has(key) && size != null) { + if (warnedKeys.size >= MAX_WARN_KEYS) warnedKeys.clear(); warnedKeys.add(key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/utils/resolveSizeVariant.ts` around lines 1 - 31, warnedKeys is an unbounded module-level Set which can grow indefinitely; replace it with a bounded, insertion-ordered store (e.g., a Map used as an LRU FIFO) and evict the oldest entry when capacity is exceeded. Concretely, change the top-level warnedKeys to a Map<string, boolean> (or similar) with a MAX_WARNED_KEYS constant, update checks in resolveSizeVariant to use warnedKeys.has(key) and warnedKeys.set(key, true), and after adding a new key, if warnedKeys.size > MAX_WARNED_KEYS remove the oldest entry by iterating warnedKeys.keys().next().value and deleting it so the “warn once” behavior is preserved without unbounded memory growth.src/components/Header.tsx (1)
17-21: Tailwind class merging: userclassNamewon't reliably overrideDEFAULT_CLASSESfor overlapping utilities.With Tailwind's JIT, utilities at the same layer have equal CSS specificity, so the last rule emitted in the generated stylesheet wins — not the last token in the className string. A consumer passing
className="px-[10mm]"to overridepx-[5mm]fromDEFAULT_CLASSESmay or may not actually take effect depending on Tailwind's class ordering. Since the whole point of this refactor is per-instance customization, this can silently bite consumers.Consider using
tailwind-mergeso the right-most caller-supplied utility wins deterministically:♻️ Proposed fix using tailwind-merge
import React from 'react'; +import { twMerge } from 'tailwind-merge'; export type PageType = 'first' | 'default' | 'last'; ... const DEFAULT_CLASSES = 'py-[1mm] px-[5mm]'; -function merged(...parts: Array<string | undefined>): string { - return parts.filter(Boolean).join(' '); -} +function merged(...parts: Array<string | undefined>): string { + return twMerge(parts.filter(Boolean).join(' ')); +}Also applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Header.tsx` around lines 17 - 21, DEFAULT_CLASSES and the merged(...) helper use naive string concat so consumer className like "px-[10mm]" may not override "px-[5mm]" reliably; replace the simple join with tailwind-merge to deterministically resolve conflicts. Import the twMerge (or merge from 'tailwind-merge') and change merged to call twMerge(DEFAULT_CLASSES, ...parts) (and update any other usage at the other occurrence mentioned around the merged call), ensuring the caller-supplied utilities win when appropriate.src/stories/components/DatasheetTemplate.stories.tsx (1)
26-53: Optional: extract the repeated margins literal.The same
{ top: 10, bottom: 10, left: 10, right: 10 }is repeated four times. Lifting it to a constant (e.g.,const MARGINS = { top: 10, bottom: 10, left: 10, right: 10 } as const;) would make future tweaks a single-line change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/components/DatasheetTemplate.stories.tsx` around lines 26 - 53, Extract the repeated margins object into a single constant (e.g., MARGINS) at the top of the story and replace every inline `{ top: 10, bottom: 10, left: 10, right: 10 }` with that constant in the Page usages; specifically update the Page props used in the default story and the MultiPage story so all four Page components reference MARGINS (you can declare it as `const MARGINS = { top: 10, bottom: 10, left: 10, right: 10 } as const`), ensuring all references to Page's margins prop use the new symbol.cli/src/bundler/vite-builder.ts (2)
97-108: Regex classification looks correct, but consider unmatched-export cases.The patterns correctly anchor on a non-
./non-/first character to exclude relative/absolute path failures. One pattern that occasionally surfaces from broken installs isSyntaxError: The requested module '<pkg>' does not provide an export named ..., which is also typically resolved by reinstalling (e.g., when a dep gets bumped under your feet). Worth adding if you see it in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/bundler/vite-builder.ts` around lines 97 - 108, Add detection for "does not provide an export named" errors to RUNTIME_REINSTALL_PATTERNS used by isMissingDepError: update the RUNTIME_REINSTALL_PATTERNS array to include a regex that matches messages like "SyntaxError: The requested module '<pkg>' does not provide an export named" (case-insensitive, allow single/double quotes and variable export names), so isMissingDepError(stderr) returns true for these broken-install export errors; ensure the new pattern follows the same non-relative/module-name anchoring style as the other entries and is pushed into the existing array constant RUNTIME_REINSTALL_PATTERNS referenced by isMissingDepError.
118-150: Loader stdout is discarded on failure.
runLoaderWithRetryonly inspectsfirstErr?.stderrwhen classifying and surfacing errors. Bun's$populates bothstdoutandstderron a non-zero exit, and Vite SSR sometimes prints the most useful frames (e.g., transformed source location) to stdout. If a non-missing-dep failure occurs, users will see only the stderr fragment in the wrappedVite SSR build failed:message and may lose the original stack context.♻️ Suggested change to include stdout in the surfaced error
- } catch (firstErr: any) { - const stderr = firstErr?.stderr?.toString?.() || ''; - if (!isMissingDepError(stderr)) { - const raw = stderr || (firstErr instanceof Error ? firstErr.message : String(firstErr)); - throw new Error(`Vite SSR build failed:\n${deduplicateOutput(raw)}`); - } + } catch (firstErr: any) { + const stderr = firstErr?.stderr?.toString?.() || ''; + const stdout = firstErr?.stdout?.toString?.() || ''; + if (!isMissingDepError(stderr) && !isMissingDepError(stdout)) { + const raw = [stderr, stdout].filter(Boolean).join('\n') + || (firstErr instanceof Error ? firstErr.message : String(firstErr)); + throw new Error(`Vite SSR build failed:\n${deduplicateOutput(raw)}`); + }Apply the same to the second-attempt catch block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/bundler/vite-builder.ts` around lines 118 - 150, The error handling in runLoaderWithRetry only inspects firstErr.stderr (and secondErr.stderr) so useful output printed to stdout by runLoaderOnce (via bun's $) is lost; change the classification and error construction to include both stdout and stderr (e.g., combine firstErr.stdout + firstErr.stderr into a single string before calling isMissingDepError and before passing to deduplicateOutput), and apply the same combination in the second catch block for secondErr so both attempts surface full output from runLoaderOnce.cli/src/commands/doctor.ts (3)
352-355:.npmrcsecret regex has redundant alternations and may false-positive on comments.
_authmatches inside_authTokenand_password(well, no —_passwordcontains no_auth, but_authTokendoes), making the first three alternations partially overlapping. More importantly, this is unanchored, so a comment like# remember to set _authToken latertriggers the path. Anchor to^after optional whitespace and split on=/:to reduce false positives:♻️ Suggested tighter detection
- const hasSecret = /_authToken|_password|_auth|certfile|keyfile/.test(raw); + const hasSecret = raw.split(/\r?\n/).some(line => { + const t = line.replace(/^\s*[#;].*/, '').trim(); + return /^([@\w./:-]+:)?(_authToken|_password|_auth|certfile|keyfile)\s*=/.test(t); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/doctor.ts` around lines 352 - 355, The current unanchored regex in the hasSecret check (/ _authToken|_password|_auth|certfile|keyfile/) can false-positive on comments and has overlapping alternations; update the test to match credential keys only at the start of a line (allowing leading whitespace) and require a separator (= or :) so comments won't trigger it. Replace the /_authToken|_password|_auth|certfile|keyfile/.test(raw) usage with a multiline-anchored pattern like /^\s*(?:_authToken|_password|_auth|certfile|keyfile)\s*(?:=|:)/m.test(raw) (put longer tokens before shorter to avoid accidental partial matches) so hasSecret only returns true for actual key assignments in doctor.ts.
356-365:.gitignoreparsing is line-exact and misses common patterns.The check accepts only
.facet,.facet/,/.facet, or/.facet/. A user who ignores via**/.facet/or who relies on a parent.gitignorewill be flagged as leaking secrets even when the directory is ignored. Consider using a smallgitignoreparser (e.g.,ignorefrom npm) orgit check-ignore:+import { execSync } from 'child_process'; +... + try { + execSync(`git -C ${JSON.stringify(consumerRoot)} check-ignore -q .facet/`, { stdio: 'ignore' }); + ignored = true; + } catch { ignored = false; }This delegates the check to git itself, which is the source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/doctor.ts` around lines 356 - 365, The current .gitignore check in doctor.ts is too strict and only matches exact lines like '.facet' or '/.facet/'; replace this ad-hoc parsing by delegating to git or a proper gitignore parser: use 'git check-ignore --no-index --quiet --path' (or spawn git check-ignore) on the path join(consumerRoot, '.facet') to determine ignore status, or install and use the 'ignore' npm package to load the .gitignore contents and test the relative path; update the logic around gitignore, gitignore variable and ignored flag so ignored accurately reflects whether the directory is ignored (including parent .gitignore and patterns like '**/.facet/') and handle errors/fallbacks when git is not available.
196-203: Native bindings check may pass when an unrelated, wrong-arch binding is co-installed.The loop iterates
CRITICAL_NATIVE_PACKAGES, finds matching entries, then assertssome(m => m.includes(expectedSuffix)). If the package has multiple arch-specific subpackages installed (e.g., both@rollup+rollup-darwin-arm64@…and@rollup+rollup-linux-x64@…got hoisted in a transferrednode_modules),some(...)returns true and the check passes — but the runtime may still load the foreign binary first and fail. Consider asserting that all matching-arch suffixes equalexpectedSuffix, or that no foreign-arch suffix is present:♻️ Tighter check
- const archMatch = matches.some(m => m.includes(expectedSuffix)); - if (!archMatch) missing.push(prefix); + const archSpecific = matches.filter(m => /-(darwin|linux|win32|freebsd)-(x64|arm64|ia32|arm)\b/.test(m)); + const goodArch = archSpecific.some(m => m.includes(expectedSuffix)); + const foreignArch = archSpecific.some(m => !m.includes(expectedSuffix)); + if (!goodArch || foreignArch) missing.push(prefix);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/doctor.ts` around lines 196 - 203, The current arch-check uses archMatch = matches.some(m => m.includes(expectedSuffix)) which can pass when a wrong-arch build is present alongside a correct one; change the logic around CRITICAL_NATIVE_PACKAGES/entries/matches/expectedSuffix so that you first collect arch-specific matches (e.g., matches.filter(m => /-\w+-\w+(@|$)/.test(m))) and then consider the package OK only if either there are no arch-specific matches or every arch-specific match ends with the expectedSuffix (i.e., archSpecific.every(m => m.includes(expectedSuffix))); if any arch-specific match has a different suffix, treat the package as missing and push to missing.cli/src/builders/facet-directory.test.ts (1)
13-20: Minor: silence info/warn in tests for cleaner output.
new Logger(false)still emitsinfo/warnper the comment. With ~15 tests exercising broken/foreign-install paths that intentionally trigger warning logs, output noise can swamp real failures. Consider a quieter test logger or capturing logs viamock.moduleif your test runner supports it. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/builders/facet-directory.test.ts` around lines 13 - 20, The tests create a Logger in beforeEach with new Logger(false) which still prints info/warn; replace this with a test-silent logger by either (A) providing a dedicated test logger that implements the same API but no-ops info/warn (e.g., replace new Logger(false) with new QuietTestLogger() or createQuietLogger()) or (B) if your test framework supports mocking, stub Logger.prototype.info and Logger.prototype.warn to no-op before tests run (or spy and restore) so that the logger variable used in beforeEach emits only errors; update the beforeEach to instantiate or install that quiet logger so tests no longer print info/warn.src/components/StatCard.tsx (1)
698-718:summaryvariant: long values can overflow the bordered card.
labelis constrained withwhitespace-nowrap overflow-hidden text-ellipsis, but the value<div>has no overflow handling. With a longformattedValue(e.g.,"$1,234,567,890.12"or a long string), the parent'smin-w-*will be exceeded and the card grows past its intended report-band width — defeating the "compact KPI" purpose. Consider mirroring the label clamping on the value too:♻️ Optional clamp
- <div className={`font-bold ${colorClasses.text} ${sizeClasses.summaryValueText} ${conditionalClasses || ''} ${valueClassName || ''}`.trim()}> + <div className={`font-bold ${colorClasses.text} ${sizeClasses.summaryValueText} truncate ${conditionalClasses || ''} ${valueClassName || ''}`.trim()}> {formattedValue} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/StatCard.tsx` around lines 698 - 718, The summary variant's value div (the element that renders formattedValue in the 'summary' branch of StatCard) lacks overflow/clamping and can force the card wider; update that div's class string to include the same truncation/overflow utilities used on the label (e.g., whitespace-nowrap overflow-hidden text-ellipsis or the Tailwind shorthand truncate) so long values are clipped instead of expanding the card, and ensure this change composes with valueClassName/conditionalClasses (avoid allowing those props to override the overflow behavior).src/components/ListTable.tsx (1)
167-172:diffDaysmath is sensitive to DST transitions.
(startOfToday.getTime() - dateStart.getTime()) / 86400000doesn't account for the 23-/25-hour days at DST boundaries —Math.floorcan land off-by-one for dates that straddle a transition, putting (e.g.) "exactly 7 days ago" into the 30-day week bucket on some days of the year. Consider computing the difference in calendar days directly:const msPerDay = 86_400_000; const utcToday = Date.UTC(now.getFullYear(), now.getMonth(), now.getDate()); const utcThen = Date.UTC(d.getFullYear(), d.getMonth(), d.getDate()); const diffDays = Math.round((utcToday - utcThen) / msPerDay);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListTable.tsx` around lines 167 - 172, The diffDays calculation in getTimeBucket is DST-sensitive and can produce off-by-one results; replace the current local-ms math using startOfToday/getTime with a UTC-based day difference: compute utcToday = Date.UTC(now.getFullYear(), now.getMonth(), now.getDate()) and utcThen = Date.UTC(d.getFullYear(), d.getMonth(), d.getDate()), set msPerDay = 86400000 and compute diffDays = Math.round((utcToday - utcThen) / msPerDay); update the code around the diffDays variable in getTimeBucket to use these UTC values to ensure correct calendar-day differences across DST transitions.examples/kitchen-sink/FindingsReport.tsx (1)
421-432: Default metadata is a fallback for missingdata.metadata, not for an empty object.
data.metadata ? data.metadata as ReportMetadata : {…defaults}substitutes the defaults only whendata.metadatais falsy. If a caller passes an explicitmetadata: {}(empty but truthy) the report will render with no reporting window, classification, etc., and the footer/header strings will collapse to bare defaults like"Internal"and the current date. Probably fine for an example, but worth being aware of if this file is the template consumers copy from.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/kitchen-sink/FindingsReport.tsx` around lines 421 - 432, The metadata fallback currently treats any truthy data.metadata (including an empty object) as authoritative; update the logic in FindingsReport to merge defaults with any provided metadata so missing fields are filled and an empty object doesn't wipe defaults. Locate the metadata variable in FindingsReport and replace the ternary check on data.metadata with a merge pattern that defines a defaults object (reportingWindow, environment, classification, audience, preparedBy, scope) and then spreads ...(data.metadata as Partial<ReportMetadata> || {}) on top, so callers can override specific fields but empty or partial metadata still inherits sensible defaults.cli/src/utils/pdf-multipass.ts (2)
434-477:replaceInPdfStreamsis unused and should be removed.This function is defined but never called anywhere in the codebase. The compositing path (lines 528-565) uses a different approach (regenerating the overlay via Puppeteer rather than rewriting PDF streams), making this dead code from an earlier design. Retaining it creates maintenance burden and divergent placeholder logic between this and
replaceInBuffer.Remove the function to clean up the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/utils/pdf-multipass.ts` around lines 434 - 477, The function replaceInPdfStreams is dead code (defined but never used) and should be removed: delete the entire async function replaceInPdfStreams(buf: Buffer, replacements: [string, string][]) and any related unused imports or constants that only support it (e.g., PDFDocument/PDFRawStream/PDFName/zlib references if they become unused after removal), keeping replaceInBuffer and the existing Puppeteer-based compositing logic as the canonical implementation.
405-419:bufferHasPlaceholdersmay inflate on every literal'x'byte.
Buffer.from([0x78])matches the byte0x78, which is also the ASCII code for'x'and appears throughout typical PDF content streams (text, names, hex strings). For each occurrence the function attemptszlib.inflateSync(buf.subarray(offset)), which throws and is caught — but the throw path through Node's zlib still does meaningful work and can dominate runtime on multi-MB overlay PDFs.Two cheaper options:
- Match the actual zlib magic more strictly (e.g.,
0x78 0x01,0x78 0x5E,0x78 0x9C,0x78 0xDA) before callinginflateSync.- Iterate
doc.context.enumerateIndirectObjects()and only inflatePDFRawStreamentries whoseFilteris/FlateDecode(the same approachreplaceInPdfStreamsalready uses at lines 437-444).Optional given that overlays are typically small, but worth noting if this ends up running on large content buffers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/utils/pdf-multipass.ts` around lines 405 - 419, bufferHasPlaceholders currently tries to inflate on every 0x78 byte which causes expensive throw paths; change the zlib detection to first check two-byte sequences (0x78 followed by one of 0x01,0x5E,0x9C,0xDA) before calling zlib.inflateSync on buf.subarray(offset), so only likely zlib streams are inflated, and keep checks for PAGE_MARKER, TOTAL_MARKER, PAGE_MARKER_HEX, TOTAL_MARKER_HEX as before; alternatively (if you prefer stream-aware scanning) restrict inflation to actual PDF stream objects by iterating doc.context.enumerateIndirectObjects() and inflating only PDFRawStream entries with Filter /FlateDecode (same approach used in replaceInPdfStreams).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43dc156a-5928-4282-abaa-23e8ef677731
⛔ Files ignored due to path filters (4)
cli/package-lock.jsonis excluded by!**/package-lock.jsoncli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackage-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (70)
.github/workflows/test.yml.gitignore.storybook/main.tsREADME.mdTaskfile.ymlcli/ARCHITECTURE.mdcli/README.mdcli/STATUS.mdcli/package.jsoncli/scripts/gen-version.cjscli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/bundler/vite-builder.test.tscli/src/bundler/vite-builder.tscli/src/cli.tscli/src/commands/doctor.tscli/src/generators/html.tscli/src/server/facet-cache.tscli/src/server/facet-types.tscli/src/server/routes.tscli/src/utils/package-manager.test.tscli/src/utils/package-manager.tscli/src/utils/pdf-generator.tscli/src/utils/pdf-multipass.tscli/src/utils/remote-resolver.test.tscli/src/utils/remote-resolver.tscli/src/version-generated.tscli/test/page-number.test.tscli/tsconfig.jsoncli/tsdown.config.tscli/vite-ssr-loader.tsexamples/kitchen-sink/BadgeExamples.tsxexamples/kitchen-sink/FindingsReport.tsxexamples/kitchen-sink/FlanksourceHeader.tsxexamples/kitchen-sink/HeaderSolid.tsxexamples/kitchen-sink/ListTableExample.tsxexamples/kitchen-sink/UberKitchenSink.tsxexamples/kitchen-sink/package.jsonexamples/kitchen-sink/sample-findings.jsonexamples/quickstart/README.mdpackage.jsonsrc/components/AIModelCard.tsxsrc/components/Avatar.tsxsrc/components/AvatarGroup.tsxsrc/components/Badge.tsxsrc/components/BulletList.tsxsrc/components/CalloutBox.tsxsrc/components/DatasheetTemplate.tsxsrc/components/Document.tsxsrc/components/Finding.tsxsrc/components/Header.tsxsrc/components/Heatmap.tsxsrc/components/ListTable.tsxsrc/components/Page.tsxsrc/components/PageNo.tsxsrc/components/ProgressBar.tsxsrc/components/ScoreGauge.tsxsrc/components/SeverityStatCard.tsxsrc/components/Shield/Shield.tsxsrc/components/StatCard.test.tsxsrc/components/StatCard.tsxsrc/components/Status.tsxsrc/components/index.tsxsrc/components/utils/resolveSizeVariant.tssrc/stories/components/AIModelCard.stories.tsxsrc/stories/components/Badge.stories.tsxsrc/stories/components/DatasheetTemplate.stories.tsxsrc/stories/components/StatCard.stories.tsxsrc/styles.csstailwind.config.js
💤 Files with no reviewable changes (6)
- cli/src/version-generated.ts
- cli/STATUS.md
- src/components/Heatmap.tsx
- src/styles.css
- cli/src/server/facet-cache.ts
- cli/ARCHITECTURE.md
| try { | ||
| // Run tailwindcss CLI: -i input.css --content "output.html" -o output.css | ||
| const tailwindCmd = $`cd ${facetRoot} && npx tailwindcss -i ${stylesInput} --content ${tempHtmlPath} -o ${outputCssPath}`; | ||
| const tailwindCmd = $`cd ${facetRoot} && pnpm exec tailwindcss -i ${stylesInput} --content ${tempHtmlPath} -o ${outputCssPath}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm tailwindcss is installed into .facet/node_modules during setup, so a direct binary path is viable
rg -nP --type=ts -C3 'tailwindcss' cli/src
fd -t f 'package.json' cli | head -20 | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'Repository: flanksource/facet
Length of output: 4671
🏁 Script executed:
# Search for where .facet directory is created and used
rg -n '\.facet' cli/src --type ts -B 2 -A 2Repository: flanksource/facet
Length of output: 32778
🏁 Script executed:
# Find where dependencies (including tailwindcss) are installed during facet setup
rg -n 'pnpm.*install|npm.*install' cli/src --type ts -B 3 -A 3 | head -60Repository: flanksource/facet
Length of output: 4374
🏁 Script executed:
# Check the facet-directory.ts file more thoroughly for the full context of the dependency list
sed -n '400,430p' cli/src/builders/facet-directory.tsRepository: flanksource/facet
Length of output: 922
🏁 Script executed:
# Check README for pnpm documentation
find . -name "README*" -o -name "readme*" | head -5 | xargs cat 2>/dev/null | grep -i pnpm -B 3 -A 3 | head -40Repository: flanksource/facet
Length of output: 656
🏁 Script executed:
# Check if pnpm is validated/checked before execution
rg -n 'resolvePackageManager|checkPnpm|pnpm.*--version' cli/src --type ts -B 2 -A 5 | head -80Repository: flanksource/facet
Length of output: 5400
🏁 Script executed:
# Verify the exact locations and context of the pnpm exec calls
sed -n '110,120p' cli/src/generators/html.ts
sed -n '490,505p' cli/src/server/routes.tsRepository: flanksource/facet
Length of output: 1064
Document pnpm as a runtime prerequisite and consider using the resolved tailwindcss binary instead of pnpm exec.
The README offers npm install -g @flanksource/facet`` as an alternative installation method without noting that pnpm must also be available at runtime. While the code enforces pnpm as a hard requirement during `.facet/` setup (resolved in `vite-builder.ts`), users installing via npm won't be aware of this runtime dependency before attempting HTML/PDF generation.
Additionally, since tailwindcss is already installed into .facet/node_modules/.bin/ during the pnpm install step, you could invoke the binary directly instead of relying on pnpm exec:
const tailwindBin = join(facetRoot, 'node_modules/.bin/tailwindcss');
const tailwindCmd = $`${tailwindBin} -i ${stylesInput} --content ${tempHtmlPath} -o ${outputCssPath}`;This eliminates the implicit assumption that pnpm is available in PATH during CSS generation and would work regardless of which package manager was used to install the CLI.
Add a note to the README install section clarifying that pnpm must be present at runtime, or apply the direct binary path approach to both cli/src/generators/html.ts (line 114) and cli/src/server/routes.ts (line 497).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/generators/html.ts` at line 114, The current CSS generation uses
`pnpm exec` which forces a runtime pnpm requirement; update
`cli/src/generators/html.ts` to resolve and call the installed tailwind binary
directly by constructing `tailwindBin` from `facetRoot` (e.g., join `facetRoot`
with `node_modules/.bin/tailwindcss`) and replace the `tailwindCmd` invocation
that currently references `stylesInput`, `tempHtmlPath`, and `outputCssPath` to
call that binary; apply the same change in `cli/src/server/routes.ts` (the
analogous tailwind invocation around line ~497) and also add a short note in the
README install section documenting that pnpm is required at runtime if you
choose not to call the local binary.
moshloop
force-pushed
the
feat/streaming-render-security
branch
from
5b11709 to
a7a45a7
Compare
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
The pnpm migration in a7a45a7 left npm references in CI/build: - test.yml setup-node cached cli/package-lock.json (no longer exists) - Dockerfile builder ran npm run build, but prebuild calls pnpm - Dockerfile final stage copied package-lock.json (no longer exists) - release.yml installed and built via npm and tried to git-add stale package-lock files Switch all of them to pnpm@8.13.1 via pnpm/action-setup, replace npm install/run with pnpm equivalents, and update lockfile paths.
pnpm 8 disables pre/post lifecycle scripts by default for security. cli/package.json relies on prebuild → gen-version to emit src/version-generated.ts; without it, the Docker build fails with "Could not resolve '../version-generated.js'". Add .npmrc with enable-pre-post-scripts=true at the root and in cli/.
facet shells out to pnpm when materialising .facet/node_modules for a template. The final image only had pnpm in the builder stage, so the in-image warmup (and any runtime PDF render) failed with: "facet requires pnpm, but it was not found on PATH". Install pnpm@8.13.1 globally in the runtime stage too.
CLI: - Complete bun test migration (remove jest deps, switch test imports to bun:test) - Add DOM to tsconfig lib (was provided transitively via jest types) - Per-pin cache in package-manager to prevent foreign-manager bypass - Reject workspace: protocol in facet overrides with descriptive error - Broaden env-var regex to case-insensitive in .npmrc parsing - Wrap doctor command in try/catch matching other CLI commands - Add shrink prop to StatCardProps - Suppress git stderr in gen-version; fix header comment filename - Drop misleading CI hint from SSR bundle preservation message Components: - Sanitize CSS inputs in Document to prevent </style> breakouts - Drop DEFAULT_CLASSES from Header children render path - Use inline styles for numeric Badge sizes (Tailwind JIT can't compile runtime arbitrary values) - Narrow Finding icon type; drop unused dot field - Add near-future/week/month bucketing to ListTable parallel to past - Fix Status aria-label fallback; move className to outer wrapper - Import Fira Code font for headless Chromium rendering Examples: - Fix FindingsReport anchor ID mismatch Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cli/src/server/facet-types.ts (1)
1574-1574: ⚡ Quick winAdd JSDoc comment for the
shrinkproperty.The new
shrink?: booleanproperty lacks documentation. For consistency with the Badge prop additions (which include JSDoc forlabelClassName,valueClassName, andwrap), add a brief JSDoc comment explaining when and why consumers should useshrink.📝 Suggested JSDoc addition
sublabel?: React.ReactNode; sublabelClassName?: string; + /** + * Reduce the card size/spacing for compact layouts. + */ shrink?: boolean; conditionalStyles?: ConditionalStyle[];🤖 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/server/facet-types.ts` at line 1574, Add a JSDoc comment for the shrink?: boolean property in the facet-types.ts type definition explaining its purpose and when to use it (e.g., "When true, reduces the component's horizontal spacing/width to fit tight layouts or inline contexts; defaults to false"). Place the comment directly above the shrink property alongside existing JSDoc for labelClassName/valueClassName/wrap so it remains consistent with the Badge prop docs and mentions expected behavior and intended use.
🤖 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 `@Dockerfile`:
- Around line 32-33: The Docker build fails because the CLI layer uses "RUN cd
cli && pnpm install --frozen-lockfile" but the cli pnpm-lock.yaml is
missing/out-of-sync or incompatible with pnpm 8.13.1; go into the CLI package
(look for the cli directory and its pnpm-lock.yaml), verify its presence and
lockfileVersion, regenerate it by removing cli/pnpm-lock.yaml and running pnpm
install inside the CLI (with the same pnpm version used in CI), then add/commit
the new pnpm-lock.yaml so the frozen lockfile install succeeds; alternatively,
if the lockfile update is not desired, remove the --frozen-lockfile flag from
the Dockerfile RUN lines to avoid failing the build.
In `@src/components/Document.tsx`:
- Around line 41-47: The regex in sanitizeCssValue and sanitizeCssBlock
(/</?\s*style/gi) misses variants with spaces between '<' and '/' or before the
tag name, allowing injection; update both functions (sanitizeCssValue and
sanitizeCssBlock) to first strip any literal angle brackets (remove all '<' and
'>') and then remove any occurrences of the word "style" as a tag/token
(case-insensitive), and ensure sanitizeCssBlock gets the same second-pass
removal of residual characters as sanitizeCssValue; verify the sanitized output
used with dangerouslySetInnerHTML (the <style> insertion) cannot contain any
angle brackets or closing style tokens.
---
Nitpick comments:
In `@cli/src/server/facet-types.ts`:
- Line 1574: Add a JSDoc comment for the shrink?: boolean property in the
facet-types.ts type definition explaining its purpose and when to use it (e.g.,
"When true, reduces the component's horizontal spacing/width to fit tight
layouts or inline contexts; defaults to false"). Place the comment directly
above the shrink property alongside existing JSDoc for
labelClassName/valueClassName/wrap so it remains consistent with the Badge prop
docs and mentions expected behavior and intended use.
🪄 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: 0cd6cc84-0c81-46f5-8f7b-7ef3353932f9
⛔ Files ignored due to path filters (4)
cli/package-lock.jsonis excluded by!**/package-lock.jsoncli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackage-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
.github/workflows/release.yml.github/workflows/test.yml.gitignore.npmrcDockerfileREADME.mdTaskfile.ymlcli/.npmrccli/ARCHITECTURE.mdcli/README.mdcli/STATUS.mdcli/package.jsoncli/scripts/gen-version.cjscli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/bundler/vite-builder.test.tscli/src/bundler/vite-builder.tscli/src/cli.tscli/src/commands/doctor.tscli/src/generators/html.tscli/src/server/facet-cache.tscli/src/server/facet-types.tscli/src/server/routes.tscli/src/utils/package-manager.test.tscli/src/utils/package-manager.tscli/src/utils/pdf-generator.tscli/src/utils/remote-resolver.test.tscli/src/utils/remote-resolver.tscli/test/jsx-parser.test.tscli/test/lint-rules.test.tscli/test/resolve-output.test.tscli/tsconfig.jsoncli/tsdown.config.tscli/vite-ssr-loader.tsexamples/kitchen-sink/BadgeExamples.tsxexamples/kitchen-sink/FindingsReport.tsxexamples/kitchen-sink/FlanksourceHeader.tsxexamples/kitchen-sink/HeaderSolid.tsxexamples/kitchen-sink/package.jsonexamples/quickstart/README.mdpackage.jsonsrc/components/Avatar.tsxsrc/components/AvatarGroup.tsxsrc/components/Badge.tsxsrc/components/BulletList.tsxsrc/components/CalloutBox.tsxsrc/components/Document.tsxsrc/components/Finding.tsxsrc/components/Header.tsxsrc/components/ListTable.tsxsrc/components/ProgressBar.tsxsrc/components/ScoreGauge.tsxsrc/components/SeverityStatCard.tsxsrc/components/Shield/Shield.tsxsrc/components/StatCard.tsxsrc/components/Status.tsxsrc/components/utils/resolveSizeVariant.tssrc/stories/components/Badge.stories.tsxsrc/styles.csstailwind.config.js
💤 Files with no reviewable changes (3)
- cli/STATUS.md
- cli/ARCHITECTURE.md
- cli/src/server/facet-cache.ts
✅ Files skipped from review due to trivial changes (11)
- cli/test/jsx-parser.test.ts
- .npmrc
- cli/.npmrc
- .gitignore
- src/components/AvatarGroup.tsx
- examples/quickstart/README.md
- cli/src/bundler/vite-builder.test.ts
- cli/README.md
- examples/kitchen-sink/package.json
- tailwind.config.js
- README.md
🚧 Files skipped from review as they are similar to previous changes (33)
- src/components/CalloutBox.tsx
- src/components/Shield/Shield.tsx
- cli/tsdown.config.ts
- src/components/Avatar.tsx
- src/components/BulletList.tsx
- examples/kitchen-sink/FlanksourceHeader.tsx
- cli/src/cli.ts
- src/components/SeverityStatCard.tsx
- src/components/ProgressBar.tsx
- cli/scripts/gen-version.cjs
- package.json
- cli/src/builders/facet-directory.test.ts
- src/components/ScoreGauge.tsx
- cli/src/utils/package-manager.test.ts
- cli/src/utils/remote-resolver.test.ts
- cli/package.json
- examples/kitchen-sink/FindingsReport.tsx
- cli/src/utils/package-manager.ts
- src/components/utils/resolveSizeVariant.ts
- cli/src/utils/remote-resolver.ts
- cli/src/commands/doctor.ts
- cli/vite-ssr-loader.ts
- cli/src/generators/html.ts
- src/components/Header.tsx
- examples/kitchen-sink/HeaderSolid.tsx
- cli/src/server/routes.ts
- examples/kitchen-sink/BadgeExamples.tsx
- src/components/Badge.tsx
- cli/src/builders/facet-directory.ts
- src/stories/components/Badge.stories.tsx
- cli/src/bundler/vite-builder.ts
- src/components/StatCard.tsx
- src/components/ListTable.tsx
…ease, harden Document.tsx sanitization
- Bump pnpm 8.13.1 -> 9.15.9 across Dockerfile, workflows, and packageManager pins to match lockfileVersion 9.0 (Docker/Helm CI was failing with ERR_PNPM_LOCKFILE_BREAKING_CHANGE).
- Pin unrun to 0.2.37 via pnpm.overrides; unrun@0.2.38 was published without its dist/ directory, breaking tsdown builds on CI.
- Regenerate root + cli pnpm-lock.yaml at lockfileVersion 9.0.
- Document.tsx: replace incomplete /<\/?\s*style/gi sanitization (bypassable via spaced "< / style >" variants flagged by CodeQL and CodeRabbit) with simple [<>{}] / [<>] stripping.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 participants
What
logoClassandclassNameprops to FlanksourceHeader for flexible stylingclassNameandstyleprops.agent/and.sandbox.yamlWhy
Summary by CodeRabbit
New Features
facet doctorCLI command performs environment preflight checks with--jsonand-v/--verboseflags.'label'variant for split label/value styling,'xxs'size, wrapping support, and custom class overrides.'summary'variant and optionalshrinkprop.'xxs'size support.Chores