Feature/order keys #262
Feature/order keys #262
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe pull request introduces a maintenance mode feature for the start app, removes session properties from event/session handling across database services, adds a ClickHouse migration script and migration tooling enhancements, refines worker logging and ping behavior, tightens track controller validation, and standardizes Docker Compose formatting and image pins. Changes
Sequence DiagramsequenceDiagram
participant Env as Environment (MAINTENANCE)
participant Server as getServerEnvs
participant Hook as useAppContext
participant Router as Router Context
participant Layout as AppLayout
participant UI as Renderer
Env->>Server: Read MAINTENANCE flag
Server->>Hook: Provide isMaintenance boolean
Hook->>Router: Include isMaintenance in context
Router->>Layout: useAppContext() -> isMaintenance
alt isMaintenance = true
Layout->>UI: Render FullPageEmptyState (maintenance)
else isMaintenance = false
Layout->>UI: Render normal app layout
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
🧹 Nitpick comments (6)
apps/start/src/server/get-envs.ts (1)
4-15: Consider adding Sentry instrumentation to this server function.Per the coding guidelines for
apps/start/**/*.{ts,tsx}, server functions should be instrumented with Sentry usingSentry.startSpan(). This is a pre-existing gap, but worth addressing.+import * as Sentry from '@sentry/tanstackstart-react'; + export const getServerEnvs = createServerFn().handler(async () => { - const envs = { - apiUrl: String(process.env.API_URL || process.env.NEXT_PUBLIC_API_URL), - dashboardUrl: String( - process.env.DASHBOARD_URL || process.env.NEXT_PUBLIC_DASHBOARD_URL, - ), - isSelfHosted: process.env.SELF_HOSTED !== undefined, - isMaintenance: process.env.MAINTENANCE === '1', - }; - - return envs; + return Sentry.startSpan({ name: 'getServerEnvs' }, () => { + const envs = { + apiUrl: String(process.env.API_URL || process.env.NEXT_PUBLIC_API_URL), + dashboardUrl: String( + process.env.DASHBOARD_URL || process.env.NEXT_PUBLIC_DASHBOARD_URL, + ), + isSelfHosted: process.env.SELF_HOSTED !== undefined, + isMaintenance: process.env.MAINTENANCE === '1', + }; + return envs; + }); });apps/start/src/routes/_app.tsx (1)
3-3: Remove unused imports.
ButtonandLinkButtonare imported but not used. OnlybuttonVariantsis utilized for styling the external anchor tag.-import { Button, LinkButton, buttonVariants } from '@/components/ui/button'; +import { buttonVariants } from '@/components/ui/button';self-hosting/docker-compose.template.yml (1)
130-131: Consider single-line formatting for consistency.While valid, this healthcheck is split across two lines whereas all others (lines 31, 54, 77, 102, 150) use single-line format.
Apply this diff for consistency:
healthcheck: - test: - ["CMD-SHELL", "curl -f http://localhost:3000/api/healthcheck || exit 1"] + test: ["CMD-SHELL", "curl -f http://localhost:3000/api/healthcheck || exit 1"] interval: 10spackages/db/src/services/event.service.ts (1)
630-630: Consider adding secondary sort key for deterministic ordering.Simplifying the ordering to only
created_at DESCcould lead to non-deterministic results when multiple events share the same timestamp. While the cursor logic (lines 478-479) only usescreated_atfor comparison, addingid DESCas a secondary sort key would ensure consistent ordering across queries.Apply this diff to add deterministic ordering:
- sb.orderBy.created_at = 'created_at DESC'; + sb.orderBy.created_at = 'created_at DESC'; + sb.orderBy.id = 'id DESC';packages/db/src/clickhouse/migration.ts (2)
255-290: Consider adding unit tests for the interval-based batching logic.The
getWeekStartandshouldContinuehelpers introduce complexity with different termination semantics per interval type. While the logic appears correct, edge cases (month/year boundaries, DST transitions, week-spanning dates) would benefit from test coverage.
329-333: Defensive check appears unnecessary.Since
upperBoundDateis initialized fromcurrentDate, which starts atendDateand only decreases, the conditionupperBoundDate > endDateshould never be true. Consider removing this dead code or documenting when it could apply.- let upperBoundDate = currentDate; - // Don't exceed the endDate - if (upperBoundDate > endDate) { - upperBoundDate = endDate; - } + const upperBoundDate = currentDate;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/api/src/controllers/track.controller.ts(2 hunks)apps/start/src/hooks/use-app-context.ts(1 hunks)apps/start/src/routes/__root.tsx(1 hunks)apps/start/src/routes/_app.tsx(2 hunks)apps/start/src/server/get-envs.ts(1 hunks)apps/worker/src/boot-workers.ts(1 hunks)apps/worker/src/jobs/cron.ping.ts(1 hunks)apps/worker/src/jobs/events.create-session-end.ts(0 hunks)apps/worker/src/jobs/events.incoming-events.test.ts(0 hunks)packages/db/code-migrations/8-order-keys.ts(1 hunks)packages/db/code-migrations/helpers.ts(1 hunks)packages/db/code-migrations/migrate.ts(3 hunks)packages/db/src/buffers/session-buffer.ts(0 hunks)packages/db/src/clickhouse/migration.ts(3 hunks)packages/db/src/services/event.service.ts(1 hunks)packages/db/src/services/session.service.ts(1 hunks)packages/queue/src/queues.ts(1 hunks)self-hosting/docker-compose.template.yml(10 hunks)
💤 Files with no reviewable changes (3)
- apps/worker/src/jobs/events.create-session-end.ts
- apps/worker/src/jobs/events.incoming-events.test.ts
- packages/db/src/buffers/session-buffer.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
When writing ClickHouse queries, always use the custom query builder from
./packages/db/src/clickhouse/query-builder.tsand./packages/db/src/clickhouse/query-functions.ts
Files:
apps/worker/src/boot-workers.tspackages/db/code-migrations/helpers.tsapps/api/src/controllers/track.controller.tspackages/db/src/services/session.service.tsapps/worker/src/jobs/cron.ping.tsapps/start/src/server/get-envs.tspackages/queue/src/queues.tspackages/db/code-migrations/migrate.tspackages/db/src/clickhouse/migration.tsapps/start/src/routes/__root.tsxapps/start/src/hooks/use-app-context.tspackages/db/src/services/event.service.tsapps/start/src/routes/_app.tsxpackages/db/code-migrations/8-order-keys.ts
apps/start/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/start/.cursorrules)
Instrument server functions with Sentry using
Sentry.startSpan()to wrap the implementation. Import Sentry with:import * as Sentry from '@sentry/tanstackstart-react'
Files:
apps/start/src/server/get-envs.tsapps/start/src/routes/__root.tsxapps/start/src/hooks/use-app-context.tsapps/start/src/routes/_app.tsx
🧠 Learnings (6)
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/__root.tsx : Use `createRootRouteWithContext<T>()` to provide typed context to the router that can be accessed in loaders and other route functions
Applied to files:
apps/start/src/routes/__root.tsxapps/start/src/routes/_app.tsx
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/src/router.tsx : Error collection is automatically configured in `src/router.tsx` - do not duplicate Sentry error collection configuration elsewhere
Applied to files:
apps/start/src/routes/__root.tsx
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use pathless layout routes by prefixing filenames with `_` (e.g., `_settingsShell`) to wrap children without matching URL segments
Applied to files:
apps/start/src/routes/_app.tsx
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use `<Outlet/>` component in layout routes to render child route components
Applied to files:
apps/start/src/routes/_app.tsx
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Un-nest routes by suffixing a segment with `_` (e.g., `posts_.$postId.edit.tsx`) to render independently from parent routes
Applied to files:
apps/start/src/routes/_app.tsx
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/__root.tsx : Create a root route using `createRootRoute()` that is always matched and wraps all other routes
Applied to files:
apps/start/src/routes/_app.tsx
🧬 Code graph analysis (4)
apps/worker/src/boot-workers.ts (1)
packages/queue/src/queues.ts (1)
queueLogger(29-29)
apps/api/src/controllers/track.controller.ts (2)
apps/api/src/utils/errors.ts (1)
HttpError(17-39)packages/sdks/sdk/src/index.ts (1)
identify(145-163)
packages/db/code-migrations/migrate.ts (1)
packages/db/code-migrations/helpers.ts (3)
getIsDry(30-32)printBoxMessage(1-15)getShouldIgnoreRecord(34-36)
apps/start/src/routes/_app.tsx (2)
apps/start/src/hooks/use-app-context.ts (1)
useAppContext(3-22)apps/public/src/components/ui/button.tsx (1)
buttonVariants(65-65)
🔇 Additional comments (23)
apps/start/src/server/get-envs.ts (1)
11-11: LGTM!The
isMaintenanceflag is correctly derived from the environment variable with a strict equality check against'1'.apps/start/src/routes/__root.tsx (1)
31-38: LGTM!The
isMaintenanceboolean is correctly added toMyRouterContext, following the established pattern for router context fields. Based on learnings, usingcreateRootRouteWithContext<T>()is the correct approach.apps/start/src/hooks/use-app-context.ts (1)
16-21: LGTM!The nullish coalescing operator (
?? false) provides a safe default, ensuring the app operates normally when maintenance mode isn't explicitly enabled. This is the correct approach for an opt-in feature.apps/start/src/routes/_app.tsx (1)
18-39: LGTM!The maintenance mode implementation is well-structured:
- Early return pattern keeps the component clean
- External link correctly uses an anchor tag with
target="_blank"and properrelattributes for security- The
buttonVariants()styling approach is appropriate for non-router links- User experience is good with icon, clear messaging, and actionable status page link
apps/api/src/controllers/track.controller.ts (2)
4-4: LGTM!The HttpError import is correctly added to support the new validation logic in the identify case.
191-197: Good addition—prevents undefined profileId from reaching the database.The validation correctly ensures that
profileIdis present before calling theidentifyfunction (which uses it as the profileidat line 308). This prevents potential database errors or incorrect behavior.The use of
HttpErrorprovides structured error handling, though it differs slightly from thereply.status(400).send()pattern used elsewhere in this handler (lines 110-116, 208-212). Both approaches are valid; thethrowpattern allows error middleware to handle responses consistently.packages/queue/src/queues.ts (1)
149-149: LGTM: Consistent logger behavior with worker initialization.The conditional logger assignment matches the pattern in
apps/worker/src/boot-workers.ts(line 120), ensuring consistent logging behavior across queue initialization and worker instantiation. Both now suppress per-queue/per-worker logging in non-production environments.apps/worker/src/boot-workers.ts (1)
120-120: Logger now production-only for GroupWorker.The logger is conditionally passed only in production environments, matching the pattern used in
packages/queue/src/queues.ts. This is safe because groupmq's Worker accepts an optional logger, and the global logger in the parent scope captures all critical worker lifecycle events (errors, job failures, completions) regardless of NODE_ENV.self-hosting/docker-compose.template.yml (5)
1-9: LGTM: Formatting standardization.The version field quoting and port formatting changes are valid YAML and improve consistency across the file.
31-31: LGTM: Healthcheck and command array formatting.The conversion to double-quoted inline arrays for PostgreSQL and Redis healthchecks and commands is syntactically correct and improves consistency.
Also applies to: 52-52, 54-54
77-77: LGTM: ClickHouse healthcheck formatting.The mixed quoting (double quotes for array, single quotes for the SQL query) is correct and necessary to preserve the query string.
163-175: LGTM: Volume definitions.All volume definitions are correctly formatted with consistent
driver: localdeclarations.
68-68: ClickHouse image version 25.10.2.65 is valid and actively maintained.The specified version exists on Docker Hub with multi-architecture support (amd64, arm64) and is currently in active use. No newly introduced vulnerabilities are reported for this release. Version pinning is good practice for reproducibility.
packages/db/src/services/event.service.ts (1)
103-149: Removal of session properties is safe – only a single caller (apps/worker/src/jobs/events.create-session-end.ts) usestransformSessionToEvent, and that code only sorts events bycreatedAtwithout accessing properties derived from sessions. No downstream dependencies on session properties in events were found.packages/db/code-migrations/helpers.ts (1)
33-36: LGTM!The new helper follows the same pattern as
getIsDry()and provides a clean way to skip migration recording when needed.packages/db/code-migrations/migrate.ts (2)
58-64: LGTM!Skipping the 10-second delay in dry-run mode is a sensible UX improvement for testing migrations without waiting.
89-101: LGTM!The combined condition correctly gates the migration record upsert, allowing migrations to be re-run without updating the migration table when using
--no-record.packages/db/src/clickhouse/migration.ts (2)
216-239: LGTM! Thecolumnsparameter is a valuable addition.This enables selective column migration, useful when source and target tables have different schemas (as used in the sessions migration in
8-order-keys.ts).
341-356: Termination logic for month/week intervals looks correct.The explicit break conditions ensure the loop terminates after processing the period containing
startDate, preventing an infinite loop whenshouldContinueand the decrement logic would otherwise cycle.packages/db/code-migrations/8-order-keys.ts (4)
17-71: LGTM! Events table schema with optimized ORDER BY.The new key ordering
[project_id, toDate(created_at), created_at, name]should improve query performance for project-scoped, time-filtered queries.
77-130: LGTM! Sessions table schema with optimized ORDER BY.Consistent ordering strategy with the events table, using
[project_id, toDate(created_at), created_at].
188-239: LGTM! Explicit column list for sessions migration.Using an explicit
columnsarray ensures only the desired columns are migrated, which is important when the source table might have columns that don't exist in the target schema.
278-293: LGTM! SQL file persistence and dry-run support.Writing generated SQL to a file provides useful audit trails, and respecting
--dryallows safe testing of migration generation without execution.
| if (process.env.DISABLE_PING) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant lines
cat -n apps/worker/src/jobs/cron.ping.ts | head -20Repository: Openpanel-dev/openpanel
Length of output: 787
🏁 Script executed:
# Search for other DISABLE_PING references in the codebase
rg "DISABLE_PING" --type ts --type tsx --type js --type jsx -B 2 -A 2Repository: Openpanel-dev/openpanel
Length of output: 94
🏁 Script executed:
# Search for similar environment variable patterns in the codebase to understand conventions
rg "process\.env\." --type ts --type tsx --type js --type jsx | grep -E "if.*process\.env\." | head -20Repository: Openpanel-dev/openpanel
Length of output: 94
🏁 Script executed:
# Search for DISABLE_PING in all files (any type)
rg "DISABLE_PING" -B 2 -A 2Repository: Openpanel-dev/openpanel
Length of output: 318
🏁 Script executed:
# Check how other boolean-style env vars are handled in this codebase
rg "process\.env\." -A 0 | grep -E "(if|when|process\.env\.)" | head -30Repository: Openpanel-dev/openpanel
Length of output: 2996
🏁 Script executed:
# Look for documentation files that might mention DISABLE_PING
fd -e md -e txt | xargs grep -l "DISABLE_PING" 2>/dev/null || echo "No matches found"Repository: Openpanel-dev/openpanel
Length of output: 83
🏁 Script executed:
# Check environment variable patterns in the worker directory
rg "process\.env\." apps/worker/src -B 1 -A 1 | head -50Repository: Openpanel-dev/openpanel
Length of output: 2107
Use consistent === undefined check for disable flag, matching codebase pattern.
The check process.env.DISABLE_PING uses JavaScript truthiness, which means any non-empty string (including "false" or "0") will disable the ping. More importantly, other disable flags in the codebase (DISABLE_BULLBOARD, DISABLE_WORKERS) check === undefined instead. Update to match:
-if (process.env.DISABLE_PING) {
+if (process.env.DISABLE_PING !== undefined) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (process.env.DISABLE_PING) { | |
| return; | |
| } | |
| if (process.env.DISABLE_PING !== undefined) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In apps/worker/src/jobs/cron.ping.ts around lines 4-6, replace the truthy
environment check (if (process.env.DISABLE_PING) { return; }) with an explicit
undefined check so the flag is only considered set when the env var is present:
change the condition to if (process.env.DISABLE_PING !== undefined) { return; }
to match the codebase pattern of using ===/!== undefined for disable flags.
| sb.orderBy.created_at = 'created_at DESC'; | ||
| } else { | ||
| sb.orderBy.created_at = 'toDate(created_at) DESC, created_at DESC, id DESC'; | ||
| sb.orderBy.created_at = 'created_at DESC'; |
There was a problem hiding this comment.
Critical: ORDER BY must match cursor pagination logic.
The cursor logic on line 200 constructs a WHERE clause that assumes ordering by both created_at and id:
(created_at < cursor_date OR (created_at = cursor_date AND id < cursor_id))However, the ORDER BY clause only includes created_at DESC, missing id DESC. This mismatch breaks cursor-based pagination because:
- Items with identical
created_attimestamps can appear in any order - The cursor's
idcomparison becomes meaningless withoutidin ORDER BY - This can cause items to be skipped or duplicated across pages
Apply this diff to fix the ordering:
if (cursor) {
const cAt = sqlstring.escape(cursor.createdAt);
const cId = sqlstring.escape(cursor.id);
sb.where.cursor = `(created_at < toDateTime64(${cAt}, 3) OR (created_at = toDateTime64(${cAt}, 3) AND id < ${cId}))`;
sb.where.cursorWindow = `created_at >= toDateTime64(${cAt}, 3) - INTERVAL ${dateIntervalInDays} DAY`;
sb.orderBy.created_at = 'created_at DESC';
+ sb.orderBy.id = 'id DESC';
} else {
sb.orderBy.created_at = 'created_at DESC';
+ sb.orderBy.id = 'id DESC';
sb.where.created_at = `created_at > now() - INTERVAL ${dateIntervalInDays} DAY`;
}🤖 Prompt for AI Agents
In packages/db/src/services/session.service.ts around lines 202 to 204, the
ORDER BY currently only uses created_at DESC which mismatches the cursor WHERE
clause that expects ordering by created_at then id; update the ORDER BY to
include id DESC as a secondary sort (e.g., created_at DESC, id DESC) so the
ordering matches the cursor comparison and prevents skipped or duplicated items
in cursor-based pagination.
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)
self-hosting/docker-compose.template.yml (1)
92-92: Pin image versions or make them configurable to ensure reproducible deployments.Using
:latesttags for production services (op-api, op-dashboard, op-worker) creates non-reproducible deployments where different pulls can retrieve different versions, making rollbacks difficult and debugging version-specific issues nearly impossible.Apply this pattern to make versions explicit and configurable:
- image: lindesvard/openpanel-api:latest + image: ${OPENPANEL_API_VERSION:-lindesvard/openpanel-api:1.2.1}- image: lindesvard/openpanel-dashboard:latest + image: ${OPENPANEL_DASHBOARD_VERSION:-lindesvard/openpanel-dashboard:1.2.2}- image: lindesvard/openpanel-worker:latest + image: ${OPENPANEL_WORKER_VERSION:-lindesvard/openpanel-worker:1.2.1}Then document the version variables in your
.env.examplefile. This allows users to explicitly control versions while providing sensible defaults.Also applies to: 122-122, 142-142
🧹 Nitpick comments (1)
self-hosting/docker-compose.template.yml (1)
1-1: Consider omitting or specifying exact Compose file version.The bare
version: "3"is deprecated. Modern Docker Compose (v2+) does not require a version key, or you can specify a precise version like"3.8"for clarity if targeting older Compose runtimes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/db/code-migrations/8-order-keys.ts(1 hunks)packages/db/src/services/session.service.ts(1 hunks)self-hosting/docker-compose.template.yml(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/code-migrations/8-order-keys.ts
- packages/db/src/services/session.service.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). (3)
- GitHub Check: build-and-push-api
- GitHub Check: build-and-push-dashboard
- GitHub Check: build-and-push-worker
🔇 Additional comments (2)
self-hosting/docker-compose.template.yml (2)
8-9: LGTM: YAML formatting standardization.The changes consistently apply double-quote strings and JSON-style array notation throughout healthchecks, commands, and volume definitions. These improve readability and maintainability without affecting functionality.
Also applies to: 31-31, 52-52, 54-54, 77-77, 102-102, 130-131, 150-150, 175-175
68-68: Verify ClickHouse compatibility configuration and upgrade migration handling for version 25.10.Upgrading from 24.3.2 to 25.10.2.65 introduces breaking changes that require explicit compatibility configuration. ClickHouse 25.10 breaks compatibility with tables containing Variant/Dynamic/JSON data types, and enables a new serialization format that prevents downgrading to versions before 25.10. Ensure that:
- Server configuration explicitly sets
escape_variant_subcolumn_filenamesandserialization_info_version/string_serialization_versionfor backward compatibility if needed- Migration scripts handle any schema or data format changes between versions
- Upgrade is tested in staging first (major version jumps can introduce configuration failures)
Switching from Alpine to the standard image significantly increases container size. Document this trade-off if there's no specific performance or compatibility reason for the change.
Summary by CodeRabbit
New Features
Bug Fixes
Data & Migrations
Infrastructure & Deployment
✏️ Tip: You can customize this high-level summary in your review settings.