Loading...
Loading...
Analyzes code based on John Ousterhout's "A Philosophy of Software Design". Identifies unnecessary complexity, shallow modules, information leaks, and design problems. Use when reviewing architecture, PRs, refactoring, or asking about code quality.
npx skill4agent add atilladeniz/kubeli software-design-review// TACTICAL (bad): Quick fix that leaks implementation
const [pods, setPods] = useState<Pod[]>([]);
const [loading, setLoading] = useState(false);
const [error, setError] = useState<Error | null>(null);
// Every component repeats this pattern...
// STRATEGIC (good): Invest in a proper abstraction
const { data: pods, isLoading, error } = useKubeQuery('pods', namespace);// SHALLOW (bad): Many props, little functionality
interface PodCardProps {
name: string;
namespace: string;
status: string;
onSelect: () => void;
onDelete: () => void;
onRestart: () => void;
onViewLogs: () => void;
isSelected: boolean;
showActions: boolean;
// ...10 more props
}
// DEEP (good): Simple interface, complex behavior inside
interface PodCardProps {
pod: Pod;
onAction?: (action: PodAction) => void;
}// SHALLOW (bad): Exposes all internal details
pub fn get_pods(
client: &Client,
namespace: &str,
label_selector: Option<&str>,
field_selector: Option<&str>,
limit: Option<u32>,
continue_token: Option<&str>,
) -> Result<Vec<Pod>>
// DEEP (good): Query object hides complexity
pub fn get_pods(query: PodQuery) -> Result<PodList>backspace()deleteSelection()// SPECIALIZED (bad): One method per UI action
class PodService {
deletePodFromContextMenu(pod: Pod) { ... }
deletePodFromKeyboard(pod: Pod) { ... }
deletePodFromBulkAction(pods: Pod[]) { ... }
}
// GENERAL (good): One general method, UI decides how to call it
class PodService {
deletePods(pods: Pod[]): Promise<DeleteResult> { ... }
}// SPECIALIZED (bad): Text class knows about UI concepts
class TextEditor {
backspace(cursor: Cursor) { ... } // UI-specific
deleteSelection(sel: Selection) { ... } // UI-specific
}
// GENERAL (good): Generic text operations
class TextEditor {
delete(start: Position, end: Position) { ... }
insert(position: Position, text: string) { ... }
}
// UI code: editor.delete(cursor.move(-1), cursor)// SPECIALIZED (bad): API mirrors exact UI needs
pub fn get_pods_for_sidebar(namespace: &str) -> Vec<PodSummary>
pub fn get_pods_for_detail_view(name: &str) -> PodDetail
pub fn get_pods_for_search(query: &str) -> Vec<PodSearchResult>
// GENERAL (good): One flexible API
pub fn get_pods(query: PodQuery) -> Vec<Pod>
// Callers transform the data for their specific needs// PASS-THROUGH (bad): TextDocument just forwards to TextArea
class PodManager {
private kubeClient: KubeClient;
getPods(namespace: string) {
return this.kubeClient.getPods(namespace); // Just forwarding!
}
deletePod(namespace: string, name: string) {
return this.kubeClient.deletePod(namespace, name); // Just forwarding!
}
}
// BETTER: Either expose kubeClient directly or add real value
class PodManager {
async getPodsWithMetrics(namespace: string): Promise<PodWithMetrics[]> {
const [pods, metrics] = await Promise.all([
this.kubeClient.getPods(namespace),
this.metricsClient.getPodMetrics(namespace),
]);
return this.mergePodMetrics(pods, metrics); // Actual value added
}
}// LEAK (bad): Caller must know about refresh mechanism
const { pods, refreshPods, setRefreshInterval, lastRefresh } = usePodsStore();
useEffect(() => {
const interval = setInterval(refreshPods, refreshInterval);
return () => clearInterval(interval);
}, [refreshInterval]);
// HIDDEN (good): Store handles refresh internally
const { pods } = usePodsStore(); // Auto-refreshes, caller doesn't care how// LEAK (bad): Exposes kubeconfig parsing details
pub struct KubeConfig {
pub clusters: Vec<Cluster>,
pub contexts: Vec<Context>,
pub current_context: String,
pub auth_info: HashMap<String, AuthInfo>, // Internal detail!
}
// HIDDEN (good): Exposes only what callers need
impl KubeConfig {
pub fn current_context(&self) -> &Context;
pub fn switch_context(&mut self, name: &str) -> Result<()>;
// Internal representation stays private
}// COMPLEXITY PUSHED UP (bad): Every caller handles retry logic
async function fetchPods(namespace: string) {
const response = await invoke('get_pods', { namespace });
if (response.error) throw new Error(response.error);
return response.data;
}
// Caller must handle: retries, timeouts, error UI, loading state...
// COMPLEXITY PULLED DOWN (good): Module handles it all
async function fetchPods(namespace: string): Promise<Pod[]> {
return retryWithBackoff(async () => {
const response = await invoke('get_pods', { namespace });
return response ?? []; // Empty array if namespace doesn't exist
}, { maxRetries: 3, timeout: 10000 });
}// WRONG SPLIT (bad): HTTP parsing split into read + parse
// (Both need HTTP format knowledge - information leak)
async function readRequest(socket: Socket): Promise<string> { ... }
async function parseRequest(text: string): Promise<HttpRequest> { ... }
// TOGETHER (good): Single module handles both
async function readAndParseRequest(socket: Socket): Promise<HttpRequest> { ... }// WRONG TOGETHER (bad): Generic utility mixed with UI-specific code
function useResourceList<T>() {
const [resources, setResources] = useState<T[]>([]);
const [selectedPodForDeletion, setSelectedPodForDeletion] = useState(null);
// ^ UI-specific in a generic hook!
}
// SEPARATE (good): Keep generic and specific apart
function useResourceList<T>() { /* generic logic only */ }
function usePodDeletion() { /* pod-specific UI logic */ }// MANY EXCEPTIONS (bad): Every edge case throws
function getSelectedPod(): Pod {
if (!selectedPodId) throw new Error('No pod selected');
const pod = pods.find(p => p.id === selectedPodId);
if (!pod) throw new Error('Pod not found');
return pod;
}
// DEFINED AWAY (good): Always returns valid result
function getSelectedPod(): Pod | null {
if (!selectedPodId) return null;
return pods.find(p => p.id === selectedPodId) ?? null;
}// COMPLEX (bad): Caller handles all error cases
pub fn find_context(&self, name: &str) -> Result<&Context, ConfigError> {
self.contexts.iter()
.find(|c| c.name == name)
.ok_or(ConfigError::ContextNotFound(name.to_string()))
}
// SIMPLE (good): Define the error away
pub fn find_context_or_current(&self, name: &str) -> &Context {
self.contexts.iter()
.find(|c| c.name == name)
.unwrap_or(&self.current_context)
}// INCONSISTENT (bad): Different patterns for same thing
const { pods } = usePodStore(); // Zustand
const [deployments] = useQuery(...); // React Query
const services = useCustomHook(); // Custom
// ^ Three different patterns for fetching K8s resources!
// CONSISTENT (good): One pattern for K8s resources
const { data: pods } = useKubeResource('pods', namespace);
const { data: deployments } = useKubeResource('deployments', namespace);
const { data: services } = useKubeResource('services', namespace);// NOT OBVIOUS (bad): Generic container hides meaning
function getClusterStatus(): [string, boolean, number] {
return [currentContext, isConnected, nodeCount];
}
const [a, b, c] = getClusterStatus(); // What are a, b, c?
// OBVIOUS (good): Named properties
interface ClusterStatus {
currentContext: string;
isConnected: boolean;
nodeCount: number;
}
function getClusterStatus(): ClusterStatus { ... }// NOT OBVIOUS (bad): Event handler registered elsewhere
useEffect(() => {
eventBus.on('pod-deleted', handlePodDeleted);
return () => eventBus.off('pod-deleted', handlePodDeleted);
}, []);
// Where does 'pod-deleted' come from? Who triggers it?
// OBVIOUS (good): Direct callback, clear data flow
<PodList onDelete={handlePodDeleted} />// ECHO (bad): Repeats code
// Set loading to true
setLoading(true);
// USEFUL (good): Explains why
// Optimistic update: show loading immediately while Tauri IPC completes
// This prevents UI flicker on fast networks
setLoading(true);// MISSING (bad): No interface documentation
pub struct KubeClientManager { ... }
// USEFUL (good): Documents the abstraction
/// Manages Kubernetes client connections across multiple clusters.
///
/// Handles automatic reconnection, context switching, and caches
/// clients to avoid repeated authentication overhead.
///
/// # Thread Safety
/// Can be shared across Tauri commands via `Arc<Mutex<>>`.
///
/// # Example
/// ```
/// let manager = KubeClientManager::new()?;
/// let client = manager.get_or_create("minikube")?;
/// ```
pub struct KubeClientManager { ... }datainfomanagerhandlerutilshelpers// DESIGN-FIRST (good): Comment reveals the abstraction
/**
* Manages WebSocket connections to Kubernetes clusters.
*
* Handles connection lifecycle, automatic reconnection on failure,
* and multiplexes watch streams over a single connection per cluster.
*
* Thread-safe: can be called from React components and background tasks.
*/
class KubeWebSocketManager {
/**
* Subscribe to resource changes in a namespace.
*
* Returns an unsubscribe function. Multiple subscriptions to the
* same resource type share a single watch stream.
*/
subscribe(resource: ResourceType, namespace: string, callback: WatchCallback): () => void;
}
// Then implement...// TACTICAL (bad): Adding special case for new requirement
function renderPodStatus(pod: Pod) {
if (pod.status === 'Running') return <RunningIcon />;
if (pod.status === 'Pending') return <PendingIcon />;
// New requirement: show different icon for init containers
if (pod.status === 'Running' && pod.initContainers?.some(c => !c.ready)) {
return <InitializingIcon />; // Special case bolted on
}
return <UnknownIcon />;
}
// STRATEGIC (good): Redesign to handle requirement cleanly
type PodPhase = 'running' | 'pending' | 'initializing' | 'failed' | 'unknown';
function getPodPhase(pod: Pod): PodPhase {
if (pod.initContainers?.some(c => !c.ready)) return 'initializing';
if (pod.status === 'Running') return 'running';
// ... clean switch on actual pod state
}
function renderPodStatus(pod: Pod) {
return <StatusIcon phase={getPodPhase(pod)} />;
}