vignesh-manel commented on PR #21534:
URL: https://github.com/apache/camel/pull/21534#issuecomment-4111282668
> ## 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
@gnodet could you please re-review, I have removed the dead code. Changed
playwright to runtime dependency which will be downloaded. Added test for
LauncherHelper, addressed the minor issues, however duplicate changes in
JolokiaAttacher was not addressed to avoid any impact on existing jolokia
functionality
--
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]
