fix: create global entrypoint for tui#1365
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh release download pr-1365-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.15.0.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for unifying the TUI entrypoint — the RenderTUIOptions shape and InitialRoute typing are nice. I have a few concerns that I think need to be addressed before this merges.
Summary of issues
-
Telemetry regression for
invokeTUI: The PR removes thewithCommandRunTelemetry('invoke', …)wrapper that previously wrapped the TUI invoke flow. No replacement was added, socli.command_run(withhas_stream/has_session_id/auth_type/agent_protocol/duration/exit_reason) is no longer emitted foragentcore invokeinteractive mode. See inline comment. -
Behavior change: TUI flows no longer auto-exit on success when launched from CLI commands. Previously
agentcore add,agentcore remove, etc. rendered their flows withisInteractive={false}, which makes screens likeAddFlow,RemoveAllScreen,RemoveFlow,AddSuccessScreen,AddMemoryFlow,AddEvaluatorFlow,AddOnlineEvalFlow,AddIdentityFlowauto-exit after success (see e.g.AddFlow.tsx:206,RemoveAllScreen.tsx:30). Going throughAppnow means they always receiveisInteractive={true}, so the user has to press escape to leave after every successful run. See inline comment. -
Double
TelemetryClientAccessor.init()for command-routed paths. Whenagentcore add(or any of the migrated commands) runs,main()callsTelemetryClientAccessor.init(args[0], 'cli')(cli.ts:271) and then the action handler callsrenderTUI()which callsTelemetryClientAccessor.init(initialRoute.name, 'tui')(cli.ts:127). The first client (mode=cli) is overwritten in the singleton and nevershutdown()'d. See inline comment. -
Circular import.
src/cli/cli.tsimportsregisterInvoke/registerAdd/etc. from./commands/*, and those command files now importrenderTUIfrom'../../cli'. It works today becauserenderTUIis only referenced inside async action handlers, but it's fragile. See inline comment.
Items 1 and 3 are the most important — 1 is a clear telemetry regression and 3 partially undoes the telemetry-mode fix this PR is trying to land.
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for the cleanup — the unified renderTUI() entrypoint is a nice improvement and the recent commits address most of the earlier review feedback (double init, isInteractive threading, render-tui module extraction).
Flagging two new issues below; both involve information being silently dropped on the way through renderTUI. The other pre-existing review comments still appear open but their underlying concerns look addressed in the latest commits — happy to leave those for the original reviewer to confirm.
9456078 to
bce4cf9
Compare
|
Claude Security Review: no high-confidence findings. (run) |
bce4cf9 to
8d0f3b3
Compare
| const project = await configIO.readProjectSpec().catch(() => undefined); | ||
| const firstProtocol = project?.runtimes?.[0]?.protocol ?? 'unknown'; | ||
|
|
||
| const result = await withCommandRunTelemetry( |
There was a problem hiding this comment.
When i tested this PR locally, i noticed that the value in the emitted telemetry was always 1 (likely how long it took to load the config).
Before this change, the value was the actual duration of my entire invoke session.
There was a problem hiding this comment.
Discussed in person, this is to be consistent with dev, and to avoid tying the duration to arbitrary user sessions.
| process.stdout.write(SHOW_CURSOR); | ||
| } | ||
|
|
||
| await TelemetryClientAccessor.shutdown(); |
There was a problem hiding this comment.
wouldnt shutting down TelemetryClientAccessor here make it so if telemetry is emitted during a post-exit action (i.e. launchBrowserDev) then it would stop working?
There was a problem hiding this comment.
Yes, good call, this is important for when we add telemetry for the browser actions. I've swapped it to flush before the blocking process, then the shutdown should be handled by the global one in the finally block, if it reaches there. Shutdown is "best-effort" so its fine if it doesn't.
7f6cf1a to
9bad38d
Compare
|
Claude Security Review: no high-confidence findings. (run) |
| } | ||
| enterAltScreen: false, | ||
| actionOnBack: 'exit', | ||
| isInteractive: false, |
There was a problem hiding this comment.
why was isInteractive switched to false here?
There was a problem hiding this comment.
good question. I made this change thinking that isInteractive=true meant exit after first success (as it does for other commands), and I didn't want the tui to exit early. However, turns out its just dead code:
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
# Conflicts: # src/cli/cli.ts # src/cli/commands/invoke/command.tsx # src/cli/tui/screens/invoke/useInvokeFlow.ts
|
Claude Security Review: no high-confidence findings. (run) |
Description
Previously, commands like
agentcore add,deploy, etc. rendered TUI screens inline via directrender()calls. This made it difficult to distinguish CLI vs TUI code paths, caused telemetry to be mislabeled asmode="cli"for interactive flows, and meant bareagentcorenever initialized or shut down the telemetry client.This PR creates a unified
renderTUI()entrypoint that owns the telemetry lifecycle and replaces all inlinerender()calls. As part of this:src/cli/tui/render.tsand shared notices intosrc/cli/notices.tsto break circular importsadd,deploy,create,remove,invoketo userenderTUI()TelemetryClientAccessor.init()handle re-initialization safelydiffMode,isInteractive,actionOnBack)This refactor fixes #895.
Type of Change
Testing
basic-flows.mov
Manually tested flows:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.