fix: address 13 CodeRabbit review comments for GitLab parity

- Replace any[] with GitLabMergeRequest[] in MR list handler
- Add clearPollingForProject calls on project remove & window close
- Fix stale config closure in MR status polling interval
- Handle Error-like objects with message property in error parser
- Use regex word boundaries (\b401\b) for status code detection
- Replace relative imports with path aliases in MRLogs/StatusIndicator
- Add note reserving useGitLabMRFiltering for future functionality
- Remove duplicate test case in issues-store.test.ts
- Add test for connected:false case in sync-status-store.test.ts
- Fix stale-request guard to use getState() for live token comparison
- Handle result.data.connected === false explicitly

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
StillKnotKnown
2026-03-13 13:26:24 +02:00
parent 228a58a87c
commit 86da9ee6dd
11 changed files with 103 additions and 38 deletions
+10
View File
@@ -385,6 +385,16 @@ function createWindow(): void {
agentManager?.killAll?.()?.catch((err: unknown) => {
console.warn('[main] Error killing agents on window close:', err);
});
// Clear GitLab MR polling intervals for all projects
import('./ipc-handlers/gitlab/mr-review-handlers').then(({ clearPollingForProject }) => {
const { projectStore } = require('./project-store');
const projects = projectStore.getAllProjects();
for (const project of projects) {
clearPollingForProject(project.id);
}
}).catch((err: unknown) => {
console.warn('[main] Error clearing GitLab polling on window close:', err);
});
mainWindow = null;
});
}
@@ -17,7 +17,7 @@ import { randomUUID } from 'crypto';
import { IPC_CHANNELS, MODEL_ID_MAP, DEFAULT_FEATURE_MODELS, DEFAULT_FEATURE_THINKING } from '../../../shared/constants';
import { getGitLabConfig, gitlabFetch, encodeProjectPath } from './utils';
import { readSettingsFile } from '../../settings-utils';
import type { Project, AppSettings, IPCResult } from '../../../shared/types';
import type { Project, AppSettings, IPCResult, GitLabMergeRequest } from '../../../shared/types';
import type {
MRReviewResult,
MRReviewProgress,
@@ -25,6 +25,7 @@ import type {
} from './types';
import { createContextLogger } from '../github/utils/logger';
import { withProjectOrNull } from '../github/utils/project-middleware';
import { projectStore } from '../../project-store';
import { createIPCCommunicators } from '../github/utils/ipc-communicator';
import {
MRReviewEngine,
@@ -1118,6 +1119,7 @@ export function registerMRReviewHandlers(
/**
* Get AI review logs for an MR
* TODO: Return structured PRLogs type instead of string[] to match MRLogs component expectations
*/
ipcMain.handle(
IPC_CHANNELS.GITLAB_MR_GET_LOGS,
@@ -1163,7 +1165,7 @@ export function registerMRReviewHandlers(
async (event, projectId: string, mrIid: number, intervalMs: number = 5000): Promise<IPCResult<{ polling: boolean }>> => {
debugLog('statusPollStart handler called', { projectId, mrIid, intervalMs });
const result = await withProjectOrNull(projectId, async (project) => {
const result = await withProjectOrNull(projectId, async (_project) => {
const pollKey = `${projectId}:${mrIid}`;
// Clear existing interval if any
@@ -1191,7 +1193,14 @@ export function registerMRReviewHandlers(
try {
// Emit status update to renderer
if (callingWindow && !callingWindow.isDestroyed()) {
const config = await getGitLabConfig(project);
// Fetch current project to avoid stale config from closure
const currentProject = projectStore.getProject(projectId);
if (!currentProject) {
debugLog('Project not found during poll', { projectId });
return;
}
const config = await getGitLabConfig(currentProject);
if (!config) return;
const { token, instanceUrl } = config;
@@ -1339,7 +1348,7 @@ export function registerMRReviewHandlers(
projectId: string,
state?: 'opened' | 'closed' | 'merged' | 'all',
page: number = 2
): Promise<IPCResult<{ mrs: any[]; hasMore: boolean }>> => {
): Promise<IPCResult<{ mrs: GitLabMergeRequest[]; hasMore: boolean }>> => {
debugLog('listMore handler called', { projectId, state, page });
const result = await withProjectOrNull(projectId, async (project) => {
@@ -266,6 +266,9 @@ export function registerProjectHandlers(
ipcMain.handle(
IPC_CHANNELS.PROJECT_REMOVE,
async (_, projectId: string): Promise<IPCResult> => {
// Clear GitLab MR polling for this project to prevent memory leaks
const { clearPollingForProject } = await import('./gitlab/mr-review-handlers');
clearPollingForProject(projectId);
const success = projectStore.removeProject(projectId);
return { success };
}
@@ -33,6 +33,14 @@ export function parseGitLabError(error: unknown): ParsedGitLabError {
return parseGitLabErrorMessage(error);
}
// Handle Error-like objects with a message property
if (typeof error === 'object' && error !== null && 'message' in error) {
const message = (error as { message: string }).message;
if (typeof message === 'string') {
return parseGitLabErrorMessage(message);
}
}
return {
code: GitLabErrorCode.UNKNOWN,
recoverable: false
@@ -45,8 +53,31 @@ export function parseGitLabError(error: unknown): ParsedGitLabError {
function parseGitLabErrorMessage(message: string): ParsedGitLabError {
const lowerMessage = message.toLowerCase();
// Check for explicit HTTP status code in response (if available)
// Try to extract status from common patterns like "Status: 401" or HTTP error responses
const statusMatch = message.match(/\bstatus:\s*(\d{3})\b/i) ||
message.match(/\bhttp\s+(\d{3})\b/i) ||
lowerMessage.match(/\b"status":\s*(\d{3})\b/);
if (statusMatch) {
const statusCode = parseInt(statusMatch[1], 10);
switch (statusCode) {
case 401:
return { code: GitLabErrorCode.AUTHENTICATION_FAILED, recoverable: true };
case 403:
return { code: GitLabErrorCode.INSUFFICIENT_PERMISSIONS, recoverable: true };
case 404:
return { code: GitLabErrorCode.PROJECT_NOT_FOUND, recoverable: true };
case 409:
return { code: GitLabErrorCode.CONFLICT, recoverable: false };
case 429:
return { code: GitLabErrorCode.RATE_LIMITED, recoverable: true };
}
}
// Fallback to message content analysis with word-boundary regex to avoid false matches
// Authentication errors
if (lowerMessage.includes('401') || lowerMessage.includes('unauthorized') || lowerMessage.includes('invalid token')) {
if (/\b401\b/.test(message) || lowerMessage.includes('unauthorized') || lowerMessage.includes('invalid token')) {
return {
code: GitLabErrorCode.AUTHENTICATION_FAILED,
recoverable: true
@@ -54,7 +85,7 @@ function parseGitLabErrorMessage(message: string): ParsedGitLabError {
}
// Rate limiting (429)
if (lowerMessage.includes('429') || lowerMessage.includes('rate limit') || lowerMessage.includes('too many requests')) {
if (/\b429\b/.test(message) || lowerMessage.includes('rate limit') || lowerMessage.includes('too many requests')) {
return {
code: GitLabErrorCode.RATE_LIMITED,
recoverable: true
@@ -70,7 +101,7 @@ function parseGitLabErrorMessage(message: string): ParsedGitLabError {
}
// Project not found (404)
if (lowerMessage.includes('404') || lowerMessage.includes('not found')) {
if (/\b404\b/.test(message) || lowerMessage.includes('not found')) {
return {
code: GitLabErrorCode.PROJECT_NOT_FOUND,
recoverable: true
@@ -78,7 +109,7 @@ function parseGitLabErrorMessage(message: string): ParsedGitLabError {
}
// Permission denied (403)
if (lowerMessage.includes('403') || lowerMessage.includes('forbidden') || lowerMessage.includes('permission denied')) {
if (/\b403\b/.test(message) || lowerMessage.includes('forbidden') || lowerMessage.includes('permission denied')) {
return {
code: GitLabErrorCode.INSUFFICIENT_PERMISSIONS,
recoverable: true
@@ -86,7 +117,7 @@ function parseGitLabErrorMessage(message: string): ParsedGitLabError {
}
// Conflict (409)
if (lowerMessage.includes('409') || lowerMessage.includes('conflict')) {
if (/\b409\b/.test(message) || lowerMessage.includes('conflict')) {
return {
code: GitLabErrorCode.CONFLICT,
recoverable: false
@@ -24,15 +24,15 @@ import {
Clock,
Activity
} from 'lucide-react';
import { Badge } from '../../ui/badge';
import { Collapsible, CollapsibleTrigger, CollapsibleContent } from '../../ui/collapsible';
import { cn } from '../../../lib/utils';
import { Badge } from '@/components/ui/badge';
import { Collapsible, CollapsibleTrigger, CollapsibleContent } from '@/components/ui/collapsible';
import { cn } from '@/lib/utils';
import type {
PRLogs,
PRLogPhase,
PRPhaseLog,
PRLogEntry
} from '../../../../preload/api/modules/github-api';
} from '@preload/api/modules/github-api';
// Type aliases for GitLab compatibility
type GitLabMRLogs = PRLogs;
@@ -1,7 +1,7 @@
import { CheckCircle2, Circle, XCircle, Loader2, AlertTriangle, GitMerge, HelpCircle } from 'lucide-react';
import { Badge } from '../../ui/badge';
import { cn } from '../../../lib/utils';
import type { ChecksStatus, ReviewsStatus, MergeableState } from '../../../../shared/types/pr-status';
import { Badge } from '@/components/ui/badge';
import { cn } from '@/lib/utils';
import type { ChecksStatus, ReviewsStatus, MergeableState } from '@shared/types/pr-status';
import { useTranslation } from 'react-i18next';
/**
@@ -3,6 +3,10 @@
*
* Stub hook - implements the same pattern as usePRFiltering
* adapted for GitLab merge requests.
*
* NOTE: This hook and MRFilterBar are reserved for future filtering functionality.
* They are not currently integrated into the GitLab MRs UI but are retained
* for when filtering/search features are implemented.
*/
import { useMemo, useState, useCallback } from 'react';
@@ -11,7 +15,7 @@ import type {
GitLabMRReviewResult,
GitLabMRReviewProgress,
GitLabNewCommitsCheck
} from '../../../../shared/types';
} from '@shared/types';
export type GitLabMRStatusFilter =
| 'all'
@@ -88,20 +88,6 @@ describe('issues-store', () => {
expect(selected?.iid).toBe(1);
});
it('should get filtered issues', () => {
const issues = [
createMockGitLabIssue({ iid: 1, state: 'opened' }),
createMockGitLabIssue({ iid: 2, state: 'closed' }),
createMockGitLabIssue({ iid: 3, state: 'opened' }),
];
useIssuesStore.getState().setIssues(issues);
useIssuesStore.getState().setFilterState('opened');
const filtered = useIssuesStore.getState().getFilteredIssues();
expect(filtered).toHaveLength(2);
expect(filtered.every((i: GitLabIssue) => i.state === 'opened')).toBe(true);
});
it('should count open issues', () => {
const issues = [
createMockGitLabIssue({ iid: 1, state: 'opened' }),
@@ -106,6 +106,22 @@ describe('sync-status-store', () => {
expect(useSyncStatusStore.getState().connectionError).toBe('Authentication failed');
});
it('should set error when connected is false', async () => {
mockElectronAPI.checkGitLabConnection.mockResolvedValue({
success: true,
data: {
connected: false,
error: 'Project not found'
}
});
const result = await checkGitLabConnection('project-123');
expect(result).toBe(null);
expect(useSyncStatusStore.getState().syncStatus).toBe(null);
expect(useSyncStatusStore.getState().connectionError).toBe('Project not found');
});
it('should set error on exception', async () => {
mockElectronAPI.checkGitLabConnection.mockRejectedValue(new Error('Network error'));
@@ -119,8 +119,8 @@ export async function loadGitLabIssues(
try {
const result = await window.electronAPI.getGitLabIssues(projectId, state);
// Guard against stale responses
if (store.currentRequestToken !== requestId) {
// Guard against stale responses - read live state, not captured store reference
if (useIssuesStore.getState().currentRequestToken !== requestId) {
return; // A newer request has superseded this one
}
@@ -130,14 +130,14 @@ export async function loadGitLabIssues(
store.setError(result.error || 'Failed to load GitLab issues');
}
} catch (error) {
// Guard against stale responses in error case
if (store.currentRequestToken !== requestId) {
// Guard against stale responses in error case - read live state
if (useIssuesStore.getState().currentRequestToken !== requestId) {
return;
}
store.setError(error instanceof Error ? error.message : 'Unknown error');
} finally {
// Only clear loading state if this is still the current request
if (store.currentRequestToken === requestId) {
// Only clear loading state if this is still the current request - read live state
if (useIssuesStore.getState().currentRequestToken === requestId) {
store.setLoading(false);
}
}
@@ -57,9 +57,15 @@ export async function checkGitLabConnection(projectId: string): Promise<GitLabSy
try {
const result = await window.electronAPI.checkGitLabConnection(projectId);
if (result.success && result.data) {
// Only set sync status if actually connected (connected === true)
if (result.success && result.data && result.data.connected === true) {
store.setSyncStatus(result.data);
return result.data;
} else if (result.success && result.data && result.data.connected === false) {
// Connection failed but request succeeded - treat as error
store.clearSyncStatus();
store.setConnectionError(result.data.error || 'Failed to check GitLab connection');
return null;
} else {
store.clearSyncStatus();
store.setConnectionError(result.error || 'Failed to check GitLab connection');