Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.2k
Feature: URL checking#3716
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
WalkthroughThe changes integrate URL validation and dynamic hyperlink configuration into the application. A new GitHub Actions step is added to the release-check workflow to verify URL validity across markdown and JSON files. In the codebase, a new Java record is implemented to represent hyperlinks, including methods for JSON deserialization from a dedicated JSON file containing URL mappings. A provider method in the module initializes this record, making it available as a dependency. Additionally, the controller is updated to replace a hardcoded URL with a dynamically retrieved value from the injected hyperlinks record. These modifications introduce static analysis in the CI workflow and enable the application to manage external URLs through a configuration file without altering existing functionalities. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/cryptomator/ui/sharevault/ShareVaultController.java (1)
24-24:Make VISIT_HUB_URL final.Since this field is initialized in the constructor and never modified, it should be marked as final.
-private final String VISIT_HUB_URL;+private final String VISIT_HUB_URL;.github/workflows/release-check.yml (1)
61-64:Enhance URL validation configuration.Several improvements can be made to the URL validation step:
- Fix YAML syntax (extra spaces after colon)
- Consider adding
.xmlfiles to catch URLs in metainfo files- Add retry mechanism for transient failures
- uses: urlstechie/urlchecker-action@0.0.34+ uses: urlstechie/urlchecker-action@0.0.34 with:- file_types: .md,.json+ file_types: .md,.json,.xml include_files: README.md,src/main/resources/hyperlinks.json+ retry_count: 3+ timeout: 10🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 61-61: too many spaces after colon
(colons)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release-check.yml(1 hunks)src/main/java/org/cryptomator/common/Hyperlinks.java(1 hunks)src/main/java/org/cryptomator/launcher/CryptomatorModule.java(2 hunks)src/main/java/org/cryptomator/ui/sharevault/ShareVaultController.java(3 hunks)src/main/resources/hyperlinks.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/hyperlinks.json
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/release-check.yml
[warning] 61-61: too many spaces after colon
(colons)
🔇 Additional comments (2)
src/main/java/org/cryptomator/common/Hyperlinks.java (1)
11-20:Consider implementing the commented-out URLs.The commented code suggests additional URLs that might be needed. Consider implementing these URLs now to avoid future technical debt.
Would you like me to help implement these additional URL fields and update the corresponding JSON file?
src/main/java/org/cryptomator/launcher/CryptomatorModule.java (1)
27-31:LGTM!The provider method is correctly implemented with appropriate scoping and follows dependency injection best practices.
| publicstaticHyperlinksload() { | ||
| try { | ||
| returnJSON_DESERIALIZER.readValue(Hyperlinks.class.getResource("/hyperlinks.json"),Hyperlinks.class); | ||
| }catch (IOExceptione) { | ||
| thrownewRuntimeException(e); | ||
| } | ||
| } |
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.
🛠️ Refactor suggestion
Improve error handling and resource loading.
The current implementation has several areas for improvement:
- The generic
RuntimeExceptiondoesn't provide enough context about the failure. - Missing null check for
getResource()result. - No validation of loaded URLs.
Consider this improved implementation:
public static Hyperlinks load() {+ var resource = Hyperlinks.class.getResource("/hyperlinks.json");+ if (resource == null) {+ throw new IllegalStateException("hyperlinks.json not found in classpath");+ } try {- return JSON_DESERIALIZER.readValue(Hyperlinks.class.getResource("/hyperlinks.json"), Hyperlinks.class);+ var hyperlinks = JSON_DESERIALIZER.readValue(resource, Hyperlinks.class);+ validateUrls(hyperlinks);+ return hyperlinks; } catch (IOException e) {- throw new RuntimeException(e);+ throw new IllegalStateException("Failed to load hyperlinks.json: " + e.getMessage(), e); } }+private static void validateUrls(Hyperlinks hyperlinks) {+ validateUrl(hyperlinks.docsVolumeType(), "docsVolumeType");+ validateUrl(hyperlinks.docsGettingStarted(), "docsGettingStarted");+ validateUrl(hyperlinks.homepageHub(), "homepageHub");+}++private static void validateUrl(String url, String field) {+ try {+ new URI(url);+ } catch (URISyntaxException e) {+ throw new IllegalArgumentException("Invalid URL in " + field + ": " + url, 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.
| publicstaticHyperlinksload() { | |
| try { | |
| returnJSON_DESERIALIZER.readValue(Hyperlinks.class.getResource("/hyperlinks.json"),Hyperlinks.class); | |
| }catch (IOExceptione) { | |
| thrownewRuntimeException(e); | |
| } | |
| } | |
| publicstaticHyperlinksload() { | |
| +varresource =Hyperlinks.class.getResource("/hyperlinks.json"); | |
| +if (resource ==null) { | |
| +thrownewIllegalStateException("hyperlinks.json not found in classpath"); | |
| + } | |
| try { | |
| -returnJSON_DESERIALIZER.readValue(Hyperlinks.class.getResource("/hyperlinks.json"),Hyperlinks.class); | |
| +varhyperlinks =JSON_DESERIALIZER.readValue(resource,Hyperlinks.class); | |
| +validateUrls(hyperlinks); | |
| +returnhyperlinks; | |
| }catch (IOExceptione) { | |
| -thrownewRuntimeException(e); | |
| +thrownewIllegalStateException("Failed to load hyperlinks.json: " +e.getMessage(),e); | |
| } | |
| } | |
| +privatestaticvoidvalidateUrls(Hyperlinkshyperlinks) { | |
| +validateUrl(hyperlinks.docsVolumeType(),"docsVolumeType"); | |
| +validateUrl(hyperlinks.docsGettingStarted(),"docsGettingStarted"); | |
| +validateUrl(hyperlinks.homepageHub(),"homepageHub"); | |
| +} | |
| + | |
| +privatestaticvoidvalidateUrl(Stringurl,Stringfield) { | |
| +try { | |
| +newURI(url); | |
| + }catch (URISyntaxExceptione) { | |
| +thrownewIllegalArgumentException("Invalid URL in " +field +": " +url,e); | |
| + } | |
| +} |
| privatestaticfinalStringSCHEME_PREFIX ="hub+"; | ||
| privatestaticfinalStringVISIT_HUB_URL ="https://cryptomator.org/hub/"; | ||
| privatestaticfinalStringBEST_PRACTICES_URL ="https://docs.cryptomator.org/en/latest/security/best-practices/#sharing-of-vaults"; |
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.
🛠️ Refactor suggestion
Move BEST_PRACTICES_URL to Hyperlinks.
For consistency with the URL management strategy, consider moving the hardcoded BEST_PRACTICES_URL to the Hyperlinks record.
Apply this change to maintain all URLs in a single location:
-private static final String BEST_PRACTICES_URL = "https://docs.cryptomator.org/en/latest/security/best-practices/#sharing-of-vaults";And update thevisitBestPractices() method:
@FXML public void visitBestPractices() {- application.get().getHostServices().showDocument(BEST_PRACTICES_URL);+ application.get().getHostServices().showDocument(links.docsBestPractices()); }📝 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.
| privatestaticfinalStringSCHEME_PREFIX ="hub+"; | |
| privatestaticfinalStringVISIT_HUB_URL ="https://cryptomator.org/hub/"; | |
| privatestaticfinalStringBEST_PRACTICES_URL ="https://docs.cryptomator.org/en/latest/security/best-practices/#sharing-of-vaults"; | |
| privatestaticfinalStringSCHEME_PREFIX ="hub+"; | |
| -privatestaticfinalStringBEST_PRACTICES_URL ="https://docs.cryptomator.org/en/latest/security/best-practices/#sharing-of-vaults"; | |
| // BEST_PRACTICES_URL has been moved to the Hyperlinks dependency | |
| // ... other class members ... | |
| @FXML | |
| publicvoidvisitBestPractices() { | |
| -application.get().getHostServices().showDocument(BEST_PRACTICES_URL); | |
| +application.get().getHostServices().showDocument(links.docsBestPractices()); | |
| } |
overheadhunter 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.
This would be a fair approach, if we had to maintain dozens of links.
However I don't think that the 2 (TWO!) links justify the added complexity. Especially given that the sheer number of different workflows and highly specific jobs is already kind of confusing.
Uh oh!
There was an error while loading.Please reload this page.
This PR improves maintainability of the app.
Links can get stale/unreachable without the developers to notice. There are url-checker tools for this task, but we have the links cluttered all over the project, which would require to either scan the whole project (might take a long time) or to specify directories/single files, which would just move the maintenance to the ci script.
In order to reduce the number of files, this PR moves all non-api URLs to a single resource file, which is loaded during app startup. The parsing fails, if a URL is missing.
TODO: