Fixed run button state reset on route change and state stale issue when a model fails to load

[ParamThakkar123]

Fixes two known issues on the foundation page:

Fixes #852
Fixes #850

[ParamThakkar123]

This PR fixes three issues on the foundation page:

1. When the model is selected on the foundation page and deleted without being ejected from the foundation page, the foundation page should no longer display it after deletion even though it's not ejected.
2. Foundation page state reset on route switch is fixed
3. The Run button was buffering even when the command line and electron alerts clearly displayed that the model has failed to load. After this fix the run button stops buffering after the model has failed to load

[deep1401]

Okay all 3 points worked for me but I have a tiny nitpick here.
When I tried to do delete a model which was running, it just ejected for me and now I'm stuck in this state:

Not sure what the right answer is here but I dont know if disabling the delete button for the running model entry would be going too far?

I'll just also wait for @dadmobile to comment on this one

[dadmobile]

I'd be fine with either:

• it stops the model before it deletes (but that might be tricky)
• it disables deleting a running model (should be pretty easy on the page...just read from experiment info and match)

[ParamThakkar123]

Sure I would try doing the second one maybe?

[ParamThakkar123]

Changes made:

• The delete button is disabled when the model is running and enabled again when the model stops running when we click the stop button on foundation page
• The bar on the top will stop displaying the model name once the model is deleted

[deep1401]

I think you'll need to implement this correctly, running just the base model disables everything related to that model's name.

I would recommend testing this by training your own model on some base model and seeing if you face the same

[deep1401]

Just updating the review here as it got posted without changing the PR state

[ParamThakkar123]

@deep1401 Noted. I implemented the changes and now only the model currently running will be disabled:

[deep1401]

Okay I tried this for 4 cases:

1. HF Model
2. Trained model by our plugin
3. GGUF model downloaded from HF
4. GGUF model exported by our plugin

It works for everything. Just adding a similar comment here as one of the other PRs that we can wait for @dadmobile to take a final look before we merge

[ParamThakkar123]

😄

[dadmobile]

Putting this in local storage seems like overkill? i'm guessing there was a problem with just using state?

[dadmobile]

There is so much new code for this one condition! I feel confident there must be a way to simplify/refactor this so it's not so complex? I feel we've already made it too complex and that's why we are having these bugs in the first place.

Let me take a closer look when I'm not on a plane and see if I can make some suggestions.

[dadmobile]

We have to go back to the drawing board here. As we've seen trying to debug the STT plugin issues, this screen is already too complicated. If anything let's try to figure out how to simplify/factor/abstract the variables to make this more clear as it did once work correctly!

Things we shouldn't be doing in this PR:

• Don't use storage: Generally the purpose of using storage would be to persist so that you can survive an app restart. That isn't what we are trying to do here.
• Avoid setInterval: If we think we need to use that it usually means we're doing something wrong/overly complicated with React.
• minimize useEffect: we're adding 3 effects with 7 different dependencies all of which can trigger re-renders. That's got to be overkill.

Before even trying to fix these bugs I'd almost be tempted to feed to original file into the AI and see if it can figure out how to simplify the dependencies. You might get some bug fixes for free. :)

[ParamThakkar123]

@dadmobile The state of the run button depends on the storage because The Run button UI reads and writes a per‑experiment marker to persistent storage (the RUN_MODEL_JOB.<id> key) and initializes its jobId from that value; the polling useEffect also consults and clears that stored marker (using -1 for “pending” and the backend job_id when available). Because the spinner state is derived from this persisted marker rather than only from in‑memory component state, the UI shows or hides buffering based on what’s in storage so if storage is cleared or the component unmounts/re-mounts without the marker being re-registered, the spinner can stop even though the backend job is still running.

[ParamThakkar123]

it also needs the setInterval there to keep polling for the state of jobId

[ParamThakkar123]

Because of that removing even one storage key stops the entire effect to happen

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!