Use fast invoke js extension methods in sync calls (#9917)#12426
Open
msynk wants to merge 28 commits into
Open
Use fast invoke js extension methods in sync calls (#9917)#12426msynk wants to merge 28 commits into
msynk wants to merge 28 commits into
Conversation
msynk
requested review from
Cyrus-Sushiant,
mhrastegari,
yasmoradi and
zahra-yousefi
|
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:
WalkthroughMigrates many JS interop call sites to FastInvoke with centralized error reporting and runtime validity checks, hardens TypeScript/DOM interactions with defensive guards and nullable types, refactors Extras resource loading to per-URL promise caching, adjusts component null-handling and lifecycle disposal, and adds comprehensive tests for sync-contract validation and runtime behavior. ChangesFastInvoke JS Interop Migration and Resilience
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs (1)
7-7:⚠️ Potential issue | 🟡 MinorMigrate the remaining
BitBlazorUI.Utilswrappers toFastInvoke(they’re synchronous)
src/BlazorUI/Bit.BlazorUI/Scripts/Utils.tsimplementsgetBodyWidth,setProperty,getProperty,getBoundingClientRect,scrollElementIntoView,selectText, andsetStyleas non-asyncfunctions that directly return values /void(no Promise usage). Update the correspondingUtilsJsRuntimeExtensions.csmethods to usejsRuntime.FastInvoke/FastInvokeVoidinstead ofInvoke/InvokeVoidfor consistency withBitUtilsToggleOverflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs` at line 7, The wrappers in UtilsJsRuntimeExtensions.cs that call BitBlazorUI.Utils should be converted from synchronous jsRuntime.Invoke/InvokeVoid to the FastInvoke/FastInvokeVoid helpers used by BitUtilsToggleOverflow; locate the methods that call "BitBlazorUI.Utils.getBodyWidth", "setProperty", "getProperty", "getBoundingClientRect", "scrollElementIntoView", "selectText" and "setStyle" and replace Invoke<T>/Invoke with jsRuntime.FastInvoke<T>/FastInvoke or InvokeVoid with FastInvokeVoid while preserving argument lists and return types so the calls remain synchronous and consistent with the non-async implementations in Utils.ts.src/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.ts (1)
15-15:⚠️ Potential issue | 🟠 MajorType contract mismatch:
setupparameter should accept nullabledotnetObj.The C# side declares
DotNetObjectReference<BitPullToRefresh>?as nullable, but the TSsetupmethod parameter is typed as requiredDotNetObject. The method will crash at runtime when callingdotnetObj.invokeMethodAsync()(lines 38, 64, 71, 75) if null is passed.While the current call site (BitPullToRefresh.razor.cs:148) always creates a non-null reference, the type contract allows null and should be enforced defensively.
Recommended fix: Make the parameter optional and add a null guard:
dotnetObj: DotNetObject | undefined) { if (!dotnetObj) return;Alternatively, change the C# signature to use non-nullable
DotNetObjectReference<BitPullToRefresh>if null should never be passed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.ts` at line 15, The setup function's dotnetObj parameter in BitPullToRefresh.ts is declared non-nullable but the C# contract allows null; update the setup signature (in the setup function) to accept DotNetObject | undefined (or optional) and add an early null guard (if (!dotnetObj) return;) before any calls to dotnetObj.invokeMethodAsync() (these occur in the setup function and in any helper closures referenced there such as the touch/move handlers), or alternatively change the C# DotNetObjectReference<BitPullToRefresh> to be non-nullable if null will never be passed.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts (1)
27-29: ⚡ Quick winRemove redundant
!element.stylecheck.The
!element.stylecondition is unreachable becauseHTMLElementalways has astyleproperty. Once the!elementcheck passes,element.styleis guaranteed to exist.♻️ Simplify the guard clause
- public static resetPaneDimensions(element: HTMLElement | undefined) { - if (!element || !element.style) return; - + public static resetPaneDimensions(element: HTMLElement | undefined) { + if (!element) return; +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts` around lines 27 - 29, The guard in BitSplitter.resetPaneDimensions redundantly checks !element.style after confirming element exists; remove the unreachable "!element.style" condition so the guard becomes "if (!element) return;" and update any surrounding comments accordingly, referencing the public static method resetPaneDimensions on BitSplitter to locate the change.
🤖 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
`@src/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cs`:
- Around line 20-23: BitPdfReaderRenderPage and BitPdfReaderRefreshPage are
still calling the old InvokeVoid; change both to use FastInvokeVoid for
consistency with BitPdfReaderDispose (which already uses FastInvokeVoid). Locate
the methods BitPdfReaderRenderPage and BitPdfReaderRefreshPage and replace
jsRuntime.InvokeVoid("BitBlazorUI.PdfReader.renderPage", ...) and
jsRuntime.InvokeVoid("BitBlazorUI.PdfReader.refreshPage", ...) with
jsRuntime.FastInvokeVoid("BitBlazorUI.PdfReader.renderPage", ...) and
jsRuntime.FastInvokeVoid("BitBlazorUI.PdfReader.refreshPage", ...) respectively,
leaving BitPdfReaderSetup (Invoke<int>) unchanged.
---
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.ts`:
- Line 15: The setup function's dotnetObj parameter in BitPullToRefresh.ts is
declared non-nullable but the C# contract allows null; update the setup
signature (in the setup function) to accept DotNetObject | undefined (or
optional) and add an early null guard (if (!dotnetObj) return;) before any calls
to dotnetObj.invokeMethodAsync() (these occur in the setup function and in any
helper closures referenced there such as the touch/move handlers), or
alternatively change the C# DotNetObjectReference<BitPullToRefresh> to be
non-nullable if null will never be passed.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs`:
- Line 7: The wrappers in UtilsJsRuntimeExtensions.cs that call
BitBlazorUI.Utils should be converted from synchronous
jsRuntime.Invoke/InvokeVoid to the FastInvoke/FastInvokeVoid helpers used by
BitUtilsToggleOverflow; locate the methods that call
"BitBlazorUI.Utils.getBodyWidth", "setProperty", "getProperty",
"getBoundingClientRect", "scrollElementIntoView", "selectText" and "setStyle"
and replace Invoke<T>/Invoke with jsRuntime.FastInvoke<T>/FastInvoke or
InvokeVoid with FastInvokeVoid while preserving argument lists and return types
so the calls remain synchronous and consistent with the non-async
implementations in Utils.ts.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts`:
- Around line 27-29: The guard in BitSplitter.resetPaneDimensions redundantly
checks !element.style after confirming element exists; remove the unreachable
"!element.style" condition so the guard becomes "if (!element) return;" and
update any surrounding comments accordingly, referencing the public static
method resetPaneDimensions on BitSplitter to locate the change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 73675b8f-81fa-4452-afd7-53844f2dc3f3
📒 Files selected for processing (20)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
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 `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs`:
- Line 25: The JS interop helper BitUtilsGetBoundingClientRect in
UtilsJsRuntimeExtensions currently returns
jsRuntime.FastInvoke<BoundingClientRect>(...) which can be null; update
BitUtilsGetBoundingClientRect to guard the FastInvoke result and return a
non-null BoundingClientRect (e.g. coalesce to a new BoundingClientRect with
Left/Top/Width/Height = 0) so callers like BitColorPicker.UpdateColor and
BitCircularTimePicker.UpdateTime can safely read properties without null checks
or NREs.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 401888d2-353d-481f-b50a-076162576f0f
📒 Files selected for processing (4)
src/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cs
- src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
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 `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Line 42: The current detection of an already-loaded script in the document
(the Array.from(document.scripts).find callback that assigns existingTag) uses
s.src.includes(url), which can produce false positives; change the predicate to
a stricter match — e.g., compare the script src filename or use
s.src.endsWith(url) (or strip query/hash and compare the last path segment) so
only exact filename/ending matches are considered; update the find call that
sets existingTag accordingly to use this precise match logic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bcc2884c-09ab-4c8c-a64e-75e6bb4e0ee6
📒 Files selected for processing (9)
src/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs (1)
15-20: ⚡ Quick winSerialize the tests that mutate
OnError.
IJSRuntimeFastExtensions.OnErroris process-global. These cases set/reset it per test, so they can race and flake if MSTest parallelization is enabled for the assembly or CI run. Mark this class[DoNotParallelize]or guard the hook with class-level synchronization.Also applies to: 76-80, 93-96, 109-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs` around lines 15 - 20, The tests mutate the process-global IJSRuntimeFastExtensions.OnError which can race when MSTest runs tests in parallel; update the test class to prevent parallel execution by adding the MSTest DoNotParallelize attribute (apply to the test class declaration) or alternatively wrap all accesses to IJSRuntimeFastExtensions.OnError used in the tests (setup/teardown and tests referencing it at lines with ResetErrorHandler and the tests at 76-80, 93-96, 109-112) with a class-level lock to serialize access; pick one approach and apply it consistently so the OnError hook is never concurrently read/modified across tests.
🤖 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 `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Around line 44-49: The script-matching currently reduces URLs to pathname only
via the normalize function and then compares normalize(s.src) to targetPath,
which conflates distinct origins and query strings; update the logic so
normalize preserves origin and query (or otherwise compute a full absolute URL
string) when generating targetPath and when comparing against document.scripts,
then use that full URL comparison to find existingTag (refer to the normalize
arrow function, targetPath constant and existingTag lookup) so scripts like
different CDNs or different ?v= queries are considered distinct.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs`:
- Around line 38-40: CollectAsyncJsMethods currently relies on
TsReturnsPromiseRegex to detect Promise-returning methods by body heuristics,
which yields false negatives for delegated returns; instead tighten the
convention so only explicit async functions or those with an explicit Promise
return type are treated as async. Update CollectAsyncJsMethods to stop using
TsReturnsPromiseRegex and only add methods to asyncJsMethods when the method has
the async modifier or its return annotation explicitly contains "Promise";
adjust any test expectations that assumed body-based detection and update
references to TsReturnsPromiseRegex (and related logic) accordingly.
---
Nitpick comments:
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs`:
- Around line 15-20: The tests mutate the process-global
IJSRuntimeFastExtensions.OnError which can race when MSTest runs tests in
parallel; update the test class to prevent parallel execution by adding the
MSTest DoNotParallelize attribute (apply to the test class declaration) or
alternatively wrap all accesses to IJSRuntimeFastExtensions.OnError used in the
tests (setup/teardown and tests referencing it at lines with ResetErrorHandler
and the tests at 76-80, 93-96, 109-112) with a class-level lock to serialize
access; pick one approach and apply it consistently so the OnError hook is never
concurrently read/modified across tests.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2defda38-b1ea-4eb2-8611-b1ad9efc9060
📒 Files selected for processing (30)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs (1)
20-28:⚠️ Potential issue | 🔴 CriticalChange to
InvokeVoidAsyncinstead ofInvokeVoid.Both
BitExtrasInitScriptsandBitExtrasInitStylesheetscall async JavaScript functions that return Promises and must be awaited. The JavaScript implementations useawait Promise.allSettled()to ensure all resources are loaded before resolving. UsingInvokeVoid()is fire-and-forget and returns a completed ValueTask immediately, breaking the async contract that callers expect. All call sites useawait, expecting the methods to wait for resource loading to complete.Use
InvokeVoidAsync()instead, which properly awaits the JavaScript Promise and returns a ValueTask that completes only after the JS function resolves.Suggested fix
public static ValueTask BitExtrasInitScripts(this IJSRuntime jsRuntime, IEnumerable<string> scripts, bool isModule = false) { return jsRuntime.InvokeVoidAsync("BitBlazorUI.Extras.initScripts", scripts, isModule); } public static ValueTask BitExtrasInitStylesheets(this IJSRuntime jsRuntime, IEnumerable<string> stylesheets) { return jsRuntime.InvokeVoidAsync("BitBlazorUI.Extras.initStylesheets", stylesheets); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs` around lines 20 - 28, The methods BitExtrasInitScripts and BitExtrasInitStylesheets are using InvokeVoid() which is a fire-and-forget operation that returns an immediately completed ValueTask, but the underlying JavaScript functions return Promises that must be awaited before resources are fully loaded. Replace InvokeVoid() with InvokeVoidAsync() in both method implementations to properly await the JavaScript Promise resolution and ensure callers' await statements actually wait for resource loading to complete.
🧹 Nitpick comments (4)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs (1)
130-135: 💤 Low valueMinor: redundant copy to result set.
Lines 130-133 copy
promiseMethodsintoresult, butresultis empty at this point and could be eliminated by directly returningpromiseMethods. This is harmless but adds a small allocation and iteration.♻️ Simplification
- foreach (var key in promiseMethods) - { - result.Add(key); - } - - return result; + return promiseMethods;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs` around lines 130 - 135, The foreach loop in lines 130-133 unnecessarily copies each item from `promiseMethods` into the empty `result` collection. Since `result` starts empty and is only populated with items from `promiseMethods`, eliminate this redundant loop by directly returning `promiseMethods` instead of copying its contents to `result`. This removes an unnecessary allocation and iteration while maintaining the same behavior.src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs (2)
62-64: 💤 Low valueClass-name uniqueness assumption in normalization.
LastTwoSegmentsreduces identifiers toClass.methodform, which assumes class names are unique across the scanned TypeScript sources. If multiple classes share the same name (e.g.,Namespace1.UtilsandNamespace2.Utils), a FastInvoke call to the synchronous method in one namespace could be flagged if the same method name in the other namespace is async. Given the single-project scope and typical TypeScript structure, this is a reasonable tradeoff, but worth noting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs` around lines 62 - 64, The LastTwoSegments method reduces identifiers to Class.method form by assuming class names are unique across TypeScript sources, which could cause false positives if multiple classes share the same name across different namespaces. Add a comment near the LastTwoSegments call explaining this assumption and documenting why it is acceptable for the current single-project scope and typical TypeScript structure, making the tradeoff explicit for future maintainers.
75-79: ⚖️ Poor tradeoffEvolution from past review: delegation gap now explicit design choice.
The past review flagged that Promise detection could miss delegated returns. The current implementation has evolved:
- Delegation IS propagated from methods with explicit
asyncor: Promise<...>annotations (see TsPromiseMethodScanner lines 101-117)- Delegation is NOT propagated from body-scanned methods (those detected via
return fetch(...)heuristics)This tradeoff is now explicitly documented in TsPromiseMethodScanner (lines 19-22) and tested (TsPromiseMethodScannerTests.cs lines 135-152). The conservative approach prevents heuristic false positives from cascading through delegation chains. If the codebase convention ensures all Promise-returning methods are explicitly marked
asyncor have: Promise<...>return types, this gap is acceptable. Otherwise, consider either removing body scanning entirely (require explicit annotations) or accepting the documented limitation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs` around lines 75 - 79, Ensure that the design tradeoff regarding Promise delegation propagation is explicitly and clearly documented in TsPromiseMethodScanner (around lines 19-22), specifically stating that delegation is propagated from methods with explicit async or : Promise<...> annotations but NOT from body-scanned heuristics like return fetch(...). Verify that TsPromiseMethodScannerTests (around lines 135-152) includes tests that validate this behavior and demonstrate the documented limitations. Additionally, confirm that the implementation in TsPromiseMethodScanner (lines 101-117) correctly implements this explicit design choice by only propagating delegation from explicitly annotated methods while intentionally excluding heuristically-detected methods.src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.ts (1)
26-30: ⚡ Quick winAdd context prefix to error logging for consistency.
The error log on line 29 lacks the
"BitBlazorUI.ColorPicker.dispose:"prefix used consistently inBitSwiper.ts,BitSplitter.ts, and other files in this layer, making debugging slightly harder.♻️ Suggested fix
try { bitController?.dotnetObj?.dispose(); } catch (e) { - console.error(e); + console.error("BitBlazorUI.ColorPicker.dispose:", e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.ts` around lines 26 - 30, The console.error call in the catch block of the dispose method lacks the context prefix used consistently across other components in this layer. Add the prefix "BitBlazorUI.ColorPicker.dispose:" to the error log in the catch block (where the console.error(e) statement is located) to match the logging convention used in BitSwiper.ts, BitSplitter.ts, and other files, improving debugging clarity.
🤖 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 `@src/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cs`:
- Around line 150-155: The XML documentation for SetupCompletedCallback states
it fires when the chart has been setup through interop and the JavaScript object
is available, but the current implementation invokes it unconditionally on first
render regardless of whether Config is null or setup succeeded. Either move the
await SetupCompletedCallback.InvokeAsync(this) call to execute only when setup
completes successfully by placing it inside the if (Config is not null) block,
or update the XML documentation comments to accurately reflect that the callback
fires unconditionally on first render. The recommended approach is to move the
callback invocation to only fire when setup actually succeeds so that consumers
relying on this callback to signal chart readiness do not receive false
positives.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cs`:
- Around line 98-104: Remove the null check from the IsRuntimeInvalid method
that returns true when jsRuntime is null. The method should not treat null as an
invalid runtime state; instead null should propagate naturally as a caller error
to fail fast. Reserve IsRuntimeInvalid() only for detecting real framework
runtime states like prerendering or disconnection, not for catching null
reference bugs which should throw immediately rather than silently no-op in
downstream Invoke*/FastInvoke* calls.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cs`:
- Around line 175-181: The ReportIfUnexpectedNull method is causing double error
reporting because it cannot distinguish between a null result that came from an
already-handled exception in FastInvoke* methods versus a truly unexpected null
from a valid runtime. The fix requires either: (1) modifying the contract so
that FastInvoke* methods return a result wrapper or sentinel value that
indicates whether the null came from an already-reported error, allowing
ReportIfUnexpectedNull to skip reporting in that case, or (2) moving the null
result validation logic entirely into the FastInvoke* methods themselves so that
ReportIfUnexpectedNull is not needed as a defensive check. Choose the approach
that best fits your architecture and ensure that only the initial interop
failure is reported, not a secondary InvalidOperationException for the same
underlying issue.
In `@src/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cs`:
- Around line 50-59: The Dispose method in BitPageVisibility is performing a
fire-and-forget JS interop call to "BitBlazorUI.PageVisibility.dispose" which
hides teardown failures and leaves the JS-side listener uncleared. Implement
IAsyncDisposable on the BitPageVisibility class instead of IDisposable, and
replace the Dispose method with a DisposeAsync method. In DisposeAsync, properly
await the JS cleanup operation using js.InvokeAsync instead of the
fire-and-forget js.InvokeVoid call, ensuring the _isInitialized flag is reset
after the async operation completes. This allows Blazor's DI lifecycle to
properly handle the asynchronous disposal of this scoped service.
---
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs`:
- Around line 20-28: The methods BitExtrasInitScripts and
BitExtrasInitStylesheets are using InvokeVoid() which is a fire-and-forget
operation that returns an immediately completed ValueTask, but the underlying
JavaScript functions return Promises that must be awaited before resources are
fully loaded. Replace InvokeVoid() with InvokeVoidAsync() in both method
implementations to properly await the JavaScript Promise resolution and ensure
callers' await statements actually wait for resource loading to complete.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.ts`:
- Around line 26-30: The console.error call in the catch block of the dispose
method lacks the context prefix used consistently across other components in
this layer. Add the prefix "BitBlazorUI.ColorPicker.dispose:" to the error log
in the catch block (where the console.error(e) statement is located) to match
the logging convention used in BitSwiper.ts, BitSplitter.ts, and other files,
improving debugging clarity.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs`:
- Around line 62-64: The LastTwoSegments method reduces identifiers to
Class.method form by assuming class names are unique across TypeScript sources,
which could cause false positives if multiple classes share the same name across
different namespaces. Add a comment near the LastTwoSegments call explaining
this assumption and documenting why it is acceptable for the current
single-project scope and typical TypeScript structure, making the tradeoff
explicit for future maintainers.
- Around line 75-79: Ensure that the design tradeoff regarding Promise
delegation propagation is explicitly and clearly documented in
TsPromiseMethodScanner (around lines 19-22), specifically stating that
delegation is propagated from methods with explicit async or : Promise<...>
annotations but NOT from body-scanned heuristics like return fetch(...). Verify
that TsPromiseMethodScannerTests (around lines 135-152) includes tests that
validate this behavior and demonstrate the documented limitations. Additionally,
confirm that the implementation in TsPromiseMethodScanner (lines 101-117)
correctly implements this explicit design choice by only propagating delegation
from explicitly annotated methods while intentionally excluding
heuristically-detected methods.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs`:
- Around line 130-135: The foreach loop in lines 130-133 unnecessarily copies
each item from `promiseMethods` into the empty `result` collection. Since
`result` starts empty and is only populated with items from `promiseMethods`,
eliminate this redundant loop by directly returning `promiseMethods` instead of
copying its contents to `result`. This removes an unnecessary allocation and
iteration while maintaining the same behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dfc615b4-856f-49f8-b792-bb6df3b8235a
📒 Files selected for processing (59)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewer.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/JsInteropConstants.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/PageVisibility.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
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 `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cs`:
- Around line 71-73: The error message in the ReportError method hardcodes
"using IJSInProcessRuntime" in the error output, but since this method is also
called from ReportIfUnexpectedNull on non-in-process paths, the message becomes
misleading. Replace the hardcoded runtime reference "using IJSInProcessRuntime"
with runtime-agnostic wording that does not assume a specific runtime type, such
as removing the runtime specification entirely or using a more generic
description that accurately applies to both in-process and other runtime
contexts.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec42ceb5-f536-42bf-a460-06c0c1bf09f1
📒 Files selected for processing (59)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewer.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/JsInteropConstants.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/PageVisibility.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cs (1)
175-181:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
ReportIfUnexpectedNullcan double-report a single FastInvoke failure.At Line 178/Line 180, a
nullresult fromFastInvoke*(already reported in catch paths at Line 265-268 and Line 365-368) is reported again asInvalidOperationException, inflating alerts for one root failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cs` around lines 175 - 181, The ReportIfUnexpectedNull method is reporting null results from FastInvoke calls as InvalidOperationException errors, but these failures are already being reported in the catch blocks of the FastInvoke methods themselves. This causes duplicate error reports for the same root failure. Remove the ReportError call from the ReportIfUnexpectedNull method and simply return the result without additional error reporting, since the actual exception is already captured and reported at the source in the FastInvoke catch paths.src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cs (1)
101-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not classify a
nullruntime as “invalid host runtime”.Line 104 currently converts a caller bug into silent no-op/default behavior in all wrappers. This hides DI/initialization defects that should fail fast.
Suggested minimal fix
- if (jsRuntime is null) return true; + ArgumentNullException.ThrowIfNull(jsRuntime);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cs` around lines 101 - 104, The null check for jsRuntime at line 104 in the method is incorrectly suppressing a legitimate initialization error by treating it as an invalid host runtime condition and returning true, which causes silent no-op behavior instead of failing fast. Remove the null check for jsRuntime (the if statement checking if jsRuntime is null and returning true) so that a null dependency injection value will propagate through and cause an appropriate failure rather than being silently suppressed, allowing initialization defects to be caught during development instead of hidden at runtime.
🤖 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
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs`:
- Around line 470-492: The `SkipReturnAnnotation` method incorrectly stops at
the first opening brace it encounters, which causes it to misparse return type
annotations that contain object literals like `{ [id: string]: GlState }`. To
fix this, modify the while loop that scans for the method body opening brace to
track the nesting depth of curly braces, square brackets, angle brackets, and
parentheses. Increment the depth counter when encountering opening delimiters
and decrement when encountering closing delimiters, then only stop when you
encounter an opening brace with a nesting depth of zero, which indicates the
actual method body start rather than a brace within the type annotation.
---
Duplicate comments:
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cs`:
- Around line 101-104: The null check for jsRuntime at line 104 in the method is
incorrectly suppressing a legitimate initialization error by treating it as an
invalid host runtime condition and returning true, which causes silent no-op
behavior instead of failing fast. Remove the null check for jsRuntime (the if
statement checking if jsRuntime is null and returning true) so that a null
dependency injection value will propagate through and cause an appropriate
failure rather than being silently suppressed, allowing initialization defects
to be caught during development instead of hidden at runtime.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cs`:
- Around line 175-181: The ReportIfUnexpectedNull method is reporting null
results from FastInvoke calls as InvalidOperationException errors, but these
failures are already being reported in the catch blocks of the FastInvoke
methods themselves. This causes duplicate error reports for the same root
failure. Remove the ReportError call from the ReportIfUnexpectedNull method and
simply return the result without additional error reporting, since the actual
exception is already captured and reported at the source in the FastInvoke catch
paths.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf23a31b-2bbc-43fe-87cf-07a55122d810
📒 Files selected for processing (59)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewer.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/JsInteropConstants.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/PageVisibility.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
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 `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Around line 241-245: The script caching mechanism in the code uses only the
URL (targetUrl) as the cache key, but it doesn't account for the isModule flag.
When kind equals 'script', modify the cache key construction to include the
isModule parameter so that the same URL loaded with different isModule values
(classic script vs module script) are cached separately. Update the cache key
assignment before the existingPromise lookup on line 244, and ensure any
subsequent cache storage operations use the same composite key format that
combines both the normalized URL and the isModule flag.
In `@src/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cs`:
- Around line 50-69: The DisposeAsync method in BitPageVisibility.cs only
catches JSDisconnectedException when invoking the JS dispose method, but other
exceptions like OperationCanceledException, JSException, and
InvalidOperationException can also be thrown and will prevent the local cleanup
code from executing. To fix this, restructure the try-catch block by moving the
_dotnetObj?.Dispose() and GC.SuppressFinalize(this) calls into a finally block
so that local cleanup always executes regardless of which exception is thrown
during the JS teardown invocation, ensuring deterministic disposal.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 47d1d232-530f-424b-9581-aefb43ca19b0
📒 Files selected for processing (59)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewer.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/JsInteropConstants.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/PageVisibility.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts (1)
197-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
isModulewhen reusing existing host script tags.Line 257 reuses an existing
<script>by URL only. Even though cache keys include module/classic mode,findExistingResource(Lines 197-203) can still treat a classic script as satisfying a module request (or vice versa), which breaks script execution semantics.Suggested fix
- private static findExistingResource(kind: 'script' | 'stylesheet', targetUrl: string): HTMLElement | undefined { + private static findExistingResource(kind: 'script' | 'stylesheet', targetUrl: string, isModule?: boolean): HTMLElement | undefined { if (kind === 'script') { return Array.from(document.scripts).find(s => !!s.src && Extras.normalizeResourceUrl(s.src) === targetUrl + && (isModule + ? s.type === 'module' + : (s.type === '' || s.type === 'text/javascript' || s.type === 'application/javascript')) && !s.hasAttribute('data-bit-load-failed') && !(document.readyState === 'complete' && !Extras.isHostScriptLoaded(s))); } @@ - const existingTag = Extras.findExistingResource(kind, targetUrl); + const existingTag = Extras.findExistingResource(kind, targetUrl, isModule);#!/bin/bash set -euo pipefail # Verify current matching logic and call site sed -n '190,275p' src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts # Verify whether both module and classic script init paths exist in the repo rg -nP --type=cs -C2 'BitExtrasInitScripts|initScripts\(' src/BlazorUIAlso applies to: 257-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts` around lines 197 - 203, The findExistingResource method only matches scripts by URL without verifying whether they are module or classic scripts, which can cause a classic script to be reused when a module script is needed (or vice versa). Update the findExistingResource method signature to accept an additional parameter indicating the required script type (module vs classic), then add a condition to the find predicate that verifies the existing script's type attribute matches the requested mode. Finally, update the call site where findExistingResource is invoked (around line 257) to pass the appropriate script type information so that module and classic scripts are correctly distinguished when reusing existing script tags.
🧹 Nitpick comments (1)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs (1)
101-153: ⚡ Quick winAdd a transitive delegation test to lock down fixpoint behavior.
Current coverage validates one-hop delegation, but not a two-hop chain from an explicit async/annotated source. A dedicated test here would prevent regressions in the scanner’s contract logic.
Suggested test case
+ [TestMethod] + public void CollectFromSource_PropagatesTransitiveSameClassDelegationFromAnnotatedCallee() + { + var ts = """ + namespace BitBlazorUI { + class Sample { + private static loadAsync(): Promise<void> { return Promise.resolve(); } + private static setup() { return Sample.loadAsync(); } + public static init() { return Sample.setup(); } + } + } + """; + + var promiseMethods = TsPromiseMethodScanner.CollectFromSource(ts); + + Assert.IsTrue(promiseMethods.Contains("Sample.loadAsync")); + Assert.IsTrue(promiseMethods.Contains("Sample.setup")); + Assert.IsTrue(promiseMethods.Contains("Sample.init")); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs` around lines 101 - 153, Add a new test method to the test class that validates two-hop transitive delegation behavior starting from an annotated or explicit async source. Create a test scenario with three methods where an annotated or explicitly async method is called by a second method, which is then called by a third method, and verify that the CollectFromSource method correctly identifies which methods should be included in the result according to the scanner's fixpoint contract (specifically testing whether transitive delegation chains are properly handled to prevent regression).
🤖 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
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs`:
- Around line 99-115: The delegation check in the while loop at the
BodyDelegatesToKnownMethods call on line 110 only validates against
explicitPromiseMethods, which never grows during the loop iterations. This
prevents the algorithm from finding transitive multi-hop Promise wrappers
because it cannot recognize methods that delegate to previously discovered
promiseMethods. Fix this by updating the BodyDelegatesToKnownMethods call to
check against a combined set that includes both explicitPromiseMethods and the
growing promiseMethods collection, allowing the fixpoint algorithm to follow
delegation chains across multiple hops.
---
Duplicate comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Around line 197-203: The findExistingResource method only matches scripts by
URL without verifying whether they are module or classic scripts, which can
cause a classic script to be reused when a module script is needed (or vice
versa). Update the findExistingResource method signature to accept an additional
parameter indicating the required script type (module vs classic), then add a
condition to the find predicate that verifies the existing script's type
attribute matches the requested mode. Finally, update the call site where
findExistingResource is invoked (around line 257) to pass the appropriate script
type information so that module and classic scripts are correctly distinguished
when reusing existing script tags.
---
Nitpick comments:
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs`:
- Around line 101-153: Add a new test method to the test class that validates
two-hop transitive delegation behavior starting from an annotated or explicit
async source. Create a test scenario with three methods where an annotated or
explicitly async method is called by a second method, which is then called by a
third method, and verify that the CollectFromSource method correctly identifies
which methods should be included in the result according to the scanner's
fixpoint contract (specifically testing whether transitive delegation chains are
properly handled to prevent regression).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c58c4c08-0b98-40ab-ac49-9fb4b1a256c7
📒 Files selected for processing (59)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewer.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/JsInteropConstants.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/PageVisibility.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
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 `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.ts`:
- Around line 13-17: The early return statement that checks for empty
inputElement?.files?.length is preventing the FileUpload.clear(id) call from
executing when append is false. Move the append mode check and
FileUpload.clear(id) call to occur before the empty files check so that the
clearing of prior files happens even when no new files are selected, preventing
stale files from being uploaded later under the same id.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7d7fc00-56a3-4db8-9312-09a537a1b80c
📒 Files selected for processing (59)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewer.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/JsInteropConstants.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/PageVisibility.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts (1)
171-194: 💤 Low valueEvent listener cleanup could be more explicit when a sibling listener settles first.
When
element.loadfires,settle()resolves the Promise, but thewindow.loadlistener remains registered until the page finishes loading. While this is benign (the Promise is already settled and subsequentsettle()calls are no-ops), it leaves a registered listener that allocates a closure. Consider extracting the cleanup:Suggested improvement
return new Promise<void>((res, rej) => { + const cleanup = () => { + element.removeEventListener('load', onLoad); + element.removeEventListener('error', onError); + window.removeEventListener('load', onWindowLoad); + }; const settle = () => { if (kind === 'stylesheet' && !Extras.isHostStylesheetApplied(element as HTMLLinkElement)) { rej(Extras.loadResourceError(kind, url)); + cleanup(); return; } if (kind === 'script' && !Extras.isHostScriptLoaded(element as HTMLScriptElement)) { rej(Extras.loadResourceError(kind, url)); + cleanup(); return; } res(); + cleanup(); }; - element.addEventListener('load', settle, { once: true }); - // On failure, mark the tag ... - element.addEventListener('error', () => { + const onLoad = settle; + const onError = () => { element.setAttribute('data-bit-load-failed', ''); settle(); - }, { once: true }); - // Final backstop: the window load event fires once all initial resources settle. - window.addEventListener('load', settle, { once: true }); + }; + const onWindowLoad = settle; + element.addEventListener('load', onLoad); + element.addEventListener('error', onError); + window.addEventListener('load', onWindowLoad); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts` around lines 171 - 194, The event listeners in the Promise settlement handler are left registered even after the Promise settles via one of them, which keeps a closure allocated longer than necessary. Add explicit cleanup by creating a cleanup function that removes all three event listeners (the load listener on element, the error listener on element, and the load listener on window), then call this cleanup function at the beginning of the settle() method to ensure listeners are removed as soon as any one of them triggers the settlement, preventing unnecessary closure allocation after the Promise is resolved or rejected.
🤖 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
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs`:
- Around line 29-30: The FastInvokeCallRegex pattern only matches literal string
identifiers enclosed in quotes within FastInvoke calls, causing it to miss cases
where identifiers are passed as variables like FastInvoke(variableName, ...).
Update the regex pattern in FastInvokeCallRegex to capture both quoted literal
strings and variable identifiers, then implement resolution logic that checks if
the captured argument is a variable and resolves it from any const string
declarations within the containing method or block scope to extract the actual
identifier value being used in the call.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Around line 171-194: The event listeners in the Promise settlement handler are
left registered even after the Promise settles via one of them, which keeps a
closure allocated longer than necessary. Add explicit cleanup by creating a
cleanup function that removes all three event listeners (the load listener on
element, the error listener on element, and the load listener on window), then
call this cleanup function at the beginning of the settle() method to ensure
listeners are removed as soon as any one of them triggers the settlement,
preventing unnecessary closure allocation after the Promise is resolved or
rejected.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0535a661-db66-4526-af3d-b81b8246dff4
📒 Files selected for processing (59)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewer.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/JsInteropConstants.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/PageVisibility.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Comment on lines
+29
to
+30
| private static readonly Regex FastInvokeCallRegex = | ||
| new(@"FastInvoke(?:Void)?\s*(?:<[^>]+>)?\s*\(\s*""(?<id>[^""]+)""", RegexOptions.Compiled); |
There was a problem hiding this comment.
FastInvoke call-site extraction misses identifier-based targets.
At Line 29, FastInvokeCallRegex only matches literal identifiers ("..."). Call sites like FastInvoke(identifier, ...) (e.g., the DataGrid extension in this PR) are skipped, so async-target violations can pass undetected.
Suggested direction
- private static readonly Regex FastInvokeCallRegex =
- new(@"FastInvoke(?:Void)?\s*(?:<[^>]+>)?\s*\(\s*""(?<id>[^""]+)""", RegexOptions.Compiled);
+ private static readonly Regex FastInvokeCallRegex =
+ new(@"FastInvoke(?:Void)?\s*(?:<[^>]+>)?\s*\(\s*(?<arg>""[^""]+""|\w+)", RegexOptions.Compiled);
+
+ private static readonly Regex ConstStringRegex =
+ new(@"\bconst\s+string\s+(?<name>\w+)\s*=\s*""(?<value>[^""]+)""\s*;", RegexOptions.Compiled);Then resolve arg as:
- literal → direct identifier
- variable → resolve from in-file
const stringmap (ideally scoped to the containing method/block)
Also applies to: 59-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs`
around lines 29 - 30, The FastInvokeCallRegex pattern only matches literal
string identifiers enclosed in quotes within FastInvoke calls, causing it to
miss cases where identifiers are passed as variables like
FastInvoke(variableName, ...). Update the regex pattern in FastInvokeCallRegex
to capture both quoted literal strings and variable identifiers, then implement
resolution logic that checks if the captured argument is a variable and resolves
it from any const string declarations within the containing method or block
scope to extract the actual identifier value being used in the call.
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
1 participant
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.
closes #9917
Summary by CodeRabbit
scrollElementIntoView(element-id based) and removed the option-id variant.OnErrorcallback.