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

Laying more foundation #5205

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Laying more foundation #5205

merged 1 commit into from
Jan 10, 2025

Conversation

jobelenus
Copy link
Contributor

@jobelenus jobelenus commented Jan 3, 2025

Looking to resolve a few fundamental problems:

  1. When making a change from HEAD the API call promises return to components that have been unmounted, which is destined to fail
  2. It is incredibly difficult to direct the UI correctly when a user makes a change to HEAD that creates a new change set.
  3. When router.push/router.replace is called, the vue router believes the currentRoute is the "original" (what is still in the URL bar), so subsequent router actions are almost always wrong which put the user right back to where they came from.
  4. We were dropping WsEvents on the floor when a user makes a change to HEAD, the WsEvent topics were correctly using the new changeSetId, however no stores were subscribed to receive them yet
  5. Now that we have mgmt functions which can create/update views & components, they may fire events that would normally navigate a user through the diagram, potentially creating chaos

With these changes:

  • Don't reference router.currentRoute, we store the "intended" URL in routerStore.currentRoute
  • Store actions that result in setSelected... or select... should not run in onActivated
  • Store onActivated generally shouldn't load data (one exception that works just fine is the component_attribute.store)
  • These should get moved to onBeforeMount of the appropriate component
  • Stores should be guarded from calling router.push/router.replace when their change set is not the selected change set
  • Do not await API responses in components and call setSelected... or select...
  • WsEvents from the system now return the "actor" (userPk) in their metadata
  • WsEvent subscriptions now should call setSelected... or select... when the actor is the current user
  • Add WsEvents that belong to a newly created change set to a buffer that gets drained once new subscriptions start
  • Keep track of which API endpoints create which WsEvents, so we can choose to ignore setSelected... or select... based on which API (like mgmt function) caused the event.

We shouldn't have to think about handling the "happy case" when in a change set, vs the "hard" case of "on head". Now, we don't have to await and check for HTTP 200, we just check the user in the WsEvent (and we don't lose any WsEvents).

@jobelenus jobelenus force-pushed the jobelenus/laying-more-foundation branch from b7c4201 to ed1fcc4 Compare January 6, 2025 20:42
Copy link

github-actions bot commented Jan 6, 2025

Dependency Review

✅ No vulnerabilities or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

@@ -525,6 +525,9 @@ const startTest = async () => {

readyToTest.value = true;

// TODO: @brit - currently test does not create a changeset
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@britmyerss TODO as we were discussing for when we get there!

@github-actions github-actions bot added the A-sdf Area: Primary backend API service [Rust] label Jan 7, 2025
@jobelenus jobelenus force-pushed the jobelenus/laying-more-foundation branch 2 times, most recently from 71ca279 to 60caf0b Compare January 7, 2025 22:43
@jobelenus jobelenus marked this pull request as ready for review January 9, 2025 16:04
@jobelenus jobelenus force-pushed the jobelenus/laying-more-foundation branch from bfc1417 to b6bff58 Compare January 9, 2025 16:08
stack72
stack72 previously approved these changes Jan 9, 2025
Copy link
Contributor

@stack72 stack72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through all cases that force a change-set and it feels way more smooth than it was before this change

@jobelenus jobelenus added this pull request to the merge queue Jan 9, 2025
@jobelenus jobelenus removed this pull request from the merge queue due to a manual request Jan 9, 2025
@jobelenus jobelenus added this pull request to the merge queue Jan 10, 2025
Merged via the queue into main with commit 647a77d Jan 10, 2025
10 checks passed
@jobelenus jobelenus deleted the jobelenus/laying-more-foundation branch January 10, 2025 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dal A-sdf Area: Primary backend API service [Rust] A-web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants