Upgrade react. #494
Upgrade react. #494
Conversation
WalkthroughThis PR upgrades multiple dependencies to newer major versions (React 17→19, TypeScript 4→5, date-fns, axios), migrates deprecated React lifecycle methods to modern equivalents, adds optional children props to several components, updates React Router configuration to v6, replaces Dropzone component usage with hooks, and extends backend file handling to support blob identifiers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
src/Payments.Mvc/ClientApp/src/components/attachmentsControl.tsx (2)
6-18: Context typing is good; consider deriving the type fromTeamContextto avoid driftExplicitly typing
context!: Team;is an improvement, but it duplicates the generic type parameter ofTeamContext. To keep these in sync ifTeamContextever changes, you could derive the type instead:static contextType = TeamContext; - context!: Team; + declare context: React.ContextType<typeof TeamContext>;This way the component always reflects the actual context type definition.
31-31: Redundantas Teamcast now thatcontextis typedSince
contextis already declared asTeamon the class, the extra cast here is unnecessary and slightly noisier:- const team = this.context as Team; + const team = this.context;This keeps the code simpler without changing behavior.
src/Payments.Mvc/ClientApp/src/components/numberControl.tsx (1)
40-45: Lifecycle migration for value syncing looks correct and loop-safe
componentDidUpdatenow only callssetStatewhenprevProps.value !== this.props.value, which is the right pattern to replacecomponentWillReceivePropsand avoids infinite render/update loops while keeping the input’s internal string state in sync with the controlled numericvalueprop. If you ever need the displayed string to react to changes informatindependently ofvalue, you could extend the condition to also compareprevProps.formatvsthis.props.format, but that’s not required for parity with the prior behavior.src/Payments.Mvc/ClientApp/src/components/editItemsTable.tsx (1)
61-75: Derived-items resync in componentDidUpdate is correct and guardedRebuilding the
{ byId, byHash }map incomponentDidUpdateonly whenprevProps.items !== this.props.itemsis a safe replacement forcomponentWillReceiveProps: it keeps internal state aligned with the latestitemswithout risking an update loop, since the second render aftersetStatesees identical props and skips the branch. Longer term, sinceitemsin state is purely derived fromprops.items, you could consider collapsing this to useprops.itemsdirectly and derive the map on the fly or via memoization, but that’s an optional cleanup and not required for this React upgrade.src/Payments.Mvc/Controllers/FilesController.cs (1)
53-54: Consider improving defaults if direct blob access is retained.If the direct blob identifier path remains after addressing the authorization issue, consider:
- Storing content type in blob metadata or deriving from file extension instead of defaulting to "application/octet-stream"
- Using a sanitized filename instead of exposing the internal blob identifier
However, if you implement Option 1 from the security fix above (database lookup for all paths), these defaults become unnecessary since you'll always have
attachment.ContentTypeandattachment.FileName.src/Payments.Mvc/ClientApp/src/components/fileUpload.tsx (2)
132-132: Rejected files are no longer handled.The previous implementation likely had access to rejected files via the callback signature
(accepted, rejected, event). The new implementation only handles accepted files. Consider addingonDropRejectedhandling or configuring dropzone options to provide user feedback for rejected files:-function DropzoneWrapper({ onDrop }: { onDrop: (files: File[]) => void }) { - const { getRootProps, getInputProps, isDragActive } = useDropzone({ onDrop }); +function DropzoneWrapper({ onDrop }: { onDrop: (files: File[]) => void }) { + const { getRootProps, getInputProps, isDragActive } = useDropzone({ + onDrop, + onDropRejected: (rejections) => { + // Log or display error for rejected files + rejections.forEach(r => console.warn(`File rejected: ${r.file.name}`, r.errors)); + }, + maxSize: 5 * 1024 * 1024, // 5 MB limit mentioned in UI + });
241-261: Consider adding file validation options to match the UI text.The dropzone UI indicates a "5 MB" limit (Line 256), but no
maxSizeoption is configured. Consider adding validation options to provide immediate feedback:function DropzoneWrapper({ onDrop }: { onDrop: (files: File[]) => void }) { - const { getRootProps, getInputProps, isDragActive } = useDropzone({ onDrop }); + const { getRootProps, getInputProps, isDragActive } = useDropzone({ + onDrop, + maxSize: 5 * 1024 * 1024, // 5 MB + });src/Payments.Mvc/ClientApp/src/components/modal.tsx (1)
47-63: Lifecycle migration is correct, with minor redundancy in the close path.The migration from deprecated
componentWillReceivePropstocomponentDidUpdateis correctly implemented. The logic properly:
- Synchronizes internal state from props changes
- Triggers
onOpen()/onClose()callbacks appropriately- Defers
init()to a subsequent update cycle when state transitions to openMinor inefficiency: when closing,
setState({ isOpen: false })is called twice—once at line 50 and again insideonClose()at line 179. React batches these, so it's not a bug, but the second call inonClose()is redundant when triggered fromcomponentDidUpdate.Consider guarding the setState in
onClose()to avoid the redundant call:private onClose() { if (this.props.onClosed) { this.props.onClosed(); } this.destroy(); - if (this._isMounted) { + if (this._isMounted && this.state.isOpen) { this.setState({ isOpen: false }); } }src/Payments.Mvc/ClientApp/src/index.tsx (1)
39-52: Route definitions look correct; outdated comment could be clarified.The migration to React Router v6
elementprop syntax is correct. The optional:id?parameter syntax is supported in v6.The comment on line 40 ("Match any server-side routes and send empty content to let MVC return the view details") appears outdated—these routes render actual page components, not empty content. Consider updating or removing it.
- {/* Match any server-side routes and send empty content to let MVC return the view details */} + {/* Client-side routes that render React pages for MVC-served views */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/Payments.Mvc/ClientApp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
src/Payments.Mvc/ClientApp/package.json(1 hunks)src/Payments.Mvc/ClientApp/src/components/alert.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/components/attachmentsControl.tsx(3 hunks)src/Payments.Mvc/ClientApp/src/components/couponSelectControl.tsx(2 hunks)src/Payments.Mvc/ClientApp/src/components/dateControl.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/components/editItemsTable.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/components/fileUpload.tsx(7 hunks)src/Payments.Mvc/ClientApp/src/components/invoiceForm.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/components/modal.tsx(3 hunks)src/Payments.Mvc/ClientApp/src/components/numberControl.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/components/portal.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/components/previewFrame.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/components/rechargeAccountsControl.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/containers/CreateInvoiceContainer.tsx(2 hunks)src/Payments.Mvc/ClientApp/src/containers/EditInvoiceContainer.tsx(3 hunks)src/Payments.Mvc/ClientApp/src/containers/FinancialApproveInvoiceContainer.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/containers/PayInvoiceContainer.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/containers/PreviewRechargeInvoiceContainer.tsx(1 hunks)src/Payments.Mvc/ClientApp/src/index.tsx(2 hunks)src/Payments.Mvc/Controllers/FilesController.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Payments.Mvc/Controllers/FilesController.cs (2)
src/Payments.Core/Services/StorageService.cs (9)
Task(22-22)Task(24-24)Task(29-29)Task(31-31)Task(43-87)Task(89-93)Task(95-113)Task(115-129)Task(131-154)src/Payments.Core/Models/Configuration/StorageSettings.cs (1)
StorageSettings(5-14)
src/Payments.Mvc/ClientApp/src/index.tsx (5)
src/Payments.Mvc/ClientApp/src/pages/CreateInvoice.tsx (1)
CreateInvoicePage(15-19)src/Payments.Mvc/ClientApp/src/pages/EditInvoice.tsx (1)
EditInvoicePage(18-29)src/Payments.Mvc/ClientApp/src/pages/PayInvoice.tsx (1)
PayInvoicePage(8-8)src/Payments.Mvc/ClientApp/src/pages/FinancialApproveInvoice.tsx (1)
FinancialApproveInvoicePage(8-10)src/Payments.Mvc/ClientApp/src/pages/PreviewRechargeInvoice.tsx (1)
PreviewRechargeInvoicePage(7-16)
src/Payments.Mvc/ClientApp/src/components/attachmentsControl.tsx (1)
src/Payments.Mvc/ClientApp/src/models/Team.tsx (1)
Team(1-9)
src/Payments.Mvc/ClientApp/src/components/fileUpload.tsx (1)
src/Payments.Mvc/ClientApp/src/models/Team.tsx (1)
Team(1-9)
🔇 Additional comments (24)
src/Payments.Mvc/Controllers/FilesController.cs (1)
27-27: Method signature supports dual-mode access pattern.The updated signature allowing string input enables backward compatibility with integer IDs while supporting direct blob identifiers. However, ensure the implementation includes proper authorization checks for both paths (see security concern below).
src/Payments.Mvc/ClientApp/src/components/dateControl.tsx (1)
53-53: LGTM!The ref callback formatting change from expression body to block body is a consistent stylistic update across the codebase with no behavioral impact.
src/Payments.Mvc/ClientApp/src/components/couponSelectControl.tsx (2)
122-125: Same type concern applies toisAftercall.If
expiresAtis a string, this comparison will also neednew Date(expiresAt):let isExpired = false; if (expiresAt) { - isExpired = isAfter(new Date(), expiresAt); + isExpired = isAfter(new Date(), new Date(expiresAt)); }
143-143: No action needed. The date-fns v3format()function accepts both Date objects and string inputs. As of v3, string date arguments were restored and are converted internally viatoDate(). The current code at line 143 will work correctly whetherexpiresAtis a Date object or a valid date string from the API/JSON.Likely an incorrect or invalid review comment.
src/Payments.Mvc/ClientApp/src/containers/EditInvoiceContainer.tsx (2)
134-134: LGTM!The ref callback formatting changes are consistent stylistic updates with no behavioral impact.
Also applies to: 177-177
98-98: No changes needed -dueDateis already typed asDatein theEditInvoiceinterface.The
EditInvoicemodel definesdueDate?: Date, so it's already aDateobject. Theformat()call on line 98 is correct as-is and does not require wrapping withnew Date().Likely an incorrect or invalid review comment.
src/Payments.Mvc/ClientApp/src/containers/FinancialApproveInvoiceContainer.tsx (1)
334-334: LGTM!Consistent ref callback formatting update.
src/Payments.Mvc/ClientApp/package.json (1)
23-23: Note: Major date-fns upgrade from v1 to v3.This upgrade skips v2 and includes significant breaking changes:
- Format tokens changed (e.g.,
YYYY→yyyy,DD→dd)- Functions now require
Dateobjects, not stringsThe code changes appear to use the correct v3 tokens (
MM/dd/yyyy), but ensure all date values are parsed toDateobjects before being passed to date-fns functions.src/Payments.Mvc/ClientApp/src/components/fileUpload.tsx (2)
31-31: LGTM on context typing.The
context!: Teamannotation with the definite assignment assertion is the correct pattern for typed class component context in React 19 with TypeScript 5.
184-191: Good defensive check forprogressEvent.total.The guard
progressEvent.totalcorrectly handles cases wheretotalmay be undefined (e.g., when Content-Length header is missing). This prevents division by zero and avoids updating with incorrect progress values.src/Payments.Mvc/ClientApp/src/components/previewFrame.tsx (1)
42-42: LGTM - Ref callback formatting update.The ref callback has been reformatted from an expression body to a block body for consistency. The functionality remains identical.
src/Payments.Mvc/ClientApp/src/containers/PayInvoiceContainer.tsx (1)
317-317: LGTM - Consistent ref callback style.The ref callback formatting aligns with the broader refactoring pattern across the PR. No functional change.
src/Payments.Mvc/ClientApp/src/components/rechargeAccountsControl.tsx (1)
1103-1107: LGTM - Ref callback consistency update.The ref callback has been updated to use block body syntax, consistent with similar changes throughout the PR. Functionality is unchanged.
src/Payments.Mvc/ClientApp/src/containers/PreviewRechargeInvoiceContainer.tsx (1)
173-173: LGTM - Ref callback formatting aligned.Another consistent ref callback update. No functional impact.
src/Payments.Mvc/ClientApp/src/components/alert.tsx (1)
6-6: LGTM - Children prop enables content projection.The optional
childrenprop is properly typed asReact.ReactNodeand correctly rendered within the alert (line 21), enabling flexible content composition.src/Payments.Mvc/ClientApp/src/containers/CreateInvoiceContainer.tsx (2)
139-139: LGTM - Ref callback formatting consistency.The form ref callback has been updated to use block body syntax, consistent with the broader refactoring pattern.
178-178: LGTM - Ref callback formatting consistency.The RechargeAccountsControl ref callback follows the same formatting update pattern.
src/Payments.Mvc/ClientApp/src/components/invoiceForm.tsx (1)
8-8: LGTM - Children prop enables form composition.The optional
childrenprop is properly typed and rendered within the form element (line 27), allowing flexible composition of invoice form content.src/Payments.Mvc/ClientApp/src/components/portal.tsx (1)
11-12: LGTM - Children prop type definition added.The optional
childrenprop has been properly added to the type definition. Since the component already rendersthis.props.children(line 36), this change improves type safety without altering behavior.src/Payments.Mvc/ClientApp/src/components/modal.tsx (2)
8-8: LGTM!Adding explicit
children?: React.ReactNodeis correct for React 18+ where children is no longer implicitly included in the component props type.
39-45: LGTM!Correctly ensures
onOpenedcallback is fired when the modal is initially rendered in an open state. This complements theinit()call in the constructor for the initially-open case.src/Payments.Mvc/ClientApp/src/index.tsx (3)
2-3: LGTM!The migration to React 18's
createRootAPI and React Router v6'sRoutes/Routecomponents is correct.
23-29: LGTM!Clean refactor from namespace import to explicit named imports. Direct function references are cleaner and enable better tree-shaking.
34-54: Optional route parameters with:id?syntax are valid and fully supported in React Router v6.28.0.The
:id?syntax works correctly and routes will match with or without the id segment as expected. However, note that the components (EditInvoicePage, PayInvoicePage, etc.) do not useuseParams()to access the URL parameters—they instead read from globalwindowvariables likeidandmodel. This is by design for this MVC hybrid application where the server renders the page and injects data via window globals before React mounts.
| "react": "^19.0.0", | ||
| "react-dom": "^19.0.0", | ||
| "react-error-overlay": "6.0.9", | ||
| "react-dropzone": "^8.0.4", | ||
| "react-router-dom": "5.3.0", | ||
| "react-scripts": "5.0.1", | ||
| "recompose": "^0.30.0" | ||
| "react-dropzone": "^14.3.5", | ||
| "react-router-dom": "^6.28.0", | ||
| "react-scripts": "5.0.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
react-scripts 5.0.1 React 19 compatibility support
💡 Result:
Short answer: [email protected] is not compatible with React 19 out of the box and will cause peer-dependency/conflict issues. react-scripts 5.x was published before React 19 and many CRA deps (and downstream libs) still expect React 18 or earlier, so installing React 19 with [email protected] commonly produces ERESOLVE/peer-dep errors. [1][2][3]
What to do (recommended)
- Stay on React 18 for projects using [email protected] (pin react/react-dom@18). [2][3]
- If you must use React 19, move off CRA/react-scripts (or wait for an official react-scripts update) — consider alternative toolchains (Vite, Next.js, or a CRA fork) or upgrade packages that declare strict peer deps. [1][3]
- Short-term workarounds: use npm/yarn install flags (--legacy-peer-deps or --force) or dependency overrides to resolve peer conflicts, but expect runtime/compatibility risk. [3]
Sources
- React 19 release notes — react.dev.
- react-scripts v5.0.1 npm page.
- create-react-app GitHub issues reporting React 19 / peer-dependency conflicts with react-scripts.
[email protected] is incompatible with React 19 and will cause peer-dependency conflicts.
React 19 produces ERESOLVE/peer-dependency errors with react-scripts 5.x, which was released before React 19 and declares strict dependency constraints for earlier React versions. Either pin React/React-DOM to version 18, or migrate away from Create React App to a toolchain that supports React 19 (Vite, Next.js, or similar).
🤖 Prompt for AI Agents
In src/Payments.Mvc/ClientApp/package.json around lines 25 to 30, package.json
currently uses react and react-dom ^19.0.0 together with react-scripts 5.0.1
which is incompatible and causes peer-dependency errors; fix by either
downgrading react and react-dom to a 18.x range (e.g., "react": "^18.2.0",
"react-dom": "^18.2.0") to remain on CRA/react-scripts 5, or remove
react-scripts and migrate the project to a React-19-compatible toolchain (e.g.,
replace react-scripts with Vite/Next.js configuration and associated build/dev
scripts and deps); update package.json and npm/Yarn lockfile accordingly and run
an install to verify no peer-dependency conflicts.
| else | ||
| { | ||
| // Treat as direct blob identifier | ||
| identifier = dbIdOrBlobIdentifier; | ||
| contentType = "application/octet-stream"; // Default content type | ||
| fileName = identifier; // Use identifier as filename | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Payments.Mvc/Controllers/FilesController.csRepository: ucdavis/payments
Length of output: 3452
🏁 Script executed:
rg -nP --type=ts --type=tsx --type=js --type=jsx -C3 'files/|GetFile|downloadFile|Identifier' src/ | head -100Repository: ucdavis/payments
Length of output: 87
🏁 Script executed:
rg -n 'files/|GetFile|downloadFile' src/ -C2 | head -80Repository: ucdavis/payments
Length of output: 6370
🏁 Script executed:
cat -n src/Payments.Core/Domain/InvoiceAttachment.csRepository: ucdavis/payments
Length of output: 1686
🏁 Script executed:
cat -n src/Payments.Mvc/Controllers/FilesApiController.csRepository: ucdavis/payments
Length of output: 2731
🏁 Script executed:
sed -n '130,160p' src/Payments.Core/Services/StorageService.csRepository: ucdavis/payments
Length of output: 924
🏁 Script executed:
rg -n "files/getfile|Files.*GetFile" src/ | head -20Repository: ucdavis/payments
Length of output: 393
Critical: Authorization bypass allows cross-team file access via blob identifiers.
The else branch (lines 49-55) accepts any blob identifier without verifying team ownership. Unlike the integer ID path (lines 36-48), which enforces a.Invoice.Team.Id == team.Id, the direct blob identifier path skips authorization entirely. Any authenticated user from Team A could access Team B's attachments by providing the blob identifier. While identifiers are cryptographically random (GUIDs), they are treated as secrets rather than being access-controlled—if an identifier is exposed through logs, error messages, or network sniffing, any TeamEditor can retrieve it.
The frontend code (attachmentsControl.tsx, line 37) deliberately uses the vulnerable identifier path: ${team.slug}/files/getfile/${attachment.identifier}, meaning this vulnerability is actively used in production.
Recommended fix:
Always require database lookup to enforce team authorization:
- else
- {
- // Treat as direct blob identifier
- identifier = dbIdOrBlobIdentifier;
- contentType = "application/octet-stream"; // Default content type
- fileName = identifier; // Use identifier as filename
- }
+ else
+ {
+ // Treat as direct blob identifier - look up by identifier to enforce team ownership
+ var attachment = await _dbContext.InvoiceAttachments
+ .FirstOrDefaultAsync(a => a.Identifier == dbIdOrBlobIdentifier && a.Invoice.Team.Id == team.Id);
+ if (attachment == null)
+ {
+ return NotFound();
+ }
+
+ identifier = attachment.Identifier;
+ contentType = attachment.ContentType;
+ fileName = attachment.FileName;
+ }🤖 Prompt for AI Agents
In src/Payments.Mvc/Controllers/FilesController.cs around lines 49-55, the else
branch currently accepts a raw blob identifier and serves the file without
verifying team ownership; change this so you do a database lookup for
attachments by identifier and enforce the same team check used for integer IDs
(e.g., verify a.Invoice.Team.Id == team.Id), returning 404/403 if not found or
not authorized; extract contentType and fileName from the DB record (or
metadata) instead of assuming application/octet-stream and using the identifier
as filename, and only then stream the blob if authorization passes.
The only change that felt dirty is my fix for the attachment download bug. Having the controller action determine if the id passed in is an int db identifier or a string blob identifier seemed easier than the refactoring necessary to only be working with a db identifier in the react code.
Below is what copilot suggested needs testing. I think I verified all of them. I don't believe that last bullet point on router navigation is applicable. The only navigation I see is
window.history.go(-1)when cancelling out of the invoice container.Key Features to Test on Each Page:
Create/Edit Invoice Pages:
✅ File upload (Dropzone - updated to v14)
✅ Attachments control (uses Context API)
✅ Date picker controls (date-fns v3)
✅ Customer modal editing
✅ Items table editing
✅ Coupon selection (date formatting)
✅ Account selection
✅ Recharge accounts control (refs, validation)
✅ Form validation
✅ Preview modal
Pay/Approve Pages:
✅ Recharge accounts display
✅ Date formatting display
✅ Payment processing forms
What to Look For:
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.