feat(settings,skills): reorganize settings and skills pages#453
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
📝 WalkthroughWalkthroughIntroduces breadcrumbs into settings navigation and SettingsHeader; standardizes panel spacing and decomposes large settings panels into subcomponents; removes several legacy/placeholder panels and helpers; and redesigns the Skills page with unified skill cards, search, category filter, and a relocated Browser Access toggle. Changes
Sequence Diagram(s)sequenceDiagram
participant Panel as Settings Panel
participant Nav as useSettingsNavigation()
participant Header as SettingsHeader
Panel->>Nav: request navigateBack, navigateToSettings, breadcrumbs
Nav-->>Panel: return navigateBack, navigateToSettings, breadcrumbs
Panel->>Header: pass title, showBackButton, onBack, breadcrumbs
Header->>Header: render breadcrumb trail (buttons or static spans)
sequenceDiagram
participant User as User
participant SkillsPage as Skills Page
participant SearchBar as SkillSearchBar
participant Filter as SkillCategoryFilter
participant CardList as Card Renderer
User->>SearchBar: type query
SearchBar-->>SkillsPage: onChange(searchQuery)
User->>Filter: select category
Filter-->>SkillsPage: onChange(selectedCategory)
SkillsPage->>SkillsPage: compute filteredItems, groupedItems
SkillsPage->>CardList: render UnifiedSkillCard / ThirdPartySkillCard per group
CardList-->>User: display cards with CTAs/menus
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
app/src/components/settings/panels/cron/RuntimeSkillCronList.tsx (1)
3-10: Use aninterfaceforCronSkillConfigto match repo TS conventions.This is an object shape and should follow the project’s TypeScript style rule for interfaces.
As per coding guidelines,
**/*.{ts,tsx}: Prefer interface for defining object shapes in TypeScript.🤖 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 3 - 10, Replace the type alias CronSkillConfig with an interface declaration to follow project TS conventions: declare interface CronSkillConfig with the same properties (skillId, name, enabled, manifestTickInterval, options, optionsError) and keep the RuntimeSkillOption reference unchanged; update any imports/usages if necessary to ensure the new interface name is resolved where CronSkillConfig is referenced (e.g., props or function signatures) so behavior and types remain identical.app/src/components/skills/SkillCategoryFilter.tsx (1)
18-22: Use an arrow function for the component declaration.This component currently uses a function declaration; project style prefers arrow functions for TSX components.
♻️ Suggested style-aligned rewrite
-export default function SkillCategoryFilter({ +const SkillCategoryFilter = ({ categories, selected, onChange, }: SkillCategoryFilterProps) { return ( @@ ); -} +}; + +export default SkillCategoryFilter;As per coding guidelines "
**/*.{js,jsx,ts,tsx}: Prefer arrow functions over function declarations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillCategoryFilter.tsx` around lines 18 - 22, The component SkillCategoryFilter is declared with a function declaration; convert it to an arrow-function component to match project style by replacing the declaration with a const assignment like: declare const SkillCategoryFilter: React.FC<SkillCategoryFilterProps> = ({ categories, selected, onChange }) => { ... } and keep the existing export default SkillCategoryFilter and all props/types (SkillCategoryFilterProps) unchanged; ensure any implicit returns or JSX remain identical after the rewrite.app/src/components/skills/SkillCard.tsx (2)
275-276: Async handler inonClickshould usevoidoperator.
handleSyncreturnsPromise<void>butonClickexpects() => void. Wrap withvoidfor consistency with the codebase pattern seen elsewhere (e.g.,LocalModelPanel.tsx).♻️ Add void operator
- onClick: handleSync, + onClick: () => void handleSync(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillCard.tsx` around lines 275 - 276, The onClick is currently passing the async function handleSync directly (which returns Promise<void>) to a prop expecting () => void; update the JSX so the onClick calls handleSync with the void operator (e.g., onClick: () => void handleSync()) to discard the Promise and match the codebase pattern used elsewhere (see LocalModelPanel.tsx) while keeping disabled: isSyncing unchanged and referring to the handleSync handler in SkillCard component.
308-315: Redundant conditional — both branches callonSetup().The if/else branches do the same thing. The comment suggests the distinction was intended but never implemented.
♻️ Simplify to single call
onCtaClick={() => { - if (connectionStatus === 'offline' && !skill.hasSetup) { - // SkillActionButton handles enable — reuse by calling onSetup which triggers install - onSetup(); - } else { - onSetup(); - } + onSetup(); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillCard.tsx` around lines 308 - 315, The conditional inside the onCtaClick handler in SkillCard is redundant because both branches call onSetup(); simplify by removing the if/else and invoke onSetup() directly. Locate the onCtaClick prop usage in the SkillCard component (referencing connectionStatus, skill.hasSetup, and onSetup) and replace the conditional block with a single call to onSetup(), keeping or moving any meaningful comment about SkillActionButton behavior if needed.app/src/pages/Skills.tsx (1)
142-146: Prefer async/await over.then()chains.Per coding guidelines, use async/await for promises. Consider refactoring to an async IIFE or extracting a helper:
♻️ Suggested refactor using async IIFE
useEffect(() => { - openhumanGetRuntimeFlags() - .then(res => setBrowserAllowAll(res.result.browser_allow_all)) - .catch(() => {}); + (async () => { + try { + const res = await openhumanGetRuntimeFlags(); + setBrowserAllowAll(res.result.browser_allow_all); + } catch { + // Silently ignore - toggle will default to false + } + })(); }, []);🤖 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 - 146, The useEffect currently uses a .then()/.catch() promise chain when calling openhumanGetRuntimeFlags; refactor it to use async/await by creating an async function (either an IIFE inside useEffect or an extracted helper) that awaits openhumanGetRuntimeFlags, sets state with setBrowserAllowAll(res.result.browser_allow_all), and handles errors with try/catch instead of .catch(); keep the call invocation inside useEffect so the effect runs once.
🤖 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-74: The breadcrumb markup in SettingsHeader.tsx should be
converted to semantic breadcrumb structure: wrap the trail in a <nav
aria-label="Breadcrumb"> and use an ordered list (<ol>) with each crumb rendered
as a list item (<li>) instead of plain divs; keep the existing interactive
element logic for crumb.onClick (button vs span) but place those inside the
<li>. Make separator icons (the SVG) presentation-only by adding
aria-hidden="true" or role="presentation" and only render separators between
items (not after the last crumb). Update the mapping over breadcrumbs to render
<li key={i}> with the crumb content and a visually displayed,
screen-reader-hidden separator SVG.
In `@app/src/components/settings/panels/autocomplete/CompletionStyleSection.tsx`:
- Around line 183-186: The className on the mapped history entry div in
CompletionStyleSection.tsx contains a duplicate "text-xs" token; remove the
duplicate so the className string only includes "text-xs" once. Locate the JSX
where historyEntries.map((entry, idx) => ( ... )) renders the div (the one with
key={`${String(entry.timestamp_ms)}-${String(idx)}`}) and edit its className to
de-duplicate the classes, leaving a single "text-xs".
In `@app/src/components/settings/panels/cron/RuntimeSkillCronList.tsx`:
- Around line 138-157: The text/number input is currently editable even when the
parent skill is disabled; change the input's disabled prop to mirror the save
button by using disabled={busy || !skill.enabled} so users cannot edit fields
for disabled skills; ensure this update is applied in the JSX where option.type
=== 'text' || option.type === 'number' (the input using value={draft} and
onChange calling onSetDraftValues) so the input state matches the button that
calls onSaveOptionValue(skill.skillId, option, draft).
In `@app/src/components/settings/panels/local-model/DeviceCapabilitySection.tsx`:
- Around line 31-35: DeviceCapabilitySection currently renders the same preset
error in two places when presets fail (!presetsLoading && !presetsData &&
presetError), causing duplication; consolidate this by computing a single
boolean (e.g., showPresetError = !presetsLoading && !presetsData && presetError)
inside DeviceCapabilitySection and use that to render the error only once
(either the top banner or the footer) and update the other location (lines
referenced around 120-124) to check the same flag or omit its rendering when
showPresetError is true so the error message appears only in one place.
In `@app/src/components/settings/panels/local-model/ModelDownloadSection.tsx`:
- Line 226: The label in the ModelDownloadSection component uses the
low-contrast class "text-stone-300"; update the label element (the <label>
inside ModelDownloadSection) to a higher-contrast utility such as
"text-stone-700" or "text-white" (or another theme-appropriate darker stone
value) so form labels are readable on the light background; keep the existing
spacing classes ("flex items-center gap-2 text-xs") and only replace the text
color utility to improve accessibility.
In `@app/src/components/settings/panels/local-model/ModelStatusSection.tsx`:
- Around line 293-298: The Ollama binary path line in ModelStatusSection uses
the low-contrast class text-stone-200; update the rendering for
diagnostics.ollama_binary_path (the div with title and truncated text) to a
higher-contrast utility such as text-stone-100 or text-white so the path is
readable on light cards, keeping the existing truncate and title behavior
intact.
- Around line 190-195: The "Set Path" button in ModelStatusSection should also
be disabled when the path input is empty to prevent empty submissions; update
the button's disabled prop to include a check for the component's path input
state (e.g., ollamaPath or the state variable that holds the input value) in
addition to isSettingPath, and ensure onSetOllamaPath early-returns or validates
the input so no request is sent when the path string is blank.
In `@app/src/components/skills/SkillCategoryFilter.tsx`:
- Around line 26-34: The buttons in SkillCategoryFilter render selection only
via visuals; update the button element (inside the map where key={cat},
onClick={() => onChange(cat)}) to include an ARIA state so assistive tech knows
which is active—add aria-pressed={selected === cat} (boolean) to the button (or
aria-current="true" when you prefer current semantics) so screen readers
announce the selected category.
In `@app/src/pages/Skills.tsx`:
- Around line 142-146: Refactor the useEffect inside BrowserAccessToggle to use
an async function with try/catch and guard Tauri calls by checking
window.__TAURI__ (or otherwise catching if undefined) before calling
openhumanGetRuntimeFlags, then setBrowserAllowAll from the awaited result; also
ensure any calls to openhumanSetBrowserAllowAll are similarly guarded and
await-based. Update tests Skills.third-party-gmail-sync.test.tsx and
Skills.third-party-notion-debug-tools.test.tsx to mock both
openhumanGetRuntimeFlags and openhumanSetBrowserAllowAll so they don’t call the
real Tauri API during test runs. Ensure references use the exact exported names
openhumanGetRuntimeFlags and openhumanSetBrowserAllowAll and that async/await
replaces .then().catch() patterns.
- Around line 291-292: Add a clarifying comment above the
eslint-disable-next-line line: explain that bestChannelStatus (the
component-level arrow function) is intentionally omitted from the dependency
array because it is recreated every render and would force unnecessary
recomputations, and that channelConnections is included instead so the memo
recalculates when channel state changes; keep the eslint suppression but
document this performance tradeoff and the rationale for leaving skillsList,
configurableChannels, and channelConnections as the actual dependencies.
---
Nitpick comments:
In `@app/src/components/settings/panels/cron/RuntimeSkillCronList.tsx`:
- Around line 3-10: Replace the type alias CronSkillConfig with an interface
declaration to follow project TS conventions: declare interface CronSkillConfig
with the same properties (skillId, name, enabled, manifestTickInterval, options,
optionsError) and keep the RuntimeSkillOption reference unchanged; update any
imports/usages if necessary to ensure the new interface name is resolved where
CronSkillConfig is referenced (e.g., props or function signatures) so behavior
and types remain identical.
In `@app/src/components/skills/SkillCard.tsx`:
- Around line 275-276: The onClick is currently passing the async function
handleSync directly (which returns Promise<void>) to a prop expecting () =>
void; update the JSX so the onClick calls handleSync with the void operator
(e.g., onClick: () => void handleSync()) to discard the Promise and match the
codebase pattern used elsewhere (see LocalModelPanel.tsx) while keeping
disabled: isSyncing unchanged and referring to the handleSync handler in
SkillCard component.
- Around line 308-315: The conditional inside the onCtaClick handler in
SkillCard is redundant because both branches call onSetup(); simplify by
removing the if/else and invoke onSetup() directly. Locate the onCtaClick prop
usage in the SkillCard component (referencing connectionStatus, skill.hasSetup,
and onSetup) and replace the conditional block with a single call to onSetup(),
keeping or moving any meaningful comment about SkillActionButton behavior if
needed.
In `@app/src/components/skills/SkillCategoryFilter.tsx`:
- Around line 18-22: The component SkillCategoryFilter is declared with a
function declaration; convert it to an arrow-function component to match project
style by replacing the declaration with a const assignment like: declare const
SkillCategoryFilter: React.FC<SkillCategoryFilterProps> = ({ categories,
selected, onChange }) => { ... } and keep the existing export default
SkillCategoryFilter and all props/types (SkillCategoryFilterProps) unchanged;
ensure any implicit returns or JSX remain identical after the rewrite.
In `@app/src/pages/Skills.tsx`:
- Around line 142-146: The useEffect currently uses a .then()/.catch() promise
chain when calling openhumanGetRuntimeFlags; refactor it to use async/await by
creating an async function (either an IIFE inside useEffect or an extracted
helper) that awaits openhumanGetRuntimeFlags, sets state with
setBrowserAllowAll(res.result.browser_allow_all), and handles errors with
try/catch instead of .catch(); keep the call invocation inside useEffect so the
effect runs once.
🪄 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: 6dbd6626-924f-4376-ada5-8d676c20df83
📒 Files selected for processing (48)
.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.tsx
💤 Files with no reviewable changes (9)
- app/src/components/settings/panels/AdvancedPanel.tsx
- app/src/components/settings/panels/ProfilePanel.tsx
- app/src/pages/Settings.tsx
- app/src/components/settings/components/SettingsPanelLayout.tsx
- app/src/components/settings/components/SettingsBackButton.tsx
- app/src/components/settings/panels/SkillsPanel.tsx
- app/src/components/settings/panels/TauriCommandsPanel.tsx
- app/src/components/settings/hooks/useSettingsAnimation.ts
- app/src/components/SkillsGrid.tsx
app/src/components/settings/panels/autocomplete/CompletionStyleSection.tsx
Outdated
Show resolved
Hide resolved
app/src/components/settings/panels/local-model/DeviceCapabilitySection.tsx
Show resolved
Hide resolved
app/src/components/settings/panels/local-model/ModelDownloadSection.tsx
Outdated
Show resolved
Hide resolved
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [skillsList, configurableChannels, channelConnections]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what dependencies the linter expects
cd app && npx eslint --no-eslintrc --parser `@typescript-eslint/parser` --plugin react-hooks --rule '{"react-hooks/exhaustive-deps": "warn"}' src/pages/Skills.tsx 2>/dev/null | head -30Repository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Read the Skills.tsx file around lines 280-300 to see the full useEffect hook
cat -n app/src/pages/Skills.tsx | sed -n '280,300p'Repository: tinyhumansai/openhuman
Length of output: 783
🏁 Script executed:
# Read the Skills.tsx file around lines 250-295 to see the full useEffect hook
cat -n app/src/pages/Skills.tsx | sed -n '250,295p'Repository: tinyhumansai/openhuman
Length of output: 1710
🏁 Script executed:
# Find the bestChannelStatus function to see what it uses
rg -n "bestChannelStatus" app/src/pages/Skills.tsx -A 5 -B 2Repository: tinyhumansai/openhuman
Length of output: 900
🏁 Script executed:
# Let's check if there's a helper or if bestChannelStatus is defined in the component
# and understand all dependencies that are actually used
rg -n "const bestChannelStatus\|function bestChannelStatus" app/src/pages/Skills.tsxRepository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Get the full bestChannelStatus function definition
cat -n app/src/pages/Skills.tsx | sed -n '201,210p'Repository: tinyhumansai/openhuman
Length of output: 606
🏁 Script executed:
# Check the ESLint configuration for react-hooks rules
find app -name ".eslintrc*" -o -name "eslint.config.*" | head -5Repository: tinyhumansai/openhuman
Length of output: 86
🏁 Script executed:
# Check the ESLint configuration
cat app/eslint.config.js | head -100Repository: tinyhumansai/openhuman
Length of output: 2715
🏁 Script executed:
# Continue reading the ESLint configuration for react-hooks rules
cat app/eslint.config.js | sed -n '100,200p'Repository: tinyhumansai/openhuman
Length of output: 3673
The exhaustive-deps suppression is intentional but add a clarifying comment explaining the tradeoff.
The eslint-disable suppresses the warning about missing bestChannelStatus dependency. While bestChannelStatus is the function actually called in the memo, it's deliberately omitted because it's recreated on every render (defined as a component-level const arrow function). Including it would cause the memo to recalculate unnecessarily. Instead, channelConnections is included to ensure the memo updates when channel connection state changes. Add a comment explaining this performance tradeoff so the suppression rationale is clear to future reviewers.
🤖 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 291 - 292, Add a clarifying comment
above the eslint-disable-next-line line: explain that bestChannelStatus (the
component-level arrow function) is intentionally omitted from the dependency
array because it is recreated every render and would force unnecessary
recomputations, and that channelConnections is included instead so the memo
recalculates when channel state changes; keep the eslint suppression but
document this performance tradeoff and the rationale for leaving skillsList,
configurableChannels, and channelConnections as the actual dependencies.
There was a problem hiding this comment.
Intentionally not adding a comment — the eslint suppression is clear from context and per project rules we don't add comments unless logic isn't self-evident.
There was a problem hiding this comment.
@graycyrus, understood — I'll defer to the project convention on that. Thanks for the clarification.
✏️ Learnings added
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 453
File: app/src/pages/Skills.tsx:296-297
Timestamp: 2026-04-09T11:31:52.450Z
Learning: In the tinyhumansai/openhuman repository, inline code comments are only added when logic is not self-evident. Do not suggest adding explanatory comments for eslint-disable suppressions or other constructs where the rationale is considered clear from context. This is an explicit project rule.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 437
File: app/src/hooks/useSubconscious.ts:69-82
Timestamp: 2026-04-08T16:46:32.293Z
Learning: In `app/src/hooks/useSubconscious.ts`, the `.catch(() => null)` pattern on each individual RPC call inside `refresh()` (subconsciousTasksList, subconsciousEscalationsList, subconsciousLogList, subconsciousStatus) before `Promise.all` is intentional. It keeps the Subconscious tab functional when individual endpoints fail (e.g., DB not yet initialized on first launch). RPC-layer errors are already logged via `coreRpcError`, so swallowing them at the hook level is by design — do not flag this as a bug.
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 167
File: app/src/components/OnboardingOverlay.tsx:30-39
Timestamp: 2026-04-01T13:52:17.936Z
Learning: In React components, do not perform synchronous `setState` (or other state-updating calls) directly inside `useEffect` bodies. The lint rule `react-hooks/set-state-in-effect` (enforced in this codebase) disallows patterns such as resetting state flags at the top of an effect before returning. Refactor to avoid calling state setters in the effect; prefer deriving state from props/render, using refs, or restructuring logic so state updates occur outside `useEffect`.
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 307
File: app/src/components/ConnectionIndicator.tsx:29-36
Timestamp: 2026-04-03T14:57:27.517Z
Learning: In `app/src/components/ConnectionIndicator.tsx`, the `description` prop was intentionally removed from `ConnectionIndicatorProps` as part of the light-theme redesign (PR `#307`). The component now renders only as a compact pill badge; any surrounding description text is the responsibility of the parent component (e.g., `Home.tsx`). Do not flag the absence of `description` rendering as a regression.
- 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
|
Addressed all CodeRabbit feedback in 7b9d9ce: Actionable fixes:
Not addressed (intentional):
|
- 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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/src/components/settings/panels/autocomplete/CompletionStyleSection.tsx (1)
58-58: Align section spacing with the PR’s standardized panel rhythm.Both section wrappers use
space-y-3; if this component is meant to follow the repo-wide settings convention from this PR, switch tospace-y-4for consistency.🎯 Proposed consistency tweak
- <section className="rounded-2xl border border-stone-200 bg-white p-4 space-y-3"> + <section className="rounded-2xl border border-stone-200 bg-white p-4 space-y-4"> ... - <section className="rounded-2xl border border-stone-200 bg-white p-4 space-y-3"> + <section className="rounded-2xl border border-stone-200 bg-white p-4 space-y-4">Also applies to: 163-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/autocomplete/CompletionStyleSection.tsx` at line 58, The section spacing in CompletionStyleSection is using "space-y-3" but should follow the repo-wide settings rhythm; update both section wrappers in the CompletionStyleSection component to use "space-y-4" instead of "space-y-3" (search for the class strings in CompletionStyleSection.tsx to locate both occurrences).app/src/components/settings/panels/local-model/ModelStatusSection.tsx (1)
154-160: Expose error-detail toggle state to assistive tech.For the “Show/Hide error details” disclosure, add
aria-expandedandaria-controlsso state is announced properly.♿ Proposed fix
<button onClick={onToggleErrorDetail} + aria-expanded={showErrorDetail} + aria-controls="model-status-error-detail" className="text-xs text-red-600 hover:text-red-500 underline"> {showErrorDetail ? 'Hide error details' : 'Show error details'} </button> {showErrorDetail && ( - <pre className="max-h-40 overflow-auto rounded bg-red-50 border border-red-200 p-2 text-[10px] text-red-600 leading-tight whitespace-pre-wrap break-words"> + <pre + id="model-status-error-detail" + className="max-h-40 overflow-auto rounded bg-red-50 border border-red-200 p-2 text-[10px] text-red-600 leading-tight whitespace-pre-wrap break-words"> {status.error_detail} </pre> )}🤖 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 154 - 160, The "Show/Hide error details" toggle button does not expose its expanded state to assistive tech; update the button rendered by onToggleErrorDetail to include aria-expanded={showErrorDetail} and aria-controls referencing the id of the <pre> containing the details (give the <pre> a stable id like "model-error-details"); ensure the same id is used on the pre so the button's aria-controls points to that element and screen readers can announce the disclosure state for showErrorDetail.app/src/components/skills/SkillCard.tsx (1)
232-254: Prefer arrow functions over function declarations.Per coding guidelines, prefer arrow functions. These helper functions should use const arrow syntax.
♻️ Proposed refactor to arrow functions
- function statusDotClass(status: SkillConnectionStatus): string { - switch (status) { - case 'connected': return 'bg-sage-500'; - case 'connecting': return 'bg-amber-500 animate-pulse'; - case 'error': return 'bg-coral-500'; - default: return 'bg-stone-400'; - } - } - - function ctaLabel(): string { - switch (connectionStatus) { - case 'offline': return 'Enable'; - case 'setup_required': return 'Setup'; - case 'error': return 'Retry'; - default: return 'Manage'; - } - } - - function ctaVariant(): 'primary' | 'sage' | 'amber' { - if (connectionStatus === 'offline') return 'sage'; - if (connectionStatus === 'error') return 'amber'; - return 'primary'; - } + const statusDotClass = (status: SkillConnectionStatus): string => { + switch (status) { + case 'connected': return 'bg-sage-500'; + case 'connecting': return 'bg-amber-500 animate-pulse'; + case 'error': return 'bg-coral-500'; + default: return 'bg-stone-400'; + } + }; + + const ctaLabel = (): string => { + switch (connectionStatus) { + case 'offline': return 'Enable'; + case 'setup_required': return 'Setup'; + case 'error': return 'Retry'; + default: return 'Manage'; + } + }; + + const ctaVariant = (): 'primary' | 'sage' | 'amber' => { + if (connectionStatus === 'offline') return 'sage'; + if (connectionStatus === 'error') return 'amber'; + return 'primary'; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillCard.tsx` around lines 232 - 254, Refactor the three helper functions statusDotClass, ctaLabel, and ctaVariant from function declarations to const arrow functions to follow the project's coding guidelines; replace "function statusDotClass(status: SkillConnectionStatus): string { ... }" with "const statusDotClass = (status: SkillConnectionStatus): string => { ... }" (and similarly for ctaLabel and ctaVariant), preserving their parameter and return types and existing switch logic and usage of connectionStatus so behavior remains identical.
🤖 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/panels/autocomplete/CompletionStyleSection.tsx`:
- Around line 125-152: In CompletionStyleSection, the three textarea controls
(controlled by styleInstructions/onSetStyleInstructions,
styleExamplesText/onSetStyleExamplesText,
disabledAppsText/onSetDisabledAppsText) lack programmatic labels; add a <label
htmlFor="..."> for each header and assign matching id attributes to the
corresponding textarea (or alternatively add unique aria-labelledby/id pairs) so
screen readers can associate the label with each textarea (e.g., ids like
style-instructions, style-examples, disabled-apps) and keep existing
onChange/value handlers intact.
In `@app/src/components/settings/panels/local-model/ModelStatusSection.tsx`:
- Around line 179-189: The text "Ollama Binary Path (optional)" is not an actual
<label>, so screen readers won't associate it with the input; add an explicit
<label> element with htmlFor that points to an id on the input (e.g.,
id="ollamaPath") and wrap or place the visible heading text inside that <label>,
then set the input's id to match and keep the existing value/handler
(ollamaPathInput, onSetOllamaPathInput) and placeholder unchanged to ensure
accessibility without altering behavior.
In `@app/src/pages/Skills.tsx`:
- Around line 173-181: The toggle div with role="switch" (using browserAllowAll,
browserBusy and handleToggle) is not keyboard accessible; add tabIndex={0} and
an onKeyDown handler that listens for Space and Enter, prevents default, and
calls handleToggle only when browserBusy is false, ensuring the existing
aria-checked remains accurate and the busy state still disables interaction;
update the element using the same identifiers (browserAllowAll, browserBusy,
handleToggle) so keyboard users can toggle the switch.
---
Nitpick comments:
In `@app/src/components/settings/panels/autocomplete/CompletionStyleSection.tsx`:
- Line 58: The section spacing in CompletionStyleSection is using "space-y-3"
but should follow the repo-wide settings rhythm; update both section wrappers in
the CompletionStyleSection component to use "space-y-4" instead of "space-y-3"
(search for the class strings in CompletionStyleSection.tsx to locate both
occurrences).
In `@app/src/components/settings/panels/local-model/ModelStatusSection.tsx`:
- Around line 154-160: The "Show/Hide error details" toggle button does not
expose its expanded state to assistive tech; update the button rendered by
onToggleErrorDetail to include aria-expanded={showErrorDetail} and aria-controls
referencing the id of the <pre> containing the details (give the <pre> a stable
id like "model-error-details"); ensure the same id is used on the pre so the
button's aria-controls points to that element and screen readers can announce
the disclosure state for showErrorDetail.
In `@app/src/components/skills/SkillCard.tsx`:
- Around line 232-254: Refactor the three helper functions statusDotClass,
ctaLabel, and ctaVariant from function declarations to const arrow functions to
follow the project's coding guidelines; replace "function statusDotClass(status:
SkillConnectionStatus): string { ... }" with "const statusDotClass = (status:
SkillConnectionStatus): string => { ... }" (and similarly for ctaLabel and
ctaVariant), preserving their parameter and return types and existing switch
logic and usage of connectionStatus so behavior remains identical.
🪄 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: 5acb1dee-4870-46e1-8cdb-2349413260c0
📒 Files selected for processing (9)
app/src/components/settings/components/SettingsHeader.tsxapp/src/components/settings/panels/autocomplete/CompletionStyleSection.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/skills/SkillCard.tsxapp/src/components/skills/SkillCategoryFilter.tsxapp/src/pages/Skills.tsx
✅ Files skipped from review due to trivial changes (1)
- app/src/components/skills/SkillCategoryFilter.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/components/settings/panels/local-model/DeviceCapabilitySection.tsx
- app/src/components/settings/panels/cron/RuntimeSkillCronList.tsx
- app/src/components/settings/panels/local-model/ModelDownloadSection.tsx
- app/src/components/settings/components/SettingsHeader.tsx
| <div className="space-y-1"> | ||
| <div className="text-xs text-stone-600">Style Instructions</div> | ||
| <textarea | ||
| value={styleInstructions} | ||
| onChange={event => onSetStyleInstructions(event.target.value)} | ||
| rows={3} | ||
| className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700" | ||
| /> | ||
| </div> | ||
| <div className="space-y-1"> | ||
| <div className="text-xs text-stone-600">Style Examples (one per line)</div> | ||
| <textarea | ||
| value={styleExamplesText} | ||
| onChange={event => onSetStyleExamplesText(event.target.value)} | ||
| rows={3} | ||
| className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700" | ||
| /> | ||
| </div> | ||
| <div className="space-y-1"> | ||
| <div className="text-xs text-stone-600"> | ||
| Disabled Apps (one bundle/app token per line) | ||
| </div> | ||
| <textarea | ||
| value={disabledAppsText} | ||
| onChange={event => onSetDisabledAppsText(event.target.value)} | ||
| rows={3} | ||
| className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700" | ||
| /> |
There was a problem hiding this comment.
Associate textarea fields with semantic labels for screen readers.
Line 125 through Line 152 use <div> text headers instead of <label htmlFor> + id, so these controls are not programmatically labeled.
♿ Proposed fix
- <div className="space-y-1">
- <div className="text-xs text-stone-600">Style Instructions</div>
+ <div className="space-y-1">
+ <label htmlFor="autocomplete-style-instructions" className="text-xs text-stone-600">
+ Style Instructions
+ </label>
<textarea
+ id="autocomplete-style-instructions"
value={styleInstructions}
onChange={event => onSetStyleInstructions(event.target.value)}
rows={3}
className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700"
/>
</div>
<div className="space-y-1">
- <div className="text-xs text-stone-600">Style Examples (one per line)</div>
+ <label htmlFor="autocomplete-style-examples" className="text-xs text-stone-600">
+ Style Examples (one per line)
+ </label>
<textarea
+ id="autocomplete-style-examples"
value={styleExamplesText}
onChange={event => onSetStyleExamplesText(event.target.value)}
rows={3}
className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700"
/>
</div>
<div className="space-y-1">
- <div className="text-xs text-stone-600">
+ <label htmlFor="autocomplete-disabled-apps" className="text-xs text-stone-600">
Disabled Apps (one bundle/app token per line)
- </div>
+ </label>
<textarea
+ id="autocomplete-disabled-apps"
value={disabledAppsText}
onChange={event => onSetDisabledAppsText(event.target.value)}
rows={3}
className="w-full rounded border border-stone-200 bg-stone-50 p-2 text-xs text-stone-700"
/>
</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/CompletionStyleSection.tsx`
around lines 125 - 152, In CompletionStyleSection, the three textarea controls
(controlled by styleInstructions/onSetStyleInstructions,
styleExamplesText/onSetStyleExamplesText,
disabledAppsText/onSetDisabledAppsText) lack programmatic labels; add a <label
htmlFor="..."> for each header and assign matching id attributes to the
corresponding textarea (or alternatively add unique aria-labelledby/id pairs) so
screen readers can associate the label with each textarea (e.g., ids like
style-instructions, style-examples, disabled-apps) and keep existing
onChange/value handlers intact.
| <div className="text-stone-500 text-xs uppercase tracking-wide"> | ||
| Ollama Binary Path (optional) | ||
| </div> | ||
| <div className="flex items-center gap-2"> | ||
| <input | ||
| type="text" | ||
| value={ollamaPathInput} | ||
| onChange={e => onSetOllamaPathInput(e.target.value)} | ||
| placeholder="/usr/local/bin/ollama" | ||
| className="flex-1 rounded-md border border-stone-200 bg-white px-2 py-1.5 text-xs text-stone-900 placeholder:text-stone-400 focus:border-primary-500 focus:outline-none" | ||
| /> |
There was a problem hiding this comment.
Associate the path input with an explicit <label>.
The current heading text is visual-only; screen readers won’t treat it as the input label. Add htmlFor/id linkage.
♿ Proposed fix
- <div className="text-stone-500 text-xs uppercase tracking-wide">
+ <label
+ htmlFor="ollama-binary-path-input"
+ className="text-stone-500 text-xs uppercase tracking-wide">
Ollama Binary Path (optional)
- </div>
+ </label>
<div className="flex items-center gap-2">
<input
+ id="ollama-binary-path-input"
type="text"
value={ollamaPathInput}🤖 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 179 - 189, The text "Ollama Binary Path (optional)" is not an actual
<label>, so screen readers won't associate it with the input; add an explicit
<label> element with htmlFor that points to an id on the input (e.g.,
id="ollamaPath") and wrap or place the visible heading text inside that <label>,
then set the input's id to match and keep the existing value/handler
(ollamaPathInput, onSetOllamaPathInput) and placeholder unchanged to ensure
accessibility without altering behavior.
| <div | ||
| role="switch" | ||
| aria-checked={browserAllowAll} | ||
| onClick={browserBusy ? undefined : handleToggle} | ||
| className={`w-9 h-5 rounded-full transition-colors relative ${browserBusy ? 'opacity-50 cursor-wait' : 'cursor-pointer'} ${browserAllowAll ? 'bg-sage-500' : 'bg-stone-200'}`}> | ||
| <div | ||
| className={`w-1.5 h-1.5 rounded-full flex-shrink-0 ${statusDotClass(connectionStatus)}`} | ||
| className={`absolute top-0.5 w-4 h-4 rounded-full bg-white shadow transition-transform ${browserAllowAll ? 'translate-x-4' : 'translate-x-0.5'}`} | ||
| /> | ||
| <span className={`text-xs flex-shrink-0 ${statusDisplay.color}`}> | ||
| {statusDisplay.text} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
Custom switch needs keyboard accessibility.
The div with role="switch" is not keyboard accessible. Screen reader and keyboard users cannot interact with this toggle. Add tabIndex={0} and an onKeyDown handler for Space/Enter keys.
♿ Proposed fix for keyboard accessibility
<div
role="switch"
aria-checked={browserAllowAll}
+ tabIndex={browserBusy ? -1 : 0}
onClick={browserBusy ? undefined : handleToggle}
+ onKeyDown={e => {
+ if (browserBusy) return;
+ if (e.key === ' ' || e.key === 'Enter') {
+ e.preventDefault();
+ handleToggle();
+ }
+ }}
className={`w-9 h-5 rounded-full transition-colors relative ${browserBusy ? 'opacity-50 cursor-wait' : 'cursor-pointer'} ${browserAllowAll ? 'bg-sage-500' : 'bg-stone-200'}`}>📝 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.
| <div | |
| role="switch" | |
| aria-checked={browserAllowAll} | |
| onClick={browserBusy ? undefined : handleToggle} | |
| className={`w-9 h-5 rounded-full transition-colors relative ${browserBusy ? 'opacity-50 cursor-wait' : 'cursor-pointer'} ${browserAllowAll ? 'bg-sage-500' : 'bg-stone-200'}`}> | |
| <div | |
| className={`w-1.5 h-1.5 rounded-full flex-shrink-0 ${statusDotClass(connectionStatus)}`} | |
| className={`absolute top-0.5 w-4 h-4 rounded-full bg-white shadow transition-transform ${browserAllowAll ? 'translate-x-4' : 'translate-x-0.5'}`} | |
| /> | |
| <span className={`text-xs flex-shrink-0 ${statusDisplay.color}`}> | |
| {statusDisplay.text} | |
| </span> | |
| </div> | |
| <div | |
| role="switch" | |
| aria-checked={browserAllowAll} | |
| tabIndex={browserBusy ? -1 : 0} | |
| onClick={browserBusy ? undefined : handleToggle} | |
| onKeyDown={e => { | |
| if (browserBusy) return; | |
| if (e.key === ' ' || e.key === 'Enter') { | |
| e.preventDefault(); | |
| handleToggle(); | |
| } | |
| }} | |
| className={`w-9 h-5 rounded-full transition-colors relative ${browserBusy ? 'opacity-50 cursor-wait' : 'cursor-pointer'} ${browserAllowAll ? 'bg-sage-500' : 'bg-stone-200'}`}> | |
| <div | |
| className={`absolute top-0.5 w-4 h-4 rounded-full bg-white shadow transition-transform ${browserAllowAll ? 'translate-x-4' : 'translate-x-0.5'}`} | |
| /> | |
| </div> |
🤖 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 173 - 181, The toggle div with
role="switch" (using browserAllowAll, browserBusy and handleToggle) is not
keyboard accessible; add tabIndex={0} and an onKeyDown handler that listens for
Space and Enter, prevents default, and calls handleToggle only when browserBusy
is false, ensuring the existing aria-checked remains accurate and the busy state
still disables interaction; update the element using the same identifiers
(browserAllowAll, browserBusy, handleToggle) so keyboard users can toggle the
switch.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx (1)
107-110: Consider extracting an overflow-menu helper to remove repeated setup.Both tests repeat the same “open More actions” sequence; a tiny helper would make intent clearer and reduce maintenance if the trigger changes.
Also applies to: 177-180
🤖 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-notion-debug-tools.test.tsx` around lines 107 - 110, Extract the repeated "open More actions" sequence into a small test helper (e.g., openOverflowMenu or openMoreActions) that encapsulates calling screen.getByTitle('More actions') and fireEvent.click(...) so both tests call that helper instead of duplicating the two lines; update references in Skills.third-party-notion-debug-tools.test.tsx to use the helper wherever the overflow menu must be opened (including the occurrences around the current getByTitle/firing click at lines ~107-110 and ~177-180).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx`:
- Around line 107-110: Extract the repeated "open More actions" sequence into a
small test helper (e.g., openOverflowMenu or openMoreActions) that encapsulates
calling screen.getByTitle('More actions') and fireEvent.click(...) so both tests
call that helper instead of duplicating the two lines; update references in
Skills.third-party-notion-debug-tools.test.tsx to use the helper wherever the
overflow menu must be opened (including the occurrences around the current
getByTitle/firing click at lines ~107-110 and ~177-180).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bb0ce0e-db38-4db8-965d-b1150e3eb7fa
📒 Files selected for processing (2)
app/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsxapp/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx
Summary
p-4 space-y-4across all 25+ panels/settings/skillsrouteUnifiedSkillCardcomponent with consistent status indicators, CTA labels, and overflow menu for secondary actionsTest plan
/settings/profile,/settings/advanced,/settings/skillsall redirect to settings homeCloses #396
Summary by CodeRabbit
New Features
Refactor