Skip to content

Commit e203596

Browse files
gnodetclaude
gnodet
and
claude
authored
refactor: consolidate TUI code, add tests, centralize HTTP client, improve error handling (#74)
* refactor: consolidate TUI code, add tests, centralize HTTP client, improve error handling Seven-phase code quality improvement: - Fix double scanFile() call, empty-list guards, synchronized(AtomicInteger), path traversal - Add PomEditSessionTest (14 tests) and SortStateTest (18 tests) - Consolidate quit/save/diff into ToolPanel base class with hook methods - Extract renderStandaloneInfoBar template method, eliminating info bar duplication - Extract SearchController, removing ~100 lines of duplicated search code from ModuleTreePane - Create MavenCentralClient centralizing all Maven Central HTTP logic, remove ReleaseDateFetcher - Add vulnFetchFailed tracking in AuditTui, add stderr logging to silent catch blocks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address SonarCloud findings (S106, S112, S1075, S1066, S3776, S5838) Replace System.err with java.util.logging.Logger in PilotMain and PilotEngine. Use IOException/ParserConfigurationException instead of generic Exception in MavenCentralClient. Extract pomBasePath to avoid hardcoded path delimiter. Merge nested if in getChildText. Split SearchController.handleSearchInput to reduce cognitive complexity. Use assertThat().isZero() in SortStateTest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address remaining SonarCloud findings (S112, S3516) Use specific exception types in MavenCentralClient.parsePomInfo. Refactor SearchController.handleSearchModeInput to void since it always returns true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: retrigger CI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review findings - Use toRealPath() for symlink-safe reactor root boundary check - Remove misleading Enter:Details hint from ConflictsTui - Fix DependenciesTui key hints: s→c for scope, view-specific actions - Fix fetchVersions partial-result ordering bug on exception - Avoid restarting date fetches when quit is cancelled in UpdatesTui - Strengthen SortStateTest reverseDirection assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address new SonarCloud findings (S6541, S1192) Extract recurseIntoModules from discoverModulesRecursive to reduce cognitive complexity. Extract HINT_REMOVE constant in DependenciesTui to eliminate duplicate string literal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 73c27cc commit e203596

15 files changed

Lines changed: 1197 additions & 758 deletions

File tree

‎pilot-cli/src/main/java/eu/maveniverse/maven/pilot/mvn4/PilotMain.java‎

Lines changed: 37 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import eu.maveniverse.maven.pilot.ReactorModel;
2828
import eu.maveniverse.maven.pilot.SearchTui;
2929
import eu.maveniverse.maven.pilot.XmlTreeModel;
30+
import java.io.IOException;
3031
import java.nio.file.Files;
3132
import java.nio.file.Path;
3233
import java.util.ArrayList;
@@ -36,6 +37,7 @@
3637
import java.util.Properties;
3738
import java.util.concurrent.CompletableFuture;
3839
import java.util.concurrent.atomic.AtomicReference;
40+
import java.util.logging.Logger;
3941
import javax.xml.parsers.DocumentBuilderFactory;
4042
import org.apache.maven.api.DownloadedArtifact;
4143
import org.apache.maven.api.Session;
@@ -65,6 +67,7 @@
6567
*/
6668
public class PilotMain {
6769

70+
private static final Logger LOGGER = Logger.getLogger(PilotMain.class.getName());
6871
private static final String POM_XML = "pom.xml";
6972

7073
record LoadedReactor(Map<Path, PilotProject> projectsByPomPath, PilotEngine engine) {}
@@ -165,10 +168,11 @@ private static Path findRootPom(Path pomPath) {
165168

166169
// ── Quick reactor discovery ─────────────────────────────────────────
167170

168-
private static List<PilotProject> discoverReactorFromXml(Path pomPath) {
171+
private static List<PilotProject> discoverReactorFromXml(Path pomPath) throws IOException {
169172
List<PilotProject> projects = new ArrayList<>();
170173
Map<PilotProject, String> declaredParentGa = new LinkedHashMap<>();
171-
discoverModulesRecursive(pomPath, null, null, projects, declaredParentGa);
174+
Path rootDir = pomPath.getParent().toRealPath();
175+
discoverModulesRecursive(pomPath.toRealPath(), null, null, projects, declaredParentGa, rootDir);
172176
// Wire parent references by matching declared <parent> GA
173177
Map<String, PilotProject> projectsByGa = new LinkedHashMap<>();
174178
for (PilotProject p : projects) {
@@ -188,7 +192,8 @@ private static void discoverModulesRecursive(
188192
String parentGroupId,
189193
String parentVersion,
190194
List<PilotProject> projects,
191-
Map<PilotProject, String> declaredParentGa) {
195+
Map<PilotProject, String> declaredParentGa,
196+
Path rootDir) {
192197
try {
193198
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
194199
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
@@ -238,23 +243,36 @@ private static void discoverModulesRecursive(
238243
declaredParentGa.put(project, declaredParentGroupId + ":" + declaredParentArtifactId);
239244
}
240245

241-
// Recurse into modules
242-
Element modulesEl = getDirectChildElement(root, "modules");
243-
if (modulesEl != null) {
244-
NodeList moduleNodes = modulesEl.getElementsByTagName("module");
245-
for (int i = 0; i < moduleNodes.getLength(); i++) {
246-
String moduleName = moduleNodes.item(i).getTextContent().trim();
247-
Path modulePom = basedir.resolve(moduleName)
248-
.resolve(POM_XML)
249-
.toAbsolutePath()
250-
.normalize();
251-
if (Files.isRegularFile(modulePom)) {
252-
discoverModulesRecursive(modulePom, groupId, version, projects, declaredParentGa);
253-
}
246+
recurseIntoModules(root, basedir, groupId, version, projects, declaredParentGa, rootDir);
247+
} catch (Exception e) {
248+
LOGGER.warning("Skipping unparseable module: " + pomPath + " (" + e.getMessage() + ")");
249+
}
250+
}
251+
252+
private static void recurseIntoModules(
253+
Element root,
254+
Path basedir,
255+
String groupId,
256+
String version,
257+
List<PilotProject> projects,
258+
Map<PilotProject, String> declaredParentGa,
259+
Path rootDir)
260+
throws IOException {
261+
Element modulesEl = getDirectChildElement(root, "modules");
262+
if (modulesEl == null) return;
263+
NodeList moduleNodes = modulesEl.getElementsByTagName("module");
264+
for (int i = 0; i < moduleNodes.getLength(); i++) {
265+
String moduleName = moduleNodes.item(i).getTextContent().trim();
266+
Path modulePom = basedir.resolve(moduleName)
267+
.resolve(POM_XML)
268+
.toAbsolutePath()
269+
.normalize();
270+
if (Files.isRegularFile(modulePom)) {
271+
Path realModulePom = modulePom.toRealPath();
272+
if (realModulePom.startsWith(rootDir)) {
273+
discoverModulesRecursive(realModulePom, groupId, version, projects, declaredParentGa, rootDir);
254274
}
255275
}
256-
} catch (Exception ignored) {
257-
// Skip unparseable modules
258276
}
259277
}
260278

@@ -429,7 +447,7 @@ private static void resolveExternalParentChain(
429447
// Recursively extend the parent's parent chain
430448
resolveExternalParentChain(session, mbs, parentProject, projectsByGa, extensionProperties);
431449
} catch (Exception e) {
432-
// External parent resolution is best-effort
450+
LOGGER.warning("Could not resolve external parent for " + project.ga() + ": " + e.getMessage());
433451
}
434452
}
435453

‎pilot-core/src/main/java/eu/maveniverse/maven/pilot/AuditTui.java‎

Lines changed: 35 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ public static class AuditEntry {
6969
List<OsvClient.Vulnerability> vulnerabilities;
7070
boolean licenseLoaded;
7171
boolean vulnsLoaded;
72+
boolean vulnFetchFailed;
7273

7374
public AuditEntry(String groupId, String artifactId, String version, String scope) {
7475
this.groupId = groupId;
@@ -141,7 +142,6 @@ boolean isGroup() {
141142
private static final String COL_SCOPE = "scope";
142143
private static final String LABEL_SCOPE = " scope: ";
143144
private static final List<String> SCOPE_FILTERS = List.of("compile", "runtime", "test", "provided");
144-
private final DiffOverlay diffOverlay = new DiffOverlay();
145145
private final TableState vulnTableState = new TableState();
146146
private final TableState byLicenseTableState = new TableState();
147147

@@ -151,7 +151,6 @@ boolean isGroup() {
151151
private int vulnsLoaded = 0;
152152
private int vulnCount = 0;
153153
private String status;
154-
private boolean pendingQuit;
155154
private int lastContentHeight;
156155
private int lastTableHeight;
157156
private String scopeFilter; // null = show all
@@ -273,6 +272,7 @@ private void fetchVulnsForEntry(AuditEntry entry) {
273272
try {
274273
return osvClient.query(entry.groupId, entry.artifactId, entry.version);
275274
} catch (Exception e) {
275+
entry.vulnFetchFailed = true;
276276
return List.<OsvClient.Vulnerability>of();
277277
}
278278
},
@@ -477,7 +477,11 @@ private void rebuildByLicenseRows() {
477477
private void updateStatus() {
478478
if (licensesLoaded >= entries.size() && vulnsLoaded >= entries.size()) {
479479
long withLicense = entries.stream().filter(e -> e.license != null).count();
480+
long failedVulns = entries.stream().filter(e -> e.vulnFetchFailed).count();
480481
status = withLicense + "/" + entries.size() + " with license info, " + vulnCount + " vulnerabilities found";
482+
if (failedVulns > 0) {
483+
status += " (" + failedVulns + " failed to check)";
484+
}
481485
} else {
482486
status = "Loading… licenses: " + licensesLoaded + "/" + entries.size() + ", vulnerabilities: " + vulnsLoaded
483487
+ "/" + entries.size();
@@ -643,22 +647,7 @@ boolean handleEvent(Event event, TuiRunner runner) {
643647
}
644648

645649
// Save prompt mode
646-
if (pendingQuit) {
647-
if (key.isCharIgnoreCase('y')) {
648-
saveAndQuit();
649-
return true;
650-
}
651-
if (key.isCharIgnoreCase('n')) {
652-
runner.quit();
653-
return true;
654-
}
655-
if (key.isKey(KeyCode.ESCAPE)) {
656-
pendingQuit = false;
657-
updateStatus();
658-
return true;
659-
}
660-
return false;
661-
}
650+
if (handlePendingQuit(key)) return true;
662651

663652
// Diff overlay in standalone
664653
if (diffOverlay.isActive()) {
@@ -795,33 +784,14 @@ private void manageGroup(VulnGroup group) {
795784
}
796785
}
797786

798-
private void requestQuit() {
799-
if (isDirty()) {
800-
pendingQuit = true;
801-
status = "Save changes to POM? (y/n/Esc)";
802-
} else {
803-
runner.quit();
804-
}
805-
}
806-
807-
private void saveAndQuit() {
808-
PomEditSession.SaveResult result = editSession.save();
809-
if (result.success()) {
810-
runner.quit();
811-
} else {
812-
pendingQuit = false;
813-
status = result.message();
814-
}
787+
@Override
788+
protected void onStatusChange(String message) {
789+
this.status = message;
815790
}
816791

817-
private void toggleDiffView() {
818-
var diffs = collectAllDiffs();
819-
if (diffs.isEmpty()) {
820-
status = "No changes to show";
821-
return;
822-
}
823-
long changes = diffOverlay.openMulti(diffs);
824-
status = changes == 0 ? "No changes to show" : changes + " line(s) changed across " + diffs.size() + " file(s)";
792+
@Override
793+
protected void onPendingQuitCancelled() {
794+
updateStatus();
825795
}
826796

827797
@Override
@@ -878,7 +848,7 @@ void renderStandalone(Frame frame) {
878848
}
879849
}
880850

881-
renderInfoBar(frame, zones.get(2));
851+
renderStandaloneInfoBar(frame, zones.get(2));
882852
}
883853

884854
private void renderHeader(Frame frame, Rect area) {
@@ -1818,53 +1788,28 @@ m Add selected dep (or all in group) to dependencyManagement
18181788
return sections;
18191789
}
18201790

1821-
private void renderInfoBar(Frame frame, Rect area) {
1822-
var rows = Layout.vertical()
1823-
.constraints(Constraint.length(1), Constraint.length(1), Constraint.length(1))
1824-
.split(area);
1825-
1826-
List<Span> statusSpans = new ArrayList<>();
1827-
statusSpans.add(Span.raw(" " + status).fg(theme.standaloneStatusColor()));
1828-
frame.renderWidget(Paragraph.from(Line.from(statusSpans)), rows.get(1));
1829-
1791+
@Override
1792+
protected List<Span> standaloneKeyHints() {
18301793
List<Span> spans = new ArrayList<>();
1831-
spans.add(Span.raw(" "));
1832-
if (pendingQuit) {
1833-
spans.add(Span.raw("y").bold());
1834-
spans.add(Span.raw(":Save and quit "));
1835-
spans.add(Span.raw("n").bold());
1836-
spans.add(Span.raw(":Discard and quit "));
1837-
spans.add(Span.raw("Esc").bold());
1838-
spans.add(Span.raw(":Cancel"));
1839-
} else if (diffOverlay.isActive()) {
1840-
spans.add(Span.raw("↑↓").bold());
1841-
spans.add(Span.raw(":Scroll "));
1842-
spans.add(Span.raw("Esc").bold());
1843-
spans.add(Span.raw(":Close "));
1844-
spans.add(Span.raw("q").bold());
1845-
spans.add(Span.raw(":Quit"));
1846-
} else {
1847-
spans.add(Span.raw("↑↓").bold());
1848-
spans.add(Span.raw(":Navigate "));
1849-
spans.add(Span.raw("Tab").bold());
1850-
spans.add(Span.raw(":Licenses/Vulns "));
1851-
spans.add(Span.raw("s").bold());
1852-
spans.add(Span.raw(":Scope" + (scopeFilter != null ? "=" + scopeFilter : "") + " "));
1853-
if (view == View.LICENSES) {
1854-
spans.add(Span.raw("g").bold());
1855-
spans.add(Span.raw(":Group "));
1856-
}
1857-
spans.add(Span.raw("m").bold());
1858-
spans.add(Span.raw(":Manage dep "));
1859-
spans.add(Span.raw("d").bold());
1860-
spans.add(Span.raw(":Diff "));
1861-
spans.add(Span.raw("h").bold());
1862-
spans.add(Span.raw(":Help "));
1863-
spans.add(Span.raw("q").bold());
1864-
spans.add(Span.raw(":Quit"));
1865-
}
1866-
1867-
frame.renderWidget(Paragraph.from(Line.from(spans)), rows.get(2));
1794+
spans.add(Span.raw("↑↓").bold());
1795+
spans.add(Span.raw(":Navigate "));
1796+
spans.add(Span.raw("Tab").bold());
1797+
spans.add(Span.raw(":Licenses/Vulns "));
1798+
spans.add(Span.raw("s").bold());
1799+
spans.add(Span.raw(":Scope" + (scopeFilter != null ? "=" + scopeFilter : "") + " "));
1800+
if (view == View.LICENSES) {
1801+
spans.add(Span.raw("g").bold());
1802+
spans.add(Span.raw(":Group "));
1803+
}
1804+
spans.add(Span.raw("m").bold());
1805+
spans.add(Span.raw(":Manage dep "));
1806+
spans.add(Span.raw("d").bold());
1807+
spans.add(Span.raw(":Diff "));
1808+
spans.add(Span.raw("h").bold());
1809+
spans.add(Span.raw(":Help "));
1810+
spans.add(Span.raw("q").bold());
1811+
spans.add(Span.raw(":Quit"));
1812+
return spans;
18681813
}
18691814

18701815
// ── ToolPanel interface ─────────────────────────────────────────────────

‎pilot-core/src/main/java/eu/maveniverse/maven/pilot/ClassFileScanner.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,9 @@ public static ScanResult scanDirectory(Path classesDir) throws IOException {
7171
try (Stream<Path> walk = Files.walk(classesDir)) {
7272
walk.filter(p -> p.toString().endsWith(".class")).forEach(p -> {
7373
try {
74-
result.referencedClasses.addAll(scanFile(p).referencedClasses);
75-
for (var entry : scanFile(p).memberReferences.entrySet()) {
74+
ScanResult fileResult = scanFile(p);
75+
result.referencedClasses.addAll(fileResult.referencedClasses);
76+
for (var entry : fileResult.memberReferences.entrySet()) {
7677
result.memberReferences
7778
.computeIfAbsent(entry.getKey(), k -> new HashSet<>())
7879
.addAll(entry.getValue());

0 commit comments

Comments
 (0)