Skip to content

Update preference management #43

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

Merged
merged 29 commits into from
May 21, 2025
Merged

Conversation

allison-truhlar
Copy link
Collaborator

@allison-truhlar allison-truhlar commented May 9, 2025

@krokicki @neomorphic
This PR is now ready for review. I can wait to fully merge it until I integrate the build changes from Jody.

The main change is adding the construction of the "uber map" in FileBrowserContext.tsx and updating the preference management to store hash keys. The format for these keys is the same as in the "uber map" for zones (zone_{fsp.zone}) and FSPs (fsp_{fsp.name}). The folder preference key format is folder_{fsp.name}_{folder.path}. In addition to being used to access zones and fsps for the favorites, the "uber map" is now also used for populating the ZoneBrowser.

I also updated some types/types names in shared.types.ts, most notably File to FileOrFolder, since that's a more accurate description and also avoids the error that would sometimes occur if I forgot to import the File type, and the built-in JS interface File would be assigned as the type.

Copy link

github-actions bot commented May 9, 2025

Binder 👈 Launch a Binder on branch JaneliaSciComp/fileglancer/update-preference-management

Copy link
Member

@krokicki krokicki left a comment

Choose a reason for hiding this comment

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

Great start! Some of my thoughts below.

@allison-truhlar
Copy link
Collaborator Author

@krokicki Do the PreferencesContext look on the right track? See commit fafb0a8

@krokicki
Copy link
Member

@krokicki Do the PreferencesContext look on the right track? See commit fafb0a8

Much improved! I can't build this PR right now though, there is some kind of build error.

flow: check if the key generated by makeMapKey is already in zoneFavorites. If so, delete entry from favorites. If not, add entry to favorites. The entry is in the same format as the zones in the "uber" map. Then, save the updated favorites to the backend. This is also in the same format as the zones in the "uber" map.
required so that PreferencesProvider can access the zone and file share paths 'uber' map
@allison-truhlar
Copy link
Collaborator Author

@krokicki The zone favorites should now be working on this branch. I decided to move forward with saving the zone favorite data in the same format at the Zone type used in the "uber" map, as opposed to the format we previously discussed: {type: zone, name: fsp.name}. The reason being I thought this would reduce the number of different states to keep track of. If we used the favorite-specific format, then I thought I would need to save these abbreviated data in a separate state to check whether a specific entry exists when updating the favorites, as well as separate state that stores the zones (or FSPs or folders) that gets passed to the components to render the favorites. Does this seem right to you? Is there any downside to saving the whole zone (or FSP) object to preferences, rather than just the abbreviated format?

@krokicki
Copy link
Member

krokicki commented May 16, 2025

Hi @allison-truhlar,

The reason for the expanded format was to make it easier to reconstitute the data. If you only have the key then you have to parse it (i.e. split on underscores). It will work to use the keys, but it introduces fragility and certain limitations. For example, it prevents the use of underscores in the values moving forward.

By the way, I was able to build the branch, but I can't see the file share paths. They seem to load fine in the console, and the zones are displayed, but it seems like they're all empty:

Screenshot 2025-05-16 at 8 37 02 AM

Also, it's still saving the entire object as a preference. The idea was to save as little information as possible, to avoid having data in multiple places that can be desynced, and also for efficiency.

Screenshot 2025-05-16 at 8 39 07 AM

Here, the preference should just be either {value: ["zone_Abbot", "zone_Beyene"]} or {value: [{type:"zone", name:"Abbot"},{type:"zone","Beyene"}]}. The rest of the information can be reconstituted when loading the preferences.

@allison-truhlar allison-truhlar marked this pull request as ready for review May 19, 2025 15:06
Copy link
Member

@krokicki krokicki left a comment

Choose a reason for hiding this comment

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

Not sure if it was the merge, but there are several things broken. I fixed one syntax issue, but there are a few in FavoritesBrowser that I can't resolve (it uses the type FolderFavorite which is not defined anywhere).

These issues are being caught if you run npm run eslint:check but for some reason we're not running that in the CI. We should probably enable that and prettier and make a habit of using both before pushing code (or even add a precommit).

@allison-truhlar allison-truhlar merged commit b1f631f into main May 21, 2025
5 checks passed
@allison-truhlar allison-truhlar deleted the update-preference-management branch May 21, 2025 20:04
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.

2 participants