Repository navigation
Conversation
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. Fresh static review of the exact three-dot comparison found two blocking and two non-blocking changed-line defects. After a rejected cron update, Cancel removes the only warning even though the pinned backend may have removed the active timer; the new field also exposes hidden or unassociated validation state to assistive technology. The new Spanish editor copy needs its missing accents restored. Separately, this PR adds a graphical schedule-editing workflow but the supplied PR body and conversation contain no screenshots or recording, so visual evidence is required. Supplied GitHub evidence is structurally valid and reports all 11 check runs successful at the reviewed head SHA; required checks still govern merge readiness. Final self-check covered the schedule list and edit, save, cancel, validation, unsupported-existing-expression, running, concurrent refresh, failed-update reconciliation, retry, focus, and collapsed-card flows; accessibility; English and Spanish localization; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system and project rules; duplicate and overlap handling; and concrete evidence plus user effect for blocking severity.
Deterministic publication result: 2 blocking and 1 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.
| function cancelEditing() { | ||
| setBaselineCron(schedule.cron); | ||
| setDraft(schedule.cron); | ||
| setError(null); |
There was a problem hiding this comment.
🤖 P1 · Keep failed schedules visibly uncertain (blocking)
A rejected update can remove the prior timer in the pinned Goose backend, and the editor correctly warns about that while it remains open. This Cancel path clears the error and collapses the editor, however, while the refreshed schedule record can still look normal because listing the record does not prove its timer survived. The collapsed row then has no state representing that known uncertainty.
User effect: After dismissing a failed edit, a schedule can look healthy in Berd even though future runs may silently never happen, so the user may rely on automation that is no longer active.
Recommended fix: Keep a per-schedule failed-update warning and recovery action visible on the collapsed row until a successful reschedule establishes a new timer; Cancel may discard the draft but must not erase the uncertainty state.
Test: Reject an update, complete reconciliation with the schedule still listed, cancel editing, and assert the collapsed row still exposes the warning and recovery action. Then complete a successful reschedule and assert the warning clears.
There was a problem hiding this comment.
🤖 Fixed in 65787f9. Failed cron expressions and timer uncertainty now live in per-schedule query cache for the app session. Cancel discards the draft but leaves a shared warning and Recover schedule action; panel navigation and list refresh do not clear them. Only a successful alternate reschedule clears uncertainty, and the failed expression remains blocked. Added reject → deferred reconciliation → Cancel → remount/refresh → recovery regressions. Shared per-schedule pending mutation state prevents overlapping updates across remount; canceled query reconciliation prevents stale cross-row snapshots overwriting newer results. The PR includes a collapsed warning/recovery screenshot. This is client-session evidence, not durable backend timer-health detection.
| onChange={(event) => { | ||
| setDraft(event.target.value); | ||
| }} | ||
| aria-invalid={!!validationError || !!error} |
There was a problem hiding this comment.
🤖 P2 · Associate the cron error (blocking)
The input is marked invalid whenever validationError or error exists, but the rendered message is conditional: an existing out-of-subset cron suppresses its validation message, and every displayed error lacks an id while aria-describedby references only the help text. Assistive technology can therefore encounter an invalid field with no stated reason, and displayed errors are not part of the field's accessible description.
User effect: A screen-reader user can be told the cron field is invalid without hearing why, and may miss the validation or save error needed to correct the schedule.
Recommended fix: Derive one displayed error state, set aria-invalid only when that error is shown, give its message an id, and include that id in aria-describedby alongside the help text. Use a non-interruptive live status for validation that changes during typing.
Test: Open an existing schedule whose cron is outside the editable subset and assert the field is not marked invalid while no error is shown. Then enter an unsupported expression and assert the field is invalid and its accessible description includes the displayed validation message.
There was a problem hiding this comment.
🤖 Fixed in 65787f9. A single displayedError controls aria-invalid, an error id, and aria-describedby alongside the help id. Unchanged existing out-of-subset expressions no longer expose hidden invalid state. Typing validation uses a polite atomic status. Tests assert the visible validation and backend failure are in the accessible description, and the existing unsupported-expression case is not invalid. Validation screenshots are in the PR description; no live screen-reader test was performed.
| "running": "En ejecucion", | ||
| "lastRun": "Ultima ejecucion: {{time}}" | ||
| "lastRun": "Ultima ejecucion: {{time}}", | ||
| "editCron": "Editar la expresion cron de {{id}}", |
There was a problem hiding this comment.
🤖 P2 · Restore Spanish diacritics (non-blocking)
The new Spanish schedule-editor strings consistently omit required accents, including expresión, día, numéricos, más, programación, actualización, falló, también, así, cambió, and válida. In messages such as 'ya fallo' and 'programacion cambio', the missing verb accents also change the grammar and meaning.
User effect: Spanish users see unfinished, sometimes ambiguous copy in the editor's instructions and failure warnings, reducing trust exactly where the schedule state may be uncertain.
Recommended fix: Correct the diacritics throughout the newly added Spanish strings and have the localized warning and validation copy reviewed as a complete set.
Test: Add or apply a localization review check for the corrected Spanish editor strings so the expected accented forms are preserved.
There was a problem hiding this comment.
🤖 Fixed in 65787f9. Corrected diacritics across the localSchedules Spanish guidance, validation, and failure/recovery copy, including expresión, día, numéricos, más, programación, actualización, falló, también, así, cambió, and válida. Added 22 literal-copy/guidance regression cases and captured the actual Spanish editor in dark theme at an 800px viewport. No external/native-speaker review is claimed.
f737461 to
65787f9
Compare
|
🤖 Review fixes are published in 65787f9 and all three inline findings have replies. Added four native GitHub screenshots under Testing: editor, persistent warning/recovery after Cancel, validation, and Spanish dark-theme validation at 800px. The production browser workflow exercised mocked ACP update failure and successful recovery, with no real scheduler writes. Focused validation: 351 tests passed; |
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. Fresh static review of the exact three-dot comparison found two blocking and one non-blocking publishable defects. The conservative cron validator globally substitutes month and weekday names before field validation, so names used in the wrong field can reach the backend despite the update path's known timer-removal risk. After a rejected update, the concurrency guard also disables changed-elsewhere detection wholesale, allowing a recovery save to overwrite a genuinely newer external schedule value. A separate canceled reconciliation can be mislabeled as a failed refresh. Two related proposals were suppressed against existing unresolved threads with substantive author replies: durable restart persistence for timer uncertainty and the stale field-error association after changing drafts. The three prior inline defects are otherwise fixed, and the supplied screenshots satisfy visual-evidence duty. Supplied GitHub evidence is structurally valid; most current checks report success, Transcript virtualization is still in progress, and older duplicate runs include cancellations. Required checks still govern merge readiness. Final self-check covered list, edit, validation, save, cancel, running-state, external-change, rejection, reconciliation, alternate recovery, remount, navigation, refresh, restart, concurrent-row, never-completes, focus, and collapsed-warning flows; accessibility; English and Spanish localization; navigation and consent guards; async, error, lifecycle, and race behavior; test honesty; design-system and project rules; duplicate and overlap handling; and concrete evidence plus user effect for blocking severity.
Deterministic publication result: 2 blocking and 1 non-blocking inline finding(s) publishable; 2 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| return "fieldCount"; | ||
| } | ||
|
|
||
| const normalized = fields.join(" ").toUpperCase(); |
There was a problem hiding this comment.
🤖 P1 · Validate aliases in their fields (blocking)
The validator replaces every month and weekday name across the entire expression before it validates fields. That turns an unsupported expression such as 0 9 * * JAN into a valid numeric weekday expression and 0 MON * * * into a valid numeric hour, while the editor sends the original raw expression to Goose. This defeats the field-aware lookup below and the conservative boundary whose purpose is to keep backend-rejectable input away from an update path that can remove the old timer before parsing the replacement.
User effect: A typo that uses a valid cron name in the wrong field can appear safe and be submitted. Goose may reject it after removing the existing timer, leaving the schedule unable to run, or interpret it with a cadence the user did not intend.
Recommended fix: Validate names within each field instead of globally canonicalizing the expression: allow month names only in the month field, weekday names only in the weekday field, and require numeric step values. Reject every name in seconds, minute, hour, day-of-month, or the wrong named field before calling ACP.
Test: Add rejection cases for 0 9 * * JAN, 0 MON * * *, JAN 9 * * *, 0 0 9 * * DEC, and named step values, while retaining acceptance coverage for weekday names and ranges in the weekday field.
There was a problem hiding this comment.
🤖 Fixed in 0721b73: month/weekday names are no longer replaced globally across the expression; they resolve only inside their owning field, so 0 9 * * JAN and 0 MON * * * are rejected and the exact text sent to Goose is what is validated. Field-level tests cover both cases.
|
|
||
| const scheduleChangedElsewhere = | ||
| isEditing && | ||
| !updateMutation.isError && |
There was a problem hiding this comment.
🤖 P1 · Preserve post-failure concurrency checks (blocking)
The changed-elsewhere guard is disabled whenever the mutation has errored or any local error remains. After reconciliation, therefore, a schedule value that differs from the editor baseline is never treated as an external change, even when it is unrelated to the failed expression. The user can enter an alternate recovery cron and save over that newer value.
User effect: If another Goose client updates the same schedule while Berd is recovering from a rejected save, Berd can silently overwrite that newer schedule configuration instead of asking the user to reopen and review it.
Recommended fix: Keep concurrent-change detection active after failures. Record the failed submitted expression and waive the baseline mismatch only when reconciliation shows the expected failed-update value; if the refreshed cron is a genuinely different value, block Save and require reopening from that value.
Test: Reject an update from baseline A to expression X, have reconciliation return an unrelated external value Z, then enter recovery value Y. Assert Berd shows the changed-elsewhere message, keeps Save disabled, and never sends Y. Retain the case where reconciliation returns X and an alternate recovery remains allowed.
There was a problem hiding this comment.
🤖 Fixed in 0721b73: the changed-elsewhere guard now also runs while a mutation error is present; only the exact rejected submitted value waives the baseline mismatch, so unrelated external changes are still detected after a failure. Tests cover the recovery-conflict path.
| queryFn: listLocalSchedules, | ||
| staleTime: 0, | ||
| }); | ||
| } catch { |
There was a problem hiding this comment.
🤖 P3 · Ignore superseded reconciliation errors (non-blocking)
The reconciliation comment intentionally relies on later updates canceling this shared schedule-list read, but the catch block treats every rejection, including TanStack Query cancellation, as a genuine refresh failure and appends the unknown-state error.
User effect: When another schedule update supersedes a failed row's reconciliation, the open editor can incorrectly tell the user that refreshing also failed even though a newer refresh replaced it successfully.
Recommended fix: Detect TanStack Query cancellation errors and leave the existing timer-uncertainty state unchanged for those; append the refresh-failed message only for a genuine list error.
Test: Keep row A's failed editor open while row B successfully saves and cancels A's reconciliation. Assert row A retains its timer warning but never shows the refresh-also-failed message; separately retain coverage for a real reconciliation rejection.
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. Fresh static review of the exact three-dot comparison found one blocking publishable defect: the new schedule-save workflow has no liveness bound, so a scheduler update or the awaited success refresh that never settles leaves the editor and per-schedule mutation lock permanently busy with no recovery path, while the remote timer state may be unknown. Three related timer-uncertainty/recovery proposals were consolidated and suppressed against the existing unresolved thread with a substantive author reply, and one stale accessible-error proposal was suppressed against its protected unresolved thread. All six previously published inline defects are otherwise fixed. The supplied screenshots satisfy visual-evidence duty. Supplied GitHub evidence is structurally valid and all ten current check runs report success at the reviewed head SHA; required checks still govern merge readiness. Final self-check covered schedule listing, editing, validation, Save, Cancel, running-state, external-change, rejected update, reconciliation, alternate recovery, equivalent expressions, remount, navigation, refresh, restart, concurrent rows, never-completes, late completion, focus, and collapsed-warning flows; accessibility; English and Spanish localization; navigation and consent guards; async, error, lifecycle, and race behavior; test honesty; design-system and project rules; duplicate and overlap handling; and concrete evidence plus user effect for blocking severity.
Deterministic publication result: 1 blocking and 0 non-blocking inline finding(s) publishable; 2 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
|
|
||
| const updateMutation = useMutation({ | ||
| mutationKey, | ||
| mutationFn: ({ scheduleId, cron }: { scheduleId: string; cron: string }) => |
There was a problem hiding this comment.
🤖 P1 · Bound schedule save liveness (blocking)
The new mutation waits directly for the ACP schedule update, then its success handler waits for an active-query invalidation/refetch before clearing isSaving, the shared mutation lock, and the editor. ACP extension requests have no built-in deadline, so either the update response or the post-success list refresh can remain pending indefinitely. The input and actions then stay disabled, remounting still sees the schedule as mutating, and there is no degraded path. If the update itself timed out locally without fencing its late completion, a retry could also race an operation whose remote timer state is unknown.
User effect: A stalled Goose connection can leave schedule editing permanently locked until the app is restarted. The user cannot tell whether the timer changed, recover safely, or edit that schedule again, and retrying after interruption could race a late backend result.
Recommended fix: Give the update a finite liveness bound tied to the captured local ACP connection generation. On timeout, synchronously detach that generation, retain timer uncertainty, release the UI from its busy state, and ignore late completion. Do not keep the successful mutation busy on an unbounded list refresh; bound that readback or finish the confirmed update state and refresh separately with an explicit recoverable refresh error.
Test: Use a never-settling update promise and assert the deadline releases the editor, preserves the timer-verification warning, invalidates the captured transport generation, and prevents a late result from clearing uncertainty. Separately let the update succeed while list refresh never settles and assert the save does not remain permanently busy and the user receives a recoverable refresh state.
There was a problem hiding this comment.
🤖 Fixed in 186c3f0, restacked on the corrected parent.
Updates now capture the local ACP generation before client acquisition, enforce a 60-second deadline, check generation before send/after response, and synchronously detach only that captured generation at timeout without awaiting cleanup. The failed request releases the editor/shared mutation lock and retains timer uncertainty; late client acquisition cannot send and late completion cannot clear the warning or replace fresh data. An older timeout cannot detach a newer replacement. This fences local completion, not remote execution or rollback: the UI explicitly requires timer verification before recovery.
List reads have a separate 15-second deadline and recoverable error state, preserving the last loaded list. Confirmed saves finish without awaiting invalidation/refetch. Ordinary rejection keeps cancelable shared-query reconciliation, now bounded; timeout/connection-change errors do not wait on it.
Nine deterministic real-connection-lifecycle regressions cover never-settling update/cleanup/readback, remount lock release, late success/error, replacement-generation safety, delayed acquisition, successful save with stalled refresh, and bounded rejection reconciliation. All 277 focused Automations/ACP tests, just check, configured pre-commit jobs and production build passed. Three synthetic production-renderer scenarios passed (deadline unlock/warning, success with stalled readback, Spanish copy).
Validation limitation: full suite was 8,593 passed, one skipped and one failure in the unchanged chat voice-scroll assertion (expected200/received354). The exact same failure was reproduced on original published head 4f8818b; root cause remains undiagnosed and no chat code/tests were changed. New exact-head CI is pending. No full-suite-green, remote cancellation, native/live scheduler or screen-reader claim.
4f8818b to
186c3f0
Compare
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
APPROVE. Fresh static review of the exact three-dot comparison found no publishable changed-line defects. The previously blocking schedule-save liveness issue is fixed: update requests have a finite deadline tied to the captured ACP connection generation, timeout synchronously detaches only that generation, late acquisition and completion are fenced, timer uncertainty remains visible, the mutation lock releases, and successful updates no longer wait on list readback. Discriminating tests cover never-settling updates, stalled cleanup and readback, remount lock release, late success and failure, replacement-generation safety, delayed acquisition, bounded reconciliation, and recoverable refresh. Two remaining product/accessibility proposals are protected duplicates of unresolved threads with substantive author replies. The supplied screenshots satisfy visual-evidence duty. Supplied GitHub evidence is structurally valid at the reviewed head; it reports successful frontend, build, clean-install, Rust, security, and DCO checks, plus a failed Transcript virtualization check and cancelled superseded jobs. Required checks still govern merge readiness. Final self-check covered schedule listing, editing, validation, Save, Cancel, running-state guards, rejected updates, timer uncertainty, alternate recovery, equivalent expressions, refresh, remount, navigation, concurrent rows, external changes, never-completes behavior, connection replacement, late completion, focus, accessibility, English and Spanish localization, responsive/theme behavior, navigation and consent guards, test honesty, design-system/project rules, duplicate overlap, and concrete blocking evidence.
Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 2 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
Completed checks currently reporting failure:
- Transcript virtualization: failure
- Frontend build smoke: cancelled
- Transcript virtualization: cancelled
- Tauri Rust checks (Windows): cancelled
- Tauri Rust checks (macOS): cancelled
- Tauri Rust checks (Linux): cancelled
- Frontend checks and unit tests: cancelled
Pending checks: 1 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
186c3f0 to
d5e6223
Compare
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
APPROVE. Fresh static review of the exact three-dot comparison found no publishable current changed-line defects. Atlas found the guarded schedule editor coherent with Berd’s local-automation workflow and operational-truth principles. Vogue found the editor and timer-verification recovery surfaces legible, responsive, localized, accessible, and design-system grounded; four supplied screenshots satisfy visual-evidence duty. Spar found finite update/read deadlines, generation-scoped invalidation, late-result fencing, query-owned uncertainty, bounded reconciliation, and concurrency protection at the ACP boundary. Engineering distinctly applied the project code-review and user Wes-review lenses and found no remaining correctness, derivation, trust-boundary, lifecycle, race, test-honesty, accessibility, localization, security, or maintainability defect. The only delta from the previously approved implementation safely hedges the Spanish scheduler-unavailable fallback and updates its literal-copy assertion. Two remaining proposals are protected duplicates of unresolved threads with substantive human replies. Supplied GitHub evidence is structurally valid at the reviewed head; Frontend checks and unit tests failed, Frontend build smoke and Transcript virtualization were skipped, successful checks cover Rust, security, clean install, and DCO, and superseded jobs were cancelled. Required checks still govern merge readiness. Final self-check covered listing, edit, Save, Cancel, validation, running guards, failure uncertainty, recovery, equivalent expressions, refresh, remount/navigation, concurrent rows, external changes, never-completes behavior, connection replacement, late completion, focus, accessibility, English and Spanish localization, responsive/theme behavior, navigation and consent guards, test honesty, design-system/project rules, duplicate overlap, and concrete blocking evidence.
Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 2 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
Completed checks currently reporting failure:
- Frontend checks and unit tests: failure
- Frontend build smoke: cancelled
- Transcript virtualization: cancelled
- Tauri Rust checks (Linux): cancelled
- Frontend checks and unit tests: cancelled
- Tauri Rust checks (macOS): cancelled
- Tauri Rust checks (Windows): cancelled
Pending checks: 1 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
Summary
Layer 1 lets OSS users view schedules managed by their embedded Goose instance. This adds a guarded cron editor so users can change a schedule without opening a separate Goose instance. Because Goose can remove the old timer when an update fails, the editor validates a conservative cron subset and warns users to verify a failed update.
Related issue
Related PR: #433. No matching issue found.
Testing
Rendered the child production build in cached Chromium with synthetic Tauri and Agent Client Protocol (ACP) responses. Save, rejected update, reconciliation, Cancel, and successful alternate recovery reached the mocked ACP boundary. No real scheduler writes or DOM changes. This is browser UI evidence, not a live native/backend end-to-end test.
Edit an existing schedule:
Cancel after a rejected update: timer uncertainty and Recover schedule remain visible despite a normal-looking list record. The warning survives panel navigation and refresh for this app session. A successful alternate update clears it; a list response cannot prove timer health.
Invalid draft: visible validation is associated with the input; Save is disabled.
Spanish, dark theme, 800px viewport: corrected accents, locale-formatted date, and wrapped validation/help copy.
Generated with Goose