-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: Add Cloud Config change history to roll back to previous values #2554
Conversation
Thanks for opening this pull request!
|
I will reformat the title to use the proper commit message syntax. |
Uffizzi Ephemeral Environment
|
My latest PR fixes the various UI/logic issues that you outlined, so I feel we don't need timestamps to prevent the dialog from breaking. I can still add if deemed necessary after a re-review
Not sure so I used an opinionated value of 10 history entries per parameter to be very light on the user's storage. I don't think we need that much data considering this feature is mostly about tracking the most recent changes 🤔 |
Uffizzi Ephemeral Environment
|
A scrollable list of buttons is an unconventional UI choice. Drop-down lists are traditionally used for this. We have the timestamp information, which is generally the primary information in a "rollback" feature, so we should display it. In a complex environment, param changes influence metrics over time, so time correlation between events is key. I understand that showing previous values in button labels can make it easier to quickly choose a value to roll back to, but that works only for short values (limited button label text length), and it is a duplicate functionality, because the full value is displayed before saving anyway. I would still suggest to change this to a drop-down list with timestamps. In future improvements we can think about convenience features such as quick value preview, version comparison (diff highlighting, etc) and how an intuitive UI should look like. Just an idea off the top of my head: if you would like to implement a quick value preview with this PR, why not add an "i" (info) icon on the side of the timestamp and show the value in a tool-tip when hovering with the mouse over it. This allows to display more of the previous value than the limited button label text length, so it would provide an even better value preview functionality than the list of buttons. Another option could be a button to fold-open the value beneath the timestamp, but that sounds more complicated to implement, maybe better to avoid for now.
I'm not sure whether Parse Server enforces any specific limit. All config params are stored in a single document. The max document size is 16MB. 16MB x 10 = 160MB max storage needs. Not sure what limits there are on the browser side. We can leave it as for now it and react to any feedback later on. |
Probably some style adjustments |
The icons are in a collective SVG I believe. Just look at any existing dialog that has an icon, it should point you to where that comes from. Could you please:
|
Uffizzi Ephemeral Environment
|
Uffizzi Ephemeral Environment
|
I tried implementing this, but this feature which connects a tooltip to its parent is only available in chrome 125 which itself seems to be in beta, even the anchor examples in that page don't run XD . Any sort of tooltip seems to be problematic to implement in one way or the other so I have removed the preview. |
Did it work? |
Uffizzi Ephemeral Environment
|
Yes, using the name and url to make a parsefile worked. I realized I was thinking about the native File() constructor initially |
How many versions are stored in the history? I thought you mentioned 10, but I just made 13 entries and they keep being stored. |
I had placed that limit, removed now |
Why removed, do we need one? |
I set it here, eventually I removed it because you suggested along the thread that parse-server has no limit |
These are 2 different questions:
Unless we look at storage size, it will be difficult to manage, because for a small config, many value versions will require little storage, while for a large config few versions require large storage. But looking at storage gives an arbitrary number of versions, which is difficult to manage for the user. Maybe the best is to implement a default of 100 versions, but make it configurable, so developers can adapt it to their use case. If you don't want to implement the configurable option now (it's quite easy), let's just leave it at 100. If we don't implement any limits, then we may create a problem for users, because if the storage size becomes too large, the user would have to clear the browser cache for the website, loosing all other cached options as well. |
The only limit is the one I had set before, no other. I had just limited it to 10 items earlier, 100 looks better. 10 was strictly an opinionated value from me because I thought some sort of limit is needed.
where in the dashboard should it be configured from? |
You could just add an option Btw, does the history work with multiple apps? For example app A has Cloud Config param |
I hadn't thought of that, probably not. Let me check. |
That is an example of how the history limit is configured in dashboard config currently, lmk if that's right. I have implemented unique history for every app by adding the application Id to the localstorage key of the app, if IDs are unique then the fix should hold. One last review, hopefully :) |
Yep that sounds perfect, great job, let's merge! |
Added README docs for new config option |
# [6.0.0-alpha.7](6.0.0-alpha.6...6.0.0-alpha.7) (2024-05-19) ### Features * Add Cloud Config change history to roll back to previous values ([#2554](#2554)) ([a784129](a784129))
🎉 This change has been released in version 6.0.0-alpha.7 |
New Pull Request Checklist
Issue Description
Adds recent config history using localstorage for better undo functionality
Closes: #2339
Approach
Every time the user updates a value in the config, it is saved to localstorage under the key
configHistory
.For each parameter in the config, an array of values is stored in descending order to show the latest value first.
TODOs before merging