fix: prevent false positive auto-shrink when window is hidden#335
fix: prevent false positive auto-shrink when window is hidden#335karry-0803 wants to merge 9 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @karry-0803, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConversationPanel removes the ConversationToolbar and its wiring, replaces toolbar-centric emits with panel-level and kebab-case events, tightens scroll/visibility guards for missing or zero-height containers, exposes timeline scrollToBottom, forces getHistoryAnchor() -> null, and centralizes scroll-to-bottom visibility with a ResizeObserver-driven flow. ChangesConversationPanel toolbar removal and API refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue (1)
317-327:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
refreshScrollToBottomVisibility()now causes duplicatelatestContentVisibilityChangeemits.This helper now emits visibility itself, but callers still emit after invoking it, causing duplicated events and redundant downstream side effects. Keep emission in one place only.
Minimal fix (keep existing caller contracts)
function refreshScrollToBottomVisibility() { if (!conversationContainer.value) { showScrollToBottom.value = false; return; } const atBottom = isScrolledToBottom(conversationContainer.value); showScrollToBottom.value = hasScrollbar() && !atBottom; - - emitLatestContentVisibility(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue` around lines 317 - 327, The helper refreshScrollToBottomVisibility currently calls emitLatestContentVisibility(), causing duplicate latestContentVisibilityChange events because callers also emit after calling it; remove the internal emitLatestContentVisibility() call from refreshScrollToBottomVisibility so that only callers (who already emit) are responsible for emitting, keeping caller contracts unchanged; search for refreshScrollToBottomVisibility and emitLatestContentVisibility to locate and update the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/package.json`:
- Around line 59-64: Remove the runtime dependency entry for "`@types/dompurify`"
from package.json (either delete it or move it to devDependencies) because
DOMPurify 3.x ships its own types, and update the pinned "dompurify": "3.4.2"
entry to a current 3.x patch (e.g. "^3.4.7" or "^3.4.x") to pick up downstream
fixes; ensure you update package.json entries for "`@types/dompurify`" and
"dompurify" accordingly and run the package manager install to update lockfiles.
---
Outside diff comments:
In `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue`:
- Around line 317-327: The helper refreshScrollToBottomVisibility currently
calls emitLatestContentVisibility(), causing duplicate
latestContentVisibilityChange events because callers also emit after calling it;
remove the internal emitLatestContentVisibility() call from
refreshScrollToBottomVisibility so that only callers (who already emit) are
responsible for emitting, keeping caller contracts unchanged; search for
refreshScrollToBottomVisibility and emitLatestContentVisibility to locate and
update the function.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fe871742-163b-4d01-bd24-a2abc51c8a56
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/desktop/package.jsonapps/desktop/src/views/SearchView/components/ConversationPanel/index.vue
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Rust Checks
- GitHub Check: Frontend Tests
- GitHub Check: Frontend Quality
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (rust)
- GitHub Check: Desktop E2E Smoke (Windows)
🔇 Additional comments (1)
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue (1)
196-200: LGTM!Also applies to: 229-235
karry-0803
force-pushed
the
fix/auto-shrink-hidden-state
branch
from
4052827 to
0be00bb
Compare
…nd fix code corruption
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue`:
- Line 27: The template has a malformed event binding on the MessageItem
component: replace the broken fragment '`@regenerate`=handleRegenerateMessage as
any)"' with a proper Vue event binding such as
`@regenerate`="handleRegenerateMessage" (or `@regenerate`="(e) =>
handleRegenerateMessage(e)" if you need the event), keeping the
:message="message" prop; fix the syntax around the MessageItem usage so the
component tag compiles (reference: MessageItem and handleRegenerateMessage).
- Around line 276-279: The computed definition for outputScrollBehavior is
broken by a misplaced line break and an extra "l);" token; fix the computed call
by declaring outputScrollBehavior = computed(() => (useSettingsStore() as
any).preferences.behavior.outputScroll) so the arrow function and parentheses
are correctly paired and remove the stray "l);". Locate the outputScrollBehavior
declaration and correct the syntax around computed, the arrow function, and the
closing parentheses so the expression is a single well-formed computed call.
- Around line 237-245: The function handleTimelineJump contains a broken
optional-chained tagged template: the querySelector call is split across lines
producing an invalid tagged template expression; fix it by calling
conversationContainer.value?.querySelector with a single template-string
argument (e.g.
conversationContainer.value?.querySelector(`[data-message-id="${messageId}"]`)
), removing the stray backticks and stray parentheses so the element lookup and
subsequent element.scrollIntoView and markUserScrollIntent() operate on the
properly retrieved element.
- Around line 58-71: The import statements in ConversationPanel (references:
AppIcon, computed, nextTick, onMounted, onUnmounted, ref, useSettingsStore,
SessionMessage, ConversationTimeline, MessageItem) are malformed (missing the
leading "i" on "import", split keywords and extra commas), causing parse
failures; fix by restoring each to a single, valid ES import line (e.g., "import
..." for AppIcon and components, and a single import from 'vue' that lists
computed,nextTick,onMounted,onUnmounted,ref without duplicate commas or line
breaks, plus correct imports for useSettingsStore and SessionMessage) so all
symbols are imported with proper syntax.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 21f64743-a379-43d4-a680-9715134351aa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (rust)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
- GitHub Check: Desktop E2E Smoke (Windows)
🧰 Additional context used
🪛 Biome (2.4.15)
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue
[error] 58-58: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 58-58: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 58-58: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 60-60: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 60-60: Expected an expression but instead found ','.
(parse)
[error] 60-60: Expected an expression but instead found ','.
(parse)
[error] 60-60: Expected an expression but instead found ','.
(parse)
[error] 60-60: Expected an expression but instead found ','.
(parse)
[error] 60-60: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 65-65: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 65-65: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 65-65: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 70-70: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 70-70: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 70-70: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 238-239: Tagged template expressions are not permitted in an optional chain.
(parse)
[error] 240-335: unterminated template literal
(parse)
[error] 335-335: expected } but instead the file ends
(parse)
🔇 Additional comments (3)
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue (3)
119-140: The visibility guard logic correctly addresses the PR objective.The defensive checks for
clientHeight === 0inemitLatestContentVisibility()(line 122) andisScrolledToBottom()(line 134) properly prevent false positives when the window is hidden. When the container is minimized or sent to tray,clientHeightbecomes0, and these guards ensure the read marker state is preserved rather than incorrectly updated.LGTM!
255-257: Verify that all call sites ofConversationPanel’sgetHistoryAnchor()safely handle thenullreturn.
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vuenow exposesgetHistoryAnchor()that returnsnullunconditionally (lines 255-257). Callers must guard fornulland provide an appropriate fallback before using the returned element.
281-283: Potential accessibility regression:ConversationPanelexposedfocus()only emitsfocus(may no longer move DOM focus).
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue(281-283) hasfunction focus() { emit('focus'); }, butapps/desktop/src/views/SearchView/index.vueandapps/desktop/src/views/SearchView/composables/useSearchPage.tscallconversationPanel.value?.focus();. Ensure the call chain actually results in focus moving to the intended element (e.g., via@focus/v-on:focushandlers that set DOM focus, or by keeping the exposedfocus()implementation focused onconversationContainer).
karry-0803
force-pushed
the
fix/auto-shrink-hidden-state
branch
from
ef0024c to
9f30677
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue (3)
108-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the existing
ConversationPanelHandlecontract.
apps/desktop/src/views/SearchView/types.ts:62-68still requiresisLatestContentVisible(), and the handle is still modeled as imperative. Dropping that method fromdefineExpose()and turningfocus()into an event emit means ref-based callers can getundefinedor a silent no-op.💡 Minimal compatibility fix
function focus() { - emit('focus'); + conversationContainer.value?.focus(); } // 暴露方法给父组件 defineExpose({ focus, revealLatestContent, scrollByDelta, getHistoryAnchor, + isLatestContentVisible, });Also applies to: 273-275, 321-326
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue` around lines 108 - 111, The ConversationPanel must preserve the imperative ConversationPanelHandle contract: re-add isLatestContentVisible() to the object passed to defineExpose() (implement using the existing isLatestContentVisible function) and keep focus() exposed as a method rather than replacing it with an event emit so ref-based callers continue to work; update every defineExpose call in this component (including the other occurrences around the focus-related changes you modified at the other spots) to return an object matching ConversationPanelHandle with isLatestContentVisible and focus methods.
32-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBind
timelineRefin the template.
revealLatestContent()relies ontimelineRef.value?.scrollToBottom()when the container is already at the bottom, but<ConversationTimeline>never assigns that ref. That makes the parent’s post-openrevealLatestContent()path a silent no-op.💡 Suggested fix
<ConversationTimeline + ref="timelineRef" :messages="messages" :container-height="maxHeight" :scroll-top="scrollTop" :scroll-height="scrollHeight" :client-height="clientHeight"Also applies to: 254-256, 277-288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue` around lines 32 - 39, The ConversationTimeline component is not bound to the parent timelineRef, so revealLatestContent() cannot call timelineRef.value?.scrollToBottom(); fix by adding the template ref binding to the ConversationTimeline elements (e.g., add ref="timelineRef" on the <ConversationTimeline .../> instances) so the timelineRef defined in setup is assigned and revealLatestContent, timelineRef.value.scrollToBottom(), and related handlers (revealLatestContent, handleTimelineJump) work correctly; apply the same ref binding to the other ConversationTimeline usages mentioned.
90-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the emitted event API compatible with the current parent wiring.
SearchViewstill listens forpin-change,maximize-toggle,new-session,history-open-change,history-prefetch,drag-start, anddrag-endinapps/desktop/src/views/SearchView/index.vue:1296-1314. Replacing the contract here without migrating that parent in the same change drops those interactions end-to-end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue` around lines 90 - 99, The component replaced its emitted event contract (defined in defineEmits) and broke parent wiring; restore backward compatibility by adding the legacy event names back (or emit both new and legacy names) so SearchView continues to receive pin-change, maximize-toggle, new-session, history-open-change, history-prefetch, drag-start, and drag-end; update the defineEmits declaration to include these string event signatures alongside the new ones (or ensure that methods like togglePin, toggleMaximize, clearSession, latestContentVisibilityChange also emit the legacy event names) so existing parent listeners in SearchView keep working.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue`:
- Around line 108-111: The ConversationPanel must preserve the imperative
ConversationPanelHandle contract: re-add isLatestContentVisible() to the object
passed to defineExpose() (implement using the existing isLatestContentVisible
function) and keep focus() exposed as a method rather than replacing it with an
event emit so ref-based callers continue to work; update every defineExpose call
in this component (including the other occurrences around the focus-related
changes you modified at the other spots) to return an object matching
ConversationPanelHandle with isLatestContentVisible and focus methods.
- Around line 32-39: The ConversationTimeline component is not bound to the
parent timelineRef, so revealLatestContent() cannot call
timelineRef.value?.scrollToBottom(); fix by adding the template ref binding to
the ConversationTimeline elements (e.g., add ref="timelineRef" on the
<ConversationTimeline .../> instances) so the timelineRef defined in setup is
assigned and revealLatestContent, timelineRef.value.scrollToBottom(), and
related handlers (revealLatestContent, handleTimelineJump) work correctly; apply
the same ref binding to the other ConversationTimeline usages mentioned.
- Around line 90-99: The component replaced its emitted event contract (defined
in defineEmits) and broke parent wiring; restore backward compatibility by
adding the legacy event names back (or emit both new and legacy names) so
SearchView continues to receive pin-change, maximize-toggle, new-session,
history-open-change, history-prefetch, drag-start, and drag-end; update the
defineEmits declaration to include these string event signatures alongside the
new ones (or ensure that methods like togglePin, toggleMaximize, clearSession,
latestContentVisibilityChange also emit the legacy event names) so existing
parent listeners in SearchView keep working.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1b06d70e-7dde-4da9-acf7-ef1390b24921
📒 Files selected for processing (1)
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CodeQL (rust)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
- GitHub Check: Frontend Quality
- GitHub Check: Desktop E2E Smoke (Windows)
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
1 participant
Summary
Fixes a bug where the window would incorrectly auto-shrink even if the user hadn't read the latest message before hiding the window.
When the window is minimized or sent to the tray, its clientHeight becomes 0. The isScrolledToBottom logic (scrollHeight - scrollTop - clientHeight < 5) would then produce a false positive, incorrectly marking the unread message as "seen" while the window was hidden. This PR adds defensive checks to ensure clientHeight > 0 before updating the latestContentVisibility state, and completely blocks visibility updates when the container is hidden, ensuring the read marker is strictly bound to actual visual exposure.
Related issue or RFC
Follow-up fix for [Feature]: adjust window auto-shrink timing based on session state, read state, and inactivity #245
AI assistance disclosure
Testing evidence
Tested locally: Reopening the window from the tray after a 5+ minute timeout while the latest message is unread correctly keeps the window expanded.
pnpm --filter @touchai/desktop test:unitpassed locally.Relying on CI for
pnpm test:prand full suite confirmation.Did you follow TDD (test-first) for feature and fix work? Strongly recommended.
No, this was a targeted bug fix for a view-layer calculation edge case.
Risk notes
Screenshots or recordings
Reopened from tray after 5+ minutes. The window correctly remains expanded because the scrollbar was not at the bottom before hiding.
Checklist
pnpm test:prfor this code PR, or this is a docs-only change.pnpm test:coverage:rustor relied on CI coverage evidence.pnpm test:e2elocally or documented why CI is the first valid proof. (Relying on CI for E2E since this is a pure UI state fix and CI provides an isolated environment).