Removed unused tinybird filters#50
Conversation
WalkthroughThis pull request removes filtering capabilities for device, browser, and operating system from Tinybird analytics pipes and their test suites, eliminates corresponding API endpoints from the service allowlist, and modifies JWT token generation behavior in the TinybirdService. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 comments (11)
✏️ Tip: You can disable this entire section by setting Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: Token object stored instead of token string.The code stores the entire
tokenDataobject (which has{token, exp}) inthis._serverToken, but:
_isJWTExpired(this._serverToken)at line 96 passes this object tojwt.decode()which expects a string- The return at line 102 returns
{token: {token, exp}, exp}instead of the expected{token: string, exp}This breaks on the second call when
this._serverTokenis already an object.🐛 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/tests/api_kpis.yaml
- 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/endpoints/api_top_locations.pipe
🔇 Additional comments (11)
ghost/core/core/server/data/tinybird/tests/api_top_utm_mediums.yaml (1)
74-79: LGTM!The updated combined filter test uses the remaining supported filters (source + pathname) after removal of device/browser/os filters. The expected results are consistent with the individual filter tests (matches the "Filtered by source - bing.com" test results at lines 42-47).
ghost/core/core/server/data/tinybird/tests/api_top_utm_terms.yaml (1)
77-82: LGTM!The updated combined filter test aligns with the removal of device/browser/os filters. Expected results match the "Filtered by source - bing.com" test case.
ghost/core/core/server/services/tinybird/TinybirdService.js (3)
162-174: Usingjwt.decodeinstead ofjwt.verifyis acceptable here.Since this method only checks expiration of self-generated tokens (not validating external tokens), using
decodeis appropriate and more efficient. The signature verification is unnecessary for tokens this service created.
61-67: LGTM!Helpful addition of local testing instructions in the class documentation.
47-59: Verify removed pipes are not referenced elsewhere.The removal of
api_top_browsers,api_top_devices, andapi_top_osfrom the allowlist aligns with the PR objective. Ensure no other code attempts to call these endpoints by searching for references to these pipe names throughout the codebase.ghost/core/core/server/data/tinybird/tests/api_top_locations.yaml (1)
71-77: LGTM!The updated combined filter test is consistent with the removal of device/browser/os filters. Expected results match the "Filtered by source - bing.com" test case.
ghost/core/core/server/data/tinybird/tests/api_top_utm_campaigns.yaml (1)
74-79: LGTM!The combined filter test update follows the same pattern as other test files, using source and pathname filters after removing device/browser/os support.
ghost/core/core/server/data/tinybird/tests/api_top_utm_contents.yaml (1)
76-81: LGTM!The combined filter test update is consistent with other test file changes in this PR.
ghost/core/core/server/data/tinybird/tests/api_top_sources.yaml (1)
84-88: Combined filter update looks consistent with new filter surface.
The new source+pathname combo and narrowed expected result align with the removal of device/browser/os filters.ghost/core/core/server/data/tinybird/tests/api_top_utm_sources.yaml (1)
83-88: Updated multi-filter expectations match the new source+pathname filters.
Looks consistent with the PR’s removal of device/browser/os filter paths.ghost/core/core/server/data/tinybird/tests/api_top_pages.yaml (1)
65-69: Multi-filter test updated appropriately for source+pathname.
No issues spotted with the revised parameters/expected result.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Benchmark PR from agentic-review-benchmarks#12
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.