fix(search): keep requested pin state#289
Open
jiang171 wants to merge 1 commit into
Open
Conversation
Reviewer's GuideAdjusts the search window pin composable to track and honor the user’s requested pinned state instead of relying solely on the native window’s reported always-on-top flag, and adds targeted regression tests for this behavior. Sequence diagram for search window pinning with desired state trackingsequenceDiagram
actor User
participant SearchView
participant useSearchWindowPin
participant currentWindow
User->>SearchView: clickPinButton
SearchView->>useSearchWindowPin: toggleWindowPin()
useSearchWindowPin->>useSearchWindowPin: queuePinOperation(operation)
useSearchWindowPin->>useSearchWindowPin: desiredPinnedState = !isPinned.value
useSearchWindowPin->>currentWindow: setAlwaysOnTop(desiredPinnedState)
useSearchWindowPin-->>SearchView: return desiredPinnedState (isPinned updated)
rect rgb(235, 235, 235)
SearchView->>useSearchWindowPin: syncWindowPinState()
useSearchWindowPin->>useSearchWindowPin: queuePinOperation(operation)
alt desiredPinnedState is not null
useSearchWindowPin-->>SearchView: return desiredPinnedState (no native check)
else desiredPinnedState is null
useSearchWindowPin->>currentWindow: isAlwaysOnTop()
currentWindow-->>useSearchWindowPin: nextState
useSearchWindowPin-->>SearchView: return nextState
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughCaches the user's requested pinned/always-on-top state in ChangesWindow pin state consistency on Linux/Wayland
Suggested labels
Suggested reviewers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Because
desiredPinnedStateshort-circuitssyncWindowPinState, any external/native changes after the first user interaction will be ignored; consider clearing or expiringdesiredPinnedStateafter a successful sync or after some condition so later syncs can reflect native state again. toggleWindowPinnow relies solely on the localisPinnedref, which defaults tofalse; ifsyncWindowPinStatehasn’t been run yet, the first toggle may invert the wrong starting state—consider either initializing fromisAlwaysOnTop()on composable creation or havingtoggleWindowPinfall back to the native state whendesiredPinnedState/isPinnedis still in its initial state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Because `desiredPinnedState` short-circuits `syncWindowPinState`, any external/native changes after the first user interaction will be ignored; consider clearing or expiring `desiredPinnedState` after a successful sync or after some condition so later syncs can reflect native state again.
- `toggleWindowPin` now relies solely on the local `isPinned` ref, which defaults to `false`; if `syncWindowPinState` hasn’t been run yet, the first toggle may invert the wrong starting state—consider either initializing from `isAlwaysOnTop()` on composable creation or having `toggleWindowPin` fall back to the native state when `desiredPinnedState`/`isPinned` is still in its initial state.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Collaborator
|
Please fix the following conflict. |
jiang171
force-pushed
the
codex/fix-window-pin-toggle
branch
from
a827a5f to
fcc1b7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/composables/useSearchPage.ts`:
- Around line 65-80: setWindowPinned and toggleWindowPin set desiredPinnedState
before awaiting currentWindow.setAlwaysOnTop, so if the native call rejects the
cached desiredPinnedState will be wrong and syncWindowPinState will keep
replaying it; fix by capturing the previousPinned value at start of each
function, wrap the await currentWindow.setAlwaysOnTop(...) in a try/catch, and
on failure restore desiredPinnedState (and do not update isPinned) to the
previous value before rethrowing/returning the error; modify queuePinOperation
closures in setWindowPinned and toggleWindowPin to perform this rollback so the
cached state and UI stay consistent with the real window state.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 16c126e7-7309-46cb-844f-1462edfe5a9c
📒 Files selected for processing (2)
apps/desktop/src/views/SearchView/composables/useSearchPage.tsapps/desktop/tests/composables/SearchView/useSearchPage.test.ts
📜 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). (4)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: CodeQL (rust)
🧰 Additional context used
🪛 ast-grep (0.42.3)
apps/desktop/tests/composables/SearchView/useSearchPage.test.ts
[warning] 292-292: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
Comment on lines
65
to
80
| function setWindowPinned(value: boolean): Promise<boolean> { | ||
| return queuePinOperation(async () => { | ||
| desiredPinnedState = value; | ||
| await currentWindow.setAlwaysOnTop(value); | ||
| const nextState = await currentWindow.isAlwaysOnTop(); | ||
| isPinned.value = nextState; | ||
| return nextState; | ||
| isPinned.value = value; | ||
| return value; | ||
| }); | ||
| } | ||
|
|
||
| function toggleWindowPin(): Promise<boolean> { | ||
| return queuePinOperation(async () => { | ||
| const currentState = await currentWindow.isAlwaysOnTop(); | ||
| await currentWindow.setAlwaysOnTop(!currentState); | ||
| const nextState = await currentWindow.isAlwaysOnTop(); | ||
| const nextState = !isPinned.value; | ||
| desiredPinnedState = nextState; | ||
| await currentWindow.setAlwaysOnTop(nextState); | ||
| isPinned.value = nextState; | ||
| return nextState; |
There was a problem hiding this comment.
Rollback the cached pin state if the native request fails.
desiredPinnedState is updated before the awaited pin/unpin call completes. If that call rejects, later syncWindowPinState() calls will keep replaying the cached value and stop consulting the real window state, so the pin button and blur-hide policy can stay flipped even though the OS never applied the change.
Suggested fix
function setWindowPinned(value: boolean): Promise<boolean> {
return queuePinOperation(async () => {
+ const previousDesiredPinnedState = desiredPinnedState;
desiredPinnedState = value;
- await currentWindow.setAlwaysOnTop(value);
- isPinned.value = value;
- return value;
+ try {
+ await currentWindow.setAlwaysOnTop(value);
+ isPinned.value = value;
+ return value;
+ } catch (error) {
+ desiredPinnedState = previousDesiredPinnedState;
+ throw error;
+ }
});
}
function toggleWindowPin(): Promise<boolean> {
return queuePinOperation(async () => {
const nextState = !isPinned.value;
+ const previousDesiredPinnedState = desiredPinnedState;
desiredPinnedState = nextState;
- await currentWindow.setAlwaysOnTop(nextState);
- isPinned.value = nextState;
- return nextState;
+ try {
+ await currentWindow.setAlwaysOnTop(nextState);
+ isPinned.value = nextState;
+ return nextState;
+ } catch (error) {
+ desiredPinnedState = previousDesiredPinnedState;
+ throw error;
+ }
});
}📝 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.
Suggested change
| function setWindowPinned(value: boolean): Promise<boolean> { | |
| return queuePinOperation(async () => { | |
| desiredPinnedState = value; | |
| await currentWindow.setAlwaysOnTop(value); | |
| const nextState = await currentWindow.isAlwaysOnTop(); | |
| isPinned.value = nextState; | |
| return nextState; | |
| isPinned.value = value; | |
| return value; | |
| }); | |
| } | |
| function toggleWindowPin(): Promise<boolean> { | |
| return queuePinOperation(async () => { | |
| const currentState = await currentWindow.isAlwaysOnTop(); | |
| await currentWindow.setAlwaysOnTop(!currentState); | |
| const nextState = await currentWindow.isAlwaysOnTop(); | |
| const nextState = !isPinned.value; | |
| desiredPinnedState = nextState; | |
| await currentWindow.setAlwaysOnTop(nextState); | |
| isPinned.value = nextState; | |
| return nextState; | |
| function setWindowPinned(value: boolean): Promise<boolean> { | |
| return queuePinOperation(async () => { | |
| const previousDesiredPinnedState = desiredPinnedState; | |
| desiredPinnedState = value; | |
| try { | |
| await currentWindow.setAlwaysOnTop(value); | |
| isPinned.value = value; | |
| return value; | |
| } catch (error) { | |
| desiredPinnedState = previousDesiredPinnedState; | |
| throw error; | |
| } | |
| }); | |
| } | |
| function toggleWindowPin(): Promise<boolean> { | |
| return queuePinOperation(async () => { | |
| const nextState = !isPinned.value; | |
| const previousDesiredPinnedState = desiredPinnedState; | |
| desiredPinnedState = nextState; | |
| try { | |
| await currentWindow.setAlwaysOnTop(nextState); | |
| isPinned.value = nextState; | |
| return nextState; | |
| } catch (error) { | |
| desiredPinnedState = previousDesiredPinnedState; | |
| throw error; | |
| } | |
| }); | |
| } |
🤖 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/composables/useSearchPage.ts` around lines
65 - 80, setWindowPinned and toggleWindowPin set desiredPinnedState before
awaiting currentWindow.setAlwaysOnTop, so if the native call rejects the cached
desiredPinnedState will be wrong and syncWindowPinState will keep replaying it;
fix by capturing the previousPinned value at start of each function, wrap the
await currentWindow.setAlwaysOnTop(...) in a try/catch, and on failure restore
desiredPinnedState (and do not update isPinned) to the previous value before
rethrowing/returning the error; modify queuePinOperation closures in
setWindowPinned and toggleWindowPin to perform this rollback so the cached state
and UI stay consistent with the real window state.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
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
2 participants
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.
Summary
Fixes #287.
Keep the requested search-window pinned state after the user toggles the pin button. This prevents Linux/Wayland from immediately reverting the UI state when
isAlwaysOnTop()reportsfalseeven aftersetAlwaysOnTop(true)was requested.Changes:
useSearchWindowPin().setAlwaysOnTop()call.isAlwaysOnTop()asfalseafter a pin request.Related issue or RFC
AI assistance disclosure
Testing evidence
Local checks:
Observed results:
Manual verification:
.debfrom the fix branch through GitHub Actions on my forkNotes:
pnpm test:prlocally due to runtime cost; this PR includes a focused regression test and relies on repository CI for the full suite.Risk notes
AgentService, runtime, MCP, or schema impact: noneScreenshots or recordings
Manual Ubuntu/GNOME verification was performed locally after installing the generated
.deb.Checklist
[WIP]or similar title prefixes.AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC.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.Summary by Sourcery
Ensure the search window pin state is driven by the user’s requested state instead of relying solely on the native always-on-top result.
Bug Fixes:
Tests: