feat(workspaces): add AI item links#555
Conversation
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR introduces workspace item relations: a new ChangesWorkspace item relations feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Tool as workspace_edit_item tool
participant EditOp as editWorkspaceItemOperation
participant RelResolver as resolveWorkspaceRelations
participant Kernel as WorkspaceKernel
participant RelStore as WorkspaceKernelRelations
Tool->>EditOp: execute(path, edits, relations)
EditOp->>RelResolver: resolveWorkspaceRelations(fromItemId, relations, tree)
RelResolver-->>EditOp: ready relations or failure
alt resolution failed
EditOp-->>Tool: failure result
else resolution ready
EditOp->>EditOp: apply markdown edits
alt applied > 0 and relations present
EditOp->>Kernel: createRelations(relations)
Kernel->>Kernel: assert items active
Kernel->>RelStore: createRelations(relations)
RelStore-->>Kernel: void
end
EditOp-->>Tool: success result
end
sequenceDiagram
participant ReadOp as readWorkspaceItemsOperation
participant Kernel as WorkspaceKernel
participant RelStore as WorkspaceKernelRelations
participant Serializer as serializeWorkspaceRelations
ReadOp->>ReadOp: build pathsByItemId from pageItems
ReadOp->>Kernel: listItemRelations(itemId)
Kernel->>RelStore: listItemRelations(itemId, limit)
RelStore-->>Kernel: WorkspaceKernelRelation[]
Kernel-->>ReadOp: relations
ReadOp->>Serializer: serializeWorkspaceRelations(item, pathsByItemId, relations)
Serializer-->>ReadOp: WorkspaceRelationOutput[]
ReadOp-->>ReadOp: attach relations to item result
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (5)
src/features/workspaces/kernel/workspace-kernel-item-commands.ts (1)
394-399: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider rejecting self-links and improving the error message.
assertLinkTargetsExistdoesn't exclude the item's own id, so an item can be linked to itself (inupdateItemLinks, the target item is already active by the time this runs). Also, failures surface as a generic"Workspace item not found."with no indication that it originated from link validation, which makes debugging harder.♻️ Optional improvement
- private assertLinkTargetsExist(linkItemIds: readonly string[]) { - for (const linkItemId of linkItemIds) { - this.store.assertActiveItem(linkItemId); - } - } + private assertLinkTargetsExist(linkItemIds: readonly string[], excludeItemId?: string) { + for (const linkItemId of linkItemIds) { + if (linkItemId === excludeItemId) { + throw new Error(`Workspace item cannot link to itself: ${linkItemId}`); + } + + try { + this.store.assertActiveItem(linkItemId); + } catch { + throw new Error(`Link target item not found: ${linkItemId}`); + } + } + }🤖 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 `@src/features/workspaces/kernel/workspace-kernel-item-commands.ts` around lines 394 - 399, Update assertLinkTargetsExist in workspace-kernel-item-commands so it rejects self-links by checking each linkItemId against the current item’s id before calling store.assertActiveItem. Also change the validation path to surface a link-specific error message instead of the generic “Workspace item not found.”, so failures from updateItemLinks clearly indicate link validation and the offending target.src/features/workspaces/ai/workspace-kernel-ai-common.ts (1)
226-249: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPath index rebuilt per item link lookup; hoist for batch reads.
getWorkspaceKernelAiItemLinksrebuilds the full item tree/path index (buildWorkspaceKernelItemPathIndex, which itself callsbuildWorkspaceKernelTree) and anitemsByIdmap from scratch on every call. Sinceworkspace_read_itemscan read multiple paths in a single request and this function is invoked once per item read (per thereadWorkspaceKernelAiItem/readWorkspaceKernelAiFileItemsnippets), the same index gets recomputed redundantly for every item in the batch.♻️ Proposed refactor: accept a precomputed index
-export function getWorkspaceKernelAiItemLinks(input: { - item: WorkspaceItemSummary; - pageItems: WorkspaceItemSummary[]; -}): WorkspaceItemLink[] { - const pathsByItemId = buildWorkspaceKernelItemPathIndex(input.pageItems); - const itemsById = new Map(input.pageItems.map((item) => [item.id, item])); +export function getWorkspaceKernelAiItemLinks(input: { + item: WorkspaceItemSummary; + pathsByItemId: ReadonlyMap<string, string>; + itemsById: ReadonlyMap<string, WorkspaceItemSummary>; +}): WorkspaceItemLink[] { + const { pathsByItemId, itemsById } = input; const links: WorkspaceItemLink[] = [];Callers (e.g. read/edit flows) would build
pathsByItemId/itemsByIdonce per request frompageItemsand pass them to every per-item call.🤖 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 `@src/features/workspaces/ai/workspace-kernel-ai-common.ts` around lines 226 - 249, `getWorkspaceKernelAiItemLinks` is recomputing the same path/index data for every item lookup, which is wasteful during batch reads. Refactor this function to accept precomputed `pathsByItemId` and `itemsById` (built once from `pageItems` by callers such as the `readWorkspaceKernelAiItem` / `readWorkspaceKernelAiFileItem` flow) instead of calling `buildWorkspaceKernelItemPathIndex` internally each time. Update the call sites to hoist the shared index creation per request and pass it through to `getWorkspaceKernelAiItemLinks`, while preserving the existing link-building behavior.src/features/workspaces/ai/workspace-kernel-ai-item-links.test.ts (3)
269-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated item-type union literal.
The inline union
"document" | "file" | "flashcard" | "folder" | "quiz"duplicates the workspace item type enum likely defined elsewhere (e.g. viaworkspaceItemTypeSchema, referenced in the kernel item-commands snippet). If the canonical type union changes, this helper silently drifts out of sync.♻️ Suggested fix
-async function expectItemLinks( - path: string, - links: Array<{ path: string; type: "document" | "file" | "flashcard" | "folder" | "quiz" }>, -) { +async function expectItemLinks( + path: string, + links: Array<{ path: string; type: WorkspaceItemType }>, +) {Import
WorkspaceItemType(orz.infer<typeof workspaceItemTypeSchema>) from the shared model module instead of re-declaring the union.🤖 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 `@src/features/workspaces/ai/workspace-kernel-ai-item-links.test.ts` around lines 269 - 281, The helper expectItemLinks is re-declaring the workspace item type union inline, which can drift from the canonical type definition. Replace the local Array<{ path: string; type: ... }> annotation with the shared WorkspaceItemType type (or z.infer<typeof workspaceItemTypeSchema>) imported from the shared model module so the test stays aligned with the source of truth.
124-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the "neither edits nor links provided" validation.
The PR objective states
editsis now optional and "Eithereditsorlinksmust be provided." This suite covers content-only edits, links-only edits, and link-resolution failures, but doesn't exercise the case where botheditsandlinksare omitted oneditWorkspaceKernelAiItem. Worth adding to lock in that validation behavior.🤖 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 `@src/features/workspaces/ai/workspace-kernel-ai-item-links.test.ts` around lines 124 - 218, Add a test in workspace-kernel-ai-item-links.test.ts that calls editWorkspaceKernelAiItem without either edits or links and asserts the validation error for the “either edits or links must be provided” case. Reuse the existing editWorkspaceKernelAiItem and createWorkspaceKernelAiItems setup patterns, and place the new coverage near the current edit validation tests so the behavior is locked in alongside the existing link and edit failure cases.
283-344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoad the test DB from the real D1 migrations instead of duplicating schema here.
wrangler.test.jsoncalready pointsmigrations_diratdrizzle, and the checked-in migrations include foreign keys, unique indexes, and check constraints that this hand-rolled DDL omits. UsereadD1Migrations/applyD1Migrations(or a shared schema fixture) so the test schema stays aligned.🤖 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 `@src/features/workspaces/ai/workspace-kernel-ai-item-links.test.ts` around lines 283 - 344, The test helper seedWorkspaceAccess is duplicating the database schema with hand-written DDL in workspaceAccessSchema and applyWorkspaceAccessSchema instead of using the real D1 migrations. Replace that setup with the existing migration-loading path (for example readD1Migrations and applyD1Migrations, or a shared schema fixture) so the test DB is built from the checked-in drizzle migrations and stays aligned with the production schema, including foreign keys, unique indexes, and check constraints.
🤖 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 `@src/features/workspaces/ai/workspace-kernel-ai-common.ts`:
- Around line 195-312: `resolveWorkspaceKernelAiLinkPaths` and
`resolveWorkspaceKernelAiLinkPath` currently allow resolving the item’s own
path, which can persist a self-referential link during the
`workspace-kernel-ai-edit` flow. Update the link-resolution flow to detect when
a resolved `itemId` matches the item being edited and fail that entry instead of
returning `ready`, using the existing resolution/failure pattern in
`resolveWorkspaceKernelAiLinkPaths` so self-links are excluded before
persistence.
In `@src/features/workspaces/ai/workspace-kernel-ai-edit.ts`:
- Around line 119-142: The link update branch in workspace-kernel-ai-edit’s edit
flow should not run when applyMarkdownEdits reports failures. Update the logic
around applyMarkdownEdits and context.kernel.updateItemLinks so links are only
mutated when result.failures is empty (or otherwise handled explicitly), and
keep the return shape in sync with the successful/failed edit outcome.
---
Nitpick comments:
In `@src/features/workspaces/ai/workspace-kernel-ai-common.ts`:
- Around line 226-249: `getWorkspaceKernelAiItemLinks` is recomputing the same
path/index data for every item lookup, which is wasteful during batch reads.
Refactor this function to accept precomputed `pathsByItemId` and `itemsById`
(built once from `pageItems` by callers such as the `readWorkspaceKernelAiItem`
/ `readWorkspaceKernelAiFileItem` flow) instead of calling
`buildWorkspaceKernelItemPathIndex` internally each time. Update the call sites
to hoist the shared index creation per request and pass it through to
`getWorkspaceKernelAiItemLinks`, while preserving the existing link-building
behavior.
In `@src/features/workspaces/ai/workspace-kernel-ai-item-links.test.ts`:
- Around line 269-281: The helper expectItemLinks is re-declaring the workspace
item type union inline, which can drift from the canonical type definition.
Replace the local Array<{ path: string; type: ... }> annotation with the shared
WorkspaceItemType type (or z.infer<typeof workspaceItemTypeSchema>) imported
from the shared model module so the test stays aligned with the source of truth.
- Around line 124-218: Add a test in workspace-kernel-ai-item-links.test.ts that
calls editWorkspaceKernelAiItem without either edits or links and asserts the
validation error for the “either edits or links must be provided” case. Reuse
the existing editWorkspaceKernelAiItem and createWorkspaceKernelAiItems setup
patterns, and place the new coverage near the current edit validation tests so
the behavior is locked in alongside the existing link and edit failure cases.
- Around line 283-344: The test helper seedWorkspaceAccess is duplicating the
database schema with hand-written DDL in workspaceAccessSchema and
applyWorkspaceAccessSchema instead of using the real D1 migrations. Replace that
setup with the existing migration-loading path (for example readD1Migrations and
applyD1Migrations, or a shared schema fixture) so the test DB is built from the
checked-in drizzle migrations and stays aligned with the production schema,
including foreign keys, unique indexes, and check constraints.
In `@src/features/workspaces/kernel/workspace-kernel-item-commands.ts`:
- Around line 394-399: Update assertLinkTargetsExist in
workspace-kernel-item-commands so it rejects self-links by checking each
linkItemId against the current item’s id before calling store.assertActiveItem.
Also change the validation path to surface a link-specific error message instead
of the generic “Workspace item not found.”, so failures from updateItemLinks
clearly indicate link validation and the offending target.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6d6f173-4dea-4d1a-9de8-ca68ba1a3a12
📒 Files selected for processing (17)
src/features/workspaces/ai/workspace-kernel-ai-common.tssrc/features/workspaces/ai/workspace-kernel-ai-create.tssrc/features/workspaces/ai/workspace-kernel-ai-edit.tssrc/features/workspaces/ai/workspace-kernel-ai-item-links.test.tssrc/features/workspaces/ai/workspace-kernel-ai-read.tssrc/features/workspaces/ai/workspace-tools.tssrc/features/workspaces/kernel/workspace-kernel-access.tssrc/features/workspaces/kernel/workspace-kernel-item-commands.tssrc/features/workspaces/kernel/workspace-kernel-types.tssrc/features/workspaces/kernel/workspace-kernel.tssrc/features/workspaces/model/workspace-item-links.tssrc/features/workspaces/model/workspace-page.tssrc/features/workspaces/realtime/messages.tssrc/features/workspaces/server/permissions.tssrc/test-worker.tsvitest.config.tswrangler.test.jsonc
|
|
||
| export function resolveWorkspaceKernelAiLinkPaths(input: { | ||
| createdItemsByPath?: ReadonlyMap<string, WorkspaceKernelAiCreatedLinkTarget>; | ||
| paths: readonly string[]; | ||
| tree: WorkspaceKernelTree; | ||
| }): WorkspaceKernelAiLinkResolution { | ||
| const linkItemIds: string[] = []; | ||
|
|
||
| for (const [index, path] of input.paths.entries()) { | ||
| const resolution = resolveWorkspaceKernelAiLinkPath({ | ||
| createdItemsByPath: input.createdItemsByPath, | ||
| path, | ||
| tree: input.tree, | ||
| }); | ||
|
|
||
| if (resolution.status === "failed") { | ||
| return { | ||
| ...resolution, | ||
| index, | ||
| }; | ||
| } | ||
|
|
||
| linkItemIds.push(resolution.itemId); | ||
| } | ||
|
|
||
| return { | ||
| linkItemIds: uniqueWorkspaceKernelAiLinkItemIds(linkItemIds), | ||
| status: "ready", | ||
| }; | ||
| } | ||
|
|
||
| export function getWorkspaceKernelAiItemLinks(input: { | ||
| item: WorkspaceItemSummary; | ||
| pageItems: WorkspaceItemSummary[]; | ||
| }): WorkspaceItemLink[] { | ||
| const pathsByItemId = buildWorkspaceKernelItemPathIndex(input.pageItems); | ||
| const itemsById = new Map(input.pageItems.map((item) => [item.id, item])); | ||
| const links: WorkspaceItemLink[] = []; | ||
|
|
||
| for (const itemId of getWorkspaceItemLinkItemIds(input.item.metadataJson)) { | ||
| const item = itemsById.get(itemId); | ||
| const path = pathsByItemId.get(itemId); | ||
|
|
||
| if (!item || !path) { | ||
| continue; | ||
| } | ||
|
|
||
| links.push({ | ||
| path, | ||
| type: item.type, | ||
| }); | ||
| } | ||
|
|
||
| return links; | ||
| } | ||
|
|
||
| function resolveWorkspaceKernelAiLinkPath(input: { | ||
| createdItemsByPath?: ReadonlyMap<string, WorkspaceKernelAiCreatedLinkTarget>; | ||
| path: string; | ||
| tree: WorkspaceKernelTree; | ||
| }): | ||
| | { | ||
| itemId: string; | ||
| status: "ready"; | ||
| } | ||
| | { | ||
| code: WorkspaceKernelAiLinkResolutionFailureCode; | ||
| path: string; | ||
| status: "failed"; | ||
| } { | ||
| let normalizedPath: string; | ||
|
|
||
| try { | ||
| normalizedPath = normalizeWorkspacePath(input.path); | ||
| } catch (error) { | ||
| if (error instanceof WorkspaceKernelPathError && error.code === "path_not_absolute") { | ||
| return { | ||
| code: "link_path_not_absolute", | ||
| path: input.path, | ||
| status: "failed", | ||
| }; | ||
| } | ||
|
|
||
| throw error; | ||
| } | ||
|
|
||
| if (normalizedPath === "/") { | ||
| return { | ||
| code: "link_path_is_root", | ||
| path: normalizedPath, | ||
| status: "failed", | ||
| }; | ||
| } | ||
|
|
||
| const createdItem = input.createdItemsByPath?.get(normalizedPath); | ||
|
|
||
| if (createdItem) { | ||
| return { | ||
| itemId: createdItem.id, | ||
| status: "ready", | ||
| }; | ||
| } | ||
|
|
||
| const item = resolveWorkspaceKernelItemPath(normalizedPath, input.tree); | ||
|
|
||
| if (!item) { | ||
| return { | ||
| code: "link_path_not_found", | ||
| path: normalizedPath, | ||
| status: "failed", | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| itemId: item.id, | ||
| status: "ready", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No guard against an item linking to itself.
resolveWorkspaceKernelAiLinkPaths/resolveWorkspaceKernelAiLinkPath resolve any valid path, including the path of the item currently being edited. Per the edit flow (workspace-kernel-ai-edit.ts), input.links is resolved without excluding resolution.item.id, and the kernel's assertLinkTargetsExist only checks existence, not self-reference. A user/model request that includes the item's own path in links will silently persist a self-referential link.
🔧 Proposed fix to exclude self-references
export function resolveWorkspaceKernelAiLinkPaths(input: {
createdItemsByPath?: ReadonlyMap<string, WorkspaceKernelAiCreatedLinkTarget>;
+ excludeItemId?: string;
paths: readonly string[];
tree: WorkspaceKernelTree;
}): WorkspaceKernelAiLinkResolution {
const linkItemIds: string[] = [];
for (const [index, path] of input.paths.entries()) {
const resolution = resolveWorkspaceKernelAiLinkPath({
createdItemsByPath: input.createdItemsByPath,
path,
tree: input.tree,
});
if (resolution.status === "failed") {
return {
...resolution,
index,
};
}
+ if (resolution.itemId === input.excludeItemId) {
+ continue;
+ }
+
linkItemIds.push(resolution.itemId);
}📝 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.
| export function resolveWorkspaceKernelAiLinkPaths(input: { | |
| createdItemsByPath?: ReadonlyMap<string, WorkspaceKernelAiCreatedLinkTarget>; | |
| paths: readonly string[]; | |
| tree: WorkspaceKernelTree; | |
| }): WorkspaceKernelAiLinkResolution { | |
| const linkItemIds: string[] = []; | |
| for (const [index, path] of input.paths.entries()) { | |
| const resolution = resolveWorkspaceKernelAiLinkPath({ | |
| createdItemsByPath: input.createdItemsByPath, | |
| path, | |
| tree: input.tree, | |
| }); | |
| if (resolution.status === "failed") { | |
| return { | |
| ...resolution, | |
| index, | |
| }; | |
| } | |
| linkItemIds.push(resolution.itemId); | |
| } | |
| return { | |
| linkItemIds: uniqueWorkspaceKernelAiLinkItemIds(linkItemIds), | |
| status: "ready", | |
| }; | |
| } | |
| export function getWorkspaceKernelAiItemLinks(input: { | |
| item: WorkspaceItemSummary; | |
| pageItems: WorkspaceItemSummary[]; | |
| }): WorkspaceItemLink[] { | |
| const pathsByItemId = buildWorkspaceKernelItemPathIndex(input.pageItems); | |
| const itemsById = new Map(input.pageItems.map((item) => [item.id, item])); | |
| const links: WorkspaceItemLink[] = []; | |
| for (const itemId of getWorkspaceItemLinkItemIds(input.item.metadataJson)) { | |
| const item = itemsById.get(itemId); | |
| const path = pathsByItemId.get(itemId); | |
| if (!item || !path) { | |
| continue; | |
| } | |
| links.push({ | |
| path, | |
| type: item.type, | |
| }); | |
| } | |
| return links; | |
| } | |
| function resolveWorkspaceKernelAiLinkPath(input: { | |
| createdItemsByPath?: ReadonlyMap<string, WorkspaceKernelAiCreatedLinkTarget>; | |
| path: string; | |
| tree: WorkspaceKernelTree; | |
| }): | |
| | { | |
| itemId: string; | |
| status: "ready"; | |
| } | |
| | { | |
| code: WorkspaceKernelAiLinkResolutionFailureCode; | |
| path: string; | |
| status: "failed"; | |
| } { | |
| let normalizedPath: string; | |
| try { | |
| normalizedPath = normalizeWorkspacePath(input.path); | |
| } catch (error) { | |
| if (error instanceof WorkspaceKernelPathError && error.code === "path_not_absolute") { | |
| return { | |
| code: "link_path_not_absolute", | |
| path: input.path, | |
| status: "failed", | |
| }; | |
| } | |
| throw error; | |
| } | |
| if (normalizedPath === "/") { | |
| return { | |
| code: "link_path_is_root", | |
| path: normalizedPath, | |
| status: "failed", | |
| }; | |
| } | |
| const createdItem = input.createdItemsByPath?.get(normalizedPath); | |
| if (createdItem) { | |
| return { | |
| itemId: createdItem.id, | |
| status: "ready", | |
| }; | |
| } | |
| const item = resolveWorkspaceKernelItemPath(normalizedPath, input.tree); | |
| if (!item) { | |
| return { | |
| code: "link_path_not_found", | |
| path: normalizedPath, | |
| status: "failed", | |
| }; | |
| } | |
| return { | |
| itemId: item.id, | |
| status: "ready", | |
| }; | |
| } | |
| export function resolveWorkspaceKernelAiLinkPaths(input: { | |
| createdItemsByPath?: ReadonlyMap<string, WorkspaceKernelAiCreatedLinkTarget>; | |
| excludeItemId?: string; | |
| paths: readonly string[]; | |
| tree: WorkspaceKernelTree; | |
| }): WorkspaceKernelAiLinkResolution { | |
| const linkItemIds: string[] = []; | |
| for (const [index, path] of input.paths.entries()) { | |
| const resolution = resolveWorkspaceKernelAiLinkPath({ | |
| createdItemsByPath: input.createdItemsByPath, | |
| path, | |
| tree: input.tree, | |
| }); | |
| if (resolution.status === "failed") { | |
| return { | |
| ...resolution, | |
| index, | |
| }; | |
| } | |
| if (resolution.itemId === input.excludeItemId) { | |
| continue; | |
| } | |
| linkItemIds.push(resolution.itemId); | |
| } | |
| return { | |
| linkItemIds: uniqueWorkspaceKernelAiLinkItemIds(linkItemIds), | |
| status: "ready", | |
| }; | |
| } |
🤖 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 `@src/features/workspaces/ai/workspace-kernel-ai-common.ts` around lines 195 -
312, `resolveWorkspaceKernelAiLinkPaths` and `resolveWorkspaceKernelAiLinkPath`
currently allow resolving the item’s own path, which can persist a
self-referential link during the `workspace-kernel-ai-edit` flow. Update the
link-resolution flow to detect when a resolved `itemId` matches the item being
edited and fail that entry instead of returning `ready`, using the existing
resolution/failure pattern in `resolveWorkspaceKernelAiLinkPaths` so self-links
are excluded before persistence.
Greptile SummaryThis PR adds AI-managed item links to the workspace kernel, storing link target IDs inside
Confidence Score: 3/5Safe to merge once the combined-edits-and-links atomicity question in workspace-kernel-ai-edit.ts is resolved; the rest of the change is well-structured and well-tested. In the combined edits+links code path, src/features/workspaces/ai/workspace-kernel-ai-edit.ts — specifically the code that calls Important Files Changed
Reviews (1): Last reviewed commit: "feat(workspaces): add AI item links" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const result = await documentSession.applyMarkdownEdits({ | ||
| edits: input.edits, | ||
| edits, | ||
| }); | ||
| const command = | ||
| links === undefined | ||
| ? null | ||
| : await context.kernel.updateItemLinks({ | ||
| itemId: resolution.item.id, | ||
| linkItemIds: links.linkItemIds, | ||
| actorUserId: input.userId, | ||
| clientMutationId: null, | ||
| }); | ||
|
|
||
| return { | ||
| applied: result.applied, | ||
| failed: result.failures, | ||
| ...(command | ||
| ? { | ||
| links: getWorkspaceKernelAiItemLinks({ | ||
| item: command.result, | ||
| pageItems: context.pageItems, | ||
| }), | ||
| } | ||
| : {}), | ||
| path: resolution.path, | ||
| warnings: result.warnings, | ||
| }; |
There was a problem hiding this comment.
When both edits and links are supplied, updateItemLinks is called unconditionally after applyMarkdownEdits — even if every edit failed (result.applied === 0, result.failures.length === edits.length). The model receives a response with applied: 0, a full failed array, but also the mutated links field, creating a split state: content unchanged, links permanently replaced. On a retry the model will set the same links a second time (harmless but wasteful), but it will never know the content edit and link update should have been atomic. Consider guarding the updateItemLinks call with if (result.applied > 0 || result.failures.length === 0), or document explicitly that links are always applied regardless of edit outcomes.
| if (links?.status === "failed") { | ||
| return { | ||
| path: resolution.path, | ||
| warnings: [], | ||
| ...failedWorkspaceAiEditResult(links.code, Math.max(edits.length, 1), links.index), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Create link failures include {code, index, path} so the caller knows which path was rejected. Edit link failures go through failedWorkspaceAiEditResult, which only returns {code, index} — the links.path that triggered the error is silently dropped. This makes it harder for the model to self-correct (it knows there was a bad link but not which one). Either thread path through failedWorkspaceAiEditResult for link-specific failures, or include path directly in the returned failure object for this branch.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/ai/workspace-kernel-ai-common.ts">
<violation number="1" location="src/features/workspaces/ai/workspace-kernel-ai-common.ts:230">
P2: Indexes rebuilt on every call to `getWorkspaceKernelAiItemLinks`, leading to O(m·n) work when reading multiple items</violation>
</file>
<file name="src/features/workspaces/model/workspace-page.ts">
<violation number="1" location="src/features/workspaces/model/workspace-page.ts:27">
P1: Metadata update events may allow stale full-item payloads to overwrite newer page state because `upsertWorkspaceItemsInPage` applies the event payload unconditionally while only the page revision is merged via `Math.max`. This is exacerbated by the new links-only metadata edits, which can be concurrent with content/name/color changes.</violation>
</file>
<file name="src/features/workspaces/model/workspace-item-links.ts">
<violation number="1" location="src/features/workspaces/model/workspace-item-links.ts:3">
P2: Using a generic `"links"` key in shared metadata risks silently overwriting unrelated metadata stored under the same key; use a namespaced key to avoid collisions.</violation>
</file>
<file name="src/features/workspaces/ai/workspace-kernel-ai-read.ts">
<violation number="1" location="src/features/workspaces/ai/workspace-kernel-ai-read.ts:146">
P2: File item reads compute links twice: the outer `readWorkspaceKernelAiItem` resolves links but does not pass them to the file branch, so `readWorkspaceKernelAiFileItem` recomputes the same links from the same item and pageItems. This adds unnecessary overhead to every file read, especially in batch reads.</violation>
</file>
<file name="src/features/workspaces/kernel/workspace-kernel-item-commands.ts">
<violation number="1" location="src/features/workspaces/kernel/workspace-kernel-item-commands.ts:35">
P1: Link target validation can be bypassed when links are supplied via `metadataJson` instead of `linkItemIds` during item creation.</violation>
<violation number="2" location="src/features/workspaces/kernel/workspace-kernel-item-commands.ts:35">
P2: Deleting a workspace item leaves dangling link IDs in other items' metadata because `deleteItems` does not clean up or revalidate inbound `metadata_json` links. Links are validated at write-time, but there is no symmetric cleanup or read-time filtering, so items can reference deleted targets.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| case "workspace.item.moved": | ||
| case "workspace.item.color.updated": | ||
| case "workspace.item.content.updated": | ||
| case "workspace.item.metadata.updated": |
There was a problem hiding this comment.
P1: Metadata update events may allow stale full-item payloads to overwrite newer page state because upsertWorkspaceItemsInPage applies the event payload unconditionally while only the page revision is merged via Math.max. This is exacerbated by the new links-only metadata edits, which can be concurrent with content/name/color changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/model/workspace-page.ts, line 27:
<comment>Metadata update events may allow stale full-item payloads to overwrite newer page state because `upsertWorkspaceItemsInPage` applies the event payload unconditionally while only the page revision is merged via `Math.max`. This is exacerbated by the new links-only metadata edits, which can be concurrent with content/name/color changes.</comment>
<file context>
@@ -24,6 +24,7 @@ export function applyWorkspaceEventToPage(
case "workspace.item.moved":
case "workspace.item.color.updated":
case "workspace.item.content.updated":
+ case "workspace.item.metadata.updated":
return upsertWorkspaceItemInPage(page, event.payload.item, event.revision);
case "workspace.items.moved":
</file context>
| UpdateWorkspaceKernelItemLinksArgs, | ||
| WriteWorkspaceKernelItemArgs, | ||
| } from "#/features/workspaces/kernel/workspace-kernel-types"; | ||
| import { withWorkspaceItemLinksMetadata } from "#/features/workspaces/model/workspace-item-links"; |
There was a problem hiding this comment.
P1: Link target validation can be bypassed when links are supplied via metadataJson instead of linkItemIds during item creation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/kernel/workspace-kernel-item-commands.ts, line 35:
<comment>Link target validation can be bypassed when links are supplied via `metadataJson` instead of `linkItemIds` during item creation.</comment>
<file context>
@@ -28,8 +29,10 @@ import type {
+ UpdateWorkspaceKernelItemLinksArgs,
WriteWorkspaceKernelItemArgs,
} from "#/features/workspaces/kernel/workspace-kernel-types";
+import { withWorkspaceItemLinksMetadata } from "#/features/workspaces/model/workspace-item-links";
import {
resolveWorkspaceItemColorForCreate,
</file context>
| item: WorkspaceItemSummary; | ||
| pageItems: WorkspaceItemSummary[]; | ||
| }): WorkspaceItemLink[] { | ||
| const pathsByItemId = buildWorkspaceKernelItemPathIndex(input.pageItems); |
There was a problem hiding this comment.
P2: Indexes rebuilt on every call to getWorkspaceKernelAiItemLinks, leading to O(m·n) work when reading multiple items
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/ai/workspace-kernel-ai-common.ts, line 230:
<comment>Indexes rebuilt on every call to `getWorkspaceKernelAiItemLinks`, leading to O(m·n) work when reading multiple items</comment>
<file context>
@@ -165,3 +192,137 @@ export function resolveWorkspaceKernelAiExistingItemPath<TRootCode extends strin
+ item: WorkspaceItemSummary;
+ pageItems: WorkspaceItemSummary[];
+}): WorkspaceItemLink[] {
+ const pathsByItemId = buildWorkspaceKernelItemPathIndex(input.pageItems);
+ const itemsById = new Map(input.pageItems.map((item) => [item.id, item]));
+ const links: WorkspaceItemLink[] = [];
</file context>
| @@ -0,0 +1,44 @@ | |||
| import type { JsonValue, WorkspaceItemSummary } from "#/features/workspaces/contracts"; | |||
|
|
|||
| export const workspaceItemLinksMetadataKey = "links"; | |||
There was a problem hiding this comment.
P2: Using a generic "links" key in shared metadata risks silently overwriting unrelated metadata stored under the same key; use a namespaced key to avoid collisions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/model/workspace-item-links.ts, line 3:
<comment>Using a generic `"links"` key in shared metadata risks silently overwriting unrelated metadata stored under the same key; use a namespaced key to avoid collisions.</comment>
<file context>
@@ -0,0 +1,44 @@
+import type { JsonValue, WorkspaceItemSummary } from "#/features/workspaces/contracts";
+
+export const workspaceItemLinksMetadataKey = "links";
+
+export interface WorkspaceItemLink {
</file context>
| path: string; | ||
| }): Promise<WorkspaceKernelAiReadItem> { | ||
| const { item } = input; | ||
| const links = getWorkspaceKernelAiItemLinks({ |
There was a problem hiding this comment.
P2: File item reads compute links twice: the outer readWorkspaceKernelAiItem resolves links but does not pass them to the file branch, so readWorkspaceKernelAiFileItem recomputes the same links from the same item and pageItems. This adds unnecessary overhead to every file read, especially in batch reads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/ai/workspace-kernel-ai-read.ts, line 146:
<comment>File item reads compute links twice: the outer `readWorkspaceKernelAiItem` resolves links but does not pass them to the file branch, so `readWorkspaceKernelAiFileItem` recomputes the same links from the same item and pageItems. This adds unnecessary overhead to every file read, especially in batch reads.</comment>
<file context>
@@ -135,9 +139,14 @@ async function readWorkspaceKernelAiItem(input: {
path: string;
}): Promise<WorkspaceKernelAiReadItem> {
const { item } = input;
+ const links = getWorkspaceKernelAiItemLinks({
+ item,
+ pageItems: input.pageItems,
</file context>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/ai/workspace-kernel-ai-common.ts">
<violation number="1" location="src/features/workspaces/ai/workspace-kernel-ai-common.ts:230">
P2: Indexes rebuilt on every call to `getWorkspaceKernelAiItemLinks`, leading to O(m·n) work when reading multiple items</violation>
</file>
<file name="src/features/workspaces/model/workspace-page.ts">
<violation number="1" location="src/features/workspaces/model/workspace-page.ts:27">
P1: Metadata update events may allow stale full-item payloads to overwrite newer page state because `upsertWorkspaceItemsInPage` applies the event payload unconditionally while only the page revision is merged via `Math.max`. This is exacerbated by the new links-only metadata edits, which can be concurrent with content/name/color changes.</violation>
</file>
<file name="src/features/workspaces/model/workspace-item-links.ts">
<violation number="1" location="src/features/workspaces/model/workspace-item-links.ts:3">
P2: Using a generic `"links"` key in shared metadata risks silently overwriting unrelated metadata stored under the same key; use a namespaced key to avoid collisions.</violation>
</file>
<file name="src/features/workspaces/ai/workspace-kernel-ai-read.ts">
<violation number="1" location="src/features/workspaces/ai/workspace-kernel-ai-read.ts:146">
P2: File item reads compute links twice: the outer `readWorkspaceKernelAiItem` resolves links but does not pass them to the file branch, so `readWorkspaceKernelAiFileItem` recomputes the same links from the same item and pageItems. This adds unnecessary overhead to every file read, especially in batch reads.</violation>
</file>
<file name="src/features/workspaces/kernel/workspace-kernel-item-commands.ts">
<violation number="1" location="src/features/workspaces/kernel/workspace-kernel-item-commands.ts:35">
P1: Link target validation can be bypassed when links are supplied via `metadataJson` instead of `linkItemIds` during item creation.</violation>
</file>
<file name="src/features/workspaces/ai/workspace-kernel-ai-create.ts">
<violation number="1" location="src/features/workspaces/ai/workspace-kernel-ai-create.ts:143">
P2: Self-link filter in createWorkspaceKernelAiItems is a no-op: the resolved link IDs cannot include the freshly generated item ID, so the filter never removes anything and gives a false sense of protection.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| const itemId = crypto.randomUUID(); | ||
| const linkItemIds = links?.linkItemIds.filter((linkItemId) => linkItemId !== itemId); |
There was a problem hiding this comment.
P2: Self-link filter in createWorkspaceKernelAiItems is a no-op: the resolved link IDs cannot include the freshly generated item ID, so the filter never removes anything and gives a false sense of protection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/ai/workspace-kernel-ai-create.ts, line 143:
<comment>Self-link filter in createWorkspaceKernelAiItems is a no-op: the resolved link IDs cannot include the freshly generated item ID, so the filter never removes anything and gives a false sense of protection.</comment>
<file context>
@@ -139,16 +139,19 @@ export async function createWorkspaceKernelAiItems(
}
+ const itemId = crypto.randomUUID();
+ const linkItemIds = links?.linkItemIds.filter((linkItemId) => linkItemId !== itemId);
let command: Awaited<ReturnType<WorkspaceKernelClient["createItem"]>>;
</file context>
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/features/workspaces/kernel/workspace-kernel-relations.ts (1)
77-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHard failure on unrecognized/legacy
kindvalues.
workspaceRelationKindSchema.parse(row.kind)throws if a stored row'skindno longer matchesworkspaceRelationKindValues(e.g. a future rename or removal of an enum value). Since this runs insidelistItemRelations, one stale row would break reads for the entire item rather than degrading gracefully.🛡️ Proposed defensive fallback
function mapKernelRelationRow(row: KernelRelationRow): WorkspaceKernelRelation { + const parsedKind = workspaceRelationKindSchema.safeParse(row.kind); + return { id: row.id, fromItemId: row.from_item_id, toItemId: row.to_item_id, - kind: workspaceRelationKindSchema.parse(row.kind), + kind: parsedKind.success ? parsedKind.data : (row.kind as WorkspaceRelationKind), note: row.note || null, }; }🤖 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 `@src/features/workspaces/kernel/workspace-kernel-relations.ts` around lines 77 - 85, `mapKernelRelationRow` is hard-failing on legacy or unrecognized `kind` values because `workspaceRelationKindSchema.parse(row.kind)` throws inside `listItemRelations`. Update the mapping to handle invalid `row.kind` defensively by validating with `workspaceRelationKindSchema` and falling back to a safe value or returning a nullable/filtered relation instead of throwing, so one stale row does not break reads for the whole item.src/features/workspaces/operations/read-items.ts (1)
118-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-item read + relations fetch adds avoidable latency.
readWorkspaceItemandkernel.listItemRelationsare independent kernel calls but are awaited sequentially per path. For multi-path reads this compounds linearly withinput.paths.length.⚡ Suggested fix
- const item = await readWorkspaceItem({ - item: resolution.item, - kernel: workspaceContext.kernel, - pages: input.pages, - path: resolution.path, - }); - const relations = serializeWorkspaceRelations({ - item: resolution.item, - pathsByItemId, - relations: await workspaceContext.kernel.listItemRelations({ - itemId: resolution.item.id, - }), - }); + const [item, kernelRelations] = await Promise.all([ + readWorkspaceItem({ + item: resolution.item, + kernel: workspaceContext.kernel, + pages: input.pages, + path: resolution.path, + }), + workspaceContext.kernel.listItemRelations({ itemId: resolution.item.id }), + ]); + const relations = serializeWorkspaceRelations({ + item: resolution.item, + pathsByItemId, + relations: kernelRelations, + });Longer-term, consider a bulk
listItemRelationskernel call keyed by multiple item ids to also eliminate the cross-path N+1.🤖 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 `@src/features/workspaces/operations/read-items.ts` around lines 118 - 148, The per-item fetch in read-items.ts is doing two independent kernel calls sequentially, which adds avoidable latency. In the read loop, update the readWorkspaceItem and kernel.listItemRelations flow so both requests are started together and awaited together for each resolution, while preserving the existing WorkspacePageSelectionError handling and result.items/result.failed behavior. Use the existing readWorkspaceItem, workspaceContext.kernel.listItemRelations, and serializeWorkspaceRelations symbols to locate the logic; if needed, also consider a future bulk relation fetch to reduce the N+1 pattern across multiple paths.
🤖 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 `@src/features/workspaces/operations/create-items.ts`:
- Around line 171-174: The unguarded workspaceContext.kernel.createRelations
call in createWorkspaceItemsOperation can throw after items have already been
persisted, causing the whole operation to fail and lose the accumulated
items/failed results. Wrap the createRelations step in error handling, record
the failure in the existing failed array for the current item, and continue or
return a partial result instead of letting the exception escape. Keep the fix
localized around createWorkspaceItemsOperation and its relations.relations check
so the caller still receives the items and failed collections.
In `@src/features/workspaces/operations/edit-item.ts`:
- Around line 100-102: The createRelations call in editWorkspaceItemOperation is
not guarded, so a failure after the markdown edits are already applied can
escape uncaught and hide the successful edit result. Update the result.applied /
relations.relations.length branch to handle errors from
workspaceContext.kernel.createRelations explicitly, and make sure the function
reports that the edit succeeded even if relation creation fails by catching the
exception and preserving the applied state. Use editWorkspaceItemOperation and
createRelations as the key symbols when locating the fix.
In `@src/features/workspaces/operations/workspace-tool-definitions.ts`:
- Around line 162-177: The workspace_edit_item tool wiring already forwards
relations correctly, but the current workspaceEditItemInputSchema still blocks
relations-only calls because edits is required upstream. Update the input schema
and any related validation in workspace-tool-schemas.ts to make edits optional
per the PR goal, and keep defineWorkspaceTool.execute in
workspace-tool-definitions.ts compatible with edits being undefined while still
passing relations through to editWorkspaceItemOperation.
In `@src/features/workspaces/operations/workspace-tool-schemas.ts`:
- Around line 120-134: The workspaceEditItemInputSchema still requires edits,
which blocks links-only updates and contradicts the intended optional edit flow.
Update the schema in workspaceEditItemInputSchema so edits can be omitted when
relations are provided, while still enforcing that at least one of edits or
relations is present. Then adjust the downstream editWorkspaceItemOperation and
edit-item.ts handling to safely accept an undefined or empty edits value and
skip applyMarkdownEdits when no text edits are supplied.
---
Nitpick comments:
In `@src/features/workspaces/kernel/workspace-kernel-relations.ts`:
- Around line 77-85: `mapKernelRelationRow` is hard-failing on legacy or
unrecognized `kind` values because `workspaceRelationKindSchema.parse(row.kind)`
throws inside `listItemRelations`. Update the mapping to handle invalid
`row.kind` defensively by validating with `workspaceRelationKindSchema` and
falling back to a safe value or returning a nullable/filtered relation instead
of throwing, so one stale row does not break reads for the whole item.
In `@src/features/workspaces/operations/read-items.ts`:
- Around line 118-148: The per-item fetch in read-items.ts is doing two
independent kernel calls sequentially, which adds avoidable latency. In the read
loop, update the readWorkspaceItem and kernel.listItemRelations flow so both
requests are started together and awaited together for each resolution, while
preserving the existing WorkspacePageSelectionError handling and
result.items/result.failed behavior. Use the existing readWorkspaceItem,
workspaceContext.kernel.listItemRelations, and serializeWorkspaceRelations
symbols to locate the logic; if needed, also consider a future bulk relation
fetch to reduce the N+1 pattern across multiple paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 969a2732-0e11-4120-9f0b-044a8569b836
📒 Files selected for processing (14)
src/features/workspaces/contracts.tssrc/features/workspaces/kernel/workspace-kernel-access.tssrc/features/workspaces/kernel/workspace-kernel-item-commands.tssrc/features/workspaces/kernel/workspace-kernel-relations.tssrc/features/workspaces/kernel/workspace-kernel-schema.tssrc/features/workspaces/kernel/workspace-kernel-types.tssrc/features/workspaces/kernel/workspace-kernel.tssrc/features/workspaces/operations/create-items.tssrc/features/workspaces/operations/edit-item.tssrc/features/workspaces/operations/read-items.tssrc/features/workspaces/operations/relations.tssrc/features/workspaces/operations/workspace-operation-context.tssrc/features/workspaces/operations/workspace-tool-definitions.tssrc/features/workspaces/operations/workspace-tool-schemas.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/features/workspaces/kernel/workspace-kernel-relations.ts (1)
77-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHard failure on unrecognized/legacy
kindvalues.
workspaceRelationKindSchema.parse(row.kind)throws if a stored row'skindno longer matchesworkspaceRelationKindValues(e.g. a future rename or removal of an enum value). Since this runs insidelistItemRelations, one stale row would break reads for the entire item rather than degrading gracefully.🛡️ Proposed defensive fallback
function mapKernelRelationRow(row: KernelRelationRow): WorkspaceKernelRelation { + const parsedKind = workspaceRelationKindSchema.safeParse(row.kind); + return { id: row.id, fromItemId: row.from_item_id, toItemId: row.to_item_id, - kind: workspaceRelationKindSchema.parse(row.kind), + kind: parsedKind.success ? parsedKind.data : (row.kind as WorkspaceRelationKind), note: row.note || null, }; }🤖 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 `@src/features/workspaces/kernel/workspace-kernel-relations.ts` around lines 77 - 85, `mapKernelRelationRow` is hard-failing on legacy or unrecognized `kind` values because `workspaceRelationKindSchema.parse(row.kind)` throws inside `listItemRelations`. Update the mapping to handle invalid `row.kind` defensively by validating with `workspaceRelationKindSchema` and falling back to a safe value or returning a nullable/filtered relation instead of throwing, so one stale row does not break reads for the whole item.src/features/workspaces/operations/read-items.ts (1)
118-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-item read + relations fetch adds avoidable latency.
readWorkspaceItemandkernel.listItemRelationsare independent kernel calls but are awaited sequentially per path. For multi-path reads this compounds linearly withinput.paths.length.⚡ Suggested fix
- const item = await readWorkspaceItem({ - item: resolution.item, - kernel: workspaceContext.kernel, - pages: input.pages, - path: resolution.path, - }); - const relations = serializeWorkspaceRelations({ - item: resolution.item, - pathsByItemId, - relations: await workspaceContext.kernel.listItemRelations({ - itemId: resolution.item.id, - }), - }); + const [item, kernelRelations] = await Promise.all([ + readWorkspaceItem({ + item: resolution.item, + kernel: workspaceContext.kernel, + pages: input.pages, + path: resolution.path, + }), + workspaceContext.kernel.listItemRelations({ itemId: resolution.item.id }), + ]); + const relations = serializeWorkspaceRelations({ + item: resolution.item, + pathsByItemId, + relations: kernelRelations, + });Longer-term, consider a bulk
listItemRelationskernel call keyed by multiple item ids to also eliminate the cross-path N+1.🤖 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 `@src/features/workspaces/operations/read-items.ts` around lines 118 - 148, The per-item fetch in read-items.ts is doing two independent kernel calls sequentially, which adds avoidable latency. In the read loop, update the readWorkspaceItem and kernel.listItemRelations flow so both requests are started together and awaited together for each resolution, while preserving the existing WorkspacePageSelectionError handling and result.items/result.failed behavior. Use the existing readWorkspaceItem, workspaceContext.kernel.listItemRelations, and serializeWorkspaceRelations symbols to locate the logic; if needed, also consider a future bulk relation fetch to reduce the N+1 pattern across multiple paths.
🤖 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 `@src/features/workspaces/operations/create-items.ts`:
- Around line 171-174: The unguarded workspaceContext.kernel.createRelations
call in createWorkspaceItemsOperation can throw after items have already been
persisted, causing the whole operation to fail and lose the accumulated
items/failed results. Wrap the createRelations step in error handling, record
the failure in the existing failed array for the current item, and continue or
return a partial result instead of letting the exception escape. Keep the fix
localized around createWorkspaceItemsOperation and its relations.relations check
so the caller still receives the items and failed collections.
In `@src/features/workspaces/operations/edit-item.ts`:
- Around line 100-102: The createRelations call in editWorkspaceItemOperation is
not guarded, so a failure after the markdown edits are already applied can
escape uncaught and hide the successful edit result. Update the result.applied /
relations.relations.length branch to handle errors from
workspaceContext.kernel.createRelations explicitly, and make sure the function
reports that the edit succeeded even if relation creation fails by catching the
exception and preserving the applied state. Use editWorkspaceItemOperation and
createRelations as the key symbols when locating the fix.
In `@src/features/workspaces/operations/workspace-tool-definitions.ts`:
- Around line 162-177: The workspace_edit_item tool wiring already forwards
relations correctly, but the current workspaceEditItemInputSchema still blocks
relations-only calls because edits is required upstream. Update the input schema
and any related validation in workspace-tool-schemas.ts to make edits optional
per the PR goal, and keep defineWorkspaceTool.execute in
workspace-tool-definitions.ts compatible with edits being undefined while still
passing relations through to editWorkspaceItemOperation.
In `@src/features/workspaces/operations/workspace-tool-schemas.ts`:
- Around line 120-134: The workspaceEditItemInputSchema still requires edits,
which blocks links-only updates and contradicts the intended optional edit flow.
Update the schema in workspaceEditItemInputSchema so edits can be omitted when
relations are provided, while still enforcing that at least one of edits or
relations is present. Then adjust the downstream editWorkspaceItemOperation and
edit-item.ts handling to safely accept an undefined or empty edits value and
skip applyMarkdownEdits when no text edits are supplied.
---
Nitpick comments:
In `@src/features/workspaces/kernel/workspace-kernel-relations.ts`:
- Around line 77-85: `mapKernelRelationRow` is hard-failing on legacy or
unrecognized `kind` values because `workspaceRelationKindSchema.parse(row.kind)`
throws inside `listItemRelations`. Update the mapping to handle invalid
`row.kind` defensively by validating with `workspaceRelationKindSchema` and
falling back to a safe value or returning a nullable/filtered relation instead
of throwing, so one stale row does not break reads for the whole item.
In `@src/features/workspaces/operations/read-items.ts`:
- Around line 118-148: The per-item fetch in read-items.ts is doing two
independent kernel calls sequentially, which adds avoidable latency. In the read
loop, update the readWorkspaceItem and kernel.listItemRelations flow so both
requests are started together and awaited together for each resolution, while
preserving the existing WorkspacePageSelectionError handling and
result.items/result.failed behavior. Use the existing readWorkspaceItem,
workspaceContext.kernel.listItemRelations, and serializeWorkspaceRelations
symbols to locate the logic; if needed, also consider a future bulk relation
fetch to reduce the N+1 pattern across multiple paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 969a2732-0e11-4120-9f0b-044a8569b836
📒 Files selected for processing (14)
src/features/workspaces/contracts.tssrc/features/workspaces/kernel/workspace-kernel-access.tssrc/features/workspaces/kernel/workspace-kernel-item-commands.tssrc/features/workspaces/kernel/workspace-kernel-relations.tssrc/features/workspaces/kernel/workspace-kernel-schema.tssrc/features/workspaces/kernel/workspace-kernel-types.tssrc/features/workspaces/kernel/workspace-kernel.tssrc/features/workspaces/operations/create-items.tssrc/features/workspaces/operations/edit-item.tssrc/features/workspaces/operations/read-items.tssrc/features/workspaces/operations/relations.tssrc/features/workspaces/operations/workspace-operation-context.tssrc/features/workspaces/operations/workspace-tool-definitions.tssrc/features/workspaces/operations/workspace-tool-schemas.ts
🛑 Comments failed to post (4)
src/features/workspaces/operations/create-items.ts (1)
171-174: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Unguarded
createRelationscall after item is already persisted.If
kernel.createRelationsthrows here, the item was already created (durable side effect), but the exception propagates out ofcreateWorkspaceItemsOperationuncaught, discarding theitems/failedarrays accumulated so far for this and all prior loop iterations. The caller gets a hard failure with no indication that some items were actually created.🤖 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 `@src/features/workspaces/operations/create-items.ts` around lines 171 - 174, The unguarded workspaceContext.kernel.createRelations call in createWorkspaceItemsOperation can throw after items have already been persisted, causing the whole operation to fail and lose the accumulated items/failed results. Wrap the createRelations step in error handling, record the failure in the existing failed array for the current item, and continue or return a partial result instead of letting the exception escape. Keep the fix localized around createWorkspaceItemsOperation and its relations.relations check so the caller still receives the items and failed collections.src/features/workspaces/operations/edit-item.ts (1)
100-102: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Unguarded
createRelationscall after edits are already applied.If
kernel.createRelationsthrows here, the markdown edits were already applied (durable side effect), but the exception propagates out ofeditWorkspaceItemOperationuncaught, and the caller never learns the edits succeeded.🤖 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 `@src/features/workspaces/operations/edit-item.ts` around lines 100 - 102, The createRelations call in editWorkspaceItemOperation is not guarded, so a failure after the markdown edits are already applied can escape uncaught and hide the successful edit result. Update the result.applied / relations.relations.length branch to handle errors from workspaceContext.kernel.createRelations explicitly, and make sure the function reports that the edit succeeded even if relation creation fails by catching the exception and preserving the applied state. Use editWorkspaceItemOperation and createRelations as the key symbols when locating the fix.src/features/workspaces/operations/workspace-tool-definitions.ts (1)
162-177: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forwarding is consistent with current schema, but inherits the edits-required gap.
executecorrectly forwardsrelationsthrough toeditWorkspaceItemOperation, matching the current (required-edits) schema. However, sinceworkspaceEditItemInputSchema.editsstill requires.min(1)(see workspace-tool-schemas.ts), a relations-only call from the model will never reach this execute function — it will fail Zod validation upstream. Onceeditsis made optional per the PR objective, this destructuring/forwarding will already work correctly sinceeditscan beundefinedhere without further changes needed to this file.🤖 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 `@src/features/workspaces/operations/workspace-tool-definitions.ts` around lines 162 - 177, The workspace_edit_item tool wiring already forwards relations correctly, but the current workspaceEditItemInputSchema still blocks relations-only calls because edits is required upstream. Update the input schema and any related validation in workspace-tool-schemas.ts to make edits optional per the PR goal, and keep defineWorkspaceTool.execute in workspace-tool-definitions.ts compatible with edits being undefined while still passing relations through to editWorkspaceItemOperation.src/features/workspaces/operations/workspace-tool-schemas.ts (1)
120-134: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
editsis still required, contradicting the stated "links-only edit" support.The PR objectives state that
workspace_edit_itemshould makeeditsoptional so that eithereditsorrelationscan be provided, enabling links-only updates. Hereeditsstill uses.min(1)without.optional(), so a relations-only call will fail validation before it ever reacheseditWorkspaceItemOperation. This defeats the primary stated goal of the PR: enabling the AI to add/update item links without touching document content.🐛 Proposed fix to make edits optional and require at least one of edits/relations
export const workspaceEditItemInputSchema = z.object({ path: z.string().min(1).describe("Absolute path of one actual ThinkEx workspace item to edit."), relations: z .array(workspaceRelationInputSchema) .max(20) .optional() .describe("Optional relationships from the edited item to existing workspace items."), edits: z .array(documentMarkdownEditSchema) - .min(1) .max(40) + .optional() .describe( `Ordered text edits to apply to the item projection. ${workspaceDocumentMarkdownMathInstruction}`, ), -}); +}).refine((data) => (data.edits?.length ?? 0) > 0 || (data.relations?.length ?? 0) > 0, { + message: "Either edits or relations must be provided.", +});Note this also requires downstream
editWorkspaceItemOperation/edit-item.ts(not in this file set) to handle anundefined/emptyeditsarray, since it currently callsinput.edits.lengthunconditionally and always invokesapplyMarkdownEdits.📝 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.export const workspaceEditItemInputSchema = z.object({ path: z.string().min(1).describe("Absolute path of one actual ThinkEx workspace item to edit."), relations: z .array(workspaceRelationInputSchema) .max(20) .optional() .describe("Optional relationships from the edited item to existing workspace items."), edits: z .array(documentMarkdownEditSchema) .max(40) .optional() .describe( `Ordered text edits to apply to the item projection. ${workspaceDocumentMarkdownMathInstruction}`, ), }).refine((data) => (data.edits?.length ?? 0) > 0 || (data.relations?.length ?? 0) > 0, { message: "Either edits or relations must be 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 `@src/features/workspaces/operations/workspace-tool-schemas.ts` around lines 120 - 134, The workspaceEditItemInputSchema still requires edits, which blocks links-only updates and contradicts the intended optional edit flow. Update the schema in workspaceEditItemInputSchema so edits can be omitted when relations are provided, while still enforcing that at least one of edits or relations is present. Then adjust the downstream editWorkspaceItemOperation and edit-item.ts handling to safely accept an undefined or empty edits value and skip applyMarkdownEdits when no text edits are supplied.
c0ae6a8 to
6868c9b
Compare
3 participants
Summary
workspace_create_items,workspace_edit_item, andworkspace_read_itemstools.kernel_items.metadata_json, so this does not add a new database table.Behavioral Change
workspace_edit_itemnow allows links-only edits by makingeditsoptional.editsorlinksmust be present.editsare unaffected.Out of Scope
Test Plan
pnpm checkpnpm test(7 tests)pnpm buildAll passing locally.
Summary by CodeRabbit
New Features
Bug Fixes
Chores