feat(UniverSheet): add auto switch darkmode function #859
Conversation
Reviewer's GuideAdds automatic dark mode switching for UniverSheet by wiring it into BootstrapBlazor’s theme change events, including registration and cleanup of a theme-change handler and a helper to update UniverSheet instances based on the current theme. Sequence diagram for UniverSheet automatic dark mode switchingsequenceDiagram
actor User
participant Browser
participant BootstrapBlazorTheme as BootstrapBlazorThemeManager
participant UniverSheetModule as UniverSheetModule
participant UniverSheetInstance as UniverSheetInstance
participant UniverAPI as UniverAPI
User->>Browser: Toggle theme control
Browser->>BootstrapBlazorTheme: Change theme
BootstrapBlazorTheme-->>Browser: Dispatch changed.bb.theme event
Browser->>UniverSheetModule: updateTheme(event)
UniverSheetModule->>UniverSheetModule: Query .bb-univer-sheet elements
UniverSheetModule->>UniverSheetInstance: Resolve instance from Data by id
UniverSheetModule->>UniverSheetModule: Check darkMode is null
UniverSheetModule->>UniverAPI: toggleDarkMode(theme == dark)
UniverAPI-->>UniverSheetInstance: Apply dark or light theme
Flow diagram for UniverSheet init and theme event lifecycleflowchart TD
A[init called with id invoke options] --> B[Get DOM element by id]
B --> C[createUniverSheetAsync]
C --> D[Store instance in Data with id]
D --> E[invoke TriggerReadyAsync]
E --> F[registerBootstrapBlazorModule UniverSheet id]
F --> G[EventHandler.on document changed.bb.theme updateTheme]
G --> H[changed.bb.theme event fires]
H --> I[updateTheme handler]
I --> J[Query .bb-univer-sheet elements]
J --> K[For each element get id]
K --> L[Get univerSheet instance from Data]
L --> M{univerSheet exists and darkMode is null}
M -- Yes --> N[Call univerAPI.toggleDarkMode with theme is dark]
M -- No --> O[Do nothing]
P[dispose called with id] --> Q[Get univerSheet from Data]
Q --> R[Call univerSheet.dispose if available]
R --> S[Get UniverSheet from window.BootstrapBlazor]
S --> T{UniverSheet exists}
T -- Yes --> U[UniverSheet.dispose id callback]
U --> V[EventHandler.off document changed.bb.theme updateTheme]
T -- No --> W[Skip event handler cleanup]
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}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The global theme handler is attached with
EventHandler.on(document, 'changed.bb.theme', updateTheme)on everyinitcall, which can lead to multiple identical listeners; consider registering this handler once (e.g., globally or keyed by module name) rather than per sheet instance. - In
updateTheme, queryingdocument.querySelectorAll('.bb-univer-sheet')on every theme change and then resolving each instance viaData.getmay be unnecessarily expensive; you could instead iterate directly over the stored UniverSheet instances inDataor maintain a registry of relevant IDs to avoid repeated DOM scanning.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The global theme handler is attached with `EventHandler.on(document, 'changed.bb.theme', updateTheme)` on every `init` call, which can lead to multiple identical listeners; consider registering this handler once (e.g., globally or keyed by module name) rather than per sheet instance.
- In `updateTheme`, querying `document.querySelectorAll('.bb-univer-sheet')` on every theme change and then resolving each instance via `Data.get` may be unnecessarily expensive; you could instead iterate directly over the stored UniverSheet instances in `Data` or maintain a registry of relevant IDs to avoid repeated DOM scanning.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds automatic dark mode switching functionality to the UniverSheet component, allowing it to respond to theme changes in the BootstrapBlazor framework. When the application theme changes, UniverSheet instances will automatically toggle their dark mode (unless manually configured).
- Implements theme change listener to automatically switch UniverSheet dark mode
- Integrates with BootstrapBlazor's module system for proper lifecycle management
- Bumps component version from 10.0.4 to 10.0.5
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| UniverSheet.razor.js | Adds event handler registration for theme changes, implements updateTheme function to toggle dark mode, and updates dispose logic to clean up event listeners |
| BootstrapBlazor.UniverSheet.csproj | Increments package version to 10.0.5 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const updateTheme = e => { | ||
| const theme = e.theme; | ||
|
|
||
| [...document.querySelectorAll('.bb-univer-sheet')].forEach(s => { | ||
| const id = s.getAttribute('id'); | ||
| if (id) { | ||
| const univerSheet = Data.get(id); | ||
| if (univerSheet && univerSheet.darkMode === null) { | ||
| const { univerAPI } = univerSheet; | ||
| if (univerAPI) { | ||
| univerAPI.toggleDarkMode(theme === 'dark'); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The event handler cleanup logic has a potential issue. The updateTheme function is defined at module level, which means it's shared across all UniverSheet instances. When removing the event listener in dispose, it will remove the handler for ALL instances, not just the one being disposed. This will break theme switching for any remaining UniverSheet instances.
The DockView component handles this correctly by storing instance-specific handler references. Consider storing the updateTheme handler reference per instance in the Data store, similar to how DockView does it.
| const { UniverSheet } = window.BootstrapBlazor; | ||
| if (UniverSheet) { | ||
| UniverSheet.dispose(id, () => { | ||
| EventHandler.off(document, 'changed.bb.theme', updateTheme); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Event handler registration happens inside the registerBootstrapBlazorModule callback, but cleanup happens conditionally based on window.BootstrapBlazor.UniverSheet existence. This creates an asymmetry - if the module system registers the handler but the global object isn't available during disposal, the event handler will leak.
Additionally, the handler is registered on document level for each instance, which means multiple instances will register the same handler multiple times. Consider registering the handler only once globally, or ensure proper cleanup that matches the registration pattern.
| [...document.querySelectorAll('.bb-univer-sheet')].forEach(s => { | ||
| const id = s.getAttribute('id'); | ||
| if (id) { | ||
| const univerSheet = Data.get(id); | ||
| if (univerSheet && univerSheet.darkMode === null) { | ||
| const { univerAPI } = univerSheet; | ||
| if (univerAPI) { | ||
| univerAPI.toggleDarkMode(theme === 'dark'); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
The updateTheme function queries all .bb-univer-sheet elements on every theme change, even if there's only one instance. This could impact performance if there are many instances. Additionally, the function doesn't verify that the queried elements are actual UniverSheet instances before attempting to retrieve data for them. Consider optimizing by only processing the relevant instance, or add validation before data retrieval.
| import { isFunction, registerBootstrapBlazorModule } from '../../BootstrapBlazor/modules/utility.js' | ||
| import { createUniverSheetAsync } from '../univer.js' | ||
| import Data from '../../BootstrapBlazor/modules/data.js' | ||
| import EventHandler from "../../BootstrapBlazor/modules/event-handler.js" |
There was a problem hiding this comment.
Import statements are inconsistent with string quote usage. Lines 1, 2, and 3 use single quotes, but line 4 uses double quotes. For consistency, all import statements should use the same quote style.
| import EventHandler from "../../BootstrapBlazor/modules/event-handler.js" | |
| import EventHandler from '../../BootstrapBlazor/modules/event-handler.js' |
Link issues
fixes #858
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add UniverSheet integration with BootstrapBlazor theme change events to support automatic dark mode switching and proper cleanup on disposal.
New Features:
Enhancements: