nigig-org/REVIEWS/COST ESTIMATE REVIEW.md

analyse if we have covered all the review below
If I were taking over this project, I would **freeze feature development for 1–2 weeks** and do nothing but architecture work. The code is at the point where every new feature will either become much easier or much harder depending on what you do next.

This is the roadmap I'd follow.

---

# Phase 1 — Establish a Real Domain (Highest Priority)

## Goal

The application should revolve around an `Estimate`, not a screen.

Today the execution flow is roughly:

```text
User

↓

CostEstimateScreen

↓

EstimateStore

↓

Estimate
```

I want:

```text
User

↓

Command

↓

Estimate

↓

Domain Event

↓

UI
```

The UI should never decide how an estimate changes.

---

## Create explicit commands

Instead of:

```rust
screen.set_region(...)
screen.recalculate(...)
```

everything becomes:

```text
SetRegion

SetProperty

AddRoom

DeleteRoom

UpdateRoomDimensions

SetFloorCount

ImportEstimate

ExportEstimate
```

Every state change enters the system exactly one way.

---

## Estimate becomes the application

Today Estimate still behaves like a data structure.

It should become the application.

It should own:

```text
Estimate

Rooms

Selection

Totals

Floors

Property

Region

Validation

Calculations
```

Nothing outside Estimate should modify these.

---

# Phase 2 — Remove UI Orchestration

This is the biggest architectural problem today.

Current pattern:

```text
set property

↓

reload dropdown

↓

reload rooms

↓

update preview

↓

update totals

↓

emit action
```

That chain should disappear.

---

Instead:

```text
SetProperty Command

↓

Estimate updates

↓

EstimateChanged event

↓

UI redraws
```

The screen should become stupid.

Its job should be:

```text
Receive Event

↓

Refresh Widgets
```

That's it.

---

# Phase 3 — Introduce Services

Estimate is already growing.

Don't let it become another monolith.

Split responsibilities.

## RateService

```text
lookup_rate()

lookup_region()

lookup_property()
```

---

## RoomTemplateService

```text
available_rooms()

default_dimensions()

room_generation()
```

---

## CostService

```text
calculate_room()

calculate_total()

floor_multiplier()
```

---

## ExportService

```text
PDF

CSV

JSON
```

---

Estimate coordinates them.

It doesn't implement them.

---

# Phase 4 — Eliminate Manual Synchronisation

This is the smell I dislike most.

Current code has many functions like:

```text
refresh_property()

refresh_header()

refresh_dropdown()

reload_rooms()

sync_preview()
```

Instead introduce a ViewModel.

```text
Estimate

↓

EstimateViewModel

↓

Widgets
```

Widgets bind to data.

No widget should know another widget exists.

---

# Phase 5 — Build a Proper State Machine

Instead of scattered booleans

```text
loading

dirty

editing

calculating
```

make them explicit.

```rust
enum EstimateState {
    Loading,
    Ready,
    Editing,
    Saving,
    Exporting,
}
```

State machines eliminate entire categories of bugs.

---

# Phase 6 — Introduce Immutable Snapshots

Today changes happen directly.

I'd rather have

```text
Estimate

↓

clone

↓

modify

↓

replace
```

This immediately enables

* Undo
* Redo
* Autosave
* Time travel debugging
* Change history

---

# Phase 7 — Replace Nested Searches

Current:

```text
Category

↓

Region

↓

Property

↓

Room

↓

Rate
```

every lookup.

After JSON loads build indices.

```text
PropertyIndex

RateIndex

RoomIndex

RegionIndex
```

Everything becomes O(1).

---

# Phase 8 — UI Decomposition

Current screen still owns too much.

Split into

```text
EstimateHeader

EstimateSidebar

RoomEditor

TotalsPanel

PropertyPanel

RoomPanel

StatusBar
```

Each owns only its widgets.

---

# Phase 9 — Stronger Types

I would remove lots of

```rust
String
```

usage.

Replace with

```rust
PropertyId

RegionId

RoomId

EstimateId
```

Compiler catches mistakes.

---

# Phase 10 — Events Everywhere

Instead of

```text
screen

↓

store

↓

screen

↓

preview
```

everything communicates through events.

Example:

```text
RoomUpdated

↓

Estimate recalculates

↓

TotalsChanged

↓

Header redraw

↓

RoomUpdated

↓

Preview redraw
```

No direct widget calls.

---

# Phase 11 — Testing

Current testing is okay.

I'd increase it dramatically.

Test

* property switching
* room generation
* totals
* floor multiplier
* imports
* exports
* region changes
* invalid JSON
* invalid rooms
* duplicate rooms
* undo

UI tests are far less valuable than domain tests.

---

# Phase 12 — Performance

Do this last.

Optimise

* lookup indices
* redraw batching
* cached calculations

Ignore

* clone
* Vec
* HashMap

They're not your bottleneck.

---

# The Architecture I Would End Up With

```text
                 UI
                  │
        EstimateViewModel
                  │
             Event Bus
                  │
        ┌─────────┴─────────┐
        │                   │
    Commands            Domain Events
        │                   │
        └─────────┬─────────┘
                  │
              Estimate
                  │
     ┌────────────┼─────────────┐
     │            │             │
RoomStore   CostService   RateService
     │            │             │
     └────────────┴─────────────┘
                  │
          Building Repository
                  │
                 JSON
```

## What I'd do first (in order)

1. **Move all business rules into `Estimate`.** Nothing outside it should calculate totals, update rooms, or apply property changes.
2. **Replace the screen's manual refresh chains with domain events.** A state change should naturally produce an event that the UI responds to.
3. **Extract calculation and lookup logic into dedicated services.** Keep `Estimate` as the coordinator rather than the implementation of every algorithm.
4. **Introduce lookup indices after loading configuration.** This simplifies logic and prepares the application for larger datasets.
5. **Split the screen into smaller presentation components** once the domain layer is stable.

---

## One thing I would **not** do

I would **not** create more files just to reduce file size.

Splitting a 600-line file into six 100-line files doesn't improve architecture if the responsibilities stay the same.

Every extraction should answer one question:

> **Does this give a single component clear ownership of a single responsibility?**

If the answer is no, it's just moving code around.

That's the principle I'd use to guide every refactor from here onward.