feat(jef-87): Experiences and skills for user - #236
Merged
Merged
Conversation
- 3 new DB tables: workExperience, education, skill (with cascade delete) - Domain entities, repository ports, Drizzle implementations, mappers - 9 use cases (CRUD for each) - GraphQL types, inputs, queries, mutations - Resolvers + DI container wiring - Migration 0009 generated
- Settings page with CRUD for work experience, education, and skills - GraphQL operations for all three entities - Nav item added to settings sidebar - Route tree regenerated
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds user-owned work experience, education, and skill CRUD across Drizzle, the API GraphQL schema, and an authenticated web settings page. ChangesExperience profile CRUD
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
Preview deployments for this PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
apps/api/src/http/schema/mutations/workExperienceMutations.ts (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated authentication guard.
The same three lines repeat in every field resolver. The cohort adds nine such fields across work experience, education, and skills. A shared helper, for example
requireUser(ctx), returns the user id and throws theUnauthorizedGraphQLError. This keeps the guard consistent if the error code or the context shape changes later.♻️ Proposed helper usage
- if (!ctx.user) - throw new GraphQLError('Unauthorized', { extensions: { code: ERROR_CODES.UNAUTHORIZED } }); - const { workExperienceResolver } = ctx.diScope.cradle; - return workExperienceResolver.createWorkExperience(ctx.user.sub, { + const userId = requireUser(ctx); + const { workExperienceResolver } = ctx.diScope.cradle; + return workExperienceResolver.createWorkExperience(userId, {Also applies to: 39-43, 61-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/http/schema/mutations/workExperienceMutations.ts` around lines 16 - 20, Extract the repeated authentication check from the work experience field resolvers into a shared requireUser(ctx) helper that validates ctx.user, throws the existing Unauthorized GraphQLError with ERROR_CODES.UNAUTHORIZED when absent, and returns the authenticated user id. Replace each inline guard in the affected resolvers with this helper and pass its result to the resolver methods.apps/api/src/http/schema/types/inputs/WorkExperienceInputs.ts (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider a
DateTimescalar for the date fields.
startDateandendDateare declared asString. Each resolver then parses the string by hand. A customDateTimescalar moves parsing and validation into the schema layer and removes the duplicated conversion across work experience, education, and skills. This also fixes the validation gap raised onapps/api/src/interface-adapters/resolvers/WorkExperienceResolver.tsat the schema level.Also applies to: 19-20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/http/schema/types/inputs/WorkExperienceInputs.ts` around lines 8 - 9, Replace the string-based startDate and endDate fields in the work-experience input type with the project’s existing DateTime scalar, preserving startDate as required and endDate as optional. Update the related education and skills input date fields similarly, then remove redundant resolver-level string parsing so resolvers consume validated DateTime values directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/infrastructure/db/repositories/DrizzleEducationRepository.ts`:
- Around line 57-63: Handle empty UPDATE RETURNING results in the update methods
of DrizzleEducationRepository, DrizzleSkillRepository, and
DrizzleWorkExperienceRepository by returning null before calling toEntity;
update the corresponding education, skill, and work-experience update use cases
to translate null into the established not-found result.
In `@apps/api/src/interface-adapters/resolvers/EducationResolver.ts`:
- Around line 44-53: Validate startDate and the optional endDate in
EducationResolver.createEducation and the corresponding update operation before
invoking their use cases, rejecting invalid date strings rather than
constructing invalid Date values. Preserve null handling for omitted endDate and
only pass valid Date instances to the use cases; use the resolver’s existing
error-handling convention for the rejection.
In `@apps/api/src/interface-adapters/resolvers/WorkExperienceResolver.ts`:
- Around line 44-73: Validate date strings in createWorkExperience and
updateWorkExperience before converting them with Date, rejecting malformed
values with a clear boundary error instead of passing Invalid Date to the use
cases. Apply the same validation to optional endDate values while preserving
null and undefined semantics in updateWorkExperience.
In `@apps/web/src/routes/_authenticated/settings/experience.tsx`:
- Around line 496-503: Update the delete handlers for work experience,
education, and skills in the settings experience component to require
confirmation before invoking the respective mutation, or provide an undo
affordance after successful deletion. Ensure the existing deleteWe flow and the
corresponding education and skills deletion flows no longer remove records on
the initial click without user confirmation.
- Around line 24-174: The inline GraphQL operations and handwritten experience
types in apps/web/src/routes/_authenticated/settings/experience.tsx#L24-L174 and
lines 208-238 are unused duplicates; remove them and update the settings page to
import and use the generated documents and types. Run GraphQL code generation
for apps/web/src/graphql/mutations/experiences.graphql#L1-L87 and
apps/web/src/graphql/queries/experiences.graphql#L1-L37, then commit the
generated output so all twelve operations and their types are consumed by the
page.
- Around line 804-822: Update the edit and delete buttons in the skill list to
use a focus visibility class alongside the existing hover opacity behavior,
ensuring keyboard focus reveals them. Make each aria-label include the
associated skill name so screen readers can distinguish actions for different
skills.
- Around line 516-528: Associate all fifteen label/input pairs in the settings
experience form with accessible identifiers: update each input to provide a
unique stable id, preferably generated with useId, and set the corresponding
label’s htmlFor to that id. Apply this consistently to the Company field and the
other pairs identified in the diff, preserving their existing registration and
validation behavior.
- Around line 573-585: Update the work experience, education, and skill form
submit flows to use each mutation’s pending state for button disabling and the
“Saving…” label instead of formState.isSubmitting, since the mutation callbacks
return void. Add mutation onError handlers that call the corresponding form’s
setError('root', ...) with the failure message so the existing root error blocks
display submission failures.
- Around line 297-307: Update the work-experience update mutation in updateWe to
send null, rather than undefined or empty strings, for cleared nullable fields
such as location, endDate, and description; apply the same null-clearing rule to
the corresponding nullable education and work-experience update inputs. In
apps/api/src/http/schema/mutations/skillMutations.ts lines 37-41, update the
skill mutation handling likewise so cleared nullable skill fields are sent and
persisted as null.
---
Nitpick comments:
In `@apps/api/src/http/schema/mutations/workExperienceMutations.ts`:
- Around line 16-20: Extract the repeated authentication check from the work
experience field resolvers into a shared requireUser(ctx) helper that validates
ctx.user, throws the existing Unauthorized GraphQLError with
ERROR_CODES.UNAUTHORIZED when absent, and returns the authenticated user id.
Replace each inline guard in the affected resolvers with this helper and pass
its result to the resolver methods.
In `@apps/api/src/http/schema/types/inputs/WorkExperienceInputs.ts`:
- Around line 8-9: Replace the string-based startDate and endDate fields in the
work-experience input type with the project’s existing DateTime scalar,
preserving startDate as required and endDate as optional. Update the related
education and skills input date fields similarly, then remove redundant
resolver-level string parsing so resolvers consume validated DateTime values
directly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e655bc10-eadc-4960-808d-1f07c554aa0e
📒 Files selected for processing (57)
apps/api/drizzle/0009_brave_famine.sqlapps/api/drizzle/meta/0009_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/domain/education/Education.tsapps/api/src/domain/skill/Skill.tsapps/api/src/domain/workExperience/WorkExperience.tsapps/api/src/http/container.tsapps/api/src/http/schema/index.tsapps/api/src/http/schema/mutations/educationMutations.tsapps/api/src/http/schema/mutations/skillMutations.tsapps/api/src/http/schema/mutations/workExperienceMutations.tsapps/api/src/http/schema/queries/educationQueries.tsapps/api/src/http/schema/queries/skillQueries.tsapps/api/src/http/schema/queries/workExperienceQueries.tsapps/api/src/http/schema/types/EducationType.tsapps/api/src/http/schema/types/SkillType.tsapps/api/src/http/schema/types/WorkExperienceType.tsapps/api/src/http/schema/types/inputs/EducationInputs.tsapps/api/src/http/schema/types/inputs/SkillInputs.tsapps/api/src/http/schema/types/inputs/WorkExperienceInputs.tsapps/api/src/infrastructure/db/repositories/DrizzleEducationRepository.tsapps/api/src/infrastructure/db/repositories/DrizzleSkillRepository.tsapps/api/src/infrastructure/db/repositories/DrizzleWorkExperienceRepository.tsapps/api/src/infrastructure/db/schema.tsapps/api/src/interface-adapters/mappers/EducationMapper.tsapps/api/src/interface-adapters/mappers/SkillMapper.tsapps/api/src/interface-adapters/mappers/WorkExperienceMapper.tsapps/api/src/interface-adapters/resolvers/EducationResolver.tsapps/api/src/interface-adapters/resolvers/SkillResolver.tsapps/api/src/interface-adapters/resolvers/WorkExperienceResolver.tsapps/api/src/use-cases/education/CreateEducationUseCase.tsapps/api/src/use-cases/education/DeleteEducationUseCase.tsapps/api/src/use-cases/education/ICreateEducationUseCase.tsapps/api/src/use-cases/education/IDeleteEducationUseCase.tsapps/api/src/use-cases/education/IUpdateEducationUseCase.tsapps/api/src/use-cases/education/UpdateEducationUseCase.tsapps/api/src/use-cases/ports/IEducationRepository.tsapps/api/src/use-cases/ports/ISkillRepository.tsapps/api/src/use-cases/ports/IWorkExperienceRepository.tsapps/api/src/use-cases/skill/CreateSkillUseCase.tsapps/api/src/use-cases/skill/DeleteSkillUseCase.tsapps/api/src/use-cases/skill/ICreateSkillUseCase.tsapps/api/src/use-cases/skill/IDeleteSkillUseCase.tsapps/api/src/use-cases/skill/IUpdateSkillUseCase.tsapps/api/src/use-cases/skill/UpdateSkillUseCase.tsapps/api/src/use-cases/workExperience/CreateWorkExperienceUseCase.tsapps/api/src/use-cases/workExperience/DeleteWorkExperienceUseCase.tsapps/api/src/use-cases/workExperience/ICreateWorkExperienceUseCase.tsapps/api/src/use-cases/workExperience/IDeleteWorkExperienceUseCase.tsapps/api/src/use-cases/workExperience/IUpdateWorkExperienceUseCase.tsapps/api/src/use-cases/workExperience/UpdateWorkExperienceUseCase.tsapps/web/src/graphql/mutations/experiences.graphqlapps/web/src/graphql/queries/experiences.graphqlapps/web/src/routeTree.gen.tsapps/web/src/routes/_authenticated/route.tsxapps/web/src/routes/_authenticated/settings/experience.tsxapps/web/src/routes/_authenticated/settings/route.tsx
Comment on lines
+57
to
+63
| async update(id: string, data: UpdateEducationData): Promise<Education> { | ||
| const [row] = await this.db | ||
| .update(education) | ||
| .set({ ...data, updatedAt: new Date() }) | ||
| .where(eq(education.id, id)) | ||
| .returning(); | ||
| return this.toEntity(row); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Handle an empty RETURNING result before mapping the row.
Each update() method assumes that RETURNING contains a row. A concurrent delete can remove the record after the ownership check and before the UPDATE. The destructuring then produces undefined, and toEntity(row) throws instead of returning a controlled not-found result.
apps/api/src/infrastructure/db/repositories/DrizzleEducationRepository.ts#L57-L63: returnnullwhen no updated row exists, then handle that result in the education update use case.apps/api/src/infrastructure/db/repositories/DrizzleSkillRepository.ts#L54-L56: returnnullwhen no updated row exists, then handle that result in the skill update use case.apps/api/src/infrastructure/db/repositories/DrizzleWorkExperienceRepository.ts#L61-L67: returnnullwhen no updated row exists, then handle that result in the work-experience update use case.
📍 Affects 3 files
apps/api/src/infrastructure/db/repositories/DrizzleEducationRepository.ts#L57-L63(this comment)apps/api/src/infrastructure/db/repositories/DrizzleSkillRepository.ts#L54-L56apps/api/src/infrastructure/db/repositories/DrizzleWorkExperienceRepository.ts#L61-L67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/infrastructure/db/repositories/DrizzleEducationRepository.ts`
around lines 57 - 63, Handle empty UPDATE RETURNING results in the update
methods of DrizzleEducationRepository, DrizzleSkillRepository, and
DrizzleWorkExperienceRepository by returning null before calling toEntity;
update the corresponding education, skill, and work-experience update use cases
to translate null into the established not-found result.
Comment on lines
+44
to
+53
| async createEducation(userId: string, input: CreateInput): Promise<EducationDTO> { | ||
| const result = await this.deps.createEducationUseCase.execute({ | ||
| userId, | ||
| institution: input.institution, | ||
| degree: input.degree, | ||
| field: input.field, | ||
| startDate: new Date(input.startDate), | ||
| endDate: input.endDate ? new Date(input.endDate) : null, | ||
| description: input.description, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid date strings before persistence.
An authenticated caller can send a non-date string for startDate or endDate. new Date() then creates an invalid Date. That value can cause a database failure or make EducationMapper.toDTO() throw at toISOString().
Validate each supplied date before calling the use case. Apply the same validation to create and update operations.
Proposed fix
+function toValidDate(value: string): Date {
+ const date = new Date(value);
+ if (Number.isNaN(date.getTime())) {
+ throw new Error('Invalid date');
+ }
+ return date;
+}
+
- startDate: new Date(input.startDate),
- endDate: input.endDate ? new Date(input.endDate) : null,
+ startDate: toValidDate(input.startDate),
+ endDate: input.endDate ? toValidDate(input.endDate) : null,
...
- startDate: input.startDate ? new Date(input.startDate) : undefined,
- endDate: input.endDate === null ? null : input.endDate ? new Date(input.endDate) : undefined,
+ startDate: input.startDate ? toValidDate(input.startDate) : undefined,
+ endDate: input.endDate === null ? null : input.endDate ? toValidDate(input.endDate) : undefined,Also applies to: 57-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/interface-adapters/resolvers/EducationResolver.ts` around lines
44 - 53, Validate startDate and the optional endDate in
EducationResolver.createEducation and the corresponding update operation before
invoking their use cases, rejecting invalid date strings rather than
constructing invalid Date values. Preserve null handling for omitted endDate and
only pass valid Date instances to the use cases; use the resolver’s existing
error-handling convention for the rejection.
Comment on lines
+44
to
+73
| async createWorkExperience(userId: string, input: CreateInput): Promise<WorkExperienceDTO> { | ||
| const result = await this.deps.createWorkExperienceUseCase.execute({ | ||
| userId, | ||
| company: input.company, | ||
| title: input.title, | ||
| location: input.location, | ||
| startDate: new Date(input.startDate), | ||
| endDate: input.endDate ? new Date(input.endDate) : null, | ||
| description: input.description, | ||
| }); | ||
| return this.deps.workExperienceMapper.toDTO(result); | ||
| } | ||
|
|
||
| async updateWorkExperience( | ||
| userId: string, | ||
| id: string, | ||
| input: UpdateInput, | ||
| ): Promise<WorkExperienceDTO> { | ||
| const result = await this.deps.updateWorkExperienceUseCase.execute({ | ||
| id, | ||
| userId, | ||
| company: input.company, | ||
| title: input.title, | ||
| location: input.location, | ||
| startDate: input.startDate ? new Date(input.startDate) : undefined, | ||
| endDate: input.endDate === null ? null : input.endDate ? new Date(input.endDate) : undefined, | ||
| description: input.description, | ||
| }); | ||
| return this.deps.workExperienceMapper.toDTO(result); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the date strings before you convert them.
startDate and endDate arrive as GraphQL String values (see apps/api/src/http/schema/types/inputs/WorkExperienceInputs.ts lines 8-9). new Date('not-a-date') returns an Invalid Date instead of throwing. The invalid value then reaches the repository, and WorkExperienceMapper.toDTO throws a RangeError on toISOString(). The client receives an opaque internal error, or an invalid timestamp is persisted.
Reject malformed dates at this boundary and return a clear error.
🛠️ Proposed fix: parse dates through a guard
+function parseDate(value: string, field: string): Date {
+ const parsed = new Date(value);
+ if (Number.isNaN(parsed.getTime())) {
+ throw new Error(`Invalid date for ${field}`);
+ }
+ return parsed;
+}
+
export class WorkExperienceResolver {- startDate: new Date(input.startDate),
- endDate: input.endDate ? new Date(input.endDate) : null,
+ startDate: parseDate(input.startDate, 'startDate'),
+ endDate: input.endDate ? parseDate(input.endDate, 'endDate') : null,- startDate: input.startDate ? new Date(input.startDate) : undefined,
- endDate: input.endDate === null ? null : input.endDate ? new Date(input.endDate) : undefined,
+ startDate: input.startDate ? parseDate(input.startDate, 'startDate') : undefined,
+ endDate:
+ input.endDate === null
+ ? null
+ : input.endDate
+ ? parseDate(input.endDate, 'endDate')
+ : undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async createWorkExperience(userId: string, input: CreateInput): Promise<WorkExperienceDTO> { | |
| const result = await this.deps.createWorkExperienceUseCase.execute({ | |
| userId, | |
| company: input.company, | |
| title: input.title, | |
| location: input.location, | |
| startDate: new Date(input.startDate), | |
| endDate: input.endDate ? new Date(input.endDate) : null, | |
| description: input.description, | |
| }); | |
| return this.deps.workExperienceMapper.toDTO(result); | |
| } | |
| async updateWorkExperience( | |
| userId: string, | |
| id: string, | |
| input: UpdateInput, | |
| ): Promise<WorkExperienceDTO> { | |
| const result = await this.deps.updateWorkExperienceUseCase.execute({ | |
| id, | |
| userId, | |
| company: input.company, | |
| title: input.title, | |
| location: input.location, | |
| startDate: input.startDate ? new Date(input.startDate) : undefined, | |
| endDate: input.endDate === null ? null : input.endDate ? new Date(input.endDate) : undefined, | |
| description: input.description, | |
| }); | |
| return this.deps.workExperienceMapper.toDTO(result); | |
| } | |
| function parseDate(value: string, field: string): Date { | |
| const parsed = new Date(value); | |
| if (Number.isNaN(parsed.getTime())) { | |
| throw new Error(`Invalid date for ${field}`); | |
| } | |
| return parsed; | |
| } | |
| async createWorkExperience(userId: string, input: CreateInput): Promise<WorkExperienceDTO> { | |
| const result = await this.deps.createWorkExperienceUseCase.execute({ | |
| userId, | |
| company: input.company, | |
| title: input.title, | |
| location: input.location, | |
| startDate: parseDate(input.startDate, 'startDate'), | |
| endDate: input.endDate ? parseDate(input.endDate, 'endDate') : null, | |
| description: input.description, | |
| }); | |
| return this.deps.workExperienceMapper.toDTO(result); | |
| } | |
| async updateWorkExperience( | |
| userId: string, | |
| id: string, | |
| input: UpdateInput, | |
| ): Promise<WorkExperienceDTO> { | |
| const result = await this.deps.updateWorkExperienceUseCase.execute({ | |
| id, | |
| userId, | |
| company: input.company, | |
| title: input.title, | |
| location: input.location, | |
| startDate: input.startDate ? parseDate(input.startDate, 'startDate') : undefined, | |
| endDate: | |
| input.endDate === null | |
| ? null | |
| : input.endDate | |
| ? parseDate(input.endDate, 'endDate') | |
| : undefined, | |
| description: input.description, | |
| }); | |
| return this.deps.workExperienceMapper.toDTO(result); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/interface-adapters/resolvers/WorkExperienceResolver.ts` around
lines 44 - 73, Validate date strings in createWorkExperience and
updateWorkExperience before converting them with Date, rejecting malformed
values with a clear boundary error instead of passing Invalid Date to the use
cases. Apply the same validation to optional endDate values while preserving
null and undefined semantics in updateWorkExperience.
Comment on lines
+24
to
+174
| const WORK_EXPERIENCES_QUERY = ` | ||
| query WorkExperiences { | ||
| workExperiences { | ||
| id | ||
| company | ||
| title | ||
| location | ||
| startDate | ||
| endDate | ||
| description | ||
| createdAt | ||
| updatedAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const EDUCATIONS_QUERY = ` | ||
| query Educations { | ||
| educations { | ||
| id | ||
| institution | ||
| degree | ||
| field | ||
| startDate | ||
| endDate | ||
| description | ||
| createdAt | ||
| updatedAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const SKILLS_QUERY = ` | ||
| query Skills { | ||
| skills { | ||
| id | ||
| name | ||
| category | ||
| proficiency | ||
| createdAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| // ── Mutations ──────────────────────────────────────────────────────────── | ||
|
|
||
| const CREATE_WORK_EXPERIENCE = ` | ||
| mutation CreateWorkExperience($input: CreateWorkExperienceInput!) { | ||
| createWorkExperience(input: $input) { | ||
| id | ||
| company | ||
| title | ||
| location | ||
| startDate | ||
| endDate | ||
| description | ||
| createdAt | ||
| updatedAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const UPDATE_WORK_EXPERIENCE = ` | ||
| mutation UpdateWorkExperience($id: ID!, $input: UpdateWorkExperienceInput!) { | ||
| updateWorkExperience(id: $id, input: $input) { | ||
| id | ||
| company | ||
| title | ||
| location | ||
| startDate | ||
| endDate | ||
| description | ||
| createdAt | ||
| updatedAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const DELETE_WORK_EXPERIENCE = ` | ||
| mutation DeleteWorkExperience($id: ID!) { | ||
| deleteWorkExperience(id: $id) | ||
| } | ||
| `; | ||
|
|
||
| const CREATE_EDUCATION = ` | ||
| mutation CreateEducation($input: CreateEducationInput!) { | ||
| createEducation(input: $input) { | ||
| id | ||
| institution | ||
| degree | ||
| field | ||
| startDate | ||
| endDate | ||
| description | ||
| createdAt | ||
| updatedAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const UPDATE_EDUCATION = ` | ||
| mutation UpdateEducation($id: ID!, $input: UpdateEducationInput!) { | ||
| updateEducation(id: $id, input: $input) { | ||
| id | ||
| institution | ||
| degree | ||
| field | ||
| startDate | ||
| endDate | ||
| description | ||
| createdAt | ||
| updatedAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const DELETE_EDUCATION = ` | ||
| mutation DeleteEducation($id: ID!) { | ||
| deleteEducation(id: $id) | ||
| } | ||
| `; | ||
|
|
||
| const CREATE_SKILL = ` | ||
| mutation CreateSkill($input: CreateSkillInput!) { | ||
| createSkill(input: $input) { | ||
| id | ||
| name | ||
| category | ||
| proficiency | ||
| createdAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const UPDATE_SKILL = ` | ||
| mutation UpdateSkill($id: ID!, $input: UpdateSkillInput!) { | ||
| updateSkill(id: $id, input: $input) { | ||
| id | ||
| name | ||
| category | ||
| proficiency | ||
| createdAt | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const DELETE_SKILL = ` | ||
| mutation DeleteSkill($id: ID!) { | ||
| deleteSkill(id: $id) | ||
| } | ||
| `; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The new .graphql documents have no consumer. This change adds twelve GraphQL operations as .graphql documents and then redefines all twelve as inline template strings in the settings page. The page uses the inline copies, so the .graphql files and any generated types are unused, and the two copies will drift.
apps/web/src/routes/_authenticated/settings/experience.tsx#L24-L174: delete the inline query and mutation strings, import the generated documents, and replace the hand-writtenWorkExperience,Education, andSkilltypes at lines 208-238 with the generated types.apps/web/src/graphql/mutations/experiences.graphql#L1-L87: run GraphQL code generation and commit the generated output so the settings page can import these nine mutations.apps/web/src/graphql/queries/experiences.graphql#L1-L37: run GraphQL code generation and commit the generated output so the settings page can import these three queries.
As per coding guidelines: "Run GraphQL code generation after changing web .graphql operations or the API schema."
📍 Affects 3 files
apps/web/src/routes/_authenticated/settings/experience.tsx#L24-L174(this comment)apps/web/src/graphql/mutations/experiences.graphql#L1-L87apps/web/src/graphql/queries/experiences.graphql#L1-L37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/routes/_authenticated/settings/experience.tsx` around lines 24 -
174, The inline GraphQL operations and handwritten experience types in
apps/web/src/routes/_authenticated/settings/experience.tsx#L24-L174 and lines
208-238 are unused duplicates; remove them and update the settings page to
import and use the generated documents and types. Run GraphQL code generation
for apps/web/src/graphql/mutations/experiences.graphql#L1-L87 and
apps/web/src/graphql/queries/experiences.graphql#L1-L37, then commit the
generated output so all twelve operations and their types are consumed by the
page.
Source: Coding guidelines
Comment on lines
+297
to
+307
| const updateWe = useMutation({ | ||
| mutationFn: (data: WorkExperienceForm) => | ||
| gqlClient.request(UPDATE_WORK_EXPERIENCE, { | ||
| id: weEditing!.id, | ||
| input: { | ||
| ...data, | ||
| location: data.location || undefined, | ||
| endDate: data.endDate || undefined, | ||
| description: data.description || undefined, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching experience.tsx and skillMutations.ts:"
fd -a 'experience\.tsx|skillMutations\.ts' . || true
echo
echo "Search relevant update paths:"
rg -n "useMutation|UPDATE_WORK_EXPERIENCE|updateEducation|skill|updateSkill|proficiency|proficiency" apps/web/src/routes/_authenticated/settings/experience.tsx apps/api/src/http/schema/mutations/skillMutations.ts || true
echo
echo "Relevant source slices:"
sed -n '260,380p' apps/web/src/routes/_authenticated/settings/experience.tsx || true
echo "--- skillMutations.ts ---"
sed -n '1,90p' apps/api/src/http/schema/mutations/skillMutations.ts || trueRepository: mankatcheung/job-finder
Length of output: 14694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate schema input types and skill resolver:"
fd -a 'SkillInputs\.ts|SkillResolver|.*resolvers.*' apps/api || true
rg -n "type UpdateSkillInput|input UpdateSkillInput|class.*SkillResolver|updateSkill\\(|nullable: true|new GraphQLString\\(" apps/api || true
echo
echo "Relevant SkillInputs.ts:"
sed -n '1,140p' apps/api/src/http/schema/types/inputs/SkillInputs.js || true
sed -n '1,120p' apps/api/src/http/schema/types/inputs/SkillInputs.ts || true
echo
echo "SkillResolver files:"
for f in $(rg -l "updateSkill\\(" apps/api/src | tr '\n' ' '); do
echo "--- $f ---"
ast-grep outline "$f" --match updateSkill || true
sed -n '1,230p' "$f"
doneRepository: mankatcheung/job-finder
Length of output: 11944
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Update skill use case files:"
fd -a 'IUpdateSkillUseCase|UpdateSkillUseCase|skill' apps/api/src/use-cases apps/api/src/interface-adapters | sort
echo
echo "Inspect skill update use case implementation:"
for f in $(fd -a 'UpdateSkillUseCase' apps/api/src || true); do
echo "--- $f ---"
wc -l "$f"
sed -n '1,240p' "$f"
done
echo
echo "Inspect SkillMapper/repository if update writes SQL/Prisma:"
for f in $(rg -l "update.*Skill|SkillRepository|ISkillRepository|skills" apps/api/src/interface-adapters apps/api/src/use-cases || true); do
echo "--- $f ---"
ast-grep outline "$f" --match update || true
rg -n "update|SQL|prisma|skill" "$f" || true
done
echo
echo "Experience schema and resolver nullable fields:"
fd -a 'WorkExperienceInputs|EducationInputs|WorkExperienceType|EducationType' apps/api/src/http/schema/types apps/api/src/http/schema/types/inputs || true
rg -n "UpdateWorkExperienceInput|UpdateEducationInput|description:.*nullable|endDate:.*nullable|category:.*nullable|proficiency:.*nullable" apps/api/src/http/schema apps/api/src/interface-adapters apps/api/src/use-cases || trueRepository: mankatcheung/job-finder
Length of output: 7403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect UpdateWorkExperienceUseCase and UpdateEducationUseCase:"
for f in apps/api/src/use-cases/workExperience/UpdateWorkExperienceUseCase.ts apps/api/src/use-cases/education/UpdateEducationUseCase.ts; do
echo "--- $f ---"
sed -n '1,140p' "$f"
done
echo
echo "Inspect GraphQL mutable fields from updated entity use-cases:"
for f in \
apps/api/src/http/schema/types/inputs/WorkExperienceInputs.ts \
apps/api/src/http/schema/types/inputs/EducationInputs.ts \
apps/api/src/http/schema/types/WorkExperienceType.ts \
apps/api/src/http/schema/types/EducationType.ts
do
echo "--- $f ---"
sed -n '1,160p' "$f"
done
echo
echo "Programmatic check: omitted-undefined/null effect in UpdateSkillUseCase behavior model"
node - <<'JS'
const updateSkill = (existing, input) => {
const changes = {
...(input.name !== undefined ? { name: input.name } : {}),
...(input.category !== undefined && input.category !== null /* repository update will not set null */ ? { category: input.category } : {}),
...(input.proficiency !== undefined && input.proficiency !== null ? { proficiency: input.proficiency } : {}),
};
return { ...existing, ...changes };
};
const state = { name: 'React', category: 'Frontend', proficiency: 'Advanced' };
console.log(JSON.stringify({
existing: state,
omittedCategory: updateSkill(state, { name: 'React', proficiency: 'Beginner' }),
explicitNullCategory: updateSkill(state, { name: 'React', category: null, proficiency: 'Beginner' }),
explicitNullProficiency: updateSkill(state, { name: 'React', category: '', proficiency: null }),
explicitEmptyProficiency: updateSkill(state, { name: 'React', category: '', proficiency: '' }),
}, null, 2));
JSRepository: mankatcheung/job-finder
Length of output: 7005
Send null for cleared nullable update fields. Empty strings are not the same as clearing a nullable field: omitting a field or sending null currently leaves the existing value unchanged for skills, and empty strings persist as blank text for skills. Use null for cleared optional work experience and education fields as well, and keep the same rule for education and work experience nullable fields across the update inputs.
📍 Affects 2 files
apps/web/src/routes/_authenticated/settings/experience.tsx#L297-L307(this comment)apps/api/src/http/schema/mutations/skillMutations.ts#L37-L41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/routes/_authenticated/settings/experience.tsx` around lines 297
- 307, Update the work-experience update mutation in updateWe to send null,
rather than undefined or empty strings, for cleared nullable fields such as
location, endDate, and description; apply the same null-clearing rule to the
corresponding nullable education and work-experience update inputs. In
apps/api/src/http/schema/mutations/skillMutations.ts lines 37-41, update the
skill mutation handling likewise so cleared nullable skill fields are sent and
persisted as null.
Comment on lines
+496
to
+503
| <button | ||
| type="button" | ||
| onClick={() => deleteWe.mutate(we.id)} | ||
| className="p-1.5 text-gray-400 hover:text-red-600 dark:hover:text-red-400" | ||
| aria-label="Delete" | ||
| > | ||
| <Trash2Icon size={14} /> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a confirmation step before deletion.
The delete button calls deleteWe.mutate(we.id) on the first click. The record is removed permanently with no confirmation and no undo. The same applies to education at line 667 and skills at line 817.
Add a confirmation dialogue, or an undo affordance after the delete succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/routes/_authenticated/settings/experience.tsx` around lines 496
- 503, Update the delete handlers for work experience, education, and skills in
the settings experience component to require confirmation before invoking the
respective mutation, or provide an undo affordance after successful deletion.
Ensure the existing deleteWe flow and the corresponding education and skills
deletion flows no longer remove records on the initial click without user
confirmation.
Comment on lines
+516
to
+528
| <div> | ||
| <label className={labelCls}>Company *</label> | ||
| <input | ||
| {...weForm.register('company')} | ||
| className={inputCls} | ||
| placeholder="Acme Corp" | ||
| /> | ||
| {weForm.formState.errors.company && ( | ||
| <p className="mt-1 text-xs text-red-600"> | ||
| {weForm.formState.errors.company.message} | ||
| </p> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Associate every label with its input.
The <label> elements carry no htmlFor, and the inputs carry no id. Screen readers announce these fields without a name. Clicking the label does not focus the field. This applies to all fifteen label and input pairs in this file, including lines 530, 543, 551, 560, 565, 686, 699, 703, 711, 720, 725, 837, 850, and 858.
Use useId to generate stable identifiers, or wrap each control inside its <label>.
🛠️ Proposed pattern for one field
+ const weCompanyId = useId(); <div>
- <label className={labelCls}>Company *</label>
+ <label htmlFor={weCompanyId} className={labelCls}>
+ Company *
+ </label>
<input
+ id={weCompanyId}
{...weForm.register('company')}
className={inputCls}
placeholder="Acme Corp"
/>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/routes/_authenticated/settings/experience.tsx` around lines 516
- 528, Associate all fifteen label/input pairs in the settings experience form
with accessible identifiers: update each input to provide a unique stable id,
preferably generated with useId, and set the corresponding label’s htmlFor to
that id. Apply this consistently to the Company field and the other pairs
identified in the diff, preserving their existing registration and validation
behavior.
Comment on lines
+573
to
+585
| {weForm.formState.errors.root?.message && ( | ||
| <p className="text-sm text-red-600 bg-red-50 dark:bg-red-900/20 rounded-lg px-3 py-2"> | ||
| {weForm.formState.errors.root.message} | ||
| </p> | ||
| )} | ||
| <div className="flex items-center gap-2"> | ||
| <button | ||
| type="submit" | ||
| disabled={weForm.formState.isSubmitting} | ||
| className="px-4 py-2 bg-blue-600 hover:bg-blue-700 disabled:opacity-60 text-white text-sm font-medium rounded-lg transition-colors" | ||
| > | ||
| {weForm.formState.isSubmitting ? 'Saving…' : weEditing ? 'Update' : 'Add'} | ||
| </button> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The submit button never disables, and mutation errors are silent.
Two connected defects:
weForm.formState.isSubmittingtracks the promise returned by thehandleSubmitcallback. The callback at lines 510-512 callsupdateWe.mutate(...), which returnsvoid.isSubmittingtherefore returns tofalseimmediately. The button at line 581 never disables, and the label never shows "Saving…". A user can submit the same record several times.- The root error block at lines 573-577 is dead code. No
onErrorhandler setsweForm.setError('root', ...). If the mutation fails, the form closes nothing, shows nothing, and the user gets no feedback.
Use the mutation pending state for the button, and add an onError handler that sets the root error. The same two defects exist in the education form at lines 733-745 and the skill form at lines 868-880.
🛠️ Proposed change for the work experience form
const updateWe = useMutation({
mutationFn: (data: WorkExperienceForm) => ...,
onSuccess: () => {
qc.invalidateQueries({ queryKey: ['workExperiences'] });
setWeFormOpen(false);
setWeEditing(null);
weForm.reset();
},
+ onError: (error: Error) => {
+ weForm.setError('root', { message: error.message });
+ },
});+ {(() => {
+ const savingWe = createWe.isPending || updateWe.isPending;
+ return null;
+ })()}
<div className="flex items-center gap-2">
<button
type="submit"
- disabled={weForm.formState.isSubmitting}
+ disabled={createWe.isPending || updateWe.isPending}
className="px-4 py-2 bg-blue-600 hover:bg-blue-700 disabled:opacity-60 text-white text-sm font-medium rounded-lg transition-colors"
>
- {weForm.formState.isSubmitting ? 'Saving…' : weEditing ? 'Update' : 'Add'}
+ {createWe.isPending || updateWe.isPending
+ ? 'Saving…'
+ : weEditing
+ ? 'Update'
+ : 'Add'}
</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/routes/_authenticated/settings/experience.tsx` around lines 573
- 585, Update the work experience, education, and skill form submit flows to use
each mutation’s pending state for button disabling and the “Saving…” label
instead of formState.isSubmitting, since the mutation callbacks return void. Add
mutation onError handlers that call the corresponding form’s setError('root',
...) with the failure message so the existing root error blocks display
submission failures.
Comment on lines
+804
to
+822
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| setSkillEditing(skill); | ||
| setSkillFormOpen(true); | ||
| }} | ||
| className="p-0.5 text-gray-400 hover:text-gray-600 dark:hover:text-gray-300 opacity-0 group-hover:opacity-100 transition-opacity" | ||
| aria-label="Edit" | ||
| > | ||
| <PencilIcon size={12} /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => deleteSkill.mutate(skill.id)} | ||
| className="p-0.5 text-gray-400 hover:text-red-600 dark:hover:text-red-400 opacity-0 group-hover:opacity-100 transition-opacity" | ||
| aria-label="Delete" | ||
| > | ||
| <Trash2Icon size={12} /> | ||
| </button> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keyboard users cannot see the focused skill buttons.
The edit and delete buttons use opacity-0 group-hover:opacity-100. The buttons stay in the tab order and stay focusable, but a keyboard user sees nothing when focus lands on them. The aria-label values "Edit" and "Delete" also repeat for every skill, so a screen reader user cannot tell which skill a button belongs to.
Add a focus-visible:opacity-100 variant, or group-focus-within:opacity-100, and include the skill name in each label.
🛠️ Proposed change
<button
type="button"
onClick={() => {
setSkillEditing(skill);
setSkillFormOpen(true);
}}
- className="p-0.5 text-gray-400 hover:text-gray-600 dark:hover:text-gray-300 opacity-0 group-hover:opacity-100 transition-opacity"
- aria-label="Edit"
+ className="p-0.5 text-gray-400 hover:text-gray-600 dark:hover:text-gray-300 opacity-0 group-hover:opacity-100 focus-visible:opacity-100 transition-opacity"
+ aria-label={`Edit ${skill.name}`}
>
<PencilIcon size={12} />
</button>
<button
type="button"
onClick={() => deleteSkill.mutate(skill.id)}
- className="p-0.5 text-gray-400 hover:text-red-600 dark:hover:text-red-400 opacity-0 group-hover:opacity-100 transition-opacity"
- aria-label="Delete"
+ className="p-0.5 text-gray-400 hover:text-red-600 dark:hover:text-red-400 opacity-0 group-hover:opacity-100 focus-visible:opacity-100 transition-opacity"
+ aria-label={`Delete ${skill.name}`}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| type="button" | |
| onClick={() => { | |
| setSkillEditing(skill); | |
| setSkillFormOpen(true); | |
| }} | |
| className="p-0.5 text-gray-400 hover:text-gray-600 dark:hover:text-gray-300 opacity-0 group-hover:opacity-100 transition-opacity" | |
| aria-label="Edit" | |
| > | |
| <PencilIcon size={12} /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => deleteSkill.mutate(skill.id)} | |
| className="p-0.5 text-gray-400 hover:text-red-600 dark:hover:text-red-400 opacity-0 group-hover:opacity-100 transition-opacity" | |
| aria-label="Delete" | |
| > | |
| <Trash2Icon size={12} /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => { | |
| setSkillEditing(skill); | |
| setSkillFormOpen(true); | |
| }} | |
| className="p-0.5 text-gray-400 hover:text-gray-600 dark:hover:text-gray-300 opacity-0 group-hover:opacity-100 focus-visible:opacity-100 transition-opacity" | |
| aria-label={`Edit ${skill.name}`} | |
| > | |
| <PencilIcon size={12} /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => deleteSkill.mutate(skill.id)} | |
| className="p-0.5 text-gray-400 hover:text-red-600 dark:hover:text-red-400 opacity-0 group-hover:opacity-100 focus-visible:opacity-100 transition-opacity" | |
| aria-label={`Delete ${skill.name}`} | |
| > | |
| <Trash2Icon size={12} /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/routes/_authenticated/settings/experience.tsx` around lines 804
- 822, Update the edit and delete buttons in the skill list to use a focus
visibility class alongside the existing hover opacity behavior, ensuring
keyboard focus reveals them. Make each aria-label include the associated skill
name so screen readers can distinguish actions for different skills.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds work experience, education, and skills management for users. This data feeds into AI-generated cover letters and other assistant features.
Changes
API (apps/api)
Frontend (apps/web)
Test plan
Linear
Closes JEF-87
Summary by CodeRabbit