refactor(processors): dynamic-import mammoth, move to optionalDependencies - #974
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
Refactors Word document processing to make the heavy mammoth dependency optional and lazily loaded at runtime, reducing required dependency footprint for installs that don’t need Word processing.
Changes:
- Replaced static
mammothimport with a cached dynamic import helper inWordProcessor. - Moved
mammothfromdependenciestooptionalDependenciesinpackage.json. - Updated
pnpm-lock.yamlto reflectmammothas an optional dependency.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/lib/processors/document/WordProcessor.ts | Introduces lazy-loading of mammoth and uses it during extraction. |
| package.json | Moves mammoth to optionalDependencies to make Word support opt-in. |
| pnpm-lock.yaml | Aligns lockfile with the dependency classification change. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ncies Convert static `import * as mammoth from "mammoth"` to a lazy dynamic import with ERR_MODULE_NOT_FOUND detection. Move mammoth (2.3 MB) from dependencies to optionalDependencies.
2a6beb4 to
ce6e043
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/processors/document/WordProcessor.ts (1)
286-321:⚠️ Potential issue | 🟠 MajorInstall-hint error is swallowed by the inner try/catch.
loadMammoth()is invoked inside the innertryblock at line 287. Whenmammothisn't installed, the helpfulError('Word document processing requires the "mammoth" package...')thrown at line 56 is caught at line 309 and re-wrapped as a genericFileErrorCode.PROCESSING_FAILEDwithreason: "Failed to extract Word document content". The install instructions never surface to the caller, defeating the main UX goal of this refactor.Consider calling
loadMammoth()before the inner try, or detecting the install-hint error and propagating it with a distinct error code (e.g.,FileErrorCode.DEPENDENCY_MISSINGor reusing the message asreason):🔧 Proposed fix
- // Step 4 & 5: Extract text and HTML content using mammoth - let textContent = ""; - let htmlContent = ""; - const warnings: string[] = []; - - try { - const mammoth = await loadMammoth(); + // Load mammoth before the extraction try/catch so the missing-dependency + // error surfaces with its install instructions instead of being masked. + const mammoth = await loadMammoth(); + + // Step 4 & 5: Extract text and HTML content using mammoth + let textContent = ""; + let htmlContent = ""; + const warnings: string[] = []; + + try { // Extract plain text const textResult = await mammoth.extractRawText({ buffer });The outer
try/catchat line 338 will then return the install-hint message viaerror.messageinUNKNOWN_ERROR, or you can add a dedicated branch that detects the dependency-missing case and maps it to a more specific error code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/processors/document/WordProcessor.ts` around lines 286 - 321, The inner try/catch in WordProcessor (around the block calling loadMammoth, mammoth.extractRawText, and mammoth.convertToHtml) is swallowing the install-hint error from loadMammoth; move the call to loadMammoth() outside that inner try so its specific error propagates, or detect the install-hint error after catching (inspect extractError.message or error type) and rethrow or return a distinct createError(FileErrorCode.DEPENDENCY_MISSING, { reason: extractError.message }, extractError) so the original install instructions from loadMammoth() are preserved instead of being wrapped as PROCESSING_FAILED.
🧹 Nitpick comments (2)
src/lib/processors/document/WordProcessor.ts (2)
45-63: Matching on error message substring is fragile.
e.message.includes("mammoth")works today because Node'sERR_MODULE_NOT_FOUNDmessage embeds the specifier, but message wording isn't part of Node's stable contract and bundlers (esbuild/ncc/vite) may rewrite it. Consider also checkingerrshape more defensively, e.g., acceptERR_MODULE_NOT_FOUNDunconditionally here (this loader only ever imports"mammoth"), or additionally matchMODULE_NOT_FOUNDfor the CJS fallback path:🔧 Suggested tweak
- const e = err instanceof Error ? (err as NodeJS.ErrnoException) : null; - if (e?.code === "ERR_MODULE_NOT_FOUND" && e.message.includes("mammoth")) { + const e = err instanceof Error ? (err as NodeJS.ErrnoException) : null; + if ( + e?.code === "ERR_MODULE_NOT_FOUND" || + e?.code === "MODULE_NOT_FOUND" + ) { throw new Error(Since this loader exclusively imports
"mammoth", anyMODULE_NOT_FOUNDhere is unambiguously about mammoth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/processors/document/WordProcessor.ts` around lines 45 - 63, The current loadMammoth function relies on err.message.includes("mammoth") which is fragile; update loadMammoth to treat any module-not-found error as a missing mammoth install by checking err.code for "ERR_MODULE_NOT_FOUND" or "MODULE_NOT_FOUND" (or by accepting "ERR_MODULE_NOT_FOUND" unconditionally since this loader only imports "mammoth"), remove the substring check, and rethrow a user-friendly Error that mentions installing mammoth (preserving the original err as the cause); reference symbols: loadMammoth, _mammoth, and error codes ERR_MODULE_NOT_FOUND / MODULE_NOT_FOUND.
65-67: Stale orphan comments.These two one-line comments (
// Re-export for consumers who import from this moduleand// Import for local use) appear to be leftovers from the removed staticimport * as mammothblock and no longer reference anything. Safe to delete for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/processors/document/WordProcessor.ts` around lines 65 - 67, Remove the two stale one-line comments left over from the removed static import of `mammoth` in the WordProcessor module—specifically delete the lines "// Re-export for consumers who import from this module" and "// Import for local use" in src/lib/processors/document/WordProcessor.ts so the top-of-file comments no longer reference nonexistent code; no functional changes required aside from deleting those orphan comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/processors/document/WordProcessor.ts`:
- Around line 286-321: The inner try/catch in WordProcessor (around the block
calling loadMammoth, mammoth.extractRawText, and mammoth.convertToHtml) is
swallowing the install-hint error from loadMammoth; move the call to
loadMammoth() outside that inner try so its specific error propagates, or detect
the install-hint error after catching (inspect extractError.message or error
type) and rethrow or return a distinct
createError(FileErrorCode.DEPENDENCY_MISSING, { reason: extractError.message },
extractError) so the original install instructions from loadMammoth() are
preserved instead of being wrapped as PROCESSING_FAILED.
---
Nitpick comments:
In `@src/lib/processors/document/WordProcessor.ts`:
- Around line 45-63: The current loadMammoth function relies on
err.message.includes("mammoth") which is fragile; update loadMammoth to treat
any module-not-found error as a missing mammoth install by checking err.code for
"ERR_MODULE_NOT_FOUND" or "MODULE_NOT_FOUND" (or by accepting
"ERR_MODULE_NOT_FOUND" unconditionally since this loader only imports
"mammoth"), remove the substring check, and rethrow a user-friendly Error that
mentions installing mammoth (preserving the original err as the cause);
reference symbols: loadMammoth, _mammoth, and error codes ERR_MODULE_NOT_FOUND /
MODULE_NOT_FOUND.
- Around line 65-67: Remove the two stale one-line comments left over from the
removed static import of `mammoth` in the WordProcessor module—specifically
delete the lines "// Re-export for consumers who import from this module" and
"// Import for local use" in src/lib/processors/document/WordProcessor.ts so the
top-of-file comments no longer reference nonexistent code; no functional changes
required aside from deleting those orphan comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c23047d5-e0ae-4b4c-8de9-d047649afa38
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.jsonsrc/lib/processors/document/WordProcessor.ts
|
🎉 This PR is included in version 9.55.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
anytypes, zero eslint-disable directivesImpact
Test plan
Summary by CodeRabbit