Clickhouse performance improvements #2175
Conversation
matt-aitken
commented
Jun 13, 2025
matt-aitken
commented
- Add organizationId and projectId filtering, to improve CH performance
- Show the run ID in the Runs table now instead of run number
- Runs ClickHouse view fix for pagination after going backwards then forward again
|
WalkthroughThis set of changes introduces organization-level and project-level scoping to several data access and presentation layers of the application. The repository interfaces, implementations, and ClickHouse query parameter schemas for task activity, running stats, and average durations are updated to require ✨ Finishing Touches
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. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
apps/webapp/test/runsRepository.test.ts (1)
64-66: 🛠️ Refactor suggestionAvoid hard-coded
setTimeout(1000)in every test – slows CI by ~15 s.Replace with a polling helper that waits until ClickHouse replication catches up or a max timeout elapses:
async function waitForReplication(ch: ClickHouse, maxMs = 10000) { const start = Date.now(); while (true) { const { data } = await ch.query({ query: "SELECT count() FROM system.mutations" }); if (Number(data[0]?.count) === 0) break; if (Date.now() - start > maxMs) throw new Error("Replication timed out"); await new Promise(r => setTimeout(r, 100)); } }This keeps tests fast while still being deterministic.
apps/webapp/app/services/environmentMetricsRepository.server.ts (1)
55-63:⚠️ Potential issuePostgrest implementation no longer satisfies the interface – will not compile
PostgrestEnvironmentMetricsRepositorymethods still accept only{ environmentId, days, tasks }, but the interface now requires{ organizationId, projectId, … }.TypeScript complains:
“Property ‘organizationId’ is missing in type …”.Besides compilation, the Postgrest queries are now un-scoped by organisation/project, returning cross-tenant data.
Fix (apply to all three methods):
- public async getDailyTaskActivity({ - environmentId, - days, - tasks, - }: { - environmentId: string; - days: number; - tasks: string[]; - }): Promise<DailyTaskActivity> { + public async getDailyTaskActivity({ + organizationId: _orgId, // presently unused + projectId: _projId, // presently unused + environmentId, + days, + tasks, + }: { + organizationId: string; + projectId: string; + environmentId: string; + days: number; + tasks: string[]; + }): Promise<DailyTaskActivity> {Either scope the SQL by these IDs or prefix the unused vars with
_so lint doesn’t complain. Repeat forgetCurrentRunningStatsandgetAverageDurations.Also applies to: 99-107, 142-150
🧹 Nitpick comments (4)
apps/webapp/app/components/primitives/CopyableText.tsx (1)
7-18: Expose copy-value prop but mind accessibility / event bubblingNice addition – separating
copyValuefrom the renderedvaluecovers the truncated-ID use-case.
A couple of small follow-ups will make the component more robust:
- The clickable element is a plain
<span>, so screen-readers and keyboard users don’t know it’s actionable.- Only
onMouseDownstops propagation; aclickstill bubbles, which could trigger parent navigation (e.g. inside a link-cell).- <span - onClick={copy} - onMouseDown={(e) => e.stopPropagation()} + <span + role="button" + tabIndex={0} + aria-label="Copy to clipboard" + onClick={(e) => { + e.stopPropagation(); // prevent outer link navigation + copy(); + }} + onKeyDown={(e) => (e.key === "Enter" || e.key === " ") && copy()} className={cn(This preserves the existing behaviour while improving a11y and eliminating the accidental navigation gotcha.
apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts (2)
48-69: Missing JSDoc & param description for neworganizationId.A short comment would help future maintainers understand why this sits outside
RunListOptions, especially now that most other identifiers live in the options object.public async call( - organizationId: string, + /** + * Org the request is scoped to – enforced at ClickHouse layer. + */ + organizationId: string, environmentId: string,
188-210: Variable naming mismatch (runIdsvsrunFriendlyIds) causes cognitive load.
runIdscoming from the client contains friendly IDs, yet it’s mapped torunFriendlyIdshere while the real DB IDs are held inrestrictToRunIds. Consider renaming the option to avoid confusion.- runFriendlyIds: runIds, - runIds: restrictToRunIds, + runFriendlyIds: runFriendlyIds, // array of `friendlyId` + runIds: restrictToRunIds, // array of primary-key IDs(Propagate the rename through the loader & URL param to keep everything aligned.)
internal-packages/clickhouse/src/taskRuns.ts (1)
133-136: Consider composite index / ORDER BY to keep new filters performant.Filtering by
organization_id AND project_id AND environment_idon large tables can become expensive if the primary key / ORDER BY tuple hasn’t been extended accordingly.
Verify the ClickHouse table definition includes these columns at the front of itsORDER BY(or a supporting materialised view) to avoid full-table scans.Also applies to: 178-183, 219-222
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/webapp/app/components/primitives/CopyableText.tsx(1 hunks)apps/webapp/app/components/runs/v3/TaskRunsTable.tsx(3 hunks)apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts(2 hunks)apps/webapp/app/presenters/v3/TaskListPresenter.server.ts(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.next.runs._index/route.tsx(1 hunks)apps/webapp/app/services/environmentMetricsRepository.server.ts(7 hunks)apps/webapp/app/services/runsRepository.server.ts(5 hunks)apps/webapp/test/runsRepository.test.ts(16 hunks)internal-packages/clickhouse/src/taskRuns.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/webapp/app/services/runsRepository.server.ts (2)
internal-packages/clickhouse/src/client/noop.ts (1)
queryBuilder(14-22)internal-packages/clickhouse/src/client/client.ts (1)
queryBuilder(218-229)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- 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
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (2)
55-56: Import ofCopyableTextlooks goodThe new component import is correct and keeps run-ID presentation concerns local to the table.
139-139: Header rename clarifies intentRenaming “Run #” to “ID” matches the new truncated-ID display and is clearer for users.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.next.runs._index/route.tsx (1)
130-132: NeworganizationIdargument wired correctly.The additional parameter is passed in the expected order and type. 👍
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsx (1)
128-132: Loader now providesorganizationIdandprojectId.Signature alignment looks good and keeps the presenter contract consistent.
No further action needed.internal-packages/clickhouse/src/taskRuns.ts (1)
115-121: Good: schemas updated to requireorganizationId&projectId.Explicit scoping is essential for multi-tenant isolation – nice catch.
apps/webapp/app/presenters/v3/TaskListPresenter.server.ts (1)
35-44: All good – parameters correctly propagatedThe new
organizationId/projectIdfields are wired through cleanly and typed; nothing else to address here.apps/webapp/app/services/runsRepository.server.ts (2)
46-55: 👍 Added tenant scopingFiltering on
organization_id,project_id, andenvironment_idat the top of the query is clear and indexed-friendly.
118-130: Cursor predicate inconsistent with new composite orderingRows are ordered by
created_at, run_idbut the pagination predicate only comparesrun_id.
If two runs share the samecreated_at, items with a smallerrun_idbut a later timestamp than the cursor can be skipped or duplicated across pages.To make the ordering and predicate compatible, consider encoding a composite cursor
(created_at, run_id):.where("(created_at, run_id) < ({createdAt: DateTime64}, {runId: String})", { createdAt: cursorCreatedAt, runId: cursorRunId, }) .orderBy("created_at DESC, run_id DESC")Same adjustment needed for the backward branch.
