Conversation
WalkthroughThe changes remove the use of the Possibly related PRs
Suggested labels
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 4ffb70c69d2ce5a20f4038c3a593df378d1a35c9 and 1c9de2f. ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (21)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (13)
🧰 Additional context used🧬 Code Definitions (1)ghost/core/test/utils/e2e-framework.js (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ghost/core/core/boot.js (1)
85-85: Removedfrontendparameter frominitCoresignature.This simplifies the function’s interface and avoids confusion about the service initialization paths. Ensure that other modules referencing
initCorewith afrontendargument are updated.ghost/core/test/utils/e2e-framework.js (3)
81-81: Defaultingfrontendtotrue.Enabling the frontend by default can lead to slightly slower tests if the frontend isn’t needed. Consider selectively disabling it for tests that only need the backend to optimize runtime.
246-246:getMembersAPIAgentalso callsstartGhost()unconditionally.Unifying agent creation reduces duplication. Ensure coverage or add tests that verify if a backend-only startup still works if needed.
312-312:getAgentsForMembersno longer composesbootOptions.Similar to other agents, removing
bootOptionscentralizes the logic instartGhost. Fine for maintenance; watch out for any test requiring custom options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between bbe6128 and 55e95f03839324922fa05ad938d70a165e5da2a3.
⛔ Files ignored due to path filters (7)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/pages.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/integration/services/email-service/__snapshots__/cards.test.js.snapis excluded by!**/*.snapghost/core/test/regression/api/admin/__snapshots__/authentication.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
ghost/core/core/boot.js(3 hunks)ghost/core/core/server/services/url/LocalFileCache.js(0 hunks)ghost/core/core/server/services/url/UrlService.js(2 hunks)ghost/core/core/server/services/url/index.js(1 hunks)ghost/core/core/shared/config/env/config.testing-browser.json(1 hunks)ghost/core/core/shared/config/env/config.testing-mysql.json(1 hunks)ghost/core/core/shared/config/env/config.testing.json(1 hunks)ghost/core/core/shared/labs.js(0 hunks)ghost/core/test/e2e-api/admin/sso.test.js(1 hunks)ghost/core/test/e2e-api/admin/utils.js(1 hunks)ghost/core/test/e2e-api/content/utils.js(1 hunks)ghost/core/test/regression/api/admin/utils.js(1 hunks)ghost/core/test/regression/api/content/utils.js(1 hunks)ghost/core/test/unit/server/services/url/LocalFileCache.test.js(0 hunks)ghost/core/test/unit/shared/config/loader.test.js(0 hunks)ghost/core/test/unit/shared/labs.test.js(2 hunks)ghost/core/test/utils/e2e-framework.js(6 hunks)ghost/core/test/utils/e2e-utils.js(1 hunks)ghost/core/test/utils/fixtures/urls/resources.json(0 hunks)ghost/core/test/utils/fixtures/urls/urls.json(0 hunks)ghost/core/test/utils/url-service-utils.js(1 hunks)
💤 Files with no reviewable changes (6)
- ghost/core/core/shared/labs.js
- ghost/core/core/server/services/url/LocalFileCache.js
- ghost/core/test/utils/fixtures/urls/urls.json
- ghost/core/test/unit/server/services/url/LocalFileCache.test.js
- ghost/core/test/utils/fixtures/urls/resources.json
- ghost/core/test/unit/shared/config/loader.test.js
🔇 Additional comments (24)
ghost/core/core/shared/config/env/config.testing.json (1)
78-79: Removal of urlCache path is appropriate for the cache feature deprecationThe removal of the urlCache path from the configuration aligns with the PR objective of completely removing the experimental URL cache feature that was never fully developed and has been causing bugs.
ghost/core/core/shared/config/env/config.testing-mysql.json (1)
82-83: Removal of urlCache path is consistent with overall PR objectiveThe removal of the urlCache configuration entry in this MySQL-specific testing config is consistent with the complete removal of the experimental URL cache feature across the codebase.
ghost/core/core/shared/config/env/config.testing-browser.json (1)
81-82: Consistent removal of urlCache configurationThis change aligns with the other configuration files, ensuring the urlCache feature is consistently removed across all testing environments.
ghost/core/test/e2e-api/admin/sso.test.js (1)
12-13: Explicit model initialization addresses test dependency issuesAdding the explicit model initialization is a good fix to ensure tests don't rely on the now-removed URL cache. According to the PR objectives, tests were unexpectedly relying on the URL cache, and this change helps decouple the tests from that dependency.
ghost/core/test/unit/shared/labs.test.js (5)
36-37: Feature flag name replaced as part of URL cache removalThe
urlCachefeature flag has been replaced withNestPlaygroundin the test setup. This is consistent with the PR objective of removing the experimental URL cache feature.
42-43: Feature flag name updated in expected output checkThe test now checks for
NestPlaygroundinstead ofurlCachein the expected output, which maintains the test coverage while removing references to the URL cache.
47-47: isSet check updated to use new feature flag nameThe
labs.isSet()check is now using the newNestPlaygroundfeature flag name instead ofurlCache, which is consistent with the overall changes in this PR.
55-56: Feature flag name replaced in another test caseThe
urlCachefeature flag has been consistently replaced withNestPlaygroundin this test case as well, ensuring all tests are updated accordingly.
65-65: Negative check for feature flag updatedThe negative check for the feature flag has been properly updated to use
NestPlaygroundinstead ofurlCache, completing the rename across all test cases.ghost/core/core/server/services/url/index.js (1)
3-3: URL service initialization simplified by removing cache parameterThe
UrlServiceis now instantiated without a cache parameter, which aligns with the PR's objective of removing the experimental URL cache feature. This simplifies the URL service initialization and maintenance.ghost/core/test/e2e-api/content/utils.js (1)
100-100:Details
✅ Verification successful
Removed default frontend/backend configuration
The default configuration that set
backend: trueandfrontend: falsehas been removed, and the method now directly passes theoverridesparameter totestUtils.startGhost(). This change is part of removing the URL cache functionality, as the frontend parameter was previously used for cache initialization.Let's verify the default values in the underlying
startGhostmethod to ensure this change doesn't cause unexpected behavior:
🏁 Script executed:
#!/bin/bash # Check the default values in the startGhost method # Look for how startGhost handles its options rg -A 15 "startGhost = async \(options\)" ghost/core/test/utils/Length of output: 1117
Clarification on Default Configuration in
startGhostAfter verifying the underlying defaults in
ghost/core/test/utils/e2e-utils.js, it appears that thestartGhostmethod now merges options with defaults of{ backend: true, frontend: true, ... }. The removal of the explicit configuration inghost/core/test/e2e-api/content/utils.js(i.e. directly passingoverridestotestUtils.startGhost()) is intentional and relies on these defaults.
- The previous inline defaults (which reportedly set
frontend: false) have been replaced. The current implementation defaults tofrontend: true, as defined ine2e-utils.js.- This change supports the removal of the URL cache functionality since frontend initialization is now handled via the merged defaults in the underlying method.
The code change seems safe as long as the tests are expected to run with both backend and frontend enabled by default.
ghost/core/test/utils/e2e-utils.js (1)
136-136:Details
✅ Verification successful
Removed URL cache configuration from URL service initialization
The
urlServiceUtils.init()call no longer includes theurlCacheparameter, which was previously set based on the frontend state. This is consistent with the PR's goal of completely removing the URL cache functionality, simplifying the service initialization.Let's check how the
urlServiceUtils.init()method has been updated to handle the absence of this parameter:
🏁 Script executed:
#!/bin/bash # Check the implementation of urlServiceUtils.init # Look for the init method definition rg -A 10 "init\(\s*\{?.*?\}?\s*\)" ghost/core/test/utils/url-service-utils.jsLength of output: 281
URL Service Initialization Cleanup Verified
The removal of the URL cache parameter is correctly implemented. The verification confirms that the
urlServiceUtils.init()call in both the e2e utilities and the URL service utilities now simply callsurlService.init();without the previously passedurlCacheparameter. This is in line with the PR's goal of eliminating the URL cache functionality.
- Confirmed in
ghost/core/test/utils/url-service-utils.jsthat the method now callsurlService.init();without additional parameters.- The change in
ghost/core/test/utils/e2e-utils.jsat line 136 is consistent with this update.ghost/core/test/e2e-api/admin/utils.js (1)
245-246: Default parameters removed in line with URL cache removalThis change removes the default parameters that were previously setting
backend: trueandfrontend: false, now directly passing the overrides totestUtils.startGhost. This simplification aligns with the PR objective to remove the experimental URL cache feature.ghost/core/test/regression/api/admin/utils.js (1)
209-210: Default parameters removed to simplify APISimilar to the other test utility files, this change removes the default parameters for
backendandfrontendthat were previously merged with overrides. The simplification is consistent with the PR's goal of removing the URL cache functionality.ghost/core/test/utils/url-service-utils.js (1)
19-21: URL cache parameter removed from init methodThe
initmethod has been modified to no longer accept or pass theurlCacheparameter to the URL service. This change directly implements the PR objective of removing the experimental URL cache functionality that was causing issues.ghost/core/test/regression/api/content/utils.js (1)
105-106: Consistent removal of default parametersThis change maintains consistency with other test utility files by removing the default parameters for
backendandfrontend. This simplification is part of the systematic removal of the URL cache feature across the codebase.ghost/core/core/boot.js (2)
117-117: UnconditionalurlService.init()call looks valid.Since the caching logic is removed, directly calling
urlService.init()streamlines boot and ensures the URL service is always ready. No issues here.
560-560: UpdatedinitCorecall withoutfrontend.Confirm that all references are aligned and no tests rely on the previous signature. Otherwise, this change is consistent with the removal of caching.
ghost/core/test/utils/e2e-framework.js (4)
222-222: Param signature simplified ingetAdminAPIAgent.The function no longer accepts additional parameters, relying solely on the default boot configuration. This is fine, but confirm that tests which relied on custom boot options are updated.
224-224: InvokingstartGhost()without manual overrides.This is consistent with enabling the frontend by default. Verify that any specialized test scenarios still pass with the new defaults.
268-268:getWebmentionsAPIAgentstreamlined similarly.No further concerns; consistent with the pattern established in other agents.
290-290:getGhostAPIAgentalso simplified.Approach parallels the other agent creation methods. No issues found.
ghost/core/core/server/services/url/UrlService.js (2)
29-29: Removedoptionsparameter from the constructor.Dropping the cache parameter clarifies the constructor. Ensure no leftover references to the old
options.cacheexist.
310-319: Simplifiedinitmethod by removing caching logic.
- Initializes event listeners and fetches resources directly.
- Immediately starts the queue without conditional checks for caching.
This reduces complexity and potential for out-of-sync caches.
daniellockyer
force-pushed
the
remove-url-cache
branch
from
55e95f0 to
55d23a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/services/url/UrlService.js (1)
320-321: Empty shutdown methodThe
shutdownmethod has been emptied, likely because it previously contained cache-related cleanup logic that is no longer needed.However, consider whether this method is still needed at all. If it no longer serves any purpose, it might be cleaner to remove it entirely.
-async shutdown() { -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 55e95f03839324922fa05ad938d70a165e5da2a3 and 55d23a9.
⛔ Files ignored due to path filters (7)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/pages.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/integration/services/email-service/__snapshots__/cards.test.js.snapis excluded by!**/*.snapghost/core/test/regression/api/admin/__snapshots__/authentication.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
ghost/core/core/boot.js(3 hunks)ghost/core/core/server/services/url/LocalFileCache.js(0 hunks)ghost/core/core/server/services/url/UrlService.js(2 hunks)ghost/core/core/server/services/url/index.js(1 hunks)ghost/core/core/shared/config/env/config.testing-browser.json(1 hunks)ghost/core/core/shared/config/env/config.testing-mysql.json(1 hunks)ghost/core/core/shared/config/env/config.testing.json(1 hunks)ghost/core/core/shared/labs.js(0 hunks)ghost/core/test/e2e-api/admin/sso.test.js(1 hunks)ghost/core/test/e2e-api/admin/utils.js(1 hunks)ghost/core/test/e2e-api/content/utils.js(1 hunks)ghost/core/test/regression/api/admin/utils.js(1 hunks)ghost/core/test/regression/api/content/utils.js(1 hunks)ghost/core/test/unit/server/services/url/LocalFileCache.test.js(0 hunks)ghost/core/test/unit/shared/config/loader.test.js(0 hunks)ghost/core/test/unit/shared/labs.test.js(2 hunks)ghost/core/test/utils/e2e-framework.js(6 hunks)ghost/core/test/utils/e2e-utils.js(1 hunks)ghost/core/test/utils/fixtures/urls/resources.json(0 hunks)ghost/core/test/utils/fixtures/urls/urls.json(0 hunks)ghost/core/test/utils/url-service-utils.js(1 hunks)
💤 Files with no reviewable changes (6)
- ghost/core/test/unit/shared/config/loader.test.js
- ghost/core/core/shared/labs.js
- ghost/core/test/utils/fixtures/urls/resources.json
- ghost/core/core/server/services/url/LocalFileCache.js
- ghost/core/test/unit/server/services/url/LocalFileCache.test.js
- ghost/core/test/utils/fixtures/urls/urls.json
🚧 Files skipped from review as they are similar to previous changes (14)
- ghost/core/core/shared/config/env/config.testing-mysql.json
- ghost/core/test/e2e-api/admin/sso.test.js
- ghost/core/core/shared/config/env/config.testing.json
- ghost/core/core/shared/config/env/config.testing-browser.json
- ghost/core/test/regression/api/admin/utils.js
- ghost/core/test/utils/e2e-utils.js
- ghost/core/test/unit/shared/labs.test.js
- ghost/core/test/regression/api/content/utils.js
- ghost/core/test/e2e-api/admin/utils.js
- ghost/core/test/utils/url-service-utils.js
- ghost/core/test/e2e-api/content/utils.js
- ghost/core/core/boot.js
- ghost/core/core/server/services/url/index.js
- ghost/core/test/utils/e2e-framework.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (2)
ghost/core/core/server/services/url/UrlService.js (2)
28-28: Constructor signature simplified by removing cache parameterThe constructor has been simplified by removing the
cacheparameter, which aligns with the PR objective of removing the experimental URL cache. This change makes the service's initialization more straightforward.
309-318: Init method streamlined to remove cache-related logicThe
initmethod has been significantly simplified by:
- Removing the
urlCacheparameter- Directly initializing event listeners and fetching resources
- Starting the queue unconditionally
This change eliminates potential issues related to stale cache data and makes the initialization process more predictable.
daniellockyer
force-pushed
the
remove-url-cache
branch
2 times, most recently
from
7b2a417 to
fbc44c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/core/core/server/services/url/UrlService.js (3)
310-311: Consider removing empty shutdown methodThe
shutdownmethod has been emptied of all cache-related logic but remains in the codebase. Since it no longer performs any operations, consider removing it entirely to improve code cleanliness.-async shutdown() { -}
13-17: Update class documentationThe class documentation should be updated to reflect that the URL service no longer uses a cache, as this was a significant feature that has now been removed.
/** * The url service class holds all instances in a centralized place. * * It's the public API you can talk to. * * It will tell you if the url generation is in progress or not. + * + * Note: URL caching functionality has been removed. */
21-27: Update constructor JSDoc commentThe JSDoc comment for the constructor still references the removed
cacheparameter. Update it to reflect the current parameter list (which is now empty)./** * - * @param {Object} [options] - * @param {Object} [options.cache] - cache handler instance - * @param {Function} [options.cache.read] - read cache by type - * @param {Function} [options.cache.write] - write into cache by type */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7b2a41788d206b1df622c74d9ea14736d5b5800a and fbc44c687c58964e1ed9809e688fe26addb301a1.
⛔ Files ignored due to path filters (7)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/pages.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snapis excluded by!**/*.snapghost/core/test/integration/services/email-service/__snapshots__/cards.test.js.snapis excluded by!**/*.snapghost/core/test/regression/api/admin/__snapshots__/authentication.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
ghost/core/core/boot.js(3 hunks)ghost/core/core/server/services/url/LocalFileCache.js(0 hunks)ghost/core/core/server/services/url/UrlService.js(2 hunks)ghost/core/core/server/services/url/index.js(1 hunks)ghost/core/core/shared/config/env/config.testing-browser.json(1 hunks)ghost/core/core/shared/config/env/config.testing-mysql.json(1 hunks)ghost/core/core/shared/config/env/config.testing.json(1 hunks)ghost/core/core/shared/labs.js(0 hunks)ghost/core/test/e2e-api/admin/sso.test.js(1 hunks)ghost/core/test/e2e-api/admin/utils.js(1 hunks)ghost/core/test/e2e-api/content/utils.js(1 hunks)ghost/core/test/regression/api/admin/utils.js(1 hunks)ghost/core/test/regression/api/content/utils.js(1 hunks)ghost/core/test/unit/server/services/url/LocalFileCache.test.js(0 hunks)ghost/core/test/unit/shared/config/loader.test.js(0 hunks)ghost/core/test/unit/shared/labs.test.js(2 hunks)ghost/core/test/utils/e2e-framework.js(6 hunks)ghost/core/test/utils/e2e-utils.js(1 hunks)ghost/core/test/utils/fixtures/urls/resources.json(0 hunks)ghost/core/test/utils/fixtures/urls/urls.json(0 hunks)ghost/core/test/utils/url-service-utils.js(1 hunks)
💤 Files with no reviewable changes (6)
- ghost/core/core/shared/labs.js
- ghost/core/test/unit/shared/config/loader.test.js
- ghost/core/core/server/services/url/LocalFileCache.js
- ghost/core/test/utils/fixtures/urls/resources.json
- ghost/core/test/utils/fixtures/urls/urls.json
- ghost/core/test/unit/server/services/url/LocalFileCache.test.js
🚧 Files skipped from review as they are similar to previous changes (14)
- ghost/core/test/e2e-api/admin/sso.test.js
- ghost/core/test/regression/api/admin/utils.js
- ghost/core/test/e2e-api/admin/utils.js
- ghost/core/test/utils/e2e-utils.js
- ghost/core/core/server/services/url/index.js
- ghost/core/test/regression/api/content/utils.js
- ghost/core/core/shared/config/env/config.testing-browser.json
- ghost/core/test/unit/shared/labs.test.js
- ghost/core/test/utils/url-service-utils.js
- ghost/core/core/shared/config/env/config.testing.json
- ghost/core/test/e2e-api/content/utils.js
- ghost/core/core/boot.js
- ghost/core/test/utils/e2e-framework.js
- ghost/core/core/shared/config/env/config.testing-mysql.json
🔇 Additional comments (2)
ghost/core/core/server/services/url/UrlService.js (2)
28-28: Constructor signature simplified appropriatelyThe constructor has been properly updated to remove the
cacheparameter, aligning with the PR objective of removing the experimental URL cache functionality. This change simplifies the class instantiation and removes unnecessary dependencies.
299-308: Init method successfully simplifiedThe
initmethod has been streamlined by removing all cache-related logic, now focusing solely on initializing event listeners, fetching resources, and starting the queue. This change makes the initialization process more straightforward and predictable.
daniellockyer
force-pushed
the
remove-url-cache
branch
2 times, most recently
from
a32ff11 to
4ffb70c
Compare
fix https://linear.app/ghost/issue/ENG-1803/remove-url-cache-code - this feature is experimental and was designed in order to speed up our URL service init by storing a cache of the object - however, it was never really finished and we've had a few bugs with it - to avoid further issues, this commit removes it - along the way, I've discovered that our tests REQUIRE the URL cache in order to pass, which I thought was weird. Turns out it's because they incorrectly set cache values into the URL service that are outdated, which meant our snapshots are wrong - this is why several of the snapshots have been updated, because URLs have changed - sadly this commit touches so many files, but this feature was really spread around the codebase - this technically removes test util API support for a split backend/frontend but that wasn't properly finished. I'd like to get that working, but first we need to unpick the mess we've got ourselves into
daniellockyer
force-pushed
the
remove-url-cache
branch
from
4ffb70c to
1c9de2f
Compare
1 participant
fix https://linear.app/ghost/issue/ENG-1803/remove-url-cache-code