Repository navigation
Conversation
kyledef
changed the title
617610c to
e235e08
Compare
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. The local-schedules capability is correctly separated from company automation services, but this graphical UI change needs screenshots or a short screen recording. Static review also found one blocking design-system violation and two non-blocking schedule-legibility issues. Supplied GitHub checks are complete and passing.
Deterministic publication result: 1 blocking and 2 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 1 blocking screenshot-evidence requirement(s) in this review body.
🤖 Blocking · Screenshots needed
This PR changes Berd’s graphical interface. Please add screenshots or a short screen recording so the visual result can be reviewed. Screenshots are review evidence; they do not replace accessibility, responsive, theme, localization, or behavior validation.
| ) : schedulesQuery.error && schedules.length === 0 ? ( | ||
| <div | ||
| role="alert" | ||
| className="rounded-xl border border-destructive/30 bg-destructive/5 p-4 text-sm" |
There was a problem hiding this comment.
🤖 P2 · Use the shared error alert (blocking)
The new first-load error state hand-builds alert chrome and destructive color states in feature code even though the shared Alert primitive already owns this behavior. This violates the repository rule that shared UI primitives and semantic variants are the default for reusable interface states.
User effect: Scheduler failures will look and behave inconsistently with other Berd errors, and this one-off treatment can drift across themes and future design-system changes.
Recommended fix: Compose the error from the shared destructive Alert, AlertTitle, and AlertDescription primitives instead of styling a feature-local alert container.
Test: Keep a component test for the initial failure state that asserts the shared alert exposes the localized title and error description.
There was a problem hiding this comment.
🤖 Fixed in 082f02c. The initial failure now composes the shared destructive Alert, AlertTitle, and AlertDescription with the warning icon. The component test checks the shared alert metadata, localized title, and backend description. The PR description includes an exact-parent error screenshot.
| </div> | ||
| <div className="mt-1 text-xs text-muted-foreground"> | ||
| <span className="font-mono">{schedule.cron}</span> | ||
| {schedule.paused |
There was a problem hiding this comment.
🤖 P2 · Show the enabled schedule state (non-blocking)
The status line adds labels only when a schedule is paused or currently running. An enabled but idle schedule therefore has no state label, even though users need to distinguish that normal state from paused.
User effect: Users cannot tell whether an idle schedule is active and will run at its next scheduled time or whether its state is simply missing.
Recommended fix: Show a localized Active label whenever the schedule is not paused, while retaining Running as an additional state when applicable.
Test: Render an enabled idle schedule and assert that Active is shown; render a paused schedule and assert that Paused is shown instead.
There was a problem hiding this comment.
🤖 Fixed in 082f02c. Every non-paused schedule now shows localized Active; paused schedules show Paused instead, and Running remains an additional label. Both idle/paused cases are covered by component tests and the parent overview screenshot.
| ? ` · ${t("localSchedules.running")}` | ||
| : ""} | ||
| {schedule.lastRun | ||
| ? ` · ${t("localSchedules.lastRun", { time: schedule.lastRun })}` |
There was a problem hiding this comment.
🤖 P2 · Localize the last-run time (non-blocking)
The scheduler's lastRun string is inserted directly into translated copy rather than being parsed and formatted through Berd's locale-aware date utilities.
User effect: Users see a machine-formatted timestamp instead of a readable local date and time, and switching the app language does not localize that value.
Recommended fix: Parse and format lastRun with the app's locale-aware date/time formatter, with a safe fallback when the backend value is invalid.
Test: Pass a known scheduler timestamp and assert the localized formatted result is shown rather than the raw source string, including the invalid-value fallback.
There was a problem hiding this comment.
🤖 Fixed in 082f02c. lastRun is parsed and formatted through useLocaleFormatting with medium date/short time; invalid values render localized Unknown. Tests cover the formatted timestamp and invalid fallback. The parent overview shows the formatted date; the child Spanish screenshot also shows locale-specific date output.
|
🤖 Review fixes are published in 082f02c, with the child restacked above it. Added two native GitHub screenshots under Testing: exact parent overview and shared initial-error Alert. These render the production app with synthetic Tauri/ACP responses; they do not claim live scheduler/native desktop coverage. Parent Automations tests: 104 passed. |
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
COMMENT. The prior alert, enabled-state, and last-run formatting issues are fixed at the current head, screenshots are present, and supplied GitHub checks are complete and passing. One new non-blocking UI finding remains: the OSS Automations surface renders a one-item tab bar with no alternative destination.
Deterministic publication result: 0 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| <TabsTrigger value="history" variant="weight"> | ||
| {t("tabs.history")} | ||
| </TabsTrigger> | ||
| {companyAutomationsEnabled ? ( |
There was a problem hiding this comment.
🤖 P3 · Remove the one-item tab bar (non-blocking)
When company automations are disabled, the change hides the History tab but leaves the Tabs container, TabsList, and Overview tab mounted. The resulting tab bar has only one destination and cannot switch views.
User effect: OSS users encounter a redundant Overview control between the page introduction and the only available content, while assistive technology announces a tab interface with no meaningful choice.
Recommended fix: Render the local-schedules overview directly when company automations are disabled, and render the tab interface only when both Overview and History are available.
Test: Render the workbench with company automations disabled and assert that no tablist or tab is present while Goose schedules remain visible; also retain coverage that both Overview and History tabs appear when company automations are enabled.
Summary
Berd's Automations view depends on Block's company automation service, so OSS users cannot see schedules from their local Goose instance. This adds a read-only Goose schedules section and keeps company-only requests off in OSS builds.
Related issue
Related PR: #245. No matching issue found.
Testing
Rendered the parent production build in cached Chromium with synthetic Tauri and Agent Client Protocol (ACP) responses. No DOM changes or real scheduler writes. This is browser UI evidence, not a live native/backend end-to-end test.
Schedule overview: active, paused, and running states, recipe filenames, and locale-formatted last-run time.
Initial scheduler failure: shared destructive Alert with title and backend description.
Generated with Goose