Draft: Feat/mock frontend objects

[thaorell]

No description provided.

[github-actions]

Code Review: Mock Frontend Objects & Onboarding Wizard

This PR introduces a comprehensive mock data infrastructure for standalone frontend development and adds an onboarding wizard. Overall, this is excellent work with solid architecture and security practices.

Key Strengths

• Well-organized mock handler structure in src/lib/mocks/handlers/
• Zero any types in TypeScript code
• Proper OAuth implementation with HMAC validation and state expiration
• Comprehensive documentation in handlers/README.md
• Good UX patterns with loading states and progress indicators

Issues to Address Before Merge

1. RBAC: Overly Permissive ConfigMap Access

File: components/manifests/rbac/backend-clusterrole.yaml:48-50
Issue: Backend can update ALL ConfigMaps in all namespaces
Fix: Limit to specific ConfigMap name github-app-installations using resourceNames

2. Backend: Type Assertion Safety

File: components/backend/handlers/github_auth.go:152
Issue: userID type assertion without checking type
Fix: Use two-value assertion: userIDStr, ok := userID.(string)

3. Backend: Missing Error Context

File: components/backend/handlers/github_auth.go:292-325
Issue: ConfigMap retry logic doesn't log which attempt failed
Fix: Add retry counter to error messages

4. Frontend: Silent Error Handling

File: components/frontend/src/lib/mocks/handlers/github.ts:29
Issue: Empty catch block hides JSON parsing errors
Fix: Add console.warn for debugging

Recommendations for Follow-up

• Add wizard state persistence to localStorage for better UX
• Display mutation errors inline with Alert components
• Add tests for OAuth flow edge cases
• Use relative timestamps in mock data instead of hardcoded dates
• Extract magic numbers to constants (10*time.Minute, etc)
• Add rate limiting to /auth/github/install endpoint

Summary

Files: 51 changed (+2,693, -16)
Critical Issues: 0
Must Fix: 4 (listed above)

Recommendation: Approve with changes requested

This is solid work! The mock infrastructure is well-designed and the OAuth implementation is secure. Please address the 4 issues above before merging.

[github-actions]

Code Review: Mock Frontend Objects & Onboarding Feature

Thank you for this comprehensive PR! I've reviewed the changes and have feedback organized by category.

---

✅ Strengths

1. Excellent Architecture

• Clean separation of mock logic into centralized handlers (src/lib/mocks/handlers/)
• Well-documented mock system with comprehensive README
• Proper TypeScript typing throughout mock implementations
• Environment-based feature flagging (USE_MOCKS) is clean and consistent

2. Frontend Standards Compliance

• All components use Shadcn UI components (Button, Card, Progress, etc.)
• React Query properly used for data mutations
• Loading states and error handling implemented correctly
• Type-safe with zero any types in new code

3. User Experience

• Onboarding wizard has excellent UX with progress tracking
• Proper empty states and loading indicators
• GitHub OAuth flow properly handles redirect URIs with parameters

---

🔴 Critical Issues

1. Bug in OnboardingWizard.tsx (Line 83-87)

const handleGitHubVerified = useCallback(() => {
  setWizardData((prev) => ({
    ...prev,
    githubConnected: false,  // ❌ BUG: Should be true!
  }));
}, []);

Issue: When GitHub is successfully connected, you're setting githubConnected: false. This prevents users from proceeding to the next step.

Fix: Change to githubConnected: true

Impact: HIGH - Blocks onboarding flow progression

---

2. Backend RBAC Permissions Missing

In components/manifests/rbac/backend-clusterrole.yaml, you added permissions for projectsettings and rfeworkflows but these may not be necessary if the backend only needs to manage basic resources.

Review Needed:

• Are these permissions actually used by the backend service account?
• Per CLAUDE.md standards, backend should use user tokens for most operations
• Only use service account for CR writes and token minting

---

3. Incomplete TODO in IntegrationsClient.tsx (Line 26)

// TODO actually invalidate the github application remotely

Issue: Disconnect function removes local state but doesn't revoke GitHub App installation remotely. Users may think they've disconnected but the app still has access.

Recommendation: Either implement proper revocation or document this limitation clearly in the UI.

---

⚠️ Medium Priority Issues

4. GitHub Setup Page: Missing Error Recovery

src/app/integrations/github/setup/page.tsx redirects automatically but lacks retry logic or manual recovery if GitHub redirect fails.

Suggestion: Add a "Retry" button when errors occur instead of just showing error message.

---

5. Mock Data Persistence

Mock projects/sessions are stored in module-level arrays, so any data created during dev is lost on refresh.

Suggestion: Consider localStorage persistence for better dev experience:

// Save to localStorage after mutations
localStorage.setItem('mockProjects', JSON.stringify(mockProjects));

---

6. Environment Variable Consistency

.env.example shows:

GITHUB_APP_SLUG=ambient-code-local

But package.json dev script uses:

GITHUB_APP_SLUG=ambient-code

Impact: Could cause confusion. Recommend aligning these.

---

7. Backend GitHub OAuth: Parameter Handling

In github_auth.go:214-225, the onboarding parameter preservation is good, but consider security:

onboarding := c.Query("onboarding")
if onboarding != "" {
  // No validation of onboarding parameter value

Recommendation: Validate that onboarding is a boolean or expected value to prevent injection attacks.

---

💡 Best Practice Suggestions

8. Component Size

OnboardingWizard.tsx is 267 lines. Per frontend guidelines, components over 200 lines should be broken down.

Suggestion: Extract step navigation logic into a custom hook:

// hooks/useWizardNavigation.ts
export function useWizardNavigation(steps, wizardData) {
  // Move navigation logic here
}

---

9. Error Handling in Mock Handlers

handlers/projects.ts:38 catches errors but doesn't provide specific error details:

} catch (error) {
  console.error("Mock project creation failed:", error);
  return NextResponse.json(
    { error: "Failed to create project" },
    { status: 500 }
  );
}

Suggestion: Return more specific error messages in development mode for easier debugging.

---

10. TypeScript: Missing Route Typing

Several route handlers are missing proper typing:

// Current
export async function GET(request: Request, { params }: { params: Promise<{ name: string }> })

// Better (per Next.js 15 conventions)
type RouteParams = { params: Promise<{ name: string }> };
export async function GET(request: Request, { params }: RouteParams)

---

🧪 Testing & Quality

11. Missing Tests

No tests added for:

• Onboarding wizard flow
• Mock handlers
• GitHub OAuth callback handling

Recommendation: Add at least smoke tests for critical paths before merging.

---

12. Linting Status

Unable to verify build/lint status. Please ensure:

cd components/frontend
npm run build  # Should pass with 0 errors, 0 warnings
npm run lint   # Should pass

cd components/backend  
go vet ./handlers/
golangci-lint run

---

📝 Documentation

13. README Updates Excellent

The mock mode documentation in frontend/README.md is clear and helpful. Great work!

Small suggestion: Add a section about how to add new mock data or extend existing mocks.

---

🔒 Security

14. No Security Issues Detected

• No tokens logged
• User authentication properly checked
• RBAC patterns followed in backend
• No XSS vulnerabilities in frontend

✅ Security review passed

---

📊 Summary

Overall Assessment: Good work! This PR adds significant value with the mock system and onboarding flow.

Must Fix Before Merge:

1. ❌ Critical Bug: githubConnected: false should be true in OnboardingWizard.tsx:86
2. ⚠️ Review backend RBAC permissions necessity
3. ⚠️ Complete or document the GitHub disconnect TODO

Recommended Improvements:

• Add error recovery UI for GitHub setup failures
• Break down OnboardingWizard component (>200 lines)
• Add tests for critical flows
• Align environment variable naming

Stats:

• Files Changed: 49 files
• Additions: +2,704 lines
• Deletions: -34 lines
• New Features: Mock system, Onboarding wizard, GitHub OAuth improvements

---

🚀 Next Steps

1. Fix the critical githubConnected bug
2. Run full build and linting locally
3. Address medium-priority issues as time permits
4. Consider adding basic tests
5. Ready for re-review after critical fix

Great work on this feature! The architecture is solid and the code quality is high overall. Just needs that one critical bug fix and it'll be ready to go. 🎉

[github-actions]

PR Review: Mock Frontend Objects & Onboarding Wizard

This PR introduces a comprehensive mock data system for frontend development and adds an onboarding wizard. Overall, this is excellent work that will significantly improve the developer experience.

Key Strengths

1. Excellent Architecture - Clean separation with dedicated mock handlers, zero any types found, follows component colocation rules
2. Mock System Design - Well-structured with environment toggling, configurable delays, centralized utilities
3. Frontend Standards - All API routes check USE_MOCKS, uses Shadcn UI, React Query patterns maintained
4. Onboarding Wizard - Great UX with progress indicator, proper state management, handles OAuth elegantly
5. Backend Integration - Smart enhancement to preserve onboarding parameter through OAuth flow

Critical Issues

1. Security: GitHub Disconnect TODO (IntegrationsClient.tsx:26)

The disconnect mutation succeeds but doesn't actually revoke the GitHub App installation. Users think they've disconnected but the app still has access. Please either implement the actual uninstallation via backend API or add a warning.

High Priority Issues

2. State Mutation (mocks/handlers/projects.ts:35,67)

Directly mutating mockProjects array can cause React Query cache inconsistencies. Consider sessionStorage for persistence or document that this is intentional.

3. Missing Error Boundary

No error.tsx for integrations/github/setup route. Project requires error.tsx for all routes.

4. Hardcoded GitHub URL (github/setup/page.tsx:56)

Hardcoded github.com doesn't support GitHub Enterprise. Add GITHUB_BASE_URL to env config.

5. Race Condition (OnboardingWizard.tsx:119-143)

useEffect depends on connectGitHubMutation which recreates on every render. Remove from deps or use a ref.

Medium Priority

• Missing Skeleton components for loading states (should replace plain text)
• Inconsistent error handling (use createMockError utility everywhere)
• Environment variable documentation needs update

Pre-Merge Checklist

• CRITICAL: Resolve GitHub disconnect TODO or document limitation
• Add error.tsx for GitHub setup route
• Test complete onboarding flow
• Consider state persistence for mock data
• Update README with mock mode usage

Overall Assessment

Status: ✅ Approve with Minor Changes Required

High-quality work following guidelines exceptionally well. Main blocker is the GitHub disconnect TODO. Other issues are minor and can be addressed before merge or in follow-up.

Great work! 🚀

Review by Claude Code | 2025-11-05