Conversation
…ample configuration
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Heretto CCMS integration (API client, publish trigger, polling, ZIP download/extract), wires it into source processing to load Changes
Sequence Diagram(s)sequenceDiagram
participant Processor as Processor (src/utils.js)
participant Heretto as Heretto Module (src/heretto.js)
participant API as Heretto CCMS API
participant FS as File System
Processor->>Heretto: loadHerettoContent(herettoConfig, log, config)
rect rgba(210,235,255,0.6)
note over Heretto,API: Init & discover scenario
Heretto->>Heretto: createApiClient()
Heretto->>API: GET /scenarios
API-->>Heretto: scenarios[]
Heretto->>API: GET /scenarios/{id}/parameters
API-->>Heretto: parameters
Heretto->>Heretto: validate scenario params
end
rect rgba(220,255,220,0.6)
note over Heretto,API: Trigger publish & poll
Heretto->>API: POST /files/{fileId}/publishes
API-->>Heretto: { jobId }
loop poll until SUCCESS/FAIL or timeout
Heretto->>API: GET /files/{fileId}/publishes/{jobId}/status
API-->>Heretto: status
end
end
rect rgba(255,250,210,0.6)
note over Heretto,FS: Download & extract
Heretto->>API: GET /files/{fileId}/publishes/{jobId}/output (zip)
API-->>Heretto: zip binary
Heretto->>FS: save zip -> temp
Heretto->>FS: extract with AdmZip (safe traversal)
Heretto->>FS: cleanup temp zip
FS-->>Heretto: extraction path
end
Heretto-->>Processor: extraction path | null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{js,ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/*.js📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-11-25T23:59:45.298ZApplied to files:
📚 Learning: 2025-11-25T23:59:45.298ZApplied to files:
📚 Learning: 2025-11-25T23:59:45.298ZApplied to files:
🧬 Code graph analysis (1)src/heretto.js (1)
🔇 Additional comments (7)
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: 9
🧹 Nitpick comments (2)
src/heretto.js (2)
64-69: Consider adding JSDoc for internal helper function.
getPublishingScenarioParameterslacks JSDoc documentation. As per coding guidelines, consider adding JSDoc comments for clarity.+/** + * Fetches parameters for a specific publishing scenario. + * @param {Object} client - Configured axios instance + * @param {string} scenarioId - ID of the publishing scenario + * @returns {Promise<Object>} Scenario parameters object + */ async function getPublishingScenarioParameters(client, scenarioId) {
187-190: Unused function:getPublishingJobHistory.This function is defined but only referenced in commented-out code (lines 346-351). Consider removing it or documenting its intended future use.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/heretto.js(1 hunks)src/heretto.test.js(1 hunks)src/index.js(1 hunks)src/utils.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Use async/await for asynchronous operations
Prefer destructuring for function parameters
Use meaningful variable names that reflect Doc Detective terminology
Add JSDoc comments for complex functions
Files:
src/heretto.test.jssrc/index.jssrc/heretto.jssrc/utils.js
**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.js: When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Mock external dependencies (file system, HTTP requests) in tests
Test both successful and error scenarios
Validate configuration handling thoroughly in tests
Use realistic test data that matches actual usage patterns
Use Mocha for unit tests
Use Chai for assertions
Files:
src/heretto.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use the built-in logging system with available log levels: debug, info, warn, error
Files:
src/heretto.test.jssrc/index.jssrc/heretto.jssrc/utils.js
🧠 Learnings (6)
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Validate configuration handling thoroughly in tests
Applied to files:
src/heretto.test.jssrc/index.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Test both successful and error scenarios
Applied to files:
src/heretto.test.jssrc/index.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use realistic test data that matches actual usage patterns
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Mock external dependencies (file system, HTTP requests) in tests
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to src/**/*.js : Use the built-in logging system with available log levels: debug, info, warn, error
Applied to files:
src/index.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.{js,ts} : Use meaningful variable names that reflect Doc Detective terminology
Applied to files:
src/index.jssrc/utils.js
🧬 Code graph analysis (2)
src/heretto.test.js (1)
src/heretto.js (1)
os(4-4)
src/utils.js (1)
src/heretto.js (1)
outputPath(385-392)
🪛 GitHub Actions: Test (& Publish)
package.json
[error] 1-1: Command failed with exit code 2. npm test ran and 2 tests failed.
src/heretto.test.js
[error] 105-105: Heretto Integration: findScenario should return scenarioId and fileId when valid scenario is found. AssertionError: expected null to deeply equal { scenarioId: 'scenario-123', ... }
[error] 342-342: Heretto Integration: Constants should export expected constants: AssertionError: expected undefined to equal 'Doc Detective'
🔇 Additional comments (13)
src/index.js (1)
37-37: LGTM!The log level change from
"warn"to"warning"aligns with the built-in logging system which supports"error","warning","info", and"debug"levels.src/utils.js (3)
227-230: LGTM!Good defensive guard for empty input sequences with appropriate warning logging.
238-286: Heretto integration logic looks correct.The implementation properly:
- Extracts the integration name from the
heretto:prefix- Looks up configuration from
config.integrations.heretto- Loads content asynchronously and handles failures gracefully
- Injects the output path into the processing sequence
- Tracks already-processed DITA maps to avoid reprocessing
One observation: mutating the
sequencearray while iterating over it (line 262, 282) works correctly here because you're inserting atcurrentIndex + 1and thencontinue-ing, so the for-loop's next iteration picks up the new item. This is intentional and correct for the flow.
307-308: LGTM!The condition correctly skips DITA maps that are already in the
ignoredDitaMapslist, preventing redundant processing of Heretto-loaded content.src/heretto.test.js (1)
1-11: LGTM!Good setup using Mocha, Sinon for stubs, proxyquire for dependency injection, and dynamic Chai import. This follows the project's testing conventions.
src/heretto.js (7)
1-12: LGTM!Clean module structure with well-organized imports and clearly defined internal constants. The polling configuration (5 seconds interval, 5 minutes timeout) is reasonable for a publishing job workflow.
14-52: Well-documented authentication and client setup.Good JSDoc comments, proper Base64 encoding for Basic auth, and sensible 30-second timeout for individual requests.
103-114: Verify API response structure fortranstypeparameter.The code checks
transtypeParam.value !== "dita"but the test data (and possibly the actual API) may return the value nested asoptions[0].value. This mismatch is causing test failures.Please verify the actual Heretto API response structure for scenario parameters. If the API returns:
{ name: "transtype", value: "dita" }→ current code is correct, fix tests{ name: "transtype", options: [{ value: "dita" }] }→ update this check totranstypeParam.options?.[0]?.value
201-238: Polling implementation is solid.Good use of:
- Timeout boundary check with
POLLING_TIMEOUT_MS- Graceful error handling with logging
- Appropriate use of
PromisewithsetTimeoutfor delays- Clear status logging for debugging
250-315: Download and extraction logic is well-implemented.Proper handling of:
- Temp directory creation with
{ recursive: true }- Unique output directory naming using MD5 hash
- Binary download with correct
responseType: "arraybuffer"- Extended timeout (5 minutes) for large downloads
- ZIP cleanup after extraction
- Graceful error handling with fallback to
null
325-403: Main orchestration function is well-structured.Clean async flow with appropriate logging at each stage and proper error handling. The function correctly chains the workflow: create client → find scenario → trigger job → poll status → download/extract.
405-417: LGTM!Clean module exports with functions and constants properly exposed for testing and external use.
package.json (1)
28-28: LGTM!The
adm-zipdependency at version 0.5.16 is appropriately added to support ZIP extraction in the new Heretto integration module. This is the latest stable version with no known vulnerabilities and is widely adopted across the ecosystem.
|
📝 Documentation updates detected! Updated existing suggestion: Document Heretto CMS integration for DITA content testing |
- Change transtype options structure to a single value - Pass scenario name to findScenario function calls - Update constant name from SCENARIO_NAME to DEFAULT_SCENARIO_NAME
There was a problem hiding this comment.
Pull request overview
This PR integrates Heretto API functionality to load external content from Heretto CCMS as processing inputs. The implementation includes API authentication, publishing job triggering, polling mechanisms with timeout handling, and ZIP file extraction. Additionally, it updates dependencies and standardizes log level usage.
- Added comprehensive Heretto API integration with enhanced error handling and logging
- Implemented polling mechanism for asynchronous publishing job completion
- Updated log severity from "warn" to "warning" for consistency
- Added logic to prevent duplicate processing of DITA maps
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| src/heretto.js | New module implementing Heretto CCMS API client, authentication, scenario management, publishing job triggering, polling, and content download/extraction |
| src/heretto.test.js | Comprehensive test suite covering Heretto integration functions with mocked API responses |
| src/utils.js | Integrated Heretto content loading into qualifyFiles, added empty input check, fixed DITA map processing logic, and prevented duplicate processing |
| src/index.js | Fixed log level from "warn" to "warning" for consistency with codebase |
| package.json | Added adm-zip dependency for ZIP handling, updated posthog-node and doc-detective-common versions, updated axios version constraint |
| package-lock.json | Updated lock file to reflect dependency changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Change 'options' to 'value' for 'transtype' in multiple test cases - Update test description to reflect that 'tool-kit-name' parameter can be missing - Add 'scenarioName' to the context in relevant tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/heretto.test.js (1)
1-6: Remove unused imports.The following imports are declared but never used:
assert,path,fs, andos. Removing them will clean up the code.Apply this diff:
-const assert = require("assert"); const sinon = require("sinon"); const proxyquire = require("proxyquire"); -const path = require("path"); -const fs = require("fs"); -const os = require("os");
🧹 Nitpick comments (2)
src/heretto.test.js (2)
204-220: Consider adding an error scenario test.While the success path is well tested, consider adding a test case for when the POST request fails (e.g., network error, invalid scenario ID). This would make the test suite more complete.
Example test case:
it("should throw error when job creation fails", async function () { mockClient.post.rejects(new Error("API error")); await expect( heretto.triggerPublishingJob(mockClient, "file-uuid", "scenario-id") ).to.be.rejected; });As per coding guidelines, testing both success and error scenarios improves test coverage.
281-337: Consider adding a success path test or integration test.The current tests cover important failure scenarios (scenario lookup and job creation failures), but there's no test for the full success path of
loadHerettoContent.Given the complexity of the function (it orchestrates scenario lookup, job triggering, polling, downloading, and extracting), you might consider:
- Adding a unit test that mocks all the intermediate steps and verifies the success path
- Creating an integration test that exercises the full flow with test fixtures
This would provide more confidence in the happy path and align with the coding guideline to "Add comprehensive test coverage when adding new features."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/heretto.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Use async/await for asynchronous operations
Prefer destructuring for function parameters
Use meaningful variable names that reflect Doc Detective terminology
Add JSDoc comments for complex functions
Files:
src/heretto.test.js
**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.js: When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Mock external dependencies (file system, HTTP requests) in tests
Test both successful and error scenarios
Validate configuration handling thoroughly in tests
Use realistic test data that matches actual usage patterns
Use Mocha for unit tests
Use Chai for assertions
Files:
src/heretto.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use the built-in logging system with available log levels: debug, info, warn, error
Files:
src/heretto.test.js
🧠 Learnings (6)
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Mock external dependencies (file system, HTTP requests) in tests
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Validate configuration handling thoroughly in tests
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Test both successful and error scenarios
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use realistic test data that matches actual usage patterns
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Add comprehensive test coverage when adding new features
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.{js,ts} : Use meaningful variable names that reflect Doc Detective terminology
Applied to files:
src/heretto.test.js
🧬 Code graph analysis (1)
src/heretto.test.js (1)
src/heretto.js (3)
authHeader(40-43)scenarioParameters(89-92)completedJob(368-374)
🔇 Additional comments (6)
src/heretto.test.js (6)
18-38: LGTM: Excellent test setup using proxyquire and sinon.The test setup properly mocks the axios client using proxyquire, which allows clean dependency injection without affecting the actual module. The beforeEach/afterEach pattern ensures proper test isolation.
Based on learnings, this follows the guideline to mock external dependencies (HTTP requests) in tests.
40-55: LGTM: Comprehensive coverage for authentication header creation.The tests properly verify Base64 encoding and include special character handling, which is important for password security scenarios.
57-73: LGTM: Proper verification of API client configuration.The test comprehensively verifies the axios client is created with the correct baseURL, authorization header, and content type.
75-202: LGTM: Excellent test coverage for scenario discovery.The test suite thoroughly covers the findScenario function with:
- Success path with valid parameters
- Scenario not found case
- Network failure handling
- Parameter validation for transtype, tool-kit-name, and file_uuid_picker
This aligns with coding guidelines to test both successful and error scenarios with realistic test data.
As per coding guidelines, this properly tests both success and error scenarios and validates configuration handling thoroughly.
222-279: LGTM: Excellent polling test using fake timers.The test suite properly covers:
- Success and failure scenarios
- Polling behavior with multiple intermediate states
- Proper use of
sinon.useFakeTimers()to avoid actual delaysThe fake timer approach is best practice for testing time-dependent async operations.
As per coding guidelines, this tests both success and error scenarios appropriately.
339-345: LGTM: Proper verification of module constants.The test correctly verifies all exported constants with their expected values.
- Update timeout constants for API requests and downloads - Modify findScenario function to use scenarioName parameter - Remove unused scenario description constant - Simplify temp directory creation logic in download function - Clean up test file by removing unnecessary imports
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/heretto.js (1)
292-296: Path traversal vulnerability in ZIP extraction.The
zip.extractAllTo()method doesn't validate that extracted file paths stay withinoutputDir. A malicious ZIP could contain entries like../../../etc/passwdthat write files outside the intended directory (zip slip attack).Consider validating each entry's path before extraction:
// Extract ZIP contents log(config, "debug", `Extracting output to ${outputDir}...`); const zip = new AdmZip(zipPath); - zip.extractAllTo(outputDir, true); + + // Validate and extract entries safely + for (const entry of zip.getEntries()) { + const entryPath = path.join(outputDir, entry.entryName); + const resolvedPath = path.resolve(entryPath); + + // Ensure the resolved path is within outputDir + if (!resolvedPath.startsWith(path.resolve(outputDir) + path.sep)) { + log(config, "warning", `Skipping potentially malicious ZIP entry: ${entry.entryName}`); + continue; + } + + if (entry.isDirectory) { + fs.mkdirSync(resolvedPath, { recursive: true }); + } else { + fs.mkdirSync(path.dirname(resolvedPath), { recursive: true }); + fs.writeFileSync(resolvedPath, entry.getData()); + } + }
🧹 Nitpick comments (3)
src/heretto.js (1)
78-84: Inaccurate JSDoc return description.The JSDoc states "Scenario object or null if creation failed" but this function only finds and validates an existing scenario—it doesn't create anything. The comment is misleading.
/** * Finds an existing publishing scenario by name and validates its configuration. * @param {Object} client - Configured axios instance * @param {Function} log - Logging function * @param {Object} config - Doc Detective config for logging - * @returns {Promise<Object|null>} Scenario object or null if creation failed + * @param {string} scenarioName - Name of the scenario to find + * @returns {Promise<Object|null>} Object with scenarioId and fileId, or null if not found/invalid */src/heretto.test.js (2)
218-275: Good test coverage, but timeout scenario is not tested.The polling tests cover SUCCESS, FAIL, and intermediate polling states well. Consider adding a test for the timeout scenario where the job never completes within
POLLING_TIMEOUT_MS.Example timeout test:
it("should return null on timeout", async function () { const clock = sinon.useFakeTimers(); // Always return PENDING status mockClient.get.resolves({ data: { id: "job-123", status: { status: "PENDING", result: null } } }); const pollPromise = heretto.pollJobStatus(mockClient, "file-uuid", "job-123", mockLog, mockConfig); // Advance past the timeout await clock.tickAsync(heretto.POLLING_TIMEOUT_MS + 1000); const result = await pollPromise; expect(result).to.be.null; clock.restore(); });
277-333: Missing success path and downloadAndExtractOutput coverage.The
loadHerettoContenttests only cover failure scenarios. Per coding guidelines, both successful and error scenarios should be tested. Additionally,downloadAndExtractOutputhas no direct test coverage despite handling critical file system and ZIP operations.Consider adding tests for:
- Success path for
loadHerettoContent- mocking the full workflow through to extraction- Direct tests for
downloadAndExtractOutput- testing download, extraction, and cleanupExample success path test structure:
it("should return output path on successful workflow", async function () { const herettoConfig = { /* ... */ }; // Mock scenario lookup mockClient.get .onCall(0).resolves({ data: { content: [{ id: "scenario-123", name: "Doc Detective" }] } }) .onCall(1).resolves({ data: { content: [/* valid params */] } }) .onCall(2).resolves({ data: { status: { result: "SUCCESS" } } }); // Mock job creation mockClient.post.resolves({ data: { jobId: "job-123" } }); // Need to also mock file system operations for downloadAndExtractOutput // ... });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/heretto.js(1 hunks)src/heretto.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Use async/await for asynchronous operations
Prefer destructuring for function parameters
Use meaningful variable names that reflect Doc Detective terminology
Add JSDoc comments for complex functions
Files:
src/heretto.test.jssrc/heretto.js
**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.js: When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Mock external dependencies (file system, HTTP requests) in tests
Test both successful and error scenarios
Validate configuration handling thoroughly in tests
Use realistic test data that matches actual usage patterns
Use Mocha for unit tests
Use Chai for assertions
Files:
src/heretto.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use the built-in logging system with available log levels: debug, info, warn, error
Files:
src/heretto.test.jssrc/heretto.js
🧠 Learnings (9)
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Validate configuration handling thoroughly in tests
Applied to files:
src/heretto.test.jssrc/heretto.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Mock external dependencies (file system, HTTP requests) in tests
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use realistic test data that matches actual usage patterns
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Test both successful and error scenarios
Applied to files:
src/heretto.test.jssrc/heretto.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Add comprehensive test coverage when adding new features
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use Chai for assertions
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.{js,ts} : Use meaningful variable names that reflect Doc Detective terminology
Applied to files:
src/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.{js,ts} : Add JSDoc comments for complex functions
Applied to files:
src/heretto.js
🧬 Code graph analysis (2)
src/heretto.test.js (1)
src/heretto.js (3)
authHeader(41-44)scenarioParameters(95-98)completedJob(361-367)
src/heretto.js (1)
src/heretto.test.js (5)
scenarioParameters(85-91)scenarioParameters(132-138)scenarioParameters(157-162)scenarioParameters(181-186)scenarioParameters(311-317)
🔇 Additional comments (12)
src/heretto.js (6)
1-13: LGTM!The imports are appropriate and the constants are well-named with clear inline documentation. The separate timeout constants (API_REQUEST_TIMEOUT_MS vs DOWNLOAD_TIMEOUT_MS) address previous magic number concerns.
15-33: LGTM!Both helper functions are simple, focused, and have appropriate JSDoc documentation. The auth header creation follows standard Basic Auth patterns.
35-53: LGTM!The axios client creation is clean with proper timeout configuration and authorization headers.
55-76: LGTM!Simple API wrapper functions with appropriate JSDoc documentation. The null-coalescing fallback (
|| []) ingetPublishingScenariosis a good defensive practice.
164-239: LGTM!The publishing job workflow functions are well-implemented:
triggerPublishingJobcleanly triggers the jobgetJobStatusis a simple status checkpollJobStatuscorrectly handles SUCCESS/FAIL states with timeout protection and appropriate logging levels
316-410: LGTM!The orchestrator function is well-structured with comprehensive error handling. The fallback to
DEFAULT_SCENARIO_NAMEwhenherettoConfig.scenarioNameis not provided is a good pattern. The module exports appropriately expose functions and constants needed for testing.src/heretto.test.js (6)
1-34: LGTM!The test setup is clean and follows best practices:
- Dynamic import for chai with global assignment handles ESM compatibility
- Proxyquire properly injects the mocked axios
afterEachensures sinon stubs are restored between tests
36-51: LGTM!Good test coverage for
createAuthHeaderincluding the edge case with special characters in credentials.
53-69: LGTM!The test properly validates that
createApiClientconfigures the axios instance with the correct baseURL, authorization header, and content type.
71-198: LGTM!Comprehensive test coverage for
findScenarioincluding:
- Success case with valid scenario
- Scenario not found
- Network error handling
- Parameter validation failures (transtype, tool-kit-name, file_uuid_picker)
The test data structure correctly uses
value: "dita"matching the production code expectations. As per coding guidelines, both successful and error scenarios are tested.
200-216: LGTM!The test validates the correct API endpoint and request payload for job triggering. The error scenario is covered indirectly in
loadHerettoContenttests.
335-341: LGTM!The constants export test properly verifies the public API contract for the exported constants.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return null; | ||
| } | ||
|
|
||
| // Make sure that scenarioParameters.content has an object with name="tool-kit-name" and value="default/dita-ot-3.6.1" |
There was a problem hiding this comment.
The comment mentions checking for 'value="default/dita-ot-3.6.1"', but the code only validates that the value exists (is truthy), not that it matches this specific version. The comment should be updated to match the actual validation logic, or the validation should be strengthened to check for the specific value if that's a requirement.
|
|
||
| const ignoredDitaMaps = []; | ||
|
|
||
| for (let source of sequence) { |
There was a problem hiding this comment.
Modifying the sequence array during iteration with for...of can lead to unexpected behavior. While splicing elements after the current index will cause them to be iterated, this pattern is error-prone and hard to reason about. Consider using a traditional for loop with an index (e.g., for (let i = 0; i < sequence.length; i++)) or collecting modifications to apply after the loop completes, to make the iteration behavior more predictable and maintainable.
| /** | ||
| * Finds an existing publishing scenario by name and validates its configuration. | ||
| * @param {Object} client - Configured axios instance | ||
| * @param {Function} log - Logging function | ||
| * @param {Object} config - Doc Detective config for logging | ||
| * @returns {Promise<Object|null>} Scenario object or null if creation failed | ||
| */ |
src/heretto.js
Outdated
| * @param {Object} client - Configured axios instance | ||
| * @param {Function} log - Logging function | ||
| * @param {Object} config - Doc Detective config for logging | ||
| * @returns {Promise<Object|null>} Scenario object or null if creation failed |
There was a problem hiding this comment.
The function documentation states it returns "Scenario object or null if creation failed", but the function doesn't create scenarios - it only finds and validates existing ones. The return description should be updated to accurately reflect that it returns a scenario object if found and validated, or null if not found or validation fails.
| path.extname(source) === ".ditamap" && | ||
| config.processDitaMap | ||
| !ignoredDitaMaps.some((ignored) => source.includes(ignored)) && | ||
| config.processDitaMaps |
There was a problem hiding this comment.
The config property name has been changed from "processDitaMap" (singular) to "processDitaMaps" (plural), which is inconsistent with the function name "processDitaMap" (singular). This appears to be a naming inconsistency that could lead to confusion. Consider using consistent singular or plural naming throughout, or verify this matches the actual config schema definition.
- Update findScenario function to include scenarioName parameter and clarify return type - Implement path traversal protection in downloadAndExtractOutput function - Add tests for error handling in triggerPublishingJob and pollJobStatus functions - Create tests for downloadAndExtractOutput to validate ZIP extraction and malicious entry handling
|
📝 Documentation updates detected! Updated existing suggestion: Document Heretto CMS integration for DITA content testing |
Implement Heretto integration with enhanced logging and error handling. Update dependencies and improve timeout settings for downloads. Revise tests and ignore already processed DITA maps to streamline processing.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.