Import / Export Feishin Settings #1163
Conversation
- This zone allows the dropping of files - The zone allows validation by parent - The zone allows customisation like icon shown
- Ability to import settings from a JSON file - Validation to ensure file compatibility - Visualiser for viewing string differences
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Sidenote - I cannot see why Vercel failed 😄 - If someone with perms could let me know if it's something I've done, that'd be great 🤗 |
kgarner7
left a comment
There was a problem hiding this comment.
- Does this export/import support upgrading versions? E.g., if you export an older version of the settings is imported. Alternatively, you may also just reject older versions of the settings. See the various
*.storemigrate functions for examples - This does not do meaningful validation of the imported data. A silly example is converting a bool value to an int, but more serous is custom CSS bypassing sanitation. Additionally, some functional things may just silently (or fatally) break (such as invalid custom fonts, bad sidebar routes).
1 - Brill, I will take a look at the migrate functions, I'll likely look for this PR to block any versions for now that mismatch the version in the current settings. 2 - Makes sense, do you have any suggestions on meaningful validation, does the app have any form of validation that could be leveraged here? I don't want to introduce a validator (e.g. ZOD, Valibot etc) as that could be a fairly big change to the codebase without proper approvals from yourself and other maintainers. |
Thanks for taking a stab at this. These two issues are why I was personally not keen to implement it. |
|
Super - I will give it another crack with this feedback in mind :) |
This commit contains the code to move settings to using ZOD, the reason for this is so that we can validate the settings schema that is being imported. This commit also adds various validation and transforms to ensure the settings being reimported match values we expect. I also removed the original crude validation and replaced it with the new ZOD parser that will handle this for us. Finally the "styles-settings" component will listen to any external content updates and update its value, the reasoning is the external import wouldn't update the existing value.
|
@kgarner7 - This commit is a slightly chunky one but has done the following at an attempt to address your comments :)
As a starting point, the keys you mentioned specifically above (apart from system font) have had their own validation / sanitisation rules
A note to add, as much as I agree with there are maybe nuances with the certain setting keys, I also would like to mention that this was the main reason I added this setting under the "Advanced" tab. I agree in trying to ensure what is imported is good data but also if a user does want to ruin their data externally and get confused on why it doesn't work when importing I feel is on them and not us as developers. I believe this is the same affect as someone copying the local storage key and doing the same thing albeit with the import we now has the measures in place to try stop the input of bad data :) With that said - I believe with the new in place ZOD schema though that if there are cases of further validation that it'll now be easier to implement which is a win for the project 🙌🏻 One final note - I believe that after this PR the logic for gathering the system fonts can be moved from the component itself, creating a cached list. This can then be used in both the font setting component and then leveraged via the ZOD validator, but I felt that was too much for this PR :) |
kgarner7
left a comment
There was a problem hiding this comment.
In theory, I like this, but given that my own schema is apparently already improper (I have a few column fields which somehow have no width), this is in practice harder.
(Example schema. Yes, I have some different keys, but the problem is tables.sideQueue.columns, userFavorite / userRating. I don't know why)
feishin-settings.json
Some thoughts:
- Is it possible to fix broken parts of the schema? (e.g., just fix or reset just the problematic blocks.
- Is it possible to maybe limit validation only to "critical" keys? (I don't know what that is, tbh). Maybe @jeffvli has better ideas?
- If validation is removed for (most) of the code (I do like the font/css changes regardless, so that might be good anyway), it might be useful to have an emergency reset settings.
- Settings were upgraded, so please d omerge that fix.
Obviously, users can just modify localStorage, but my biggest concern is that importing a file does not cause the state to break (and allow injecting at least invalid CSS, some of which could override the urls).
Some subfields are easy to validate, others are much harder, and knowing if something is "broken" can be tricky, and I honestly don't know how to approach it.
|
Thanks for the settings file - I'll take a look at this :) I think it would be possible to update / fix parts of broken schema, we have the initial state so I don't see any problems with there being some sort of step being like "Hey your schema is not quite right, we need to replace xyz with these default values" The Settings UI also has the "Reset" if the user gets in a pickle, but you're right that we should try not break the state, the state breaking could result in the UI crashing and forcing a a clear storage. |
|
@ThatsOurJake is attempting to deploy a commit to the jeffvli's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@kgarner7 - I had a look into If this is a bug or not outside of this, it is a tough call on which direction we take. I do wonder though if this is possible if this feature is something we community outsource to test? Make this feature an alpha / pre-release build open up a discussion for people to submit their issues and we can understand how people are using settings, it might also provide some insights into ways settings aren't "working" like your issue with there not being a default width on the queue sidebar. |
|
You can ignore any settings that pertain to the table / grid config. Those are currently being reworked in https://github.com/jeffvli/feishin/tree/feat/new-lists. Unfortunately I haven't been following very closely due to lack of time, but I will review all the discussion soon. |
|
Overall I'm fine with the PR's functionality as it's currently implemented.
|
|
Brill - Thanks @jeffvli, I'll action these comments shortly and re-request reviews 🎉 |
- Split Settings schema into two parts, schema that is validated on import and schema that is not - Schemas are merged to make the full SettingsStateSchema
- Migration is done as part of validation - Updated the store version to v10 as there has been changes to the settings - Migrate will now add the fields from v9 to v10
|
@jeffvli & @kgarner7 - This PR has now been updated with the actions from our discussions. Table is still part of the schema but not part of the validation and I've also updated the settings store version and added a migration for the new settings that were added. Let me know of any further changes and thanks both for your help on this 🙌🏻 |
- This is to ensure that every value of bindings is not null
Feature: The ability export and import the settings of Feishin
Motivation: I don't host Feishin as a docker image and instead use the standalone apps, I have one on my macbook and one of my windows PC and would like the settings across these apps to be synced. As a initial concept the settings are exported as JSON and imported into the other app instance.
Changes:
Evidence
The below screenshots show parts of the new feature in action: