[LUM-498] Fix inferCategory main-thread hang by caching results and offloading computation#24274
Conversation
… (LUM-498) SkillsManager.rebuildCategoryMap now reuses cached values from the existing categoryMap, only calling inferCategory for skills not already in the cache. This eliminates redundant string allocation and keyword matching on every objectWillChange notification. IdentityPanel.fetchSkills now runs the inferCategory loop in a Task.detached so it executes off the main actor, preventing the main thread hang during initial skill load. Co-Authored-By: tkheyfets <timur@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![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: 560decc714
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| map.reserveCapacity(skills.count) | ||
| for skill in skills { | ||
| map[skill.id] = inferCategory(skill) | ||
| map[skill.id] = categoryMap[skill.id] ?? inferCategory(skill) |
There was a problem hiding this comment.
Using categoryMap[skill.id] as the sole cache key means inferCategory is never rerun for skills whose id stays constant but whose name/description changes. SkillsStore.fetchSkills(force: true) can refresh skill metadata without changing IDs, so category filters and the constellation grouping can become stale for the rest of the session after catalog updates or local skill edits. Invalidate per-skill cache entries when classification inputs change (or key the cache by a metadata fingerprint) so refreshed skill data is reflected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 115258c. rebuildCategoryMap now tracks a fingerprint (name + "\0" + description) alongside each cached entry via a parallel categoryFingerprints dictionary. The cached category is only reused when the fingerprint matches; any metadata change triggers re-inference. Stale entries are naturally evicted because both maps are rebuilt fresh each call (skills removed from the list drop out).
The @ObservationIgnored annotation on categoryFingerprints ensures SwiftUI doesn't trigger view updates for fingerprint changes — only the public categoryMap drives observation.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Track a fingerprint (name + description) alongside each cached category entry so that rebuildCategoryMap re-runs inferCategory when a skill's classification inputs change while keeping the same ID. Co-Authored-By: tkheyfets <timur@vellum.ai>
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…ffloading computation (#24274) * fix: cache inferCategory results and move computation off main thread (LUM-498) SkillsManager.rebuildCategoryMap now reuses cached values from the existing categoryMap, only calling inferCategory for skills not already in the cache. This eliminates redundant string allocation and keyword matching on every objectWillChange notification. IdentityPanel.fetchSkills now runs the inferCategory loop in a Task.detached so it executes off the main actor, preventing the main thread hang during initial skill load. Co-Authored-By: tkheyfets <timur@vellum.ai> * fix: invalidate category cache when skill metadata changes Track a fingerprint (name + description) alongside each cached category entry so that rebuildCategoryMap re-runs inferCategory when a skill's classification inputs change while keeping the same ID. Co-Authored-By: tkheyfets <timur@vellum.ai> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: tkheyfets <timur@vellum.ai>
1 participant
Fixes a main-thread stall in the Skills panel caused by
inferCategoryrunning redundantly on everySkillsStore.objectWillChangenotification — including non-skill property changes likeisLoadingandloadedBodies. Two targeted changes eliminate the redundant work:SkillsManager.rebuildCategoryMapnow caches inferred categories keyed by skill ID with a fingerprint (name + "\0" + description) for invalidation. When the fingerprint matches, the cached category is reused (O(1) dictionary lookup); when metadata changes,inferCategoryis re-run for that skill. This reduces the hot path from O(N×K) string matching to O(N) dictionary lookups in the common case (skills unchanged), while still correctly re-classifying skills whose name or description changes.IdentityPanel.fetchSkillsmoves theinferCategoryloop into aTask.detachedso it runs off the main actor, preventing the keyword-matching work from blocking UI during initial skill load.Both are safe because
inferCategoryis a non-isolated pure function overSendablevalue types with no shared mutable state. ThecategoryFingerprintsdictionary is marked@ObservationIgnoredso it doesn't trigger unnecessary SwiftUI view updates.Review & Testing Checklist for Human
SkillsManager.swiftandIdentityPanel.swiftcompile without errors or warnings.inferCategorycalled) and removed skills drop out of the map.Notes
Task.detachedwas chosen overTask { }because the enclosingfetchSkills()is called fromIdentityPanel(a SwiftUIView), so a plainTaskinherits main actor isolation.Task.detachedensures keyword matching runs on the cooperative thread pool.\0) separator to prevent false cache hits from coincidentalname + descriptioncollisions.rebuildCategoryMapentirely when skill IDs haven't changed, but the fingerprint-based caching is simpler and sufficient.Link to Devin session: https://app.devin.ai/sessions/ee5299fa3a7142c693319834eb37d19b
Requested by: @tkheyfets