feat(api): add global UI events#1105
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a global UI event service with lifecycle management, API methods, notifications, host rendering, launch-guard integration, installer notices/loaders, and playlist picker interactions. ChangesGlobal UI events
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/zapscript/playlist.go (1)
605-628: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlaylist queue update happens before the UI/picker failure paths — successful side effect gets reported as a failure.
queuePlaylistUpdateruns at line 605 and mutates state (playlist queued) before theenv.UI == nilcheck (609) andenv.UI.Openerror handling (623). Both failure branches return a non-nil error alongside a populatedCmdResult{PlaylistChanged: true, Playlist: pls}— butRunCommanddiscards the entireCmdResultwhenevererr != nil, so callers never see that the playlist was actually queued. The net effect: wheneverenv.UIis nil (a legitimate state — seeRunControlScriptincontrol.go, which passesnilforui) orOpenfails,playlist.openreports total failure even though the playlist already started changing.Consider treating a missing/failed UI as non-fatal (log a warning, skip the picker, still return success) since the substantive playlist-open action already succeeded — or at minimum, perform the nil/Open checks before
queuePlaylistUpdateso no side effect fires before you know whether the command can fully proceed.🔧 Suggested fix (non-fatal picker failure)
- log.Info().Any("items", playlistItemsForLog(pls.Items)).Msgf("open playlist: %s", env.Cmd.Args) - if err := queuePlaylistUpdate(&env, pls); err != nil { - return platforms.CmdResult{}, err - } - - if env.UI == nil { - return platforms.CmdResult{ - PlaylistChanged: true, - Playlist: pls, - }, errors.New("UI event service is unavailable") - } - handle, openErr := env.UI.Open(env.ServiceCtx, &uievents.Request{ + log.Info().Any("items", playlistItemsForLog(pls.Items)).Msgf("open playlist: %s", env.Cmd.Args) + if err := queuePlaylistUpdate(&env, pls); err != nil { + return platforms.CmdResult{}, err + } + + result := platforms.CmdResult{PlaylistChanged: true, Playlist: pls} + if env.UI == nil { + log.Warn().Msg("UI event service unavailable, skipping playlist picker") + return result, nil + } + handle, openErr := env.UI.Open(env.ServiceCtx, &uievents.Request{ Kind: apimodels.UIEventKindPicker, Title: pls.Name, Choices: choices, SelectedChoice: pls.Index, Timeout: 30 * time.Second, Dismissible: true, }) if openErr != nil { - return platforms.CmdResult{ - PlaylistChanged: true, - Playlist: pls, - }, fmt.Errorf("failed to open picker: %w", openErr) + log.Warn().Err(openErr).Msg("failed to open playlist picker") + return result, nil } go runPickerResult(env.Cfg, handle, client.LocalClient) - return platforms.CmdResult{ - PlaylistChanged: true, - Playlist: pls, - }, nil + return result, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/zapscript/playlist.go` around lines 605 - 628, Update the playlist.open flow around queuePlaylistUpdate, env.UI, and env.UI.Open so picker unavailability or failure is non-fatal after the playlist has been queued: log a warning, skip the picker, and return the successful CmdResult with PlaylistChanged and Playlist populated. Preserve error propagation for queuePlaylistUpdate failures and keep normal picker behavior when the UI opens successfully.
🧹 Nitpick comments (9)
pkg/ui/widgets/widgets.go (2)
166-184: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the IPC call in
sendUIResponse.
client.LocalClient(context.Background(), ...)has no deadline; if the local API server is unresponsive, the dismiss/select goroutine hangs indefinitely and the widget never receives its stop signal for that action.♻️ Bound the call with a timeout
func sendUIResponse( cfg *config.Instance, eventID string, action models.UIResponseAction, choiceID string, ) error { params, err := json.Marshal(models.UIRespondParams{ ID: eventID, Action: action, ChoiceID: choiceID, }) if err != nil { return fmt.Errorf("failed to marshal UI response: %w", err) } - if _, err = client.LocalClient(context.Background(), cfg, models.MethodUIRespond, string(params)); err != nil { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if _, err = client.LocalClient(ctx, cfg, models.MethodUIRespond, string(params)); err != nil { return fmt.Errorf("failed to send UI response: %w", err) } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/widgets/widgets.go` around lines 166 - 184, Update sendUIResponse to create a timeout-bound context and pass it to client.LocalClient instead of context.Background(). Ensure the context is cancelled after the call, while preserving the existing parameter marshaling and wrapped error handling.
166-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew logic (
sendUIResponse,watchCompletion, event-driven dismiss/select) has no accompanying tests.These are new, hardware-independent functions/branches that are straightforward to unit test (e.g. mocking
client.LocalClientor exercising the file-based completion watcher), but nowidgets_test.gochanges ship with this PR.As per coding guidelines, "Write tests for all new code — see TESTING.md and pkg/testing/README.md".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/widgets/widgets.go` around lines 166 - 399, The new behavior in sendUIResponse, watchCompletion, and the event-driven dismiss/select branches lacks unit coverage. Add widgets_test.go tests that mock or stub client.LocalClient to verify UI response payloads and error propagation, and exercise watchCompletion with a temporary completion file to verify removal and application shutdown; cover dismiss/select success and failure paths without requiring hardware.Source: Coding guidelines
pkg/platforms/mister/platform.go (1)
1440-1452: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
PresentUIignores itscontext.Contextduring the synchronous delay.The 3s
time.Sleepruns unconditionally onceneedsDelayis true, with no way to abort viactx.Done(). SinceOpen()callspresent()synchronously before returning theHandle, this blocks the caller (and, per the linkedOpengraph evidence, holdss.mureleased but the calling goroutine) for up to 3s with no cancellation path, e.g. on shutdown or when the event gets superseded immediately after.♻️ Make the delay cancellable
func (p *Platform) PresentUI( - _ context.Context, + ctx context.Context, event *models.UIEvent, ) (func() error, error) { const orphanTimeoutSeconds = int(time.Hour / time.Second) p.platformMu.Lock() needsDelay := time.Since(p.lastUIHidden) < 2*time.Second p.platformMu.Unlock() if needsDelay { log.Debug().Msg("waiting for previous UI event to finish") - time.Sleep(3 * time.Second) + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(3 * time.Second): + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/platforms/mister/platform.go` around lines 1440 - 1452, Update PresentUI’s needsDelay path to replace the unconditional 3-second time.Sleep with a context-aware wait that returns promptly when the provided context is canceled. Preserve the existing delay duration and continue normal UI presentation when the delay completes without cancellation.pkg/zapscript/commands.go (1)
397-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
RunCommandnow has 14 positional parameters, with 3 consecutive nilable ones.
waitForMediaReady,playbackManager, anduiare three adjacent parameters of different types (func / interface / pointer) that are all commonly passed asnil— visible already in this PR's benchmark call sites (nil, nil, nilsequences) and inRunControlScript. A future transposition of these three arguments compiles cleanly sincenilsatisfies all three types, so the compiler won't catch a misordering.Consider grouping the less-frequently-varying/optional dependencies (
waitForMediaReady,playbackManager,ui,lm) into a small options/deps struct to make call sites self-documenting and remove the silent-swap risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/zapscript/commands.go` around lines 397 - 412, Refactor RunCommand to accept a small options/dependencies struct grouping the optional dependencies waitForMediaReady, playbackManager, ui, and lm, rather than four positional parameters. Update all RunCommand call sites, including benchmark paths and RunControlScript, to populate the named fields and preserve existing behavior.pkg/zapscript/control.go (1)
109-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect param mapping, but the new
nilforuiis uncommented.Verified against
RunCommand's 14-parameter signature — the newly insertednilon line 126 correctly lands on theui *uievents.Serviceparameter, keeping control scripts without an event service. The other three positionalnils (waitForMediaReady,playbackManager,ui) are undocumented, unlikelmwhich has an explanatory comment. Given this is a growing positional-arg list, a stray future insertion is easy to miss.✏️ Suggested inline comment
nil, // lm not needed — control commands cannot launch media nil, nil, - nil, + nil, // ui not needed — control scripts have no interactive UI context &env,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/zapscript/control.go` around lines 109 - 128, Add an inline comment to the newly inserted nil argument in the RunCommand call within the control-script command loop, identifying it as the ui parameter and documenting that control commands do not use an event service. Leave the existing argument mapping and behavior unchanged.pkg/ui/events/service.go (2)
87-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
SelectedChoicezero value silently pre-selects the first choice.Zero value of
SelectedChoice(0) selects the first choice vianewActiveEvent'sif i == request.SelectedChoicecheck;-1is required for "no default." This isn't called out on the field itself, only inferable fromvalidateRequest's bounds check. A future caller that forgets to setSelectedChoice: -1for a picker will get an unintended default selection.📝 Proposed doc fix
type Request struct { Kind models.UIEventKind Title string Message string Choices []Choice + // SelectedChoice is the index of the pre-selected choice. The zero value + // (0) selects the first choice; use -1 for no default selection. SelectedChoice intAlso applies to: 569-582, 609-646
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/events/service.go` around lines 87 - 98, Document on Request.SelectedChoice that -1 means no choice is preselected and that zero selects the first choice, so callers must explicitly use -1 when no default is intended. Keep the existing newActiveEvent and validateRequest behavior unchanged.
283-321: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Update()doesn't check event expiry, unlikeRespond()/complete().Both
Respond()(Line 255) andcomplete()(Line 383) calls.eventExpired(entry)and reject/expire stale entries before mutating state.Update()skips this check, so a producer racing the timeout timer (e.g. readers.go's "re-tap during delay"guardUI.Update(...)) could extend/mutate an event that should already have timed out, effectively resurrecting it just beforeresolveTimeoutfires.♻️ Proposed fix
s.mu.Lock() entry, err := s.activeForIDLocked(id) if err != nil { s.mu.Unlock() return err } + if s.eventExpired(entry) { + state, resolution := s.expireLocked(entry) + s.mu.Unlock() + closeRenderer(entry) + s.publishState(state) + s.deliverResult(entry, Result{Resolution: resolution}) + return ErrEventExpired + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/events/service.go` around lines 283 - 321, Update the Service.Update method to call eventExpired(entry) after locating the active entry and before applying any fields or timer changes. Match the existing Respond and complete behavior by expiring or rejecting stale entries, unlocking the mutex appropriately, and only mutating non-expired events.pkg/platforms/shared/installer/installer.go (1)
82-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
time.Sleepignores context cancellation.
showPreNoticesleeps forhandle.MinimumDisplayunconditionally instead of racing againstctx.Done(). If the caller's context is cancelled during this window (e.g. app shutdown), the sleep still runs to completion beforeComplete()is called.♻️ Proposed fix
if handle.MinimumDisplay > 0 { log.Debug().Dur("delay", handle.MinimumDisplay).Msg("delaying pre-notice") - time.Sleep(handle.MinimumDisplay) + timer := time.NewTimer(handle.MinimumDisplay) + select { + case <-timer.C: + case <-ctx.Done(): + timer.Stop() + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/platforms/shared/installer/installer.go` around lines 82 - 110, Update showPreNotice so the minimum-display delay respects ctx cancellation instead of unconditionally calling time.Sleep. Wait for either handle.MinimumDisplay to elapse or ctx.Done(), then proceed to complete the notice while preserving the existing completion error handling.pkg/api/methods/ui_test.go (1)
72-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the result wait.
A regression that prevents resolution will hang this test until the suite-wide timeout, obscuring the failure.
Proposed fix
import ( "encoding/json" "testing" + "time" @@ - result := <-handle.Results - assert.Equal(t, models.UIOutcomeConfirmed, result.Resolution.Outcome) + select { + case result := <-handle.Results: + assert.Equal(t, models.UIOutcomeConfirmed, result.Resolution.Outcome) + case <-time.After(time.Second): + t.Fatal("timed out waiting for UI result") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/methods/ui_test.go` around lines 72 - 73, Update the result wait in the UI test around handle.Results to use a bounded timeout instead of blocking indefinitely. Assert that a result is received within the timeout, then verify its Resolution.Outcome remains models.UIOutcomeConfirmed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api/methods.md`:
- Around line 133-255: Update the revision description in the ui result table to
state that it is the monotonic revision of global UI state shared across
clients, not state scoped to the current connection. Preserve the instruction to
ignore older snapshots.
In `@pkg/api/notifications/notifications.go`:
- Line 40: Ensure models.NotificationUIChanged uses a latest-state/coalescing or
otherwise recoverable delivery path in sendNotification, rather than being
dropped when the source queue is full; preserve delivery so clients can observe
UI interactions under queue pressure, not merely the criticalNotifications
log-level classification.
In `@pkg/platforms/batocera/platform.go`:
- Around line 811-826: Add test cases for Platform.PresentUI covering successful
notice and loader events dispatched through esapi.APINotify, including
message/title fallback as applicable, plus picker and confirm events returning
platforms.ErrNotSupported. Follow the existing patterns in platform_test.go and
verify returned values and errors.
---
Outside diff comments:
In `@pkg/zapscript/playlist.go`:
- Around line 605-628: Update the playlist.open flow around queuePlaylistUpdate,
env.UI, and env.UI.Open so picker unavailability or failure is non-fatal after
the playlist has been queued: log a warning, skip the picker, and return the
successful CmdResult with PlaylistChanged and Playlist populated. Preserve error
propagation for queuePlaylistUpdate failures and keep normal picker behavior
when the UI opens successfully.
---
Nitpick comments:
In `@pkg/api/methods/ui_test.go`:
- Around line 72-73: Update the result wait in the UI test around handle.Results
to use a bounded timeout instead of blocking indefinitely. Assert that a result
is received within the timeout, then verify its Resolution.Outcome remains
models.UIOutcomeConfirmed.
In `@pkg/platforms/mister/platform.go`:
- Around line 1440-1452: Update PresentUI’s needsDelay path to replace the
unconditional 3-second time.Sleep with a context-aware wait that returns
promptly when the provided context is canceled. Preserve the existing delay
duration and continue normal UI presentation when the delay completes without
cancellation.
In `@pkg/platforms/shared/installer/installer.go`:
- Around line 82-110: Update showPreNotice so the minimum-display delay respects
ctx cancellation instead of unconditionally calling time.Sleep. Wait for either
handle.MinimumDisplay to elapse or ctx.Done(), then proceed to complete the
notice while preserving the existing completion error handling.
In `@pkg/ui/events/service.go`:
- Around line 87-98: Document on Request.SelectedChoice that -1 means no choice
is preselected and that zero selects the first choice, so callers must
explicitly use -1 when no default is intended. Keep the existing newActiveEvent
and validateRequest behavior unchanged.
- Around line 283-321: Update the Service.Update method to call
eventExpired(entry) after locating the active entry and before applying any
fields or timer changes. Match the existing Respond and complete behavior by
expiring or rejecting stale entries, unlocking the mutex appropriately, and only
mutating non-expired events.
In `@pkg/ui/widgets/widgets.go`:
- Around line 166-184: Update sendUIResponse to create a timeout-bound context
and pass it to client.LocalClient instead of context.Background(). Ensure the
context is cancelled after the call, while preserving the existing parameter
marshaling and wrapped error handling.
- Around line 166-399: The new behavior in sendUIResponse, watchCompletion, and
the event-driven dismiss/select branches lacks unit coverage. Add
widgets_test.go tests that mock or stub client.LocalClient to verify UI response
payloads and error propagation, and exercise watchCompletion with a temporary
completion file to verify removal and application shutdown; cover dismiss/select
success and failure paths without requiring hardware.
In `@pkg/zapscript/commands.go`:
- Around line 397-412: Refactor RunCommand to accept a small
options/dependencies struct grouping the optional dependencies
waitForMediaReady, playbackManager, ui, and lm, rather than four positional
parameters. Update all RunCommand call sites, including benchmark paths and
RunControlScript, to populate the named fields and preserve existing behavior.
In `@pkg/zapscript/control.go`:
- Around line 109-128: Add an inline comment to the newly inserted nil argument
in the RunCommand call within the control-script command loop, identifying it as
the ui parameter and documenting that control commands do not use an event
service. Leave the existing argument mapping and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2d1d5f9-e456-4549-b9c7-af05b0ffa7bb
📒 Files selected for processing (48)
cmd/mister/main.godocs/ARCHITECTURE.mddocs/api/index.mddocs/api/methods.mddocs/api/notifications.mdpkg/api/methods/ui.gopkg/api/methods/ui_test.gopkg/api/models/models.gopkg/api/models/params.gopkg/api/models/requests/requests.gopkg/api/models/responses.gopkg/api/notifications/notifications.gopkg/api/notifications/notifications_test.gopkg/api/server.gopkg/platforms/batocera/platform.gopkg/platforms/libreelec/platform.gopkg/platforms/mac/platform.gopkg/platforms/mister/platform.gopkg/platforms/mister/platform_test.gopkg/platforms/mister/ui.gopkg/platforms/mistex/platform.gopkg/platforms/platforms.gopkg/platforms/recalbox/platform.gopkg/platforms/replayos/platform.gopkg/platforms/replayos/platform_test.gopkg/platforms/retropie/platform.gopkg/platforms/shared/installer/installer.gopkg/platforms/shared/installer/installer_test.gopkg/platforms/shared/linuxbase/base.gopkg/platforms/shared/linuxbase/base_test.gopkg/platforms/windows/platform.gopkg/service/context.gopkg/service/queues.gopkg/service/reader_manager_test.gopkg/service/readers.gopkg/service/scan_to_launch_bench_test.gopkg/service/service.gopkg/service/state/state.gopkg/testing/mocks/platform.gopkg/ui/events/service.gopkg/ui/events/service_test.gopkg/ui/widgets/models/models.gopkg/ui/widgets/widgets.gopkg/zapscript/commands.gopkg/zapscript/control.gopkg/zapscript/launch.gopkg/zapscript/playlist.gopkg/zapscript/playlist_test.go
💤 Files with no reviewable changes (11)
- pkg/platforms/windows/platform.go
- pkg/platforms/replayos/platform_test.go
- pkg/platforms/libreelec/platform.go
- pkg/platforms/mistex/platform.go
- pkg/platforms/replayos/platform.go
- pkg/platforms/shared/linuxbase/base.go
- pkg/platforms/mac/platform.go
- pkg/platforms/recalbox/platform.go
- pkg/platforms/retropie/platform.go
- pkg/platforms/shared/linuxbase/base_test.go
- pkg/testing/mocks/platform.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ui/widgets/widgets_test.go`:
- Around line 41-48: Replace the host filesystem usage in writeWidgetArgs with
the existing injected mock filesystem fixture, and thread that filesystem
through the widget file-access paths. In pkg/ui/widgets/widgets_test.go:41-48,
use the fixture when writing arguments; in
pkg/ui/widgets/widgets_test.go:122-133, use the same fixture to create and
verify completion markers. Reuse existing mocks from pkg/testing/ rather than
creating new ones.
In `@pkg/zapscript/commands.go`:
- Around line 50-56: Add a RunCommand test covering the RunCommandOptions.UI
injection boundary. Configure opts.UI with a distinctive service instance,
execute a command that inspects platforms.CmdEnv, and assert it receives the
exact same UI pointer; keep existing command behavior and test patterns
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 373250ad-73c1-4f11-a9d3-622061cdfe2b
📒 Files selected for processing (25)
docs/api/methods.mddocs/api/notifications.mdpkg/api/methods/ui_test.gopkg/api/notifications/notifications.gopkg/api/notifications/notifications_test.gopkg/platforms/batocera/platform_test.gopkg/platforms/mister/platform.gopkg/platforms/mister/platform_test.gopkg/platforms/shared/installer/installer.gopkg/platforms/shared/installer/installer_test.gopkg/service/broker/broker.gopkg/service/broker/broker_test.gopkg/service/queues.gopkg/service/reader_manager_test.gopkg/service/scan_to_launch_bench_test.gopkg/service/service.gopkg/testing/helpers/esapi_server.gopkg/ui/events/service.gopkg/ui/events/service_test.gopkg/ui/widgets/widgets.gopkg/ui/widgets/widgets_test.gopkg/zapscript/commands.gopkg/zapscript/control.gopkg/zapscript/playlist.gopkg/zapscript/playlist_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/api/notifications.md
- pkg/api/notifications/notifications_test.go
- pkg/service/queues.go
- docs/api/methods.md
- pkg/api/methods/ui_test.go
- pkg/zapscript/playlist.go
- pkg/platforms/mister/platform.go
- pkg/platforms/shared/installer/installer_test.go
- pkg/platforms/shared/installer/installer.go
- pkg/service/service.go
- pkg/ui/widgets/widgets.go
- pkg/service/reader_manager_test.go
- pkg/ui/events/service.go
| // RunCommandOptions groups optional services used by specific command types. | ||
| type RunCommandOptions struct { | ||
| WaitForMediaReady func(context.Context) error | ||
| PlaybackManager audio.PlaybackManager | ||
| UI *uievents.Service | ||
| LauncherManager *state.LauncherManager | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add direct coverage for RunCommandOptions.UI.
The new UI injection boundary is not exercised by the supplied tests; add a RunCommand test that verifies a command receives the exact opts.UI instance through platforms.CmdEnv.
As per coding guidelines, “Write tests for all new code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/zapscript/commands.go` around lines 50 - 56, Add a RunCommand test
covering the RunCommandOptions.UI injection boundary. Configure opts.UI with a
distinctive service instance, execute a command that inspects platforms.CmdEnv,
and assert it receives the exact same UI pointer; keep existing command behavior
and test patterns unchanged.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/ui/widgets/widgets.go (3)
376-432: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSerialize event-backed picker responses.
Each selection or dismissal starts a new asynchronous
ui.respondrequest while the picker remains interactive. Rapid input can submit multiple selections; because the service uses first-response-wins arbitration, a later selection may arrive first and execute instead of the user’s initial choice. Disable input while a response is pending or add an atomic in-flight guard that is cleared on failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/widgets/widgets.go` around lines 376 - 432, Serialize event-backed picker responses in the run and dismiss handlers by adding an atomic in-flight guard or disabling picker input before starting sendUIResponse. Reject subsequent selections or dismissals while a request is pending, and clear the guard when sendUIResponse fails so the user can retry; retain the existing successful app-stop behavior.
193-210: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftTie the completion watcher to the UI lifecycle.
This goroutine and ticker only terminate when the completion file eventually appears. If the UI exits through timeout, dismissal, or selection, the watcher continues polling indefinitely; repeated widgets can accumulate leaked goroutines and tickers. Pass a cancellation context/channel and stop it whenever the application exits. Treat non-
not existsStaterrors as terminal rather than retrying silently.Suggested shape
-func watchCompletion(app *tview.Application, fs afero.Fs, completePath string) { +func watchCompletion(ctx context.Context, app *tview.Application, fs afero.Fs, completePath string) { ... - for range ticker.C { + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + } + ... + }Ensure callers cancel the context when the application stops.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/widgets/widgets.go` around lines 193 - 210, Update watchCompletion to accept a cancellation context or channel and select on it alongside ticker.C, stopping the ticker and goroutine when the UI exits; ensure all callers cancel that signal whenever the application stops, including timeout, dismissal, and selection paths. In the Stat handling, continue polling only for not-exists errors, while treating other errors as terminal and exiting the watcher.
170-232: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the missing widget test cases The suite already covers injected FS use, completion cleanup, response payloads, dismissibility, and Enter/Esc handling; add coverage for the UI response timeout path and the picker
Selectedfallback/arbitration when the index is out of range.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/widgets/widgets.go` around lines 170 - 232, Extend the widget tests around buildNoticeUI and sendUIResponse to cover local-client timeout behavior, asserting the timeout error is returned and the request context expires. Add picker coverage for out-of-range indices, verifying the Selected fallback and arbitration behavior choose the expected option without panicking. Preserve the existing injected filesystem, cleanup, payload, dismissibility, and keyboard-handling tests.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/ui/widgets/widgets.go`:
- Around line 376-432: Serialize event-backed picker responses in the run and
dismiss handlers by adding an atomic in-flight guard or disabling picker input
before starting sendUIResponse. Reject subsequent selections or dismissals while
a request is pending, and clear the guard when sendUIResponse fails so the user
can retry; retain the existing successful app-stop behavior.
- Around line 193-210: Update watchCompletion to accept a cancellation context
or channel and select on it alongside ticker.C, stopping the ticker and
goroutine when the UI exits; ensure all callers cancel that signal whenever the
application stops, including timeout, dismissal, and selection paths. In the
Stat handling, continue polling only for not-exists errors, while treating other
errors as terminal and exiting the watcher.
- Around line 170-232: Extend the widget tests around buildNoticeUI and
sendUIResponse to cover local-client timeout behavior, asserting the timeout
error is returned and the request context expires. Add picker coverage for
out-of-range indices, verifying the Selected fallback and arbitration behavior
choose the expected option without panicking. Preserve the existing injected
filesystem, cleanup, payload, dismissibility, and keyboard-handling tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1956e33a-7a85-4454-96f0-6e5cbf7863dc
📒 Files selected for processing (3)
pkg/ui/widgets/widgets.gopkg/ui/widgets/widgets_test.gopkg/zapscript/commands_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/ui/widgets/widgets_test.go
Guard event-backed picker selections and dismissals with an atomic in-flight flag so concurrent input cannot send duplicate responses; the guard clears on send failure to allow retry. Thread a cancellation context through watchCompletion so its ticker goroutine stops on every app-exit path (timeout, dismissal, selection), and treat non-not-exist Stat errors as terminal. Cover local-client timeout propagation and out-of-range picker Selected fallback.
1 participant
Summary
ui,ui.respond, andui.changed, keeping picker actions private behind opaque choice IDsCloses #226
Summary by CodeRabbit
uiandui.respondAPI methods and authoritativeui.changednotifications.