Skip to content

Enhance error handling and update JSON writing logic - #767

Open
truekasun wants to merge 1 commit into
TryGhost:mainfrom
truekasun:main
Open

Enhance error handling and update JSON writing logic#767
truekasun wants to merge 1 commit into
TryGhost:mainfrom
truekasun:main

Conversation

@truekasun

Copy link
Copy Markdown

This pull request improves error handling and file path correctness in the asset fetching workflow, and adds comprehensive tests to ensure the new behaviors. The main changes include updating how errors are reported, ensuring JSON output is written to a file (not a directory), and adding tests to prevent regressions.

Error handling improvements:

  • Updated error reporting in commands/fetch-assets.js to fall back to the thrown error if context.errors is empty, ensuring more informative error messages.
  • Changed the zip file reporting logic to only log the zip file path if it was actually created.

File path handling:

  • Modified the JSON export in tasks/fetch-assets.js to write to a file within the jsonDir directory (ghost-import.json) instead of the directory itself, preventing file system errors.
  • Added the necessary join import from node:path to support correct file path construction.

Testing improvements:

  • Added new tests in test/fetch-assets.test.js to verify that the JSON file is written to the correct path and to ensure that no zip file is reported if an earlier task fails.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The fetch-assets task now writes updated scraper JSON to ghost-import.json within the configured JSON directory. Command error handling falls back to the thrown error, returns immediately after logging, and requires an output file before zip handling. New tests verify the JSON file path and confirm failed fetches do not invoke zip creation.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: error handling improvements and JSON output path updates.
Description check ✅ Passed The description clearly matches the changeset, covering error handling, JSON path fixes, and added tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/fetch-assets.test.js (1)

69-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the error-reporting fallback itself.

This test proves only that execution does not reject and zip writing is skipped. It would still pass if the catch block logged an empty or incorrect error. Assert that ui.log.error receives the thrown InternalServerError when context.errors is empty.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/fetch-assets.test.js` around lines 69 - 80, Update the test around
command.default.run to assert the error-reporting fallback: when
mockWriteUpdatedJson rejects with the InternalServerError and context.errors is
empty, verify that ui.log.error receives that exact thrown error. Keep the
existing non-rejection and mockZipWrite call-count assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/fetch-assets.test.js`:
- Around line 69-80: Update the test around command.default.run to assert the
error-reporting fallback: when mockWriteUpdatedJson rejects with the
InternalServerError and context.errors is empty, verify that ui.log.error
receives that exact thrown error. Keep the existing non-rejection and
mockZipWrite call-count assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 705d4de6-007f-4bf5-952d-72b836eb4731

📥 Commits

Reviewing files that changed from the base of the PR and between 7177c1a and 8abc83b.

📒 Files selected for processing (3)
  • commands/fetch-assets.js
  • tasks/fetch-assets.js
  • test/fetch-assets.test.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant