feat(desktop): enhance Windows app resolution and UI loading states#13320
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found |
| createEffect(() => { | ||
| const value = prefs.app | ||
| if (options().some((o) => o.id === value)) return | ||
| setPrefs("app", options()[0]?.id ?? "finder") | ||
| }) |
There was a problem hiding this comment.
gonna leave this for now but for future reference i'd handle something like this at read-time rather than writing to a signal in an effect
const [_prefs, setPrefs] = persisted(...);
const prefs = mergeProps(_prefs, {
get app() {
if (options().some((o) => o.id === _prefs.app)) return _prefs.app;
return options()[0]?.id ?? "finder"
}
});
Brendonovich
left a comment
Brendonovich
left a comment
There was a problem hiding this comment.
sorry for all the solid nits, wanted to use this for future reference for writing more idiomatic solid in future
There was a problem hiding this comment.
Pull request overview
This PR enhances Windows app resolution and adds UI loading states for the desktop application. It addresses a regression from PR #13084 where terminal windows were appearing and causing system freezes (issue #13154) by adding the CREATE_NO_WINDOW flag to prevent console windows from flashing when running system commands.
Changes:
- Significantly improved Windows app resolution logic with registry checking, environment variable expansion, and better path handling
- Added CREATE_NO_WINDOW flag (0x08000000) to Command spawns to prevent terminal window flashing
- Implemented loading state UI with spinner and disabled buttons to prevent double-clicking when opening apps
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/desktop/src-tauri/src/lib.rs | Complete rewrite of Windows app resolution with registry queries, environment variable expansion, candidate generation strategies, and CREATE_NO_WINDOW flags for spawned processes |
| packages/app/src/components/session/session-header.tsx | Added loading state management with openRequest store, spinner UI, disabled button states, and import reorganization for better structure |
Comments suppressed due to low confidence (1)
packages/desktop/src-tauri/src/lib.rs:405
- The path resolution logic in resolve_cmd has a potential path traversal security issue. While the code handles ".." by popping from the path (line 401), it does not validate that the final resolved path stays within expected boundaries. A malicious batch file could use "../../../" sequences to escape to arbitrary directories on the system. Consider validating that the resolved path remains within safe boundaries or checking against an allowlist of acceptable base directories.
for part in suffix.replace('/', "\\").split('\\') {
if part.is_empty() || part == "." {
continue;
}
if part == ".." {
let _ = resolved.pop();
continue;
}
resolved.push(part);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nomalyco#13320) Co-authored-by: Brendan Allan <git@brendonovich.dev> Co-authored-by: Brendan Allan <brendonovich@outlook.com>
2 participants
What does this PR do?
#13084 same as here but with fixed regression. based on link
If you paste a large clearly AI generated description here your PR may be IGNORED or CLOSED!
How did you verify your code works?