Open
Conversation
added 10 commits
February 27, 2026 13:16
…ook for opening skill info while updating composer rendering
…, refine composer regex ordering, and adjust file mention handling
…thout overflowing
… and enhanced slash command hook interaction
…resentation, add translation keys across locales
…s while always rendering the skill info button in CommandMenu
…ty state and add selected indicator to command menu
…udge inline hint positioning
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds provider-scoped skills: server-side skill discovery/caching and API extensions plus frontend wiring to prefetch, reload, decorate, and inspect skills; expands slash/command hooks and chat UI to support skill tokens, tooltips, and a skill info dialog; adds i18n keys and settings UI for reloading skills. Changes
Sequence DiagramsequenceDiagram
participant User
participant ChatUI as Chat Interface
participant Composer as Composer Hook
participant Slash as Slash Hook
participant Server
participant Cache as Skills Cache
User->>ChatUI: Select provider
ChatUI->>Slash: prefetchSkillsForProvider(provider)
Slash->>Cache: check cached skills for provider
alt not cached
Slash->>Server: POST /api/commands/list (provider, includeSkills=true)
Server->>Server: loadSkillsList(agent), normalize skills
Server->>Cache: cache skills by provider
Server-->>Slash: return commands + skills
else cached
Cache-->>Slash: return cached skills
end
User->>ChatUI: Type "/" or touch token
ChatUI->>Composer: renderInputWithSkillDecorations(text)
Composer-->>ChatUI: decorated input, activeSkillTooltip
User->>ChatUI: View skill info
ChatUI->>Composer: openSkillInfoDialogFromMenu / token
Composer-->>ChatUI: SkillInfoDialog rendered
User->>ChatUI: Click reload in settings
ChatUI->>Slash: reloadSkillsForProvider(provider, forceReload=true)
Slash->>Server: POST /api/commands/list (forceReloadSkills=true)
Server->>Cache: clear & reload skills
Server-->>Slash: return refreshed skills
Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
server/utils/loadSkillsList.js (2)
36-410: Consider removing commented-out agent configurations.Over 350 lines of commented-out code for unsupported agents makes the file harder to maintain. If these are planned for future implementation, consider:
- Moving them to a separate reference file or documentation
- Adding a TODO comment with a tracking issue
- Removing them entirely and re-adding when needed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/loadSkillsList.js` around lines 36 - 410, The file's agents object contains >350 lines of commented-out agent configs (e.g., commented keys around 'claude-code', 'codex', 'cursor') which clutter loadSkillsList.js; remove these commented-out agent blocks and either move them to a new separate reference file (e.g., create server/utils/agents-reference.js or agents/README) or convert them into concise TODO entries with tracking links, and keep only the active agent entries inside the agents object ('claude-code', 'codex', 'cursor') so loadSkillsList.js contains only live configuration and a brief comment pointing to the reference file or issue.
437-458: Add error handling for filesystem operations.The
readdirSynccall on line 442 andreadFileSyncon line 448 can throw exceptions (e.g., permission errors, race conditions if directory is deleted betweenexistsSyncand read). While the outer function doesn't explicitly handle these, consider wrapping in try-catch to prevent a single bad skill directory from breaking the entire skills load.🛡️ Proposed error handling
for (const skillsDir of skillsDirs) { for (const subDir of subSkillsDir) { const fullSkillsDir = join(skillsDir, subDir); if (!existsSync(fullSkillsDir)) continue; - for (const f of readdirSync(fullSkillsDir)) { - const skillDir = join(fullSkillsDir, f) - // find /SKILL.md files in skillDir - const skillFileMdPath = join(skillDir, 'SKILL.md'); - if (!existsSync(skillFileMdPath)) continue; - - const fileContents = readFileSync(skillFileMdPath, 'utf-8'); - const { data } = matter(fileContents); - if (!data.name) continue; - - // TODO: handle `.user-invocable`, `.disable-model-invocation`? - loadedSkills.push({ - ...data, - skillDir, - }) - } + try { + for (const f of readdirSync(fullSkillsDir)) { + try { + const skillDir = join(fullSkillsDir, f) + // find /SKILL.md files in skillDir + const skillFileMdPath = join(skillDir, 'SKILL.md'); + if (!existsSync(skillFileMdPath)) continue; + + const fileContents = readFileSync(skillFileMdPath, 'utf-8'); + const { data } = matter(fileContents); + if (!data.name) continue; + + // TODO: handle `.user-invocable`, `.disable-model-invocation`? + loadedSkills.push({ + ...data, + skillDir, + }) + } catch (err) { + console.warn(`Error loading skill from ${f}:`, err.message); + } + } + } catch (err) { + console.warn(`Error reading skills directory ${fullSkillsDir}:`, err.message); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/loadSkillsList.js` around lines 437 - 458, Wrap the filesystem operations that can throw inside the nested loops — specifically the readdirSync(fullSkillsDir) and readFileSync(skillFileMdPath) calls used to populate loadedSkills — with try/catch so a single bad skillDir or transient FS error doesn't abort the whole load; on error log the issue (including skillDir and skillFileMdPath) and continue to the next entry, and only parse/use matter(fileContents) when readFileSync succeeds and returns a string before pushing into loadedSkills.src/i18n/locales/ja/chat.json (1)
238-242: Consider translating action button labels to Japanese.The action button labels (
clear,usage,close) are in English while other UI elements are translated to Japanese. This appears inconsistent with other localized action buttons in this file (e.g.,input.send→ "送信",input.stop→ "中止").If the English labels are intentional for consistency with technical terminology, consider adding a comment or documentation to clarify this decision.
💡 Suggested Japanese translations
"actions": { - "clear": "Clear", - "usage": "Usage", - "close": "Close" + "clear": "クリア", + "usage": "使用方法", + "close": "閉じる" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/ja/chat.json` around lines 238 - 242, The action labels in the "actions" object (`clear`, `usage`, `close`) are still in English; update their string values to Japanese to match the rest of the locale (e.g., change "clear", "usage", "close" to appropriate Japanese strings such as 消去/クリア, 使用量/ヘルプ, 閉じる), replacing the values under the "actions" key so keys remain unchanged; if leaving any English is intentional, add a brief inline comment explaining the decision for future maintainers.src/i18n/locales/ko/chat.json (1)
253-257: Consider translating action button labels to Korean.Similar to the Japanese locale, the action button labels are in English while other UI elements are translated. For consistency, consider:
💡 Suggested Korean translations
"actions": { - "clear": "Clear", - "usage": "Usage", - "close": "Close" + "clear": "지우기", + "usage": "사용법", + "close": "닫기" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/ko/chat.json` around lines 253 - 257, The action button labels inside the "actions" object ("clear", "usage", "close") are still in English; update their string values to Korean (e.g., replace "Clear" with a suitable Korean term like "지우기" or "초기화", "Usage" with "사용량", and "Close" with "닫기") in the src/i18n/locales/ko/chat.json file so the "actions" object values are fully localized.server/routes/commands.js (1)
451-496: Consider validating theproviderparameter.The
providerparameter is used directly without validation. While there's a fallback toclaudeingetSkillsForProvider, consider validating against known providers for clearer error messages.💡 Optional validation
router.post('/list', async (req, res) => { try { const { projectPath, provider = 'claude', includeSkills = true, forceReloadSkills = false, } = req.body; + + const validProviders = Object.keys(providerToAgentMap); + const normalizedProvider = validProviders.includes(provider) ? provider : 'claude'; + const allCommands = [...builtInCommands];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/commands.js` around lines 451 - 496, Validate the provider parameter before calling getSkillsForProvider: define an allowedProviders list (or derive it from an existing source if available) and check the extracted provider variable; if it's not in allowedProviders, return a 400 response with a clear error message instead of continuing, otherwise proceed to call getSkillsForProvider({ provider, forceReloadSkills }). Update the route handler (router.post('/list', ...)) to perform this check right after destructuring provider from req.body and before computing skills; reference the provider variable, the getSkillsForProvider call, and the router.post('/list' handler) when locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/utils/loadSkillsList.js`:
- Around line 9-24: The function findConfigHome references the variable home
before it is declared which causes a ReferenceError; fix it by moving the
declaration const home = homedir() above findConfigHome (or refactor
findConfigHome to accept home as an argument) and then compute configHome and
codexHome after that; update references to use the hoisted home so
findConfigHome, configHome and codexHome all see a defined home value.
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 569-571: The hook currently derives skillCommands using only
command.type === 'skill' which diverges from the menu path that recognizes
skills by command.namespace === 'skills'; update the skill detection predicate
used in useChatComposerState (the skillCommands useMemo and the similar check
around lines ~678-680) to a unified predicate (e.g., treat a command as a skill
if command.type === 'skill' || command.namespace === 'skills') so both token
highlighting/hints and menu-driven skill info will work for namespace-only
payloads; apply the same predicate to all spots that branch on skills in this
file.
- Around line 787-790: The trailing-space check currently depends on hasUsage
and can merge tokens when usageText is empty; update the needsTrailingSpace
computation in useChatComposerState so it does not reference hasUsage. Replace
the line computing needsTrailingSpace with: const needsTrailingSpace =
suffixedInput.length > 0 && !/^\s/.test(suffixedInput); leaving
needsLeadingSpace, insertedText and nextValue logic unchanged so prefixedInput,
insertedText, and suffixedInput remain properly separated.
In `@src/components/chat/hooks/useSlashCommands.ts`:
- Around line 125-131: The prefetch path is overwriting the currently visible
slashCommands by calling setSlashCommands with results for a non-active
targetProvider; update the logic in buildSortedCommands usage and in
prefetchSkillsForProvider so you only call setSlashCommands when targetProvider
=== activeProvider (or derive activeProvider from state/props), and when
updating skillsByProviderRef ensure async results are gated by the same
activeProvider check (e.g., compare targetProvider to currentProvider before
setSlashCommands). Specifically modify places referencing buildSortedCommands,
baseCommandsRef.current, skillsByProviderRef.current[targetProvider],
setSlashCommands, and prefetchSkillsForProvider to guard writes so stale
prefetch responses cannot replace the active provider’s command list.
- Around line 347-364: selectCommandFromKeyboard currently executes non-skill
commands but doesn't call trackCommandUsage, so keyboard selections aren't
tracked; update selectCommandFromKeyboard to call trackCommandUsage(command) for
non-skill commands (e.g., after resetCommandMenuState() and before calling
onExecuteCommand(command)) so keyboard-invoked commands contribute to
history/frequent sorting; keep existing promise handling for the executionResult
unchanged.
In `@src/components/chat/view/subcomponents/CommandMenu.tsx`:
- Around line 231-235: The icon-only skill info button in CommandMenu is missing
an accessible name; update the button JSX (the element with onMouseDown handling
the skill info) to include an explicit aria-label (use the same translated
string as title, e.g. t('commandMenu.viewSkillInfo')) so screen readers announce
the control; keep title as-is and ensure aria-label matches for accessibility.
In `@src/components/chat/view/subcomponents/SkillInfoDialog.tsx`:
- Around line 136-142: The label in SkillInfoDialog.tsx is not bound to its
textarea which breaks accessibility; add a matching id on the textarea (e.g.,
"skill-usage-textarea" or a generated unique id) and set the label's htmlFor to
that id so screen readers associate the label with the textarea used by
state.usageText and onUsageChange (and keep the existing placeholder/state
binding intact).
- Around line 89-93: The dialog with role="dialog" in SkillInfoDialog.tsx lacks
an accessible name; fix by connecting the dialog to the visible title: give the
title element (the div showing state.info.commandName) a stable id (e.g.,
commandNameLabel) and add aria-labelledby="commandNameLabel" to the dialog
container, or alternatively set aria-label={state.info.commandName} on the
dialog; update the JSX around the role="dialog" element and the title div
accordingly so assistive tech can announce the dialog name.
In `@src/i18n/locales/zh-CN/chat.json`:
- Around line 254-256: The "usage" label in the zh-CN locale file is still
English; update the value for the "usage" key in
src/i18n/locales/zh-CN/chat.json (the JSON object containing "clear", "usage",
"close") to a proper Chinese translation (e.g., "使用情况" or "用量") so the button
group is fully localized; ensure you only change the string value for the
"usage" key and keep JSON formatting intact.
---
Nitpick comments:
In `@server/routes/commands.js`:
- Around line 451-496: Validate the provider parameter before calling
getSkillsForProvider: define an allowedProviders list (or derive it from an
existing source if available) and check the extracted provider variable; if it's
not in allowedProviders, return a 400 response with a clear error message
instead of continuing, otherwise proceed to call getSkillsForProvider({
provider, forceReloadSkills }). Update the route handler (router.post('/list',
...)) to perform this check right after destructuring provider from req.body and
before computing skills; reference the provider variable, the
getSkillsForProvider call, and the router.post('/list' handler) when locating
the change.
In `@server/utils/loadSkillsList.js`:
- Around line 36-410: The file's agents object contains >350 lines of
commented-out agent configs (e.g., commented keys around 'claude-code', 'codex',
'cursor') which clutter loadSkillsList.js; remove these commented-out agent
blocks and either move them to a new separate reference file (e.g., create
server/utils/agents-reference.js or agents/README) or convert them into concise
TODO entries with tracking links, and keep only the active agent entries inside
the agents object ('claude-code', 'codex', 'cursor') so loadSkillsList.js
contains only live configuration and a brief comment pointing to the reference
file or issue.
- Around line 437-458: Wrap the filesystem operations that can throw inside the
nested loops — specifically the readdirSync(fullSkillsDir) and
readFileSync(skillFileMdPath) calls used to populate loadedSkills — with
try/catch so a single bad skillDir or transient FS error doesn't abort the whole
load; on error log the issue (including skillDir and skillFileMdPath) and
continue to the next entry, and only parse/use matter(fileContents) when
readFileSync succeeds and returns a string before pushing into loadedSkills.
In `@src/i18n/locales/ja/chat.json`:
- Around line 238-242: The action labels in the "actions" object (`clear`,
`usage`, `close`) are still in English; update their string values to Japanese
to match the rest of the locale (e.g., change "clear", "usage", "close" to
appropriate Japanese strings such as 消去/クリア, 使用量/ヘルプ, 閉じる), replacing the values
under the "actions" key so keys remain unchanged; if leaving any English is
intentional, add a brief inline comment explaining the decision for future
maintainers.
In `@src/i18n/locales/ko/chat.json`:
- Around line 253-257: The action button labels inside the "actions" object
("clear", "usage", "close") are still in English; update their string values to
Korean (e.g., replace "Clear" with a suitable Korean term like "지우기" or "초기화",
"Usage" with "사용량", and "Close" with "닫기") in the src/i18n/locales/ko/chat.json
file so the "actions" object values are fully localized.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
server/routes/commands.jsserver/utils/loadSkillsList.jssrc/components/QuickSettingsPanel.jsxsrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useFileMentions.tsxsrc/components/chat/hooks/useSlashCommands.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/ChatComposer.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/components/chat/view/subcomponents/CommandMenu.tsxsrc/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsxsrc/components/chat/view/subcomponents/SkillInfoDialog.tsxsrc/components/chat/view/subcomponents/SkillInfoTooltip.tsxsrc/i18n/locales/en/chat.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/ja/chat.jsonsrc/i18n/locales/ja/settings.jsonsrc/i18n/locales/ko/chat.jsonsrc/i18n/locales/ko/settings.jsonsrc/i18n/locales/zh-CN/chat.jsonsrc/i18n/locales/zh-CN/settings.json
…ing while syncing slash commands only for the active provider
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 169-177: findSkillTokenRanges currently rebuilds a dynamic
alternation RegExp from skillNames on every call (using escapeRegExp and
sortedSkillNames), which is executed in render/decoration hot paths and causes
responsiveness issues; change the implementation to avoid per-call regex
construction by precomputing and memoizing a lookup/tokenizer when skillNames
change (e.g., build the RegExp once or construct a Set/trie of normalized skill
tokens) and update findSkillTokenRanges to accept the precompiled pattern or
lookup structure instead of raw skillNames, ensuring callers (the composer
decoration flow and the hook that invokes findSkillTokenRanges) pass the
memoized object (useMemo or cached singleton keyed by skill list) so matching
uses cheap Set/trie lookups or a reused RegExp instead of rebuilding on every
render.
- Around line 606-613: Change the matching logic to prefer an exact skill name
match before falling back to prefix disambiguation: first search skillCommands
for a command whose name strictly equals fullToken and return that single
command if found; only if no exact match exists, compute matchingCommands =
skillCommands.filter(c => c.name.startsWith(fullToken)) and proceed with the
existing length check and selection (the existing variables to update are
matchingCommands, fullToken, and the use where command is assigned).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useSlashCommands.tssrc/components/chat/view/subcomponents/CommandMenu.tsxsrc/components/chat/view/subcomponents/SkillInfoDialog.tsxsrc/i18n/locales/zh-CN/chat.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/chat/view/subcomponents/SkillInfoDialog.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/chat/hooks/useChatComposerState.ts (2)
813-814: Use a guarded fallback before trimming aroundslashPosition.At Line 813,
slashPositionis always passed as fallback. If no local slash token is near the caret, this can cause replacement behavior based on stale menu state instead of pure cursor insertion. Guard the fallback (or disable it) unless it still points to the active token context.Proposed adjustment
- const { nextInput: trimmedInput, nextCursor } = trimSkillTokenAroundCursor(baseInput, currentCursor, slashPosition); + const safeFallbackSlash = + slashPosition >= 0 && + slashPosition < baseInput.length && + baseInput[slashPosition] === '/' && + slashPosition <= currentCursor && + !/\s/.test(baseInput.slice(slashPosition, currentCursor)) + ? slashPosition + : -1; + const { nextInput: trimmedInput, nextCursor } = trimSkillTokenAroundCursor( + baseInput, + currentCursor, + safeFallbackSlash, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` around lines 813 - 814, The call to trimSkillTokenAroundCursor currently always passes slashPosition, which can cause stale menu-state replacement instead of a plain caret insertion; change the caller so it only passes slashPosition when it actually points to the active local slash token context (e.g., compute a boolean like isActiveSlashToken or validate that slashPosition is within the currently detected token bounds) and otherwise pass undefined/null as the fallback so trimSkillTokenAroundCursor performs a pure cursor insertion; update the invocation around baseInput/currentCursor/trimSkillTokenAroundCursor to use the guarded value and keep using nextInput/nextCursor as before.
866-895: Inline-hint element construction is repeated in three branches.The same hint overlay/spacer/text structure appears multiple times. Extracting a small helper renderer would reduce drift risk and make future UI tweaks safer.
Also applies to: 910-962, 1004-1056
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` around lines 866 - 895, Extract the repeated inline-hint overlay/span structure into a small helper renderer (e.g., renderInlineSkillHintOverlay) and replace the three duplicate blocks that build the hint spacer/overlay/text and surrounding before/after spans (the code that uses inlineSkillArgumentHint, activeTypedSkillToken, hintWidthCh, renderInputWithMentions and createElement keys like 'skill-inline-hint-overlay'/'skill-inline-hint-spacer'/'skill-inline-hint-text') with calls to that helper; the helper should accept the hint string, computed hintWidthCh, the before/after text (or the full text and cursor) and return the same array of createElement spans so UI structure and keys remain stable while centralizing styling/markup for future tweaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 813-814: The call to trimSkillTokenAroundCursor currently always
passes slashPosition, which can cause stale menu-state replacement instead of a
plain caret insertion; change the caller so it only passes slashPosition when it
actually points to the active local slash token context (e.g., compute a boolean
like isActiveSlashToken or validate that slashPosition is within the currently
detected token bounds) and otherwise pass undefined/null as the fallback so
trimSkillTokenAroundCursor performs a pure cursor insertion; update the
invocation around baseInput/currentCursor/trimSkillTokenAroundCursor to use the
guarded value and keep using nextInput/nextCursor as before.
- Around line 866-895: Extract the repeated inline-hint overlay/span structure
into a small helper renderer (e.g., renderInlineSkillHintOverlay) and replace
the three duplicate blocks that build the hint spacer/overlay/text and
surrounding before/after spans (the code that uses inlineSkillArgumentHint,
activeTypedSkillToken, hintWidthCh, renderInputWithMentions and createElement
keys like
'skill-inline-hint-overlay'/'skill-inline-hint-spacer'/'skill-inline-hint-text')
with calls to that helper; the helper should accept the hint string, computed
hintWidthCh, the before/after text (or the full text and cursor) and return the
same array of createElement spans so UI structure and keys remain stable while
centralizing styling/markup for future tweaks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/chat/hooks/useChatComposerState.ts (1)
865-897: Extract repeated inline-hint rendering into a helper.The same inline-hint overlay block appears in three paths. Pulling this into one helper will reduce drift and make future UI changes safer.
Also applies to: 912-967, 1008-1063
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` around lines 865 - 897, Extract the repeated inline-hint overlay JSX into a single helper (e.g., renderInlineSkillArgumentHint) and use it in all three places inside useChatComposerState where the overlay is duplicated (the blocks that reference inlineSkillArgumentHint, activeTypedSkillToken, renderInputWithMentions and use createElement with keys like 'skill-inline-hint-overlay', 'skill-inline-hint-spacer', 'skill-inline-hint-text'); the helper should accept the hint text and return the same createElement structure (preserving classes, spacer/text spans and keys or derive keys from a passed suffix) so you replace the three inline copies with calls to renderInlineSkillArgumentHint(inlineSkillArgumentHint) while keeping surrounding before/after renderInputWithMentions slices intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/chat/view/subcomponents/SkillInfoDialog.tsx`:
- Around line 59-72: The dialog currently only listens for Escape but doesn't
move focus into the modal, trap Tab/Shift+Tab, or restore the previously focused
element on close; update the SkillInfoDialog component to add a ref for the
dialog container, and in the useEffect that depends on state.open and onClose:
when opening, save document.activeElement, move focus to the first focusable
element inside the dialog (or the dialog container), and add a keydown handler
that handles Escape (calls onClose) and Tab/Shift+Tab to loop focus within the
dialog; on cleanup (or when closing) remove the keydown listener and restore
focus to the saved element. Ensure the effect references the dialog ref and
cleans up all listeners and saved state so focus is reliably trapped and
restored.
---
Nitpick comments:
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 865-897: Extract the repeated inline-hint overlay JSX into a
single helper (e.g., renderInlineSkillArgumentHint) and use it in all three
places inside useChatComposerState where the overlay is duplicated (the blocks
that reference inlineSkillArgumentHint, activeTypedSkillToken,
renderInputWithMentions and use createElement with keys like
'skill-inline-hint-overlay', 'skill-inline-hint-spacer',
'skill-inline-hint-text'); the helper should accept the hint text and return the
same createElement structure (preserving classes, spacer/text spans and keys or
derive keys from a passed suffix) so you replace the three inline copies with
calls to renderInlineSkillArgumentHint(inlineSkillArgumentHint) while keeping
surrounding before/after renderInputWithMentions slices intact.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/chat/hooks/useChatComposerState.tssrc/components/chat/view/subcomponents/SkillInfoDialog.tsxsrc/components/chat/view/subcomponents/SkillInfoTooltip.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/chat/view/subcomponents/SkillInfoTooltip.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/chat/view/subcomponents/SkillInfoDialog.tsx`:
- Around line 67-72: The focusableSelector used by getFocusableElements omits
the interactive <summary> element, so keyboard users can be skipped; update the
focusableSelector (and any other place building that selector) to include
summary:not([disabled]) (or ensure the rendered <summary> has a tabindex) so
querySelectorAll in getFocusableElements will return the summary control and the
focus trap cycles correctly; update references to focusableSelector and verify
the interactive <summary> rendered in SkillInfoDialog is discoverable.
…us trap includes the metadata summary.
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.
load skills
/<skill-name> arg1 arg2display skills in list
display placeholder
argument hinthover
/skillin composer display Skill Info Dialogreaload skills in Quick Settings
on mobile
press View icon show Skill Info Dialog with Form Usage. Enter args or none then press usage will insert
/skill args...at cursorSummary by CodeRabbit
New Features
Localization