Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces timezone support to analytics queries by adding an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Route
participant Query_Engine
participant DB
Client->>API_Route: Request with query params (may include timezone)
API_Route->>Query_Engine: executeQuery(request, websiteDomain, timezone)
Query_Engine->>Query_Engine: Validate & extend request with timezone
Query_Engine->>Query_Engine: Build query (pass timezone to customSql)
Query_Engine->>DB: Execute SQL with timezone-aware date functions
DB-->>Query_Engine: Return results
Query_Engine-->>API_Route: Return processed data
API_Route-->>Client: Respond with results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/src/query/index.ts (1)
43-43: Same timezone fallback issue as in executeQuery.The fallback logic
timezone ?? validated.timezonehas the same issue wherevalidated.timezonewill beundefinedsince theQuerySchemadoesn't include a timezone field.
🧹 Nitpick comments (6)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx (1)
289-294: Consider performance implications of Object.assign in loopThe change from
.reduce()to.forEach()withObject.assign()is functionally correct but creates new objects on each iteration, which could impact performance with large datasets.For better performance with large datasets, consider:
- const data: Record<string, any> = {}; - performanceResults - .filter(result => result.success && result.data) - .forEach(result => { - Object.assign(data, result.data); - }); + const data = performanceResults + .filter(result => result.success && result.data) + .reduce((acc, result) => ({ ...acc, ...result.data }), {} as Record<string, any>);Or for maximum performance:
+ const data: Record<string, any> = {}; + performanceResults + .filter(result => result.success && result.data) + .forEach(result => { + Object.assign(data, result.data); + });apps/api/biome.json (1)
19-25: Avoid duplicating theassistoverride unless an override is really neededBecause the monorepo root
biome.jsonalready disablesorganizeImports, repeating the same flag in this nested config is redundant and increases maintenance overhead. Deleting these lines would let the file inherit the root setting.- "assist": { - "actions": { - "source": { - "organizeImports": "off" - } - } - },.vscode/settings.json (1)
2-2: Consider using forward slashes for cross-platform compatibility.The Windows-style backslashes may cause issues on Unix-based systems. Node.js and most tools accept forward slashes on all platforms.
- "typescript.tsdk": "node_modules\\typescript\\lib" + "typescript.tsdk": "node_modules/typescript/lib"apps/api/src/query/builders/devices.ts (1)
4-4: Consider adding explicit type annotation for consistency.For consistency with
GeoBuilders, consider adding the explicit type parameter:-export const DevicesBuilders: Record<string, SimpleQueryConfig> = { +export const DevicesBuilders: Record<string, SimpleQueryConfig<typeof Analytics.events>> = {apps/api/src/query/builders/sessions.ts (1)
123-123: Consider updating hardcoded table names in customSql.The customSql function still uses hardcoded
'analytics.events'table names in the SQL strings. For consistency and maintainability, consider using a template variable or constant that can be dynamically replaced.This could be addressed by either:
- Using template variables in the SQL string
- Creating a helper function that injects the correct table name
- Using string interpolation with the Analytics.events constant
Also applies to: 152-152
apps/api/src/query/builders/pages.ts (1)
54-54: Consider updating hardcoded table names in customSql functions.The customSql functions still use hardcoded
'analytics.events'table names in the SQL strings. For consistency and maintainability, consider using a template variable or constant that can be dynamically replaced.This could be addressed by using string interpolation or template variables to reference the centralized table name.
Also applies to: 111-111, 124-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.gitignore(1 hunks).vscode/settings.json(1 hunks)apps/api/biome.json(2 hunks)apps/api/src/query/builders/devices.ts(11 hunks)apps/api/src/query/builders/errors.ts(5 hunks)apps/api/src/query/builders/geo.ts(5 hunks)apps/api/src/query/builders/pages.ts(4 hunks)apps/api/src/query/builders/performance.ts(8 hunks)apps/api/src/query/builders/profiles.ts(6 hunks)apps/api/src/query/builders/sessions.ts(7 hunks)apps/api/src/query/builders/summary.ts(9 hunks)apps/api/src/query/builders/traffic.ts(5 hunks)apps/api/src/query/index.ts(1 hunks)apps/api/src/query/simple-builder.ts(1 hunks)apps/api/src/query/types.ts(2 hunks)apps/api/src/routes/query.ts(1 hunks)apps/api/src/types/tables.ts(1 hunks)apps/basket/src/routes/basket.ts(12 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx(5 hunks)apps/dashboard/components/autumn/pricing-table.tsx(3 hunks)apps/docs/components/landing/hero.tsx(0 hunks)biome.json(1 hunks)index.ts(0 hunks)
💤 Files with no reviewable changes (2)
- apps/docs/components/landing/hero.tsx
- index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (46)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx (2)
3-3: LGTM: Clean icon import renameThe rename from
QuestiontoQuestionIconimproves clarity and avoids potential naming conflicts.
150-150: LGTM: Consistent icon usage updateThe icon usage correctly reflects the import rename.
biome.json (1)
24-30: Verify Biome ≥ 2.1 Support for the NewassistBlockIt looks like we didn’t find any Biome dependency in your
package.json, so it’s unclear which version of Biome your CI or local tooling is running. Since theassist.actions.source.organizeImports = "off"flag is only recognized in Biome 2.1 and above, please confirm that:
- Your local installs (e.g. via
npm install,pnpm install, etc.) pull in Biome ≥ 2.1- Your CI workflows or any global installs of Biome are updated to ≥ 2.1
You can verify by running:
biome --version # or, if installed via npm: npx biome --versionIf you’re pinned to an older release, you’ll need to upgrade to avoid schema errors or ignored settings.
apps/api/biome.json (1)
11-13: Includes block reformatted – looks goodPure formatting change from one-liner to multi-line array; no functional impact.
apps/dashboard/components/autumn/pricing-table.tsx (3)
209-209: Good practice: Usingnullfor conditional rendering.This change from an empty React fragment to
nullis semantically correct and follows React best practices for early returns when no content should be rendered.
269-269: Good cleanup: Removing unused destructured variable.Removing
itemsfrom the destructuring is appropriate since it's not used as a standalone variable in this scope. The code still correctly accessesproduct.itemsdirectly where needed.
508-508: Verify accessibility impact of removingaria-labelledby.The removal of the
aria-labelledbyattribute may affect accessibility for screen readers. While the Switch component has its ownaria-label(line 514), the connection to the descriptive label above (id="billing-interval-label"on line 505) has been lost.Please confirm that this removal doesn't negatively impact accessibility. Consider whether the Switch component's individual
aria-labelprovides sufficient context or if the connection to the descriptive text should be maintained.apps/api/src/types/tables.ts (1)
1-31: Excellent type safety infrastructure.This new module provides a well-structured, centralized approach to analytics table references with several benefits:
- Type Safety: Replaces magic strings with typed constants, preventing typos
- Centralization: Single source of truth for table names
- IDE Support: Better autocomplete and refactoring capabilities
- Field Mapping:
TableFieldsMapenables type-safe field access- Consistency: Uniform naming pattern across all analytics tables
The use of
as constand proper TypeScript patterns demonstrates strong understanding of type system best practices.apps/api/src/query/builders/performance.ts (1)
1-1: Excellent refactoring: Improved type safety with centralized table references.This systematic replacement of hardcoded table name strings with typed constants from the
Analyticsobject provides several benefits:
- Type Safety: Compile-time checking prevents table name typos
- Maintainability: Single source of truth for table names
- Consistency: Uniform approach across all query builders
- IDE Support: Better autocomplete and refactoring capabilities
The refactoring is applied consistently across all performance query configurations without altering any business logic.
Also applies to: 6-6, 24-24, 47-47, 70-70, 99-99, 123-123, 146-146, 169-169
apps/basket/src/routes/basket.ts (4)
151-151: Good enhancement: Event enrichment with geolocation and user agent data.The function signature updates and enrichment logic provide valuable analytics metadata:
- Consistent Pattern: Mirrors the enrichment approach used in
insertTrackEvent- Geolocation Data: Adds country/region information for analytics
- User Agent Parsing: Extracts browser, OS, and device information
- Performance: Uses
Date.now()instead ofnew Date().getTime()The enriched fields will enable more detailed analytics reporting and user behavior analysis.
Also applies to: 161-170, 198-207
228-230: Consistent enhancement: Web vitals enrichment follows established pattern.The
insertWebVitalsfunction updates mirror the improvements made toinsertError:
- Contextual Data: Adds geolocation and user agent information to performance metrics
- Analytics Value: Enables correlation of web vitals with user demographics and device types
- Consistency: Follows the same enrichment pattern across event types
This enhancement will provide richer context for performance analysis and optimization efforts.
Also applies to: 241-250, 268-276
594-594: Complete function call updates.All call sites for
insertErrorandinsertWebVitalshave been properly updated to pass the newuserAgentandipparameters. The updates cover both single event processing and batch processing routes, ensuring no runtime errors from signature mismatches.Also applies to: 613-613, 715-715, 752-752
161-161: Good practice: Standardized timestamp generation.The consistent use of
Date.now()instead ofnew Date().getTime()across all event insertion functions provides:
- Performance: More efficient timestamp generation
- Consistency: Uniform approach across the codebase
- Readability: Cleaner, more direct API usage
Also applies to: 241-241, 318-318, 469-469
.gitignore (1)
14-14: Standard practice: Ignoring IDE-specific directories.Adding
.vscodeto the gitignore is recommended practice for preventing Visual Studio Code workspace settings and metadata from being version controlled. This avoids conflicts between developers' individual IDE configurations and keeps the repository clean.apps/api/src/routes/query.ts (1)
246-250: LGTM! Timezone support properly integrated.The timezone parameter is correctly extracted from query parameters and passed through to both the query request object and the executeQuery function call. This completes the timezone support integration at the route level.
apps/api/src/query/simple-builder.ts (1)
68-69: LGTM! Timezone parameter correctly passed to customSql.The timezone from the request is properly passed as the eighth argument to the customSql function, enabling timezone-aware query compilation. The change is consistent with the broader timezone support implementation.
apps/api/src/query/builders/geo.ts (3)
2-2: LGTM! Type safety improvement with centralized table references.
4-4: LGTM! Enhanced type annotation for better type safety.The explicit type annotation with
SimpleQueryConfig<typeof Analytics.events>provides better type checking and IntelliSense support.
6-6: LGTM! Consistent replacement of hardcoded table names.All hardcoded
'analytics.events'strings have been replaced with the typedAnalytics.eventsconstant, improving maintainability and type safety across all geo query builders.Also applies to: 24-24, 41-41, 58-58, 75-75
apps/api/src/query/builders/devices.ts (2)
1-1: LGTM! Proper import of centralized table constants.
6-6: LGTM! Consistent replacement of hardcoded table names.All hardcoded
'analytics.events'strings have been systematically replaced with the typedAnalytics.eventsconstant across all device query builders, improving maintainability and type safety.Also applies to: 23-23, 40-40, 56-56, 72-72, 96-96, 114-114, 131-131, 147-147, 164-164, 180-180
apps/api/src/query/builders/errors.ts (3)
2-2: LGTM: Import added for centralized table definitions.Good addition of the Analytics import to support typed table references.
4-4: LGTM: Enhanced type safety with parameterized SimpleQueryConfig.The type annotation improvement from generic
SimpleQueryConfigtoSimpleQueryConfig<typeof Analytics.errors>provides better type safety by explicitly linking query configurations to the errors table schema.
6-6: LGTM: Consistent replacement of string literals with typed constants.All table references have been consistently updated from
'analytics.errors'string literals to the typedAnalytics.errorsconstant, improving maintainability and type safety.Also applies to: 27-27, 44-44, 58-58, 74-74
apps/api/src/query/builders/traffic.ts (3)
2-2: LGTM: Import added for centralized table definitions.Consistent with other query builders, adding the Analytics import to support typed table references.
4-4: LGTM: Enhanced type safety with parameterized SimpleQueryConfig.The type annotation improvement from generic
SimpleQueryConfigtoSimpleQueryConfig<typeof Analytics.events>provides better type safety by explicitly linking query configurations to the events table schema.
6-6: LGTM: Consistent replacement of string literals with typed constants.All table references have been consistently updated from
'analytics.events'string literals to the typedAnalytics.eventsconstant, improving maintainability and type safety across all traffic-related query builders.Also applies to: 37-37, 54-54, 71-71, 88-88
apps/api/src/query/builders/sessions.ts (3)
2-2: LGTM: Import added for centralized table definitions.Consistent with other query builders, adding the Analytics import to support typed table references.
4-4: LGTM: Enhanced type safety with parameterized SimpleQueryConfig.The type annotation improvement provides better type safety by explicitly linking query configurations to the events table schema.
6-6: LGTM: Consistent replacement of string literals with typed constants.All simple query builder table references have been consistently updated from
'analytics.events'string literals to the typedAnalytics.eventsconstant.Also applies to: 20-20, 42-42, 58-58, 75-75, 91-91, 189-189
apps/api/src/query/builders/pages.ts (3)
2-2: LGTM: Import added for centralized table definitions.Consistent with other query builders, adding the Analytics import to support typed table references.
4-4: LGTM: Enhanced type safety with parameterized SimpleQueryConfig.The type annotation improvement provides better type safety by explicitly linking query configurations to the events table schema.
6-6: LGTM: Consistent replacement of string literals with typed constants.All simple query builder table references have been consistently updated from
'analytics.events'string literals to the typedAnalytics.eventsconstant.Also applies to: 23-23, 83-83, 153-153
apps/api/src/query/types.ts (2)
52-54: LGTM: Added timezone support to customSql function signature.The addition of the optional
timezone?: stringparameter maintains backward compatibility while enabling timezone-aware queries. The placement afteroffsetfollows a logical parameter ordering.
69-69: Timezone support flows correctly through the pipeline.Route handlers in apps/api/src/routes/query.ts extract the
timezonefrom headers or URL, pass it intoexecuteQuery/compileQueryin apps/api/src/query/index.ts, and theSimpleQueryBuilderis initialized with that value (falling back torequest.timezone). No further changes required.apps/api/src/query/index.ts (2)
23-23: Function signature correctly updated for timezone support.The optional
timezoneparameter is properly typed and positioned.
35-35: Function signature correctly updated for timezone support.Consistent with
executeQueryfunction signature.apps/api/src/query/builders/profiles.ts (3)
2-2: Good addition of typed table references.Importing
Analyticsfrom the centralized tables module improves type safety and consistency.
4-4: Excellent type safety improvement.The explicit typing with
SimpleQueryConfig<typeof Analytics.events>provides better type checking and ensures consistency across all profile query configurations.
117-117: Consistent table reference improvements.All table references have been correctly updated from string literals to the typed
Analytics.eventsconstant. This eliminates magic strings and improves maintainability.Also applies to: 131-131, 153-153, 169-169, 186-186
apps/api/src/query/builders/summary.ts (6)
2-2: Consistent import pattern.The Analytics import follows the same pattern as other query builder files, maintaining consistency across the codebase.
4-4: Type safety improvement.Consistent with other query builders, this provides better type checking for summary query configurations.
74-74: Table reference updated consistently.The table property correctly uses the typed
Analytics.eventsconstant instead of a string literal.
91-92: Well-implemented timezone parameter handling.The function signature properly adds the optional
timezoneparameter, and the fallback to 'UTC' is a sensible default for timezone-aware queries.
107-107: Correct timezone application in SQL time functions.All time-based functions (
toStartOfHour,toDate) are properly wrapped withtoTimeZone(..., {timezone:String})to ensure date calculations respect the specified timezone. The implementation is consistent across both hourly and daily query branches.Also applies to: 128-128, 179-179, 200-200
163-163: Timezone parameter correctly added to SQL params.The
timezone: tzparameter is properly included in both the hourly and daily query parameter objects, ensuring the SQL queries receive the timezone value.Also applies to: 235-235
1 participant
Pull Request
Description
Please include a summary of the change and which issue is fixed. Also include relevant motivation and context.
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores