Skip to content

Develop#160

Merged
Diplow merged 7 commits into
mainfrom
develop
Nov 15, 2025
Merged

Develop#160
Diplow merged 7 commits into
mainfrom
develop

Conversation

@Diplow
Copy link
Copy Markdown
Owner

@Diplow Diplow commented Nov 15, 2025

Summary by CodeRabbit

  • New Features

    • Preview generation model is now configurable via GENERATE_PREVIEW_MODEL (defaults to gpt-4o-mini).
    • CLI tools to find and clean up orphaned map items (dry-run, delete, reparent).
  • Chores

    • Email sending simplified to a single provider path; missing configuration now surfaces a clear error.
    • Toolchain and npm scripts updated; new DB maintenance scripts added.
  • Bug Fixes

    • Map move/copy now uses an atomic batch update and enforces hierarchy/descendant limits.
  • Tests

    • Repository mocks updated to support batch-update testing.

Diplow and others added 2 commits November 15, 2025 11:12
Fixed preview generation failing with "Claude Code process exited with code 1" error.

Root cause: Preview generation was using ClaudeAgentSDKRepository (based on LLM_PROVIDER env var) which spawns Claude Code processes unnecessarily for simple preview generation tasks.

Changes:
- Force preview generation to always use OpenRouterRepository for simple, fast model calls
- Add GENERATE_PREVIEW_MODEL env var to configure preview model (default: gpt-4o-mini)
- Remove all debug console.log statements added during investigation
- Update .env.production.example with new env var documentation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Remove MISTRAL_API_KEY, YOUTUBE_API_KEY, RESEND_API_KEY, and SENDGRID_API_KEY from environment validation. These variables are unused in the main application (Mistral/YouTube are only used in a non-functional sub-project, Resend/SendGrid were commented fallback code). Simplified email.ts to only reference Brevo.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented Nov 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
hexframe Ready Ready Preview Comment Nov 15, 2025 4:15pm

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 15, 2025

Walkthrough

Adds GENERATE_PREVIEW_MODEL env var and wires it into preview generation using OpenRouter; removes several unused API key env declarations; consolidates email sending to Brevo-only; adds orphan-finder and cleanup scripts; implements atomic batch move/update for map items and related constants and DB write-query.

Changes

Cohort / File(s) Summary
Env / Config
\.env.production.example, src/env.js
Add GENERATE_PREVIEW_MODEL (default "gpt-4o-mini") with docs; remove MISTRAL_API_KEY, YOUTUBE_API_KEY, RESEND_API_KEY, and SENDGRID_API_KEY from server schema and runtimeEnv.
Preview generation (agentic)
src/lib/domains/agentic/infrastructure/inngest/functions.ts, src/lib/domains/agentic/services/preview-generator.service.ts, src/lib/domains/agentic/repositories/openrouter.repository.ts, src/lib/domains/agentic/services/dependencies.json
Switch preview generation to OpenRouterRepository (requires OPENROUTER_API_KEY), throw if repo not configured; use env.GENERATE_PREVIEW_MODEL for model selection; add stack to error logs; add "~/env" to allowed dependencies; minor formatting tweaks.
Email sending
src/server/email.ts
Remove multi-provider fallback; retain Brevo-only path and throw explicit configuration error when BREVO_API_KEY is missing.
Mapping constants
src/lib/domains/mapping/constants.ts
Add exported constants: MAX_HIERARCHY_DEPTH, MAX_DESCENDANTS_FOR_OPERATION, DEFAULT_DESCENDANTS_LIMIT, BATCH_INSERT_SIZE.
Map-item repository API & DB
src/lib/domains/mapping/_repositories/map-item.ts, src/lib/domains/mapping/infrastructure/map-item/db.ts, src/lib/domains/mapping/infrastructure/map-item/queries/write-queries.ts
Add batchUpdateItemAndDescendants to repository and DB layers; new write-query implements a single atomic path rewrite/update for moved item and all descendants.
Map-item move flow & tests
src/lib/domains/mapping/_actions/_map-item-movement-helpers.ts, src/lib/domains/mapping/_actions/__tests__/map-item-transactions.test.ts
Move flow now calls single _batchUpdateItemAndDescendants and renames getDescendants_getDescendants; tests updated to mock new batchUpdateItemAndDescendants.
Deep-copy / copy logic
src/lib/domains/mapping/services/_item-services/_item-management.service.ts
Add hierarchy validation (_validateHierarchyLimits) and batched depth-aware creation (_batchCreateByDepth) to deepCopyMapItem; import new constants and types.
Orphan tooling scripts & package
scripts/find-orphan-items.ts, scripts/cleanup-orphan-items.ts, package.json
Add scripts to find and optionally delete/reparent orphan map items; add npm scripts db:find-orphans and db:cleanup-orphans.
Test/type path updates
src/test-utils/event-bus.ts, src/test-utils/__tests__/event-bus.test.ts
Update type import paths for AppEvent, EventListener, and EventBusService to ~/lib/utils/event-bus/types.
Misc server metadata
src/app/services/mcp/server.ts
Remove description and preview fields from Server constructor options.
Small refactors / nullish-coalescing
multiple files
src/app/map/Canvas/Tile/Base/BaseFrame.tsx, src/app/map/Chat/_hooks/useAIChatIntegration.ts, src/app/map/Chat/Timeline/_core/UnifiedTimeline.tsx, src/app/map/Services/DragAndDrop/GlobalDragService.ts, src/lib/debug/debug-logger.ts, src/lib/domains/agentic/repositories/claude-agent-sdk.repository.ts, src/lib/domains/agentic/repositories/openrouter.repository.ts, src/lib/test-logger.ts, src/app/map/Cache/State/selectors/regions.ts
Minor code cleanups replacing explicit checks with optional chaining/nullish coalescing and logging tweaks; no behavioral changes.
Package / tooling
package.json
Add db:find-orphans and db:cleanup-orphans scripts; adjust Vite/Vitest versions and pnpm overrides.

