Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors internal logic by extracting helpers across multiple modules (formatting, merging, lint rules, monaco editor, utilities), tightens ESLint structural rules, updates tests and a lighthouse threshold, and makes a Shadow DOM DOM-query fix; no public API signature changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
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 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
🤖 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/format-relative-time/format-relative-time.ts`:
- Around line 74-81: The thresholds array is being recreated on every call to
the `#computeUnit` method; extract it to a single shared immutable value to avoid
per-call allocation. Create a module-level constant (or a static readonly field
on the class that contains `#computeUnit`) named e.g. THRESHOLDS with the same
structure and replace the inline thresholds declaration in `#computeUnit` with a
reference to THRESHOLDS; ensure the type annotation matches Array<{ unit:
Intl.RelativeTimeFormatUnit; divisor: number; max: number }> and that existing
logic still reads from the new constant.
In `@projects/core/src/internal/utils/objects.ts`:
- Line 38: Replace the Object.assign call that creates a temporary object with a
direct property assignment to avoid allocations: change the use of
Object.assign(target, { [key]: sourceValue }) to assign the value directly to
target using the existing identifiers (target, key, sourceValue) so the line
uses target[key] = sourceValue; in the function in objects.ts.
In `@projects/forms/src/internal/schema.ts`:
- Around line 176-178: In the switch branch that currently does `case 'array':
case 'object': return JSON.parse(value) as T;` wrap the JSON.parse in a
try/catch and return undefined on parse failure so the function honors the `T |
undefined` contract; update the branch in the function containing that switch
(the branch handling 'array'/'object') to catch and suppress parse errors
(optionally log or attach context) rather than letting the raw exception
propagate.
- Line 175: The boolean parsing currently returns (!!value || value === 'true')
which treats any non-empty string (including "false") as true; change the logic
to perform strict comparisons against true-like values instead (e.g. replace
that expression with (value === true || value === 'true') as T) so that "false"
correctly yields false; locate the return statement using the value variable in
this file and update it accordingly.
- Line 180: The enum check fails for numeric enums because it compares raw input
string to enum members; update the logic around the line using
schema.enum.includes(value) (in the enum parsing branch) to compare normalized
strings instead: find the enum member where String(member) === String(value) (or
map schema.enum to String values) and return that matched enum member cast as T
instead of returning the raw input string; this ensures numeric enum members
like 1 match form input "1" and the returned value preserves the original enum
member type.
In `@projects/internals/eslint/src/configs/typescript.js`:
- Line 140: The 'id-length' rule is currently disabled but still includes an
unused options object; simplify the rule entry by removing the array/options and
set the rule to just 'off' (replace the "'id-length': ['off', { min: 2,
exceptions: ['_'] }]" entry with a simple "'id-length': 'off'") so the disabled
rule configuration is clear and concise.
In `@projects/internals/tools/src/examples/utils.ts`:
- Around line 158-173: The code currently recompiles RegExp objects on every
line inside countTagDelta/findBlockEnd for the given tag; modify findBlockEnd so
it constructs and caches the three regexes (open, self-close, close) once per
call using an escaped tag string, then update countTagDelta to accept these
precomputed RegExp objects (or inline its logic into findBlockEnd) and use them
to compute opens/selfCloses/closes, ensuring isSelfContainedBlock(tag) is still
respected; this eliminates per-line RegExp allocations and makes tag escaping
explicit.
In `@projects/site/src/_internal/search/search.ts`:
- Around line 222-229: Eviction currently skips deleting the oldest cache entry
when the oldest key is an empty string because the condition uses truthiness;
update the guard in `#evictOldestCacheEntryIfFull` to treat empty-string keys as
valid by checking for undefined explicitly (e.g., test firstCacheKey !==
undefined or verify iterator has a value) before calling
this.#searchCache.delete(firstCacheKey), ensuring the cache never grows past
DocsSearch.CACHE_MAX_SIZE when search() stores '' as a key.
🪄 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: 26fc3269-72a0-4093-9bb0-f51a55f60f9a
📒 Files selected for processing (14)
projects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/validation.examples.tsprojects/core/src/internal/utils/objects.tsprojects/forms/src/internal/schema.tsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/metadata/src/tasks/wireit.utils.tsprojects/internals/tools/src/examples/utils.tsprojects/lint/src/eslint/internals/utils.tsprojects/lint/src/eslint/rules/no-missing-control-label.tsprojects/monaco/src/internal/base/input.tsprojects/monaco/src/internal/formats/problems-format.tsprojects/monaco/src/problems/problems.tsprojects/site/src/_internal/search/search.tsprojects/site/src/_internal/theme-preview/theme-preview.ts
💤 Files with no reviewable changes (1)
- projects/core/src/forms/validation.examples.ts
Comment on lines
+176
to
+178
| case 'array': | ||
| case 'object': | ||
| return JSON.parse(value) as T; |
There was a problem hiding this comment.
Handle invalid JSON input instead of throwing raw parse errors.
JSON.parse(value) can throw and bypass the T | undefined contract, causing unhandled exceptions in callers expecting parse failure signaling.
Proposed fix
case 'array':
case 'object':
- return JSON.parse(value) as T;
+ try {
+ return JSON.parse(value) as T;
+ } catch {
+ return undefined;
+ }📝 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
| case 'array': | |
| case 'object': | |
| return JSON.parse(value) as T; | |
| case 'array': | |
| case 'object': | |
| try { | |
| return JSON.parse(value) as T; | |
| } catch { | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/forms/src/internal/schema.ts` around lines 176 - 178, In the switch
branch that currently does `case 'array': case 'object': return
JSON.parse(value) as T;` wrap the JSON.parse in a try/catch and return undefined
on parse failure so the function honors the `T | undefined` contract; update the
branch in the function containing that switch (the branch handling
'array'/'object') to catch and suppress parse errors (optionally log or attach
context) rather than letting the raw exception propagate.
| case 'object': | ||
| return JSON.parse(value) as T; | ||
| case 'enum': | ||
| return (schema.enum.includes(value) ? value : undefined) as T; |
There was a problem hiding this comment.
Enum parsing fails for numeric enum values from string form inputs.
Form input is string, so includes(value) won’t match numeric enum members (e.g., "1" vs 1).
Proposed fix
case 'enum':
- return (schema.enum.includes(value) ? value : undefined) as T;
+ if (schema.enum.includes(value)) return value as T;
+ const numericValue = Number(value);
+ return (Number.isNaN(numericValue) ? undefined : schema.enum.includes(numericValue) ? numericValue : undefined) as T;📝 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
| return (schema.enum.includes(value) ? value : undefined) as T; | |
| if (schema.enum.includes(value)) return value as T; | |
| const numericValue = Number(value); | |
| return (Number.isNaN(numericValue) ? undefined : schema.enum.includes(numericValue) ? numericValue : undefined) as T; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/forms/src/internal/schema.ts` at line 180, The enum check fails for
numeric enums because it compares raw input string to enum members; update the
logic around the line using schema.enum.includes(value) (in the enum parsing
branch) to compare normalized strings instead: find the enum member where
String(member) === String(value) (or map schema.enum to String values) and
return that matched enum member cast as T instead of returning the raw input
string; this ensures numeric enum members like 1 match form input "1" and the
returned value preserves the original enum member type.
coryrylan
force-pushed
the
topic-lint-fixup-5
branch
from
9d7c8a8 to
0d392b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
projects/site/src/_internal/search/search.ts (1)
222-229:⚠️ Potential issue | 🟠 MajorFix cache eviction guard to handle empty-string keys.
On Line 227,
if (firstCacheKey)skips deletion when the oldest key is'', so cache eviction can fail and exceedDocsSearch.CACHE_MAX_SIZE.Suggested fix
`#evictOldestCacheEntryIfFull`() { if (this.#searchCache.size < DocsSearch.CACHE_MAX_SIZE) { return; } - const firstCacheKey = this.#searchCache.keys().next().value; - if (firstCacheKey) { - this.#searchCache.delete(firstCacheKey); - } + const firstEntry = this.#searchCache.keys().next(); + if (!firstEntry.done) { + this.#searchCache.delete(firstEntry.value); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/site/src/_internal/search/search.ts` around lines 222 - 229, The cache-eviction guard in `#evictOldestCacheEntryIfFull` wrongly treats an empty-string key as missing because it uses a truthy check on firstCacheKey; change the condition to explicitly test for undefined (e.g., check that firstCacheKey !== undefined or use the iterator's done flag) before calling this.#searchCache.delete(firstCacheKey) so empty-string keys are evicted correctly when this.#searchCache.size >= DocsSearch.CACHE_MAX_SIZE.
🤖 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/tooltip/tooltip.test.lighthouse.ts`:
- Line 20: The test loosened the JS payload threshold at the assertion
expect(report.payload.javascript.kb) — either revert to the previous numeric
limit or, if the increase is intentional, add an explicit regression rationale
immediately above the assertion describing the measured baseline value, test
environment, date, and a link to the PR/issue that justifies the change;
alternatively extract the threshold into a named constant (e.g.,
JS_PAYLOAD_KB_LIMIT) and add the same rationale comment above that constant so
future reviewers can see why
expect(report.payload.javascript.kb).toBeLessThan(...) was raised.
In `@projects/monaco/src/internal/base/input.ts`:
- Around line 446-469: In `#handleValidationVersion`, clear any pending validation
state before applying the required-field error: when the `#isRequiredAndEmpty`()
branch is taken, call this.#clearValidation() first, then call
this.#setRequiredValidationError() (and still dispatch the
'syntax-validation-changed' event as done in the other branches) so the
validating flag does not remain set after the value is emptied.
In `@projects/monaco/src/problems/problems.ts`:
- Around line 192-224: The teardown currently clears Monaco-related fields but
leaves the host state flag set to 'ready'; update the disposal path so that when
the editor is disposed (the onDidDispose handler) you also remove or clear the
ready flag from this._internals.states (e.g. delete or filter out the 'ready'
entry), by adding that mutation either inside the onDidDispose callback or
inside `#resetEditorState`(); refer to the onDidDispose listener, the
`#resetEditorState` method, and this._internals.states/'ready' symbol to locate
where to apply the change.
---
Duplicate comments:
In `@projects/site/src/_internal/search/search.ts`:
- Around line 222-229: The cache-eviction guard in `#evictOldestCacheEntryIfFull`
wrongly treats an empty-string key as missing because it uses a truthy check on
firstCacheKey; change the condition to explicitly test for undefined (e.g.,
check that firstCacheKey !== undefined or use the iterator's done flag) before
calling this.#searchCache.delete(firstCacheKey) so empty-string keys are evicted
correctly when this.#searchCache.size >= DocsSearch.CACHE_MAX_SIZE.
🪄 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: 1b440978-168d-40a2-b6f9-f0f44cf0e925
📒 Files selected for processing (15)
projects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/validation.examples.tsprojects/core/src/internal/utils/objects.tsprojects/core/src/tooltip/tooltip.test.lighthouse.tsprojects/forms/src/internal/schema.tsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/metadata/src/tasks/wireit.utils.tsprojects/internals/tools/src/examples/utils.tsprojects/lint/src/eslint/internals/utils.tsprojects/lint/src/eslint/rules/no-missing-control-label.tsprojects/monaco/src/internal/base/input.tsprojects/monaco/src/internal/formats/problems-format.tsprojects/monaco/src/problems/problems.tsprojects/site/src/_internal/search/search.tsprojects/site/src/_internal/theme-preview/theme-preview.ts
💤 Files with no reviewable changes (1)
- projects/core/src/forms/validation.examples.ts
coryrylan
force-pushed
the
topic-lint-fixup-5
branch
from
0d392b5 to
0086e2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
patches/monaco-editor.patch (2)
241-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Delayer.cancel()leaves in-flight promises unsettled.Removing
doReject(...)without an alternate settle path means previously returned promises can remain pending forever after cancellation.Proposed fix
- // this.doReject?.(new CancellationError()); + // Treat cancellation as non-error, but still settle the pending promise. + this.task = null; + this.doResolve?.(null); this.completionPromise = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 241 - 254, Delayer.cancel currently comments out this.doReject(...), leaving previously returned promises unresolved; update Delayer.cancel to explicitly settle any pending promise instead of throwing: if there's a pending resolver, call this.doResolve?.(undefined) (or resolve with a suitable sentinel) then null out this.doResolve and this.doReject and set this.completionPromise = null; reference the Delayer.cancel method and the doResolve/doReject and completionPromise fields when making the change so in-flight promises are always settled on cancel without causing an unhandled rejection.
804-806:⚠️ Potential issue | 🔴 CriticalIncorrect event method name causes runtime error.
The wrapper function
onDidValidateModelVersioncallsmodel.onDidValidateModelVersion(listener), but the model patch only adds theonDidValidateVersion(listener)method. This will throw "undefined is not a function" at runtime when listeners are registered.Fix
export function onDidValidateModelVersion(model, listener) { - return model.onDidValidateModelVersion(listener); + return model.onDidValidateVersion(listener); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 804 - 806, The exported wrapper onDidValidateModelVersion currently calls model.onDidValidateModelVersion(listener) which doesn't exist and causes a runtime error; update the wrapper to call the actual patched method model.onDidValidateVersion(listener) (or add a forwarding alias on the model) so listeners are registered correctly; locate the onDidValidateModelVersion function and replace the internal call to use model.onDidValidateVersion(listener).
♻️ Duplicate comments (5)
projects/core/src/tooltip/tooltip.test.lighthouse.ts (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument why the JS payload budget was increased.
Line 20 raises the threshold from the prior baseline, but there’s still no explicit rationale in-test (baseline measurement/date/link). Please either revert the limit or add a short regression note and a named constant so this doesn’t silently normalize a regression.
As per coding guidelines, "Review /projects/site/src/docs/internal/guidelines/testing-lighthouse.md before writing performance tests; verify performance, accessibility, and best practices scoring."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/src/tooltip/tooltip.test.lighthouse.ts` at line 20, The JS payload threshold in tooltip.test.lighthouse.ts (the assertion on report.payload.javascript.kb) was raised without documentation; either revert the numeric change or replace the magic number with a named constant (e.g., JS_PAYLOAD_KB_BUDGET) and add a short regression note/comment in the test that states the reason, baseline value/date/measurement link, and reviewer who approved the bump; update the test to use the named constant in expect(report.payload.javascript.kb).toBeLessThan(JS_PAYLOAD_KB_BUDGET) so future readers can see why the budget was increased.projects/forms/src/internal/schema.ts (3)
176-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle invalid JSON input instead of throwing raw parse errors.
JSON.parse(value)can throw and bypass theT | undefinedcontract, causing unhandled exceptions in callers expecting parse failure signaling.🛡️ Proposed fix
case 'array': case 'object': - return JSON.parse(value) as T; + try { + return JSON.parse(value) as T; + } catch { + return undefined; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/forms/src/internal/schema.ts` around lines 176 - 178, The code currently returns JSON.parse(value) for the 'array' and 'object' branches which can throw and violate the expected T | undefined contract; update the branches handling 'array' and 'object' to wrap JSON.parse(value) in a try/catch, returning the parsed value on success and undefined on any JSON.parse error (optionally log/contextualize the error) so callers receive undefined instead of an uncaught exception.
179-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnum parsing fails for numeric enum values from string form inputs.
Form input is string, so
includes(value)won't match numeric enum members (e.g.,"1"vs1).🛡️ Proposed fix
case 'enum': - return (schema.enum.includes(value) ? value : undefined) as T; + if (schema.enum.includes(value)) return value as T; + const numericValue = Number(value); + return (Number.isNaN(numericValue) ? undefined : schema.enum.includes(numericValue) ? numericValue : undefined) as T;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/forms/src/internal/schema.ts` around lines 179 - 180, The enum case currently uses schema.enum.includes(value) which fails when form inputs are strings but enum members are numbers; update the 'enum' branch in the parser to find a matching enum member by comparing String(member) === String(value) (or coerce value to number if all enum members are numeric), then return the matched enum member (not the raw input) cast to T; locate the case 'enum' block that references schema.enum.includes(value) and replace it with a lookup that returns the actual enum element when a stringified match is found.
174-175:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBoolean parsing is incorrect for
"false"input.
!!valuemakes any non-empty string truthy, so"false"is parsed astrue. This is a critical correctness issue.🐛 Proposed fix
case 'boolean': - return (!!value || value === 'true') as T; + if (value === 'true') return true as T; + if (value === 'false') return false as T; + return undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/forms/src/internal/schema.ts` around lines 174 - 175, The boolean branch (case 'boolean') incorrectly uses "!!value" which treats any non-empty string like "false" as true; update the branch to explicitly handle boolean and string inputs: if typeof value === 'boolean' return value, else if value === 'true' return true, else if value === 'false' return false, otherwise fall back to Boolean(value) or false as the desired default. Ensure you replace the current return (!!value || value === 'true') as T with these explicit checks referencing the same case 'boolean', value variable, and T generic.projects/internals/tools/src/examples/utils.ts (1)
158-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrecompute and escape tag regexes once per
findBlockEndcall.
countTagDeltacurrently recompiles three regexes for every scanned line and interpolatestagdirectly. Build escaped regexes once infindBlockEndand pass them in.♻️ Proposed fix
+function escapeRegex(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +function buildTagRegex(tag: string): { open: RegExp; selfClose: RegExp; close: RegExp } { + const safeTag = escapeRegex(tag); + return { + open: new RegExp(`<${safeTag}[\\s>]`, 'g'), + selfClose: new RegExp(`<${safeTag}[^>]*/>`, 'g'), + close: new RegExp(`</${safeTag}>`, 'g') + }; +} + -function countTagDelta(line: string, tag: string): number { - const opens = (line.match(new RegExp(`<${tag}[\\s>]`, 'g')) || []).length; - const selfCloses = (line.match(new RegExp(`<${tag}[^>]*/>`, 'g')) || []).length; - const closes = (line.match(new RegExp(`</${tag}>`, 'g')) || []).length; +function countTagDelta(line: string, regex: { open: RegExp; selfClose: RegExp; close: RegExp }): number { + const opens = (line.match(regex.open) || []).length; + const selfCloses = (line.match(regex.selfClose) || []).length; + const closes = (line.match(regex.close) || []).length; return opens - selfCloses - closes; } function findBlockEnd(lines: string[], start: number, tag: string): number { if (isSelfContainedBlock(lines[start]!, tag)) { return start; } + const regex = buildTagRegex(tag); let depth = 1; for (let i = start + 1; i < lines.length; i++) { - depth += countTagDelta(lines[i]!, tag); + depth += countTagDelta(lines[i]!, regex); if (depth <= 0) { return i; } }#!/bin/bash # Verify where tag values reach findBlockEnd/countTagDelta and confirm call-site shape. rg -nP --type=ts -C3 '\bfunction\s+findBlockEnd\b|\bfindBlockEnd\s*\(' rg -nP --type=ts -C3 '\bfunction\s+countTagDelta\b|\bcountTagDelta\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/internals/tools/src/examples/utils.ts` around lines 158 - 173, countTagDelta is recompiling three tag regexes per line and interpolates raw tag text; modify findBlockEnd to escape the tag once (e.g., via a safe escape function), precompile the three RegExp objects (open, selfClose, close) there, and either pass those RegExp instances into countTagDelta (change its signature) or inline the counting logic in findBlockEnd; keep the isSelfContainedBlock check intact and ensure behavior of countTagDelta(openRegex, selfCloseRegex, closeRegex, line) matches the original opens - selfCloses - closes result.
🤖 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/button-group/button-group.ts`:
- Line 24: Update the JSDoc on the ButtonGroup declaration to tighten wording:
replace the existing description with something like "A button group organizes
related buttons and supports either single-select (mutually exclusive) or
multi-select behavior; prefer use in toolbars." Ensure this new prose replaces
the current line in the JSDoc for the ButtonGroup symbol and follows Google
style and write-good rules for Vale linting.
In `@projects/forms/src/internal/schema.ts`:
- Around line 172-173: The number branch in the parser (case 'number' in
projects/forms/src/internal/schema.ts) currently returns parseFloat(value) which
yields NaN for invalid input; change it to parseFloat the value into a temp
variable, check Number.isNaN(result) and return undefined on failure, otherwise
return the parsed number cast to T so the function adheres to the T | undefined
contract.
---
Outside diff comments:
In `@patches/monaco-editor.patch`:
- Around line 241-254: Delayer.cancel currently comments out this.doReject(...),
leaving previously returned promises unresolved; update Delayer.cancel to
explicitly settle any pending promise instead of throwing: if there's a pending
resolver, call this.doResolve?.(undefined) (or resolve with a suitable sentinel)
then null out this.doResolve and this.doReject and set this.completionPromise =
null; reference the Delayer.cancel method and the doResolve/doReject and
completionPromise fields when making the change so in-flight promises are always
settled on cancel without causing an unhandled rejection.
- Around line 804-806: The exported wrapper onDidValidateModelVersion currently
calls model.onDidValidateModelVersion(listener) which doesn't exist and causes a
runtime error; update the wrapper to call the actual patched method
model.onDidValidateVersion(listener) (or add a forwarding alias on the model) so
listeners are registered correctly; locate the onDidValidateModelVersion
function and replace the internal call to use
model.onDidValidateVersion(listener).
---
Duplicate comments:
In `@projects/core/src/tooltip/tooltip.test.lighthouse.ts`:
- Line 20: The JS payload threshold in tooltip.test.lighthouse.ts (the assertion
on report.payload.javascript.kb) was raised without documentation; either revert
the numeric change or replace the magic number with a named constant (e.g.,
JS_PAYLOAD_KB_BUDGET) and add a short regression note/comment in the test that
states the reason, baseline value/date/measurement link, and reviewer who
approved the bump; update the test to use the named constant in
expect(report.payload.javascript.kb).toBeLessThan(JS_PAYLOAD_KB_BUDGET) so
future readers can see why the budget was increased.
In `@projects/forms/src/internal/schema.ts`:
- Around line 176-178: The code currently returns JSON.parse(value) for the
'array' and 'object' branches which can throw and violate the expected T |
undefined contract; update the branches handling 'array' and 'object' to wrap
JSON.parse(value) in a try/catch, returning the parsed value on success and
undefined on any JSON.parse error (optionally log/contextualize the error) so
callers receive undefined instead of an uncaught exception.
- Around line 179-180: The enum case currently uses schema.enum.includes(value)
which fails when form inputs are strings but enum members are numbers; update
the 'enum' branch in the parser to find a matching enum member by comparing
String(member) === String(value) (or coerce value to number if all enum members
are numeric), then return the matched enum member (not the raw input) cast to T;
locate the case 'enum' block that references schema.enum.includes(value) and
replace it with a lookup that returns the actual enum element when a stringified
match is found.
- Around line 174-175: The boolean branch (case 'boolean') incorrectly uses
"!!value" which treats any non-empty string like "false" as true; update the
branch to explicitly handle boolean and string inputs: if typeof value ===
'boolean' return value, else if value === 'true' return true, else if value ===
'false' return false, otherwise fall back to Boolean(value) or false as the
desired default. Ensure you replace the current return (!!value || value ===
'true') as T with these explicit checks referencing the same case 'boolean',
value variable, and T generic.
In `@projects/internals/tools/src/examples/utils.ts`:
- Around line 158-173: countTagDelta is recompiling three tag regexes per line
and interpolates raw tag text; modify findBlockEnd to escape the tag once (e.g.,
via a safe escape function), precompile the three RegExp objects (open,
selfClose, close) there, and either pass those RegExp instances into
countTagDelta (change its signature) or inline the counting logic in
findBlockEnd; keep the isSelfContainedBlock check intact and ensure behavior of
countTagDelta(openRegex, selfCloseRegex, closeRegex, line) matches the original
opens - selfCloses - closes result.
🪄 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: ea5b9426-3e20-4cb8-a226-6f4445217387
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
patches/monaco-editor.patchprojects/core/src/button-group/button-group.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/validation.examples.tsprojects/core/src/internal/utils/objects.tsprojects/core/src/tooltip/tooltip.test.lighthouse.tsprojects/forms/src/internal/schema.tsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/metadata/src/tasks/wireit.utils.tsprojects/internals/tools/src/examples/utils.tsprojects/lint/src/eslint/internals/utils.tsprojects/lint/src/eslint/rules/no-missing-control-label.tsprojects/lint/src/eslint/rules/no-nested-container-types.test.tsprojects/lint/src/eslint/rules/no-nested-container-types.tsprojects/monaco/src/internal/base/input.test.tsprojects/monaco/src/internal/base/input.tsprojects/monaco/src/internal/formats/problems-format.tsprojects/monaco/src/problems/problems.tsprojects/site/src/_internal/search/search.tsprojects/site/src/_internal/theme-preview/theme-preview.ts
💤 Files with no reviewable changes (1)
- projects/core/src/forms/validation.examples.ts
Comment on lines
+172
to
+173
| case 'number': | ||
| return parseFloat(value) as T; |
There was a problem hiding this comment.
Handle invalid numeric input by returning undefined instead of NaN.
parseFloat returns NaN for invalid input (e.g., "abc"), which violates the T | undefined contract. Callers expecting undefined for parse failures won't catch NaN, allowing invalid values to propagate.
🛡️ Proposed fix
case 'number':
- return parseFloat(value) as T;
+ const num = parseFloat(value);
+ return (Number.isNaN(num) ? undefined : num) as T;📝 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
| case 'number': | |
| return parseFloat(value) as T; | |
| case 'number': | |
| const num = parseFloat(value); | |
| return (Number.isNaN(num) ? undefined : num) as T; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/forms/src/internal/schema.ts` around lines 172 - 173, The number
branch in the parser (case 'number' in projects/forms/src/internal/schema.ts)
currently returns parseFloat(value) which yields NaN for invalid input; change
it to parseFloat the value into a temp variable, check Number.isNaN(result) and
return undefined on failure, otherwise return the parsed number cast to T so the
function adheres to the T | undefined contract.
298e0d8 to
5e95aa1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
projects/core/src/button-group/button-group.ts (1)
48-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo in JSDoc comment.
The word "selction" should be "selection".
📝 Proposed fix
- /** By default the button group is stateless. Add the `behavior-select` attribute and set to `single` or `multi` to enable stateful selction handling. */ + /** By default the button group is stateless. Add the `behavior-select` attribute and set to `single` or `multi` to enable stateful selection handling. */As per coding guidelines, "Follow Google developer documentation style guide and write-good rules for prose linting using Vale on JSDoc comments."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/src/button-group/button-group.ts` at line 48, Fix the typo in the JSDoc above the ButtonGroup implementation: change "selction" to "selection" in the comment that reads "By default the button group is stateless. Add the `behavior-select` attribute and set to `single` or `multi` to enable stateful selction handling." Update the JSDoc in projects/core/src/button-group/button-group.ts (the JSDoc for the ButtonGroup class) so it reads "selection" to comply with style/linting rules.patches/monaco-editor.patch (2)
241-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Delayer.cancel()now leaves pending work unresolved.Removing the reject path makes canceled
completionPromiseinstances potentially never settle, which can hang awaiters and retain resources longer than intended.Proposed fix
if (this.completionPromise) { - // this.doReject?.(new CancellationError()); + this.doReject?.(new CancellationError()); this.completionPromise = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 241 - 253, The patch removed calling this.doReject(new CancellationError()) in Delayer.cancel(), which leaves completionPromise unresolved and can hang awaiters; restore proper settlement by reintroducing the rejection: call this.doReject?.(new CancellationError()) before clearing this.completionPromise (and ensure any rejection is safe to observe where callers await it). Update the Delayer.cancel() implementation (references: Delayer.cancel, this.doReject, this.completionPromise, CancellationError) so canceled promises are rejected with CancellationError and then set completionPromise = null to avoid leaking unresolved promises.
804-805:⚠️ Potential issue | 🔴 CriticalUse the correct model event API (
onDidValidateVersion).This code subscribes via
model.onDidValidateModelVersion(...), but the model event added in this patch isonDidValidateVersion(...). At runtime, this will fail with a "is not a function" error.Proposed fix
export function onDidValidateModelVersion(model, listener) { - return model.onDidValidateModelVersion(listener); + return model.onDidValidateVersion(listener); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 804 - 805, The exported helper onDidValidateModelVersion is calling a non-existent API; update it to subscribe to the correct model event by replacing the call to model.onDidValidateModelVersion(...) with model.onDidValidateVersion(...); ensure the function name onDidValidateModelVersion remains as the exported symbol but uses model.onDidValidateVersion(listener) so runtime will not error.
♻️ Duplicate comments (2)
projects/site/src/_internal/search/search.ts (1)
226-229:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix eviction guard for empty-string cache keys.
This issue was already flagged in a previous review: the truthiness check on Line 227 will skip deletion when the oldest cache key is an empty string (
''), causing the cache to exceedCACHE_MAX_SIZE. Sinceinput.valueis passed directly tosearch()without validation, empty strings are valid cache keys.🔧 Recommended fix (from previous review)
`#evictOldestCacheEntryIfFull`() { if (this.#searchCache.size < DocsSearch.CACHE_MAX_SIZE) { return; } - const firstCacheKey = this.#searchCache.keys().next().value; - if (firstCacheKey) { - this.#searchCache.delete(firstCacheKey); - } + const firstEntry = this.#searchCache.keys().next(); + if (!firstEntry.done) { + this.#searchCache.delete(firstEntry.value); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/site/src/_internal/search/search.ts` around lines 226 - 229, The eviction guard skips deletion when the oldest cache key is an empty string because it checks truthiness; update the logic in the block that computes firstCacheKey (used with this.#searchCache.delete) to test for presence explicitly (e.g., check iterator result.done or compare firstCacheKey !== undefined) so empty-string keys are treated as valid and the oldest entry is removed when cache exceeds CACHE_MAX_SIZE; this change should be applied where firstCacheKey is computed and used in the search() / `#searchCache` eviction code.projects/core/src/internal/utils/objects.ts (1)
38-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPrefer direct assignment over
Object.assignfor a single key.This can be simplified to
target[key] = sourceValue;to avoid creating a temporary object.Proposed simplification
- Object.assign(target, { [key]: sourceValue }); + target[key] = sourceValue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/src/internal/utils/objects.ts` at line 38, Replace the single-key Object.assign usage with a direct property set to avoid creating a temporary object: change the call to Object.assign(target, { [key]: sourceValue }) to assign the value directly via target[key] = sourceValue; locate the occurrence where Object.assign is used with a single computed key (the Object.assign call shown) and update it to use direct assignment to target using the same key and sourceValue variables.
🤖 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/site/src/_internal/search/search.ts`:
- Around line 222-230: Add a JSDoc comment above the private helper
`#evictOldestCacheEntryIfFull` describing that it implements FIFO eviction using
Map insertion order, that it removes the oldest entry when
this.#searchCache.size >= DocsSearch.CACHE_MAX_SIZE, that it has no inputs and
returns void, and note constraints/side-effects (mutates this.#searchCache and
relies on Map iteration order) so callers understand behavior and limits.
In `@projects/site/src/_internal/theme-preview/theme-preview.ts`:
- Around line 40-46: Add a JSDoc comment above the helper function
collectCustomPropertiesFromRule explaining its purpose (extracts CSS custom
properties from a CSSStyleRule), describing parameters (rule: CSSStyleRule,
target: Map<string,string>), and documenting side effects (mutates the provided
target Map by setting property names to trimmed values). Also note that it only
considers properties starting with '--' and that values are trimmed, and mention
expected types to aid maintainability and IDE tooling.
---
Outside diff comments:
In `@patches/monaco-editor.patch`:
- Around line 241-253: The patch removed calling this.doReject(new
CancellationError()) in Delayer.cancel(), which leaves completionPromise
unresolved and can hang awaiters; restore proper settlement by reintroducing the
rejection: call this.doReject?.(new CancellationError()) before clearing
this.completionPromise (and ensure any rejection is safe to observe where
callers await it). Update the Delayer.cancel() implementation (references:
Delayer.cancel, this.doReject, this.completionPromise, CancellationError) so
canceled promises are rejected with CancellationError and then set
completionPromise = null to avoid leaking unresolved promises.
- Around line 804-805: The exported helper onDidValidateModelVersion is calling
a non-existent API; update it to subscribe to the correct model event by
replacing the call to model.onDidValidateModelVersion(...) with
model.onDidValidateVersion(...); ensure the function name
onDidValidateModelVersion remains as the exported symbol but uses
model.onDidValidateVersion(listener) so runtime will not error.
In `@projects/core/src/button-group/button-group.ts`:
- Line 48: Fix the typo in the JSDoc above the ButtonGroup implementation:
change "selction" to "selection" in the comment that reads "By default the
button group is stateless. Add the `behavior-select` attribute and set to
`single` or `multi` to enable stateful selction handling." Update the JSDoc in
projects/core/src/button-group/button-group.ts (the JSDoc for the ButtonGroup
class) so it reads "selection" to comply with style/linting rules.
---
Duplicate comments:
In `@projects/core/src/internal/utils/objects.ts`:
- Line 38: Replace the single-key Object.assign usage with a direct property set
to avoid creating a temporary object: change the call to Object.assign(target, {
[key]: sourceValue }) to assign the value directly via target[key] =
sourceValue; locate the occurrence where Object.assign is used with a single
computed key (the Object.assign call shown) and update it to use direct
assignment to target using the same key and sourceValue variables.
In `@projects/site/src/_internal/search/search.ts`:
- Around line 226-229: The eviction guard skips deletion when the oldest cache
key is an empty string because it checks truthiness; update the logic in the
block that computes firstCacheKey (used with this.#searchCache.delete) to test
for presence explicitly (e.g., check iterator result.done or compare
firstCacheKey !== undefined) so empty-string keys are treated as valid and the
oldest entry is removed when cache exceeds CACHE_MAX_SIZE; this change should be
applied where firstCacheKey is computed and used in the search() / `#searchCache`
eviction code.
🪄 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: 1a92748a-fb1e-4191-a5db-0ab71443bcc7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
patches/monaco-editor.patchprojects/core/src/button-group/button-group.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/validation.examples.tsprojects/core/src/internal/utils/objects.tsprojects/core/src/tooltip/tooltip.test.lighthouse.tsprojects/forms/src/internal/schema.tsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/metadata/src/tasks/wireit.utils.tsprojects/internals/tools/src/examples/utils.tsprojects/lint/src/eslint/internals/utils.tsprojects/lint/src/eslint/rules/no-missing-control-label.tsprojects/lint/src/eslint/rules/no-nested-container-types.test.tsprojects/lint/src/eslint/rules/no-nested-container-types.tsprojects/monaco/src/internal/base/input.test.tsprojects/monaco/src/internal/base/input.tsprojects/monaco/src/internal/formats/problems-format.tsprojects/monaco/src/problems/problems.tsprojects/site/src/_internal/search/search.tsprojects/site/src/_internal/theme-preview/theme-preview.ts
💤 Files with no reviewable changes (1)
- projects/core/src/forms/validation.examples.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
patches/monaco-editor.patch (2)
241-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Delayer.cancel()must still settle the pending promise.Line 241 previously rejected the pending completion promise; removing that settlement path causes callers awaiting the returned promise to hang indefinitely after cancellation.
Suggested fix
- // this.doReject?.(new CancellationError()); + this.doResolve?.(undefined); this.completionPromise = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 241 - 253, The change removed settling the pending promise in Delayer.cancel(), causing callers awaiting completionPromise to hang; restore promise settlement by calling the appropriate resolver instead of throwing: in the Delayer.cancel() implementation (look for methods named cancel, this.doReject, this.doResolve, and the completionPromise field), call this.doResolve?.(undefined) (or otherwise resolve the pending completionPromise with a neutral value) before setting this.completionPromise = null so the promise always settles on cancel while preserving the new behavior of not treating cancellation as an exception.
804-806:⚠️ Potential issue | 🟠 Major
onDidValidateModelVersioncalls the wrong model method and will fail at runtime.The patch defines
onDidValidateVersionon the model (line 767 interface, line 567 implementation), but the exported function tries to callmodel.onDidValidateModelVersion(listener)which doesn't exist.Suggested fix
export function onDidValidateModelVersion(model, listener) { - return model.onDidValidateModelVersion(listener); + return model.onDidValidateVersion(listener); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 804 - 806, The exported function onDidValidateModelVersion calls a non-existent model method; change it to call the actual implementation name onDidValidateVersion on the model (i.e., return model.onDidValidateVersion(listener)), or rename the model method to onDidValidateModelVersion so both names match; update the export in onDidValidateModelVersion to reference the exact method name used by the model implementation.
♻️ Duplicate comments (8)
projects/core/src/tooltip/tooltip.test.lighthouse.ts (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument or revert the increased JS payload budget.
Line 20 raises the allowed JS size to
14.6KB without test-local rationale, which can hide a real bundle regression. Please either revert to the previous threshold or add a measured justification (baseline/date/link) in the test.Suggested guardrail update
- expect(report.payload.javascript.kb).toBeLessThan(14.6); + // Baseline measured on YYYY-MM-DD in CI: X KB (see PR/issue: <link>) + const JS_PAYLOAD_KB_LIMIT = 14.4; + expect(report.payload.javascript.kb).toBeLessThan(JS_PAYLOAD_KB_LIMIT);As per coding guidelines, "Review /projects/site/src/docs/internal/guidelines/testing-lighthouse.md before writing performance tests; verify performance, accessibility, and best practices scoring."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/src/tooltip/tooltip.test.lighthouse.ts` at line 20, The test raises the JS budget by asserting expect(report.payload.javascript.kb).toBeLessThan(14.6) which lacks a documented baseline; either revert that numeric threshold to the prior value (restore the previous limit used before this change) or keep 14.6 but add an inline justification comment in the tooltip.test.lighthouse.ts test referencing the measurement details (baseline value, date, and a link or commit id) so future reviewers can verify the increase—update the assertion line’s surrounding test (the code that reads report.payload.javascript.kb) accordingly.projects/internals/tools/src/examples/utils.ts (1)
158-173:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrecompute and escape tag regexes once per block scan.
countTagDeltarecompiles three regexes per scanned line, which adds avoidable allocation churn infindBlockEnd. Build escaped regexes once perfindBlockEndcall and pass them intocountTagDelta.♻️ Proposed fix
+function escapeRegex(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +function buildTagRegex(tag: string): { open: RegExp; selfClose: RegExp; close: RegExp } { + const safeTag = escapeRegex(tag); + return { + open: new RegExp(`<${safeTag}[\\s>]`, 'g'), + selfClose: new RegExp(`<${safeTag}[^>]*/>`, 'g'), + close: new RegExp(`</${safeTag}>`, 'g') + }; +} + -function countTagDelta(line: string, tag: string): number { - const opens = (line.match(new RegExp(`<${tag}[\\s>]`, 'g')) || []).length; - const selfCloses = (line.match(new RegExp(`<${tag}[^>]*/>`, 'g')) || []).length; - const closes = (line.match(new RegExp(`</${tag}>`, 'g')) || []).length; +function countTagDelta(line: string, regex: { open: RegExp; selfClose: RegExp; close: RegExp }): number { + const opens = (line.match(regex.open) || []).length; + const selfCloses = (line.match(regex.selfClose) || []).length; + const closes = (line.match(regex.close) || []).length; return opens - selfCloses - closes; } function findBlockEnd(lines: string[], start: number, tag: string): number { if (isSelfContainedBlock(lines[start]!, tag)) { return start; } + const regex = buildTagRegex(tag); let depth = 1; for (let i = start + 1; i < lines.length; i++) { - depth += countTagDelta(lines[i]!, tag); + depth += countTagDelta(lines[i]!, regex); if (depth <= 0) { return i; } }#!/bin/bash # Verify current hotspot and call path for repeated regex construction. # Expected: three `new RegExp` calls in `countTagDelta`, and `findBlockEnd` invoking it in a loop. rg -n -C2 'function countTagDelta|new RegExp|function findBlockEnd|depth \+= countTagDelta' projects/internals/tools/src/examples/utils.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/internals/tools/src/examples/utils.ts` around lines 158 - 173, countTagDelta currently creates three RegExp objects per line; change findBlockEnd to build the escaped tag string and compile the three regexes once (e.g., openRe, selfCloseRe, closeRe) before the loop (use a proper escape for the tag), then update countTagDelta to accept those precompiled RegExp instances (or inline its logic into findBlockEnd) and use them inside the loop, keeping the existing behavior and continuing to call isSelfContainedBlock(lines[start], tag) where needed.projects/forms/src/internal/schema.ts (4)
172-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle invalid numeric input by returning
undefinedinstead ofNaN.
parseFloatreturnsNaNfor invalid input (e.g.,"abc"), which violates theT | undefinedcontract and allows invalid values to propagate to callers expectingundefinedfor parse failures.🛡️ Proposed fix
case 'number': - return parseFloat(value) as T; + const num = parseFloat(value); + return (Number.isNaN(num) ? undefined : num) as T;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/forms/src/internal/schema.ts` around lines 172 - 173, The 'case "number"' branch currently returns parseFloat(value) as T which yields NaN on invalid input; change it to parse the value into a number (e.g., const n = parseFloat(value)), then if Number.isNaN(n) return undefined as T | undefined, otherwise return n as T so invalid numeric input yields undefined instead of NaN; update the branch replacing the direct parseFloat(...) return with this NaN-check flow.
179-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnum parsing fails for numeric enum values from string form inputs.
Form input is always a string, so
schema.enum.includes(value)won't match numeric enum members (e.g., the string"1"won't match the number1in the enum array).🛡️ Proposed fix
case 'enum': - return (schema.enum.includes(value) ? value : undefined) as T; + if (schema.enum.includes(value)) return value as T; + const numericValue = Number(value); + return (Number.isNaN(numericValue) ? undefined : schema.enum.includes(numericValue) ? numericValue : undefined) as T;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/forms/src/internal/schema.ts` around lines 179 - 180, The enum branch currently checks schema.enum.includes(value) which fails when form inputs are strings but enum members are numbers; update the 'case "enum"' logic (referencing the switch case handling 'enum', the variables schema.enum and value) to match by comparing stringified members to the input (e.g., find a member where String(member) === String(value)) and return that matched enum member (cast to T) or undefined if none match; this ensures numeric enum values represented as strings from forms are correctly recognized.
176-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle invalid JSON input instead of throwing raw parse errors.
JSON.parse(value)throws on malformed JSON, bypassing theT | undefinedcontract and causing unhandled exceptions in callers expectingundefinedto signal parse failure.🛡️ Proposed fix
case 'array': case 'object': - return JSON.parse(value) as T; + try { + return JSON.parse(value) as T; + } catch { + return undefined; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/forms/src/internal/schema.ts` around lines 176 - 178, The switch branch that handles 'array' and 'object' currently does "return JSON.parse(value) as T" which throws on malformed JSON; wrap the parse in a try/catch inside the same function (the branch handling case 'array' / case 'object') so that on successful parse you return the parsed value cast to T and on any parse error you return undefined (or the expected failure sentinel) instead of letting the raw exception propagate; use the existing variable names (value) and keep the same return type T | undefined.
174-175:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBoolean parsing is incorrect for
"false"input.
!!valuecoerces any non-empty string totrue, so the string"false"incorrectly parses astrue.🐛 Proposed fix
case 'boolean': - return (!!value || value === 'true') as T; + if (value === 'true') return true as T; + if (value === 'false') return false as T; + return undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/forms/src/internal/schema.ts` around lines 174 - 175, The boolean parsing branch (case 'boolean' in internal/schema.ts) wrongly treats any non-empty string like "false" as true due to !!value; update the branch to explicitly detect truth by checking if the input is the boolean true or if its string representation lowercased equals "true" (avoid using coercion like !!value); change the return logic in the case 'boolean' branch to perform these explicit comparisons so "false" parses to false and "true" parses to true.projects/monaco/src/problems/problems.ts (1)
211-224:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
readystate when resetting disposed editor state.After disposal, Line 211–224 clears Monaco references but leaves
_internals.statesstill advertisingready.Suggested fix
`#resetEditorState`() { this.#monaco = undefined; this.#editor = undefined; this.#model = undefined; @@ this.#selectedLineNumber = undefined; this.#downLineNumber = undefined; this.#getProblemByLine = undefined; + this._internals.states.delete('ready'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/monaco/src/problems/problems.ts` around lines 211 - 224, The reset routine `#resetEditorState` clears Monaco references but doesn't update the internal readiness flag, leaving _internals.states.ready true after disposal; update `#resetEditorState` to explicitly clear or set _internals.states.ready to false (or reset the entire _internals.states object) so the editor is no longer advertised as ready when disposed, ensuring any code that checks _internals.states.ready observes the disposed state.projects/monaco/src/internal/base/input.ts (1)
454-469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequired-empty validation path should clear pending state before returning.
Line 454–457 can leave
:state(validating)set from prior pending syntax validation, and returns before firingsyntax-validation-changed.Suggested fix
`#handleValidationVersion`(model: monaco.editor.ITextModel, versionId: number) { @@ if (this.#isRequiredAndEmpty()) { + this.#clearValidation(); this.#setRequiredValidationError(); + this.dispatchEvent(new CustomEvent('syntax-validation-changed', { bubbles: true, composed: true })); return; } @@ this.dispatchEvent(new CustomEvent('syntax-validation-changed', { bubbles: true, composed: true })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/monaco/src/internal/base/input.ts` around lines 454 - 469, The required-empty branch in the validation flow can return while a prior ":state(validating)" remains set and no 'syntax-validation-changed' event is dispatched; fix by clearing pending validation state and emitting the change before returning: call the instance method `#clearValidation`() (or otherwise clear the validating state) immediately before or after invoking `#setRequiredValidationError`(), and ensure the existing dispatchEvent(new CustomEvent('syntax-validation-changed', ...)) is executed on that path so `#isRequiredAndEmpty`(), `#setRequiredValidationError`(), `#clearValidation`(), and dispatchEvent are all used to reset state and notify listeners prior to the early return.
🤖 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/internals/metadata/src/tasks/wireit.utils.ts`:
- Around line 98-107: Add a short JSDoc for the addDependency helper that
documents expected input types, its behavior and side-effects: state that first
param dep is expected to be a string or string-like dependency spec parsed by
parseDependency, second param script is an object with packagePath: string and
dependencies: string[] which will be mutated (script.dependencies will be
appended with `${packagePath}:${scriptName}`), and third param key is a
diagnostic string used in error logs; also note that the function swallows parse
errors and logs err.message. Place the JSDoc immediately above function
addDependency and reference parseDependency, script.dependencies mutation, and
the error logging behavior in the docstring.
- Line 105: Replace the interpolated console.error in wireit.utils.ts with
structured logging that emits fields for the dependency key and the full error
(message and stack) so parse failures are easily queryable; locate the call that
currently does console.error(`error parsing dependency in ${key}:`, (err as
Error).message) and change it to use the project's structured logger (e.g.,
logger.error or processLogger.error) and include properties like { event:
"dependency_parse_failure", key, error: err } or equivalent so both key and full
error details are recorded.
---
Outside diff comments:
In `@patches/monaco-editor.patch`:
- Around line 241-253: The change removed settling the pending promise in
Delayer.cancel(), causing callers awaiting completionPromise to hang; restore
promise settlement by calling the appropriate resolver instead of throwing: in
the Delayer.cancel() implementation (look for methods named cancel,
this.doReject, this.doResolve, and the completionPromise field), call
this.doResolve?.(undefined) (or otherwise resolve the pending completionPromise
with a neutral value) before setting this.completionPromise = null so the
promise always settles on cancel while preserving the new behavior of not
treating cancellation as an exception.
- Around line 804-806: The exported function onDidValidateModelVersion calls a
non-existent model method; change it to call the actual implementation name
onDidValidateVersion on the model (i.e., return
model.onDidValidateVersion(listener)), or rename the model method to
onDidValidateModelVersion so both names match; update the export in
onDidValidateModelVersion to reference the exact method name used by the model
implementation.
---
Duplicate comments:
In `@projects/core/src/tooltip/tooltip.test.lighthouse.ts`:
- Line 20: The test raises the JS budget by asserting
expect(report.payload.javascript.kb).toBeLessThan(14.6) which lacks a documented
baseline; either revert that numeric threshold to the prior value (restore the
previous limit used before this change) or keep 14.6 but add an inline
justification comment in the tooltip.test.lighthouse.ts test referencing the
measurement details (baseline value, date, and a link or commit id) so future
reviewers can verify the increase—update the assertion line’s surrounding test
(the code that reads report.payload.javascript.kb) accordingly.
In `@projects/forms/src/internal/schema.ts`:
- Around line 172-173: The 'case "number"' branch currently returns
parseFloat(value) as T which yields NaN on invalid input; change it to parse the
value into a number (e.g., const n = parseFloat(value)), then if Number.isNaN(n)
return undefined as T | undefined, otherwise return n as T so invalid numeric
input yields undefined instead of NaN; update the branch replacing the direct
parseFloat(...) return with this NaN-check flow.
- Around line 179-180: The enum branch currently checks
schema.enum.includes(value) which fails when form inputs are strings but enum
members are numbers; update the 'case "enum"' logic (referencing the switch case
handling 'enum', the variables schema.enum and value) to match by comparing
stringified members to the input (e.g., find a member where String(member) ===
String(value)) and return that matched enum member (cast to T) or undefined if
none match; this ensures numeric enum values represented as strings from forms
are correctly recognized.
- Around line 176-178: The switch branch that handles 'array' and 'object'
currently does "return JSON.parse(value) as T" which throws on malformed JSON;
wrap the parse in a try/catch inside the same function (the branch handling case
'array' / case 'object') so that on successful parse you return the parsed value
cast to T and on any parse error you return undefined (or the expected failure
sentinel) instead of letting the raw exception propagate; use the existing
variable names (value) and keep the same return type T | undefined.
- Around line 174-175: The boolean parsing branch (case 'boolean' in
internal/schema.ts) wrongly treats any non-empty string like "false" as true due
to !!value; update the branch to explicitly detect truth by checking if the
input is the boolean true or if its string representation lowercased equals
"true" (avoid using coercion like !!value); change the return logic in the case
'boolean' branch to perform these explicit comparisons so "false" parses to
false and "true" parses to true.
In `@projects/internals/tools/src/examples/utils.ts`:
- Around line 158-173: countTagDelta currently creates three RegExp objects per
line; change findBlockEnd to build the escaped tag string and compile the three
regexes once (e.g., openRe, selfCloseRe, closeRe) before the loop (use a proper
escape for the tag), then update countTagDelta to accept those precompiled
RegExp instances (or inline its logic into findBlockEnd) and use them inside the
loop, keeping the existing behavior and continuing to call
isSelfContainedBlock(lines[start], tag) where needed.
In `@projects/monaco/src/internal/base/input.ts`:
- Around line 454-469: The required-empty branch in the validation flow can
return while a prior ":state(validating)" remains set and no
'syntax-validation-changed' event is dispatched; fix by clearing pending
validation state and emitting the change before returning: call the instance
method `#clearValidation`() (or otherwise clear the validating state) immediately
before or after invoking `#setRequiredValidationError`(), and ensure the existing
dispatchEvent(new CustomEvent('syntax-validation-changed', ...)) is executed on
that path so `#isRequiredAndEmpty`(), `#setRequiredValidationError`(),
`#clearValidation`(), and dispatchEvent are all used to reset state and notify
listeners prior to the early return.
In `@projects/monaco/src/problems/problems.ts`:
- Around line 211-224: The reset routine `#resetEditorState` clears Monaco
references but doesn't update the internal readiness flag, leaving
_internals.states.ready true after disposal; update `#resetEditorState` to
explicitly clear or set _internals.states.ready to false (or reset the entire
_internals.states object) so the editor is no longer advertised as ready when
disposed, ensuring any code that checks _internals.states.ready observes the
disposed state.
🪄 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: e8d2e033-da24-4a00-9b8f-4bbbef0c7625
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
patches/monaco-editor.patchprojects/core/src/button-group/button-group.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/validation.examples.tsprojects/core/src/internal/utils/objects.tsprojects/core/src/tooltip/tooltip.test.lighthouse.tsprojects/forms/src/internal/schema.tsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/metadata/src/tasks/wireit.utils.tsprojects/internals/tools/src/examples/utils.tsprojects/lint/src/eslint/internals/utils.tsprojects/lint/src/eslint/rules/no-missing-control-label.tsprojects/lint/src/eslint/rules/no-nested-container-types.test.tsprojects/lint/src/eslint/rules/no-nested-container-types.tsprojects/monaco/src/internal/base/input.test.tsprojects/monaco/src/internal/base/input.tsprojects/monaco/src/internal/formats/problems-format.tsprojects/monaco/src/problems/problems.tsprojects/site/src/_internal/search/search.tsprojects/site/src/_internal/theme-preview/theme-preview.ts
💤 Files with no reviewable changes (1)
- projects/core/src/forms/validation.examples.ts
coryrylan
force-pushed
the
topic-lint-fixup-5
branch
3 times, most recently
from
e6746bc to
427accc
Compare
| const value = Math.round(absDiff / divisor); | ||
| if (value < max) return { value: sign * value, unit }; | ||
| } | ||
| return { value: sign * Math.round(absDiff / 31536000000), unit: 'year' }; |
Collaborator
There was a problem hiding this comment.
Might just be me, but it's odd to see hoisting to a const THRESHOLDS lookup that omits one unit expressed in the type union and specifying that omitted unit inline.
Collaborator
Author
There was a problem hiding this comment.
Refactored this a bit split out the divisors
| onMouseLeaveListener, | ||
| hoverProvider | ||
| ]; | ||
| for (const d of disposables) d.dispose(); |
Collaborator
There was a problem hiding this comment.
This looks like an LLM was trying to work around the lint limits and produced something silly.
Likely the better (and symmetrical!) refactor would be to break out the listener registration like was done in monaco/src/internal/base/input.ts in this changeset?
Collaborator
Author
There was a problem hiding this comment.
Fixed, no idea why it did this. There was no line violation.
johnyanarella
approved these changes
Apr 30, 2026
coryrylan
force-pushed
the
topic-lint-fixup-5
branch
from
427accc to
a87ddfa
Compare
- max-nested-callbacks - max-statements - max-depth Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-lint-fixup-5
branch
from
a87ddfa to
cb5fb8b
Compare
Collaborator
Author
|
🎉 This issue has been resolved in version 0.2.0 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.12 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
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 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.
Enabled following eslint rules:
Summary by CodeRabbit
Refactor
Chores
Tests
Documentation