fix(plugins): keep updated plugin marked Installed instead of reoffering Update#1626
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e78cefbae
ℹ️ 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".
| let info = bundle.infoDictionary ?? [:] | ||
| let version: String | ||
| if let declared = info["CFBundleShortVersionString"] as? String { | ||
| if let declared = Self.bundleShortVersion(at: url) { |
There was a problem hiding this comment.
Don’t mark a cached bundle as the new version
When updating a plugin that has already been discovered or loaded at this same URL, loadPluginAsync still obtains and loads the Bundle before replaceExistingPlugin unloads the existing entry, so the bundle passed here can be the cached CFBundle object from the previous install. Reading only CFBundleShortVersionString from the replaced Info.plist then creates a PluginEntry that advertises the registry version while its principal class/capabilities can still come from the cached old bundle, causing registryUpdate(for:) to stop offering the update even though the running plugin code was not refreshed. Please avoid mixing the disk version with a cached Bundle (or unload/avoid the cached bundle before loading the replacement).
Useful? React with 👍 / 👎.
Problem
After updating a plugin in Settings, the row shows the green Installed state, then a few seconds later (often noticed when you click to another plugin and back) it shows the Update button again. The plugin never settles into a stable Installed state.
Root cause
The revert is not triggered by navigation, it is a stale version read plus a timer.
PluginInstaller.atomicReplaceswaps the plugin bundle on disk at the same URL.PluginManager.registerBundlethen read the new version frombundle.infoDictionary["CFBundleShortVersionString"]. ButBundle(url:)returns a process-cachedCFBundlewhose info dictionary was populated at launch-time discovery with the oldInfo.plist, andCFBundlenever re-reads it. So the newPluginEntry.versionequalled the old version andhasUpdate(for:)stayed true. On success the install tracker shows a green checkmark for 3 seconds, then its timer clears the tracker entry, and becausehasUpdatewas still true the row fell back to the Update button.Changes
PluginManager: readCFBundleShortVersionStringstraight fromContents/Info.pliston disk via a newbundleShortVersion(at:), bypassing the staleCFBundlecache.hasUpdatenow turns false after an update completes.PluginManager+Install: drop the duplicatecompleteInstallinupdateFromRegistry(the coordinator already calls it; the reconciliation path never opened a tracker entry). Stops two 3s timers racing.BrowsePluginsView: restructurerowStatusBadgeso the tracker phase is one switch checked first (was duplicated and nested underhasUpdate, making the success flash unreachable once the version was correct).hasUpdate(for:)now delegates to the canonicalPluginManager.registryUpdate(for:), which adds the user-installed and non-theme guards the local copy lacked.Tests
New
PluginBundleVersionTestsseeds theCFBundlecache, rewritesInfo.plistin place, then assertsbundleShortVersion(at:)returns the new version whileBundle(url:).infoDictionarystill returns the stale one, covering both the bug and the fix.No UI test: the install/update flow needs a live registry download and is not deterministic.