Skip to content

Fix: now properly removing dangling default links#103

Merged
quintesse merged 3 commits into
mainfrom
fix_102
May 26, 2026
Merged

Fix: now properly removing dangling default links#103
quintesse merged 3 commits into
mainfrom
fix_102

Conversation

@quintesse
Copy link
Copy Markdown
Contributor

@quintesse quintesse commented May 25, 2026

The links for global default and version default JDKs weren't properly removed, confusing older JBang versions.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f2133d09-59af-4114-8d3f-413036291010

📥 Commits

Reviewing files that changed from the base of the PR and between a7fdea8 and 9f30c12.

📒 Files selected for processing (3)
  • src/main/java/dev/jbang/devkitman/JdkManager.java
  • src/main/java/dev/jbang/devkitman/util/FileUtils.java
  • src/test/java/dev/jbang/devkitman/TestJdkManager.java

📝 Walkthrough

Walkthrough

This PR refactors JDK uninstall handling to use nullable object references for tracking default state instead of boolean flags, adds robust file-identity comparison with fallback logic, corrects versioned-default detection, implements Windows directory junction support in FileUtils, and extends test coverage to validate default replacement selection and link removal.

Changes

JDK Uninstall Default State Handling

Layer / File(s) Summary
Default state detection and reset logic
src/main/java/dev/jbang/devkitman/JdkManager.java
uninstallJdk replaces boolean reset flags with nullable Jdk.InstalledJdk references (resetDefault, resetDefaultVer), uses Files.isSameFile (with IOException fallback to equals) to detect matching defaults, fixes versioned-default lookup to getDefaultJdkForVersion(jdk.majorVersion()), and conditionally replaces detected defaults with next/prev installed JDK or unsets via direct uninstall() call when no replacement exists.
Windows directory link creation
src/main/java/dev/jbang/devkitman/util/FileUtils.java
createLink detects directory targets on Windows and creates them as junctions with logging; non-directory and non-Windows paths fall back to symbolic link creation.
Test coverage for default uninstall and link verification
src/test/java/dev/jbang/devkitman/TestJdkManager.java
Test suite adds coverage for getDefaultJdkForVersion in default and default-plus scenarios, expands uninstall tests to verify pre-uninstall per-version defaults, assert replacement selection after uninstalling global/versioned defaults, and verify uninstalled JDK homes/links no longer exist via Files.exists with NOFOLLOW_LINKS.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A JDK falls, but all is not lost,
With nullables tracking the default's cost,
Windows gets junctions where symlinks won't dance,
Tests verify each link's last chance. 🔗✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing the removal of dangling default links, which is the primary objective addressed across the JdkManager refactoring and test expansions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix_102

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

@quintesse quintesse marked this pull request as ready for review May 26, 2026 08:05
@quintesse quintesse merged commit a3539fa into main May 26, 2026
4 checks passed
@quintesse quintesse deleted the fix_102 branch May 26, 2026 08:27
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.

1 participant