Conversation
Add visibility column to map items allowing tiles to be marked as public or private. Private tiles (default) are only visible to the owner, while public tiles can be viewed by anyone. Changes: - Add visibility column to vde_map_items table with 'private' default - Implement visibility filtering at repository layer with requesterUserId - Update all domain services to support visibility parameter - Add UI controls: lock icon on private tiles, context menu toggle - Fix architecture violations by exporting Visibility from utils - Fix unique constraint on base_item_versions table 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add isAuthTransitioning state to cache to block auto-refetch during login/logout - Listen for auth.login and auth.logout events to invalidate cache and tRPC queries - Fix visibility filtering to use empty string for anonymous users (not undefined) - Remove debug console logs from auth state coordinator - Delete unused scripts/youtube.js 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…t rules Security Implementation: - Add RequesterUserId branded type for compile-time enforcement of visibility filtering - Add SYSTEM_INTERNAL sentinel for internal operations that bypass visibility - Add ANONYMOUS_REQUESTER for unauthenticated users - Update all repository methods to require RequesterContext parameter - Add ESLint rule to restrict drizzle-orm imports to infrastructure layer Architecture Fixes: - Export setAuthTransitioning from Cache/State barrel exports - Update imports to use proper barrel exports instead of internal paths Chat Security: - Update /logout command to emit auth.logout event for consistent cache clearing - Ensures chat history and cached data are cleared on logout Documentation: - Add Tile Visibility System section to mapping domain README - Add Security Architecture section with code examples - Add Visibility & Access Control section to UBIQUITOUS.md - Define Owner, Requester, Visibility Filter terminology 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
feat: implement public/private tile visibility system
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@Diplow has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a tile visibility system and requester-aware access control across DB, repository, queries, services, cache, UI, and tests; implements single & batch visibility mutations (optimistic + rollback), auth-transition prefetch gating, an ESLint import restriction, a DB migration, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (UI)
participant Cache as Cache Layer
participant Service as Item Service
participant Repo as MapItemRepository
participant DB as Database
Note over UI,DB: Visibility toggle (optimistic → persisted → reconcile)
UI->>Cache: updateItemOptimistic(coordId, { visibility: "public" })
Cache->>UI: render optimistic visibility
Cache->>Service: updateItem({ coords, visibility })
Service->>Service: validateOwnership & visibility inheritance
Service->>Repo: updateVisibility(itemId, PUBLIC)
Repo->>DB: UPDATE vde_map_items SET visibility='public' WHERE id=...
DB-->>Repo: OK
Repo->>Repo: getOne(itemId, SYSTEM_INTERNAL)
Repo->>DB: SELECT ... WHERE id=... (no visibility filter)
DB-->>Repo: row { visibility }
Repo-->>Service: MapItemWithId { visibility }
Service-->>Cache: reconciled MapItemContract { visibility }
Cache->>UI: emit updated TileData { visibility }
UI->>UI: render visibility indicator
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas needing careful attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/map/Cache/Handlers/NavigationHandler/_helpers/_validation/tile-converter.ts (1)
10-32: Add visibility field to convertToTileData parameter and read from item instead of hardcoding.The function receives items with visibility information from the server but ignores it, hardcoding all tiles to
Visibility.PRIVATE. This causes loss of visibility state (items set to public become private). Addvisibility?: Visibilityto the item parameter type and change line 32 tovisibility: item.visibility ?? Visibility.PRIVATE.src/lib/domains/mapping/infrastructure/map-item/queries/read-queries.ts (1)
98-120: Optionalrequesterparameter infetchManyByIdscould lead to unintended unfiltered queries.Unlike other methods where
requesteris required, this method makes it optional. This creates a potential security gap where callers might accidentally omit the requester and receive unfiltered results. The visibility filter utilities already handle system/internal contexts viaisSystemInternal().Consider making
requesterrequired for consistency and safety:async fetchManyByIds( ids: number[], { limit = 50, offset = 0 }: { limit?: number; offset?: number }, - requester?: RequesterContext + requester: RequesterContext ): Promise<DbMapItemWithBase[]> { // Build filter for multi-owner scenario - const visibilityFilter = requester - ? buildMultiOwnerVisibilityFilter(requester) - : undefined; + const visibilityFilter = buildMultiOwnerVisibilityFilter(requester);If there are legitimate cases where no filter is needed, use the
SYSTEM_INTERNALcontext explicitly at the call site.
🧹 Nitpick comments (28)
src/app/map/Cache/Handlers/ancestor-loader.ts (1)
8-8: Ancestor visibility propagation looks correct and consistentImporting
Visibilityand defaulting ancestor tile data withvisibility: ancestor.visibility ?? Visibility.PRIVATEensures all ancestor items carry a well-typed visibility value, even for legacy/undefined records. The use of??avoids unintentionally overriding valid falsy enum values. This aligns with the broader visibility model described in the PR.If you want to tidy things further, you could optionally combine the
CoordSystemandVisibilityimports into a single import from~/lib/domains/mapping/utils, but that’s purely cosmetic.Also applies to: 73-81
src/lib/domains/mapping/README.md (1)
76-76: Consider adding reference to ESLint constraint configuration.The mention of the ESLint rule that prevents direct Drizzle imports outside the infrastructure layer is good for awareness, but developers may need to know where this rule is defined or configured (e.g.,
.eslintrc, a dedicated rules file, ordependencies.json). Consider adding a brief reference to help developers understand where to find the constraint definition.Example addition:
~6. ESLint rule prevents direct Drizzle imports outside infrastructure layer +6. ESLint rule prevents direct Drizzle imports outside infrastructure layer (see `.eslintrc` or architecture rules for configuration).eslintrc.cjs (1)
61-112: Drizzle import restriction override looks solid; minor DRY and comment‑accuracy nitsThe override correctly fences
drizzle-ormusage to infra/db/services and tests while still re‑enforcing the absolute~/import convention for app code, which aligns with the repo guidelines on avoiding relative imports. Based on learnings, this matches the preferred import style.Two small, non‑blocking points:
Because
no-restricted-importsis already defined globally, this override replaces the base rule forsrc/**/*.ts(x)files. You’ve re‑declared the relative‑import pattern here, so behavior is correct, but future tweaks to the global rule will need to be mirrored. If you want to avoid drift, you could extract the shared"../*", "./*"pattern into a small constant reused in both spots.The comment for
src/lib/domains/*/types/**says “Type definitions (type-only imports allowed)”, but these paths are excluded from the drizzle restriction entirely, so value imports fromdrizzle-ormare also permitted there. If your intent is truly “type‑only”, you’ll need an additional, targeted lint rule (or adjustedno-restricted-importsconfig) for those files, or otherwise soften the comment to match the current behavior.src/app/map/Canvas/Tile/Item/_components/__tests__/item-tile-content.test.tsx (1)
56-56: Consider adding test coverage for the visibility indicator.While adding
visibility: Visibility.PRIVATEto the test fixture is correct, there are no tests verifying that the Lock icon actually renders when visibility is PRIVATE. Consider adding a test case that:
- Renders a tile with
Visibility.PRIVATEand checks for the Lock icon- Renders a tile with
Visibility.PUBLICand verifies the Lock icon is absent- Tests the scale-dependent rendering (icon should not appear at scale < 1)
Example test structure:
it("should render Lock icon for private items at scale >= 1", () => { const testTile = createTestTile("1,0:1", "1"); const { container } = render( <TileActionsProvider> <ItemTileContent item={testTile} scale={1} // ... other props /> </TileActionsProvider> ); // Check for Lock icon presence const lockIcon = container.querySelector('svg'); // or use a more specific selector expect(lockIcon).toBeInTheDocument(); });src/app/map/_components/MapUI.tsx (1)
3-4: UseVisibilityenum values instead of raw strings in toggle handlerThe visibility toggle wiring and optimistic update look correct and integrate cleanly with
TileActionsProvider.To keep things type-safe and resilient to future enum changes, consider using the enum values instead of string literals:
- const handleToggleVisibility = useCallback((tileData: TileData) => { - const currentVisibility = tileData.data.visibility ?? Visibility.PRIVATE; - const newVisibility = currentVisibility === Visibility.PRIVATE ? "public" : "private"; - void cache.updateItemOptimistic(tileData.metadata.coordId, { - visibility: newVisibility, - }); - }, [cache]); + const handleToggleVisibility = useCallback((tileData: TileData) => { + const currentVisibility = tileData.data.visibility ?? Visibility.PRIVATE; + const newVisibility = + currentVisibility === Visibility.PRIVATE + ? Visibility.PUBLIC + : Visibility.PRIVATE; + + void cache.updateItemOptimistic(tileData.metadata.coordId, { + visibility: newVisibility, + }); + }, [cache]);This keeps the toggle aligned with the domain type.
Also applies to: 13-14, 229-235, 271-284
src/app/services/mcp/handlers/tools.ts (1)
244-249: Schema description mentions default but nodefaultproperty is set.The description states "default: private" but the schema doesn't include a
defaultproperty like other optional fields (e.g.,groupIdat line 108). Consider addingdefault: "private"for consistency, or adjust the description to clarify that the server-side default applies.url: { type: "string", description: "Optional URL for the tile" }, visibility: { type: "string", enum: ["public", "private"], - description: "Visibility of the tile (default: private). Public tiles are visible to all users; private tiles are only visible to the owner." + description: "Visibility of the tile (default: private). Public tiles are visible to all users; private tiles are only visible to the owner.", + default: "private" }src/lib/domains/mapping/_actions/_map-item-creation-helpers.ts (1)
113-122: Redundant default value.The default
Visibility.PRIVATEis set both increateMapItem(line 31) and_buildMapItem(line 113). While harmless, consider removing one to clarify the single source of truth for the default.src/lib/domains/mapping/services/_map-management.service.ts (1)
96-115: Consider addingrequestercontext tocreateMap.Unlike
getMapDataandgetManyUserMaps,createMapdoesn't accept arequesterparameter. While creation may not need visibility filtering, accepting the context would maintain API consistency and enable future authorization checks.src/app/map/Cache/Lifecycle/MutationCoordinator/_mutation-wrappers.ts (1)
52-62: Prefer destructuring to forward all MapItemUpdateAttributes fieldsManually mapping individual fields (title/content/preview/link/visibility) works but is easy to forget when
MapItemUpdateAttributesgains new properties.You can make this wrapper future‑proof and reduce duplication by forwarding the entire update payload except
coords:- const wrappedUpdateItemMutation = { - mutateAsync: async (params: { coords: Coord } & MapItemUpdateAttributes) => { - return mutations.updateItemMutation.mutateAsync({ - coords: params.coords, - data: { - title: params.title, - content: params.content, - preview: params.preview, - link: params.link, - visibility: params.visibility, - }, - }); - }, - }; + const wrappedUpdateItemMutation = { + mutateAsync: async (params: { coords: Coord } & MapItemUpdateAttributes) => { + const { coords, ...data } = params; + return mutations.updateItemMutation.mutateAsync({ + coords, + data, + }); + }, + };This keeps the wrapper aligned automatically if the update attributes evolve.
src/app/map/Canvas/TileActionsContext.tsx (1)
91-91: Consider adding onToggleVisibility to the context value.The
onToggleVisibilityhandler is forwarded toContextMenuContainerbut is not included in theTileActionsContextValueinterface or theuseMemodependencies. Other similar handlers (likeonDeleteClick,onCompositionToggle) are exposed in both the context value and passed toContextMenuContainer.If components outside the context menu need access to visibility toggling, consider:
- Adding
onToggleVisibility?: (tileData: TileData) => voidto theTileActionsContextValueinterface- Including it in the
valueobject returned byuseMemo- Adding it to the
useMemodependency arrayIf visibility toggling is intentionally limited to the context menu only, this is fine as-is.
Also applies to: 209-209
src/lib/domains/mapping/infrastructure/map-item/queries/write-queries.ts (1)
6-10: Consider returning affected row count fromupdateVisibilityfor symmetry and safety
updateVisibilitycorrectly performs a targeted update, but it currently ignores whether any row was actually updated. For consistency withdeleteMapItem(which returns a boolean) and to help callers detect missing IDs, consider returning a boolean or theresult.lengthfrom.returning()instead ofvoid.Example:
- async updateVisibility(itemId: number, visibility: Visibility): Promise<void> { - await this.db - .update(mapItems) - .set({ visibility }) - .where(eq(mapItems.id, itemId)); - } + async updateVisibility(itemId: number, visibility: Visibility): Promise<boolean> { + const result = await this.db + .update(mapItems) + .set({ visibility }) + .where(eq(mapItems.id, itemId)) + .returning({ id: mapItems.id }); + + return result.length > 0; + }Also applies to: 180-188
src/lib/domains/mapping/_repositories/__tests__/map-item-repository-bulk-methods.test.ts (1)
13-322: Add explicit assertions that created items preservevisibilityThe tests now pass
visibility: Visibility.PRIVATEinto manycreate/createManycalls and useSYSTEM_INTERNALfor repository access, but none of the expectations currently assert on the resultingattrs.visibility. That means regressions in mapping/persistence of the visibility field wouldn’t be caught here.Consider adding a few focused checks, e.g.:
const createdItems = await mapItemRepo.createMany(mapItemsToCreate); expect(createdItems).toHaveLength(2); expect(createdItems[0]!.ref.id).toBe(baseItem1.id); expect(createdItems[1]!.ref.id).toBe(baseItem2.id); + expect(createdItems[0]!.attrs.visibility).toBe(Visibility.PRIVATE); + expect(createdItems[1]!.attrs.visibility).toBe(Visibility.PRIVATE);and similarly in one or two of the other hierarchical/negative-direction scenarios, to assert that visibility survives through the repository layer.
Also applies to: 325-840
src/lib/domains/mapping/infrastructure/map-item/queries/README.md (1)
26-33: Align documentedWriteQueriesmethods and visibility notes with the actual implementationThe README’s
WriteQueriessection still listsbulkDelete()and doesn’t mentioncreateManyMapItems,batchUpdateItemAndDescendants, orupdateVisibility, which are present in the implementation, whilebulkDeletedoes not appear in the current class.To avoid confusion for future maintainers, consider updating the bullets to reflect the real API surface, for example:
createMapItem()createManyMapItems()updateMapItem()batchUpdateItemAndDescendants()updateVisibility()deleteMapItem()and remove
bulkDelete()unless it’s added as a concrete method.Also applies to: 53-60
src/app/map/Canvas/_internals/menu/_builders/edit-actions.ts (1)
1-2: Make thevisibilityguard explicit to avoid coupling to enum representation
_buildVisibilityItem’s behavior (Unlock/“Make Public” whenVisibility.PRIVATE, Lock/“Make Private” otherwise) is good, but the guard:if (!canEdit || !onToggleVisibility || !visibility) return [];relies on
visibilitybeing truthy. IfVisibilitywere ever implemented as a numeric enum starting at0(e.g.PUBLIC = 0), the PUBLIC case would be filtered out as falsy.Safer and clearer:
-export function _buildVisibilityItem( - canEdit: boolean, - visibility: Visibility | undefined, - onToggleVisibility?: () => void, -): MenuItem[] { - if (!canEdit || !onToggleVisibility || !visibility) return []; +export function _buildVisibilityItem( + canEdit: boolean, + visibility: Visibility | undefined, + onToggleVisibility?: () => void, +): MenuItem[] { + if (!canEdit || !onToggleVisibility || visibility === undefined) return [];The rest of the toggle logic looks solid.
Also applies to: 185-201
src/app/map/Cache/Lifecycle/_provider/_internals/center-change-handler.ts (1)
43-59:createDeferredPrefetchutility is minimal and fit for purposeThe deferred prefetch wrapper using
setTimeout(..., 0)plus a clearable timeout ID is straightforward and appropriate for breaking dispatch cascades. If you ever need to extend it (e.g., to handle rejections), you can wrapprefetchFunctionin a try/catch, but current behavior is acceptable for fire-and-forget usage.src/app/services/mcp/services/map-items.ts (1)
37-83: Visibility threading into MCP map-item handlers is consistentAdding the optional
visibility?: "public" | "private"parameter toaddItemHandlerand extendingupdatesinupdateItemHandleris wired correctly: the value is passed through tomap.addItemand preserved when convertingurl→linkforupdateItem. This should integrate cleanly with the new visibility-aware backend.You might consider reusing the shared
Visibilitytype from the mapping domain to keep the surface consistent across layers, but the current string union is type-safe and workable.Also applies to: 93-106
src/lib/domains/mapping/_actions/map-item-actions/move-orchestrator.ts (1)
8-9: ConfirmSYSTEM_INTERNALusage doesn’t weaken visibility boundariesSwitching internal reads in the move orchestrator to use
SYSTEM_INTERNAL(target lookup, moved-item fetch, temp-target fetch, and swapped descendants) matches the intent to avoid visibility filtering for internal move bookkeeping, and the code changes themselves are coherent.However, because
modifiedItemsis built from these SYSTEM_INTERNAL reads, it’s important that:
- Only appropriately authorized code paths can invoke this orchestrator, and
- Any layer that maps
modifiedItemsinto API responses re-applies requester-based visibility before sending data to clients.If those guarantees already exist in the surrounding services/mappers, this change is fine; otherwise, consider threading a
requesterthroughMoveContextand only usingSYSTEM_INTERNALwhere strictly necessary for invariants, keeping downstream responses filtered by the actual requester.Also applies to: 104-112, 200-222, 234-247
src/app/map/Cache/State/__tests__/reducer.test.ts (1)
10-39: Consider reusing shared MapItem fixtures to reduce duplicationMultiple tests manually construct MapItem-like objects (including
visibility: Visibility.PRIVATE) with nearly identical shapes. SincecreateMockMapItem/createMockMapItemsexist inCache/__tests__/test-fixtures.ts, you could use them here and override only the differing fields (ids, coordinates, depths) to keep MapItem shapes consistent across tests and ease future schema changes.Also applies to: 185-215, 251-288, 325-336, 524-541, 624-639, 676-692
src/app/map/Chat/Timeline/_utils/tile-handlers.ts (1)
4-19: Visibility toggle handler is correct; consider tightening typesThe new
handleToggleVisibilitycorrectly:
- Reads the current visibility from
getItem(tileId)(falling back toVisibility.PRIVATE),- Flips between private/public, and
- Reuses the existing optimistic‑update + error‑event pattern.
Two minor follow‑ups you might consider:
- Align the
updateItemOptimisticpayload to use theVisibilitytype (or a shared alias) instead of a local"public" | "private"union to keep types centralized if more states are ever added.- Document that
getItemshould be provided when wiring tile handlers; without it, the toggle always sends"public"and can’t reliably flip back based on actual state.Also applies to: 21-26, 118-140, 141-150
src/lib/domains/mapping/_actions/__tests__/map-item-copy-helpers.test.ts (1)
10-13: Requester context and visibility additions align with the updated domain modelPassing
SYSTEM_INTERNALinto the various repository methods and addingvisibility: Visibility.PRIVATEto prepared items keeps these tests consistent with the new visibility + requester‑context requirements.If you want stronger coverage, you could also assert in the
_buildMapItemsWithCopiedRefstest that the resulting MapItems’attrs.visibilitymatches the prepared value, to ensure visibility is preserved through the copy pipeline.Also applies to: 45-48, 87-90, 125-126, 162-165, 215-220, 293-304, 355-357, 378-385, 387-397
src/lib/domains/mapping/infrastructure/map-item/queries/visibility-filter.ts (1)
61-70: Consider explicit anonymous check for clarity.The
if (requester)check works becauseANONYMOUS_REQUESTERis an empty string (falsy), but this relies on implicit knowledge. Consider making it explicit:- // Authenticated user: can see own tiles + public tiles - if (requester) { + // Authenticated user: can see own tiles + public tiles + if (requester !== "") {This makes the anonymous case more obvious to future readers.
src/app/map/Cache/Lifecycle/_provider/lifecycle-effects.ts (1)
72-107: Consider extracting auth invalidation sequence to a named function for testability.The cache invalidation sequence (lines 88-91) performs three distinct operations. While correct, extracting this to a separate testable function would improve maintainability.
+const invalidateAllCaches = ( + dispatch: Dispatch<CacheAction>, + trpcUtils: ReturnType<typeof api.useUtils> +) => { + clearPreFetchedData(); + dispatch(invalidateAll()); + void trpcUtils.map.invalidate(); +}; + function useAuthStateEffect( eventBus: EventBusService | undefined, dispatch: Dispatch<CacheAction> ): void { const trpcUtils = api.useUtils(); useEffect(() => { if (!eventBus) { return; } const handleAuthChange = () => { dispatch(setAuthTransitioning(true)); - clearPreFetchedData(); - dispatch(invalidateAll()); - void trpcUtils.map.invalidate(); + invalidateAllCaches(dispatch, trpcUtils); setTimeout(() => { dispatch(setAuthTransitioning(false)); }, AUTH_TRANSITION_DELAY_MS); };src/lib/domains/mapping/infrastructure/map-item/queries/read-queries.ts (1)
34-41: Post-fetch visibility check approach is correct but could be optimized.The post-fetch authorization check correctly prevents information leakage by using the same error message for both "not found" and "unauthorized" cases. However, for single-item fetches, adding the visibility filter to the WHERE clause would be more efficient than fetching then rejecting.
The current approach works correctly. If optimization is desired later:
async fetchItemWithBase( id: number, requester: RequesterContext ): Promise<DbMapItemWithBase> { + // Note: For optimization, consider adding visibility filter to WHERE clause + // after first fetching coord_user_id in a subquery or using a JOIN pattern const result = await this.db .select() .from(mapItems) .leftJoin(baseItems, eq(mapItems.refItemId, baseItems.id)) .where(eq(mapItems.id, id)) .limit(1);src/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.ts (1)
1023-1034: Visibility mapping logic is correct but could be clearer.The conditional visibility mapping handles all cases correctly. Consider extracting the mapping to improve readability:
+const mapVisibilityString = (vis: "public" | "private"): Visibility => + vis === "public" ? Visibility.PUBLIC : Visibility.PRIVATE; + private _prepareOptimisticUpdate( existingItem: TileData, data: { // ... visibility?: "public" | "private"; } ): { optimisticItem: MapItemAPIContract; previousData: MapItemAPIContract } { const previousData = this._reconstructApiData(existingItem); const optimisticItem: MapItemAPIContract = { // ... - visibility: data.visibility ? (data.visibility === "public" ? Visibility.PUBLIC : Visibility.PRIVATE) : previousData.visibility, + visibility: data.visibility ? mapVisibilityString(data.visibility) : previousData.visibility, };src/lib/domains/mapping/infrastructure/map-item/queries/specialized-queries.ts (1)
106-139: Consider makingrequesterrequired for consistency.While other methods like
fetchRootItemandfetchDescendantsByParentrequirerequester, this method has it as optional. This inconsistency could lead to confusion or accidental unfiltered queries.async fetchRootItemsForUser( userId: string, { limit = 50, offset = 0 }: { limit?: number; offset?: number }, - requester?: RequesterContext + requester: RequesterContext ): Promise<DbMapItemWithBase[]> { - // Build visibility filter for the owner of this tile - const visibilityFilter = requester - ? buildVisibilityFilter(requester, userId) - : undefined; + const visibilityFilter = buildVisibilityFilter(requester, userId);src/lib/domains/mapping/services/_item-services/_item-crud.service.ts (1)
309-373: Consider extracting ancestor traversal to a shared utility.The
_getAncestorsUnfilteredmethod duplicates the ancestor traversal logic found inItemQueryService._getAncestorsInternalandMapItemQueryHelpers.getAncestors. Consider extracting this to a shared utility to reduce duplication.This is a minor DRY concern - the current implementation is correct and can be refactored later if needed.
src/lib/domains/mapping/services/_item-services/_item-query.service.ts (1)
346-374: Consider extracting the dynamic import to the top of the file.The
TransactionManageris imported dynamically at line 354, which adds runtime overhead on each call. Since this is a service class, consider importing it statically at the top of the file.+import { TransactionManager } from "~/lib/domains/mapping/infrastructure/transaction-manager"; // ... in moveMapItem method: - // Import TransactionManager at the top of the file - const { TransactionManager } = await import("~/lib/domains/mapping/infrastructure/transaction-manager"); - // Wrap the move operation in a transaction to ensure atomicityBased on coding guidelines, prefer absolute imports with
~/prefix.src/lib/domains/mapping/_repositories/map-item.ts (1)
222-230: Visibility update surface looks good; consider whether writes should also be requester-scoped
updateVisibility(itemId: number, visibility: Visibility): Promise<MapItemWithId>is a clear, focused addition and matches the rest of the repo’s shape.Design question (not a blocker): all read paths are requester‑aware, but this write is not. If visibility changes should also be audited/authorized per requester, you might eventually want a
requester: RequesterContexthere as well; otherwise it’s fine to keep this as a low-level primitive guarded by services above.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Make requester parameter mandatory to ensure visibility filtering is always applied and callers cannot bypass access control. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The documentation incorrectly stated PUBLIC as the default visibility for new tiles, but the schema and migrations set PRIVATE as default. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/domains/mapping/infrastructure/map-item/db.ts (1)
185-199: SimplifyremoveByIdrand restore idempotent “delete” semantics via_resolveItemIdIn the non-
idbranch ofremoveByIdr, you now callgetOneByIdr({ idr }, SYSTEM_INTERNAL)and then checkif (!itemToFetch), butgetOneByIdrthrows if_resolveItemIdreturnsundefined, so thatifbranch is effectively dead and the warning path is never reached. This also means an attempted delete of a nonexistent item by coords will now reject rather than behave idempotently (likeremove(id)does).A cleaner, cheaper, and more obviously idempotent approach is to call
_resolveItemIddirectly withSYSTEM_INTERNAL, log-and-return onundefined, and only callremovewhen there’s a concrete numeric id:async removeByIdr({ idr }: { idr: MapItemIdr }): Promise<void> { if (!("id" in idr)) { - // Use SYSTEM_INTERNAL for internal removal operations - const itemToFetch = await this.getOneByIdr({ idr }, SYSTEM_INTERNAL); - if (!itemToFetch) { - console.warn( - `MapItem with idr ${JSON.stringify(idr)} not found for removal.`, - ); - return; - } - await this.remove(itemToFetch.id); + // Use SYSTEM_INTERNAL for internal removal operations while keeping delete idempotent + const mapItemId = await this._resolveItemId(idr, SYSTEM_INTERNAL); + if (!mapItemId) { + console.warn( + `MapItem with idr ${JSON.stringify(idr)} not found for removal.`, + ); + return; + } + await this.remove(mapItemId); } else { await this.remove(idr.id); } }This reuses the new
_resolveItemIdhelper, avoids an unnecessary full aggregate load, and maintains idempotent delete behavior.Also applies to: 289-299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/domains/mapping/_repositories/map-item.ts(9 hunks)src/lib/domains/mapping/infrastructure/map-item/db.ts(15 hunks)src/lib/domains/mapping/services/_map-management.service.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/domains/mapping/services/_map-management.service.ts
- src/lib/domains/mapping/_repositories/map-item.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute imports with
~/prefix instead of relative imports (./or../) in TypeScript and JavaScript files
Files:
src/lib/domains/mapping/infrastructure/map-item/db.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Domain-Driven Design patterns in TypeScript files within/src/lib/domains/with clear boundaries between mapping, IAM, and other domains. Reference/src/lib/domains/README.mdfor domain implementation details
Use the hierarchical tile architecture for hexframe tiles: positive directions 1-6 for subtask children, negative directions -1 to -6 for context children, and direction 0 for hexplan (execution state and agent guidance). Tiles can have both subtask and context children simultaneously
In AI orchestration and hexplan implementation, agents must use emoji prefixes when updating hexplan tiles: 🟡 STARTED for task begun, ✅ COMPLETED for task finished, 🔴 BLOCKED for task stuck. Use standard MCP tools (getItemByCoords,updateItem) for hexplan updates
Implement Hexframe's execution philosophy: define system structure as a hierarchy with context and subtasks, run hexecute autonomously, monitor progress via hexplan tiles at direction-0, adjust by editing relevant hexplan tiles and restarting. Structure serves as the control interface, not chat history
Files:
src/lib/domains/mapping/infrastructure/map-item/db.ts
🧠 Learnings (2)
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to drizzle/migrations/**/*.ts : Use Drizzle ORM with PostgreSQL for data persistence. All database migrations must be stored in `/drizzle/migrations/`
Applied to files:
src/lib/domains/mapping/infrastructure/map-item/db.ts
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to **/*.{ts,tsx} : Implement Domain-Driven Design patterns in TypeScript files within `/src/lib/domains/` with clear boundaries between mapping, IAM, and other domains. Reference `/src/lib/domains/README.md` for domain implementation details
Applied to files:
src/lib/domains/mapping/infrastructure/map-item/db.ts
🧬 Code graph analysis (1)
src/lib/domains/mapping/infrastructure/map-item/db.ts (6)
src/lib/domains/mapping/types/index.ts (2)
RequesterContext(15-15)SYSTEM_INTERNAL(18-18)src/lib/domains/mapping/types/requester.ts (2)
RequesterContext(38-38)SYSTEM_INTERNAL(64-64)src/lib/domains/mapping/_objects/index.ts (3)
MapItemWithId(9-9)MapItemIdr(8-8)Visibility(7-7)src/lib/domains/mapping/_objects/map-item.ts (2)
MapItemWithId(140-140)MapItemIdr(60-66)src/lib/domains/mapping/_repositories/map-item.ts (1)
MapItemIdr(12-18)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests - Phase 1 (Main Suite)
- GitHub Check: Build
- GitHub Check: Tests - Phase 2 (React Components)
- GitHub Check: ESLint
🔇 Additional comments (4)
src/lib/domains/mapping/infrastructure/map-item/db.ts (4)
2-9: Imports and visibility wiring align with mapping-domain DDD boundariesThe new
VisibilityandRequesterContext/SYSTEM_INTERNALimports, plus thevisibility: attrs.visibilitymapping in_buildCreateAttrs, are consistent with the mapping domain’s separation of concerns and keep infra code depending only on mapping-domain types and DB schema. No changes requested.Based on learnings, this respects the DDD/domain layout for
/src/lib/domains/.Also applies to: 17-20, 342-351
394-401:updateVisibilityimplementation matches new visibility column and repository responsibilities
updateVisibilitycleanly delegates towriteQueries.updateVisibilityand then re-reads the updated item, matching the newvisibilitycolumn wiring and the repository’s existing patterns. Aside from the broader SYSTEM_INTERNAL considerations already mentioned, the method itself is cohesive and type-safe.
56-63: Parameter signatures and call sites are consistent; RequesterContext propagation is correctly implementedThe RequesterContext propagation through
getOne,getOneByIdr,exists,getMany,getRootItem,getRootItemsForUser,getManyByIdr,getDescendantsByParent,getDescendantsWithDepth,getContextForCenter, and_resolveItemIdaligns across theMapItemRepositoryinterface and all call sites. The updated method signatures correctly match the interface definitions—notablygetRootItemsForUser(userId, requester, limit?, offset?)andgetManyByIdr(params, requester)are consistently ordered and required. MapItemRepository intentionally overrides the GenericRepository signature to add RequesterContext for visibility filtering, while BaseItemRepository uses the original signature without it. All call sites properly supply the required parameters in the correct order.
141-145: SYSTEM_INTERNAL post-write reads may bypass visibility; ensure these remain internal-only or parameterize by requesterUsing
SYSTEM_INTERNALfor post-write and relation reads ensures internal consistency by reading freshly-mutated state regardless of caller visibility. However, since these methods are public onDbMapItemRepository, their return values risk bypassingRequesterContext-based filtering if passed directly to user-facing layers—particularly viagetOne(...)which pulls neighbors underSYSTEM_INTERNAL, creating a potential visibility leak.TypeScript visibility markers like
SYSTEM_INTERNALare compile-time only and do not provide runtime enforcement; they can be bypassed through direct imports, casting, or if the module boundary isn't reinforced via package.json "exports" or linting rules.Two safe patterns:
- Treat these as strictly internal helpers and never return their aggregates (or at least never serialize them directly to clients).
- Pass a
requester: RequesterContextinto user-facing write methods and use it when reading back, keepingSYSTEM_INTERNALfor truly internal flows only.Apply this pattern consistently across
create,updateByIdr,removeByIdr(non-id branch viagetOneByIdr), the*ByIdrrelation methods,createMany, andupdateVisibility. Audit call sites—especially those at lines 141-145, 172-174, 187-189, 244-246, 264-266, 284-286, 333-338, 397-401—to confirm whether results flow to HTTP responses or other user-facing layers. Ensure the module boundary is also reinforced via package.json "exports" and ESLint rules (e.g., no-restricted-imports) to prevent bypass at the packaging level.
…ty updates Implements a dedicated backend endpoint to update tile visibility with all descendants in a single database request using path prefix matching. Backend changes: - Add batchUpdateVisibilityWithDescendants to write queries with SQL path prefix matching (handles empty path for root tiles) - Add repository and service methods with ownership validation - Add tRPC endpoint updateVisibilityWithDescendants Frontend changes: - Add updateVisibilityWithDescendantsOptimistic to MutationCoordinator with optimistic updates and rollback support - Update MapUI and TileWidget handlers to use new optimized endpoint - Replace client-side loop approach with single API call 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/mapping/services/_item-services/_item-crud.service.ts (4)
141-191: Make requester parameter required for updates to prevent authorization bypass.The
requesterparameter defaults toSYSTEM_INTERNAL(Line 148), allowing callers to bypass both visibility filtering and ownership checks. While Line 163 checks ownership, the check is skipped whenrequester === SYSTEM_INTERNAL, meaning a caller who omits the parameter can change visibility without authorization.Apply this diff to require explicit requester context:
async updateItem({ coords, title, content, preview, link, visibility, - requester = SYSTEM_INTERNAL, + requester, }: { coords: Coord; title?: string; content?: string; preview?: string; link?: string; visibility?: Visibility; - requester?: RequesterContext; + requester: RequesterContext; }): Promise<MapItemContract> {
233-236: Add authorization checks to prevent unauthorized deletion.The
removeItemmethod lacks any authorization checks. Any caller with valid coordinates can delete any tile, regardless of ownership or permissions. This is a critical security vulnerability.Add a
requesterparameter and validate ownership before deletion:- async removeItem({ coords }: { coords: Coord }): Promise<void> { + async removeItem({ + coords, + requester + }: { + coords: Coord; + requester: RequesterContext; + }): Promise<void> { + // Validate ownership - only the owner can delete tiles + if (requester !== SYSTEM_INTERNAL && requester !== coords.userId) { + throw new Error("Only the owner can delete tiles."); + } const itemToRemove = await this.actions.getMapItem({ coords }); await this.actions.removeItem({ idr: { id: itemToRemove.id } }); }
242-269: Add authorization checks to prevent unauthorized moves.The
moveMapItemmethod lacks authorization checks. Any caller can move any tile to any location without ownership validation. This is a critical security vulnerability that could allow unauthorized structure changes.Add a
requesterparameter and validate ownership:async moveMapItem({ oldCoords, newCoords, + requester, }: { oldCoords: Coord; newCoords: Coord; + requester: RequesterContext; }): Promise<{ modifiedItems: MapItemContract[]; movedItemId: number; affectedCount: number; }> { + // Validate ownership - only the owner can move tiles + if (requester !== SYSTEM_INTERNAL && requester !== oldCoords.userId) { + throw new Error("Only the owner can move tiles."); + } return await TransactionManager.runInTransaction(async (tx) => {
281-303: Add authorization checks to prevent unauthorized child deletion.The
removeChildrenByTypemethod lacks authorization checks. Any caller can delete children of any tile without ownership validation.Add a
requesterparameter and validate ownership:async removeChildrenByType({ coords, directionType, + requester, }: { coords: Coord; directionType: 'structural' | 'composed' | 'hexPlan'; + requester: RequesterContext; }): Promise<{ deletedCount: number }> { + // Validate ownership - only the owner can delete children + if (requester !== SYSTEM_INTERNAL && requester !== coords.userId) { + throw new Error("Only the owner can delete children."); + } const parentItem = await this.actions.getMapItem({ coords });
🧹 Nitpick comments (6)
src/app/map/Cache/Lifecycle/MutationCoordinator/optimistic-tracker.ts (1)
10-14: NewpreviousVisibilityDatafield is type‑safe and consistent with existing metadataThe additional
previousVisibilityData?: MapItemAPIContract[];fits cleanly with the existingmetadatashape and uses the sharedMapItemAPIContracttype, so it should integrate smoothly into the rollback logic.As this
metadataobject now carries multiple flavors of “previous state” (previousChildrenData,previousVisibilityData), consider a short TSDoc comment on themetadatablock (or a dedicatedOptimisticChangeMetadatatype) to clarify the intended semantics of each field before this grows further.src/server/api/routers/map/map-items.ts (1)
10-27: Visibility enum mapping is clean; consider avoiding the non-null assertionCentralizing string →
Visibilityconversion intoVisibilityEnumand reusing it inaddItem,updateItem, andupdateVisibilityWithDescendantskeeps the API consistent. The!intoVisibilityEnum(input.visibility)!is safe as long as the input schema staysz.enum(['public','private']), but you could tighten this by:
- Overloading
toVisibilityEnum(optional vs required) or- Inlining a narrower helper for the mutation that accepts only the non-optional enum
to remove the need for the assertion and make future schema changes safer.
Also applies to: 131-139, 193-214, 458-490
src/app/map/Canvas/TileContextMenu.tsx (1)
6-6: Minor: Inconsistent quote style in import.This import uses single quotes while the other imports (lines 3-5) use double quotes. Consider using consistent quote style throughout the file.
-import type { Visibility } from '~/lib/domains/mapping/utils'; +import type { Visibility } from "~/lib/domains/mapping/utils";src/app/map/Chat/Timeline/_utils/tile-handlers.ts (1)
118-159: Consider extracting the visibility conversion to reduce duplication.The visibility enum-to-string conversion logic is duplicated in both handlers:
const newVisibility = visibility === Visibility.PUBLIC ? "public" : "private";This is a minor duplication but could be extracted to a helper for consistency, especially if more visibility values are added later.
+const toVisibilityString = (visibility: Visibility): "public" | "private" => + visibility === Visibility.PUBLIC ? "public" : "private"; + const handleSetVisibility = async (visibility: Visibility) => { const tileData = widget.data as TileSelectedPayload; try { - const newVisibility = visibility === Visibility.PUBLIC ? "public" : "private"; + const newVisibility = toVisibilityString(visibility); await updateItemOptimistic(tileData.tileId, { visibility: newVisibility, }); // ... }; const handleSetVisibilityWithDescendants = async (visibility: Visibility) => { const tileData = widget.data as TileSelectedPayload; const coordId = tileData.tileId; - const newVisibility = visibility === Visibility.PUBLIC ? "public" : "private"; + const newVisibility = toVisibilityString(visibility); // ... };src/app/map/Chat/Timeline/Widgets/TileWidget/_internals/menu/_MenuDropdown.tsx (1)
91-125: Consider extracting duplicated visibility menu logic.This visibility submenu logic (lines 91-125) duplicates
_buildVisibilitySubmenufromsrc/app/map/Canvas/_internals/menu/_builders/edit-actions.ts. Both implement identical patterns: checkingisPrivate, computingtargetVisibilityandactionLabel, and conditionally rendering single item vs submenu.Consider reusing
_buildVisibilitySubmenuhere to maintain DRY principles and ensure consistency if the logic changes.+import { _buildVisibilitySubmenu } from '~/app/map/Canvas/_internals/menu/_builders/edit-actions';Then replace lines 91-125 with:
// Build visibility items using shared builder const visibilityItems = _buildVisibilitySubmenu( true, // canEdit - if showing menu, editing is allowed visibility, { onSetVisibility, onSetVisibilityWithDescendants } ); menuItems.push(...visibilityItems);src/lib/domains/mapping/_repositories/map-item.ts (1)
223-243: Consider documenting authorization requirements for mutation methods.The mutation methods
updateVisibilityandbatchUpdateVisibilityWithDescendantsdon't includerequesterparameters in their signatures. While authorization is correctly handled in the service layer before calling these methods, documenting this design decision would clarify that callers (typically services) are responsible for authorization before invoking these repository operations.Add documentation noting the authorization responsibility:
/** * Update the visibility of a map item. + * + * Note: Authorization must be validated by the caller before invoking this method. * * @param itemId - ID of the item to update
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/app/map/Cache/Lifecycle/MutationCoordinator/_mutation-wrappers.ts(4 hunks)src/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.ts(8 hunks)src/app/map/Cache/Lifecycle/MutationCoordinator/optimistic-tracker.ts(1 hunks)src/app/map/Cache/Lifecycle/MutationCoordinator/use-mutation-operations.ts(3 hunks)src/app/map/Cache/Lifecycle/_callbacks/mutation-callbacks.ts(3 hunks)src/app/map/Cache/types/handlers.ts(2 hunks)src/app/map/Cache/types/index.ts(1 hunks)src/app/map/Canvas/DynamicFrameCore.tsx(8 hunks)src/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx(5 hunks)src/app/map/Canvas/Tile/Item/item.tsx(2 hunks)src/app/map/Canvas/TileActionsContext.tsx(4 hunks)src/app/map/Canvas/TileContextMenu.tsx(5 hunks)src/app/map/Canvas/_components/ContextMenuContainer.tsx(4 hunks)src/app/map/Canvas/_internals/menu/_builders/edit-actions.ts(2 hunks)src/app/map/Canvas/_internals/menu/items-builder.ts(7 hunks)src/app/map/Chat/Timeline/Widgets/TileWidget/ActionMenu.tsx(4 hunks)src/app/map/Chat/Timeline/Widgets/TileWidget/TileHeader.tsx(6 hunks)src/app/map/Chat/Timeline/Widgets/TileWidget/_internals/header/_HeaderActions.tsx(3 hunks)src/app/map/Chat/Timeline/Widgets/TileWidget/_internals/menu/_MenuDropdown.tsx(4 hunks)src/app/map/Chat/Timeline/Widgets/TileWidget/tile-widget.tsx(5 hunks)src/app/map/Chat/Timeline/_components/_renderers/_tile-renderers.tsx(3 hunks)src/app/map/Chat/Timeline/_components/_renderers/widget-renderers.tsx(1 hunks)src/app/map/Chat/Timeline/_core/WidgetManager.tsx(2 hunks)src/app/map/Chat/Timeline/_core/_widget-handler-factory.ts(2 hunks)src/app/map/Chat/Timeline/_utils/tile-handlers.ts(3 hunks)src/app/map/_components/MapUI.tsx(4 hunks)src/lib/domains/mapping/_actions/__tests__/map-item-transactions.test.ts(1 hunks)src/lib/domains/mapping/_repositories/map-item.ts(9 hunks)src/lib/domains/mapping/infrastructure/map-item/db.ts(15 hunks)src/lib/domains/mapping/infrastructure/map-item/queries/write-queries.ts(2 hunks)src/lib/domains/mapping/services/_item-services/_item-crud.service.ts(8 hunks)src/server/api/routers/map/map-items.ts(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/domains/mapping/_actions/tests/map-item-transactions.test.ts
- src/app/map/Chat/Timeline/Widgets/TileWidget/tile-widget.tsx
- src/app/map/Cache/Lifecycle/_callbacks/mutation-callbacks.ts
- src/app/map/Cache/Lifecycle/MutationCoordinator/_mutation-wrappers.ts
- src/app/map/Chat/Timeline/Widgets/TileWidget/ActionMenu.tsx
- src/app/map/_components/MapUI.tsx
- src/app/map/Canvas/TileActionsContext.tsx
- src/lib/domains/mapping/infrastructure/map-item/queries/write-queries.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute imports with
~/prefix instead of relative imports (./or../) in TypeScript and JavaScript files
Files:
src/app/map/Canvas/Tile/Item/item.tsxsrc/app/map/Cache/Lifecycle/MutationCoordinator/optimistic-tracker.tssrc/app/map/Canvas/TileContextMenu.tsxsrc/app/map/Cache/types/handlers.tssrc/app/map/Chat/Timeline/_components/_renderers/widget-renderers.tsxsrc/server/api/routers/map/map-items.tssrc/app/map/Canvas/_internals/menu/_builders/edit-actions.tssrc/app/map/Cache/Lifecycle/MutationCoordinator/use-mutation-operations.tssrc/app/map/Chat/Timeline/_utils/tile-handlers.tssrc/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.tssrc/app/map/Chat/Timeline/_components/_renderers/_tile-renderers.tsxsrc/app/map/Chat/Timeline/Widgets/TileWidget/_internals/header/_HeaderActions.tsxsrc/app/map/Cache/types/index.tssrc/app/map/Chat/Timeline/Widgets/TileWidget/TileHeader.tsxsrc/app/map/Canvas/_components/ContextMenuContainer.tsxsrc/app/map/Chat/Timeline/Widgets/TileWidget/_internals/menu/_MenuDropdown.tsxsrc/lib/domains/mapping/services/_item-services/_item-crud.service.tssrc/app/map/Chat/Timeline/_core/WidgetManager.tsxsrc/lib/domains/mapping/_repositories/map-item.tssrc/lib/domains/mapping/infrastructure/map-item/db.tssrc/app/map/Chat/Timeline/_core/_widget-handler-factory.tssrc/app/map/Canvas/DynamicFrameCore.tsxsrc/app/map/Canvas/Tile/Item/_components/item-tile-content.tsxsrc/app/map/Canvas/_internals/menu/items-builder.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Domain-Driven Design patterns in TypeScript files within/src/lib/domains/with clear boundaries between mapping, IAM, and other domains. Reference/src/lib/domains/README.mdfor domain implementation details
Use the hierarchical tile architecture for hexframe tiles: positive directions 1-6 for subtask children, negative directions -1 to -6 for context children, and direction 0 for hexplan (execution state and agent guidance). Tiles can have both subtask and context children simultaneously
In AI orchestration and hexplan implementation, agents must use emoji prefixes when updating hexplan tiles: 🟡 STARTED for task begun, ✅ COMPLETED for task finished, 🔴 BLOCKED for task stuck. Use standard MCP tools (getItemByCoords,updateItem) for hexplan updates
Implement Hexframe's execution philosophy: define system structure as a hierarchy with context and subtasks, run hexecute autonomously, monitor progress via hexplan tiles at direction-0, adjust by editing relevant hexplan tiles and restarting. Structure serves as the control interface, not chat history
Files:
src/app/map/Canvas/Tile/Item/item.tsxsrc/app/map/Cache/Lifecycle/MutationCoordinator/optimistic-tracker.tssrc/app/map/Canvas/TileContextMenu.tsxsrc/app/map/Cache/types/handlers.tssrc/app/map/Chat/Timeline/_components/_renderers/widget-renderers.tsxsrc/server/api/routers/map/map-items.tssrc/app/map/Canvas/_internals/menu/_builders/edit-actions.tssrc/app/map/Cache/Lifecycle/MutationCoordinator/use-mutation-operations.tssrc/app/map/Chat/Timeline/_utils/tile-handlers.tssrc/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.tssrc/app/map/Chat/Timeline/_components/_renderers/_tile-renderers.tsxsrc/app/map/Chat/Timeline/Widgets/TileWidget/_internals/header/_HeaderActions.tsxsrc/app/map/Cache/types/index.tssrc/app/map/Chat/Timeline/Widgets/TileWidget/TileHeader.tsxsrc/app/map/Canvas/_components/ContextMenuContainer.tsxsrc/app/map/Chat/Timeline/Widgets/TileWidget/_internals/menu/_MenuDropdown.tsxsrc/lib/domains/mapping/services/_item-services/_item-crud.service.tssrc/app/map/Chat/Timeline/_core/WidgetManager.tsxsrc/lib/domains/mapping/_repositories/map-item.tssrc/lib/domains/mapping/infrastructure/map-item/db.tssrc/app/map/Chat/Timeline/_core/_widget-handler-factory.tssrc/app/map/Canvas/DynamicFrameCore.tsxsrc/app/map/Canvas/Tile/Item/_components/item-tile-content.tsxsrc/app/map/Canvas/_internals/menu/items-builder.ts
src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend must use Next.js 15 App Router with progressive enhancement, following static → progressive → dynamic component patterns, and implement localStorage caching for performance. Reference
/src/app/map/README.mdfor UI architecture
Files:
src/app/map/Canvas/Tile/Item/item.tsxsrc/app/map/Cache/Lifecycle/MutationCoordinator/optimistic-tracker.tssrc/app/map/Canvas/TileContextMenu.tsxsrc/app/map/Cache/types/handlers.tssrc/app/map/Chat/Timeline/_components/_renderers/widget-renderers.tsxsrc/app/map/Canvas/_internals/menu/_builders/edit-actions.tssrc/app/map/Cache/Lifecycle/MutationCoordinator/use-mutation-operations.tssrc/app/map/Chat/Timeline/_utils/tile-handlers.tssrc/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.tssrc/app/map/Chat/Timeline/_components/_renderers/_tile-renderers.tsxsrc/app/map/Chat/Timeline/Widgets/TileWidget/_internals/header/_HeaderActions.tsxsrc/app/map/Cache/types/index.tssrc/app/map/Chat/Timeline/Widgets/TileWidget/TileHeader.tsxsrc/app/map/Canvas/_components/ContextMenuContainer.tsxsrc/app/map/Chat/Timeline/Widgets/TileWidget/_internals/menu/_MenuDropdown.tsxsrc/app/map/Chat/Timeline/_core/WidgetManager.tsxsrc/app/map/Chat/Timeline/_core/_widget-handler-factory.tssrc/app/map/Canvas/DynamicFrameCore.tsxsrc/app/map/Canvas/Tile/Item/_components/item-tile-content.tsxsrc/app/map/Canvas/_internals/menu/items-builder.ts
src/server/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Backend must use tRPC for type-safe API with server-side caching and optimizations. Reference
/src/server/README.mdfor backend architecture details
Files:
src/server/api/routers/map/map-items.ts
🧠 Learnings (5)
📚 Learning: 2025-09-16T20:38:01.707Z
Learnt from: Diplow
Repo: Diplow/hexframe PR: 123
File: src/app/map/Chat/Timeline/Widgets/DebugLogsWidget.tsx:20-33
Timestamp: 2025-09-16T20:38:01.707Z
Learning: DebugLogsWidget in src/app/map/Chat/Timeline/Widgets/DebugLogsWidget.tsx must not use debugLogger for its own logging operations to avoid infinite loops, since render operations are logged by the debug system.
Applied to files:
src/app/map/Chat/Timeline/_components/_renderers/widget-renderers.tsxsrc/app/map/Chat/Timeline/_core/WidgetManager.tsx
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to **/*.{ts,tsx} : Use the hierarchical tile architecture for hexframe tiles: positive directions 1-6 for subtask children, negative directions -1 to -6 for context children, and direction 0 for hexplan (execution state and agent guidance). Tiles can have both subtask and context children simultaneously
Applied to files:
src/app/map/Chat/Timeline/_utils/tile-handlers.tssrc/app/map/Canvas/DynamicFrameCore.tsx
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to drizzle/migrations/**/*.ts : Use Drizzle ORM with PostgreSQL for data persistence. All database migrations must be stored in `/drizzle/migrations/`
Applied to files:
src/lib/domains/mapping/infrastructure/map-item/db.ts
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to **/*.{ts,tsx} : Implement Domain-Driven Design patterns in TypeScript files within `/src/lib/domains/` with clear boundaries between mapping, IAM, and other domains. Reference `/src/lib/domains/README.md` for domain implementation details
Applied to files:
src/lib/domains/mapping/infrastructure/map-item/db.ts
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to src/app/**/*.{ts,tsx} : Frontend must use Next.js 15 App Router with progressive enhancement, following static → progressive → dynamic component patterns, and implement localStorage caching for performance. Reference `/src/app/map/README.md` for UI architecture
Applied to files:
src/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx
🧬 Code graph analysis (15)
src/app/map/Canvas/Tile/Item/item.tsx (2)
src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)
src/app/map/Cache/Lifecycle/MutationCoordinator/optimistic-tracker.ts (1)
src/server/api/types/contracts.ts (1)
MapItemAPIContract(29-29)
src/app/map/Canvas/TileContextMenu.tsx (3)
src/lib/domains/mapping/_objects/index.ts (1)
Visibility(7-7)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)
src/server/api/routers/map/map-items.ts (5)
src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (2)
Visibility(28-28)Coord(9-9)src/server/api/routers/map/_map-auth-helpers.ts (1)
_getRequesterUserId(52-56)src/lib/domains/mapping/utils/hex-coordinates.ts (1)
Coord(22-29)src/server/api/routers/map/map-schemas.ts (1)
hexCoordSchema(4-8)
src/app/map/Canvas/_internals/menu/_builders/edit-actions.ts (4)
src/lib/domains/mapping/_objects/index.ts (1)
Visibility(7-7)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)src/app/map/Canvas/_internals/menu/items-builder.ts (1)
MenuItem(21-21)
src/app/map/Cache/Lifecycle/MutationCoordinator/use-mutation-operations.ts (1)
src/commons/trpc/react.tsx (1)
api(74-74)
src/app/map/Chat/Timeline/Widgets/TileWidget/_internals/header/_HeaderActions.tsx (3)
src/lib/domains/mapping/_objects/index.ts (1)
Visibility(7-7)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)
src/app/map/Chat/Timeline/Widgets/TileWidget/TileHeader.tsx (3)
src/lib/domains/mapping/_objects/index.ts (1)
Visibility(7-7)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)
src/app/map/Canvas/_components/ContextMenuContainer.tsx (5)
src/app/map/types/tile-data.ts (1)
TileData(90-90)src/app/map/types/index.ts (1)
TileData(10-10)src/lib/domains/mapping/_objects/index.ts (1)
Visibility(7-7)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)
src/app/map/Chat/Timeline/Widgets/TileWidget/_internals/menu/_MenuDropdown.tsx (4)
src/lib/domains/mapping/_objects/index.ts (1)
Visibility(7-7)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)src/components/ui/context-menu.tsx (1)
ContextMenuItemData(13-21)
src/lib/domains/mapping/services/_item-services/_item-crud.service.ts (7)
src/lib/domains/mapping/_repositories/map-item.ts (1)
MapItemRepository(35-244)src/lib/domains/mapping/types/contracts.ts (3)
Visibility(30-30)MapItemContract(29-29)adapt(124-129)src/lib/domains/mapping/utils/index.ts (3)
Visibility(28-28)Coord(9-9)MapItemContract(31-31)src/lib/domains/mapping/types/requester.ts (2)
SYSTEM_INTERNAL(64-64)RequesterContext(38-38)src/app/map/types/tile-data.ts (1)
adapt(89-89)src/app/map/types/index.ts (1)
adapt(11-11)src/lib/domains/mapping/_objects/map-item.ts (1)
MapItemWithId(140-140)
src/app/map/Chat/Timeline/_core/WidgetManager.tsx (2)
src/app/map/Cache/use-map-cache.ts (1)
useMapCache(20-73)src/app/map/index.ts (1)
useMapCache(12-12)
src/lib/domains/mapping/_repositories/map-item.ts (7)
src/lib/domains/utils/generic-repository.ts (1)
GenericRepository(8-109)src/lib/domains/mapping/_objects/index.ts (3)
MapItemWithId(9-9)MapItemIdr(8-8)Visibility(7-7)src/lib/domains/mapping/_objects/map-item.ts (2)
MapItemWithId(140-140)MapItemIdr(60-66)src/lib/domains/mapping/types/index.ts (1)
RequesterContext(15-15)src/lib/domains/mapping/types/requester.ts (1)
RequesterContext(38-38)src/lib/domains/mapping/utils/index.ts (2)
Coord(9-9)Visibility(28-28)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)
src/app/map/Chat/Timeline/_core/_widget-handler-factory.ts (1)
src/app/map/Chat/Timeline/_utils/tile-handlers.ts (1)
createTileHandlers(21-172)
src/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx (5)
src/lib/domains/mapping/_objects/index.ts (1)
Visibility(7-7)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)src/lib/domains/mapping/_objects/map-item.ts (1)
isCenter(131-137)src/lib/domains/mapping/utils/hex-coordinates.ts (1)
isCenter(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests - Phase 2 (React Components)
- GitHub Check: Build
- GitHub Check: Tests - Phase 1 (Main Suite)
- GitHub Check: ESLint
🔇 Additional comments (49)
src/app/map/Canvas/DynamicFrameCore.tsx (4)
93-96: LGTM: Parent visibility correctly propagated for non-expanded tiles.The parent resolution and visibility extraction logic is sound. Optional chaining ensures safe access when parent doesn't exist (e.g., root tiles).
Also applies to: 109-109
432-432: LGTM: Composition frame visibility correctly handled.The visibility propagation for composition frame children is correct. The comment on line 432 helpfully clarifies the parent relationship, and the direct property access at line 445 is safe since
parentItemis a required prop.Also applies to: 445-445
564-566: LGTM: Frame center tile visibility properly resolved.The frame parent visibility computation is correct, and the use of optional chaining appropriately handles frames at the root level.
Also applies to: 579-579
609-611: LGTM: Non-expanded frame slot visibility correctly propagated.The parent visibility resolution for non-expanded tiles in frame slots follows the established pattern and uses appropriate optional chaining.
Also applies to: 624-624
src/app/map/Canvas/Tile/Item/item.tsx (1)
11-27: Visibility typing and prop threading look correctAdding
parentVisibility?: Visibilityas an optional prop and importing the type cleanly extends the API without changing behavior; forwarding via{...props}keeps ItemTileContent aligned.src/app/map/Cache/types/handlers.ts (1)
3-4: MutationOperations extension for visibility updates is consistentThe new
Visibilityimport andupdateVisibilityWithDescendantsmutation fit cleanly into the existing handler types and reuse theMutationResult & { updatedCount: number }pattern used elsewhere.Also applies to: 94-100
src/app/map/Cache/Lifecycle/MutationCoordinator/use-mutation-operations.ts (1)
37-55: Visibility-with-descendants mutation is wired correctly through the coordinatorThe new
updateVisibilityWithDescendantsMutationis initialized, wrapped, included in memo deps, and exposed viacoordinator.updateVisibilityWithDescendantsin a way that’s consistent with existing mutations.Also applies to: 79-94
src/server/api/routers/map/map-items.ts (1)
35-40: Requester-aware reads and visibility mutation align with the auth modelThreading
requesterthrough the various read paths (root, coords, collections, descendants/ancestors, composed children, composition checks) and using it when authorizingremoveItem,updateItem,moveMapItem, andupdateVisibilityWithDescendantsis consistent with the visibility-aware design:
- Ownership checks for destructive/privileged actions follow the existing pattern.
- The new
updateVisibilityWithDescendantsmutation correctly gates on ownership and delegates to a bulk operation constrained bycoordsandrequester.This looks coherent with the broader requester/visibility model.
Also applies to: 51-66, 80-85, 148-169, 176-205, 225-250, 279-296, 305-322, 458-490
src/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx (2)
13-15: Visibility types and props are added safelyImporting
Visibilityand threading an optionalparentVisibilitythroughItemTileContentPropsand the component keeps the API backward compatible while enabling visibility-aware rendering; no behavioral regressions introduced here.Also applies to: 28-48, 54-73
121-122: Visibility indicator logic is sensible; confirm behavior without parentVisibilityThe z-index adjustment (
isCenter ? 20 : 25) matches the comment about neighbors taking priority, and the visibility badge logic:scale >= 1 && (isCenter || item.data.visibility !== parentVisibility)will show:
- An icon for the center tile, and
- An icon for non-center tiles whose visibility differs from their parent (or always, when
parentVisibilityis left undefined).If the intent is for tiles with no known parent visibility (e.g., true roots) to always show their own visibility, this matches that behavior.
Also applies to: 155-179
src/app/map/Chat/Timeline/_core/WidgetManager.tsx (1)
15-30: WidgetManager correctly exposes optimistic visibility updates to widgetsPulling
updateVisibilityWithDescendantsOptimisticfromuseMapCacheand threading it through thedepsobject cleanly opens up visibility controls to widget handlers without changing the external WidgetManager API.src/app/map/Chat/Timeline/_components/_renderers/widget-renderers.tsx (1)
6-20: WidgetHandlers visibility callbacks are a safe, extensible additionAdding
handleSetVisibility/handleSetVisibilityWithDescendantswith theVisibilitytype extends the handler surface area in a backward-compatible way; render helpers correctly just forward the expanded handlers object.src/app/map/Chat/Timeline/_components/_renderers/_tile-renderers.tsx (1)
18-22: TileWidget visibility wiring is consistent with existing patternsReading
visibilityfrom the cached tile data and passing it, along withonSetVisibility/onSetVisibilityWithDescendants, intoTileWidgetmirrors how title/preview/content are already sourced and wired to handlers. This keeps the renderer layer thin while enabling the new visibility controls.Also applies to: 30-31, 33-52
src/app/map/Canvas/TileContextMenu.tsx (1)
26-28: LGTM! Visibility props are well-integrated.The visibility-related props (
onSetVisibility,onSetVisibilityWithDescendants,visibility) are properly added to the interface, destructured in the component, and forwarded tobuildMenuItems. The optional typing maintains backward compatibility with existing usages.Also applies to: 54-56, 69-69, 84-85
src/app/map/Chat/Timeline/_core/_widget-handler-factory.ts (2)
34-39: LGTM! Explicit dependency passing is cleaner.Narrowing the dependencies passed to
createTileHandlersto only what's needed improves clarity and reduces coupling compared to passing the entiredepsobject.
11-12: Remove this comment—getItemis actively used and properly wired.The
getItemdependency is not dead code. It is destructured inWidgetManager.tsx(line 15), passed to_createWidgetHandlers(line 26), and then forwarded to_renderWidget(line 38). Within the rendering pipeline,_widget-renderer-factory.tsxreceivesgetItemas a parameter (line 21) and uses it inrenderTileWidget(line 25). Additionally, the fulldepsobject containinggetItemis passed tocreateCreationHandlers(line 41). The dependency is correctly declared and properly threaded through the handler creation and rendering flow.Likely an incorrect or invalid review comment.
src/app/map/Chat/Timeline/_utils/tile-handlers.ts (1)
161-171: LGTM! Handler exports are complete.Both new visibility handlers are properly included in the return object alongside existing handlers.
src/app/map/Cache/types/index.ts (1)
97-107: LGTM! Type definitions are well-structured.The
visibilityfield addition toupdateItemOptimisticand the newupdateVisibilityWithDescendantsOptimisticmethod follow the established patterns in this interface. The return type{ success: boolean; updatedCount: number }is consistent with similar bulk operations likedeleteChildrenByTypeOptimistic.src/app/map/Chat/Timeline/Widgets/TileWidget/TileHeader.tsx (1)
7-7: LGTM! Visibility props are properly threaded through TileHeader.The visibility-related props (
visibility,onSetVisibility,onSetVisibilityWithDescendants) are correctly:
- Imported from the domain utils
- Added to the
TileHeaderPropsinterface- Destructured in the component function
- Forwarded to
_HeaderActionsThis follows the established pattern for other action callbacks in this component.
Also applies to: 19-19, 28-29, 47-47, 56-57, 113-113, 121-122
src/app/map/Canvas/_internals/menu/items-builder.ts (1)
1-104: LGTM!The visibility feature integration follows the established patterns in this file. The new props are properly typed, destructured, and passed to
_buildVisibilitySubmenuwith the correct callback structure.src/app/map/Canvas/_internals/menu/_builders/edit-actions.ts (2)
190-238: LGTM!The
_buildVisibilitySubmenufunction follows the same pattern as_buildDeleteSubmenuwith proper guards, conditional item building, and smart UX (direct item vs submenu based on option count).
240-257: Remove the unused deprecated_buildVisibilityItemfunction.The function is exported but has no callers in the codebase. Since
_buildVisibilitySubmenuhas replaced it and is the new implementation,_buildVisibilityItemshould be removed to reduce maintenance burden and avoid confusion.src/app/map/Chat/Timeline/Widgets/TileWidget/_internals/header/_HeaderActions.tsx (1)
6-84: LGTM!Clean prop threading for visibility controls. The component correctly forwards
visibility,onSetVisibility, andonSetVisibilityWithDescendantsto theActionMenucomponent.src/app/map/Chat/Timeline/Widgets/TileWidget/_internals/menu/_MenuDropdown.tsx (1)
1-177: LGTM on functionality.The visibility feature implementation is functionally correct. Props are properly typed, guards handle undefined values, and the UX pattern (single item vs submenu) is consistent with the rest of the menu.
src/app/map/Canvas/_components/ContextMenuContainer.tsx (1)
85-87: Thevisibilityproperty exists ontileData.dataand is guaranteed to be defined. In theadaptfunction (line 74 of tile-data.ts), visibility is set with a nullish coalescing operator:visibility: item.visibility ?? Visibility.PRIVATE, ensuring the property always has a value. The direct access in ContextMenuContainer.tsx line 87 is safe.src/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.ts (7)
13-13: LGTM! Import follows coding guidelines.The absolute import path with
~/prefix correctly references the Visibility enum from the mapping domain utilities.
64-69: LGTM! Well-typed mutation configuration.The optional mutation config follows the established pattern and provides appropriate type safety for the batch visibility update operation.
707-757: LGTM! Helper methods follow established patterns.The three visibility helper methods correctly implement:
- Optimistic updates with rollback tracking
- Safe rollback with data restoration
- Event emission with appropriate payload
The code is consistent with existing mutation patterns in the file.
370-370: LGTM! Visibility parameter correctly passed to server mutation.The visibility field is properly included in the update mutation call, consistent with other updateable attributes.
1080-1080: LGTM! Secure default visibility for new items.Setting
Visibility.PRIVATEas the default for newly created items is a sensible secure-by-default choice.
1236-1236: LGTM! Safe fallback for visibility reconstruction.The fallback to
Visibility.PRIVATEwhen reconstructing API data ensures backward compatibility with legacy data while maintaining secure defaults.
1150-1160: Visibility enum mapping is correct.The Visibility enum in
src/lib/domains/mapping/_objects/map-item.tsproperly defines PUBLIC = "public" and PRIVATE = "private", matching the string-to-enum conversion at line 1160. The conversion logic correctly handles optional visibility with proper fallback to previous data.src/lib/domains/mapping/services/_item-services/_item-crud.service.ts (5)
1-30: LGTM!The imports and constructor initialization properly set up the visibility infrastructure, including
RequesterContext,SYSTEM_INTERNAL, and themapItemRepositorymember.
43-109: Well-structured visibility inheritance validation.The method correctly:
- Defaults to
Visibility.PRIVATE(secure by default)- Uses
SYSTEM_INTERNALfor internal parent lookups (Line 63)- Validates visibility inheritance only when creating public tiles under existing parents (Lines 92-94)
- Passes visibility through to the creation flow (Line 104)
202-228: LGTM!The batch visibility update method correctly:
- Requires
requesteras a non-optional parameter (Line 209)- Validates ownership before updates (Lines 212-214)
- Enforces visibility inheritance rules (Lines 217-219)
- Delegates to the repository for atomic batch operations (Lines 222-225)
356-380: LGTM!The visibility inheritance validation correctly:
- Short-circuits for non-PUBLIC visibility (Lines 361-363)
- Uses unfiltered ancestor fetch to check all ancestors (Line 367)
- Detects private ancestors and provides a clear error message (Lines 370-379)
386-410: LGTM!The unfiltered ancestor fetch correctly uses
SYSTEM_INTERNAL(Lines 398-401) to bypass visibility filtering for internal validation purposes, ensuring all ancestors are checked regardless of their visibility state.src/lib/domains/mapping/_repositories/map-item.ts (4)
1-34: LGTM!The type definitions and
BaseMapItemRepositorysetup correctly use TypeScript's type system to exclude methods that will be overridden withRequesterContextparameters (Lines 24-33).
35-79: LGTM!All overridden CRUD methods correctly require
requester: RequesterContextas a non-optional parameter (Lines 41, 50, 58, 67, 77), preventing visibility filtering bypass. The past review concern about optional requester parameters has been properly addressed in commit 80b28a2.
80-161: LGTM!All specialized query methods consistently require
RequesterContext(Lines 89, 101, 130, 160), ensuring visibility filtering is enforced across all read operations.
189-204: LGTM!The
getContextForCentermethod correctly requiresrequester: RequesterContextin its config parameter (Line 197), maintaining consistency with the visibility filtering architecture.src/lib/domains/mapping/infrastructure/map-item/db.ts (8)
1-54: LGTM!The imports correctly include
RequesterContextandSYSTEM_INTERNAL(Lines 17-20) for the visibility filtering infrastructure.
56-131: LGTM!All read methods correctly accept and propagate
RequesterContextto the underlying query layer (Lines 56, 65, 76, 84, 95, 104, 118), ensuring visibility filtering is consistently applied across all data access paths.
133-174: LGTM!Create and update operations correctly use
SYSTEM_INTERNALwhen fetching the resulting items (Lines 143, 173), ensuring internal operations can retrieve the full item state without visibility filtering side effects.
176-199: LGTM!The remove operations correctly use
SYSTEM_INTERNAL(Line 188) for internal item lookups. Authorization should be handled at the service layer before calling these repository methods.
201-226: LGTM!Descendant query methods correctly accept
requesterin their parameter objects (Lines 207, 221) and propagate it to the underlying query layer.
228-287: LGTM!Relation update operations consistently use
SYSTEM_INTERNALwhen fetching updated items (Lines 245, 265, 285), ensuring internal operations retrieve complete item state.
289-352: LGTM!Helper methods correctly:
- Propagate
requesterfor coordinate-based ID resolution (Lines 289-299)- Include
visibilityin create attributes (Line 349)- Use
SYSTEM_INTERNALfor internal operations increateMany(Line 337)
354-412: LGTM!The visibility management methods correctly:
- Require
requesteringetContextForCenterconfig (Line 362)- Use
SYSTEM_INTERNALfor fetching updated items after visibility changes (Line 400)- Delegate batch operations to the write query layer (Line 411)
Move visibility indicator outside clipped hexagon area and use TileTooltip component for consistent styling. The tooltip now properly displays with correct z-index and explains visibility behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx (1)
156-162: Visibility indicator positioning works correctly, but comment is slightly misleading.The comment says "positioned outside clipped area" but the
VisibilityIndicatoris rendered inside thediv(line 132) which is a child of the clipped hexagon area (lines 115-130). However, this works because:
- The clip-path is on the outer div (line 119), not the inner content div
- The
VisibilityIndicatorusesz-50which positions it above the clipped contentConsider updating the comment to clarify the actual positioning mechanism.
- {/* Visibility indicator - positioned outside clipped area for proper tooltip rendering */} + {/* Visibility indicator - uses z-50 to render above clipped hexagon content */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx(5 hunks)src/app/map/Canvas/_shared/VisibilityIndicator.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute imports with
~/prefix instead of relative imports (./or../) in TypeScript and JavaScript files
Files:
src/app/map/Canvas/_shared/VisibilityIndicator.tsxsrc/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Domain-Driven Design patterns in TypeScript files within/src/lib/domains/with clear boundaries between mapping, IAM, and other domains. Reference/src/lib/domains/README.mdfor domain implementation details
Use the hierarchical tile architecture for hexframe tiles: positive directions 1-6 for subtask children, negative directions -1 to -6 for context children, and direction 0 for hexplan (execution state and agent guidance). Tiles can have both subtask and context children simultaneously
In AI orchestration and hexplan implementation, agents must use emoji prefixes when updating hexplan tiles: 🟡 STARTED for task begun, ✅ COMPLETED for task finished, 🔴 BLOCKED for task stuck. Use standard MCP tools (getItemByCoords,updateItem) for hexplan updates
Implement Hexframe's execution philosophy: define system structure as a hierarchy with context and subtasks, run hexecute autonomously, monitor progress via hexplan tiles at direction-0, adjust by editing relevant hexplan tiles and restarting. Structure serves as the control interface, not chat history
Files:
src/app/map/Canvas/_shared/VisibilityIndicator.tsxsrc/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx
src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend must use Next.js 15 App Router with progressive enhancement, following static → progressive → dynamic component patterns, and implement localStorage caching for performance. Reference
/src/app/map/README.mdfor UI architecture
Files:
src/app/map/Canvas/_shared/VisibilityIndicator.tsxsrc/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx
🧬 Code graph analysis (1)
src/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx (5)
src/lib/domains/mapping/_objects/index.ts (1)
Visibility(7-7)src/lib/domains/mapping/utils/index.ts (1)
Visibility(28-28)src/lib/domains/mapping/types/contracts.ts (1)
Visibility(30-30)src/lib/domains/mapping/utils/hex-coordinates.ts (1)
isCenter(45-47)src/app/map/Canvas/_shared/VisibilityIndicator.tsx (1)
VisibilityIndicator(12-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build
- GitHub Check: Tests - Phase 2 (React Components)
- GitHub Check: Tests - Phase 1 (Main Suite)
- GitHub Check: ESLint
- GitHub Check: Type Check
- GitHub Check: Architecture Check
🔇 Additional comments (3)
src/app/map/Canvas/_shared/VisibilityIndicator.tsx (1)
1-43: Well-structured visibility indicator component.The component follows good practices with clean conditional rendering, proper absolute imports per coding guidelines, and appropriate accessibility via
cursor-helpand tooltip. The scale-based sizing logic is consistent throughout.One minor note: consider extracting the repeated scale threshold (
scale >= 2) and magic numbers (7, 5, 8, 4, 14, 10) into named constants if this pattern expands, but acceptable as-is for a focused component.src/app/map/Canvas/Tile/Item/_components/item-tile-content.tsx (2)
45-45: OptionalparentVisibilityresults in indicator showing when undefined.When
parentVisibilityis not provided (undefined), the conditionitem.data.visibility !== parentVisibilitywill always betrue, causing the indicator to display. This appears intentional for root tiles or contexts where parent visibility is unavailable, but worth confirming this matches expected behavior.
121-121: Clean refactor to explicitisCenterprop.The z-index logic using the explicit
isCenterprop is cleaner than the previous internal flag approach. The comment clearly explains the corner priority reasoning.
Make the requester parameter required in ItemCrudService.getItem() to prevent accidental visibility bypass. Previously defaulting to SYSTEM_INTERNAL allowed code paths to skip visibility filtering. - Remove default value and optional marker from requester parameter - Update production callers in map-items.ts and agentic.ts routers - Update all test files to pass explicit SYSTEM_INTERNAL requester 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lib/domains/mapping/services/_item-services/_item-crud.service.ts (2)
111-128:getItemnow requiresrequester, resolving the earlier visibility‑bypass concernChanging
getItemto accept a mandatoryrequester: RequesterContextand delegating tomapItemRepository.getOneByIdrwith that requester removes the previous implicitSYSTEM_INTERNALdefault. This ensures all callers must be explicit about their access context and aligns with the visibility‑aware repository behavior mentioned in the past review.
141-191: MakeupdateItem’srequesterrequired to avoid silent SYSTEM_INTERNAL escalation
updateItemcurrently defaultsrequestertoSYSTEM_INTERNAL:async updateItem({ coords, title, content, preview, link, visibility, requester = SYSTEM_INTERNAL, }: { ...; requester?: RequesterContext; })and then uses it to:
- Load the item via
this.actions.getMapItem({ coords, requester }).- Gate visibility changes (
if (requester !== SYSTEM_INTERNAL && requester !== coords.userId) throw).Any caller that forgets to pass
requesterwill therefore run asSYSTEM_INTERNAL, bypassing both visibility‑based filtering and the “only owner can change visibility” guard inside this method. The router correctly passes a real requester, but this default is a foot‑gun for other internal callers and mirrors the risk previously identified forgetItem.Recommend making
requesterrequired here as well so TypeScript enforces explicit context at all call sites:- async updateItem({ - coords, - title, - content, - preview, - link, - visibility, - requester = SYSTEM_INTERNAL, - }: { + async updateItem({ + coords, + title, + content, + preview, + link, + visibility, + requester, + }: { coords: Coord; title?: string; content?: string; preview?: string; link?: string; - visibility?: Visibility; - requester?: RequesterContext; + visibility?: Visibility; + requester: RequesterContext; }): Promise<MapItemContract> {Callers that truly intend to bypass visibility (tests or internal flows) can and should pass
SYSTEM_INTERNALexplicitly.To locate and update all
updateItemcall sites, you can run:#!/bin/bash # Find all ItemCrudService.updateItem call sites to ensure they pass requester explicitly rg -n "updateItem\({" src | sed -n '1,200p'
🧹 Nitpick comments (3)
src/lib/domains/mapping/services/__tests__/helpers/movement/_movement-validation-helpers.ts (1)
40-43: Consistent use ofSYSTEM_INTERNALfor testgetItemcallsUsing
requester: SYSTEM_INTERNALin these helpers makes the test assertions check raw item state independent of requester visibility rules, which is exactly what you want from low-level movement validations. The pattern is consistent across all helpers and aligns with the new access-control model.If you ever find this spreading further, a tiny test helper like
getItemInternal(testEnv, coords)could reduce repetition, but it’s not necessary for this file.Also applies to: 50-55, 71-82, 119-124, 149-155, 164-176, 250-281
src/lib/domains/mapping/services/__tests__/item-remove-children.integration.test.ts (1)
94-101: Requester-awaregetItemusage keeps removal tests focused on actual deletionPassing
requester: SYSTEM_INTERNALinto allgetItemcalls in these assertions is a good change: it ensures the tests validate whether children were truly deleted or preserved, without conflating that with requester-specific visibility rules. The pattern is consistent across structural, composed, hexPlan, and edge-case scenarios and matches the updatedgetItemcall shape.If this pattern shows up broadly in mapping tests, you might consider a small helper like
getInternalItem({ coords })to reduce repetition and make the intent even clearer, but that’s optional.Also applies to: 105-108, 182-189, 235-242, 310-316, 359-366
src/lib/domains/mapping/services/_item-services/_item-crud.service.ts (1)
193-228: Bulk visibility update + inheritance helpers look correct; consider stricter ancestor error handling
updateVisibilityWithDescendantscorrectly:
- Treats
requesteras required and only allows visibility changes from the owner (viacoords.userId) orSYSTEM_INTERNAL.- Reuses
_validateVisibilityInheritanceto prevent making a subtree PUBLIC under any PRIVATE ancestor.- Delegates the batch write to
mapItemRepository.batchUpdateVisibilityWithDescendantsand returns anupdatedCount.The helpers:
_validateVisibilityInheritanceonly runs for PUBLIC and uses_getAncestorsUnfilteredto ignore requester filtering when examining ancestors._getAncestorsUnfilteredwalks up viaCoordSystem.getParentCoordandmapItemRepository.getOneByIdr(..., SYSTEM_INTERNAL).Functionally this enforces the intended “no PUBLIC under PRIVATE ancestor” rule. One minor consideration:
_getAncestorsUnfilteredcurrently swallows any ancestor lookup error and stops the walk, effectively failing open if the ancestor chain is corrupted or partially missing. For stricter safety around visibility invariants, you might prefer to surface or at least log such errors instead of silently treating missing ancestors as acceptable.Also applies to: 347-410
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-get-helpers.ts(3 hunks)src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-remove-children-helpers.ts(11 hunks)src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-update-helpers.ts(3 hunks)src/lib/domains/mapping/services/__tests__/helpers/movement/_movement-validation-helpers.ts(10 hunks)src/lib/domains/mapping/services/__tests__/item-deep-copy-negative-directions.test.ts(9 hunks)src/lib/domains/mapping/services/__tests__/item-deep-copy.integration.test.ts(12 hunks)src/lib/domains/mapping/services/__tests__/item-relationships.integration.test.ts(2 hunks)src/lib/domains/mapping/services/__tests__/item-remove-children.integration.test.ts(6 hunks)src/lib/domains/mapping/services/_item-services/_item-crud.service.ts(8 hunks)src/server/api/routers/agentic/agentic.ts(5 hunks)src/server/api/routers/map/__tests__/map-items-copy.integration.test.ts(4 hunks)src/server/api/routers/map/__tests__/map-items-negative-directions.integration.test.ts(7 hunks)src/server/api/routers/map/map-items.ts(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/domains/mapping/services/tests/item-deep-copy.integration.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute imports with
~/prefix instead of relative imports (./or../) in TypeScript and JavaScript files
Files:
src/server/api/routers/agentic/agentic.tssrc/lib/domains/mapping/services/__tests__/helpers/movement/_movement-validation-helpers.tssrc/server/api/routers/map/__tests__/map-items-copy.integration.test.tssrc/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-get-helpers.tssrc/lib/domains/mapping/services/__tests__/item-remove-children.integration.test.tssrc/lib/domains/mapping/services/__tests__/item-deep-copy-negative-directions.test.tssrc/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-remove-children-helpers.tssrc/server/api/routers/map/__tests__/map-items-negative-directions.integration.test.tssrc/lib/domains/mapping/services/_item-services/_item-crud.service.tssrc/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-update-helpers.tssrc/lib/domains/mapping/services/__tests__/item-relationships.integration.test.tssrc/server/api/routers/map/map-items.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Domain-Driven Design patterns in TypeScript files within/src/lib/domains/with clear boundaries between mapping, IAM, and other domains. Reference/src/lib/domains/README.mdfor domain implementation details
Use the hierarchical tile architecture for hexframe tiles: positive directions 1-6 for subtask children, negative directions -1 to -6 for context children, and direction 0 for hexplan (execution state and agent guidance). Tiles can have both subtask and context children simultaneously
In AI orchestration and hexplan implementation, agents must use emoji prefixes when updating hexplan tiles: 🟡 STARTED for task begun, ✅ COMPLETED for task finished, 🔴 BLOCKED for task stuck. Use standard MCP tools (getItemByCoords,updateItem) for hexplan updates
Implement Hexframe's execution philosophy: define system structure as a hierarchy with context and subtasks, run hexecute autonomously, monitor progress via hexplan tiles at direction-0, adjust by editing relevant hexplan tiles and restarting. Structure serves as the control interface, not chat history
Files:
src/server/api/routers/agentic/agentic.tssrc/lib/domains/mapping/services/__tests__/helpers/movement/_movement-validation-helpers.tssrc/server/api/routers/map/__tests__/map-items-copy.integration.test.tssrc/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-get-helpers.tssrc/lib/domains/mapping/services/__tests__/item-remove-children.integration.test.tssrc/lib/domains/mapping/services/__tests__/item-deep-copy-negative-directions.test.tssrc/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-remove-children-helpers.tssrc/server/api/routers/map/__tests__/map-items-negative-directions.integration.test.tssrc/lib/domains/mapping/services/_item-services/_item-crud.service.tssrc/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-update-helpers.tssrc/lib/domains/mapping/services/__tests__/item-relationships.integration.test.tssrc/server/api/routers/map/map-items.ts
src/server/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Backend must use tRPC for type-safe API with server-side caching and optimizations. Reference
/src/server/README.mdfor backend architecture details
Files:
src/server/api/routers/agentic/agentic.tssrc/server/api/routers/map/__tests__/map-items-copy.integration.test.tssrc/server/api/routers/map/__tests__/map-items-negative-directions.integration.test.tssrc/server/api/routers/map/map-items.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vitest (not Jest) for all tests in TypeScript files
Files:
src/server/api/routers/map/__tests__/map-items-copy.integration.test.tssrc/lib/domains/mapping/services/__tests__/item-remove-children.integration.test.tssrc/lib/domains/mapping/services/__tests__/item-deep-copy-negative-directions.test.tssrc/server/api/routers/map/__tests__/map-items-negative-directions.integration.test.tssrc/lib/domains/mapping/services/__tests__/item-relationships.integration.test.ts
🧠 Learnings (3)
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to **/*.{ts,tsx} : Implement Hexframe's execution philosophy: define system structure as a hierarchy with context and subtasks, run hexecute autonomously, monitor progress via hexplan tiles at direction-0, adjust by editing relevant hexplan tiles and restarting. Structure serves as the control interface, not chat history
Applied to files:
src/server/api/routers/agentic/agentic.ts
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to **/*.{ts,tsx} : Use the hierarchical tile architecture for hexframe tiles: positive directions 1-6 for subtask children, negative directions -1 to -6 for context children, and direction 0 for hexplan (execution state and agent guidance). Tiles can have both subtask and context children simultaneously
Applied to files:
src/server/api/routers/agentic/agentic.ts
📚 Learning: 2025-12-06T16:06:30.746Z
Learnt from: CR
Repo: Diplow/hexframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T16:06:30.746Z
Learning: Applies to **/*.{ts,tsx} : In AI orchestration and hexplan implementation, agents must use emoji prefixes when updating hexplan tiles: 🟡 STARTED for task begun, ✅ COMPLETED for task finished, 🔴 BLOCKED for task stuck. Use standard MCP tools (`getItemByCoords`, `updateItem`) for hexplan updates
Applied to files:
src/server/api/routers/agentic/agentic.ts
🧬 Code graph analysis (7)
src/server/api/routers/agentic/agentic.ts (1)
src/server/api/routers/map/_map-auth-helpers.ts (1)
_getRequesterUserId(52-56)
src/lib/domains/mapping/services/__tests__/helpers/movement/_movement-validation-helpers.ts (1)
src/lib/domains/mapping/services/__tests__/helpers/_test-utilities.ts (1)
SYSTEM_INTERNAL(12-12)
src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-get-helpers.ts (2)
src/lib/domains/mapping/services/__tests__/helpers/_test-utilities.ts (1)
SYSTEM_INTERNAL(12-12)src/lib/domains/mapping/types/index.ts (1)
SYSTEM_INTERNAL(18-18)
src/lib/domains/mapping/services/__tests__/item-deep-copy-negative-directions.test.ts (3)
src/lib/domains/mapping/services/__tests__/helpers/_test-utilities.ts (1)
SYSTEM_INTERNAL(12-12)src/lib/domains/mapping/types/index.ts (1)
SYSTEM_INTERNAL(18-18)src/lib/domains/mapping/types/requester.ts (1)
SYSTEM_INTERNAL(64-64)
src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-remove-children-helpers.ts (1)
src/lib/domains/mapping/services/__tests__/helpers/_test-utilities.ts (1)
SYSTEM_INTERNAL(12-12)
src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-update-helpers.ts (2)
src/lib/domains/mapping/services/__tests__/helpers/_test-utilities.ts (1)
SYSTEM_INTERNAL(12-12)src/lib/domains/mapping/types/index.ts (1)
SYSTEM_INTERNAL(18-18)
src/lib/domains/mapping/services/__tests__/item-relationships.integration.test.ts (2)
src/lib/domains/mapping/services/__tests__/helpers/_test-utilities.ts (1)
SYSTEM_INTERNAL(12-12)src/lib/domains/mapping/types/index.ts (1)
SYSTEM_INTERNAL(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests - Phase 1 (Main Suite)
- GitHub Check: Build
- GitHub Check: Tests - Phase 2 (React Components)
- GitHub Check: ESLint
🔇 Additional comments (25)
src/server/api/routers/map/__tests__/map-items-copy.integration.test.ts (2)
11-11: SYSTEM_INTERNAL import is appropriate for requester-aware testsBringing in
SYSTEM_INTERNALfrom the mapping types aligns these integration tests with the new requester/visibility model, and the absolute~/import matches project guidelines.
47-50: Good use of SYSTEM_INTERNAL requester to keep copy tests visibility-agnosticAdding
requester: SYSTEM_INTERNALto thesegetItemcalls makes the assertions resilient to visibility rules, so the tests validate copy semantics rather than access control behavior. This is consistent and clear across all three call sites.Also applies to: 176-179, 201-204
src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-update-helpers.ts (2)
6-6: LGTM! Import follows coding guidelines.The absolute import with
~/prefix correctly follows the coding guidelines for TypeScript files.
46-46: LGTM! Appropriate use of SYSTEM_INTERNAL for test validation.Using
SYSTEM_INTERNALas the requester in these test helper functions is correct, as they need to verify item state after updates regardless of visibility settings. This ensures CRUD operation tests can validate data changes without being blocked by access control.Note: When implementing visibility-specific tests, those tests should use different requester contexts (e.g., specific user IDs) to verify that visibility and access control logic works correctly.
Also applies to: 103-103
src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-remove-children-helpers.ts (2)
8-8: LGTM! Import follows coding guidelines.The import uses the absolute
~/prefix as required by the coding guidelines and provides the necessary SYSTEM_INTERNAL constant for test helper operations.
268-268: LGTM! Consistent requester parameter addition.All
getItemcalls have been correctly updated to includerequester: SYSTEM_INTERNAL, which is appropriate for test helpers that need unrestricted internal access to verify state during validation. The changes are consistent and align with the visibility system being introduced in this PR.Also applies to: 277-277, 287-287, 328-328, 337-337, 375-375, 385-385, 394-394, 404-404, 435-435
src/lib/domains/mapping/services/__tests__/helpers/item-crud/_item-get-helpers.ts (3)
1-7: LGTM - Import follows conventions and is correctly sourced.The absolute import with
~/prefix adheres to the coding guidelines. Using the canonical source (~/lib/domains/mapping/types) forSYSTEM_INTERNALis appropriate for clarity about the type's origin.
41-44: LGTM - Requester context correctly added.The
SYSTEM_INTERNALrequester enables bypassing visibility checks during test validation, which is the expected behavior for internal test helpers verifying item retrieval.
60-62: LGTM - Consistent requester pattern applied.The error validation helper correctly passes the same
SYSTEM_INTERNALrequester context, maintaining consistency with the other getItem call in this file.src/lib/domains/mapping/services/__tests__/item-deep-copy-negative-directions.test.ts (6)
1-11: LGTM - Imports follow conventions and use correct testing framework.Uses Vitest as required by coding guidelines, and the absolute import with
~/prefix forSYSTEM_INTERNALis correctly applied.
85-101: LGTM - Requester context consistently applied to getItem calls.Both
getItemcalls for verifying copied composed children correctly includerequester: SYSTEM_INTERNAL, enabling the test to retrieve items regardless of visibility settings.
169-188: LGTM - Mixed children verification uses consistent requester pattern.Both structural and composed child retrievals correctly pass the
SYSTEM_INTERNALrequester.
248-258: LGTM - Loop-based verification maintains requester consistency.The loop verifying all 6 composed children correctly includes
requester: SYSTEM_INTERNALin each iteration.
318-327: LGTM - Nested composed child verification follows pattern.The deeply nested composed child retrieval correctly uses the
SYSTEM_INTERNALrequester.
375-402: LGTM - Repository-level calls correctly pass SYSTEM_INTERNAL as second parameter.The
getOneByIdrcalls useSYSTEM_INTERNALas the second argument, which aligns with the repository method signature. This allows direct repository access for verifying parent-child relationships without visibility restrictions.src/lib/domains/mapping/services/__tests__/helpers/movement/_movement-validation-helpers.ts (1)
10-10: Import ofSYSTEM_INTERNALlooks correctAbsolute path is consistent with project guidelines and matches how other domain-level types are imported. No issues here.
src/lib/domains/mapping/services/__tests__/item-relationships.integration.test.ts (2)
11-11: LGTM: Correct import for internal requester constant.The import follows the coding guidelines by using an absolute import with the
~/prefix and correctly imports theSYSTEM_INTERNALconstant needed for the requester-aware API.
91-94: LGTM: Appropriate use of internal requester for integration test.Using
SYSTEM_INTERNALas the requester is appropriate for this integration test, which validates descendant relationship logic and requires unrestricted access to the item hierarchy. The test continues to validate the same behavior while now explicitly providing the required requester context. This change is consistent with similar integration tests across the mapping services.src/lib/domains/mapping/services/__tests__/item-remove-children.integration.test.ts (1)
18-18: SYSTEM_INTERNAL import aligns tests with internal requester semanticsBringing in
SYSTEM_INTERNALfrom the mapping types via an absolute import is consistent with the visibility model and your import guidelines; no issues here.src/server/api/routers/agentic/agentic.ts (2)
7-7: Requester resolution via shared helper is correct and consistentImporting and using
_getRequesterUserId(ctx.user)inhexecutecentralizesRequesterUserIdderivation, cleanly handling anonymous users while keeping the procedurepublic. This aligns with the visibility model and respects the absolute-import guideline (~/prefix).Also applies to: 478-478
498-501: Visibility-aware item reads look good; confirm composed context is equally scopedPassing
requesterinto allmappingService.items.crud.getItemcalls for the task tile, structural children (1–6), and hexplan tile (direction‑0) makes hexecution’s direct reads permission-aware, which is exactly what you want with the new visibility model.One thing to double-check:
composedChildrenstill come fromctx.mappingService.context.getContextForCenter(taskCoords, ContextStrategies.STANDARD)without an explicitrequesterargument. Please ensure that this context API internally applies the same requester-based visibility rules; otherwise, hidden tiles might still influence the prompt via the composed context even though directgetItemcalls are gated.Based on learnings, this keeps tile visibility consistent with the hierarchical tile architecture and execution philosophy.
Also applies to: 522-524, 544-547
src/server/api/routers/map/__tests__/map-items-negative-directions.integration.test.ts (1)
11-12: SYSTEM_INTERNAL requester wiring in integration tests looks appropriateImporting
SYSTEM_INTERNALand passing it asrequestertotestEnv.service.items.crud.getItemkeeps these tests squarely in the “internal caller” lane, which matches their goal of validating negative‑direction behavior without visibility filtering getting in the way. This is consistent across all updated call sites and aligns with the new requester‑aware service API.Also applies to: 69-72, 121-125, 166-170, 501-505, 562-566, 627-630
src/server/api/routers/map/map-items.ts (2)
10-27: Visibility enum plumbing viatoVisibilityEnumis straightforward and consistentImporting
Visibilityand centralizing string→enum conversion intoVisibilityEnumkeeps the router signatures simple while enforcing internal enum usage. Usage inaddItem,updateItem, andupdateVisibilityWithDescendantsis consistent and respects optional vs required visibility fields; the non‑null assertion inupdateVisibilityWithDescendantsis safe given thez.enum(['public', 'private'])input.Also applies to: 132-140, 194-213, 461-493
461-493:updateVisibilityWithDescendantsendpoint matches the new visibility + ownership modelThe
updateVisibilityWithDescendantsmutation correctly:
- Uses
dualAuthProcedure+mappingServiceMiddleware.- Authenticates and derives
currentUserIdandrequester.- Verifies ownership via
crud.getItem({ coords, requester })before proceeding.- Delegates to the service’s
updateVisibilityWithDescendantswith a proper enum value.This keeps router‑level checks and service‑level enforcement aligned and provides a clear
{ success, updatedCount }API.src/lib/domains/mapping/services/_item-services/_item-crud.service.ts (1)
12-13: Visibility‑awareaddItemToMapand direct repo usage are coherentStoring
mapItemRepositoryand using it withSYSTEM_INTERNALfor parent lookup keeps internal invariants (tree structure, coord consistency) independent of requester visibility. Adding avisibilityparam withVisibility.PRIVATEdefault and validating inheritance (preventing PUBLIC children under PRIVATE ancestors) gives you a clear, enforceable model at creation time. This is a good fit for the domain service layer.Also applies to: 17-30, 32-59, 91-105
| const requester = _getRequesterUserId(ctx.user); | ||
| const item = await ctx.mappingService.items.query.getItemById({ | ||
| itemId: input.mapItemId, | ||
| requester, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the file structure and find the copyMapItem implementation
find src/server -name "*.ts" -type f | head -20Repository: Diplow/hexframe
Length of output: 954
🏁 Script executed:
# Look at the map-items.ts file to understand the structure
wc -l src/server/api/routers/map/map-items.tsRepository: Diplow/hexframe
Length of output: 102
🏁 Script executed:
# Find copyMapItem definition
rg -n "copyMapItem" src/server/api/routers/map/map-items.ts -A 30 | head -60Repository: Diplow/hexframe
Length of output: 1199
🏁 Script executed:
# Search for getItemByCoords and getItemById definitions
rg -n "getItemByCoords|getItemById" src --type ts | grep -E "(function|const|=|export)" | head -40Repository: Diplow/hexframe
Length of output: 3239
🏁 Script executed:
# Find the mappingService implementation
find src/lib -name "*mapping*" -o -name "*service*" | grep -i mapping | head -20Repository: Diplow/hexframe
Length of output: 863
🏁 Script executed:
# Look for the query methods implementation
find src/lib -name "*.ts" -type f | xargs rg -l "getItemByCoords|getItemById" | head -20Repository: Diplow/hexframe
Length of output: 129
🏁 Script executed:
# Find where mappingService is defined
rg -n "items\.query\." src/lib --type ts -B 2 -A 2 | head -100Repository: Diplow/hexframe
Length of output: 11166
🏁 Script executed:
# Read the item query service to see method signatures
cat -n src/lib/domains/mapping/services/_item-services/_item-query.service.tsRepository: Diplow/hexframe
Length of output: 14568
🏁 Script executed:
# Also check the mapping service to see how query is exposed
cat -n src/lib/domains/mapping/services/mapping.service.ts | head -80Repository: Diplow/hexframe
Length of output: 1340
🏁 Script executed:
# Let's look at more context around copyMapItem to understand its purpose
sed -n '387,450p' src/server/api/routers/map/map-items.tsRepository: Diplow/hexframe
Length of output: 2264
🏁 Script executed:
# Check how _getRequesterUserId is used elsewhere in this file
rg -n "_getRequesterUserId" src/server/api/routers/map/map-items.ts | head -20Repository: Diplow/hexframe
Length of output: 945
🏁 Script executed:
# Look for the test file to understand intended behavior
cat -n src/server/api/routers/map/__tests__/map-items-copy.integration.test.ts | head -100Repository: Diplow/hexframe
Length of output: 4440
🏁 Script executed:
# Verify the pattern across other endpoints to confirm the inconsistency
grep -A 3 "const requester = _getRequesterUserId" src/server/api/routers/map/map-items.ts | head -60Repository: Diplow/hexframe
Length of output: 2389
🏁 Script executed:
# Check if there are any comments explaining why copyMapItem doesn't use requester
sed -n '380,430p' src/server/api/routers/map/map-items.tsRepository: Diplow/hexframe
Length of output: 1705
Pass requester to getItemByCoords and getItemById in copyMapItem for consistency with visibility filtering
The copyMapItem endpoint (lines 387–428) is the only endpoint in this file that does not thread _getRequesterUserId(ctx.user) through to query methods. All other endpoints—getItem, getItemWithGenerations, getItems, moveMapItem, getDescendants, getAncestors, getComposedChildren, hasComposition, and removeChildrenByType—follow the pattern of extracting requester and passing it to their respective service calls.
Currently, getItemByCoords (line 395) and getItemById (line 407) are called without the requester parameter, which defaults to SYSTEM_INTERNAL. While the endpoint manually checks ownership via ownerId comparison, bypassing visibility filtering for these lookups is inconsistent with the broader pattern and should be aligned:
const requester = _getRequesterUserId(ctx.user);
const sourceItem = await ctx.mappingService.items.query.getItemByCoords({
coords: input.sourceCoords as Coord,
requester,
});Also applies to: 407–409
🤖 Prompt for AI Agents
In src/server/api/routers/map/map-items.ts around lines 387–428 (specifically
the getItemByCoords call at ~395 and getItemById call at ~407–409), the
copyMapItem flow currently omits the requester and thus defaults queries to
SYSTEM_INTERNAL; extract const requester = _getRequesterUserId(ctx.user) at the
top of copyMapItem (if not already present) and pass requester into both
ctx.mappingService.items.query.getItemByCoords({...}) and getItemById({...})
calls so they read: getItemByCoords({ coords: ..., requester }) and
getItemById({ itemId: ..., requester }); keep existing ownerId checks and types
unchanged.
Export _getRequesterUserId from map router's public index and update agentic router to import from the public interface instead of bypassing it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
New Features
Behavior
Migrations
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.