refactor: consolidate ClickHouse configuration into single CLICKHOUSE… #2607

LeoKaynan · 2025-10-15T04:34:42Z

Summary

This PR consolidates redundant ClickHouse environment variables and improves the replication service initialization logic. ClickHouse remains required (as intended since PR #2264).

Root Cause: The dashboard always queried ClickHouse by default but did not show the runs because RUN_REPLICATION_CLICKHOUSE_URL should be defined.

Background & Context

PR #2035 - Original ClickHouse Implementation (May 2024)

Introduced PostgreSQL → ClickHouse logical replication
Initially used a single CLICKHOUSE_URL for all operations

PR #2264 - Bulk Actions 2.0 (July 2024)

Migrated all run listings to ClickHouse (NextRunListPresenter)
Made CLICKHOUSE_URL required - correct strategic decision
Quote: "it is definitely required after this PR" - @matt-aitken

Current State

At some point, RUN_REPLICATION_CLICKHOUSE_URL was introduced as a duplicate, creating unnecessary complexity.

Changes

1. Remove Redundant `RUN_REPLICATION_CLICKHOUSE_URL`

Consolidate back to single CLICKHOUSE_URL (original feat: replicate task runs to clickhouse to power dashboard improvements #2035 design)
Both read and write operations use the same URL
No architectural benefit to having separate URLs (they pointed to same instance)

Benefits:

✅ Semantic correctness: feature flag → configuration dependency
✅ Avoids creating objects unnecessarily when disabled
✅ Clearer log messages
✅ Removes redundant nested conditional

✅ Checklist

I have followed every step in the contributing guide
The PR title follows the convention.
I ran and tested the code works

Testing

I performed several tasks

Changelog

Screenshots

💯

…_URL variable

changeset-bot · 2025-10-15T04:34:46Z

⚠️ No Changeset found

Latest commit: 10f0a1c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

coderabbitai · 2025-10-15T04:34:55Z

Walkthrough

Adds CLICKHOUSE_URL to environment examples and replaces prior RUN_REPLICATION_CLICKHOUSE_URL references across app code, Docker, and Helm manifests. Updates the webapp environment schema by removing RUN_REPLICATION_CLICKHOUSE_URL and setting RUN_REPLICATION_ENABLED default to "1". Updates service initialization to check RUN_REPLICATION_ENABLED, returning early if not "1", and otherwise proceed with startup; signal handlers are attached unconditionally. Routes creating the replication service now use CLICKHOUSE_URL. Docker Compose and Helm templates remove the deprecated variable. Helm chart version is bumped from 4.0.4 to 4.0.5.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The description is missing the required “Closes #” line and provides only a vague testing section without detailed steps, and the Changelog heading is present but empty, so it does not fully follow the repository’s template.	Please add the “Closes #” line at the top, expand the Testing section with specific steps you performed, and include a summary under Changelog to fully comply with the template.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title clearly and concisely summarizes the primary change by indicating that ClickHouse configuration is being refactored to consolidate into a single environment variable.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 63b6fc9 and 138abc2.

📒 Files selected for processing (8)

.env.example (1 hunks)
apps/webapp/app/env.server.ts (1 hunks)
apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts (1 hunks)
apps/webapp/app/services/runsReplicationInstance.server.ts (2 hunks)
hosting/docker/.env.example (0 hunks)
hosting/docker/webapp/docker-compose.yml (0 hunks)
hosting/k8s/helm/Chart.yaml (1 hunks)
hosting/k8s/helm/templates/webapp.yaml (0 hunks)

💤 Files with no reviewable changes (3)

hosting/docker/webapp/docker-compose.yml
hosting/docker/.env.example
hosting/k8s/helm/templates/webapp.yaml

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts
apps/webapp/app/services/runsReplicationInstance.server.ts
apps/webapp/app/env.server.ts

{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts
apps/webapp/app/services/runsReplicationInstance.server.ts
apps/webapp/app/env.server.ts

apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts
apps/webapp/app/services/runsReplicationInstance.server.ts
apps/webapp/app/env.server.ts

{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly

Files:

apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts
apps/webapp/app/services/runsReplicationInstance.server.ts
apps/webapp/app/env.server.ts

apps/webapp/app/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead

Files:

apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts
apps/webapp/app/services/runsReplicationInstance.server.ts
apps/webapp/app/env.server.ts

🧠 Learnings (3)

📚 Learning: 2025-06-25T13:18:44.103Z

Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/Chart.yaml:1-18
Timestamp: 2025-06-25T13:18:44.103Z
Learning: For the Trigger.dev Helm chart, the chart name should be "trigger" (not "trigger-v4") since this is the first official chart release. Helper templates should use the actual chart name from .Chart.Name rather than hardcoded prefixes.

Applied to files:

hosting/k8s/helm/Chart.yaml

📚 Learning: 2025-06-25T13:20:17.174Z

Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/values.yaml:22-51
Timestamp: 2025-06-25T13:20:17.174Z
Learning: In the Trigger.dev Helm chart values.yaml, the maintainer prefers to use explicit comprehensive warnings for security-sensitive default values rather than implementing secure-by-default behavior that would fail installation. The project uses deterministic default secrets with clear "TESTING ONLY" warnings and instructions for production deployment.

Applied to files:

hosting/k8s/helm/Chart.yaml

📚 Learning: 2025-06-25T13:18:04.827Z

Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/templates/extra-manifests.yaml:1-4
Timestamp: 2025-06-25T13:18:04.827Z
Learning: In the Trigger.dev v4 Helm chart, the user prefers to rely on documentation and examples in values files rather than implementing defensive coding in templates, particularly for features like extraManifests where proper usage is documented.

Applied to files:

hosting/k8s/helm/Chart.yaml

🧬 Code graph analysis (2)

apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts (1)

apps/webapp/app/env.server.ts (1)

env (1202-1202)

apps/webapp/app/services/runsReplicationInstance.server.ts (4)

apps/webapp/app/env.server.ts (1)

env (1202-1202)

internal-packages/clickhouse/src/index.ts (1)

ClickHouse (61-172)

apps/webapp/app/services/signals.server.ts (1)

signalsEmitter (32-32)

apps/webapp/app/v3/marqs/index.server.ts (1)

signalsEmitter (155-158)

⏰ 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). (22)

GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
GitHub Check: typecheck / typecheck

🔇 Additional comments (10)

hosting/k8s/helm/Chart.yaml (1)

5-5: LGTM! Standard version bump.

The chart version increment aligns with the consolidation changes across the codebase.

.env.example (1)

24-26: LGTM! Clear documentation of the new variable.

The comment accurately describes CLICKHOUSE_URL's purpose, and the default value is appropriate for local development.

apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts (1)

81-81: LGTM! Correctly uses the consolidated CLICKHOUSE_URL.

The change aligns with the removal of RUN_REPLICATION_CLICKHOUSE_URL and correctly references env.CLICKHOUSE_URL for the ClickHouse client initialization.

apps/webapp/app/env.server.ts (2)

1080-1080: Verify the impact of enabling replication by default.

Changing RUN_REPLICATION_ENABLED to default to "1" means replication is now enabled by default. Combined with CLICKHOUSE_URL being required (Line 1106), this is a breaking change for existing deployments that don't have CLICKHOUSE_URL configured.

Ensure that:

Deployment documentation is updated to reflect that CLICKHOUSE_URL is now required

Migration guides are provided for existing deployments

The change aligns with the stated objective that "ClickHouse remains required"

Based on the PR objectives, this appears intentional. However, confirm that users who previously had replication disabled are prepared to provide CLICKHOUSE_URL.

1106-1106: LGTM! CLICKHOUSE_URL is now the single source for ClickHouse configuration.

The addition of CLICKHOUSE_URL as a required field consolidates the previously redundant RUN_REPLICATION_CLICKHOUSE_URL. This aligns with the PR objective to simplify configuration.

apps/webapp/app/services/runsReplicationInstance.server.ts (5)

18-21: LGTM! Cleaner guard logic with early return.

The flag-based check with an early return is more readable than the previous approach. The log message correctly indicates the service is disabled.

23-23: LGTM! Log message accurately reflects startup phase.

Changing from "service enabled" to "service starting" better represents the actual state at this point in the initialization.

26-26: LGTM! Correctly uses the consolidated CLICKHOUSE_URL.

The change from env.RUN_REPLICATION_CLICKHOUSE_URL to env.CLICKHOUSE_URL aligns with the PR objective to consolidate redundant environment variables.

71-80: LGTM! Simplified startup flow.

Removing the nested conditional around service.start() makes the code cleaner. Since the early return at Line 18-21 ensures the service is only created when enabled, the unconditional start call here is correct.

82-83: LGTM! Signal handlers correctly attached only when service exists.

The signal handlers are positioned after the early return guard (Lines 18-21), ensuring they're only attached when the service is created. This is the correct placement.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

nicktrn

Thanks @LeoKaynan but let's not touch the webapp here. We try to follow the same pattern as we do for redis so we can use different URLs for different services if needed.

It's fine to fix this for self-hosters if there are any bugs though!

One thing to consider is that people have been known to use old versions of the compose files. The clickhouse env vars are present in the latest version:

trigger.dev/hosting/docker/webapp/docker-compose.yml

Lines 64 to 69 in 63b6fc9

    
           # ClickHouse configuration 
        
           CLICKHOUSE_URL: ${CLICKHOUSE_URL:-http://default:password@clickhouse:8123?secure=false} 
        
           CLICKHOUSE_LOG_LEVEL: ${CLICKHOUSE_LOG_LEVEL:-info} 
        
           # Run replication 
        
           RUN_REPLICATION_ENABLED: ${RUN_REPLICATION_ENABLED:-1} 
        
           RUN_REPLICATION_CLICKHOUSE_URL: ${RUN_REPLICATION_CLICKHOUSE_URL:-http://default:password@clickhouse:8123}

If there are any helm issues it would be worth looking at this block:

trigger.dev/hosting/k8s/helm/templates/_helpers.tpl

Lines 420 to 435 in 63b6fc9

    
           {{/* 
        
           ClickHouse URL for replication (without secure parameter) 
        
           */}} 
        
           {{- define "trigger-v4.clickhouse.replication.url" -}} 
        
           {{- if .Values.clickhouse.deploy -}} 
        
           {{- $protocol := ternary "https" "http" .Values.clickhouse.secure -}} 
        
           {{ $protocol }}://{{ .Values.clickhouse.auth.username }}:{{ .Values.clickhouse.auth.password }}@{{ include "trigger-v4.clickhouse.hostname" . }}:8123 
        
           {{- else if .Values.clickhouse.external.host -}} 
        
           {{- $protocol := ternary "https" "http" .Values.clickhouse.external.secure -}} 
        
           {{- if .Values.clickhouse.external.existingSecret -}} 
        
           {{ $protocol }}://{{ .Values.clickhouse.external.username }}:${CLICKHOUSE_PASSWORD}@{{ .Values.clickhouse.external.host }}:{{ .Values.clickhouse.external.httpPort | default 8123 }} 
        
           {{- else -}} 
        
           {{ $protocol }}://{{ .Values.clickhouse.external.username }}:{{ .Values.clickhouse.external.password }}@{{ .Values.clickhouse.external.host }}:{{ .Values.clickhouse.external.httpPort | default 8123 }} 
        
           {{- end -}} 
        
           {{- end -}} 
        
           {{- end }}

LeoKaynan · 2025-10-15T09:56:46Z

Thanks for the feedback @nicktrn

I understand now that the separation of URLs is intentional to follow the Redis pattern and allow flexibility for different services. I'll close this PR.

The consolidation seemed like a simplification but I see the architectural reasoning behind keeping them separate for future scalability scenarios.

Thanks for clarifying! 👍

You might have to be careful here, uses ClickHouse by default:
https://github.dev/triggerdotdev/trigger.dev/blob/a445af1b7998b91c2ebe765104ff5faeb97780d9/apps/webapp/app/services/runsRepository/runsRepository.server.ts#L133

RUN_REPLICATION_CLICKHOUSE_URL is optional.

we need to update .env.example, can I open a PR for this?

refactor: consolidate ClickHouse configuration into single CLICKHOUSE…

138abc2

…_URL variable

Merge branch 'main' into consolidate-clickhouse-url

10f0a1c

nicktrn requested changes Oct 15, 2025

View reviewed changes

LeoKaynan closed this Oct 15, 2025

LeoKaynan deleted the consolidate-clickhouse-url branch October 15, 2025 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor: consolidate ClickHouse configuration into single CLICKHOUSE… #2607

refactor: consolidate ClickHouse configuration into single CLICKHOUSE… #2607

Uh oh!

LeoKaynan commented Oct 15, 2025 •

edited

Loading

Uh oh!

changeset-bot bot commented Oct 15, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Oct 15, 2025 •

edited

Loading

Uh oh!

nicktrn left a comment

Uh oh!

LeoKaynan commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	# ClickHouse configuration
	CLICKHOUSE_URL: ${CLICKHOUSE_URL:-http://default:password@clickhouse:8123?secure=false}
	CLICKHOUSE_LOG_LEVEL: ${CLICKHOUSE_LOG_LEVEL:-info}
	# Run replication
	RUN_REPLICATION_ENABLED: ${RUN_REPLICATION_ENABLED:-1}
	RUN_REPLICATION_CLICKHOUSE_URL: ${RUN_REPLICATION_CLICKHOUSE_URL:-http://default:password@clickhouse:8123}

	{{/*
	ClickHouse URL for replication (without secure parameter)
	*/}}
	{{- define "trigger-v4.clickhouse.replication.url" -}}
	{{- if .Values.clickhouse.deploy -}}
	{{- $protocol := ternary "https" "http" .Values.clickhouse.secure -}}
	{{ $protocol }}://{{ .Values.clickhouse.auth.username }}:{{ .Values.clickhouse.auth.password }}@{{ include "trigger-v4.clickhouse.hostname" . }}:8123
	{{- else if .Values.clickhouse.external.host -}}
	{{- $protocol := ternary "https" "http" .Values.clickhouse.external.secure -}}
	{{- if .Values.clickhouse.external.existingSecret -}}
	{{ $protocol }}://{{ .Values.clickhouse.external.username }}:${CLICKHOUSE_PASSWORD}@{{ .Values.clickhouse.external.host }}:{{ .Values.clickhouse.external.httpPort \| default 8123 }}
	{{- else -}}
	{{ $protocol }}://{{ .Values.clickhouse.external.username }}:{{ .Values.clickhouse.external.password }}@{{ .Values.clickhouse.external.host }}:{{ .Values.clickhouse.external.httpPort \| default 8123 }}
	{{- end -}}
	{{- end -}}
	{{- end }}

Uh oh!

refactor: consolidate ClickHouse configuration into single CLICKHOUSE… #2607

refactor: consolidate ClickHouse configuration into single CLICKHOUSE… #2607

Uh oh!

Conversation

LeoKaynan commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background & Context

PR #2035 - Original ClickHouse Implementation (May 2024)

PR #2264 - Bulk Actions 2.0 (July 2024)

Current State

Changes

1. Remove Redundant RUN_REPLICATION_CLICKHOUSE_URL

✅ Checklist

Testing

Changelog

Screenshots

Uh oh!

changeset-bot bot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

coderabbitai bot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

nicktrn left a comment

Choose a reason for hiding this comment

Uh oh!

LeoKaynan commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LeoKaynan commented Oct 15, 2025 •

edited

Loading

1. Remove Redundant `RUN_REPLICATION_CLICKHOUSE_URL`

changeset-bot bot commented Oct 15, 2025 •

edited

Loading

coderabbitai bot commented Oct 15, 2025 •

edited

Loading