-
Notifications
You must be signed in to change notification settings - Fork 34
fix(update): Implement proper "once" behavior for check_updates setting #160
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?
Conversation
When check_updates is set to "once", the extension now correctly checks for updates only once per component, preventing repeated GitHub API calls on every Zed restart. Previously, the "once" mode would check GitHub API on every restart when no local installation existed, making it functionally identical to "always" mode until a download completed. Changes: - Add persistence mechanism using .update_checked marker files - Store downloaded version in marker files for future reference - Implement has_checked_once() and mark_checked_once() helpers - Update should_use_local_or_download() to check marker before allowing download - Apply fix to all components: JDTLS, Lombok, Debugger, and JDK Behavior after fix: - First run: GitHub API called → Download → Create marker with version - Subsequent runs: No GitHub API call (uses local or returns error) Marker files created: - jdtls/.update_checked - lombok/.update_checked - debugger/.update_checked - jdk/.update_checked
playdohface
left a comment
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.
Looks good and makes sense to me. Caveat: I haven't thoroughly tested that it works as expected on my setup, I could do that later tonight if you want
| .filter_map(Result::ok) | ||
| .map(|entry| entry.path()) | ||
| .filter(|path| path.is_dir()) | ||
| .filter_map(|path| { |
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.
nit: I have the feeling that the filter_map - map - filter - filter_map can be better expressed as a single filter_map, something along the lines of:
std::fs::read_dir(&jdk_path)
.ok()?
.filter_map(|entry| {
let entry = entry.ok()?;
let metadata = entry.metadata().ok()?; // I think we even avoid the second syscall this way
if !metadata.is_dir() {
return None;
}
let time = metadata.created().ok()?;
Some((entry.path(), time))
})
.max_by_key(|&(_, time)| time)
.map(|(path, _)| path)
```
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.
The same filter_map - map - filter - filter_map sequence is used also in other places, like in jdtls.rs .
We could have it cleaned throughout the project in a different PR
As I created a new revision right after your approval take the due time to test. There's no need to push it right away. |
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.
Appears to work as intended for me 🚀
When check_updates is set to "once", the extension now correctly checks for updates only once per component, preventing repeated GitHub API calls on every Zed restart.
Previously, the "once" mode would check GitHub API on every restart when no local installation existed, making it functionally identical to "always" mode until a download completed.
Changes:
Behavior after fix:
Marker files created: