Removed unused tinybird filters#36
Conversation
WalkthroughThis pull request removes browser, device, and OS filtering capabilities from multiple Tinybird pipe definitions and corresponding test cases. Additionally, the TinybirdService is updated to remove references to three browser/device/OS pipes and modify JWT token handling logic. 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 (10)
✏️ 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 (2)
ghost/core/core/server/services/tinybird/TinybirdService.js (2)
92-104: Return a token string, not the token object.Line 98 now stores the full object, so
getToken()returns{ token: { token, exp }, exp }, and_isJWTExpiredreceives an object. This breaks call sites/tests that expectresult.tokento be a JWT string (e.g.,ghost/core/test/unit/server/services/tinybird/TinybirdService.test.jsLine 53 usesjwt.verify(result.token, ...)). Please keep_serverTokenas the string or update all usages.🐛 Proposed fix (minimal)
- this._serverToken = tokenData; + this._serverToken = tokenData.token;
162-170: Add signature verification to prevent acceptance of tokens signed with rotated keys.Currently
jwt.decodeonly decodes without validating the signature. IfadminTokenis rotated, tokens signed with the old key will still be accepted until theirexptime, creating a security window. Usejwt.verifywithignoreExpiration: trueto validate the signature while preserving manual expiration checking:Proposed fix
- const decoded = jwt.decode(token); + const decoded = jwt.verify(token, this.tinybirdConfig.adminToken, {ignoreExpiration: true});
📜 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_locations.pipe
- ghost/core/core/server/data/tinybird/endpoints/api_kpis.pipe
- ghost/core/core/server/data/tinybird/endpoints/api_top_pages.pipe
- ghost/core/core/server/data/tinybird/pipes/filtered_sessions.pipe
- ghost/core/core/server/data/tinybird/tests/api_kpis.yaml
🧰 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 (10)
ghost/core/core/server/data/tinybird/tests/api_top_utm_campaigns.yaml (1)
74-79: LGTM! Test case correctly updated to use remaining filter dimensions.The combined filter test now uses
sourceandpathnameinstead of the removeddeviceandbrowserfilters. The expected results (retention_q4andnewsletter_weekly) correctly represent the intersection of the individualsource=bing.com(lines 42-47) andpathname=/about/(lines 22-30) filter results.ghost/core/core/server/data/tinybird/tests/api_top_utm_terms.yaml (1)
77-82: LGTM! Combined filter test correctly updated.The expected results (
subscribersandloyal_customers) correctly represent the intersection of sessions matching bothsource=bing.comandpathname=/about/based on the individual filter test cases in this file.ghost/core/core/server/data/tinybird/tests/api_top_utm_contents.yaml (1)
76-81: LGTM! Test parameters and expected results correctly updated.The combined filter results match the intersection of the
source=bing.comandpathname=/about/individual filter results defined earlier in this file.ghost/core/core/server/data/tinybird/tests/api_top_utm_sources.yaml (1)
83-88: LGTM! Combined filter test correctly reflects the intersection of filter criteria.The expected results are consistent with the intersection of
source=bing.comandpathname=/about/individual test results.ghost/core/core/server/data/tinybird/tests/api_top_sources.yaml (1)
84-88: LGTM! Combined filter test correctly updated.The expected result of
bing.comwith 2 visits is consistent with filtering by bothsource=bing.comandpathname=/about/, as verified against the individual filter test cases.ghost/core/core/server/data/tinybird/tests/api_top_utm_mediums.yaml (1)
74-79: LGTM! Combined filter test correctly updated with proper intersection logic.The expected results (
socialwith 1 visit andsource=bing.comindividual filter results.ghost/core/core/server/data/tinybird/tests/api_top_locations.yaml (1)
71-76: Update aligns with new filter set.Looks consistent after switching the combined filters to
source+pathnameand narrowing the expected result accordingly.ghost/core/core/server/data/tinybird/tests/api_top_pages.yaml (1)
65-70: Combined filter test looks consistent.The updated parameters and narrowed expected result line up with the new
source+pathnamefilter combination.ghost/core/core/server/services/tinybird/TinybirdService.js (2)
61-67: Helpful local Tinybird test guidance.Nice addition for local test setup.
128-152: No action needed—Tinybird accepts standard JWT claims.Tinybird's JWT validation only enforces the required claims (
workspace_id,name,exp,scopes) and does not reject extra standard claims. Theiatclaim added by default injwt.sign()is compliant with RFC 7519 and will not cause issues with Tinybird token validation. ReintroducingnoTimestampis unnecessary.
✏️ 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.