fix: view listener never updates the cover widget's view field#8872
Open
matanrotman wants to merge 1 commit into
Open
fix: view listener never updates the cover widget's view field#8872matanrotman wants to merge 1 commit into
matanrotman wants to merge 1 commit into
Conversation
In `_DocumentCoverWidgetState.initState`, the `onViewUpdated` callback parameter is named `view`, which shadows the state field of the same name. `view = view` therefore assigns the parameter to itself and the field is never updated. The field keeps the `ViewPB` captured when the widget was first built, for the lifetime of the widget. It is read in `build`, most notably by `DocumentCover(view: view)`, so that widget receives a stale view even though the listener exists specifically to keep it current. Renaming the parameter makes the assignment do what it reads as. The icon and cover assignments on the surrounding lines are unaffected -- they already read from the parameter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFixes a shadowing bug in the document cover widget’s view listener so that the widget’s internal view state is correctly updated when the listener fires. Sequence diagram for updated view handling in DocumentCoverWidgetsequenceDiagram
actor User
participant DocumentCoverWidget
participant _DocumentCoverWidgetState
participant ViewListener
participant DocumentCover
User->>DocumentCoverWidget: open document view
DocumentCoverWidget->>_DocumentCoverWidgetState: initState()
_DocumentCoverWidgetState->>ViewListener: start(onViewUpdated)
ViewListener-->>_DocumentCoverWidgetState: onViewUpdated(updatedView)
_DocumentCoverWidgetState->>_DocumentCoverWidgetState: setState(update viewIcon, cover, view = updatedView)
_DocumentCoverWidgetState->>DocumentCover: build(DocumentCover(view))
DocumentCover->>DocumentCover: render with current view
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider renaming the state field
viewto something likecurrentViewto avoid shadowing with callback parameters and make the intent clearer throughout the widget.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming the state field `view` to something like `currentView` to avoid shadowing with callback parameters and make the intent clearer throughout the widget.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Bug
In
_DocumentCoverWidgetState.initState, theonViewUpdatedcallback parameter is namedview, which shadows the state field of the same name:view = viewresolves to the parameter on both sides, so the state field is never updated. It keeps theViewPBcaptured when the widget was first built, for the widget's lifetime.The field is not unused — it is read in
build(), most notably byDocumentCover(view: view), which therefore receives a stale view even though the listener exists specifically to keep it current. The twoview.idreads are unaffected in practice, since a view's id does not change.Fix
Rename the parameter to
updatedViewso the assignment does what it reads as. The icon and cover assignments on the surrounding lines are unchanged in behaviour — they already read from the parameter.Four lines, rename only. No new behaviour, no new imports or identifiers.
Note on verification
I found this while working in a fork and am reporting it as a latent correctness bug rather than a fix for a specific reproduction: I have not reproduced a user-visible symptom on unmodified
main, so I have deliberately not claimed one. The defect itself is unambiguous from the code.I am not able to compile upstream locally (this needs Flutter >= 3.32; I'm pinned to 3.27.4 by another dependency), so CI here will be its first real compile. Happy to adjust the naming or approach if you'd prefer it done differently.
Summary by Sourcery
Bug Fixes: