Conversation
✅ Deploy Preview for vortexfi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR aims to mitigate “orphan” phase-processing effects by centralizing persistence of phase transitions in the PhaseProcessor, rather than persisting during handler execution.
Changes:
PhaseProcessornow persists the handler result via.save()after thePromise.race(...)completes.BasePhaseHandler.transitionToNextPhasewas changed to return a new in-memoryRampStateinstance instead of updating/reloading from the DB.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/src/api/services/phases/phase-processor.ts | Saves the handler-returned state after the timeout race to make persistence happen in one place. |
| apps/api/src/api/services/phases/base-phase-handler.ts | Changes phase transition helper to return a newly built (in-memory) RampState instance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -105,12 +105,11 @@ export abstract class BasePhaseHandler implements PhaseHandler { | |||
| } | |||
| ]; | |||
|
|
|||
| await state.update({ | |||
| return RampState.build({ | |||
| ...state.get(), | |||
| currentPhase: nextPhase, | |||
| phaseHistory | |||
| }); | |||
There was a problem hiding this comment.
transitionToNextPhase now returns RampState.build({...state.get(), ...}). In Sequelize, Model.build() creates an unsaved instance with isNewRecord=true, so later calling .save() will attempt an INSERT (likely failing with a duplicate PK) rather than updating the existing ramp row. Additionally, copying ...state.get() risks persisting stale fields (e.g., processingLock) that were updated in the DB via RampState.update but not reflected on this in-memory instance.
Prefer mutating the existing state instance (e.g., state.set({ currentPhase: nextPhase, phaseHistory })) and returning that same instance so the processor can save only the intended changes without clobbering unrelated columns.
There was a problem hiding this comment.
I think we can ignore this comment as your changes should work as intended.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| // to avoid inserting new records or clobbering unrelated columns. | ||
| const updatedState = await state.update( | ||
| { currentPhase: pendingState.currentPhase }, | ||
| { fields: ["currentPhase"] } |
There was a problem hiding this comment.
I think we should also include the field phaseHistory here, as it's also returned from the 'transitionToNextPhase()' function.
| @@ -105,12 +105,11 @@ export abstract class BasePhaseHandler implements PhaseHandler { | |||
| } | |||
| ]; | |||
|
|
|||
| await state.update({ | |||
| return RampState.build({ | |||
| ...state.get(), | |||
| currentPhase: nextPhase, | |||
| phaseHistory | |||
| }); | |||
There was a problem hiding this comment.
I think we can ignore this comment as your changes should work as intended.
No description provided.