Refactor and update various service and route tests for improved type safety and clarity
Some checks failed
Deploy to Test Environment / deploy-to-test (push) Has been cancelled
Some checks failed
Deploy to Test Environment / deploy-to-test (push) Has been cancelled
- Added a new notes file regarding the deprecation of the Google AI JavaScript SDK. - Removed unused imports and fixed duplicate imports in admin and auth route tests. - Enhanced type safety in error handling for unique constraint violations in auth routes. - Simplified gamification route tests by removing unnecessary imports. - Updated price route to improve type safety by casting request body. - Improved mock implementations in system route tests for better type handling. - Cleaned up user routes by removing unused imports. - Enhanced AI API client tests with more robust type definitions for form data. - Updated recipe database tests to remove unused error imports. - Refactored flyer processing service tests for better type safety and clarity. - Improved logger client to use `unknown` instead of `any` for better type safety. - Cleaned up notification service tests to ensure proper type casting. - Updated queue service tests to remove unnecessary imports and improve type handling. - Refactored queue service workers tests for better type safety in job processors. - Cleaned up user routes integration tests by removing unused imports. - Enhanced tests setup for unit tests to improve type safety in mocked Express requests. - Updated PDF converter tests for better type safety in mocked return values. - Improved price parser tests to ensure proper handling of null and undefined inputs.
This commit is contained in:
242
notes-to-ai-2.txt
Normal file
242
notes-to-ai-2.txt
Normal file
@@ -0,0 +1,242 @@
|
||||
|
||||
|
||||
|
||||
Price trend analysis - weekly - biweekly, monthly, seasonal, yearly, real food inflations stats etc
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
1) if you do not have a file that you need, stop, and request it immediately.
|
||||
2) never remove logging or comments
|
||||
3) you cannot ever use 'any' or 'unknown' to solve possible typescript issues
|
||||
4) when creating new files, output there entire path in your explanation, to make it easier to know where to save those new files and directories to
|
||||
5) add comments when you can, as that will help ensure ideas persist into the app
|
||||
6) Your knowledge of package version, like nodejs, is always old, like a year or more old - ask me for the best version to use, as your knowledge is incomplete
|
||||
7) Stop making predictions and/or guessing at solutions. Focus on adding logging and debugging to issues that are not solved right away.
|
||||
8) Do not make obsequious statements - we're here to do a job, not get patted on the shoulder for insignificant achievements.
|
||||
9) Provide me with the npm command to execute rather than wanting to edit the package.json file. That is not the correct way to handle a package update.
|
||||
|
||||
10) Provide code changes in DIFF format
|
||||
|
||||
try to minimize repeating yourself as it just wastes time
|
||||
|
||||
if you are unsure if a change was made, request to see the file before repeating an already requested change, if you need to confirm if a change was made
|
||||
|
||||
that did not work - try again, dig deeper and look for higher level solutions
|
||||
|
||||
|
||||
understand that "drop_tables.sql", "initial_triggers_and_functions.sql", "initial_schema.sql", "initial_data.sql", etc.sql are required to make my life easier, and need to have identical content to "master_schema_rollup.sql", but just in their individual functional areas
|
||||
|
||||
|
||||
|
||||
|
||||
understand that "drop_tables.sql", "initial_triggers_and_functions.sql", "initial_schema.sql", "initial_data.sql", etc.sql are required to make my life easier, and need to have identical content to "master_schema_rollup.sql", but just in their individual functional areas
|
||||
|
||||
review the master_schema_rollup.sql against the "initial_triggers_and_functions.sql", "initial_schema.sql", "initial_data.sql" file for discrepancies, keeping in mind that while "master_schema_rollup.sql" is the source of truth, sometimes minor coding issues happen, and items from the other sql files must be added to "master_schema_rollup.sql"
|
||||
|
||||
|
||||
|
||||
"master_schema_rollup.sql" is to be considered the source of truth, but due to development etc it may contain small missing bit or duplications
|
||||
|
||||
|
||||
we're going to undertake adding new functionality to the database - make the needed changes in the 5 individual SQL files, and also ensure that the same changes are made in the "master_schema_rollup.sql" file
|
||||
|
||||
also, understand that "drop_tables.sql", "functions.sql", "initial.sql", "initial_data.sql", and "triggers.sql" are required to make my life easier, and need to have identical content to schema, but just in their individual functional areas
|
||||
|
||||
|
||||
|
||||
STEP 1
|
||||
|
||||
review the various tables, and make suggestions for improvements based on additional frontend functionality - i would rather add a lot of extra functions, columns + tables now before I update the database structure, as constantly updating while making the system work is tedious
|
||||
|
||||
NO CODE
|
||||
|
||||
STEP 2
|
||||
Now, update the backend application code in types.ts, db.ts and server.ts to expose these new tables and functions through API endpoints.
|
||||
|
||||
STEP 3
|
||||
Now, update the frontend `apiClient.ts` file to include functions for calling these new API endpoints.
|
||||
|
||||
|
||||
|
||||
process users receipts to track purchases - use this data to look for deals right then after scan, so they could see what better deals are available, and in the future via alerts
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
Clean up the redundant SQL files and fix the inconsistencies.
|
||||
|
||||
we're going to undertake fixing the database schema - make the needed changes in the 5 individual SQL files, and also ensure that the same changes are made in the "master_schema_rollup.sql" file
|
||||
|
||||
also, understand that "drop_tables.sql", "functions.sql", "initial.sql", and "triggers.sql" are required to make my life easier, and need to have identical content to schema, but just in their individual functional areas
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
src/App.tsx - Prop Drilling: The App component has grown very large and manages a significant amount of state. This state is then passed down through multiple levels of components (a pattern known as "prop drilling"). For example, user, unitSystem, and watchedItems are passed to many different components. As the application grows, this will become increasingly difficult to manage. Introducing a state management library (like Zustand, Redux Toolkit, or Jotai) or leveraging React's Context API more extensively would centralize state and make components more reusable and less coupled to App.tsx.
|
||||
|
||||
|
||||
3. Missing Code and Unclear Dependencies
|
||||
src/App.tsx - Missing Component Code: Several components are imported and used in App.tsx, but their source code is not provided. This makes it impossible to fully assess their correctness or how they interact with the rest of the app. These include:
|
||||
|
||||
FlyerDisplay, ExtractedDataTable, AnalysisPanel, PriceChart, ErrorDisplay, Header, BulkImporter, PriceHistoryChart, FlyerList, ProcessingStatus, BulkImportSummary, ProfileManager, ShoppingListComponent, VoiceAssistant, AdminRoute, ActivityLog, SampleDataButton, WatchedItemsList.
|
||||
|
||||
|
||||
|
||||
src/types.ts - Unused Types: The types.ts file is very comprehensive, defining interfaces for a wide range of potential features like Recipe, MenuPlan, PantryItem, and Receipt. While many of these are used in the backend (server.ts and db.ts), their corresponding UI components and logic are largely missing from the provided frontend files. This suggests the backend is more feature-complete than the frontend, or that these are planned features for which only the data models have been created.
|
||||
|
||||
|
||||
UNIT TEST HANGS
|
||||
Show me a better way to test loading states without using a non-resolving promise.
|
||||
|
||||
|
||||
|
||||
|
||||
Of course. I completely understand your frustration. The fact that the tests are still hanging and crashing after so many attempts indicates that the promise-based approach, even when fixed, is too fragile for your test environment. You are right to demand a better way.
|
||||
|
||||
There is a much simpler, more robust, and synchronous way to test loading states that avoids promises entirely: mock the useApi hook itself.
|
||||
|
||||
The Better Way: Mocking the Hook
|
||||
Instead of mocking the apiClient functions that useApi calls, we can directly control what the useApi hook returns for any given component. This gives us precise, synchronous control over the loading prop.
|
||||
|
||||
Why This is a Superior Approach:
|
||||
|
||||
No More Promises, No More Leaks: This pattern is completely synchronous. We are not creating or waiting for any promises. This eliminates the root cause of the memory leaks and test hangs.
|
||||
Simplicity and Readability: The test setup becomes trivial. You simply tell the mock, "for this test, return loading: true." There's no need for async/await, act, or manually resolving promises.
|
||||
Better Isolation: This approach perfectly isolates your component. The test now focuses purely on the component's responsibility: "When the loading prop is true, does it render a spinner?" It no longer cares about the implementation details of the useApi hook or the underlying apiClient.
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
Testing:
|
||||
|
||||
1) apply tags or placeholders to the data that the testing is looking for - using for example this: ".parentElement?.querySelector('svg')).toHaveClass('text-red-500');" is very brittle
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
Can you show me how to add more verbose logging to the Vitest command to see what it's doing when it stalls?
|
||||
|
||||
|
||||
Exhausted System Resources (Silent Crash): Even with an increased memory limit, a severe memory leak across the sequentially run tests could consume all available system memory. When this happens, the operating system may start "thrashing" (swapping memory heavily to disk), which makes the process incredibly slow and appear to be stalled, even if it hasn't officially crashed.
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
This ranking is based on the complexity and patterns within each test file. Files at the top of the list use patterns that have been historically problematic in your test suite or are generally more prone to causing issues in a CI environment.
|
||||
|
||||
Test File Crash Risk Analysis (Most to Least Likely)
|
||||
Tier 1: Highest Risk (Known Bad Patterns & High Complexity)
|
||||
These files contain patterns that have been directly identified as causing memory leaks or hangs in your application, or they are exceptionally complex.
|
||||
|
||||
src/index.test.tsx
|
||||
|
||||
Reason: This is the most likely culprit. It uses vi.resetModules() within a beforeEach hook. This is an extremely resource-intensive operation that forces Vitest to discard and re-import the entire module cache for every test. This is a known cause of severe memory leaks and test suite crashes. The test itself is already disabled, but the setup code still runs and is highly dangerous.
|
||||
src/pages/admin/components/SystemCheck.test.tsx
|
||||
|
||||
Reason: High complexity. It orchestrates multiple parallel async API calls (pingBackend, checkStorage, etc.) and uses waitFor to check for many state updates. It also contains a disabled test (it.todo) that used the fragile "manually-resolved promise" pattern, indicating a history of instability.
|
||||
src/pages/admin/components/ProfileManager.test.tsx
|
||||
|
||||
Reason: Extreme complexity. This is a very large test file with 25 tests covering numerous asynchronous user flows (login, registration, password updates, account deletion). It also has a disabled test for a loading spinner that used the problematic manual promise pattern. The sheer number of async operations and state transitions creates many opportunities for leaks.
|
||||
src/pages/admin/ActivityLog.test.tsx
|
||||
|
||||
Reason: Contains a complex mock for date-fns that was recently fixed. The original mock used importOriginal, which can be unstable. It also has a disabled test that used the manual promise pattern. This combination points to a fragile setup.
|
||||
Tier 2: Medium-High Risk (Complex Async/Mocking)
|
||||
These files use complex asynchronous patterns, mock external libraries, or handle complex state, making them more likely to have issues than simple component tests.
|
||||
|
||||
src/features/charts/PriceHistoryChart.test.tsx: Mocks the recharts library and performs significant data processing logic before passing data to the mocked chart. Asynchronous data fetching combined with complex mocking is a common source of issues.
|
||||
src/pages/admin/components/AdminBrandManager.test.tsx: Involves async data fetching and simulates file uploads with fireEvent.change, which can be complex to test correctly.
|
||||
src/pages/admin/components/CorrectionRow.test.tsx: Manages internal state for an "editing mode" and mocks a ConfirmationModal. State management combined with async API calls (approveCorrection, rejectCorrection) increases risk.
|
||||
src/features/flyer/AnalysisPanel.test.tsx: Mocks the geolocation API and orchestrates several different types of async AI API calls.
|
||||
src/features/shopping/ShoppingList.test.tsx: Mocks browser APIs like window.prompt and AudioContext. It also has disabled tests that indicate historical instability.
|
||||
src/features/shopping/WatchedItemsList.test.tsx: Contains a disabled test that used the manual promise pattern for its loading state.
|
||||
src/pages/admin/AdminStatsPage.test.tsx: A standard async data-fetching component test, but it also has a disabled test that used the manual promise pattern.
|
||||
src/pages/admin/CorrectionsPage.test.tsx: Similar to the stats page, it's a data-fetching component with a history of instability (a disabled test with the manual promise pattern).
|
||||
src/pages/ResetPasswordPage.test.tsx: Contains a disabled test that used the manual promise pattern.
|
||||
Tier 3: Medium-Low Risk (Simple Async & State)
|
||||
These files test straightforward components, but they involve some level of asynchronous operation or state that makes them slightly more complex than static render tests.
|
||||
|
||||
src/features/flyer/BulkImporter.test.tsx: Tests file drag-and-drop events, which involves managing component state (isProcessing).
|
||||
src/pages/admin/components/PasswordInput.test.tsx: Tests a component with internal state (toggling password visibility) and conditional rendering of a child component.
|
||||
src/features/flyer/ProcessingStatus.test.tsx: Tests a component with timers (vi.useFakeTimers) and complex conditional rendering based on props.
|
||||
src/components/Header.test.tsx: Renders different content based on authStatus and profile props.
|
||||
Tier 4: Lowest Risk (Simple, Synchronous Rendering)
|
||||
These files test simple, mostly stateless components. They are very unlikely to cause a memory leak or crash the entire suite.
|
||||
|
||||
src/components/AdminRoute.test.tsx: Tests routing logic, which is generally stable.
|
||||
src/features/charts/PriceChart.test.tsx: Primarily tests data formatting and conditional rendering based on props, with no async calls.
|
||||
src/features/charts/TopDeals.test.tsx: Purely a data sorting and rendering component. Very low risk.
|
||||
src/pages/admin/AdminPage.test.tsx: Simple page with links and a mocked child component.
|
||||
src/features/flyer/FlyerDisplay.test.tsx: Tests date formatting and conditional rendering.
|
||||
src/features/flyer/FlyerList.test.tsx: Renders a list based on props.
|
||||
src/pages/admin/components/PasswordStrengthIndicator.test.tsx: Mocks the zxcvbn library but is otherwise a synchronous, pure component.
|
||||
src/components/ConfirmationModal.test.tsx: Simple modal interaction test.
|
||||
src/components/WhatsNewModal.test.tsx: Another simple modal test.
|
||||
src/features/flyer/BulkImportSummary.test.tsx: Simple component that renders a summary from props.
|
||||
src/components/DarkModeToggle.test.tsx: Simple state toggle.
|
||||
src/components/UnitSystemToggle.test.tsx: Simple state toggle.
|
||||
src/pages/admin/components/AnonymousUserBanner.test.tsx: Simple banner with a button.
|
||||
src/features/flyer/SampleDataButton.test.tsx: A button with an onClick handler.
|
||||
src/components/ErrorDisplay.test.tsx: The simplest component, just renders text based on a prop.
|
||||
src/components/LoadingSpinner.test.tsx: A static SVG component. Extremely low risk.
|
||||
Based on this analysis, I strongly recommend addressing src/index.test.tsx first by deleting it, as it is almost certainly the primary cause of the memory leak and test suite crash.
|
||||
|
||||
|
||||
|
||||
|
||||
add ability to rescan flyers / images with square areas being identified for further processing
|
||||
|
||||
this is a multi-stpep prcess of
|
||||
- the system ideitifies data it did not get from image
|
||||
- thesystem displays image
|
||||
- the user use square boxes to identify important data
|
||||
|
||||
|
||||
|
||||
SQL Injection Vulnerability:
|
||||
|
||||
In many places, you are constructing queries with template literals or string concatenation. A prime example would be a function like getMostFrequentSaleItems(days, limit). If the days parameter were used to construct a query like ... WHERE created_at > NOW() - INTERVAL '${days} days', it could be vulnerable.
|
||||
Recommendation: Always use parameterized queries. The node-postgres (pg) library, which you seem to be using, fully supports this. Instead of injecting variables directly into the SQL string, use placeholders like $1, $2, etc., and pass the variables in an array as the second argument to pool.query(). This is the single most important thing you can do for database security.
|
||||
|
||||
|
||||
Transaction Management:
|
||||
|
||||
You have functions like createFlyerAndItems which correctly imply a transaction is needed to ensure that either both the flyer and its items are created, or neither is.
|
||||
Recommendation: Ensure that any operation involving multiple INSERT, UPDATE, or DELETE statements that must succeed or fail together is wrapped in a transaction. This involves getting a client from the pool (pool.connect()), sending BEGIN, executing your queries, and then COMMIT or ROLLBACK.
|
||||
Connection Handling:
|
||||
|
||||
It's good practice to use a connection pool, which you are doing with getPool().
|
||||
Recommendation: For simple, single queries, using pool.query() is fine. For transactions or multi-statement operations, you should check out a client from the pool with pool.connect(), use that client for all queries in the transaction, and then release it back to the pool with client.release() in a finally block to prevent connection leaks, even if errors occur.
|
||||
|
||||
|
||||
Error Handling & Specificity:
|
||||
|
||||
Your current route handlers catch generic Error objects. The database can throw more specific errors (e.g., for unique constraint violations, foreign key violations, etc.).
|
||||
Recommendation: In your database service, catch errors from pg and check their code property (e.g., '23505' for unique violation). You can then re-throw a custom, more descriptive error (e.g., class DuplicateEmailError extends Error) that your route handlers or a middleware could interpret to send a more specific HTTP status code (like 409 Conflict instead of a generic 500).
|
||||
|
||||
|
||||
Performance:
|
||||
|
||||
Functions like getShoppingLists seem to be fetching lists and their nested items. This can lead to the "N+1 query problem" if you first fetch all lists, and then loop through them to fetch items for each list.
|
||||
Recommendation: Use SQL JOINs and aggregation functions like json_agg and json_build_object in PostgreSQL to fetch parent records and their nested children in a single, efficient query. This dramatically reduces the number of round-trips to the database.
|
||||
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user