Code Review Assistant

This skill performs comprehensive code review following Phoenix/FAAD HMI standards.

When to Use

• Reviewing pull requests or merge requests
• Before creating a commit or PR
• Validating new implementations
• Checking code against standards
• Finding common mistakes before CI/CD

Review Categories

This skill checks code across multiple dimensions:

1. MobX Patterns - Reactivity, observables, actions
2. UI Components - Phoenix shared components, proper usage
3. Architecture - MVVM separation, proper layering
4. Property ViewModels - Correct property VM usage
5. Testing - Test coverage and patterns
6. Common Pitfalls - Known issues and anti-patterns
7. Code Quality - Naming, structure, maintainability

Review Process

Step 1: Identify Files Changed

Use grep/glob to find:

• ViewModels (*.ts files extending BaseViewModel)
• Views (*.tsx files with React components)
• Entity ViewModels (*EntityViewModel.ts)
• Model files (in /model/*/src/)
• Test files (*.test.ts)

Step 2: MobX Pattern Review

Reference: MOBX_ESSENTIALS.md

Critical Checks:

✅ Constructor Pattern

// MUST have makeObservable(this)
constructor(services: IFrameworkServices) {
    super(services);
    makeObservable(this);  // ← Required!
}

Look for: Missing makeObservable(this) in ViewModel constructors Impact: HIGH - Breaks all reactivity

---

✅ Observable State

// State properties must be @observable
@observable isLoading: boolean = false;
@observable selectedId: string | null = null;

Look for: State properties without @observable Impact: HIGH - Component won't update

---

✅ Async Operations

// MUST use runInAction after await
@action async loadData(): Promise<void> {
    const data = await fetchData();
    runInAction(() => {
        this.data = data;  // ← Must be in runInAction
    });
}

Look for: State updates after await without runInAction Impact: HIGH - Runtime warnings, unreliable updates

---

✅ Array Updates

// MUST replace arrays, not mutate
@action addItem(item: Item): void {
    this.items = [...this.items, item];  // ← Correct
    // NOT: this.items.push(item);  // ← Wrong
}

Look for: .push(), .splice(), .sort() on observable arrays Impact: MEDIUM - Unreliable reactivity

---

✅ Override Decorator

// ONLY @override, NOT @action when overriding
@override
doSomething(): void {  // ← Correct (base has @action)
    // NOT: @override @action  // ← Wrong
}

Look for: Both @override and @action on same method Impact: HIGH - Build fails

Step 3: UI Component Review

Reference: UI_COMPONENT_GUIDELINES.md

Critical Checks:

✅ Phoenix Components Only

// MUST use Phoenix components
import { Button, Label, TextInput } from '@tektonux/phoenix-components-shared';

// ❌ WRONG
<span>Text</span>
<button>Click</button>
<input type="text" />

// ✅ CORRECT
<Label>Text</Label>
<Button>Click</Button>
<TextInput />

Look for: Native HTML elements (<span>, <button>, <input>, <select>) Impact: CRITICAL - Violates framework standards

---

✅ Observer Wrapper

// MUST wrap with observer()
export const MyView = observer((props: Props) => {
    return <div>{props.viewModel.value}</div>;
});

Look for: React components accessing ViewModels without observer() Impact: HIGH - Component won't react to changes

---

✅ SelectPortal Usage

// MUST use SelectPortal for dropdowns
<Select>
    <SelectTrigger>
        <SelectValue />
    </SelectTrigger>
    <SelectPortal>  {/* ← Required! */}
        <SelectContent position="popper">
            <SelectViewport>
                {/* items */}
            </SelectViewport>
        </SelectContent>
    </SelectPortal>
</Select>

Look for: <SelectContent> without wrapping <SelectPortal> Impact: MEDIUM - Dropdown can expand parent containers

---

✅ Checkbox/Radio Pattern

// Checkbox and Label are SIBLINGS, not nested
<div className="init-checkbox-group">
    <Checkbox ... />
    <Label>Text</Label>
</div>

// ❌ WRONG
<Label>
    <Checkbox ... />  {/* Blocks clicks! */}
</Label>

Look for: Checkbox or RadioGroupItem wrapped inside Label Impact: HIGH - Breaks user interaction

Step 4: Property ViewModel Review

Reference: PROPERTY_VIEWMODEL_GUIDE.md

Critical Checks:

✅ @computed Decorator Usage

// ✅ CORRECT - @computed when getter includes configuration
@computed
get modeVM(): ICommandedVM<ModeType, IEnumFormatOptions<ModeType>> {
    const vm = this.createPropertyVM('mode', CommandedEnumViewModel<ModeType>);
    vm.configure({
        labelConverter: ModeTypeLabel,
        defaultValue: ModeType.AUTO
    });
    return vm;
}

// ✅ CORRECT - @computed for derived values
@computed
get activeItems(): Item[] {
    return this.items.filter(item => item.isActive);
}

// ✅ OK - Simple forwarding, @computed optional
get nameVM(): IPropertyVM<string, IStringFormatOptions> {
    return this.createPropertyVM('name', StringViewModel);
}

Look for:

• Property VM getters with .configure() calls missing @computed
• Derived/filtered/computed values missing @computed
• Over-use of @computed on simple forwarding (not wrong, just unnecessary)

Impact: MEDIUM - Performance issues (re-running configuration) or missing reactivity

---

✅ Commanded Pattern

// Use .commanded(defaultValue), NOT double fallback
const value = vm.speedVM.commanded(DEFAULT_SPEED);  // ✅

// ❌ WRONG
const value = vm.speedVM.commanded() ?? vm.speedVM.actual() ?? DEFAULT_SPEED;

Look for: .commanded() ?? .actual() patterns Impact: LOW - Code quality, redundant calls

---

✅ Correct Property VM Types

// Arrays
CommandedArrayViewModel<string>  // ✅ For arrays
// NOT: ArrayViewModel  // ❌

// Numbers with constraints
RangedCommandedNumberViewModel  // ✅ For min/max
// NOT: CommandedNumberViewModel  // ❌

Look for: Wrong property VM types Impact: MEDIUM - Missing functionality

---

✅ Label Converter Pattern

// MUST have type hints for null safety
labelConverter: (value: number | null | undefined) => {
    if (value == null) return '---';  // ✅ Use '---'
    return `${value} units`;
}

Look for: Missing null type hints, returning 'N/A' instead of '---' Impact: LOW - Type safety, consistency

Step 5: Entity ViewModel Review

Reference: ENTITY_ARCHITECTURE.md

Critical Checks:

✅ Base Class

// Use EntityViewModel from phoenix-core
import { EntityViewModel } from '@tektonux/phoenix-core';
export class MyEntityVM extends EntityViewModel<MyEntity> {
    // ✅ Correct

// ❌ WRONG
import { BaseEntityViewModel } from '@tektonux/framework-shared-plugin';

Look for: Using BaseEntityViewModel directly Impact: MEDIUM - Should use phoenix-core version

---

✅ Required Methods

getEntityClassName(): string {
    return MyEntity.class;  // ✅ Required
}

getEntityCtr(): IEntityConstructor<MyEntity> {
    return MyEntity;  // ✅ Required
}

Look for: Missing implementation of required methods Impact: HIGH - Runtime errors

---

✅ Label Converters

// Create in adapters/types.ts, not inline
export const ModeTypeLabel: Record<ModeType, string> = {
    [ModeType.AUTO]: 'Automatic',
    [ModeType.MANUAL]: 'Manual',
};

Look for: Inline label objects in ViewModel Impact: LOW - Code organization

Step 6: Model Changes Review

Reference: MODEL_GENERATION_GUIDE.md

Critical Checks:

✅ Source vs Generated

// ONLY edit source models
/model/*/src/  // ✅ Edit here

// NEVER edit generated
/client/libs/*/model/  // ❌ Auto-generated
/server/com.*.model/   // ❌ Auto-generated

Look for: Edits to generated model files Impact: CRITICAL - Changes will be overwritten

---

✅ Members Array

/**
 * @description {
 *      "clazz" : {
 *          "members": [
 *              "class",
 *              "property1",  // ✅ All properties listed
 *              "property2"
 *          ]
 *      }
 * }
 */

Look for: Properties not in members array Impact: HIGH - Properties won't generate

---

✅ Property Names with Units

// ✅ GOOD
contourLineWidthPixels?: CommandedProperty<number>;
targetAltitudeMeters?: RangedCommandedProperty<number>;

// ❌ BAD - Missing units
contourLineWidth?: CommandedProperty<number>;
targetAltitude?: RangedCommandedProperty<number>;

Look for: Numeric properties without unit suffixes Impact: MEDIUM - Unclear semantics

---

✅ Class Name Pattern

/**
 * @default quicktype.projectname.ClassName    // ✅ Has package
 */
public static class: string = "quicktype.ClassName";  // ✅ No package

Look for: Mismatch in package names Impact: CRITICAL - Server won't recognize entity

Step 7: Testing Review

Reference: TESTING_GUIDE.md

Critical Checks:

✅ Test Coverage

• New ViewModels have tests
• Public methods tested
• Computed properties tested
• Error cases tested

Look for: New code without tests Impact: MEDIUM - Code quality

---

✅ Mock Pattern

// Proper mock services
mockServices = {
    logging: { info: vi.fn(), warn: vi.fn(), error: vi.fn() },
    eventBus: { publish: vi.fn(), subscribe: vi.fn() }
} as unknown as IFrameworkServices;

Look for: Incomplete mocks Impact: LOW - Test reliability

Step 8: Common Pitfalls Check

Reference: COMMON_PITFALLS.md

Run through documented pitfalls:

• Numeric defaults using || instead of ??
• Grid references not refreshed after operations
• Mock types using vi.Mock instead of ReturnType<typeof vi.fn>
• Container size not set in tests (offsetWidth/Height)

Step 9: Code Quality Review

General Checks:

✅ File Size

• Files under 300 lines preferred
• Hard limit 500 lines
• If larger, suggest refactoring

✅ Naming Conventions

• ViewModels end with ViewModel
• Entity VMs end with EntityViewModel
• Views end with View
• Enums end with Type

✅ Import Organization

// Framework imports first
import { IFrameworkServices } from '@tektonux/framework-api';
// Model imports
import { MyEntity } from '@tektonux/model';
// Core ViewModels
import { EntityViewModel } from '@tektonux/phoenix-core';
// MobX
import { makeObservable, observable, action } from 'mobx';

✅ NOTE-AI Comments

• Complex decisions documented
• Rationale captured
• Alternatives considered

Review Output Format

Structure feedback as:

## Code Review Summary

### Critical Issues (Must Fix)
1. [File:Line] Missing makeObservable(this) in constructor
2. [File:Line] Using native <button> instead of Button component

### High Priority
1. [File:Line] State update after await without runInAction
2. [File:Line] Component not wrapped with observer()

### Medium Priority
1. [File:Line] Array mutated with .push() instead of replaced
2. [File:Line] Property not in members array

### Low Priority / Suggestions
1. [File:Line] Use .commanded(default) instead of double fallback
2. [File:Line] Consider extracting label converter to adapters

### Positive Observations
- ✅ Excellent use of computed properties for derived state
- ✅ Good test coverage with edge cases
- ✅ Proper MobX patterns throughout

Severity Levels

• CRITICAL: Breaks build, violates core requirements, data loss risk
• HIGH: Runtime errors likely, major functionality broken
• MEDIUM: Bugs possible, inconsistent with patterns, poor UX
• LOW: Code quality, minor inconsistencies, suggestions

Ask User

• Which files/directories to review?
• Full review or specific categories (MobX, UI, etc.)?
• Should I suggest fixes or just identify issues?
• Are there specific concerns to focus on?

Key References

• REVIEW.md - Complete review checklist
• MOBX_ESSENTIALS.md - MobX patterns
• UI_COMPONENT_GUIDELINES.md - UI standards
• COMMON_PITFALLS.md - Known issues