gnodet commented on PR #21534:
URL: https://github.com/apache/camel/pull/21534#issuecomment-4065431988
## Code Review
Great feature — visualizing Camel routes directly from `camel` CLI is very
useful. Here's detailed feedback on the implementation:
### Dead code
`DiagramPage.java` has several unused/empty methods:
- `selectRoutesFolder()` — private, never called
- `waitForDiagramAssets()` — private, never called
- `prepareDiagramForScreenshot()` — called from two places but has an empty
body
These should be removed or connected to the call paths where they're
intended.
### `LauncherHelper.normalizeJarPath()` returns `null` — potential NPE
The new `normalizeJarPath()` returns `null` when the path doesn't contain
`.jar`. Callers at lines 68 and 162 in `getLauncherJarPath()` previously
returned a valid path string but now may return `null` if running from an
exploded classpath directory. This is a behavioral regression that could cause
NPEs downstream.
### Duplicated Jolokia logic
`JolokiaAttacher` directly uses `com.sun.tools.attach.VirtualMachine` and
reimplements PID-finding logic. The existing `Jolokia` command in
`camel-jbang-core` already handles this with the proper Jolokia client
abstractions and `ProcessBaseCommand.findPids()`. Consider reusing the existing
code rather than duplicating it. If the plugin module can't depend on
`ProcessBaseCommand`, consider extracting the shared logic into a utility.
### Minimal test coverage
The 5 tests only cover basic parameter binding. There are no tests for:
- `LauncherHelper.normalizeJarPath()` — a utility with several branches now
used in critical paths
- `LauncherHelper.findJolokiaAgentJar()` — filesystem scanning
- `JolokiaAttacher` — PID resolution, port finding, agent loading
- `DiagramPngExporter` — the core export workflow
- `PluginHelper.scanLauncherForPlugins()` — nested JAR scanning
At minimum, `normalizeJarPath()` needs unit tests since it's in shared core
code affecting all JBang commands.
### Playwright is a heavyweight dependency
Playwright (~50MB+ with browser binaries) is a compile-time dependency of
the entire plugin module but only needed for `--output` PNG export. When
bundled into the launcher fat JAR, it increases download size for all users.
Consider making it a runtime dependency downloaded on demand (similar to how
Hawtio downloads its dependencies via `MavenDependencyDownloader`).
### Minor issues
- **`arity = "0..9"`** limits file arguments to 9 — other JBang commands use
`"0..*"` for unlimited files. Is this intentional?
- **`new Random().nextLong()`** for log file naming can produce negative
numbers and has a NOSONAR suppression. `Files.createTempFile("diagram-run",
".log")` would be cleaner.
- **`--openUrl`** uses camelCase while all other options use kebab-case
(`--keep-running`, `--route-id`). Consider adding a kebab-case alias
`--open-url`.
- **HTTP 4xx treated as success** in `waitForEndpoint()` — `code >= 200 &&
code < 500 && code != 404` accepts 401/403/405 as "endpoint available." Should
be documented.
- **`URLClassLoader` leak** in `PluginHelper.scanLauncherForPlugins()` — the
comment explains the rationale but consider storing loaders for shutdown
cleanup.
- **JS code tightly coupled to Hawtio internals** — `diagram-scripts.js`
uses PatternFly v5 CSS classes and React Flow internals. Any Hawtio UI update
could break this silently. Consider documenting which Hawtio version this was
tested against.
### Summary
| Priority | Issue |
|----------|-------|
| High | Dead code (unused/empty methods in DiagramPage) |
| High | `normalizeJarPath` returns null — behavioral regression |
| High | Duplicated Jolokia attach logic |
| High | Minimal test coverage for shared code changes |
| Medium | Playwright heavyweight compile dependency |
| Medium | File arity limited to 9 |
| Low | Random filenames, option naming, HTTP 4xx handling, classloader leak
|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
