Skip to content
Merged
1 change: 1 addition & 0 deletions .github/workflows/claude-code-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ on:

jobs:
claude-review:
if: false # Disabled - using opencode-review.yml instead

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid if: false (breaks actionlint / permanently disables the job with no easy toggle).

Since actionlint flags constant-false conditions, prefer a repo variable toggle so it’s disabled by default but can be re-enabled without changing the workflow.

Proposed change
 jobs:
   claude-review:
-    if: false  # Disabled - using opencode-review.yml instead
+    # Disabled by default; set repo variable ENABLE_CLAUDE_REVIEW=true to enable.
+    if: ${{ vars.ENABLE_CLAUDE_REVIEW == 'true' }}
     runs-on: ubuntu-latest
📝 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.

Suggested change
if: false # Disabled - using opencode-review.yml instead
claude-review:
# Disabled by default; set repo variable ENABLE_CLAUDE_REVIEW=true to enable.
if: ${{ vars.ENABLE_CLAUDE_REVIEW == 'true' }}
runs-on: ubuntu-latest
🧰 Tools
🪛 actionlint (1.7.10)

15-15: constant expression "false" in condition. remove the if: section

(if-cond)

🤖 Prompt for AI Agents
In @.github/workflows/claude-code-review.yml at line 15, Replace the hard-coded
"if: false" constant with a repo-configurable boolean variable so the job is
disabled by default but can be toggled without editing the workflow; e.g.,
change the condition to reference a repository/organization variable like
OPENCODE_REVIEW_ENABLED (use the expression syntax: ${{
vars.OPENCODE_REVIEW_ENABLED == 'true' }} or similar) and document or set the
default variable to "false" in repo settings so the job stays off until the
variable is flipped.

runs-on: ubuntu-latest
permissions:
contents: read
Expand Down
21 changes: 20 additions & 1 deletion .github/workflows/opencode-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,32 @@ jobs:
uses: actions/checkout@v6
with:
fetch-depth: 1
persist-credentials: false

- name: Clear git extraheader to avoid duplicate auth
- name: Clear git credentials to avoid duplicate auth
if: |
github.event.pull_request.changed_files >= 5 ||
steps.calc.outputs.total >= 20
run: |
# Clear all GitHub-related git config to prevent auth conflicts
git config --global --unset-all http.https://github.com/.extraheader || true
git config --local --unset-all http.https://github.com/.extraheader || true
git config --global --unset-all credential.helper || true
git config --local --unset-all credential.helper || true
git config --global --unset-all credential."https://github.com".helper || true
git config --local --unset-all credential."https://github.com".helper || true
# Remove any credential URLs
git config --global --unset-all credential.url || true
git config --local --unset-all credential.url || true
# Clear any includeIf configs that might add credentials
# Note: git config doesn't support wildcards, so we iterate over matching keys
# Use case-insensitive grep to catch both "includeIf" and "includeif"
for key in $(git config --global --list --name-only 2>/dev/null | grep -i "^includeif\." || true); do
git config --global --unset "$key" || true
done
for key in $(git config --local --list --name-only 2>/dev/null | grep -i "^includeif\." || true); do
git config --local --unset "$key" || true
done

Comment thread
frankbria marked this conversation as resolved.
- name: Run OpenCode PR Review
# Only review substantial changes (5+ files OR 20+ lines changed)
Expand All @@ -54,6 +71,7 @@ jobs:
steps.calc.outputs.total >= 20
uses: anomalyco/opencode/github@latest
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
ZHIPU_API_KEY: ${{ secrets.ZHIPU_API_KEY }}
# Pass PR context as environment variables for the review
PR_NUMBER: ${{ github.event.pull_request.number }}
Expand All @@ -62,6 +80,7 @@ jobs:
REPO_NAME: ${{ github.repository }}
with:
model: zai-coding-plan/glm-4.7
use_github_token: true
prompt: |
You are reviewing PR #${{ github.event.pull_request.number }} in repository ${{ github.repository }}.

Expand Down
87 changes: 86 additions & 1 deletion tests/e2e/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,92 @@ test('should show correct state when X already completed', async ({ page }) => {
### State Reconciliation Test Files

- `test_state_reconciliation.spec.ts` - Comprehensive state reconciliation tests
- `test_late_joining_user.spec.ts` - Additional late-joining user scenarios
- `test_late_joining_user.spec.ts` - Late-joining user scenarios (may catch WebSocket events)
- `test_returning_user.spec.ts` - Returning user scenarios (no WebSocket events)

## Returning User vs Late-Joining User

**Critical distinction** (GitHub Issue #231):

| Scenario | WebSocket | Data Source | Test Pattern |
|----------|-----------|-------------|--------------|
| **Late-Joining** | May catch some events | API + partial WebSocket | Navigate during active session |
| **Returning User** | No events received | API only | Block WebSocket, navigate to seeded project |

### The Returning User Problem (Fixed in #231)

Users who navigate to a project AFTER all events occurred (page refresh, login later, new tab) don't receive WebSocket history. Before the fix:

```typescript
// OLD BEHAVIOR: Tasks only loaded via WebSocket events
useEffect(() => {
// Intentionally empty - tasks managed via WebSocket
}, [tasksData]);
```

After the fix:

```typescript
// NEW BEHAVIOR: Tasks loaded from API on mount
useEffect(() => {
if (tasksData?.data?.tasks) {
dispatch({ type: 'TASKS_LOADED', payload: tasksData.data.tasks });
}
}, [tasksData]);
```

### Writing Returning User Tests

Block WebSocket to ensure tests don't rely on real-time events:

```typescript
import { blockWebSocketConnections } from './test-utils';

test('should show state when returning to project', async ({ page }) => {
// Block WebSocket BEFORE navigation
const unblock = await blockWebSocketConnections(page);

// Navigate as returning user (no WebSocket history)
await page.goto(`${FRONTEND_URL}/projects/${PROJECT_ID}`);

// Wait for API data to load
await page.waitForLoadState('networkidle');

// Verify UI shows correct state from API
await expect(page.locator('[data-testid="task-card"]')).toHaveCount(5);

// Cleanup
await unblock();
});
```

### Helper Functions

Use these utilities from `test-utils.ts`:

```typescript
// Block WebSocket connections
const unblock = await blockWebSocketConnections(page);

// Verify task state from API
await verifyTaskStateFromAPI(page, projectId, {
inProgress: 2,
completed: 3,
total: 5,
});

// Verify task state from DOM
const { actualCounts, passed, errors } = await verifyTaskStateFromDOM(page, {
inProgress: 2,
completed: 3,
});

// Verify project phase
await verifyProjectPhaseFromAPI(page, projectId, 'active');

// Verify project completion
const { isComplete, hasActiveWork } = await verifyProjectCompletionFromDOM(page);
```

### Smoke Tests

Expand Down
235 changes: 235 additions & 0 deletions tests/e2e/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -717,3 +717,238 @@ export async function answerDiscoveryQuestion(
// This is a valid end state, so we continue
}
}

// ============================================================================
// STATE VERIFICATION HELPERS (for returning user tests)
// ============================================================================

/**
* Expected task state counts by status
*/
export interface ExpectedTaskState {
inProgress?: number;
completed?: number;
pending?: number;
blocked?: number;
total?: number;
}

/**
* Verify task state from API matches expected counts
*
* Use this to validate that API returns expected task data before checking UI.
* This is critical for returning user tests where WebSocket is blocked.
*
* @param page - Playwright page object
* @param projectId - Project ID to check
* @param expected - Expected task counts by status
* @throws Error if task counts don't match
*
* @example
* await verifyTaskStateFromAPI(page, '3', {
* inProgress: 2,
* completed: 1,
* total: 5
* });
*/
export async function verifyTaskStateFromAPI(
page: Page,
projectId: string,
expected: ExpectedTaskState
): Promise<{ tasks: any[]; counts: Required<ExpectedTaskState> }> {
// Get auth token
const token = await getAuthToken(page);
if (!token) {
throw new Error('No auth token available');
}

// Fetch tasks from API
const response = await page.request.get(`${BACKEND_URL}/api/projects/${projectId}/tasks`, {
headers: { Authorization: `Bearer ${token}` },
});

if (!response.ok()) {
throw new Error(`Failed to fetch tasks: ${response.status()}`);
}

const data = await response.json();
const tasks = data.tasks || [];

// Count tasks by status
const counts = {
inProgress: tasks.filter((t: { status: string }) => t.status === 'in_progress').length,
completed: tasks.filter((t: { status: string }) => t.status === 'completed').length,
pending: tasks.filter((t: { status: string }) => t.status === 'pending').length,
blocked: tasks.filter((t: { status: string }) => t.status === 'blocked').length,
total: tasks.length,
};

// Validate expected counts
if (expected.inProgress !== undefined && counts.inProgress !== expected.inProgress) {
throw new Error(`Expected ${expected.inProgress} in-progress tasks, got ${counts.inProgress}`);
}
if (expected.completed !== undefined && counts.completed !== expected.completed) {
throw new Error(`Expected ${expected.completed} completed tasks, got ${counts.completed}`);
}
if (expected.pending !== undefined && counts.pending !== expected.pending) {
throw new Error(`Expected ${expected.pending} pending tasks, got ${counts.pending}`);
}
if (expected.blocked !== undefined && counts.blocked !== expected.blocked) {
throw new Error(`Expected ${expected.blocked} blocked tasks, got ${counts.blocked}`);
}
if (expected.total !== undefined && counts.total !== expected.total) {
throw new Error(`Expected ${expected.total} total tasks, got ${counts.total}`);
}

return { tasks, counts };
}

/**
* Verify project phase from API
*
* @param page - Playwright page object
* @param projectId - Project ID to check
* @param expectedPhase - Expected phase (discovery, planning, active, review, complete)
* @throws Error if phase doesn't match
*/
export async function verifyProjectPhaseFromAPI(
page: Page,
projectId: string,
expectedPhase: string
): Promise<{ project: any }> {
const token = await getAuthToken(page);
if (!token) {
throw new Error('No auth token available');
}

const response = await page.request.get(`${BACKEND_URL}/api/projects/${projectId}`, {
headers: { Authorization: `Bearer ${token}` },
});

if (!response.ok()) {
throw new Error(`Failed to fetch project: ${response.status()}`);
}

const project = await response.json();

if (project.phase !== expectedPhase) {
throw new Error(`Expected project phase '${expectedPhase}', got '${project.phase}'`);
}

return { project };
}

/**
* Verify task state from DOM elements
*
* Checks the actual UI for task status indicators.
* Use after page has loaded to verify UI matches expected state.
*
* @param page - Playwright page object
* @param expected - Expected task counts by status
* @returns Object with actual counts and whether validation passed
*/
export async function verifyTaskStateFromDOM(
page: Page,
expected: ExpectedTaskState
): Promise<{ actualCounts: ExpectedTaskState; passed: boolean; errors: string[] }> {
const errors: string[] = [];

// Look for task items with status indicators
// Common patterns: data-status, data-task-status, status badge classes
const inProgressLocator = page.locator('[data-status="in_progress"], [data-task-status="in_progress"]');
const completedLocator = page.locator('[data-status="completed"], [data-task-status="completed"]');
const pendingLocator = page.locator('[data-status="pending"], [data-task-status="pending"]');
const blockedLocator = page.locator('[data-status="blocked"], [data-task-status="blocked"]');
const allTasksLocator = page.locator('[data-testid="task-item"], [data-testid="task-card"]');

const actualCounts: ExpectedTaskState = {
inProgress: await inProgressLocator.count(),
completed: await completedLocator.count(),
pending: await pendingLocator.count(),
blocked: await blockedLocator.count(),
total: await allTasksLocator.count(),
};

// Validate expected counts
if (expected.inProgress !== undefined && actualCounts.inProgress !== expected.inProgress) {
errors.push(`Expected ${expected.inProgress} in-progress tasks in DOM, found ${actualCounts.inProgress}`);
}
if (expected.completed !== undefined && actualCounts.completed !== expected.completed) {
errors.push(`Expected ${expected.completed} completed tasks in DOM, found ${actualCounts.completed}`);
}
if (expected.pending !== undefined && actualCounts.pending !== expected.pending) {
errors.push(`Expected ${expected.pending} pending tasks in DOM, found ${actualCounts.pending}`);
}
if (expected.blocked !== undefined && actualCounts.blocked !== expected.blocked) {
errors.push(`Expected ${expected.blocked} blocked tasks in DOM, found ${actualCounts.blocked}`);
}
if (expected.total !== undefined && actualCounts.total !== expected.total) {
errors.push(`Expected ${expected.total} total tasks in DOM, found ${actualCounts.total}`);
}

return { actualCounts, passed: errors.length === 0, errors };
}

/**
* Verify project completion state from UI
*
* Checks for completion indicators in the UI.
*
* @param page - Playwright page object
* @returns Object with completion state details
*/
export async function verifyProjectCompletionFromDOM(
page: Page
): Promise<{ isComplete: boolean; hasActiveWork: boolean; details: string }> {
// Check for completion badge/status
const statusBadge = page.locator('[data-testid="project-status"], [data-testid="phase-badge"]');
let statusText = '';
if (await statusBadge.first().isVisible()) {
statusText = await statusBadge.first().textContent() || '';
}

const isComplete = /complete|done|finished/i.test(statusText);

// Check for any in-progress or pending tasks
const inProgressTasks = await page.locator('[data-status="in_progress"]').count();
const pendingTasks = await page.locator('[data-status="pending"]').count();
const hasActiveWork = inProgressTasks > 0 || pendingTasks > 0;

return {
isComplete,
hasActiveWork,
details: `Status: "${statusText}", In-progress: ${inProgressTasks}, Pending: ${pendingTasks}`,
};
}

/**
* Block WebSocket connections to simulate returning user scenario
*
* CRITICAL: Call this BEFORE navigating to the project page.
* Returns a cleanup function to restore WebSocket connections.
*
* @param page - Playwright page object
* @returns Cleanup function to unblock WebSocket
*
* @example
* const unblock = await blockWebSocketConnections(page);
* await page.goto('/projects/3');
* // ... test assertions ...
* await unblock();
*/
export async function blockWebSocketConnections(page: Page): Promise<() => Promise<void>> {
// Block all WebSocket upgrade requests
await page.route('**/ws**', async (route) => {
await route.abort('connectionrefused');
});

await page.route('**/localhost**/ws**', async (route) => {
await route.abort('connectionrefused');
});

return async () => {
await page.unroute('**/ws**');
await page.unroute('**/localhost**/ws**');
};
}
Loading
Loading