fix(cli): mute interactions on non tty sessions#172
Conversation
github-actions
Bot
added
scope(core)
scope(internals)
scope(lint)
scope(cli)
scope(docs)
labels
📝 WalkthroughWalkthroughThis PR gates CLI banner and spinner output on terminal interactivity, lowers the shared api/examples result limit from 5 to 3 with matching tests, updates packages service test expectations, clarifies component JSDoc, and adds a new ESLint rule with config, tests, and docs. ChangesMixed CLI, tools, docs, and lint updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
projects/cli/src/index.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. projects/cli/src/utils.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. projects/core/src/page/page-panel/page-panel.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.test.ts`:
- Line 11: The `preferAriaLabelInCompactContainers` test setup is using a double
cast to `JSRuleDefinition`, which bypasses type safety and hides incompatible
rule-shape changes. Update the assertion in the test to rely on proper
structural typing or an explicit type guard instead of `as unknown as`, and keep
the check aligned with the actual rule object shape used by
`preferAriaLabelInCompactContainers`.
🪄 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: b9467d34-774e-4690-a8f0-63c1d47b9b0a
📒 Files selected for processing (16)
projects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/core/src/page/page-panel/page-panel.tsprojects/core/src/sort-button/sort-button.tsprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/api/service.tsprojects/internals/tools/src/examples/service.test.tsprojects/internals/tools/src/examples/service.tsprojects/internals/tools/src/packages/service.test.tsprojects/lint/README.mdprojects/lint/src/eslint/configs/html.tsprojects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.test.tsprojects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.tsprojects/site/src/docs/lint/index.md
| import { elementsHtmlConfig } from '../configs/html.js'; | ||
| import preferAriaLabelInCompactContainers from './prefer-aria-label-in-compact-containers.js'; | ||
|
|
||
| const rule = preferAriaLabelInCompactContainers as unknown as JSRuleDefinition; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Double unknown cast bypasses type checking for the rule shape.
preferAriaLabelInCompactContainers as unknown as JSRuleDefinition sidesteps structural type checking between the custom rule object and ESLint's expected rule type, so a future incompatible change to the rule shape wouldn't be caught here.
As per coding guidelines, **/*.ts files should follow /projects/site/src/docs/internal/guidelines/typescript.md for type safety and type guards.
🤖 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
`@projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.test.ts`
at line 11, The `preferAriaLabelInCompactContainers` test setup is using a
double cast to `JSRuleDefinition`, which bypasses type safety and hides
incompatible rule-shape changes. Update the assertion in the test to rely on
proper structural typing or an explicit type guard instead of `as unknown as`,
and keep the check aligned with the actual rule object shape used by
`preferAriaLabelInCompactContainers`.
Source: Coding guidelines
coryrylan
force-pushed
the
topic-lint-rule-fixes
branch
from
96ff3a5 to
d002590
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@projects/internals/tools/src/packages/service.test.ts`:
- Line 99: The `PackagesService.changelogsGet as ToolMethod<unknown>` cast is
bypassing TypeScript’s type safety in the test. Update the test to use the
actual `PackagesService.changelogsGet` type directly, or adjust the `ToolMethod`
typing so `inputSchema` can be accessed without an unsafe cast. Keep the change
localized around `changelogsGet` and `metadata.inputSchema`, and prefer
inference or proper narrowing over asserting to `ToolMethod<unknown>`.
In `@projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.ts`:
- Around line 27-70: The ESLint rule object is only inferred from the literal,
so it is not being checked against the expected rule type. Update the `rule`
definition in `prefer-aria-label-in-compact-containers` to use an explicit
`Rule.RuleModule` or `JSRuleDefinition` annotation while keeping the existing
`create` and `meta` structure, so future changes are type-validated instead of
relying on `as const` alone.
🪄 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: e0b11890-a4d6-4dfe-8108-aabdfa24254a
📒 Files selected for processing (16)
projects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/core/src/page/page-panel/page-panel.tsprojects/core/src/sort-button/sort-button.tsprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/api/service.tsprojects/internals/tools/src/examples/service.test.tsprojects/internals/tools/src/examples/service.tsprojects/internals/tools/src/packages/service.test.tsprojects/lint/README.mdprojects/lint/src/eslint/configs/html.tsprojects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.test.tsprojects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.tsprojects/site/src/docs/lint/index.md
| const listSet = new Set(Array.from(list.matchAll(/^## (\S+) v/gm), m => m[1])); | ||
|
|
||
| it('should expose the same package set in the changelogs schema and unknown-package error', async () => { | ||
| const schema = (PackagesService.changelogsGet as ToolMethod<unknown>).metadata.inputSchema; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Type cast bypasses type safety.
PackagesService.changelogsGet as ToolMethod<unknown> casts around the actual type instead of narrowing it. Consider whether ToolMethod can be inferred/typed directly on changelogsGet to avoid the cast, per TypeScript guidance on type safety.
As per coding guidelines, "When working with TypeScript code, follow /projects/site/src/docs/internal/guidelines/typescript.md for type safety, type guards, discriminated unions, and exhaustive checking."
🤖 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 `@projects/internals/tools/src/packages/service.test.ts` at line 99, The
`PackagesService.changelogsGet as ToolMethod<unknown>` cast is bypassing
TypeScript’s type safety in the test. Update the test to use the actual
`PackagesService.changelogsGet` type directly, or adjust the `ToolMethod` typing
so `inputSchema` can be accessed without an unsafe cast. Keep the change
localized around `changelogsGet` and `metadata.inputSchema`, and prefer
inference or proper narrowing over asserting to `ToolMethod<unknown>`.
Source: Coding guidelines
| const rule = { | ||
| meta: { | ||
| type: 'problem' as const, | ||
| docs: { | ||
| description: 'Prefer aria-label on form controls inside toolbars and page headers.', | ||
| category: 'Best Practice', | ||
| recommended: true, | ||
| url: `${__ELEMENTS_PAGES_BASE_URL__}/docs/lint/` | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| ['prefer-aria-label']: | ||
| 'Remove <label> from <{{control}}> inside <{{container}}> and use aria-label instead to preserve the compact layout.' | ||
| } | ||
| }, | ||
| create(context: Rule.RuleContext) { | ||
| return createVisitors(context, { | ||
| Tag(node: HtmlTagNode) { | ||
| if (node.name.toLowerCase() !== 'label') { | ||
| return; | ||
| } | ||
|
|
||
| const control = findAncestor(node.parent, FORM_CONTROLS); | ||
| if (!control) { | ||
| return; | ||
| } | ||
|
|
||
| const container = findAncestor(control.parent, COMPACT_CONTAINERS); | ||
| if (!container) { | ||
| return; | ||
| } | ||
|
|
||
| context.report({ | ||
| node, | ||
| messageId: 'prefer-aria-label', | ||
| data: { | ||
| control: control.name.toLowerCase(), | ||
| container: container.name.toLowerCase() | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| } as const; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rule object lacks an explicit Rule.RuleModule (or JSRuleDefinition) type annotation.
rule is inferred purely from the object literal (with as const), so there's no structural check against ESLint's rule type. This is likely why the test file needed as unknown as JSRuleDefinition — the object shape isn't verified to satisfy ESLint's expected type, so an incompatible future change (e.g. renaming create, mistyped meta) wouldn't be caught at compile time here.
As per coding guidelines, **/*.ts files should follow /projects/site/src/docs/internal/guidelines/typescript.md for type safety and type guards.
♻️ Proposed fix
-const rule = {
+const rule: Rule.RuleModule = {
meta: {📝 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.
| const rule = { | |
| meta: { | |
| type: 'problem' as const, | |
| docs: { | |
| description: 'Prefer aria-label on form controls inside toolbars and page headers.', | |
| category: 'Best Practice', | |
| recommended: true, | |
| url: `${__ELEMENTS_PAGES_BASE_URL__}/docs/lint/` | |
| }, | |
| schema: [], | |
| messages: { | |
| ['prefer-aria-label']: | |
| 'Remove <label> from <{{control}}> inside <{{container}}> and use aria-label instead to preserve the compact layout.' | |
| } | |
| }, | |
| create(context: Rule.RuleContext) { | |
| return createVisitors(context, { | |
| Tag(node: HtmlTagNode) { | |
| if (node.name.toLowerCase() !== 'label') { | |
| return; | |
| } | |
| const control = findAncestor(node.parent, FORM_CONTROLS); | |
| if (!control) { | |
| return; | |
| } | |
| const container = findAncestor(control.parent, COMPACT_CONTAINERS); | |
| if (!container) { | |
| return; | |
| } | |
| context.report({ | |
| node, | |
| messageId: 'prefer-aria-label', | |
| data: { | |
| control: control.name.toLowerCase(), | |
| container: container.name.toLowerCase() | |
| } | |
| }); | |
| } | |
| }); | |
| } | |
| } as const; | |
| const rule: Rule.RuleModule = { | |
| meta: { | |
| type: 'problem' as const, | |
| docs: { | |
| description: 'Prefer aria-label on form controls inside toolbars and page headers.', | |
| category: 'Best Practice', | |
| recommended: true, | |
| url: `${__ELEMENTS_PAGES_BASE_URL__}/docs/lint/` | |
| }, | |
| schema: [], | |
| messages: { | |
| ['prefer-aria-label']: | |
| 'Remove <label> from <{{control}}> inside <{{container}}> and use aria-label instead to preserve the compact layout.' | |
| } | |
| }, | |
| create(context: Rule.RuleContext) { | |
| return createVisitors(context, { | |
| Tag(node: HtmlTagNode) { | |
| if (node.name.toLowerCase() !== 'label') { | |
| return; | |
| } | |
| const control = findAncestor(node.parent, FORM_CONTROLS); | |
| if (!control) { | |
| return; | |
| } | |
| const container = findAncestor(control.parent, COMPACT_CONTAINERS); | |
| if (!container) { | |
| return; | |
| } | |
| context.report({ | |
| node, | |
| messageId: 'prefer-aria-label', | |
| data: { | |
| control: control.name.toLowerCase(), | |
| container: container.name.toLowerCase() | |
| } | |
| }); | |
| } | |
| }); | |
| } | |
| } as const; |
🤖 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 `@projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.ts`
around lines 27 - 70, The ESLint rule object is only inferred from the literal,
so it is not being checked against the expected rule type. Update the `rule`
definition in `prefer-aria-label-in-compact-containers` to use an explicit
`Rule.RuleModule` or `JSRuleDefinition` annotation while keeping the existing
`create` and `meta` structure, so future changes are type-validated instead of
relying on `as const` alone.
Source: Coding guidelines
coryrylan
changed the title
- Introduced a new ESLint rule to enforce the use of aria-label on form controls within compact containers (e.g., toolbars and page headers) Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-lint-rule-fixes
branch
from
d002590 to
08ed588
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.ts (1)
27-70: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing explicit
Rule.RuleModuletype annotation onrule.Same concern raised previously: the object literal is only inferred via
as const, with no structural check against ESLint's rule type, which is why the test file needsas unknown as JSRuleDefinition.♻️ Proposed fix
-const rule = { +const rule: Rule.RuleModule = { meta: {🤖 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 `@projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.ts` around lines 27 - 70, The `rule` object in `prefer-aria-label-in-compact-containers` is only inferred with `as const`, so it is not being checked against ESLint’s rule contract. Add an explicit `Rule.RuleModule` annotation (or equivalent typed rule definition) to the `rule` constant so `meta`, `messages`, and `create(context)` are validated structurally. Keep the existing `createVisitors`-based implementation and `Tag` visitor, but make sure the exported rule is typed directly instead of relying on downstream casts like `JSRuleDefinition`.projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.test.ts (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDouble
unknowncast bypasses type checking for the rule shape.Same concern as flagged previously — casting through
unknownhides incompatible shape changes between the custom rule object and ESLint'sJSRuleDefinition. Resolving this depends on adding theRule.RuleModuleannotation in the rule file (see comment there).🤖 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 `@projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.test.ts` at line 11, The test is bypassing type safety by casting preferAriaLabelInCompactContainers through unknown to JSRuleDefinition, which can hide shape mismatches. Update the test to use the rule’s proper typed export once the rule file is annotated with Rule.RuleModule, and remove the double cast so the compiler validates the rule shape directly.
🤖 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 `@projects/internals/tools/src/packages/service.test.ts`:
- Around line 99-100: The `PackagesService.changelogsGet` metadata access in
`service.test.ts` is not fully guarded for optional values. Update the
`ToolMethod.metadata` lookup so `inputSchema` is accessed only through optional
chaining, and also guard the `name` property before reading `enum` when building
`schemaSet`. Use the existing `PackagesService.changelogsGet` and
`schema`/`schemaSet` test setup to keep the change localized and compatible with
strict null checks.
---
Duplicate comments:
In
`@projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.test.ts`:
- Line 11: The test is bypassing type safety by casting
preferAriaLabelInCompactContainers through unknown to JSRuleDefinition, which
can hide shape mismatches. Update the test to use the rule’s proper typed export
once the rule file is annotated with Rule.RuleModule, and remove the double cast
so the compiler validates the rule shape directly.
In `@projects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.ts`:
- Around line 27-70: The `rule` object in
`prefer-aria-label-in-compact-containers` is only inferred with `as const`, so
it is not being checked against ESLint’s rule contract. Add an explicit
`Rule.RuleModule` annotation (or equivalent typed rule definition) to the `rule`
constant so `meta`, `messages`, and `create(context)` are validated
structurally. Keep the existing `createVisitors`-based implementation and `Tag`
visitor, but make sure the exported rule is typed directly instead of relying on
downstream casts like `JSRuleDefinition`.
🪄 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: ceb1d10a-9196-44cc-8315-371d950b4bf5
📒 Files selected for processing (16)
projects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/core/src/page/page-panel/page-panel.tsprojects/core/src/sort-button/sort-button.tsprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/api/service.tsprojects/internals/tools/src/examples/service.test.tsprojects/internals/tools/src/examples/service.tsprojects/internals/tools/src/packages/service.test.tsprojects/lint/README.mdprojects/lint/src/eslint/configs/html.tsprojects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.test.tsprojects/lint/src/eslint/rules/prefer-aria-label-in-compact-containers.tsprojects/site/src/docs/lint/index.md
| const schema = (PackagesService.changelogsGet as ToolMethod<unknown>).metadata.inputSchema; | ||
| const schemaSet = new Set(schema?.properties?.name.enum ?? []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd tsconfig.json projects/internals/tools -x cat {}
fd tsconfig.json -x cat {} \; 2>/dev/null | rg -n 'strict'Repository: NVIDIA/elements
Length of output: 2036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== service.test.ts excerpt ==\n'
cat -n projects/internals/tools/src/packages/service.test.ts | sed -n '80,115p'
printf '\n== ToolMethod definition ==\n'
rg -n "type ToolMethod|interface ToolMethod|export .*ToolMethod" projects/internals/tools/src -g '*.ts' -A 8 -B 4
printf '\n== metadata / inputSchema / properties usage ==\n'
rg -n "\.metadata(\?|)\.inputSchema|properties\?\.name\.enum|properties\?\.name\?\.enum|name\.enum" projects/internals/tools/src -g '*.ts' -A 2 -B 2Repository: NVIDIA/elements
Length of output: 16152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ToolMetadata / Schema definitions ==\n'
cat -n projects/internals/tools/src/internal/tools.ts | sed -n '1,120p'
printf '\n== packages service schema setup ==\n'
rg -n "changelogsGet|inputSchema|properties|enum" projects/internals/tools/src/packages -g '*.ts' -A 4 -B 4Repository: NVIDIA/elements
Length of output: 13579
Guard the optional metadata chain here projects/internals/tools/src/packages/service.test.ts:99-100
ToolMethod.metadata is optional, so .metadata.inputSchema can trip strict null checks, and schema?.properties?.name.enum still leaves name unguarded. Use optional chaining on both access paths.
🛡️ Proposed fix for consistent optional chaining
- const schema = (PackagesService.changelogsGet as ToolMethod<unknown>).metadata.inputSchema;
- const schemaSet = new Set(schema?.properties?.name.enum ?? []);
+ const schema = (PackagesService.changelogsGet as ToolMethod<unknown>).metadata?.inputSchema;
+ const schemaSet = new Set(schema?.properties?.name?.enum ?? []);📝 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.
| const schema = (PackagesService.changelogsGet as ToolMethod<unknown>).metadata.inputSchema; | |
| const schemaSet = new Set(schema?.properties?.name.enum ?? []); | |
| const schema = (PackagesService.changelogsGet as ToolMethod<unknown>).metadata?.inputSchema; | |
| const schemaSet = new Set(schema?.properties?.name?.enum ?? []); |
🤖 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 `@projects/internals/tools/src/packages/service.test.ts` around lines 99 - 100,
The `PackagesService.changelogsGet` metadata access in `service.test.ts` is not
fully guarded for optional values. Update the `ToolMethod.metadata` lookup so
`inputSchema` is accessed only through optional chaining, and also guard the
`name` property before reading `enum` when building `schemaSet`. Use the
existing `PackagesService.changelogsGet` and `schema`/`schemaSet` test setup to
keep the change localized and compatible with strict null checks.
1 participant
Summary by CodeRabbit
New Features
<label>usage for form controls inside compact page headers/toolbars and recommends usingaria-label.Bug Fixes
Documentation