fix(sidebar): improve pin feedback and state management for session i… - #1473
Conversation
📝 WalkthroughWalkthroughThis PR refactors the sidebar's pin/unpin UI and animation system. Key changes include removing chevron icons, introducing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/renderer/src/components/WindowSideBar.vue (1)
884-897: Consider consolidating cleanup logic.The
clone.remove()call appears both in the success path (line 884) and thefinallyblock (line 896). While this is safe (removing an already-removed element is a no-op), it may indicate the cleanup could be streamlined.The
finallyblock is meant to guarantee cleanup, but since lines 884-890 already handle the success case, and the early return at line 850 handles the error case, thefinallyblock only needs to handle unexpected exceptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/WindowSideBar.vue` around lines 884 - 897, Consolidate the duplicate cleanup by moving all guaranteed teardown into the finally block: remove the duplicate clone.remove(), pinDockedSessionId.value reset, and pinFlightSessionId.value reset from the success path and keep a single cleanup sequence in the finally block that calls clone.remove(), clears pinDockedSessionId.value if it matches session.id, and sets pinFlightSessionId.value = null; ensure applyPinFeedback(session.id, nextPinned) and await nextTick() remain in the try-success path so only teardown is centralized in finally and unexpected exceptions still trigger cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/src/components/WindowSideBarSessionItem.vue`:
- Around line 373-375: The CSS rule for .session-item[data-pin-state='docked']
.pin-button:hover .pin-button__icon contains an invalid transform value
("scale(1.06) 2s"); remove the stray "2s" from the transform and, if the intent
was an animated effect, add a separate transition property (e.g., transition:
transform 0.2s ease) on .session-item[data-pin-state='docked'] .pin-button__icon
so hover only changes the transform to translateY(-1px) scale(1.06) while timing
is handled by transition.
- Around line 117-129: Add a new computed property named pinVisualState in the
WindowSideBarSessionItem component that returns 'overlay' when pinFeedbackMode
indicates a pinning animation/feedback (e.g., truthy or a specific mode) and
returns 'docked' otherwise, then bind it to the root div as
:data-pin-visual-state="pinVisualState" alongside the existing :data-pin-state
and :data-pin-fx attributes so tests expecting the separate visual state
(overlay vs docked) pass.
In `@test/renderer/components/WindowSideBarSessionItem.test.ts`:
- Around line 118-129: The test is asserting a non-existent attribute
data-pin-visual-state on the .session-item element; update the component
template (the element that currently binds :data-pin-state="pinState") to also
bind :data-pin-visual-state="pinVisualState" (or whatever computed/prop name you
use for visual state) so the attribute is rendered, or alternatively modify the
tests in WindowSideBarSessionItem.test.ts to stop asserting
data-pin-visual-state and only assert data-pin-state; locate bindings around the
.session-item in the component where :data-pin-state="pinState" is defined and
add the matching visual-state binding or adjust the test expectations
accordingly.
---
Nitpick comments:
In `@src/renderer/src/components/WindowSideBar.vue`:
- Around line 884-897: Consolidate the duplicate cleanup by moving all
guaranteed teardown into the finally block: remove the duplicate clone.remove(),
pinDockedSessionId.value reset, and pinFlightSessionId.value reset from the
success path and keep a single cleanup sequence in the finally block that calls
clone.remove(), clears pinDockedSessionId.value if it matches session.id, and
sets pinFlightSessionId.value = null; ensure applyPinFeedback(session.id,
nextPinned) and await nextTick() remain in the try-success path so only teardown
is centralized in finally and unexpected exceptions still trigger cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c907a74e-6bbf-41f2-a49b-f499246c6fe8
📒 Files selected for processing (3)
src/renderer/src/components/WindowSideBar.vuesrc/renderer/src/components/WindowSideBarSessionItem.vuetest/renderer/components/WindowSideBarSessionItem.test.ts
| <template> | ||
| <div | ||
| class="session-item select-none cursor-pointer no-drag flex w-full items-center gap-1 rounded-md px-2 py-1.5 text-left transition-colors duration-200" | ||
| :class="active ? 'bg-accent text-accent-foreground' : 'text-foreground/80 hover:bg-accent/50'" | ||
| class="session-item no-drag flex w-full cursor-pointer select-none items-center rounded-lg px-2.5 text-left transition-colors duration-200" | ||
| :class="[ | ||
| active ? 'bg-accent text-accent-foreground' : 'text-foreground/80 hover:bg-accent/50', | ||
| heroHidden && 'is-hero-hidden' | ||
| ]" | ||
| :data-pin-fx="pinFeedbackMode ?? undefined" | ||
| :data-hero-hidden="heroHidden ? 'true' : undefined" | ||
| :data-pin-state="pinState" | ||
| :data-session-region="region" | ||
| :data-session-id="session.id" | ||
| @click="emit('select', session)" | ||
| > |
There was a problem hiding this comment.
Missing data-pin-visual-state attribute binding.
The test file expects a data-pin-visual-state attribute on .session-item, but it's not bound in the template. Based on test expectations, this attribute should reflect a different state than data-pin-state (e.g., overlay during pinning animation even when pinState is docked).
Consider adding a computed property for pinVisualState and binding it:
🔧 Suggested addition
:data-pin-fx="pinFeedbackMode ?? undefined"
:data-pin-state="pinState"
+:data-pin-visual-state="pinVisualState"
:data-session-region="region"And add the corresponding computed property that returns 'overlay' during pinning feedback and 'docked' otherwise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/WindowSideBarSessionItem.vue` around lines 117 -
129, Add a new computed property named pinVisualState in the
WindowSideBarSessionItem component that returns 'overlay' when pinFeedbackMode
indicates a pinning animation/feedback (e.g., truthy or a specific mode) and
returns 'docked' otherwise, then bind it to the root div as
:data-pin-visual-state="pinVisualState" alongside the existing :data-pin-state
and :data-pin-fx attributes so tests expecting the separate visual state
(overlay vs docked) pass.
| .session-item[data-pin-state='docked'] .pin-button:hover .pin-button__icon { | ||
| transform: translateY(-1px) scale(1.06) 2s; | ||
| } |
There was a problem hiding this comment.
Invalid CSS syntax: stray 2s in transform value.
The transform property has an invalid value. scale(1.06) 2s is not valid CSS—the 2s appears to be a leftover from a transition definition.
🐛 Proposed fix
.session-item[data-pin-state='docked'] .pin-button:hover .pin-button__icon {
- transform: translateY(-1px) scale(1.06) 2s;
+ transform: translateY(-1px) scale(1.06);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .session-item[data-pin-state='docked'] .pin-button:hover .pin-button__icon { | |
| transform: translateY(-1px) scale(1.06) 2s; | |
| } | |
| .session-item[data-pin-state='docked'] .pin-button:hover .pin-button__icon { | |
| transform: translateY(-1px) scale(1.06); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/WindowSideBarSessionItem.vue` around lines 373 -
375, The CSS rule for .session-item[data-pin-state='docked'] .pin-button:hover
.pin-button__icon contains an invalid transform value ("scale(1.06) 2s"); remove
the stray "2s" from the transform and, if the intent was an animated effect, add
a separate transition property (e.g., transition: transform 0.2s ease) on
.session-item[data-pin-state='docked'] .pin-button__icon so hover only changes
the transform to translateY(-1px) scale(1.06) while timing is handled by
transition.
| expect(item.attributes('data-pin-state')).toBe('docked') | ||
| expect(item.attributes('data-pin-visual-state')).toBe('overlay') | ||
| }, 10000) | ||
|
|
||
| it('keeps the pin layout docked while unpinning feedback is active', async () => { | ||
| const wrapper = await mountComponent({ | ||
| isPinned: false, | ||
| pinFeedbackMode: 'unpinning' | ||
| }) | ||
|
|
||
| expect(wrapper.find('.session-item').attributes('data-pin-state')).toBe('docked') | ||
| expect(wrapper.find('.session-item').attributes('data-pin-visual-state')).toBe('docked') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if data-pin-visual-state is bound anywhere in the component
rg -n 'data-pin-visual-state' src/renderer/src/components/WindowSideBarSessionItem.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
#!/bin/bash
# First, check the test file content at lines 118-129
echo "=== Test file (lines 110-135) ==="
sed -n '110,135p' test/renderer/components/WindowSideBarSessionItem.test.ts
echo ""
echo "=== Component template content ==="
# Find the component file and check its template
find src -name "WindowSideBarSessionItem.vue" -exec cat {} \;Repository: ThinkInAIXYZ/deepchat
Length of output: 15561
Tests expect data-pin-visual-state attribute that is not rendered by the component.
The test assertions on lines 119 and 129 check for data-pin-visual-state attribute, but the component template only binds :data-pin-state="pinState" to the .session-item element. There is no data-pin-visual-state binding, so these assertions will fail.
Either add the missing :data-pin-visual-state binding to the component template, or update the test expectations to remove references to this non-existent attribute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/renderer/components/WindowSideBarSessionItem.test.ts` around lines 118 -
129, The test is asserting a non-existent attribute data-pin-visual-state on the
.session-item element; update the component template (the element that currently
binds :data-pin-state="pinState") to also bind
:data-pin-visual-state="pinVisualState" (or whatever computed/prop name you use
for visual state) so the attribute is rendered, or alternatively modify the
tests in WindowSideBarSessionItem.test.ts to stop asserting
data-pin-visual-state and only assert data-pin-state; locate bindings around the
.session-item in the component where :data-pin-state="pinState" is defined and
add the matching visual-state binding or adjust the test expectations
accordingly.
…tems
close #1455
Summary by CodeRabbit
Bug Fixes
UI/UX Improvements