ammachado commented on PR #25514: URL: https://github.com/apache/camel/pull/25514#issuecomment-5309957073
## Review feedback addressed in 29bb3178a7d9 Thanks for the review. All four actionable items are fixed; replies are inline on each thread, summarized here. | # | Finding | Resolution | |---|---------|------------| | 1 | `TuiCommand` missing `--record-size` / `--record-fps` / `--record-duration` | Fixed. Options declared and forwarded; `doCall()` split so `buildArgs()` is testable. | | 2 | `--web` + `--record` pollutes browser sessions | Fixed by rejecting the combination, plus doc and upgrade guide entries. | | 3 | `tamboui.record*` never cleared | Fixed. Six keys in one `RECORD_PROPERTIES` constant, cleared in `finally` by the session that set them. | | 4 | Test cleanup completeness | Fixed with junit-pioneer `@ClearSystemProperty` on both recording test classes. | | 5 | Add a `TuiCommand` forwarding test | Added `TuiCommandRecordOptionsTest`. | On **#2**, I took the "reject and document" option rather than skipping `applyRecording()` for web backends. Skipping would leave the local session recording while browser sessions ran unrecorded in the same process, which is confusing rather than useful. Rejecting up front also means the properties are never set at all when `--web` is in play, so no other code path has to defend against them. This is the one user-visible behavior change in the PR, so it now has an entry in `camel-4x-upgrade-guide-4_23.adoc`. 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. `RecordingConfig` caches the loaded config in a static field and the shutdown hook writes the cast file from 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 they cannot drift. Module suite: 1001 tests, 0 failures. The PR description has been updated to match the current changeset. _Claude Code on behalf of @ammachado_ -- 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]
