Conversation
|
Please resolve your conflicts |
|
Can you also resolve your merge conflicts? |
ostepan8
force-pushed
the
applications-hooks
branch
from
1013e30 to
fff28fc
Compare
There was a problem hiding this comment.
Pull request overview
Adds a frontend data-fetching hook and wires it into the Admin dashboard so the “Recent Applications” table renders backend-driven application data (instead of hardcoded mocks), alongside new path aliases and updated tests.
Changes:
- Added
getApplications()to the frontend API client and introduced auseApplicationshook to load applications for the Admin dashboard. - Refactored
ApplicationTableto acceptapplicationsas props, added date formatting, and expanded filtering to include email. - Added
@hookspath alias support (Vite + TS) and updated/added unit tests for the hook/table.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/frontend/vite.config.ts | Adds @hooks alias for Vite module resolution. |
| apps/frontend/tsconfig.json | Adds @hooks/* path mapping for TypeScript builds/tests. |
| apps/frontend/src/hooks/useApplications.ts | Introduces useApplications hook and ApplicationRow mapping. |
| apps/frontend/src/hooks/useApplications.test.ts | Adds unit tests for the new hook’s loading/success/error flows. |
| apps/frontend/src/containers/AdminLanding.tsx | Wires useApplications into the Admin dashboard and passes data into the table. |
| apps/frontend/src/components/ApplicationTable.tsx | Switches table to props-driven rendering, adds date formatting + email filtering. |
| apps/frontend/src/components/ApplicationTable.test.tsx | Adds unit tests for table rendering, filtering, and date formatting. |
| apps/frontend/src/app.spec.tsx | Updates assertion to match the current dashboard heading text. |
| apps/frontend/src/api/apiClient.ts | Adds getApplications() API client method. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function toRows(applications: Application[]): ApplicationRow[] { | ||
| return applications.map((app) => ({ | ||
| appId: app.appId, | ||
| name: app.email, | ||
| email: app.email, | ||
| proposedStartDate: '', | ||
| actualStartDate: '', | ||
| experienceType: app.experienceType, | ||
| discipline: app.discipline, | ||
| applicantType: app.applicantType, | ||
| status: app.appStatus, | ||
| })); |
There was a problem hiding this comment.
toRows is currently setting name to app.email and hard-coding proposedStartDate/actualStartDate to empty strings. This makes the table’s “Name” column show an email address and the date columns always render placeholders. Either populate these fields from real backend fields (or from the applicants endpoint) or adjust the table columns/row shape to match the data actually available from Application.
There was a problem hiding this comment.
Use the backend fields from the application - retrieve the name from candidateInfo and proposedStartDate and actualStartDate should already be in application itself
| async function fetchData() { | ||
| setLoading(true); | ||
| setError(null); | ||
| try { | ||
| const apps = await apiClient.getApplications(); | ||
| if (!cancelled) { | ||
| setApplications(toRows(apps)); | ||
| } |
There was a problem hiding this comment.
The PR description says the hook fetches applications and applicants in parallel and merges them by appId, but this implementation only calls apiClient.getApplications() and never loads/joins applicant data. Please either implement the applicants fetch + merge (e.g., Promise.all and a lookup map keyed by appId) or update the PR description and UI expectations accordingly.
| public async getApplications(): Promise<Application[]> { | ||
| return this.get('/api/applications') as Promise<Application[]>; | ||
| } |
There was a problem hiding this comment.
The PR description mentions adding both getApplications and getApplicants to the API client, but this diff only adds getApplications. If the frontend needs the applicants endpoint for name/date enrichment, add the missing getApplicants method here (and corresponding types), or update the PR description to match what was actually implemented.
| if (!cancelled) { | ||
| setApplications(toRows(apps)); | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
The catch block discards the actual error, and always shows a generic message. Capturing the thrown value (e.g., catch (err)) and including/logging the message (while still showing a user-friendly fallback) will make debugging API failures significantly easier.
| } catch { | |
| } catch (err) { | |
| // Log the actual error for debugging while showing a generic message to users. | |
| console.error('Failed to load applications:', err); |
|
Also, use the status pills component |
ℹ️ Issue
Closes #201
📝 Description
added a react hook to fetch all applications from the backend api and display them in the admin dashboard table instead of hardcoded mock data
1 added getApplications and getApplicants methods to the api client so the frontend can call both endpoints
2 created a useApplications hook that fetches applications and applicants in parallel then merges them by app id to get names and dates alongside application data
3 updated the application table component to accept real data as props instead of using a hardcoded array
4 wired the hook into the root container so the dashboard cards show live counts and the table renders actual database records
5 added a hooks path alias to vite config and tsconfig
6 fixed the app spec test that was checking for a welcome message that no longer exists
7 wrote 18 tests covering the hook and table component including loading states error handling filtering and data merging
✔️ Verification
1 started the backend with nx serve backend and frontend with nx serve frontend
2 confirmed both api endpoints return data by curling localhost 3000 api applications and localhost 3000 api applicants
3 opened localhost 4200 in the browser and verified the table shows real application rows with names dates disciplines and statuses from the database
4 ran nx test frontend and confirmed all 20 tests pass across 3 test files
5 ran nx lint frontend and confirmed zero lint errors
6 tested search filtering in the table and confirmed it filters by name email discipline and status
🏕️ (Optional) Future Work / Notes
right now the backend sends back every application in one go which is fine for now but will definitely start to lag once we have a lot more data in there so down the line we should look into adding pagination on the server side
also the frontend is making two separate api calls to get applications and applicants then stitching them together which works but its an extra round trip we could avoid if the backend just joined those tables before sending the response
one other thing i noticed is that the discipline admin name column from the original design doesnt actually have any data backing it yet so i swapped it out for applicant type for now until we get that relationship set up on the backend