fix: avoid creating redundant snippet/ dir when snippets/ already exists + path tracking refactor#34
Open
bew wants to merge 1 commit into
Open
fix: avoid creating redundant snippet/ dir when snippets/ already exists + path tracking refactor#34bew wants to merge 1 commit into
bew wants to merge 1 commit into
Conversation
When the alt `snippets/` directory already contained a config, the old code always wrote a new `snippet/` dir and config there — because paths were hardcoded to the preferred dir regardless of what existed on disk. Fix by computing the active snippets dir once at startup via `resolveSnippetDir`: uses alt only when it exists and preferred does not, falls back to preferred in all other cases. The same resolution applies to project-scoped snippets — both `snippet/` and `snippets/` are now scanned, with the active dir chosen by the same logic. The fix is supported by a broader path-tracking refactor: - Add `SnippetPaths` interface with unified fields for global/project-scoped paths. - Isolate the dir selection logic in easily testable pure function: `resolveSnippetDir(preferred, alt)` - Replace `PATHS` export with `GLOBAL_PATHS` (computed once at startup) - Update all consumers (`config.ts`, `loader.ts`, `logger.ts`, `pending-drafts.ts`, `reload-signal.ts`, `commands.ts`) - Refactor tests to override `GLOBAL_PATHS` fields directly; add `withGlobalDir` helper in integration tests - Add `src/constants.test.ts` covering all four `resolveSnippetDir` cases
Author
|
Hello @JosXa , friendly ping 🙃 Would it be possible to get a review and potentially merge this PR ? |
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
1 participant
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.
Hello! I'm trying your plugin and really liking it!
If you already have a
snippets/directory (with a config inside or not), the plugin creates a brand newsnippet/directory and drops a fresh config there on every startup, completely ignoring my existingsnippets/directory..This PR fixes this! 🚀
I ended up doing a small refactor around path handling, which felt necessary to make the fix clean.
The old
PATHSconstant didn't distinguish between the preferred dir, the alt dir, and the currently active one, I think that ambiguity was the root cause of the bug. 🤔👉 I replaced it with a
GLOBAL_PATHSobject typed asSnippetPaths, which makes all three explicit.The selection logic lives in a small pure function
resolveSnippetDir(preferred, alt)that's easy to test in isolation (which I did).The new
GLOBAL_PATHSis populated once on startup withgetGlobalPathswhich works the same way asgetProjectPaths, with the new active dir selection.The same fix applies to project-scoped paths (
.opencode/snippet/vs.opencode/snippets/), which had the same issue.Existing tests have been updated, and all 341 tests pass!
One thing I noticed while digging around:
writeStatewrites runtime state into the config directory.This feels like it goes against XDG conventions, and runtime state probably belongs in
$XDG_STATE_HOME(or$XDG_RUNTIME_DIRfor truly ephemeral data) instead of$XDG_CONFIG_HOME.👉 Happy to make a separate PR if you think this is a good idea!