refactor(tui): migrate to OpenTUI/Solid.js framework#32
Conversation
Complete TUI rewrite from Ink/React to OpenTUI/Solid.js: - Replace React with Solid.js for reactive state management - Migrate all 20 screens to OpenTUI component primitives - Add monochromatic design with 14 color themes - Implement cinematic splash screen with ASCII animation - Add vi-key navigation (j/k/h/l) throughout - Create 32 reusable components with consistent styling - Add service layer for @skillkit/core API integration - Remove deprecated react-opentui.d.ts type definitions - Update build config for Solid.js JSX transform Features covered: - Browse, Marketplace, Recommend (discovery) - Installed, Sync, Translate (management) - Workflow, Execute, Plan, History (execution) - Team, Context, Memory, Mesh, Message (collaboration) - Plugins, Methodology, Settings (configuration) - Help screen with 25+ keyboard shortcuts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces the React/Ink TUI with Solid/@opentui/solid: updates package/build config, switches JSX/renderer, migrates most components and screens to Solid primitives, adds many new Solid UI components and services, and centralizes services/state exports. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (process)
participant Renderer as Renderer (`@opentui/core` createCliRenderer)
participant App as App (Solid)
participant Services as Services (service modules)
CLI->>Renderer: createCliRenderer(config)
CLI->>Renderer: render(() => App)
Renderer->>App: mount (Solid render)
App->>Services: request data (load*/get* calls)
Services-->>App: return data / loading / error
App->>Renderer: UI updates via Solid reactivity
CLI->>Renderer: SIGINT/SIGTERM or exit
Renderer->>App: dispose renderer / cleanup
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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)
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}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tui/src/components/Sidebar.tsx (1)
7-10:⚠️ Potential issue | 🟠 Major
onNavigateprop is defined but never used.The
onNavigatecallback is declared in the interface but the component never calls it. Sidebar items lack click/selection handlers, so users cannot navigate by clicking on them. Either remove the unused prop or wire up navigation handlers on each item.🔧 Proposed fix to add navigation handler to items
<For each={nav.items}> {(item) => { const active = () => props.screen === item.screen; return ( - <text fg={active() ? terminalColors.accent : terminalColors.text}> + <text + fg={active() ? terminalColors.accent : terminalColors.text} + onClick={() => props.onNavigate(item.screen)} + > {active() ? (Note: Verify the correct event handler prop name for the OpenTUI
<text>component (e.g.,onClick,onSelect, or similar).packages/tui/src/screens/Mesh.tsx (1)
6-12:⚠️ Potential issue | 🟠 MajorImplement missing keyboard handlers and use props:
onNavigate,cols, androwsare defined but never referenced.The component displays keyboard shortcuts ('d' discover, 'a' add host, 'r' remove, 's' security, 'h' health check) but has no handler implementation. All other screens in the TUI follow this pattern: they import
useKeyboardfrom '@opentui/solid', set up keyboard handlers that callprops.onNavigate(), and usecols/rowsfor responsive layout (e.g., calculating content width and compact mode). Mesh should do the same.
🤖 Fix all issues with AI agents
In `@packages/tui/src/App.tsx`:
- Around line 5-11: The imports for ThemeProvider, KeybindProvider,
ToastProvider, DialogProvider, CommandProvider, RouteProvider and
SidebarProvider cannot be resolved because the corresponding context modules are
missing; either create a ./context/* module for each named export (implement and
export ThemeProvider/useTheme, KeybindProvider/useKeybinds,
ToastProvider/useToast, DialogProvider/useDialog, CommandProvider/useCommand,
RouteProvider/useRoute, SidebarProvider/useSidebar) and place them next to
App.tsx, or update the import paths in App.tsx to point to the existing provider
modules in the codebase (ensuring each named symbol matches the exported
identifiers exactly) so the bundler can resolve them.
In `@packages/tui/src/components/ProgressBar.tsx`:
- Around line 5-22: The ProgressBar should defensively validate width and
progress to avoid invalid repeat() arguments: in ProgressBar update width() to
coerce props.width to a finite integer with a sensible lower bound (e.g.,
Math.max(1, Math.floor(...))) and fallback when props.width is undefined; update
clampedProgress() to guard against non-finite values (NaN/Infinity) by coercing
to 0 and then clamping to 0–100; change filledWidth() to compute using the
validated width and clampedProgress with Math.round/Math.floor as desired and
ensure emptyWidth() uses Math.max(0, width - filledWidth) so it can never be
negative; use these validated helpers (width(), clampedProgress(),
filledWidth(), emptyWidth(), filled(), empty()) throughout.
In `@packages/tui/src/index.tsx`:
- Around line 55-59: The package export file re-exports missing barrels (exports
of './context/index.js', './themes/index.js', './ui/index.js',
'./hooks/index.js', './services/index.js') which breaks the build; either remove
these export lines from packages/tui/src/index.tsx or create the corresponding
barrel modules (e.g., src/context/index.ts, src/themes/index.ts,
src/ui/index.ts, src/hooks/index.ts, src/services/index.ts) that re-export the
intended public symbols, ensure the export paths and extensions match your build
(use .ts/.tsx or index.ts files) and update any consumers to the correct paths
if you rename or remove exports.
In `@packages/tui/src/screens/Browse.tsx`:
- Around line 67-102: The bug is that loadRepoSkills uses an index into the full
repos() array (e.g., repos()[index]) while selectedIndex and UI actions operate
on a filtered list; change loadRepoSkills (and the similar code at the other
occurrence) to resolve the repo by identity instead of by index: accept or
compute the repoId (or repo object) from the filtered view (or pass the repo
from the caller), find the matching repo in the full repos state by a unique
identifier (e.g., repo.id or repo.source) and then update that repo entry in
setRepos by mapping and matching on that identifier rather than using the passed
index; update any callers (where selectedIndex is used) to pass the repo object
or id instead of the filtered index so loadRepoSkills and the code at the second
occurrence update the correct repository.
In `@packages/tui/src/screens/Context.tsx`:
- Around line 31-35: Add a local helper to change the current section and wire
it into the existing key handler so left/right (or h/l) updates the UI: create a
function (eg. changeSection(delta: number)) that reads selectedSection(), adds
delta, clamps the result between 0 and sections.length - 1, and calls
setSelectedSection(newIndex); then call changeSection(-1) for left/h and
changeSection(+1) for right/l inside the key handling logic so the view actually
moves away from "Overview".
- Around line 40-68: Wrap the async calls inside loadData, handleRefresh, and
handleExport with try/catch/finally: call
loadProjectContext/analyzeProjectContext/getStackTags in a try block, on catch
setState to include an error message and clear loading/analyzing flags (use
setState((s)=>({...s, loading:false, analyzing:false, error: err.message ||
String(err)}))), and in finally ensure loading/analyzing are reset; similarly
wrap refreshContext/getStackTags in handleRefresh and exportContext in
handleExport and surface errors (setState and/or console.error) so the UI never
remains stuck without an error.
In `@packages/tui/src/screens/Memory.tsx`:
- Around line 32-34: Import useKeyboard from '@opentui/solid' and register
keyboard handlers inside the Memory component (next to where selectedIndex,
setSelectedIndex, showDetail, setShowDetail are declared) so the advertised
shortcuts actually update state; add handlers for 'j'/'k' to increment/decrement
selectedIndex (clamping to list bounds), 'Enter' to setShowDetail(true) for
current selection, 'd' to delete the current item, 'c' to clear all, 'i' to
initialize, and 'Escape' to setShowDetail(false)/navigate back; ensure you call
setSelectedIndex and setShowDetail (the existing setters) and reference the
list/length used to bound selection.
In `@packages/tui/src/screens/Methodology.tsx`:
- Around line 42-46: The loadData function doesn't catch errors from
loadMethodologies so loading remains true on failure; wrap the await call in
try/catch inside loadData, on success call setState(result) as now, and on catch
call setState((s)=>({...s, loading:false, error: caughtError})) (or similar to
preserve existing state), ensuring error is set and loading is false; reference:
loadData, loadMethodologies, setState and the state fields loading/error.
- Around line 48-91: Add re-entry guards and ensure installing/syncing flags are
always reset: in handleInstall, handleUninstall and handleSync check the current
installing/syncing flag up-front and return if already true; wrap the async
operation and loadData call in try/finally so setInstalling(false) or
setSyncing(false) runs even on error, and surface errors (rethrow or log) from
installMethodologyPack/uninstallMethodologyPack/syncMethodologyPack so callers
can observe failures; use the existing symbols handleInstall, handleUninstall,
handleSync, selectedIndex(), state().packs, setInstalling, setSyncing,
installMethodologyPack, uninstallMethodologyPack, syncMethodologyPack, and
loadData to locate and apply the changes.
In `@packages/tui/src/screens/Plan.tsx`:
- Around line 82-95: The execution block can divide by zero, leak state if
executeFn throws, and call loadData without awaiting; fix by guarding totalTasks
(use totalTasks = plan.tasks.length || 1 or handle zero by setting progress to
0/100 explicitly), wrap the await executeFn(...) call in try/catch/finally so
exceptions setExecuting(false) in finally and setExecutionProgress
appropriately, increment completedCount and call
setExecutionProgress((completedCount / totalTasks) * 100) only when totalTasks >
0 (or compute denominator safely), and await loadData() after setting state (use
await loadData()) inside the try/finally to avoid races; update references:
executeFn (derived from executePlanDryRun/executePlan), completedCount,
totalTasks, setExecutionProgress, setState, setExecuting, and loadData.
In `@packages/tui/src/screens/Team.tsx`:
- Around line 70-83: The keyboard handler currently routes 'i' unconditionally
to handleInitialize(), conflicting with the detail-pane's "Install" action;
change the useKeyboard switch so when key.name === 'i' you first check
showDetail() and, if true, call the detail-pane install handler (e.g.
handleInstall or onInstall) instead of handleInitialize(), otherwise call
handleInitialize(); similarly, ensure other detail-specific keys are routed to
the detail-pane handlers while details are open and the global initializer is
gated when showDetail() is true so detail actions become reachable.
- Around line 45-49: The loadData function currently awaits loadTeamConfig
without handling rejections, which can leave the UI stuck in loading; wrap the
await loadTeamConfig() call in a try/catch and ensure you always clear loading
(setState(..., loading: false)) in both success and error paths, setting error
on failure (e.g., setState(prev => ({ ...prev, loading: false, error:
err.message || String(err) })) ), and on success merge the loaded result while
setting loading: false and error: null so setState(result) is not left as the
only update.
In `@packages/tui/src/screens/Translate.tsx`:
- Around line 29-33: readSkillContent currently calls join(skillPath,
'SKILL.md') without guarding skillPath which will throw if undefined (e.g. when
selectedSkill().path is missing); update readSkillContent to first check that
skillPath is a non-empty string and if not return a clear message like "No skill
path available" (or undefined per your UX) and only call join/readFileSync when
skillPath is valid; apply the same guard/behavior to the related code block
around lines 104-123 that also uses path.join with the skill path.
In `@packages/tui/src/theme/animations.ts`:
- Around line 256-265: springCurve currently returns 1 at progress=0 because the
oscillation term is zero; modify springCurve(progress, tension, friction) to
multiply the computed curve by progress so the value is 0 at start and still 1
at end (e.g., return progress * (1 - damping * oscillation * (1 - progress)));
keep using the existing oscillation and damping calculations and optionally
clamp the final value to [0,1] if needed.
🟡 Minor comments (15)
packages/tui/src/components/AgentGrid.tsx-13-21 (1)
13-21:⚠️ Potential issue | 🟡 MinorClamp
maxVisibleand derivehiddenCountfrom actual visibility.Negative
maxVisibleproduces surprising slices and inflated counts. Clamping and basinghiddenCountonvisibleAgents().lengthavoids inconsistent UI states.🔧 Suggested defensive tweak
- const maxVisible = () => props.maxVisible ?? 12; + const maxVisible = () => Math.max(0, props.maxVisible ?? 12); const showStatus = () => props.showStatus ?? true; const detectedAgents = () => props.detectedAgents ?? []; const columns = () => Math.max(1, props.columns ?? 4); const allAgents = Object.entries(AGENT_LOGOS); const visibleAgents = () => allAgents.slice(0, maxVisible()); - const hiddenCount = () => allAgents.length - maxVisible(); + const hiddenCount = () => Math.max(0, allAgents.length - visibleAgents().length);packages/tui/src/screens/Plan.tsx-50-64 (1)
50-64:⚠️ Potential issue | 🟡 MinorMissing try/catch in
handleValidate.If
loadPlanorvalidatePlanthrows,validatingwill remaintrueand the UI will be stuck showing the validation spinner.🛡️ Proposed fix
const handleValidate = async () => { const plans = state().plans; if (plans.length === 0) return; const planItem = plans[selectedIndex()]; if (!planItem) return; setValidating(true); - const plan = await loadPlan(planItem.path); - if (plan) { - const validation = await validatePlan(plan); - setState((s) => ({ ...s, current: plan, validation })); + try { + const plan = await loadPlan(planItem.path); + if (plan) { + const validation = await validatePlan(plan); + setState((s) => ({ ...s, current: plan, validation })); + } + } catch (err) { + setState((s) => ({ + ...s, + error: err instanceof Error ? err.message : 'Validation failed', + })); + } finally { + setValidating(false); } - setValidating(false); };packages/tui/src/screens/Plan.tsx-44-48 (1)
44-48:⚠️ Potential issue | 🟡 MinorMissing error handling in
loadData.If
loadPlansList()throws an exception, the error won't be caught andloadingwill remaintrue, leaving the UI in a stuck state. Wrap the call in try/catch to handle unexpected failures.🛡️ Proposed fix
const loadData = async () => { setState((s) => ({ ...s, loading: true, error: null })); - const result = await loadPlansList(); - setState(result); + try { + const result = await loadPlansList(); + setState(result); + } catch (err) { + setState((s) => ({ + ...s, + loading: false, + error: err instanceof Error ? err.message : 'Failed to load plans', + })); + } };packages/tui/src/components/StatsCard.tsx-57-72 (1)
57-72:⚠️ Potential issue | 🟡 MinorPrevent transient “undefined” values when items length changes.
If
props.itemsgrows,displayValues()[index()]can beundefineduntil the reset effect runs, leading to a brief “undefined” render. A small fallback avoids the flicker.🛠️ Suggested fix
- {(item, index) => { - const fg = () => (item.color ? terminalColors[item.color] : terminalColors.text); + {(item, index) => { + const fg = () => (item.color ? terminalColors[item.color] : terminalColors.text); + const value = () => + displayValues()[index()] ?? (animated() ? 0 : item.value); return ( <box flexDirection="column" alignItems="center" padding={2} borderStyle="single" borderColor={terminalColors.border} > <text fg={fg()}> - <b>{String(displayValues()[index()])}</b> + <b>{String(value())}</b> </text> <text fg={terminalColors.textMuted}>{item.label}</text> </box> ); }}packages/tui/src/screens/Installed.tsx-106-109 (1)
106-109:⚠️ Potential issue | 🟡 MinorShortcut label says “d disable” but handler toggles.
dcallshandleToggle, so it enables disabled items too. Either change the label to “toggle” or makeddisable-only to avoid misleading UX.Suggested label fix
- ? 'j/k nav enter toggle d disable esc back' - : 'j/k navigate enter toggle d disable esc back'; + ? 'j/k nav enter toggle d toggle esc back' + : 'j/k navigate enter toggle d toggle esc back';packages/tui/src/screens/Installed.tsx-165-172 (1)
165-172:⚠️ Potential issue | 🟡 MinorAdd handlers for 'b' and 'm' keys or adjust the prompt text.
The UI prompts "Press 'b' to browse or 'm' for marketplace", but theuseKeyboardhook in this screen doesn't handle these keys. Either implement navigation handlers for these keys (following the existingprops.onNavigatepattern used for 'escape'), or remove the suggestion from the empty-state message.packages/tui/src/screens/Recommend.tsx-107-111 (1)
107-111:⚠️ Potential issue | 🟡 MinorGuard against stale selection after refresh. If the recommendations list shrinks,
selectedIndex()can point past the end, leavingselectedRec()undefined and the detail view non-responsive. Clamp or guard the index.✅ Minimal guard
const selectedRec = () => { const recs = state().recommendations; if (recs.length === 0) return null; - return recs[selectedIndex()]; + const idx = selectedIndex(); + if (idx < 0 || idx >= recs.length) return null; + return recs[idx]; };packages/tui/src/screens/Recommend.tsx-41-58 (1)
41-58:⚠️ Potential issue | 🟡 MinorHandle promise rejections to prevent perpetual loading state. If
analyzeProject()orgetRecommendations()throws, the error goes unhandled and leaves the screen stuck in a loading/analyzing state. Wrap the async operations in a try-catch to capture errors and update state accordingly:🔧 Suggested fix
createEffect(() => { + void loadData(); }); const loadData = async () => { setState((s) => ({ ...s, loading: true, analyzing: true, error: null })); + setProjectInfo(null); - const analysis = await analyzeProject(); - if (analysis) { - setProjectInfo({ - languages: analysis.languages, - frameworks: analysis.frameworks, - }); - } - - const result = await getRecommendations(); - setState(result); + try { + const analysis = await analyzeProject(); + if (analysis) { + setProjectInfo({ + languages: analysis.languages, + frameworks: analysis.frameworks, + }); + } + + const result = await getRecommendations(); + setState(result); + } catch (err) { + const message = + err instanceof Error ? err.message : 'Failed to load recommendations'; + setState((s) => ({ ...s, loading: false, analyzing: false, error: message })); + } };packages/tui/src/screens/Execute.tsx-52-62 (1)
52-62:⚠️ Potential issue | 🟡 MinorMissing error handling in
loadData.If
getAgentAvailability()throws, the loading state will remaintrueindefinitely with no error feedback to the user.🛡️ Proposed fix
const loadData = async () => { setLoading(true); - - const skillsWithDetails = loadSkillsWithDetails(); - setSkills(skillsWithDetails); - - const agentAvailability = await getAgentAvailability(); - setAgents(agentAvailability); - - setLoading(false); + try { + const skillsWithDetails = loadSkillsWithDetails(); + setSkills(skillsWithDetails); + + const agentAvailability = await getAgentAvailability(); + setAgents(agentAvailability); + } catch (err) { + console.error('Failed to load data:', err); + } finally { + setLoading(false); + } };packages/tui/src/screens/Workflow.tsx-44-48 (1)
44-48:⚠️ Potential issue | 🟡 MinorMissing error handling in
loadData.The
loadWorkflowsListcall could throw, but errors aren't caught. The state would remain withloading: trueif an exception occurs beforesetState(result)is called.🛡️ Proposed fix to add error handling
const loadData = async () => { setState((s) => ({ ...s, loading: true, error: null })); - const result = await loadWorkflowsList(); - setState(result); + try { + const result = await loadWorkflowsList(); + setState(result); + } catch (err) { + setState((s) => ({ + ...s, + loading: false, + error: err instanceof Error ? err.message : 'Failed to load workflows', + })); + } };packages/tui/src/screens/Execute.tsx-104-112 (1)
104-112:⚠️ Potential issue | 🟡 MinorNavigation allows selecting items beyond visible range.
handleKeyNavusesskills().lengthfor bounds, but the UI only rendersskills().slice(0, 8). Users can navigate to indices 8+ which won't be visible, causing confusion.🛡️ Proposed fix to cap navigation to visible items
const handleKeyNav = (delta: number) => { if (focusedPane() === 'skills') { - const max = skills().length - 1; + const max = Math.min(skills().length, 8) - 1; setSelectedSkillIndex((prev) => Math.max(0, Math.min(prev + delta, max))); } else { - const max = agents().length - 1; + const max = Math.min(agents().length, 8) - 1; setSelectedAgentIndex((prev) => Math.max(0, Math.min(prev + delta, max))); } };packages/tui/src/screens/Workflow.tsx-68-71 (1)
68-71:⚠️ Potential issue | 🟡 MinorEdge case: navigation on empty list yields -1 max.
When
state().workflows.lengthis 0,maxbecomes -1, andMath.min(prev + delta, -1)could set index to -1. WhileselectedWorkflow()handles empty lists, a negative index could cause unexpected behavior.🛡️ Proposed fix
const handleKeyNav = (delta: number) => { - const max = state().workflows.length - 1; - setSelectedIndex((prev) => Math.max(0, Math.min(prev + delta, max))); + const len = state().workflows.length; + if (len === 0) return; + setSelectedIndex((prev) => Math.max(0, Math.min(prev + delta, len - 1))); };packages/tui/src/screens/Marketplace.tsx-33-35 (1)
33-35:⚠️ Potential issue | 🟡 MinorSearch filtering is unreachable without an input or key path.
searchQuerydrives filtering, but there’s no UI or keyboard path to update it, so users can’t search and the Esc clearing behavior becomes confusing. Wire aSearchInput(or a search-mode key handler) to updatesearchQuery, or remove the unused state.Also applies to: 117-133, 207-221
packages/tui/src/screens/Translate.tsx-154-174 (1)
154-174:⚠️ Potential issue | 🟡 Minor“Browse” action key isn’t handled.
The empty state advertises key
b, butuseKeyboardnever handles it, so users can’t leave from that state. Add a handler.✅ Suggested fix
- else if (key.name === 'r') loadData(); + else if (key.name === 'b') props.onNavigate('browse'); + else if (key.name === 'r') loadData();Also applies to: 271-279
packages/tui/src/screens/Sync.tsx-46-77 (1)
46-77:⚠️ Potential issue | 🟡 MinorReset sync status/message on reload.
Reloading data leaves the previous success/error banner visible because
syncStatusandsyncMessagearen’t cleared. Reset them when startingloadData().✅ Suggested fix
const loadData = async () => { setLoading(true); setError(null); + setSyncStatus('idle'); + setSyncMessage('');
🧹 Nitpick comments (13)
packages/tui/src/utils/helpers.ts (1)
67-87: MemoizegetVersion()to avoid repeated sync disk reads.If
getVersion()can be called more than once per run, caching avoids redundant file I/O.♻️ Suggested memoization
+let cachedVersion: string | null = null; + export function getVersion(): string { + if (cachedVersion) return cachedVersion; try { const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); const paths = [ join(__dirname, '..', 'package.json'), join(__dirname, '..', '..', 'package.json'), join(__dirname, '..', '..', '..', 'package.json'), ]; for (const packagePath of paths) { try { const packageJson = JSON.parse(readFileSync(packagePath, 'utf-8')); if (packageJson.name === '@skillkit/tui' || packageJson.name === 'skillkit') { - return packageJson.version || '1.8.0'; + cachedVersion = packageJson.version || '1.8.0'; + return cachedVersion; } } catch {} } - return '1.8.0'; + cachedVersion = '1.8.0'; + return cachedVersion; } catch { - return '1.8.0'; + cachedVersion = '1.8.0'; + return cachedVersion; } }packages/tui/src/screens/Mesh.tsx (1)
13-17: Consider extracting static mock data or adding a TODO for service integration.The
hostsarray is hardcoded static data. Given the PR mentions "a service layer for integration with@skillkit/core", this data should eventually come from the service layer. Consider either:
- Adding a TODO comment indicating this is placeholder data.
- Moving mock data to a separate file for easier replacement during integration.
packages/tui/src/screens/Plan.tsx (3)
20-24: Unusedcolsandrowsprops.The
colsandrowsproperties are declared inPlanPropsbut never used in the component. Consider removing them if they're no longer needed after the migration.🧹 Proposed cleanup
interface PlanProps { onNavigate: (screen: Screen) => void; - cols?: number; - rows?: number; }
102-110: No guard against concurrent execution from rapid key presses.Users pressing
v,x, ordwhile an operation is already in progress could trigger multiple concurrent executions. Consider checkingexecuting()andvalidating()before starting new operations.♻️ Proposed fix
useKeyboard((key: { name?: string }) => { if (key.name === 'j' || key.name === 'down') handleKeyNav(1); else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); - else if (key.name === 'v') handleValidate(); - else if (key.name === 'x') handleExecute(false); - else if (key.name === 'd') handleExecute(true); - else if (key.name === 'r') loadData(); + else if (key.name === 'v' && !validating() && !executing()) handleValidate(); + else if (key.name === 'x' && !executing() && !validating()) handleExecute(false); + else if (key.name === 'd' && !executing() && !validating()) handleExecute(true); + else if (key.name === 'r' && !state().loading) loadData(); else if (key.name === 'escape') props.onNavigate('home'); });
138-142: UnusedselectedPlanfunction.This helper is defined but never called in the component. Remove it if not needed, or use it to consolidate the repeated
plans[selectedIndex()]pattern inhandleValidateandhandleExecute.♻️ Option 1: Remove unused code
- const selectedPlan = () => { - const plans = state().plans; - if (plans.length === 0) return null; - return plans[selectedIndex()]; - };♻️ Option 2: Use the helper in handlers
const handleValidate = async () => { - const plans = state().plans; - if (plans.length === 0) return; - - const planItem = plans[selectedIndex()]; + const planItem = selectedPlan(); if (!planItem) return; // ... };packages/tui/src/screens/Methodology.tsx (1)
1-1: ReplacecreateEffectwithonMountfor one-time initialization.This effect has no reactive dependencies and is intended to run once on component mount;
onMountis the semantic fit and prevents accidental re-runs if dependencies are introduced later.♻️ Proposed refactor
-import { createSignal, createEffect, Show, For } from 'solid-js'; +import { createSignal, onMount, Show, For } from 'solid-js'; @@ - createEffect(() => { - loadData(); - }); + onMount(() => { + void loadData(); + });packages/tui/src/screens/Context.tsx (1)
1-1: PreferonMountfor the one‑time load (and avoid TDZ risk)Line 36–38 uses
createEffectwith no reactive dependencies;onMountis the Solid idiom for one-time work and avoids any chance ofloadDatabeing invoked before initialization (declared at Line 40).♻️ Suggested adjustment
-import { createSignal, createEffect, onCleanup, Show, For } from 'solid-js'; +import { createSignal, onCleanup, Show, For, onMount } from 'solid-js'; @@ - createEffect(() => { - loadData(); - }); + onMount(() => { + void loadData(); + });Also applies to: 36-38
packages/tui/src/state/skills.ts (1)
131-145: Consider consistent error handling withloadSkills.The
loadSkillsfunction (lines 47-77) returns structured error state with{ skills: [], loading: false, error: string }, whileloadSkillsWithDetailssilently returns an empty array on failure. This inconsistency could make debugging harder for consumers who can't distinguish between "no skills found" and "failed to load skills."If this silent failure is intentional for the translation use case, consider documenting that behavior explicitly in the JSDoc.
packages/tui/tsup.config.ts (1)
10-11: Fragile path resolution for Solid.js runtime.The regex replacement assumes solid-js's internal dist structure (
dist/server.cjs→dist/solid.js). This is brittle because:
- Package internals may change between versions
- The replacement silently produces incorrect paths if the structure changes
Consider adding validation or using a more robust approach:
🛠️ Proposed validation
const solidPath = require.resolve('solid-js').replace(/dist\/server\.(cjs|js)$/, 'dist/solid.js'); const solidWebPath = require.resolve('solid-js/web').replace(/dist\/server\.(cjs|js)$/, 'dist/web.js'); + +// Validate the resolved paths exist +import { existsSync } from 'fs'; +if (!existsSync(solidPath)) { + throw new Error(`solid-js runtime not found at expected path: ${solidPath}`); +} +if (!existsSync(solidWebPath)) { + throw new Error(`solid-js/web runtime not found at expected path: ${solidWebPath}`); +}packages/tui/src/screens/Execute.tsx (1)
20-26: Synchronous file reads could block the event loop.
readSkillContentuses synchronousexistsSyncandreadFileSync. For large SKILL.md files, this could cause UI jank. Consider using async alternatives or note that skill files are typically small.♻️ Async alternative (if needed)
+import { existsSync } from 'node:fs'; +import { readFile } from 'node:fs/promises'; +import { join } from 'node:path'; -function readSkillContent(skillPath: string): string | undefined { +async function readSkillContent(skillPath: string): Promise<string | undefined> { const skillMdPath = join(skillPath, 'SKILL.md'); if (existsSync(skillMdPath)) { - return readFileSync(skillMdPath, 'utf-8'); + return readFile(skillMdPath, 'utf-8'); } return undefined; }packages/tui/src/components/Splash.tsx (2)
102-114: Potential issue:clearIntervalinsidesetProgresscallback.The
intervalvariable is captured by thesetProgresscallback closure. If the callback runs after the effect's cleanup has already cleared the interval, this is harmless. However, callingclearInterval(interval)from within the interval callback while also relying ononCleanupcreates unclear ownership. Consider using a flag or ref instead.♻️ Cleaner approach using a completion flag
createEffect(() => { + let completed = false; const interval = setInterval(() => { + if (completed) return; setProgress((p) => { const newProgress = p + (Math.random() * 3 + 1); if (newProgress >= 100) { - clearInterval(interval); + completed = true; return 100; } return newProgress; }); }, 50); - onCleanup(() => clearInterval(interval)); + onCleanup(() => { + completed = true; + clearInterval(interval); + }); });
72-74: Terminal dimensions not reactive to resize.
colsandrowsare read once at component creation fromprocess.stdout. If the terminal is resized during the splash screen, the layout won't adapt. This may be acceptable for a short-lived splash, but worth noting.packages/tui/src/screens/Message.tsx (1)
12-19: Consider moving static data outside component.The
messagesarray andunreadCountare defined inside the component. In SolidJS, components run once (not on every render like React), so this isn't a performance issue. However, moving static mock data outside improves clarity about what's reactive vs. constant.♻️ Move static data outside component
+const MOCK_MESSAGES = [ + { id: '1', from: 'claude@laptop', subject: 'Code review completed', time: '2 min ago', unread: true }, + { id: '2', from: 'cursor@workstation', subject: 'Build artifacts ready', time: '15 min ago', unread: true }, + { id: '3', from: 'codex@server', subject: 'Test results', time: '1 hour ago', unread: false }, +]; export function Message(props: MessageProps) { - const messages = [ - { id: '1', from: 'claude@laptop', subject: 'Code review completed', time: '2 min ago', unread: true }, - { id: '2', from: 'cursor@workstation', subject: 'Build artifacts ready', time: '15 min ago', unread: true }, - { id: '3', from: 'codex@server', subject: 'Test results', time: '1 hour ago', unread: false }, - ]; + const messages = MOCK_MESSAGES;
packages/tui/src/screens/Plan.tsx
Outdated
| const executeFn = dryRun ? executePlanDryRun : executePlan; | ||
| let completedCount = 0; | ||
| const totalTasks = plan.tasks.length; | ||
| const result = await executeFn(plan, undefined, (event) => { | ||
| if (event === 'plan:task_completed') { | ||
| completedCount++; | ||
| setExecutionProgress((completedCount / totalTasks) * 100); | ||
| } | ||
| }); | ||
|
|
||
| setState((s) => ({ ...s, execution: result })); | ||
| setExecuting(false); | ||
| loadData(); | ||
| }; |
There was a problem hiding this comment.
Division by zero when plan has no tasks; missing error handling.
- If
plan.tasks.lengthis 0, line 88 divides by zero, producingInfinityfor progress. - If
executeFnthrows,executingremainstrue, leaving the UI stuck. loadData()at line 94 is not awaited, which could cause state race conditions.
🛡️ Proposed fix
const executeFn = dryRun ? executePlanDryRun : executePlan;
let completedCount = 0;
const totalTasks = plan.tasks.length;
- const result = await executeFn(plan, undefined, (event) => {
- if (event === 'plan:task_completed') {
- completedCount++;
- setExecutionProgress((completedCount / totalTasks) * 100);
- }
- });
-
- setState((s) => ({ ...s, execution: result }));
- setExecuting(false);
- loadData();
+
+ try {
+ const result = await executeFn(plan, undefined, (event) => {
+ if (event === 'plan:task_completed') {
+ completedCount++;
+ setExecutionProgress(totalTasks > 0 ? (completedCount / totalTasks) * 100 : 100);
+ }
+ });
+ setState((s) => ({ ...s, execution: result }));
+ } catch (err) {
+ setState((s) => ({
+ ...s,
+ error: err instanceof Error ? err.message : 'Execution failed',
+ }));
+ } finally {
+ setExecuting(false);
+ await loadData();
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const executeFn = dryRun ? executePlanDryRun : executePlan; | |
| let completedCount = 0; | |
| const totalTasks = plan.tasks.length; | |
| const result = await executeFn(plan, undefined, (event) => { | |
| if (event === 'plan:task_completed') { | |
| completedCount++; | |
| setExecutionProgress((completedCount / totalTasks) * 100); | |
| } | |
| }); | |
| setState((s) => ({ ...s, execution: result })); | |
| setExecuting(false); | |
| loadData(); | |
| }; | |
| const executeFn = dryRun ? executePlanDryRun : executePlan; | |
| let completedCount = 0; | |
| const totalTasks = plan.tasks.length; | |
| try { | |
| const result = await executeFn(plan, undefined, (event) => { | |
| if (event === 'plan:task_completed') { | |
| completedCount++; | |
| setExecutionProgress(totalTasks > 0 ? (completedCount / totalTasks) * 100 : 100); | |
| } | |
| }); | |
| setState((s) => ({ ...s, execution: result })); | |
| } catch (err) { | |
| setState((s) => ({ | |
| ...s, | |
| error: err instanceof Error ? err.message : 'Execution failed', | |
| })); | |
| } finally { | |
| setExecuting(false); | |
| await loadData(); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Plan.tsx` around lines 82 - 95, The execution block
can divide by zero, leak state if executeFn throws, and call loadData without
awaiting; fix by guarding totalTasks (use totalTasks = plan.tasks.length || 1 or
handle zero by setting progress to 0/100 explicitly), wrap the await
executeFn(...) call in try/catch/finally so exceptions setExecuting(false) in
finally and setExecutionProgress appropriately, increment completedCount and
call setExecutionProgress((completedCount / totalTasks) * 100) only when
totalTasks > 0 (or compute denominator safely), and await loadData() after
setting state (use await loadData()) inside the try/finally to avoid races;
update references: executeFn (derived from executePlanDryRun/executePlan),
completedCount, totalTasks, setExecutionProgress, setState, setExecuting, and
loadData.
packages/tui/src/screens/Team.tsx
Outdated
| useKeyboard((key: { name?: string }) => { | ||
| if (key.name === 'j' || key.name === 'down') handleKeyNav(1); | ||
| else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); | ||
| else if (key.name === 'tab' || key.name === 'l' || key.name === 'right') { | ||
| setFocusedPane((p) => (p === 'members' ? 'skills' : 'members')); | ||
| } else if (key.name === 'h' || key.name === 'left') { | ||
| setFocusedPane((p) => (p === 'skills' ? 'members' : 'skills')); | ||
| } else if (key.name === 'return') setShowDetail(true); | ||
| else if (key.name === 'i') handleInitialize(); | ||
| else if (key.name === 'r') loadData(); | ||
| else if (key.name === 'escape') { | ||
| if (showDetail()) setShowDetail(false); | ||
| else props.onNavigate('home'); | ||
| } |
There was a problem hiding this comment.
Detail-pane action keys conflict with the global i initializer.
The detail pane advertises i for “Install,” but the handler always routes i to initialization. This makes the action unreachable and can trigger the wrong operation. Gate the initializer while details are open and wire the detail actions.
✅ Suggested fix (prevents the collision)
- else if (key.name === 'i') handleInitialize();
+ else if (key.name === 'i' && !showDetail()) handleInitialize();Also applies to: 319-338
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Team.tsx` around lines 70 - 83, The keyboard handler
currently routes 'i' unconditionally to handleInitialize(), conflicting with the
detail-pane's "Install" action; change the useKeyboard switch so when key.name
=== 'i' you first check showDetail() and, if true, call the detail-pane install
handler (e.g. handleInstall or onInstall) instead of handleInitialize(),
otherwise call handleInitialize(); similarly, ensure other detail-specific keys
are routed to the detail-pane handlers while details are open and the global
initializer is gated when showDetail() is true so detail actions become
reachable.
| export function springCurve( | ||
| progress: number, | ||
| tension: number = 0.5, | ||
| friction: number = 0.5 | ||
| ): number { | ||
| const p = progress; | ||
| const oscillation = Math.sin(p * Math.PI * (1 + tension * 4)); | ||
| const damping = Math.exp(-p * friction * 6); | ||
| return 1 - damping * oscillation * (1 - p); | ||
| } |
There was a problem hiding this comment.
Spring curve returns incorrect value at progress=0.
At progress=0: oscillation = sin(0) = 0, so the function returns 1 - damping * 0 * (1-p) = 1. Animation curves should return 0 at the start and 1 at the end, but this returns 1 at start.
🔧 Proposed fix using a standard spring oscillation formula
export function springCurve(
progress: number,
tension: number = 0.5,
friction: number = 0.5
): number {
const p = progress;
- const oscillation = Math.sin(p * Math.PI * (1 + tension * 4));
- const damping = Math.exp(-p * friction * 6);
- return 1 - damping * oscillation * (1 - p);
+ const frequency = 1 + tension * 4;
+ const decay = friction * 6;
+ // Spring oscillation that starts at 0, overshoots 1, and settles to 1
+ return 1 - Math.exp(-p * decay) * Math.cos(p * Math.PI * frequency);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function springCurve( | |
| progress: number, | |
| tension: number = 0.5, | |
| friction: number = 0.5 | |
| ): number { | |
| const p = progress; | |
| const oscillation = Math.sin(p * Math.PI * (1 + tension * 4)); | |
| const damping = Math.exp(-p * friction * 6); | |
| return 1 - damping * oscillation * (1 - p); | |
| } | |
| export function springCurve( | |
| progress: number, | |
| tension: number = 0.5, | |
| friction: number = 0.5 | |
| ): number { | |
| const p = progress; | |
| const frequency = 1 + tension * 4; | |
| const decay = friction * 6; | |
| // Spring oscillation that starts at 0, overshoots 1, and settles to 1 | |
| return 1 - Math.exp(-p * decay) * Math.cos(p * Math.PI * frequency); | |
| } |
🤖 Prompt for AI Agents
In `@packages/tui/src/theme/animations.ts` around lines 256 - 265, springCurve
currently returns 1 at progress=0 because the oscillation term is zero; modify
springCurve(progress, tension, friction) to multiply the computed curve by
progress so the value is 0 at start and still 1 at end (e.g., return progress *
(1 - damping * oscillation * (1 - progress))); keep using the existing
oscillation and damping calculations and optionally clamp the final value to
[0,1] if needed.
Critical fixes: - Remove non-existent module exports from index.tsx (context, themes, ui, hooks, services) - Simplify App.tsx to remove missing context provider imports Major fixes: - ProgressBar: Add defensive guards for width/progress (NaN, Infinity, negative) - Browse: Fix filtered index loading wrong repository - Context: Add section navigation (h/l keys) and async error handling - Methodology: Add error handling with try/catch/finally for all async ops - Plan: Fix division by zero when plan has no tasks, add error handling - Team: Add error handling for loadTeamConfig and handleInitialize - Translate: Guard against missing skill.path before read operations
- Memory: Add error handling for loadData, handleDelete, handleClearAll, handleInitialize - Team: Fix key conflict by blocking global keys when detail pane is open - animations: Fix springCurve to return 0 at progress=0 and 1 at progress=1
Required for TypeScript to recognize OpenTUI JSX elements (box, text, span) with their custom props (fg, bg, flexDirection, etc.)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/tui/src/screens/Context.tsx`:
- Around line 243-257: Guard access to the optional patterns field on ctx before
calling Object.keys/Object.entries: update the conditional and iteration in the
Patterns block (where currentSection(), ctx(), and patterns are used) to safely
handle undefined by using optional chaining or a default empty object (e.g., use
ctx()?.patterns && Object.keys(ctx()!.patterns).length > 0 or
Object.entries(ctx()?.patterns ?? {})); ensure the Show when check and the For
iterator both use the safe expression so Object.keys/Object.entries are never
called on undefined.
In `@packages/tui/src/screens/Team.tsx`:
- Around line 299-328: The skills list rendering uses
state().sharedSkills.slice(0, 8) but navigation (selectedSkillIndex in
handleKeyNav) can point outside that slice; implement windowing so the rendered
slice centers on the selection: compute visibleCount = 8, start = Math.max(0,
Math.min(selectedSkillIndex() - Math.floor(visibleCount/2),
state().sharedSkills.length - visibleCount)), then render
state().sharedSkills.slice(start, start + visibleCount) and adjust the selection
check to compare idx() + start === selectedSkillIndex(); update any references
in the For loop and selection predicate and ensure edge cases when
sharedSkills.length < visibleCount still work.
In `@packages/tui/src/screens/Translate.tsx`:
- Around line 127-134: handleTranslate can be re-entered by rapid key presses;
add a re-entry guard by introducing and checking a boolean state (e.g.,
isTranslating) at the top of handleTranslate, return early if it's true, then
set it to true immediately when starting the translation and set it back to
false in a finally block after the async work (also ensure errors still call
setError). Reference handleTranslate, selectedSkill, targetAgent and setError
when adding/using the guard so you prevent concurrent executions and always
clear the flag on completion or error.
🧹 Nitpick comments (7)
packages/tui/src/screens/Browse.tsx (1)
41-41: Remove unusedcontentWidthvariable.
contentWidthis defined but never used in the component. Consider removing this dead code.const cols = () => props.cols ?? 80; const rows = () => props.rows ?? 24; - const contentWidth = () => Math.max(1, Math.min(cols() - 4, 60));packages/tui/src/screens/Team.tsx (2)
10-17: Unused import:shareSkillis never called.The
shareSkillfunction is imported but not used anywhere in the component. Consider removing it or wiring it to the "Share Skill" action displayed in the member detail pane (line 341).🧹 Remove unused import or wire it to an action
If not needed, remove:
import { loadTeamConfig, initializeTeam, - shareSkill, type TeamServiceState, type TeamMemberDisplay, type SharedSkillDisplay, } from '../services/team.service.js';Or wire it to the 's' key handler when detail pane is open (see related comment on keyboard handling).
45-57: Ensureloading: falseis set on successful load.In the catch block,
loading: falseis explicitly set, but in the success path the entire state is replaced byresult. IfloadTeamConfig()doesn't returnloading: falsein its result, the UI may remain stuck in loading state.Consider explicitly setting
loading: falseon success for consistency:✅ Explicit loading state on success
try { const result = await loadTeamConfig(); - setState(result); + setState({ ...result, loading: false, error: null }); } catch (err) {packages/tui/src/screens/Methodology.tsx (2)
56-123: Re-entry guards are partial—consider full mutual exclusion.The error handling with try/catch/finally is well implemented. However, the re-entry guards only check their own flag rather than both:
handleInstall/handleUninstallcheckinstalling()but notsyncing()handleSynccheckssyncing()but notinstalling()This allows overlapping operations (e.g., starting install while sync is running) which could cause state inconsistencies.
♻️ Suggested improvement for full mutual exclusion
const handleInstall = async () => { const packs = state().packs; - if (packs.length === 0 || installing()) return; + if (packs.length === 0 || installing() || syncing()) return;const handleUninstall = async () => { const packs = state().packs; - if (packs.length === 0 || installing()) return; + if (packs.length === 0 || installing() || syncing()) return;const handleSync = async () => { const packs = state().packs; - if (packs.length === 0 || syncing()) return; + if (packs.length === 0 || installing() || syncing()) return;
130-142: Global keyboard handler still active when detail pane is open.The PR objectives mention fixing key conflicts by blocking global keys when the detail pane is open, but this handler processes
j/k/i/u/s/rregardless ofshowDetail()state. Onlyescapehas conditional handling.Consider guarding navigation and action keys when the detail pane is visible to prevent unintended operations.
♻️ Suggested guard for detail pane
useKeyboard((key: { name?: string }) => { + // Block most keys when detail pane is open + if (showDetail()) { + if (key.name === 'escape') setShowDetail(false); + else if (key.name === 'i') handleInstall(); + else if (key.name === 'u') handleUninstall(); + else if (key.name === 's') handleSync(); + return; + } + if (key.name === 'j' || key.name === 'down') handleKeyNav(1); else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); else if (key.name === 'return') setShowDetail(true); else if (key.name === 'i') handleInstall(); else if (key.name === 'u') handleUninstall(); else if (key.name === 's') handleSync(); else if (key.name === 'r') loadData(); - else if (key.name === 'escape') { - if (showDetail()) setShowDetail(false); - else props.onNavigate('home'); - } + else if (key.name === 'escape') props.onNavigate('home'); });packages/tui/src/screens/Memory.tsx (1)
1-52: PreferonMountfor one-time data loading.
createEffectimplies reactive dependencies and may rerun if signals are later read;onMountmakes the intent explicit.♻️ Suggested change
-import { createSignal, createEffect, Show, For } from 'solid-js'; +import { createSignal, Show, For, onMount } from 'solid-js'; - createEffect(() => { - loadData(); - }); + onMount(() => { + void loadData(); + });packages/tui/src/screens/Context.tsx (1)
1-3: PreferonMountfor one‑time data load.
createEffectwithout dependencies works, butonMount(loadData)is clearer for a one-time init and avoids ambiguity about when the callback runs relative to initialization.♻️ Suggested refactor
-import { createSignal, createEffect, Show, For } from 'solid-js'; +import { createSignal, onMount, Show, For } from 'solid-js'; @@ - createEffect(() => { - loadData(); - }); + onMount(loadData);Also applies to: 51-53
| <Show when={currentSection() === 'Patterns'}> | ||
| <box flexDirection="column"> | ||
| <Show | ||
| when={Object.keys(ctx()!.patterns).length > 0} | ||
| fallback={<EmptyState title="No patterns detected" compact />} | ||
| > | ||
| <For each={Object.entries(ctx()!.patterns)}> | ||
| {([key, value]) => ( | ||
| <box flexDirection="row" marginBottom={1}> | ||
| <text fg={terminalColors.textMuted} width={18}>{key}:</text> | ||
| <text fg={terminalColors.text}>{String(value)}</text> | ||
| </box> | ||
| )} | ||
| </For> | ||
| </Show> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ContextServiceState and its patterns field definition
rg "ContextServiceState|patterns" packages/tui/src/ -A 5 -B 1 -g "*.ts*" | head -100Repository: rohitg00/skillkit
Length of output: 1854
🏁 Script executed:
#!/bin/bash
# Find the type definition of ctx() to understand the patterns field
rg "interface.*Context|type.*Context" packages/tui/src/ -A 10 -g "*.ts*" | head -150Repository: rohitg00/skillkit
Length of output: 916
🏁 Script executed:
#!/bin/bash
# Search for where ctx is defined or used to understand what it returns
rg "ctx\(\)" packages/tui/src/screens/Context.tsx -B 2 -A 2 | head -50Repository: rohitg00/skillkit
Length of output: 2410
🏁 Script executed:
#!/bin/bash
# Find the context.service file
find packages/tui/src -name "*context.service*" -type fRepository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Read the context.service file to see ContextServiceState type definition
cat packages/tui/src/services/context.service.ts | head -100Repository: rohitg00/skillkit
Length of output: 137
🏁 Script executed:
#!/bin/bash
# List files in the services directory
ls -la packages/tui/src/services/Repository: rohitg00/skillkit
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Search for ContextServiceState type definition in all files
rg "type ContextServiceState|interface ContextServiceState" -A 20 packages/tui/src/Repository: rohitg00/skillkit
Length of output: 1425
🏁 Script executed:
#!/bin/bash
# Search entire repository for ContextServiceState definition
rg "export.*type ContextServiceState|export.*interface ContextServiceState" -A 15Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# List the entire packages directory structure to understand layout
find packages -type f -name "*.ts" -o -name "*.tsx" | grep -i context | head -20Repository: rohitg00/skillkit
Length of output: 503
🏁 Script executed:
#!/bin/bash
# Read the types file where ContextServiceState might be defined
cat packages/core/src/context/types.tsRepository: rohitg00/skillkit
Length of output: 4885
🏁 Script executed:
#!/bin/bash
# Search for files named context.service or ContextServiceState export in packages/tui
find packages/tui -type f -name "*.ts" -o -name "*.tsx" | xargs rg "export.*ContextServiceState" 2>/dev/nullRepository: rohitg00/skillkit
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find context.service file in packages/tui
find packages/tui -type f -name "*context*service*"Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search broadly for ContextServiceState
rg "ContextServiceState" -A 10 --type ts --type tsxRepository: rohitg00/skillkit
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for ContextServiceState without file type restrictions
rg "ContextServiceState" -A 10 packages/Repository: rohitg00/skillkit
Length of output: 1425
🏁 Script executed:
#!/bin/bash
# Look at the full Context.tsx file to understand the structure
cat packages/tui/src/screens/Context.tsx | head -150Repository: rohitg00/skillkit
Length of output: 4456
🏁 Script executed:
#!/bin/bash
# Search for where ContextServiceState is exported
rg "export.*type ContextServiceState" packages/Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the entire tui source structure
find packages/tui/src -type f | head -30Repository: rohitg00/skillkit
Length of output: 1199
🏁 Script executed:
#!/bin/bash
# Check if services directory exists elsewhere
find packages -type d -name "services"Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look at the actual directory structure of packages/tui
ls -la packages/tui/src/Repository: rohitg00/skillkit
Length of output: 629
🏁 Script executed:
#!/bin/bash
# Check what's in the state directory
ls -la packages/tui/src/state/Repository: rohitg00/skillkit
Length of output: 533
🏁 Script executed:
#!/bin/bash
# Search for context.service anywhere in the repo
find packages -name "*context.service*" -o -name "*context*service*"Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there's a dist or build directory
find packages/tui -type d \( -name "dist" -o -name "build" -o -name ".output" \) 2>/dev/nullRepository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there's a tsconfig or build configuration
find packages/tui -name "tsconfig.json" -o -name "package.json" | head -5Repository: rohitg00/skillkit
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Look at the package.json of tui package
cat packages/tui/package.json | head -50Repository: rohitg00/skillkit
Length of output: 956
🏁 Script executed:
#!/bin/bash
# Search for the functions imported from context.service
rg "loadProjectContext|analyzeProjectContext|refreshContext|exportContext|getStackTags" packages/tui/src/ -lRepository: rohitg00/skillkit
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for where these functions are defined in the entire repo
rg "export.*loadProjectContext|export.*analyzeProjectContext" -A 3Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if these functions are in the core package
rg "loadProjectContext|analyzeProjectContext|getStackTags" packages/core/src/ -lRepository: rohitg00/skillkit
Length of output: 275
🏁 Script executed:
#!/bin/bash
# Look at the dist output of tui if it exists
find packages/tui -type d -name "dist" -exec ls -la {} \;Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if context.service exists in the root or a common location
find . -name "context.service*" 2>/dev/nullRepository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check git status to see if context.service is a new file in the PR
cd packages/tui/src && git status 2>/dev/null | head -50Repository: rohitg00/skillkit
Length of output: 269
🏁 Script executed:
#!/bin/bash
# List all recently modified files in packages/tui
find packages/tui -type f -newer packages/tui/package.json 2>/dev/null | head -20Repository: rohitg00/skillkit
Length of output: 553
🏁 Script executed:
#!/bin/bash
# Check if there's a .gitignore that might be hiding the services directory
cat packages/tui/.gitignore 2>/dev/null || echo "No .gitignore"Repository: rohitg00/skillkit
Length of output: 74
🏁 Script executed:
#!/bin/bash
# Search for all TypeScript/TSX files and look for any context-related service code
rg "ContextServiceState" --type-list | head -5Repository: rohitg00/skillkit
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Let's look at a larger section of Context.tsx to understand what ctx() should be
sed -n '240,270p' packages/tui/src/screens/Context.tsxRepository: rohitg00/skillkit
Length of output: 1258
🏁 Script executed:
#!/bin/bash
# Search more carefully for ContextServiceState in the entire repo
grep -r "type ContextServiceState\|interface ContextServiceState" packages/Repository: rohitg00/skillkit
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Check if context.service might be created in a build step or monorepo root
find . -type f -name "*context*service*" 2>/dev/nullRepository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look at what ctx() is actually typed as - check the full Context.tsx file
wc -l packages/tui/src/screens/Context.tsx && cat packages/tui/src/screens/Context.tsxRepository: rohitg00/skillkit
Length of output: 10189
🏁 Script executed:
#!/bin/bash
# Confirm patterns is optional by checking how it's used in other parts of the codebase
rg "patterns" packages/core/src/context/types.ts -B 2 -A 2Repository: rohitg00/skillkit
Length of output: 372
🏁 Script executed:
#!/bin/bash
# Check if there are other places in the codebase accessing patterns
rg "\.patterns" packages/ -C 2 | head -40Repository: rohitg00/skillkit
Length of output: 2716
Guard patterns access against undefined.
Object.keys(ctx()!.patterns) will throw if patterns is undefined. The patterns field in ProjectContext is optional, so it must be safely accessed.
Safer access
- <Show
- when={Object.keys(ctx()!.patterns).length > 0}
+ <Show
+ when={Object.keys(ctx()?.patterns ?? {}).length > 0}
fallback={<EmptyState title="No patterns detected" compact />}
>
- <For each={Object.entries(ctx()!.patterns)}>
+ <For each={Object.entries(ctx()?.patterns ?? {})}>🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Context.tsx` around lines 243 - 257, Guard access to
the optional patterns field on ctx before calling Object.keys/Object.entries:
update the conditional and iteration in the Patterns block (where
currentSection(), ctx(), and patterns are used) to safely handle undefined by
using optional chaining or a default empty object (e.g., use ctx()?.patterns &&
Object.keys(ctx()!.patterns).length > 0 or Object.entries(ctx()?.patterns ??
{})); ensure the Show when check and the For iterator both use the safe
expression so Object.keys/Object.entries are never called on undefined.
| <For each={state().sharedSkills.slice(0, 8)}> | ||
| {(skill, idx) => { | ||
| const selected = () => | ||
| focusedPane() === 'skills' && idx() === selectedSkillIndex(); | ||
| return ( | ||
| <box flexDirection="row" marginBottom={1}> | ||
| <text | ||
| fg={selected() ? terminalColors.accent : terminalColors.text} | ||
| width={3} | ||
| > | ||
| {selected() ? '▸ ' : ' '} | ||
| </text> | ||
| <text | ||
| fg={selected() ? terminalColors.accent : terminalColors.text} | ||
| width={18} | ||
| > | ||
| {skill.name} | ||
| </text> | ||
| <text fg={terminalColors.textMuted}> | ||
| by {skill.sharedBy} | ||
| </text> | ||
| </box> | ||
| ); | ||
| }} | ||
| </For> | ||
| <Show when={state().sharedSkills.length > 8}> | ||
| <text fg={terminalColors.textMuted}> | ||
| +{state().sharedSkills.length - 8} more | ||
| </text> | ||
| </Show> |
There was a problem hiding this comment.
Navigation can select skills beyond the visible slice.
The skills list is capped at 8 items with .slice(0, 8), but handleKeyNav allows selectedSkillIndex to range from 0 to sharedSkills.length - 1. If a user navigates to index 8+, no item appears selected in the UI, creating a confusing experience.
Either cap navigation to visible items or implement scrolling/windowing:
Option 1: Cap navigation to visible range
const handleKeyNav = (delta: number) => {
if (focusedPane() === 'members') {
const max = state().members.length - 1;
setSelectedMemberIndex((prev) => Math.max(0, Math.min(prev + delta, max)));
} else {
- const max = state().sharedSkills.length - 1;
+ const max = Math.min(state().sharedSkills.length - 1, 7); // Cap at visible range
setSelectedSkillIndex((prev) => Math.max(0, Math.min(prev + delta, max)));
}
};Option 2: Window the slice around selection
+ const skillsWindow = createMemo(() => {
+ const skills = state().sharedSkills;
+ const idx = selectedSkillIndex();
+ const start = Math.max(0, Math.min(idx - 3, skills.length - 8));
+ return { items: skills.slice(start, start + 8), offset: start };
+ });
- <For each={state().sharedSkills.slice(0, 8)}>
+ <For each={skillsWindow().items}>
{(skill, idx) => {
const selected = () =>
- focusedPane() === 'skills' && idx() === selectedSkillIndex();
+ focusedPane() === 'skills' && (idx() + skillsWindow().offset) === selectedSkillIndex();🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Team.tsx` around lines 299 - 328, The skills list
rendering uses state().sharedSkills.slice(0, 8) but navigation
(selectedSkillIndex in handleKeyNav) can point outside that slice; implement
windowing so the rendered slice centers on the selection: compute visibleCount =
8, start = Math.max(0, Math.min(selectedSkillIndex() -
Math.floor(visibleCount/2), state().sharedSkills.length - visibleCount)), then
render state().sharedSkills.slice(start, start + visibleCount) and adjust the
selection check to compare idx() + start === selectedSkillIndex(); update any
references in the For loop and selection predicate and ensure edge cases when
sharedSkills.length < visibleCount still work.
| const handleTranslate = async () => { | ||
| const skill = selectedSkill(); | ||
| const target = targetAgent(); | ||
| if (!skill || !target || !skill.path) { | ||
| setError('No skill path available for translation'); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing re-entry guard for translation.
handleTranslate doesn't check if a translation is already in progress. Rapid key presses could start multiple concurrent translations. Add a guard at the start.
🛡️ Suggested fix
const handleTranslate = async () => {
+ if (translating()) return;
+
const skill = selectedSkill();
const target = targetAgent();
if (!skill || !target || !skill.path) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleTranslate = async () => { | |
| const skill = selectedSkill(); | |
| const target = targetAgent(); | |
| if (!skill || !target || !skill.path) { | |
| setError('No skill path available for translation'); | |
| return; | |
| } | |
| const handleTranslate = async () => { | |
| if (translating()) return; | |
| const skill = selectedSkill(); | |
| const target = targetAgent(); | |
| if (!skill || !target || !skill.path) { | |
| setError('No skill path available for translation'); | |
| return; | |
| } | |
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Translate.tsx` around lines 127 - 134,
handleTranslate can be re-entered by rapid key presses; add a re-entry guard by
introducing and checking a boolean state (e.g., isTranslating) at the top of
handleTranslate, return early if it's true, then set it to true immediately when
starting the translation and set it back to false in a finally block after the
async work (also ensure errors still call setError). Reference handleTranslate,
selectedSkill, targetAgent and setError when adding/using the guard so you
prevent concurrent executions and always clear the flag on completion or error.
- Add minimal stub service implementations for all TUI screens: - context.service.ts - project context loading - executor.service.ts - skill execution - memory.service.ts - memory management - methodology.service.ts - methodology packs - plan.service.ts - plan management - plugin.service.ts - plugin management - recommend.service.ts - skill recommendations - team.service.ts - team collaboration - translator.service.ts - skill translation - workflow.service.ts - workflow orchestration - Fix type errors in screens: - Plan.tsx: Add non-null assertions for optional issues array - Plugins.tsx: Cast getPluginInfo result to Plugin type These stubs provide the expected interfaces for screens while the full integration with @skillkit/core is completed later.
Add the component files that screens depend on: - StatusIndicator - loading/success/error status display - DetailPane - side panel for detailed view - EmptyState/ErrorState - empty and error state components - SelectList - selection list component - SplitPane - split view layout - AnimatedText - text animations - Breadcrumb - navigation breadcrumb - Button/Clickable - interactive elements - CodeBlock - code display - FormField - form input wrapper - HoverHighlight - hover effect wrapper - RightSidebar/BottomStatusBar - layout components - TabBar - tab navigation - ErrorBoundary - error handling wrapper These components are required by the screen files and were missing from the previous commit.
- RightSidebar: inline sidebar state instead of importing from context - services/index.ts: fix duplicate AgentType export by using explicit exports for executor and translator services
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/tui/src/screens/Plugins.tsx`:
- Around line 42-79: Wrap the async logic in loadData, handleToggle, and
handleShowDetail with try/catch (and finally where appropriate) so thrown errors
don't leave UI state inconsistent: in loadData set loading true before awaiting
loadPluginsList() and in a finally block set loading false and setState to an
error tuple in catch; in handleToggle wrap enablePlugin/disablePlugin and
loadData calls in try/catch and ensure setToggling(false) runs in finally and
set an error state on failure; in handleShowDetail catch errors from
getPluginInfo and set an error state (and avoid leaving showDetail toggled on)
so failures are surfaced to the UI. Include references to setState, setToggling,
setShowDetail, loadPluginsList, enablePlugin, disablePlugin, and getPluginInfo
when making the changes.
- Line 77: The selected plugin setter is being forced to use a cast because
getPluginInfo returns Promise<unknown>; update the getPluginInfo signature to
Promise<Plugin | null> (or modify its stub to return Plugin | null) so callers
don't need a cast, then remove the cast at the call site where
setSelectedPlugin(info as Plugin | null) is used; target getPluginInfo and the
setSelectedPlugin call and ensure the Plugin type is imported/visible to the
getPluginInfo return type.
In `@packages/tui/src/services/index.ts`:
- Around line 6-26: The barrel export conflicts because executor.service.ts and
translator.service.ts both export a type named AgentType with incompatible
definitions; in packages/tui/src/services/index.ts remove the wildcard re-export
of './translator.service.js' and instead export only the needed named members
from translator.service (e.g. export { translate, translatorService, /* etc. */
}) while aliasing its AgentType to a distinct name (e.g. export type
TranslatorAgentType = AgentType from './translator.service.js') so the
translator's AgentType does not collide with executor.service's AgentType; keep
the existing wildcard exports and re-exports for other services (memoryService,
workflowService, planService, pluginService, teamService, methodologyService,
recommendService, contextService, executorService) unchanged.
🧹 Nitpick comments (13)
packages/tui/src/App.tsx (1)
16-19: Consider usingexecFileor escaping to prevent command injection.While
DOCS_URLis hardcoded, theexecwith string interpolation is a command injection pattern. If this function were reused with user-provided URLs, shell metacharacters could be exploited.🛡️ Safer alternative using spawn
-function openUrl(url: string): void { - const cmd = process.platform === 'darwin' ? 'open' : process.platform === 'win32' ? 'start' : 'xdg-open'; - exec(`${cmd} ${url}`); +import { spawn } from 'node:child_process'; + +function openUrl(url: string): void { + const cmd = process.platform === 'darwin' ? 'open' : process.platform === 'win32' ? 'cmd' : 'xdg-open'; + const args = process.platform === 'win32' ? ['/c', 'start', '', url] : [url]; + spawn(cmd, args, { detached: true, stdio: 'ignore' }).unref(); }packages/tui/src/services/context.service.ts (1)
13-13: Consider defining a proper type forstack.
stack: unknown | nullis loosely typed. When real implementation is added, consider defining aStackInfointerface to improve type safety and IDE support.packages/tui/src/services/executor.service.ts (1)
31-45:executeSkillandexecuteSkillWithAgentappear identical.Both functions have the same signature and return the same result. If they're meant to have different behaviors when real implementation is added, consider adding a TODO comment clarifying the intended distinction.
packages/tui/src/services/team.service.ts (1)
16-20:TeamConfighas potentially redundant fields.Both
nameandteamNameexist in the interface. Consider clarifying the distinction or consolidating if they serve the same purpose.packages/tui/src/services/plan.service.ts (1)
27-32:PlanValidationResulthas redundant error representations.The interface has both
errors: string[]/warnings: string[]andissues?: PlanValidationIssue[]. Consider consolidating to avoid ambiguity about which field consumers should use.♻️ Suggested consolidation
export interface PlanValidationResult { valid: boolean; - errors: string[]; - warnings: string[]; - issues?: PlanValidationIssue[]; + issues: PlanValidationIssue[]; }Then derive error/warning counts from
issuesfiltered bytype.packages/tui/src/screens/Plan.tsx (1)
40-42: Consider usingonMountinstead ofcreateEffectfor one-time data loading.
createEffectwith no dependencies will run once, butonMountfrom solid-js is the idiomatic pattern for initialization side effects.♻️ Suggested change
-import { createSignal, createEffect, Show, For } from 'solid-js'; +import { createSignal, onMount, Show, For } from 'solid-js'; ... - createEffect(() => { + onMount(() => { loadData(); });packages/tui/src/services/translator.service.ts (4)
81-83: Gate translation by supported agents.
Always returningtruecan enable unsupported flows; consider validating againstgetSupportedAgents().♻️ Suggested update
export function checkCanTranslate(_from: AgentType, _to: AgentType): boolean { - return true; + const supported = getSupportedAgents(); + return supported.includes(_from) && supported.includes(_to); }
85-99: Avoid false-positive success in the stub translator.
If this is a placeholder, consider marking it as not yet integrated so the UI can surface that.♻️ Suggested update
export function translate( _content: string, targetAgent: AgentType, options?: TranslationOptions ): TranslationDisplay { return { sourceFormat: options?.sourceFilename || 'SKILL.md', targetAgent, - success: true, + success: false, content: '', filename: undefined, - warnings: [], + warnings: ['Translation service not yet integrated'], incompatible: [], }; }
101-115: Keep the file-translation stub in sync withtranslate.
Delegating totranslateavoids duplicate placeholder logic.♻️ Suggested update
export function translateFromFile( sourcePath: string, targetAgent: AgentType, _options?: TranslationOptions ): TranslationDisplay { - return { - sourceFormat: sourcePath, - targetAgent, - success: true, - content: '', - filename: undefined, - warnings: [], - incompatible: [], - }; + return translate('', targetAgent, { ..._options, sourceFilename: sourcePath }); }
125-134: Surface stub status in preview results.
Returningcompatible: truewith an empty preview can be misleading.♻️ Suggested update
export function previewTranslation( _content: string, _targetAgent: AgentType ): { preview: string; compatible: boolean; issues: string[] } { return { preview: '', - compatible: true, - issues: [], + compatible: false, + issues: ['Translation preview not yet implemented'], }; }packages/tui/src/services/methodology.service.ts (1)
29-39: Consider not reporting success for unimplemented actions.
Always returningtruecan mislead the UI into showing successful installs/syncs.♻️ Suggested update
export async function installMethodologyPack(_name: string): Promise<boolean> { - return true; + return false; } export async function uninstallMethodologyPack(_name: string): Promise<boolean> { - return true; + return false; } export async function syncMethodologyPack(_name: string): Promise<boolean> { - return true; + return false; }packages/tui/src/services/plugin.service.ts (1)
26-43: Avoid reporting success for unimplemented plugin actions.
Returningtruecan make the UI think enable/disable/install succeeded when they’re still stubs.♻️ Suggested update
export async function enablePlugin(_name: string, _manager: unknown): Promise<boolean> { - return true; + return false; } export async function disablePlugin(_name: string, _manager: unknown): Promise<boolean> { - return true; + return false; } export async function installPluginFromPath( _pluginPath: string, _projectPath?: string ): Promise<boolean> { - return true; + return false; }packages/tui/src/services/memory.service.ts (1)
26-36: Consider not returning success for unimplemented memory actions.
Returningtruecan cause the UI to show operations as successful when they’re stubs.♻️ Suggested update
export async function deleteMemoryEntry(_key: string): Promise<boolean> { - return true; + return false; } export async function clearMemory(_path?: string, _scope?: string): Promise<boolean> { - return true; + return false; } export async function initializeMemory(): Promise<boolean> { - return true; + return false; }
packages/tui/src/screens/Plugins.tsx
Outdated
| if (!plugin) return; | ||
|
|
||
| const info = await getPluginInfo(plugin.name, manager); | ||
| setSelectedPlugin(info as Plugin | null); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the getPluginInfo function definition and its return type
ast-grep --pattern 'async function getPluginInfo($$$) {
$$$
}'Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
# Also search for getPluginInfo with variations
rg 'getPluginInfo' -A 5 -B 2 --type=ts --type=tsxRepository: rohitg00/skillkit
Length of output: 88
🏁 Script executed:
# Find the Plugin type definition
ast-grep --pattern 'type Plugin = $$$'Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
# Also check for interface Plugin
ast-grep --pattern 'interface Plugin {
$$$
}'Repository: rohitg00/skillkit
Length of output: 1136
🏁 Script executed:
# Search for getPluginInfo in all files
rg 'getPluginInfo' -A 5 -B 2Repository: rohitg00/skillkit
Length of output: 1899
🏁 Script executed:
# Also search for where it's imported in the Plugins.tsx file
rg 'import.*getPluginInfo' --type=tsRepository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
# Look at the Plugins.tsx file around line 77
head -85 packages/tui/src/screens/Plugins.tsx | tail -15Repository: rohitg00/skillkit
Length of output: 487
🏁 Script executed:
# Get more context of the Plugins.tsx file
cat packages/tui/src/screens/Plugins.tsx | head -100Repository: rohitg00/skillkit
Length of output: 3150
Remove the type cast or fix the return type of getPluginInfo.
The function returns Promise<unknown>, requiring the cast info as Plugin | null. Instead of casting, either:
- Type
getPluginInfoto returnPromise<Plugin | null>directly, or - Ensure the stub implementation properly reflects the expected return type.
The cast itself is not problematic—it's a necessary consequence of the overly broad unknown return type. Fixing the function signature eliminates the need for it.
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Plugins.tsx` at line 77, The selected plugin setter
is being forced to use a cast because getPluginInfo returns Promise<unknown>;
update the getPluginInfo signature to Promise<Plugin | null> (or modify its stub
to return Plugin | null) so callers don't need a cast, then remove the cast at
the call site where setSelectedPlugin(info as Plugin | null) is used; target
getPluginInfo and the setSelectedPlugin call and ensure the Plugin type is
imported/visible to the getPluginInfo return type.
- App: restore process.exit(0) fallback when onExit is undefined - Execute: pass skill path instead of content to executeSkillWithAgent - Memory: add useKeyboard handler for j/k/Enter/d/c/i/Esc shortcuts
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@packages/tui/src/screens/Execute.tsx`:
- Around line 167-186: The skills list rendering currently slices
skills().slice(0, 8) while navigation (handleKeyNav / selectedSkillIndex()) can
move selection off-screen; change the rendered window to compute a dynamic start
index so the selectedSkillIndex() is always visible (e.g., let viewSize = 8; let
start = clamp(selectedSkillIndex() - Math.floor(viewSize/2), 0, Math.max(0,
skills().length - viewSize)); then render skills().slice(start, start +
viewSize) and adjust the “+N more” indicator accordingly), and apply the same
windowing logic to the agents list rendering (the mirror range at the other
lines).
- Around line 54-67: The handler handleExecute must guard against re-entrancy by
early-returning if executing() is already true, then setExecuting(true)
immediately before awaiting work; modify handleExecute to check if (executing())
return at the top, and wrap the async execution logic in try { ... } finally {
setExecuting(false); } so executing is always cleared even on error; reference
symbols: handleExecute, executing(), setExecuting, selectedSkillIndex(),
selectedAgentIndex().
- Around line 24-52: The loadData function can throw (e.g.,
getAgentAvailability) and never resets the loading signal; wrap the async work
in try/catch/finally inside loadData: set a new error signal (e.g.,
createSignal<string | null> error) in the catch with the caught error message,
ensure setLoading(false) is called in finally, and update the component render
to show an ErrorState when error() is non-null; reference the loadData function,
getAgentAvailability call, setLoading, and the new error signal/ErrorState so
the UI never stays stuck loading.
In `@packages/tui/src/screens/Marketplace.tsx`:
- Around line 92-98: The navigation is limited because visibleSkills() always
slices from 0 and handleKeyNav clamps to that slice; change the logic so
navigation indexes the full filteredSkills() list rather than the visible slice:
let handleKeyNav clamp against filteredSkills().length - 1 (not
visibleSkills().length) and compute the visible window from selectedIndex (e.g.,
a pageStart derived from selectedIndex and maxVisible()) in the rendering code;
update maxVisible, visibleSkills (or replace it) to accept/derive an offset
based on selectedIndex so the UI can show the correct "+N more" and allow
selection to move beyond the first page (also apply same change to the
corresponding code around the 230-262 region).
In `@packages/tui/src/screens/Memory.tsx`:
- Around line 132-156: The keyboard handler in Memory.tsx advertises "r" to
retry but doesn't handle it; update the useKeyboard callback to check for
key.name === 'r' and invoke loadData() (the same function used to fetch/refresh
the entries) so pressing "r" retries loading; locate the useKeyboard((key: {
name?: string }) => { ... }) block and add an else-if branch for 'r' that calls
loadData(), ensuring it follows the existing style (e.g., similar to how 'c'
calls handleClearAll()) and preserves focus/selection state.
In `@packages/tui/src/screens/Recommend.tsx`:
- Around line 72-88: The navigation currently clamps selection against
state().recommendations length causing the index to point outside the rendered
visibleRecs() window; update handleKeyNav (and the keyboard handler logic that
calls it) to clamp against the visibleRecs() range instead of the full state
list: compute visibleMax = Math.max(0, visibleRecs().length - 1) and use
setSelectedIndex(prev => Math.max(0, Math.min(prev + delta, visibleMax))); also
ensure Enter/handleInstall and showDetail logic operate on the currently visible
index (selectedIndex) so Enter acts on a visible item when showDetail() is
toggled.
In `@packages/tui/src/screens/Sync.tsx`:
- Around line 78-105: handleSync captures selected agent only after an async
delay causing potential drift if selectedIndex() changes; fix by reading and
storing the selected index and agent (e.g., const idx = selectedIndex(); const
agent = agentList[idx];) before any await, then use those locals when calling
setAgents and when composing messages (use idx instead of selectedIndex() and
the captured agent instead of re-reading agentList) and keep skillCount() if
it's stable or capture its value similarly.
In `@packages/tui/src/screens/Workflow.tsx`:
- Around line 50-66: The handleExecute function can leave executing=true if
executeWorkflow rejects; wrap the await executeWorkflow(...) call in a
try/finally in handleExecute so setExecuting(false) always runs (use try { const
result = await executeWorkflow(...); if (result) loadData(); } finally {
setExecuting(false); }). Modify the block around executeWorkflow in
handleExecute, referencing the handleExecute function, executeWorkflow call,
setExecuting, and loadData to ensure executing is cleared even on rejection;
optionally catch and log the error before rethrowing if you need to surface it.
🧹 Nitpick comments (6)
packages/tui/src/components/BottomStatusBar.tsx (1)
33-53: Consider centralizing the shortcut hint string.
"/ cmd h home m market q quit"is hard-coded and can easily drift as keybindings/features evolve (e.g., vi keys, theme switch). A shared keymap/shortcut constant would keep this in sync.packages/tui/src/components/Button.tsx (1)
142-158: Consider using<For>for idiomatic Solid.js list rendering.
Array.map()works but causes the entire list to re-render on any change. Solid's<For>provides keyed, fine-grained updates. This is a minor optimization since button arrays are typically static.♻️ Proposed refactor using For
+import { Show, For } from 'solid-js'; ... return ( <box flexDirection={direction() === 'horizontal' ? 'row' : 'column'}> - {props.buttons.map((btn, index) => ( + <For each={props.buttons}> + {(btn, index) => ( <box - marginRight={direction() === 'horizontal' && index < props.buttons.length - 1 ? gap() : 0} - marginBottom={direction() === 'vertical' && index < props.buttons.length - 1 ? gap() : 0} + marginRight={direction() === 'horizontal' && index() < props.buttons.length - 1 ? gap() : 0} + marginBottom={direction() === 'vertical' && index() < props.buttons.length - 1 ? gap() : 0} > <Button label={btn.label} shortcut={btn.shortcut} icon={btn.icon} variant={btn.variant} disabled={btn.disabled} focused={btn.id === props.focusedId} hovered={btn.id === props.selectedId} onClick={btn.onClick} /> </box> - ))} + )} + </For> </box> );Note: With
<For>,indexbecomes a signal accessorindex().packages/tui/src/screens/Workflow.tsx (1)
24-48: Handle load failures to avoid stuck loading state.Line 45: if
loadWorkflowsList()rejects, the screen can remain in a loading state without showing an error. A small try/catch keeps the UI responsive.🛠️ Suggested fix
const loadData = async () => { setState((s) => ({ ...s, loading: true, error: null })); - const result = await loadWorkflowsList(); - setState(result); + try { + const result = await loadWorkflowsList(); + setState(result); + } catch (err) { + setState((s) => ({ + ...s, + loading: false, + error: err instanceof Error ? err.message : 'Failed to load workflows', + })); + } };packages/tui/src/screens/Sync.tsx (1)
45-76: Reset sync banners on refresh to avoid stale status.Line 45:
loadDataclears loading/error but keepssyncStatus/syncMessage, so old success/error banners can persist after refresh.🛠️ Suggested fix
const loadData = async () => { setLoading(true); setError(null); + setSyncStatus('idle'); + setSyncMessage('');packages/tui/src/App.tsx (1)
97-101: Clamp computed content sizes for tiny terminals.Line 99:
cols()androws()can go negative on very small terminals, which can break downstream layout.🛠️ Suggested fix
const screenProps = () => ({ onNavigate: handleNavigate, - cols: cols() - sidebarWidth() - 2, - rows: contentHeight() - 1, + cols: Math.max(1, cols() - sidebarWidth() - 2), + rows: Math.max(1, contentHeight() - 1), });packages/tui/src/screens/Recommend.tsx (1)
23-56: Guard async load with try/catch to surface failures.Line 46: exceptions from
analyzeProject/getRecommendationswill currently bubble and can leave loading/analyzing true without error UI.🛠️ Suggested fix
const loadData = async () => { setState((s) => ({ ...s, loading: true, analyzing: true, error: null })); - const analysis = await analyzeProject(); - if (analysis) { - setProjectInfo({ - languages: analysis.languages, - frameworks: analysis.frameworks, - }); - } - - const result = await getRecommendations(); - setState(result); + try { + const analysis = await analyzeProject(); + if (analysis) { + setProjectInfo({ + languages: analysis.languages, + frameworks: analysis.frameworks, + }); + } + + const result = await getRecommendations(); + setState(result); + } catch (err) { + setState((s) => ({ + ...s, + loading: false, + analyzing: false, + error: err instanceof Error ? err.message : 'Failed to load recommendations', + })); + } };
| const handleExecute = async () => { | ||
| const skillList = skills(); | ||
| const agentList = agents(); | ||
|
|
||
| if (skillList.length === 0 || agentList.length === 0) return; | ||
|
|
||
| const skill = skillList[selectedSkillIndex()]; | ||
| const agent = agentList[selectedAgentIndex()]; | ||
|
|
||
| if (!skill || !agent || !agent.available) return; | ||
|
|
||
| setExecuting(true); | ||
| setExecutionResult(null); | ||
|
|
There was a problem hiding this comment.
Prevent re-entrant executions while a run is in progress.
handleExecute can be triggered again while executing() is true, which can start concurrent skill runs.
🛠️ Suggested fix
const handleExecute = async () => {
+ if (executing()) return;
const skillList = skills();
const agentList = agents();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleExecute = async () => { | |
| const skillList = skills(); | |
| const agentList = agents(); | |
| if (skillList.length === 0 || agentList.length === 0) return; | |
| const skill = skillList[selectedSkillIndex()]; | |
| const agent = agentList[selectedAgentIndex()]; | |
| if (!skill || !agent || !agent.available) return; | |
| setExecuting(true); | |
| setExecutionResult(null); | |
| const handleExecute = async () => { | |
| if (executing()) return; | |
| const skillList = skills(); | |
| const agentList = agents(); | |
| if (skillList.length === 0 || agentList.length === 0) return; | |
| const skill = skillList[selectedSkillIndex()]; | |
| const agent = agentList[selectedAgentIndex()]; | |
| if (!skill || !agent || !agent.available) return; | |
| setExecuting(true); | |
| setExecutionResult(null); | |
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Execute.tsx` around lines 54 - 67, The handler
handleExecute must guard against re-entrancy by early-returning if executing()
is already true, then setExecuting(true) immediately before awaiting work;
modify handleExecute to check if (executing()) return at the top, and wrap the
async execution logic in try { ... } finally { setExecuting(false); } so
executing is always cleared even on error; reference symbols: handleExecute,
executing(), setExecuting, selectedSkillIndex(), selectedAgentIndex().
Security & Robustness: - App: use execFile instead of exec to prevent command injection in openUrl - App: clamp cols/rows to minimum 1 to prevent negative dimensions - index: await render() and handle failures with exitTUI(1) Execute screen: - Add error state for load failures - Prevent re-entrant execution with executing() guard - Use try/finally to always clear executing state Memory screen: - Add missing 'r' shortcut for retry/refresh Workflow screen: - Use try/finally to always clear executing state even on rejection Sync screen: - Capture selectedIndex and agent values before await to prevent drift
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/tui/src/index.tsx`:
- Around line 37-41: The local type interface CliRendererConfig is missing
mouse-related options used when calling createCliRenderer; update the
CliRendererConfig declaration to include optional properties useMouse?: boolean
and enableMouseMovement?: boolean (along with existing exitOnCtrlC, stdin,
stdout) so calls to createCliRenderer({ exitOnCtrlC, useMouse,
enableMouseMovement, ... }) type-check correctly.
In `@packages/tui/src/screens/Execute.tsx`:
- Around line 109-124: The EmptyState advertises a "Browse (b)" key but the
useKeyboard handler in Execute.tsx doesn't handle 'b'; add a branch in the
useKeyboard callback to handle key.name === 'b' and call
props.onNavigate('browse') (or the app's existing browse navigation) so the
advertised key actually performs the browse action; update the same useKeyboard
block (near handleKeyNav, setFocusedPane, handleExecute, loadData,
executionResult, props.onNavigate) to include this branch.
In `@packages/tui/src/screens/Memory.tsx`:
- Around line 73-85: handleClearAll currently calls clearMemory and then
loadData but does not reset selectedIndex, leaving it pointing at a non-existent
entry; update the handleClearAll flow (the function handleClearAll that calls
clearMemory and loadData) to reset selectedIndex in state (e.g., set
selectedIndex to -1 or null) after a successful clearMemory before or
immediately after calling loadData by using setState(...) to update
selectedIndex along with clearing any error state so the UI no longer references
a removed entry.
- Around line 53-71: handleDelete currently removes an entry then calls
loadData() but doesn't adjust the selection, which can leave selectedIndex
out-of-bounds and cause selectedEntry() to return undefined; after a successful
delete (inside the if (success) block in handleDelete), clamp or set
selectedIndex to Math.min(selectedIndex(), Math.max(0, newEntriesLength - 1)) or
explicitly compute the new length via the refreshed state returned from
loadData(), and update selectedIndex (or call setState to adjust the selection)
so it never exceeds entries.length - 1 and never stays negative, ensuring
selectedEntry() and the detail pane remain valid.
In `@packages/tui/src/screens/Workflow.tsx`:
- Around line 44-48: The loadData function currently awaits loadWorkflowsList()
without error handling, so if loadWorkflowsList throws the UI stays stuck with
loading: true; wrap the await in try/catch (or try/finally) inside loadData, on
success call setState(result) as before, on error call setState(s => ({ ...s,
loading: false, error: err })) with a useful error message/object, and ensure
loading is set to false in a finally or in both branches so the spinner clears
and the UI can show a retry/error state; reference the loadData function,
loadWorkflowsList, and setState/state keys loading and error when making the
change.
🧹 Nitpick comments (6)
packages/tui/src/screens/Workflow.tsx (2)
10-16: Unused import:loadWorkflow
loadWorkflowis imported but never used in this file.🧹 Remove unused import
import { loadWorkflowsList, - loadWorkflow, executeWorkflow, type WorkflowServiceState, type WorkflowListItem, } from '../services/workflow.service.js';
37-37: Unused computed:isCompact
isCompactis defined but never referenced in the component. Either remove it or use it for responsive layout adjustments.packages/tui/src/screens/Memory.tsx (1)
284-286: Consider adding 'r refresh' to the footer shortcuts.The
rkey is wired toloadData()and is advertised in the error state, but the general footer omits it. Users may want to refresh data even when there's no error.💡 Suggested footer update
<text fg={terminalColors.textMuted}> - j/k navigate Enter view d delete c clear all i init Esc back + j/k navigate Enter view d delete c clear all i init r refresh Esc back </text>packages/tui/src/screens/Sync.tsx (2)
111-131: Consider capturing available agents count before the async operation.While this works correctly in Solid.js (signal updates are synchronous), the pattern of reading
agents()aftersetAgents()at line 124 is fragile. If the user triggers another action during the 2-second delay, the count could reflect unintended changes.♻️ Proposed defensive fix
const handleSyncAll = async () => { + const availableAgents = agents().filter((a) => a.available); + const countToSync = availableAgents.length; + setSyncStatus('syncing'); setSyncMessage('Syncing to all available agents...'); try { await new Promise((resolve) => setTimeout(resolve, 2000)); setAgents((prev) => prev.map((a) => a.available ? { ...a, synced: true, skillCount: skillCount() } : a ) ); - const syncedCount = agents().filter((a) => a.available).length; setSyncStatus('done'); - setSyncMessage(`Synced to ${syncedCount} agents`); + setSyncMessage(`Synced to ${countToSync} agents`); } catch { setSyncStatus('error'); setSyncMessage('Sync failed'); } };
41-43: Consider usingonMountfor one-time initialization.The
createEffecthere has no reactive dependencies, so it only runs once on mount. UsingonMountfromsolid-jswould make the intent clearer.♻️ Suggested change
-import { createSignal, createEffect, createMemo, Show, For } from 'solid-js'; +import { createSignal, createMemo, onMount, Show, For } from 'solid-js'; ... - createEffect(() => { - loadData(); - }); + onMount(() => { + loadData(); + });packages/tui/src/App.tsx (1)
101-105: Consider memoizingscreenPropsto avoid repeated object creation.
screenProps()is a function that creates a new object on each call. While Solid.js handles this efficiently, memoizing the props object could reduce allocations during rapid updates (e.g., window resizing).♻️ Suggested change
- const screenProps = () => ({ + const screenProps = createMemo(() => ({ onNavigate: handleNavigate, cols: Math.max(1, cols() - sidebarWidth() - 2), rows: Math.max(1, contentHeight() - 1), - }); + }));
| renderer = await createCliRenderer({ | ||
| exitOnCtrlC: false, | ||
| useMouse: true, | ||
| enableMouseMovement: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/tui && cat package.json | grep -A5 "@opentui"Repository: rohitg00/skillkit
Length of output: 254
🏁 Script executed:
cd packages/tui && head -50 src/index.tsxRepository: rohitg00/skillkit
Length of output: 1299
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.tsx" -o -name "*.d.ts" | xargs grep -l "createCliRenderer" | head -10Repository: rohitg00/skillkit
Length of output: 127
🏁 Script executed:
cat -n packages/tui/src/solid-opentui.d.tsRepository: rohitg00/skillkit
Length of output: 4562
🏁 Script executed:
rg "createCliRenderer" --type ts --type tsx -A 10 -B 2Repository: rohitg00/skillkit
Length of output: 88
🏁 Script executed:
rg "useMouse|enableMouseMovement" --type ts --type txtRepository: rohitg00/skillkit
Length of output: 165
🌐 Web query:
@opentui/core createCliRenderer API documentation mouse options
💡 Result:
In @opentui/core, the createCliRenderer(config?) mouse-related options are:
useMouse?: boolean(defaulttrue) — enables mouse input and tracking.enableMouseMovement?: boolean(defaulttrue) — tracks mouse movement events (not just clicks).
Example:
import { createCliRenderer } from "@opentui/core";
const renderer = await createCliRenderer({
useMouse: true,
enableMouseMovement: false,
});[1]
Update type definitions for createCliRenderer to include mouse options.
The useMouse and enableMouseMovement options are valid and supported by @opentui/core (v0.1.75), but the local type definitions in packages/tui/src/solid-opentui.d.ts are incomplete. The CliRendererConfig interface is missing these properties. Update the interface to include:
export interface CliRendererConfig {
exitOnCtrlC?: boolean;
useMouse?: boolean;
enableMouseMovement?: boolean;
stdin?: NodeJS.ReadStream;
stdout?: NodeJS.WriteStream;
}🤖 Prompt for AI Agents
In `@packages/tui/src/index.tsx` around lines 37 - 41, The local type interface
CliRendererConfig is missing mouse-related options used when calling
createCliRenderer; update the CliRendererConfig declaration to include optional
properties useMouse?: boolean and enableMouseMovement?: boolean (along with
existing exitOnCtrlC, stdin, stdout) so calls to createCliRenderer({
exitOnCtrlC, useMouse, enableMouseMovement, ... }) type-check correctly.
| useKeyboard((key: { name?: string }) => { | ||
| if (key.name === 'j' || key.name === 'down') handleKeyNav(1); | ||
| else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); | ||
| else if (key.name === 'tab' || key.name === 'l' || key.name === 'right') { | ||
| setFocusedPane((p) => (p === 'skills' ? 'agents' : 'skills')); | ||
| } | ||
| else if (key.name === 'h' || key.name === 'left') { | ||
| setFocusedPane((p) => (p === 'agents' ? 'skills' : 'agents')); | ||
| } | ||
| else if (key.name === 'return' || key.name === 'x') handleExecute(); | ||
| else if (key.name === 'r') loadData(); | ||
| else if (key.name === 'escape') { | ||
| if (executionResult()) setExecutionResult(null); | ||
| else props.onNavigate('home'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Wire the advertised “Browse (b)” action.
The EmptyState shows key b, but there’s no handler. Add a binding or remove the hint to avoid a dead key.
💡 Suggested fix
- else if (key.name === 'return' || key.name === 'x') handleExecute();
+ else if (key.name === 'return' || key.name === 'x') handleExecute();
+ else if (key.name === 'b') props.onNavigate('browse'); // use the correct Screen id
else if (key.name === 'r') loadData();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useKeyboard((key: { name?: string }) => { | |
| if (key.name === 'j' || key.name === 'down') handleKeyNav(1); | |
| else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); | |
| else if (key.name === 'tab' || key.name === 'l' || key.name === 'right') { | |
| setFocusedPane((p) => (p === 'skills' ? 'agents' : 'skills')); | |
| } | |
| else if (key.name === 'h' || key.name === 'left') { | |
| setFocusedPane((p) => (p === 'agents' ? 'skills' : 'agents')); | |
| } | |
| else if (key.name === 'return' || key.name === 'x') handleExecute(); | |
| else if (key.name === 'r') loadData(); | |
| else if (key.name === 'escape') { | |
| if (executionResult()) setExecutionResult(null); | |
| else props.onNavigate('home'); | |
| } | |
| }); | |
| useKeyboard((key: { name?: string }) => { | |
| if (key.name === 'j' || key.name === 'down') handleKeyNav(1); | |
| else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); | |
| else if (key.name === 'tab' || key.name === 'l' || key.name === 'right') { | |
| setFocusedPane((p) => (p === 'skills' ? 'agents' : 'skills')); | |
| } | |
| else if (key.name === 'h' || key.name === 'left') { | |
| setFocusedPane((p) => (p === 'agents' ? 'skills' : 'agents')); | |
| } | |
| else if (key.name === 'return' || key.name === 'x') handleExecute(); | |
| else if (key.name === 'b') props.onNavigate('browse'); // use the correct Screen id | |
| else if (key.name === 'r') loadData(); | |
| else if (key.name === 'escape') { | |
| if (executionResult()) setExecutionResult(null); | |
| else props.onNavigate('home'); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Execute.tsx` around lines 109 - 124, The EmptyState
advertises a "Browse (b)" key but the useKeyboard handler in Execute.tsx doesn't
handle 'b'; add a branch in the useKeyboard callback to handle key.name === 'b'
and call props.onNavigate('browse') (or the app's existing browse navigation) so
the advertised key actually performs the browse action; update the same
useKeyboard block (near handleKeyNav, setFocusedPane, handleExecute, loadData,
executionResult, props.onNavigate) to include this branch.
| const handleDelete = async () => { | ||
| const entries = state().entries; | ||
| if (entries.length === 0) return; | ||
|
|
||
| const entry = entries[selectedIndex()]; | ||
| if (!entry) return; | ||
|
|
||
| try { | ||
| const success = await deleteMemoryEntry(entry.key); | ||
| if (success) { | ||
| loadData(); | ||
| } | ||
| } catch (err) { | ||
| setState((s) => ({ | ||
| ...s, | ||
| error: err instanceof Error ? err.message : 'Failed to delete entry', | ||
| })); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Clamp selectedIndex after delete to prevent out-of-bounds access.
After a successful delete, loadData() refreshes the entries but selectedIndex may now exceed the new array length. If the user deletes the last item (e.g., index 2 of 3 entries), the index remains 2 while entries now has only 2 items. This causes selectedEntry() to return undefined instead of null, and breaks detail pane rendering.
🐛 Proposed fix to clamp selectedIndex after delete
try {
const success = await deleteMemoryEntry(entry.key);
if (success) {
- loadData();
+ await loadData();
+ setSelectedIndex((prev) => Math.min(prev, Math.max(0, state().entries.length - 1)));
}
} catch (err) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDelete = async () => { | |
| const entries = state().entries; | |
| if (entries.length === 0) return; | |
| const entry = entries[selectedIndex()]; | |
| if (!entry) return; | |
| try { | |
| const success = await deleteMemoryEntry(entry.key); | |
| if (success) { | |
| loadData(); | |
| } | |
| } catch (err) { | |
| setState((s) => ({ | |
| ...s, | |
| error: err instanceof Error ? err.message : 'Failed to delete entry', | |
| })); | |
| } | |
| }; | |
| const handleDelete = async () => { | |
| const entries = state().entries; | |
| if (entries.length === 0) return; | |
| const entry = entries[selectedIndex()]; | |
| if (!entry) return; | |
| try { | |
| const success = await deleteMemoryEntry(entry.key); | |
| if (success) { | |
| await loadData(); | |
| setSelectedIndex((prev) => Math.min(prev, Math.max(0, state().entries.length - 1))); | |
| } | |
| } catch (err) { | |
| setState((s) => ({ | |
| ...s, | |
| error: err instanceof Error ? err.message : 'Failed to delete entry', | |
| })); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Memory.tsx` around lines 53 - 71, handleDelete
currently removes an entry then calls loadData() but doesn't adjust the
selection, which can leave selectedIndex out-of-bounds and cause selectedEntry()
to return undefined; after a successful delete (inside the if (success) block in
handleDelete), clamp or set selectedIndex to Math.min(selectedIndex(),
Math.max(0, newEntriesLength - 1)) or explicitly compute the new length via the
refreshed state returned from loadData(), and update selectedIndex (or call
setState to adjust the selection) so it never exceeds entries.length - 1 and
never stays negative, ensuring selectedEntry() and the detail pane remain valid.
| const handleClearAll = async () => { | ||
| try { | ||
| const success = await clearMemory(undefined, 'all'); | ||
| if (success) { | ||
| loadData(); | ||
| } | ||
| } catch (err) { | ||
| setState((s) => ({ | ||
| ...s, | ||
| error: err instanceof Error ? err.message : 'Failed to clear memory', | ||
| })); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Reset selectedIndex after clearing all memories.
After clearMemory succeeds, the entries array becomes empty but selectedIndex retains its previous value. This leaves selectedIndex pointing to a non-existent entry.
🐛 Proposed fix to reset selectedIndex after clear
try {
const success = await clearMemory(undefined, 'all');
if (success) {
- loadData();
+ await loadData();
+ setSelectedIndex(0);
}
} catch (err) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleClearAll = async () => { | |
| try { | |
| const success = await clearMemory(undefined, 'all'); | |
| if (success) { | |
| loadData(); | |
| } | |
| } catch (err) { | |
| setState((s) => ({ | |
| ...s, | |
| error: err instanceof Error ? err.message : 'Failed to clear memory', | |
| })); | |
| } | |
| }; | |
| const handleClearAll = async () => { | |
| try { | |
| const success = await clearMemory(undefined, 'all'); | |
| if (success) { | |
| await loadData(); | |
| setSelectedIndex(0); | |
| } | |
| } catch (err) { | |
| setState((s) => ({ | |
| ...s, | |
| error: err instanceof Error ? err.message : 'Failed to clear memory', | |
| })); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Memory.tsx` around lines 73 - 85, handleClearAll
currently calls clearMemory and then loadData but does not reset selectedIndex,
leaving it pointing at a non-existent entry; update the handleClearAll flow (the
function handleClearAll that calls clearMemory and loadData) to reset
selectedIndex in state (e.g., set selectedIndex to -1 or null) after a
successful clearMemory before or immediately after calling loadData by using
setState(...) to update selectedIndex along with clearing any error state so the
UI no longer references a removed entry.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const handleKeyNav = (delta: number) => { | ||
| if (focusedPane() === 'skills') { | ||
| const max = skills().length - 1; | ||
| setSelectedSkillIndex((prev) => Math.max(0, Math.min(prev + delta, max))); | ||
| } else if (focusedPane() === 'source') { | ||
| const max = supportedAgents().length - 1; | ||
| setSourceAgentIndex((prev) => Math.max(0, Math.min(prev + delta, max))); | ||
| } else { | ||
| const max = supportedAgents().length - 1; | ||
| setTargetAgentIndex((prev) => Math.max(0, Math.min(prev + delta, max))); | ||
| } | ||
| }; | ||
|
|
||
| useKeyboard((key: { name?: string }) => { | ||
| if (key.name === 'j' || key.name === 'down') handleKeyNav(1); | ||
| else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); | ||
| else if (key.name === 'tab') { | ||
| setFocusedPane((p) => { | ||
| if (p === 'skills') return 'source'; | ||
| if (p === 'source') return 'target'; | ||
| return 'skills'; | ||
| }); | ||
| } else if (key.name === 'p') handlePreview(); | ||
| else if (key.name === 'return' || key.name === 't') handleTranslate(); | ||
| else if (key.name === 'r') loadData(); | ||
| else if (key.name === 'escape') { | ||
| if (result() || preview()) { | ||
| setResult(null); | ||
| setPreview(null); | ||
| } else { | ||
| props.onNavigate('home'); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const visibleAgents = () => supportedAgents().slice(0, 6); | ||
|
|
||
| return ( | ||
| <box flexDirection="column" paddingLeft={1}> | ||
| <Header | ||
| title="Translate Skills" | ||
| subtitle="Convert skills between agent formats" | ||
| icon="⇄" | ||
| icon="⇄" | ||
| /> | ||
|
|
||
| <box flexDirection="row" marginTop={1} marginBottom={1}> | ||
| <box flexDirection="column" width={25}> | ||
| <text fg={terminalColors.textMuted}>Source Agent</text> | ||
| <text fg={terminalColors.accent}> | ||
| <b>{sourceInfo?.icon} {sourceInfo?.name}</b> | ||
| </text> | ||
| <Show when={error()}> | ||
| <ErrorState | ||
| message={error()!} | ||
| action={{ label: 'Retry', key: 'r' }} | ||
| compact | ||
| /> | ||
| </Show> | ||
|
|
||
| <Show when={loading()}> | ||
| <Spinner label="Loading agents and skills..." /> | ||
| </Show> | ||
|
|
||
| <Show when={!loading() && !error()}> | ||
| <box flexDirection="row" marginBottom={1}> | ||
| <box flexDirection="column" width={25}> | ||
| <text | ||
| fg={ | ||
| focusedPane() === 'source' | ||
| ? terminalColors.accent | ||
| : terminalColors.textMuted | ||
| } | ||
| > | ||
| Source Agent | ||
| </text> | ||
| <Show when={sourceAgent()}> | ||
| <text fg={terminalColors.accent}> | ||
| <b> | ||
| {AGENT_LOGOS[sourceAgent()!]?.icon || '◇'}{' '} | ||
| {AGENT_LOGOS[sourceAgent()!]?.name || sourceAgent()} | ||
| </b> | ||
| </text> | ||
| <text fg={terminalColors.textMuted}> | ||
| {getAgentFormatInfo(sourceAgent()!).formatName} | ||
| </text> | ||
| </Show> | ||
| </box> | ||
|
|
||
| <box flexDirection="column" width={5} justifyContent="center"> | ||
| <text fg={terminalColors.textMuted}> → </text> | ||
| </box> | ||
|
|
||
| <box flexDirection="column" width={25}> | ||
| <text | ||
| fg={ | ||
| focusedPane() === 'target' | ||
| ? terminalColors.accent | ||
| : terminalColors.textMuted | ||
| } | ||
| > | ||
| Target Agent | ||
| </text> | ||
| <Show when={targetAgent()}> | ||
| <text fg={terminalColors.text}> | ||
| <b> | ||
| {AGENT_LOGOS[targetAgent()!]?.icon || '◇'}{' '} | ||
| {AGENT_LOGOS[targetAgent()!]?.name || targetAgent()} | ||
| </b> | ||
| </text> | ||
| <text fg={terminalColors.textMuted}> | ||
| {getAgentFormatInfo(targetAgent()!).formatName} | ||
| </text> | ||
| </Show> | ||
| </box> | ||
|
|
||
| <Show when={!canDoTranslation()}> | ||
| <text fg={terminalColors.warning}> ⚠ Incompatible</text> | ||
| </Show> | ||
| </box> | ||
|
|
||
| <text fg={terminalColors.textMuted}>─────────────────────────────────────────────</text> | ||
| <text> </text> | ||
|
|
||
| <box flexDirection="row"> | ||
| <box flexDirection="column" width={30}> | ||
| <text | ||
| fg={ | ||
| focusedPane() === 'skills' | ||
| ? terminalColors.accent | ||
| : terminalColors.text | ||
| } | ||
| > | ||
| <b>Select Skill</b> | ||
| </text> | ||
| <text> </text> | ||
|
|
||
| <Show | ||
| when={skills().length > 0} | ||
| fallback={ | ||
| <EmptyState | ||
| title="No skills installed" | ||
| description="Install skills first" | ||
| action={{ label: 'Browse', key: 'b' }} | ||
| compact | ||
| /> | ||
| } | ||
| > | ||
| <For each={skills().slice(0, 6)}> | ||
| {(skill, idx) => { | ||
| const selected = () => | ||
| focusedPane() === 'skills' && idx() === selectedSkillIndex(); | ||
| return ( | ||
| <box marginBottom={1}> | ||
| <text | ||
| fg={selected() ? terminalColors.accent : terminalColors.text} | ||
| > | ||
| {selected() ? '▸ ' : ' '} | ||
| {skill.name} | ||
| </text> | ||
| </box> | ||
| ); | ||
| }} | ||
| </For> | ||
| <Show when={skills().length > 6}> | ||
| <text fg={terminalColors.textMuted}>+{skills().length - 6} more</text> | ||
| </Show> | ||
| </Show> | ||
| </box> | ||
|
|
||
| <box width={2}> | ||
| <text fg={terminalColors.border}>│</text> | ||
| </box> | ||
|
|
||
| <box flexDirection="column" width={35}> | ||
| <Show when={focusedPane() !== 'skills'}> | ||
| <text fg={terminalColors.text}> | ||
| <b>{focusedPane() === 'source' ? 'Source' : 'Target'} Agents</b> | ||
| </text> | ||
| <text> </text> | ||
| <For each={visibleAgents()}> | ||
| {(agent, idx) => { | ||
| const isSource = focusedPane() === 'source'; | ||
| const selectedIdx = isSource ? sourceAgentIndex() : targetAgentIndex(); | ||
| const selected = () => idx() === selectedIdx; | ||
| return ( | ||
| <box marginBottom={1}> | ||
| <text | ||
| fg={selected() ? terminalColors.accent : terminalColors.textSecondary} | ||
| > | ||
| {selected() ? '▸ ' : ' '} | ||
| {AGENT_LOGOS[agent]?.icon || '◇'} {AGENT_LOGOS[agent]?.name || agent} | ||
| </text> | ||
| </box> | ||
| ); | ||
| }} | ||
| </For> | ||
| </Show> |
There was a problem hiding this comment.
🟡 Translate screen agent selection breaks when navigating beyond first 6 agents (selection becomes invisible / wrong agent shown)
The Translate screen only renders the first 6 supported agents (visibleAgents = supportedAgents().slice(0, 6)), but keyboard navigation allows sourceAgentIndex/targetAgentIndex to move across the full supportedAgents().length.
Actual behavior: after moving the selection index past 5, the UI still shows only the first 6 agents and no longer highlights the selected agent, while sourceAgent()/targetAgent() will point at agents outside the rendered list. This makes the currently-selected agent effectively invisible and can lead to translating to/from an agent the user can’t see/select accurately.
Expected behavior: either clamp indices to the visible range, or implement scrolling/windowing so the selected agent is always within the rendered visibleAgents list.
Click to expand
- Rendering is hard-limited to first 6 agents:
const visibleAgents = () => supportedAgents().slice(0, 6);(packages/tui/src/screens/Translate.tsx:194) - But navigation clamps to full agent list length:
const max = supportedAgents().length - 1; setTargetAgentIndex((prev) => ... max)(packages/tui/src/screens/Translate.tsx:163-169) - Selection highlight compares
idx()from sliced list to the global index (sourceAgentIndex()/targetAgentIndex()), so it can never match once the index exceeds 5 (packages/tui/src/screens/Translate.tsx:332-346).
Recommendation: Make agent list windowed/scrollable (similar to other list components) so visibleAgents() depends on the selected index, or clamp sourceAgentIndex/targetAgentIndex to visibleAgents().length - 1 when you intentionally only support choosing among the first 6.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const paths = [ | ||
| join(__dirname, '..', 'package.json'), | ||
| join(__dirname, '..', '..', 'package.json'), | ||
| join(__dirname, '..', '..', '..', 'package.json'), | ||
| ]; | ||
| for (const packagePath of paths) { | ||
| try { | ||
| const packageJson = JSON.parse(readFileSync(packagePath, 'utf-8')); | ||
| if (packageJson.name === '@skillkit/tui' || packageJson.name === 'skillkit') { | ||
| return packageJson.version || '1.8.0'; | ||
| } | ||
| } catch {} | ||
| } | ||
| return '1.8.0'; | ||
| } catch { | ||
| return '1.0.0'; | ||
| return '1.8.0'; |
There was a problem hiding this comment.
🟡 getVersion() can return stale hardcoded version even when package.json version changes
getVersion() now falls back to the hardcoded string '1.8.0' if it can’t find/read a matching package.json, which means the UI can silently display the wrong version in cases where the layout changes (monorepo structure changes, build output moved, etc.).
Actual behavior: version can incorrectly show 1.8.0 even if the actual package version is different.
Expected behavior: version should either be derived reliably (e.g., injected at build time) or fall back to a neutral value ('unknown') rather than a specific pinned version.
Click to expand
- Hardcoded fallback is returned when no matching package.json found:
return '1.8.0';(packages/tui/src/utils/helpers.ts:84-86) - The search checks only a few relative paths, which may not cover all runtime layouts:
paths = [ join(__dirname, '..', 'package.json'), ... ](packages/tui/src/utils/helpers.ts:71-75).
Recommendation: Avoid hardcoding a specific version as fallback. Consider (a) importing version from package.json via bundler define, (b) using an environment/build-time constant, or (c) falling back to 'unknown' if the version cannot be resolved.
Was this helpful? React with 👍 or 👎 to provide feedback.
Execute, Marketplace, Recommend screens now use windowed list rendering that follows the selection, preventing the selected item from being off-screen when navigating through long lists. - Add createMemo for computing visible window based on selectedIndex - Show ▲/▼ indicators when items are above/below the visible window - Navigate through full list instead of clamping to visible window size
| const handleSyncAll = async () => { | ||
| setSyncStatus('syncing'); | ||
| syncTimeoutRef.current = setTimeout(() => setSyncStatus('done'), 2000); | ||
| }, [syncStatus]); | ||
| setSyncMessage('Syncing to all available agents...'); | ||
|
|
||
| // Clamp selectedIndex when agents list shrinks | ||
| useEffect(() => { | ||
| setSelectedIndex(prev => Math.max(0, Math.min(prev, agents.length - 1))); | ||
| }, [agents.length]); | ||
| try { | ||
| await new Promise((resolve) => setTimeout(resolve, 2000)); | ||
|
|
||
| setAgents((prev) => | ||
| prev.map((a) => | ||
| a.available ? { ...a, synced: true, skillCount: skillCount() } : a | ||
| ) | ||
| ); | ||
|
|
||
| const syncedCount = agents().filter((a) => a.available).length; | ||
| setSyncStatus('done'); | ||
| setSyncMessage(`Synced to ${syncedCount} agents`); | ||
| } catch { |
There was a problem hiding this comment.
🟡 Sync-all success message uses stale agents() state, reporting wrong synced agent count
After handleSyncAll() marks agents as synced via setAgents((prev) => ...), it computes syncedCount from agents() immediately after. In Solid, setAgents updates synchronously, but reading agents() inside the same tick can still yield the pre-update value depending on batching; regardless, this code is logically inconsistent because it counts from the external signal instead of the prev array it just transformed.
Actual vs expected:
- Actual:
Synced to ${syncedCount} agentsmay be incorrect (often0) even though the UI list shows agents marked synced. - Expected: The success message should reflect the number of agents that were synced in that operation.
Code path
setAgents((prev) =>
prev.map((a) => (a.available ? { ...a, synced: true, skillCount: skillCount() } : a))
);
const syncedCount = agents().filter((a) => a.available).length;
setSyncMessage(`Synced to ${syncedCount} agents`);packages/tui/src/screens/Sync.tsx:118-127
Recommendation: Compute the count from the same data used to update the signal, e.g. inside the updater: setAgents(prev => { const next = ...; const count = next.filter(a => a.available).length; setSyncMessage(...count...); return next; }), or compute from agentList.filter(a => a.available).length before/after update.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/tui/src/screens/Marketplace.tsx`:
- Around line 74-90: filteredSkills can shrink causing selectedIndex to point
past the end; add a reactive clamp so selectedIndex is kept within bounds
whenever filteredSkills (or searchQuery) changes. Create a createEffect that
depends on filteredSkills() (or searchQuery()) and calls
setSelectedIndex(Math.max(0, Math.min(selectedIndex(), Math.max(0,
filteredSkills().length - 1)))) or sets to -1 if you prefer no selection when
empty; reference filteredSkills, selectedIndex, setSelectedIndex and
handleCategorySelect to implement this behavior consistently.
- Around line 46-72: The loadData function never sets the error signal so
ErrorState never renders; after the fetch loop in loadData, check failed.length
against the attempted repo count (DEFAULT_REPOS.slice(0, 5).length) and call
setError with a descriptive message (e.g., include failed repo names from
failed) when all repos failed (or set a partial error message when some failed),
otherwise ensure setError(null) on success; update references to setError,
failedRepos (failed array) and setFailedRepos accordingly so the ErrorState
component can render when appropriate.
In `@packages/tui/src/screens/Recommend.tsx`:
- Around line 43-55: The loadData function currently awaits analyzeProject() and
getRecommendations() without error handling, which can leave loading/analyzing
flags set and prevent ErrorState from rendering; wrap the async flow in a
try/catch inside loadData, call analyzeProject() and getRecommendations() in the
try, update setProjectInfo and setState with the successful result there, and in
the catch setState to clear loading and analyzing and set an error message
(e.g., setState(s => ({ ...s, loading: false, analyzing: false, error: err })))
so failures surface in the UI; ensure a finally block or end-of-flow also clears
loading/analyzing if not already cleared.
- Around line 54-56: After calling getRecommendations() and updating the list
via setState(result) you must clamp the selectedIndex to the new bounds so it
never points past result.length-1; update the selected index (e.g. via
setSelectedIndex or the selectedIndex state) to Math.max(0,
Math.min(selectedIndex, result.length - 1)) (or equivalent) immediately after
setState(result) so the highlight and Enter behavior remain valid when the
refreshed list is shorter.
🧹 Nitpick comments (3)
packages/tui/src/screens/Marketplace.tsx (2)
54-66: Consider parallelizing repo fetches.The sequential
for...ofwithawaitfetches repos one at a time, which can be slow. If rate limiting isn't a concern,Promise.allSettledwould fetch in parallel.⚡ Optional parallel fetch
- for (const repo of DEFAULT_REPOS.slice(0, 5)) { - try { - const result = await fetchRepoSkills(repo.source, DEFAULT_REPOS); - if (result.skills.length > 0) { - skills.push(...result.skills); - loaded.push(repo.name); - } else if (result.error) { - failed.push(repo.name); - } - } catch { - failed.push(repo.name); - } - } + const results = await Promise.allSettled( + DEFAULT_REPOS.slice(0, 5).map((repo) => + fetchRepoSkills(repo.source, DEFAULT_REPOS).then((r) => ({ repo, result: r })) + ) + ); + + for (const res of results) { + if (res.status === 'fulfilled') { + const { repo, result } = res.value; + if (result.skills.length > 0) { + skills.push(...result.skills); + loaded.push(repo.name); + } else if (result.error) { + failed.push(repo.name); + } + } else { + // Promise rejected - need repo reference from closure + } + }
14-14:SearchInputis imported andsearchQuerysignal created but never used.
SearchInputis imported at line 14 andsearchQuerysignal is created at line 34, but there's no<SearchInput>in the render output. The search query can only be cleared via Escape, never set. Either remove the unused imports/state or add the search UI.🔧 If search is intended, add the component
<text fg={terminalColors.textMuted}>─────────────────────────────────────────────</text> <text> </text> + <SearchInput + value={searchQuery()} + onChange={setSearchQuery} + placeholder="Search skills..." + /> + <box flexDirection="row" marginBottom={1}> <text fg={terminalColors.text}>Categories: </text>Or if search is not needed, remove the unused code:
-import { SearchInput } from '../components/SearchInput.js';- const [searchQuery, setSearchQuery] = createSignal('');Also applies to: 34-34
packages/tui/src/screens/Recommend.tsx (1)
97-103: Consider reusing the shared grading helper.
getGradeduplicates the logic inpackages/core/src/quality/benchmark.ts. Importing the shared helper would keep grading consistent and avoid drift.
Marketplace.tsx: - Set error signal when all repos fail or no skills loaded - Add reactive effect to clamp selectedIndex when filteredSkills changes Recommend.tsx: - Wrap loadData in try/catch to surface errors in UI - Clamp selectedIndex after recommendations update
| const loadData = async () => { | ||
| setState((s) => ({ ...s, loading: true, error: null })); | ||
| const result = await loadPluginsList(); | ||
| setState(result); | ||
| }; | ||
|
|
||
| const handleToggle = async () => { | ||
| const plugins = state().plugins; | ||
| const manager = state().manager; | ||
| if (plugins.length === 0 || !manager) return; | ||
|
|
||
| const plugin = plugins[selectedIndex()]; | ||
| if (!plugin) return; | ||
|
|
||
| setToggling(true); | ||
|
|
||
| if (plugin.enabled) { | ||
| await disablePlugin(plugin.name, manager); | ||
| } else { | ||
| await enablePlugin(plugin.name, manager); | ||
| } | ||
|
|
||
| await loadData(); | ||
| setToggling(false); | ||
| }; | ||
|
|
||
| const handleShowDetail = async () => { | ||
| const plugins = state().plugins; | ||
| const manager = state().manager; | ||
| if (plugins.length === 0 || !manager) return; | ||
|
|
||
| const plugin = plugins[selectedIndex()]; | ||
| if (!plugin) return; | ||
|
|
||
| const info = await getPluginInfo(plugin.name, manager); | ||
| setSelectedPlugin(info as Plugin | null); | ||
| setShowDetail(true); | ||
| }; |
There was a problem hiding this comment.
🔴 Plugins screen can hang or crash due to missing error handling in async service calls
The Plugins screen calls async service functions without any try/catch or finally.
loadData()awaitsloadPluginsList()without catching errors. If the service throws/rejects, the promise rejection propagates to the keyboard/event loop;state.loadingwas set totrueand will never be reset, leaving the UI stuck in a loading state.handleToggle()setstoggling(true)but does not wrapenablePlugin/disablePlugin+loadData()intry/finally. If any of those throw,togglingstaystrueand the screen can also crash due to an unhandled rejection.handleShowDetail()also awaitsgetPluginInfo()with no error handling, so it can crash the TUI on failure.
Code references
packages/tui/src/screens/Plugins.tsx:42-46(loadDatahas no error handling)packages/tui/src/screens/Plugins.tsx:48-66(handleTogglelacks try/finally)packages/tui/src/screens/Plugins.tsx:68-79(handleShowDetaillacks try/catch)
Impact: transient IO/plugin-manager failures can crash the app or wedge the UI in a perpetual loading/toggling state. Expected behavior is to surface an error and recover gracefully.
Recommendation: Wrap loadPluginsList, enablePlugin/disablePlugin, and getPluginInfo calls in try/catch (set state.error) and use finally to reliably reset loading/toggling flags.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const handleExecute = async () => { | ||
| if (executing()) return; | ||
|
|
||
| export function Workflow({ onNavigate, cols = 80, rows = 24 }: WorkflowProps) { | ||
| const [selectedIndex, setSelectedIndex] = useState(0); | ||
| const [animPhase, setAnimPhase] = useState(0); | ||
| const workflows = state().workflows; | ||
| if (workflows.length === 0) return; | ||
|
|
||
| const isCompact = cols < 60; | ||
| const contentWidth = Math.max(1, Math.min(cols - 4, 60)); | ||
| const workflow = workflows[selectedIndex()]; | ||
| if (!workflow) return; | ||
|
|
||
| // Entrance animation | ||
| useEffect(() => { | ||
| if (animPhase >= 2) return; | ||
| const timer = setTimeout(() => setAnimPhase(p => p + 1), 100); | ||
| return () => clearTimeout(timer); | ||
| }, [animPhase]); | ||
| setExecuting(true); | ||
| try { | ||
| const result = await executeWorkflow(workflow.name, undefined, (progress) => { | ||
| setState((s) => ({ ...s, progress })); | ||
| }); | ||
|
|
||
| const successCount = WORKFLOWS.filter(w => w.status === 'success').length; | ||
| const failedCount = WORKFLOWS.filter(w => w.status === 'failed').length; | ||
| if (result) { | ||
| loadData(); | ||
| } | ||
| } finally { | ||
| setExecuting(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔴 Workflow execution can crash TUI due to unhandled rejection from executeWorkflow
handleExecute() uses try/finally but does not catch errors from executeWorkflow(...). If executeWorkflow rejects (e.g., file read error, invalid workflow, runtime error), the rejection propagates out of the keyboard handler.
Even though executing is reset in finally, the unhandled error can still tear down the render loop and exit via the global unhandledRejection handler (added in src/index.tsx). Expected behavior for a screen action is to show an in-UI error state and keep the app running.
Code references
packages/tui/src/screens/Workflow.tsx:50-71(executeWorkflowawaited without catch)- Global fatal handler that will exit the app:
packages/tui/src/index.tsx:31-35
Impact: a recoverable workflow execution error can force a full TUI shutdown instead of being handled locally.
Recommendation: Add catch in handleExecute to set state.error (and/or a dedicated execution error) while keeping the TUI alive; only fall back to global fatal handlers for truly unexpected errors.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const loadData = async () => { | ||
| setState((s) => ({ ...s, loading: true, error: null })); | ||
| const result = await loadWorkflowsList(); | ||
| setState(result); | ||
| }; |
There was a problem hiding this comment.
🟡 Workflow list loading can leave screen stuck in loading state on service failure
loadData() in the Workflow screen sets loading: true and then awaits loadWorkflowsList() without try/catch. If the service rejects, loading is never cleared and the screen will remain in the loading spinner indefinitely.
Code references
packages/tui/src/screens/Workflow.tsx:44-48
Impact: workflow discovery failures wedge the screen instead of showing an error with retry.
Recommendation: Wrap the await in try/catch, set state.error, and ensure loading becomes false in finally or by setting it in the error path.
Was this helpful? React with 👍 or 👎 to provide feedback.
Plugins.tsx: - Wrap loadData in try/catch to prevent stuck loading state - Wrap handleToggle in try/catch/finally to always reset toggling flag - Wrap handleShowDetail in try/catch to surface errors gracefully Workflow.tsx: - Wrap loadData in try/catch to prevent stuck loading state - Add catch block to handleExecute to show execution errors in UI
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/tui/src/screens/Workflow.tsx`:
- Around line 67-72: Before calling executeWorkflow, clear any stale UI state by
resetting error and progress in the component state; specifically, update the
state (via setState or equivalent) to set error: null (or undefined per your
state shape) and progress: 0 (or an initial value) before calling
setExecuting(true) and executeWorkflow. Locate the start-of-execution sequence
around setExecuting, executeWorkflow, and the progress callback, and ensure you
call setState({ ...s, error: null, progress: 0 }) (or your state initializer)
immediately before toggling executing and starting executeWorkflow so old
error/progress cannot flash during a new run.
| setExecuting(true); | ||
| try { | ||
| const result = await executeWorkflow(workflow.name, undefined, (progress) => { | ||
| setState((s) => ({ ...s, progress })); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Clear stale error/progress before starting execution.
If a previous error is set, the UI stays in the error state even when a new execute starts; progress can also briefly show old values. Consider resetting error and progress before setting executing.
🔧 Proposed fix
const workflow = workflows[selectedIndex()];
if (!workflow) return;
+ setState((s) => ({ ...s, error: null, progress: null }));
setExecuting(true);
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setExecuting(true); | |
| try { | |
| const result = await executeWorkflow(workflow.name, undefined, (progress) => { | |
| setState((s) => ({ ...s, progress })); | |
| }); | |
| const handleExecute = async () => { | |
| const workflows = state().workflows; | |
| if (workflows.length === 0) return; | |
| const workflow = workflows[selectedIndex()]; | |
| if (!workflow) return; | |
| setState((s) => ({ ...s, error: null, progress: null })); | |
| setExecuting(true); | |
| try { | |
| const result = await executeWorkflow(workflow.name, undefined, (progress) => { | |
| setState((s) => ({ ...s, progress })); | |
| }); | |
| if (result) { | |
| await loadData(); | |
| } | |
| } catch (err) { | |
| setState((s) => ({ | |
| ...s, | |
| error: err instanceof Error ? err.message : 'Failed to execute workflow', | |
| })); | |
| } finally { | |
| setExecuting(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@packages/tui/src/screens/Workflow.tsx` around lines 67 - 72, Before calling
executeWorkflow, clear any stale UI state by resetting error and progress in the
component state; specifically, update the state (via setState or equivalent) to
set error: null (or undefined per your state shape) and progress: 0 (or an
initial value) before calling setExecuting(true) and executeWorkflow. Locate the
start-of-execution sequence around setExecuting, executeWorkflow, and the
progress callback, and ensure you call setState({ ...s, error: null, progress: 0
}) (or your state initializer) immediately before toggling executing and
starting executeWorkflow so old error/progress cannot flash during a new run.
| const handleKeyNav = (delta: number) => { | ||
| if (focusedPane() === 'skills') { | ||
| const max = skills().length - 1; | ||
| setSelectedSkillIndex((prev) => Math.max(0, Math.min(prev + delta, max))); | ||
| } else if (focusedPane() === 'source') { | ||
| const max = supportedAgents().length - 1; | ||
| setSourceAgentIndex((prev) => Math.max(0, Math.min(prev + delta, max))); | ||
| } else { | ||
| const max = supportedAgents().length - 1; | ||
| setTargetAgentIndex((prev) => Math.max(0, Math.min(prev + delta, max))); | ||
| } | ||
| }; | ||
|
|
||
| useKeyboard((key: { name?: string }) => { | ||
| if (key.name === 'j' || key.name === 'down') handleKeyNav(1); | ||
| else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); | ||
| else if (key.name === 'tab') { | ||
| setFocusedPane((p) => { | ||
| if (p === 'skills') return 'source'; | ||
| if (p === 'source') return 'target'; | ||
| return 'skills'; | ||
| }); | ||
| } else if (key.name === 'p') handlePreview(); | ||
| else if (key.name === 'return' || key.name === 't') handleTranslate(); | ||
| else if (key.name === 'r') loadData(); | ||
| else if (key.name === 'escape') { | ||
| if (result() || preview()) { | ||
| setResult(null); | ||
| setPreview(null); | ||
| } else { | ||
| props.onNavigate('home'); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const visibleAgents = () => supportedAgents().slice(0, 6); | ||
|
|
||
| return ( | ||
| <box flexDirection="column" paddingLeft={1}> | ||
| <Header | ||
| title="Translate Skills" | ||
| subtitle="Convert skills between agent formats" | ||
| icon="⇄" | ||
| icon="⇄" | ||
| /> | ||
|
|
||
| <box flexDirection="row" marginTop={1} marginBottom={1}> | ||
| <box flexDirection="column" width={25}> | ||
| <text fg={terminalColors.textMuted}>Source Agent</text> | ||
| <text fg={terminalColors.accent}> | ||
| <b>{sourceInfo?.icon} {sourceInfo?.name}</b> | ||
| </text> | ||
| <Show when={error()}> | ||
| <ErrorState | ||
| message={error()!} | ||
| action={{ label: 'Retry', key: 'r' }} | ||
| compact | ||
| /> | ||
| </Show> | ||
|
|
||
| <Show when={loading()}> | ||
| <Spinner label="Loading agents and skills..." /> | ||
| </Show> | ||
|
|
||
| <Show when={!loading() && !error()}> | ||
| <box flexDirection="row" marginBottom={1}> | ||
| <box flexDirection="column" width={25}> | ||
| <text | ||
| fg={ | ||
| focusedPane() === 'source' | ||
| ? terminalColors.accent | ||
| : terminalColors.textMuted | ||
| } | ||
| > | ||
| Source Agent | ||
| </text> | ||
| <Show when={sourceAgent()}> | ||
| <text fg={terminalColors.accent}> | ||
| <b> | ||
| {AGENT_LOGOS[sourceAgent()!]?.icon || '◇'}{' '} | ||
| {AGENT_LOGOS[sourceAgent()!]?.name || sourceAgent()} | ||
| </b> | ||
| </text> | ||
| <text fg={terminalColors.textMuted}> | ||
| {getAgentFormatInfo(sourceAgent()!).formatName} | ||
| </text> | ||
| </Show> | ||
| </box> | ||
|
|
||
| <box flexDirection="column" width={5} justifyContent="center"> | ||
| <text fg={terminalColors.textMuted}> → </text> | ||
| </box> | ||
|
|
||
| <box flexDirection="column" width={25}> | ||
| <text | ||
| fg={ | ||
| focusedPane() === 'target' | ||
| ? terminalColors.accent | ||
| : terminalColors.textMuted | ||
| } | ||
| > | ||
| Target Agent | ||
| </text> | ||
| <Show when={targetAgent()}> | ||
| <text fg={terminalColors.text}> | ||
| <b> | ||
| {AGENT_LOGOS[targetAgent()!]?.icon || '◇'}{' '} | ||
| {AGENT_LOGOS[targetAgent()!]?.name || targetAgent()} | ||
| </b> | ||
| </text> | ||
| <text fg={terminalColors.textMuted}> | ||
| {getAgentFormatInfo(targetAgent()!).formatName} | ||
| </text> | ||
| </Show> | ||
| </box> | ||
|
|
||
| <Show when={!canDoTranslation()}> | ||
| <text fg={terminalColors.warning}> ⚠ Incompatible</text> | ||
| </Show> | ||
| </box> | ||
|
|
||
| <text fg={terminalColors.textMuted}>─────────────────────────────────────────────</text> | ||
| <text> </text> | ||
|
|
||
| <box flexDirection="row"> | ||
| <box flexDirection="column" width={30}> | ||
| <text | ||
| fg={ | ||
| focusedPane() === 'skills' | ||
| ? terminalColors.accent | ||
| : terminalColors.text | ||
| } | ||
| > | ||
| <b>Select Skill</b> | ||
| </text> | ||
| <text> </text> | ||
|
|
||
| <Show | ||
| when={skills().length > 0} | ||
| fallback={ | ||
| <EmptyState | ||
| title="No skills installed" | ||
| description="Install skills first" | ||
| action={{ label: 'Browse', key: 'b' }} | ||
| compact | ||
| /> | ||
| } | ||
| > | ||
| <For each={skills().slice(0, 6)}> | ||
| {(skill, idx) => { | ||
| const selected = () => | ||
| focusedPane() === 'skills' && idx() === selectedSkillIndex(); | ||
| return ( | ||
| <box marginBottom={1}> | ||
| <text | ||
| fg={selected() ? terminalColors.accent : terminalColors.text} | ||
| > | ||
| {selected() ? '▸ ' : ' '} | ||
| {skill.name} | ||
| </text> | ||
| </box> | ||
| ); | ||
| }} | ||
| </For> | ||
| <Show when={skills().length > 6}> | ||
| <text fg={terminalColors.textMuted}>+{skills().length - 6} more</text> | ||
| </Show> |
There was a problem hiding this comment.
🟡 Translate screen lets selection move beyond rendered lists (selection becomes invisible / wrong item translated)
The Translate screen only renders the first 6 skills and first 6 agents, but keyboard navigation allows selecting any index in the full arrays. This can make the currently-selected agent/skill invisible, and actions (preview/translate) operate on an item the user cannot see.
Click to expand
Skills list mismatch
- Navigation allows selecting up to
skills().length - 1regardless of what’s displayed:Translate.tsx:159-169. - UI renders only
skills().slice(0, 6):Translate.tsx:299-318.
If there are >6 skills, pressing j/k can move selectedSkillIndex() beyond 5, but the UI still shows only the first 6 skills. The user can end up translating a different skill than the one they think is selected.
Agents list mismatch
- Navigation allows selecting up to
supportedAgents().length - 1:Translate.tsx:159-169. - UI renders only
visibleAgents() = supportedAgents().slice(0, 6)and usesidx()from that slice to compare against the absolute selected index:Translate.tsx:194-195,Translate.tsx:329-347.
So selecting agent index >= 6 can never be highlighted in the rendered list.
Impact: confusing UX and potentially translating/executing against the wrong selection.
Recommendation: Implement a scrolling window (like other screens’ *Window/pagination logic) for both skills and agents, and render the windowed slice while keeping selection indices consistent with the slice offset.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (key.name === 'j' || key.name === 'down') handleKeyNav(1); | ||
| else if (key.name === 'k' || key.name === 'up') handleKeyNav(-1); | ||
| else if (key.sequence === '/') setSearchMode(true); | ||
| else if (key.name === 'return') { | ||
| const repo = filteredRepos[selectedIndex]; | ||
| if (repo) { | ||
| // Open/view the selected repo (navigate to installed to see skills from this repo) | ||
| onNavigate('installed'); | ||
| } | ||
| else if (key.name === 'return') handleOpenRepo(); | ||
| else if (key.name === 'r') loadData(); | ||
| else if (key.name === 'escape') { | ||
| if (showSkills()) setShowSkills(false); | ||
| else if (searchQuery()) setSearchQuery(''); | ||
| else props.onNavigate('home'); | ||
| } | ||
| else if (key.name === 'escape') onNavigate('home'); | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (selectedIndex >= filteredRepos.length && filteredRepos.length > 0) { | ||
| setSelectedIndex(filteredRepos.length - 1); | ||
| createEffect(() => { | ||
| if (selectedIndex() >= filteredRepos().length && filteredRepos().length > 0) { | ||
| setSelectedIndex(filteredRepos().length - 1); | ||
| } | ||
| }, [filteredRepos.length, selectedIndex]); | ||
| }); | ||
|
|
||
| // Calculate visible repos | ||
| const maxVisible = Math.max(5, rows - 10); | ||
| const startIdx = Math.max(0, Math.min(selectedIndex - Math.floor(maxVisible / 2), filteredRepos.length - maxVisible)); | ||
| const visibleRepos = filteredRepos.slice(startIdx, startIdx + maxVisible); | ||
| const selectedRepo = () => { | ||
| const repoList = filteredRepos(); | ||
| if (repoList.length === 0) return null; | ||
| return repoList[selectedIndex()]; | ||
| }; | ||
|
|
||
| const divider = useMemo(() => | ||
| <text fg={terminalColors.textMuted}>{'─'.repeat(contentWidth)}</text>, | ||
| [contentWidth] | ||
| ); | ||
| const repoDetailFields = () => { | ||
| const repo = selectedRepo(); | ||
| if (!repo) return []; | ||
|
|
||
| const shortcuts = isCompact | ||
| ? 'j/k nav enter open / search esc back' | ||
| : 'j/k navigate enter open / search esc back'; | ||
| return [ | ||
| { label: 'Name', value: repo.name }, | ||
| { label: 'Source', value: repo.source }, | ||
| { label: 'Skills', value: String(repo.skills.length) }, | ||
| { | ||
| label: 'Status', | ||
| value: repo.loading ? 'Loading...' : repo.error ? 'Error' : 'Ready', | ||
| }, | ||
| ]; | ||
| }; | ||
|
|
||
| return ( | ||
| <box flexDirection="column" padding={1}> | ||
| {/* Header */} | ||
| {animPhase >= 1 && ( | ||
| <box flexDirection="column"> | ||
| <box flexDirection="row" justifyContent="space-between" width={contentWidth}> | ||
| <text fg={terminalColors.browse}>⌕ Browse</text> | ||
| <text fg={terminalColors.textMuted}>{filteredRepos.length} repos</text> | ||
| </box> | ||
| <text fg={terminalColors.textMuted}>explore skill repositories</text> | ||
| <text> </text> | ||
| <box flexDirection="column" paddingLeft={1}> | ||
| <Header | ||
| title="Browse" | ||
| subtitle="Explore skill repositories" | ||
| icon="⌕" | ||
| count={repos().length} | ||
| /> | ||
|
|
||
| <Show when={error()}> | ||
| <ErrorState | ||
| message={error()!} | ||
| action={{ label: 'Retry', key: 'r' }} | ||
| compact | ||
| /> | ||
| </Show> | ||
|
|
||
| <Show when={loading()}> | ||
| <Spinner label="Loading repositories..." /> | ||
| </Show> | ||
|
|
||
| <Show when={!loading() && !error()}> | ||
| <box flexDirection="row" marginBottom={1}> | ||
| <text fg={terminalColors.textMuted}>/ </text> | ||
| <text | ||
| fg={ | ||
| searchQuery() || searchMode() | ||
| ? terminalColors.text | ||
| : terminalColors.textMuted | ||
| } | ||
| > | ||
| {searchQuery() || (searchMode() ? '|' : 'type to filter...')} | ||
| </text> | ||
| </box> |
There was a problem hiding this comment.
🟡 Browse screen search input UI doesn’t accept characters unless “searchMode” is explicitly entered
Browse shows a search prompt (/ type to filter...) even when not in searchMode, but keystrokes are only appended to searchQuery when searchMode() is true.
Click to expand
- Search mode is entered only via
/:Browse.tsx:164-166. - Character input handling occurs only inside
if (searchMode()) { ... }:Browse.tsx:144-160. - But the UI displays
type to filter...outside of search mode:Browse.tsx:222-234.
Actual: typing letters without pressing / does nothing.
Expected: either the UI should indicate “press / to search”, or allow direct typing to start filtering.
(Refers to lines 143-234)
Recommendation: Either (a) change the hint text to explicitly say “press / to search”, or (b) treat alphanumeric keys outside searchMode as starting searchMode and appending to the query.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export async function startTUI(): Promise<never> { | ||
| // Setup signal handlers for clean exit | ||
| const handleSignal = () => exitTUI(0); | ||
| process.on('SIGINT', handleSignal); | ||
| process.on('SIGTERM', handleSignal); | ||
|
|
||
| const renderer = await createCliRenderer({ exitOnCtrlC: false }); | ||
| rootInstance = createRoot(renderer); | ||
| rootInstance.render(<App onExit={exitTUI} />); | ||
| process.on('SIGINT', () => exitTUI(0)); | ||
| process.on('SIGTERM', () => exitTUI(0)); | ||
| process.on('uncaughtException', (err) => { | ||
| console.error('TUI Error:', err.message); | ||
| exitTUI(1); | ||
| }); | ||
| process.on('unhandledRejection', (err) => { | ||
| console.error('TUI Error:', err); | ||
| exitTUI(1); | ||
| }); |
There was a problem hiding this comment.
🟡 startTUI installs process-level handlers without cleanup (handlers accumulate if TUI restarted)
startTUI() registers SIGINT/SIGTERM/uncaughtException/unhandledRejection handlers but never removes them. If startTUI() is called more than once in the same Node process (e.g., programmatically restarting the TUI), handlers will accumulate and fire multiple times.
Click to expand
Handlers are attached unconditionally on each call: packages/tui/src/index.tsx:25-35.
Actual: repeated calls can cause duplicate exit behavior and duplicated logging.
Expected: handlers should be registered once, or removed during exitTUI().
Recommendation: Guard with a handlersInstalled flag or use process.once for signals/exceptions, and/or remove handlers in exitTUI() using named functions.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Changes
Framework Migration
Screens (20 total)
Components (32 total)
Features
Test plan
pnpm buildto verify build succeedsskillkit uito test TUI launchesSummary by CodeRabbit
New Features
New Services
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.