feat(frontend): classic skin default + frontend UI rework - #4914
feat(frontend): classic skin default + frontend UI rework#4914LittleChenLiya wants to merge 37 commits into
Conversation
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed the skin framework, scheduled-task redesign, and settings rework against head 10fa3f8. The skin module (types/storage/context) is clean and well-tested, but I found one functional bug in the zh-CN skill localization (the enable toggle sends the display name to the API), a status-dot lookup bug in the new task cards, a memory-import validation regression that can crash the settings page, and the scheduled-tasks e2e specs were left stale after the UI rework. Smaller notes: core/scheduled-tasks/recipes.ts, the scheduledTasks.recipes/unused filters.* i18n keys, and describeSchedule (now only referenced by its own unit test) survive as dead code after the recipe UI was dropped; about-settings-page.tsx uses locale === "zh-CN" where the rest of the codebase uses locale.startsWith("zh"); and the no-op PageEnter wrapper plus the unused prefersReducedMotion export could wait for PR #4915. Per the repo's test policy, please update the e2e specs and add coverage for the new /workspace/scheduled-tasks/new create flow.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-reviewed at the new head d46dcb7 (previous review was against 10fa3f8). Since then the PR was stripped down — core/skins/ (SkinProvider/storage/applySkinToDocument/SKIN_IDS), chats-settings-page.tsx, and the skill/about localizations are all gone — but the PR description still advertises every one of those (skin framework with persistence and data-skin switching, chats embedded in the settings menu, skills/about translated to Chinese). The description and screenshots no longer match the diff: what remains of the skin feature is a static, non-selectable SkinCard plus the skins.classic i18n keys, so PR #4915 will have nothing to build on here. Please update the description before merge. Status of the earlier findings at this head: the localizeSkill enable-toggle bug is resolved by reverting the skill translation, while the status-dot lookup in StatusBadge, the weakened isImportedMemory validation, the stale frontend/tests/e2e/scheduled-tasks.spec.ts, and the filtered-vs-empty state on the tasks page are all still present (existing comments cover them). One new inline finding below on the timezone dropdown, which was not in the previously reviewed scope.
github-actions
Bot
added
area:frontend
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-review at head 40bcb8e (third pass; earlier reviews were against 10fa3f8 and d46dcb7). All six prior findings are resolved at this head: the zh-CN skill localization moved out to #4915, StatusBadge now receives the raw status, the memory import re-validates all six sections via isImportedMemory, the scheduled-tasks e2e suite was rewritten for the card-grid/detail-dialog UI, the filtered empty state now distinguishes no-matches from no-tasks, and the timezone dropdown restores the full Intl.supportedValuesOf list behind a common/all toggle. The remaining items are coordination/coverage notes, not blockers: the sidebar Chats removal makes the conversation-list page unreachable until #4922 lands, and the new one-time date validation has no unit coverage.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Fourth pass at head 40bcb8e (unchanged since the last review yesterday). Two new inline findings: the split-out create page drops the ?thread_id= context that the old inline form used to pre-fill reuse_thread + the target thread id, and the new year/month/day inputs lost the accessible name the old datetime-local input had. Everything else from the earlier passes still holds at this head — the six prior findings remain fixed, and the open coordination/coverage notes (conversation-list entry point pending #4922, unit coverage for the one-time date validation) are unchanged.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Fifth pass, re-reviewed at head 8687d5e. The three open items from the last review are all resolved here: both create entry points now forward ?thread_id= and seed reuse_thread + the target thread id on the new create page, the year/month/day inputs carry aria-label, and buildOnceRunAtLocal was extracted into cron.ts with unit coverage for Feb 29/30, month bounds, empty time, pre-1970, and the zoned round-trip. Also verified the input-box.tsx change is a pure wrapper (<div className="relative">) plus prettier reformatting with no behavioral delta, and the new sidebar e2e regexes match the actual i18n strings at this head. Two small new notes below: the edit dialog can save an empty once-spec (the create page guards this, the edit path does not), and the new FilterChips convey their toggle state only through color.
| timezone: editSchedule.timezone || "UTC", | ||
| }) | ||
| } | ||
| disabled={updateTask.isPending} |
There was a problem hiding this comment.
Suggestion: the create page disables submit while the schedule is incomplete (!createSchedule.schedule_spec.cron && !createSchedule.schedule_spec.run_at), but this edit submit only checks updateTask.isPending. With the one-time date now split into four inputs, clearing any one of them makes the schedule input emit schedule_spec: {} (see the runAt ? { run_at: runAt } : {} branch in scheduled-task-schedule-input.tsx), and Save then PUTs an empty spec — the user just gets the generic update-failure toast with no pointer at which field is wrong. Consider mirroring the create guard here, e.g. disabled={updateTask.isPending || (!editSchedule.schedule_spec.cron && !editSchedule.schedule_spec.run_at)} (title/prompt too, if you want full parity).
There was a problem hiding this comment.
Fixed in cbc1c00: the edit submit now mirrors the create guard — disabled={updateTask.isPending || (!editSchedule.schedule_spec.cron && !editSchedule.schedule_spec.run_at)} — so Save cannot PUT an empty spec when any once-spec field is cleared.
| }) { | ||
| return ( | ||
| <button | ||
| type="button" |
There was a problem hiding this comment.
Nit (a11y): the Enabled/Paused chips behave as toggle buttons, but their state is conveyed only by the primary-colored fill, so a screen reader announces them as plain buttons with no on/off state. Adding aria-pressed={active} would announce the active filter. (The card corner dots and StatusBadge dots are already correctly aria-hidden.)
There was a problem hiding this comment.
Fixed in cbc1c00: added aria-pressed={active} to the Enabled/Paused FilterChips so screen readers announce the toggle state.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Sixth pass, re-reviewed at head cbc1c00 (after 10fa3f8 / d46dcb7 / 40bcb8e / 8687d5e). Everything raised in earlier rounds is resolved at this head: the StatusBadge dot lookup, the memory import re-validation, the timezone list with the common/all toggle, the thread_id forwarding into the new create page, the edit-submit guard, and the FilterChip aria-pressed. Three new findings on the final state, all around the scheduled-task page: (1) the new empty state renders during the initial fetch because hasTasks is derived from data before the query resolves, (2) the create page's formError path is unreachable and an invalid one-time date gives no feedback now that the schedule preview is gone, and (3) the detail dialog dropped the lastRunId/lastError rows but their i18n keys were left behind and task.last_error is no longer surfaced anywhere.
| timezone: "UTC", | ||
| }); | ||
| const [createNonce, setCreateNonce] = useState(0); | ||
| const hasTasks = (data ?? []).length > 0; |
There was a problem hiding this comment.
Loading flash: hasTasks is derived from data, which is undefined until the react-query fetch resolves (useScheduledTasks/useThreadScheduledTasks have no placeholderData), so on every initial load hasTasks is false, queryError is null, and the filteredData.length === 0 branch renders the full "No scheduled tasks yet" empty state with the create CTA — only for it to be replaced by the card grid a moment later. The earlier fix in this thread distinguished no-tasks from no-matches, but not pending from empty. Gating on the query state (e.g. const isLoading = threadId ? threadTasksQuery.isPending : allTasksQuery.isPending and skipping the empty state while it's true) would keep the flash out.
There was a problem hiding this comment.
Fixed in 1b08bc7 — the empty states are now gated on the query state: while the react-query fetch is pending, isLoading renders a loading placeholder instead of the "No scheduled tasks yet" block, so the empty state (and its create CTA) can no longer flash before the first data resolves.
| !hasSchedule || | ||
| (contextMode === "reuse_thread" && !targetThreadId) | ||
| ) { | ||
| setFormError(st.create.fillRequired); |
There was a problem hiding this comment.
This validation and the formError render below are unreachable: the submit button is disabled under exactly the same conditions (!title || !prompt || (!cron && !run_at) || (reuse_thread && !targetThreadId)), so handleCreate can never hit the setFormError branch. That matters more now that the schedule preview (data-testid="schedule-preview") is gone: with the once schedule split into year/month/day inputs, an invalid combination (Feb 30, month 13, or an untouched time field) makes buildOnceRunAtLocal return "" and the button silently greys out with no explanation. Consider dropping the dead branch and instead showing a hint when createSchedule.schedule_type === "once" && !createSchedule.schedule_spec.run_at (e.g. "Enter a valid date and time"), which the old preview line used to communicate.
There was a problem hiding this comment.
Fixed in 1b08bc7 — the dead validation branch and formError render were dropped from the create page, and the once-type hint now renders under the schedule input (schedule_type === "once" && !schedule_spec.run_at) showing st.create.invalidOnce ("Enter a valid date and time"), which covers the invalid-date cases the old preview line used to communicate.
There was a problem hiding this comment.
Fixed in 1b08bc7: dropped the unreachable formError branch. The create page now shows the "Enter a valid date and time" hint under the schedule input whenever the once spec is incomplete (once with no run_at), restoring the discoverability the removed schedule-preview line provided.
There was a problem hiding this comment.
Fixed - the dead formError branch is gone (the submit guard already covered those cases); the once-type missing-date case now renders a reachable invalidOnce hint under the schedule input instead (acca5a2).
| value: formatTimestamp(task.next_run_at, locale), | ||
| }, | ||
| { | ||
| label: stDetail.lastRun, |
There was a problem hiding this comment.
Nit: the reworked detail rows stop after lastRun, but the old dialog also showed lastRunId and lastError — the i18n keys scheduledTasks.detail.lastRunId/lastError are still present in en-US.ts (lines 387-388), zh-CN.ts, and types.ts, and are now unreferenced. More substantively, task.last_error is no longer surfaced anywhere on the page (only the per-run run.error inside the run list), so a failed task whose runs have rolled out of the list has no visible error. Either remove the two dead keys, or keep a lastError row (it's the most useful field when a task shows the red status dot).
There was a problem hiding this comment.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Seventh pass, re-reviewed at head 1b08bc7 (one commit since cbc1c00: the empty-state / form-error / lastError fixes). All three findings from the last round verified fixed: the empty state is now gated on the correct per-branch isPending (the disabled thread query is never consulted for the flag, and cached data keeps the 15s refetch interval from re-flashing the loading state), the unreachable formError branch is gone and replaced by an invalidOnce hint with keys added to all three locale files, and the detail dialog surfaces task.last_error again. Two small notes below on the final state.
| prompt: "Prompt", | ||
| submit: "Create", | ||
| fillRequired: "Fill all required fields", | ||
| invalidOnce: "Enter a valid date and time", |
There was a problem hiding this comment.
Nit: now that the unreachable formError branch is gone, scheduledTasks.create.fillRequired has no remaining references (en-US.ts:369, zh-CN.ts:353, types.ts:291) — the old inline create form on main was its last consumer. It can be dropped the same way t.sidebar.chats was in 8687d5e (scheduledTasks.detail.lastRunId is the other orphan, already noted in the detail-rows thread).
There was a problem hiding this comment.
Fixed - scheduledTasks.create.fillRequired and the last remaining references were removed (49b5ad7).
| } | ||
| disabled={ | ||
| updateTask.isPending || | ||
| (!editSchedule.schedule_spec.cron && |
There was a problem hiding this comment.
Suggestion: the new invalidOnce hint was added on the create page only, but the edit dialog has the same silent state. For a once-type task, clearing any of the year/month/day/time fields makes the schedule input emit schedule_spec: {}, and Save greys out via this guard with no explanation — the discoverability gap the create-page hint just fixed. Rendering the same hint under the edit ScheduledTaskScheduleInput (e.g. when editSchedule.schedule_type === "once" && !editSchedule.schedule_spec.run_at) would give edit-mode parity with /workspace/scheduled-tasks/new.
There was a problem hiding this comment.
Fixed in b8d2a6f — the edit dialog now renders the same hint under the ScheduledTaskScheduleInput when editSchedule.schedule_type === "once" && !editSchedule.schedule_spec.run_at (using st.edit.invalidOnce), giving edit mode parity with the create page.
There was a problem hiding this comment.
Fixed in b8d2a6f: the edit dialog now renders the same hint under the schedule input when the once spec is incomplete (editSchedule.schedule_type once with no run_at), giving edit-mode parity with /workspace/scheduled-tasks/new.
There was a problem hiding this comment.
Fixed - the edit dialog now mirrors the create guard: it renders the invalidOnce hint when a once schedule lacks run_at and keeps submit disabled until the schedule is complete (49b5ad7).
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Eighth pass, re-reviewed at head 532a891 (39fa5c9 / b8d2a6f / 532a891 since the last review at 1b08bc7). Everything raised across the seven earlier rounds is verified resolved at this head: the TaskDetail lastError row with the ?? fallback, the invalidOnce hint mirrored in the edit dialog, the orphan fillRequired / lastRunId / sidebar.chats keys dropped from all three locale files, the timezone common/all toggle whose COMMON_TIMEZONES list matches the new i18n timezone.common map, and the empty states gated on the per-branch isPending (create errors still surface via the useCreateScheduledTask onError toast, confirmed in hooks.ts). The input-box.tsx change is a pure re-indent inside a new relative wrapper with no behavioral diff, and the globals.css classic palette / message-bubble / settings-dialog restyling look internally consistent. Two new minor suggestions below.
| schedule_spec: createSchedule.schedule_spec, | ||
| timezone: createSchedule.timezone || "UTC", | ||
| }, | ||
| { onSuccess: () => router.push("/workspace/scheduled-tasks") }, |
There was a problem hiding this comment.
Suggestion: the inbound half of the thread_id round-trip is handled (createHref on the list page forwards ?thread_id=, and this page seeds reuse_thread + the target id), but every exit from this page hardcodes the unfiltered URL — this success callback, plus the Back button (line 58) and Cancel (line 127). A user arriving from a thread's "Scheduled tasks" link who creates the task, or just navigates back, lands on the global list and loses the thread context this page deliberately preserved. Since initialThreadId is already in scope, router.push(initialThreadId ? /workspace/scheduled-tasks?thread_id=${encodeURIComponent(initialThreadId)} : "/workspace/scheduled-tasks") (or plain router.back() for Back/Cancel) would keep the filter symmetrical.
There was a problem hiding this comment.
Fixed in d8da426. Create success, Back, and Cancel now all return to listHref, which keeps ?thread_id= when the page was seeded from inbound query params.
|
|
||
| await page.getByPlaceholder("Task title").fill("Created from UI"); | ||
| await page.getByPlaceholder("Prompt").fill("Summarize thread"); | ||
| await page.getByRole("button", { name: "Create", exact: true }).click(); |
There was a problem hiding this comment.
Coverage: this test creates with the default recurring schedule only — nothing in the reworked spec clicks the One-time toggle or fills the new year/month/day/time inputs, so the once branch of the redesigned create flow (the split date inputs, the invalidOnce hint, and the run_at submission path on /workspace/scheduled-tasks/new) has no e2e coverage. Worth noting because an earlier fix reply on this PR states the year/month/day create flow "has its own e2e coverage", which the spec at this head does not actually provide — buildOnceRunAtLocal's unit tests cover the date math, but not the UI wiring. Extending this test to switch to One-time, clear one field (assert the hint + disabled Create), then fill a valid date would lock in the most logic-dense part of the redesign.
There was a problem hiding this comment.
Fixed in d8da426. Added an e2e that switches to One-time, asserts the invalidOnce hint + disabled Create, fills year/month/day/time, then submits. Also added a filtered-list round-trip test for inbound thread_id.
|
@LittleChenLiya please fix the conflicts with the main branch. |
Make the new warm-cream workspace appearance the default DeerFlow look, including refreshed message, welcome, and settings chrome.
Stack Observatory on the new classic workspace look as an opt-in skin, keeping classic as the default.
…error state, memory empty state, once-schedule validation
…ead noTasks i18n key
…geEnter live in observatory layer)
…quire time for one-time tasks
- Scheduled task timezone select defaults to a short localized common list and expands to the full IANA set via a More timezones toggle; labels go through i18n (en-US/zh-CN) instead of hardcoded zh text. - Raise the chat header z-index so the mobile sidebar trigger stays clickable above the welcome layout. - Gate the scheduled-tasks empty states on !queryError so a failed load doesn't also advertise an empty list. - Adapt e2e specs to the slimmed sidebar (Scheduled Tasks instead of Chats, updated welcome copy) and disambiguate the header link via its aria-label. - Mock token-usage and workspace-changes endpoints as 404 to prevent the fetcher's 401 login redirect from firing when the backend is absent.
…ation, a11y label - Forward the ?thread_id= context into /workspace/scheduled-tasks/new and seed reuse_thread + target thread id. - Extract one-time run-at validation into buildOnceRunAtLocal in cron.ts with rstest coverage. - Add aria-label to once year/month/day inputs. - Drop unused t.sidebar.chats i18n key.
…e filter chip state
…r branch, surface last task error
…main Re-apply the bytedance#4918 ReuseThreadNotice (alert) in the redesigned scheduled-tasks UI: detail dialog shows it for reuse_thread tasks, the create page shows it when reuse_thread is selected. Adds scheduled-task-detail/scheduled-task-create-form testids and adapts the e2e spec to the new layout.
532a891 to
78f3d19
Compare
…in reuse-thread e2e The dialog overlay intercepts the create-toggle click; press Escape and await dialog dismissal first.
| <header className="flex shrink-0 items-center justify-between gap-3 border-b px-4 py-3"> | ||
| <div className="flex items-center gap-3"> | ||
| <Button | ||
| variant="ghost" |
There was a problem hiding this comment.
[P2] Give the back button an accessible name
This icon-only navigation button has no aria-label or other accessible name, so assistive technology announces it only as an unnamed button. Please add a localized label such as “Back to scheduled tasks” (and optionally a matching title).
There was a problem hiding this comment.
Fixed in d8da426. The icon-only back button now has a localized aria-label/title (st.create.back: "Back to scheduled tasks" / "返回定时任务").
… cover one-time e2e Round-trip inbound thread_id on Back/Cancel/success from /workspace/scheduled-tasks/new. Give the icon-only back button a localized aria-label. Add e2e for the once schedule year/month/day/time path and for returning to the filtered list.
| schedule_spec: createSchedule.schedule_spec, | ||
| timezone: createSchedule.timezone || "UTC", | ||
| }, | ||
| { onSuccess: () => router.push(listHref) }, |
There was a problem hiding this comment.
[P2] Return to a list that can contain the created task
listHref is always derived from the inbound initialThreadId, but this form lets the user switch to fresh_thread_per_run or edit targetThreadId. In those cases the POST creates a task with thread_id: null or a different thread ID, then success navigates back to the original thread's filtered list, where the new task cannot appear. Keeping listHref for Back/Cancel makes sense, but the success destination should follow the submitted context (global for fresh tasks, or the chosen target for reuse tasks). The new round-trip test covers only the unchanged pre-seeded thread ID, so it misses both branches.
There was a problem hiding this comment.
Fixed in bd73174. Back/Cancel still use inbound listHref. Create success now follows the submitted context: global list for fresh_thread_per_run, or ?thread_id= of the chosen targetThreadId for reuse_thread. Added e2e for switching to Fresh from a seeded filter, and for editing the reuse thread ID before submit.
…ain them Keep inbound thread_id for Back/Cancel, but send create success to the submitted context: global list for fresh_thread_per_run, or the chosen target thread for reuse_thread. Cover both branches in e2e.
Keep the redesigned memory facts list from this PR. Main's Streamdown sanitization on memory summaries does not apply because this branch no longer renders summaries via SafeStreamdown. Auto-merged chat-page (ThreadSubagentBatches), i18n, and mock-api branch coverage from bytedance#4983/bytedance#4998/bytedance#4987.
|
Merged origin/main in bbd2aae to clear the DIRTY conflict. The only conflict was memory-settings-page.tsx (this PR's redesigned facts list vs main's Streamdown sanitization on summaries). Kept this PR's facts-list UI; sanitizer does not apply because this branch no longer renders summaries via SafeStreamdown. Chat page (ThreadSubagentBatches), i18n, and mock-api branch coverage from main auto-merged. #4915 and #4922 were rebased onto the new head. |
Keep the card+dialog scheduled-tasks UI from this PR. Port bytedance#5064 duplication into the create page via query params (title/prompt/context/schedule), restore schedule-preview via describeSchedule, and adapt the duplicate e2e to open the detail dialog then navigate to /new.
|
Merged origin/main in eb7ee01. Conflict was scheduled-tasks page (this PR's card+dialog UI vs #5064 duplicate-into-inline-form). Kept the redesigned UI and ported duplication: Duplicate in the detail dialog navigates to /workspace/scheduled-tasks/new with draft query params. Restored schedule-preview via describeSchedule so the duplicate e2e still checks the filled create form. #4915 and #4922 rebased onto the new head. |
|
@LittleChenLiya, please resolve the conflict with the main branch. |
# Conflicts: # frontend/src/components/workspace/input-box.tsx
|
Merged origin/main in b5f19c0. The remaining conflict was input-box.tsx (this PR's relative PromptInput wrapper vs #5050 model-name truncation). Kept the wrapper for skin/z-index and applied main's truncation classes ( |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
I found two blocking regressions in the new scheduled-task duplicate flow. The focused cron tests pass and the current CI suite is green, but the existing duplicate E2E case covers only a short prompt with reuse_thread, so it misses both cases below.
| const [contextMode, setContextMode] = useState< | ||
| "fresh_thread_per_run" | "reuse_thread" | ||
| >( | ||
| initialContextMode === "reuse_thread" || initialThreadId |
There was a problem hiding this comment.
[P1] Preserve explicit fresh-thread mode
The duplicate route can provide context_mode=fresh_thread_per_run together with a stored thread_id, but this condition converts that combination to reuse_thread. Submitting the duplicate then changes it from isolated runs to reusing an existing conversation. Give an explicit context_mode precedence and infer reuse from thread_id only when the mode parameter is absent.
There was a problem hiding this comment.
Addressed both P1s:
- Duplicate no longer serializes the prompt (or other task contents) into the query string. It now navigates with
?from=<taskId>and hydrates the create form from sessionStorage, falling back to the scheduled-tasks list cache/API. - Explicit
context_modetakes precedence overthread_id. Duplicating afresh_thread_per_runtask that still has a storedthread_idstays on isolated runs instead of being coerced toreuse_thread.
Also merged origin/main (copy-data cache + sandbox timeout) so this PR is no longer DIRTY.
| "title", | ||
| `${selectedTask.title}${st.actions.duplicateTitleSuffix}`, | ||
| ); | ||
| params.set("prompt", selectedTask.prompt); |
There was a problem hiding this comment.
[P1] Keep task prompts out of URLs
Serializing the complete prompt into the query string exposes task contents in browser history and Nginx access logs. Prompts have no length limit, so a valid long prompt can also exceed request-line limits and make Duplicate fail. Navigate with the task ID and fetch the source task, or use navigation/session state that does not put user content in the URL.
There was a problem hiding this comment.
Addressed both P1s:
- Duplicate no longer serializes the prompt (or other task contents) into the query string. It now navigates with
?from=<taskId>and hydrates the create form from sessionStorage, falling back to the scheduled-tasks list cache/API. - Explicit
context_modetakes precedence overthread_id. Duplicating afresh_thread_per_runtask that still has a storedthread_idstays on isolated runs instead of being coerced toreuse_thread.
Also merged origin/main (copy-data cache + sandbox timeout) so this PR is no longer DIRTY.
Honor an explicit context_mode when duplicating, and pass the source task id plus sessionStorage instead of serializing the prompt into the query string.
…llout
The describe("describeSchedule") block (once/daily en+zh, weekly en+zh,
weekly-no-weekdays fallback, hourly, monthly, custom) was accidentally
deleted during a rebase onto main while buildOnceRunAtLocal was extracted.
Restore it so the locale branches of describeSchedule stay covered.
3 participants
概述
前端 UI 重构（含经典皮肤外观）。
改动内容
侧边栏精简
设置面板重构
定时任务页重设计
其他
截图
验证
相关 PR