Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh related entries on changes #1377

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Jan 14, 2025

Fixes #1333.

One flaw in this design is that the $entries list in viewer/src/ProjectView.svelte comes from a derived, meaning it's read-only, so the only way to refresh an entry is to refresh the entire list. That's why I've left in a console.log('TODO: ...') line: so that if we decide to merge this as-is despite this flaw, we have an annoying console message nagging at us to find a better solution. I might want to work with Tim on fixing this, since it might involve a major rewrite to how we store entries. (Or we might decide it's not worth fixing, and we're okay with refreshing the entire entries list every time an entry's list of components or complex forms is changed).

Other than that, this works as desired: when you add a complex form or a component, the related entry is refreshed. When you delete a complex form or a component, the related entry is also refreshed.

@rmunn rmunn requested a review from hahn-kev January 14, 2025 21:50
@rmunn rmunn self-assigned this Jan 14, 2025
@rmunn rmunn changed the title Feat/related entries should refresh on changes Refresh related entries on changes Jan 14, 2025
Copy link

github-actions bot commented Jan 14, 2025

UI unit Tests

12 tests   12 ✅  0s ⏱️
 4 suites   0 💤
 1 files     0 ❌

Results for commit 2376f9a.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 14, 2025

C# Unit Tests

104 tests   104 ✅  5s ⏱️
 16 suites    0 💤
  1 files      0 ❌

Results for commit 2376f9a.

♻️ This comment has been updated with latest results.

@rmunn
Copy link
Contributor Author

rmunn commented Jan 14, 2025

@hahn-kev - Although this is marked as draft, it's fully functional except for the two flaws I mentioned in the description, one of which I'm going to fix tomorrow morning. So this isn't going to change much, and it should be suitable for a review right now.

rmunn added 3 commits January 14, 2025 17:14
Prepares for the 'refreshEntries' event coming soon
Now instead of dispatching multiple `refreshEnrtry` events, when complex
forms or comonents change and we need to refresh the related entries, we
will dispatch only one event so that a single API call can be made to
refresh all relevant entries at once.
@rmunn
Copy link
Contributor Author

rmunn commented Jan 14, 2025

Managed to fix the second flaw I mentioned in the description before going home for the evening, so I've updated the description to remove any mention of that second flaw (see edit history if you want to see it). So this PR has come as far as I'm going to be able to take it, unless we decide to make major structural changes to how we store entries so that we can update a selected subset.

So now I'm going to take this out of draft status and mark it as ready for review.

@rmunn rmunn marked this pull request as ready for review January 14, 2025 22:29
Copy link
Collaborator

for the issue of not being able to update a single entry, take a look at the onEntryUpdated callback on line 49 of ProjectView.svelte you would just need to call lexboxApi.getEntry then use the logic in onEntryUpdated to update the entries list.

That said, considering we have all the pluming for onEntryUpdated I'm not sure we even should be doing this in the front end. We could push the changes from the backend, it doesn't work this way today, but as an example inside of CreateComplexFormComponent we could notify that 2 entries were changed. That said it does have a number of downsides, the main one being that the said method should notify that both entries were changed, and if CreateComplexFormComponent is being called from UpdateEntry it's possible that there's a number of other changes, meaning we might end up notifying that the same entry changed multiple times, there are simple ways to mitigate that with the reactive model we're using in C#, but I'm not sure I want to go down that road.

I bring this up because I don't really like bringing back change detection code inside Editor.svelte we were doing that before and decided to shift that to the backend, now we just call update(before, after). That's going to set us down the path of duplicating checks for changes and bugs will come up from those getting out of sync. My above suggestion is a way to avoid them getting out of sync, but has other caveats. Alternitvly we could shift the refresh notification to be inside ComplexForms and ComplexFormComponents as it knows what data was just changed, so it could emit a refreshEntries event at the same time it emits a change. This would also be tricky as we would run into race conditions between the save and refresh events where the data hasn't actually been changed server side so the refresh would get the old data. State management, yaya.

@myieye
Copy link
Contributor

myieye commented Jan 15, 2025

@rmunn only _entries is derived. But we simply assign that reactivly to entries, which is writable.

I think I like the idea of moving the related entry change detection to the backend. 😬

But, yeah...it's the first time we'll start shooting off events without exactly knowing what context we're in, so it's a new concept.
Theoretically, the code in EntrySync.cs knows if we need to notify about the component changing or the complex-form changing, so that we wouldn't have to always notify about both.
With regards to duplicate notifications. That doesn't seem to critical to me. It might actually even be a feature: the UI is notified about some changes sooner than it might be otherwise 🤷.
If we were to try to trigger the events from EntrySync.cs I'm not sure exactly how we'd want to go about doing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

related entries don't refresh on changes
3 participants