feat(mentorship): connect GitHub and LinkedIn from the profile card - #2385
Conversation
Introduce the mentor-facing registration page at /mentorship/mentor — the default landing for the mentor section until the profiles API can tell us whether the signed-in user already holds a mentor profile. Page structure: - From Your LFX Profile card at the top, sourced live from UserService.getCurrentUserProfile / getUserEmails / getIdentities, with each endpoint failing independently so a partial outage costs only the affected rows. GitHub and LinkedIn are gated on inAuth0, matching the profile panel — CDP also surfaces unclaimed guesses. - Program picker with an optional list of program requests, since a mentor can register a profile and apply to programs later. - Introduction, skills, resume upload (client-side validation, no backend yet), compliance confirmation, terms acknowledgement. - Single parent FormGroup and shared validator in @lfx-one/shared/utils; errors stay hidden until first submit. Shared components — extracted so the enroll wizard reuses them: - lfx-mentorship-terms-acknowledgement - lfx-mentorship-skills-picker - lfx-mentorship-profile-card Refactors that came with the scope: - Promote MentorshipComingSoonService from admin/program-detail up to modules/mentorship/services/ now that the mentor page uses it too; rename MENTORSHIP_PROGRAM_DETAIL_COMING_SOON to the module-wide MENTORSHIP_COMING_SOON_DETAIL. - Fix MENTORSHIP_MENTOR_REQUEST_STATUS_LABELS.pending: was the lowercase 'requested', which contradicted its own spec; now 'Pending', matching the mentor's own view of the request. Adds one route (/mentorship/mentor) and one sidebar item. No new BFF endpoints — submission stops at a toast until the mentorship service exists. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
The pre-PR review trio flagged two error-handling defects and a set of smaller correctness, accessibility and duplication issues in the Become a Mentor page. All are fixed here rather than amended into the feature commit so the review trail stays readable. Error handling: - Drop the component-level catchError on getPrograms. MentorshipService already ends that stream in its own catchError returning EMPTY_MENTORSHIP_PROGRAMS_RESPONSE, so the handler was unreachable. - Log before degrading in the profile card. Its three fetches each fell back silently, so a real profile-endpoint outage rendered placeholder rows indistinguishable from genuinely empty fields. Correctness: - Merge the duplicated styleClass on the resume Browse button; only one of the two values could ever reach the input, silently dropping the other. - Show a loading state on the program picker instead of an empty list while the fetch is in flight. - Derive the resume accept filter, helper text and both error strings from MENTORSHIP_MENTOR_RESUME_EXTENSIONS and MENTORSHIP_MENTOR_RESUME_MAX_BYTES so a new format or a changed cap cannot leave them behind the validator. Accessibility: - Give the skills picker and program selects a programmatic label via ariaLabelledBy, matching TermsAcknowledgementComponent. - Drop the [title] from the non-focusable program-name span; that cell does not truncate, so the tooltip added a keyboard gap and nothing else. - Wrap profile-card emails rather than truncating them, and expose a long resume file name through a title where the control must stay one line. Also corrects two comments that described behavior the code does not have: the profile card's edit button raises the coming-soon toast rather than navigating, and the parent owns the request list because it will POST the registration, not because validation reads its length. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
The second pre-PR review trio found no critical issues and confirmed every round-one fix landed. It raised sixteen findings across the Become a Mentor page, two of them independently by two reviewers. Fixed here rather than amended into the earlier commits so the review trail stays readable. Fabricated data: - Start the mentor's program requests empty. MENTORSHIP_MENTOR_SEED_REQUESTS presented two real program names as requests the mentor had raised, one already accepted, and a user could not tell them from their own history. The two rows move into the spec as a local fixture, so the add and withdraw behaviour is still covered. Correctness: - Cap the introduction at MENTORSHIP_MENTOR_INTRODUCTION_MAX. It was checked only for emptiness, while the enroll wizard's equivalent description field has had both checks all along, and this text is bound for a POST body. - Give every accepted resume format its own MIME type in the picker's accept filter. Only application/pdf was listed, so on macOS — where Finder filters by MIME type rather than suffix, as the comment itself said — a mentor could not select the .doc or .docx the validator accepts. The map is typed against the extension list, so a new format cannot skip it. - Confirm before withdrawing a program request, as the enroll wizard does before deleting a term. The Withdraw button is the destructive control an accepted request sits behind, and it is where the withdraw call will land. Accessibility: - Mark the five field-error paragraphs as role="alert", the repo's established pattern. Submit surfaces only the first error as a toast, so the remaining fields were failing silently for a screen-reader user; the resume picker's rejection had no toast at all. Three of the five live in the extracted components, so the enroll wizard gains this too. - Make the chosen resume file name a tab stop while one is set, and move its tooltip onto pTooltip with tooltipEvent="both", so a keyboard user can read a name the truncation hides. Conventions: - Derive LfxProfileSocialProvider from LFX_PROFILE_SOCIAL_LINKS instead of restating the union, so adding a provider to the constant cannot leave the type behind. - Add the missing data-testids to the resume file name and the per-chip skill removal button, and select through them in the specs instead of a Tailwind class and a direct method call. - Build the resume spec's input and change event from real DOM objects, removing both casts through unknown. - Cite the design behind the two arbitrary Tailwind values, on the primary email badge and the requests table. Also corrects the LfxProfileSummary docstring, which credited lfx-person-avatar for deriving initials and a shared per-person color; the card renders lfx-avatar and styles the fallback itself. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
Stop the Become a Mentor page from confirming work it never persisted, and give its introduction editor a name screen readers can read. - Submit raises the module's coming-soon toast instead of "Registration submitted", which told the mentor each program admin had been notified while nothing was written anywhere. - The program picker's copy no longer promises that choosing a program sends a request to its administrator; choices stay on the page until the registration endpoint exists. - `lfx-rich-editor` takes an optional `ariaLabelledBy`, applied to TipTap's contenteditable, and the introduction label supplies it. A `<label for>` cannot name that element, so the field had no accessible name at all. - Drop the redundant trailing-slash regex from the profile-card social-link resolver, which CodeQL flagged as polynomial on uncontrolled input. Splitting on '/' and dropping empty segments already handled it. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
A profile URL copied from the browser's address bar arrives with its query attached, so `https://github.com/ada?tab=repositories` resolved to the handle `ada?tab=repositories` and rendered a broken link on the profile card. Cut everything from the first '?' or '#' before taking the last path segment. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
…b and LinkedIn Updated the profile card component to replace placeholders for unconnected GitHub and LinkedIn accounts with actionable "Connect" buttons. This allows mentors to initiate the account linking process directly from the profile card. Additionally, the component's tests were updated to verify the new functionality, ensuring that the dialog for adding accounts opens correctly and that the profile refreshes upon successful connection. - Replaced placeholder text with connect buttons in profile-card.component.html - Updated tests in profile-card.component.spec.ts to cover new connect button functionality - Introduced aria-labels for accessibility on connect buttons - Added logic to handle dialog opening and user identity refresh after connecting accounts Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
The social-link callback returns to whichever page opened the Add-identity dialog and reports itself in success/error query params. Under /profile those are read by the Identities tab and ProfileLayoutComponent, but the mentorship forms mount under the main layout where neither exists, so a mentor could complete the whole Auth0 handshake, land back on the registration form, and be told nothing while a stale error param sat in the URL. The card now reads those params on init: it confirms a link and re-reads its summary, names the already_linked conflict (which is in neither shared error map) with a pointer to the tab that can resolve it, reports the Flow C codes that no shell owns here, and refuses to call a bare token mint a linked account. Params are stripped with replaceState so the message cannot replay and the form beneath the card survives. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
Limitation 1 said the social-link callback's impersonation block is toasted by ProfileLayoutComponent. That shell only mounts under /profile, and the callback's returnTo is now an allowlisted caller-supplied page, so the claim no longer held unconditionally. Names which component reports the error on each surface, and warns that a page added to the allowlist has to read these codes itself. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
PR SummaryMedium Risk Overview The OAuth round trip is wired end-to-end: the dialog passes the current pathname as
Reviewed by Cursor Bugbot for commit c7e05f7. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds in-place identity linking to the mentorship profile card while preserving safe OAuth return routing and impersonation protections.
Changes:
- Adds GitHub and LinkedIn Connect actions and callback notifications.
- Propagates allowlisted return paths through social OAuth and Flow C.
- Adds unit coverage and updates impersonation documentation.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/constants/mentorship-lfx-profile-card.constants.ts |
Adds identity-linking labels and messages. |
docs/architecture/backend/impersonation.md |
Documents caller-specific callback handling. |
apps/lfx-one/src/types/express.d.ts |
Adds social return-path session state. |
apps/lfx-one/src/server/services/social-verification.service.ts |
Stores and clears callback return paths. |
apps/lfx-one/src/server/controllers/profile.controller.ts |
Validates and propagates social return paths. |
apps/lfx-one/src/server/controllers/profile.controller.spec.ts |
Tests return-path handling and chaining. |
apps/lfx-one/src/app/modules/profile/components/add-account-dialog/add-account-dialog.component.ts |
Sends the current path when starting OAuth. |
apps/lfx-one/src/app/modules/mentorship/mentor/mentor-register/mentor-register.component.spec.ts |
Updates profile-card service mocks. |
apps/lfx-one/src/app/modules/mentorship/components/profile-card/profile-card.component.ts |
Opens the dialog, refreshes identities, and reports callbacks. |
apps/lfx-one/src/app/modules/mentorship/components/profile-card/profile-card.component.spec.ts |
Covers connection, callbacks, and impersonation. |
apps/lfx-one/src/app/modules/mentorship/components/profile-card/profile-card.component.html |
Replaces missing social identities with Connect buttons. |
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address review comments from the reviewers on PR #2385: - profile-card.component.html: render Connect as a text button ([text], not [link]) because lfx-button does not forward link to p-button (per cursor[bot]) - add-account-dialog / profile-card: offer only GitHub and LinkedIn from mentorship Connect, so Email cannot send Flow C to /profile/emails (per copilot-pull-request-reviewer[bot]) - mentorship-lfx-profile-card: treat a failed identities fetch as unavailable and show the placeholder instead of Connect (per copilot-pull-request-reviewer[bot]) Resolves 3 review threads. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
Review Feedback AddressedCommit: 3a70ac1 Changes Made
Threads Resolved3 of 3 unresolved threads addressed in this iteration. |
…lback browser-only Connect starts a top-level navigation to /api/profile/identities/social/connect, which sits behind blockDuringImpersonation inside the /api error-handler mount — so a click during impersonation replaced the registration form with the error JSON. The card also shows the impersonated user's profile while the link could only ever attach to the impersonator, the wrong-account risk that block exists to catch. Disable both buttons the way the Identities tab disables Add identity, with the same explanation, and refuse in onConnect too. ngOnInit was also handling the OAuth callback on the server: this page is server-rendered with hydration and the app's only p-toast is in the root template, so a callback return rendered a toast into the HTML that the client then added again, and SSR spent a second round of profile fetches nothing could read. Guard the whole handler on isPlatformBrowser. The spec that sets up the callback URL now preserves history.state and restores the URL afterwards, so it stops leaving later specs on /mentorship/mentor. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
Keep the platform in the impersonating aria-label so the two disabled Connect buttons are not announced alike, and test the programmatic onConnect refuse plus the server-side callback skip. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
Address review comments from the reviewers on PR #2385: - profile-card.component.html: render Connect as a text button ([text], not [link]) because lfx-button does not forward link to p-button (per cursor[bot]) - add-account-dialog / profile-card: offer only GitHub and LinkedIn from mentorship Connect, so Email cannot send Flow C to /profile/emails (per copilot-pull-request-reviewer[bot]) - mentorship-lfx-profile-card: treat a failed identities fetch as unavailable and show the placeholder instead of Connect (per copilot-pull-request-reviewer[bot]) Resolves 3 review threads. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 536a5c6. Configure here.
536a5c6 to
a6d0b46
Compare
…ruction with open dialog Added a test case to ensure that the user identities are not refreshed when the profile card is destroyed while the dialog remains open. Updated the ProfileCardComponent to utilize DestroyRef and takeUntilDestroyed for proper cleanup during dialog closure. Signed-off-by: Sameh16 <sameh_mohamed16@hotmail.com>
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
Verified independently against the full diff (not just the PR body). The returnTo chain is allowlist-checked server-side on both legs — normalizeSocialReturnTo on the outbound startSocialConnect redirect, and again in handleSocialCallback off the session-stored value (defensive against a stale session outliving a page rename, per its own comment) — so there's no open-redirect surface even though the client hands the server an arbitrary pathname. identitiesAvailable: boolean correctly distinguishes "identities fetch failed" (null) from "fetch succeeded, nothing linked" ([]); Connect is only offered in the latter case, matching the new buildLfxProfileSummary test. linkErrorDetail's lookups into IDENTITY_LINK_ERROR_MESSAGES/PROFILE_AUTH_ERROR_MESSAGES are Object.hasOwn-guarded, so a crafted ?error=toString falls through to the fallback message instead of resolving Object.prototype.toString. The SSR guard is applied at both ngOnInit (reference) and clearCallbackParams (callee), consistent with this repo's stated convention. New types (identitiesAvailable, AddAccountDialogData.allowedProviders) and constants are correctly placed in @lfx-one/shared/interfaces/@lfx-one/shared/constants, not locally. onConnect() is double-guarded against impersonation (disabled attribute plus an in-method check for the programmatic path). Test coverage is thorough — impersonation disable + programmatic refusal, dialog-open with correct existingProviders/allowedProviders, refresh-on-close across linked/dismissed/destroyed-before-close, and the full callback-param suite including the prototype-pollution guard and history-state preservation. Also ran the code-standards-enforcer checklist against this PR: PASS — no template function-call violations, no locally-declared interfaces.
Non-blocking: branch feat/mentorship-mentor-register-connect, all 14 commits, and the PR body carry no ticket reference (JIRA or GitHub issue), which this repo's commit-workflow.md asks for under Branch Naming / Ticket Tracking — worth adding before merge, not a reason to block.
Summary
Follow-up to #2307. On Become a Mentor, an unconnected GitHub or LinkedIn row is no longer a dead-end placeholder: it offers Connect, which opens the profile module's Add-identity dialog in place so the mentor can start linking without being sent to
/profile/identities.Linking is still an OAuth handshake (a full-page redirect). Opening the dialog keeps the mock registration form behind it; choosing a provider unloads the page. Become a Mentor still has no registration endpoint (submit only toasts coming-soon, program requests and the resume file are local), so this PR does not persist/restore that draft across Auth0 — that belongs with the real registration POST.
The dialog names the current page as
returnTo, the social-connect BFF allowlists that path, and the profile card reports the outcome when the mentor lands back on/mentorship/mentor— the Identities tab and profile shell never mount on that route, so without this they would finish Auth0 and be told nothing.What's in it
Profile card
AddAccountDialogComponent(GitHub/LinkedIn only) and refresh identities on close.null, not[], so Connect is not offered for accounts we could not see.?success=/?error=on init, toast the result (includingalready_linkedand Flow C codes the profile shell would otherwise own), then strip the params withhistory.replaceStateso a reload cannot replay the message.isPlatformBrowser): this page hydrates, and the app's only toast outlet lives in the root template, so an SSR pass would double-render the toast.Impersonation
onConnectalso refuses, because the connect route is behindblockDuringImpersonationinside the/apierror-handler mount and the dialog reaches it with a top-level navigation — a click would replace the page with the error JSON. The card is also showing the impersonated user's profile, while the link could only attach to the impersonator.Social-connect return path
window.location.pathnameasreturnTo./mentorship/mentorjoinsallowedProfileReturnPaths.Docs
docs/architecture/backend/impersonation.mdnow says social-linkreturnTois caller-supplied, and that a page added to the allowlist has to toastimpersonation_read_onlyitself.Not in this PR
Submit still only validates and toasts — there is still no mentor registration endpoint. Form draft persist/restore across the OAuth redirect waits for that endpoint. This does not add a new OAuth provider or change how Identities-tab connect works for
/profile/*.Test plan
/mentorship/mentorwith GitHub linked and LinkedIn missing, Connect opens the Add-identity dialog in place (form still visible behind it until a provider is chosen)./mentorship/mentor, shows a success toast, and the LinkedIn row becomes a profile link without a full reload of the linked account.already_linkedfailure toasts the conflict and points at Profile → Identities; the query params are gone from the URL./profile/identitiesstill returns to the Identities tab (defaultreturnTounchanged).returnTocannot aim the callback elsewhere.Unit coverage: profile-card (dialog, callback outcomes, impersonation, server-skip, destroy-with-dialog-open), mentor-register mock, and
ProfileControllersocial-connectreturnToallowlist / chaining specs.