Conversation
…nization - Added HerettoUploader class to handle file uploads to Heretto CMS. - Integrated uploader into the existing upload process for changed screenshots. - Enhanced screenshot saving logic to track changes and source integration metadata. - Created documentation for Heretto screenshot sync feature, detailing the upload process and architecture. - Updated resolver to build file mappings and handle Heretto-specific configurations. - Introduced utility functions to find Heretto integrations based on file paths. - Added error handling and logging for upload operations to improve reliability.
- Update `saveScreenshot.js` to mark new screenshots as changed for upload. - Add a new test file `screenshot.test.js` to verify sourceIntegration preservation and changed status for screenshots. - Create a new test file `upload.test.js` to validate the upload module, including collecting changed files and integration configuration. - Modify `package-lock.json` to link to local `doc-detective-common` instead of a versioned package. - Enhance `heretto.js` to retrieve resource dependencies from the Heretto REST API, including detailed logging and XML parsing.
… integration tests - Refactor folder resolution logic to ensure correct target folder is identified before file upload. - Implement integration tests for Heretto upload functionality, covering document retrieval and upload processes. - Update existing tests to reflect changes in folder resolution behavior.
…tails - Add screenshotPath to outputs when overwriting existing files - Set changed flag to true when a file is overwritten - Preserve sourceIntegration metadata in outputs if provided
- Bump appium-chromium-driver to version 2.1.0 - Bump appium-geckodriver to version 2.1.6 - Bump appium-safari-driver to version 4.1.6 - Update doc-detective-common to version 3.6.0-dev.2 - Update doc-detective-resolver to version 3.6.1-dev.2 - Upgrade posthog-node to version 5.18.1 - Upgrade webdriverio to version 9.23.0
- Change "doc-detective-common" and "doc-detective-resolver" versions to fixed versions
|
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 CMS uploader and an integrations registry, propagates sourceIntegration metadata for changed screenshots, triggers optional post-test uploads (uploadOnChange), and introduces unit/integration tests plus dependency bumps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Test Runner
participant SaveShot as saveScreenshot
participant Integrations as Integrations Module
participant Uploader as HerettoUploader
participant Heretto as Heretto API
participant FS as File System
Runner->>SaveShot: execute screenshot step
SaveShot->>SaveShot: diff/crop -> determine changed
alt changed
SaveShot-->>Runner: result (outputs.changed=true, outputs.sourceIntegration)
else unchanged
SaveShot-->>Runner: result (outputs.changed=false)
end
Runner->>Integrations: tests complete -> collectChangedFiles(report)
Integrations->>Integrations: getUploader(sourceIntegration)
loop per changed file
Integrations->>Uploader: upload({config, integrationConfig, localFilePath,...})
Uploader->>Uploader: resolveFromDependencies / findParentFolderFromDependencies
Uploader->>Heretto: GET/SEARCH folders & documents
Heretto-->>Uploader: metadata / IDs
Uploader->>FS: read binary file
FS-->>Uploader: file bytes
Uploader->>Heretto: POST/PUT upload content
Heretto-->>Uploader: upload response
Uploader-->>Integrations: {status, description}
end
Integrations-->>Runner: aggregated uploadResults
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
dev/index.js (1)
11-11: Remove commented developer-specific path.This commented line contains a local absolute path that appears to be developer-specific debugging code and should be removed before merging.
🔎 Proposed fix
input: ["heretto:example"], - // input: ["c:\\Users\\hawkeyexl\\AppData\\Local\\Temp\\doc-detective\\heretto_c96e625d5c1ee50972362046445a5ca4\\ot-output\\dita\\_topics\\espresso.dita"], logLevel: "debug",src/tests.js (1)
801-817: Consider making the integration check more generic.The upload logic currently checks specifically for
config?.integrations?.heretto?.length > 0, but theuploadChangedFilesfunction appears to support multiple integration types through a registry pattern. Consider checking for any configured integrations that support upload rather than hardcoding Heretto.🔎 Proposed refactor to support multiple integration types
- // Upload changed files back to source integrations (best-effort) - // This automatically syncs any changed screenshots back to their source CMS - if (config?.integrations?.heretto?.length > 0) { + // Upload changed files back to source integrations (best-effort) + // This automatically syncs any changed screenshots back to their source CMS + if (config?.integrations && Object.keys(config.integrations).length > 0) { try { const uploadResults = await uploadChangedFiles({ config, report, log }); report.uploadResults = uploadResults; } catch (error) { log(config, "warning", `Failed to upload changed files: ${error.message}`); report.uploadResults = { total: 0, successful: 0, failed: 0, skipped: 0, error: error.message, }; } }test/screenshot.test.js (1)
43-51: Follow coding guidelines for temporary file cleanup.The cleanup pattern doesn't use try/finally blocks as recommended in the coding guidelines. While the current afterEach approach works, using try/finally within each test would ensure cleanup even if assertions fail.
Based on learnings, test files should use
fs.writeFileSync()+fs.unlinkSync()in try/finally blocks for temporary test files to ensure cleanup.Example pattern:
it("test case", async function () { const tempFilePath = path.join(tempDir, "test-spec.json"); try { fs.writeFileSync(tempFilePath, JSON.stringify(testSpec, null, 2)); const result = await runTests({ input: tempFilePath, logLevel: "silent" }); // assertions... } finally { if (fs.existsSync(tempFilePath)) { fs.unlinkSync(tempFilePath); } } });test/heretto-upload.test.js (2)
128-162: Consider test isolation for document creation.The test stores
testDocumentIdin a shared variable that's used by subsequent tests, creating interdependence. If this test fails, all subsequent tests that depend ontestDocumentIdwill be skipped. Consider making tests more independent or using abeforehook to set up shared test resources.This is acceptable for integration tests but worth noting for maintainability.
279-377: Consider cleanup of test documents in Heretto.The integration tests create documents in Heretto CMS (e.g., lines 314-332 create a file in the _media folder) but don't clean them up afterward. While this may be acceptable for a test environment, consider whether these test artifacts should be removed to prevent accumulation.
If cleanup is desired, you could track created document IDs and delete them in an
afterhook:const createdDocIds = []; after(async function () { if (hasCredentials && createdDocIds.length > 0) { // Delete test documents for (const docId of createdDocIds) { // await herettoUploader.deleteDocument({ ... }); } } });test/upload.test.js (1)
308-309: Naming nit:HerettoUploaderis an instance, not a class.Using PascalCase (
HerettoUploader) typically implies a class or constructor, butgetUploader()returns an instance. Consider renaming toherettoUploaderfor clarity.🔎 Suggested rename
describe("HerettoUploader", function () { - const HerettoUploader = getUploader({ type: "heretto" }); + const herettoUploader = getUploader({ type: "heretto" }); describe("resolveFromDependencies", function () { // Update all references from HerettoUploader to herettoUploadersrc/integrations/heretto.js (3)
380-395: Regex-based XML parsing may be fragile with attribute ordering variations.The regex pattern assumes
nameattribute comes beforeid:`<folder\\s+name="${escapedFolderName}"\\s+id="([^"]+)"`If the API returns attributes in different order (e.g.,
<folder id="..." name="...">), the match will fail. Consider matching both orderings or using a more flexible pattern.🔎 Suggested fix to handle attribute order variations
- const folderMatch = data.match(new RegExp(`<folder\\s+name="${escapedFolderName}"\\s+id="([^"]+)"`, 'i')); + // Match folder with name attribute, handling attribute order variations + const folderRegex = new RegExp( + `<folder\\s+(?:[^>]*?\\s)?name="${escapedFolderName}"[^>]*?\\sid="([^"]+)"|` + + `<folder\\s+(?:[^>]*?\\s)?id="([^"]+)"[^>]*?\\sname="${escapedFolderName}"`, + 'i' + ); + const folderMatch = data.match(folderRegex); if (folderMatch && folderMatch[1]) { - log("debug", `Found child folder '${folderName}' with ID: ${folderMatch[1]}`); - resolve(folderMatch[1]); + const folderId = folderMatch[1] || folderMatch[2]; + log("debug", `Found child folder '${folderName}' with ID: ${folderId}`); + resolve(folderId);
624-628: Fallback to first search result may upload to wrong folder.When an exact folder name match isn't found, the code falls back to the first search result. This could lead to uploading files to an unintended folder if the search returns partial matches (e.g., searching for "media" returns "media-archive" first).
Consider returning
nullinstead and letting the caller handle the ambiguity, or at minimum logging at "warning" level since this is a potentially incorrect resolution.🔎 Suggested fix for safer behavior
if (match) { log("debug", `Found folder: ${folderName} with ID: ${match.uuid || match.id}`); resolve(match.uuid || match.id); } else { - // Take first result as fallback - log("debug", `Exact folder match not found, using first result: ${result.searchResults[0].uuid || result.searchResults[0].id}`); - resolve(result.searchResults[0].uuid || result.searchResults[0].id); + // No exact match - return null to avoid uploading to wrong folder + log("warning", `No exact folder match for '${folderName}', found ${result.searchResults.length} partial matches`); + resolve(null); }
704-709: Same fallback concern applies to file search.Similar to
searchFolderByName, falling back to the first search result for files could upload content to the wrong file if multiple files share the same name in different folders.🔎 Consistent with folder search suggestion
if (match) { resolve(match.uuid || match.id); } else { - // Take first result as fallback - resolve(result.searchResults[0].uuid || result.searchResults[0].id); + // No exact match - return null to avoid overwriting wrong file + log("warning", `No exact file match for '${filename}', found ${result.searchResults.length} partial matches`); + resolve(null); }
📜 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 (9)
dev/index.jspackage.jsonsrc/integrations/heretto.jssrc/integrations/index.jssrc/tests.jssrc/tests/saveScreenshot.jstest/heretto-upload.test.jstest/screenshot.test.jstest/upload.test.js
🧰 Additional context used
📓 Path-based instructions (4)
src/tests.js
📄 CodeRabbit inference engine (AGENTS.md)
src/tests.js: Add new driver-based step actions to thedriverActionsarray insrc/tests.jsand implement case inrunStep()switch statement
UsestepExecutionFailedflag to skip remaining steps in a context after first failure
Set Appium driver timeout to 10 minutes (newCommandTimeout: 600) for all driver-based steps
runSpecs()insrc/tests.jsorchestrates test execution flow: spec → test → context → step hierarchy
Files:
src/tests.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Uselog(config, level, message)for all logging, where level = "debug"|"info"|"warning"|"error"
UsegetAvailableApps()to detect installed browsers instead of hardcoding browser paths
Files:
src/tests.jssrc/integrations/heretto.jssrc/tests/saveScreenshot.jssrc/integrations/index.js
test/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*.js: Usefs.writeFileSync()+fs.unlinkSync()in try/finally blocks for temporary test files to ensure cleanup
HTTP request tests must run against test server on port 8092 intest/server/
Files:
test/upload.test.jstest/heretto-upload.test.jstest/screenshot.test.js
src/tests/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/tests/*.js: Create new step type handlers as async functions exported fromsrc/tests/[actionName].js
Step handlers must return object with{ status: "PASS"|"FAIL"|"WARNING", description: string, outputs: {} }
Validate step schema before resolving to object and setting defaults in new step handlers
Always handle driver cleanup in try/finally blocks in browser automation code
Set step variables viastep.variables = { MY_VAR: "$$response.body.token" }to store data as environment variables
UsesetViewportSize()to calculate inner dimensions rather than directly setting window size in browser automation
Mark unsafe steps withstep.unsafe = trueand requireconfig.allowUnsafeSteps = trueto execute
Files:
src/tests/saveScreenshot.js
🧠 Learnings (13)
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/**/*.js : Use `fs.writeFileSync()` + `fs.unlinkSync()` in try/finally blocks for temporary test files to ensure cleanup
Applied to files:
src/tests.jstest/upload.test.jstest/heretto-upload.test.jstest/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Create new step type handlers as async functions exported from `src/tests/[actionName].js`
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: `src/tests.js` is the core orchestrator handling 600+ LOC of Appium/WebDriver management
Applied to files:
src/tests.jstest/upload.test.jstest/heretto-upload.test.jstest/screenshot.test.jspackage.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : `runSpecs()` in `src/tests.js` orchestrates test execution flow: spec → test → context → step hierarchy
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/index.js : `src/index.js` must export `runTests()` as the main entry point for the package
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Individual step types have dedicated handler implementations in `src/tests/` (e.g., `httpRequest.js`, `runShell.js`)
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Tests follow nested hierarchy: spec (file) → test → context (browser/platform combo) → step (action), with contexts running serially and steps skipping after first failure
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/config.js : Browsers and drivers must match the current platform (detected via `puppeteer/browsers`) with Appium drivers installed for chromium, gecko, and safari
Applied to files:
src/tests.jspackage.json
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : Add new driver-based step actions to the `driverActions` array in `src/tests.js` and implement case in `runStep()` switch statement
Applied to files:
src/tests.jstest/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/**/*.js : HTTP request tests must run against test server on port 8092 in `test/server/`
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Use `setViewportSize()` to calculate inner dimensions rather than directly setting window size in browser automation
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Mark unsafe steps with `step.unsafe = true` and require `config.allowUnsafeSteps = true` to execute
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Step handlers must return object with `{ status: "PASS"|"FAIL"|"WARNING", description: string, outputs: {} }`
Applied to files:
src/tests/saveScreenshot.js
🧬 Code graph analysis (5)
test/upload.test.js (1)
test/heretto-upload.test.js (4)
assert(3-3)require(4-4)result(314-332)mockLog(38-42)
test/heretto-upload.test.js (1)
test/upload.test.js (3)
HerettoUploader(309-309)mockLog(312-312)mockLog(431-431)
test/screenshot.test.js (1)
src/tests/saveScreenshot.js (1)
result(21-27)
src/tests/saveScreenshot.js (3)
src/tests/findElement.js (1)
result(15-19)src/tests/runShell.js (2)
result(15-25)fractionalDiff(141-144)src/utils.js (1)
fractionalDiff(284-284)
package.json (1)
scripts/bump-sync-version-resolver.js (1)
main(21-102)
⏰ 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). (9)
- GitHub Check: test (ubuntu-latest, 22)
- GitHub Check: test (macos-latest, 22)
- GitHub Check: test (macos-latest, 24)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (windows-latest, 24)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (ubuntu-latest, 24)
- GitHub Check: test (windows-latest, 22)
- GitHub Check: test (ubuntu-latest, 20)
🔇 Additional comments (25)
dev/index.js (1)
34-34: LGTM! New Heretto upload feature enabled.The
uploadOnChange: trueflag correctly enables automatic upload of changed screenshots back to Heretto CMS, aligning with the PR's objectives.src/tests.js (1)
29-29: LGTM! Clean integration import.The import of
uploadChangedFilesfrom the new integrations module is properly structured.test/screenshot.test.js (4)
54-97: LGTM! Comprehensive test for new screenshot sourceIntegration.The test correctly verifies that sourceIntegration metadata is preserved when creating new screenshots, and that the
changedflag is set totrue. The assertions cover all required fields.
99-169: LGTM! Thorough test for variation threshold handling.The test properly validates the scenario where screenshot variation exceeds the threshold, verifying both the WARNING result and the preservation of sourceIntegration metadata with
changed: true.
171-241: LGTM! Validates within-variation behavior correctly.The test accurately checks that sourceIntegration is preserved when screenshots are within the variation threshold, with
changed: falseand a PASS result.
243-272: LGTM! Correctly validates absence of sourceIntegration.The test appropriately confirms that sourceIntegration is not set when not provided in the screenshot configuration.
test/heretto-upload.test.js (5)
15-50: LGTM! Proper environment-based test gating.The test suite correctly checks for required credentials and skips tests when unavailable, with clear messaging to developers. The debug logging via
DEBUG_HERETTOis a nice touch.
52-92: LGTM! Comprehensive getDocumentInfo validation.The tests thoroughly validate document retrieval, including verification of all expected metadata fields (id, name, folderUuid, uri, mimeType) and correct folder placement.
94-126: LGTM! Proper file lookup testing.The tests correctly verify both successful file lookup and null return for non-existent files, covering positive and negative cases.
284-376: LGTM! Proper cleanup with try/finally pattern.The test correctly uses try/finally blocks to ensure temporary files and directories are cleaned up regardless of test outcome, following the coding guidelines.
Based on learnings, test files should use
fs.writeFileSync()+fs.unlinkSync()in try/finally blocks for temporary test files to ensure cleanup.
379-436: LGTM! Thorough validation of document updates.The test comprehensively verifies updating existing documents, including content verification with PNG signature validation. Well structured.
package.json (1)
35-49: Upgrade appium-chromium-driver to address CVE-2016-10557 (insecure binary download vulnerability).The installed version 2.1.0 is affected by CVE-2016-10557, which allows man-in-the-middle attacks during Chromedriver binary download over insecure HTTP, potentially enabling remote code execution. Upgrade to version 2.9.4 or later to remediate.
Other checked dependencies (appium-geckodriver, webdriverio, posthog-node, axios) are secure in their current versions.
⛔ Skipped due to learnings
Learnt from: CR Repo: doc-detective/core PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-03T00:21:10.834Z Learning: Applies to src/config.js : Browsers and drivers must match the current platform (detected via `puppeteer/browsers`) with Appium drivers installed for chromium, gecko, and safariLearnt from: CR Repo: doc-detective/core PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-03T00:21:10.834Z Learning: `src/tests.js` is the core orchestrator handling 600+ LOC of Appium/WebDriver managementsrc/tests/saveScreenshot.js (5)
24-27: LGTM! Well-structured outputs initialization.The outputs object correctly initializes with
changed: falseas the default, following the step handler convention of returning{ status, description, outputs }. Based on learnings, this aligns with the expected step handler return structure.
296-301: Correct metadata propagation for overwrite scenario.The overwrite path properly sets
changed: trueand preservessourceIntegrationwhen present, enabling the upload workflow to identify and process changed files.
373-378: Correct handling of above-variation case.When the screenshot difference exceeds
maxVariation, the code properly marks it as changed and preserves the integration metadata for subsequent upload processing.
384-388: Correct behavior for within-variation case.When screenshots are within acceptable variation,
changedcorrectly remainsfalse(from initialization) while still preserving thesourceIntegrationmetadata for output consistency.
396-405: Good handling of new screenshot case.The guard condition
!result.outputs.screenshotPathensures this block only executes for truly new screenshots. Marking new files aschanged: trueenables them to be uploaded to the CMS.test/upload.test.js (1)
1-650: Comprehensive test coverage for the upload module.The test suite thoroughly covers:
- Edge cases for
collectChangedFiles(null, empty, missing fields)getIntegrationConfiglookup scenariosgetUploaderregistry resolution- HerettoUploader utility methods (
resolveFromDependencies,findParentFolderFromDependencies,canHandle,getContentType,escapeXml)- Upload validation (missing config, credentials)
The tests use in-memory mock objects appropriately, avoiding the need for temporary file cleanup.
src/integrations/index.js (3)
79-191: Well-designed best-effort upload orchestration.The function properly handles:
- Missing uploaders (skipped, not failed)
- Missing integration configs (skipped with reason)
- Upload failures (caught and logged, continues processing)
- Detailed result tracking with status per file
The logging follows the
log(config, level, message)pattern per coding guidelines.
39-68: Correct report traversal for collecting changed files.The function properly navigates the nested report structure and validates all required conditions before collecting a file. The inclusion of
stepId,testId, andspecIdin the result provides useful context for debugging and tracking.
218-223: Good interface validation for uploader registration.The validation ensures all registered uploaders implement the required
canHandleanduploadmethods, maintaining type safety at runtime.src/integrations/heretto.js (4)
520-527: Good handling of attribute order variations in file matching.The dual-regex approach (
nameIdMatchandidNameMatch) correctly handles both possible attribute orderings in the XML response.
34-228: Well-structured upload workflow with comprehensive fallback strategy.The upload method implements a robust resolution chain:
- Try resource dependencies map (most accurate)
- Find parent folder from dependencies or via API
- Look for file in target folder
- Create new document if not found
- Fall back to global search as last resort
Each step includes appropriate logging for debugging failed uploads.
557-564: Correct XML escaping implementation.The
&character is correctly escaped first to prevent double-escaping other entities. All five standard XML entities are properly handled.
913-937: Comprehensive content type mapping.The
getContentTypemethod covers common image formats and DITA/XML content types relevant to documentation workflows. The fallback toapplication/octet-streamfor unknown extensions is appropriate.
- Set a 30-second timeout for various requests in HerettoUploader to prevent hanging. - Update getIntegrationConfig to return null instead of undefined for unknown integrations. - Increase timeout for screenshot tests to 60 seconds for better stability.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/upload.test.js (1)
262-272: Verify assertion matches actual function behavior.The test description says "returns null when heretto config not found by name" and line 271 asserts
resultequalsnull. However, if the underlying implementation usesArray.find()(which returnsundefinedwhen no match is found), this test may fail.Verify that
getIntegrationConfigexplicitly returnsnull(notundefined) when no config is found:#!/bin/bash # Description: Check getIntegrationConfig implementation to verify return value rg -A 10 "function getIntegrationConfig|getIntegrationConfig.*=" src/integrations/index.js
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/integrations/heretto.jssrc/integrations/index.jstest/screenshot.test.jstest/upload.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/integrations/index.js
- src/integrations/heretto.js
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*.js: Usefs.writeFileSync()+fs.unlinkSync()in try/finally blocks for temporary test files to ensure cleanup
HTTP request tests must run against test server on port 8092 intest/server/
Files:
test/upload.test.jstest/screenshot.test.js
🧠 Learnings (9)
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: `src/tests.js` is the core orchestrator handling 600+ LOC of Appium/WebDriver management
Applied to files:
test/upload.test.jstest/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/**/*.js : Use `fs.writeFileSync()` + `fs.unlinkSync()` in try/finally blocks for temporary test files to ensure cleanup
Applied to files:
test/upload.test.jstest/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/**/*.js : HTTP request tests must run against test server on port 8092 in `test/server/`
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Use `setViewportSize()` to calculate inner dimensions rather than directly setting window size in browser automation
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : Add new driver-based step actions to the `driverActions` array in `src/tests.js` and implement case in `runStep()` switch statement
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/core.test.js : Test suite runs with mocha using `this.timeout(0)` for indefinite timeout in `test/core.test.js`
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : Set Appium driver timeout to 10 minutes (`newCommandTimeout: 600`) for all driver-based steps
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Mark unsafe steps with `step.unsafe = true` and require `config.allowUnsafeSteps = true` to execute
Applied to files:
test/screenshot.test.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Step handlers must return object with `{ status: "PASS"|"FAIL"|"WARNING", description: string, outputs: {} }`
Applied to files:
test/screenshot.test.js
🧬 Code graph analysis (1)
test/screenshot.test.js (4)
src/integrations/heretto.js (3)
fs(5-5)require(9-9)path(6-6)src/tests/saveScreenshot.js (6)
fs(5-5)require(1-1)require(2-2)require(3-3)path(4-4)saveScreenshot(20-379)src/integrations/index.js (1)
require(10-10)test/core.test.js (5)
screenshotPath(227-317)require(419-825)cleanup(451-465)file(46-57)cleanup(809-824)
⏰ 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). (10)
- GitHub Check: Agent
- GitHub Check: test (windows-latest, 22)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (macos-latest, 24)
- GitHub Check: test (macos-latest, 22)
- GitHub Check: test (ubuntu-latest, 22)
- GitHub Check: test (ubuntu-latest, 20)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (ubuntu-latest, 24)
- GitHub Check: test (windows-latest, 24)
🔇 Additional comments (4)
test/upload.test.js (4)
308-428: LGTM: Comprehensive test coverage for HerettoUploader path resolution.The tests for
resolveFromDependencies(lines 311-428) provide excellent coverage of edge cases including:
- Null/undefined handling
- Exact path matching
- Relative path normalization
- Filename+folder matching fallback
- Filename-only matching as last resort
- Internal key filtering (underscore prefix)
- Windows backslash normalization
This thorough testing will help catch path-matching issues across different operating systems and directory structures.
430-524: LGTM: Thorough testing of folder resolution logic.The tests for
findParentFolderFromDependencies(lines 430-524) cover critical scenarios:
- Null/undefined safety
- Sibling file matching
- Direct path matching
- Ditamap fallback behavior
- Preference ordering (sibling over fallback)
- Relative path handling
This ensures robust folder resolution when uploading new files.
526-610: LGTM: Complete utility function coverage.The tests for
canHandle,getContentType, andescapeXml(lines 526-610) provide comprehensive validation:
- Type validation for multiple integration types
- Content-type mapping for common image formats
- XML escaping for all special characters
- Edge cases (null, undefined, empty strings, unknown extensions)
These utility tests ensure correct behavior for file handling and XML safety.
612-648: LGTM: Essential validation checks for upload.The upload validation tests (lines 612-648) verify that the uploader fails gracefully with clear error messages when required configuration is missing (integrationConfig, organizationId, apiToken). This prevents confusing runtime errors during actual upload operations.
There was a problem hiding this comment.
Pull request overview
This PR implements a Heretto CMS uploader to automatically synchronize changed screenshots back to Heretto CMS. It introduces an extensible integration framework that can be expanded to support other CMS platforms in the future.
Key Changes:
- Adds HerettoUploader class with folder resolution, file searching, document creation, and content upload capabilities
- Enhances screenshot saving logic to track changes and preserve source integration metadata
- Implements automatic upload of changed screenshots after test execution
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/integrations/index.js | Provides integration uploader registry and orchestrates changed file uploads |
| src/integrations/heretto.js | Implements Heretto-specific upload logic with resource dependency resolution and API interactions |
| src/tests/saveScreenshot.js | Tracks screenshot changes and preserves sourceIntegration metadata in outputs |
| src/tests.js | Triggers automatic upload of changed files after test completion for configured integrations |
| test/upload.test.js | Unit tests for integration module functions |
| test/screenshot.test.js | Integration tests for screenshot sourceIntegration preservation |
| test/heretto-upload.test.js | Integration tests for Heretto upload functionality requiring live credentials |
| package.json | Updates dependencies to newer versions |
| dev/index.js | Adds uploadOnChange configuration flag to development config |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/integrations/heretto.js
Outdated
| /** | ||
| * Resolves a file path to its UUID using the resource dependencies map. | ||
| * @param {Object} options - Resolution options | ||
| * @returns {Object|null} File info with uuid and parentFolderId, or null if not found |
There was a problem hiding this comment.
The JSDoc for this method is incomplete. It should document the structure of the options parameter, including the required properties: resourceDependencies, filePath, filename, and log. Additionally, the return value structure with uuid and parentFolderId properties should be documented.
| * @returns {Object|null} File info with uuid and parentFolderId, or null if not found | |
| * @param {Object.<string, {uuid: string, parentFolderId: string}>} options.resourceDependencies - Map of resource paths to resource metadata | |
| * @param {string} options.filePath - Original file path to resolve | |
| * @param {string} options.filename - Filename extracted from the file path | |
| * @param {Function} options.log - Logging function | |
| * @returns {{uuid: string, parentFolderId: string}|null} File info with uuid and parentFolderId, or null if not found |
src/integrations/index.js
Outdated
| /** | ||
| * Collects all changed files from a test report that have source integrations. | ||
| * @param {Object} report - Test execution report | ||
| * @returns {Array<Object>} Array of { localPath, sourceIntegration } objects |
There was a problem hiding this comment.
The JSDoc return type documentation is incomplete. The return array should include all properties that are pushed to the changedFiles array: localPath, sourceIntegration, stepId, testId, and specId. Update the JSDoc to reflect the complete structure.
| * @returns {Array<Object>} Array of { localPath, sourceIntegration } objects | |
| * @returns {Array<Object>} Array of objects with properties: | |
| * - localPath | |
| * - sourceIntegration | |
| * - stepId | |
| * - testId | |
| * - specId |
dev/index.js
Outdated
| async function main() { | ||
| const json = { | ||
| input: ["heretto:example"], | ||
| // input: ["c:\\Users\\hawkeyexl\\AppData\\Local\\Temp\\doc-detective\\heretto_c96e625d5c1ee50972362046445a5ca4\\ot-output\\dita\\_topics\\espresso.dita"], |
There was a problem hiding this comment.
This commented-out line contains a hardcoded Windows absolute path that appears to be from a developer's local machine. This should be removed as it provides no value and could expose internal directory structure information.
| // input: ["c:\\Users\\hawkeyexl\\AppData\\Local\\Temp\\doc-detective\\heretto_c96e625d5c1ee50972362046445a5ca4\\ot-output\\dita\\_topics\\espresso.dita"], |
src/integrations/heretto.js
Outdated
| try { | ||
| // Parse XML to find the folder by name in children | ||
| // Looking for: <folder name="folderName" id="uuid"/> | ||
| const escapedFolderName = folderName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); |
There was a problem hiding this comment.
The regex pattern uses single backslash for escaping, but in JavaScript strings, backslashes need to be doubled. The character class should use '\\' to properly escape the backslash character itself. This may cause issues when matching folder names containing backslashes.
src/integrations/heretto.js
Outdated
| // Parse XML to find the file by name | ||
| // Looking for child resources with matching name | ||
| // Example: <resource id="uuid" name="filename">... | ||
| const escapedFilename = filename.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); |
There was a problem hiding this comment.
The regex pattern uses single backslash for escaping, but in JavaScript strings, backslashes need to be doubled. The character class should use '\\' to properly escape the backslash character itself. This may cause issues when matching filenames containing backslashes.
src/integrations/index.js
Outdated
| * @param {Object} options.config - Doc Detective config | ||
| * @param {Object} options.report - Test execution report | ||
| * @param {Function} options.log - Logging function | ||
| * @returns {Promise<Object>} Upload results summary |
There was a problem hiding this comment.
The JSDoc return type documentation is incomplete. The return object structure should document all properties: total, successful, failed, skipped, and details (which is an array of objects with localPath, status, and description/reason properties). Update the JSDoc to reflect the complete structure.
| * @returns {Promise<Object>} Upload results summary | |
| * @returns {Promise<{ | |
| * total: number, | |
| * successful: number, | |
| * failed: number, | |
| * skipped: number, | |
| * details: Array<{ | |
| * localPath: string, | |
| * status: string, | |
| * description?: string, | |
| * reason?: string | |
| * }> | |
| * }>} Upload results summary |
| for (const file of changedFiles) { | ||
| const uploader = getUploader(file.sourceIntegration); | ||
|
|
||
| if (!uploader) { | ||
| log( | ||
| config, | ||
| "warning", | ||
| `No uploader found for integration type: ${file.sourceIntegration.type}` | ||
| ); | ||
| results.skipped++; | ||
| results.details.push({ | ||
| localPath: file.localPath, | ||
| status: "SKIPPED", | ||
| reason: `No uploader for type: ${file.sourceIntegration.type}`, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| // Get the integration config for API credentials | ||
| const integrationConfig = getIntegrationConfig( | ||
| config, | ||
| file.sourceIntegration | ||
| ); | ||
|
|
||
| if (!integrationConfig) { | ||
| log( | ||
| config, | ||
| "warning", | ||
| `No integration config found for: ${file.sourceIntegration.integrationName}` | ||
| ); | ||
| results.skipped++; | ||
| results.details.push({ | ||
| localPath: file.localPath, | ||
| status: "SKIPPED", | ||
| reason: `No integration config found for: ${file.sourceIntegration.integrationName}`, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| log( | ||
| config, | ||
| "info", | ||
| `Uploading ${file.localPath} to ${file.sourceIntegration.type}...` | ||
| ); | ||
|
|
||
| const uploadResult = await uploader.upload({ | ||
| config, | ||
| integrationConfig, | ||
| localFilePath: file.localPath, | ||
| sourceIntegration: file.sourceIntegration, | ||
| log, | ||
| }); | ||
|
|
||
| if (uploadResult.status === "PASS") { | ||
| results.successful++; | ||
| log(config, "info", `Successfully uploaded: ${file.localPath}`); | ||
| } else { | ||
| results.failed++; | ||
| log( | ||
| config, | ||
| "warning", | ||
| `Failed to upload ${file.localPath}: ${uploadResult.description}` | ||
| ); | ||
| } | ||
|
|
||
| results.details.push({ | ||
| localPath: file.localPath, | ||
| status: uploadResult.status, | ||
| description: uploadResult.description, | ||
| }); | ||
| } catch (error) { | ||
| results.failed++; | ||
| log( | ||
| config, | ||
| "warning", | ||
| `Error uploading ${file.localPath}: ${error.message}` | ||
| ); | ||
| results.details.push({ | ||
| localPath: file.localPath, | ||
| status: "FAIL", | ||
| description: error.message, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The uploads are performed sequentially in a for loop. For better performance when uploading multiple files, consider using Promise.all() or Promise.allSettled() to upload files in parallel. This would significantly reduce the total upload time when there are multiple changed files.
src/tests.js
Outdated
|
|
||
| // Upload changed files back to source integrations (best-effort) | ||
| // This automatically syncs any changed screenshots back to their source CMS | ||
| if (config?.integrations?.heretto?.length > 0) { |
There was a problem hiding this comment.
The upload functionality is automatically triggered for all tests when any Heretto integration is configured, but there is no configuration option to control this behavior. Consider checking the uploadOnChange flag mentioned in dev/index.js to allow users to opt-in or opt-out of automatic uploads.
| if (config?.integrations?.heretto?.length > 0) { | |
| const uploadOnChange = config?.uploadOnChange ?? true; | |
| if (uploadOnChange && config?.integrations?.heretto?.length > 0) { |
src/integrations/heretto.js
Outdated
| if (!resourceDependencies) return null; | ||
|
|
||
| // Normalize the file path for comparison | ||
| const normalizedPath = filePath.replace(/\\/g, "/").replace(/^\.\//, "").replace(/^\.\.\//, ""); |
There was a problem hiding this comment.
The path normalization only removes leading './' and '../' once, but paths could have multiple levels of relative references like '../../folder/file.png'. Consider using a more robust path normalization method or a loop to handle all relative path segments.
| const normalizedPath = filePath.replace(/\\/g, "/").replace(/^\.\//, "").replace(/^\.\.\//, ""); | |
| const normalizedPath = path.posix | |
| .normalize(filePath.replace(/\\/g, "/")) | |
| .replace(/^(\.\/|(\.\.\/))+/g, ""); |
src/integrations/heretto.js
Outdated
| if (!resourceDependencies) return result; | ||
|
|
||
| // Normalize path and get parent directory | ||
| const normalizedPath = filePath.replace(/\\/g, "/").replace(/^\.\//, "").replace(/^\.\.\//, ""); |
There was a problem hiding this comment.
The path normalization only removes leading './' and '../' once, but paths could have multiple levels of relative references like '../../folder/file.png'. Consider using a more robust path normalization method or a loop to handle all relative path segments.
| const normalizedPath = filePath.replace(/\\/g, "/").replace(/^\.\//, "").replace(/^\.\.\//, ""); | |
| const normalizedPath = path.posix.normalize(filePath.replace(/\\/g, "/")); |
- Remove hardcoded input path in main function for flexibility. - Update file path normalization to use posix for cross-platform support. - Expand documentation for resolveFromDependencies and findParentFolderFromDependencies methods. - Add additional properties to collectChangedFiles return type for better clarity. - Implement parallel uploads in uploadChangedFiles for improved performance. - Modify screenshot tests to ensure consistent port usage and cleanup of temporary files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/integrations/heretto.js (1)
312-314: Consider simplifying the path normalization regex.The current regex
.replace(/^(\.\/)|(\.\.\/)*/g, "")works but has a subtle issue:(\.\.\/)*can match an empty string, causing unnecessary empty matches. A cleaner pattern would be:🔎 Suggested improvement
const normalizedPath = path.posix .normalize(filePath.replace(/\\/g, "/")) - .replace(/^(\.\/)|(\.\.\/)*/g, ""); + .replace(/^(\.\.?\/)+/g, "");This matches one or more occurrences of either
./or../at the start, avoiding empty matches.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dev/index.jssrc/integrations/heretto.jssrc/integrations/index.jssrc/tests.jstest/screenshot.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- test/screenshot.test.js
- src/tests.js
- dev/index.js
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Uselog(config, level, message)for all logging, where level = "debug"|"info"|"warning"|"error"
UsegetAvailableApps()to detect installed browsers instead of hardcoding browser paths
Files:
src/integrations/heretto.jssrc/integrations/index.js
🧠 Learnings (1)
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Step handlers must return object with `{ status: "PASS"|"FAIL"|"WARNING", description: string, outputs: {} }`
Applied to files:
src/integrations/index.js
🧬 Code graph analysis (2)
src/integrations/heretto.js (1)
test/heretto-upload.test.js (4)
path(2-2)content(215-221)content(263-269)content(423-429)
src/integrations/index.js (2)
src/integrations/heretto.js (1)
require(9-9)test/upload.test.js (13)
sourceIntegration(255-255)sourceIntegration(268-268)sourceIntegration(276-276)report(26-57)report(71-100)report(107-132)report(139-167)report(174-224)config(236-236)config(242-242)config(254-254)config(263-267)config(275-275)
⏰ 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). (9)
- GitHub Check: test (macos-latest, 24)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (macos-latest, 22)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (windows-latest, 24)
- GitHub Check: test (ubuntu-latest, 22)
- GitHub Check: test (ubuntu-latest, 24)
- GitHub Check: test (windows-latest, 22)
- GitHub Check: test (ubuntu-latest, 20)
🔇 Additional comments (16)
src/integrations/heretto.js (11)
1-23: LGTM!The class declaration and
canHandlemethod are well-structured. The optional chaining onsourceIntegration?.typeprovides safe null handling.
34-228: Well-structured upload orchestration with comprehensive error handling.The multi-step resolution strategy (dependencies → ditamap parent → folder search → global search) provides good fallback coverage. The logging wrapper pattern at lines 66-67 correctly adapts the internal
log(level, msg)calls to the requiredlog(config, level, msg)format per coding guidelines.
239-291: LGTM!The three-tier resolution strategy (exact path → filename+folder → filename-only) provides good fallback behavior. The path normalization using
path.posix.normalize()correctly handles cross-platform path differences.
359-424: LGTM!Timeout handling is properly implemented. The regex escape pattern at line 391 is correct—in a JavaScript regex literal,
[\]\\]correctly matches either]or\characters. The graceful degradation viaresolve(null)on errors is appropriate for fallback-based resolution.
431-499: LGTM!The document creation handles edge cases well, including the "already exists" scenario. The XML escaping via
escapeXml()prevents injection, and timeout handling is properly placed.
506-574: LGTM!The dual regex approach at lines 540-541 handles XML attribute ordering flexibility well. Timeout and error handling follow the established pattern.
581-588: LGTM!The XML escaping correctly handles all five predefined XML entities in the proper order (ampersand first to prevent double-escaping).
648-651: Verify fallback behavior for non-exact folder matches.When an exact folder name match isn't found, the code falls back to the first search result. This could potentially target an incorrect folder if the search returns partial matches.
Consider whether logging a warning here would help debugging when uploads go to unexpected locations.
770-820: LGTM!The upload implementation correctly handles binary content via Buffer and includes proper timeout handling.
827-960: LGTM!The XML parsing approach at lines 862-877 is reasonable for a known schema. Binary content handling in
getDocumentContentcorrectly usesBuffer.concat(chunks).
967-986: LGTM!The content type mapping covers the common formats for screenshots and DITA content. The
application/octet-streamfallback is appropriate for unknown types.src/integrations/index.js (5)
1-32: LGTM!The registry pattern with
getUploaderprovides a clean, extensible mechanism for adding new integration uploaders. The null safety with optional chaining is appropriate.
34-73: LGTM!The report traversal correctly extracts changed files with source integrations. The JSDoc at lines 37-43 properly documents the return structure.
95-240: LGTM!The parallel upload implementation using
Promise.allSettledis well-designed. The internal error catching at lines 185-188 preserves file references for proper result aggregation. Logging follows the requiredlog(config, level, message)format per coding guidelines.
242-261: LGTM!The nullish coalescing operator at line 257 ensures consistent
nullreturns when no config is found, addressing the previous review feedback.
263-280: LGTM!The interface validation at lines 268-270 ensures registered uploaders conform to the expected contract before being added to the registry.
- Normalize file paths to handle multiple levels of relative references. - Update documentation for resource dependencies and upload functions. - Check both global and per-integration uploadOnChange settings for better control.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/tests.js (1)
801-824: Implementation addresses the uploadOnChange control requirement.The logic correctly checks both global
config.uploadOnChangeand per-integrationuploadOnChangesettings, with sensible defaults (true for backward compatibility). Error handling with try/catch and warning logging is appropriate.Minor: The condition on line 810 has a redundant check—
herettoConfigs.length > 0is implied whenhasUploadEnabledIntegrationis true (since.some()returns false for empty arrays).🔎 Optional simplification
- if (globalUploadOnChange && hasUploadEnabledIntegration && herettoConfigs.length > 0) { + if (globalUploadOnChange && hasUploadEnabledIntegration) {src/integrations/index.js (1)
159-226: Parallel upload execution with Promise.allSettled looks good.The approach of catching errors within each promise to preserve the file reference is smart. The comment on line 189 correctly explains why all promises should be fulfilled.
One minor defensive consideration: while the current implementation ensures all promises resolve (not reject), directly accessing
settled.valuewithout checkingsettled.status === 'fulfilled'could be fragile if future refactoring changes the error-handling pattern.🔎 Optional defensive check
for (const settled of settledResults) { - // All promises should be fulfilled since we catch errors internally - const { file, uploadResult, error } = settled.value; + // All promises should be fulfilled since we catch errors internally + if (settled.status !== 'fulfilled') continue; // Defensive guard + const { file, uploadResult, error } = settled.value;src/integrations/heretto.js (1)
608-691: Minor concern: Fallback to first search result may return incorrect folder.Lines 662-664 fall back to the first search result when no exact name match is found. This could silently select the wrong folder if the search returns multiple results with similar names.
Consider either:
- Returning
nullinstead of the first result to force more explicit resolution- Adding a warning log when using the fallback
The current approach is acceptable since it's logged at debug level and the overall upload will still be tracked.
🔎 Optional: More explicit fallback handling
if (match) { log("debug", `Found folder: ${folderName} with ID: ${match.uuid || match.id}`); resolve(match.uuid || match.id); } else { - // Take first result as fallback - log("debug", `Exact folder match not found, using first result: ${result.searchResults[0].uuid || result.searchResults[0].id}`); - resolve(result.searchResults[0].uuid || result.searchResults[0].id); + // Take first result as fallback (may not be the intended folder) + log("warning", `Exact folder match not found for '${folderName}', using first result: ${result.searchResults[0].uuid || result.searchResults[0].id}`); + resolve(result.searchResults[0].uuid || result.searchResults[0].id); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/integrations/heretto.jssrc/integrations/index.jssrc/tests.js
🧰 Additional context used
📓 Path-based instructions (2)
src/tests.js
📄 CodeRabbit inference engine (AGENTS.md)
src/tests.js: Add new driver-based step actions to thedriverActionsarray insrc/tests.jsand implement case inrunStep()switch statement
UsestepExecutionFailedflag to skip remaining steps in a context after first failure
Set Appium driver timeout to 10 minutes (newCommandTimeout: 600) for all driver-based steps
runSpecs()insrc/tests.jsorchestrates test execution flow: spec → test → context → step hierarchy
Files:
src/tests.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Uselog(config, level, message)for all logging, where level = "debug"|"info"|"warning"|"error"
UsegetAvailableApps()to detect installed browsers instead of hardcoding browser paths
Files:
src/tests.jssrc/integrations/heretto.jssrc/integrations/index.js
🧠 Learnings (10)
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to test/**/*.js : Use `fs.writeFileSync()` + `fs.unlinkSync()` in try/finally blocks for temporary test files to ensure cleanup
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Create new step type handlers as async functions exported from `src/tests/[actionName].js`
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : Add new driver-based step actions to the `driverActions` array in `src/tests.js` and implement case in `runStep()` switch statement
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests.js : `runSpecs()` in `src/tests.js` orchestrates test execution flow: spec → test → context → step hierarchy
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: `src/tests.js` is the core orchestrator handling 600+ LOC of Appium/WebDriver management
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/index.js : `src/index.js` must export `runTests()` as the main entry point for the package
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Individual step types have dedicated handler implementations in `src/tests/` (e.g., `httpRequest.js`, `runShell.js`)
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Tests follow nested hierarchy: spec (file) → test → context (browser/platform combo) → step (action), with contexts running serially and steps skipping after first failure
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/config.js : Browsers and drivers must match the current platform (detected via `puppeteer/browsers`) with Appium drivers installed for chromium, gecko, and safari
Applied to files:
src/tests.js
📚 Learning: 2025-12-03T00:21:10.834Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T00:21:10.834Z
Learning: Applies to src/tests/*.js : Step handlers must return object with `{ status: "PASS"|"FAIL"|"WARNING", description: string, outputs: {} }`
Applied to files:
src/integrations/index.js
🧬 Code graph analysis (1)
src/integrations/index.js (2)
src/integrations/heretto.js (1)
require(9-9)src/tests.js (18)
require(4-4)require(6-6)require(7-7)require(8-8)require(9-9)require(10-10)require(11-11)require(12-12)require(13-13)require(14-14)require(15-15)require(16-16)require(17-17)require(18-18)require(19-19)report(383-411)config(368-368)config(1001-1001)
⏰ 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). (9)
- GitHub Check: test (macos-latest, 24)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (macos-latest, 22)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (ubuntu-latest, 22)
- GitHub Check: test (ubuntu-latest, 24)
- GitHub Check: test (windows-latest, 24)
- GitHub Check: test (windows-latest, 22)
- GitHub Check: test (ubuntu-latest, 20)
🔇 Additional comments (18)
src/tests.js (1)
29-29: LGTM!Import added correctly for the new integrations module.
src/integrations/index.js (4)
10-32: LGTM!The uploader registry pattern is well-implemented with a clean iteration approach. Good early return when
sourceIntegration?.typeis missing.
44-73: LGTM!The traversal logic correctly navigates the nested report structure and collects changed files with all necessary metadata. Good use of null-safe iteration with
|| [].
243-256: LGTM!Consistent
nullreturn across all code paths (past review addressed with?? nullon line 252). The switch statement is well-structured for future integration type expansion.
262-267: LGTM!Good runtime validation ensuring uploaders implement the required interface before registration.
src/integrations/heretto.js (13)
14-49: LGTM!Clean interface implementation with good early validation of required configuration. Descriptive error messages will help users troubleshoot configuration issues.
78-198: Comprehensive file resolution with multiple fallback strategies.The multi-step resolution approach (dependencies → folder API → global search) is well-designed for robustness. The race condition handling at lines 157-172 is a good defensive measure.
The complexity is justified given the various ways files might need to be located in the CMS.
239-295: LGTM!The path normalization using
path.posix.normalizecombined with regex cleanup handles multiple levels of relative references correctly. The three-tier matching strategy (exact → folder+filename → filename-only) provides good flexibility for resolving files.
297-363: LGTM!Well-documented function with consistent path normalization. The sibling-file lookup strategy is a clever way to find parent folder IDs when the exact file isn't in dependencies.
370-436: LGTM!Good implementation with:
- Proper regex escaping for special characters in folder names (line 403)
- 30-second timeout handling (lines 424-427, addresses past review)
- Appropriate error handling that resolves
nullrather than rejecting
443-511: LGTM!Well-structured document creation with:
- Proper XML escaping for filenames
- Graceful handling of "already exists" conflicts
- Timeout handling in place
The
req.setTimeout+req.on("error")pattern correctly handles timeouts.
518-587: LGTM!Good defensive approach with two regex patterns to handle different XML attribute orderings. Consistent timeout and error handling.
594-601: LGTM!Standard and complete XML entity escaping.
698-776: LGTM!Consistent implementation with
searchFolderByName. Since this is used as a "last resort" global search (line 179), the fallback to first result is acceptable here.
783-833: LGTM!Clean upload implementation with proper
Content-Lengthheader for binary content and consistent timeout handling.
840-920: LGTM!Good handling of the XML response structure, correctly distinguishing between attributes and child elements. Including
rawXmlin the response is helpful for debugging.
927-973: LGTM!Correct binary content handling using
Buffer.concatfor chunks. Consistent timeout and error handling pattern.
980-999: LGTM!Good coverage of common file types with a safe fallback. The inclusion of
.ditaand.ditamapextensions is appropriate for the Heretto CMS use case.
|
📝 Documentation updates detected! Updated existing suggestion: Document Heretto CMS integration for DITA content testing |
Introduce a HerettoUploader class to automate the upload of changed screenshots to Heretto CMS. Enhance the screenshot saving logic to track changes and integrate metadata. Add documentation and improve error handling for upload operations. Update dependencies and implement integration tests for the new functionality.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.