Sequence Diagram(s)

sequenceDiagram
  actor Caller
  participant Inngest as Inngest fn
  participant Service as PreviewGeneratorService
  participant Env as ServerEnv
  participant OpenRouter as OpenRouterRepository

  Caller->>Inngest: generatePreview(tile)
  Inngest->>Service: generatePreview(tile)
  Service->>Env: read GENERATE_PREVIEW_MODEL
  Service->>OpenRouter: instantiate with OPENROUTER_API_KEY
  OpenRouter-->>Service: model response (uses GENERATE_PREVIEW_MODEL)
  Service-->>Inngest: preview result
  Note right of OpenRouter: if not configured -> throw error (stack logged)
Loading
sequenceDiagram
  actor API
  participant Service as MapItemService
  participant Repo as MapItemRepository
  participant DB as DbWriteQueries

  API->>Service: move(item, newCoords, newParent)
  Service->>Repo: batchUpdateItemAndDescendants(params)
  Repo->>DB: writeQueries.batchUpdateItemAndDescendants(params)
  DB-->>Repo: updatedCount
  Repo-->>Service: updatedCount
  Service-->>API: success / updatedCount
  Note right of DB: single atomic path rewrite for item + descendants
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Focus review on the new atomic SQL write query (path rewrite correctness, edge cases for root/empty paths, performance and indexes).
  • Validate hierarchy enforcement and batch-create logic to avoid OOM/timeouts and ensure parent ID resolution correctness.
  • Confirm env/schema changes and OpenRouter integration are safe for deployments and error paths.
  • Review orphan cleanup script for safe dry-run behavior and correct DB transaction/cleanup semantics.

Possibly related PRs

  • Develop #150 — overlaps on mapping deep-copy/move features and batchUpdateItemAndDescendants changes.
  • Develop #41 — related mapping domain changes around move/swap and transactional updates.
  • Develop #123 — touches preview/header UI and preview generation surfaces related to agentic preview changes.

Poem

🐇 I hopped through envs and paths tonight,
swapped keys for one soft guiding light.
Previews hum from OpenRouter's tune,
Brevo carries mail beneath the moon.
I stitch up maps and clear the fray—hop on, let's sort and play!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Develop' is vague and generic, providing no meaningful information about the specific changes in this pull request. Replace with a descriptive title that summarizes the main change, such as 'Add preview model configuration and batch map item updates' or similar.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch develop

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d8e8ec and 7baa886.

📒 Files selected for processing (9)
  • src/app/map/Cache/State/selectors/regions.ts (1 hunks)
  • src/app/map/Canvas/Tile/Base/BaseFrame.tsx (1 hunks)
  • src/app/map/Chat/Timeline/_core/UnifiedTimeline.tsx (1 hunks)
  • src/app/map/Chat/_hooks/useAIChatIntegration.ts (1 hunks)
  • src/app/map/Services/DragAndDrop/GlobalDragService.ts (1 hunks)
  • src/lib/debug/debug-logger.ts (1 hunks)
  • src/lib/domains/agentic/repositories/claude-agent-sdk.repository.ts (1 hunks)
  • src/lib/domains/agentic/types/__tests__/sdk.types.test.ts (1 hunks)
  • src/lib/test-logger.ts (1 hunks)
⏰ 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: Build
  • GitHub Check: Tests - Phase 2 (React Components)
  • GitHub Check: Tests - Phase 1 (Main Suite)
  • GitHub Check: ESLint
🔇 Additional comments (9)
src/app/map/Cache/State/selectors/regions.ts (1)

62-62: LGTM! Modern optional chaining improves readability.

The refactor from explicit truthy check to optional chaining is semantically equivalent and more idiomatic. Both versions evaluate to false when the cache entry is missing, making this a safe and clean improvement.

src/app/map/Canvas/Tile/Base/BaseFrame.tsx (1)

43-43: LGTM! Clean refactor to optional chaining.

The refactor preserves the original behavior while improving readability. When centerItem is undefined, the expression evaluates to false instead of undefined, but since it's used in a boolean context (!isUserTile on line 44), the outcome remains the same.

src/lib/domains/agentic/types/__tests__/sdk.types.test.ts (1)

380-380: LGTM! Clean modernization of null guard.

The optional chaining operator is functionally equivalent to the explicit && guard and is more idiomatic in modern TypeScript. This aligns well with the broader shift toward optional chaining across the codebase.

src/app/map/Chat/Timeline/_core/UnifiedTimeline.tsx (1)

54-54: LGTM! Clean refactor to nullish coalescing assignment.

The nullish coalescing assignment operator (??=) is more concise and idiomatic than the explicit conditional initialization. The behavior is preserved since object properties are undefined by default.

src/lib/domains/agentic/repositories/claude-agent-sdk.repository.ts (1)

62-62: LGTM! Clean refactor to nullish coalescing assignment.

The nullish coalescing assignment (??=) simplifies the code while preserving the original behavior of conditionally saving the API key only when ANTHROPIC_API_KEY_ORIGINAL is not already set.

src/app/map/Services/DragAndDrop/GlobalDragService.ts (1)

37-37: LGTM! Clean singleton initialization with nullish coalescing.

The nullish coalescing assignment provides a more concise implementation of the singleton pattern while maintaining the same behavior.

src/app/map/Chat/_hooks/useAIChatIntegration.ts (1)

25-26: LGTM! Clean refactor using optional chaining.

The optional chaining operator simplifies the guard condition while preserving the same early-return behavior: when latestMessage is nullish, latestMessage?.actor evaluates to undefined, and undefined !== 'user' returns true, triggering the early return as intended.

src/lib/test-logger.ts (1)

73-73: LGTM! Better preservation of falsy values in test logs.

The change from a ternary operator to nullish coalescing improves logging by preserving falsy values like 0, false, or ''. Previously, these would be replaced with an empty string, potentially hiding meaningful test data. This change ensures you see the actual value in server logs.

src/lib/debug/debug-logger.ts (1)

214-215: Operator change is correct and verified as safe.

After searching the codebase, all instances of rawInput appear exclusively in TRPC server logging contexts with structured object values (e.g., { userId: 'user-456' }). No evidence found of rawInput containing primitive falsy values (0, false, empty strings). The ?? operator is actually an improvement here—it correctly preserves legitimate falsy values instead of silently falling back to input with ||. Tests pass with the new operator, confirming no regressions.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bd5d9f2 and a399136.

📒 Files selected for processing (6)
  • .env.production.example (1 hunks)
  • src/env.js (2 hunks)
  • src/lib/domains/agentic/infrastructure/inngest/functions.ts (2 hunks)
  • src/lib/domains/agentic/repositories/openrouter.repository.ts (3 hunks)
  • src/lib/domains/agentic/services/preview-generator.service.ts (2 hunks)
  • src/server/email.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib/domains/agentic/services/preview-generator.service.ts (1)
src/env.js (2)
  • env (7-89)
  • env (7-89)
src/lib/domains/agentic/infrastructure/inngest/functions.ts (2)
src/lib/domains/agentic/repositories/openrouter.repository.ts (1)
  • OpenRouterRepository (49-357)
src/env.js (2)
  • env (7-89)
  • env (7-89)
⏰ 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). (3)
  • GitHub Check: Build
  • GitHub Check: Tests - Phase 1 (Main Suite)
  • GitHub Check: ESLint
🔇 Additional comments (8)
src/lib/domains/agentic/repositories/openrouter.repository.ts (1)

67-88: LGTM! Formatting improvements enhance readability.

The whitespace additions around the request body construction, debug logging, and response parsing improve code clarity without changing behavior.

.env.production.example (1)

48-51: LGTM! Well-documented configuration addition.

The new GENERATE_PREVIEW_MODEL variable is clearly documented with sensible defaults and helpful guidance on model selection. The default "gpt-4o-mini" aligns with the schema definition in src/env.js.

src/lib/domains/agentic/infrastructure/inngest/functions.ts (1)

265-266: LGTM! Enhanced error logging.

Adding the stack trace to error logging will significantly improve debugging capabilities when preview generation fails.

src/server/email.ts (1)

76-78: LGTM! Clearer error messaging.

The updated error message explicitly states that BREVO_API_KEY is required, which improves clarity for users configuring production email. This aligns well with the PR's goal of consolidating to a single email provider.

src/env.js (2)

22-22: LGTM! Proper environment variable schema.

The GENERATE_PREVIEW_MODEL variable is correctly added with appropriate type validation and a sensible default of "gpt-4o-mini".


63-63: LGTM! Runtime environment mapping is correct.

The runtime environment mapping for GENERATE_PREVIEW_MODEL is properly configured to read from process.env.

src/lib/domains/agentic/services/preview-generator.service.ts (2)

4-4: LGTM! Proper environment import.

The env import is correctly added to support dynamic model selection.


52-52: LGTM! Configurable model selection.

Replacing the hardcoded model with env.GENERATE_PREVIEW_MODEL provides flexibility while maintaining backward compatibility through the default value. This aligns well with the PR's objective of making preview generation configurable.

Comment thread src/lib/domains/agentic/infrastructure/inngest/functions.ts
Diplow and others added 2 commits November 15, 2025 15:15
### Deprecated Code Removal
- Remove `batchUpdateDescendantPaths` function (deprecated)
- Remove `batchUpdateChildrenParentIds` function (unused)
- Simplify `_batchUpdateItemAndDescendants` to use atomic updates only
- Remove legacy fallback code and sequential update methods
- Clean up unused imports (`like` from drizzle-orm)

### Orphan Detection Tools
- Add `pnpm db:find-orphans` script for detecting orphan map items
- Add `pnpm db:cleanup-orphans` script with TypeScript/Drizzle implementation
  - Two cleanup modes: `--mode=delete` or `--mode=reparent`
  - Dry run support: `--dry-run` flag
  - Detailed reporting and verification
- Remove old SQL-based cleanup script

### Type System Improvements
- Add `batchUpdateItemAndDescendants` to MapItemRepository interface
- Update test mocks to include new repository method
- Remove unsafe type casting in movement helpers

All changes maintain backward compatibility and pass typecheck + lint.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (11)
src/lib/domains/mapping/_actions/_map-item-movement-helpers.ts (1)

72-83: Batch move integration looks good; clean up legacy helper

The move method’s switch to _batchUpdateItemAndDescendants is a nice simplification and aligns with the new atomic DB operation. Keeping _getDescendants in the signature but unused (with a leading underscore) preserves the public API while avoiding lint noise.

The private _updateItemCoordinates method is now effectively dead (private and not referenced in this class). You can safely delete it to reduce noise and avoid future confusion about which move path is actually used.

Also applies to: 180-196

src/lib/domains/mapping/infrastructure/map-item/db.ts (1)

345-356: Repository delegate for batch move matches write-queries contract

The new batchUpdateItemAndDescendants method cleanly delegates to WriteQueries and matches the expected signature. From a behavior standpoint this looks correct.

You might optionally tighten the typing by reusing the shared Coord interface for oldCoords/newCoords instead of repeating { userId; groupId; path }, but that’s just a consistency improvement.

src/lib/domains/mapping/services/_item-services/_item-management.service.ts (1)

167-259: Solid batch creation logic with proper depth ordering.

The depth-based grouping and sequential processing maintains referential integrity while batching inserts efficiently.

One optional improvement: consider adding a defensive check at line 250 to verify createdItems.length === createDataArray.length before iterating. While the repository contract should guarantee this, an explicit check would make the assumption visible and fail fast if violated.

Optional defensive check:

     );
+
+    // Verify batch creation succeeded completely
+    if (createdItems.length !== createDataArray.length) {
+      throw new Error(
+        `Batch creation mismatch: expected ${createDataArray.length} items, got ${createdItems.length}`
+      );
+    }

     // Store mappings for next level
     for (let i = 0; i < createDataArray.length; i++) {
scripts/cleanup-orphan-items.ts (4)

49-116: De-duplicate orphan detection logic and consider batching queries

This findOrphanItems largely duplicates the detection logic in scripts/find-orphan-items.ts, but uses the drizzle mapItems table instead of the vde_map_items view. Over time, these can drift.

It may be worth:

  • Extracting a shared orphan-detection helper (parameterized by query backend) so both scripts stay in sync.
  • Optionally batching lookups (e.g., pre-building a map of existing paths per user/group) to avoid two DB roundtrips per item when scanning large datasets.

125-135: Use inArray for cleaner, type-safe filtering by orphanIds

The ANY expression via sql\${mapItems.id} = ANY(${orphanIds})`` works but is less idiomatic and can be brittle with array parameter typing.

Drizzle provides inArray for this case:

-import { eq, and, sql } from "drizzle-orm";
+import { eq, and, sql, inArray } from "drizzle-orm";
@@
-    .innerJoin(mapItems, eq(baseItems.id, mapItems.refItemId))
-    .where(sql`${mapItems.id} = ANY(${orphanIds})`);
+    .innerJoin(mapItems, eq(baseItems.id, mapItems.refItemId))
+    .where(inArray(mapItems.id, orphanIds));

This should also make it easier to extend with additional predicates if needed.


137-185: Optional: batch delete/reparent operations for many orphans

Both deleteOrphans and reparentOrphans issue one SQL statement per orphan. That’s fine for small maintenance runs, but if you ever clean up thousands of rows, it may be faster to use a single IN/inArray query per mode.

You can keep the per-item logging but still group IDs into a batched delete/update once logging is done.


219-285: Avoid process.exit so the client can shut down cleanly

Inside main, calling process.exit(1) in the catch can cut short the await client.end() in finally, depending on Node’s shutdown timing.

A safer pattern is:

-  } catch (error) {
-    console.error("❌ Error:", error);
-    process.exit(1);
-  } finally {
+  } catch (error) {
+    console.error("❌ Error:", error);
+    process.exitCode = 1;
+  } finally {
     await client.end();
   }

This still returns a non-zero exit code but lets the DB client close gracefully.

scripts/find-orphan-items.ts (4)

1-10: Align usage docs with the new db:find-orphans script

Now that package.json exposes db:find-orphans (with dotenv wiring), consider updating the header to mention:

  • pnpm db:find-orphans as the primary entrypoint, and
  • optionally keep pnpm tsx scripts/find-orphan-items.ts as an advanced/alt form.

This keeps the docs in sync with how the tool is meant to be run.


36-104: Orphan detection works, but may be chatty for large datasets

The detection logic looks correct for the “missing immediate parent path” definition, and the ancestry check is a nice extra signal. Just note that:

  • findOrphanItems issues two queries per item (parent + root checks).
  • checkAncestryToRoot performs an additional query per step up the path.

That’s fine for moderate sizes and one-off maintenance, but if you expect very large vde_map_items, you might eventually want to batch parent lookups or reuse the drizzle-based helper from scripts/cleanup-orphan-items.ts to avoid N+1-style patterns.

Also applies to: 106-141


174-189: Reference the new cleanup script alongside the raw SQL

Given you now have scripts/cleanup-orphan-items.ts (exposed via pnpm db:cleanup-orphans), it may help to mention that script here as the preferred cleanup mechanism, with the example SQL kept as a low-level fallback.

This keeps users on the same remediation path as your automated tooling.


193-198: Use exitCode instead of process.exit for graceful shutdown

Similar to the cleanup script, the process.exit(1) in catch can interfere with the async await sql.end() in finally.

You can keep the non-zero exit status and let the client close cleanly by doing:

-  } catch (error) {
-    console.error("❌ Error:", error);
-    process.exit(1);
-  } finally {
+  } catch (error) {
+    console.error("❌ Error:", error);
+    process.exitCode = 1;
+  } finally {
     await sql.end();
   }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a399136 and ef29b7c.

📒 Files selected for processing (11)
  • package.json (1 hunks)
  • scripts/cleanup-orphan-items.ts (1 hunks)
  • scripts/find-orphan-items.ts (1 hunks)
  • src/lib/domains/agentic/infrastructure/inngest/functions.ts (2 hunks)
  • src/lib/domains/mapping/_actions/__tests__/map-item-transactions.test.ts (1 hunks)
  • src/lib/domains/mapping/_actions/_map-item-movement-helpers.ts (2 hunks)
  • src/lib/domains/mapping/_repositories/map-item.ts (1 hunks)
  • src/lib/domains/mapping/constants.ts (1 hunks)
  • src/lib/domains/mapping/infrastructure/map-item/db.ts (1 hunks)
  • src/lib/domains/mapping/infrastructure/map-item/queries/write-queries.ts (3 hunks)
  • src/lib/domains/mapping/services/_item-services/_item-management.service.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/domains/agentic/infrastructure/inngest/functions.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/lib/domains/mapping/services/_item-services/_item-management.service.ts (1)
