fix(shell): remove login-shell overhead & queue in-flight icon index scans in AppLibrary - #6505
fix(shell): remove login-shell overhead & queue in-flight icon index scans in AppLibrary#6505shrijit37 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Reduces AppLibrary scan overhead and queues debounce-triggered icon rescans.
Changes:
- Replaces login shells with standard Bash shells.
- Adds pending icon-index rescan handling.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onTriggered: { | ||
| if (iconIndexScan.running) { | ||
| root.pendingIconIndexRescan = true | ||
| } else { | ||
| iconIndexScan.running = true | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13de8aec79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Process { | ||
| id: iconIndexScan | ||
| command: ["bash", "-lc", root.iconIndexScanCommand()] | ||
| command: ["bash", "-c", root.iconIndexScanCommand()] |
There was a problem hiding this comment.
When the session is started by UWSM, default/uwsm/env.d/10-omarchy explicitly notes that UWSM doesn't source /etc/profile.d; before this change the login shell did, so profile.d additions to XDG_DATA_DIRS (for example Flatpak export dirs) were visible to this scanner. With bash -c, iconIndexScanCommand() falls back to only /usr/local/share:/usr/share whenever the Quickshell process didn't already import those dirs, so app icons installed under those profile-provided data dirs stop being indexed after package/app changes; the adjacent hidden-entry scan has the same blind spot. Please keep the data-dir initialization explicit if you remove the login shell.
Useful? React with 👍 / 👎.
13de8ae to
ee0b0dc
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
shell/services/AppLibrary.qml:195
- Switching this scan to a non-login shell removes the
/etc/profile.d/flatpak.shinitialization that previously populatedXDG_DATA_DIRS. Omarchy's UWSM environment explicitly does not source/etc/profile.d(default/uwsm/env.d/10-omarchy:3), andhidden-entries.sh:97falls back to only/usr/local/share:/usr/share, so hidden/NoDisplay Flatpak entries can now be missed whenever the session did not already export this variable. Supply the Flatpak export directories in the fallback before invoking the script.
command: ["bash", "-c", root.hiddenEntryScanCommand()]
shell/services/AppLibrary.qml:132
- The old login shell picked up both system and per-user Flatpak export paths from Flatpak's profile setup. This replacement fallback restores only
/var/lib/flatpak/exports/share; whenXDG_DATA_DIRSis unset, icons for Flatpaks installed with--userunder~/.local/share/flatpak/exports/shareare no longer indexed. Include the per-user export directory in the fallback as well.
This issue also appears on line 195 of the same file.
'IFS=":"; for d in ${XDG_DATA_DIRS:-/usr/local/share:/usr/share:/var/lib/flatpak/exports/share}; do dirs="$dirs $d/icons"; done; unset IFS;',
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
shell/services/AppLibrary.qml:78
- The in-flight queue is the core race fix, but
test/shell.d/app-search-test.shcovers otherAppLibrary.qmlicon behavior without exercising this transition. Add a focused regression test where a refresh arrives during a running scan and verify that exactly one follow-up scan starts after exit; otherwise the dropped-rescan bug can be reintroduced unnoticed.
if (iconIndexScan.running) {
root.pendingIconIndexRescan = true
return
}
iconIndexScan.running = true
shell/services/AppLibrary.qml:131
- The user Flatpak icon directory is added explicitly here and is also supplied by the fallback on the next line (and normally by
XDG_DATA_DIRS). This makes each scan traverse the same Flatpak tree twice for both SVG and PNG files, adding avoidable work to the path this PR is optimizing. Let the XDG list/fallback provide this directory once.
'dirs="$HOME/.icons $HOME/.local/share/icons $HOME/.local/share/flatpak/exports/share/icons";',
shell/services/hidden-entries.sh:97
- This changes launcher discovery by injecting Flatpak exports when
XDG_DATA_DIRSis unset, but the PR description only scopes the change to removing login shells and queuing icon rescans. Because this can alter which desktop entry wins for duplicate IDs, either document and test the Flatpak discovery change as part of this PR or split it into a separate change.
IFS=":" read -ra data_dirs <<< "${XDG_DATA_DIRS:-/usr/local/share:/usr/share:/var/lib/flatpak/exports/share:$HOME/.local/share/flatpak/exports/share}"
…scans in AppLibrary - Use 'bash -c' instead of 'bash -lc' for hiddenEntryScan and iconIndexScan to eliminate profile startup latency. - Include Flatpak export directories (/var/lib/flatpak/exports/share and ~/.local/share/flatpak/exports/share) in XDG_DATA_DIRS fallback. - Queue concurrent icon index rescans in pendingIconIndexRescan and restart debounce on completion.
50e6d92 to
98a8cec
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
shell/services/AppLibrary.qml:78
- The in-flight rescan queue is the central regression fix, but the existing AppLibrary checks do not cover its state transitions. Add focused coverage for a refresh/debounce request during a running scan and verify that exit clears the pending flag and schedules exactly one follow-up scan; otherwise a future simplification can silently reintroduce the dropped-update bug.
if (iconIndexScan.running) {
root.pendingIconIndexRescan = true
return
}
iconIndexScan.running = true
shell/services/hidden-entries.sh:97
- The fallback reverses Flatpak's user/system precedence:
seen_idskeeps the first desktop ID, so a system-wide Flatpak entry is processed before a user installation of the same app and the user's visibility metadata is ignored. Put the user export first (and both Flatpak exports ahead of the standard system directories), matching the normal FlatpakXDG_DATA_DIRSordering.
IFS=":" read -ra data_dirs <<< "${XDG_DATA_DIRS:-/usr/local/share:/usr/share:/var/lib/flatpak/exports/share:$HOME/.local/share/flatpak/exports/share}"
shell/services/AppLibrary.qml:132
- This fallback scans
/usrand the system Flatpak export before the user Flatpak export, whileindexIconLine()deliberately keeps the first path for each icon name. A user-installed Flatpak therefore cannot override an identically named system icon. Preserve user-before-system XDG precedence in the fallback.
'IFS=":"; for d in ${XDG_DATA_DIRS:-/usr/local/share:/usr/share:/var/lib/flatpak/exports/share:$HOME/.local/share/flatpak/exports/share}; do dirs="$dirs $d/icons"; done; unset IFS;',
…scans The login-shell removal dropped /etc/profile.d/flatpak.sh, which populated XDG_DATA_DIRS with flatpak exports. The fallback now carries both export dirs with user-before-system precedence so user flatpaks still win over duplicate system ids and per-user icons are indexed even when XDG_DATA_DIRS is unset. Adds app-search regression coverage for the in-flight icon rescan queue and the flatpak fallback ordering.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/shell.d/app-search-test.sh:157
indexOf('local/share')finds the substring inside the first user Flatpak path, so this comparison is always0 < 7for the current fallback and does not verify that/usr/local/share(or/usr/share) follows the user export. Compare directory entries rather than the concatenated string so an ordering regression is detected.
fallbackDirs.join('').indexOf('$HOME/.local/share/flatpak/exports/share') < fallbackDirs.join('').indexOf('local/share'),
test/shell.d/app-search-test.sh:146
- This assertion is not scoped to
refreshIcons(): after matching the pending flag on line 75, the broad[\s\S]*?continues to the} else {andiconIndexScan.running = trueiniconIndexDebounce. Consequently, deleting the idle-start assignment fromrefreshIcons()would still pass this test. Capture the function body first so both branches are actually checked within that function.
This issue also appears on line 157 of the same file.
/function refreshIcons\(\) \{[\s\S]*?if \(iconIndexScan\.running\) \{[\s\S]*?root\.pendingIconIndexRescan = true[\s\S]*?\} else \{[\s\S]*?iconIndexScan\.running = true/.test(appLibraryQml),
…ot substring order
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/shell.d/app-search-test.sh:146
- This assertion is not scoped to
refreshIcons(): its} else { ... iconIndexScan.running = truematch comes fromiconIndexDebounceat lines 227–228. It therefore stays green even ifrefreshIcons()stops starting an idle scan. Capture the function body first so the regression test actually verifies both branches of the changed function.
/function refreshIcons\(\) \{[\s\S]*?if \(iconIndexScan\.running\) \{[\s\S]*?root\.pendingIconIndexRescan = true[\s\S]*?\} else \{[\s\S]*?iconIndexScan\.running = true/.test(appLibraryQml),
Summary
hiddenEntryScanandiconIndexScaninAppLibrary.qmlranbash -lclogin shells, incurring unnecessary startup profile overhead per desktop file scan.iconIndexDebouncedropped rescan requests ificonIndexScan.runningwastrue(if (!iconIndexScan.running) iconIndexScan.running = true), causing desktop entry or package changes occurring mid-scan to be lost.Fix
bash -cinstead ofbash -lc.pendingIconIndexRescanand restart the debounce timer oniconIndexScan.onExited.Review follow-up
Initial Copilot review flagged the Flatpak side effects of dropping the login shell. Updated branch:
XDG_DATA_DIRSfallback in bothAppLibrary.qmlandhidden-entries.shnow lists user flatpak exports before system flatpak and standard dirs, so a user install still wins over a duplicate system id (seen_idskeeps the first hit).XDG_DATA_DIRSis unset ($HOME/.local/share/flatpak/exports/shareincluded)./etc/profile.d/flatpak.shpopulated them previously, and UWSM does not source/etc/profile.d.test/shell.d/app-search-test.sh: in-flight icon rescan queues exactly one follow-up scan after exit, and the flatpak fallback ordering holds.Round 2 (post-re-review)
local/share, which the user dir itself contains — it could never fail. It now asserts array positions of the parsed fallback dirs, so user-flatpak must actually come before system-flatpak and the standard dirs.Testing: app-search-test.sh green · full
./test/shellgreen against the same single pre-existing baseline failure.