CAMEL-24397: camel-tui - Fix --record producing no cast file - #25514
CAMEL-24397: camel-tui - Fix --record producing no cast file#25514ammachado wants to merge 2 commits into
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 3 tested, 1 compile-only — current: 2 all testedMaveniverse Scalpel detected 4 affected modules (current approach: 2).
|
atiaomar1978-hub
left a comment
atiaomar1978-hub
left a comment
There was a problem hiding this comment.
Review summary
AI-generated review on behalf of atiaomar1978-hub
Solid fix for a subtle regression: the root-cause analysis is correct, TuiBackendHelper.applyRecording() is the right place to restore recording when an explicit JLineBackend is supplied, and the new tests would catch a re-break of --record. CI is green on the PR.
Two issues should be addressed before merge — one is a functional gap on the primary CLI entry point, the other affects --web combined with --record.
Must fix
-
TuiCommanddoes not forward the new recording options — the user manual documentscamel tui --record=demo.tape --record-size=160x44, butTuiCommandonly forwards--record. Users hitting the documented path get an unknown-option error or silently fall back to defaults. -
--web+--recordpollutes browser sessions —tamboui.record*system properties are process-wide; web sessions created byTuiWebServerstill callapplyRecording()and inherit headless recording even though theirCamelMonitorinstance has no tape configured.
Suggestions (non-blocking)
-
Clear
tamboui.record.fps/tamboui.record.durationin test@AfterEachalongside the other recording properties. -
Consider an integration-style test that exercises
TuiCommandargument forwarding (or a smallCommandLineparse test) so the CLI wiring cannot drift again. -
Dependency on
dev.tamboui.internal.recordis acceptable short-term given the upstream follow-up noted in the PR; please link the TamboUI PR in a follow-up comment when it exists.
Review performed with code inspection, Bugbot, and local test intent verification.
Note: This review covers project rules, conventions, and code patterns. It does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
| System.clearProperty("tamboui.record"); | ||
| System.clearProperty("tamboui.record.config"); | ||
| System.clearProperty("tamboui.record.width"); | ||
| System.clearProperty("tamboui.record.height"); |
There was a problem hiding this comment.
Suggestion — test cleanup completeness
RecordingConfig.load() reads tamboui.record.fps and tamboui.record.duration as well. Consider clearing those properties in @AfterEach alongside width/height so a future test ordering change cannot leak recording config into unrelated tests.
There was a problem hiding this comment.
Fixed in 29bb317, using junit-pioneer as suggested by @ammachado rather than extending the manual @AfterEach.
The class is annotated with @ClearSystemProperty for all six keys, which clears them before each test and restores the original values afterwards. That covers values the test body sets itself (the paths depend on @TempDir, so @SetSystemProperty was not an option) and it cannot drift out of sync the way a hand-written teardown does. junit-pioneer 2.3.0 is already managed in parent/pom.xml and used by camel-jbang-core, so this is not a new dependency for the project. The same annotations were applied to CamelMonitorRecordOptionsTest, which now also touches those properties.
The @AfterEach remains, reduced to uninstalling the AnsiTerminalCapture that RecordingConfig.load() installs.
AI-generated on behalf of @ammachado
| System.setProperty("tamboui.record.width", String.valueOf(size[0])); | ||
| System.setProperty("tamboui.record.height", String.valueOf(size[1])); | ||
| System.setProperty("tamboui.record.duration", String.valueOf(recordDuration)); | ||
| System.setProperty("tamboui.record.fps", String.valueOf(recordFps)); |
There was a problem hiding this comment.
Medium — recording system properties are never cleared
These six tamboui.record* properties are set at session start but the finally block (~679) tears down shell/MCP/web without clearing them. With the new applyRecording() path, any later TUI backend creation in the same JVM still sees recording enabled via RecordingConfig.isEnabled().
Consider clearing all tamboui.record* keys in finally (mirror the test @AfterEach cleanup), especially important for --web where multiple sessions share one process.
There was a problem hiding this comment.
Fixed in 29bb317. The six keys now live in a single CamelMonitor.RECORD_PROPERTIES constant, and the finally block clears them when this session is the one that set them (record != null).
Worth noting for the record: clearing the properties does not lose the recording. RecordingConfig caches the loaded config in a static field and writes the cast file from a shutdown hook using that copy, so clearing only affects RecordingConfig.isEnabled() and therefore whether a later backend gets wrapped.
CamelMonitorRecordOptionsTest.configuringRecordingSetsEveryPropertyThatIsClearedAgainAfterwards pins the set list and the clear list together, so a property added to configureRecording() but forgotten in the constant fails the build.
AI-generated on behalf of @ammachado
Follow-up — Bugbot + manual cross-checkAI-generated on behalf of atiaomar1978-hub — review only, no code changes from reviewer. Bugbot independently flagged the same two high items already called out inline:
What looks good
VerdictRequest changes until Review performed with code inspection and Bugbot. Does not replace specialized tools (CodeRabbit, Sourcery, SonarCloud). |
Review feedback addressed in 29bb317Thanks for the review. All four actionable items are fixed; replies are inline on each thread, summarized here.
On #2, I took the "reject and document" option rather than skipping On #6 (link the TamboUI PR): that upstream change is not open yet, so there is nothing to link today. I will post the link on this thread once it exists. The Camel-side wrap is written to be idempotent with it, so the two compose safely and Camel can drop its own copy when the TamboUI version pin moves. One clarification on #3 worth recording, since it affects how the fix is read: clearing the properties does not lose the recording. Module suite: 1001 tests, 0 failures. The PR description has been updated to match the current changeset. Claude Code on behalf of @ammachado |
Upstream follow-up link (review suggestion #5)The TamboUI change is now open: tamboui/tamboui#418 ("fix: apply recording to explicitly configured backends"). It does what the "Follow-up upstream" section of this PR describes:
So once the TamboUI version pin moves past that release, Claude Code on behalf of @ammachado |
atiaomar1978-hub
left a comment
atiaomar1978-hub
left a comment
There was a problem hiding this comment.
Re-review — all prior feedback addressed
AI-generated re-review on behalf of atiaomar1978-hub
Verified the second commit against the three issues from the earlier review. Bugbot reports clean — no remaining bugs found.
Prior issues — resolved
| Issue | Fix | Verified by |
|---|---|---|
TuiCommand missing --record-size / --record-fps / --record-duration |
Options declared on TuiCommand; buildArgs() forwards non-default values to CamelMonitor |
TuiCommandRecordOptionsTest |
--web + --record polluting browser sessions |
configureRecording() rejects the combination with ParameterException; documented in user manual + 4.23 upgrade guide |
CamelMonitorRecordOptionsTest.rejectsRecordingCombinedWithTheWebTerminal |
tamboui.record* never cleared |
RECORD_PROPERTIES constant; clearRecordingProperties() in finally when record != null |
CamelMonitorRecordOptionsTest.configuringRecordingSetsEveryPropertyThatIsClearedAgainAfterwards |
What looks good
TuiBackendHelper.applyRecording()remains the correct fix for the original silent-no-.castregression.@ClearSystemProperty/ junit-pioneer keeps test isolation tight.- Upgrade-guide entry for the new
--web+--recordrejection is appropriate. - CI green on Java 17 and 25.
Verdict
Approve — ready to merge from a functional and test-coverage standpoint.
Re-review performed with code inspection and Bugbot. Does not replace specialized tools (CodeRabbit, Sourcery, SonarCloud).
| * Every option declared here must be forwarded, otherwise the option is silently accepted and then ignored. Only | ||
| * non-default values are passed on, so the delegate keeps applying its own defaults. | ||
| */ | ||
| List<String> buildArgs() { |
There was a problem hiding this comment.
Resolved — buildArgs() now forwards all three recording tuning flags to CamelMonitor, and TuiCommandRecordOptionsTest pins the wiring so the documented camel tui --record-size=160x44 path keeps working. Nice refactor extracting buildArgs() for testability.
| if (record == null) { | ||
| return; | ||
| } | ||
| if (web) { |
There was a problem hiding this comment.
Resolved — rejecting --web + --record up front is the right call given JVM-wide tamboui.record* properties. Upgrade-guide entry documents the behaviour change for upgraders.
| } | ||
| deleteMcpJson(mcpJsonFile); | ||
| this.runner = null; | ||
| if (record != null) { |
There was a problem hiding this comment.
Resolved — clearRecordingProperties() in finally paired with the RECORD_PROPERTIES constant keeps set/clear in sync. The javadoc explaining why clearing is safe after RecordingConfig.load() is helpful.
Re-review complete ✅AI-generated on behalf of atiaomar1978-hub All three items from the earlier Request changes review are fixed in commit CI: all checks green (Java 17 + 25). Verdict: Approve — LGTM, ready to merge. Thanks @ammachado for the thorough follow-up — especially |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Solid fix for the --record regression. The root cause analysis is well-documented — commit 36f9299d132 introduced an explicit JLineBackend that bypassed BackendFactory.create(), and this PR correctly wraps the backend at the TuiBackendHelper level instead.
Key strengths:
- Good use of
RecordingConfig.isEnabled()as a guard beforeRecordingConfig.load()to avoid side effects when recording is not configured - Property set/clear lifecycle is well-designed with fail-fast validation and
finally-block cleanup - Test pinning (
configuringRecordingSetsEveryPropertyThatIsClearedAgainAfterwards) ensures the set/clear property lists cannot drift - Good decision to reject
--web+--recordupfront rather than silently skipping recording - Upgrade guide entry documents the only user-visible behavior change
- Tests follow all project conventions (package-private visibility, AssertJ assertions, junit-pioneer
@ClearSystemProperty, noThread.sleep())
Minor note: the use of dev.tamboui.internal.record.* API is acknowledged in the PR with a reference to the upstream follow-up (tamboui/tamboui#418) that will make it public.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Solid fix for the silent recording regression in camel-tui. The root cause — explicit JLineBackend bypassing BackendFactory.create() where TamboUI normally applies recording — is correctly identified and fixed at the TuiBackendHelper level.
Strengths:
- Root cause analysis well-supported by git history (commit 36f9299 introduced the bypass)
- Recording options properly tested with three new test classes covering option parsing, forwarding, and backend wrapping
--web+--recordrejection is well-reasoned (process-wide system properties), documented, and has an upgrade guide entry- System property lifecycle (
configureRecording/clearRecordingProperties) is well-designed with theRECORD_PROPERTIESconstant keeping set/clear in sync - Follows all project conventions: package-private test classes/methods, AssertJ assertions, ASF license headers, junit-pioneer for system property isolation
Minor observations (non-blocking):
- The imports of
dev.tamboui.internal.record.RecordingBackendandRecordingConfigintroduce a dependency on TamboUI's internal API. The PR description acknowledges this and references upstream PR tamboui/tamboui#418 for a public alternative. The usage is well-isolated toapplyRecording()making future migration straightforward. A follow-up JIRA linking to the upstream PR could help track the cleanup.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
TamboUI applies recording in exactly one place, BackendFactory.create(), which calls RecordingConfig.load() (installing the System.out capture and the shutdown hook that writes the cast) and wraps the backend in RecordingBackend (which loads the tape via InteractionPlayer). TuiRunner.create() only calls that factory when no explicit backend is configured. TuiBackendHelper always supplies an explicit JLineBackend, so recording was never engaged: --record replayed no tape, wrote no .cast file, and still exited cleanly. The explicit backend is deliberate and has to stay, because ServiceLoader auto-discovery can otherwise pick the Aesh backend that --web puts on the classpath. So apply the recording wrapper ourselves instead of reverting to auto-discovery. Also replace the hardcoded 200x50 recording geometry with --record-size, --record-fps and --record-duration. 200 columns is too wide to embed in a documentation page. Defaults are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Forward --record-size, --record-fps and --record-duration from TuiCommand to CamelMonitor. TuiCommand is the entry point registered by the TUI plugin, so the options documented in the user manual were rejected as unknown options on the primary CLI path. Reject --record combined with --web. The tamboui.record* system properties are process-wide, so every browser session spawned by TuiWebServer would be wrapped for recording and write to the same cast file. Clear the tamboui.record* properties when the session that set them ends, so a TUI backend created later in the same JVM is not wrapped for recording again. Manage the recording system properties in tests with junit-pioneer, so every key RecordingConfig reads is restored, including fps and duration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
29bb317 to
bcda723
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
LGTM — re-review after new commits. The second commit comprehensively addresses all prior review feedback:
--web+--recordconflict: Now rejected upfront with aParameterException, documented in both TUI docs and the 4.23 upgrade guide- System property cleanup: Recording system properties are cleared in the
finallyblock viaclearRecordingProperties(), preventing JVM-wide leakage - CLI option forwarding:
--record-size,--record-fps, and--record-durationare now declared onTuiCommandand forwarded toCamelMonitor, fixing the documented CLI path - Test isolation: Uses junit-pioneer
@ClearSystemPropertyinstead of manual@AfterEachteardown
The RECORD_PROPERTIES constant paired with a test verifying every property set is also cleared is a nice defensive pattern. Four new tests pin the new behavior. CI is green. ✅
AI-generated review — Claude Code on behalf of @gnodet
This review does not replace specialized AI review tools or static analysis.
3 participants
Description
camel tui --record=<tape>exits cleanly but neither replays the tape nor writes the.castfile. Nothing is logged, so the failure is indistinguishable from success. This is the path documented as "Scripted Recording" incamel-jbang-tui.adoc.Root cause
CamelMonitorsets thetamboui.record*system properties correctly, but nothing ever reads them.TamboUI applies recording in exactly one place,
BackendFactory.create(), which callsRecordingConfig.load()(installing theSystem.outcapture and the shutdown hook that writes the cast) and wraps the backend inRecordingBackend(which loads the tape viaInteractionPlayer).TuiRunner.create()only calls that factory when no explicit backend is configured:TuiBackendHelperalways supplies an explicitJLineBackend, soBackendFactory.create()never runs. That single cause explains both symptoms at once (no replay and no cast) plus the clean exit.Regression
--record, using a bareTuiRunner.create(). Worked.The explicit backend is deliberate and has to stay. As the comment in
TuiBackendHelperrecords, withtamboui-aesh-backendon the classpath for--web, ServiceLoader auto-discovery can selectAeshBackendfor the local session, which drives a nativePosixSysTerminalthat does not shut down cleanly. Reverting to auto-discovery would restore recording and reintroduce that bug, so this change applies the recording wrapper explicitly instead.Recording geometry
The recording size was hardcoded at 200x50, along with fps 10 and duration 120000. 200 columns is too wide to embed in a documentation page (roughly 5px per character in an 800px content column). This adds
--record-size,--record-fpsand--record-duration, declared onCamelMonitorand forwarded byTuiCommand, which is the entry point the TUI plugin actually registers. Defaults are unchanged, so no existing command line behaves differently.Recording is process-wide
TamboUI accepts a recording configuration only through the
tamboui.record*system properties, so "is this session recording" is a JVM-global question. Two consequences are handled explicitly:--recordis now rejected together with--web. Every browser session served byTuiWebServerbuilds its ownCamelMonitorand goes through the sameTuiBackendHelper.applyRecording()path, so it would inherit the local session's recording and write into the same cast file. The two modes are conceptually exclusive anyway, since--recorddrives a headless TUI from a tape rather than from a connected terminal. This is the one user-visible behavior change and it has an upgrade guide entry: the combination used to be accepted while silently recording nothing.RecordingConfigkeeps its own copy and the shutdown hook still writes the cast file, so nothing is lost.Follow-up upstream
dev.tamboui.internal.recordis documented as internal API. The companion change is open upstream as tamboui/tamboui#418: it adds a publicBackendFactory.applyRecording(Backend)and makesTuiRunnerwrap explicitly configured backends, fixing this at the source for every downstream application. It is idempotent, so this Camel-side wrap and the upstream one compose safely, andTuiBackendHelper.applyRecording()can be deleted once the TamboUI version pin moves past that release.Target
Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally and I have committed all auto-generated changes.Verification
mvn test -pl dsl/camel-jbang/camel-jbang-plugin-tui: 1001 tests, 0 failures, across three test classes touched or added here (TuiBackendHelperRecordingTest,CamelMonitorRecordOptionsTest,TuiCommandRecordOptionsTest).camel tui --record=demo.tape --record-size=120x30against the built module in a non-tty environment. It printedRecording saved to: .../demo.cast/Frames captured: 21and produced a valid asciicast v2 file whose header reads"width": 120, "height": 30, confirming both the fix and the new sizing option.mvnd clean install -Dquicklyfrom the repository root: BUILD SUCCESS, with a clean working tree afterwards (no uncommitted generated files).Recording system properties are managed in tests with junit-pioneer's
@ClearSystemProperty(version already managed inparent/pom.xmland used bycamel-jbang-core), so every keyRecordingConfigreads is restored after each test rather than only the four that a hand-written teardown happened to list.Claude Code on behalf of @ammachado
🤖 Generated with Claude Code