Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts SummerNote editor callback wiring so event handlers are only registered when corresponding callbacks exist, fixing the image upload event handling issue. Sequence diagram for SummerNote editor ImageUpload callback handlingsequenceDiagram
actor User
participant BrowserEditor as EditorComponent
participant SummerNote as SummerNoteEditor
participant Callbacks as CallbackRegistry
participant Handler as ImageUploadHandler
User->>BrowserEditor: Select image for upload
BrowserEditor->>SummerNote: Trigger ImageUpload with option.callbacks
SummerNote-->>SummerNote: Call option.callbacks.onImageUpload(fileList)
SummerNote->>Handler: onImageUpload(fileList)
Handler-->>SummerNote: Process upload
SummerNote-->>BrowserEditor: Update editor content
BrowserEditor-->>User: Render uploaded image
Flow diagram for reloadCallbacks option callback wiringflowchart TD
A["reloadCallbacks(id, option)"] --> B["Read window.BootstrapBlazor.SummerNote.callbacks"]
B --> C["Find cb where cb.id === id"]
C -->|cb not found| D["Do not register any callbacks"]
C -->|cb found| E["Iterate events: Blur, BlurCodeview, Change, ... , Scroll"]
E --> F["For each event get method cb[onEvent]"]
F -->|method is undefined| G["Skip this event"]
F -->|method exists| H["Assign option.callbacks[onEvent] = wrapper calling method.apply"]
H --> I["Next event"]
G --> I
I -->|all events processed| J["Callbacks registered only for existing methods"]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- By resolving
cbonce and only wiring callbacks when it exists, the change removes the previous ability to lazily register callbacks afterinit; consider whether callbacks need to be discoverable dynamically at call time as before. - Previously, all SummerNote events were wired to a no-op wrapper even if no handler was defined; now missing handlers result in no callback being set at all—if the editor or plugins rely on the presence of a callback key, you may want to preserve the wrapper and just guard inside it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By resolving `cb` once and only wiring callbacks when it exists, the change removes the previous ability to lazily register callbacks after `init`; consider whether callbacks need to be discoverable dynamically at call time as before.
- Previously, all SummerNote events were wired to a no-op wrapper even if no handler was defined; now missing handlers result in no callback being set at all—if the editor or plugins rely on the presence of a callback key, you may want to preserve the wrapper and just guard inside it.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 fixes a bug where image upload functionality in the Editor component was not working correctly. The issue was caused by empty callback wrappers being set up for all events, which interfered with the onImageUpload callback.
Key changes:
- Refactored the
reloadCallbacksfunction to only create callback wrappers for events that actually have registered callbacks - Bumped the package version to 10.0.1 (patch version for bug fix)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/BootstrapBlazor.SummerNote/Components/Editor/Editor.razor.js | Refactored callback registration logic to prevent empty wrappers from interfering with image upload functionality |
| src/components/BootstrapBlazor.SummerNote/BootstrapBlazor.SummerNote.csproj | Bumped version to 10.0.1 for this bug fix release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 #850
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Bug Fixes: