Closed nickkeesG closed 3 months ago
Name | Link |
---|---|
Latest commit | 337dd426dae9ba7443683f1be7961d272993e9a6 |
Latest deploy log | https://app.netlify.com/sites/gleeful-biscuit-343233/deploys/665603ffd1dfce00071539b2 |
Netlify deployment looks great!
I need to think about how to write the migrations so that people don't lose their old daemons... Also I think we're missing a 'delete daemon' button, I might add that
I changed the base/target branch of this PR too to simplify the changeset and make it quicker for me to review
I'm noticing a weird bug when creating new daemons. The button to create a new daemon never reappears after the first click, and every time I save the most recently created daemon a new daemon gets created. It seems like it isn't exiting the "I'm creating a new daemon" state?
I've updated how new daemons are added. Now, the button never disappears. When the button is clicked, a new daemon is added with the name "new daemon" That's it. There's no intermediate "adding new daemon" state anymore. This fixes the bug I had before, and I think it's a lot easier to understand now.
Requesting additional review for the changes to adding new daemons (want to make sure I'm not messing something up that I don't understand)
Also, migration looks good!
Aah sorry I was going to review this today! I'll still do it 🙏
btw did you notice this before merging?
No! I missed that, but I fixed it with another commit to main. I know that's not good hygiene, but everything should be ok now.
There was a missing dependency somewhere. I thought about reverting the commit and doing it before the merge, but that looked complicated and scary.
Yeah no worries, no harm done :)
Finished reviewing, didn't find anything alarming in the code. Also thanks for fixing the migration. The result is good for at least my existing state on the main branch environment.
Found a bug: I noticed that if you have an incorrect API key, the daemons will print their most recent instruction instead of being quiet and showing an error (see img). Flagging here - I assume it'll be quick for you to fix but I can also look into it tomorrow. Let's fix it (and I'll also do some minor UI fixes) before pushing to production.
REDUX CHANGES HERE!
Changes:
Please let me know if anything seems wrong or confusing. This is the pull request I'm expecting will take the most work in setting up a migration.
I am planning to make future changes to the redux state, namely:
I don't know if you want to make this all part of the same migration, or have multiple migrations. I'm just going to continue adding the features I want to add. Please complain if this isn't working for you!