feat(upload): Implement Resume Upload Module, Form Validation, and Mock API Integration#6
feat(upload): Implement Resume Upload Module, Form Validation, and Mock API Integration#6kingandrenz wants to merge 9 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Elvicharde
left a comment
Elvicharde
left a comment
There was a problem hiding this comment.
Final summary review (revisions required)
What was done well
- Great UX-first work: added an Upload page, a polished UploadForm with drag & drop, clear upload states, and a small mock API to simulate latency.
- FileValidator is simple and easy to reason about; components show sensible visual feedback.
Checklist before re-requesting review
[ ] Fix Navbar/NavLinks circular import and align exports with filenames
[ ] Add Upload import in App.tsx and confirm build
[ ] Implement or guard useAuth
[ ] Fix Tailwind typos and run style/lint checks
[ ] Add try/catch/finally around mockUploadResume in UploadForm
[ ] Run: npm run build (if available) and confirm green or tsc --noEmit
Next steps
- Please address the inline review comments in the changed files (App.tsx, NavLinks.tsx, Navbar.tsx, UploadForm.tsx, FileValidator.ts, mockApi.ts, pages/Upload.tsx).
- After you’ve made the required fixes, push the changes and request another review. I’ll re-check the navbar structure, TypeScript strictness, Tailwind fixes, and UploadForm error flows.
I am open to discussing this review, if necessary, and I look forward to merging your revisions.
Nice work!
There was a problem hiding this comment.
Issues: Missing Import for Upload component and Routing Architecture Inconsistency
- Missing Import for Upload Component
Observation(s):
The Upload component is rendered but never imported.
Reasoning: JSX usage without a corresponding import triggers a TypeScript compile error and prevents the app from building. Imports should always be included to avoid breaking app during runtime (or compile in the tsx case). Ideally, every PR should run without components, functions, etc. depending on another PR for declaration/resolution.
Actionable Revisions:
(1). Add the missing import for the Upload component.
(2). Always run npm run build to resolve issues locally before making a PR. Alternatively, you can run tsc --noEmit.
- Routing Architecture Inconsistency
Observation(s):
Upload is rendered directly inside App.tsx.
Reasoning: If the project uses react-router, placing a page component directly in App breaks routing structure and causes unexpected behaviour. It is expected that Upload should be a page of its own and should be routed as such, unless that is not the plan. If this is temporary (for testing), it should have been explained in the PR description.
Actionable Revisions:
Render Upload through the router if it’s meant to be a page i.e. <Route path="/upload" element={}>.
There was a problem hiding this comment.
Issues: Circular/Self Import, Component Misalignment, Invalid Tailwind Classes, Avatar Accessibility Issues, and useAuth Inconsistency
- Circular/Self Import
Observation(s):
NavLinks.tsx imports NavLinks from itself (import NavLinks from './NavLinks').
Reasoning:
Self-importing creates a circular dependency that can cause runtime errors, infinite recursion, broken hot reload, and inconsistent component resolution. A component should never import itself unless the import path leads to a different module (which is not the case here).
Actionable Revisions:
(1) Remove the self-import entirely
(2) Re-check the file to confirm no logic relies on this import.
- Component Misalignment Between Files
Observation(s):
(1) NavLinks.tsx and Navbar.tsx appear to export incorrect components (Navbar logic appears inside NavLinks and vice versa).
(2) NavLinks attempts to render inside itself (infinite recursion risk).
Reasoning:
Rendering the same component from within itself results in stack overflows or build failures.
Actionable Revisions:
(1) Ensure Navbar.tsx exports Navbar and handles menu toggle state.
(2) Ensure NavLinks.tsx exports NavLinks and contains only link-list UI.
(3) Remove any recursive usage inside NavLinks.tsx.
- Invalid Tailwind Classes
Observation(s):
Invalid utilities detected i.e. bgr-gray-200/50, size-8, size-10, min-w[84px] (missing hyphen), tracking-[0.00156m]
Reasoning:
Invalid Tailwind utilities silently fail and break layout without warning, and this reduces UI consistency across the project. Every PR should avoid introducing non-existent classes unless explicitly configured in tailwind.config.js.
Actionable Revisions:
(1) Replace bad classes with valid equivalents i.e. bg-gray-200/50, w-8 h-8, w-10 h-10, min-w-[84px] respectively.
(2) Validate all arbitrary values using Tailwind’s standards.
(3) Confirm no other non-standard classes remain.
- Avatar Accessibility Issues
Observation(s):
Avatar NavLink is self-closing and lacks accessible text (aria-label, , or children).
Reasoning:
Self-closing links with no accessible name are invisible to screen readers and break keyboard navigation. Every interactive element must expose an accessible name.
Actionable Revisions:
(1) Provide accessible text for the Navlink e.g. aria-label="Profile"
(2) Avoid self-closing unless using aria-label.
- useAuth Hook Inconsistency
Observation(s):
useAuth is imported, but comments indicate the hook doesn't exist yet.
Reasoning:
As previously stated, importing a non-existent or incomplete files, functions, or hooks, leads to runtime crashes, undefined values, and type failures. PRs should not include broken imports of functions/components that aren’t implemented yet.
Actionable Revisions:
Implement the useAuth hook or notify the assigned individual of its requirement.
There was a problem hiding this comment.
Issues: Component Swap with NavLinks component i.e. Duplicated Logic, and Repeated Tailwind Typos
- Component Logic and Export Mismatch (same as Navlinks)
Observation(s):
Duplicates functionality defined in NavLinks.tsx.
Reasoning:
Having duplicate functionalities in multiple components exposes the whole app to unpredictable behaviour and duplicate bugs. Also, inconsistent behavior between environments due to loading the wrong component.
Actionable Revisions:
(1). Move all NavLinks UI logic back to NavLinks.tsx or refactor the common logic into an independent component.
(2). If Navbar needs to be retained, let it be a wrapper for render of the Navlinks with the following exportable functionalities: menu state, toggle handlers.
(3). Remove duplicated NavLinks code.
(4). Fix Tailwind Typos or issues as highlighted in Navlinks.tsx
There was a problem hiding this comment.
Issues: Hardcoded Validation Values
- Hardcoded Validation Rules
Observation(s):
Allowed MIME types and max file size are inline constants within the function.
Reasoning:
Hardcoding makes values harder to maintain, test, or update. Externalizing them improves reusability and helps avoid duplication across the codebase.
Actionable Revisions:
(1). Define such constraints in an external file that can be imported into this and can be re-used in other components that may later require such definitions. This pattern is also good to adopt for defining types and interfaces that may otherwise clutter a component.
Export constants like so (from an external file):
export const ALLOWED_TYPES = [...];
export const MAX_FILE_SIZE = ...There was a problem hiding this comment.
Issues: Potential Tailwind Config Mismatch
- Tailwind Color Token Inconsistency
Observation(s):
Classes like text-text-secondary-light may not exist. Unless it has already been defined in tailwind config following our adopted colour scheme?
Reasoning:
Invalid Tailwind tokens silently fail and degrade the page’s visual consistency.
Actionable Revisions:
(1). Confirm these tokens exist in tailwind.config.js and replace or rename accordingly.
There was a problem hiding this comment.
Issues: Missing Error Handling, Validation Mismatch, Accessibility Gaps, and Typing Improvements
- Missing Error Handling Around mockUploadResume
Observation(s):
(1). Upload function assumes success and has no try/catch.
Reasoning:
Unhandled promise rejections can break UX, leave the UI stuck in loading state, or cause console errors. While it is expected for this to always resolve since it is demo data (that is guaranteed), it is better to implement the error handling logic in advance. That way, mockUploadResume is replaced with an actual Api call without further modifications.
Actionable Revisions:
(1). Wrap upload logic in try/catch/finally to handle failures gracefully
(2). Ensure uploading state resets correctly even when errors occur.
- Accept Attribute vs FileValidator Mismatch
Observation(s):
Input accepts .pdf and .docx, while validator accepts .doc as well.
Reasoning:
Mismatch forces users into confusing scenarios where files pass validator but cannot be selected in the input dialog.
Actionable Revisions:
Decide whether .doc should be supported, then make both validator and input consistent.
- Accessibility Issues with Hidden File Input
Observation(s):
(1). Hidden file input lacks an associated visible or sr-only label for screen readers.
(2). Drag-and-drop area has no accessible name.
Reasoning:
Screen readers cannot interact with un-labeled inputs. Drop zones must expose clear ARIA descriptions to assistive technologies.
Actionable Revisions:
(1). Add a visually hidden label for file input.
(2). Add aria-label or role="button" to drag region.
- Typing and Minor Optimization
Observation(s):
(1). This Component lacks an explicit return type (Typescript).
(2). The Inline dragOver style object is re-created on every render.
Reasoning:
(1). Explicit component return types improve TS inference.
(2). Inline objects cause unnecessary re-renders.
Actionable Revisions:
(1). Add explicit return type:
function UploadForm(): JSX.Element.(2). Memoize dragOverStyle, with isdragging as a dependency.
2 participants
This PR introduces the complete, functional Resume Upload Form component, including client-side file validation, drag-and-drop functionality, state management for file selection/upload, and integration with the mock upload API service.
My implementation Details:
@Elvicharde / @queenhabeebah Please review for any possible correction