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.

Sign up for GitHub

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

Jump to bottom

6913 lock version conflict mitigation #986

Merged
merged 9 commits into from
Dec 13, 2024
Merged

6913 lock version conflict mitigation #986

merged 9 commits into from
Dec 13, 2024

Conversation

ttoomey
Copy link
Contributor

@ttoomey ttoomey commented Dec 1, 2024 •
edited
Loading

Description

GH Issue

Two changes to try and reduce occurrence of users encountering lock version errors. Spot checking suggests this is often due to opening the same record in multiple tabs.

  1. add cross-tab sync mechanism. When a tab receives a new version (detected via lock-version), surface that on all other tabs (for clients, enrollments, and assessments) and prompt the user to reload
    • This does not address conflicts due to server side changes
    • users can close snack bar alert
    • known issue: if the user has a form open when the reload, their current form data would be lost. Seems like this could be addressed with improved props handling or maybe some way to temporarily save and re-apply form-state
    • The current implementation uses BroadcastChannel which is a newer feature. Looks like it's now supported on evergreen browsers
  2. when clicking the button to edit the client record, re-fetch the client data (could extend to other pages)
    • this might catch server side changes
    • we could take this approach on other buttons but the client form is the low hanging fruit

misc fix:

  • bug fix for ConfirmationDialog not showing cancelText

##testing

  1. testing cross tab sync: open a two tabs with the same client. Update the client record in one, the other tab should display prompt to reload the client record. Reloading should load new data and close the prompt. Clicking continue should close the prompt but not reload.
  2. test client refresh: go to the client dash. In the console or else where, update the client (client.touch in rails). Open client details form and save, there should be no lock-version error

Type of change

  • Bug fix
  • New feature (adds functionality)

Checklist before requesting review

  • I have performed a self-review of my code
  • I have run the code that is being changed under ideal conditions, and it doesn't fail
  • My code includes comments and/or descriptive variable names to help other engineers understand the intent (or not applicable)
  • My code follows the style guidelines of this project (eslint)
  • I have updated the documentation (or not applicable)
  • If it's not obvious how to test this change, I have provided testing instructions in this PR or the related issue

@ttoomey ttoomey changed the title 6913 cross tab sync 6913 lock version conflict mitigation Dec 1, 2024
ttoomey
ttoomey commented Dec 1, 2024
View reviewed changes
src/components/elements/ConfirmationDialog.tsx
@@ -73,7 +73,7 @@ const ConfirmationDialog = ({
variant='gray'
data-testid='cancelDialogAction'
>
{cancelText || unconfirmable ? 'Close' : 'Cancel'}
{cancelText || (unconfirmable ? 'Close' : 'Cancel')}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like a bug where cancelText was not displayed

src/modules/enrollment/components/pages/EnrollmentDashboard.tsx Show resolved Hide resolved
src/modules/localVersionCoordination/hooks/useLocalVersionCoordination.tsx
const channelRef = useRef<BroadcastChannel | null>(null);
useEffect(() => {
const channelName = `record-sync-${recordType}-${recordId}`;
channelRef.current = new BroadcastChannel(channelName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BroadcastChannel docs here

src/modules/assessments/components/IndividualAssessmentPage.tsx Show resolved Hide resolved
src/modules/localVersionCoordination/LocalVersionCoordinationPrompt.tsx Show resolved Hide resolved
@ttoomey ttoomey requested a review from gigxz December 1, 2024 22:46
@ttoomey
6913-cross-tab-sync
09c4cd4 
* use snackbar to display refresh local version, adjust language
* add generic snackbar alert component
* adjust eslint to allow us to use console statements in stories
* adjust storybook so it doesn't try to open itself on start (breaks my
  docker container)
@ttoomey
cleanups
152c6d3
@ttoomey
language
7e863d3
ttoomey
ttoomey commented Dec 5, 2024
View reviewed changes
src/components/elements/SnackbarAlert.tsx
alertProps?: AlertProps;
title?: string;
}
const SnackbarAlert: React.FC<Props> = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we'd use this in the existing Apollo Alert but refactoring that felt like a bit too much scope creep for this branch

src/modules/enrollment/components/pages/EnrollmentDashboard.tsx Show resolved Hide resolved
@ttoomey ttoomey marked this pull request as ready for review December 5, 2024 15:59
@gigxz gigxz changed the base branch from release-143 to release-144 December 13, 2024 14:18
gigxz
gigxz approved these changes Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@gigxz gigxz left a comment

Choose a reason for hiding this comment

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

This looks great @ttoomey!

@gigxz
Copy link
Collaborator

gigxz commented Dec 13, 2024

Screenshot 2024-12-13 at 12 46 09 PM

5808a54

Linking PR #991 and added a TODO for it, because I will update Alerts to use the grayscale button variant when merged, per design library!

@gigxz gigxz merged commit f825592 into release-144 Dec 13, 2024
3 checks passed
@gigxz gigxz deleted the 6913-cross-tab-sync branch December 13, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gigxz gigxz gigxz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ttoomey @gigxz