JMCP: Papers - jmcp/orcid-linking-for-research-identity-nRq9P4wE#1
Closed
landigf wants to merge 1 commit into
Closed
JMCP: Papers - jmcp/orcid-linking-for-research-identity-nRq9P4wE#1landigf wants to merge 1 commit into
landigf wants to merge 1 commit into
Conversation
This was referenced Mar 24, 2026
Owner
Author
|
Closing: will integrate valuable features directly on main to avoid cascade merge conflicts. |
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
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.
Review: ORCID Linking for Research Identity
Change summary
Four files touched (+162 / −3), adding link/unlink ORCID to the demo repository flow:
packages/contracts/src/index.tsORCID_REGEX,linkOrcidInputSchema(Zod),LinkOrcidInputtypepackages/db/src/index.tslinkOrcid/unlinkOrcidonPapersRepositoryinterface +DemoRepositoryimplementation; validates input, checks uniqueness, mutates demo stateapps/web/app/actions.tslinkOrcidAction/unlinkOrcidActionserver actions; revalidate relevant paths, redirectapps/web/app/settings/identity/page.tsxpatternvalidationValidation confidence: Medium-High
patternattribute give double validation on format.Risks still remaining
No server-side validation in
linkOrcidAction— the action callsrepository.linkOrcidwhich runslinkOrcidInputSchema.parse, so Zod will throw on bad input. But the error is unhandled — a malformed ORCID will produce an unformatted server error instead of a user-friendly message. Consider wrapping with try/catch or using.safeParse+ returning an error state.unlinkOrcidActionhas no CSRF / confirmation — a single<form>submit unlinks with no confirmation dialog. Low severity for demo mode, but worth a UX note before this goes live.isVerifiedResearcherset totrueon demo self-link — in demo mode anyone can type any ORCID and become "verified." This is fine for the demo, but the flag name implies real verification. Add a comment or rename to avoid confusion when the OAuth path lands.ORCID checksum not validated — the regex validates the format (
NNNN-NNNN-NNNN-NNNX) but not the ISO 7064 Mod 11-2 check digit. A typo like0000-0001-2345-6780would pass. Low priority but worth noting.Revalidation blast radius — both actions revalidate
/,/feed, profile, and two settings paths. This is broad; if caching becomes important, trim to only paths that actually render the ORCID.No integration with the OAuth path yet — the UI shows the "Sign in with ORCID" link when
session.orcid.configuredis true, but there's no corresponding server route or callback handler. The link will 404. Either gate it behind a "coming soon" label or stub the route.Verdict
Clean, well-layered change for demo-mode ORCID linking. The main gap is error handling in the server action (#1) — that's worth fixing before merge. The rest are follow-up items.