added editing for ocr extracted receipt Closes #11#117
added editing for ocr extracted receipt Closes #11#117archa8 merged 7 commits intoCode-A2Z:masterfrom
Conversation
✅ Deploy Preview for paisable ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds editing functionality for OCR extracted receipt data, allowing users to review and modify the extracted information before saving it as a transaction. The implementation introduces a two-step process: first extracting data from the receipt, then allowing user verification/editing before final save.
Key changes:
- Separated receipt processing from transaction creation with a confirmation step
- Added modal-based editing interface for extracted receipt data
- Implemented new backend endpoint for saving user-confirmed transactions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/pages/ReceiptsPage.jsx | Added editing modal, confirmation UI, and two-step transaction creation workflow |
| backend/routes/receiptRoutes.js | Added new route for saving confirmed transactions |
| backend/controllers/receiptController.js | Removed automatic transaction creation and added endpoint for user-confirmed transaction saving |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| const savedTransaction = await newTransaction.save(); | ||
|
|
||
| // update the receipt with the final confirmed data |
There was a problem hiding this comment.
Corrected capitalization: 'update' should be 'Update' at the beginning of the comment.
| // update the receipt with the final confirmed data | |
| // Update the receipt with the final confirmed data |
frontend/src/pages/ReceiptsPage.jsx
Outdated
| const handleEditReceiptSubmit = async (formData, transactionId) => { | ||
| try { | ||
| // Update the receiptResult with the edited data | ||
| const updatedReceiptResult = { | ||
| ...receiptResult, | ||
| extractedData: { | ||
| merchant: formData.name, | ||
| amount: parseFloat(formData.cost), | ||
| category: formData.category, | ||
| date: formData.addedOn, | ||
| isIncome: formData.isIncome | ||
| } | ||
| }; | ||
|
|
||
| setReceiptResult(updatedReceiptResult); | ||
| setOpenEditReceiptResult(false); | ||
| } catch (err) { | ||
| setError('Failed to update receipt data. Please try again.'); | ||
| console.error(err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The function parameter transactionId is not used anywhere in the function body. Either remove it or implement the intended functionality that uses this parameter.
|
what i have done is like, |
|
@archa8 could you please review this PR when you get a chance. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
frontend/src/pages/ReceiptsPage.jsx
Outdated
| const handleEditReceiptSubmit = async (formData) => { | ||
| try { | ||
| // Update the receiptResult with the edited data | ||
| const updatedReceiptResult = { | ||
| ...receiptResult, | ||
| extractedData: { | ||
| merchant: formData.name, | ||
| amount: parseFloat(formData.cost), | ||
| category: formData.category, | ||
| date: formData.addedOn, | ||
| isIncome: formData.isIncome | ||
| } | ||
| }; | ||
|
|
||
| setReceiptResult(updatedReceiptResult); | ||
| setOpenEditReceiptResult(false); | ||
| } catch (err) { | ||
| setError('Failed to update receipt data. Please try again.'); | ||
| console.error(err); | ||
| } |
There was a problem hiding this comment.
The function is marked as async but does not contain any await operations. Since it only performs synchronous state updates, the try-catch block will not catch errors from asynchronous operations that don't exist. Remove the async keyword and consider removing the try-catch block as synchronous state updates are unlikely to throw errors.
| const handleEditReceiptSubmit = async (formData) => { | |
| try { | |
| // Update the receiptResult with the edited data | |
| const updatedReceiptResult = { | |
| ...receiptResult, | |
| extractedData: { | |
| merchant: formData.name, | |
| amount: parseFloat(formData.cost), | |
| category: formData.category, | |
| date: formData.addedOn, | |
| isIncome: formData.isIncome | |
| } | |
| }; | |
| setReceiptResult(updatedReceiptResult); | |
| setOpenEditReceiptResult(false); | |
| } catch (err) { | |
| setError('Failed to update receipt data. Please try again.'); | |
| console.error(err); | |
| } | |
| const handleEditReceiptSubmit = (formData) => { | |
| // Update the receiptResult with the edited data | |
| const updatedReceiptResult = { | |
| ...receiptResult, | |
| extractedData: { | |
| merchant: formData.name, | |
| amount: parseFloat(formData.cost), | |
| category: formData.category, | |
| date: formData.addedOn, | |
| isIncome: formData.isIncome | |
| } | |
| }; | |
| setReceiptResult(updatedReceiptResult); | |
| setOpenEditReceiptResult(false); |
frontend/src/pages/ReceiptsPage.jsx
Outdated
| <p className="text-gray-700 dark:text-gray-300"><strong>Date:</strong> {new Date(receiptResult.extractedData.date).toLocaleDateString()}</p> | ||
| <div className="mb-4"> | ||
| <p className="text-gray-700 dark:text-gray-300"><strong>Merchant:</strong> {receiptResult.extractedData.merchant}</p> | ||
| <p className="text-gray-700 dark:text-gray-300"><strong>Amount:</strong> {receiptResult.extractedData.amount.toFixed(2)}</p> |
There was a problem hiding this comment.
Potential runtime error if receiptResult.extractedData.amount is null or undefined. Since the amount comes from user-edited data and parseFloat can return NaN, consider adding a fallback: {(receiptResult.extractedData.amount || 0).toFixed(2)}
| <p className="text-gray-700 dark:text-gray-300"><strong>Amount:</strong> {receiptResult.extractedData.amount.toFixed(2)}</p> | |
| <p className="text-gray-700 dark:text-gray-300"><strong>Amount:</strong> {(parseFloat(receiptResult.extractedData.amount) || 0).toFixed(2)}</p> |
| name: transactionData.name, | ||
| category: transactionData.category, | ||
| cost: transactionData.cost, | ||
| addedOn: new Date(transactionData.addedOn), |
There was a problem hiding this comment.
If transactionData.addedOn is an invalid date string, new Date() will create an Invalid Date object, which could cause issues downstream. Add validation to ensure the date is valid before creating the transaction.
|
@archa8 could you please review this pr again, for any issues. |
|
@archa8 Do I have to make any further changes? |
|
This PR implements the feature mention in Issue: #11 |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
frontend/src/pages/ReceiptsPage.jsx:17
- Unused variable isEditingResult.
const [isEditingResult, setIsEditingResult] = useState(false);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/pages/ReceiptsPage.jsx
Outdated
| // Handle mobile responsive resize | ||
| useEffect(() => { | ||
| const handleResize = () => { | ||
| return setIsMobile(window.innerWidth <= 767); |
There was a problem hiding this comment.
The handleResize function unnecessarily returns the result of setIsMobile. Since setIsMobile returns undefined, this return statement serves no purpose. Remove the return keyword to make the function's intent clearer.
| return setIsMobile(window.innerWidth <= 767); | |
| setIsMobile(window.innerWidth <= 767); |
frontend/src/pages/ReceiptsPage.jsx
Outdated
| new Date().toISOString().split("T")[0], | ||
| isIncome: receiptResult.extractedData.isIncome || false, | ||
| }} | ||
| categories={categories} |
There was a problem hiding this comment.
The TransactionModal component expects separate expenseCategories and incomeCategories props (see line 17 of TransactionModal.jsx), but this code passes a single categories prop. This will cause the modal to not display any categories in its dropdown. Update the prop name to match the expected API, or fetch and pass separate category arrays for expenses and income.
| categories={categories} | |
| expenseCategories={categories ? categories.filter(cat => !cat.isIncome) : []} | |
| incomeCategories={categories ? categories.filter(cat => cat.isIncome) : []} |
frontend/src/pages/ReceiptsPage.jsx
Outdated
| const handleNewCategory = (newCategory) => { | ||
| setCategories((prev) => [...prev, newCategory].sort()); |
There was a problem hiding this comment.
The onNewCategory callback in TransactionModal passes two parameters: newCategory and isIncome (line 65 of TransactionModal.jsx), but handleNewCategory only accepts one parameter. This mismatch means the isIncome flag is ignored, preventing proper categorization of income vs. expense categories. Update the function signature to (newCategory, isIncome) and handle the categories accordingly.
frontend/src/pages/ReceiptsPage.jsx
Outdated
|
|
||
| const [openEditReceiptResult, setOpenEditReceiptResult] = useState(false); | ||
| const [categories, setCategories] = useState([]); | ||
| const [isEditingResult, setIsEditingResult] = useState(false); |
There was a problem hiding this comment.
The isEditingResult state variable is set in handleEditResult (line 142) and cleared in the modal's onClose callback (line 262), but it is never used elsewhere in the component. This unused state should be removed to reduce complexity.
| const [isEditingResult, setIsEditingResult] = useState(false); |
frontend/src/pages/ReceiptsPage.jsx
Outdated
| </div> | ||
|
|
||
| <img | ||
| src={`http://localhost:5001${receiptResult.fileUrl}`} |
There was a problem hiding this comment.
The hardcoded http://localhost:5001 URL creates a production deployment issue and won't work outside of local development. Use an environment variable (e.g., process.env.REACT_APP_API_BASE_URL) or construct the URL dynamically based on the current environment to ensure the application works correctly in all deployment scenarios.
frontend/src/pages/ReceiptsPage.jsx
Outdated
| isIncome: receiptResult.extractedData.isIncome || false, | ||
| }; | ||
|
|
||
| const response = await api.post("/receipts/save-transaction", { |
There was a problem hiding this comment.
Unused variable response.
| const response = await api.post("/receipts/save-transaction", { | |
| await api.post("/receipts/save-transaction", { |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {receiptResult.extractedData.isIncome && ( | ||
| <p className="text-gray-700 dark:text-gray-300"> | ||
| <strong>Income:</strong> Yes | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
The code attempts to access receiptResult.extractedData.isIncome, but the backend's uploadReceipt endpoint (lines 68-73 in receiptController.js) doesn't set this field when creating the receipt. This will always be undefined until after the user saves the transaction. Consider setting a default value for isIncome in the extractedData when the receipt is first created, or handle the undefined case appropriately.
| {receiptResult.extractedData.isIncome && ( | |
| <p className="text-gray-700 dark:text-gray-300"> | |
| <strong>Income:</strong> Yes | |
| </p> | |
| )} | |
| {receiptResult.extractedData.isIncome === true && ( | |
| <p className="text-gray-700 dark:text-gray-300"> | |
| <strong>Income:</strong> Yes | |
| </p> | |
| )} | |
| {receiptResult.extractedData.isIncome === false && ( | |
| <p className="text-gray-700 dark:text-gray-300"> | |
| <strong>Income:</strong> No | |
| </p> | |
| )} |
| src={`${import.meta.env.VITE_API_URL?.replace( | ||
| "/api", | ||
| "" | ||
| )}${receiptResult.fileUrl}`} |
There was a problem hiding this comment.
The image URL construction uses string replacement to remove '/api' from the VITE_API_URL. This is fragile and could break if the API URL doesn't contain '/api' or has it in a different position. Consider using a separate environment variable for the base URL without '/api', or construct the URL more reliably (e.g., using URL parsing).
| }); | ||
|
|
||
| setTimeout(() => navigate("/dashboard"), 1000); |
There was a problem hiding this comment.
Using a hardcoded setTimeout delay before navigation can lead to the user missing the success toast if they navigate away. The 1-second delay is arbitrary and may not be sufficient on slower devices. Consider navigating immediately or using the toast's onClose callback to navigate after the toast is dismissed.
| }); | |
| setTimeout(() => navigate("/dashboard"), 1000); | |
| onClose: () => navigate("/dashboard"), | |
| }); |
| res.status(201).json({ | ||
| message: 'Transaction saved successfully', | ||
| transaction: savedTransaction, | ||
| receipt: receipt |
There was a problem hiding this comment.
[nitpick] Using shorthand property syntax would be more concise. Change receipt: receipt to just receipt.
| receipt: receipt | |
| receipt |

No description provided.