refactor(workbench): Hub projects 端口抽取 + 契约切片（#1546 #1528） - #1608
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughChangesWorkbench projects integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant AgentHubWorkbench
participant WorkbenchRoutes
participant useWorkbenchProjectsRoute
participant WorkbenchProjectsPort
participant HubClient
participant ProjectNav
App->>AgentHubWorkbench: provide authenticated projectsPort
AgentHubWorkbench->>WorkbenchRoutes: forward projectsPort
WorkbenchRoutes->>useWorkbenchProjectsRoute: initialize route
useWorkbenchProjectsRoute->>WorkbenchProjectsPort: listProjects
WorkbenchProjectsPort->>HubClient: request workspace projects
HubClient-->>WorkbenchProjectsPort: paginated project response
WorkbenchProjectsPort-->>useWorkbenchProjectsRoute: WorkbenchProjectsPage
useWorkbenchProjectsRoute-->>ProjectNav: projects and pagination state
ProjectNav->>useWorkbenchProjectsRoute: retry loadMore after failure
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/shared/src/workbench/WorkbenchRoutes.tsx (1)
277-285: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEnforce exclusive project ownership or require parent mutation callbacks.
useWorkbenchProjectsRoutedisables port-backed project reads whenprojectsis supplied, but it still callsprojectsPortfor create/update whenonProjectCreateoronProjectUpdateare absent. That allows port-managed project mutations while the parent-managedprojectssource can remain stale.Make the route input an exclusive ownership type, or require both parent mutation callbacks whenever
projectsis provided.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/shared/src/workbench/WorkbenchRoutes.tsx` around lines 277 - 285, The WorkbenchProjectsRouteGate input currently allows parent-managed projects with missing mutation callbacks, while useWorkbenchProjectsRoute still sends create/update operations through projectsPort. Enforce an exclusive ownership contract in the route props, or require both onProjectCreate and onProjectUpdate whenever projects is provided, so parent-managed project state cannot coexist with port-backed mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/shared/src/workbench/pages/projects/ProjectNav.tsx`:
- Around line 149-167: Update the loadMoreError rendering in ProjectNav to put
role="alert" on StatusNotice, pass the retry button through its action prop, and
remove the outer div’s role="alert" so the error and retry action use one alert
region.
- Around line 150-161: Update the retry block in ProjectNav’s statusStack to
apply `?? ''` to each new CSS module lookup, and replace the direct
`onClick={onLoadMore}` binding with a synchronous callback that invokes
`onLoadMore` using `void` to discard its async result.
In `@app/shared/src/workbench/useWorkbenchProjectsRoute.ts`:
- Around line 104-124: Update the project-loading flows centered on loadProjects
and the related load-more and mutation handlers to invalidate requests when
projectsPort changes and guard every post-await state update with both the
mounted check and current request generation or cancellation token. Apply the
guards in success, catch, and finally paths, including setLoadingMore(false),
and prevent mutation-triggered loadProjects refreshes after unmount. Add
coverage for port replacement, load-more unmount, and mutation unmount.
- Line 81: Update the ownership selection around portProjectsEnabled and the
list/create/update paths to resolve one project owner for the entire hook: use
the port for reads and mutations when projectsPort is provided, otherwise use
the configured mutation callbacks consistently. Add or refine a discriminated
options type so configurations combining projectsPort with mutation callbacks,
or supplying only one callback, are rejected; ensure the affected list and
mutation operations all route through the selected owner.
In `@app/shared/src/workbench/WorkbenchFrameParts.test.tsx`:
- Line 487: Remove the explicit projectsPort={undefined} assignment from the
fixture in WorkbenchFrameParts tests, leaving the optional prop omitted while
preserving all other fixture fields.
In `@scripts/verify/verify-shared-ui-hubclient.ps1`:
- Around line 64-67: Update Strip-Comments and the verification flow to scan
comment-free TypeScript rather than the original lines: preserve block-comment
state across lines, ignore comment markers inside string literals, and apply the
stripped result to every import and forbidden type-name check covering the logic
around Strip-Comments and lines 74-107.
---
Outside diff comments:
In `@app/shared/src/workbench/WorkbenchRoutes.tsx`:
- Around line 277-285: The WorkbenchProjectsRouteGate input currently allows
parent-managed projects with missing mutation callbacks, while
useWorkbenchProjectsRoute still sends create/update operations through
projectsPort. Enforce an exclusive ownership contract in the route props, or
require both onProjectCreate and onProjectUpdate whenever projects is provided,
so parent-managed project state cannot coexist with port-backed mutations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 784bf850-1d2b-446f-8aed-d572c6576f5c
📒 Files selected for processing (29)
app/desktop/src/App.tsxapp/desktop/src/platform/desktopWorkbenchProjectsPort.test.tsapp/desktop/src/platform/desktopWorkbenchProjectsPort.tsapp/shared/src/i18n/workbench.tsapp/shared/src/workbench/AgentHubWorkbenchHelpers.test.tsapp/shared/src/workbench/AgentHubWorkbenchHelpers.tsapp/shared/src/workbench/AgentHubWorkbenchTypes.tsapp/shared/src/workbench/WorkbenchFrame.tsxapp/shared/src/workbench/WorkbenchFrameParts.test.tsxapp/shared/src/workbench/WorkbenchRoutes.tsxapp/shared/src/workbench/hubDataMapping.test.tsapp/shared/src/workbench/index.tsapp/shared/src/workbench/pages/ProjectsPage.tsxapp/shared/src/workbench/pages/projects/ProjectNav.tsxapp/shared/src/workbench/pages/projects/types.tsapp/shared/src/workbench/useWorkbenchProjectsRoute.test.tsapp/shared/src/workbench/useWorkbenchProjectsRoute.tsapp/shared/src/workbench/workbenchFramePartsHelpers.test.tsapp/shared/src/workbench/workbenchFramePartsHelpers.tsapp/shared/src/workbench/workbenchFrameTypes.tsapp/shared/src/workbench/workbenchProjectsPort.tsapp/shared/src/workbench/workbenchRoutesHelpers.tsapp/shared/src/workbench/workbenchRoutesTypes.tsapp/shared/vitest.config.tsapp/web/src/App.tsxapp/web/src/platform/webWorkbenchProjectsPort.test.tsapp/web/src/platform/webWorkbenchProjectsPort.tsdocs/progress/MASTER.mdscripts/verify/verify-shared-ui-hubclient.ps1
| {loadMoreError ? ( | ||
| <div className={styles.statusStack} role="alert"> | ||
| <StatusNotice | ||
| {...(styles.statusNotice ? { className: styles.statusNotice } : {})} | ||
| icon={<DesignNavIcon name="error404" size={14} />} | ||
| > | ||
| {t('projects.loadMoreError', { message: loadMoreError })} | ||
| </StatusNotice> | ||
| {onLoadMore ? ( | ||
| <button | ||
| type="button" | ||
| className={`${styles.newProjectBtn} ${styles.navNewProjectBtn} outline-action`} | ||
| onClick={onLoadMore} | ||
| > | ||
| {t('projects.retryLoadMore')} | ||
| </button> | ||
| ) : null} | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one alert region for the error and retry action.
The outer role="alert" contains a StatusNotice that defaults to role="status". Assistive technology can announce the same error through both live regions.
Set role="alert" on StatusNotice. Pass the retry button through its action prop. Remove the outer alert role.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/shared/src/workbench/pages/projects/ProjectNav.tsx` around lines 149 -
167, Update the loadMoreError rendering in ProjectNav to put role="alert" on
StatusNotice, pass the retry button through its action prop, and remove the
outer div’s role="alert" so the error and retry action use one alert region.
| <div className={styles.statusStack} role="alert"> | ||
| <StatusNotice | ||
| {...(styles.statusNotice ? { className: styles.statusNotice } : {})} | ||
| icon={<DesignNavIcon name="error404" size={14} />} | ||
| > | ||
| {t('projects.loadMoreError', { message: loadMoreError })} | ||
| </StatusNotice> | ||
| {onLoadMore ? ( | ||
| <button | ||
| type="button" | ||
| className={`${styles.newProjectBtn} ${styles.navNewProjectBtn} outline-action`} | ||
| onClick={onLoadMore} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the TypeScript UI boundary rules to the retry block.
Add ?? '' to the new CSS module lookups. Wrap the retry call in a synchronous handler that discards the async result with void.
Proposed fix
- <div className={styles.statusStack} role="alert">
+ <div className={styles.statusStack ?? ''} role="alert">
...
- className={`${styles.newProjectBtn} ${styles.navNewProjectBtn} outline-action`}
- onClick={onLoadMore}
+ className={`${styles.newProjectBtn ?? ''} ${styles.navNewProjectBtn ?? ''} outline-action`}
+ onClick={() => {
+ void onLoadMore();
+ }}As per coding guidelines, CSS module indexing must use ?? '', and async handlers passed to () => void must use void fn().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={styles.statusStack} role="alert"> | |
| <StatusNotice | |
| {...(styles.statusNotice ? { className: styles.statusNotice } : {})} | |
| icon={<DesignNavIcon name="error404" size={14} />} | |
| > | |
| {t('projects.loadMoreError', { message: loadMoreError })} | |
| </StatusNotice> | |
| {onLoadMore ? ( | |
| <button | |
| type="button" | |
| className={`${styles.newProjectBtn} ${styles.navNewProjectBtn} outline-action`} | |
| onClick={onLoadMore} | |
| <div className={styles.statusStack ?? ''} role="alert"> | |
| <StatusNotice | |
| {...(styles.statusNotice ? { className: styles.statusNotice } : {})} | |
| icon={<DesignNavIcon name="error404" size={14} />} | |
| > | |
| {t('projects.loadMoreError', { message: loadMoreError })} | |
| </StatusNotice> | |
| {onLoadMore ? ( | |
| <button | |
| type="button" | |
| className={`${styles.newProjectBtn ?? ''} ${styles.navNewProjectBtn ?? ''} outline-action`} | |
| onClick={() => { | |
| void onLoadMore(); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/shared/src/workbench/pages/projects/ProjectNav.tsx` around lines 150 -
161, Update the retry block in ProjectNav’s statusStack to apply `?? ''` to each
new CSS module lookup, and replace the direct `onClick={onLoadMore}` binding
with a synchronous callback that invokes `onLoadMore` using `void` to discard
its async result.
Source: Coding guidelines
| activeProjectId={undefined} | ||
| projectsStatus={undefined} | ||
| hubClient={undefined} | ||
| projectsPort={undefined} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Omit projectsPort from this fixture.
Line 487 explicitly assigns undefined to an optional prop. Remove the prop to test the omitted state and preserve exact optional-property semantics.
Proposed fix
- projectsPort={undefined}As per coding guidelines: “仅在值已定义时赋值”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| projectsPort={undefined} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/shared/src/workbench/WorkbenchFrameParts.test.tsx` at line 487, Remove
the explicit projectsPort={undefined} assignment from the fixture in
WorkbenchFrameParts tests, leaving the optional prop omitted while preserving
all other fixture fields.
Source: Coding guidelines
| function Strip-Comments([string]$Line) { | ||
| $code = [regex]::Replace($Line, '/\*.*?\*/', ' ', 'Singleline') | ||
| $code = [regex]::Replace($code, '//.*$', ' ') | ||
| return $code |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strip comments before every forbidden-reference scan.
The import scans use the original $line. A line such as // import type { HubClient } from './hubClient' therefore fails the gate.
Strip-Comments also handles only block comments that start and end on one line. An interior HubClient line in a multi-line documentation comment also fails.
Tokenize the TypeScript source, or use a stateful scanner that tracks block comments and string literals. Run all import and type-name checks against the resulting code.
Also applies to: 74-107
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'verify-shared-ui-hubclient.ps1'
(PSUseBOMForUnicodeEncodedFile)
[warning] 64-64: The cmdlet 'Strip-Comments' uses an unapproved verb.
(PSUseApprovedVerbs)
[warning] 64-64: The cmdlet 'Strip-Comments' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Strip-Comments'
(PSUseSingularNouns)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verify/verify-shared-ui-hubclient.ps1` around lines 64 - 67, Update
Strip-Comments and the verification flow to scan comment-free TypeScript rather
than the original lines: preserve block-comment state across lines, ignore
comment markers inside string literals, and apply the stripped result to every
import and forbidden type-name check covering the logic around Strip-Comments
and lines 74-107.
f8fa164 to
a947242
Compare
shared Workbench 不再以 type/value 形式引用 concrete HubClient： - 新增窄领域端口 WorkbenchProjectsPort（shared/workbench/workbenchProjectsPort.ts）， Desktop/Web composition root 各实现 adapter（desktop/webWorkbenchProjectsPort.ts） 注入 projectsPort，transport DTO 映射与默认名称归一化移到 app 层； - useWorkbenchProjectsRoute 的 internal-fetch 路径改为纯端口驱动（单一 ownership： parent-managed 或 port，不再运行时猜测 hubClient）； - 分页失败不再静默：loadMoreError 可见、hasMore 停哨兵、重试按钮（ProjectNav + zh/en i18n）； - 同 contract 双端 adapter 测试（desktop 5 / web 6 例）+ shared route 端口测试 11 例； - verify-shared-ui-hubclient.ps1 升级为同时禁止 value/type 引用 concrete HubClient。
#1546 的 projects slice 收尾，shared 层只依赖领域契约： - hubDataMapping.test.ts：为双端 ports 共用的 DTO→ProjectInfo 映射补契约测试， 该文件重新进入 shared coverage（uncovered 回落至基线 38）； - shared vitest.config.ts：将纯类型契约 workbenchProjectsPort.ts 列入窄排除 （与 types.ts/index.ts 同先例，零运行时语句）； - docs/progress/MASTER.md：登记本切片完成状态（agents/catalog 收口留待后续切片）。
a947242 to
3e64888
Compare
* refactor(workbench): Hub projects 依赖端口化为 WorkbenchProjectsPort（#1546） shared Workbench 不再以 type/value 形式引用 concrete HubClient： - 新增窄领域端口 WorkbenchProjectsPort（shared/workbench/workbenchProjectsPort.ts）， Desktop/Web composition root 各实现 adapter（desktop/webWorkbenchProjectsPort.ts） 注入 projectsPort，transport DTO 映射与默认名称归一化移到 app 层； - useWorkbenchProjectsRoute 的 internal-fetch 路径改为纯端口驱动（单一 ownership： parent-managed 或 port，不再运行时猜测 hubClient）； - 分页失败不再静默：loadMoreError 可见、hasMore 停哨兵、重试按钮（ProjectNav + zh/en i18n）； - 同 contract 双端 adapter 测试（desktop 5 / web 6 例）+ shared route 端口测试 11 例； - verify-shared-ui-hubclient.ps1 升级为同时禁止 value/type 引用 concrete HubClient。 * refactor(workbench): projects 契约切片收口映射合同与纯类型端口（#1528） #1546 的 projects slice 收尾，shared 层只依赖领域契约： - hubDataMapping.test.ts：为双端 ports 共用的 DTO→ProjectInfo 映射补契约测试， 该文件重新进入 shared coverage（uncovered 回落至基线 38）； - shared vitest.config.ts：将纯类型契约 workbenchProjectsPort.ts 列入窄排除 （与 types.ts/index.ts 同先例，零运行时语句）； - docs/progress/MASTER.md：登记本切片完成状态（agents/catalog 收口留待后续切片）。 * fix(web): webWorkbenchProjectsPort 测试补 beforeEach 导入 (#1546) --------- Co-authored-by: Codex <codex@vectorcontrol.tech>
Summary
Closes #1546 and completes the #1528 projects slice. The shared Workbench no longer depends on the concrete HubClient — neither as a value import nor as a type.
WorkbenchProjectsPort(app/shared/src/workbench/workbenchProjectsPort.ts); Desktop/Web composition roots implement it (desktopWorkbenchProjectsPort.ts/webWorkbenchProjectsPort.ts) and injectprojectsPort.useWorkbenchProjectsRouteis now port-driven only — single ownership (parent-managed XOR port, no runtimeprojects ?? hubClient ?? mockguessing).loadMoreErrorsurfaces inProjectNavwith an explicit retry button (zh/en i18n),hasMorestops the scroll sentinel, in-flight guard prevents duplicate pages, unmount-safe state writes.hubDataMapping(shared by both adapters) gets a contract test; pure-types contract file joins the narrow coverage exclusion set (same precedent astypes.ts/index.ts).verify-shared-ui-hubclient.ps1now forbids both value AND type references to concrete HubClient in shared presentation layers.Verification
--max-warnings 0): 0 errors / 0 warnings; web lint: 0 errors (20 pre-existing warnings, identical to master)verify-shared-ui-hubclient.ps1/verify-shared-boundary.ps1/verify-shared-barrel.ps1/verify-coverage-baseline.ps1: all pass (coverage above baseline: shared 76.99 / web 64.85 / desktop 49.54 lines)git diff --check: cleanNotes
pnpm typecheckresolves the app-root hoisted TypeScript 6.0.3 (which fails on the deprecatedbaseUrloption); CI uses the lockfile-pinned 5.8.x. Verified locally with the pinned 5.8.3 binary.tsc --noEmithas 53 pre-existing errors on master (test-file/storybook debt, not run by CI); this change adds zero new errors.Summary by CodeRabbit