Flatpak Update Mechanism #117
Conversation
|
Warning Rate limit exceeded@overheadhunter has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds Flatpak update support. pom.xml: bumps api.version to 1.8.0-beta1 and adds properties for slf4j, jackson, and flatpak-update-portal plus dependencies on jackson-databind and flatpak-update-portal. module-info.java: adds requires for org.purejava.portal, java.net.http, and com.fasterxml.jackson.databind; opens org.cryptomator.linux.update to org.cryptomator.integrations.api and com.fasterxml.jackson.databind; provides UpdateMechanism with FlatpakUpdater. New classes: FlatpakUpdater (implements UpdateMechanism, performs Flathub appstream HTTP checks, DBus UpdatePortal monitoring, preparation/apply/cancel flows, and Appstream response models) and FlatpakUpdateInfo (record). Adds META-INF services registration for FlatpakUpdater. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (2)
49-52: Remove redundant CreateUpdateMonitor call in constructor.The
CreateUpdateMonitorcall in the constructor appears redundant as it's called again ingetUpdateMonitor()method, and its return value is ignored here.public FlatpakUpdater() { this.portal = new UpdatePortal(); - portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY); }
173-193: Consider extracting a helper method to reduce code duplication.The pattern of extracting string values from variants is repeated multiple times.
+private String extractStringFromVariant(Map<String, Variant<?>> map, String key) { + Variant<?> variant = map.get(key); + return variant != null ? (String) variant.getValue() : ""; +} + private void notifyOnUpdateAvailable(Flatpak.UpdateMonitor.UpdateAvailable signal) { - String remoteCommit = ""; - Variant<?> remoteCommitVariant = signal.update_info.get("remote-commit"); - if (null != remoteCommitVariant) { - remoteCommit = (String) remoteCommitVariant.getValue(); - } - String runningCommit = ""; - Variant<?> runningCommitVariant = signal.update_info.get("running-commit"); - if (null != runningCommitVariant) { - runningCommit = (String) runningCommitVariant.getValue(); - } - String localCommit = ""; - Variant<?> localCommitVariant = signal.update_info.get("local-commit"); - if (null != localCommitVariant) { - localCommit = (String) localCommitVariant.getValue(); - } + String remoteCommit = extractStringFromVariant(signal.update_info, "remote-commit"); + String runningCommit = extractStringFromVariant(signal.update_info, "running-commit"); + String localCommit = extractStringFromVariant(signal.update_info, "local-commit"); UpdateAvailable updateAvailable = new UpdateAvailable(runningCommit, localCommit, remoteCommit); for (UpdateAvailableListener listener : updateAvailableListeners) { listener.onUpdateAvailable(updateAvailable); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xml(2 hunks)src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java(1 hunks)src/main/resources/META-INF/services/org.cryptomator.integrations.update.UpdateService(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/main/java/module-info.java (1)
Learnt from: infeo
PR: #80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the FreedesktopAutoStartService in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (1)
Learnt from: infeo
PR: #80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the FreedesktopAutoStartService in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.
🔇 Additional comments (4)
src/main/resources/META-INF/services/org.cryptomator.integrations.update.UpdateService (1)
1-1: LGTM!The service provider configuration correctly registers the
FlatpakUpdaterimplementation.pom.xml (2)
43-43: Verify the use of SNAPSHOT version for production readiness.The PR depends on a SNAPSHOT version of
integrations-api. If this PR is intended for production, consider waiting for the stable release of version 1.7.0.
46-46: LGTM!The new Flatpak update portal dependency is properly configured with a stable version.
Also applies to: 92-96
src/main/java/module-info.java (1)
6-6: LGTM!The module configuration correctly integrates the new Flatpak update service with proper dependencies, service provisions, and package access.
Also applies to: 9-9, 14-14, 22-22, 31-31, 36-36
| public UpdateCheckerTask getLatestReleaseChecker(DistributionChannel.Value channel) { | ||
| if (channel != DistributionChannel.Value.LINUX_FLATPAK) { | ||
| LOG.error("Wrong channel provided: {}", channel); | ||
| return null; | ||
| } | ||
| portal.setUpdateCheckerTaskFor(APP_NAME); | ||
| return portal.getUpdateCheckerTaskFor(APP_NAME); | ||
| } |
There was a problem hiding this comment.
Throw exception instead of returning null for invalid channel.
Returning null for an invalid channel could lead to NullPointerException in the calling code. Consider throwing an IllegalArgumentException instead.
@Override
public UpdateCheckerTask getLatestReleaseChecker(DistributionChannel.Value channel) {
if (channel != DistributionChannel.Value.LINUX_FLATPAK) {
- LOG.error("Wrong channel provided: {}", channel);
- return null;
+ throw new IllegalArgumentException("Wrong channel provided: " + channel + ". Expected: LINUX_FLATPAK");
}
portal.setUpdateCheckerTaskFor(APP_NAME);
return portal.getUpdateCheckerTaskFor(APP_NAME);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public UpdateCheckerTask getLatestReleaseChecker(DistributionChannel.Value channel) { | |
| if (channel != DistributionChannel.Value.LINUX_FLATPAK) { | |
| LOG.error("Wrong channel provided: {}", channel); | |
| return null; | |
| } | |
| portal.setUpdateCheckerTaskFor(APP_NAME); | |
| return portal.getUpdateCheckerTaskFor(APP_NAME); | |
| } | |
| @Override | |
| public UpdateCheckerTask getLatestReleaseChecker(DistributionChannel.Value channel) { | |
| if (channel != DistributionChannel.Value.LINUX_FLATPAK) { | |
| throw new IllegalArgumentException("Wrong channel provided: " + channel + ". Expected: LINUX_FLATPAK"); | |
| } | |
| portal.setUpdateCheckerTaskFor(APP_NAME); | |
| return portal.getUpdateCheckerTaskFor(APP_NAME); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
60 to 67, the method getLatestReleaseChecker returns null when an invalid
channel is provided, which risks causing NullPointerExceptions. Replace the null
return with throwing an IllegalArgumentException that clearly states the channel
is invalid. This change ensures callers are immediately informed of the misuse
and can handle the error properly.
| public void triggerUpdate() throws UpdateFailedException { | ||
| var monitor = getUpdateMonitor(); | ||
| portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY); | ||
| } |
There was a problem hiding this comment.
Avoid hardcoding the display parameter.
The hardcoded "x11:0" display parameter may not work correctly in Wayland environments or other display configurations.
Consider obtaining the display parameter dynamically:
@Override
public void triggerUpdate() throws UpdateFailedException {
var monitor = getUpdateMonitor();
- portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY);
+ String display = System.getenv("DISPLAY");
+ if (display == null || display.isEmpty()) {
+ display = System.getenv("WAYLAND_DISPLAY");
+ if (display != null && !display.isEmpty()) {
+ display = "wayland:" + display;
+ } else {
+ display = "x11:0"; // fallback
+ }
+ }
+ portal.updateApp(display, monitor, UpdatePortal.OPTIONS_DUMMY);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void triggerUpdate() throws UpdateFailedException { | |
| var monitor = getUpdateMonitor(); | |
| portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY); | |
| } | |
| @Override | |
| public void triggerUpdate() throws UpdateFailedException { | |
| var monitor = getUpdateMonitor(); | |
| String display = System.getenv("DISPLAY"); | |
| if (display == null || display.isEmpty()) { | |
| display = System.getenv("WAYLAND_DISPLAY"); | |
| if (display != null && !display.isEmpty()) { | |
| display = "wayland:" + display; | |
| } else { | |
| display = "x11:0"; // fallback | |
| } | |
| } | |
| portal.updateApp(display, monitor, UpdatePortal.OPTIONS_DUMMY); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
70 to 73, the display parameter "x11:0" is hardcoded in the portal.updateApp
call, which can cause issues in Wayland or other display environments. Modify
the code to dynamically obtain the display parameter from the environment or
system properties instead of using a fixed string. Replace the hardcoded "x11:0"
with a variable that retrieves the current display setting at runtime.
| @Override | ||
| public void close() throws Exception { | ||
| try { | ||
| if (null != updateMonitor) { | ||
| portal.cancelUpdateMonitor(updateMonitor); | ||
| } | ||
| portal.close(); | ||
| } catch (Exception e) { | ||
| LOG.error(e.toString(), e.getCause()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling in close method.
The current implementation catches a generic Exception and the logging could be more informative.
@Override
public void close() throws Exception {
try {
if (null != updateMonitor) {
portal.cancelUpdateMonitor(updateMonitor);
}
portal.close();
- } catch (Exception e) {
- LOG.error(e.toString(), e.getCause());
+ } catch (DBusException e) {
+ LOG.error("Failed to close DBus connection properly", e);
+ throw e;
+ } catch (Exception e) {
+ LOG.error("Unexpected error during resource cleanup", e);
+ throw e;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void close() throws Exception { | |
| try { | |
| if (null != updateMonitor) { | |
| portal.cancelUpdateMonitor(updateMonitor); | |
| } | |
| portal.close(); | |
| } catch (Exception e) { | |
| LOG.error(e.toString(), e.getCause()); | |
| } | |
| } | |
| @Override | |
| public void close() throws Exception { | |
| try { | |
| if (null != updateMonitor) { | |
| portal.cancelUpdateMonitor(updateMonitor); | |
| } | |
| portal.close(); | |
| } catch (DBusException e) { | |
| LOG.error("Failed to close DBus connection properly", e); | |
| throw e; | |
| } catch (Exception e) { | |
| LOG.error("Unexpected error during resource cleanup", e); | |
| throw e; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
98 to 108, improve the exception handling in the close method by catching more
specific exceptions if possible, and enhance the logging to provide a clearer
and more informative message. Instead of logging e.toString() and e.getCause()
separately, log the full exception with its stack trace to capture complete
context for debugging.
| private void notifyOnUpdateProceeds(Flatpak.UpdateMonitor.Progress signal) { | ||
| long status = ((UInt32) signal.info.get("status").getValue()).longValue(); | ||
| long progress = 0; | ||
| Variant<?> progressVariant = signal.info.get("progress"); | ||
| if (null != progressVariant) { | ||
| progress = ((UInt32) progressVariant.getValue()).longValue(); | ||
| } | ||
| long nOps = -1; | ||
| Variant<?> nOpsVariant = signal.info.get("n_ops"); | ||
| if (null != nOpsVariant) { | ||
| nOps = ((UInt32) nOpsVariant.getValue()).longValue(); | ||
| } | ||
| long oP = -1; | ||
| Variant<?> oPVariant = signal.info.get("op"); | ||
| if (null != oPVariant) { | ||
| oP = ((UInt32) oPVariant.getValue()).longValue(); | ||
| } | ||
| String error = ""; | ||
| Variant<?> errorVariant = signal.info.get("error"); | ||
| if (null != errorVariant) { | ||
| error = (String) errorVariant.getValue(); | ||
| } | ||
| String errorMessage = ""; | ||
| Variant<?> errorMessageVariant = signal.info.get("error_message"); | ||
| if (null != errorMessageVariant) { | ||
| errorMessage = (String) errorMessageVariant.getValue(); | ||
| } | ||
| Progress p = new Progress(nOps, oP, status, progress, error, errorMessage); | ||
| for (ProgressListener listener : progressListeners) { | ||
| listener.onProgress(p); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety checks and reduce code duplication.
The direct cast without type checking could throw ClassCastException. Also, the code has repetitive patterns.
+private long extractLongFromVariant(Map<String, Variant<?>> map, String key, long defaultValue) {
+ Variant<?> variant = map.get(key);
+ if (variant != null && variant.getValue() instanceof UInt32) {
+ return ((UInt32) variant.getValue()).longValue();
+ }
+ return defaultValue;
+}
+
private void notifyOnUpdateProceeds(Flatpak.UpdateMonitor.Progress signal) {
- long status = ((UInt32) signal.info.get("status").getValue()).longValue();
- long progress = 0;
- Variant<?> progressVariant = signal.info.get("progress");
- if (null != progressVariant) {
- progress = ((UInt32) progressVariant.getValue()).longValue();
- }
- long nOps = -1;
- Variant<?> nOpsVariant = signal.info.get("n_ops");
- if (null != nOpsVariant) {
- nOps = ((UInt32) nOpsVariant.getValue()).longValue();
- }
- long oP = -1;
- Variant<?> oPVariant = signal.info.get("op");
- if (null != oPVariant) {
- oP = ((UInt32) oPVariant.getValue()).longValue();
- }
- String error = "";
- Variant<?> errorVariant = signal.info.get("error");
- if (null != errorVariant) {
- error = (String) errorVariant.getValue();
- }
- String errorMessage = "";
- Variant<?> errorMessageVariant = signal.info.get("error_message");
- if (null != errorMessageVariant) {
- errorMessage = (String) errorMessageVariant.getValue();
- }
+ long status = extractLongFromVariant(signal.info, "status", 0);
+ long progress = extractLongFromVariant(signal.info, "progress", 0);
+ long nOps = extractLongFromVariant(signal.info, "n_ops", -1);
+ long oP = extractLongFromVariant(signal.info, "op", -1);
+ String error = extractStringFromVariant(signal.info, "error");
+ String errorMessage = extractStringFromVariant(signal.info, "error_message");
Progress p = new Progress(nOps, oP, status, progress, error, errorMessage);
for (ProgressListener listener : progressListeners) {
listener.onProgress(p);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java lines 195 to
226, the method notifyOnUpdateProceeds directly casts values from signal.info
without type checks, risking ClassCastException, and repeats similar code
patterns. Refactor by adding type checks before casting to ensure safety, and
extract the repeated pattern of retrieving and casting values from the map into
a helper method to reduce duplication and improve readability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (4)
59-62: Avoid hardcoding the display parameter.The hardcoded
"x11:0"display parameter may not work correctly in Wayland environments or other display configurations.Consider obtaining the display parameter dynamically:
@Override public void triggerUpdate() throws UpdateFailedException { var monitor = getUpdateMonitor(); - portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY); + String display = System.getenv("DISPLAY"); + if (display == null || display.isEmpty()) { + display = System.getenv("WAYLAND_DISPLAY"); + if (display != null && !display.isEmpty()) { + display = "wayland:" + display; + } else { + display = "x11:0"; // fallback + } + } + portal.updateApp(display, monitor, UpdatePortal.OPTIONS_DUMMY); }
82-92: Improve exception handling in close method.The current implementation catches a generic
Exceptionand the logging could be more informative. Additionally, exceptions should not be swallowed inclose()method as perAutoCloseablecontract.@Override public void close() throws Exception { try { if (null != updateMonitor) { portal.cancelUpdateMonitor(updateMonitor); } portal.close(); - } catch (Exception e) { - LOG.error(e.toString(), e.getCause()); + } catch (DBusException e) { + LOG.error("Failed to close DBus connection properly", e); + throw e; + } catch (Exception e) { + LOG.error("Unexpected error during resource cleanup", e); + throw e; } }
124-135: Add type safety checks to prevent ClassCastException.The direct cast without type checking could throw
ClassCastException. Add proper type validation before casting.private void notifyOnUpdateProceeds(Flatpak.UpdateMonitor.Progress signal) { - long status = ((UInt32) signal.info.get("status").getValue()).longValue(); + long status = 0; + Variant<?> statusVariant = signal.info.get("status"); + if (statusVariant != null && statusVariant.getValue() instanceof UInt32) { + status = ((UInt32) statusVariant.getValue()).longValue(); + } else { + LOG.warn("Missing or invalid status in update progress signal"); + } + long progress = 0; Variant<?> progressVariant = signal.info.get("progress"); - if (null != progressVariant) { + if (progressVariant != null && progressVariant.getValue() instanceof UInt32) { progress = ((UInt32) progressVariant.getValue()).longValue(); } Progress p = new Progress(status, progress); for (ProgressListener listener : progressListeners) { listener.onProgress(p); } }
94-112: Handle monitor creation failure more robustly.The method can return
nullif monitor creation fails, which could causeNullPointerExceptionintriggerUpdate(). Additionally, only theProgresssignal handler is registered, missing theUpdateAvailablehandler mentioned in past reviews.-private synchronized Flatpak.UpdateMonitor getUpdateMonitor() { +private synchronized Flatpak.UpdateMonitor getUpdateMonitor() throws UpdateFailedException { if (updateMonitor == null) { var updateMonitorPath = portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY); if (updateMonitorPath != null) { LOG.debug("UpdateMonitor successful created at {}", updateMonitorPath); updateMonitor = portal.getUpdateMonitor(updateMonitorPath.toString()); try { + portal.getDBusConnection().addSigHandler(Flatpak.UpdateMonitor.UpdateAvailable.class, signal -> { + LOG.info("Update available signal received"); + // Handle update available notification if needed + }); portal.getDBusConnection().addSigHandler(Flatpak.UpdateMonitor.Progress.class, signal -> { notifyOnUpdateProceeds(signal); }); } catch (DBusException e) { - LOG.error(e.toString(), e.getCause()); + updateMonitor = null; // Reset on error + throw new UpdateFailedException("Failed to register DBus signal handlers", e); } } else { - LOG.error("Failed to create UpdateMonitor on DBus"); + throw new UpdateFailedException("Failed to create UpdateMonitor on DBus"); } } return updateMonitor; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: for the `freedesktopautostartservice` in the cryptomator project, exceptions are preferred to contai...
Learnt from: infeo
PR: cryptomator/integrations-linux#80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.
Applied to files:
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java
| public FlatpakUpdater() { | ||
| this.portal = new UpdatePortal(); | ||
| portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary monitor creation in constructor.
The constructor creates an update monitor but doesn't check or use the result. This monitor should be created lazily when first needed via getUpdateMonitor().
public FlatpakUpdater() {
this.portal = new UpdatePortal();
- portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public FlatpakUpdater() { | |
| this.portal = new UpdatePortal(); | |
| portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY); | |
| } | |
| public FlatpakUpdater() { | |
| this.portal = new UpdatePortal(); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java between lines
42 and 45, remove the call to
portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY) from the constructor
since the update monitor is created but never used there. Instead, implement
lazy initialization of the update monitor inside the getUpdateMonitor() method
so that the monitor is only created when first accessed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (2)
41-44: Remove redundant monitor creation in constructor
portal.CreateUpdateMonitor(...)is invoked eagerly but its result is ignored and a fresh monitor is created later inprepareUpdate(), wasting a DBus round-trip. Drop this call and lazily create the monitor only when needed.
99-106: Handle potential DBusException fromupdateAppand make display selection dynamic
portal.updateApp(...)can throwDBusException; wrap the call in try/catch and fail the latch accordingly.- The display parameter is hard-coded to
"x11:0", which breaks on Wayland or non-standard X11 displays. Obtain it fromDISPLAY/WAYLAND_DISPLAYenv vars with a fallback.
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (1)
109-116: Null-safety for signal map access
signal.info.get("progress")is accessed without null-check before the cast at Line 114. Although unlikely, a missing key would raiseNullPointerException. Guard with a null check:Variant<?> progressVariant = signal.info.get("progress"); if (progressVariant != null && progressVariant.getValue() instanceof UInt32 u) { progress = u.doubleValue() / 100.0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/module-info.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-16T22:36:32.769Z
Learnt from: infeo
PR: cryptomator/integrations-linux#80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.
Applied to files:
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java
| portal.cancelUpdateMonitor(monitor); | ||
| stopReceivingSignals(); | ||
| portal.close(); // TODO: is this right? belongs to parent class. update can not be retried afterwards. or should each process have its own portal instance? | ||
| error = new UpdateFailedException("Update cancelled by user"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Portal lifecycle & resource leak
cancel() closes the shared portal, but the normal success path (applyUpdate) never does, leaving the DBus connection open for the lifetime of the JVM. Either:
• Make FlatpakUpdateProcess own a dedicated UpdatePortal and always close it in finally, or
• Let FlatpakUpdater implement AutoCloseable and close the portal when the application shuts down.
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
154 to 158, the shared portal is closed on cancel but not on successful update,
causing a resource leak. To fix this, either refactor FlatpakUpdateProcess to
own a dedicated UpdatePortal instance and ensure it is closed in a finally block
after update attempts, or modify FlatpakUpdater to implement AutoCloseable and
close the shared portal when the application shuts down, ensuring the DBus
connection is properly released in all cases.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (4)
40-43: Remove unused UpdateMonitor creation in constructor.The constructor creates an UpdateMonitor at line 42 but doesn't capture or use the result. This is wasteful since
firstStep()creates its own monitor when needed. Remove this call and rely on lazy initialization.Apply this diff:
public FlatpakUpdater() { this.portal = new UpdatePortal(); - portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY); }
101-110: Avoid hardcoding the display parameter.Line 109 hardcodes
"x11:0"which will fail in Wayland or non-standard X11 display configurations. Obtain the display parameter dynamically from environment variables.Consider this approach:
+ private static String getDisplayParameter() { + String display = System.getenv("DISPLAY"); + if (display != null && !display.isEmpty()) { + return display; + } + String waylandDisplay = System.getenv("WAYLAND_DISPLAY"); + if (waylandDisplay != null && !waylandDisplay.isEmpty()) { + return "wayland:" + waylandDisplay; + } + return "x11:0"; // fallback + } + @Override public void start() { try { this.signalHandler = portal.getDBusConnection().addSigHandler(Flatpak.UpdateMonitor.Progress.class, this::handleProgressSignal); } catch (DBusException e) { LOG.error("DBus error", e); latch.countDown(); } - portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY); + portal.updateApp(getDisplayParameter(), monitor, UpdatePortal.OPTIONS_DUMMY); }
156-162: Portal lifecycle issue: shared resource closed prematurely.Line 160 closes the shared
portalinstance from the parentFlatpakUpdater, making it unusable for subsequent operations. The TODO comment indicates developer uncertainty. Either:
- Let the parent
FlatpakUpdatermanage portal lifecycle (don't close here), or- Give each
FlatpakUpdateStepits own portal instanceRecommended approach - remove portal closure from cancel:
@Override public void cancel() { portal.cancelUpdateMonitor(monitor); stopReceivingSignals(); - portal.close(); // TODO: is this right? belongs to parent class. update can not be retried afterwards. or should each process have its own portal instance? error = new UpdateFailedException("Update cancelled by user"); }And ensure
FlatpakUpdaterproperly closes the portal when the updater itself is disposed (implementAutoCloseableor add cleanup method).
50-72: Add timeout to update availability check.The
cdl.await()call at line 65 blocks indefinitely. If the update check signal never arrives (e.g., due to DBus issues), this will hang forever. Add a reasonable timeout (e.g., 30 seconds) and handle the timeout case.Apply this diff:
try { - cdl.await(); - return updateAvailable.get(); + if (!cdl.await(30, TimeUnit.SECONDS)) { + LOG.warn("Update check timed out after 30 seconds"); + checkTask.cancel(); + return false; + } + return updateAvailable.get(); } catch (InterruptedException e) { checkTask.cancel(); Thread.currentThread().interrupt();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (1)
src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java (1)
CheckAvailability(21-81)
🔇 Additional comments (9)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (9)
45-48: LGTM!The availability check correctly delegates to the portal's availability status.
74-82: LGTM!The method properly validates the monitor creation and throws an appropriate exception on failure.
92-94: LGTM!Simple constructor correctly stores the monitor reference.
96-99: LGTM!Progress description is clear and properly formatted.
140-149: LGTM!Signal handler cleanup is properly implemented with appropriate error handling.
151-154: LGTM!Progress getter is correctly implemented with volatile field for thread safety.
164-172: LGTM!Await methods correctly delegate to the CountDownLatch.
174-182: LGTM!The non-blocking completion check with proper interrupt handling is correctly implemented.
184-187: LGTM!Clean use of the
UpdateStep.of()factory method.
| private void handleProgressSignal(Flatpak.UpdateMonitor.Progress signal) { | ||
| int status = ((UInt32) signal.info.get("status").getValue()).intValue(); | ||
| switch (status) { | ||
| case 0 -> { // In progress | ||
| Variant<?> progressVariant = signal.info.get("progress"); | ||
| if (progressVariant != null) { | ||
| progress = ((UInt32) progressVariant.getValue()).doubleValue() / 100.0; // progress reported as int in range [0, 100] | ||
| } | ||
| } | ||
| case 1 -> { // No update available | ||
| error = new UpdateFailedException("No update available"); | ||
| latch.countDown(); | ||
| } | ||
| case 2 -> { // Update complete | ||
| progress = 1.0; | ||
| latch.countDown(); | ||
| } | ||
| case 3 -> { // Update failed | ||
| error = new UpdateFailedException("Update preparation failed"); | ||
| latch.countDown(); | ||
| } | ||
| default -> { | ||
| error = new UpdateFailedException("Unknown update status " + status); | ||
| latch.countDown(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null and type safety checks for signal data.
Line 113 performs an unsafe cast without checking if signal.info.get("status") is null or if the value is actually a UInt32. This risks NullPointerException or ClassCastException.
Apply defensive checks:
private void handleProgressSignal(Flatpak.UpdateMonitor.Progress signal) {
- int status = ((UInt32) signal.info.get("status").getValue()).intValue();
+ Variant<?> statusVariant = signal.info.get("status");
+ if (statusVariant == null || !(statusVariant.getValue() instanceof UInt32)) {
+ LOG.warn("Invalid or missing status in progress signal");
+ return;
+ }
+ int status = ((UInt32) statusVariant.getValue()).intValue();
switch (status) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void handleProgressSignal(Flatpak.UpdateMonitor.Progress signal) { | |
| int status = ((UInt32) signal.info.get("status").getValue()).intValue(); | |
| switch (status) { | |
| case 0 -> { // In progress | |
| Variant<?> progressVariant = signal.info.get("progress"); | |
| if (progressVariant != null) { | |
| progress = ((UInt32) progressVariant.getValue()).doubleValue() / 100.0; // progress reported as int in range [0, 100] | |
| } | |
| } | |
| case 1 -> { // No update available | |
| error = new UpdateFailedException("No update available"); | |
| latch.countDown(); | |
| } | |
| case 2 -> { // Update complete | |
| progress = 1.0; | |
| latch.countDown(); | |
| } | |
| case 3 -> { // Update failed | |
| error = new UpdateFailedException("Update preparation failed"); | |
| latch.countDown(); | |
| } | |
| default -> { | |
| error = new UpdateFailedException("Unknown update status " + status); | |
| latch.countDown(); | |
| } | |
| } | |
| } | |
| private void handleProgressSignal(Flatpak.UpdateMonitor.Progress signal) { | |
| Variant<?> statusVariant = signal.info.get("status"); | |
| if (statusVariant == null || !(statusVariant.getValue() instanceof UInt32)) { | |
| LOG.warn("Invalid or missing status in progress signal"); | |
| return; | |
| } | |
| int status = ((UInt32) statusVariant.getValue()).intValue(); | |
| switch (status) { | |
| case 0 -> { // In progress | |
| Variant<?> progressVariant = signal.info.get("progress"); | |
| if (progressVariant != null) { | |
| progress = ((UInt32) progressVariant.getValue()).doubleValue() / 100.0; // progress reported as int in range [0, 100] | |
| } | |
| } | |
| case 1 -> { // No update available | |
| error = new UpdateFailedException("No update available"); | |
| latch.countDown(); | |
| } | |
| case 2 -> { // Update complete | |
| progress = 1.0; | |
| latch.countDown(); | |
| } | |
| case 3 -> { // Update failed | |
| error = new UpdateFailedException("Update preparation failed"); | |
| latch.countDown(); | |
| } | |
| default -> { | |
| error = new UpdateFailedException("Unknown update status " + status); | |
| latch.countDown(); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
112 to 138, the code unsafely casts signal.info.get("status").getValue() to
UInt32 and similarly assumes "progress" exists and is UInt32 — add defensive
null and type checks: retrieve the Variant<?> statusVariant =
signal.info.get("status"), if null set error = new
UpdateFailedException("Missing update status") and latch.countDown(); otherwise
get Object statusVal = statusVariant.getValue(), verify statusVal instanceof
UInt32 before casting, and if not set error with a descriptive message and
latch.countDown(); apply the same pattern for the "progress" variant (check null
and instanceof UInt32 before using), and ensure every early-error path sets
error and counts down the latch so the caller is not left waiting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (1)
135-135: Stop hardcoding the X11 display forportal.updateApp.
Wayland sessions (and even non-:0X11 seats) choke on"x11:0", soupdateAppthrows and the preparation step aborts immediately. Resolve the display token fromDISPLAY/WAYLAND_DISPLAYwith a fallback instead of hardcoding the X11 default.- portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY); + String displayToken = System.getenv("DISPLAY"); + if (displayToken == null || displayToken.isBlank()) { + String waylandDisplay = System.getenv("WAYLAND_DISPLAY"); + if (waylandDisplay != null && !waylandDisplay.isBlank()) { + displayToken = "wayland:" + waylandDisplay; + } + } + if (displayToken == null || displayToken.isBlank()) { + displayToken = "x11:0"; + } + portal.updateApp(displayToken, monitor, UpdatePortal.OPTIONS_DUMMY);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xml(3 hunks)src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/linux/update/FlatpakUpdateInfo.java(1 hunks)src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-16T22:36:32.769Z
Learnt from: infeo
Repo: cryptomator/integrations-linux PR: 80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.
Applied to files:
src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java
🔇 Additional comments (1)
src/main/java/org/cryptomator/linux/update/FlatpakUpdateInfo.java (1)
6-7:FlatpakUpdateInforecord fits the UpdateInfo contract.
The record cleanly carries the version plus mechanism and lines up with the generics expected byUpdateInfo.
| public void cancel() { | ||
| portal.cancelUpdateMonitor(monitor); | ||
| stopReceivingSignals(); | ||
| portal.close(); // TODO: is this right? belongs to parent class. update can not be retried afterwards. or should each process have its own portal instance? | ||
| error = new UpdateFailedException("Update cancelled by user"); | ||
| } |
There was a problem hiding this comment.
Ensure cancel completes and leaves the portal reusable.
After a user cancels, callers waiting on await()/isDone() block forever because the latch is never counted down. On top of that, closing the shared UpdatePortal here makes every subsequent update attempt with this FlatpakUpdater fail against the closed connection. Drop the close from the per-step cancel path and count the latch down so cancellation is deterministic; manage portal lifecycle at the updater level instead.
- portal.cancelUpdateMonitor(monitor);
- stopReceivingSignals();
- portal.close(); // TODO: is this right? belongs to parent class. update can not be retried afterwards. or should each process have its own portal instance?
- error = new UpdateFailedException("Update cancelled by user");
+ portal.cancelUpdateMonitor(monitor);
+ stopReceivingSignals();
+ error = new UpdateFailedException("Update cancelled by user");
+ latch.countDown();🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
183-188, the cancel() method currently closes the shared UpdatePortal and never
signals completion, causing await()/isDone() to block and later updates to fail
on a closed portal; modify cancel() to NOT call portal.close() (remove that
line), ensure the latch or completion signal used by await()/isDone() is counted
down (or set the done flag and notify listeners) so callers unblock
deterministically, keep portal.cancelUpdateMonitor(monitor) and
stopReceivingSignals(), and manage the portal lifecycle at the updater level
instead of per-step so subsequent updates can reuse the portal.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/module-info.java(1 hunks)
🔇 Additional comments (3)
src/main/java/module-info.java (3)
22-26: LGTM!The new module dependencies are appropriate for the Flatpak update feature:
org.purejava.portalfor D-Bus portal communicationjava.net.httpfor fetching update information from Flathubcom.fasterxml.jackson.databindfor parsing Appstream JSON responses
33-33: LGTM!The service provider registration correctly exposes
FlatpakUpdateras an implementation ofUpdateMechanismusing the module system'sprovides...withclause.
38-38: LGTM!Opening the
org.cryptomator.linux.updatepackage to bothorg.cryptomator.integrations.apiandcom.fasterxml.jackson.databindis appropriate:
- The integrations API needs reflective access to the
FlatpakUpdaterimplementation- Jackson needs reflective access to serialize/deserialize the Appstream response models
overheadhunter
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pom.xml(3 hunks)
🔇 Additional comments (1)
pom.xml (1)
90-94: Confirm dependency scopes and placement.The new jackson-databind and flatpak-update-portal dependencies are correctly placed in the runtime scope (compile scope, no
<scope>element). Verify these are indeed intended as compile-time dependencies and not test-only, given they support FlatpakUpdater functionality.Also applies to: 112-116
| <api.version>1.8.0-beta1</api.version> | ||
| <slf4j.version>2.0.17</slf4j.version> | ||
| <jackson.version>2.20.0</jackson.version> | ||
| <secret-service.version>2.0.1-alpha</secret-service.version> | ||
| <kdewallet.version>1.4.0</kdewallet.version> | ||
| <slf4j.version>2.0.17</slf4j.version> | ||
| <flatpakupdateportal.version>1.1.0</flatpakupdateportal.version> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify dependency versions for security and compatibility.
Three new properties are being introduced (lines 42-47). Before merge, please verify:
- Jackson 2.20.0 has no known CVEs and is compatible with SLF4J 2.0.17 and JDK 25
- SLF4J 2.0.17 is stable and compatible with existing transitive dependencies
- Flatpak Update Portal 1.1.0 from
org.purejavais accessible and verified
Additionally, confirm whether the slf4j.version property addition (line 43) is consistent—lines 88 and 126 already reference ${slf4j.version} without modification markers, which suggests this property may have been partially used elsewhere already.
🌐 Web query:
Are there any known CVEs for Jackson 2.20.0 and SLF4J 2.0.17?
💡 Result:
Short answer:
- Jackson 2.20.0 — no public CVE(s) identified affecting the upstream Jackson 2.20.0 release. (scanned NVD/Snyk/other vulnerability DBs). [1][2]
- SLF4J 2.0.17 — I found no NVD entry identifying a CVE against the upstream org.slf4j 2.0.17 artifacts, but there are CVEs/Red Hat advisories that affect RHEL/EAP packaged variants (e.g., CVE-2025-2251, CVE-2025-27611 referenced in Snyk/Red Hat advisories). Those advisories relate to distribution-specific packages and surrounding platforms (WildFly / JBoss EAP) rather than a direct upstream slf4j code bug in the 2.0.17 release. Verify applicability for your exact artifact/build. [3][4][5]
Sources:
- FasterXML Jackson release/wiki / package listings (2.20.0). [1]
- Snyk / package vulnerability pages (no direct vulnerabilities reported for jackson 2.20.0). [2]
- Snyk / NVD entries and Red Hat advisories for CVE-2025-2251 (WildFly/JBoss/EAP / packaged slf4j). [3]
- Snyk entries referencing eap8-slf4j-api / eap8-slf4j and CVE-2025-27611 / CVE-2025-2251. [4]
- SLF4J project releases (v_2.0.17). [5]
If you want, I can (a) check your exact Maven coordinates/JARs to confirm whether those RHEL/EAP CVEs apply, or (b) fetch latest NVD entries again now.
🌐 Web query:
Is flatpak-update-portal 1.1.0 from org.purejava available on Maven Central?
💡 Result:
No — I couldn't find org.purejava:flatpak-update-portal version 1.1.0 on Maven Central. Maven listings show the artifact but not a 1.1.0 release (libraries.io lists 1.0.0; mvnrepository shows the org.purejava flatpak-update-portal artifact without a 1.1.0 entry). [1][2]
Sources:
[1] libraries.io — org.purejava:flatpak-update-portal. (libraries.io)
[2] mvnrepository — org.purejava (flatpak-update-portal artifact listing). (mvnrepository.com)
Citations:
- 1: https://libraries.io/maven/org.purejava%3Aflatpak-update-portal?utm_source=openai
- 2: https://mvnrepository.com/artifact/org.purejava?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check pom.xml for:
# 1. slf4j.version property definition location
# 2. flatpak-update-portal dependency declaration
# 3. Custom repositories configuration
echo "=== Checking slf4j.version property definition ==="
rg -n "slf4j.version" pom.xml -A2 -B2
echo -e "\n=== Checking flatpak-update-portal dependency ==="
rg -n "flatpak-update-portal\|flatpakupdateportal" pom.xml -A3 -B3
echo -e "\n=== Checking repositories section ==="
rg -n "<repositories>" pom.xml -A20Length of output: 284
🏁 Script executed:
#!/bin/bash
# Check pom.xml for:
# 1. slf4j.version property definition location
# 2. flatpak-update-portal dependency declaration
# 3. Custom repositories configuration
echo "=== Checking slf4j.version property definition ==="
rg -n "slf4j.version" pom.xml -A2 -B2
echo -e "\n=== Checking flatpak-update-portal dependency ==="
rg -n "flatpak-update-portal\|flatpakupdateportal" pom.xml -A3 -B3
echo -e "\n=== Checking repositories section ==="
rg -n "<repositories>" pom.xml -A20
echo -e "\n=== Full properties section for context ==="
rg -n "<properties>" pom.xml -A50Length of output: 3186
🏁 Script executed:
#!/bin/bash
# Search for flatpak-update-portal dependency declaration anywhere in pom.xml
echo "=== Searching for flatpak dependency anywhere in pom.xml ==="
rg -n "flatpak\|purejava" pom.xml -i -A5 -B2
echo -e "\n=== Total line count and end of dependencies ==="
wc -l pom.xml
echo ""
tail -50 pom.xmlLength of output: 1316
Critical: Missing dependency declaration and invalid version for flatpak-update-portal.
Verification reveals two blocking issues:
-
Missing dependency declaration: The property
flatpakupdateportal.versionis defined at line 47 but has NO corresponding dependency declaration anywhere in pom.xml. The dependency must be added. -
Invalid version: Version 1.1.0 is not available on Maven Central; only version 1.0.0 is available. The build will fail at dependency resolution.
Action required: Either remove the unused property or add the missing dependency with a valid version (1.0.0 exists; verify if 1.1.0 should be obtained from a different repository).
Additional findings (verified as correct):
slf4j.versionproperty (line 43) is properly defined and referenced at lines 88 and 126 ✓- Jackson 2.20.0 has no known CVEs ✓
- SLF4J 2.0.17 upstream has no CVEs (distribution-specific advisories for RHEL/EAP packages do not apply) ✓
2 participants
Depends on
cryptomator/integrations-api#62cryptomator/integrations-api#72.Needs flathub/org.cryptomator.Cryptomator#128.