chore(ci): update eslint#27
Merged
Merged
Conversation
- progress improving lint rules to simplify the complexity levels of source Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
commented
Apr 24, 2026
| 'max-lines-per-function': ['off', 50], | ||
| 'max-statements': ['off', 15], | ||
| 'max-params': ['error', 6], // goal 3 | ||
| 'max-statements': ['error', 20], // goal 15 |
Collaborator
Author
There was a problem hiding this comment.
Made a cutoff for this PR to cut down the length. Plan on followups that chip away at these last disabled rules.
johnyanarella
approved these changes
Apr 24, 2026
- fix regression with line height and text wrap Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
📝 WalkthroughWalkthroughThis pull request refactors variable and parameter naming conventions throughout the codebase for clarity and consistency, adds ESLint suppression comments to manage rule strictness, introduces helper functions to avoid parameter reassignment, updates CSS properties, and enhances ESLint configuration rules across multiple projects. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
projects/monaco/src/internal/formats/problems-format.ts (1)
178-208: 🧹 Nitpick | 🔵 TrivialAvoid function-wide ESLint disable for
appendSourceCode.Disabling
max-paramsandno-param-reassignacross the whole function is broad and can hide future regressions. Refactor to an args object and a localnextTextvariable so the suppression can be removed.♻️ Proposed refactor
-/* eslint-disable max-params, no-param-reassign */ -function appendSourceCode( - monaco: Monaco, - lineNumber: number, - decorations: monaco.editor.IModelDeltaDecoration[], - text: string, - source: string, - code: string, - target: string | { toString(): string } | undefined -): string { +interface AppendSourceCodeArgs { + monaco: Monaco; + lineNumber: number; + decorations: monaco.editor.IModelDeltaDecoration[]; + text: string; + source: string; + code: string; + target: string | { toString(): string } | undefined; +} + +function appendSourceCode({ + monaco, + lineNumber, + decorations, + text, + source, + code, + target +}: AppendSourceCodeArgs): string { if (source.length === 0 && code.length === 0) { return text; } - const sourceStart = text.length + 1; - text += source; + let nextText = text; + const sourceStart = nextText.length + 1; + nextText += source; if (code.length > 0) { - const codeStart = text.length + 1; - text += `(${code})`; + const codeStart = nextText.length + 1; + nextText += `(${code})`; if (target) { decorations.push( toRangeDecoration(monaco, lineNumber, codeStart + 1, codeStart + code.length + 1, 'problem-source-target') ); } } - decorations.push(toRangeDecoration(monaco, lineNumber, sourceStart, text.length + 1, 'problem-source-code')); - text += ' '; + decorations.push(toRangeDecoration(monaco, lineNumber, sourceStart, nextText.length + 1, 'problem-source-code')); + nextText += ' '; - return text; + return nextText; } -/* eslint-enable max-params, no-param-reassign */- text = appendSourceCode(monaco, lineNumber, decorations, text, source, code, target); + text = appendSourceCode({ monaco, lineNumber, decorations, text, source, code, target });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/monaco/src/internal/formats/problems-format.ts` around lines 178 - 208, The function appendSourceCode currently disables max-params and no-param-reassign for the whole function; refactor it to accept a single options object (e.g., { monaco, lineNumber, decorations, text, source, code, target }) instead of many positional params and stop mutating the input parameter `text` by using a local variable like `nextText` to build the string; preserve the existing logic around computing sourceStart/codeStart and pushing decorations via toRangeDecoration (and keep decorations as a mutable array), then remove the function-level eslint-disable comments.projects/site/src/docs/metrics/wireit.11ty.js (1)
86-130:⚠️ Potential issue | 🟡 MinorRemove the stale
sitecolor mapping from the colorScale object.The
colorScaleobject inwireit.ts(line 23) defines asitecolor ('#82ccd9'), but no Wireit nodes appear to use this category. The legend inwireit.11ty.jscorrectly omits a Site entry. Remove the unused color mapping to prevent confusion and keep the code aligned with actual node categories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/site/src/docs/metrics/wireit.11ty.js` around lines 86 - 130, The colorScale object in wireit.ts contains an unused 'site' mapping ('#82ccd9') that isn't referenced by any Wireit nodes and is omitted from the legend in wireit.11ty.js; remove the 'site' key from the colorScale definition in wireit.ts (the colorScale constant) so the mapping only includes actual node categories and update any related comments/tests if they reference 'site'.projects/lint/src/eslint/rules/no-unexpected-css-value.ts (1)
145-154: 🛠️ Refactor suggestion | 🟠 MajorRefactor parameter list instead of suppressing
max-params.The new disable comment bypasses the just-tightened lint policy. Prefer collapsing range/token/fallback arguments into one options object.
♻️ Suggested refactor
-// eslint-disable-next-line max-params -function checkDimensionProperty( - context: Rule.RuleContext, - node: CssDeclarationNode, - min: number, - max: number, - tokens: TokenEntry[], - fallback: string, - extraCheck?: (v: number) => boolean -) { +type DimensionCheckOptions = { + min: number; + max: number; + tokens: TokenEntry[]; + fallback: string; + extraCheck?: (v: number) => boolean; +}; + +function checkDimensionProperty( + context: Rule.RuleContext, + node: CssDeclarationNode, + options: DimensionCheckOptions +) { + const { min, max, tokens, fallback, extraCheck } = options; const child = findPixelDimensionChild(node, min, max, extraCheck); if (!child) return; const alternate = findTokenAlternate(tokens, parseInt(child.value ?? '')) ?? fallback; reportTokenViolation(context, node, { value: child.value ?? '', unit: child.unit ?? '', alternate }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/lint/src/eslint/rules/no-unexpected-css-value.ts` around lines 145 - 154, Refactor checkDimensionProperty to accept a single options object instead of many positional params: replace the params min, max, tokens, fallback, extraCheck with one options parameter (e.g., {min, max, tokens, fallback, extraCheck}) and update the function signature and body to read from that object; remove the eslint-disable-next-line max-params comment; then update all call sites of checkDimensionProperty to pass an options object (preserving the same values) so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/core/src/color/color.test.ts`:
- Around line 49-57: The test creates a Color element by hand and appends it to
document.body, which bypasses fixture helpers and risks leaking state if the
test fails; replace direct DOM ops with the project's fixture utilities: use
createFixture to instantiate the element with Color.metadata.tag, await
elementIsStable (or the provided elementIsStable helper) instead of
customElement.updateComplete, and wrap assertions in try/finally calling
removeFixture to guarantee teardown; reference the Color metadata via
Color.metadata.tag and the helpers createFixture, removeFixture, and
elementIsStable to make the change.
In `@projects/core/src/internal/controllers/type-native-popover.utils.test.ts`:
- Around line 202-205: The test never exercises the "body" anchor branch because
host.anchor is not set and the appended bodyAnchor isn't cleaned up; update the
test so host.anchor = 'body' before calling getHostAnchor(host) (ensuring the
created element has id 'body' and is appended to document.body) and remove the
appended bodyAnchor after the assertion (or use a teardown/afterEach) to avoid
leaking DOM nodes; reference the getHostAnchor function, the host.anchor
property, and the bodyAnchor element when making these changes.
In `@projects/internals/tools/src/internal/validate.ts`:
- Around line 227-252: The code in validateTemplate computes
customElementsNonBooleanAttributes incorrectly because it assumes
Object.values(allowedAttributes) yields objects with { name, values }, but some
entries are arrays (or the object shape differs), so the filter/flatMap never
extracts literal-valued custom attribute names; update the extraction to handle
all possible value shapes produced by buildAllowedAttributes: iterate
Object.values(allowedAttributes) and for each value, if it's a string skip it,
if it's an array concat its items, and if it's an object with a values property
concat that values array (use Array.isArray checks) so
customElementsNonBooleanAttributes contains all literal attribute names before
merging into nonBooleanAttributes.
In `@projects/internals/tools/src/project/setup-agent.ts`:
- Around line 174-202: Add a short docstring above configureAgentTarget
describing its purpose, accepted AgentTarget values, side effects (which
files/dirs it writes), return value, and failure behavior; then instrument the
function with structured logging that records the selected target (use
targetLabels[target]), each file/directory written (references to
writeClaudeSettings, writeElementsSkill, writeMcpJsonConfig,
writeMcpTomlConfig), and any caught errors with error objects instead of plain
strings; mirror the same structured-error logging style in reportVSCodeSetup
when writeVSCodeSettings throws (use reportVSCodeSetup and writeVSCodeSettings
names in the log). Ensure logs include explicit keys like target, action, path,
status, and error to match repo TS agent-helper conventions.
In `@projects/internals/tools/src/project/setup.ts`:
- Around line 19-36: hasExistingElementsDeps currently returns true if any
CORE_DEPENDENCIES entry exists, causing setupProject to short-circuit even when
only some Elements packages are present; update the logic to either (A) require
all CORE_DEPENDENCIES to exist by replacing .some(...) with .every(...) in
hasExistingElementsDeps so the early return only triggers when the project is
fully configured, or (B) stop short-circuiting and instead compute which
CORE_DEPENDENCIES are missing in setupProject and merge only those missing
entries into package.json and install them before returning; use the function
names hasExistingElementsDeps, setupProject and the CORE_DEPENDENCIES symbol to
locate and modify the code.
- Around line 10-17: readPackageJson currently returns the parsed JSON even when
JSON.parse yields null or a non-object value, which later causes crashes in
consumers like hasExistingElementsDeps; change readPackageJson to validate the
parsed value after JSON.parse and return 'invalid' if the result is not a plain
object (i.e., ensure typeof parsed === 'object' && parsed !== null &&
!Array.isArray(parsed)) so only proper object shapes are returned, while
preserving the existing existsSync check and try/catch behavior.
In `@projects/lint/src/eslint/rules/no-deprecated-css-variable.ts`:
- Around line 29-31: The chained AST traversal using anonymous single-letter
callbacks (?.filter(c => c.name === 'var') ?.flatMap(c => c.children) ?.find(c
=> c?.name?.includes('--mlv'))) should use descriptive parameter names to
improve readability; update the three callbacks to meaningful identifiers (e.g.,
in the filter use varNode or node, in flatMap use varNodeChildren or varNode,
and in find use valueChild or childNode) so the intent is clear when locating
the 'var' node and its children in this chain inside
no-deprecated-css-variable.ts.
In `@projects/monaco/src/internal/formats/problems-format.ts`:
- Around line 280-281: Rename the arrow callback parameter in getProblemByLine
to avoid shadowing the outer variable and remove the eslint-disable comment;
specifically, change the parameter name (e.g., from lineNumber to
queryLineNumber or line) in the getProblemByLine definition and update its usage
inside the function body so it calls problemsByLine.get(queryLineNumber) (or
.get(line)) instead of the shadowed name, then remove the //
eslint-disable-next-line no-shadow suppression.
---
Outside diff comments:
In `@projects/lint/src/eslint/rules/no-unexpected-css-value.ts`:
- Around line 145-154: Refactor checkDimensionProperty to accept a single
options object instead of many positional params: replace the params min, max,
tokens, fallback, extraCheck with one options parameter (e.g., {min, max,
tokens, fallback, extraCheck}) and update the function signature and body to
read from that object; remove the eslint-disable-next-line max-params comment;
then update all call sites of checkDimensionProperty to pass an options object
(preserving the same values) so behavior remains identical.
In `@projects/monaco/src/internal/formats/problems-format.ts`:
- Around line 178-208: The function appendSourceCode currently disables
max-params and no-param-reassign for the whole function; refactor it to accept a
single options object (e.g., { monaco, lineNumber, decorations, text, source,
code, target }) instead of many positional params and stop mutating the input
parameter `text` by using a local variable like `nextText` to build the string;
preserve the existing logic around computing sourceStart/codeStart and pushing
decorations via toRangeDecoration (and keep decorations as a mutable array),
then remove the function-level eslint-disable comments.
In `@projects/site/src/docs/metrics/wireit.11ty.js`:
- Around line 86-130: The colorScale object in wireit.ts contains an unused
'site' mapping ('#82ccd9') that isn't referenced by any Wireit nodes and is
omitted from the legend in wireit.11ty.js; remove the 'site' key from the
colorScale definition in wireit.ts (the colorScale constant) so the mapping only
includes actual node categories and update any related comments/tests if they
reference 'site'.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c7040ad9-dd11-4f4a-8b9a-1ff1a6521f3f
📒 Files selected for processing (64)
projects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/mcp/mcp.test.tsprojects/cli/src/mcp/mcp.tsprojects/cli/src/utils.tsprojects/code/src/codeblock/codeblock.tsprojects/core/build/icons.jsprojects/core/src/badge/badge.cssprojects/core/src/breadcrumb/breadcrumb.cssprojects/core/src/card/card-content.cssprojects/core/src/card/card-header.cssprojects/core/src/color/color.test.tsprojects/core/src/combobox/combobox.examples.tsprojects/core/src/combobox/combobox.test.tsprojects/core/src/copy-button/copy-button.tsprojects/core/src/dropdown-group/dropdown-group.tsprojects/core/src/dropzone/dropzone.test.tsprojects/core/src/dropzone/dropzone.util.tsprojects/core/src/forms/utils/states.tsprojects/core/src/icon/icons.tsprojects/core/src/internal/base/button.test.tsprojects/core/src/internal/controllers/audit.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/controllers/type-native-popover.utils.test.tsprojects/core/src/internal/controllers/type-ssr.controller.tsprojects/core/src/internal/services/i18n.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/polyfills/index.tsprojects/core/src/preferences-input/preferences-input.tsprojects/core/src/skeleton/skeleton.tsprojects/core/src/toolbar/toolbar.cssprojects/core/src/tree/tree-node.cssprojects/core/src/tree/utils.tsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/metadata/eslint.config.jsprojects/internals/metadata/src/tasks/api.utils.test.tsprojects/internals/metadata/src/tasks/api.utils.tsprojects/internals/metadata/src/tasks/tests.utils.tsprojects/internals/testing/src/index.tsprojects/internals/tools/src/internal/validate.tsprojects/internals/tools/src/playground/utils.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/project/setup.tsprojects/lint/src/eslint/internals/attributes.tsprojects/lint/src/eslint/internals/slots.tsprojects/lint/src/eslint/rules/no-deprecated-css-variable.tsprojects/lint/src/eslint/rules/no-unexpected-attribute-value.tsprojects/lint/src/eslint/rules/no-unexpected-css-value.tsprojects/lint/src/eslint/rules/no-unexpected-global-attribute-value.tsprojects/markdown/src/markdown/markdown.tsprojects/monaco/src/diff-editor/diff-editor.test.tsprojects/monaco/src/internal/base/editor.tsprojects/monaco/src/internal/base/input.tsprojects/monaco/src/internal/formats/problems-format.tsprojects/site/src/_11ty/layouts/page.11ty.jsprojects/site/src/_internal/canvas-editable/canvas-editable.tsprojects/site/src/_internal/canvas/canvas.tsprojects/site/src/_internal/metrics-carousel/metrics-carousel.tsprojects/site/src/_internal/search/search.tsprojects/site/src/_internal/stories/theme/theme-generator.tsprojects/site/src/_internal/stories/visualization/visualization-demo.tsprojects/site/src/_internal/theme-preview/theme-preview.tsprojects/site/src/docs/metrics/wireit.11ty.jsprojects/site/src/docs/metrics/wireit.ts
💤 Files with no reviewable changes (5)
- projects/core/src/combobox/combobox.examples.ts
- projects/core/src/copy-button/copy-button.ts
- projects/internals/metadata/src/tasks/api.utils.test.ts
- projects/site/src/_internal/theme-preview/theme-preview.ts
- projects/site/src/_internal/metrics-carousel/metrics-carousel.ts
Comment on lines
+49
to
+57
| const customElement = document.createElement(Color.metadata.tag) as Color; | ||
| const input = document.createElement('input'); | ||
| input.value = '#fff'; | ||
|
|
||
| element.appendChild(input); | ||
| document.body.appendChild(element); | ||
| await element.updateComplete; | ||
| customElement.appendChild(input); | ||
| document.body.appendChild(customElement); | ||
| await customElement.updateComplete; | ||
| expect(input.value).toBe('#fff'); | ||
| element.remove(); | ||
| customElement.remove(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use fixture helpers and guaranteed cleanup for this test path.
At Line 54, attaching directly to document.body bypasses the test fixture pattern, and cleanup at Line 57 is skipped if the test fails early. Prefer createFixture/removeFixture and elementIsStable, with try/finally for deterministic teardown.
Proposed refactor
it('should not apply default if custom default is provided', async () => {
- const customElement = document.createElement(Color.metadata.tag) as Color;
- const input = document.createElement('input');
- input.value = '#fff';
-
- customElement.appendChild(input);
- document.body.appendChild(customElement);
- await customElement.updateComplete;
- expect(input.value).toBe('#fff');
- customElement.remove();
+ const customFixture = await createFixture(html`
+ <nve-color>
+ <input value="#fff" />
+ </nve-color>
+ `);
+ const customElement = customFixture.querySelector(Color.metadata.tag) as Color;
+ const input = customElement.querySelector('input') as HTMLInputElement;
+
+ try {
+ await elementIsStable(customElement);
+ expect(input.value).toBe('#fff');
+ } finally {
+ removeFixture(customFixture);
+ }
});As per coding guidelines, projects/core/src/**/*.test.ts: “use createFixture and elementIsStable patterns”.
📝 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.
Suggested change
| const customElement = document.createElement(Color.metadata.tag) as Color; | |
| const input = document.createElement('input'); | |
| input.value = '#fff'; | |
| element.appendChild(input); | |
| document.body.appendChild(element); | |
| await element.updateComplete; | |
| customElement.appendChild(input); | |
| document.body.appendChild(customElement); | |
| await customElement.updateComplete; | |
| expect(input.value).toBe('#fff'); | |
| element.remove(); | |
| customElement.remove(); | |
| const customFixture = await createFixture(html` | |
| <nve-color> | |
| <input value="#fff" /> | |
| </nve-color> | |
| `); | |
| const customElement = customFixture.querySelector(Color.metadata.tag) as Color; | |
| const input = customElement.querySelector('input') as HTMLInputElement; | |
| try { | |
| await elementIsStable(customElement); | |
| expect(input.value).toBe('#fff'); | |
| } finally { | |
| removeFixture(customFixture); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/src/color/color.test.ts` around lines 49 - 57, The test creates
a Color element by hand and appends it to document.body, which bypasses fixture
helpers and risks leaking state if the test fails; replace direct DOM ops with
the project's fixture utilities: use createFixture to instantiate the element
with Color.metadata.tag, await elementIsStable (or the provided elementIsStable
helper) instead of customElement.updateComplete, and wrap assertions in
try/finally calling removeFixture to guarantee teardown; reference the Color
metadata via Color.metadata.tag and the helpers createFixture, removeFixture,
and elementIsStable to make the change.
Comment on lines
+227
to
252
| export function validateTemplate( | ||
| source: string, | ||
| elements: Element[], | ||
| config: { allowStyles?: boolean; allowGlobalElements?: boolean; allowVulnerableTags?: boolean } = {} | ||
| ): string { | ||
| const mergedConfig: TemplateConfig = { | ||
| allowStyles: true, | ||
| allowGlobalElements: false, | ||
| allowVulnerableTags: true, | ||
| ...config | ||
| }; | ||
|
|
||
| const allowedAttributes = buildAllowedAttributes(elements, mergedConfig); | ||
| const customElementsNonBooleanAttributes = Object.values(allowedAttributes) | ||
| .filter(i => typeof i !== 'string') | ||
| .flatMap(i => (Array.isArray(i?.values) ? i.values : [])); | ||
|
|
||
| const nonBooleanAttributes = sanitizeHtml.defaults.nonBooleanAttributes.filter((attr: string) => attr !== 'hidden'); | ||
|
|
||
| source = removeBodyStyleSelectors(source); | ||
|
|
||
| const result = sanitizeHtml(source, { | ||
| allowedTags, | ||
| return sanitizeHtml(removeBodyStyleSelectors(source), { | ||
| allowedTags: buildAllowedTags(elements, mergedConfig), | ||
| allowedAttributes, | ||
| allowVulnerableTags: mergedConfig.allowVulnerableTags || mergedConfig.allowStyles, // allows script and style tags | ||
| allowVulnerableTags: mergedConfig.allowVulnerableTags || mergedConfig.allowStyles, | ||
| nonBooleanAttributes: [...nonBooleanAttributes, ...customElementsNonBooleanAttributes], | ||
| parser: { | ||
| lowerCaseTags: false, | ||
| lowerCaseAttributeNames: false | ||
| } | ||
| parser: { lowerCaseTags: false, lowerCaseAttributeNames: false } | ||
| }); | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Fix custom element value extraction before building nonBooleanAttributes.
As written, Object.values(allowedAttributes) returns arrays, so this filter(...).flatMap(...) chain never reaches the { name, values } entries created above. That leaves customElementsNonBooleanAttributes empty and can break literal-valued custom attributes.
♻️ Suggested fix
- const customElementsNonBooleanAttributes = Object.values(allowedAttributes)
- .filter(i => typeof i !== 'string')
- .flatMap(i => (Array.isArray(i?.values) ? i.values : []));
+ const customElementsNonBooleanAttributes = Object.values(allowedAttributes).flatMap(attributes =>
+ attributes.flatMap(attribute => (typeof attribute === 'string' ? [] : attribute.values))
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/internals/tools/src/internal/validate.ts` around lines 227 - 252,
The code in validateTemplate computes customElementsNonBooleanAttributes
incorrectly because it assumes Object.values(allowedAttributes) yields objects
with { name, values }, but some entries are arrays (or the object shape
differs), so the filter/flatMap never extracts literal-valued custom attribute
names; update the extraction to handle all possible value shapes produced by
buildAllowedAttributes: iterate Object.values(allowedAttributes) and for each
value, if it's a string skip it, if it's an array concat its items, and if it's
an object with a values property concat that values array (use Array.isArray
checks) so customElementsNonBooleanAttributes contains all literal attribute
names before merging into nonBooleanAttributes.
Comment on lines
+10
to
+17
| function readPackageJson(path: string): Record<string, unknown> | 'missing' | 'invalid' { | ||
| if (!existsSync(path)) return 'missing'; | ||
| try { | ||
| packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf-8')); | ||
| return JSON.parse(readFileSync(path, 'utf-8')); | ||
| } catch { | ||
| return { | ||
| dependencies: { | ||
| message: 'Failed to parse package.json. Skipping project setup.', | ||
| status: 'danger' | ||
| } | ||
| }; | ||
| return 'invalid'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate the parsed package.json shape before using it.
JSON.parse can return null or another non-object JSON value, which will pass the current sentinel checks and then crash when hasExistingElementsDeps reads dependencies. Return 'invalid' for non-object parses here.
Proposed fix
function readPackageJson(path: string): Record<string, unknown> | 'missing' | 'invalid' {
if (!existsSync(path)) return 'missing';
try {
- return JSON.parse(readFileSync(path, 'utf-8'));
+ const parsed = JSON.parse(readFileSync(path, 'utf-8'));
+ return parsed !== null && typeof parsed === 'object' && !Array.isArray(parsed)
+ ? parsed
+ : 'invalid';
} catch {
return 'invalid';
}
}📝 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.
Suggested change
| function readPackageJson(path: string): Record<string, unknown> | 'missing' | 'invalid' { | |
| if (!existsSync(path)) return 'missing'; | |
| try { | |
| packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf-8')); | |
| return JSON.parse(readFileSync(path, 'utf-8')); | |
| } catch { | |
| return { | |
| dependencies: { | |
| message: 'Failed to parse package.json. Skipping project setup.', | |
| status: 'danger' | |
| } | |
| }; | |
| return 'invalid'; | |
| } | |
| } | |
| function readPackageJson(path: string): Record<string, unknown> | 'missing' | 'invalid' { | |
| if (!existsSync(path)) return 'missing'; | |
| try { | |
| const parsed = JSON.parse(readFileSync(path, 'utf-8')); | |
| return parsed !== null && typeof parsed === 'object' && !Array.isArray(parsed) | |
| ? parsed | |
| : 'invalid'; | |
| } catch { | |
| return 'invalid'; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/internals/tools/src/project/setup.ts` around lines 10 - 17,
readPackageJson currently returns the parsed JSON even when JSON.parse yields
null or a non-object value, which later causes crashes in consumers like
hasExistingElementsDeps; change readPackageJson to validate the parsed value
after JSON.parse and return 'invalid' if the result is not a plain object (i.e.,
ensure typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed))
so only proper object shapes are returned, while preserving the existing
existsSync check and try/catch behavior.
Comment on lines
+19
to
+36
| function hasExistingElementsDeps(packageJson: Record<string, unknown>): boolean { | ||
| const dependencies = (packageJson.dependencies ?? {}) as Record<string, string>; | ||
| const devDependencies = (packageJson.devDependencies ?? {}) as Record<string, string>; | ||
| const allDeps = { ...dependencies, ...devDependencies }; | ||
|
|
||
| const hasElementsDeps = CORE_DEPENDENCIES.some(dep => allDeps[dep]); | ||
| if (hasElementsDeps) { | ||
| return { | ||
| dependencies: { | ||
| message: 'Project already has Elements dependencies.', | ||
| status: 'info' | ||
| } | ||
| }; | ||
| return CORE_DEPENDENCIES.some(dep => dependencies[dep] || devDependencies[dep]); | ||
| } | ||
|
|
||
| export function setupProject(cwd: string): Report { | ||
| const packageJsonPath = resolve(join(cwd, 'package.json')); | ||
| const packageJson = readPackageJson(packageJsonPath); | ||
|
|
||
| if (packageJson === 'missing') { | ||
| return { dependencies: { message: 'No package.json found. Skipping project setup.', status: 'warning' } }; | ||
| } | ||
| if (packageJson === 'invalid') { | ||
| return { dependencies: { message: 'Failed to parse package.json. Skipping project setup.', status: 'danger' } }; | ||
| } | ||
| if (hasExistingElementsDeps(packageJson)) { | ||
| return { dependencies: { message: 'Project already has Elements dependencies.', status: 'info' } }; |
There was a problem hiding this comment.
Don't short-circuit on any existing Elements package.
hasExistingElementsDeps treats a single matching package as “project already set up,” so a partially configured package.json will skip setup entirely and never receive the remaining Elements dependencies. Either check that all three packages are already present, or merge in only the missing entries before returning early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/internals/tools/src/project/setup.ts` around lines 19 - 36,
hasExistingElementsDeps currently returns true if any CORE_DEPENDENCIES entry
exists, causing setupProject to short-circuit even when only some Elements
packages are present; update the logic to either (A) require all
CORE_DEPENDENCIES to exist by replacing .some(...) with .every(...) in
hasExistingElementsDeps so the early return only triggers when the project is
fully configured, or (B) stop short-circuiting and instead compute which
CORE_DEPENDENCIES are missing in setupProject and merge only those missing
entries into package.json and install them before returning; use the function
names hasExistingElementsDeps, setupProject and the CORE_DEPENDENCIES symbol to
locate and modify the code.
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.9 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.1.0 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
4 similar comments
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.10 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
1 similar comment
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
This was referenced May 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Style Updates
Documentation
Configuration
Chores