fix(ui): show custom skills reliably on startup#2821
Open
ilgax wants to merge 1 commit intocharmbracelet:mainfrom
Open
fix(ui): show custom skills reliably on startup#2821ilgax wants to merge 1 commit intocharmbracelet:mainfrom
ilgax wants to merge 1 commit intocharmbracelet:mainfrom
Conversation
Resolve race condition by implementing a package-level cache for skill discovery states. Includes deep-cloning for isolation and deterministic sorting.
d501d25 to
e0b6dc5
Compare
Contributor
Author
|
Hey! Just wanted to add that I verified this also fixes the duplication issues from #2683 and #2694. I tested it with some symlinks and the new deduplication in the Coordinator handles it perfectly. It feels like a much cleaner way to solve it since it handles both the symlink stuff and the startup race condition in |
Member
|
I very much appreciate this! This likely does solve those two, but we'll verify! |
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
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.
Description
Resolves a race condition where custom skills were occasionally missing on initial launch. This happened because the backend's asynchronous skill discovery would sometimes fire its "Updated" event before the TUI had finished subscribing to the pubsub broker.
Solution
The fix was to add a thread-safe cache for discovered skills in the
internal/skillspackage. Instead of just firing a "discovery finished" event and hoping the UI catches it in time, the backend now saves those results so the UI can grab them whenever it's ready. This means that even on the very first frame, the UI has the full list of builtins and user skills.This also let us centralize how we merge and sort everything in the
Coordinator, making the startup experience more deterministic.Key Changes
GetLatestStatesandSetLatestStatesininternal/skillswith full deep-cloning ofSkillStateobjects to prevent memory corruption across package boundaries.internal/ui/model/ui.go) to seed its initialskillStatesfrom the discovery cache.skills.DeduplicateStatesto the coordinator logic. When a user skill overrides a builtin, the UI status sidebar now only displays the state of the active user-defined skill.slices.SortStableFuncwith case-insensitive path sorting for both theskillsandstatesslices to ensure a stable UI layout.diagnostics_test.goto verify memory isolation and cache health.Visuals
Tests
internal/skillstests pass, including new isolation checks.CONTRIBUTING.md.