refactor: improve logging and code quality #537
Conversation
Logging: - Add structured logger for browser extension (lib/logger.ts) - Add structured logger for web frontend (lib/logger.ts) - Replace console.log with proper logger calls across codebase Code Quality: - Extract constants to dedicated file (lib/constants.ts) - Extract connector auth utilities to connector_auth.py - Improve route handlers with better error handling - Add frontend unit tests for components, hooks, and utilities Files updated: - 50+ files with console.log ??? logger replacements - New logger implementations for extension and web - Frontend tests: Logo, Button, useMediaQuery, utils, pagination
|
@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 PR refactors the codebase to improve logging and code quality by introducing structured loggers, extracting constants and utilities, and adding comprehensive frontend unit tests.
Key changes:
- Structured logging with environment-aware loggers for web and browser extension
- Extracted connector authentication utilities to a dedicated module (
connector_auth.py) - Extracted hardcoded connector IDs and configurations to a constants file
- Added unit tests for frontend utilities, hooks, and UI components
- Replaced 50+
console.logcalls with proper logger usage - Improved error handling in backend routes with better exception catching and logging
Reviewed changes
Copilot reviewed 83 out of 83 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| surfsense_web/lib/logger.ts | New structured logger for web frontend with environment-aware log levels |
| surfsense_browser_extension/lib/logger.ts | New structured logger for browser extension with timestamp formatting |
| surfsense_web/lib/constants.ts | Extracted connector IDs and configurations to centralized constants |
| surfsense_backend/app/utils/connector_auth.py | Extracted Airtable auth token refresh logic from routes to utilities |
| surfsense_web/tests/* | Added comprehensive unit tests for utilities, hooks, and components |
| surfsense_web/lib/apis/*.ts | Replaced console.log with logger calls and fixed "frendly" typos |
| surfsense_web/hooks/*.ts | Replaced console.error with logger.error across all hooks |
| surfsense_web/components/*.tsx | Updated components to use structured logger |
| surfsense_web/app/dashboard/**/*.tsx | Updated dashboard pages to use logger |
| surfsense_backend/app/routes/*.py | Improved error handling with specific exception types and logging |
| surfsense_backend/app/tasks/document_processors/file_processors.py | Improved error handling using OSError instead of generic Exception |
| surfsense_browser_extension/**/*.ts | Updated extension code to use structured logger |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| { | ||
| id: CONNECTOR_IDS.YOUTUBE_VIDEO, | ||
| name: "YouTube Video", |
There was a problem hiding this comment.
Inconsistent spelling: Line 27 uses "YouTube Video" (correct) but line 61 uses "Youtube Video" (incorrect capitalization). Should be "YouTube Video" for brand consistency.
| const copyToClipboard = useCallback(async () => { | ||
| if (!apiKey) return; | ||
|
|
||
| try { | ||
| if (navigator.clipboard && window.isSecureContext) { | ||
| // Use Clipboard API if available and in secure context | ||
| await navigator.clipboard.writeText(apiKey); | ||
| setCopied(true); | ||
| toast.success("API key copied to clipboard"); | ||
| await navigator.clipboard.writeText(apiKey); | ||
| setCopied(true); | ||
| toast.success("API key copied to clipboard"); | ||
|
|
||
| setTimeout(() => { | ||
| setCopied(false); | ||
| }, 2000); | ||
| } else { | ||
| // Fallback for non-secure contexts or browsers without clipboard API | ||
| fallbackCopyTextToClipboard(apiKey); | ||
| } | ||
| setTimeout(() => { | ||
| setCopied(false); | ||
| }, 2000); | ||
| } catch (err) { | ||
| console.error("Failed to copy:", err); | ||
| logger.error("Failed to copy:", err); | ||
| toast.error("Failed to copy API key"); | ||
| } |
There was a problem hiding this comment.
The removal of the fallbackCopyTextToClipboard function eliminates backward compatibility for browsers that don't support the Clipboard API or non-secure contexts (non-HTTPS). While modern browsers support navigator.clipboard, the fallback using document.execCommand('copy') was useful for older browsers or non-HTTPS development environments. Consider keeping the fallback or documenting that the feature requires HTTPS and modern browser support.
| } catch (err: any) { | ||
| setError(err.message || "Failed to fetch logs"); | ||
| console.error("Error fetching logs:", err); | ||
| logger.error("Error fetching logs:", err); |
There was a problem hiding this comment.
Missing import: logger is used multiple times in this file (lines 115, 166, 198, 223, 244, 289) but not imported. Add import { logger } from "@/lib/logger"; at the top of the file.
| } catch (err) { | ||
| setError(err instanceof Error ? err : new Error("An unknown error occurred")); | ||
| console.error("Error fetching document types:", err); | ||
| logger.error("Error fetching document types:", err); |
There was a problem hiding this comment.
Missing import: logger is used on line 59 but not imported. Add import { logger } from "@/lib/logger"; at the top of the file.
| ) from None | ||
| except Exception: | ||
| except Exception as e: | ||
| await session.rollback() |
There was a problem hiding this comment.
The f-string interpolation is missing the 'f' prefix. Line 75 shows detail="Token refresh failed: {token_response.text}" but should be detail=f"Token refresh failed: {token_response.text}" to properly format the error message with the response text.
|
|
||
| if token_response.status_code != 200: | ||
| raise HTTPException( | ||
| status_code=400, detail="Token refresh failed: {token_response.text}" |
There was a problem hiding this comment.
The f-string interpolation is missing the 'f' prefix. Line 75 shows detail="Token refresh failed: {token_response.text}" but should be detail=f"Token refresh failed: {token_response.text}" to properly format the error message with the response text.
| status_code=400, detail="Token refresh failed: {token_response.text}" | |
| status_code=400, detail=f"Token refresh failed: {token_response.text}" |
| if (config.isDevelopment) { | ||
| console.info(formatMessage('info', message), ...args); | ||
| } |
There was a problem hiding this comment.
Inconsistency between logger implementations: The browser extension's info method only logs in development mode (line 47-48), whereas the web application's info method always logs (surfsense_web/lib/logger.ts:48). This inconsistency could lead to confusion about when info-level logs appear. Consider aligning the behavior or documenting why they differ.
| if (config.isDevelopment) { | |
| console.info(formatMessage('info', message), ...args); | |
| } | |
| console.info(formatMessage('info', message), ...args); |
| } catch (err: any) { | ||
| setError(err.message || "Failed to fetch search spaces"); | ||
| console.error("Error fetching search spaces:", err); | ||
| logger.error("Error fetching search spaces:", err); |
There was a problem hiding this comment.
Missing import: logger is used on line 42 but not imported. Add import { logger } from "@/lib/logger"; at the top of the file.
|
|
||
| # Re-export for backward compatibility - this import is used by other modules | ||
| # that import refresh_airtable_token from this module | ||
| from app.utils.connector_auth import refresh_airtable_token as refresh_airtable_token # noqa: F401 |
There was a problem hiding this comment.
Import of 'refresh_airtable_token' is not used.
![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 601489b..80bbcb7
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (50)
• 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/tasks/connector_indexers/airtable_indexer.py
• surfsense_backend/app/tasks/document_processors/file_processors.py
• surfsense_backend/app/utils/connector_auth.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
• 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
⏭️ Files skipped (33)
| Locations |
|---|
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/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 |
|
@ankitpasayat , can you please resolve copilot comments. ? |
Got it, I’ll need some time to resolve the copilot comments. : ) |
|
@ankitpasayat I've resolved all Copilot review comments. The fixes are in my fork: https://github.com/unitagain/SurfSense-CN/tree/pr-537 To apply them, run: git remote add unitagain [https://github.com/unitagain/SurfSense-CN.git](https://github.com/unitagain/SurfSense-CN.git)
git fetch unitagain pr-537
git cherry-pick 187700ce6
git pushChanges made: Fixed YouTube capitalization for brand consistency |
|
My mistake — I tagged the wrong person earlier, which caused the confusion. Thanks @unitagain for the fixes. @ankitpasayat, feel free to pull them into your PR branch if helpful. Sorry for the mix-up! |
|
@CREDO23 That’s totally fine ! Hope the fixes prove helpful, and I’m always happy to collaborate on items like this. Feel free to reach out whenever you need assistance with anything! : ) |
3 participants
Logging:
Code Quality:
Files updated:
Description
Motivation and Context
FIX #
Screenshots
API Changes
Change Type
Testing Performed
Checklist
High-level PR Summary
This PR improves code quality and maintainability through comprehensive logging and refactoring changes. The main improvements include replacing
console.logstatements with structured loggers across 50+ files in both the browser extension and web frontend, extracting hardcoded constants to a dedicatedconstants.tsfile, moving connector authentication utilities to a separateconnector_auth.pymodule to avoid circular imports, enhancing error handling in backend route handlers with proper exception logging and more granular SQLAlchemy error handling, and adding frontend unit tests for components (Logo,Button), hooks (useMediaQuery), and utilities (auth-utils,pagination,utils).⏱️ Estimated Review Time: 15-30 minutes
💡 Review Order Suggestion
surfsense_web/lib/logger.tssurfsense_browser_extension/lib/logger.tssurfsense_web/lib/constants.tssurfsense_backend/app/utils/connector_auth.pysurfsense_backend/app/routes/airtable_add_connector_route.pysurfsense_backend/app/tasks/connector_indexers/airtable_indexer.pysurfsense_backend/app/routes/chats_routes.pysurfsense_backend/app/routes/podcasts_routes.pysurfsense_backend/app/tasks/document_processors/file_processors.pysurfsense_browser_extension/background/index.tssurfsense_browser_extension/background/messages/savedata.tssurfsense_browser_extension/background/messages/savesnapshot.tssurfsense_browser_extension/routes/pages/HomePage.tsxsurfsense_browser_extension/utils/commons.tssurfsense_web/app/(home)/login/GoogleLoginButton.tsxsurfsense_web/app/api/contact/route.tssurfsense_web/app/api/convert-to-blocknote/route.tssurfsense_web/app/api/convert-to-markdown/route.tssurfsense_web/app/dashboard/[search_space_id]/connectors/(manage)/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/[connector_id]/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/airtable-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/baidu-search-api/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/clickup-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/confluence-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/discord-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/elasticsearch-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/github-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/google-calendar-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/google-gmail-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/jira-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/linear-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/linkup-api/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/luma-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/notion-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/searxng/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/serper-api/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/slack-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/tavily-api/page.tsxsurfsense_web/app/dashboard/[search_space_id]/connectors/add/webcrawler-connector/page.tsxsurfsense_web/app/dashboard/[search_space_id]/documents/(manage)/components/RowActions.tsxsurfsense_web/app/dashboard/[search_space_id]/documents/(manage)/page.tsxsurfsense_web/app/dashboard/[search_space_id]/editor/[documentId]/page.tsxsurfsense_web/app/dashboard/[search_space_id]/logs/(manage)/page.tsxsurfsense_web/app/dashboard/[search_space_id]/onboard/page.tsxsurfsense_web/app/dashboard/[search_space_id]/podcasts/podcasts-client.tsxsurfsense_web/app/dashboard/[search_space_id]/researcher/[[...chat_id]]/page.tsxsurfsense_web/app/dashboard/[search_space_id]/team/page.tsxsurfsense_web/app/dashboard/page.tsxsurfsense_web/app/dashboard/searchspaces/page.tsxsurfsense_web/components/TokenHandler.tsxsurfsense_web/components/UserDropdown.tsxsurfsense_web/components/chat/ChatPanel/ChatPanelContainer.tsxsurfsense_web/components/chat/ChatPanel/PodcastPlayer/PodcastPlayer.tsxsurfsense_web/components/chat/CodeBlock.tsxsurfsense_web/components/contact/contact-form.tsxsurfsense_web/components/json-metadata-viewer.tsxsurfsense_web/components/onboard/setup-prompt-step.tsxsurfsense_web/components/settings/prompt-config-manager.tsxsurfsense_web/components/sidebar/AppSidebarProvider.tsxsurfsense_web/hooks/use-api-key.tssurfsense_web/hooks/use-community-prompts.tssurfsense_web/hooks/use-connector-edit-page.tssurfsense_web/hooks/use-document-by-chunk.tssurfsense_web/hooks/use-document-types.tssurfsense_web/hooks/use-documents.tssurfsense_web/hooks/use-github-stars.tssurfsense_web/hooks/use-llm-configs.tssurfsense_web/hooks/use-logs.tssurfsense_web/hooks/use-rbac.tssurfsense_web/hooks/use-search-source-connectors.tssurfsense_web/hooks/use-search-space.tssurfsense_web/hooks/use-search-spaces.tssurfsense_web/hooks/use-user.tssurfsense_web/lib/apis/auth-api.service.tssurfsense_web/lib/apis/base-api.service.tssurfsense_web/lib/apis/chats-api.service.tssurfsense_web/lib/apis/podcasts-api.service.tssurfsense_web/tests/components/Logo.test.tsxsurfsense_web/tests/components/ui/button.test.tsxsurfsense_web/tests/hooks/use-media-query.test.tssurfsense_web/tests/lib/auth-utils.test.tssurfsense_web/tests/lib/pagination.test.tssurfsense_web/tests/lib/utils.test.ts