Upgrade/refactor search module/chris #128
Conversation
|
""" WalkthroughThis update refactors the search functionality across the backend and frontend. It consolidates separate user and content search endpoints, controllers, and services into unified methods and routes. The backend now uses PostgreSQL for search queries, while the frontend introduces a reusable search bar, a dedicated search results page, unified search service, and enhanced UI/UX for search-related components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SearchBar (FE)
participant SearchService (FE)
participant Backend /search endpoint
participant SearchService (BE)
participant SearchRepository (BE)
participant Postgres DB
User->>SearchBar (FE): Types in search input
SearchBar (FE)->>SearchService (FE): search(query, type, limit, offset)
SearchService (FE)->>Backend /search endpoint: GET /search?searchText=...&searchType=...&limit=...&offset=...
Backend /search endpoint->>SearchService (BE): search(searchText, searchType, limit, offset)
SearchService (BE)->>SearchRepository (BE): findUsers/findContent
SearchRepository (BE)->>Postgres DB: SQL query (users/content)
Postgres DB-->>SearchRepository (BE): Results
SearchRepository (BE)-->>SearchService (BE): User/Content arrays
SearchService (BE)-->>Backend /search endpoint: Combined SearchResponse
Backend /search endpoint-->>SearchService (FE): SearchResponse
SearchService (FE)-->>SearchBar (FE): SearchResponse
SearchBar (FE)-->>User: Display dropdown results
User->>SearchBar (FE): Submits search or clicks "View all"
SearchBar (FE)->>App Router: Navigate to /search?query=...
App Router->>SearchResults Page: Render component
SearchResults Page->>SearchService (FE): search(query, "all", limit, offset)
SearchService (FE)->>Backend /search endpoint: (as above)
Backend /search endpoint-->>SearchService (FE): (as above)
SearchService (FE)-->>SearchResults Page: SearchResponse
SearchResults Page-->>User: Display paginated results
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (18)
frontend/src/utils/colorUtils.ts (1)
6-35: Well-implemented color generation utility with minor hash improvement suggestion.The function correctly implements a deterministic color generator using a hash-based approach. The color palette is well-chosen with visually distinct colors.
Consider making the hash function more robust by ensuring proper integer handling:
// Use the string to generate a hash code let hash = 0; for (let i = 0; i < text.length; i++) { - hash = text.charCodeAt(i) + ((hash << 5) - hash); + hash = (text.charCodeAt(i) + ((hash << 5) - hash)) | 0; }The
| 0operation ensures the result is treated as a 32-bit integer, preventing potential precision issues with very large hash values.frontend/src/components/search/UserSearchResult.tsx (1)
26-26: Simplify conditional check using optional chaining.The current condition can be simplified using optional chaining as suggested by the static analysis tool.
- {user && user.profile_image ? ( + {user?.profile_image ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
backend/src/modules/search/controllers/search.controller.ts (1)
7-39: Consider refactoring to use functions instead of static class.As noted by the static analysis tool, classes with only static members can be replaced with simple functions for better simplicity and tree-shaking.
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-39: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
backend/src/modules/search/repository/search.repository.ts (1)
46-46: Consider adding length validation for summary fallback.The
LEFT(c.content, 150)fallback is good, but consider ensuring the content field exists and isn't null before applying the LEFT function.- COALESCE(c.summary, LEFT(c.content, 150)) as summary, + COALESCE(c.summary, LEFT(COALESCE(c.content, ''), 150)) as summary,frontend/src/components/search/ContentSearchResult.tsx (2)
82-84: Consider simplifying the first letter extraction.The function declaration can be simplified to a const expression.
Apply this diff to simplify:
- function getFirstLetter() { - return content.title ? content.title[0].toUpperCase() : 'A'; - }; + const getFirstLetter = () => content.title?.[0]?.toUpperCase() || 'A';
98-119: Simplify image visibility control.The component uses both CSS classes and inline styles to control image visibility, which is redundant.
Remove the redundant inline style since the CSS class already handles visibility:
- style={{ display: imageLoaded && !imageError ? 'block' : 'none' }}backend/src/modules/search/services/search.service.ts (1)
29-40: Clean up comment and consider simplifying promise construction.The comment on line 29 has a trailing comma. Also, the promise array construction could be more elegant.
Apply this diff to fix the comment and simplify the logic:
- // Add promises to the array based on the searchType, + // Add promises to the array based on the searchType if (searchType === 'users' || searchType === 'all') { promises.push(searchRepository.findUsers(searchText, limit, offset)); } else { - promises.push(Promise.resolve([])); // Add empty promise to maintain structure + promises.push(Promise.resolve([])); } if (searchType === 'content' || searchType === 'all') { promises.push(searchRepository.findContent(searchText, limit, offset)); } else { - promises.push(Promise.resolve([])); // Add empty promise to maintain structure + promises.push(Promise.resolve([])); }Alternatively, consider a more concise approach:
const promises = [ (searchType === 'users' || searchType === 'all') ? searchRepository.findUsers(searchText, limit, offset) : Promise.resolve([]), (searchType === 'content' || searchType === 'all') ? searchRepository.findContent(searchText, limit, offset) : Promise.resolve([]) ];backend/src/modules/search/types/search.types.ts (1)
13-22: Consider the trade-offs of denormalized user data in Content interface.The Content interface duplicates user fields (username, first_name, last_name, profile_image). While this avoids additional lookups for display purposes, it creates maintenance overhead if user field requirements change.
Consider these alternatives:
- Keep as-is for performance (current approach)
- Create a shared
UserInfointerface that bothUserandContentcan use- Include only
user_idand join data at the display layerExample of approach 2:
interface UserInfo { username: string; first_name: string; last_name: string; profile_image?: string; } export interface User extends UserInfo { user_id: string; } export interface Content extends UserInfo { content_id: string; title: string; summary?: string; date_created: Date; }frontend/src/components/search/SearchBar.tsx (1)
37-56: Consider optimizing the debounce logic.The debounce implementation works but has some potential issues:
- The
initialRenderref logic is complex and could be simplified- The 500ms delay might be too long for a responsive user experience
Consider this cleaner debounce implementation:
- // Debounce the search query with a 500ms delay - useEffect(() => { - // Skip if this is the initial render - if (initialRender.current) { - initialRender.current = false; - return; - } - - const handler = setTimeout(() => { - if (query.trim().length >= 3) { - setDebouncedQuery(query); - } else { - setShowResults(false); - } - }, 500); - - return () => { - clearTimeout(handler); - }; - }, [query]); + // Debounce the search query with a 300ms delay + useEffect(() => { + const handler = setTimeout(() => { + if (query.trim().length >= 3) { + setDebouncedQuery(query); + } else { + setDebouncedQuery(''); + setShowResults(false); + } + }, 300); + + return () => { + clearTimeout(handler); + }; + }, [query]);frontend/src/pages/search/SearchResults.tsx (2)
23-25: Consider making limits configurable.Hard-coded limits might not scale well and could be better as constants or configuration.
Consider moving these to a configuration file or making them configurable:
- const USER_LIMIT = 10; - const CONTENT_LIMIT = 20; - const TOTAL_LIMIT = USER_LIMIT + CONTENT_LIMIT; + const CONFIG = { + USER_LIMIT: 10, + CONTENT_LIMIT: 20, + get TOTAL_LIMIT() { return this.USER_LIMIT + this.CONTENT_LIMIT; } + };
126-136: Button accessibility could be improved.The load more button could benefit from better accessibility attributes and loading state indication.
<button className="load-more-button" onClick={() => fetchResults().catch(error => { console.error("Failed to load more results:", error); })} disabled={loading} + aria-label={loading ? "Loading more results..." : "Load more search results"} + aria-describedby="search-results-status" > {loading ? "Loading..." : "Load More Results"} </button>And add a status region:
+ <div id="search-results-status" className="sr-only" aria-live="polite"> + {loading ? "Loading more results..." : ""} + </div>frontend/src/services/SearchService.ts (2)
25-27: Consider request timeout and cancellation.For a search service that might be called frequently, consider adding request timeouts and cancellation support to prevent stale requests.
const response = await axios.get(`${apiURL}/search`, { params: { searchText, searchType, limit, offset }, + timeout: 10000, // 10 second timeout + signal: AbortSignal.timeout(10000) // Modern browsers });Or for a more robust solution, consider implementing request cancellation in the calling components.
30-36: Error handling could be more specific.The current error handling is generic. Consider providing more specific error types for different failure scenarios.
} catch (error) { + if (axios.isAxiosError(error)) { + if (error.response?.status === 404) { + return new Error("Search endpoint not found"); + } else if (error.response?.status >= 500) { + return new Error("Server error occurred during search"); + } else if (error.code === 'ECONNABORTED') { + return new Error("Search request timed out"); + } + return new Error(`Search failed: ${error.response?.statusText || error.message}`); + } if (error instanceof Error) { - return new Error(`Failed to fetch search data: ${error.message}`); + return new Error(`Network error during search: ${error.message}`); } return new Error("Failed to fetch search data: Unknown error"); }frontend/src/styles/search/search.scss (3)
50-66: Consider dropdown positioning edge cases.The fixed width dropdown (350px) might cause overflow issues on smaller screens or when the search bar is positioned near the edge.
Consider using a more flexible approach:
.search-dropdown { position: absolute; top: 100%; left: 0; - width: 350px; + width: max(350px, 100%); + min-width: 300px; max-height: 400px;This ensures the dropdown is at least as wide as the search bar but has a reasonable minimum width.
175-190: Potential performance issue with hover transitions.The transition on all properties (
transition: all 0.3s ease) can impact performance, especially with many search results.Consider being more specific about which properties to transition:
- transition: all 0.3s ease; + transition: background-color 0.3s ease, transform 0.2s ease;
291-309: Creative placeholder design with accessibility concern.The content thumbnail placeholder with colored background and emoji is visually appealing. However, consider adding better accessibility support.
span { font-size: 2.5rem; font-weight: bold; color: rgba(255, 255, 255, 0.7); text-shadow: 1px 1px 2px rgba(0, 0, 0, 0.2); + /* Ensure content is not read by screen readers as it's decorative */ + aria-hidden: true; }Also, consider adding an aria-label to the placeholder container for better accessibility.
frontend/src/components/Navbar.tsx (2)
99-104: Consider handling logout errors for better user experience.The logout function now removes the "title" from localStorage, which is good for cleanup. However, consider wrapping the logout call in a try-catch to handle potential errors gracefully.
const handleLogout = () => { if (typeof window !== "undefined") { localStorage.removeItem("title"); } - auth.logout(); + try { + auth.logout(); + } catch (error) { + console.error("Logout failed:", error); + // Optionally show a user-friendly error message + } };
330-330: Use optional chaining for cleaner code.The condition can be simplified using optional chaining.
-{auth.user && auth.user.profileImage ? ( +{auth.user?.profileImage ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 330-330: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
backend/.env.sample(1 hunks)backend/package.json(2 hunks)backend/src/modules/search/controllers/search.controller.ts(1 hunks)backend/src/modules/search/repository/search.repository.ts(1 hunks)backend/src/modules/search/routes/search.routes.ts(1 hunks)backend/src/modules/search/services/search.service.ts(1 hunks)backend/src/modules/search/types/index.ts(1 hunks)backend/src/modules/search/types/search.types.ts(1 hunks)backend/src/shared/config/environment.ts(1 hunks)backend/src/shared/database/postgres.connector.ts(1 hunks)frontend/src/App.tsx(2 hunks)frontend/src/components/Navbar.tsx(5 hunks)frontend/src/components/search/ContentSearchResult.tsx(2 hunks)frontend/src/components/search/SearchBar.tsx(1 hunks)frontend/src/components/search/SearchListResults.tsx(0 hunks)frontend/src/components/search/UserSearchResult.tsx(2 hunks)frontend/src/models/SearchResult.ts(1 hunks)frontend/src/pages/search/SearchResults.tsx(1 hunks)frontend/src/services/SearchService.ts(1 hunks)frontend/src/styles/search/search.scss(1 hunks)frontend/src/utils/colorUtils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/search/SearchListResults.tsx
🧰 Additional context used
🧬 Code Graph Analysis (9)
backend/src/modules/search/routes/search.routes.ts (1)
backend/src/modules/search/controllers/search.controller.ts (1)
SearchController(7-39)
frontend/src/models/SearchResult.ts (1)
backend/src/modules/search/types/search.types.ts (1)
SearchResponse(25-28)
backend/src/shared/database/postgres.connector.ts (1)
backend/src/shared/config/environment.ts (1)
env(28-71)
frontend/src/components/search/UserSearchResult.tsx (2)
frontend/src/models/SearchResult.ts (1)
SearchUser(4-10)frontend/src/utils/colorUtils.ts (1)
generateColorFromText(6-35)
backend/src/modules/search/controllers/search.controller.ts (3)
backend/src/modules/search/types/search.types.ts (1)
searchType(1-1)backend/src/shared/utils/logger.ts (1)
logger(28-49)backend/src/modules/search/services/search.service.ts (1)
SearchService(6-56)
frontend/src/services/SearchService.ts (3)
backend/src/modules/search/types/search.types.ts (2)
searchType(1-1)SearchResponse(25-28)frontend/src/models/SearchResult.ts (1)
SearchResponse(23-26)frontend/src/scripts/api.ts (1)
apiURL(8-8)
backend/src/modules/search/repository/search.repository.ts (3)
backend/src/modules/search/types/search.types.ts (2)
User(4-10)Content(13-22)backend/src/shared/database/postgres.connector.ts (1)
query(14-16)backend/src/shared/utils/logger.ts (1)
logger(28-49)
backend/src/modules/search/services/search.service.ts (2)
backend/src/modules/search/types/search.types.ts (4)
searchType(1-1)SearchResponse(25-28)User(4-10)Content(13-22)backend/src/shared/utils/logger.ts (1)
logger(28-49)
backend/src/modules/search/types/search.types.ts (1)
frontend/src/models/SearchResult.ts (1)
SearchResponse(23-26)
🪛 Biome (1.9.4)
frontend/src/components/search/UserSearchResult.tsx
[error] 26-26: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
backend/src/modules/search/controllers/search.controller.ts
[error] 7-39: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
frontend/src/components/Navbar.tsx
[error] 330-330: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (25)
backend/src/modules/search/types/index.ts (1)
1-1: Standard barrel export pattern - looks good.This follows TypeScript best practices for module organization by providing a clean import interface for search types.
backend/.env.sample (1)
14-19: Standard database configuration variables added appropriately.The PostgreSQL connection parameters follow standard naming conventions and are properly organized under a clear section header. This addition aligns well with the search module's transition to PostgreSQL.
frontend/src/App.tsx (2)
26-26: Clean import addition for SearchResults component.The import statement follows the established pattern and correctly references the new search results page component.
102-103: Well-organized route addition for search functionality.The new
/searchroute is appropriately placed within the routing structure and follows React Router best practices. This completes the integration of the new search results page into the application navigation.backend/src/modules/search/routes/search.routes.ts (1)
6-6: Breaking change: Verify client compatibility.The consolidation of separate
/usersand/contentsendpoints into a single/endpoint is a good architectural improvement that simplifies the API. However, this is a breaking change that will cause existing clients calling the old endpoints to receive 404 responses.Ensure all frontend components and API consumers have been updated to use the new unified search endpoint with appropriate
searchTypequery parameters.backend/package.json (3)
25-25: LGTM: Type dependency correctly moved to devDependencies.Moving
@types/jsonwebtokento devDependencies is appropriate since type definitions are only needed during development and build time.
27-27: PostgreSQL dependencies correctly added.The addition of
pgand@types/pgpackages properly supports the new PostgreSQL integration for search functionality.Also applies to: 56-56
57-57:✅ Verification successful
Verify Stripe version compatibility.
The Stripe library was upgraded from
^18.1.1to^18.2.1. Please ensure this version upgrade doesn't introduce breaking changes in your payment processing implementation.
🌐 Web query:
What are the breaking changes in Stripe library version 18.2.1 compared to 18.1.1?💡 Result:
Stripe's SDKs adhere to semantic versioning, where major version increments indicate breaking changes. Minor and patch versions introduce new features and fixes without breaking existing functionality. Therefore, an update from version 18.1.1 to 18.2.1 would typically be a minor or patch release, introducing new features or fixes without breaking changes.
However, to identify any specific breaking changes between these versions, you should consult the official changelog for the Stripe SDK you're using. The changelog provides detailed information on updates, including any breaking changes, new features, and fixes. You can find the changelog in the GitHub repository of the respective SDK.
For example, if you're using the Stripe Node.js SDK, the changelog is available here: (github.com)
Similarly, for the Stripe Python SDK, the changelog is available here: (github.com)
Reviewing the changelog will provide you with a comprehensive list of changes between versions 18.1.1 and 18.2.1, including any breaking changes that may require adjustments in your integration.
Citations:
- 1: https://github.com/stripe/stripe-node/releases?utm_source=openai
- 2: https://github.com/stripe/stripe-python/releases?utm_source=openai
No breaking changes expected for Stripe upgrade
Updating from 18.1.1 to 18.2.1 is a minor/patch bump under Stripe’s semver policy and should not introduce breaking changes. You can double-check the release notes here:
https://github.com/stripe/stripe-node/releases/tag/v18.2.1• File: backend/package.json (line 57) – upgraded
"stripe"from^18.1.1to^18.2.1frontend/src/models/SearchResult.ts (1)
1-26: Well-structured interface definitions.The interfaces are correctly defined and align well with the backend types. The separation of SearchUser, SearchContent, and SearchResponse provides good type safety for the unified search functionality.
frontend/src/components/search/UserSearchResult.tsx (1)
2-9: Good integration of new search data model.The component correctly imports and uses the new SearchUser interface, and the integration of the color generation utility for placeholder images enhances the user experience.
backend/src/modules/search/repository/search.repository.ts (2)
8-31: Excellent implementation of user search with proper security.The
findUsersfunction correctly uses parameterized queries to prevent SQL injection, includes pagination support, and has appropriate error handling. The use ofILIKEfor case-insensitive search is appropriate for PostgreSQL.
37-63: Well-designed content search with smart summary handling.The
findContentfunction demonstrates good practices:
- JOIN with users table to include creator information
- Smart use of
COALESCEto fallback to content excerpt when summary is missing- Searches both title and content fields
- Proper ordering by creation date
- Parameterized queries for security
frontend/src/components/search/ContentSearchResult.tsx (1)
60-68: Good cleanup implementation!The effect properly cleans up the timer on component unmount, preventing memory leaks.
backend/src/modules/search/services/search.service.ts (1)
50-54: Good error handling pattern!Logging the error with context (searchType) while re-throwing allows proper error tracking without swallowing exceptions.
backend/src/modules/search/types/search.types.ts (1)
25-28: Well-structured response interface!The SearchResponse interface provides a clean API contract that aligns well with the frontend expectations and supports partial results.
frontend/src/components/search/SearchBar.tsx (1)
1-21: Good import organization and type safety.The imports are well-organized and properly typed. The use of React hooks and React Router is appropriate for this component's functionality.
frontend/src/pages/search/SearchResults.tsx (2)
79-87: Good defensive programming with error handling.The initial fetch logic properly handles errors and prevents duplicate calls. This is a good pattern for ensuring robust data fetching.
50-53:❓ Verification inconclusive
Potential edge case in pagination logic.
The pagination logic assumes that if fewer results are returned than the limit, there are no more results. This might not always be accurate if the backend has different pagination behavior.
Verify that the backend pagination behavior aligns with this assumption. If the backend might return fewer results for other reasons (e.g., filtering, permissions), this logic could incorrectly stop pagination.
🏁 Script executed:
#!/bin/bash # Verify backend pagination logic for search endpoint ast-grep --pattern $'if ($limit) { $$$ }'Length of output: 44
Review pagination assumption
The front-end logic infrontend/src/pages/search/SearchResults.tsx(lines 50–53) treats any response smaller thanTOTAL_LIMITas the end of available results:// Check if we've reached the end of results if ((newUsers.length + newContent.length) < TOTAL_LIMIT) { setHasMore(false); }Please verify that the backend search endpoint always returns exactly
TOTAL_LIMITitems when more results exist (and only returns fewer items when truly at the end). If the API can return fewer items for other reasons (e.g. filtering, permissions), this check could prematurely stop pagination.– Confirm the search‐endpoint signature and implementation:
• Does it accept alimit(orpageSize) parameter and always return that many items when available?
• Does it provide an explicit “hasNextPage” flag or total count?– If the API cannot guarantee a full page on every non-final fetch, modify the front-end to rely on a server‐provided indicator (or compare offsets/page numbers) rather than solely on result count.
frontend/src/services/SearchService.ts (2)
7-17: Comprehensive JSDoc documentation.The method documentation is thorough and clearly explains all parameters and return types. This is excellent for maintainability.
18-23: Good use of union types and default parameters.The method signature properly uses TypeScript union types and provides sensible defaults. The parameter types align well with the backend API contract.
frontend/src/styles/search/search.scss (2)
4-8: Good responsive design foundation.The search bar container uses relative positioning and max-width, providing a solid foundation for responsive design.
415-445: Excellent mobile responsiveness.The mobile breakpoint styles properly handle the layout changes needed for smaller screens, including full-width dropdowns and stacked content layouts.
frontend/src/components/Navbar.tsx (3)
196-198: Good modularization with the SearchBar component.The integration of the SearchBar component in both large and small screen layouts improves code maintainability and consistency.
Also applies to: 355-357
359-369: Ensure consistent navigation cleanup across all Create Content buttons.The mobile Create Content button correctly removes "title" from localStorage and "content" from cookies, matching the desktop version. This consistency is good for preventing stale data.
383-492: Excellent UX improvements in the profile menu.The redesigned menu with SVG icons, clear visual hierarchy, and dynamic subscription text significantly improves the user experience. The consistent navigation and cleanup on menu item clicks is well implemented.
| import { Pool } from 'pg'; | ||
| import { env } from '../config/environment'; // Assuming you have db config here | ||
|
|
||
| // Use this to connect to the DB. The pool will maintain a pool of connections so that | ||
| // you don't have to keep creating new connections | ||
| const pool = new Pool({ | ||
| user: env.db.user, | ||
| host: env.db.host, | ||
| database: env.db.database, | ||
| password: env.db.password, | ||
| port: parseInt(env.db.port as string), | ||
| }); | ||
|
|
||
| export function query(text: string, params?: any[]) : Promise<any> { | ||
| return pool.query(text, params); | ||
| } | ||
|
|
||
| export default pool; No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add graceful shutdown handling.
Consider adding a cleanup function for graceful application shutdown to properly close database connections.
Add this function to handle graceful shutdown:
export async function closePool(): Promise<void> {
try {
await pool.end();
console.log('Database pool closed successfully');
} catch (error) {
console.error('Error closing database pool:', error);
throw error;
}
}Then import and use this in your main server file's shutdown handlers.
🤖 Prompt for AI Agents
In backend/src/shared/database/postgres.connector.ts lines 1 to 18, add a new
async function named closePool that calls pool.end() inside a try-catch block to
gracefully close the database connections on application shutdown. Log success
or error messages accordingly. This function should then be imported and invoked
in the main server file's shutdown handlers to ensure proper cleanup of the
database pool.
| const pool = new Pool({ | ||
| user: env.db.user, | ||
| host: env.db.host, | ||
| database: env.db.database, | ||
| password: env.db.password, | ||
| port: parseInt(env.db.port as string), | ||
| }); |
There was a problem hiding this comment.
Add error handling and validation for database connection.
The connection pool lacks proper error handling and validation, which could cause runtime failures in production.
+// Validate required database configuration
+if (!env.db.user || !env.db.host || !env.db.database || !env.db.password) {
+ throw new Error('Missing required database configuration. Please check DB_USER, DB_HOST, DB_DATABASE, and DB_PASSWORD environment variables.');
+}
+
+const dbPort = parseInt(env.db.port as string, 10);
+if (isNaN(dbPort)) {
+ throw new Error(`Invalid database port: ${env.db.port}. Must be a valid number.`);
+}
+
const pool = new Pool({
user: env.db.user,
host: env.db.host,
database: env.db.database,
password: env.db.password,
- port: parseInt(env.db.port as string),
+ port: dbPort,
+ max: 20, // Maximum number of connections in the pool
+ idleTimeoutMillis: 30000, // Close idle connections after 30 seconds
+ connectionTimeoutMillis: 2000, // Return error if connection takes longer than 2 seconds
});
+
+// Handle pool errors
+pool.on('error', (err) => {
+ console.error('Unexpected error on idle client', err);
+ process.exit(-1);
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pool = new Pool({ | |
| user: env.db.user, | |
| host: env.db.host, | |
| database: env.db.database, | |
| password: env.db.password, | |
| port: parseInt(env.db.port as string), | |
| }); | |
| // Validate required database configuration | |
| if (!env.db.user || !env.db.host || !env.db.database || !env.db.password) { | |
| throw new Error( | |
| 'Missing required database configuration. Please check DB_USER, DB_HOST, DB_DATABASE, and DB_PASSWORD environment variables.' | |
| ); | |
| } | |
| const dbPort = parseInt(env.db.port as string, 10); | |
| if (isNaN(dbPort)) { | |
| throw new Error(`Invalid database port: ${env.db.port}. Must be a valid number.`); | |
| } | |
| const pool = new Pool({ | |
| user: env.db.user, | |
| host: env.db.host, | |
| database: env.db.database, | |
| password: env.db.password, | |
| port: dbPort, | |
| max: 20, // Maximum number of connections in the pool | |
| idleTimeoutMillis: 30000, // Close idle connections after 30 seconds | |
| connectionTimeoutMillis: 2000, // Return error if connection takes longer than 2 seconds | |
| }); | |
| // Handle pool errors | |
| pool.on('error', (err) => { | |
| console.error('Unexpected error on idle client', err); | |
| process.exit(-1); | |
| }); |
🤖 Prompt for AI Agents
In backend/src/shared/database/postgres.connector.ts around lines 6 to 12, the
database connection pool is created without error handling or validation. Add
validation to ensure all required environment variables for the database
connection are present and valid before creating the pool. Implement error
handling to catch and log any errors during pool creation or connection
attempts, preventing unhandled runtime failures.
| export function query(text: string, params?: any[]) : Promise<any> { | ||
| return pool.query(text, params); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve query function with proper error handling.
The query function should include proper error handling and logging for debugging.
-export function query(text: string, params?: any[]) : Promise<any> {
- return pool.query(text, params);
-}
+export async function query(text: string, params?: any[]): Promise<any> {
+ try {
+ const start = Date.now();
+ const result = await pool.query(text, params);
+ const duration = Date.now() - start;
+ console.log('Executed query', { text, duration, rows: result.rowCount });
+ return result;
+ } catch (error) {
+ console.error('Database query error', { text, params, error });
+ throw error;
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function query(text: string, params?: any[]) : Promise<any> { | |
| return pool.query(text, params); | |
| } | |
| export async function query(text: string, params?: any[]): Promise<any> { | |
| try { | |
| const start = Date.now(); | |
| const result = await pool.query(text, params); | |
| const duration = Date.now() - start; | |
| console.log('Executed query', { text, duration, rows: result.rowCount }); | |
| return result; | |
| } catch (error) { | |
| console.error('Database query error', { text, params, error }); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/src/shared/database/postgres.connector.ts around lines 14 to 16, the
query function currently lacks error handling, which can obscure issues during
database operations. Modify the function to include a try-catch block that
catches any errors thrown by pool.query, logs the error details for debugging
purposes, and then rethrows the error to ensure calling code can handle it
appropriately.
| profile_image?: string; | ||
| title: string; | ||
| summary?: string; | ||
| date_created: Date; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using string type for date_created field.
API responses typically return dates as ISO strings, not Date objects. Unless there's explicit date parsing in the service layer, this should be string to match the actual data format received from the backend.
- date_created: Date;
+ date_created: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| date_created: Date; | |
| date_created: string; |
🤖 Prompt for AI Agents
In frontend/src/models/SearchResult.ts at line 20, the date_created field is
typed as Date, but API responses usually provide dates as ISO strings. Change
the type of date_created from Date to string to accurately reflect the data
format received from the backend unless there is explicit parsing to Date
objects elsewhere.
| const [showTooltip, setShowTooltip] = useState(false); | ||
| const [imageLoaded, setImageLoaded] = useState(false); | ||
| const [imageError, setImageError] = useState(false); | ||
| const tooltipTimer = useRef<NodeJS.Timeout | null>(null); |
There was a problem hiding this comment.
Use browser-compatible timer type.
The NodeJS.Timeout type is specific to Node.js environment. In browser environments, setTimeout returns a number.
Apply this diff to fix the type:
- const tooltipTimer = useRef<NodeJS.Timeout | null>(null);
+ const tooltipTimer = useRef<number | null>(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tooltipTimer = useRef<NodeJS.Timeout | null>(null); | |
| const tooltipTimer = useRef<number | null>(null); |
🤖 Prompt for AI Agents
In frontend/src/components/search/ContentSearchResult.tsx at line 11, replace
the type of tooltipTimer from NodeJS.Timeout | null to number | null because
setTimeout in browsers returns a number, not a NodeJS.Timeout object. This
change ensures compatibility with the browser environment.
| const formatDate = (date: Date) => { | ||
| if (!date) return ""; | ||
| const dateObj = new Date(date); | ||
| return dateObj.toLocaleDateString('en-US', { | ||
| year: 'numeric', | ||
| month: 'short', | ||
| day: 'numeric' | ||
| }); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify date type consistency.
The function checks for falsy date values, but if SearchContent.date_created is typed as non-nullable Date, this check may be unnecessary.
Run the following script to verify the SearchContent type definition:
🏁 Script executed:
#!/bin/bash
# Description: Check if date_created can be null/undefined in SearchContent type
# Search for SearchContent interface definition
ast-grep --pattern 'interface SearchContent {
$$$
}'
# Also check for any optional date_created field
rg -A 5 "date_created.*\?" frontend/src/models/Length of output: 701
Remove redundant null check in formatDate
Since SearchContent.date_created is declared as a non-nullable Date, the if (!date) guard in formatDate is no longer needed and can be removed to simplify the code.
• File: frontend/src/components/search/ContentSearchResult.tsx
• Location: around line 31
• Change:
- if (!date) return "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const formatDate = (date: Date) => { | |
| if (!date) return ""; | |
| const dateObj = new Date(date); | |
| return dateObj.toLocaleDateString('en-US', { | |
| year: 'numeric', | |
| month: 'short', | |
| day: 'numeric' | |
| }); | |
| }; | |
| const formatDate = (date: Date) => { | |
| const dateObj = new Date(date); | |
| return dateObj.toLocaleDateString('en-US', { | |
| year: 'numeric', | |
| month: 'short', | |
| day: 'numeric' | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/search/ContentSearchResult.tsx around lines 30 to 38,
the formatDate function includes a check for falsy date values which is
redundant because SearchContent.date_created is typed as a non-nullable Date.
Remove the if (!date) return ""; guard to simplify the function, ensuring the
function directly formats the passed Date object without unnecessary null
checks.
| const fetchSearchResults = async () => { | ||
| if (fetchInProgress.current) return; | ||
|
|
||
| fetchInProgress.current = true; | ||
| setLoading(true); | ||
|
|
||
| try { | ||
| console.log(`SearchBar: Fetching dropdown results for "${debouncedQuery}"`); | ||
| const response = await SearchService.search(debouncedQuery, "all", 5, 0); | ||
|
|
||
| if (!(response instanceof Error)) { | ||
| // Explicitly type the response to ensure type safety | ||
| const users: SearchUser[] = response.users; | ||
| const content: SearchContent[] = response.content; | ||
|
|
||
| setUserResults(users); | ||
| setContentResults(content); | ||
| setShowResults(true); | ||
| } else { | ||
| console.error('Search error:', response); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error fetching search results:', error); | ||
| } finally { | ||
| setLoading(false); | ||
| fetchInProgress.current = false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and loading state management.
The error handling could be more robust, and there's a potential race condition with concurrent fetches.
const fetchSearchResults = async () => {
- if (fetchInProgress.current) return;
+ if (fetchInProgress.current || debouncedQuery.trim().length < 3) return;
fetchInProgress.current = true;
setLoading(true);
try {
console.log(`SearchBar: Fetching dropdown results for "${debouncedQuery}"`);
const response = await SearchService.search(debouncedQuery, "all", 5, 0);
if (!(response instanceof Error)) {
- // Explicitly type the response to ensure type safety
- const users: SearchUser[] = response.users;
- const content: SearchContent[] = response.content;
-
- setUserResults(users);
- setContentResults(content);
+ setUserResults(response.users || []);
+ setContentResults(response.content || []);
setShowResults(true);
} else {
- console.error('Search error:', response);
+ console.error('Search error:', response.message);
+ // Reset results on error
+ setUserResults([]);
+ setContentResults([]);
+ setShowResults(false);
}
} catch (error) {
console.error('Error fetching search results:', error);
+ // Reset results on error
+ setUserResults([]);
+ setContentResults([]);
+ setShowResults(false);
} finally {
setLoading(false);
fetchInProgress.current = false;
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchSearchResults = async () => { | |
| if (fetchInProgress.current) return; | |
| fetchInProgress.current = true; | |
| setLoading(true); | |
| try { | |
| console.log(`SearchBar: Fetching dropdown results for "${debouncedQuery}"`); | |
| const response = await SearchService.search(debouncedQuery, "all", 5, 0); | |
| if (!(response instanceof Error)) { | |
| // Explicitly type the response to ensure type safety | |
| const users: SearchUser[] = response.users; | |
| const content: SearchContent[] = response.content; | |
| setUserResults(users); | |
| setContentResults(content); | |
| setShowResults(true); | |
| } else { | |
| console.error('Search error:', response); | |
| } | |
| } catch (error) { | |
| console.error('Error fetching search results:', error); | |
| } finally { | |
| setLoading(false); | |
| fetchInProgress.current = false; | |
| } | |
| }; | |
| const fetchSearchResults = async () => { | |
| if (fetchInProgress.current || debouncedQuery.trim().length < 3) return; | |
| fetchInProgress.current = true; | |
| setLoading(true); | |
| try { | |
| console.log(`SearchBar: Fetching dropdown results for "${debouncedQuery}"`); | |
| const response = await SearchService.search(debouncedQuery, "all", 5, 0); | |
| if (!(response instanceof Error)) { | |
| setUserResults(response.users || []); | |
| setContentResults(response.content || []); | |
| setShowResults(true); | |
| } else { | |
| console.error('Search error:', response.message); | |
| // Reset results on error | |
| setUserResults([]); | |
| setContentResults([]); | |
| setShowResults(false); | |
| } | |
| } catch (error) { | |
| console.error('Error fetching search results:', error); | |
| // Reset results on error | |
| setUserResults([]); | |
| setContentResults([]); | |
| setShowResults(false); | |
| } finally { | |
| setLoading(false); | |
| fetchInProgress.current = false; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/search/SearchBar.tsx around lines 76 to 103, improve
error handling by ensuring that any error returned from SearchService.search is
properly handled and does not rely on instanceof Error, which may not work as
expected. Also, prevent race conditions by canceling or ignoring outdated fetch
results if a new fetch starts before the previous one finishes. Adjust loading
state management to accurately reflect the current fetch status, ensuring
setLoading(false) is only called after the latest fetch completes.
| // Fetch search results when debounced query changes | ||
| useEffect(() => { | ||
| // Don't fetch results if we're already on the search results page | ||
| if (location.pathname === '/search') { | ||
| return; | ||
| } | ||
|
|
||
| // Don't fetch on initial render | ||
| if (initialRender.current) { | ||
| return; | ||
| } | ||
|
|
||
| if (debouncedQuery.trim().length >= 3) { | ||
| fetchSearchResults(); | ||
| } | ||
| }, [debouncedQuery, location.pathname]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the fetch trigger logic.
The current logic has redundant checks and complex ref management. The initialRender check is duplicated from the debounce effect.
- // Fetch search results when debounced query changes
- useEffect(() => {
- // Don't fetch results if we're already on the search results page
- if (location.pathname === '/search') {
- return;
- }
-
- // Don't fetch on initial render
- if (initialRender.current) {
- return;
- }
-
- if (debouncedQuery.trim().length >= 3) {
- fetchSearchResults();
- }
- }, [debouncedQuery, location.pathname]);
+ // Fetch search results when debounced query changes
+ useEffect(() => {
+ // Don't fetch results if we're already on the search results page or query is too short
+ if (location.pathname === '/search' || debouncedQuery.trim().length < 3) {
+ return;
+ }
+
+ fetchSearchResults();
+ }, [debouncedQuery, location.pathname]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Fetch search results when debounced query changes | |
| useEffect(() => { | |
| // Don't fetch results if we're already on the search results page | |
| if (location.pathname === '/search') { | |
| return; | |
| } | |
| // Don't fetch on initial render | |
| if (initialRender.current) { | |
| return; | |
| } | |
| if (debouncedQuery.trim().length >= 3) { | |
| fetchSearchResults(); | |
| } | |
| }, [debouncedQuery, location.pathname]); | |
| // Fetch search results when debounced query changes | |
| useEffect(() => { | |
| // Don't fetch results if we're already on the search results page or query is too short | |
| if (location.pathname === '/search' || debouncedQuery.trim().length < 3) { | |
| return; | |
| } | |
| fetchSearchResults(); | |
| }, [debouncedQuery, location.pathname]); |
🤖 Prompt for AI Agents
In frontend/src/components/search/SearchBar.tsx around lines 58 to 73, simplify
the useEffect that triggers fetching search results by removing the redundant
initialRender check, as it duplicates logic already handled in the debounce
effect. Keep only the necessary condition to avoid fetching when on the search
results page and when the debounced query length is at least 3 characters. This
will streamline the effect and reduce complexity.
| const fetchResults = useCallback(async () => { | ||
| // Prevent duplicate calls and check valid query | ||
| if (!query || | ||
| query.length < 3 || | ||
| loading || | ||
| !hasMore || | ||
| fetchInProgress.current) { | ||
| return; | ||
| } | ||
|
|
||
| fetchInProgress.current = true; | ||
| setLoading(true); | ||
|
|
||
| try { | ||
| console.log(`Fetching results for "${query}" with offset ${offset}`); | ||
| const response = await SearchService.search(query, "all", TOTAL_LIMIT, offset); | ||
|
|
||
| if (response instanceof Error) { | ||
| console.error("Error fetching search results:", response); | ||
| } else { | ||
| const newUsers: SearchUser[] = response.users || []; | ||
| const newContent: SearchContent[] = response.content || []; | ||
|
|
||
| // Check if we've reached the end of results | ||
| if ((newUsers.length + newContent.length) < TOTAL_LIMIT) { | ||
| setHasMore(false); | ||
| } | ||
|
|
||
| setUsers(prev => [...prev, ...newUsers]); | ||
| setContent(prev => [...prev, ...newContent]); | ||
| setOffset(prev => prev + newUsers.length + newContent.length); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error in fetchResults:", error); | ||
| } finally { | ||
| setLoading(false); | ||
| fetchInProgress.current = false; | ||
| } | ||
| }, [query, loading, hasMore, offset, TOTAL_LIMIT]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex fetchResults function needs refactoring.
The function has multiple responsibilities and complex guard conditions that make it hard to maintain.
Consider breaking this down into smaller, more focused functions:
+ const isValidQuery = (query: string): boolean => {
+ return query && query.length >= 3;
+ };
+
+ const canFetchMore = (): boolean => {
+ return !loading && hasMore && !fetchInProgress.current;
+ };
const fetchResults = useCallback(async () => {
- // Prevent duplicate calls and check valid query
- if (!query ||
- query.length < 3 ||
- loading ||
- !hasMore ||
- fetchInProgress.current) {
+ if (!isValidQuery(query) || !canFetchMore()) {
return;
}Also, the offset calculation logic could be more explicit:
- setOffset(prev => prev + newUsers.length + newContent.length);
+ const newItemsCount = newUsers.length + newContent.length;
+ setOffset(prev => prev + newItemsCount);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchResults = useCallback(async () => { | |
| // Prevent duplicate calls and check valid query | |
| if (!query || | |
| query.length < 3 || | |
| loading || | |
| !hasMore || | |
| fetchInProgress.current) { | |
| return; | |
| } | |
| fetchInProgress.current = true; | |
| setLoading(true); | |
| try { | |
| console.log(`Fetching results for "${query}" with offset ${offset}`); | |
| const response = await SearchService.search(query, "all", TOTAL_LIMIT, offset); | |
| if (response instanceof Error) { | |
| console.error("Error fetching search results:", response); | |
| } else { | |
| const newUsers: SearchUser[] = response.users || []; | |
| const newContent: SearchContent[] = response.content || []; | |
| // Check if we've reached the end of results | |
| if ((newUsers.length + newContent.length) < TOTAL_LIMIT) { | |
| setHasMore(false); | |
| } | |
| setUsers(prev => [...prev, ...newUsers]); | |
| setContent(prev => [...prev, ...newContent]); | |
| setOffset(prev => prev + newUsers.length + newContent.length); | |
| } | |
| } catch (error) { | |
| console.error("Error in fetchResults:", error); | |
| } finally { | |
| setLoading(false); | |
| fetchInProgress.current = false; | |
| } | |
| }, [query, loading, hasMore, offset, TOTAL_LIMIT]); | |
| const isValidQuery = (query: string): boolean => { | |
| return query && query.length >= 3; | |
| }; | |
| const canFetchMore = (): boolean => { | |
| return !loading && hasMore && !fetchInProgress.current; | |
| }; | |
| const fetchResults = useCallback(async () => { | |
| if (!isValidQuery(query) || !canFetchMore()) { | |
| return; | |
| } | |
| fetchInProgress.current = true; | |
| setLoading(true); | |
| try { | |
| console.log(`Fetching results for "${query}" with offset ${offset}`); | |
| const response = await SearchService.search(query, "all", TOTAL_LIMIT, offset); | |
| if (response instanceof Error) { | |
| console.error("Error fetching search results:", response); | |
| } else { | |
| const newUsers: SearchUser[] = response.users || []; | |
| const newContent: SearchContent[] = response.content || []; | |
| // Check if we've reached the end of results | |
| if ((newUsers.length + newContent.length) < TOTAL_LIMIT) { | |
| setHasMore(false); | |
| } | |
| setUsers(prev => [...prev, ...newUsers]); | |
| setContent(prev => [...prev, ...newContent]); | |
| const newItemsCount = newUsers.length + newContent.length; | |
| setOffset(prev => prev + newItemsCount); | |
| } | |
| } catch (error) { | |
| console.error("Error in fetchResults:", error); | |
| } finally { | |
| setLoading(false); | |
| fetchInProgress.current = false; | |
| } | |
| }, [query, loading, hasMore, offset, TOTAL_LIMIT]); |
🤖 Prompt for AI Agents
In frontend/src/pages/search/SearchResults.tsx around lines 27 to 65, the
fetchResults function is too complex with multiple responsibilities and
complicated guard conditions. Refactor by extracting smaller helper functions
for validation checks (e.g., isFetchAllowed), result processing (e.g.,
updateResults), and offset calculation. Make the offset update explicit by
clearly summing newUsers and newContent lengths before setting the new offset.
This will improve readability and maintainability.
center dropdown, better scrollbar search page margins normalized
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores