Convert to esm package#4
Conversation
WalkthroughThe project migrates from CommonJS to ES modules (ESM) by updating TypeScript and Jest configurations, adding explicit Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
29-33: Clarify the installation approach.The installation instructions recommend using
npm linkfor local development, but the MCP configuration examples later usenpx private-journal-mcp, which assumes the package is published to npm. This could be confusing for users.Consider one of these approaches:
For published package: Update installation to use
npm install -g private-journal-mcpinstead ofnpm link.For local development: Update the MCP configuration examples to reference the linked local package:
{ "command": "node", "args": ["/path/to/private-journal-mcp/dist/index.js"] }For both: Add separate sections clearly distinguishing between "Development Setup" and "Using the Published Package."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
README.md(5 hunks)jest.config.cjs(1 hunks)package.json(2 hunks)src/index.ts(1 hunks)src/journal.ts(1 hunks)src/search.ts(1 hunks)src/server.ts(2 hunks)tests/embeddings.test.ts(1 hunks)tests/journal.test.ts(9 hunks)tests/paths.test.ts(1 hunks)tests/setup.ts(1 hunks)tsconfig.json(1 hunks)
🔇 Additional comments (27)
src/server.ts (2)
10-12: ESM-local imports with.jsextensions look correctUsing
./journal.js,./types.js, and./search.jsaligns with the ESNext module config and should compile to matching specifiers indist. No issues here.
26-32: Server capabilities object is wired as expectedPassing a second argument with
capabilities: { tools: {}, resources: {} }matches the MCPServerconstructor shape and advertises tool support without changing behavior. This looks consistent with the rest of the server setup.src/index.ts (1)
7-8: Entrypoint imports updated for ESM correctlySwitching to
./server.jsand./paths.jskeeps runtime resolution aligned with the builtdistfiles under ES modules; the rest of the file remains unchanged and should behave identically.tsconfig.json (1)
4-5: TypeScript ESM settings are consistent with the migrationUsing
module: "ESNext"plusmoduleResolution: "node"andallowSyntheticDefaultImports: trueis a reasonable configuration for this ESM shift and matches the updated.jsspecifiers across the codebase. If you later run into resolution quirks with packages that rely heavily on conditional exports, you might consider switchingmoduleResolutiontonode16/nodenextorbundler, but there’s no immediate need to change anything given tests are passing.Also applies to: 11-11
src/search.ts (2)
1-2: ABOUTME comments accurately describe the search behaviorThe new header comments match the existing embedding-based semantic search implementation and improve discoverability without affecting behavior.
6-7: SearchService ESM imports are aligned with the new module setupImporting
EmbeddingServiceand path resolvers via./embeddings.jsand./paths.jsis consistent with the ESNext config and other updated modules; this should keep type-checking and runtime resolution in sync.tests/paths.test.ts (1)
5-5: Test import updated to match ESM build outputPointing the tests to
../src/paths.jskeeps them consistent with the new ESM module specifiers; the rest of the test file is unchanged and should continue to validate the same behavior.tests/embeddings.test.ts (1)
8-10: LGTM!The import paths are correctly updated to use explicit
.jsextensions, which is required for ES modules. The changes are consistent with the ESM migration across the codebase.src/journal.ts (1)
6-8: LGTM!The import paths are correctly updated to use explicit
.jsextensions as required for ES modules.tests/setup.ts (2)
4-13: LGTM!The updated mock implementation generates deterministic but text-dependent embeddings, which is more realistic for testing semantic search functionality. The hash-based approach ensures test consistency while maintaining text differentiation.
15-16: LGTM!Mocking
console.errorreduces noise in test output, which improves test readability.package.json (3)
5-5: LGTM!Adding
"type": "module"correctly enables ES module support for the package, which is necessary for compatibility with the ESM-only@xenova/transformers.
42-43: Verify iftsxdependency is actually needed; update to latest if retained.Security verification passed: both
@xenova/transformersv2.17.2 andtsxv4.19.4 have zero known vulnerabilities. However,tsxv4.19.4 is behind the latest v4.20.6, and critically,tsxis not used anywhere—it's absent from npm scripts and source code imports. Confirm whethertsxis needed; if so, update to v4.20.6 (allowed by caret constraint).
41-41: I need more specific information about breaking changes between versions 0.4.0 and 1.12.1. Let me search for migration details.Based on my web search findings, the SSE transport is now deprecated in favor of Streamable HTTP, and new implementations should use Streamable HTTP while existing SSE implementations should plan to migrate. This represents a known breaking change within the v1.x series.
From the earlier search, v1.0.0 itself didn't introduce API-breaking changes beyond establishing the baseline/stable API, and the project policy aims to avoid breaking changes where possible. However, some later 1.x releases did contain or revert breaking changes (for example a breaking change introduced in v1.13.3 was reverted in v1.15.0). Since the codebase targets v1.12.1 (which is before the v1.13.3 change), it should avoid that particular issue.
Verify that the codebase does not use the deprecated SSE transport.
If your server implementation uses
SseServerTransport, it must be migrated toStreamableHTTPServerTransportto remain compatible with v1.12.1 and forward. If you're using modern patterns (e.g.,StdioServerTransportorStreamableHTTPServerTransport), the upgrade should proceed without issues.jest.config.cjs (1)
2-12: LGTM!The Jest configuration is correctly updated to support ES modules with the standard ts-jest ESM preset, proper module transformation, and name mapping to resolve
.jsimports to their.tssource files during testing.tests/journal.test.ts (9)
8-8: LGTM!The import path is correctly updated to use the explicit
.jsextension as required for ES modules.
136-142: LGTM!The test correctly expects only the
.mdfile for empty content since embedding generation is skipped when content is empty. The updated file discovery usingfind()is more robust than relying on fixed array positions.
161-165: LGTM!The dynamic file discovery approach using
find()is more robust than assuming a specific array index, especially when file ordering may vary.
181-187: LGTM!The test is correctly updated to expect 2 files (
.mdand.embedding) and uses dynamic file discovery for robustness.
207-213: LGTM!The test correctly accounts for both the
.mdand.embeddingfiles being created for non-empty content.
239-258: LGTM!The test is properly updated to dynamically discover
.mdfiles in both project and user directories, making the tests more resilient to file ordering variations.
278-283: LGTM!The test correctly uses dynamic file discovery and expects both
.mdand.embeddingfiles for non-empty user content.
305-310: LGTM!The test is properly updated to use dynamic file discovery for the project directory scenario.
332-337: LGTM!The test correctly uses dynamic file discovery for the custom user path scenario, ensuring robustness across different file orderings.
README.md (3)
9-25: LGTM!The expanded feature documentation provides clear, granular details about the journaling capabilities, search functionality, and privacy features, improving user understanding of the tool's capabilities.
80-111: LGTM!The enhanced MCP tools documentation with detailed parameter descriptions and default values significantly improves usability and clarity.
136-153: LGTM!The entry format example clearly demonstrates the YAML frontmatter structure and section organization, which helps users understand how their journal entries are stored.
Design for review-batch item #1 (dimension-mismatch crash). Covers both the noisy dimension-crash and the silent same-dimension-different-model mode via a shared model-identity compatibility predicate, a read-only skip-and-defer search guard, a dimension floor as corruption backstop, and a hardened startup migration (observable throwing regen, truthful count, generous init timeout). Hardened through two adversarial read-only review rounds; residual holes (quantized/prefix in the key = item obra#4, large-journal startup latency, concurrent writers) recorded as out-of-scope.
1 participant
Motivation and Context
The package is broken on later versions of dependencies as @xenova/transformers is now an esm-only package
How Has This Been Tested?
Tested on my local as an MCP server
Breaking Changes
Just make sure to npm i
Types of changes
Checklist
Summary by CodeRabbit
Documentation
Chores