Removed experimental URL cache #21751
Conversation
daniellockyer
force-pushed
the
remove-url-cache
branch
3 times, most recently
from
fdbe914 to
50ba6e1
Compare
daniellockyer
force-pushed
the
remove-url-cache
branch
from
50ba6e1 to
6b2f4de
Compare
WalkthroughThe changes remove caching functionality and simplify several core services. The Suggested labels
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (1)
ghost/core/test/utils/url-service-utils.js (1)
20-21: Consider awaiting the async init call.Since “urlService.init()” is an async function, you may want to return or await it to ensure proper test sequencing. This helps avoid race conditions if any tests depend on the initialization completing first.
Here’s an example adjustment:
- module.exports.init = () => { - urlService.init(); - }; + module.exports.init = async () => { + await urlService.init(); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
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/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-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/unit/server/services/url/LocalFileCache.test.js
- ghost/core/test/utils/fixtures/urls/resources.json
- ghost/core/core/server/services/url/LocalFileCache.js
- ghost/core/test/utils/fixtures/urls/urls.json
✅ Files skipped from review due to trivial changes (2)
- ghost/core/core/shared/config/env/config.testing.json
- ghost/core/core/shared/config/env/config.testing-mysql.json
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Lint
🔇 Additional comments (10)
ghost/core/test/unit/shared/labs.test.js (2)
36-37: LGTM! The test cases correctly handle the newNestPlaygroundflag.The changes consistently replace the old
urlCacheflag withNestPlaygroundacross the test suite, aligning with the PR's objective of removing the URL cache feature.Also applies to: 42-43, 47-47
55-56: LGTM! The test verifies that alpha flags are disabled when dev experiments are off.The test case correctly ensures that the
NestPlaygroundflag is disabled when developer experiments are turned off, maintaining the expected behavior for alpha features.Also applies to: 65-65
ghost/core/core/server/services/url/UrlService.js (2)
299-310: Asynchronous init logic streamlined.Removing the cache handling here and simplifying resource fetching is consistent with the goal of removing caching. The updated method body looks coherent, with initialization steps clearly laid out. No issues found.
17-17: Constructor param removal appears consistent.Removing the
optionsparam from the constructor aligns with phasing out the URL cache. Please confirm that no other code path still relies on the old constructor signature.Run this script to search references of a previous constructor signature passing an argument:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Look for code passing arguments into UrlService constructor rg "new UrlService\(\{"Length of output: 25
I'll run an additional verification script to ensure there are no invocations of the UrlService constructor with non-empty arguments and no lingering references to the old "options" parameter.
🏁 Script executed:
#!/bin/bash # Search for UrlService invocations that pass any parameter echo "Searching for new UrlService invocations with non-empty arguments:" rg "new UrlService\(\s*[^)]" echo "" echo "Searching for references to a constructor with an 'options' parameter:" rg "constructor\s*\(\s*options\s*\)"Length of output: 1047
Constructor Signature Change Verified
- No calls to the UrlService constructor passing parameters (e.g., object literals) were found in the codebase.
- References to constructors using an "options" parameter appear in unrelated classes; none target UrlService.
These findings confirm that removing the "options" parameter from the UrlService constructor does not impact any code paths.
ghost/core/core/server/services/url/index.js (1)
3-3: Instantiation matches new constructor signature.Instantiating the service without parameters aligns with the removal of the cache logic. No conflicts found.
ghost/core/test/utils/e2e-utils.js (1)
136-136: LGTM!The removal of the
urlCacheparameter fromurlServiceUtils.init()aligns with the PR objective of removing the experimental URL cache feature.ghost/core/core/boot.js (3)
83-83: LGTM!The removal of the
frontendparameter from theinitCorefunction signature is consistent with the PR objective of removing URL caching functionality.
564-564: LGTM!The updated
initCorecall correctly reflects the removal of thefrontendparameter.
120-120: LGTM!The removal of the URL cache-related callback parameter aligns with the overall cleanup of the URL caching feature.
ghost/core/core/shared/config/env/config.testing-browser.json (1)
82-82: LGTM!The removal of the
urlCachepath from the configuration file is consistent with the PR objective of removing the experimental URL cache feature.
daniellockyer
force-pushed
the
remove-url-cache
branch
2 times, most recently
from
1c78096 to
db205ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ghost/core/test/e2e-api/content/utils.js (1)
99-101: Consider handling undefined parameters.While the change is correct, it would be more robust to maintain the default parameter to ensure
testUtils.startGhostalways receives an object.- async startGhost(overrides) { + async startGhost(overrides = {}) { return await testUtils.startGhost(overrides); }ghost/core/test/utils/e2e-framework.js (2)
202-204: Consider cleaning up the now redundantfrontendparameter.
These lines always setfrontend: true, which seems to be the new baseline for these tests. Removing the optional parameter fromstartGhostaltogether (or documenting its new purpose) might reduce confusion in the future.
224-227: Align documentation with enforced frontend boot.
These lines similarly hardcodefrontend: true. Ensure that any references or docs implying conditional frontend booting are updated accordingly to avoid mismatched expectations.ghost/core/core/server/services/url/UrlService.js (2)
25-26: Consider addressing the initialization order technical debt.The comment indicates that
UrlsandResourcesinitialization should be moved to theinitmethod, but this change is blocked by test dependencies. Consider creating a follow-up task to refactor the tests and move the initialization to theinitmethod for better separation of concerns.Would you like me to help create an issue to track this technical debt?
312-314: Consider improving the shutdown method.The method is marked as
asyncbut contains no implementation. Consider either:
- Removing the
asynckeyword if no asynchronous cleanup is needed- Adding cleanup for other resources (e.g., removing event listeners, closing connections)
Example improvement:
-async shutdown() { +shutdown() { + this.resources.releaseAll(); + this.queue.removeListener('started', this._onQueueStartedListener); + this.queue.removeListener('ended', this._onQueueEndedListener); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
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!**/*.snap
📒 Files selected for processing (19)
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/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(3 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/unit/server/services/url/LocalFileCache.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
🚧 Files skipped from review as they are similar to previous changes (8)
- ghost/core/core/shared/config/env/config.testing-browser.json
- ghost/core/test/utils/url-service-utils.js
- ghost/core/core/shared/config/env/config.testing-mysql.json
- ghost/core/test/e2e-api/admin/utils.js
- ghost/core/core/shared/config/env/config.testing.json
- ghost/core/test/utils/e2e-utils.js
- ghost/core/test/unit/shared/labs.test.js
- ghost/core/core/boot.js
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: i18n
- GitHub Check: Lint
🔇 Additional comments (5)
ghost/core/test/e2e-api/content/utils.js (1)
99-101: LGTM! Changes align with URL cache removal.The simplified implementation supports the PR's objective of removing the URL cache functionality by making the test setup more explicit.
ghost/core/test/utils/e2e-framework.js (1)
302-303: Looks good.
Always enabling the frontend here is consistent with the other agents.ghost/core/core/server/services/url/index.js (1)
3-3: Removal of cache parameter looks correct.
InstantiatingUrlServicewithout caching parameters aligns with removing the experimental URL cache.ghost/core/test/e2e-api/admin/sso.test.js (1)
12-13: Model initialization improvement.
Callingmodels.init()early ensures all tables and model relationships are fully set up before test execution. This change looks good.ghost/core/core/server/services/url/UrlService.js (1)
299-310: LGTM! The initialization flow is now cleaner and more maintainable.The method has been simplified to focus on core initialization steps with a clear sequence of operations. The async/await usage properly handles asynchronous operations.
daniellockyer
force-pushed
the
remove-url-cache
branch
from
db205ce to
92b2382
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/services/url/UrlService.js (1)
312-313: Consider removing empty method.The
shutdownmethod is now empty after removing cache writing logic. Consider removing this method if it's no longer needed.-async shutdown() { -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
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!**/*.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(4 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
🚧 Files skipped from review as they are similar to previous changes (11)
- ghost/core/core/shared/config/env/config.testing-mysql.json
- ghost/core/test/e2e-api/admin/utils.js
- ghost/core/core/shared/config/env/config.testing-browser.json
- ghost/core/test/e2e-api/admin/sso.test.js
- ghost/core/core/shared/config/env/config.testing.json
- ghost/core/test/utils/e2e-utils.js
- ghost/core/test/e2e-api/content/utils.js
- ghost/core/test/utils/url-service-utils.js
- ghost/core/core/boot.js
- ghost/core/test/unit/shared/labs.test.js
- ghost/core/core/server/services/url/index.js
🔇 Additional comments (15)
ghost/core/core/server/services/url/UrlService.js (5)
17-34: LGTM! Constructor simplified by removing cache options.The constructor has been simplified by removing the
cacheoption, which aligns with the PR objective of removing the experimental URL cache.
299-310: LGTM! Initialization process simplified by removing cache-related logic.The initialization process has been streamlined by removing the
urlCacheparameter and its associated logic, which aligns with the PR objective of removing the experimental URL cache.
312-313: LGTM! Shutdown process simplified by removing cache writing logic.The shutdown process has been streamlined by removing the cache writing logic, which aligns with the PR objective of removing the experimental URL cache.
17-17: LGTM!The constructor has been simplified by removing the cache-related options parameter, which aligns with the PR objectives of removing URL cache functionality.
299-310: LGTM!The
initmethod has been simplified by:
- Removing the cache-related parameter
- Streamlining the initialization flow
- Directly starting the queue after fetching resources
These changes align with the PR objectives of removing URL cache functionality.
ghost/core/test/utils/e2e-framework.js (8)
81-81: LGTM! Frontend booting enforced by default.The method now enforces frontend booting by default, which aligns with the PR objective of removing the experimental URL cache and simplifies the initialization process.
202-204: LGTM! Frontend booting explicitly enabled for content API agent.The method now explicitly sets
frontend: true, ensuring consistent behavior across all agent creation functions.
225-227: LGTM! Frontend booting enabled for admin API agent.The method now sets
frontend: true, ensuring consistent behavior across all agent creation functions.
302-302: LGTM! Frontend booting enabled for Ghost API agent.The method now sets
frontend: true, ensuring consistent behavior across all agent creation functions.
81-81: LGTM!Setting
frontend: trueas the default instartGhostensures consistent behavior across all test agents.
202-204: LGTM!Explicitly setting
frontend: trueingetContentAPIAgentaligns with the new default behavior.
225-227: LGTM!Simplified
getAdminAPIAgentby settingfrontend: truealigns with the new default behavior.
302-302: LGTM!Setting
frontend: trueingetGhostAPIAgentaligns with the new default behavior.ghost/core/test/regression/api/content/utils.js (1)
105-106: LGTM!The simplification of the
startGhostfunction by removing the default parameter and directly passing overrides aligns with the PR objectives of removing URL cache functionality.ghost/core/test/regression/api/admin/utils.js (1)
209-210: LGTM!The simplification of the
startGhostfunction by removing the default parameter and directly passing overrides aligns with the PR objectives of removing URL cache functionality.
daniellockyer
force-pushed
the
remove-url-cache
branch
2 times, most recently
from
75b523b to
16adbb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/services/url/UrlService.js (2)
305-309: Consider making queue configuration more flexible.The
requiredSubscriberCountis hardcoded to 1, which might be too rigid. Consider making it configurable to handle changes in the number of subscribers.- this.queue.start({ - event: 'init', - tolerance: 100, - requiredSubscriberCount: 1 - }); + this.queue.start({ + event: 'init', + tolerance: 100, + requiredSubscriberCount: this.urlGenerators.length || 1 + });
312-313: Consider removing the empty shutdown method.Since the cache writing logic has been removed, this empty method serves no purpose and could be safely removed.
- async shutdown() { - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
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/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/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/core/shared/config/env/config.testing-browser.json
- ghost/core/test/utils/e2e-utils.js
- ghost/core/test/regression/api/content/utils.js
- ghost/core/test/e2e-api/admin/utils.js
- ghost/core/core/shared/config/env/config.testing.json
- ghost/core/test/e2e-api/admin/sso.test.js
- ghost/core/test/utils/url-service-utils.js
- ghost/core/core/server/services/url/index.js
- ghost/core/core/boot.js
- ghost/core/test/e2e-api/content/utils.js
- ghost/core/test/regression/api/admin/utils.js
- ghost/core/test/utils/e2e-framework.js
- ghost/core/test/unit/shared/labs.test.js
🔇 Additional comments (2)
ghost/core/core/server/services/url/UrlService.js (2)
17-17: LGTM! Constructor simplified by removing cache parameter.The change aligns with the PR objective of removing the experimental URL cache feature.
299-304: LGTM! Initialization flow simplified.The removal of cache-related logic makes the initialization process more straightforward and maintainable.
daniellockyer
force-pushed
the
remove-url-cache
branch
from
16adbb6 to
09f7040
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/services/url/UrlService.js (1)
312-313: Add JSDoc comment to empty shutdown method.While keeping the empty method is good for interface compatibility and future extensibility, it should be documented to explain its purpose.
+/** + * @description Placeholder method maintained for interface compatibility and future extensibility. + */ async shutdown() { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ 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/unit/server/services/url/LocalFileCache.test.js
- ghost/core/test/unit/shared/config/loader.test.js
- ghost/core/test/utils/fixtures/urls/urls.json
- ghost/core/test/utils/fixtures/urls/resources.json
🚧 Files skipped from review as they are similar to previous changes (14)
- ghost/core/core/shared/config/env/config.testing-browser.json
- ghost/core/test/regression/api/content/utils.js
- ghost/core/core/shared/config/env/config.testing-mysql.json
- ghost/core/core/boot.js
- ghost/core/core/shared/config/env/config.testing.json
- ghost/core/test/e2e-api/admin/utils.js
- ghost/core/core/server/services/url/index.js
- ghost/core/test/e2e-api/admin/sso.test.js
- ghost/core/test/unit/shared/labs.test.js
- ghost/core/test/utils/url-service-utils.js
- ghost/core/test/regression/api/admin/utils.js
- ghost/core/test/utils/e2e-utils.js
- ghost/core/test/utils/e2e-framework.js
- ghost/core/test/e2e-api/content/utils.js
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Lint
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: i18n
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 18.12.1, mysql8)
🔇 Additional comments (2)
ghost/core/core/server/services/url/UrlService.js (2)
17-17: LGTM! Constructor simplified by removing cache parameter.The constructor has been streamlined to focus on core initialization, aligning with the PR objective of removing URL cache functionality.
299-310: LGTM! Initialization flow simplified by removing cache logic.The
initmethod has been streamlined to follow a clear, direct initialization flow:
- Set callback
- Initialize listeners
- Fetch resources
- Start queue
The changes maintain essential functionality while reducing complexity.
daniellockyer
force-pushed
the
remove-url-cache
branch
from
09f7040 to
2cfa836
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/services/url/UrlService.js (1)
312-313: Consider adding a comment explaining the empty method.The shutdown method is now empty but retained for interface compatibility. Consider adding a comment to explain why this method exists.
async shutdown() { + // Method retained for interface compatibility }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ 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/test/utils/fixtures/urls/resources.json
- 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
🚧 Files skipped from review as they are similar to previous changes (13)
- ghost/core/core/shared/config/env/config.testing-mysql.json
- ghost/core/core/shared/config/env/config.testing-browser.json
- ghost/core/core/shared/config/env/config.testing.json
- ghost/core/test/regression/api/content/utils.js
- ghost/core/test/utils/url-service-utils.js
- ghost/core/test/e2e-api/admin/sso.test.js
- ghost/core/test/regression/api/admin/utils.js
- ghost/core/test/utils/e2e-utils.js
- ghost/core/core/boot.js
- ghost/core/test/e2e-api/content/utils.js
- ghost/core/test/unit/shared/labs.test.js
- ghost/core/core/server/services/url/index.js
- ghost/core/test/e2e-api/admin/utils.js
🔇 Additional comments (4)
ghost/core/core/server/services/url/UrlService.js (2)
17-34: LGTM! Constructor simplified by removing cache options.The constructor has been simplified by removing cache-related options, which aligns with the PR objective of removing the experimental URL cache.
299-310: LGTM! Initialization logic streamlined.The init method has been simplified by removing cache-related logic and directly initializing resources and queue. The changes make the code more maintainable.
ghost/core/test/utils/e2e-framework.js (2)
79-83: LGTM! Frontend is now always enabled by default.The change to enable frontend by default simplifies the test setup and aligns with the removal of URL cache functionality.
222-313: LGTM! API agent creation methods simplified.The API agent creation methods have been simplified by removing boot options, which is consistent with the new default frontend behavior.
fix https://linear.app/ghost/issue/ENG-1803/remove-url-cache-code