chore: enable additional Claude plugins and add state sync sprint documentation
- Enabled frontend-design, serena, typescript-lsp, playwright, pyright-lsp, and rust-analyzer-lsp plugins in Claude settings for enhanced development capabilities. - Updated client submodule to latest commit. - Added comprehensive documentation for SPRINT-GAP-012 addressing state synchronization and observability gaps, including detailed implementation checklist covering server address timing fixes, OAuth state consistency improvements
This commit is contained in:
@@ -1,5 +1,11 @@
|
||||
{
|
||||
"enabledPlugins": {
|
||||
"feature-dev@claude-plugins-official": true
|
||||
"feature-dev@claude-plugins-official": true,
|
||||
"frontend-design@claude-plugins-official": true,
|
||||
"serena@claude-plugins-official": true,
|
||||
"typescript-lsp@claude-plugins-official": true,
|
||||
"playwright@claude-plugins-official": true,
|
||||
"pyright-lsp@claude-plugins-official": true,
|
||||
"rust-analyzer-lsp@claude-plugins-official": true
|
||||
}
|
||||
}
|
||||
|
||||
2
client
2
client
Submodule client updated: ed5146be04...33098a1598
@@ -0,0 +1,392 @@
|
||||
# Implementation Checklist: SPRINT-GAP-012
|
||||
|
||||
Step-by-step implementation guide for state synchronization and observability fixes.
|
||||
|
||||
## Pre-Implementation
|
||||
|
||||
- [ ] Read and understand the full README.md
|
||||
- [ ] Ensure development environment is running (`docker compose up -d`)
|
||||
- [ ] Run `make quality` to establish baseline
|
||||
|
||||
---
|
||||
|
||||
## Phase 1: Server Address Timing Fix (Priority: P1)
|
||||
|
||||
### 1.1 Add effectiveServerUrl Refresh After Connect
|
||||
|
||||
**File**: `client/src/pages/Settings.tsx`
|
||||
|
||||
Find `handleConnect()` (around line 260) and add URL refresh after successful connection:
|
||||
|
||||
```typescript
|
||||
const handleConnect = async () => {
|
||||
setIsConnecting(true);
|
||||
try {
|
||||
const normalized = normalizeServerInput(serverHost, serverPort);
|
||||
if (normalized.host !== serverHost) {
|
||||
setServerHost(normalized.host);
|
||||
}
|
||||
if (normalized.port !== serverPort) {
|
||||
setServerPort(normalized.port);
|
||||
}
|
||||
if (normalized.host || normalized.port) {
|
||||
preferences.setServerConnection(normalized.host, normalized.port);
|
||||
}
|
||||
if (isTauriEnvironment()) {
|
||||
await getAPI().savePreferences(preferences.get());
|
||||
}
|
||||
const api = getAPI();
|
||||
const info = await api.connect(buildServerUrl(normalized.host, normalized.port));
|
||||
setIsConnected(true);
|
||||
setServerInfo(info);
|
||||
|
||||
// ADD: Refresh effective URL after successful connection
|
||||
const urlInfo = await api.getEffectiveServerUrl();
|
||||
setEffectiveServerUrl(urlInfo);
|
||||
} catch (error) {
|
||||
setIsConnected(false);
|
||||
toastError({
|
||||
title: 'Connection Failed',
|
||||
error,
|
||||
fallback: 'Unable to reach server',
|
||||
});
|
||||
} finally {
|
||||
setIsConnecting(false);
|
||||
}
|
||||
};
|
||||
```
|
||||
|
||||
**Verify**:
|
||||
- [ ] Connect to a new server address
|
||||
- [ ] Navigate to another page
|
||||
- [ ] Navigate back to Settings
|
||||
- [ ] Address should match what was saved
|
||||
|
||||
### 1.2 Add E2E Test for Address Persistence
|
||||
|
||||
**File**: `client/e2e/connection-persistence.spec.ts` (NEW FILE)
|
||||
|
||||
```typescript
|
||||
import { test, expect } from './fixtures';
|
||||
|
||||
test.describe('Connection Address Persistence', () => {
|
||||
test('server address persists across navigation', async ({ page }) => {
|
||||
// Go to settings
|
||||
await page.goto('/settings?tab=status');
|
||||
await page.waitForLoadState('networkidle');
|
||||
|
||||
// Clear and set new address
|
||||
const hostInput = page.locator('#host');
|
||||
const portInput = page.locator('#port');
|
||||
|
||||
await hostInput.clear();
|
||||
await hostInput.fill('127.0.0.1');
|
||||
await portInput.clear();
|
||||
await portInput.fill('50051');
|
||||
|
||||
// Note: We can't actually connect in e2e without a running server
|
||||
// This test validates the UI state persistence
|
||||
|
||||
// Navigate away
|
||||
await page.goto('/');
|
||||
await page.waitForLoadState('networkidle');
|
||||
|
||||
// Navigate back
|
||||
await page.goto('/settings?tab=status');
|
||||
await page.waitForLoadState('networkidle');
|
||||
|
||||
// Verify values persisted
|
||||
await expect(hostInput).toHaveValue('127.0.0.1');
|
||||
await expect(portInput).toHaveValue('50051');
|
||||
});
|
||||
|
||||
test('address changes are reflected in effective URL tooltip', async ({ page }) => {
|
||||
await page.goto('/settings?tab=status');
|
||||
await page.waitForLoadState('networkidle');
|
||||
|
||||
// Set custom address
|
||||
const hostInput = page.locator('#host');
|
||||
await hostInput.clear();
|
||||
await hostInput.fill('custom.server.local');
|
||||
|
||||
// The effective URL info icon should be present
|
||||
const infoIcon = page.locator('[data-testid="effective-url-info"]');
|
||||
if (await infoIcon.isVisible()) {
|
||||
await infoIcon.hover();
|
||||
// Tooltip should show the address source
|
||||
await expect(page.locator('[role="tooltip"]')).toContainText('Source:');
|
||||
}
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
**Verify**:
|
||||
- [ ] Run `make e2e` or `npx playwright test connection-persistence`
|
||||
- [ ] Tests pass
|
||||
|
||||
---
|
||||
|
||||
## Phase 2: OAuth State Consistency (Priority: P1)
|
||||
|
||||
### 2.1 Extend Existing Validation Helper
|
||||
|
||||
**File**: `client/src/lib/integration-utils.ts`
|
||||
|
||||
The file already has `hasRequiredIntegrationFields()`. Add OIDC case and convenience wrapper:
|
||||
|
||||
```typescript
|
||||
// In the existing switch statement, add OIDC case:
|
||||
export function hasRequiredIntegrationFields(integration: Integration): boolean {
|
||||
switch (integration.type) {
|
||||
case 'auth':
|
||||
return !!(integration.oauth_config?.client_id && integration.oauth_config?.client_secret);
|
||||
case 'email':
|
||||
return integration.email_config?.provider_type === 'api'
|
||||
? !!integration.email_config?.api_key
|
||||
: !!(integration.email_config?.smtp_host && integration.email_config?.smtp_username);
|
||||
case 'calendar':
|
||||
return !!(integration.oauth_config?.client_id && integration.oauth_config?.client_secret);
|
||||
case 'pkm':
|
||||
return !!(integration.pkm_config?.api_key || integration.pkm_config?.vault_path);
|
||||
case 'custom':
|
||||
return !!integration.webhook_config?.url;
|
||||
// ADD THIS CASE:
|
||||
case 'oidc':
|
||||
return !!(integration.oidc_config?.issuer_url && integration.oidc_config?.client_id);
|
||||
default:
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
// ADD below the existing function:
|
||||
/** Check if integration is connected AND has required credentials */
|
||||
export const isEffectivelyConnected = (integration: Integration): boolean =>
|
||||
integration.status === 'connected' && hasRequiredIntegrationFields(integration);
|
||||
```
|
||||
|
||||
**Verify**:
|
||||
- [ ] File compiles without errors
|
||||
- [ ] Run `npm run type-check`
|
||||
|
||||
### 2.2 Add Validation to handleIntegrationToggle
|
||||
|
||||
**File**: `client/src/components/settings/integrations-section.tsx`
|
||||
|
||||
Find `handleIntegrationToggle` (around line 122) and add validation:
|
||||
|
||||
```typescript
|
||||
import { hasRequiredIntegrationFields } from '@/lib/integration-utils';
|
||||
|
||||
const handleIntegrationToggle = (integration: Integration) => {
|
||||
// ADD: Validate before allowing connect
|
||||
if (integration.status === ConnStatus.DISCONNECTED) {
|
||||
if (!hasRequiredIntegrationFields(integration)) {
|
||||
toast({
|
||||
title: 'Missing credentials',
|
||||
description: `Configure ${integration.name} credentials before connecting`,
|
||||
variant: 'destructive',
|
||||
});
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
const newStatus =
|
||||
integration.status === ConnStatus.CONNECTED ? ConnStatus.DISCONNECTED : ConnStatus.CONNECTED;
|
||||
preferences.updateIntegration(integration.id, { status: newStatus });
|
||||
setIntegrations(preferences.getIntegrations());
|
||||
toast({
|
||||
title: newStatus === ConnStatus.CONNECTED ? 'Connected' : 'Disconnected',
|
||||
description: `${integration.name} ${newStatus}`,
|
||||
});
|
||||
};
|
||||
```
|
||||
|
||||
**Verify**:
|
||||
- [ ] Toggling integration without credentials shows error toast
|
||||
- [ ] Toggling integration with valid credentials succeeds
|
||||
- [ ] Disconnecting always works (no validation needed)
|
||||
|
||||
### 2.3 Add Credential Warning Indicator (Optional Enhancement)
|
||||
|
||||
**File**: Find the integration card/list component
|
||||
|
||||
Add visual indicator for "connected" integrations missing credentials:
|
||||
|
||||
```typescript
|
||||
import { AlertTriangle } from 'lucide-react';
|
||||
import { hasRequiredIntegrationFields } from '@/lib/integration-utils';
|
||||
|
||||
// Near the status indicator:
|
||||
{integration.status === 'connected' && !hasRequiredIntegrationFields(integration) && (
|
||||
<Tooltip>
|
||||
<TooltipTrigger>
|
||||
<AlertTriangle className="h-4 w-4 text-warning" />
|
||||
</TooltipTrigger>
|
||||
<TooltipContent>
|
||||
Credentials missing - integration may not work correctly
|
||||
</TooltipContent>
|
||||
</Tooltip>
|
||||
)}
|
||||
```
|
||||
|
||||
**Verify**:
|
||||
- [ ] Warning icon appears on misconfigured integrations
|
||||
- [ ] Tooltip provides helpful guidance
|
||||
|
||||
---
|
||||
|
||||
## Phase 3: E2E Test Enrichment (Priority: P2)
|
||||
|
||||
Use existing fixtures from `client/e2e/fixtures.ts` (`callAPI`, `navigateTo`, `waitForAPI`).
|
||||
|
||||
### 3.1 Add Connection Persistence Test
|
||||
|
||||
**File**: `client/e2e/connection-persistence.spec.ts` (NEW FILE)
|
||||
|
||||
```typescript
|
||||
import { test, expect } from '@playwright/test';
|
||||
import { navigateTo, waitForAPI } from './fixtures';
|
||||
|
||||
const shouldRun = process.env.NOTEFLOW_E2E === '1';
|
||||
|
||||
test.describe('Connection Address Persistence', () => {
|
||||
test.skip(!shouldRun, 'Set NOTEFLOW_E2E=1 to enable end-to-end tests.');
|
||||
|
||||
test('server address persists across navigation', async ({ page }) => {
|
||||
await navigateTo(page, '/settings?tab=status');
|
||||
await waitForAPI(page);
|
||||
|
||||
const hostInput = page.locator('#host');
|
||||
const portInput = page.locator('#port');
|
||||
|
||||
await hostInput.clear();
|
||||
await hostInput.fill('127.0.0.1');
|
||||
await portInput.clear();
|
||||
await portInput.fill('50051');
|
||||
|
||||
// Navigate away and back
|
||||
await page.goto('/');
|
||||
await page.waitForLoadState('networkidle');
|
||||
await navigateTo(page, '/settings?tab=status');
|
||||
|
||||
// Verify values persisted
|
||||
await expect(hostInput).toHaveValue('127.0.0.1');
|
||||
await expect(portInput).toHaveValue('50051');
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
### 3.2 Add Integration State Test
|
||||
|
||||
**File**: `client/e2e/integration-state.spec.ts` (NEW FILE)
|
||||
|
||||
```typescript
|
||||
import { test, expect } from '@playwright/test';
|
||||
import { navigateTo, waitForAPI } from './fixtures';
|
||||
|
||||
const shouldRun = process.env.NOTEFLOW_E2E === '1';
|
||||
|
||||
test.describe('Integration State', () => {
|
||||
test.skip(!shouldRun, 'Set NOTEFLOW_E2E=1 to enable end-to-end tests.');
|
||||
|
||||
test('integration list renders without errors', async ({ page }) => {
|
||||
await navigateTo(page, '/settings?tab=integrations');
|
||||
await waitForAPI(page);
|
||||
|
||||
// Tab content should be visible
|
||||
await expect(page.locator('.settings-tab-content')).toBeVisible();
|
||||
|
||||
// No error toasts on load
|
||||
const errorToast = page.locator('[role="alert"]').filter({ hasText: /error|failed/i });
|
||||
await expect(errorToast).not.toBeVisible({ timeout: 3000 }).catch(() => {});
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
**Verify**:
|
||||
- [ ] Run `NOTEFLOW_E2E=1 npx playwright test connection-persistence integration-state`
|
||||
- [ ] Tests pass
|
||||
|
||||
---
|
||||
|
||||
## Phase 4: Logging Improvements (Priority: P2)
|
||||
|
||||
### 4.1 Audit contextlib.suppress Usage
|
||||
|
||||
Run this search to find all suppress usages:
|
||||
|
||||
```bash
|
||||
grep -r "contextlib.suppress" src/noteflow/ --include="*.py" -n
|
||||
```
|
||||
|
||||
For each occurrence, evaluate:
|
||||
- [ ] Is the exception being logged before suppression?
|
||||
- [ ] Should this be a try/except with logging instead?
|
||||
|
||||
### 4.2 Add Logging to Silent Returns
|
||||
|
||||
Search for silent returns in critical paths:
|
||||
|
||||
```bash
|
||||
grep -rn "return$" src/noteflow/grpc/_mixins/ --include="*.py" | head -20
|
||||
```
|
||||
|
||||
For each early return:
|
||||
- [ ] Add `logger.debug()` explaining why the return happened
|
||||
- [ ] Include relevant context (IDs, state)
|
||||
|
||||
### 4.3 Document Logging Standards
|
||||
|
||||
**File**: `docs/development/logging-standards.md` (NEW FILE)
|
||||
|
||||
```markdown
|
||||
# Logging Standards
|
||||
|
||||
## Principles
|
||||
|
||||
1. **Never suppress without logging**: Replace `contextlib.suppress()` with explicit try/except
|
||||
2. **Log early returns**: Any function that returns early should log at DEBUG level
|
||||
3. **Structured logging**: Always include context (IDs, operation names)
|
||||
4. **Appropriate levels**:
|
||||
- ERROR: Unexpected failures
|
||||
- WARNING: Expected failures (validation errors)
|
||||
- INFO: Significant state changes
|
||||
- DEBUG: Operational flow details
|
||||
|
||||
## Examples
|
||||
|
||||
### Before (Bad)
|
||||
```python
|
||||
with contextlib.suppress(Exception):
|
||||
await risky_operation()
|
||||
```
|
||||
|
||||
### After (Good)
|
||||
```python
|
||||
try:
|
||||
await risky_operation()
|
||||
except SpecificError as e:
|
||||
logger.warning("Expected error in operation", error=str(e))
|
||||
except Exception as e:
|
||||
logger.error("Unexpected error in operation", error=str(e), exc_info=True)
|
||||
```
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Validation Checklist
|
||||
|
||||
After completing all phases:
|
||||
|
||||
- [ ] `make quality` passes
|
||||
- [ ] `make e2e` passes (or tests appropriately skipped)
|
||||
- [ ] Server address persists across navigation (manual test)
|
||||
- [ ] Integration warning badges appear correctly
|
||||
- [ ] No new TypeScript or Python lint errors
|
||||
|
||||
## Rollback Plan
|
||||
|
||||
If issues arise:
|
||||
1. Revert changes to `Settings.tsx`
|
||||
2. Remove new test files (they don't affect production)
|
||||
3. Keep documentation updates
|
||||
@@ -0,0 +1,595 @@
|
||||
# SPRINT-GAP-012: State Synchronization & Observability Gaps
|
||||
|
||||
| Attribute | Value |
|
||||
|-----------|-------|
|
||||
| **Sprint** | GAP-012 |
|
||||
| **Size** | L (Large) |
|
||||
| **Owner** | TBD |
|
||||
| **Phase** | Hardening |
|
||||
| **Prerequisites** | None |
|
||||
|
||||
## Executive Summary
|
||||
|
||||
Investigation revealed multiple gaps in state synchronization, observability, and test coverage that cause confusing user experiences and difficult debugging:
|
||||
|
||||
1. **Server Address Timing Bug**: After saving a new server address and navigating away, the displayed address resets to the previous value despite successful persistence
|
||||
2. **OAuth State Consistency**: Integrations display "connected" status with blank OAuth fields, causing silent sync failures
|
||||
3. **Server Logging Gaps**: Silent failures throughout the codebase make debugging impossible
|
||||
4. **E2E Test Coverage**: Tests validate structure but not functional round-trips
|
||||
|
||||
## Open Issues
|
||||
|
||||
- [ ] Define canonical source of truth for server address (localStorage vs Rust state vs gRPC client)
|
||||
- [ ] Determine OAuth credential loading strategy (eager vs lazy)
|
||||
- [ ] Establish logging standards for failure modes
|
||||
- [ ] Define e2e test functional coverage requirements
|
||||
|
||||
---
|
||||
|
||||
## Issue 1: Server Address Timing/Synchronization Bug
|
||||
|
||||
### Symptom
|
||||
|
||||
User saves server address `127.0.0.1:50051`, connection succeeds, but navigating to another page and back shows `192.168.50.151:50051` (previous value).
|
||||
|
||||
### Root Cause Analysis
|
||||
|
||||
The server address exists in **three separate locations** with different update timings:
|
||||
|
||||
| Location | Update Trigger | Read On |
|
||||
|----------|---------------|---------|
|
||||
| TypeScript localStorage (`noteflow_preferences`) | `preferences.setServerConnection()` | Page mount via `preferences.get()` |
|
||||
| Rust `AppState.preferences` | `save_preferences` IPC command | `get_effective_server_url` IPC command |
|
||||
| gRPC client internal `endpoint` RwLock | `connect()` IPC command | Connection health checks |
|
||||
|
||||
### Race Condition in `handleConnect()`
|
||||
|
||||
**File**: `client/src/pages/Settings.tsx` (lines 260-289)
|
||||
|
||||
```typescript
|
||||
const handleConnect = async () => {
|
||||
setIsConnecting(true);
|
||||
try {
|
||||
const normalized = normalizeServerInput(serverHost, serverPort);
|
||||
// Step 1: Update local state
|
||||
if (normalized.host !== serverHost) setServerHost(normalized.host);
|
||||
if (normalized.port !== serverPort) setServerPort(normalized.port);
|
||||
|
||||
// Step 2: Persist to localStorage (sync)
|
||||
if (normalized.host || normalized.port) {
|
||||
preferences.setServerConnection(normalized.host, normalized.port);
|
||||
}
|
||||
|
||||
// Step 3: Persist to Rust state (async, may race)
|
||||
if (isTauriEnvironment()) {
|
||||
await getAPI().savePreferences(preferences.get());
|
||||
}
|
||||
|
||||
// Step 4: Connect via gRPC (updates gRPC client endpoint)
|
||||
const api = getAPI();
|
||||
const info = await api.connect(buildServerUrl(normalized.host, normalized.port));
|
||||
|
||||
setIsConnected(true);
|
||||
setServerInfo(info);
|
||||
// BUG: effectiveServerUrl is NOT re-fetched here
|
||||
} catch (error) {
|
||||
// ...
|
||||
}
|
||||
};
|
||||
```
|
||||
|
||||
### Critical Bug
|
||||
|
||||
After `handleConnect()` completes:
|
||||
- `effectiveServerUrl` state still holds the **old** value from mount
|
||||
- When user navigates away and the component unmounts, then returns:
|
||||
- `checkConnection()` fetches `effectiveServerUrl` from Rust state
|
||||
- Rust state may read from different source based on priority logic in `get_effective_server_url`
|
||||
|
||||
**File**: `client/src-tauri/src/commands/connection.rs` (lines 100-121)
|
||||
|
||||
```rust
|
||||
pub fn get_effective_server_url(state: State<'_, Arc<AppState>>) -> EffectiveServerUrl {
|
||||
let prefs = state.preferences.read(); // Reads from Rust state
|
||||
let cfg = config();
|
||||
let prefs_url = format!("{}:{}", prefs.server_host, prefs.server_port);
|
||||
let default_url = &cfg.server.default_address;
|
||||
|
||||
// If prefs match default, returns environment/default instead
|
||||
if !prefs.server_host.is_empty() && prefs_url != *default_url {
|
||||
return EffectiveServerUrl {
|
||||
url: prefs_url,
|
||||
source: ServerAddressSource::Preferences,
|
||||
};
|
||||
}
|
||||
EffectiveServerUrl {
|
||||
url: default_url.clone(),
|
||||
source: cfg.server.address_source,
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Proposed Fix
|
||||
|
||||
**After successful connect, re-fetch `effectiveServerUrl`**:
|
||||
|
||||
```typescript
|
||||
// In handleConnect(), after setServerInfo(info):
|
||||
const urlInfo = await api.getEffectiveServerUrl();
|
||||
setEffectiveServerUrl(urlInfo);
|
||||
```
|
||||
|
||||
This ensures the displayed URL reflects the actual connected state after all async operations complete.
|
||||
|
||||
### Validation Test
|
||||
|
||||
```typescript
|
||||
// e2e test to add
|
||||
test('server address persists across navigation', async ({ page }) => {
|
||||
await page.goto('/settings?tab=status');
|
||||
await page.fill('#host', '127.0.0.1');
|
||||
await page.fill('#port', '50051');
|
||||
await page.click('button:has-text("Connect")');
|
||||
await expect(page.getByText('Connected')).toBeVisible();
|
||||
|
||||
// Navigate away and back
|
||||
await page.goto('/');
|
||||
await page.goto('/settings?tab=status');
|
||||
|
||||
// Verify persisted values
|
||||
await expect(page.locator('#host')).toHaveValue('127.0.0.1');
|
||||
await expect(page.locator('#port')).toHaveValue('50051');
|
||||
});
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Issue 2: OAuth/Integration State Consistency
|
||||
|
||||
### Symptom
|
||||
|
||||
Integrations display "connected" status without valid credentials, causing:
|
||||
1. UI confusion (users see "connected" but credentials are missing)
|
||||
2. Potential failures when OAuth operations are attempted
|
||||
|
||||
**Note**: The sync scheduler only processes `calendar` and `pkm` integration types, so auth/oidc integrations with blank credentials don't cause sync failures directly.
|
||||
|
||||
### Root Cause Analysis
|
||||
|
||||
Primary vulnerability and contributing factors:
|
||||
|
||||
#### 2.0 Direct Status Toggle Without Validation (PRIMARY)
|
||||
|
||||
**File**: `client/src/components/settings/integrations-section.tsx` (lines 122-131)
|
||||
|
||||
```typescript
|
||||
const handleIntegrationToggle = (integration: Integration) => {
|
||||
const newStatus =
|
||||
integration.status === ConnStatus.CONNECTED ? ConnStatus.DISCONNECTED : ConnStatus.CONNECTED;
|
||||
preferences.updateIntegration(integration.id, { status: newStatus });
|
||||
// NO VALIDATION - allows "connected" without credentials
|
||||
};
|
||||
```
|
||||
|
||||
This is the primary entry point for invalid state. Users can toggle integrations to "connected" without any credential validation.
|
||||
|
||||
#### 2.1 Empty OAuth Defaults
|
||||
|
||||
**File**: `client/src/lib/default-integrations.ts` (lines 21, 41-67)
|
||||
|
||||
```typescript
|
||||
const emptyOAuth = { client_id: '', client_secret: '', redirect_uri: '', scopes: [] as string[] };
|
||||
|
||||
// Used in:
|
||||
createIntegration('Google SSO', 'auth', {
|
||||
oauth_config: { ...emptyOAuth, scopes: ['openid', 'email', 'profile'] },
|
||||
}),
|
||||
```
|
||||
|
||||
Integrations start with empty credentials by design, but status can become "connected" through various paths.
|
||||
|
||||
#### 2.2 Status Updated Without Credential Validation
|
||||
|
||||
**File**: `client/src/lib/preferences.ts` (updateIntegration function)
|
||||
|
||||
The `updateIntegration()` function merges partial updates without validating that required fields for "connected" status are present:
|
||||
|
||||
```typescript
|
||||
updateIntegration(id: string, updates: Partial<Integration>): void {
|
||||
withPreferences((prefs) => {
|
||||
const idx = prefs.integrations.findIndex((i) => i.id === id);
|
||||
if (idx !== -1) {
|
||||
prefs.integrations[idx] = { ...prefs.integrations[idx], ...updates };
|
||||
// No validation that connected status has valid oauth_config
|
||||
}
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
#### 2.3 Type System Allows Invalid States
|
||||
|
||||
**File**: `client/src/api/types.ts`
|
||||
|
||||
The `Integration` type allows `status: 'connected'` without requiring `oauth_config` to be populated:
|
||||
|
||||
```typescript
|
||||
interface Integration {
|
||||
id: string;
|
||||
name: string;
|
||||
type: IntegrationType;
|
||||
status: IntegrationStatus; // Can be 'connected'
|
||||
oauth_config?: OAuthConfig; // Optional - allows empty
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
#### 2.4 Secrets Stored Separately from Integration Objects
|
||||
|
||||
OAuth secrets are encrypted and stored separately via `useSecureIntegrationSecrets`, but the integration object may be loaded without secrets:
|
||||
|
||||
```typescript
|
||||
// In Settings.tsx loadEncryptedApiKeys():
|
||||
const integrationsWithSecrets = await loadAllSecrets(preferences.getIntegrations());
|
||||
setIntegrations(integrationsWithSecrets);
|
||||
// If loadAllSecrets fails silently, integrations remain without credentials
|
||||
```
|
||||
|
||||
#### 2.5 Backend Separation of Secrets
|
||||
|
||||
The backend stores OAuth secrets separately from Integration records (IntegrationSecretModel). When fetching integrations, secrets may not be joined, leading to "connected" integrations without credentials.
|
||||
|
||||
#### 2.6 No Credential Validation on Sync Trigger
|
||||
|
||||
**File**: `client/src/hooks/use-integration-sync.ts`
|
||||
|
||||
The sync scheduler triggers sync for any integration with `status === 'connected'` without validating credentials exist:
|
||||
|
||||
```typescript
|
||||
// In triggerSync():
|
||||
if (integration.status !== 'connected') {
|
||||
return { success: false, error: 'Integration not connected' };
|
||||
}
|
||||
// No check for oauth_config.client_id presence
|
||||
```
|
||||
|
||||
#### 2.7 Graceful Error Handling Masks Root Cause
|
||||
|
||||
Sync failures are caught and displayed as toast errors, but the underlying "connected without credentials" state persists, causing repeated failures.
|
||||
|
||||
### Proposed Fixes
|
||||
|
||||
1. **Extend existing validation helper** (`client/src/lib/integration-utils.ts`):
|
||||
|
||||
The file already has `hasRequiredIntegrationFields()` - extend it to add OIDC support and a convenience wrapper:
|
||||
```typescript
|
||||
// Add OIDC case to existing switch statement
|
||||
case 'oidc':
|
||||
return !!(integration.oidc_config?.issuer_url && integration.oidc_config?.client_id);
|
||||
|
||||
// Add convenience wrapper below existing function
|
||||
export const isEffectivelyConnected = (integration: Integration): boolean =>
|
||||
integration.status === 'connected' && hasRequiredIntegrationFields(integration);
|
||||
```
|
||||
|
||||
2. **Validate in handleIntegrationToggle** (`integrations-section.tsx`):
|
||||
```typescript
|
||||
import { hasRequiredIntegrationFields } from '@/lib/integration-utils';
|
||||
|
||||
const handleIntegrationToggle = (integration: Integration) => {
|
||||
if (integration.status === ConnStatus.DISCONNECTED) {
|
||||
// Trying to connect - validate first
|
||||
if (!hasRequiredIntegrationFields(integration)) {
|
||||
toast({
|
||||
title: 'Missing credentials',
|
||||
description: `Configure ${integration.name} credentials before connecting`,
|
||||
variant: 'destructive',
|
||||
});
|
||||
return;
|
||||
}
|
||||
}
|
||||
const newStatus =
|
||||
integration.status === ConnStatus.CONNECTED ? ConnStatus.DISCONNECTED : ConnStatus.CONNECTED;
|
||||
preferences.updateIntegration(integration.id, { status: newStatus });
|
||||
// ... rest unchanged
|
||||
};
|
||||
```
|
||||
|
||||
3. **UI indicator for missing credentials**:
|
||||
Display warning badge on integrations that are "connected" but fail `hasRequiredIntegrationFields()`.
|
||||
|
||||
### Affected Integrations
|
||||
|
||||
| Integration | OAuth Required | Credential Source |
|
||||
|-------------|---------------|-------------------|
|
||||
| Google SSO | Yes | User-provided |
|
||||
| Microsoft 365 | Yes | User-provided |
|
||||
| Google Calendar | Yes | OAuth flow |
|
||||
| Outlook Calendar | Yes | OAuth flow |
|
||||
| Authentik (OIDC) | Yes | User-provided |
|
||||
| Keycloak (OIDC) | Yes | User-provided |
|
||||
|
||||
---
|
||||
|
||||
## Issue 3: Server Logging Gaps
|
||||
|
||||
### Symptom
|
||||
|
||||
Server operates silently during failures. When issues occur, there's no log trail to diagnose problems.
|
||||
|
||||
### Identified Logging Gaps
|
||||
|
||||
| Location | Pattern | Impact |
|
||||
|----------|---------|--------|
|
||||
| `contextlib.suppress()` usage | Exceptions swallowed entirely | Failures invisible |
|
||||
| Silent returns in ASR processing | `return` without logging | Audio processing gaps undetectable |
|
||||
| Fire-and-forget webhook delivery | Background task failures lost | Webhook reliability unknown |
|
||||
| gRPC interceptor exceptions | Caught and converted to status codes | Root cause hidden |
|
||||
| Database connection errors | Connection pool handles silently | Connection issues masked |
|
||||
| Optional service initialization | Services disabled without warning | Missing features unclear |
|
||||
|
||||
### Examples of Silent Failures
|
||||
|
||||
#### 3.1 ASR Processing Silent Return
|
||||
|
||||
```python
|
||||
# src/noteflow/grpc/_mixins/streaming/_asr.py (approximate)
|
||||
async def _process_audio_chunk(self, chunk: bytes) -> None:
|
||||
if not self._asr_engine:
|
||||
return # Silent - no log that ASR is unavailable
|
||||
# ...
|
||||
```
|
||||
|
||||
**Fix**:
|
||||
```python
|
||||
async def _process_audio_chunk(self, chunk: bytes) -> None:
|
||||
if not self._asr_engine:
|
||||
logger.debug("ASR engine unavailable, skipping chunk processing")
|
||||
return
|
||||
# ...
|
||||
```
|
||||
|
||||
#### 3.2 contextlib.suppress Overuse
|
||||
|
||||
```python
|
||||
# Multiple locations
|
||||
with contextlib.suppress(Exception):
|
||||
await potentially_failing_operation()
|
||||
# Failure completely invisible
|
||||
```
|
||||
|
||||
**Fix**: Replace with explicit exception handling:
|
||||
```python
|
||||
try:
|
||||
await potentially_failing_operation()
|
||||
except SpecificException as e:
|
||||
logger.warning("Operation failed", error=str(e), exc_info=True)
|
||||
except Exception as e:
|
||||
logger.error("Unexpected failure in operation", error=str(e), exc_info=True)
|
||||
```
|
||||
|
||||
#### 3.3 Webhook Fire-and-Forget
|
||||
|
||||
```python
|
||||
# Background webhook delivery
|
||||
asyncio.create_task(self._deliver_webhook(config, payload))
|
||||
# Task failures are never observed
|
||||
```
|
||||
|
||||
**Fix**:
|
||||
```python
|
||||
async def _deliver_with_logging(self, config, payload):
|
||||
try:
|
||||
await self._deliver_webhook(config, payload)
|
||||
except Exception as e:
|
||||
logger.error("Webhook delivery failed",
|
||||
webhook_id=config.id,
|
||||
error=str(e),
|
||||
exc_info=True)
|
||||
|
||||
asyncio.create_task(self._deliver_with_logging(config, payload))
|
||||
```
|
||||
|
||||
### Proposed Logging Standards
|
||||
|
||||
1. **Always log on early returns**: Any function that returns early due to missing prerequisites should log at DEBUG level
|
||||
2. **Never suppress without logging**: Replace `contextlib.suppress()` with try/except that logs
|
||||
3. **Log background task lifecycle**: Log when background tasks start and when they complete/fail
|
||||
4. **Structured logging for all failures**: Include context (IDs, operation names, parameters)
|
||||
5. **Log at appropriate levels**:
|
||||
- ERROR: Unexpected failures that need investigation
|
||||
- WARNING: Expected failures (validation, user errors)
|
||||
- INFO: Significant state changes
|
||||
- DEBUG: Detailed operational flow
|
||||
|
||||
### Validation
|
||||
|
||||
Add log assertion tests:
|
||||
```python
|
||||
def test_asr_unavailable_logs_debug(caplog):
|
||||
with caplog.at_level(logging.DEBUG):
|
||||
await process_chunk_without_asr()
|
||||
assert "ASR engine unavailable" in caplog.text
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Issue 4: E2E Test Coverage Gaps
|
||||
|
||||
### Current State
|
||||
|
||||
- ~10 Playwright test files in `client/e2e/`
|
||||
- `connection.spec.ts` validates basic API round-trips (`getServerInfo`, `isConnected`)
|
||||
- Tests validate page structure, navigation, and some API responses
|
||||
|
||||
### Coverage Gaps
|
||||
|
||||
| Gap | Description | Risk |
|
||||
|-----|-------------|------|
|
||||
| No response data validation | Tests check elements exist, not content correctness | Data corruption undetected |
|
||||
| No state persistence tests | Navigation doesn't verify localStorage/Rust state sync | State bugs undetected |
|
||||
| No error recovery tests | Happy path only | Error handling untested |
|
||||
| No connection lifecycle tests | Connect/disconnect/reconnect not tested | Connection bugs undetected |
|
||||
| No OAuth flow tests | Integration auth flows untested | Auth bugs undetected |
|
||||
| No sync operation tests | Calendar/integration sync untested | Sync failures undetected |
|
||||
|
||||
### Recommended Test Additions
|
||||
|
||||
#### 4.1 Connection Round-Trip Test
|
||||
|
||||
```typescript
|
||||
// client/e2e/connection-roundtrip.spec.ts
|
||||
import { test, expect } from './fixtures';
|
||||
import { callAPI, navigateTo, waitForAPI } from './fixtures';
|
||||
|
||||
test.describe('Connection Round-Trip', () => {
|
||||
test.beforeEach(async ({ page }) => {
|
||||
await navigateTo(page, '/settings?tab=status');
|
||||
await waitForAPI(page);
|
||||
});
|
||||
|
||||
test('connects to server and validates response', async ({ page }) => {
|
||||
// Set server address
|
||||
await page.fill('#host', '127.0.0.1');
|
||||
await page.fill('#port', '50051');
|
||||
|
||||
// Connect
|
||||
await page.click('button:has-text("Connect")');
|
||||
|
||||
// Validate response data via existing callAPI fixture
|
||||
const serverInfo = await callAPI(page, 'getServerInfo');
|
||||
expect(serverInfo).toHaveProperty('version');
|
||||
expect(serverInfo).toHaveProperty('asr_model');
|
||||
});
|
||||
|
||||
test('persists connection across navigation', async ({ page }) => {
|
||||
await page.fill('#host', '127.0.0.1');
|
||||
await page.fill('#port', '50051');
|
||||
await page.click('button:has-text("Connect")');
|
||||
await expect(page.getByText('Connected')).toBeVisible();
|
||||
|
||||
// Navigate away
|
||||
await page.goto('/meetings');
|
||||
await page.waitForLoadState('networkidle');
|
||||
|
||||
// Navigate back
|
||||
await page.goto('/settings?tab=status');
|
||||
|
||||
// Should still show connected with correct address
|
||||
await expect(page.locator('#host')).toHaveValue('127.0.0.1');
|
||||
await expect(page.locator('#port')).toHaveValue('50051');
|
||||
});
|
||||
|
||||
test('handles connection failure gracefully', async ({ page }) => {
|
||||
await page.fill('#host', 'invalid.host.local');
|
||||
await page.fill('#port', '99999');
|
||||
|
||||
await page.click('button:has-text("Connect")');
|
||||
|
||||
// Should show error toast or remain disconnected, not crash
|
||||
await expect(page.getByText('Not connected').or(page.getByText('Connection Failed'))).toBeVisible();
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
#### 4.2 Integration State Test
|
||||
|
||||
```typescript
|
||||
// client/e2e/integration-state.spec.ts
|
||||
import { test, expect } from './fixtures';
|
||||
import { navigateTo, waitForAPI } from './fixtures';
|
||||
|
||||
test.describe('Integration State Consistency', () => {
|
||||
test.beforeEach(async ({ page }) => {
|
||||
await navigateTo(page, '/settings?tab=integrations');
|
||||
await waitForAPI(page);
|
||||
});
|
||||
|
||||
test('integrations tab loads without errors', async ({ page }) => {
|
||||
// Verify tab content is visible
|
||||
await expect(page.locator('.settings-tab-content')).toBeVisible();
|
||||
// No error toasts should appear on load
|
||||
const errorToast = page.locator('[role="alert"]').filter({ hasText: /error|failed/i });
|
||||
await expect(errorToast).not.toBeVisible({ timeout: 3000 }).catch(() => {});
|
||||
});
|
||||
|
||||
test('integration without credentials shows warning when toggled', async ({ page }) => {
|
||||
// Find any integration toggle button
|
||||
const toggleButton = page.locator('button').filter({ hasText: /connect/i }).first();
|
||||
|
||||
if (await toggleButton.isVisible()) {
|
||||
await toggleButton.click();
|
||||
|
||||
// Should show validation toast if credentials missing
|
||||
// (after fix is implemented)
|
||||
const toast = page.locator('[role="alert"]');
|
||||
// Either success or "missing credentials" warning
|
||||
await expect(toast).toBeVisible({ timeout: 5000 }).catch(() => {});
|
||||
}
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
#### 4.3 Recording Round-Trip Test
|
||||
|
||||
```typescript
|
||||
// client/e2e/recording-roundtrip.spec.ts
|
||||
import { test, expect } from './fixtures';
|
||||
import { callAPI, navigateTo, waitForAPI } from './fixtures';
|
||||
|
||||
test.describe('Recording Round-Trip', () => {
|
||||
test.skip(true, 'Requires running backend with audio capture');
|
||||
|
||||
test('recording page loads', async ({ page }) => {
|
||||
await navigateTo(page, '/recording');
|
||||
await waitForAPI(page);
|
||||
|
||||
// Verify recording UI is present
|
||||
const recordButton = page.locator('button').filter({ hasText: /start|record/i });
|
||||
await expect(recordButton).toBeVisible();
|
||||
});
|
||||
|
||||
test('can list meetings via API', async ({ page }) => {
|
||||
await navigateTo(page, '/');
|
||||
await waitForAPI(page);
|
||||
|
||||
// Verify API round-trip works
|
||||
const meetings = await callAPI(page, 'listMeetings');
|
||||
expect(Array.isArray(meetings)).toBe(true);
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
### Test Infrastructure
|
||||
|
||||
The existing `client/e2e/fixtures.ts` already provides:
|
||||
- `callAPI<T>(page, method, ...args)` - Call API methods and return typed results
|
||||
- `navigateTo(page, path)` - Navigate with proper waiting
|
||||
- `waitForAPI(page)` - Wait for API to be available
|
||||
- `getConnectionState(page)` - Get current connection state
|
||||
|
||||
These fixtures should be used consistently across all new tests.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Priority
|
||||
|
||||
| Issue | Priority | Effort | Impact |
|
||||
|-------|----------|--------|--------|
|
||||
| Server Address Timing | P1 | Small | High - User-facing bug |
|
||||
| OAuth State Consistency | P1 | Medium | High - Silent failures |
|
||||
| E2E Round-Trip Tests | P2 | Medium | Medium - Regression prevention |
|
||||
| Server Logging Gaps | P2 | Large | Medium - Debugging capability |
|
||||
|
||||
## Success Criteria
|
||||
|
||||
1. Server address persists correctly across navigation (manual test + e2e test)
|
||||
2. Integrations cannot reach "connected" status without valid credentials
|
||||
3. All gRPC round-trips have corresponding e2e tests
|
||||
4. No `contextlib.suppress()` without logging in server code
|
||||
5. `make quality` passes
|
||||
|
||||
## References
|
||||
|
||||
- GAP-003: Error Handling Patterns
|
||||
- GAP-004: Diarization Lifecycle
|
||||
- GAP-011: Post-Processing Pipeline
|
||||
Reference in New Issue
Block a user