src/lib/domains/mapping/constants.ts (2)
  • MAX_DESCENDANTS_FOR_OPERATION (35-35)
  • MAX_HIERARCHY_DEPTH (22-22)
scripts/cleanup-orphan-items.ts (1)
src/server/db/index.ts (2)
  • schema (78-78)
  • db (75-75)
src/lib/domains/mapping/_repositories/map-item.ts (1)
src/lib/domains/mapping/utils/hex-coordinates.ts (1)
  • Coord (22-29)
src/lib/domains/mapping/infrastructure/map-item/queries/write-queries.ts (3)
src/lib/domains/mapping/utils/hex-coordinates.ts (1)
  • Coord (22-29)
src/lib/domains/mapping/infrastructure/map-item/mappers.ts (1)
  • pathToString (34-36)
src/server/db/schema/_tables/mapping/map-items.ts (1)
  • mapItems (38-85)
src/lib/domains/mapping/_actions/_map-item-movement-helpers.ts (2)
src/lib/domains/mapping/_objects/map-item.ts (1)
  • MapItemWithId (133-133)
src/lib/domains/mapping/utils/hex-coordinates.ts (1)
  • Coord (22-29)
🔇 Additional comments (10)
src/lib/domains/mapping/_actions/__tests__/map-item-transactions.test.ts (1)

19-44: Mock updated to cover new batch API is consistent

Adding batchUpdateItemAndDescendants: vi.fn() keeps the test repository aligned with the MapItemRepository interface and ensures transaction-aware flows using the new batch operation won’t crash under test due to missing methods. No issues from a test behavior standpoint.

src/lib/domains/mapping/constants.ts (1)

1-56: Well-documented operational limits for mapping operations

The constants and accompanying comments are clear and give good context for why these values were chosen. From a code perspective this looks solid; just make sure PERFORMANCE_ANALYSIS.md stays in sync with future tuning so the rationale doesn’t drift over time.

src/lib/domains/mapping/infrastructure/map-item/queries/write-queries.ts (1)

1-1: Remove redundant try/catch block; edge cases prevented by upstream validation

The batchUpdateItemAndDescendants method correctly handles path transformation, and the empty-path edge cases flagged in the review are actually prevented by upstream validation. Line 90-91 in _map-item-movement-helpers.ts explicitly forbids moving center items (CANNOT_MOVE_CENTER_ITEM), so both oldCoords.path and newCoords.path are guaranteed non-empty before reaching this query.

That said, two improvements are warranted:

  1. Remove the redundant try/catch (lines 145-147). The wrapper adds no value; the error propagates identically without it.

  2. Raw SQL parameterization (lines 147-150) uses sql.raw() to inject '${newPathString}' and '${oldPathString},%'. While injection risk is low (Direction enum is constrained to integers), parameterizing through drizzle is cleaner and more maintainable for future changes. This is a style improvement, not a bug.

Likely an incorrect or invalid review comment.

src/lib/domains/mapping/_repositories/map-item.ts (1)

131-147: LGTM! Clean interface addition for atomic batch updates.

The new method signature is well-documented and addresses the duplicate key conflict issue mentioned in the description. Consolidating the moved item and descendant updates into a single atomic operation is a solid improvement.

src/lib/domains/mapping/services/_item-services/_item-management.service.ts (4)

11-11: LGTM! Import additions support the new validation and batch logic.

The new type imports and constants are properly utilized in the helper methods below.

Also applies to: 17-20


101-102: LGTM! Good defensive validation before expensive copy operations.

Failing fast when hierarchy limits are exceeded prevents wasted resources and provides clear feedback.


123-129: LGTM! Batch creation strategy improves performance.

Processing items by depth level allows efficient batch inserts while maintaining parent-child dependencies.


261-289: LGTM! Robust validation with clear error messages.

The hierarchy limit checks protect against resource exhaustion, and the error messages provide helpful guidance. The relative depth calculation correctly handles the source item and its descendants.

package.json (1)

17-18: New orphan DB scripts are wired correctly

The commands align with the new TS entrypoints and follow the existing dotenv pattern for DB scripts; no issues from my side.

scripts/cleanup-orphan-items.ts (1)

46-47: Current code is valid TypeScript—refactor suggestion is optional best practice, not critical

The review's claim that ReturnType<typeof drizzle<typeof schema>> is "not valid TypeScript" and "will fail type-checking" is contradicted by official drizzle documentation and community examples. Web search results confirm this pattern is valid and produces correctly typed results.

