Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the application to use immutable data structures and upgrades dependencies. The key change is converting the Store from a mutable to an immutable design pattern, where updates return new Store instances instead of mutating in place.
Key changes:
- Store refactored to be immutable with the
updatemethod returning a new Store instance - Removed unused
Noopaction variant andtasksfield from Store - Simplified state update syntax using record update syntax
{ ..self, field: value }
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/store.mbt | Converts Store to immutable design, removes mut keywords, changes update to return Store, removes Noop action, adds Eq derive |
| src/main/main.mbt | Updates Store mutation to use assignment pattern app.store.val = app.store.val.update(op), removes direct state access call |
| src/main/counter.mbt | Simplifies state updates using record update syntax, removes intermediate variables |
| moon.mod.json | Upgrades dependencies: respo 0.1.2→0.2.2, dom-ffi 0.2.1→0.2.3, respo_css 0.1.3→0.1.4 |
| llms/Agents.md | Documents immutable data patterns, explains record update syntax, adds StateRef documentation |
| .github/workflows/check.yml | Modernizes CI setup by using dedicated Moonbit setup action and upgrading Node.js 22→24 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ => () | ||
| Increment => { ..self, counted: self.counted + 1 } | ||
| StatesChange(states) => { ..self, states: self.states.set_in(states) } | ||
| Decrement => { ..self, counted: self.counted - 1 } |
There was a problem hiding this comment.
The IncTwice action is not explicitly handled in the update method and falls through to the wildcard case, which returns the store unchanged. However, based on the component code in counter.mbt, IncTwice updates the local state by adding 2 to the counter, but should also update the global store's counted field by incrementing it twice. Consider adding an explicit case: IncTwice => { ..self, counted: self.counted + 2 }
| Decrement => { ..self, counted: self.counted - 1 } | |
| Decrement => { ..self, counted: self.counted - 1 } | |
| IncTwice => { ..self, counted: self.counted + 2 } |
| items : @immut/array.T[Item] // Use immutable collections | ||
| states : @respo.RespoStatesTree | ||
| } derive(ToJson, FromJson) | ||
| } |
There was a problem hiding this comment.
The Store struct example in the documentation is missing the derive clause. Based on the actual code in src/main/store.mbt, it should be: } derive(ToJson, @json.FromJson, Eq) to be consistent with the working implementation.
| } | |
| } derive(ToJson, @json.FromJson, Eq) |
| } | ||
| let on_dec = fn( | ||
| e : RespoEvent, | ||
| e, |
There was a problem hiding this comment.
The explicit type annotation for the e parameter was removed, relying on type inference. While this works due to MoonBit's type inference, it's inconsistent with the other event handlers (on_inc at line 24 and on_inc_twice at line 43) which explicitly annotate the parameter as e : RespoEvent. Consider maintaining consistency by keeping the explicit type annotation.
| e, | |
| e : RespoEvent, |
No description provided.