feat(JitViewer): add JitViewer component#8024
Merged
Merged
Conversation
Contributor
Reviewer's GuideAdds a new JitViewer demo page, JS interop bootstrap, and associated sample assets, and wires it into the server demo navigation and localization resources. Sequence diagram for JitViewer initialization and theme handlingsequenceDiagram
actor User
participant JitViewers
participant JitViewer
participant SampleJsModule
participant UtilityModule
participant WindowJitViewer
participant Viewer
participant EventHandler
User->>JitViewers: navigate /jit-viewer
JitViewers->>JitViewer: render File parameter
JitViewer->>SampleJsModule: init(id, invoke, options)
SampleJsModule->>SampleJsModule: addLink(jit-viewer.css)
SampleJsModule->>SampleJsModule: addScript(jit-viewer.min.js)
SampleJsModule->>UtilityModule: getTheme()
UtilityModule-->>SampleJsModule: theme
SampleJsModule->>WindowJitViewer: createViewer(target, options)
WindowJitViewer-->>SampleJsModule: viewer
SampleJsModule->>Viewer: mount()
SampleJsModule->>EventHandler: on(document, changed.bb.theme, updateTheme)
EventHandler-->>Viewer: setTheme(e.theme) on theme change
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
JitViewersclass is duplicated inwwwroot/samples/sample.cswith what appears to be real component code under a staticwwwrootpath; consider replacing this with a minimal, non-compilable sample (e.g., stripped-down snippet or plain text) to avoid namespace/type duplication and accidental compilation issues. - In
wwwroot/samples/sample.js, thechanged.bb.themeevent handler is registered ininitbut never removed indispose; consider unsubscribing indisposeto prevent leaks when the viewer is destroyed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `JitViewers` class is duplicated in `wwwroot/samples/sample.cs` with what appears to be real component code under a static `wwwroot` path; consider replacing this with a minimal, non-compilable sample (e.g., stripped-down snippet or plain text) to avoid namespace/type duplication and accidental compilation issues.
- In `wwwroot/samples/sample.js`, the `changed.bb.theme` event handler is registered in `init` but never removed in `dispose`; consider unsubscribing in `dispose` to prevent leaks when the viewer is destroyed.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor.Server/wwwroot/samples/sample.js" line_range="51-61" />
<code_context>
+ }
+}
+
+export function dispose(id) {
+ const data = Data.get(id);
+ if (data === null) {
+ return;
+ }
+
+ const { viewer } = data;
+ if (viewer) {
+ viewer.destroy();
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Data store entries are not cleared on dispose, which can lead to stale references.
`dispose` destroys the viewer but leaves its entry in `Data`, which can keep DOM and delegate references alive unnecessarily. After destroying the viewer, also remove the entry (e.g. `Data.remove(id)`) so the store doesn’t retain stale objects.
```suggestion
export function dispose(id) {
const data = Data.get(id);
if (data === null) {
return;
}
const { viewer } = data;
if (viewer) {
viewer.destroy();
}
Data.remove(id);
}
```
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor.Server/wwwroot/samples/sample.md" line_range="5" />
<code_context>
+
+### Bugs
+* fix(Table): update resetTableWidth method by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7836
+* fix(DockView): TitleTemplate set ShowClose to false not work by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7875
+* fix(DataTableDynamicContext): not unload BootstrapBlazor_DynamicAssembly under IsExcel mode by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7900
+* fix(Localizer): makes JsonStringLocalizerFactory thread-safe by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7919
</code_context>
<issue_to_address>
**suggestion (typo):** Consider rephrasing this bullet to fix the grammar around "not work".
For example: "TitleTemplate with ShowClose set to false does not work" or "When ShowClose is set to false, TitleTemplate does not work."
```suggestion
* fix(DockView): When ShowClose is set to false, TitleTemplate does not work by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7875
```
</issue_to_address>
### Comment 3
<location path="src/BootstrapBlazor.Server/wwwroot/samples/sample.md" line_range="6" />
<code_context>
+### Bugs
+* fix(Table): update resetTableWidth method by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7836
+* fix(DockView): TitleTemplate set ShowClose to false not work by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7875
+* fix(DataTableDynamicContext): not unload BootstrapBlazor_DynamicAssembly under IsExcel mode by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7900
+* fix(Localizer): makes JsonStringLocalizerFactory thread-safe by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7919
+
</code_context>
<issue_to_address>
**suggestion (typo):** The phrase "not unload" is missing an auxiliary verb.
Consider changing this to “does not unload BootstrapBlazor_DynamicAssembly under IsExcel mode” (or similar) so the sentence has the auxiliary verb and reads more naturally.
```suggestion
* fix(DataTableDynamicContext): does not unload BootstrapBlazor_DynamicAssembly under IsExcel mode by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7900
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+61
| export function dispose(id) { | ||
| const data = Data.get(id); | ||
| if (data === null) { | ||
| return; | ||
| } | ||
|
|
||
| const { viewer } = data; | ||
| if (viewer) { | ||
| viewer.destroy(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Data store entries are not cleared on dispose, which can lead to stale references.
dispose destroys the viewer but leaves its entry in Data, which can keep DOM and delegate references alive unnecessarily. After destroying the viewer, also remove the entry (e.g. Data.remove(id)) so the store doesn’t retain stale objects.
Suggested change
| export function dispose(id) { | |
| const data = Data.get(id); | |
| if (data === null) { | |
| return; | |
| } | |
| const { viewer } = data; | |
| if (viewer) { | |
| viewer.destroy(); | |
| } | |
| } | |
| export function dispose(id) { | |
| const data = Data.get(id); | |
| if (data === null) { | |
| return; | |
| } | |
| const { viewer } = data; | |
| if (viewer) { | |
| viewer.destroy(); | |
| } | |
| Data.remove(id); | |
| } |
|
|
||
| ### Bugs | ||
| * fix(Table): update resetTableWidth method by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7836 | ||
| * fix(DockView): TitleTemplate set ShowClose to false not work by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7875 |
Contributor
There was a problem hiding this comment.
suggestion (typo): Consider rephrasing this bullet to fix the grammar around "not work".
For example: "TitleTemplate with ShowClose set to false does not work" or "When ShowClose is set to false, TitleTemplate does not work."
Suggested change
| * fix(DockView): TitleTemplate set ShowClose to false not work by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7875 | |
| * fix(DockView): When ShowClose is set to false, TitleTemplate does not work by @ArgoZhang in https://github.com/dotnetcore/BootstrapBlazor/pull/7875 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8024 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 766 766
Lines 34123 34123
Branches 4692 4692
=========================================
Hits 34123 34123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new JitViewer demo entry to the BootstrapBlazor.Server samples site (menu + docs routing) and includes additional sample files under wwwroot/samples for previewing different formats.
Changes:
- Added a new
/jit-viewersample page (JitViewers) and wired it into docs routing and the demo menu. - Added localized menu text for
JitViewerin en-US/zh-CN. - Added several new static sample files (txt/md/js/css/csv/dxf/cs) for the viewer demo to open.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor.Server/wwwroot/samples/sample.txt | Adds a plain-text sample file for preview. |
| src/BootstrapBlazor.Server/wwwroot/samples/sample.md | Adds a markdown sample file for preview. |
| src/BootstrapBlazor.Server/wwwroot/samples/sample.js | Adds a JavaScript sample file for preview. |
| src/BootstrapBlazor.Server/wwwroot/samples/sample.dxf | Adds a DXF sample file for preview. |
| src/BootstrapBlazor.Server/wwwroot/samples/sample.csv | Adds a CSV sample file for preview. |
| src/BootstrapBlazor.Server/wwwroot/samples/sample.css | Adds a CSS sample file for preview. |
| src/BootstrapBlazor.Server/wwwroot/samples/sample.cs | Adds a C# sample file for preview. |
| src/BootstrapBlazor.Server/Locales/zh-CN.json | Adds the JitViewer menu localization entry (zh-CN). |
| src/BootstrapBlazor.Server/Locales/en-US.json | Adds the JitViewer menu localization entry (en-US). |
| src/BootstrapBlazor.Server/Extensions/MenusLocalizerExtensions.cs | Adds the JitViewer menu item pointing to jit-viewer. |
| src/BootstrapBlazor.Server/docs.json | Registers jit-viewer → JitViewers for docs routing. |
| src/BootstrapBlazor.Server/Components/Samples/JitViewers.razor.cs | Adds the backing code and selectable sample document list. |
| src/BootstrapBlazor.Server/Components/Samples/JitViewers.razor | Adds the new sample page UI that hosts <JitViewer>. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Introduction": "Introduction", | ||
| "Ip": "IpAddress", | ||
| "JitViewer": "JitViewer", | ||
| "JSExtension": "JSRuntime Extensions", |
| "Introduction": "简介", | ||
| "Ip": "IP 地址 IpAddress", | ||
| "JitViewer": "文件预览器 JitViewer", | ||
| "JSExtension": "JSRuntime 扩展", |
Comment on lines
+17
to
+20
| <JitViewer File="@_doc"></JitViewer> | ||
| </DemoBlock> | ||
|
|
||
| <AttributeTable Type="typeof(JitViewer)"></AttributeTable> |
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
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.
Link issues
fixes #7880
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a new JitViewer sample page and integrate it into the demo navigation.
New Features:
Enhancements: