fix(ConnectionState): unify connection selection with block selection pattern#300
Open
lilylilps wants to merge 2 commits into
Open
fix(ConnectionState): unify connection selection with block selection pattern#300lilylilps wants to merge 2 commits into
lilylilps wants to merge 2 commits into
Conversation
Reviewer's GuideRefactors connection selection so that File-Level Changes
Possibly 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:
- In
ConnectionState.$selectedyou accessthis.$rawState.value.idwhile$rawStateis initialized withundefined; depending on the signal implementation this may run before the constructor assigns a value and cause a runtime error, so consider guarding for!this.$rawState.valueor providing a non-undefined initial value. asTConnectionandtoJSONnow duplicate the same merge logic of$rawStatewith$selected; consider extracting a small helper to avoid divergence if this shape changes again.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ConnectionState.$selected` you access `this.$rawState.value.id` while `$rawState` is initialized with `undefined`; depending on the signal implementation this may run before the constructor assigns a value and cause a runtime error, so consider guarding for `!this.$rawState.value` or providing a non-undefined initial value.
- `asTConnection` and `toJSON` now duplicate the same merge logic of `$rawState` with `$selected`; consider extracting a small helper to avoid divergence if this shape changes again.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Preview is ready. |
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.
Problem
Connection selection state was inconsistent with the block selection pattern.
ConnectionStatestored selected directly in$statesignal, whileBlockStatederives it from the selection bucket via a computed signal. This caused a bug where omitting the selected field from connection data (e.g. when callingsetEntities) would not reset selection —Object.assigninupdateConnectionpreserved the stale selected: true value since undefined keys don't overwrite existing ones.Solution
Aligned ConnectionState architecture with BlockState:
$statewas a writablesignal<T>holding all connection data including selected$rawState(protected signal) holds raw data;$stateis a computed signal that derives selected from the selection bucketThis makes the selection bucket the single source of truth for connection selection, exactly as it is for blocks.
Changes
ConnectionState.ts:$statesignal →$rawState(protected) + computed$statethat overrides selected from$selected$selecteddeclaration before $state to avoid circular dependency (reads id from$rawState)updateConnection()now writes to$rawStateisSelected()reads from$selected(bucket-derived)toJSON()/asTConnection()construct output from$rawState+$selectedConnectionList.ts:setSelectionmethod (was never called, wrote directly to$state.selected)deleteSelectedConnectionsnow checks$selected.valueinstead of$state.value.selectedSummary by Sourcery
Unify connection selection handling with the existing block selection pattern so that selection state is always derived from the selection bucket rather than stored directly on connection entities.
Bug Fixes:
Enhancements:
Tests: