fix: adjust pr commnets

This commit is contained in:
Shirone
2026-01-12 21:21:20 +01:00
parent 19c12b7813
commit cca4638b71
15 changed files with 91 additions and 271 deletions

View File

@@ -1,203 +0,0 @@
# Implementation Plan: Fix Mobile Model Selection
## Problem Statement
Users cannot change the model when creating a new task on mobile devices. The model selector uses fixed-width Radix UI Popovers with nested secondary popovers that extend to the right, causing the interface to go off-screen on mobile devices (typically 375-428px width).
## Root Cause Analysis
### Current Implementation Issues:
1. **Fixed Widths**: Main popover is 320px, secondary popovers are 220px
2. **Horizontal Nesting**: Secondary popovers (thinking levels, reasoning effort, cursor variants) position to the right of the main popover
3. **No Collision Handling**: Radix Popover doesn't have sufficient collision padding configured
4. **No Mobile-Specific UI**: Same component used for all screen sizes
### Affected Files:
- `/apps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsx` - Core implementation
- `/apps/ui/src/components/views/agent-view/shared/agent-model-selector.tsx` - Wrapper for agent view
- `/apps/ui/src/components/views/agent-view/input-area/input-controls.tsx` - Usage location
## Proposed Solution: Responsive Popover with Mobile Optimization
### Approach: Add Responsive Width & Collision Handling
**Rationale**: Minimal changes, maximum compatibility, leverages existing Radix UI features
### Implementation Steps:
#### 1. Create a Custom Hook for Mobile Detection
**File**: `/apps/ui/src/hooks/use-mobile.ts` (new file)
```typescript
import { useEffect, useState } from 'react';
export function useMobile(breakpoint: number = 768) {
const [isMobile, setIsMobile] = useState(false);
useEffect(() => {
const mediaQuery = window.matchMedia(`(max-width: ${breakpoint}px)`);
const handleChange = () => {
setIsMobile(mediaQuery.matches);
};
// Set initial value
handleChange();
// Listen for changes
mediaQuery.addEventListener('change', handleChange);
return () => mediaQuery.removeEventListener('change', handleChange);
}, [breakpoint]);
return isMobile;
}
```
**Why**: Follows existing pattern from `use-sidebar-auto-collapse.ts`, reusable across components
#### 2. Update Phase Model Selector with Responsive Behavior
**File**: `/apps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsx`
**Changes**:
- Import and use `useMobile()` hook
- Apply responsive widths:
- Mobile: `w-[calc(100vw-32px)] max-w-[340px]` (full width with padding)
- Desktop: `w-80` (320px - current)
- Add collision handling to Radix Popover:
- `collisionPadding={16}` - Prevent edge overflow
- `avoidCollisions={true}` - Enable collision detection
- `sideOffset={4}` - Add spacing from trigger
- Secondary popovers:
- Mobile: Position `side="bottom"` instead of `side="right"`
- Desktop: Keep `side="right"` (current behavior)
- Mobile width: `w-[calc(100vw-32px)] max-w-[340px]`
- Desktop width: `w-[220px]` (current)
**Specific Code Changes**:
```typescript
// Add at top of component
const isMobile = useMobile(768);
// Main popover content
<PopoverContent
className={cn(
isMobile ? "w-[calc(100vw-32px)] max-w-[340px]" : "w-80",
"p-0"
)}
collisionPadding={16}
avoidCollisions={true}
sideOffset={4}
>
// Secondary popovers (thinking level, reasoning effort, etc.)
<PopoverContent
side={isMobile ? "bottom" : "right"}
className={cn(
isMobile ? "w-[calc(100vw-32px)] max-w-[340px]" : "w-[220px]",
"p-2"
)}
collisionPadding={16}
avoidCollisions={true}
sideOffset={4}
>
```
#### 3. Test Responsive Behavior
**Test Cases**:
- [ ] Mobile (< 768px): Popovers fit within screen, secondary popovers open below
- [ ] Tablet (768-1024px): Popovers use optimal width
- [ ] Desktop (> 1024px): Current behavior preserved
- [ ] Edge cases: Very narrow screens (320px), screen rotation
- [ ] Functionality: All model selections work correctly on all screen sizes
## Alternative Approaches Considered
### Alternative 1: Use Sheet Component for Mobile
**Pros**: Better mobile UX, full-screen takeover common pattern
**Cons**: Requires duplicating component logic, more complex state management, different UX between mobile/desktop
**Verdict**: Rejected - Too much complexity for the benefit
### Alternative 2: Simplify Mobile UI (Remove Nested Popovers)
**Pros**: Simpler mobile interface
**Cons**: Removes functionality (thinking levels, reasoning effort) on mobile, poor UX
**Verdict**: Rejected - Removes essential features
### Alternative 3: Horizontal Scrolling Container
**Pros**: Preserves exact desktop layout
**Cons**: Poor mobile UX, non-standard pattern, accessibility issues
**Verdict**: Rejected - Bad mobile UX
## Technical Considerations
### Breakpoint Selection
- **768px chosen**: Standard tablet breakpoint
- Matches pattern in existing codebase (`use-sidebar-auto-collapse.ts` uses 1024px)
- Covers iPhone SE (375px) through iPhone 14 Pro Max (428px)
### Collision Handling
- `collisionPadding={16}`: 16px buffer from edges (standard spacing)
- `avoidCollisions={true}`: Radix will automatically reposition if needed
- `sideOffset={4}`: Small gap between trigger and popover
### Performance
- `useMobile` hook uses `window.matchMedia` (performant, native API)
- Re-renders only on breakpoint changes (not every resize)
- No additional dependencies
### Compatibility
- Works with existing compact/full modes
- Preserves all functionality
- No breaking changes to props/API
- Compatible with existing styles
## Implementation Checklist
- [ ] Create `/apps/ui/src/hooks/use-mobile.ts`
- [ ] Update `phase-model-selector.tsx` with responsive behavior
- [ ] Test on mobile devices/emulators (Chrome DevTools)
- [ ] Test on tablet breakpoint
- [ ] Test on desktop (ensure no regression)
- [ ] Verify all model variants are selectable
- [ ] Check nested popovers (thinking level, reasoning effort, cursor)
- [ ] Verify compact mode still works in agent view
- [ ] Test keyboard navigation
- [ ] Test with touch interactions
## Rollback Plan
If issues arise:
1. Revert `phase-model-selector.tsx` changes
2. Remove `use-mobile.ts` hook
3. Original functionality immediately restored
## Success Criteria
✅ Users can select any model on mobile devices (< 768px width)
All nested popover options are accessible on mobile
Desktop behavior unchanged (no regressions)
UI fits within viewport on all screen sizes (320px+)
No horizontal scrolling required
Touch interactions work correctly
## Estimated Effort
- Implementation: 30-45 minutes
- Testing: 15-20 minutes
- **Total**: ~1 hour
## Dependencies
None - uses existing Radix UI Popover features
## Risks & Mitigation
| Risk | Likelihood | Impact | Mitigation |
|------|-----------|--------|------------|
| Breaks desktop layout | Low | Medium | Thorough testing, conditional logic |
| Poor mobile UX | Low | Medium | Follow mobile-first best practices |
| Touch interaction issues | Low | Low | Use Radix UI touch handlers |
| Breakpoint conflicts | Low | Low | Use standard 768px breakpoint |
## Notes for Developer
- The `compact` prop in `agent-model-selector.tsx` is preserved and still works
- All existing functionality (thinking levels, reasoning effort, cursor variants) remains
- Only visual layout changes on mobile - no logic changes
- Consider adding this pattern to other popovers if successful

View File

@@ -185,7 +185,6 @@ export class ClaudeUsageService {
} as Record<string, string>,
});
} catch (spawnError) {
// ... (error handling omitted for brevity in replace block, keep existing)
const errorMessage = spawnError instanceof Error ? spawnError.message : String(spawnError);
logger.error('[executeClaudeUsageCommandPty] Failed to spawn PTY:', errorMessage);

View File

@@ -72,17 +72,17 @@ export function UsagePopover() {
const [codexError, setCodexError] = useState<UsageError | null>(null);
// Check authentication status
const isClaudeCliVerified = !!claudeAuthStatus?.authenticated;
const isClaudeAuthenticated = !!claudeAuthStatus?.authenticated;
const isCodexAuthenticated = codexAuthStatus?.authenticated;
// Determine which tab to show by default
useEffect(() => {
if (isClaudeCliVerified) {
if (isClaudeAuthenticated) {
setActiveTab('claude');
} else if (isCodexAuthenticated) {
setActiveTab('codex');
}
}, [isClaudeCliVerified, isCodexAuthenticated]);
}, [isClaudeAuthenticated, isCodexAuthenticated]);
// Check if data is stale (older than 2 minutes)
const isClaudeStale = useMemo(() => {
@@ -173,10 +173,10 @@ export function UsagePopover() {
// Auto-fetch on mount if data is stale
useEffect(() => {
if (isClaudeStale && isClaudeCliVerified) {
if (isClaudeStale && isClaudeAuthenticated) {
fetchClaudeUsage(true);
}
}, [isClaudeStale, isClaudeCliVerified, fetchClaudeUsage]);
}, [isClaudeStale, isClaudeAuthenticated, fetchClaudeUsage]);
useEffect(() => {
if (isCodexStale && isCodexAuthenticated) {
@@ -189,7 +189,7 @@ export function UsagePopover() {
if (!open) return;
// Fetch based on active tab
if (activeTab === 'claude' && isClaudeCliVerified) {
if (activeTab === 'claude' && isClaudeAuthenticated) {
if (!claudeUsage || isClaudeStale) {
fetchClaudeUsage();
}
@@ -213,7 +213,7 @@ export function UsagePopover() {
activeTab,
claudeUsage,
isClaudeStale,
isClaudeCliVerified,
isClaudeAuthenticated,
codexUsage,
isCodexStale,
isCodexAuthenticated,
@@ -348,7 +348,7 @@ export function UsagePopover() {
);
// Determine which tabs to show
const showClaudeTab = isClaudeCliVerified;
const showClaudeTab = isClaudeAuthenticated;
const showCodexTab = isCodexAuthenticated;
return (

View File

@@ -263,7 +263,7 @@ export function BoardHeader({
/>
{/* Plan Button with Settings - only show on desktop, mobile has it in the menu */}
{!isMobile && (
{isMounted && !isMobile && (
<div className={controlContainerClass} data-testid="plan-button-container">
<button
onClick={onOpenPlanDialog}

View File

@@ -1,4 +1,10 @@
export { ListHeader, LIST_COLUMNS, getColumnById, getColumnWidth, getColumnAlign } from './list-header';
export {
ListHeader,
LIST_COLUMNS,
getColumnById,
getColumnWidth,
getColumnAlign,
} from './list-header';
export type { ListHeaderProps } from './list-header';
export { ListRow, getFeatureSortValue, sortFeatures } from './list-row';

View File

@@ -1,3 +1,5 @@
// TODO: Remove @ts-nocheck after fixing BaseFeature's index signature issue
// The `[key: string]: unknown` in BaseFeature causes property access type errors
// @ts-nocheck
import { memo, useCallback, useMemo } from 'react';
import { cn } from '@/lib/utils';

View File

@@ -1,4 +1,3 @@
// @ts-nocheck
import { memo, useMemo, useCallback, useState } from 'react';
import { ChevronDown, ChevronRight, Plus } from 'lucide-react';
import { cn } from '@/lib/utils';
@@ -10,7 +9,7 @@ import { ListHeader } from './list-header';
import { ListRow, sortFeatures } from './list-row';
import { createRowActionHandlers, type RowActionHandlers } from './row-actions';
import { getStatusLabel, getStatusOrder } from './status-badge';
import { COLUMNS, getColumnsWithPipeline } from '../../constants';
import { getColumnsWithPipeline } from '../../constants';
import type { SortConfig, SortColumn } from '../../hooks/use-list-view-state';
/**
@@ -98,11 +97,7 @@ const StatusGroupHeader = memo(function StatusGroupHeader({
>
{/* Collapse indicator */}
<span className="text-muted-foreground">
{isExpanded ? (
<ChevronDown className="w-4 h-4" />
) : (
<ChevronRight className="w-4 h-4" />
)}
{isExpanded ? <ChevronDown className="w-4 h-4" /> : <ChevronRight className="w-4 h-4" />}
</span>
{/* Status color indicator */}
@@ -123,11 +118,7 @@ const StatusGroupHeader = memo(function StatusGroupHeader({
/**
* EmptyState displays a message when there are no features
*/
const EmptyState = memo(function EmptyState({
onAddFeature,
}: {
onAddFeature?: () => void;
}) {
const EmptyState = memo(function EmptyState({ onAddFeature }: { onAddFeature?: () => void }) {
return (
<div
className={cn(
@@ -210,11 +201,7 @@ export const ListView = memo(function ListView({
const features = columnFeaturesMap[column.id] || [];
if (features.length > 0) {
// Sort features within the group according to current sort config
const sortedFeatures = sortFeatures(
features,
sortConfig.column,
sortConfig.direction
);
const sortedFeatures = sortFeatures(features, sortConfig.column, sortConfig.direction);
groups.push({
id: column.id as FeatureStatusWithPipeline,
@@ -366,20 +353,12 @@ export const ListView = memo(function ListView({
}
}
}
}, [
onToggleFeatureSelection,
selectionState.allSelected,
selectedFeatureIds,
statusGroups,
]);
}, [onToggleFeatureSelection, selectionState.allSelected, selectedFeatureIds, statusGroups]);
// Show empty state if no features
if (totalFeatures === 0) {
return (
<div
className={cn('flex flex-col h-full bg-background', className)}
data-testid="list-view"
>
<div className={cn('flex flex-col h-full bg-background', className)} data-testid="list-view">
<EmptyState onAddFeature={onAddFeature} />
</div>
);
@@ -466,20 +445,13 @@ export const ListView = memo(function ListView({
/**
* Helper to get all features from the columnFeaturesMap as a flat array
*/
export function getFlatFeatures(
columnFeaturesMap: Record<string, Feature[]>
): Feature[] {
export function getFlatFeatures(columnFeaturesMap: Record<string, Feature[]>): Feature[] {
return Object.values(columnFeaturesMap).flat();
}
/**
* Helper to count total features across all groups
*/
export function getTotalFeatureCount(
columnFeaturesMap: Record<string, Feature[]>
): number {
return Object.values(columnFeaturesMap).reduce(
(sum, features) => sum + features.length,
0
);
export function getTotalFeatureCount(columnFeaturesMap: Record<string, Feature[]>): number {
return Object.values(columnFeaturesMap).reduce((sum, features) => sum + features.length, 0);
}

View File

@@ -1,4 +1,3 @@
// @ts-nocheck
import { memo, useCallback, useState } from 'react';
import {
MoreHorizontal,
@@ -253,7 +252,13 @@ export const RowActions = memo(function RowActions({
// Use controlled or uncontrolled state
const open = isOpen ?? internalOpen;
const setOpen = onOpenChange ?? setInternalOpen;
const setOpen = (value: boolean) => {
if (onOpenChange) {
onOpenChange(value);
} else {
setInternalOpen(value);
}
};
const handleOpenChange = useCallback(
(newOpen: boolean) => {

View File

@@ -160,10 +160,7 @@ export const StatusBadge = memo(function StatusBadge({
size = 'default',
className,
}: StatusBadgeProps) {
const display = useMemo(
() => getStatusDisplay(status, pipelineConfig),
[status, pipelineConfig]
);
const display = useMemo(() => getStatusDisplay(status, pipelineConfig), [status, pipelineConfig]);
const sizeClasses = {
sm: 'px-1.5 py-0.5 text-[10px]',

View File

@@ -98,17 +98,22 @@ export function CommitWorktreeDialog({
}
setIsGenerating(true);
let cancelled = false;
const generateMessage = async () => {
try {
const api = getElectronAPI();
if (!api?.worktree?.generateCommitMessage) {
setIsGenerating(false);
if (!cancelled) {
setIsGenerating(false);
}
return;
}
const result = await api.worktree.generateCommitMessage(worktree.path);
if (cancelled) return;
if (result.success && result.message) {
setMessage(result.message);
} else {
@@ -117,15 +122,22 @@ export function CommitWorktreeDialog({
setMessage('');
}
} catch (err) {
if (cancelled) return;
// Don't show error toast for generation failures
console.warn('Error generating commit message:', err);
setMessage('');
} finally {
setIsGenerating(false);
if (!cancelled) {
setIsGenerating(false);
}
}
};
generateMessage();
return () => {
cancelled = true;
};
}
}, [open, worktree, enableAiCommitMessages]);

View File

@@ -52,7 +52,14 @@ function validateViewMode(value: unknown): ViewMode {
* Validates and returns a valid SortColumn, defaulting to 'createdAt' if invalid
*/
function validateSortColumn(value: unknown): SortColumn {
const validColumns: SortColumn[] = ['title', 'status', 'category', 'priority', 'createdAt', 'updatedAt'];
const validColumns: SortColumn[] = [
'title',
'status',
'category',
'priority',
'createdAt',
'updatedAt',
];
if (typeof value === 'string' && validColumns.includes(value as SortColumn)) {
return value as SortColumn;
}
@@ -139,7 +146,9 @@ export interface UseListViewStateReturn {
export function useListViewState(): UseListViewStateReturn {
// Initialize state from localStorage
const [viewMode, setViewModeState] = useState<ViewMode>(() => loadPersistedState().viewMode);
const [sortConfig, setSortConfigState] = useState<SortConfig>(() => loadPersistedState().sortConfig);
const [sortConfig, setSortConfigState] = useState<SortConfig>(
() => loadPersistedState().sortConfig
);
// Derived state
const isListView = viewMode === 'list';
@@ -199,6 +208,16 @@ export function useListViewState(): UseListViewStateReturn {
setSortConfig,
resetSort,
}),
[viewMode, setViewMode, toggleViewMode, isListView, isKanbanView, sortConfig, setSortColumn, setSortConfig, resetSort]
[
viewMode,
setViewMode,
toggleViewMode,
isListView,
isKanbanView,
sortConfig,
setSortColumn,
setSortConfig,
resetSort,
]
);
}

View File

@@ -9,8 +9,6 @@ import { useResponsiveKanban } from '@/hooks/use-responsive-kanban';
import { getColumnsWithPipeline, type ColumnId } from './constants';
import type { PipelineConfig } from '@automaker/types';
import { cn } from '@/lib/utils';
import type { ViewMode } from './hooks/use-list-view-state';
interface KanbanBoardProps {
sensors: any;
collisionDetectionStrategy: (args: any) => any;
@@ -59,8 +57,6 @@ interface KanbanBoardProps {
isDragging?: boolean;
/** Whether the board is in read-only mode */
isReadOnly?: boolean;
// View mode for transition animation
viewMode?: ViewMode;
/** Additional className for custom styling (e.g., transition classes) */
className?: string;
}
@@ -101,7 +97,6 @@ export function KanbanBoard({
onAiSuggest,
isDragging = false,
isReadOnly = false,
viewMode,
className,
}: KanbanBoardProps) {
// Generate columns including pipeline steps

View File

@@ -1,4 +1,4 @@
import { useEffect, useCallback } from 'react';
import { useEffect, useCallback, useState } from 'react';
import { RefreshCw, AlertTriangle } from 'lucide-react';
import { cn } from '@/lib/utils';
import { getElectronAPI } from '@/lib/electron';
@@ -104,6 +104,8 @@ function UsageItem({
export function MobileUsageBar({ showClaudeUsage, showCodexUsage }: MobileUsageBarProps) {
const { claudeUsage, claudeUsageLastUpdated, setClaudeUsage } = useAppStore();
const { codexUsage, codexUsageLastUpdated, setCodexUsage } = useAppStore();
const [isClaudeLoading, setIsClaudeLoading] = useState(false);
const [isCodexLoading, setIsCodexLoading] = useState(false);
// Check if data is stale (older than 2 minutes)
const isClaudeStale =
@@ -111,6 +113,7 @@ export function MobileUsageBar({ showClaudeUsage, showCodexUsage }: MobileUsageB
const isCodexStale = !codexUsageLastUpdated || Date.now() - codexUsageLastUpdated > 2 * 60 * 1000;
const fetchClaudeUsage = useCallback(async () => {
setIsClaudeLoading(true);
try {
const api = getElectronAPI();
if (!api.claude) return;
@@ -120,10 +123,13 @@ export function MobileUsageBar({ showClaudeUsage, showCodexUsage }: MobileUsageB
}
} catch {
// Silently fail - usage display is optional
} finally {
setIsClaudeLoading(false);
}
}, [setClaudeUsage]);
const fetchCodexUsage = useCallback(async () => {
setIsCodexLoading(true);
try {
const api = getElectronAPI();
if (!api.codex) return;
@@ -133,6 +139,8 @@ export function MobileUsageBar({ showClaudeUsage, showCodexUsage }: MobileUsageB
}
} catch {
// Silently fail - usage display is optional
} finally {
setIsCodexLoading(false);
}
}, [setCodexUsage]);
@@ -166,7 +174,7 @@ export function MobileUsageBar({ showClaudeUsage, showCodexUsage }: MobileUsageB
<UsageItem
icon={AnthropicIcon}
label="Claude"
isLoading={false}
isLoading={isClaudeLoading}
onRefresh={fetchClaudeUsage}
>
{claudeUsage ? (
@@ -189,7 +197,12 @@ export function MobileUsageBar({ showClaudeUsage, showCodexUsage }: MobileUsageB
)}
{showCodexUsage && (
<UsageItem icon={OpenAIIcon} label="Codex" isLoading={false} onRefresh={fetchCodexUsage}>
<UsageItem
icon={OpenAIIcon}
label="Codex"
isLoading={isCodexLoading}
onRefresh={fetchCodexUsage}
>
{codexUsage?.rateLimits ? (
<>
{codexUsage.rateLimits.primary && (

View File

@@ -235,6 +235,8 @@ export function WorktreePanel({
onStartDevServer={handleStartDevServer}
onStopDevServer={handleStopDevServer}
onOpenDevServerUrl={handleOpenDevServerUrl}
onRunInitScript={handleRunInitScript}
hasInitScript={hasInitScript}
/>
)}

View File

@@ -340,7 +340,7 @@ export type ClaudeModel = 'opus' | 'sonnet' | 'haiku';
export interface Feature extends Omit<
BaseFeature,
'steps' | 'imagePaths' | 'textFilePaths' | 'status'
'steps' | 'imagePaths' | 'textFilePaths' | 'status' | 'planSpec'
> {
id: string;
title?: string;
@@ -354,6 +354,7 @@ export interface Feature extends Omit<
textFilePaths?: FeatureTextFilePath[]; // Text file attachments for context
justFinishedAt?: string; // UI-specific: ISO timestamp when agent just finished
prUrl?: string; // UI-specific: Pull request URL
planSpec?: PlanSpec; // Explicit planSpec type to override BaseFeature's index signature
}
// Parsed task from spec (for spec and full planning modes)