Refactor/code quality improvements #527
Conversation
…ments Testing Infrastructure: - Add pytest configuration with asyncio support (pytest.ini) - Add test dependencies to pyproject.toml (pytest, pytest-asyncio, pytest-cov, httpx) - Create test fixtures for async database sessions and mock users (conftest.py) Backend Tests (95 tests): - test_validators.py: Input validation for search_space_id, document_ids, connectors, research_mode, search_mode, top_k, messages, email, url, uuid - test_connector_config.py: Connector configuration validation for all connector types (Tavily, Notion, GitHub, Jira, Slack, Discord, etc.) - test_rbac.py: Security-critical RBAC tests for access control and permissions - test_connector_service.py: ConnectorService resilience and search result transformation - test_llm_service.py: LLM role constants and configuration lookup - test_blocknote_converter.py: Markdown/BlockNote conversion error handling CI/CD Improvements: - Add test.yml workflow for automated backend testing on PRs - Add dependabot.yml for automated dependency updates (pip, npm, github-actions, docker) - Fix docker-publish.yml with proper metadata-action, caching, and release triggers Code Quality: - Improve exception handling with specific exception types (SQLAlchemyError, IntegrityError) - Add structured logging to replace print statements - Tests use pytest.importorskip for graceful skipping when deps unavailable
- Add vitest configuration with React and jsdom support - Add test setup with Next.js component mocks (Image, Link, Router) - Add utility tests: cn() function, getChatTitleFromMessages() - Add pagination tests: normalizeListResponse() API normalization - Add auth utility tests: token management, authentication state - Add hook tests: useMediaQuery, useIsMobile responsive hooks - Add component tests: Button variants/sizes, Logo accessibility - Update test.yml workflow to run frontend tests on surfsense_web changes - Add test scripts to package.json (test, test:watch, test:coverage) Tests validate: - Tailwind class merging edge cases - API response format normalization - Authentication flow and token handling - Responsive design breakpoints - UI component behavior and accessibility 87 frontend tests passing
- Remove unused imports (AsyncGenerator, HTTPException, rbac functions) - Move logger initialization after all imports per PEP 8
- Add 517 tests covering schemas, routes, services, and utilities - Improve coverage from 14% to 25% - Test files added: - test_schemas.py: Pydantic schema validation tests - test_permissions.py: Permission system tests - test_document_converters.py: Document converter tests - test_page_limit_service.py: Page limit service tests - test_rbac_schemas.py: RBAC schema tests - test_rbac_utils.py: RBAC utility tests - test_routes_search_spaces.py: Search spaces route tests - test_routes_llm_config.py: LLM config route tests - test_routes_documents.py: Documents route tests - test_connector_service_extended.py: Extended connector service tests - test_llm_service_extended.py: Extended LLM service tests - test_agent_configuration.py: Agent configuration tests - test_retrievers.py: Hybrid search retriever tests - test_config.py: Config module tests - test_db_models.py: Database model tests
…d CI
- Add comprehensive unit tests for Celery tasks and connector indexers
- test_celery_tasks.py: Tests for connector task functions
- test_celery_tasks_comprehensive.py: Extended tests for schedule checker,
blocknote migration, and document reindex tasks
- test_connector_indexers_comprehensive.py: Tests for all connector indexers
(Slack, Notion, GitHub, Jira, Confluence, Linear, Discord, etc.)
- Refactor airtable authentication
- Extract refresh_airtable_token to app/utils/connector_auth.py to avoid
circular imports between routes and indexers
- Add re-export in airtable_add_connector_route.py for backward compatibility
- Update CI/CD pipeline for Docker-based testing
- Modify .github/workflows/test.yml to use Docker Compose for tests
- Add docker layer caching for faster CI builds
- Run tests against real PostgreSQL and Redis containers
- Enhance Docker configuration
- Add pytest dependencies to backend Dockerfile
- Add backend-test service profile in docker-compose.yml
- Mount tests directory in backend service for development
- Update .gitignore with comprehensive Python patterns
- Add coverage, cache, and IDE file patterns
- Exclude uv.lock from version control
- Add backend README with testing instructions
…d remove unused imports - Add catch-all Exception handlers to chats_routes.py and podcasts_routes.py - Remove redundant 'import logging' in podcasts_routes.py - Remove unused imports from test files: - test_celery_tasks.py: asyncio, datetime - test_celery_tasks_comprehensive.py: asyncio, call - test_config.py: pytest, patch, MagicMock, os - test_connector_indexers_comprehensive.py: datetime, timedelta, db imports - test_connector_service_extended.py: patch - test_db_models.py: pytest, mock imports, datetime, uuid4 - test_document_converters.py: AsyncMock, patch - test_page_limit_service.py: patch - test_permissions.py: pytest - Rename unused 'result' variables to '_result' in route tests - Remove unused 'vi' import from auth-utils.test.ts
## Backend Changes ### Directory Renaming - Rename 'retriver' ??? 'retriever' (fix typo in module name) - Update all import statements in db.py, connector_service.py, and tests ### Import Path Updates - Update ChucksHybridSearchRetriever imports to use correct path - Update DocumentHybridSearchRetriever imports to use correct path ## Browser Extension Changes ### Add Logger Utility (lib/logger.ts) - Environment-aware logging (dev vs production) - Structured log format with timestamps and levels - Methods: debug, info, warn, error - Only warnings/errors shown in production ### Replace console.log with Logger - background/index.ts: Error handling in tab listeners - background/messages/savedata.ts: Memory clearing and data saving - background/messages/savesnapshot.ts: Snapshot capture debugging - routes/pages/HomePage.tsx: Search space loading and errors - utils/commons.ts: Web history initialization errors ## Web Application Changes ### Add Logger Utility (lib/logger.ts) - Same environment-aware logging pattern as browser extension - Consistent API across frontend codebase ### Add Constants Module (lib/constants.ts) - Extract hardcoded connector IDs into named constants - CONNECTOR_IDS: CRAWLED_URL, FILE, EXTENSION, YOUTUBE_VIDEO - API_CONNECTOR_ID_OFFSET for dynamic connector ID generation - DEFAULT_CONNECTORS array for built-in connectors - getApiConnectorId() helper function ### Replace console.log with Logger - All dashboard pages, hooks, components, and API routes - Approximately 70+ files updated for consistent logging ## .gitignore Updates - Add .env.* pattern (exclude .env.example) - Add *.sublime-workspace and *.sublime-project - Add *.log and logs/ directory - Add tmp/, temp/, *.tmp, *.temp - Add *.bak, *.backup for editor backups - Add *~ for Unix backup files ## Benefits - Cleaner production logs (no debug noise) - Consistent error handling across codebase - Better maintainability with centralized constants - Fixed typo that could cause confusion in imports
|
@ankitpasayat is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This pull request introduces significant code quality improvements by adding a comprehensive testing infrastructure and replacing ad-hoc console logging with a centralized logger utility. The changes also extract hardcoded connector configuration values into a reusable constants file.
Key Changes
- Testing Infrastructure: Adds Vitest, @testing-library/react, and @testing-library/jest-dom with comprehensive test coverage for utilities, hooks, and UI components
- Logger Utility: Introduces an environment-aware logger that suppresses debug logs in production while maintaining error/warning visibility
- Constants Extraction: Centralizes connector IDs and configurations to eliminate magic numbers and improve maintainability
Reviewed changes
Copilot reviewed 123 out of 128 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| surfsense_web/vitest.config.ts | Vitest configuration with jsdom environment, coverage setup, and path aliases |
| surfsense_web/tests/setup.tsx | Test setup with mocks for Next.js components, browser APIs, and localStorage |
| surfsense_web/tests/lib/*.test.ts | Comprehensive unit tests for utility functions (cn, pagination, auth-utils) |
| surfsense_web/tests/hooks/*.test.ts | Tests for media query hooks with mock window.matchMedia |
| surfsense_web/tests/components/**/*.test.tsx | Component tests for Button and Logo with accessibility checks |
| surfsense_web/lib/logger.ts | Environment-aware logging utility with timestamp formatting |
| surfsense_web/lib/constants.ts | Centralized connector IDs and configurations with API offset handling |
| surfsense_web/package.json | Added test scripts and testing dependencies (@vitest, @testing-library/*) |
| surfsense_web/hooks/use-search-source-connectors.ts | Refactored to use constants instead of hardcoded connector data |
| 40+ files | Replaced console.* calls with logger.* throughout the application |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Environment-aware logger utility for SurfSense web application. | ||
| * | ||
| * In development mode: Uses console.log for debugging | ||
| * In production mode: Suppresses debug logs, only shows warnings and errors | ||
| * | ||
| * Usage: | ||
| * import { logger } from '@/lib/logger'; | ||
| * logger.debug('Debug message', data); | ||
| * logger.info('Info message'); | ||
| * logger.warn('Warning message'); | ||
| * logger.error('Error message', error); | ||
| */ |
There was a problem hiding this comment.
Consider adding a note about test environment behavior. In test environments (NODE_ENV === 'test'), all logs might be suppressed or mocked depending on the test setup, which could be important for developers to know.
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 9bbea0b..843f334
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (49)
• .github/dependabot.yml
• .github/workflows/docker-publish.yml
• .github/workflows/test.yml
• .gitignore
• CONTRIBUTING.md
• docker-compose.yml
• surfsense_backend/Dockerfile
• surfsense_backend/README.md
• surfsense_backend/app/routes/airtable_add_connector_route.py
• surfsense_backend/app/routes/chats_routes.py
• surfsense_backend/app/routes/podcasts_routes.py
• surfsense_backend/app/services/connector_service.py
• surfsense_backend/app/tasks/connector_indexers/airtable_indexer.py
• surfsense_backend/app/tasks/document_processors/file_processors.py
• surfsense_backend/app/utils/connector_auth.py
• surfsense_backend/pyproject.toml
• surfsense_backend/pytest.ini
• surfsense_backend/tests/__init__.py
• surfsense_backend/tests/conftest.py
• surfsense_backend/tests/test_agent_configuration.py
• surfsense_backend/tests/test_blocknote_converter.py
• surfsense_backend/tests/test_celery_tasks.py
• surfsense_backend/tests/test_celery_tasks_comprehensive.py
• surfsense_backend/tests/test_config.py
• surfsense_backend/tests/test_connector_config.py
• surfsense_backend/tests/test_connector_indexers_comprehensive.py
• surfsense_backend/tests/test_connector_service.py
• surfsense_backend/tests/test_connector_service_extended.py
• surfsense_backend/tests/test_db_models.py
• surfsense_backend/tests/test_document_converters.py
• surfsense_backend/tests/test_llm_service.py
• surfsense_backend/tests/test_llm_service_extended.py
• surfsense_backend/tests/test_page_limit_service.py
• surfsense_backend/tests/test_permissions.py
• surfsense_backend/tests/test_rbac.py
• surfsense_backend/tests/test_rbac_schemas.py
• surfsense_backend/tests/test_rbac_utils.py
• surfsense_backend/tests/test_retrievers.py
• surfsense_backend/tests/test_routes_documents.py
• surfsense_backend/tests/test_routes_llm_config.py
• surfsense_backend/tests/test_routes_search_spaces.py
• surfsense_backend/tests/test_schemas.py
• surfsense_backend/tests/test_validators.py
• surfsense_browser_extension/background/index.ts
• surfsense_browser_extension/background/messages/savedata.ts
• surfsense_browser_extension/background/messages/savesnapshot.ts
• surfsense_browser_extension/lib/logger.ts
• surfsense_browser_extension/routes/pages/HomePage.tsx
• surfsense_browser_extension/utils/commons.ts
⏭️ Files skipped (78)
| Locations |
|---|
surfsense_backend/app/retriever/__init__.py |
surfsense_backend/app/retriever/chunks_hybrid_search.py |
surfsense_backend/app/retriever/documents_hybrid_search.py |
surfsense_web/app/(home)/login/GoogleLoginButton.tsx |
surfsense_web/app/api/contact/route.ts |
surfsense_web/app/api/convert-to-blocknote/route.ts |
surfsense_web/app/api/convert-to-markdown/route.ts |
surfsense_web/app/dashboard/[search_space_id]/connectors/(manage)/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/[connector_id]/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/airtable-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/baidu-search-api/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/clickup-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/confluence-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/discord-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/elasticsearch-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/github-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/google-calendar-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/google-gmail-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/jira-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/linear-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/linkup-api/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/luma-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/notion-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/searxng/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/serper-api/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/slack-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/tavily-api/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/connectors/add/webcrawler-connector/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/documents/(manage)/components/RowActions.tsx |
surfsense_web/app/dashboard/[search_space_id]/documents/(manage)/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/editor/[documentId]/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/logs/(manage)/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/onboard/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/podcasts/podcasts-client.tsx |
surfsense_web/app/dashboard/[search_space_id]/researcher/[[...chat_id]]/page.tsx |
surfsense_web/app/dashboard/[search_space_id]/team/page.tsx |
surfsense_web/app/dashboard/page.tsx |
surfsense_web/app/dashboard/searchspaces/page.tsx |
surfsense_web/components/TokenHandler.tsx |
surfsense_web/components/UserDropdown.tsx |
surfsense_web/components/chat/ChatPanel/ChatPanelContainer.tsx |
surfsense_web/components/chat/ChatPanel/PodcastPlayer/PodcastPlayer.tsx |
surfsense_web/components/chat/CodeBlock.tsx |
surfsense_web/components/contact/contact-form.tsx |
surfsense_web/components/json-metadata-viewer.tsx |
surfsense_web/components/onboard/setup-prompt-step.tsx |
surfsense_web/components/settings/prompt-config-manager.tsx |
surfsense_web/components/sidebar/AppSidebarProvider.tsx |
surfsense_web/hooks/use-api-key.ts |
surfsense_web/hooks/use-community-prompts.ts |
surfsense_web/hooks/use-connector-edit-page.ts |
surfsense_web/hooks/use-document-by-chunk.ts |
surfsense_web/hooks/use-document-types.ts |
surfsense_web/hooks/use-documents.ts |
surfsense_web/hooks/use-github-stars.ts |
surfsense_web/hooks/use-llm-configs.ts |
surfsense_web/hooks/use-logs.ts |
surfsense_web/hooks/use-rbac.ts |
surfsense_web/hooks/use-search-source-connectors.ts |
surfsense_web/hooks/use-search-space.ts |
surfsense_web/hooks/use-search-spaces.ts |
surfsense_web/hooks/use-user.ts |
surfsense_web/lib/apis/auth-api.service.ts |
surfsense_web/lib/apis/base-api.service.ts |
surfsense_web/lib/apis/chats-api.service.ts |
surfsense_web/lib/apis/podcasts-api.service.ts |
surfsense_web/lib/constants.ts |
surfsense_web/lib/logger.ts |
surfsense_web/package.json |
surfsense_web/pnpm-lock.yaml |
surfsense_web/tests/components/Logo.test.tsx |
surfsense_web/tests/components/ui/button.test.tsx |
surfsense_web/tests/hooks/use-media-query.test.ts |
surfsense_web/tests/lib/auth-utils.test.ts |
surfsense_web/tests/lib/pagination.test.ts |
surfsense_web/tests/lib/utils.test.ts |
surfsense_web/tests/setup.tsx |
surfsense_web/vitest.config.ts |
- Fix typo 'frendly' ??? 'friendly' in podcasts-api.service.ts (5 occurrences) - Fix branding 'Youtube Video' ??? 'YouTube Video' in constants.ts - Add test environment behavior note to logger.ts documentation - Fix tautological comparison in test_agent_configuration.py - Remove unused _result variables in test files - Clarify re-export intent in airtable_add_connector_route.py - Fix enum iteration warnings with list() in test_db_models.py and test_permissions.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 123 out of 128 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@CREDO23 Can you please review this PR when you get some time |
|
Thanks for the contribution @ankitpasayat ! Could you please break it into smaller, logically-grouped PRs ? |
I agree. This is too much to review and test. |
In that case @CREDO23 Please review and merge #532 #533 #534 #535 #536 #537 in order! |
3 participants
This PR addresses code review feedback with typo fixes, documentation improvements, and test quality enhancements.
Description
This PR addresses code review comments and minor quality improvements:
podcasts-api.service.ts)constants.tstest_agent_configuration.py_resultvariables in test fileslist()wrapperairtable_add_connector_route.pyMotivation and Context
These changes address feedback from code review comments to improve code quality, fix typos, and eliminate static analysis warnings.
Screenshots
N/A - No UI changes
API Changes
Change Type
Testing Performed
All 592 backend tests pass with no new failures.
Checklist
High-level PR Summary
This PR fixes code review comments including typo corrections, documentation improvements, and test quality enhancements. The changes are minor maintenance fixes that improve code clarity without affecting functionality.
⏱️ Estimated Review Time: 5-10 minutes
Review Order Suggestion
surfsense_web/lib/apis/podcasts-api.service.tssurfsense_web/lib/logger.tssurfsense_web/lib/constants.tssurfsense_backend/tests/test_agent_configuration.pysurfsense_backend/tests/test_routes_search_spaces.pysurfsense_backend/tests/test_routes_llm_config.pysurfsense_backend/tests/test_routes_documents.pysurfsense_backend/app/routes/airtable_add_connector_route.pysurfsense_backend/tests/test_db_models.pysurfsense_backend/tests/test_permissions.py