fix(skills): per-card loading state on skill enable#454
Merged
graycyrus merged 5 commits intotinyhumansai:mainfrom Apr 9, 2026
Merged
fix(skills): per-card loading state on skill enable#454graycyrus merged 5 commits intotinyhumansai:mainfrom
graycyrus merged 5 commits intotinyhumansai:mainfrom
Conversation
…stency and usability - Remove dead code: TauriCommandsPanel, useSettingsAnimation, SettingsPanelLayout, SettingsBackButton, ProfilePanel, AdvancedPanel, SkillsPanel, SkillsGrid (~1900 lines) - Standardize settings panel padding to p-4 space-y-4 across all panels - Add breadcrumb navigation to SettingsHeader with route-derived breadcrumbs - Decompose oversized panels: LocalModelPanel, AutocompletePanel, CronJobsPanel, ScreenIntelligencePanel into sub-components in dedicated subdirectories - Deduplicate skills management: move browser access toggle to Skills page, remove redundant /settings/skills route - Unify skill card layout: UnifiedSkillCard component with overflow menu for secondary actions, consistent status/CTA patterns across all skill types - Add skill search bar and category filter with grouped results Closes tinyhumansai#396
- Use semantic nav/ol/li for breadcrumbs with aria-hidden separators - Fix duplicate text-xs class in CompletionStyleSection - Use interface instead of type for CronSkillConfig - Disable cron option inputs when parent skill is disabled - Deduplicate preset error rendering in DeviceCapabilitySection - Fix low-contrast labels: text-stone-300 → text-stone-700, text-stone-200 → text-stone-600 - Disable "Set Path" button when input is empty - Add aria-pressed to category filter buttons - Convert SkillCategoryFilter to arrow function - Use void operator for async onClick handler - Simplify redundant conditional in ThirdPartySkillCard - Use async/await instead of .then() in BrowserAccessToggle
- Mock openhumanGetRuntimeFlags/openhumanSetBrowserAllowAll for BrowserAccessToggle - Open overflow menu before clicking sync/debug buttons (now in secondary actions) - Remove assertion for deleted "3rd Party Skills" heading
…ll list on enable When clicking Enable on a third-party skill (e.g. Gmail, Notion), the entire skill list was replaced with a loading message. Now only the clicked card's button shows "Enabling..." with a disabled state while the install RPC runs, keeping all other cards visible and interactive.
Contributor
📝 WalkthroughWalkthroughThis PR refactors the Settings and Skills UI architecture. It removes deprecated settings panels and helper components, introduces breadcrumb navigation throughout the Settings UI via an updated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 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)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
app/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsx (1)
75-79: Scope the overflow-menu lookup to Gmail’s card.
screen.getByTitle('More actions')will become ambiguous as soon as another card renders the same control, so this test can start failing without any Gmail sync regression. Query the Gmail card first and usewithin(...)for the menu trigger and sync action.As per coding guidelines, "prefer testing behavior over implementation details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsx` around lines 75 - 79, The test currently calls screen.getByTitle('More actions') which is ambiguous; instead locate the Gmail card element first (e.g., query for its heading or a unique container), then scope the overflow-menu lookup to that card using within(gmailCard) to click the More actions button and to find the sync action (test id 'skill-sync-button-gmail'); update the calls so you use within(...) for both the menu trigger and the await find of 'skill-sync-button-gmail' to ensure the lookup is scoped to Gmail's card.app/src/pages/Skills.tsx (1)
142-151: Consider logging runtime flag fetch failures in development.The empty catch block silently swallows errors. While the comment explains the default behavior, logging in development mode could help diagnose connectivity or API issues.
♻️ Optional: Log errors in dev mode
useEffect(() => { (async () => { try { const res = await openhumanGetRuntimeFlags(); setBrowserAllowAll(res.result.browser_allow_all); } catch { - // Silently ignore — toggle defaults to false + // Toggle defaults to false on error + if (IS_DEV) console.debug('[BrowserAccessToggle] Failed to fetch runtime flags'); } })(); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Skills.tsx` around lines 142 - 151, The empty catch in the useEffect around openhumanGetRuntimeFlags swallows errors; update the catch to log the caught error in development only (e.g., check NODE_ENV or an isDev flag) and include context (mention openhumanGetRuntimeFlags and setBrowserAllowAll) so connectivity/API failures are visible during local testing while keeping production behavior unchanged.app/src/components/settings/panels/LocalModelPanel.tsx (1)
418-426: Consider memoizing callback props to prevent unnecessary re-renders.The
onApplyPresetcallback is created inline on each render. For better performance, consider wrapping it withuseCallback.♻️ Optional: Memoize callback
+import { useCallback, useEffect, useMemo, useState } from 'react'; ... +const handleApplyPreset = useCallback((tier: string) => { + void applyPreset(tier); +}, []); ... <DeviceCapabilitySection presetsData={presetsData} presetsLoading={presetsLoading} presetError={presetError} presetSuccess={presetSuccess} isApplyingPreset={isApplyingPreset} - onApplyPreset={tier => void applyPreset(tier)} + onApplyPreset={handleApplyPreset} formatRamGb={formatRamGb} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/LocalModelPanel.tsx` around lines 418 - 426, The inline arrow passed to DeviceCapabilitySection as the onApplyPreset prop causes a new function each render; wrap the applyPreset call in a stable callback using React's useCallback (e.g., create a memoized onApplyPreset that calls applyPreset(tier)) and pass that memoized function to onApplyPreset to avoid needless re-renders; reference the existing applyPreset function and the onApplyPreset prop on DeviceCapabilitySection when implementing the useCallback.app/src/components/settings/panels/autocomplete/AppFilterSection.tsx (1)
131-132: Prefer mutually exclusive feedback for success vs errorLines 131–132 can render both
messageanderrortogether. Consider giving error precedence to avoid contradictory status text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/autocomplete/AppFilterSection.tsx` around lines 131 - 132, The component AppFilterSection currently renders both message and error simultaneously which can show conflicting feedback; update the JSX to make them mutually exclusive by giving error precedence—render error when the error variable is truthy, otherwise render message—so only one status line (error or success) is shown at a time (adjust the conditional rendering around the message and error variables accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/settings/components/SettingsHeader.tsx`:
- Around line 45-77: Add the current page as the final, non-clickable breadcrumb
inside the existing <nav aria-label="Breadcrumb"> so screen readers receive a
complete trail: after rendering the mapped breadcrumbs (the breadcrumbs.map and
per-crumb <li> using crumb.label and crumb.onClick), append one more <li> that
renders the current page title (the same string used in the adjacent <h2>) as a
<span> with aria-current="page" and the same text styling, and ensure the
separator SVG is not rendered after this final item.
In `@app/src/components/settings/hooks/useSettingsNavigation.ts`:
- Around line 150-203: The hook is missing coverage for the "tools" settings
route so currentRoute falls back to 'home' and breadcrumbs/back-navigation
break; add "tools" to the SettingsRoute union/type, ensure getCurrentRoute()
maps the "/settings/tools" path to 'tools', and add a case 'tools' in
getBreadcrumbs (alongside 'ai-tools') returning [settingsCrumb, aiToolsCrumb] so
the breadcrumbs/back navigation behave correctly.
In `@app/src/components/settings/panels/autocomplete/AppFilterSection.tsx`:
- Around line 81-87: The textarea currently rendered with
value={contextOverride} and onChange={event =>
onSetContextOverride(event.target.value)} lacks an accessible label; add a
semantic <label> associated via htmlFor and give the textarea a matching id (for
example id="contextOverride") so assistive tech can identify it, updating the
surrounding <div> text to be a <label htmlFor="contextOverride">Context Override
(optional)</label> and ensure the textarea keeps its value and onChange handlers
(contextOverride and onSetContextOverride).
- Around line 43-45: In AppFilterSection.tsx the three lines rendering
status?.platform_supported, status?.enabled and status?.running currently
default to 'no' when status is null/undefined; update the rendering logic to
show 'unknown' for missing values by checking each property explicitly (e.g.,
for platform_supported, use status?.platform_supported === true ? 'yes' :
status?.platform_supported === false ? 'no' : 'unknown') and apply the same
pattern to enabled and running so users see 'unknown' until status is loaded.
In `@app/src/components/settings/panels/cron/CoreJobList.tsx`:
- Around line 75-76: The UI currently renders new
Date(job.next_run).toLocaleString() directly which can show "Invalid Date" for
malformed timestamps; update the rendering in CoreJobList.tsx (both the
occurrence in the list and the other occurrence around the second spot) to
validate the parsed date first (e.g., create a Date from job.next_run, check
isNaN(date.getTime()) or date.toString() !== 'Invalid Date') and then render
date.toLocaleString() when valid or a stable fallback string like "—" /
"Unknown" when invalid; make this change for the JSX expressions that currently
call new Date(job.next_run).toLocaleString().
In `@app/src/components/settings/panels/cron/RuntimeSkillCronList.tsx`:
- Around line 88-90: The draft fallback currently uses an empty string which can
hide server values; change how drafts are read so that when computing draft (the
variable derived from draftValues[optionKey] in RuntimeSkillCronList.tsx) you
fall back to option.value instead of ''. Update every place that computes draft
(the occurrences around optionKey, draftValues, draft, and busy using savingKey)
so the UI displays the server-provided option.value when no draft exists and
prevents saving accidental empty values.
In `@app/src/components/settings/panels/local-model/ModelStatusSection.tsx`:
- Around line 90-101: Clamp the progress value to [0,1] before using it in both
the inline style width and the percentage text; create a local clampedProgress
(e.g., const clampedProgress = Math.min(Math.max(progress ?? 0, 0), 1)) and
replace raw uses of progress in the width calculation (style={{ width:
`${Math.round((isIndeterminateDownload ? 1 : progress) * 100)}%` }}) and the
percentage rendering (`${Math.round(progress * 100)}%`) so they use
clampedProgress and still respect isIndeterminateDownload and isInstalling
branches.
In
`@app/src/components/settings/panels/screen-intelligence/SessionAndVisionSection.tsx`:
- Around line 114-124: The parent card div wrapping recentVisionSummaries (the
element using key={summary.id}) mistakenly includes the Tailwind class
text-stone-200 which conflicts with its darker child text colors; remove
text-stone-200 from that container's className (or replace it with a suitable
color like text-stone-800 or simply rely on child colors/inherit) so the card
does not unintentionally force a light text color that gets overridden or
inherited inconsistently.
In `@app/src/components/skills/SkillSearchBar.tsx`:
- Around line 28-49: The input and clear button in SkillSearchBar are missing
accessible names; update the input element (the text input using props
value/onChange/placeholder) to include an explicit label or an aria-label (e.g.,
aria-label={placeholder || "Search skills"}) and add an aria-label="Clear
search" to the clear button (the button with onClick={() => onChange('')} and
the SVG) so screen readers can identify both controls.
---
Nitpick comments:
In `@app/src/components/settings/panels/autocomplete/AppFilterSection.tsx`:
- Around line 131-132: The component AppFilterSection currently renders both
message and error simultaneously which can show conflicting feedback; update the
JSX to make them mutually exclusive by giving error precedence—render error when
the error variable is truthy, otherwise render message—so only one status line
(error or success) is shown at a time (adjust the conditional rendering around
the message and error variables accordingly).
In `@app/src/components/settings/panels/LocalModelPanel.tsx`:
- Around line 418-426: The inline arrow passed to DeviceCapabilitySection as the
onApplyPreset prop causes a new function each render; wrap the applyPreset call
in a stable callback using React's useCallback (e.g., create a memoized
onApplyPreset that calls applyPreset(tier)) and pass that memoized function to
onApplyPreset to avoid needless re-renders; reference the existing applyPreset
function and the onApplyPreset prop on DeviceCapabilitySection when implementing
the useCallback.
In `@app/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsx`:
- Around line 75-79: The test currently calls screen.getByTitle('More actions')
which is ambiguous; instead locate the Gmail card element first (e.g., query for
its heading or a unique container), then scope the overflow-menu lookup to that
card using within(gmailCard) to click the More actions button and to find the
sync action (test id 'skill-sync-button-gmail'); update the calls so you use
within(...) for both the menu trigger and the await find of
'skill-sync-button-gmail' to ensure the lookup is scoped to Gmail's card.
In `@app/src/pages/Skills.tsx`:
- Around line 142-151: The empty catch in the useEffect around
openhumanGetRuntimeFlags swallows errors; update the catch to log the caught
error in development only (e.g., check NODE_ENV or an isDev flag) and include
context (mention openhumanGetRuntimeFlags and setBrowserAllowAll) so
connectivity/API failures are visible during local testing while keeping
production behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24d51c27-d6b4-43ed-b00b-a8f265adaf8c
📒 Files selected for processing (50)
.claude/memory.mdapp/src/components/SkillsGrid.tsxapp/src/components/settings/SettingsSectionPage.tsxapp/src/components/settings/components/SettingsBackButton.tsxapp/src/components/settings/components/SettingsHeader.tsxapp/src/components/settings/components/SettingsPanelLayout.tsxapp/src/components/settings/hooks/useSettingsAnimation.tsapp/src/components/settings/hooks/useSettingsNavigation.tsapp/src/components/settings/panels/AIPanel.tsxapp/src/components/settings/panels/AccessibilityPanel.tsxapp/src/components/settings/panels/AdvancedPanel.tsxapp/src/components/settings/panels/AgentChatPanel.tsxapp/src/components/settings/panels/AutocompletePanel.tsxapp/src/components/settings/panels/BillingPanel.tsxapp/src/components/settings/panels/ConnectionsPanel.tsxapp/src/components/settings/panels/CronJobsPanel.tsxapp/src/components/settings/panels/DeveloperOptionsPanel.tsxapp/src/components/settings/panels/LocalModelPanel.tsxapp/src/components/settings/panels/MemoryDataPanel.tsxapp/src/components/settings/panels/MemoryDebugPanel.tsxapp/src/components/settings/panels/MessagingPanel.tsxapp/src/components/settings/panels/PrivacyPanel.tsxapp/src/components/settings/panels/ProfilePanel.tsxapp/src/components/settings/panels/RecoveryPhrasePanel.tsxapp/src/components/settings/panels/ScreenIntelligencePanel.tsxapp/src/components/settings/panels/SkillsPanel.tsxapp/src/components/settings/panels/TauriCommandsPanel.tsxapp/src/components/settings/panels/TeamInvitesPanel.tsxapp/src/components/settings/panels/TeamManagementPanel.tsxapp/src/components/settings/panels/TeamMembersPanel.tsxapp/src/components/settings/panels/TeamPanel.tsxapp/src/components/settings/panels/ToolsPanel.tsxapp/src/components/settings/panels/VoicePanel.tsxapp/src/components/settings/panels/WebhooksDebugPanel.tsxapp/src/components/settings/panels/autocomplete/AppFilterSection.tsxapp/src/components/settings/panels/autocomplete/CompletionStyleSection.tsxapp/src/components/settings/panels/cron/CoreJobList.tsxapp/src/components/settings/panels/cron/RuntimeSkillCronList.tsxapp/src/components/settings/panels/local-model/DeviceCapabilitySection.tsxapp/src/components/settings/panels/local-model/ModelDownloadSection.tsxapp/src/components/settings/panels/local-model/ModelStatusSection.tsxapp/src/components/settings/panels/screen-intelligence/PermissionsSection.tsxapp/src/components/settings/panels/screen-intelligence/SessionAndVisionSection.tsxapp/src/components/skills/SkillCard.tsxapp/src/components/skills/SkillCategoryFilter.tsxapp/src/components/skills/SkillSearchBar.tsxapp/src/pages/Settings.tsxapp/src/pages/Skills.tsxapp/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsxapp/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx
💤 Files with no reviewable changes (9)
- app/src/components/settings/panels/ProfilePanel.tsx
- app/src/components/settings/panels/AdvancedPanel.tsx
- app/src/components/settings/components/SettingsPanelLayout.tsx
- app/src/components/settings/panels/SkillsPanel.tsx
- app/src/components/settings/components/SettingsBackButton.tsx
- app/src/pages/Settings.tsx
- app/src/components/settings/panels/TauriCommandsPanel.tsx
- app/src/components/settings/hooks/useSettingsAnimation.ts
- app/src/components/SkillsGrid.tsx
Comment on lines
+45
to
+77
| {/* Breadcrumbs */} | ||
| {breadcrumbs && breadcrumbs.length > 0 && ( | ||
| <nav aria-label="Breadcrumb" className="mr-1"> | ||
| <ol className="flex items-center gap-1"> | ||
| {breadcrumbs.map((crumb, i) => ( | ||
| <li key={i} className="flex items-center gap-1"> | ||
| {crumb.onClick ? ( | ||
| <button | ||
| onClick={crumb.onClick} | ||
| className="text-xs text-stone-400 hover:text-stone-600 transition-colors"> | ||
| {crumb.label} | ||
| </button> | ||
| ) : ( | ||
| <span className="text-xs text-stone-400">{crumb.label}</span> | ||
| )} | ||
| <svg | ||
| aria-hidden="true" | ||
| className="w-3 h-3 text-stone-300" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| viewBox="0 0 24 24"> | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M9 5l7 7-7 7" | ||
| /> | ||
| </svg> | ||
| </li> | ||
| ))} | ||
| </ol> | ||
| </nav> | ||
| )} |
Contributor
There was a problem hiding this comment.
Include the current page in the breadcrumb nav.
The <nav aria-label="Breadcrumb"> stops at the parent links and the active page name is only rendered in the adjacent <h2>, so screen readers never get a complete breadcrumb trail. Add the current page as the last non-clickable crumb with aria-current="page" instead of keeping it only outside the nav.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/components/SettingsHeader.tsx` around lines 45 -
77, Add the current page as the final, non-clickable breadcrumb inside the
existing <nav aria-label="Breadcrumb"> so screen readers receive a complete
trail: after rendering the mapped breadcrumbs (the breadcrumbs.map and per-crumb
<li> using crumb.label and crumb.onClick), append one more <li> that renders the
current page title (the same string used in the adjacent <h2>) as a <span> with
aria-current="page" and the same text styling, and ensure the separator SVG is
not rendered after this final item.
Comment on lines
+150
to
+203
| const getBreadcrumbs = (): BreadcrumbItem[] => { | ||
| switch (currentRoute) { | ||
| // Section pages | ||
| case 'account': | ||
| case 'automation': | ||
| case 'ai-tools': | ||
| return [settingsCrumb]; | ||
|
|
||
| // Leaf panels under account | ||
| case 'billing': | ||
| case 'recovery-phrase': | ||
| case 'team': | ||
| case 'connections': | ||
| return [settingsCrumb, accountCrumb]; | ||
|
|
||
| // Leaf panels under automation | ||
| case 'accessibility': | ||
| case 'screen-intelligence': | ||
| case 'autocomplete': | ||
| case 'messaging': | ||
| case 'cron-jobs': | ||
| return [settingsCrumb, automationCrumb]; | ||
|
|
||
| // Leaf panels under ai-tools | ||
| case 'voice': | ||
| case 'local-model': | ||
| case 'ai': | ||
| return [settingsCrumb, aiToolsCrumb]; | ||
|
|
||
| // Team sub-pages | ||
| case 'team-members': | ||
| case 'team-invites': | ||
| return [settingsCrumb, accountCrumb, teamCrumb]; | ||
|
|
||
| // Developer sub-pages | ||
| case 'webhooks-debug': | ||
| case 'memory-data': | ||
| case 'memory-debug': | ||
| return [settingsCrumb, developerCrumb]; | ||
|
|
||
| // Other leaf pages | ||
| case 'privacy': | ||
| case 'agent-chat': | ||
| case 'developer-options': | ||
| return [settingsCrumb]; | ||
|
|
||
| case 'home': | ||
| default: | ||
| return []; | ||
| } | ||
| }; | ||
|
|
||
| const breadcrumbs = getBreadcrumbs(); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that every nested /settings route is represented in useSettingsNavigation.
sed -n '1,230p' app/src/components/settings/hooks/useSettingsNavigation.ts
sed -n '252,320p' app/src/pages/Settings.tsxRepository: tinyhumansai/openhuman
Length of output: 9657
Add tools to route coverage in the hook.
app/src/pages/Settings.tsx defines a /settings/tools route, but it's missing from the SettingsRoute type, getCurrentRoute(), and getBreadcrumbs(). When accessed, the route falls back to 'home', resulting in no breadcrumbs and incorrect back-navigation behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/hooks/useSettingsNavigation.ts` around lines 150
- 203, The hook is missing coverage for the "tools" settings route so
currentRoute falls back to 'home' and breadcrumbs/back-navigation break; add
"tools" to the SettingsRoute union/type, ensure getCurrentRoute() maps the
"/settings/tools" path to 'tools', and add a case 'tools' in getBreadcrumbs
(alongside 'ai-tools') returning [settingsCrumb, aiToolsCrumb] so the
breadcrumbs/back navigation behave correctly.
Comment on lines
+43
to
+45
| <div>Platform supported: {status?.platform_supported ? 'yes' : 'no'}</div> | ||
| <div>Enabled: {status?.enabled ? 'yes' : 'no'}</div> | ||
| <div>Running: {status?.running ? 'yes' : 'no'}</div> |
Contributor
There was a problem hiding this comment.
Avoid showing "no" when status is unknown
Lines 43–45 currently render "no" when status is null/undefined, which can mislead users before status loads. Render "unknown" for missing values.
Suggested fix
+ const toYesNoUnknown = (value: boolean | undefined) =>
+ value === true ? 'yes' : value === false ? 'no' : 'unknown';
+
return (
<>
<section className="rounded-2xl border border-stone-200 bg-white p-4 space-y-3">
<h3 className="text-sm font-semibold text-stone-900">Runtime</h3>
<div className="text-sm text-stone-700 space-y-1">
- <div>Platform supported: {status?.platform_supported ? 'yes' : 'no'}</div>
- <div>Enabled: {status?.enabled ? 'yes' : 'no'}</div>
- <div>Running: {status?.running ? 'yes' : 'no'}</div>
+ <div>Platform supported: {toYesNoUnknown(status?.platform_supported)}</div>
+ <div>Enabled: {toYesNoUnknown(status?.enabled)}</div>
+ <div>Running: {toYesNoUnknown(status?.running)}</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/autocomplete/AppFilterSection.tsx` around
lines 43 - 45, In AppFilterSection.tsx the three lines rendering
status?.platform_supported, status?.enabled and status?.running currently
default to 'no' when status is null/undefined; update the rendering logic to
show 'unknown' for missing values by checking each property explicitly (e.g.,
for platform_supported, use status?.platform_supported === true ? 'yes' :
status?.platform_supported === false ? 'no' : 'unknown') and apply the same
pattern to enabled and running so users see 'unknown' until status is loaded.
Comment on lines
+81
to
+87
| <div className="text-xs text-stone-600">Context Override (optional)</div> | ||
| <textarea | ||
| value={contextOverride} | ||
| onChange={event => onSetContextOverride(event.target.value)} | ||
| rows={3} | ||
| className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700" | ||
| /> |
Contributor
There was a problem hiding this comment.
Associate the textarea with a semantic <label>
On Line 81, the text is in a <div>, so the <textarea> on Line 82 has no explicit accessible name for assistive tech. Please use <label htmlFor> + id.
Suggested fix
- <div className="text-xs text-stone-600">Context Override (optional)</div>
+ <label htmlFor="context-override" className="text-xs text-stone-600">
+ Context Override (optional)
+ </label>
<textarea
+ id="context-override"
value={contextOverride}
onChange={event => onSetContextOverride(event.target.value)}
rows={3}
className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700"
/>📝 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.
Suggested change
| <div className="text-xs text-stone-600">Context Override (optional)</div> | |
| <textarea | |
| value={contextOverride} | |
| onChange={event => onSetContextOverride(event.target.value)} | |
| rows={3} | |
| className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700" | |
| /> | |
| <label htmlFor="context-override" className="text-xs text-stone-600"> | |
| Context Override (optional) | |
| </label> | |
| <textarea | |
| id="context-override" | |
| value={contextOverride} | |
| onChange={event => onSetContextOverride(event.target.value)} | |
| rows={3} | |
| className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/autocomplete/AppFilterSection.tsx` around
lines 81 - 87, The textarea currently rendered with value={contextOverride} and
onChange={event => onSetContextOverride(event.target.value)} lacks an accessible
label; add a semantic <label> associated via htmlFor and give the textarea a
matching id (for example id="contextOverride") so assistive tech can identify
it, updating the surrounding <div> text to be a <label
htmlFor="contextOverride">Context Override (optional)</label> and ensure the
textarea keeps its value and onChange handlers (contextOverride and
onSetContextOverride).
Comment on lines
+75
to
+76
| {new Date(job.next_run).toLocaleString()} | ||
| </span> |
Contributor
There was a problem hiding this comment.
Guard timestamp formatting to avoid “Invalid Date” in UI.
If a run/job timestamp is malformed, this currently leaks Invalid Date to users. Add a small formatter fallback.
🛠️ Suggested patch
+const formatDateTime = (value: string): string => {
+ const date = new Date(value);
+ return Number.isNaN(date.getTime()) ? 'n/a' : date.toLocaleString();
+};
+
const CoreJobList = ({
@@
- {new Date(job.next_run).toLocaleString()}
+ {formatDateTime(job.next_run)}
@@
- {new Date(run.finished_at).toLocaleString()}
+ {formatDateTime(run.finished_at)}Also applies to: 129-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/cron/CoreJobList.tsx` around lines 75 -
76, The UI currently renders new Date(job.next_run).toLocaleString() directly
which can show "Invalid Date" for malformed timestamps; update the rendering in
CoreJobList.tsx (both the occurrence in the list and the other occurrence around
the second spot) to validate the parsed date first (e.g., create a Date from
job.next_run, check isNaN(date.getTime()) or date.toString() !== 'Invalid Date')
and then render date.toLocaleString() when valid or a stable fallback string
like "—" / "Unknown" when invalid; make this change for the JSX expressions that
currently call new Date(job.next_run).toLocaleString().
Comment on lines
+88
to
+90
| const optionKey = `${skill.skillId}:${option.name}`; | ||
| const draft = draftValues[optionKey] ?? ''; | ||
| const busy = savingKey === optionKey; |
Contributor
There was a problem hiding this comment.
Use option.value as fallback when draft entry is missing.
Current fallback to empty string can show blank controls and allow saving empty values even when a server-provided value exists.
🛠️ Suggested patch
{skill.options.map(option => {
const optionKey = `${skill.skillId}:${option.name}`;
- const draft = draftValues[optionKey] ?? '';
+ const draft =
+ draftValues[optionKey] ??
+ (option.value == null ? '' : String(option.value));
const busy = savingKey === optionKey;Also applies to: 121-127, 142-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/cron/RuntimeSkillCronList.tsx` around
lines 88 - 90, The draft fallback currently uses an empty string which can hide
server values; change how drafts are read so that when computing draft (the
variable derived from draftValues[optionKey] in RuntimeSkillCronList.tsx) you
fall back to option.value instead of ''. Update every place that computes draft
(the occurrences around optionKey, draftValues, draft, and busy using savingKey)
so the UI displays the server-provided option.value when no draft exists and
prevents saving accidental empty values.
Comment on lines
+90
to
+101
| style={{ width: `${Math.round((isIndeterminateDownload ? 1 : progress) * 100)}%` }} | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="flex flex-wrap items-center gap-x-3 gap-y-1 text-xs text-stone-500"> | ||
| <span> | ||
| Progress:{' '} | ||
| {isInstalling | ||
| ? 'Installing Ollama runtime...' | ||
| : isIndeterminateDownload | ||
| ? 'Downloading (size unknown)' | ||
| : `${Math.round(progress * 100)}%`} |
Contributor
There was a problem hiding this comment.
Clamp progress before rendering width/percentage.
A value outside [0, 1] can render invalid width/percentage output.
🛠️ Suggested patch
const ModelStatusSection = ({
@@
}: ModelStatusSectionProps) => {
+ const normalizedProgress = Math.min(1, Math.max(0, progress));
+
return (
@@
- style={{ width: `${Math.round((isIndeterminateDownload ? 1 : progress) * 100)}%` }}
+ style={{
+ width: `${Math.round((isIndeterminateDownload ? 1 : normalizedProgress) * 100)}%`,
+ }}
@@
- : `${Math.round(progress * 100)}%`}
+ : `${Math.round(normalizedProgress * 100)}%`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/local-model/ModelStatusSection.tsx` around
lines 90 - 101, Clamp the progress value to [0,1] before using it in both the
inline style width and the percentage text; create a local clampedProgress
(e.g., const clampedProgress = Math.min(Math.max(progress ?? 0, 0), 1)) and
replace raw uses of progress in the width calculation (style={{ width:
`${Math.round((isIndeterminateDownload ? 1 : progress) * 100)}%` }}) and the
percentage rendering (`${Math.round(progress * 100)}%`) so they use
clampedProgress and still respect isIndeterminateDownload and isInstalling
branches.
Comment on lines
+114
to
+124
| {recentVisionSummaries.map(summary => ( | ||
| <div | ||
| key={summary.id} | ||
| className="rounded-xl border border-stone-200 bg-white p-3 text-xs text-stone-200"> | ||
| <div className="text-stone-500"> | ||
| {new Date(summary.captured_at_ms).toLocaleTimeString()} ·{' '} | ||
| {summary.app_name ?? 'Unknown App'} | ||
| {summary.window_title ? ` · ${summary.window_title}` : ''} | ||
| </div> | ||
| <div className="mt-1 text-stone-800">{summary.actionable_notes}</div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Likely incorrect text color class on vision summary card.
Line 117 uses text-stone-200 for the summary card container, but the child elements use darker colors like text-stone-500 and text-stone-800. The text-stone-200 on the parent div appears to be a typo and may cause unexpected inheritance issues or is simply unused. Consider removing it or using an appropriate color if it was intended for something specific.
🐛 Suggested fix
<div
key={summary.id}
- className="rounded-xl border border-stone-200 bg-white p-3 text-xs text-stone-200">
+ className="rounded-xl border border-stone-200 bg-white p-3 text-xs">
<div className="text-stone-500">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/components/settings/panels/screen-intelligence/SessionAndVisionSection.tsx`
around lines 114 - 124, The parent card div wrapping recentVisionSummaries (the
element using key={summary.id}) mistakenly includes the Tailwind class
text-stone-200 which conflicts with its darker child text colors; remove
text-stone-200 from that container's className (or replace it with a suitable
color like text-stone-800 or simply rely on child colors/inherit) so the card
does not unintentionally force a light text color that gets overridden or
inherited inconsistently.
Comment on lines
+28
to
+49
| <input | ||
| type="text" | ||
| value={value} | ||
| onChange={e => onChange(e.target.value)} | ||
| placeholder={placeholder} | ||
| className="w-full rounded-xl border border-stone-200 bg-white py-2 pl-9 pr-9 text-sm text-stone-900 placeholder-stone-400 focus:border-primary-300 focus:outline-none focus:ring-1 focus:ring-primary-200" | ||
| /> | ||
| {value && ( | ||
| <button | ||
| type="button" | ||
| onClick={() => onChange('')} | ||
| className="absolute inset-y-0 right-3 flex items-center text-stone-400 hover:text-stone-600"> | ||
| <svg className="h-4 w-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M6 18L18 6M6 6l12 12" | ||
| /> | ||
| </svg> | ||
| </button> | ||
| )} |
Contributor
There was a problem hiding this comment.
Add accessible names to the search and clear controls.
The input currently relies on placeholder text, and the clear button is icon-only with no label. That makes the new filter hard to use with screen readers. Add an explicit label or aria-label for the input, and an aria-label such as "Clear search" for the button.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/skills/SkillSearchBar.tsx` around lines 28 - 49, The input
and clear button in SkillSearchBar are missing accessible names; update the
input element (the text input using props value/onChange/placeholder) to include
an explicit label or an aria-label (e.g., aria-label={placeholder || "Search
skills"}) and add an aria-label="Clear search" to the clear button (the button
with onClick={() => onChange('')} and the SVG) so screen readers can identify
both controls.
Contributor
Author
|
Closing — will re-open on a dedicated branch off main. |
This was referenced Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
app/src/pages/Skills.tsx: Removedinstallingfrom the full-list loading gate; passisInstallingprop to individualThirdPartySkillCardapp/src/components/skills/SkillCard.tsx: AddedisInstallingprop to show "Enabling..." label andctaDisabledprop to disable the CTA button with reduced opacityTest plan
yarn test --run src/pages/__tests__/Skills)Summary by CodeRabbit
New Features
Refactoring