[email protected]: fix persistent data #2547
Conversation
…files created by the app
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All changes look good. Wait for review from human collaborators. everything-alpha
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bucket/everything-alpha.json(3 hunks)
🔇 Additional comments (2)
bucket/everything-alpha.json (2)
23-23: Verify NO_ALPHA_INSTANCE file persistence.The PR objectives mention that users create a
NO_ALPHA_INSTANCEempty file to disable the alpha instance. However, the file patterns on lines 23 and 47 ('*.csv', '*.lng', '*.db', '*.ini', 'Session*.json') won't match this filename.Confirm whether this file should be explicitly added to the copy patterns (e.g.,
'NO_ALPHA_INSTANCE') or if it's stored in a location already covered by the plugins directory persistence.Also applies to: 47-47
17-48: Plugin directory approach looks sound.The separation of plugin persistence from session/config data is well-organized, and ensuring directories exist before operations is defensive. Once the
-Recurseflags are added, the persistence strategy should be robust.
| "if (!(Test-Path \"$persist_dir\\Everything*.ini\")) { Invoke-ExternalCommand \"$dir\\Everything.exe\" -Args @('-install-config', 'null') | Out-Null }", | ||
| "Get-ChildItem \"$persist_dir\\*\" -Include 'Bookmarks*.csv', 'Everything.lng', 'Everything*.db', 'Everything*.ini', 'Filters*.csv', 'Plugins*.ini', 'Run History*.csv', 'Search History*.csv' | Copy-Item -Destination \"$dir\" -ErrorAction SilentlyContinue", | ||
| "Get-ChildItem \"$persist_dir\\*\" -Include '*.csv', '*.lng', '*.db', '*.ini', 'Session*.json' | Copy-Item -Destination \"$dir\" -ErrorAction SilentlyContinue", | ||
| "Get-ChildItem \"$persist_dir\\plugins\\*\" | Copy-Item -Destination \"$dir\\plugins\" -ErrorAction SilentlyContinue -Force", |
There was a problem hiding this comment.
Add -Recurse flag to preserve nested plugin directory structures.
The Copy-Item command without -Recurse won't copy subdirectories and their contents. Since plugins typically organize files in nested directories, this will lose plugin structure during install.
Apply this diff:
- "Get-ChildItem \"$persist_dir\\plugins\\*\" | Copy-Item -Destination \"$dir\\plugins\" -ErrorAction SilentlyContinue -Force",
+ "Get-ChildItem \"$persist_dir\\plugins\\*\" | Copy-Item -Destination \"$dir\\plugins\" -ErrorAction SilentlyContinue -Force -Recurse",📝 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.
| "Get-ChildItem \"$persist_dir\\plugins\\*\" | Copy-Item -Destination \"$dir\\plugins\" -ErrorAction SilentlyContinue -Force", | |
| "Get-ChildItem \"$persist_dir\\plugins\\*\" | Copy-Item -Destination \"$dir\\plugins\" -ErrorAction SilentlyContinue -Force -Recurse", |
🤖 Prompt for AI Agents
In bucket/everything-alpha.json around line 24, the Copy-Item call used to copy
plugins omits the -Recurse flag so subdirectories and their contents won’t be
copied; update the command to include -Recurse (i.e., add the -Recurse option to
the Copy-Item invocation) so nested plugin directory structures are preserved
during install.
There was a problem hiding this comment.
The existing plugins are all single DLLs for now.
There was a problem hiding this comment.
@SiriosDev, thank you for clarifying! If the plugins are currently single DLL files without subdirectories, then the current implementation is sufficient. The -Recurse flag would only be needed if plugins start using nested directory structures in the future. Your approach works well for the current use case.
/\_/\
( o.o )
> ^ < *noted!*
✏️ Learnings added
Learnt from: SiriosDev
Repo: ScoopInstaller/Versions PR: 2547
File: bucket/everything-alpha.json:24-24
Timestamp: 2025-10-31T11:53:16.842Z
Learning: In the Everything application (everything-alpha package), plugins are currently single DLL files stored in the `plugins` folder without nested subdirectories, so `-Recurse` flag is not needed when copying plugin files.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| "}", | ||
| "if ($cmd -eq 'uninstall') { reg import \"$dir\\uninstall-context.reg\" }", | ||
| "Get-ChildItem \"$dir\\*\" -Include 'Bookmarks*.csv', 'Everything.lng', 'Everything*.db', 'Everything*.ini', 'Filters*.csv', 'Plugins*.ini', 'Run History*.csv', 'Search History*.csv' | Copy-Item -Destination \"$persist_dir\" -ErrorAction SilentlyContinue -Force" | ||
| "Get-ChildItem \"$dir\\plugins\\*\" | Copy-Item -Destination \"$persist_dir\\plugins\" -ErrorAction SilentlyContinue -Force", |
There was a problem hiding this comment.
Add -Recurse flag to preserve nested plugin directory structures during uninstall.
Same issue as line 24: plugin subdirectories won't be copied back to persist_dir without the -Recurse flag.
Apply this diff:
- "Get-ChildItem \"$dir\\plugins\\*\" | Copy-Item -Destination \"$persist_dir\\plugins\" -ErrorAction SilentlyContinue -Force",
+ "Get-ChildItem \"$dir\\plugins\\*\" | Copy-Item -Destination \"$persist_dir\\plugins\" -ErrorAction SilentlyContinue -Force -Recurse",📝 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.
| "Get-ChildItem \"$dir\\plugins\\*\" | Copy-Item -Destination \"$persist_dir\\plugins\" -ErrorAction SilentlyContinue -Force", | |
| "Get-ChildItem \"$dir\\plugins\\*\" | Copy-Item -Destination \"$persist_dir\\plugins\" -ErrorAction SilentlyContinue -Force -Recurse", |
🤖 Prompt for AI Agents
In bucket/everything-alpha.json around line 46, the PowerShell Copy-Item
invocation used during uninstall is missing the -Recurse flag so nested plugin
subdirectories are not preserved; update the command to include -Recurse (i.e.,
add the -Recurse parameter to Copy-Item) so that plugin subdirectories are
copied back into "$persist_dir\plugins" recursively, keeping ErrorAction
SilentlyContinue and -Force as-is.
There was a problem hiding this comment.
The existing plugins are all single DLLs for now.
There was a problem hiding this comment.
@SiriosDev, thank you for the clarification! If all current plugins are single DLL files without nested subdirectories, then the current implementation without -Recurse will work correctly for the existing use case.
The suggestion was meant as a future-proofing measure in case plugins ever include subdirectories, but it's perfectly fine to keep it as-is given the current plugin architecture. Your implementation handles the current requirements appropriately.
1 participant
Fixed installed Plugin Persistence
Fixed additional file persistence (using broader pattern for
csv,db,lngandinifilesCloses [Bug]: Improve persist of everything-alpha #1520
<manifest-name[@version]|chore>: <general summary of the pull request>Summary by CodeRabbit