Skip to content

Conversation

@infeo
Copy link
Member

@infeo infeo commented Jan 7, 2026

No description provided.

@infeo infeo added this to the next milestone Jan 7, 2026
@infeo infeo self-assigned this Jan 7, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 7, 2026

Walkthrough

The changes update logging statements across eight files in the CryptoFileSystem module. Updates include converting string concatenation to SLF4J parameterized messages, fixing log message formatting issues (such as stray braces), adjusting log levels in a few cases, and enhancing log message text for improved clarity. No changes to control flow, error handling semantics, or public APIs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

enhancement

🚥 Pre-merge checks | ❌ 3
❌ Failed checks (1 warning, 2 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'fixed some logging issues' is vague and generic. While it relates to the changeset, it lacks specificity about which logging issues were fixed or what the main improvements entail. Provide a more specific title that describes the primary logging improvements, such as 'Replace string concatenation with parameterized logging' or 'Improve log message formatting and consistency'.
Description check ❓ Inconclusive No pull request description was provided by the author, making it impossible to assess whether the description relates to the changeset. Add a pull request description explaining the logging issues fixed, the rationale for using parameterized logging, and the affected files or components.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/main/java/org/cryptomator/cryptofs/common/BackupHelper.java (1)

74-74: Minor formatting: Remove space before period.

The log message has an extra space before the final period.

✨ Suggested fix
-			LOG.warn("Failed to compare backup {} to original {} .", backupFile, originalFile, e);
+			LOG.warn("Failed to compare backup {} to original {}.", backupFile, originalFile, e);
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d75e7b and 387deff.

📒 Files selected for processing (9)
  • src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java
  • src/main/java/org/cryptomator/cryptofs/CryptoFileSystems.java
  • src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java
  • src/main/java/org/cryptomator/cryptofs/common/BackupHelper.java
  • src/main/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilter.java
  • src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java
  • src/main/java/org/cryptomator/cryptofs/health/dirid/DirIdCheck.java
  • src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java
  • src/main/java/org/cryptomator/cryptofs/migration/v7/MigratingVisitor.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-10T12:28:49.092Z
Learnt from: infeo
Repo: cryptomator/cryptofs PR: 263
File: src/main/java/org/cryptomator/cryptofs/FileNameDecryptor.java:53-54
Timestamp: 2024-12-10T12:28:49.092Z
Learning: In the `FileNameDecryptor` class (`org.cryptomator.cryptofs`), the `validatePath()` method ensures that filenames have a minimum length of 26 characters. Therefore, in `decryptFilenameInternal()`, it's safe to perform substring operations like `fullCipherNodeName.substring(fullCipherNodeName.length() - 4)` without additional length checks.

Applied to files:

  • src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java
📚 Learning: 2025-10-17T13:54:22.510Z
Learnt from: infeo
Repo: cryptomator/cryptofs PR: 319
File: src/main/java/org/cryptomator/cryptofs/event/BrokenFileNodeEvent.java:14-18
Timestamp: 2025-10-17T13:54:22.510Z
Learning: In the cryptofs event package (e.g., BrokenFileNodeEvent, ConflictResolutionFailedEvent, ConflictResolvedEvent), the event constructors are intended to be called only from within the filesystem implementation, so null-safety guards like Objects.requireNonNull() are not needed because the call sites are controlled and paths are guaranteed to be non-null.

Applied to files:

  • src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java
  • src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java
  • src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java
📚 Learning: 2025-10-14T16:55:45.287Z
Learnt from: infeo
Repo: cryptomator/cryptofs PR: 312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.

Applied to files:

  • src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java
📚 Learning: 2025-10-17T13:49:30.221Z
Learnt from: infeo
Repo: cryptomator/cryptofs PR: 312
File: src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java:108-118
Timestamp: 2025-10-17T13:49:30.221Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java`, the owner field is validated in `CryptoFileSystemProperties.withOwner(String)` to enforce a maximum length of 100 characters. This validation ensures that the serialized Properties payload (owner + lastUpdated timestamp + Properties overhead) never exceeds the 1000-byte INUSE_CLEARTEXT_SIZE buffer, making additional size checks in writeInUseFile() unnecessary.

Applied to files:

  • src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java
🧬 Code graph analysis (1)
src/main/java/org/cryptomator/cryptofs/health/dirid/DirIdCheck.java (1)
src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
  • Constants (13-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Analyse
  • GitHub Check: Build and Test
🔇 Additional comments (10)
src/main/java/org/cryptomator/cryptofs/CryptoFileSystems.java (1)

100-100: LGTM! Correct parameterized logging syntax.

The log statement correctly uses SLF4J placeholders with matching parameters, improving over string concatenation.

src/main/java/org/cryptomator/cryptofs/migration/v7/MigratingVisitor.java (1)

58-58: LGTM! Proper exception logging.

The parameterized logging correctly includes the exception as the last parameter, following SLF4J conventions.

src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (1)

108-108: Verify the log level change is appropriate for this context.

The log level was changed from debug to warn for InterruptedException. While the subsequent behavior (closing the token and marking the completion stage as failed) suggests this is an error condition, consider whether interruptions might occur during normal shutdown scenarios, in which case warn could generate noise during graceful termination.

Based on learnings, the RealUseToken class manages delayed persistence of in-use files with scheduled refreshes. If interruptions are expected during application shutdown, the log level might need adjustment.

src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java (2)

73-73: LGTM! Appropriate log level for recoverable failure.

The change from error to warn is appropriate since the IOException is handled gracefully (emitting an event and returning an empty stream) without propagating the exception. This is consistent with other warning logs in this class.


136-136: LGTM! Improved diagnostic context.

Adding the alternativePath parameter provides valuable context for debugging failed conflict resolution attempts.

src/main/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilter.java (1)

33-37: LGTM! Proper parameterized logging.

Both log statements correctly use SLF4J parameterized logging, improving performance by avoiding string concatenation when logging is disabled.

src/main/java/org/cryptomator/cryptofs/common/BackupHelper.java (1)

61-61: LGTM! Improved logging with SLF4J parameterized message.

The conversion from string concatenation to parameterized logging improves performance and follows SLF4J best practices.

src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java (1)

42-42: LGTM! Excellent conversion to SLF4J parameterized logging.

Both changes properly replace string concatenation with parameterized messages, improving performance by avoiding unnecessary string operations when logging is disabled.

Also applies to: 55-55

src/main/java/org/cryptomator/cryptofs/health/dirid/DirIdCheck.java (1)

114-114: LGTM! Appropriate log level adjustment.

Downgrading these messages from warn to debug is correct since all issues are already captured and reported through the resultCollector (LooseDirFile, ObeseDirFile, EmptyDirFile, DirIdCollision). The improved message clarity with the consistent "Encountered..." prefix is also beneficial.

Also applies to: 120-120, 123-123, 130-130

src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java (1)

108-108: LGTM! Improved message clarity.

The new message is much more descriptive, clearly explaining what's wrong (missing identification files) and what types could be expected (dir, symlink, or shortened file).

@infeo
Copy link
Member Author

infeo commented Jan 8, 2026

Note: Sonatype complains about "new, uncovered lines of code". This is a false positive, there are no "new" codelines.

@infeo infeo merged commit 5e63006 into develop Jan 8, 2026
6 of 7 checks passed
@infeo infeo deleted the feature/logging-issues branch January 8, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants