Conversation
Complete implementation of tile-based template storage with 6 subtasks:
1. Extend itemType to accept string values
- Add type guards: isBuiltInItemType(), isReservedItemType(), isCustomItemType()
- Add RESERVED_ITEM_TYPES constant
- 33 unit tests
2. Add templateName column
- Migration 0015_add_template_name_column.sql
- Update types across layers (domain → infrastructure → API)
- 19 integration tests
3. Implement Template Resolver Service
- TemplateResolverService for tile-based template lookup
- TemplateData and TemplateWithChildren types
- TemplateNotFoundError for clear error handling
- 25 unit tests
4. Migrate Built-in Templates to Tile Storage
- Seed script: drizzle/seeds/templates.seed.ts (idempotent)
- Well-known coordinates under D1i4gEqbi01JWS2F6I7GUN8ekRiU2mjK,0:1,2,*
- 33 unit tests
5. Update buildPrompt to Use Tile-Based Templates
- Tile-based template lookup with TypeScript fallback
- {{@ChildTemplateName}} pre-processor syntax for sub-templates
- 43 unit tests
6. Add User Template Allowlist Enforcement
- TemplateAllowlistService for security validation
- Built-in templates always allowed
- Case-insensitive matching
- Visibility validation (public tiles require public templates)
- 56 unit tests
Architecture improvements (Rule-of-6 compliance):
- services/_templates/ subfolder for template services
- services/_context/ subfolder for context builders
- templates/_internals/ for shared utilities
Total: 209+ new tests
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…e column The templateName column was created with VARCHAR(255) and no unique constraint, but the spec in TEMPLATES_AS_TILES.md requires: - VARCHAR(100) length limit - UNIQUE constraint for global template name uniqueness Changes: - Add migration 0016 to add UNIQUE constraint and change column to VARCHAR(100) - Update schema definition to match (length: 100, uniqueIndex) This fixes 4 failing integration tests that validate these constraints. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add user_template_allowlist table for per-user allowed templates - Create DrizzleTemplateAllowlistRepository for database operations - Add tRPC endpoints: getEffectiveAllowlist, addToAllowlist, removeFromAllowlist - Update _TypeSelectorField to fetch allowed types from API dynamically - Change itemType from MapItemType enum to ItemTypeValue (MapItemType | string) - Update entire type chain: domain → infrastructure → services → API → cache - Add type assertions for enum comparisons to fix ESLint errors - Isolate flaky template-name-column tests in test runner - Fix migration to reference "users" table (not "user") 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement two-phase template rendering with child path syntax:
- child[-3].title, child[-2,-1].content for direct field access
- child[-6..-1], child[1..6] for range iteration
- {{@RenderChildren range=[-6..-1] fallback='generic'}} for pool dispatch
New modules:
- _pre-processor/_path-parser.ts: Parse child path expressions
- _pool/: Template pool types and builder for itemType matching
- _compiler/: Two-phase compile (structural template) + render (Mustache)
Key features:
- Templates match tiles by templateName (title) to itemType
- Recursive depth control via nested template pools
- Variable prefixing: {{title}} → {{child[-3].title}}
- Inspectable compiled templates showing exact prompt structure
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…te context
- Add {{@RenderAncestors fallback='...'}} tag for ancestor pool dispatch
- Allow direction 0 in range for hexplan tiles (range=[0..0])
- Add template context support via template[-1], template[-2], etc.
- Add ancestor path syntax: ancestor[-1] (parent), ancestor[-2] (grandparent)
- Update System Task Template to use:
- {{{template[-1].content}}} for hexrun-intro
- {{@RenderAncestors fallback='ancestor'}} for ancestors
- {{@RenderChildren range=[0..0] fallback='hexplan'}} for hexplan
- Add sub-templates: system (subtasks), ancestor, hexplan
- Add hexrun-intro as composed child of template tile
- Add plan for User Interlocutor Template migration
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove the restriction that prevented user tiles (tiles with empty path) from being compose expanded. This enables users to add composed children to their root tile, which is now relevant for the template system. Changes: - Remove isUserTile check from canShowComposition in MapUI.tsx - Remove isUserTile check from DynamicFrameCore.tsx - Remove isUserTile check from BaseFrame.tsx - Update test to expect user tiles can now show composition 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add USER_SUB_TEMPLATES with organizational, context, generic, section,
and recent-history templates for type-specific rendering
- Add USER_TEMPLATE_CONTEXT for user intro content at template[-1]
- Update USER_TEMPLATE to use {{@RenderChildren}} for pool-based dispatch
- Add _buildUserTemplateTile() and _buildTaskTileForUserTemplate() helpers
- Fix _renderChild to prefer tag fallback over pool default fallback
- Simplify UserTemplateData interface for pool-based rendering
Template dispatch priority: exact itemType match → tag fallback → pool fallback
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Architecture fixes: - Export isBuiltInItemType from mapping/utils for cross-domain access - Update _prompt-builder.ts to import from mapping/utils instead of infrastructure - Add TemplateAllowlistService and DrizzleTemplateAllowlistRepository to agentic index - Update agentic router to use domain index imports - Create dependencies.json for template-allowlist infrastructure - Add template-allowlist to infrastructure subsystems Test fix: - Update BaseFrame-composition test to expect user tiles CAN show composition (aligns with cc4d2c1 which removed user tile restriction) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fix @typescript-eslint/no-unsafe-enum-comparison errors caused by comparing ItemTypeValue (MapItemType | string) with MapItemType enum. Root cause: ESLint cache was stale locally, hiding errors that CI (fresh checkout) detected. Fixed by adding explicit type assertions. Files fixed: - validation-strategy.ts (3 comparisons) - map-item-validation.ts (2 comparisons) - _map-management.service.ts (2 comparisons) - _map-item-movement-helpers.ts (1 comparison) - move-orchestrator.ts (1 comparison) - contracts.ts (1 comparison) - map-item-copy-helpers.test.ts (1 comparison) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Migrations 0013-0017 were not registered in meta/_journal.json, causing CI to skip them. This resulted in "column template_name does not exist" errors because the schema wasn't applied. Added journal entries for: - 0013_alter_tile_favorites_mapitemid_to_integer - 0014_migrate_item_types - 0015_add_template_name_column - 0016_fix_template_name_constraints - 0017_add_user_template_allowlist 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Migration fix: - Make 0013 migration idempotent - checks if column is already integer before attempting text→integer conversion (0012 already creates as int) Circular import fix: - Move item-type-utils from infrastructure to utils to break cycle: map-item.ts → utils → infrastructure → map-item.ts - Use lazy initialization for MapItemType set to avoid undefined during module loading - Update infrastructure/index.ts to note new location - Update test imports to use new utils location 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Delete old infrastructure/map-item/item-type-utils.ts (had eager initialization causing circular import) - Update _item-crud.service.ts to import isBuiltInItemType from utils instead of infrastructure The lazy-initialized version in utils/item-type-utils.ts is the correct source for these utilities. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Feature template tiles
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a comprehensive tile-based template system with custom item type support, replacing enum-based types with flexible strings. It adds user template allowlists, a two-phase template compiler/renderer, stream event extraction utilities, tool call widgets, and related database schema changes. Extensive refactoring reorganizes agentic services and generalizes item type constraints throughout the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/domains/agentic/services/task-execution.service.ts (1)
12-16: Use buildPrompt from prompt-executor.serviceThis file lives under
src/lib/domains/agentic/**, so it should usebuildPrompt()fromsrc/lib/domains/agentic/services/prompt-executor.service.tsfor deterministic XML prompt generation.As per coding guidelines, ...
♻️ Proposed import update
-import { buildPrompt } from '~/lib/domains/agentic/utils' +import { buildPrompt } from '~/lib/domains/agentic/services/prompt-executor.service'src/app/map/Cache/Handlers/NavigationHandler/_helpers/_validation/tile-converter.ts (1)
10-34: Normalize legacy null itemType to avoid leaking invalid values.
If legacy data can still produce null itemType, pass-through here will propagate null into TileData. Consider widening the input type and normalizing to undefined.✅ Suggested defensive normalization
-export function convertToTileData(item: { +export function convertToTileData(item: { id: string; coordinates: string; title: string; content: string; preview: string | undefined; link: string; parentId: string | null; ownerId: string; depth: number; - itemType: ItemTypeValue; + itemType: ItemTypeValue | null; }): TileData { @@ - itemType: item.itemType, + itemType: item.itemType ?? undefined,Based on learnings, legacy tiles with null itemType should be treated as undefined.
src/lib/domains/mapping/services/_item-services/_item-crud.service.ts (1)
552-583: Custom item types are still blocked on update under built‑in parents
_validateItemTypeForUpdatedoesn’t skip hierarchy checks for non‑built‑in types (unlike creation), so changing a structural child to a custom type under SYSTEM/CONTEXT will throw. This undermines the custom item type support and is inconsistent with creation rules.🐛 Suggested fix
const parentItemType = parent.attrs.itemType; + + // Skip hierarchy validation for non-built-in/custom types + if (!parentItemType || !isBuiltInItemType(parentItemType) || !isBuiltInItemType(newItemType)) { + return; + }src/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.ts (1)
1084-1111: Optimistic item ignores the provideditemTypeparameter.The
_createOptimisticItemmethod receivesdatawhich includesitemTypevia thecreateItemmethod signature (Line 292), but Line 1106 hardcodesMapItemType.CONTEXTinstead of using the provided type. This could cause the optimistic UI to show the wrong item type until the server response arrives.🐛 Proposed fix
private _createOptimisticItem( coordId: string, coords: ReturnType<typeof CoordSystem.parseId>, data: { title?: string; name?: string; description?: string; content?: string; preview?: string; link?: string; + itemType?: string; }, parentId: string | null ): MapItemAPIContract { return { id: `temp_${Date.now()}`, coordinates: coordId, title: data.title ?? "New Item", content: data.content ?? "", preview: data.preview, link: data.link ?? "", depth: coords.path.length, parentId, - itemType: MapItemType.CONTEXT, + itemType: data.itemType ?? MapItemType.CONTEXT, ownerId: this.config.mapContext?.userId ?? "unknown", originId: null, visibility: Visibility.PRIVATE, }; }
🤖 Fix all issues with AI agents
In `@docs/features/TEMPLATES_AS_TILES.md`:
- Around line 33-35: The docs incorrectly state the unique constraint is on
itemType; update the text in TEMPLATES_AS_TILES.md to say "Unique constraint on
templateName (for now)" and adjust the future note to "(userId, templateName)
unique constraint to allow per-user template names", referencing the fields
templateName and itemType so readers understand itemType remains non-unique
(e.g., multiple "context" or "system" tiles).
In `@drizzle/migrations/0013_alter_tile_favorites_mapitemid_to_integer.sql`:
- Around line 24-35: Before dropping and renaming columns in the tile_favorites
migration, add a check that verifies no rows have map_item_id_new IS NULL after
the UPDATE: query tile_favorites for COUNT(*) where map_item_id_new IS NULL and
RAISE EXCEPTION (or otherwise abort the migration) with a clear message (include
the count and an example key if useful) so the migration fails explicitly
instead of erroring during ALTER COLUMN; reference the tile_favorites table and
the map_item_id_new column in this guard and run it after the UPDATE and before
the DROP/RENAME/SET NOT NULL steps.
In `@drizzle/migrations/0017_add_user_template_allowlist.sql`:
- Around line 7-12: The table comment is misleading: `allowed_templates` is
defined as a PostgreSQL TEXT[] column but the preceding comment says "stores a
JSON array of template names"; update the comment to reflect the actual datatype
(e.g., "The allowed_templates column stores an array of template names as
TEXT[]") or change the column to JSONB if you intend a JSON array—modify either
the comment or the column in the user_template_allowlist definition accordingly,
referencing the user_template_allowlist table and the allowed_templates column.
In `@drizzle/seeds/templates.seed.ts`:
- Around line 114-141: The code inserts a grandparent vde_map_items row using
parent_id = rootId which can be null if the system root lookup (rootResult /
rootId) returned no row; before executing the INSERT into vde_map_items (the
mapResult/INSERT block), add an explicit guard that either throws a clear error
(e.g., "Missing system root tile") or creates the root tile first, and ensure
you reference rootResult/rootId and SYSTEM_USER_ID when performing that check so
the INSERT always gets a valid parent_id (or you handle creation beforehand).
In `@src/app/map/Chat/Timeline/Widgets/TileWidget/useTileState.tsx`:
- Around line 31-33: useTileState currently coerces a null/undefined itemType
into 'context' which misclassifies legacy untyped tiles; change the
initialization in useTileState so editItemType is set to itemType (leave it
null/undefined) instead of itemType ?? 'context', and update the other
assignment that forces 'context' (the setEditItemType call referenced around
line 64) to only set a concrete string when the user explicitly chooses a type,
otherwise leave editItemType undefined/null so legacy tiles remain untyped.
In `@src/lib/domains/agentic/services/_templates/template-allowlist.service.ts`:
- Around line 140-158: The docstring for getUserAllowlist is inaccurate: it
claims it returns "the user's custom allowed templates" but the implementation
returns BUILT_IN_TEMPLATES for anonymous users and also when an authenticated
user has no custom allowlist (via _repository.getUserAllowlist returning null).
Update the getUserAllowlist docstring to clearly state the behavior: for
null/undefined userId return built-in templates, for authenticated users with no
stored allowlist also return built-in templates, and otherwise return
userAllowlist.allowedTemplates; reference getUserAllowlist, BUILT_IN_TEMPLATES,
_repository.getUserAllowlist, and userAllowlist.allowedTemplates in the
description so callers understand the exact return cases.
In `@src/lib/domains/agentic/templates/_compiler/_render-children.ts`:
- Around line 117-123: The child render context currently reuses
context.ancestors unchanged, so the parent tile isn't included and
RenderAncestors will skip it; update the childContext construction
(CompileContext for childContext) to create a new ancestors array that appends
the current parent tile (e.g., context.taskTile) to context.ancestors
(preserving immutability) so children see the full ancestor chain when
rendering.
In `@src/lib/domains/agentic/templates/README.md`:
- Around line 161-166: Update the fenced code block that shows the template
hierarchy so it includes a language specifier (e.g., change ``` to ```text)
around the block containing Template "my-agent-template" (parent) and its
sub-templates (Sub-template "HeaderSection", "ContextBlock", "TaskSection") to
ensure proper syntax highlighting and tooling support for that example.
- Around line 114-137: The fenced code block showing the templates/ file tree in
README.md is missing a language specifier; update the opening fence for that
block (the block that starts with the "templates/" tree) from ``` to ```text so
the directory listing is rendered with the proper language
highlighting/formatting.
In `@src/lib/domains/mapping/infrastructure/map-item/README.md`:
- Around line 152-168: The README's "Template Name Column" is inconsistent with
the migrations: inspect drizzle/migrations/0015_add_template_name_column.sql and
drizzle/migrations/0016_fix_template_name_constraints.sql to confirm the final
schema for template_name, then update the README section (the "Template Name
Column" paragraph and the table entry showing `template_name`) to exactly match
the migration (e.g., change varchar(255) to varchar(100) and add the note about
the unique constraint and any nullability change), or if the migrations are
wrong, adjust the migrations to match intended docs—ensure the README text and
the migration files for template_name are in agreement.
In `@src/lib/domains/mapping/README.md`:
- Around line 105-109: The README references the wrong path for the item-type
utilities; update the doc to point to the actual location (verify whether the
utilities `isBuiltInItemType`, `isReservedItemType`, and `isCustomItemType` live
under `mapping/utils` rather than `infrastructure/map-item/item-type-utils.ts`)
and correct the import path in the documentation to match the repository (or
move/rename the files if the doc is correct); ensure the README lists the exact
file path where the functions `isBuiltInItemType`, `isReservedItemType`, and
`isCustomItemType` are defined.
In `@src/server/api/routers/agentic/agentic.ts`:
- Around line 615-649: The addToAllowlist mutation allows adding built-in
templates and stores the original (non-normalized) input, causing duplicates and
preventing removal; update addToAllowlist to reject built-in templates and
persist a normalized name: use TemplateAllowlistService.isBuiltInTemplate to
check input.templateName (normalize with toLowerCase before that check), return
a bad request if it's a built-in, derive currentTemplates by normalizing
existing entries (use service.isBuiltInTemplate and .toLowerCase for
comparison), and call repository.saveUserAllowlist with the normalized
input.templateName (not the raw input) so getUserAllowlist/getEffectiveAllowlist
work consistently.
In `@src/server/db/schema/__tests__/template-name-column.integration.test.ts`:
- Around line 57-58: The test currently silently falls back to 0 when inserting
the user by using testUserId = userItem[0]?.id ?? 0; instead assert or throw if
the insert returned no rows: after the insert that produces userItem, verify
userItem[0] exists (e.g. expect(userItem.length).toBeGreaterThan(0) or if
(!userItem[0]) throw new Error) and then assign testUserId = userItem[0].id;
this ensures the test fails immediately on insertion failure rather than
continuing with id 0; update references around testUserId and userItem
accordingly.
🧹 Nitpick comments (43)
src/lib/domains/mapping/infrastructure/map-item/queries/specialized-queries.ts (1)
563-563: Inconsistent path prefix matching in JavaScript filter.The SQL query in
_pathStartsWithcorrectly usesLIKE '${pathPrefix},%'(with comma), but this JavaScript filter usesstartsWith(centerPathString)without the comma delimiter. This could incorrectly match unrelated paths (e.g., for center "1", path "11,-1" would match since"11,-1".startsWith("1")is true).While the SQL query already filters correctly, making this redundant check inconsistent could cause subtle issues if query logic changes.
Suggested fix
- if (centerPathString && !path.startsWith(centerPathString)) return false; + if (centerPathString && !path.startsWith(centerPathString + ",")) return false;docs/plans/user-template-pool-dispatch.md (1)
4-4: Coordinate format appears inconsistent with other references.The template coordinate
D1i4gEqbi01JWS2F6I7GUN8ekRiU2mjK,0:1,2,2uses a base64-like userId prefix, while later references use bracket notation like[1,2,1]. Consider clarifying whether these represent different coordinate systems or if one format should be standardized in the document.src/lib/domains/agentic/templates/_hexrun-orchestrator-template.ts (1)
99-101: Local_hasContentduplicates utility from_internals/utils.ts.This function is identical to
_hasContentinsrc/lib/domains/agentic/templates/_internals/utils.ts. Consider importing from the shared utility to reduce duplication.♻️ Suggested refactor
-// ==================== INTERNAL UTILITIES ==================== - -function _hasContent(text: string | undefined): boolean { - return !!text && text.trim().length > 0 -} +import { _hasContent } from '~/lib/domains/agentic/templates/_internals/utils'Note: You may need to export
_hasContentfrom the utils module if it's not already exported.src/lib/domains/agentic/templates/__tests__/prompt-builder-tile-templates.test.ts (1)
73-149: Tests are spec-only; they don’t exercise buildPrompt or the template store.These assertions only check local constants and PromptData fields, so they’ll pass even if buildPrompt ignores the store/pre‑processor. Consider invoking buildPrompt with a mocked TemplateStore and asserting on output + lookup calls to validate real behavior. As per coding guidelines, use buildPrompt() from prompt-executor.service.ts in agentic code/tests.
src/server/db/schema/_tables/auth/user-template-allowlist.ts (1)
32-34: Redundant user_id index.
uniqueIndex("unique_user_allowlist")already provides an index onuserId; the extra non‑unique index adds write overhead without new query capabilities.♻️ Suggested change
}, (table) => ({ uniqueUserAllowlist: uniqueIndex("unique_user_allowlist").on(table.userId), - userIdIdx: index("user_template_allowlist_user_id_idx").on(table.userId), }));src/lib/domains/mapping/utils/item-type-utils.ts (1)
19-19: Consider usingas const satisfies readonly string[]for type safety.The current typing loses the literal type information from
as const. If you want both the literal types and the array constraint:-export const RESERVED_ITEM_TYPES: readonly string[] = ["user"] as const; +export const RESERVED_ITEM_TYPES = ["user"] as const satisfies readonly string[];This preserves the literal
"user"type while satisfying thereadonly string[]constraint.src/lib/domains/agentic/templates/_internals/section-builders.ts (1)
50-58: Subtasks wrapper is emitted even when all subtasks filter to empty strings.If all items in
structuralChildrenproduce empty strings afterGenericTile/TileOrFolder, the result would be<subtasks>\n\n</subtasks>. Consider filtering before wrapping:♻️ Proposed fix
const subtasks = structuralChildren.map(child => { if (supportFolders && child.itemType === MapItemType.ORGANIZATIONAL) { return TileOrFolder(child as TileData, ['title', 'preview'], 'subtask-preview', 3) } return GenericTile(child as TileData, ['title', 'preview'], 'subtask-preview') }) - return `<subtasks>\n${subtasks.filter(s => s.length > 0).join('\n\n')}\n</subtasks>` + const filteredSubtasks = subtasks.filter(s => s.length > 0) + if (filteredSubtasks.length === 0) { + return '' + } + return `<subtasks>\n${filteredSubtasks.join('\n\n')}\n</subtasks>` }src/lib/domains/agentic/services/_templates/template-resolver.service.ts (1)
108-124: Defensive null/undefined/type checks are unnecessary with TypeScript strict mode.The method signature declares
templateName: string, so TypeScript already prevents passingnull,undefined, or non-strings at compile time. These runtime checks add complexity without benefit in a typed codebase.If you want to keep defensive checks for JavaScript callers or runtime safety, consider a single guard:
♻️ Simplified validation
private _validateTemplateName(templateName: string): void { - if (templateName === null || templateName === undefined) { - throw new Error('Template name cannot be null or undefined') - } - - if (typeof templateName !== 'string') { - throw new Error('Template name must be a string') - } - - if (templateName.length === 0) { - throw new Error('Template name cannot be empty') - } - - if (templateName.trim().length === 0) { + if (!templateName?.trim()) { throw new Error('Template name cannot be whitespace only') } }src/lib/domains/mapping/_actions/_map-item-creation-helpers.ts (1)
82-89: The explicit cast is unnecessary and the error message may be misleading.
The cast
(MapItemType.USER as ItemTypeValue)is not needed sinceMapItemType.USERis already a validItemTypeValue(the union includesMapItemType).The error message says "BASE type item" but this branch handles all non-USER types, including custom item types.
♻️ Proposed fix
- if (itemType === (MapItemType.USER as ItemTypeValue)) { + if (itemType === MapItemType.USER) { if (parent) { throw new Error("USER type item cannot have a parentId."); } } else { if (!parent) { - throw new Error("BASE type item must have a parent."); + throw new Error("Non-USER item must have a parent."); }src/lib/domains/agentic/services/_templates/template-allowlist.service.ts (2)
198-209:validateVisibilityis unnecessarilyasync.This method contains no
awaitstatements, so theasynckeyword adds unnecessary overhead by wrapping the result in a Promise.Suggested fix
- async validateVisibility( + validateVisibility( templateName: string, tileVisibility: Visibility, templateVisibility: Visibility - ): Promise<void> { + ): void { const isPublicTileUsingPrivateTemplate = tileVisibility === 'public' && templateVisibility === 'private' if (isPublicTileUsingPrivateTemplate) { throw new TemplateVisibilityError(templateName, tileVisibility, templateVisibility) } }
133-138:isBuiltInTemplateskips template name validation.Unlike
validateAllowlist, this method doesn't call_validateTemplateName, allowing empty or whitespace-only strings to returnfalseinstead of throwing an error. Consider whether this inconsistency is intentional.Suggested fix for consistency
isBuiltInTemplate(templateName: string): boolean { + this._validateTemplateName(templateName) const normalizedTemplateName = templateName.toLowerCase() return BUILT_IN_TEMPLATES.some( builtInTemplate => builtInTemplate.toLowerCase() === normalizedTemplateName ) }src/lib/domains/mapping/_actions/map-item-actions/move-orchestrator.ts (1)
123-125: Type assertion may mask invalid item types.The
as MapItemTypecast silences the type checker but doesn't provide runtime safety. IfitemTypecontains a custom string value that's not aMapItemTypemember, this comparison will silently pass (since the value won't equalMapItemType.USER), which may be the intended behavior but could also hide bugs.Consider using a type guard or comparing against the string literal directly for clarity:
Alternative approach
- if ((targetItem.attrs.itemType as MapItemType) === MapItemType.USER && oldCoords.path.length > 0) { + if (targetItem.attrs.itemType === 'USER' && oldCoords.path.length > 0) {This makes the comparison explicit and avoids the cast, assuming
MapItemType.USERequals'USER'.#!/bin/bash # Verify MapItemType.USER value to confirm string comparison would work ast-grep --pattern 'enum MapItemType { $$$ }'src/lib/domains/agentic/services/__tests__/canvas-context-builder.test.ts (1)
146-165: Consider tracking or removing placeholder test stubs.Several test cases have empty bodies with comments like "This will be tested with the actual StandardStrategy implementation". Consider either:
- Adding a
TODOcomment with a tracking mechanism- Using
it.todo()from Vitest to mark pending tests more explicitly- Removing these stubs until the actual implementations are ready
Example using Vitest's it.todo()
describe('StandardStrategy', () => { - it('should build context with center tile and 2 generations', async () => { - // This will be tested with the actual StandardStrategy implementation - }) + it.todo('should build context with center tile and 2 generations') - it('should filter empty tiles when includeEmptyTiles is false', async () => { - // This will be tested with the actual StandardStrategy implementation - }) + it.todo('should filter empty tiles when includeEmptyTiles is false')src/lib/domains/mapping/services/_map-management.service.ts (2)
166-168: Same type assertion concern as line 138.This duplicates the casting pattern. If the refactor suggested above is applied, it should be consistent here as well.
138-140: Type assertion may mask type mismatches.Casting
rootItem.attrs.itemType as MapItemTypebypasses TypeScript's type checking. Per the AI summary, utility functions likeisBuiltInItemTypewere introduced in~/lib/domains/mapping/utils. Consider using a type guard instead for runtime safety:Suggested approach using type guard
+import { isBuiltInItemType } from '~/lib/domains/mapping/utils'; ... -if ((rootItem.attrs.itemType as MapItemType) !== MapItemType.USER) { +if (rootItem.attrs.itemType !== MapItemType.USER) { throw new Error("Cannot update map info for a non-root item."); }If
itemTypeis nowItemTypeValue(string union), direct comparison withMapItemType.USERshould work without casting sinceMapItemType.USERis a string literal.#!/bin/bash # Verify the type of itemType and available type guards rg -n "isBuiltInItemType|ItemTypeValue" --type=ts -C2 src/lib/domains/mapping/utils/src/lib/domains/mapping/types/contracts.ts (1)
49-53: Same type assertion concern as in_map-management.service.ts.Consider whether direct comparison works without casting, given
ItemTypeValueis a string union that includesMapItemTypevalues:Potential simplification
-if ((rootItem.attrs.itemType as MapItemType) !== MapItemType.USER) { +if (rootItem.attrs.itemType !== MapItemType.USER) { throw new Error( "MapContract can only be created from a USER type root MapItem.", ); }If TypeScript complains, it may indicate a type definition issue worth investigating rather than silencing with a cast.
src/lib/domains/mapping/_actions/_map-item-movement-helpers.ts (1)
92-95: Prefer a guard over a cast for USER checks.The cast suppresses type safety and can mask legacy
nullor custom string values. A guard keeps intent explicit. Based on learnings, legacy tiles may havenullitemType.♻️ Possible refactor
- if ((item.attrs.itemType as MapItemType) === MapItemType.USER) { + const itemType = item.attrs.itemType + if (itemType === MapItemType.USER) { throw new Error( "Root USER items cannot be moved to become children, and their userId/groupId cannot change via move.", ); }src/app/map/Cache/types/handlers.ts (1)
94-96: PreferItemTypeValueoverstringforitemType.
Keeps type alignment with the broader ItemTypeValue migration while still allowing custom types.♻️ Suggested change
-import type { MapItemUpdateAttributes, MapItemCreateAttributes, Visibility } from "~/lib/domains/mapping/utils"; +import type { MapItemUpdateAttributes, MapItemCreateAttributes, Visibility, ItemTypeValue } from "~/lib/domains/mapping/utils"; @@ - createItem: (coordId: string, data: Omit<MapItemCreateAttributes, 'coords' | 'itemType'> & { parentId?: number; itemType: string }) => Promise<MutationResult>; + createItem: (coordId: string, data: Omit<MapItemCreateAttributes, 'coords' | 'itemType'> & { parentId?: number; itemType: ItemTypeValue }) => Promise<MutationResult>;src/lib/domains/mapping/_objects/map-item-validation.ts (1)
16-16: Explicit casts align with ItemTypeValue migration.The casts are appropriate given the broader
ItemTypeValuestring union type now used foritemType. The comparisons will correctly handle both enum values and non-matching custom types.Consider adding a type guard or utility function if these casts proliferate across the codebase, to centralize the comparison logic:
// Example utility function isUserItemType(itemType: ItemTypeValue | null | undefined): boolean { return itemType === MapItemType.USER; }Based on the retrieved learning that unclassified legacy tiles may have
nullitemType, you may also want to verify this validation handlesnullgracefully if such tiles reach this code path.Also applies to: 51-53
src/lib/domains/agentic/services/sandbox-session/session-store-factory.ts (1)
28-29:defaultTtlSecondsis not passed toMemorySessionStore.The
MemorySessionStoredoesn't receive thedefaultTtlSecondsparameter. While the in-memory store implementation doesn't currently support TTL-based expiration, this creates an inconsistency where the parameter is effectively ignored when Redis is not configured.Consider either:
- Documenting that TTL is only enforced with Redis
- Implementing TTL support in
MemorySessionStorefor paritysrc/server/api/routers/map/map-schemas.ts (1)
57-60: Consider adding.trim()validation to reject whitespace-only item types.The current validation allows strings that contain only whitespace (e.g.,
" "), which would passmin(1)but aren't valid semantic types.♻️ Suggested refinement
-export const itemTypeSchema = z.string().min(1).refine( +export const itemTypeSchema = z.string().trim().min(1).refine( (val) => val.toLowerCase() !== "user", { message: "The 'user' item type is reserved for system-created root tiles" } );src/lib/domains/agentic/services/__tests__/template-resolver.service.test.ts (3)
104-108: Consider asserting the specific error message for empty template name.Based on the
_validateTemplateNameimplementation in the service, the error message for an empty string is'Template name cannot be empty'. Adding this specific assertion would make the test more precise and serve as better documentation.Suggested improvement
it('should throw error for empty template name', async () => { await expect(service.getTemplateByName('')) .rejects - .toThrow() + .toThrow('Template name cannot be empty') })
167-214: Test name is misleading - tests sibling count, not nesting depth.The test name "should handle deeply nested sub-template structures" suggests testing recursive nesting, but the test actually verifies handling of 6 sibling sub-templates at the same level. The
TemplateWithChildreninterface only supports one level ofsubTemplates, not recursive nesting.Suggested rename
- it('should handle deeply nested sub-template structures', async () => { + it('should handle maximum structural children (6 sub-templates)', async () => {
226-238: Consider using Vitest's built-in error assertion patterns.The try-catch pattern with
expect.fail()is verbose. Vitest provides cleaner patterns for asserting error properties. However, if you need to check multiple error properties (instanceof, message, name), the current approach is acceptable.Alternative using Vitest patterns
it('should provide descriptive error for template not found', async () => { vi.mocked(mockRepository.findByTemplateName).mockResolvedValue(null) await expect(service.getTemplateByName('missing-template')) .rejects .toSatisfy((error: Error) => { expect(error).toBeInstanceOf(TemplateNotFoundError) expect(error.message).toContain('missing-template') expect(error.name).toBe('TemplateNotFoundError') return true }) })src/server/db/schema/__tests__/template-name-column.integration.test.ts (1)
314-314: Unusual dynamic import pattern for drizzle-orm operators.The dynamic
import("drizzle-orm")is used in multiple tests instead of a static import at the top of the file. This pattern is unusual and adds runtime overhead to each test.Consider static import
import { describe, beforeEach, it, expect } from "vitest"; import { db } from "~/server/db"; import { schema } from "~/server/db"; +import { eq, and, isNull } from "drizzle-orm"; import { _createUniqueTestParams } from "~/lib/domains/mapping/services/__tests__/helpers/_test-utilities";Then replace all dynamic imports like
const { eq } = await import("drizzle-orm");with direct usage of the importedeq.src/lib/domains/mapping/types/item-attributes.ts (1)
1-1: UseItemTypeValueto keep a single source of truth.
Rawstringloses the centralized item-type alias and weakens downstream type guards. Prefer the shared alias.♻️ Proposed change
-import type { VisibilityString } from "~/lib/domains/mapping/_objects"; +import type { VisibilityString, ItemTypeValue } from "~/lib/domains/mapping/_objects"; ... - itemType?: string; + itemType?: ItemTypeValue;Also applies to: 34-39
src/lib/domains/agentic/templates/__tests__/builtin-templates.test.ts (1)
78-161: Seed-script tests are currently tautologicalMost of the “seed” assertions compare constants to hard-coded strings without invoking the actual seed function or repository, so these tests will still pass if the seed logic breaks. Consider wiring these specs to the real seed entry point (or a test repository fixture) to validate creation, idempotency, and updates end-to-end.
src/lib/domains/mapping/_objects/map-item.ts (1)
135-142: Type cast pattern is correct but consider a helper function.The
(MapItemType.USER as ItemTypeValue)cast is necessary for TypeScript type narrowing when comparing against the union type. While correct, this pattern is used in multiple places (Lines 135 and 178). Consider extracting a helper constant for readability.♻️ Optional: Extract constant for cleaner comparisons
// At module level const USER_TYPE: ItemTypeValue = MapItemType.USER; // Then use in comparisons: if (attrs.itemType === USER_TYPE) { ... }src/lib/domains/agentic/templates/_prompt-builder.ts (2)
50-79: Good routing of custom item types, but exhaustive check is now unreachable.Lines 57-59 correctly route non-built-in (custom) types to
SYSTEM_TEMPLATE. However, thedefaultbranch (Lines 74-77) with the exhaustive check becomes unreachable code since:
- Non-built-in types return early at Line 58
- All
MapItemTypeenum values are handled in the switchThe exhaustive check pattern is good practice but will never execute after the
isBuiltInItemTypeguard.♻️ Consider restructuring for clarity
function _getTemplateByItemType(itemType: ItemTypeValue | null | undefined): string { if (itemType === null || itemType === undefined) { throw new Error(TEMPLATE_LOOKUP_ERRORS.noItemType) } - // Handle custom (non-built-in) item types by using SYSTEM template - // Custom types like "template" are treated as executable tiles - if (!isBuiltInItemType(itemType)) { - return SYSTEM_TEMPLATE - } - switch (itemType) { case MapItemType.SYSTEM: return SYSTEM_TEMPLATE case MapItemType.USER: return USER_TEMPLATE case MapItemType.ORGANIZATIONAL: throw new Error(TEMPLATE_LOOKUP_ERRORS.organizationalNotSupported) case MapItemType.CONTEXT: throw new Error(TEMPLATE_LOOKUP_ERRORS.contextNotImplemented) default: - const exhaustiveCheck: never = itemType - throw new Error(`Unknown itemType: ${String(exhaustiveCheck)}`) + // Custom (non-built-in) item types use SYSTEM template + return SYSTEM_TEMPLATE } }
334-342: Direction extraction could fail silently on malformed coords.The
_extractDirectionfunction returnsundefinedfor coords without a path part, which is handled gracefully. However, if the coords format changes or is malformed (e.g., missing colon separator), the function silently returnsundefinedrather than signaling an error.src/lib/domains/agentic/services/__tests__/template-allowlist.service.test.ts (2)
270-280: Consider adding negative test case for built-in templates with private visibility.The test verifies built-in templates work with
'public'visibility, but doesn't test the scenario where a built-in template is passed with'private'template visibility. SincevalidateVisibilitydoesn't special-case built-in templates (it only checks tile vs template visibility), this test may give a false sense of security.💡 Suggested additional test case
it('should throw for built-in templates if public tile uses them with private visibility', async () => { // Built-in templates should follow the same visibility rules await expect(service.validateVisibility('system', 'public', 'private')) .rejects .toThrow(TemplateVisibilityError) })Alternatively, if built-in templates should always be allowed regardless of visibility, update the test description to clarify and add a test for that behavior:
it('should allow built-in templates for public tiles regardless of template visibility', async () => { // If built-in templates are always public by design await expect(service.validateVisibility('system', 'public', 'private')) .resolves.not.toThrow() })
170-176: Type cast masks potential undefined handling gap.Line 171 uses
undefined as unknown as UserAllowlist | nullto test undefined handling. While this is valid for testing runtime behavior, the cast bypasses TypeScript's type checking. The service method signature should ideally acceptundefinedexplicitly if this is expected behavior.src/lib/domains/agentic/infrastructure/template-allowlist/drizzle-allowlist-repository.ts (1)
34-52: Potential race condition in upsert pattern.The read-then-write pattern (
getUserAllowlistfollowed byupdate/insert) is susceptible to race conditions when concurrent requests attempt to save an allowlist for the same user. Consider using Drizzle'sonConflictDoUpdatefor an atomic upsert.♻️ Suggested atomic upsert
async saveUserAllowlist(allowlist: UserAllowlist): Promise<void> { - const existing = await this.getUserAllowlist(allowlist.userId); - - if (existing) { - await this.db - .update(userTemplateAllowlist) - .set({ - allowedTemplates: allowlist.allowedTemplates, - updatedAt: new Date(), - }) - .where(eq(userTemplateAllowlist.userId, allowlist.userId)); - } else { - await this.db.insert(userTemplateAllowlist).values({ - id: nanoid(), - userId: allowlist.userId, - allowedTemplates: allowlist.allowedTemplates, - }); - } + await this.db + .insert(userTemplateAllowlist) + .values({ + id: nanoid(), + userId: allowlist.userId, + allowedTemplates: allowlist.allowedTemplates, + }) + .onConflictDoUpdate({ + target: userTemplateAllowlist.userId, + set: { + allowedTemplates: allowlist.allowedTemplates, + updatedAt: new Date(), + }, + }); }src/lib/domains/agentic/templates/_compiler/_two-phase-render.ts (1)
103-105: Simple string matching may produce false positives.Using
includes()will match occurrences in comments or string literals. If template content could contain these tags as documentation or examples, consider using a regex that excludes commented sections or validates proper tag structure.src/lib/domains/agentic/templates/_compiler/_variable-prefixer.ts (2)
13-17: Regex may match mismatched braces.The pattern
/(\{\{\{?)(\w+)(\}\}\}?)/gwill match{{{var}}(3 open, 2 close) or{{var}}}(2 open, 3 close). If strict validation is needed, consider a more precise pattern.♻️ Stricter regex alternative
-const MUSTACHE_VAR_PATTERN = /(\{\{\{?)(\w+)(\}\}\}?)/g +const MUSTACHE_VAR_PATTERN = /(\{\{\{)(\w+)(\}\}\})|(\{\{)(\w+)(\}\})/gNote: This would require adjusting the replace callback to handle both capture groups.
42-68: Consider extracting shared prefixing logic.
prefixVariablesandprefixAncestorVariablesshare nearly identical logic, differing only in how the path string is constructed. A shared helper could reduce duplication.♻️ Proposed refactor
function _prefixWithPath(template: string, pathStr: string): string { return template.replace(MUSTACHE_VAR_PATTERN, ( match: string, openBraces: string, varName: string, closeBraces: string ) => { if (RESERVED_VARS.has(varName) || !PREFIXABLE_FIELDS.has(varName)) { return match } return `${openBraces}${pathStr}.${varName}${closeBraces}` }) } export function prefixVariables(template: string, pathPrefix: number[]): string { if (pathPrefix.length === 0) return template return _prefixWithPath(template, `child[${pathPrefix.join(',')}]`) } export function prefixAncestorVariables(template: string, ancestorIndex: number): string { return _prefixWithPath(template, `ancestor[${ancestorIndex}]`) }Also applies to: 102-124
src/lib/domains/agentic/templates/_pre-processor/index.ts (1)
147-169: Switch statement lacks compile-time exhaustiveness check.The
defaultcase throws at runtime, but if new templates are added toTemplateRegistry, TypeScript won't warn about missing handlers. Consider adding an exhaustiveness check.♻️ Add exhaustiveness check
default: + // Exhaustiveness check: ensure all registry keys are handled + const _exhaustiveCheck: never = templateName as never throw new TemplateError(`No handler for template "${templateName}"`, templateName, originalParams)Note: This will cause a compile error if
templateNametype is narrowed viakeyof TemplateRegistry, forcing updates when new templates are added.src/lib/domains/agentic/templates/_compiler/index.ts (2)
71-87: Consider documenting the default maxDepth limit.The function sets
maxDepth: 10which is reasonable for preventing runaway recursion. However, this could be surprising if deeply nested templates are expected.Consider exposing
maxDepthas an optional parameter for callers who need deeper nesting:♻️ Optional enhancement
export function compileTemplate( template: string, taskTile: TileData, templatePool: TemplatePool, - ancestors: TileData[] = [] + ancestors: TileData[] = [], + maxDepth: number = 10 ): string { const context: CompileContext = { currentPath: [], taskTile, ancestors, templatePool, - maxDepth: 10, + maxDepth, currentDepth: 0 }
151-184: Verify silent skip behavior for malformed range parameters.When a
{{@RenderChildren}}tag is found but the range parameter is missing or malformed (Line 162-164), the match is silently skipped viacontinue. This may make debugging template issues difficult.Consider logging a warning or returning an error comment in the output:
♻️ Optional: Add debug visibility
const rangeMatch = RANGE_PATTERN.exec(params) if (!rangeMatch) { + // Optionally emit warning for debugging: + // console.warn(`Invalid RenderChildren tag at index ${match.index}: missing range parameter`) continue }src/lib/domains/agentic/templates/_pre-processor/_path-parser.ts (2)
117-119: Prefix-based check may have false positives.
isChildRefuses a simple prefix check which would returntruefor malformed strings likechild[invalidorchild[]. This is acceptable for a quick filter, but callers should useparseChildReffor validation.
205-223: Consider enforcing a lower bound on ancestor index.Currently, any negative index is accepted (e.g.,
ancestor[-100]). While functionally this will just return nothing if the ancestor doesn't exist, you might want to validate against a reasonable bound for consistency with the [-6, 6] child direction validation.♻️ Optional: Add lower bound validation
// Index must be negative (e.g., -1 = parent, -2 = grandparent) - if (isNaN(index) || index >= 0) { + // Reasonable lower bound prevents unrealistic ancestor depths + if (isNaN(index) || index >= 0 || index < -100) { return null }src/lib/domains/agentic/templates/_user-template.ts (1)
81-82: Clarify the coordinate reference in the comment.The comment references a specific coordinate
D1i4gEqbi01JWS2F6I7GUN8ekRiU2mjK,0:1,2,2which appears to be a tile-based template location. This may become stale if that tile is modified.Consider linking to documentation or using a more stable reference:
-/** - * Sub-templates for USER tile pool-based dispatch. - * These match the tile-based templates at D1i4gEqbi01JWS2F6I7GUN8ekRiU2mjK,0:1,2,2. - */ +/** + * Sub-templates for USER tile pool-based dispatch. + * These provide built-in templates that can be overridden by tile-based templates. + */src/lib/domains/agentic/repositories/_helpers/stream-event-extractors.ts (1)
50-76: Consider handling JSON.stringify errors for robustness.
JSON.stringify(block.input ?? {})on Line 70 could theoretically throw ifblock.inputcontains circular references, though this is unlikely for Claude SDK events.♻️ Optional: Add try-catch for extra safety
const block = event.content_block as { id?: string; name?: string; input?: unknown } + let inputJson: string + try { + inputJson = JSON.stringify(block.input ?? {}) + } catch { + inputJson = '{}' + } return { event: { type: 'tool_call_start', toolCallId: block.id ?? '', toolName: block.name ?? '', - arguments: JSON.stringify(block.input ?? {}) + arguments: inputJson }, contentBlockIndex: event.index }
| - Built-in types (`user`, `organizational`, `context`, `system`) are reserved | ||
| - Unique constraint on `itemType` (for now) | ||
| - Future: `(userId, itemType)` unique constraint to allow per-user type names |
There was a problem hiding this comment.
Documentation inconsistency: Unique constraint is on templateName, not itemType.
Line 34 states "Unique constraint on itemType (for now)", but based on the schema and other documentation in this PR, the unique constraint is on templateName, not itemType. The itemType field is not unique (multiple tiles can have the same type like "context" or "system").
Suggested fix
**Constraints:**
- Built-in types (`user`, `organizational`, `context`, `system`) are reserved
-- Unique constraint on `itemType` (for now)
+- Unique constraint on `templateName` (globally unique identifier for template lookup)
- Future: `(userId, itemType)` unique constraint to allow per-user type names📝 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.
| - Built-in types (`user`, `organizational`, `context`, `system`) are reserved | |
| - Unique constraint on `itemType` (for now) | |
| - Future: `(userId, itemType)` unique constraint to allow per-user type names | |
| **Constraints:** | |
| - Built-in types (`user`, `organizational`, `context`, `system`) are reserved | |
| - Unique constraint on `templateName` (globally unique identifier for template lookup) | |
| - Future: `(userId, itemType)` unique constraint to allow per-user type names |
🤖 Prompt for AI Agents
In `@docs/features/TEMPLATES_AS_TILES.md` around lines 33 - 35, The docs
incorrectly state the unique constraint is on itemType; update the text in
TEMPLATES_AS_TILES.md to say "Unique constraint on templateName (for now)" and
adjust the future note to "(userId, templateName) unique constraint to allow
per-user template names", referencing the fields templateName and itemType so
readers understand itemType remains non-unique (e.g., multiple "context" or
"system" tiles).
| -- Migrate data by parsing coordinate strings | ||
| UPDATE "tile_favorites" f | ||
| SET "map_item_id_new" = m.id | ||
| FROM "vde_map_items" m | ||
| WHERE m.coord_user_id = split_part(f.map_item_id::text, ',', 1) | ||
| AND m.coord_group_id = split_part(split_part(f.map_item_id::text, ',', 2), ':', 1)::integer | ||
| AND m.path = split_part(f.map_item_id::text, ':', 2); | ||
|
|
||
| -- Swap columns | ||
| ALTER TABLE "tile_favorites" DROP COLUMN "map_item_id"; | ||
| ALTER TABLE "tile_favorites" RENAME COLUMN "map_item_id_new" TO "map_item_id"; | ||
| ALTER TABLE "tile_favorites" ALTER COLUMN "map_item_id" SET NOT NULL; |
There was a problem hiding this comment.
Guard against unmapped rows before enforcing NOT NULL.
If any tile_favorites.map_item_id doesn’t resolve to vde_map_items, map_item_id_new stays NULL and the ALTER COLUMN ... SET NOT NULL step will fail mid‑migration. Add an explicit guard to surface a clear error before swapping columns.
🛡️ Suggested guard
UPDATE "tile_favorites" f
SET "map_item_id_new" = m.id
FROM "vde_map_items" m
WHERE m.coord_user_id = split_part(f.map_item_id::text, ',', 1)
AND m.coord_group_id = split_part(split_part(f.map_item_id::text, ',', 2), ':', 1)::integer
AND m.path = split_part(f.map_item_id::text, ':', 2);
+ -- Guard against unmapped rows before enforcing NOT NULL
+ IF EXISTS (
+ SELECT 1 FROM "tile_favorites" WHERE "map_item_id_new" IS NULL
+ ) THEN
+ RAISE EXCEPTION 'Migration 0013: some tile_favorites rows could not be mapped to vde_map_items';
+ END IF;📝 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.
| -- Migrate data by parsing coordinate strings | |
| UPDATE "tile_favorites" f | |
| SET "map_item_id_new" = m.id | |
| FROM "vde_map_items" m | |
| WHERE m.coord_user_id = split_part(f.map_item_id::text, ',', 1) | |
| AND m.coord_group_id = split_part(split_part(f.map_item_id::text, ',', 2), ':', 1)::integer | |
| AND m.path = split_part(f.map_item_id::text, ':', 2); | |
| -- Swap columns | |
| ALTER TABLE "tile_favorites" DROP COLUMN "map_item_id"; | |
| ALTER TABLE "tile_favorites" RENAME COLUMN "map_item_id_new" TO "map_item_id"; | |
| ALTER TABLE "tile_favorites" ALTER COLUMN "map_item_id" SET NOT NULL; | |
| -- Migrate data by parsing coordinate strings | |
| UPDATE "tile_favorites" f | |
| SET "map_item_id_new" = m.id | |
| FROM "vde_map_items" m | |
| WHERE m.coord_user_id = split_part(f.map_item_id::text, ',', 1) | |
| AND m.coord_group_id = split_part(split_part(f.map_item_id::text, ',', 2), ':', 1)::integer | |
| AND m.path = split_part(f.map_item_id::text, ':', 2); | |
| -- Guard against unmapped rows before enforcing NOT NULL | |
| DO $$ | |
| BEGIN | |
| IF EXISTS ( | |
| SELECT 1 FROM "tile_favorites" WHERE "map_item_id_new" IS NULL | |
| ) THEN | |
| RAISE EXCEPTION 'Migration 0013: some tile_favorites rows could not be mapped to vde_map_items'; | |
| END IF; | |
| END $$; | |
| -- Swap columns | |
| ALTER TABLE "tile_favorites" DROP COLUMN "map_item_id"; | |
| ALTER TABLE "tile_favorites" RENAME COLUMN "map_item_id_new" TO "map_item_id"; | |
| ALTER TABLE "tile_favorites" ALTER COLUMN "map_item_id" SET NOT NULL; |
🤖 Prompt for AI Agents
In `@drizzle/migrations/0013_alter_tile_favorites_mapitemid_to_integer.sql` around
lines 24 - 35, Before dropping and renaming columns in the tile_favorites
migration, add a check that verifies no rows have map_item_id_new IS NULL after
the UPDATE: query tile_favorites for COUNT(*) where map_item_id_new IS NULL and
RAISE EXCEPTION (or otherwise abort the migration) with a clear message (include
the count and an example key if useful) so the migration fails explicitly
instead of erroring during ALTER COLUMN; reference the tile_favorites table and
the map_item_id_new column in this guard and run it after the UPDATE and before
the DROP/RENAME/SET NOT NULL steps.
| -- The allowed_templates column stores a JSON array of template names. | ||
|
|
||
| CREATE TABLE IF NOT EXISTS user_template_allowlist ( | ||
| id TEXT PRIMARY KEY, | ||
| user_id TEXT NOT NULL REFERENCES "users"(id) ON DELETE CASCADE, | ||
| allowed_templates TEXT[] NOT NULL DEFAULT '{}', |
There was a problem hiding this comment.
Comment/data-type mismatch for allowed_templates.
The comment says JSON array, but the column is TEXT[]. Update the comment to avoid confusion.
✏️ Suggested change
--- The allowed_templates column stores a JSON array of template names.
+-- The allowed_templates column stores a TEXT[] array of template names.🤖 Prompt for AI Agents
In `@drizzle/migrations/0017_add_user_template_allowlist.sql` around lines 7 - 12,
The table comment is misleading: `allowed_templates` is defined as a PostgreSQL
TEXT[] column but the preceding comment says "stores a JSON array of template
names"; update the comment to reflect the actual datatype (e.g., "The
allowed_templates column stores an array of template names as TEXT[]") or change
the column to JSONB if you intend a JSON array—modify either the comment or the
column in the user_template_allowlist definition accordingly, referencing the
user_template_allowlist table and the allowed_templates column.
| // Find root tile | ||
| const rootResult = await sql<Array<{ id: number }>>` | ||
| SELECT id FROM vde_map_items | ||
| WHERE coord_user_id = ${SYSTEM_USER_ID} | ||
| AND coord_group_id = 0 | ||
| AND path = '' | ||
| LIMIT 1 | ||
| ` | ||
| const rootId = rootResult[0]?.id ?? null | ||
|
|
||
| // Create grandparent organizational tile | ||
| const baseResult = await sql<Array<{ id: number }>>` | ||
| INSERT INTO vde_base_items (title, content, created_at, updated_at) | ||
| VALUES ('System', 'System-level organizational tiles', NOW(), NOW()) | ||
| RETURNING id | ||
| ` | ||
| const baseItemId = baseResult[0]?.id | ||
| if (!baseItemId) throw new Error('Failed to create grandparent base item') | ||
|
|
||
| const mapResult = await sql<Array<{ id: number }>>` | ||
| INSERT INTO vde_map_items ( | ||
| coord_user_id, coord_group_id, path, item_type, visibility, | ||
| parent_id, ref_item_id, created_at, updated_at | ||
| ) | ||
| VALUES ( | ||
| ${SYSTEM_USER_ID}, 0, ${grandparentPath}, 'organizational', 'public', | ||
| ${rootId}, ${baseItemId}, NOW(), NOW() | ||
| ) |
There was a problem hiding this comment.
Fail fast if the system root tile is missing.
If the root tile lookup returns no row, parent_id becomes NULL, which can violate constraints or create an orphaned “System” tile. Add an explicit error (or create the root) before inserting the grandparent.
🧩 Suggested guard
- const rootId = rootResult[0]?.id ?? null
+ const rootId = rootResult[0]?.id
+ if (!rootId) {
+ throw new Error(`System root tile not found for user ${SYSTEM_USER_ID}`)
+ }📝 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.
| // Find root tile | |
| const rootResult = await sql<Array<{ id: number }>>` | |
| SELECT id FROM vde_map_items | |
| WHERE coord_user_id = ${SYSTEM_USER_ID} | |
| AND coord_group_id = 0 | |
| AND path = '' | |
| LIMIT 1 | |
| ` | |
| const rootId = rootResult[0]?.id ?? null | |
| // Create grandparent organizational tile | |
| const baseResult = await sql<Array<{ id: number }>>` | |
| INSERT INTO vde_base_items (title, content, created_at, updated_at) | |
| VALUES ('System', 'System-level organizational tiles', NOW(), NOW()) | |
| RETURNING id | |
| ` | |
| const baseItemId = baseResult[0]?.id | |
| if (!baseItemId) throw new Error('Failed to create grandparent base item') | |
| const mapResult = await sql<Array<{ id: number }>>` | |
| INSERT INTO vde_map_items ( | |
| coord_user_id, coord_group_id, path, item_type, visibility, | |
| parent_id, ref_item_id, created_at, updated_at | |
| ) | |
| VALUES ( | |
| ${SYSTEM_USER_ID}, 0, ${grandparentPath}, 'organizational', 'public', | |
| ${rootId}, ${baseItemId}, NOW(), NOW() | |
| ) | |
| // Find root tile | |
| const rootResult = await sql<Array<{ id: number }>>` | |
| SELECT id FROM vde_map_items | |
| WHERE coord_user_id = ${SYSTEM_USER_ID} | |
| AND coord_group_id = 0 | |
| AND path = '' | |
| LIMIT 1 | |
| ` | |
| const rootId = rootResult[0]?.id | |
| if (!rootId) { | |
| throw new Error(`System root tile not found for user ${SYSTEM_USER_ID}`) | |
| } | |
| // Create grandparent organizational tile | |
| const baseResult = await sql<Array<{ id: number }>>` | |
| INSERT INTO vde_base_items (title, content, created_at, updated_at) | |
| VALUES ('System', 'System-level organizational tiles', NOW(), NOW()) | |
| RETURNING id | |
| ` | |
| const baseItemId = baseResult[0]?.id | |
| if (!baseItemId) throw new Error('Failed to create grandparent base item') | |
| const mapResult = await sql<Array<{ id: number }>>` | |
| INSERT INTO vde_map_items ( | |
| coord_user_id, coord_group_id, path, item_type, visibility, | |
| parent_id, ref_item_id, created_at, updated_at | |
| ) | |
| VALUES ( | |
| ${SYSTEM_USER_ID}, 0, ${grandparentPath}, 'organizational', 'public', | |
| ${rootId}, ${baseItemId}, NOW(), NOW() | |
| ) |
🤖 Prompt for AI Agents
In `@drizzle/seeds/templates.seed.ts` around lines 114 - 141, The code inserts a
grandparent vde_map_items row using parent_id = rootId which can be null if the
system root lookup (rootResult / rootId) returned no row; before executing the
INSERT into vde_map_items (the mapResult/INSERT block), add an explicit guard
that either throws a clear error (e.g., "Missing system root tile") or creates
the root tile first, and ensure you reference rootResult/rootId and
SYSTEM_USER_ID when performing that check so the INSERT always gets a valid
parent_id (or you handle creation beforehand).
| const [editItemType, setEditItemType] = useState<string>( | ||
| itemType ?? 'context' | ||
| ); |
There was a problem hiding this comment.
Avoid coercing null itemType to 'context'.
Line 31-33 and Line 64 currently coerce null/undefined into 'context', which will misclassify legacy tiles that should remain “untyped.” Please preserve nullish values and only set a concrete type when the user explicitly chooses one.
💡 Suggested adjustment
- const [editItemType, setEditItemType] = useState<string>(
- itemType ?? 'context'
- );
+ const [editItemType, setEditItemType] = useState<string | undefined>(
+ itemType ?? undefined
+ );
...
- setEditItemType(itemType ?? 'context');
+ setEditItemType(itemType ?? undefined);Based on learnings, unclassified legacy tiles with null itemType should remain undefined.
Also applies to: 64-64
🤖 Prompt for AI Agents
In `@src/app/map/Chat/Timeline/Widgets/TileWidget/useTileState.tsx` around lines
31 - 33, useTileState currently coerces a null/undefined itemType into 'context'
which misclassifies legacy untyped tiles; change the initialization in
useTileState so editItemType is set to itemType (leave it null/undefined)
instead of itemType ?? 'context', and update the other assignment that forces
'context' (the setEditItemType call referenced around line 64) to only set a
concrete string when the user explicitly chooses a type, otherwise leave
editItemType undefined/null so legacy tiles remain untyped.
| ``` | ||
| Template "my-agent-template" (parent) | ||
| ├── [1] Sub-template "HeaderSection" | ||
| ├── [2] Sub-template "ContextBlock" | ||
| └── [3] Sub-template "TaskSection" | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The template hierarchy example should have a language specifier.
📝 Proposed fix
-```
+```text
Template "my-agent-template" (parent)
├── [1] Sub-template "HeaderSection"
...📝 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.
| ``` | |
| Template "my-agent-template" (parent) | |
| ├── [1] Sub-template "HeaderSection" | |
| ├── [2] Sub-template "ContextBlock" | |
| └── [3] Sub-template "TaskSection" | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@src/lib/domains/agentic/templates/README.md` around lines 161 - 166, Update
the fenced code block that shows the template hierarchy so it includes a
language specifier (e.g., change ``` to ```text) around the block containing
Template "my-agent-template" (parent) and its sub-templates (Sub-template
"HeaderSection", "ContextBlock", "TaskSection") to ensure proper syntax
highlighting and tooling support for that example.
| | `template_name` | varchar(255) | Template identifier for template provenance tracking | | ||
| | `created_at` | timestamp | Creation timestamp | | ||
| | `updated_at` | timestamp | Last update timestamp | | ||
|
|
||
| ### Template Name Column | ||
|
|
||
| The `template_name` column tracks template provenance for tiles: | ||
|
|
||
| **Purpose:** | ||
| - Records which template a tile was created from (if any) | ||
| - Enables "created from template X" features in the UI | ||
| - Supports the Templates-as-Tiles feature where templates are stored as tile data | ||
|
|
||
| **Constraints:** | ||
| - Nullable: Most tiles are not created from templates | ||
| - Max length: 255 characters | ||
| - No uniqueness constraint: Multiple tiles can reference the same template |
There was a problem hiding this comment.
Documentation may be inconsistent with migration constraints.
Based on the AI summary, migration 0016_fix_template_name_constraints.sql narrows template_name to VARCHAR(100) and adds a unique constraint, but this documentation states:
- Line 152:
varchar(255) - Line 168: "No uniqueness constraint"
Please verify the documentation matches the final schema after migrations.
#!/bin/bash
# Verify template_name column constraints in migrations
echo "=== Migration 0015 (initial) ==="
cat drizzle/migrations/0015_add_template_name_column.sql 2>/dev/null || echo "File not found"
echo ""
echo "=== Migration 0016 (constraints fix) ==="
cat drizzle/migrations/0016_fix_template_name_constraints.sql 2>/dev/null || echo "File not found"🤖 Prompt for AI Agents
In `@src/lib/domains/mapping/infrastructure/map-item/README.md` around lines 152 -
168, The README's "Template Name Column" is inconsistent with the migrations:
inspect drizzle/migrations/0015_add_template_name_column.sql and
drizzle/migrations/0016_fix_template_name_constraints.sql to confirm the final
schema for template_name, then update the README section (the "Template Name
Column" paragraph and the table entry showing `template_name`) to exactly match
the migration (e.g., change varchar(255) to varchar(100) and add the note about
the unique constraint and any nullability change), or if the migrations are
wrong, adjust the migrations to match intended docs—ensure the README text and
the migration files for template_name are in agreement.
| **Utilities** (in `infrastructure/map-item/item-type-utils.ts`): | ||
| - `isBuiltInItemType()` - Type guard for MapItemType enum values | ||
| - `isReservedItemType()` - Check if type is reserved (only "user") | ||
| - `isCustomItemType()` - Check if type is custom (non-built-in) | ||
|
|
There was a problem hiding this comment.
Doc path for item-type utils looks off.
The utilities appear to live under mapping/utils rather than infrastructure/map-item. Please verify the path to avoid stale docs.
✏️ Suggested doc fix
-**Utilities** (in `infrastructure/map-item/item-type-utils.ts`):
+**Utilities** (in `utils/item-type-utils.ts`):📝 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.
| **Utilities** (in `infrastructure/map-item/item-type-utils.ts`): | |
| - `isBuiltInItemType()` - Type guard for MapItemType enum values | |
| - `isReservedItemType()` - Check if type is reserved (only "user") | |
| - `isCustomItemType()` - Check if type is custom (non-built-in) | |
| **Utilities** (in `utils/item-type-utils.ts`): | |
| - `isBuiltInItemType()` - Type guard for MapItemType enum values | |
| - `isReservedItemType()` - Check if type is reserved (only "user") | |
| - `isCustomItemType()` - Check if type is custom (non-built-in) | |
🤖 Prompt for AI Agents
In `@src/lib/domains/mapping/README.md` around lines 105 - 109, The README
references the wrong path for the item-type utilities; update the doc to point
to the actual location (verify whether the utilities `isBuiltInItemType`,
`isReservedItemType`, and `isCustomItemType` live under `mapping/utils` rather
than `infrastructure/map-item/item-type-utils.ts`) and correct the import path
in the documentation to match the repository (or move/rename the files if the
doc is correct); ensure the README lists the exact file path where the functions
`isBuiltInItemType`, `isReservedItemType`, and `isCustomItemType` are defined.
| // Add a template to the user's allowlist | ||
| addToAllowlist: protectedProcedure | ||
| .input(z.object({ templateName: z.string().min(1) })) | ||
| .mutation(async ({ input, ctx }) => { | ||
| const repository = new DrizzleTemplateAllowlistRepository(db) | ||
| const service = new TemplateAllowlistService(repository) | ||
|
|
||
| const userId = ctx.session?.userId | ||
| if (!userId) { | ||
| _throwBadRequest('User ID is required to modify allowlist') | ||
| } | ||
|
|
||
| // Get current allowlist | ||
| const currentAllowlist = await service.getUserAllowlist(userId) | ||
| const currentTemplates = currentAllowlist.filter( | ||
| t => !service.isBuiltInTemplate(t) | ||
| ) | ||
|
|
||
| // Add new template if not already present | ||
| const normalizedNewTemplate = input.templateName.toLowerCase() | ||
| const alreadyExists = currentTemplates.some( | ||
| t => t.toLowerCase() === normalizedNewTemplate | ||
| ) | ||
|
|
||
| if (!alreadyExists) { | ||
| await repository.saveUserAllowlist({ | ||
| userId, | ||
| allowedTemplates: [...currentTemplates, input.templateName] | ||
| }) | ||
| } | ||
|
|
||
| // Return updated effective allowlist | ||
| const updatedAllowlist = await service.getEffectiveAllowlist(userId) | ||
| return { allowedTypes: updatedAllowlist } | ||
| }), |
There was a problem hiding this comment.
Guard against adding built‑in templates and normalize names before persist.
As written, a built‑in template can be added to the user allowlist, then it can’t be removed (removal blocks built‑ins). This can pollute user allowlists and potentially surface duplicates.
🧩 Suggested fix
addToAllowlist: protectedProcedure
.input(z.object({ templateName: z.string().min(1) }))
.mutation(async ({ input, ctx }) => {
const repository = new DrizzleTemplateAllowlistRepository(db)
const service = new TemplateAllowlistService(repository)
const userId = ctx.session?.userId
if (!userId) {
_throwBadRequest('User ID is required to modify allowlist')
}
+ const normalizedNewTemplate = input.templateName.trim()
+ if (!normalizedNewTemplate) {
+ _throwBadRequest('Template name must not be empty')
+ }
+ if (service.isBuiltInTemplate(normalizedNewTemplate)) {
+ _throwBadRequest('Built-in templates are already allowed')
+ }
+
// Get current allowlist
const currentAllowlist = await service.getUserAllowlist(userId)
const currentTemplates = currentAllowlist.filter(
t => !service.isBuiltInTemplate(t)
)
// Add new template if not already present
- const normalizedNewTemplate = input.templateName.toLowerCase()
+ const normalizedLower = normalizedNewTemplate.toLowerCase()
const alreadyExists = currentTemplates.some(
- t => t.toLowerCase() === normalizedNewTemplate
+ t => t.toLowerCase() === normalizedLower
)
if (!alreadyExists) {
await repository.saveUserAllowlist({
userId,
- allowedTemplates: [...currentTemplates, input.templateName]
+ allowedTemplates: [...currentTemplates, normalizedNewTemplate]
})
}🤖 Prompt for AI Agents
In `@src/server/api/routers/agentic/agentic.ts` around lines 615 - 649, The
addToAllowlist mutation allows adding built-in templates and stores the original
(non-normalized) input, causing duplicates and preventing removal; update
addToAllowlist to reject built-in templates and persist a normalized name: use
TemplateAllowlistService.isBuiltInTemplate to check input.templateName
(normalize with toLowerCase before that check), return a bad request if it's a
built-in, derive currentTemplates by normalizing existing entries (use
service.isBuiltInTemplate and .toLowerCase for comparison), and call
repository.saveUserAllowlist with the normalized input.templateName (not the raw
input) so getUserAllowlist/getEffectiveAllowlist work consistently.
…n.test.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Add toolCalls field to Message interface for embedded tool call data - Track tool calls per stream in streaming handlers and attach to messages - Render ToolCallWidget components inside message boxes - Filter out standalone tool-call widgets from timeline (now embedded) - Restyle CollapsiblePrompt to match ToolCallWidget appearance - Add extractToolResult() to capture tool results with error detection - Update backend to emit tool_call_end with proper result/error fields - Update tests to reflect new embedded tool call architecture Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/map/Chat/_hooks/_streaming-chat-callbacks.ts`:
- Around line 95-110: The code currently sets parsedArguments = {} and always
calls chatState.updateToolCallWidget(toolCallId, resultContent, success,
parsedArguments), which can overwrite previously captured args when argsString
is missing or parse fails; change the logic so parsedArguments is only set on
successful JSON.parse (leave it undefined otherwise) and call
chatState.updateToolCallWidget with that undefined when no valid args exist
(i.e., only pass parsedArguments when argsString is truthy and parse succeeded),
referencing parsedArguments, argsString, updateToolCallWidget, and toolCallId to
locate where to adjust the call; do not alter chatState.endToolCall/getStreamId
usage.
In `@src/app/map/Chat/Timeline/_components/CollapsiblePrompt.tsx`:
- Around line 22-41: The collapsible button currently lacks disclosure semantics
and a type, so update the button in CollapsiblePrompt to include type="button"
to prevent implicit form submits and add aria-expanded={isOpen} and
aria-controls referencing a stable id (e.g., derive from "collapsible-prompt-" +
unique suffix) so screen readers know the state; wrap the content div that
renders MarkdownRenderer with an id matching aria-controls and add role="region"
plus aria-labelledby pointing back to the button id (or use the button id as the
label) so the region is associated with the button and exposes the prompt
content (prompt) to assistive tech while preserving existing setIsOpen and
isOpen logic.
🧹 Nitpick comments (7)
src/app/map/Chat/Timeline/_components/CollapsiblePrompt.tsx (2)
9-19: Persist open/closed state in localStorage to align with caching guidance.Storing only the boolean state avoids sensitive prompt data while meeting the localStorage caching guideline and preserving user preference across navigation. As per coding guidelines, consider this change.
♻️ Suggested implementation
-'use client'; +'use client'; -import { useState } from 'react'; +import { useEffect, useState } from 'react'; @@ export function CollapsiblePrompt({ prompt }: CollapsiblePromptProps) { - const [isOpen, setIsOpen] = useState(false); + const [isOpen, setIsOpen] = useState(false); + const storageKey = 'collapsiblePrompt.isOpen'; + + useEffect(() => { + const saved = localStorage.getItem(storageKey); + if (saved !== null) setIsOpen(saved === 'true'); + }, []); + + useEffect(() => { + localStorage.setItem(storageKey, String(isOpen)); + }, [isOpen]);
23-23: Prefer functional state update for toggles.Avoids stale closures if the handler is ever reused in async contexts.
🧹 Proposed tweak
- onClick={() => setIsOpen(!isOpen)} + onClick={() => setIsOpen((prev) => !prev)}src/lib/domains/agentic/repositories/_helpers/stream-event-extractors.ts (1)
64-73: Fallback to empty string for missingidornamemay mask malformed events.When
block.idorblock.nameis undefined, this returns empty strings rather than signaling an invalid event. If downstream code relies on non-emptytoolCallIdortoolNamefor correlation (e.g., thependingToolCallsmap inclaude-agent-sdk.repository.ts), empty IDs could cause silent failures or orphaned entries.Consider returning
undefinedwhen required fields are missing, or at minimum log a warning when falling back to empty strings.🔧 Optional: Return undefined for malformed tool_use blocks
const block = event.content_block as { id?: string; name?: string; input?: unknown } + // Require id and name for valid tool call start + if (!block.id || !block.name) { + return undefined + } return { event: { type: 'tool_call_start', - toolCallId: block.id ?? '', - toolName: block.name ?? '', + toolCallId: block.id, + toolName: block.name, arguments: JSON.stringify(block.input ?? {}) }, contentBlockIndex: event.index }src/lib/domains/agentic/repositories/claude-agent-sdk.repository.ts (1)
273-274: Consider cleanup for orphaned pending tool calls.The
pendingToolCallsmap tracks tool calls awaiting results, but if a tool result never arrives (e.g., due to stream termination or errors), these entries remain orphaned for the duration of the streaming session. While this is unlikely to cause memory issues in practice, consider clearingpendingToolCallsat the end of the streaming loop or logging a warning if entries remain.src/app/map/Chat/Timeline/_components/_renderers/_tool-call-renderer.tsx (1)
1-15: Add a type guard to avoid runtime crashes if misused.Right now, a non–tool-call widget would cause
data.toolNameaccess on the wrong shape. A simple guard makes the helper safer for future callers.♻️ Suggested change
export function _renderToolCallWidget(widget: Widget) { - const data = widget.data as ToolCallWidgetData; + if (widget.type !== 'tool-call') return null; + const data = widget.data as ToolCallWidgetData; return ( <ToolCallWidget toolName={data.toolName} arguments={data.arguments} status={data.status} result={data.result} /> ); }src/app/map/Chat/_state/_events/event.types.ts (1)
45-54: Reduce duplication between ToolCallData and ToolCallWidgetData.ToolCallWidgetData repeats the same fields as ToolCallData. Consider reusing the base type to keep them in sync.
♻️ Suggested change
export interface ToolCallData { toolCallId: string; toolName: string; arguments: Record<string, unknown>; status: 'running' | 'completed' | 'failed'; result?: string; } @@ -export interface ToolCallWidgetData { - toolCallId: string; - streamId: string; - toolName: string; - arguments: Record<string, unknown>; - status: 'running' | 'completed' | 'failed'; - result?: string; -} +export interface ToolCallWidgetData extends ToolCallData { + streamId: string; +}Also applies to: 63-64, 179-187
src/app/map/Chat/Timeline/Widgets/ToolCallWidget/ToolCallWidget.tsx (1)
32-65: Addaria-expandedfor the collapsible toggle.Small a11y boost for assistive tech users.
♿ Proposed tweak
- <button - onClick={() => setIsOpen(!isOpen)} - className="flex items-center gap-1 text-xs text-muted-foreground hover:text-foreground transition-colors" - > + <button + onClick={() => setIsOpen(!isOpen)} + aria-expanded={isOpen} + className="flex items-center gap-1 text-xs text-muted-foreground hover:text-foreground transition-colors" + >
| // Parse the final arguments from the end event (args are streamed via deltas) | ||
| let parsedArguments: Record<string, unknown> = {} | ||
| if (argsString) { | ||
| try { | ||
| parsedArguments = JSON.parse(argsString) as Record<string, unknown> | ||
| } catch { | ||
| // Keep empty object if parsing fails | ||
| } | ||
| } | ||
|
|
||
| // End tool call in message operations | ||
| chatState.endToolCall(getStreamId(), toolCallId, resultContent, success) | ||
|
|
||
| // Update tool call widget | ||
| chatState.updateToolCallWidget(toolCallId, resultContent, success) | ||
| // Update tool call widget with final arguments | ||
| chatState.updateToolCallWidget(toolCallId, resultContent, success, parsedArguments) | ||
|
|
There was a problem hiding this comment.
Avoid wiping tool-call arguments when end event omits args.
parsedArguments defaults to {} and is always passed to updateToolCallWidget, so a missing/empty argsString can overwrite previously captured arguments. Consider only passing parsed arguments when they exist (and parsed successfully), letting the widget retain prior args.
🐛 Suggested change
- let parsedArguments: Record<string, unknown> = {}
- if (argsString) {
- try {
- parsedArguments = JSON.parse(argsString) as Record<string, unknown>
- } catch {
- // Keep empty object if parsing fails
- }
- }
+ let parsedArguments: Record<string, unknown> | undefined
+ if (argsString) {
+ try {
+ parsedArguments = JSON.parse(argsString) as Record<string, unknown>
+ } catch {
+ // Leave undefined if parsing fails
+ }
+ }
@@
- chatState.updateToolCallWidget(toolCallId, resultContent, success, parsedArguments)
+ chatState.updateToolCallWidget(toolCallId, resultContent, success, parsedArguments)Run the following to confirm updateToolCallWidget can accept undefined and preserves existing args:
#!/bin/bash
rg -n -C3 "\bupdateToolCallWidget\b" --type=ts🤖 Prompt for AI Agents
In `@src/app/map/Chat/_hooks/_streaming-chat-callbacks.ts` around lines 95 - 110,
The code currently sets parsedArguments = {} and always calls
chatState.updateToolCallWidget(toolCallId, resultContent, success,
parsedArguments), which can overwrite previously captured args when argsString
is missing or parse fails; change the logic so parsedArguments is only set on
successful JSON.parse (leave it undefined otherwise) and call
chatState.updateToolCallWidget with that undefined when no valid args exist
(i.e., only pass parsedArguments when argsString is truthy and parse succeeded),
referencing parsedArguments, argsString, updateToolCallWidget, and toolCallId to
locate where to adjust the call; do not alter chatState.endToolCall/getStreamId
usage.
| <button | ||
| onClick={() => setIsOpen(!isOpen)} | ||
| className="flex items-center gap-2 w-full text-left" | ||
| > | ||
| <Wrench className="h-3.5 w-3.5 text-muted-foreground" /> | ||
| <span className="text-sm font-medium">hexecute</span> | ||
| <Sparkles className="h-3.5 w-3.5 text-primary" /> | ||
| <span className="text-xs text-muted-foreground ml-auto flex items-center gap-1"> | ||
| {isOpen ? ( | ||
| <ChevronDown className="h-3 w-3" /> | ||
| ) : ( | ||
| <ChevronRight className="h-3 w-3" /> | ||
| )} | ||
| <span>Backend Prompt</span> | ||
| </span> | ||
| </button> | ||
| {isOpen && ( | ||
| <div className="mt-2 p-3 bg-muted/30 rounded-md overflow-x-auto max-h-96 overflow-y-auto"> | ||
| <MarkdownRenderer content={prompt} isSystemMessage={false} /> | ||
| </div> |
There was a problem hiding this comment.
Add disclosure semantics and button type for accessibility.
Custom collapsibles should expose state and an associated region for screen readers; also avoid implicit form submission.
🔧 Proposed fix
<button
+ type="button"
onClick={() => setIsOpen(!isOpen)}
+ aria-expanded={isOpen}
+ aria-controls="collapsible-prompt-panel"
className="flex items-center gap-2 w-full text-left"
>
@@
- {isOpen && (
- <div className="mt-2 p-3 bg-muted/30 rounded-md overflow-x-auto max-h-96 overflow-y-auto">
+ {isOpen && (
+ <div
+ id="collapsible-prompt-panel"
+ role="region"
+ aria-label="Backend prompt"
+ className="mt-2 p-3 bg-muted/30 rounded-md overflow-x-auto max-h-96 overflow-y-auto"
+ >
<MarkdownRenderer content={prompt} isSystemMessage={false} />
</div>
)}📝 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.
| <button | |
| onClick={() => setIsOpen(!isOpen)} | |
| className="flex items-center gap-2 w-full text-left" | |
| > | |
| <Wrench className="h-3.5 w-3.5 text-muted-foreground" /> | |
| <span className="text-sm font-medium">hexecute</span> | |
| <Sparkles className="h-3.5 w-3.5 text-primary" /> | |
| <span className="text-xs text-muted-foreground ml-auto flex items-center gap-1"> | |
| {isOpen ? ( | |
| <ChevronDown className="h-3 w-3" /> | |
| ) : ( | |
| <ChevronRight className="h-3 w-3" /> | |
| )} | |
| <span>Backend Prompt</span> | |
| </span> | |
| </button> | |
| {isOpen && ( | |
| <div className="mt-2 p-3 bg-muted/30 rounded-md overflow-x-auto max-h-96 overflow-y-auto"> | |
| <MarkdownRenderer content={prompt} isSystemMessage={false} /> | |
| </div> | |
| <button | |
| type="button" | |
| onClick={() => setIsOpen(!isOpen)} | |
| aria-expanded={isOpen} | |
| aria-controls="collapsible-prompt-panel" | |
| className="flex items-center gap-2 w-full text-left" | |
| > | |
| <Wrench className="h-3.5 w-3.5 text-muted-foreground" /> | |
| <span className="text-sm font-medium">hexecute</span> | |
| <Sparkles className="h-3.5 w-3.5 text-primary" /> | |
| <span className="text-xs text-muted-foreground ml-auto flex items-center gap-1"> | |
| {isOpen ? ( | |
| <ChevronDown className="h-3 w-3" /> | |
| ) : ( | |
| <ChevronRight className="h-3 w-3" /> | |
| )} | |
| <span>Backend Prompt</span> | |
| </span> | |
| </button> | |
| {isOpen && ( | |
| <div | |
| id="collapsible-prompt-panel" | |
| role="region" | |
| aria-label="Backend prompt" | |
| className="mt-2 p-3 bg-muted/30 rounded-md overflow-x-auto max-h-96 overflow-y-auto" | |
| > | |
| <MarkdownRenderer content={prompt} isSystemMessage={false} /> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
In `@src/app/map/Chat/Timeline/_components/CollapsiblePrompt.tsx` around lines 22
- 41, The collapsible button currently lacks disclosure semantics and a type, so
update the button in CollapsiblePrompt to include type="button" to prevent
implicit form submits and add aria-expanded={isOpen} and aria-controls
referencing a stable id (e.g., derive from "collapsible-prompt-" + unique
suffix) so screen readers know the state; wrap the content div that renders
MarkdownRenderer with an id matching aria-controls and add role="region" plus
aria-labelledby pointing back to the button id (or use the button id as the
label) so the region is associated with the button and exposes the prompt
content (prompt) to assistive tech while preserving existing setIsOpen and
isOpen logic.
…e; update integration test setup
- Create _components subsystem with proper dependencies.json and README.md - Add index.ts to expose public API for _components subsystem - Update imports across Timeline/_core and Widgets to use subsystem interface - Re-export ToolCallWidgetData from _state/index.ts for external consumers - Register _components as subsystem in Timeline/dependencies.json Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move widget renderer functions from _components/_renderers/ to Widgets/Adapters/ to better organize the codebase. The Adapters subsystem transforms Widget state objects into Widget UI components. - Create Widgets/Adapters/ with adapter files and dependencies - Move _markdown-components.tsx to _components/ (used by message rendering) - Update Widgets/index.ts to re-export Adapters - Remove _components as a subsystem (no longer has dependencies.json) - Update imports in _core files to use Widgets instead of _components Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add new traversal subsystem that finds leaf tiles (tiles with no structural children) in a hexagonal hierarchy. Supports depth-first traversal in direction order (1-6), skipping composed children and hexplans. Key features: - getAllLeafTiles: Returns all leaves under a root coordinate - getNextIncompleteLeaf: Finds first incomplete leaf given a completion set - Handles meta-leaf pattern where tiles gain children between traversals Part of run orchestration feature (Plan 1 of 5). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… orchestration - Add parseAgentResponse utility to extract structured status from agent responses - Update SYSTEM template with execution-instructions section and blockage context - Remove orchestrator template - orchestration now handled at API layer - Add wasBlocked/blockageReason fields to PromptData for resuming blocked runs - Update tests and documentation Plan 3 of run-orchestration feature. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add new _run-services subsystem to manage "run" objects that track autonomous execution state of SYSTEM tiles. Includes database table, repository, service, and 24 integration tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements external orchestration pattern where each API call executes one leaf tile and returns. Callers loop until isComplete is true. - Add agentic.run tRPC mutation with tile type validation - Add run MCP tool handler for agent access - Integrate LeafTraversalService and RunService from domain layer - Parse agent responses for completion/blockage status - Include blockage context when resuming failed runs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add frontend integration for SYSTEM tile execution via the agentic.run endpoint. SYSTEM tiles and custom type tiles now show a "Run" action in the context menu that triggers execution. Changes: - Add useRun hook for managing run execution state and callbacks - Add run-actions.ts builder for SYSTEM tile menu items - Wire up onRunClick through context menu component chain - Add 18 tests for useRun hook covering all status transitions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Change coord_user_id from varchar(255) to text in schema to match actual database type (converted in migration 0010) - Regenerate snapshots 0007-0011 with correct UUID format - Add missing snapshots 0012-0017 for manual migrations This fixes drizzle-kit "malformed snapshot" errors and eliminates the data-loss warning when running migrations locally. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ion input Replace immediate run execution with an interactive RunWidget that: - Accepts optional instructions before starting execution - Shows real-time execution progress with current step - Displays clickable tile navigation links for executed steps - Tracks elapsed time during execution - Handles blocked state with resume functionality - Shows completion summary with all executed steps The widget integrates with the existing useRun hook and follows the event-driven architecture pattern used by other widgets. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add hexecutePrompt to run mutation response for transparency - Create PromptDisplay component with collapsible prompt view - Update RunningState to show current step's prompt - Update StepsList with per-step prompt expansion - Update BlockedState with input field for resume context - Add resumeWithInput to pass user context when resuming Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Added hexplan content handling in useRunWidget, including current step and parent hexplan data. - Updated useRun hook to pass step title and hexplan content on step completion and blockage. - Modified RunService to resume blocked runs instead of creating new ones, ensuring proper state management. - Implemented hexplan fetching and instruction propagation in the agentic router. - Enhanced prompt generation to include hexplan content and instructions for better user guidance. - Updated tests to cover new hexplan functionality and ensure proper integration.
- Restructure prompt template: execution instructions at beginning, hexplan at end for better agent context - Update execution instructions to guide agents on editing hexplan (not creating) with discussion flow guidance - Remove [User input on resume] prefix - backend now handles formatting - Append user feedback to hexplan end instead of prepending instruction - Update hexplan generators to use Initial Instruction format and clearer status markers for discussion history Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… RunWidget - Add ToolCallEntry type for tracking MCP tool calls during execution - Extend ExecutionLogEntry with hexplanContent and toolCalls fields - Update RunService.markStepCompleted/markStepBlocked to store new fields - Capture tool calls via StreamCallbacks in run mutation - Show executed steps in idle state for historical visibility - Add purple-themed tool calls display with collapsible arguments/results Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
getRunState was using getOpenRun() which only found runs with status='open', causing blocked runs to not display when opening the RunWidget. - Rename getOpenRun() to getResumableRun() for clarity - Use findResumableByRootCoords() to include both open and blocked runs - Remove unused findOpenByRootCoords() repository method - Update tests to verify blocked runs are now returned Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…edState UI - Add /run command: shows RunsListWidget (no args) or RunWidget (with coords) - Create RunsListWidget with status filter (Active/Closed/All) and run list - Add listRuns backend: repository, service, and tRPC procedure - Extract StepsList into subsystem with separate components for better maintainability - Redesign BlockedState with compact layout, collapsible sections, and inline controls - Add CollapsibleSection component for reusable expandable UI - Add cleanup-runs.ts script for database maintenance Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…gation - Fix empty hexplan status bug: treat empty as PENDING, not COMPLETE - Simplify EXECUTION_INSTRUCTIONS_SECTION to concise one-liner - Simplify HEXRUN_INTRO to one-liner - Remove step-based orchestration from hexplan (RunService handles this) - Simplify generateParentHexplanContent/generateLeafHexplanContent to just contain instruction (no more 📋 steps tracking) - Restore ancestor hexplan inclusion for instruction propagation - Skip step-based instructions when hexplan is empty - Update tests to match simplified hexplan format The key insight: RunService now handles orchestration externally, so hexplans no longer need step tracking. They're now just for instruction propagation from root to subtasks via ancestor context. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
LeafTraversalService was explicitly excluding the root tile from being considered a leaf, even when it had no structural children. This caused `run` to immediately close without executing anything when called on a tile without subtasks. Now when getAllLeafTiles is called on a tile with no children, the tile itself is returned as the leaf to execute. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The Claude Agent SDK spawns a subprocess with the prompt as command-line arguments, which has OS size limits (~128KB-2MB). When prompts exceed this limit, Node throws a cryptic "spawn E2BIG" error. Added _checkPromptSize() helper that throws a helpful PAYLOAD_TOO_LARGE error with guidance on how to fix (move large content to context tiles). The check is applied to: - run procedure (before step execution) - executeTask procedure (before LLM call) - hexecute procedure (before returning prompt) Limit set to 100KB as a conservative threshold. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…storage Move hexplan storage from direction-0 tiles (ephemeral, per-task) to a run_hexplans join table (persistent, per-run). This enables: - Multiple runs to have independent hexplan state - Historical preservation of execution context - No need for deleteHexplan cleanup between runs - Cleaner separation between tile content and execution state Changes: - Add run_hexplans schema with composite primary key (runId, coords) - Add RunHexplanRepository for CRUD operations - Extend RunService with hexplan methods (get, set, getForRun) - Add updateRunHexplan mutation and getActiveRunForCoords query - Update hexecute MCP tool to use runId instead of deleteHexplan - Add updateRunHexplan MCP tool for agents to update run hexplans - Update RunWidget UI to use new API endpoints - Remove "Delete Hexplan" option from tile context menus - Update compose expansion to show center tile instead of direction-0 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When updating a tile with only content, the preview field was being unexpectedly set to null. The root cause was that updateRef was reconstructing an object with all properties (including undefined ones), which triggered hasOwnProperty checks in the repository layer, causing undefined ?? null to evaluate to null. Fix both service and actions layers to only include explicitly defined fields in update objects, preserving existing values for unspecified fields. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Relaxes the itemType hierarchy constraint to permit ORGANIZATIONAL tiles as children of CONTEXT tiles, enabling better organization within reference material hierarchies. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…/Monitor Replace abstract "system thinkers" framing with concrete product description: - Create: decompose goals, get AI help, attach context - Activate: run, observe, refine on failure - Share: publish, fork, learn from others - Compose: reuse context, systems as tools - Monitor: track usage and health across portfolio Remove MISSION.md as the mission isn't clear yet. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ommand - Deleted the refactor for clarity guide from `.claude/commands/refactor-clarity.md`. - Removed the walkthrough command documentation from `.claude/commands/walkthrough.md`. chore: update .gitignore to include archive directory - Added `.claude/archive/` to the .gitignore file to prevent tracking of archive files. docs: enhance CLAUDE.md with subsystem architecture details - Introduced a new section on subsystem architecture, detailing the structure and constraints of subsystems. - Added workflows for feature planning, impact analysis, and new subsystem introduction. - Updated the core development section with additional commands for subsystem navigation. feat: add subsystem tree utility for visualizing subsystem hierarchy - Implemented a new script to dump the subsystem hierarchy as an ASCII tree or JSON. - Created `scripts/subsystem-tree/README.md` for usage instructions. - Added discovery and rendering logic for subsystem trees in `scripts/subsystem-tree/discovery.py` and `scripts/subsystem-tree/renderer.py`. - Introduced a CLI entry point in `scripts/subsystem-tree/main.py` to facilitate command-line usage.
…odule - Merge scripts/subsystem-tree/ into architecture/tree/ with relative imports - Move scripts/checks/shared/ (TypeScript parser) into architecture/shared/ - Update ruleof6 to import parser from architecture.shared - Remove deadcode checker and all references (CI, package.json, CLAUDE.md, tests) - Replace scripts/checks/architecture/ with git submodule (Diplow/subsystem-architecture) - Add submodules: recursive to architecture and ruleof6 CI checkout steps - Create scripts/checks/run-subsystem-tree.py as hexframe-specific wrapper Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Point subsystem-architecture submodule to rewritten README - Fix CLAUDE.md reference: scripts/subsystem-tree/README.md → scripts/checks/architecture/README.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update check:ruleof6 script to use submodule CLI - Delete hexframe-local ruleof6 directory (logic moved to submodule) - Add Rule of 6 section to CLAUDE.md - Update submodule to include ruleof6/ module Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Wrap all domain service exports with withLogging() pattern (10 service_logging errors) - Fix deep import in useRunsListState to use domain index (7 import_boundary errors) - Add RunWidget subsystem files: dependencies.json and README.md (1 complexity error) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Feature hexsystemplan
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.