Testing a change to TB #22359
Testing a change to TB #22359
Conversation
WalkthroughThe pull request primarily updates multiple files within the ghost/tinybird-dedicated directory by incrementing version numbers from 1 to 2. A new field, 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/tinybird-dedicated/pipes/_hits.pipe (1)
19-19: Consider adding data validation for post_type.While the
toString()function safely handles null values, consider adding validation or default values forpost_typeif it's expected to have specific values. This would improve data quality and prevent issues downstream.- toString(payload.post_type) as post_type, + coalesce(toString(payload.post_type), 'unknown') as post_type,Also applies to: 38-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
ghost/tinybird-dedicated/datasources/_pages_mv.datasource(2 hunks)ghost/tinybird-dedicated/datasources/_sessions_mv.datasource(1 hunks)ghost/tinybird-dedicated/datasources/_sources_mv.datasource(1 hunks)ghost/tinybird-dedicated/pipes/_hits.pipe(3 hunks)ghost/tinybird-dedicated/pipes/_kpis.pipe(1 hunks)ghost/tinybird-dedicated/pipes/_pages.pipe(3 hunks)ghost/tinybird-dedicated/pipes/_sessions.pipe(1 hunks)ghost/tinybird-dedicated/pipes/_sources.pipe(1 hunks)ghost/tinybird-dedicated/pipes/_top_browsers.pipe(1 hunks)ghost/tinybird-dedicated/pipes/_top_devices.pipe(1 hunks)ghost/tinybird-dedicated/pipes/_top_locations.pipe(1 hunks)ghost/tinybird-dedicated/pipes/_top_pages.pipe(2 hunks)ghost/tinybird-dedicated/pipes/_top_sources.pipe(1 hunks)ghost/tinybird-dedicated/pipes/analytics_pages.pipe(1 hunks)ghost/tinybird-dedicated/pipes/analytics_sessions.pipe(1 hunks)ghost/tinybird-dedicated/pipes/chris_test.pipe(1 hunks)ghost/tinybird-dedicated/pipes/top_browsers.pipe(1 hunks)ghost/tinybird-dedicated/pipes/top_devices.pipe(1 hunks)ghost/tinybird-dedicated/pipes/top_locations.pipe(1 hunks)ghost/tinybird-dedicated/pipes/top_os.pipe(1 hunks)ghost/tinybird-dedicated/pipes/top_pages.pipe(1 hunks)ghost/tinybird-dedicated/pipes/top_pages_copy.pipe(1 hunks)ghost/tinybird-dedicated/pipes/top_sources.pipe(1 hunks)
✅ Files skipped from review due to trivial changes (18)
- ghost/tinybird-dedicated/pipes/_top_browsers.pipe
- ghost/tinybird-dedicated/pipes/_sources.pipe
- ghost/tinybird-dedicated/pipes/top_pages_copy.pipe
- ghost/tinybird-dedicated/pipes/top_pages.pipe
- ghost/tinybird-dedicated/datasources/_sources_mv.datasource
- ghost/tinybird-dedicated/pipes/_sessions.pipe
- ghost/tinybird-dedicated/pipes/analytics_pages.pipe
- ghost/tinybird-dedicated/pipes/_top_devices.pipe
- ghost/tinybird-dedicated/pipes/top_browsers.pipe
- ghost/tinybird-dedicated/pipes/top_locations.pipe
- ghost/tinybird-dedicated/pipes/chris_test.pipe
- ghost/tinybird-dedicated/pipes/_top_sources.pipe
- ghost/tinybird-dedicated/pipes/_kpis.pipe
- ghost/tinybird-dedicated/pipes/top_os.pipe
- ghost/tinybird-dedicated/pipes/_top_locations.pipe
- ghost/tinybird-dedicated/pipes/analytics_sessions.pipe
- ghost/tinybird-dedicated/pipes/top_devices.pipe
- ghost/tinybird-dedicated/datasources/_sessions_mv.datasource
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Tinybird Tests
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: i18n
- GitHub Check: Lint
🔇 Additional comments (15)
ghost/tinybird-dedicated/pipes/top_sources.pipe (2)
1-1: Version number incremented appropriately.The version has been updated from 1 to 2, which aligns with the schema changes across the system.
5-5: Documentation improvement for existing functionality.The description now correctly mentions the
skipandlimitparameters used for pagination in line 45, which improves API documentation for users.ghost/tinybird-dedicated/pipes/_top_pages.pipe (2)
1-1: Version number incremented appropriately.The version increment from 1 to 2 is consistent with other file changes in this PR.
26-26: Filter for new post_type field implemented correctly.The new conditional filter for
post_typefollows the same pattern as other filters in the query, maintaining consistency in the codebase.ghost/tinybird-dedicated/pipes/_pages.pipe (3)
1-1: Version number incremented appropriately.Version has been updated from 1 to 2, consistent with schema changes.
14-14: New field added to SELECT clause.The
post_typefield has been added to the SELECT clause, which is necessary to expose this field in the query results.
27-28: GROUP BY clause updated correctly.The GROUP BY clause has been updated to include both
post_uuidand the newpost_typefield. Note that line 27 showspost_uuidhas been moved from its previous position to accommodate the addition ofpost_type.ghost/tinybird-dedicated/datasources/_pages_mv.datasource (4)
1-1: Version number incremented appropriately.The version has been updated from 1 to 2, consistent with the schema changes.
7-7: New schema field added.The
post_typefield of typeStringhas been added to the schema, which enables tracking and filtering by post type.
19-19: Engine sorting key updated to include new field.The ENGINE_SORTING_KEY has been properly updated to include the new
post_typefield, which is necessary for optimal query performance when filtering or grouping by this field.
1-20:❓ Verification inconclusive
Verify that existing data migration is handled.
While the schema changes look good, it's important to ensure there's a plan for handling existing data that doesn't have the
post_typefield populated. This might involve a data migration strategy or handling null/default values.
🏁 Script executed:
#!/bin/bash # Check if there are any other files in the PR that might handle data migration echo "Checking for migration-related files in this PR..." git diff --name-only HEAD~1 | grep -i migrat # Check if there's any documentation about handling this schema change echo "Checking for documentation on schema changes..." git diff --name-only HEAD~1 | grep -i readme git diff --name-only HEAD~1 | grep -i docLength of output: 914
ACTION REQUIRED: Confirm Data Migration for
post_typeFieldThe schema update adds the
post_typefield, but we haven’t detected any accompanying migration scripts or documentation to handle pre-existing data. Please verify that there’s a clear strategy for addressing records that lack this field—whether through a backfill migration, default value, or another approach—to ensure data integrity.
- Confirm that a migration plan is in place to populate
post_typefor already existing records.- Update documentation or include migration files as necessary.
ghost/tinybird-dedicated/pipes/_hits.pipe (4)
1-1: Version increment is properly applied.The pipe version has been correctly incremented from 1 to 2, reflecting the addition of the new
post_typefield to the schema.
19-19: New post_type field properly added to parsed_hits query.The
post_typefield is correctly extracted from the payload usingtoString(), following the same pattern as other payload fields in this query. This ensures consistent handling of potentially null values by converting them to strings.
38-38: New post_type field correctly included in analytics_hits_data.The
post_typefield is properly passed through from the parsed_hits node to the analytics_hits_data node, ensuring the data flows correctly through the pipeline.
1-66:❓ Verification inconclusive
Verify data consumers are updated for this schema change.
The addition of the
post_typefield and version increment indicates a schema change. Ensure that any downstream consumers of this pipe's data (dashboards, API integrations, etc.) are updated to handle the new field appropriately.
🏁 Script executed:
#!/bin/bash # Search for files that might be consuming data from this pipe echo "Searching for potential consumers of _hits.pipe..." rg -g "!node_modules" -g "!.git" "_hits.pipe|analytics_hits_data" --type=all # Check for any imports or references to analytics_hits_data echo "Searching for references to analytics_hits_data..." rg -g "!node_modules" -g "!.git" "analytics_hits_data" --type=all # Look for any API endpoints that might be using this data echo "Searching for potential API endpoints using this data..." rg -g "!node_modules" -g "!.git" -g "*.js" "hits|analytics.*hits" --type=jsLength of output: 1486
Schema Change Verification: Ensure Downstream Consumers Handle the New
post_typeFieldThe recent grep results reveal that there are multiple references to the analytics endpoints (e.g., in test files like
ghost/admin/tests/acceptance/staff-test.js,ghost/admin/tests/acceptance/editor-test.js,ghost/api-framework/lib/Frame.js, and others). Although these references indicate that various parts of the application consume data from the updated pipe, it's not immediately clear if those consumers have been updated to accommodate the newpost_typefield.Action Items:
- Review Test Cases and API Endpoints: Confirm that all tests and API endpoints handling
analytics_hits_datanow account for the newpost_typefield.- Double-check Downstream Integrations: Verify that dashboards, integrations, or any external systems consuming this data have been updated for the schema change.
|
This was an experiment / demo that I don't need anymore |
Got some code for us? Awesome 🎊!
Please include a description of your change & check your PR against this list, thanks!
yarn test:allandyarn lint)We appreciate your contribution!