chore: force select only the matching guide when forced guide key is set#843
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
thomaswhyyou
force-pushed
the
thomas-kno-11142-sdk-guide-toolbar-v2-poc-1
branch
from
3ca6b2e to
bfd9122
Compare
|
@cursor review |
thomaswhyyou
changed the title
thomaswhyyou
requested review from
kylemcd and
meryldakin
and removed request for
a team
| @@ -1,2 +1,2 @@ | |||
| nodejs 24.12.0 | |||
| nodejs 24.13.0 | |||
There was a problem hiding this comment.
Unrelated, upgrading to the same node version we are using in control.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| // If in debug mode, put the forced guide at the beginning of the display sequence. | ||
| if (state.debug.forcedGuideKey) { | ||
| const forcedKeyIndex = displaySequence.indexOf(state.debug.forcedGuideKey); |
There was a problem hiding this comment.
Preview guides not in display sequence cannot be shown
Low Severity
The removal of the displaySequence.unshift(state.debug.forcedGuideKey) logic means guides that exist only in previewGuides (but aren't yet in the display_sequence) can no longer be force-shown. Previously, the forced guide key was always added to the front of the display sequence copy, allowing iteration over keys not originally in the sequence. Now, only keys already in display_sequence are iterated, so a newly-created guide being previewed before publication to a group won't appear.
There was a problem hiding this comment.
This is true, and I think we should move toward the behavior where only published guides can be rendered.
|
don't forget a changeset! |
thomaswhyyou
force-pushed
the
thomas-kno-11142-sdk-guide-toolbar-v2-poc-1
branch
from
8c99027 to
9243f08
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| guide = state.previewGuides[guideKey]; | ||
| } | ||
|
|
||
| const guide = state.previewGuides[guideKey] || state.guides[guideKey]; |
There was a problem hiding this comment.
Preview content leaks beyond forced guide
Medium Severity
select now always prefers state.previewGuides[guideKey] over state.guides[guideKey], even when debug.forcedGuideKey is not set. This lets preview payloads override normal guide data for any matching key, so regular selection can render preview versions outside the intended forced-guide path.
Not releasable by itself in this PR, so I will include one in a downstream PR when we are ready to get it out in a release. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #843 +/- ##
==========================================
+ Coverage 68.35% 68.53% +0.18%
==========================================
Files 193 193
Lines 8122 8067 -55
Branches 1076 1073 -3
==========================================
- Hits 5552 5529 -23
+ Misses 2550 2513 -37
- Partials 20 25 +5
|
3 participants
Description
This is prep work ahead of actual Guide Toolbar v2 changes in downstream PRs.
Taking a step back - right now we have a fair amount of Live Preview logic living in the guide client module directly, and it's resulting in a lot of added complexities that I think would be better contained in a separate toolbar/debugging module. I want to pull out as much "debugging" related code out of the guide client, and try to keep to a minimal set of properties the guide client understands to provide debugging behaviors/functionalities.
This PR simplifies the current live preview related logic by only force rendering the target guide when
forcedGuideKeyis present (vs rendering other guides also). This was brought up a few times as a point of confusion when using live preview, so I think this makes sense from both the code and the product perspectives. I think eventually I want to get rid of "live preview" with streaming updates altogether, but that's to be discussed later.