fix(platform): show access denied for non-members and optimize execution query#578
Conversation
…ion query Show an AccessDenied screen when a user without membership navigates to a workspace dashboard. Optimize hasRunningExecution to use a compound index (by_definition_status) with parallel queries instead of a filter scan.
Greptile SummaryImproved UX by displaying an access denied message when authenticated users navigate to workspaces they're not members of (previously showed nothing), and optimized workflow execution queries by replacing filtered scans with compound index lookups. Key Changes:
Code Quality:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| services/platform/app/routes/dashboard/$id.tsx | Added access denied message for non-members and split loading states for better UX - shows spinner while auth or member query loads, then access denied if user lacks membership |
| services/platform/convex/workflow_engine/helpers/scheduler/has_running_execution.ts | Optimized to use compound by_definition_status index with parallel queries instead of filtering, improving query performance |
| services/platform/convex/workflows/schema.ts | Added compound index by_definition_status on wfExecutions table to support optimized execution status queries |
Last reviewed commit: c39ee38
📝 WalkthroughWalkthroughThis PR integrates authentication and internationalization handling into the dashboard route, replacing null membership fallbacks with an AccessDenied component. It introduces a new composite database index for workflow execution queries and refactors the query logic to perform concurrent lookups by status. The PR adds comprehensive test coverage for the dashboard layout component and workflow execution helpers, includes a new localization entry for membership denial messages, and updates the test configuration to include route-level tests. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/platform/convex/workflow_engine/helpers/scheduler/has_running_execution.test.ts`:
- Around line 19-21: The mock for query.withIndex in
has_running_execution.test.ts currently ignores the _indexName, allowing false
positives; update the withIndex mock (the vi.fn inside the query mock) to assert
that _indexName === 'by_definition_status' (or throw/fail the test if not)
before invoking the provided callback, so the test fails if hasRunningExecution
stops using the expected index; locate the query mock and its withIndex
implementation and add this explicit check/assertion.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
services/platform/app/routes/dashboard/$id.tsxservices/platform/app/routes/dashboard/__tests__/dashboard-layout.test.tsxservices/platform/convex/workflow_engine/helpers/scheduler/has_running_execution.test.tsservices/platform/convex/workflow_engine/helpers/scheduler/has_running_execution.tsservices/platform/convex/workflows/schema.tsservices/platform/messages/en.jsonservices/platform/vitest.ui.config.mjs
| query: vi.fn((_table: string) => ({ | ||
| withIndex: vi.fn((_indexName: string, cb: (q: unknown) => unknown) => { | ||
| const filters: Record<string, string> = {}; |
There was a problem hiding this comment.
Assert index usage in the mock to prevent false-positive regressions.
Right now _indexName is ignored, so tests can still pass even if hasRunningExecution stops using by_definition_status. That misses the exact optimization this PR introduces.
Suggested test hardening
function createMockCtx(executions: MockExecution[]) {
const db = {
- query: vi.fn((_table: string) => ({
- withIndex: vi.fn((_indexName: string, cb: (q: unknown) => unknown) => {
+ query: vi.fn((table: string) => ({
+ withIndex: vi.fn((indexName: string, cb: (q: unknown) => unknown) => {
+ expect(table).toBe('wfExecutions');
+ expect(indexName).toBe('by_definition_status');
const filters: Record<string, string> = {};
const q = {
eq: (field: string, value: string) => {
filters[field] = value;
return q;
},
};
cb(q);
const match = executions.find(
(e) =>
e.wfDefinitionId === filters['wfDefinitionId'] &&
e.status === filters['status'],
);
return {
first: vi.fn(async () => match ?? null),
};
}),
})),
};Also applies to: 30-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/convex/workflow_engine/helpers/scheduler/has_running_execution.test.ts`
around lines 19 - 21, The mock for query.withIndex in
has_running_execution.test.ts currently ignores the _indexName, allowing false
positives; update the withIndex mock (the vi.fn inside the query mock) to assert
that _indexName === 'by_definition_status' (or throw/fail the test if not)
before invoking the provided callback, so the test fails if hasRunningExecution
stops using the expected index; locate the query mock and its withIndex
implementation and add this explicit check/assertion.
Summary
hasRunningExecutionquery by using a compoundby_definition_statusindex instead of filtering, enabling parallel lookups forrunningandpendingstatuseshasRunningExecutionhelperTest plan
hasRunningExecutioncorrectly detects running/pending executions via compound indexhasRunningExecutionsreturns correct map for multiple definitions🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests