feat(frontend): extra features - chats settings, zh skill localization, zh about, scheduled-tasks loading - #4922
Conversation
github-actions
Bot
added
area:frontend
65bd700 to
550aa16
Compare
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at 550aa16 (re-reviewed after the five follow-up commits; the earlier timezone-list regression, optional once-time midnight default, weakened memory import validation, stale e2e spec, and dead code have all been addressed — thanks). Two remaining findings, one functional: the zh skill localization replaces skill.name, which is also the identifier the enable/disable API consumes, so toggling skills breaks in zh-CN. Also note this PR still carries the full globals.css palette rewrite and message-bubble restyle despite being scoped as the non-skin split, which will make it conflict-prone with the observatory-skin PR.
| () => | ||
| skills | ||
| .filter((skill) => skill.category === filter) | ||
| .map((skill) => localizeSkill(skill, locale)), |
There was a problem hiding this comment.
Bug: localizeSkill replaces skill.name with the Chinese display name (e.g. deep-research -> 深度研究), but name is the skill identifier downstream: the switch calls enableSkill({ skillName: skill.name, ... }) (line 244) and the list uses it as the React key (line 228). In zh-CN, toggling any built-in skill sends the localized display name to the backend, so the enable/disable request fails silently (useEnableSkill has no onError handling, and the switch only refreshes via invalidation on success). Keep the original name for identity and localize only for display, e.g. return { ...skill, displayName: zh.name, description: zh.description } from localizeSkill and render displayName in ItemTitle.
| <div className="text-muted-foreground flex h-40 items-center justify-center text-sm"> | ||
| {t.common.loading} | ||
| </div> | ||
| ) : filteredData.length === 0 ? ( |
There was a problem hiding this comment.
Nit: when loading fails, the queryError banner renders and then this branch renders too — data is empty (so hasTasks is false and isLoading is false), leaving "Failed to load scheduled tasks" directly above the "No scheduled tasks yet" empty state and its create button. Gate the empty states on !queryError so a load failure doesn't also advertise an empty list.
There was a problem hiding this comment.
Fixed — the empty states are now gated on !queryError, so a load failure no longer also shows the empty list.
550aa16 to
7a24983
Compare
|
Thanks — addressed the two remaining findings:
On the scope note (the PR still carrying the |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-review at 7f8b1f3, scoped to this PR's increment over its base #4914 (chats settings page, zh skill localization, zh about content, plus the scheduled-tasks error/empty-state fix). Both previously reported findings are verified fixed at this head — localizeSkill now keeps skill.name for identity and adds displayName, and the empty states are gated on !queryError. Three new findings below, the first one functional: the chats page's load-more-to-search affordance is unreachable exactly when it's needed (search matching nothing in the loaded pages), plus a suggestion on the hardcoded zh skill descriptions and a copy nit.
| <MessagesSquare className="text-muted-foreground/50 size-8" /> | ||
| <div> | ||
| <div className="text-sm font-medium"> | ||
| {isSearching ? t.chats.noMatches : t.chats.emptyTitle} |
There was a problem hiding this comment.
Functional gap: search only covers already-loaded pages, and the "Load more to search older conversations" button (lines 157-170) is rendered only inside the filteredThreads.length > 0 branch. When a query matches nothing in the loaded pages — exactly the case where older, not-yet-loaded conversations might match — this empty branch shows "No matching conversations" with no way to load more, making those conversations unreachable via search. Render the load-more row in this branch too when hasNextPage && isSearching, or hoist it out of the filteredThreads.length > 0 conditional.
There was a problem hiding this comment.
Fixed in 78836eb — the load-more button is now rendered in the empty search branch when hasNextPage && isSearching.
| <div className="text-muted-foreground mt-0.5 flex items-center gap-2 text-xs"> | ||
| <span> | ||
| {thread.updated_at | ||
| ? formatTimeAgo(thread.updated_at) |
There was a problem hiding this comment.
Nit: when updated_at is missing, the meta row renders t.pages.untitled ("Untitled") in the timestamp slot — a title string where a relative time is expected. "—" (or hiding the span) reads better.
There was a problem hiding this comment.
Fixed in 78836eb — a missing updated_at now renders an em dash instead of Untitled.
| if (!locale.startsWith("zh")) return skill; | ||
| const zh = SKILL_ZH[skill.name]; | ||
| if (!zh) return skill; | ||
| return { ...skill, displayName: zh.name, description: zh.description }; |
There was a problem hiding this comment.
Suggestion: localizeSkill hard-overrides description with the frozen copy in SKILL_ZH, so zh-CN users always see this hardcoded text even after the backend's skill descriptions change, while en-US users see the live description. It would also replace the description of any non-built-in skill whose name happens to collide with a map key. Consider localizing only the display name here and letting the description come from the backend (or driving both from skill metadata) so the map can't drift.
There was a problem hiding this comment.
Fixed in 78836eb — SKILL_ZH is now a name-only map and localizeSkill localizes only displayName; the description always comes from the backend.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-review at 78836eb. All previously flagged items are verified fixed at this head: localizeSkill keeps skill.name for identity and only overrides displayName, the scheduled-tasks empty states are gated on !queryError, the load-more-to-search button now renders in the empty-search branch, a missing updated_at renders an em dash, and SKILL_ZH no longer freezes the description copy. Three new findings below — the main one is that removing the sidebar Chats link leaves the existing /workspace/chats page unreachable.
| </Link> | ||
| </SidebarMenuButton> | ||
| </SidebarMenuItem> | ||
| <SidebarMenuItem> |
There was a problem hiding this comment.
This removes the last navigation entry to /workspace/chats, but the page itself (frontend/src/app/workspace/chats/page.tsx, with its virtualized list and channel badges) is still registered at this head — I can't find any other link to it (the header, command palette, and /workspace redirect all target /workspace/chats/new or thread pages). It's now orphaned dead UI duplicating the new Chats settings page. Either delete the page (and its ChatsPage imports) or redirect it somewhere reachable, so the two surfaces can't drift apart.
There was a problem hiding this comment.
Fixed in f78c2f8: frontend/src/app/workspace/chats/page.tsx (and its ChatsPage imports) is deleted, so the Chats settings page is now the single surface for the conversation list.
| </div> | ||
| ) : filteredThreads.length > 0 ? ( | ||
| <div className="overflow-hidden rounded-lg border"> | ||
| {filteredThreads.map((thread) => { |
There was a problem hiding this comment.
Suggestion: this renders every loaded thread as a plain DOM row, while the /workspace/chats page it replaces used VirtualThreadList for the same data. With the 50-thread pages auto-loading via the sentinel, a user who scrolls deep accumulates unbounded rows inside the dialog's ScrollArea. Rows are light so this is fine for the first few pages, but if this page is meant to fully replace the old one, consider capping auto-loading or reusing the virtualized list.
There was a problem hiding this comment.
Fixed in f78c2f8: chats-settings-page.tsx now reuses VirtualThreadList (the same virtualizer the old page used) inside the dialog ScrollArea, so deep scrolls do not accumulate unbounded DOM rows.
| <div className="flex size-full flex-col"> | ||
| <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 |
There was a problem hiding this comment.
Nit: the icon-only back button has no accessible name, so screen readers announce it as just "button". Add aria-label={t.common.back} (or the equivalent existing common key) to match the other icon buttons in the codebase.
There was a problem hiding this comment.
Fixed in f78c2f8: the icon-only back button now carries aria-label={t.common.close} (the same common key used by sibling icon buttons), so screen readers announce it.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-review at f78c2f8. All three findings from the last pass are verified fixed: /workspace/chats is deleted with no remaining in-app links to it (only thread-detail routes still reference the path prefix), the back button now has an accessible name via the existing t.common.close key, and the chats settings page now uses VirtualThreadList with a matching estimateSize (56 = min-h-14) and the correct scroll parent for the dialog's ScrollArea viewport. Two new findings below, both fallout from the page deletion: the e2e suite still navigates to the deleted route (CI-breaking, and it also loses the issue #3482 regression coverage since the replacement page has no testids), plus a small border seam nit.
| </div> | ||
| ) : filteredThreads.length > 0 ? ( | ||
| <div className="overflow-hidden rounded-lg border"> | ||
| <VirtualThreadList |
There was a problem hiding this comment.
Deleting /workspace/chats orphans the e2e specs that drive it — neither file is in this PR's diff, and Playwright's testDir is ./tests/e2e, so CI will hit a 404 here: frontend/tests/e2e/thread-list-infinite-scroll.spec.ts:36 and :96 both page.goto("/workspace/chats") and additionally assert the chats-page-sentinel / chats-page-load-more testids that only existed on the deleted page, and frontend/tests/e2e/thread-history.spec.ts:751 and :789 do the same for the "chats list page shows all threads" and "IM channel threads show their source" tests. Since this settings page is the replacement surface, repointing those specs here would also keep the issue #3482 regression coverage (sentinel-driven infinite scroll, no auto-pagination while searching) — the new page currently has no testids, so that coverage silently disappears. Alternatively, keep a redirect at /workspace/chats if you'd rather leave the specs untouched.
There was a problem hiding this comment.
Fixed in 9fcc2ff — both e2e specs were repointed to the replacement surface: they now goto("/workspace/settings?settings=chats") and target the new chats-settings-page / chats-settings-sentinel / chats-settings-load-more testids added to ChatsSettingsPage, preserving the issue #3482 sentinel-driven coverage.
| <Link | ||
| key={thread.thread_id} | ||
| href={pathOfThread(thread)} | ||
| className="hover:bg-secondary/50 flex min-h-14 items-center gap-3 border-b px-4 transition-colors" |
There was a problem hiding this comment.
Nit: the pre-virtualizer rows ended with last:border-b-0; without it every row keeps a bottom border, so the final row's border sits directly on the container's own bottom border (a 1px double seam at the bottom of the card). last: can't work reliably once the list virtualizes (the last rendered element is only the last item when scrolled to the bottom), so if the seam is noticeable the container border is the side to adjust rather than the rows.
There was a problem hiding this comment.
Fixed in 9fcc2ff — with the list now virtualized, last: is unreliable, so the container's own bottom border was dropped instead; the final row's border-b now sits flush against the card edge with no double seam.
798f5ea to
eed2c43
Compare
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-review at eed2c43. The previous findings are verified fixed at this head: the e2e specs are repointed off the deleted route, the back button carries aria-label={t.common.close}, and the list container now uses border-x border-t so there is no double bottom seam (the dialog's ScrollArea viewport also correctly serves as the virtual list's scroll parent via closest('[data-slot="scroll-area-viewport"]')). One new finding, unfortunately CI-breaking in the same way as the one it was meant to fix: the repointed specs navigate to /workspace/settings?settings=chats, but no such route exists — the deep link only mounts via WorkspaceContent on real workspace routes — so all four repointed tests will hit Next's 404 page and time out. Anchored on both spec files below; pointing them at an existing workspace route with the query param (e.g. /workspace/chats/new?settings=chats) or adding a real /workspace/settings route resolves it.
| mockLangGraphAPI(page, { threads: THREADS }); | ||
|
|
||
| await page.goto("/workspace/chats"); | ||
| await page.goto("/workspace/settings?settings=chats"); |
There was a problem hiding this comment.
CI-breaking: page.goto("/workspace/settings?settings=chats") targets a route that does not exist. frontend/src/app/workspace/ only contains page.tsx (which redirects to /workspace/chats/new), agents/, chats/[thread_id]/, and scheduled-tasks/ — there is no settings/ segment and no rewrite for it in next.config.js. Next.js serves the 404 page, so WorkspaceContent (and with it WorkspaceSettingsDeepLink, the only thing that turns ?settings= into an open dialog) never mounts; both tests in this file then time out waiting for chats-settings-page, i.e. the same failure mode the repoint was meant to remove. Navigate to an existing workspace route that mounts the deep link instead, e.g. /workspace/chats/new?settings=chats (line 96 needs the same change; the IM-channel test in thread-history.spec.ts already proves /workspace/chats/new works under mockLangGraphAPI), or add a real /workspace/settings page so the URL exists.
There was a problem hiding this comment.
Fixed in 6d20129. Both chats-settings gotos in this spec now navigate to /workspace/chats/new?settings=chats so WorkspaceSettingsDeepLink can mount on an existing workspace route.
| mockLangGraphAPI(page, { threads: THREADS }); | ||
|
|
||
| await page.goto("/workspace/chats"); | ||
| await page.goto("/workspace/settings?settings=chats"); |
There was a problem hiding this comment.
Same issue as thread-list-infinite-scroll.spec.ts: both goto("/workspace/settings?settings=chats") calls here (lines 751 and 789) hit a route that does not exist under frontend/src/app/workspace/, so these two tests 404 before the settings dialog can open and time out on chats-settings-page. Same fix — navigate to an existing workspace route carrying the query, e.g. /workspace/chats/new?settings=chats (the /workspace/chats/new goto at line 769 above already works under this mock setup).
There was a problem hiding this comment.
Fixed in 6d20129. Both gotos here now use /workspace/chats/new?settings=chats instead of the non-existent /workspace/settings path.
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
…in reuse-thread e2e The dialog overlay intercepts the create-toggle click; press Escape and await dialog dismissal first.
dbe679f to
bd995ff
Compare
… 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.
bd995ff to
6d20129
Compare
…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.
6d20129 to
44fbf0d
Compare
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.
44fbf0d to
7644a11
Compare
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.
7644a11 to
4ea6068
Compare
# Conflicts: # frontend/src/components/workspace/input-box.tsx
4ea6068 to
46b3551
Compare
Honor an explicit context_mode when duplicating, and pass the source task id plus sessionStorage instead of serializing the prompt into the query string.
46b3551 to
f9e2029
Compare
f9e2029 to
5de2735
Compare
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-review at 5de2735 (after the rebase onto origin/main and the observatory-skin layer being merged into this branch). All previously flagged items are verified fixed at this head: both chats-list e2e specs now navigate to /workspace/chats/new?settings=chats, which is a real route — /workspace/chats/[thread_id] matches new, and the workspace layout mounts WorkspaceContent → SettingsDialogHost + WorkspaceSettingsDeepLink, so the dialog does open on the chats section (testids chats-settings-page / chats-settings-sentinel / chats-settings-load-more all exist). The earlier fixes also survived the rebase: localizeSkill keeps skill.name as identity and only overrides displayName, the scheduled-tasks empty states are still gated on !queryError, the chats list still virtualizes via VirtualThreadList, and no references remain to the deleted /workspace/chats list route. The rebase itself is clean — no conflict markers in any changed file, the restored isLoading definition is in place, and en-US/zh-CN key sets are identical. Two new findings below: a test-coverage regression (the whole describeSchedule block was dropped while the function is still rendered in the UI) and a scope note — this head now carries every commit of #4915 on top of #4914's head, so this "non-skin extras" PR is a strict superset of both sibling PRs.
| weekdays: [], | ||
| dayOfMonth: 1, | ||
| } as CronParts; | ||
| describe("buildOnceRunAtLocal", () => { |
There was a problem hiding this comment.
Coverage regression: this diff deletes the entire describe("describeSchedule") block (the once/daily/weekly en+zh, weekly-no-weekdays fallback, hourly, monthly, and custom tests) and drops describeSchedule from the imports — but the function is still exported from frontend/src/core/scheduled-tasks/cron.ts and still rendered in the UI (scheduled-task-schedule-input.tsx renders describeSchedule(...) in the schedule-preview row). Its locale branches (每天 09:00, 每周 周一、周二 …, 每小时第 N 分钟) are exactly the kind of string formatting that regresses silently without tests. Looks like rebase fallout rather than an intentional removal — please restore the block (only the buildOnceRunAtLocal suite is new here).
There was a problem hiding this comment.
Fixed - the full describe(describeSchedule) block (once/daily/weekly en+zh, weekly-no-weekdays fallback, hourly, monthly, custom) was restored in 78dd7a5 after being dropped during the rebase onto main; the pushed branch head now includes it.
| @@ -0,0 +1,18 @@ | |||
| export { | |||
There was a problem hiding this comment.
Scope note: this head now contains the complete observatory skin layer — every commit of #4915, up to and including 3f8d266d, which is #4915's current head — layered on top of #4914's head (05a1fe0e). So this PR, described as the non-skin extras split out to shrink the other two, is now a strict superset of both siblings: merging any one of #4914 / #4915 / #4922 lands the other two's content and leaves them to merge empty or conflict. Either drop the observatory commits back onto #4915 (this file, core/skins/*, components/workspace/skins/*, and the obs-* globals.css block are the boundary), or update this PR's description and close the superseded PRs so it's clear which one is meant to be reviewed.
…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.
…zation, zh about, scheduled-tasks loading state
…ay; use startsWith for zh locale detection
…ing updated_at, allow load-more on empty search
…ettings, add aria-label to back button
The /workspace/chats route is gone (single surface is now the Chats settings page). Add data-testids to ChatsSettingsPage (page/sentinel/load-more), allow ?settings=chats deep link, and update the two e2e specs that drove the deleted page so issue bytedance#3482 coverage is preserved. Also drop the list container bottom border so the final row's border-b no longer forms a double seam.
/workspace/settings does not exist, so chats-list e2e was 404ing before WorkspaceSettingsDeepLink could mount. Navigate to /workspace/chats/new?settings=chats instead.
5de2735 to
ff0e471
Compare
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-review at ff0e471. This head is content-identical to 5de2735 for every file this PR owns (checked byte-for-byte across all nine of the PR's own files), so the rebase is clean and nothing regressed: it now sits on 78dd7a5, which puts the restored describe("describeSchedule") block in the ancestry, and the observatory-skin commits are stripped, so this is once again the non-skin extras PR. Both findings from the last pass are resolved at this head — en-US/zh-CN key sets are still identical, the e2e chats-list specs still goto /workspace/chats/new?settings=chats, localizeSkill still keeps skill.name as identity and only overrides displayName, and the scheduled-tasks empty states are still gated on !queryError. Two new functional findings in ChatsSettingsPage (the dialog stays open after clicking a conversation; dropping buildThreadListModel loses cross-page dedup and pinned-first ordering) plus one orphaned-i18n-key nit.
| renderItem={(thread) => { | ||
| const channelSource = channelSourceOfThread(thread); | ||
| return ( | ||
| <Link |
There was a problem hiding this comment.
Functional bug: the rows navigate with a plain <Link> and nothing closes the settings dialog, so clicking a conversation routes to /workspace/chats/[thread_id] with the dialog still modal on top of it.
SettingsDialogHost is mounted once at the workspace root (src/app/workspace/workspace-content.tsx:44, reached through app/workspace/layout.tsx), so it survives client-side navigation, and the store in settings-dialog-store.ts has no route listener — the only close path is onOpenChange, i.e. the user dismissing it by hand. Since opening a conversation is this page's primary action, the user lands on the thread with a full-screen modal still covering it.
The sibling flows in this same PR close first: SkillSettingsPage.handleCreateSkill calls onClose?.() before router.push, and duplicateSelectedTask on the scheduled-tasks page calls setDetailOpen(false) before router.push. Suggest wiring the same here — pass onClose={() => props.onOpenChange?.(false)} at the activeSection === "chats" render site in settings-dialog.tsx (exactly as SkillSettingsPage already receives it) and call it from the row's onClick.
There was a problem hiding this comment.
Fixed in 66c8db5 — ChatsSettingsPage now accepts an onClose prop, wired to props.onOpenChange?.(false) at the activeSection === "chats" render site in settings-dialog.tsx (mirroring SkillSettingsPage), and the thread row's Link calls onClose?.() before navigating, so the modal is dismissed before the thread route opens.
| refetch, | ||
| } = useInfiniteThreads(); | ||
| const threads = useMemo( | ||
| () => infiniteThreads?.pages.flat() ?? [], |
There was a problem hiding this comment.
This reimplements what buildThreadListModel already does, and drops two behaviours the deleted /workspace/chats page got from it:
(a) No cross-page dedup. useInfiniteThreads paginates by a running offset (getInfiniteThreadsNextPageParam sums the allPages lengths) over a list ordered updated_at desc, so any thread created or updated between two page fetches shifts the window and repeats at the next page boundary. pages.flat() keeps the duplicate, and VirtualThreadList sets getItemKey: (index) => items[index]?.thread_id ?? index, so a repeat produces duplicate React keys and the conversation rendered twice in the list. buildThreadListModel's byId map exists precisely to collapse that.
(b) Pinning is ignored. The model returns sortPinnedThreads(threads), so pinned conversations float to the top; this sort is updated_at only, so a pinned thread with an older updated_at sinks below recent ones — inconsistent with recent-chat-list.tsx, which still goes through the model.
Suggest const model = useMemo(() => buildThreadListModel(infiniteThreads?.pages ?? []), [infiniteThreads]) and filtering model.threads, keeping the search filter and this sort only as a tie-break within the unpinned group.
There was a problem hiding this comment.
Fixed in 66c8db5 — the page now uses buildThreadListModel(infiniteThreads?.pages ?? []): the byId map collapses cross-page duplicates (no duplicate React keys), and the model's pinned-first ordering is preserved. The updated_at sort is kept only as a tie-break within the unpinned group, so pinned conversations stay on top.
| @@ -628,11 +629,17 @@ export const enUS: Translations = { | |||
|
|
|||
| // Chats | |||
| chats: { | |||
There was a problem hiding this comment.
Nit: with /workspace/chats deleted in this PR, pages.chats is now unreferenced in both locales (en-US.ts:712, zh-CN.ts:683) — its only consumer was the deleted page's document.title effect. sidebar.chats was correctly removed in the same change, so this one looks like it slipped through; the Translations type will keep compiling either way, so nothing flags it.
There was a problem hiding this comment.
Fixed in 66c8db5 — the unreferenced pages.chats key is removed from en-US.ts, zh-CN.ts, and the Translations type (types.ts).
- close the settings dialog when navigating to a thread from the chats list - use buildThreadListModel so cross-page duplicates collapse and pinned threads stay on top; keep updated_at sort as a tie-break within the unpinned group - remove the now-orphaned pages.chats i18n key (en-US, zh-CN, types)
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at 5d6074c. The increment over ff0e471 (chats-settings onClose wiring, buildThreadListModel reuse with pinned-first ordering, and the pages.chats key removal from en-US/zh-CN/types) is all verified in place and correct. Three new findings below, all in code outside that increment: the memory settings page no longer renders or searches stored summaries, the isMemorySummaryEmpty -> hasAnyMemorySummary swap drops a trim, and the duplicate-create page's schedule-input remount key changes on every schedule edit.
| <div> | ||
| {filteredFacts.length === 0 ? ( | ||
| <div className="text-muted-foreground text-sm"> | ||
| {normalizedQuery ? noMatches : noFacts} |
There was a problem hiding this comment.
Functional regression: normalizedQuery is now matched against memory.facts only, but the input above is still labelled t.settings.memory.searchPlaceholder ("Search memory") and this branch reports t.settings.memory.noMatches ("No matching memory found."). The stored summaries (workContext / personalContext / topOfMind / recentMonths / earlierContext / longTermBackground) are no longer rendered or searched anywhere — hasAnyMemorySummary is now their only consumer. On main the page rendered them via buildMemorySectionGroups + summariesToMarkdown inside SafeStreamdown and included them in the search through filteredSectionGroups, so a query that matches only a summary now produces a false "no matching memory found", and the summaries themselves are unreachable in the UI short of an export.
If dropping the summaries view is intentional, please scope the search copy to facts (e.g. "Search facts" / "No matching facts") and call the removal out in the PR description — it is well outside the scope this PR advertises. Otherwise the read-only summaries block needs restoring.
| ) : ( | ||
| <div className="space-y-4"> | ||
| {isMemorySummaryEmpty(memory) && memory.facts.length === 0 ? ( | ||
| {memory.facts.length === 0 && !hasAnyMemorySummary(memory) ? ( |
There was a problem hiding this comment.
Behaviour change versus the isMemorySummaryEmpty this replaces: the old helper trimmed before comparing (summary.trim() === ""), while hasAnyMemorySummary treats a whitespace-only summary as content. A memory whose six summaries are all blank strings now skips the memoryFullyEmpty empty state and just shows "No saved facts yet." with no explanation of where the rest of the memory went.
Keeping the old semantics is a one-liner:
function hasAnyMemorySummary(memory: UserMemory): boolean {
return [
memory.user.workContext.summary,
memory.user.personalContext.summary,
memory.user.topOfMind.summary,
memory.history.recentMonths.summary,
memory.history.earlierContext.summary,
memory.history.longTermBackground.summary,
].some((summary) => summary.trim() !== "");
}| key={ | ||
| sourceTaskId | ||
| ? `${sourceTaskId}:${createSchedule.schedule_type}:${JSON.stringify(createSchedule.schedule_spec)}` | ||
| : "new" |
There was a problem hiding this comment.
This remount key changes on every schedule_spec edit, so the one-time date fields lose focus mid-edit in the duplicate flow. ScheduledTaskScheduleInput emits onChange from its effect on every internal change (including onceYear/onceMonth/onceDay/onceTime), and the parent stores that straight into createSchedule. Once year/month/day/time form a valid run_at, every subsequent keystroke produces a new schedule_spec JSON -> a new key -> React unmounts and remounts the input, replacing the DOM node the user is typing into. Editing a valid ...-15T09:30 to ...-15T09:31, or clearing the day (which flips the spec back to {}), drops focus after a single keystroke.
Note this only bites when sourceTaskId is set — the plain create path uses the constant "new" key and is unaffected.
initial is only read inside useState initializers, which is why the key hack exists. A cheaper form is to key on the draft identity plus an explicit nonce that applyDraft bumps once, so the remount happens exactly when a draft lands instead of on every field edit.
2 participants
概述
从 PR #4914 / #4915 中拆分出的非皮肤功能，独立成 PR 以便聚焦 review。
包含功能
说明