The suggested refactor to use PostgresJsDatabase<typeof schema> is indeed the official recommended pattern from drizzle docs and is cleaner, but it's an optional improvement, not a critical fix. The current implementation works correctly and does not fail type-checking.

Likely an incorrect or invalid review comment.

Comment on lines +1 to +13
/**
* Cleanup orphan map items that don't have a valid ancestry path to a root item.
*
* This script provides options to:
* 1. Delete orphan items (and their base items if not referenced elsewhere)
* 2. Re-parent orphans to root (convert them to root items)
*
* Run with:
* pnpm db:cleanup-orphans
* pnpm db:cleanup-orphans --mode=delete
* pnpm db:cleanup-orphans --mode=reparent
* pnpm db:cleanup-orphans --dry-run
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Docstring over-promises base-item cleanup

The header says this script can “delete orphan items (and their base items if not referenced elsewhere)”, but the implementation only deletes/reparents entries in mapItems; base items are never inspected or removed.

Either:

  • implement the base-item cleanup logic, or
  • trim the comment so it only describes the current behavior.

### Test Failures Fixed
- Resolved `ReferenceError: __vite_ssr_exportName__ is not defined` error
- All 1082 tests now passing (1074 passed, 8 skipped, 0 failed)
- Tests were failing because Vite 7 introduced breaking changes in SSR transformation

### Package Downgrades
- Downgrade `vitest` from 3.0.7 to 2.1.9 (latest stable 2.x)
- Downgrade `@vitest/browser`, `@vitest/coverage-v8`, `@vitest/ui` to 2.1.9
- Change Vite from pinned `6.3.5` to `^6.3.5` for patch updates
- Add pnpm override `"vite@>=7.0.0": "6.3.5"` to block Vite 7
- Remove conflicting Vite overrides from vulnerability fix

### Import Path Improvements
- Update `src/test-utils/event-bus.ts` to import from `~/lib/utils/event-bus/types`
- Update `src/test-utils/__tests__/event-bus.test.ts` to use direct imports
- Avoid barrel exports (`~/app/map`) to prevent circular dependencies
- More explicit dependency graph and better maintainability

### Architecture Fix
- Add `~/env` to allowed dependencies in agentic services
- Fixes undeclared dependency violation in `preview-generator.service.ts`

### Root Cause
The "fix vulnerabilities" commit (e9707d1) added pnpm overrides that
inadvertently allowed Vitest 3.x to pull in Vite 7.x. Vite 7 has breaking
changes in SSR transformation that are incompatible with the current setup.
Staying on Vite 6.x until proper Vite 7 support is available.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef29b7c and 49c50bf.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (4)
  • package.json (4 hunks)
  • src/lib/domains/agentic/services/dependencies.json (1 hunks)
  • src/test-utils/__tests__/event-bus.test.ts (1 hunks)
  • src/test-utils/event-bus.ts (1 hunks)
⏰ 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). (5)
  • GitHub Check: Build
  • GitHub Check: Tests - Phase 2 (React Components)
  • GitHub Check: Tests - Phase 1 (Main Suite)
  • GitHub Check: ESLint
  • GitHub Check: Dependabot
🔇 Additional comments (5)
src/lib/domains/agentic/services/dependencies.json (1)

5-5: Dependency allowlist update looks good.

Adding "~/env" to the allowed dependencies is appropriate—it enables the agentic service to consume the new GENERATE_PREVIEW_MODEL environment variable as described in the PR context. The entry follows the existing path-alias pattern and maintains the manifest's clarity.

src/test-utils/__tests__/event-bus.test.ts (1)

3-3: LGTM! Import path update is consistent.

The import path update aligns with the refactoring in src/test-utils/event-bus.ts, consolidating event bus types in the dedicated types module.

package.json (3)

17-18: New database maintenance scripts follow existing patterns.

The orphan-finder and cleanup scripts are properly integrated with consistent naming, environment handling, and command structure matching existing db maintenance scripts.


156-157: Clarify the js-yaml override and vite override intent.

The new js-yaml override properly enforces a minimum version, but the vite override needs clarification:

  • "vite@>=7.0.0": "6.3.5" forces Vite 7.x back to 6.3.5, which suggests a transitive dependency issue.
  • If this is intentional, it should be documented in a comment or commit message explaining why 7.x support is being declined.

121-123: Remove "contradiction" framing; the override is necessary but reveals an underlying compatibility issue.

The pnpm override "vite@>=7.0.0": "6.3.5" is not contradictory to the caret constraint ^6.3.5. The override manages transitive dependencies (if any require vite ≥7.0.0) while the caret governs direct dependencies—both work together correctly.

However, there is a real compatibility concern: vitest 2.1.9 supports Vite 3, 4, and 5 only, but package.json specifies vite 6.3.5. This violates vitest's peer dependency contract and may cause runtime issues. The override exists as a defensive measure but doesn't resolve the core incompatibility. Before proceeding, verify that vitest 2.1.9 and vite 6.3.5 are tested together in your environment, or consider upgrading to a Vitest version with Vite 6 support (vitest 3.x or later).

@@ -1,5 +1,5 @@
import { vi } from 'vitest';
import type { AppEvent, EventListener, EventBusService } from '~/app/map';
import type { AppEvent, EventListener, EventBusService } from '~/lib/utils/event-bus/types';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Import path refactoring looks good, but verify completeness.

The import path update from ~/app/map to ~/lib/utils/event-bus/types improves code organization by consolidating event bus type definitions in a dedicated module.

Run the following script to verify the refactoring is complete:


🏁 Script executed:

#!/bin/bash

# Verify the new types module exists and exports all required types
echo "=== Checking new types module ==="
fd -t f "types.ts" src/lib/utils/event-bus --exec cat {}

# Check for any remaining imports from the old path
echo -e "\n=== Checking for old import paths ==="
rg -n "from ['\"]~/app/map['\"]" --type ts

# Check if old export location still exists
echo -e "\n=== Checking old export location ==="
fd -t f -p "src/app/map" --exec echo "Found: {}"

Length of output: 34681


Refactoring is incomplete—16+ files still import from the old path.

The update to src/test-utils/event-bus.ts is valid, but the refactoring itself was not completed. Multiple files throughout the codebase still import EventBusService (and related types) from ~/app/map instead of ~/lib/utils/event-bus/types:

  • src/test-utils/providers.tsx:6-7
  • src/app/map/_components/MapUI.tsx:11
  • src/app/map/_hooks/use-tile-select-for-chat.tsx:2
  • src/app/map/Cache/Lifecycle/MutationCoordinator/use-mutation-operations.ts:10
  • src/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.ts:10
  • src/app/map/Cache/Handlers/NavigationHandler/navigation-handler-factories.ts:6
  • src/app/map/Cache/Handlers/NavigationHandler/navigation-handler.ts:5
  • src/app/map/Cache/Handlers/NavigationHandler/_core/navigation-core.ts:6
  • src/app/map/Canvas/canvas.tsx:25
  • And others

Update all remaining imports to use the new ~/lib/utils/event-bus/types path to complete the refactoring.

🤖 Prompt for AI Agents
In src/test-utils/event-bus.ts around line 2 and across the codebase (notably
src/test-utils/providers.tsx:6-7, src/app/map/_components/MapUI.tsx:11,
src/app/map/_hooks/use-tile-select-for-chat.tsx:2,
src/app/map/Cache/Lifecycle/MutationCoordinator/use-mutation-operations.ts:10,
src/app/map/Cache/Lifecycle/MutationCoordinator/mutation-coordinator.ts:10,
src/app/map/Cache/Handlers/NavigationHandler/navigation-handler-factories.ts:6,
src/app/map/Cache/Handlers/NavigationHandler/navigation-handler.ts:5,
src/app/map/Cache/Handlers/NavigationHandler/_core/navigation-core.ts:6,
src/app/map/Canvas/canvas.tsx:25 and other files), remaining imports still
reference the old ~/app/map path; update each import that currently pulls
EventBusService, AppEvent, EventListener, or related types from ~/app/map to
import from the new ~/lib/utils/event-bus/types path, run a project-wide search
for those type names to find any missed occurrences, update them to the new
path, and run type check/build to confirm no unresolved imports remain.

- Downgrade from Vite 6.4.1 to 5.4.21 for compatibility with vitest 2.1.9
- Keep vitest at 2.1.9 (Vite 6 requires vitest 4.x which has breaking changes)
- Fix MCP server: remove unsupported 'description' field from Server constructor
- Add pnpm override to prevent Vite 6+ installation
- TypeScript type checking now passes

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Replace logical OR (||) and conditional assignments with nullish coalescing
operators (?? and ??=) across 9 files for better type safety and cleaner code.

Changes:
- Use ??= for singleton patterns and conditional initialization
- Replace || with ?? for safer default value handling
- Simplify conditional assignment patterns

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Diplow Diplow merged commit da98f81 into main Nov 15, 2025
15 of 18 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Nov 19, 2025
@coderabbitai coderabbitai Bot mentioned this pull request Dec 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant