-
-
Notifications
You must be signed in to change notification settings - Fork 157
feat: preserve user settings when visiting random instance #454
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
base: main
Are you sure you want to change the base?
feat: preserve user settings when visiting random instance #454
Conversation
src/settings.rs
Outdated
|
||
// Get current user settings as JSON for API consumption | ||
pub async fn get_json(req: Request<Body>) -> Result<Response<Body>, String> { | ||
// Create preferences - this should be safe but we wrap it for completeness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we wrapping, exactly? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Removed the confusing comment entirely.
src/settings.rs
Outdated
// Try to encode preferences, but provide fallback if it fails | ||
let url_encoded = match prefs.to_urlencoded() { | ||
Ok(encoded) => Some(encoded), | ||
Err(e) => { | ||
eprintln!("Warning: Failed to encode preferences for settings transfer: {e}"); | ||
None | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything but the "url_encoded field" isn't even accessed in the javascript at all. Remove everything after here, just return the urlencoded value as a string here. We can test for an error via the HTTP code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Simplified the endpoint to return just the URL-encoded string directly with HTTP 200, or a plain text error message with HTTP 500. The javascript now reads the response as text instead of JSON.
static/check_update.js
Outdated
// Fetch list of available instances | ||
const instancesResponse = await fetch('/instances.json'); | ||
const instancesData = await instancesResponse.json(); | ||
const randomInstance = instancesData.instances[Math.floor(Math.random() * instancesData.instances.length)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of renaming these variables? No need for diff churn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! Reverted the variable names back to response
and data
to avoid unnecessary diff churn.
|
||
// Fetch current user settings to transfer them to the new instance | ||
let targetUrl = instanceUrl + window.location.pathname; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let text = "Visit Random Instance" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! Implemented this approach - much cleaner than my original variable naming.
static/check_update.js
Outdated
const settingsData = await settingsResponse.json(); | ||
// Check if settings were successfully encoded and are available for transfer | ||
if (settingsData.success && settingsData.url_encoded) { | ||
targetUrl = instanceUrl + '/settings/restore/?' + settingsData.url_encoded + '&redirect=' + encodeURIComponent(window.location.pathname.substring(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
targetUrl = instanceUrl + '/settings/restore/?' + settingsData.url_encoded + '&redirect=' + encodeURIComponent(window.location.pathname.substring(1)); | |
targetUrl = instanceUrl + '/settings/restore/?' + settingsData.url_encoded + '&redirect=' + encodeURIComponent(window.location.pathname.substring(1)); | |
text += " (bringing preferences)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
static/check_update.js
Outdated
} | ||
|
||
// Set the href of the <a> tag to the instance URL with path and settings included | ||
document.getElementById('random-instance').href = targetUrl; | ||
document.getElementById('random-instance').innerText = "Visit Random Instance"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document.getElementById('random-instance').innerText = "Visit Random Instance"; | |
document.getElementById('random-instance').innerText = text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
- Add /settings.json API endpoint to export current preferences - Enhance random instance redirect to include settings transfer - Implement comprehensive error handling with graceful fallbacks - Maintain full backward compatibility with existing functionality Fixes user experience issue where custom settings were lost when switching instances during errors. Users now seamlessly retain their personalized experience across instances.
- Simplify settings.json endpoint to return URL-encoded string directly - Use HTTP status codes for error handling (200/500) - Update button text to show '(bringing preferences)' when transferring - Revert unnecessary variable renaming (response, data) - Remove confusing comments per maintainer feedback
f82197c
to
a49a94b
Compare
Cleanup unused import after simplifying settings endpoint to return plain text
Fixes user experience issue where custom settings were lost when switching instances during errors. Users now seamlessly retain their personalized experience across instances.