Skip to content

Conversation

@rmgpinto
Copy link
Contributor

ref https://linear.app/ghost/issue/BER-3132

  • The require('@tryghost/errors') adds 50-100ms on boot time, this is due to the package's own initialization and require(). Since it's not needed at boot, it can be moved inside the beforeSend function.

@cursor
Copy link

cursor bot commented Dec 17, 2025

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on January 3.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 17, 2025

Walkthrough

The top-level require('@tryghost/errors') in ghost/core/core/shared/sentry.js was moved into the beforeSend function and wrapped with a private _errors cache so the module is lazy-loaded per call (const errors = _errors || (_errors = require('@tryghost/errors'))). No other functional changes to control flow, exported declarations, or error-handling logic were made.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving require() inside beforeSend to improve boot performance. It directly addresses the primary optimization in the changeset.
Description check ✅ Passed The description is related to the changeset, explaining the performance motivation and implementation approach. It references the issue and provides context for why the change was made.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56e1a3e and ea35d84.

📒 Files selected for processing (1)
  • ghost/core/core/shared/sentry.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.428Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx:29-34
Timestamp: 2025-03-13T09:02:50.102Z
Learning: In the Ghost ActivityPub module, error handling for mutations is handled at the hook level (in use-activity-pub-queries.ts) rather than in individual components. This allows for centralized error handling across the application.
📚 Learning: 2025-08-11T19:39:00.428Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.428Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.

Applied to files:

  • ghost/core/core/shared/sentry.js
🔇 Additional comments (2)
ghost/core/core/shared/sentry.js (2)

4-4: LGTM! Cache variable properly scoped for lazy loading.

The module-level _errors cache enables memoization across multiple beforeSend invocations, ensuring the module is loaded only once on first use rather than at boot time.


13-13: Excellent optimization! Successfully defers boot-time dependency.

The lazy-loading pattern correctly achieves the PR's objective of eliminating the 50-100ms boot-time cost. The memoization ensures the module is loaded exactly once when first needed, and the existing try-catch wrapper (lines 67-71) provides graceful fallback if the deferred require fails.


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 and usage tips.

@rmgpinto rmgpinto force-pushed the shift-require-sentry branch 2 times, most recently from 8e6135d to 286b803 Compare December 17, 2025 16:43
ref https://linear.app/ghost/issue/BER-3132

- The `require('@tryghost/errors')` adds 50-100ms on boot time, this is
due to the package's own initialization and `require()`. Since it's not
needed at boot, it can be moved inside the `beforeSend` function.
@rmgpinto rmgpinto force-pushed the shift-require-sentry branch from 286b803 to ea35d84 Compare December 17, 2025 16:48
@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jan 6, 2026

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kevinansfield
Copy link
Member

Approved 👍 Out of interest, is there anything we can do in @tryghost/errors to improve that packages performance across the board?

@rmgpinto rmgpinto merged commit 1b1fac0 into main Jan 8, 2026
60 of 66 checks passed
@rmgpinto rmgpinto deleted the shift-require-sentry branch January 8, 2026 09:47
@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jan 8, 2026

IIRC, Claude mentioned: https://github.com/TryGhost/framework/blob/main/packages/errors/src/utils.ts#L1 as the culprit.

@kevinansfield
Copy link
Member

In that case, would it be preferable to do the same delayed require in that package so we only pay the cost on the first stack trace generation?

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jan 8, 2026

I'd say so. I wanted to address the boot time on Ghost, but probably makes sense to get the package changed with what you suggested.

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.

3 participants