fix(site): hide slash skills menu when user has no personal skills#26953
fix(site): hide slash skills menu when user has no personal skills#26953ibetitsmike wants to merge 2 commits into
Conversation
…h skills menu when user has no personal skills
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 2 findings (1 Nit, 1 Note), APPROVE. Review Finding inventoryFindings
Round logRound 1Panel (13 reviewers: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Nami, Ging-ts, Ging-react, Gon, Leorio, Komugi, Chopper, Meruem). Netero: 0 findings. Panel: 2 Nit, 1 Note new. 1 dropped. Reviewed against e3ac65a..d724bd9. Round 2Panel (4 reviewers: Bisky, Mafuuu, Gon, Kite). Netero: 0 findings. CRF-1, CRF-2 verified fixed. Gon re-raised CRF-1 as incomplete (3x P2 on our own R1 suggested text); dropped as churn (Kite verified complete, 2v1). No new findings. APPROVE. Reviewed against e3ac65a..15707c2. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean, well-scoped fix. The three-state distinction (resolved-empty, filtered-empty, loading) is the minimum logic needed, the staleTime follows existing convention, and the two stories test the exact behavioral fork. 13 reviewers, zero correctness issues.
2 Nit, 1 Note below.
Commit subject is 115 characters (43 over the 72-char convention). The scope site/src/pages/AgentsPage/components/ChatMessageInput burns 53 characters. fix(site) contains both changed files and brings it to 66.
"I tried to build a case against this change across all four framings. The problem is correctly understood, the solution is proportional, and the fix is at the right causal level." - Pariston
site/src/pages/AgentsPage/components/ChatMessageInput/PersonalSkillsTriggerMenu.tsx:158
Note [CRF-3] "No personal skills found." is now unreachable in the integrated ChatMessageInput.
The new gating closes the menu before the settled-empty state that would trigger this !query branch. Loading shows the spinner, error shows the error message, filtered-empty shows "No personal skills match that query." The base-empty case (no query, no skills, no loading, no error) cannot happen because isResolvedEmptySkillsList closes the menu first.
The message survives in PersonalSkillsTriggerMenu's own stories and remains reachable by any future caller with different gating logic. Not a bug. Worth knowing so nobody writes a test expecting this text in the integrated component.
(Leorio)
🤖
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All R1 findings addressed. CRF-1 comments trimmed to the suggested forms, CRF-2 simplified to isFetching. CRF-3 (unreachable "No personal skills found." in the integrated component) remains informational.
R2 panel (Bisky, Mafuuu, Gon, Kite) plus Netero found no new issues. The fix cleanly separates trigger detection from menu display, handles all query states correctly, and has complementary story coverage for both empty-state paths.
"The change is single-purpose: suppress the skills popover for users with zero skills so
/behaves as plain text." - Kite
🤖 This review was automatically generated with Coder Agents.
Typing
/in the /agents chat composer always opened the personal skills popover, even for users with zero skills. The empty popover offered no value and, worse,SkillsTriggerPluginswallowed Enter and arrow keys whenever it was open, so a zero-skill user pressing Enter after/could not submit the message. Additionally, the skills query re-enabled every time the caret re-entered a slash token, refetching on every toggle with the app's defaultstaleTime: 0.Changes in
ChatMessageInput:skillsMenuOpenon a definitively empty base skill list: when the query has settled (isSuccess && !isFetching) and returned no skills,/behaves like plain text with no popover and no key interception. Loading and error states still show the popover, and filtered-empty (user has skills, query matches none) keeps showing the no-match message.staleTime: 60_000at the call site so caret movement no longer refetches on each trigger toggle. The settings page keeps its refetch-on-mount freshness.Stories:
EmptySkillsnow asserts no popover renders and Enter submits; newFilteredEmptyKeepsMenuOpenasserts the no-match message stays and Enter is swallowed when the user has skills.