refactor(desktop): let the import page name the conversation it lost track of - #3075
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughSummaryThis PR refactors the Desktop import tasks page without changing import behavior.
The PR extends the existing import task state and action flow. It does not create a parallel import path or change the import source of truth. The solution is the smallest coherent change. Conversation metadata and adapter-aware matching are necessary for accurate import tracking. Page-owned state is necessary because React 19 transitions delayed the lock when using Astryx’s Reported validation includes formatting, linting, Desktop type checking, Storybook build, and Storybook smoke checks. Storybook states were also verified manually. Repository-wide tests and end-to-end tests were not run. Deferred-import interaction tests are unavailable because the repository lacks jsdom and React Testing Library, the fake DOM does not mount Astryx, and visual smoke tests disable Storybook play functions. Required-check status remains unverified without direct check evidence. Review-relevant risksNo protected-area effect was identified in the current diff. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe import page now tracks active and uncertain imports by adapter, source session, and conversation name. It disables matching rows and catalog controls, displays affected conversation names in English and Chinese, and clarifies Codex source-file behavior. ChangesExternal import outcomes
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized import-page update improves status messaging and disables conflicting controls while imports are active. The reported interaction risks are addressed, and no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
605f1a8 to
b8254eb
Compare
…track of 设置 · 活动 · 导入任务 has one action, and taking it ends the page: a successful import closes Settings and navigates to the new task. The one state that outlives that action is an import Desktop Main could neither confirm nor fail, and the page could not say which conversation it belonged to. It kept only the source id, so the warning banner had to speak in the abstract — 查找这个对话, with no way to say which one. Pointing at the row was not an option either: by the time the banner renders, the archived filter or a new page may have taken that row away. Carry the record instead of the id. `uncertainImports` holds the conversation's name and its adapter, so the banner names every import whose outcome is unknown, and it stays true when the row is gone. The adapter is part of the record because a source-native id is unique only within its own source; matching on a bare id would eventually mark an unrelated row in another source. 加载更多 was a centred ghost label under the list, which read as a caption rather than the control that extends it. It is now a full-width secondary button — the shape Astryx documents `width="100%"` for — and it uses `clickAction` like the row buttons, so repeat clicks are dropped by the promise rather than by state that lands one render later. The no-source empty state repeated its own title. It now says what to do about it instead, and keeps the read-only promise that earns the permission to look at another app's files. Not fixed, deliberately: the page still cannot tell you a conversation has already been imported. A Maka session records nothing about the external conversation it came from, and a successful import unmounts this page, so no page-local memory survives to accumulate. The list keeps the one honest sentence it can — importing the same conversation again creates an independent task — and #3081 tracks the provenance change a per-row answer would need. Generated-by: Claude Code
b8254eb to
ba2e67f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 91484343-88cf-4454-b7d6-c815c77de55c
📒 Files selected for processing (2)
apps/desktop/src/renderer/locales/external-session-import-copy.tsapps/desktop/src/renderer/settings/import-tasks-settings-page.tsx
…light The archived filter is already frozen during an import because either control replaces the catalog, which takes away the row the in-flight import belongs to while every remaining row is still disabled by `importingId !== null` — a page of dead buttons with the one spinner gone. The source switch does exactly the same thing and was not frozen. It also removes the collision `importingId` is open to: it holds a bare source id, and those are unique only within their own source, so a second adapter carrying the same id could show 正在导入… on an unrelated row. Freezing the switch closes that at the source rather than widening the key, which is the smaller change and the one that also keeps the row visible. No user-visible change today: the switch only renders when more than one adapter is detected, and Codex is the only one. Generated-by: Claude Code
M4n5ter
left a comment
There was a problem hiding this comment.
English
Blocking: the page-level import lock never becomes active
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx:275
The new source-selector guard depends on importingId !== null, but imports are started through Astryx Button.clickAction. Button executes that callback inside a React 19 asynchronous transition, so the initial setImportingId(session.id) at line 174 is deferred until the import Promise settles. Before that transition commits, the finally block queues setImportingId(null) at line 198.
Consequently, the page never observes a non-null importingId while the import is pending. I reproduced this with the repository's actual Astryx Button and React 19.2.8:
importingId while pending: null
action button aria-busy: true
source control disabled: false
The clicked button protects itself through its internal pending state, but the source selector, archived filter, and other import rows remain interactive. A user can therefore replace the catalog during an import or start another import—the exact state inconsistency this revision is intended to prevent. This also means the previously resolved cross-adapter review thread is not actually addressed.
Please move ownership of the page-level import lock outside the clickAction transition. One small approach is to use a synchronous ref as the mutual-exclusion guard and start the import from a normal onClick, allowing setImportingId to commit immediately while retaining same-tick duplicate protection.
Please also add a deferred-import interaction test that keeps the import Promise unresolved and verifies that the source selector, archived filter, and every other import row remain unavailable until the Promise settles.
简体中文
Blocking：页面级导入锁从未真正进入 active 状态
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx:275
新增的来源选择器禁用逻辑依赖 importingId !== null，但导入操作是通过 Astryx 的 Button.clickAction 启动的。Button 会在 React 19 的异步 transition 中执行该回调，因此第 174 行的 setImportingId(session.id) 会被延迟到导入 Promise 结束后才提交；在该 transition 提交前，第 198 行的 finally 又会排入 setImportingId(null)。
因此，在导入处于 pending 状态期间，页面从未观察到非空的 importingId。我使用仓库实际的 Astryx Button 和 React 19.2.8 进行了复现：
pending 期间的 importingId：null
操作按钮 aria-busy：true
来源控件 disabled：false
被点击的按钮会通过自身的 pending 状态保护自己，但来源选择器、已归档筛选器以及其他导入行仍然可以操作。用户因此可以在导入期间替换 catalog，或者启动另一项导入——这正是本次修改声称要消除的状态不一致。这也意味着之前已 resolve 的跨 adapter review thread 实际上尚未解决。
请将页面级导入锁的所有权移出 clickAction transition。一个较小的方案是：使用同步 ref 作为互斥 guard，并从普通 onClick 启动导入，使 setImportingId 能立即提交，同时保留对同一 tick 内重复点击的防护。
还请增加一条 deferred-import 交互测试：让导入 Promise 保持 unresolved，并验证在 Promise settle 之前，来源选择器、已归档筛选器和其他所有导入行均不可操作。
The row import button ran `importConversation` through Astryx's `clickAction`, which Astryx executes inside a React 19 async transition. React holds a transition's state updates until the action settles, so `setImportingId` committed only after the import was already over. For the whole duration of an import the page therefore behaved as if none were running: the row kept reading 导入 instead of 正在导入…, the other rows stayed clickable, and both catalog controls stayed live -- including the archived filter, frozen for that purpose since #3033, and the source switch frozen in 552156f. A second import could be started, or the catalog replaced out from under the in-flight one. `clickAction` earns the clicked button its own pending state and nothing more. This lock is a page fact, so the page owns it: plain `onClick` runs outside the transition and `importingId` commits before the next event can be dispatched. `加载更多` moves back for the same reason -- inside the transition `loadingMore` could neither disable it nor say 正在加载…. Verified in Storybook against a never-settling import: with `onClick` the source switch, the archived filter and every other row are inert until it settles, and a second click on any of them is dropped. Generated-by: Claude Code
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx (1)
399-408: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDisable all catalog controls during an active import.
isDisabled={loadingMore}leaves “加载更多” active whileimportingId !== null. The catalog retry button at Line 300 through Line 304 has the same gap. If a load-more request failed, retry callsloadCatalogwithout a cursor. Line 125 then clearscatalog, so the active import row can disappear before the import settles.Add
importingId !== nullto both controls.Proposed fix
<Button variant="ghost" size="sm" label={copy.retry} + isDisabled={importingId !== null} onClick={() => void loadCatalog(adapterId)} />- isDisabled={loadingMore} + isDisabled={loadingMore || importingId !== null}
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d854fdb2-ee42-4596-bc81-a085a6197793
📒 Files selected for processing (1)
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
EnglishConfirmed. Reproduced against the repository's own Astryx Two corrections to the framing. The defect predates this PR: Fixed in 771e69b: Then the page changed shape, and it is worth saying why, because what you reviewed is no longer what is there. Making the lock work exposed what it was for. This PR had already made the opposite choice one state over: The case those freezes existed for now passes without them: with an import still in flight, turning the archived filter off reloads the catalog and takes its row away, and the banner still names it while every remaining row stays disabled. I did not add the synchronous ref. Measured on the fixed build: one macrotask after the first click — the earliest a second real click can arrive — both row buttons already carry On the deferred-import test: there is no seam that runs it in CI today. The repository has no jsdom and no So the test is not something I can add without first building one of those. My preference is a renderer component-test harness that can mount Astryx, because this bug class — a design-system prop whose pending contract is button-local — will reach other pages. I would rather do that as its own change than fold new test infrastructure into this one, but I will do it here if you want the fix and its test to land together. Your call. 简体中文已确认。 使用仓库自身的 Astryx 有两处需要更正。该缺陷早于本 PR：行按钮上的 已在 771e69b 修复： 之后页面的形状变了，这里有必要说明原因，因为你评审过的版本已经不是现在的版本。 把锁修好之后，才看清它是为什么存在的。 而本 PR 在相邻的另一个状态上早已做过相反的选择： 那些冻结原本要防的场景，现在没有冻结也成立：导入仍在进行时关闭归档筛选，目录重载并带走该行，横幅依然指名该对话，其余每一行仍为禁用。 我没有加同步 ref。 在修复后的构建上实测：第一次点击后的下一个 macrotask（真实第二次点击最早的到达时机），两个行按钮已带 关于 deferred-import 测试：目前仓库没有能在 CI 中运行它的接缝。 没有 jsdom，也没有 因此这条测试我无法在不先搭建其中之一的前提下加上。我倾向于建一个能 mount Astryx 的 renderer 组件测试 harness，因为这类缺陷——设计系统某个 prop 的 pending 契约仅限按钮自身——还会出现在其他页面。我更希望把它作为独立改动，而不是把新的测试基建塞进本 PR；但如果你希望修复与测试一起落地，我也可以在这里做。由你决定。 |
重试 on the catalog error banner calls `loadCatalog` without a cursor, which resets the catalog to page one. Clicking it during an import takes away the row that import belongs to -- the same hole the archived filter and the source switch are frozen against, at a third control that was missed because each of them carried the rule separately. The rule now has a name. `catalogFrozen` says once why an in-flight import freezes the catalog, and the controls that can replace it read it. 加载更多 appends and would keep the row, but it is frozen too: a page where every control but one holds still has to be read control by control before it can be trusted, and an import settles in well under a second. Reported by CodeRabbit. Verified in Storybook with a failing `list` and a never-settling `import`: 重试 is live while nothing is importing and disabled once something is, alongside 加载更多, the filter and the switch. Generated-by: Claude Code
|
@coderabbitai Both real, fixed in 861fdcc. 重试 is the one that matters: it calls So the fix names the rule rather than adding a fourth copy of it: 加载更多 I fixed for a different reason than the one given. It appends, so it would keep the row and the invariant holds either way. It is frozen because a page where every control but one holds still has to be read control by control before it can be trusted, and nothing is lost by waiting out an import that takes well under a second. Verified in Storybook with a failing Superseded by 0f28ea0. Finding a third and fourth control carrying the same rule was the signal that the rule was in the wrong place. |
|
Tip For best results, initiate chat on the files or code changes.
Your The Storybook cases verify both required states. The findings are addressed. ✏️ Learnings added
🧠 Learnings usedYou are interacting with an AI system. |
… catalog `importingId` was a bare source id — a pointer into a list the page lets the user replace. Everything it fed said "which import is running", and all of that lived on the row, so the row had to be kept alive: the archived filter was frozen for it in #3033, the source switch in 552156f, 重试 and 加载更多 in 861fdcc. Four controls, one rule, added one review round at a time. This PR already made the same fix for the unconfirmed banner, whose ids had the same problem. Making it once more removes the reason for all four freezes: `activeImport` carries the adapter, the id and the name, so the page can say 正在导入「X」 above the list, where no catalog change can take it away, and name why every 导入 is disabled. The catalog is then free to move underneath an import. `catalogFrozen` and its four readers are gone, and so is the id collision the source-switch freeze was closing — the row predicate compares adapter and id together. `ImportAttempt` is one record type for both slots, since "which conversation, in which source" is the same question whether the import is running or came back unconfirmed. Verified in Storybook: with an import that never settles, filtering its row away leaves the banner naming it and every remaining row disabled; with the fixture's failing import, one name then two in the unconfirmed banner, both rows disabled, and the lock released in between. Generated-by: Claude Code
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
file="apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx"
ast-grep outline "$file" --items all --view expanded
printf '\n--- active import and catalog-control references ---\n'
rg -n -C 5 '\b(activeImport|catalogFrozen|importingId|loadCatalog|handleImport)\b' "$file"Length of output: 7067
The earlier ⛔ Learnings removedYou are interacting with an AI system. |
Summary
设置 · 活动 · 导入任务 shipped in #3033 as a relocation, not a design, and never got a pass over how it reads. The import behaviour itself does not change here.
The page has one action, and taking it ends the page: a successful import calls
closeSettings()+openSessionInChat()(app-shell.tsx:3408), so the page unmounts. Everything below follows from that.One thing runs through most of this: the page kept source ids where it needed records. A bare id is a pointer into a list the user is free to replace — filter it, switch source, retry a failed page — so everything built on one either went mute or had to be protected by freezing the list. Both places that held ids now hold
ImportAttempt({ adapterId, sourceSessionId, name }), because "which conversation, in which source" is the same question whether an import is running or came back unconfirmed.importingIdwas the same bare id, and it fed the only thing that said which import was running: the row's own 正在导入…. Keeping that readable meant keeping the row alive, so the archived filter was frozen for it in refactor(desktop): rebuild the task rail on one shape and one vocabulary #3033, the source switch in 552156f, and 重试 and 加载更多 in 861fdcc — four controls carrying one rule, added a review round at a time.activeImportcarries the record, so the page says 正在导入「X」 in the banner slot, where no catalog change can take it away, and says why every 导入 is disabled. The catalog is then free to move underneath an import and all four freezes are gone, along with the id collision the source-switch freeze was closing: the row predicate compares adapter and id together. (The freezes were raised by CodeRabbit across two rounds; this deletes them rather than completing the set.)加载更多is a full-widthsecondarybutton, not a centred ghost label that read as a caption under the list.width="100%"is the shape Astryx documents for this (Button.tsx:332-335,:543). No "N remaining" is offered: the adapter returns a cursor, not a total.clickActioninside a React 19 async transition, and React holds a transition's state updates until the action settles — so the state committed only after the import was over, and nothing on the page could tell one was running. PlainonClickruns outside the transition and commits before the next event can be dispatched.加载更多moves back for the same reason: inside the transitionloadingMorecould neither disable it nor say 正在加载…. Reported by @M4n5ter; the defect predates this PR, but every claim this PR makes about in-flight state was inert without it. Single-flight itself stays, for the one reason that survives: a success callsonImported, which closes Settings, so a second import would be orphaned on a page the user can no longer see.Deliberately not fixed
「再次导入同一个对话会创建一个独立的任务」 stays where it is. It only matters to someone asking "have I already imported this one", and the page unmounts on success, so it can never know the answer. Until an imported session persists its external origin, that sentence cannot become per-row information — and no other position beats a single visible line above the list. See #3081.
Concurrent imports are still not offered. A rework was tried and dropped early. Single-flight is not a limitation of the importer — Desktop Main can take two — it follows from the page ending on the first success, and lifting it would mean deciding what happens to the imports that were still running when Settings closed.
Appending a page still moves nothing in the list. Skeleton placeholder rows were tried and reverted:
Listdoes not forwardaria-busy(it destructures a closed prop list with no rest spread,List.tsx:143-165), so the ARIA half was dead on arrival, andSkeletonhas a built-in 1000ms animation delay against a local directory that answers in tens of milliseconds.Refs #2984
Before / after
Verification
npm run format:check,npm run lint— passnpm --workspace @maka/desktop run typecheck— passnpm --workspace @maka/desktop run build-storybook+smoke:storybook— pass, 130 storiesproduct-settings-pages--import-tasks: the unconfirmed banner naming one conversation, then two, then surviving an archived-filter toggle with both names intact and both rows still disabled; and--import-tasks-no-sourcefor the rewritten empty stateimportstubbed to never settle. WithclickAction: the source switch, the archived filter and every sibling row stay interactive and the row still reads 导入. WithonClick: the banner reads 正在导入「Draft the release notes for 0.9.0」, that row spins, every other 导入 is disabledimport: one name in the banner, then two, both rows disabled, and the lock released between the two clicksNot run: repository-wide tests, e2e.
Review focus
No test fails without this change. Every difference is visual or in what a control says, and the unconfirmed state is only reachable by clicking —
scripts/storybook-visual-smoke.mjsdisables everyplay, so a story for it would be skipped in CI. The existingImportTasksfixture already returns{ ok: false }, which puts that state one click away in Storybook; adding aplayon top of that buys nothing CI can see. The evidence I have is the manual pass above.This branch was reviewed adversarially by Codex and by independent Claude subagents across two rounds. That review removed a concurrent-import rework, skeleton rows, a per-row
Badge, a per-row tooltip and the archived-filter unfreeze from earlier versions of this PR, and produced the state collapse this revision is built on. It is AI review and does not count as independent human review.AI use
Select exactly one:
Tool(s) and scope: Claude Code — wrote the implementation and copy changes and ran the verification above, under direction from the human contributor, who chose each option against rendered Storybook comparisons. Codex and Claude subagents performed the adversarial review described above. The commit carries a
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?