Repository navigation
Conversation
morgmart
left a comment
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
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.
|
|
||
| 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.
| 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.
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