Removed unused tinybird filters#23
Conversation
WalkthroughThis PR removes device, browser, and OS filtering capabilities from multiple Tinybird pipe endpoints and their tests, while removing the corresponding browser, device, and OS-focused pipes from service configuration. Token handling logic in TinybirdService is refactored. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
💤 Files with no reviewable changes (5)
🧰 Additional context used🧬 Code graph analysis (1)ghost/core/core/server/services/tinybird/TinybirdService.js (3)
🔇 Additional comments (12)
✏️ Tip: You can disable this entire section by setting 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/tinybird/TinybirdService.js (1)
96-104: Critical bug: Type mismatch causes broken token handling.Line 98 stores the entire
tokenDataobject ({token, exp}) inthis._serverToken, but:
- Line 96 passes
this._serverTokento_isJWTExpired(), which callsjwt.decode()expecting a string- Line 102 returns
this._serverTokenas the token value, but callers expect a string (seetinybird.js:16and test files usingjwt.verify(result.token, ...))This will cause
jwt.decode()to fail on subsequent calls, and API consumers will receive an object instead of a JWT string.🐛 Proposed fix
if (!this._serverToken || this._isJWTExpired(this._serverToken)) { const tokenData = this._generateToken({name, expiresInMinutes}); - this._serverToken = tokenData; + this._serverToken = tokenData.token; this._serverTokenExp = tokenData.exp; } return { token: this._serverToken, exp: this._serverTokenExp };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ghost/core/core/server/data/tinybird/endpoints/api_kpis.pipeghost/core/core/server/data/tinybird/endpoints/api_top_locations.pipeghost/core/core/server/data/tinybird/endpoints/api_top_pages.pipeghost/core/core/server/data/tinybird/pipes/filtered_sessions.pipeghost/core/core/server/data/tinybird/tests/api_kpis.yamlghost/core/core/server/data/tinybird/tests/api_top_locations.yamlghost/core/core/server/data/tinybird/tests/api_top_pages.yamlghost/core/core/server/data/tinybird/tests/api_top_sources.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_campaigns.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_contents.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_mediums.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_sources.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_terms.yamlghost/core/core/server/services/tinybird/TinybirdService.js
💤 Files with no reviewable changes (5)
- ghost/core/core/server/data/tinybird/endpoints/api_top_pages.pipe
- ghost/core/core/server/data/tinybird/endpoints/api_kpis.pipe
- ghost/core/core/server/data/tinybird/pipes/filtered_sessions.pipe
- ghost/core/core/server/data/tinybird/tests/api_kpis.yaml
- ghost/core/core/server/data/tinybird/endpoints/api_top_locations.pipe
🧰 Additional context used
🧬 Code graph analysis (1)
ghost/core/core/server/services/tinybird/TinybirdService.js (3)
ghost/core/core/server/api/endpoints/tinybird.js (1)
tokenData(17-17)ghost/core/test/unit/server/services/tinybird/TinybirdService.test.js (5)
jwt(3-3)decoded(54-54)decoded(60-60)decoded(67-67)decoded(79-79)ghost/core/test/e2e-api/admin/tinybird.test.js (2)
jwt(53-53)decoded(54-54)
🔇 Additional comments (12)
ghost/core/core/server/data/tinybird/tests/api_top_locations.yaml (1)
73-75: Multi-filter scenario aligns with new filter set.
The updated source+pathname parameters and expected_result look consistent with the removal of device/browser/os filters.ghost/core/core/server/data/tinybird/tests/api_top_sources.yaml (1)
86-88: Multi-filter test update looks good.
Switching to source+pathname with the narrowed expected_result matches the new filter behavior.ghost/core/core/server/data/tinybird/tests/api_top_utm_campaigns.yaml (1)
76-79: Multi-filter UTM campaign fixture update is consistent.
The new source+pathname parameters and expected_result match the revised filter scope.ghost/core/core/server/data/tinybird/tests/api_top_utm_contents.yaml (1)
78-81: Multi-filter UTM content scenario update looks solid.
The expected_result reflects the new source+pathname filter combination.ghost/core/core/server/data/tinybird/tests/api_top_utm_sources.yaml (1)
85-88: Multi-filter UTM source expectations look aligned.
The updated parameters and expected_result match the new filter inputs.ghost/core/core/server/data/tinybird/tests/api_top_pages.yaml (1)
67-69: Multi-filter parameters updated appropriately.
Switching to source+pathname aligns with the removal of device/browser filtering.ghost/core/core/server/data/tinybird/tests/api_top_utm_terms.yaml (1)
77-82: LGTM!The updated combined filter test using
source=bing.com&pathname=%2Fabout%2Fis consistent with the PR's objective to remove browser/device/OS filters. The expected results correctly reflect the intersection of source and pathname filters based on the individual test cases above.ghost/core/core/server/data/tinybird/tests/api_top_utm_mediums.yaml (1)
74-79: LGTM!The updated combined filter test is consistent with the individual filter tests. The expected results correctly reflect the intersection of
source=bing.com(social:1, email:1) andpathname=/about/filters.ghost/core/core/server/services/tinybird/TinybirdService.js (4)
47-59: LGTM!The removal of
api_top_browsers,api_top_devices, andapi_top_osfrom the pipes list aligns with the PR objective to remove unused Tinybird filters.
61-67: LGTM!Helpful documentation for local Tinybird test setup.
162-174: Acceptable change to usejwt.decode()for expiration check.Using
jwt.decode()instead ofjwt.verify()is reasonable here since we're only checking the expiration of our own locally-generated token. Signature verification isn't necessary for self-generated tokens cached in memory.
147-148: JWT will now includeiatclaim.Removing the
{noTimestamp: true}option means generated tokens will include aniat(issued at) claim. This is beneficial for debugging and audit purposes. The existing tests validate specific token claims without asserting on exact payload structure, so they will continue to pass with the addediatclaim.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Benchmark PR from qodo-benchmark#248
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.