Skip to content

Conversation

@infeo
Copy link
Member

@infeo infeo commented Oct 17, 2025

This only effects classes in the event package.

Some events contained CryptoPaths, which cannot be accessed anymore if the actual filesystem is closed. This can lead to unforseen errors in consumer libraries, which expect the path objects can still be "used".

This is a breaking change.

@infeo infeo added this to the next milestone Oct 17, 2025
@infeo infeo self-assigned this Oct 17, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025

Walkthrough

This pull request modifies three event record classes in the cryptofs event system, converting Path-typed fields to String-typed fields. Specifically: BrokenFileNodeEvent's cleartextPath field changes from Path to String; ConflictResolutionFailedEvent's canonicalCleartextPath changes from Path to String; and ConflictResolvedEvent's canonicalCleartextPath and resolvedCleartextPath change from Path to String. All affected constructors are updated to convert Path parameters to Strings via toString(). Additionally, some records include getTimestamp() override methods.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Rationale: Three files are affected with a repetitive pattern of Path-to-String field conversions and constructor adjustments. While the changes are structurally straightforward (type conversions via toString()), they constitute public API modifications to record signatures that warrant careful verification of downstream consumer impact and serialization behavior. The homogeneous nature of the conversions reduces complexity compared to highly heterogeneous changes, but the scope of API modifications across multiple event records justifies moderate review effort.

Possibly related PRs

  • Feature: More events and event refinement  #284 — Modifies the same event record classes (BrokenFileNodeEvent and others), with direct code-level overlap where this PR's Path-to-String conversions build upon or depend on changes introduced in that PR.
  • Feature: Add timestamp to file system events #291 — Both PRs edit the identical event record declarations; the retrieved PR adds Instant timestamp and getTimestamp() accessors while this PR converts Path fields to String, indicating sequential or overlapping refactoring of these records.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "Fix: Remove CryptoPaths from API" directly aligns with the main objective of the changeset. The changes convert Path fields to String fields across multiple event classes (BrokenFileNodeEvent, ConflictResolutionFailedEvent, and ConflictResolvedEvent) to remove CryptoPath objects from the public API. The title is concise, clear, and accurately captures the primary intent without being vague or off-topic. While it doesn't mention implementation details like the String conversion or specific event classes, it correctly summarizes the core purpose of the PR.
Description Check ✅ Passed The pull request description is directly related to the changeset. The author clearly states that the changes affect "classes in the event package" and explains the motivation: events contained CryptoPath objects that cannot be accessed after the filesystem closes, which can cause errors in consumer libraries. The changeset confirms this by showing three event classes being modified to convert Path fields to String fields, effectively removing CryptoPaths from the API. The description provides meaningful information about both the problem being solved and the scope of the changes, and accurately characterizes this as a breaking change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/remove-cryptopaths-in-api

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: 3

🧹 Nitpick comments (3)
src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java (1)

10-10: Document string path format (absolute/relative; separator normalization)

Please clarify whether the stored string is absolute or relative to the vault root and which separator is guaranteed (e.g., forward slash). Consider normalizing before toString() if needed to ensure stable semantics across platforms.

src/main/java/org/cryptomator/cryptofs/event/BrokenFileNodeEvent.java (1)

10-10: Javadoc: specify path string canonicalization

Please note whether cleartextPath is normalized (e.g., no “.”/“..”), and the expected separator. This helps downstream consumers avoid platform-conditional parsing.

src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java (1)

15-18: Clarify path string semantics

Please document whether these strings are absolute vs. vault-relative and the separator guarantees. If they are vault-relative, say so explicitly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 60c6df5 and 9c0df88.

📒 Files selected for processing (3)
  • src/main/java/org/cryptomator/cryptofs/event/BrokenFileNodeEvent.java (1 hunks)
  • src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java (1 hunks)
  • src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java (1 hunks)
⏰ 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). (1)
  • GitHub Check: Analyse

@infeo infeo merged commit 7924b6e into develop Oct 17, 2025
8 checks passed
@infeo infeo deleted the feature/remove-cryptopaths-in-api branch October 17, 2025 13:56
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