Conversation
…ry/secondary variants
WalkthroughLayout adjustments move DownloadMetadata to the top-left and adjust Claim spacing; Claim and ClaimStatus components switch to DataCiteButton, add contextual help and refine error handling. DataCiteButton gains a color prop and CSS-variable theming. New profile URL constants and color tokens added. Session exports a User type. Styles add .text-justify. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Page as DOI Content
participant Claim as Claim Component
participant API as Claims API
participant Cache as Client Cache
U->>Page: Open DOI page
Page->>Claim: mount (useGetClaimQuery)
Claim->>API: fetch claim status
API-->>Claim: claim status + messages
Claim->>Cache: addOrUpdateExistingClaim (store error if present)
Claim-->>U: render DataCiteButton / ClaimStatus + help text
U->>Claim: Click Add/Remove
Claim->>API: mutate claim
API-->>Claim: result (success/error)
Claim->>Cache: sync claim/error
Claim-->>U: update button/state/help
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–25 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
rationalize-claim-error-handling
|
| Run status |
|
| Run duration | 01m 32s |
| Commit |
|
| Committer | Bryceson Laing |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
3
|
Skipped
|
0
|
Passing
|
46
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
src/data/constants.ts (1)
50-51: Consider aligning color tokens with a shared theme or CSS variablesACCENT_GREEN and WARNING are fine, but consider exposing a Colors map or CSS variables (e.g., --color-accent-green, --color-warning) to align with the CSS-variable theming used by DataCiteButton.
src/components/Header/ServerButtons.tsx (1)
104-114: LGTM: Use centralized URLs for Settings and Sign InThis aligns with constants.ts and avoids per-callsite URL assembly. Consider adding PROFILES_SIGN_OUT_URL for consistency in a follow-up.
src/styles.css (2)
206-208: Use a fallback for margin-block-start if targeting older browsersmargin-block-start is fine; optionally add margin-top for broader compatibility.
-.form-inline .dropdown-menu { - margin-block-start: 3px; -} +.form-inline .dropdown-menu { + margin-block-start: 3px; + /* Fallback for older browsers */ + margin-top: 3px; +}
253-254: Remove empty rule blockThis no-op selector can be dropped to reduce noise.
-@media(max-width:1200px) { - .input-group input[name="query-facets"] {} -} +@media(max-width:1200px) { + /* intentionally left blank */ +}Or remove the entire media query if unused.
src/components/DataCiteButton/DataCiteButton.tsx (2)
17-20: Expose COLORS in constants or accept a default via CSS variablesCOLORS is fine here, but consider centralizing via theme constants (or CSS variables) to avoid drift across components.
27-33: WrapLink receives props with custom fieldsAfter destructuring, ensure WrapLink gets href if present; the refactor above preserves that. Alternatively, only pass href/downstream Link props explicitly.
src/components/Claim/Claim.tsx (1)
20-32: Consider extracting the support email to constants.The help content system is well-designed. However, the support email is hardcoded in the fallback message.
Consider adding to constants:
// In src/data/constants.ts +export const SUPPORT_EMAIL = 'support@datacite.org'Then update line 31:
-return HELP_CONTENT[message] || <>We encountered an error adding this work to your ORCID profile. Contact DataCite Support for help: <a href="mailto: support@datacite.org" target='_blank' rel='noreferrer'>support@datacite.org</a>. (Error code: {message})</> +return HELP_CONTENT[message] || <>We encountered an error adding this work to your ORCID profile. Contact DataCite Support for help: <a href={`mailto:${SUPPORT_EMAIL}`} target='_blank' rel='noreferrer'>{SUPPORT_EMAIL}</a>. (Error code: {message})</>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/app/doi.org/[...doi]/Content.tsx(1 hunks)src/components/Claim/Claim.module.scss(0 hunks)src/components/Claim/Claim.tsx(2 hunks)src/components/ClaimStatus/ClaimStatus.tsx(3 hunks)src/components/DataCiteButton/DataCiteButton.module.scss(1 hunks)src/components/DataCiteButton/DataCiteButton.tsx(1 hunks)src/components/DiscoverWorksAlert/DiscoverWorksAlert.tsx(2 hunks)src/components/Header/ServerButtons.tsx(3 hunks)src/data/constants.ts(2 hunks)src/styles.css(9 hunks)src/utils/session.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/components/Claim/Claim.module.scss
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/components/ClaimStatus/ClaimStatus.tsx (2)
src/data/constants.ts (1)
ACCENT_GREEN(50-50)src/components/DataCiteButton/DataCiteButton.tsx (1)
DataCiteButton(22-38)
src/app/doi.org/[...doi]/Content.tsx (2)
src/components/DownloadMetadata/DownloadMetadata.tsx (1)
DownloadMetadata(11-39)src/components/DownloadMetadata/ExportMetadata.tsx (1)
ExportMetadata(13-148)
src/components/Claim/Claim.tsx (5)
src/data/constants.ts (4)
PROFILES_SETTINGS_URL(34-34)PROFILES_SIGN_IN_URL(35-35)ACCENT_GREEN(50-50)WARNING(51-51)src/utils/session.ts (1)
User(5-8)src/data/types.ts (1)
Claim(146-153)src/data/queries/claimQuery.ts (1)
useGetClaimQuery(5-15)src/components/DataCiteButton/DataCiteButton.tsx (1)
DataCiteButton(22-38)
src/components/Header/ServerButtons.tsx (1)
src/data/constants.ts (2)
PROFILES_SETTINGS_URL(34-34)PROFILES_SIGN_IN_URL(35-35)
src/components/DiscoverWorksAlert/DiscoverWorksAlert.tsx (1)
src/data/constants.ts (1)
PROFILES_SETTINGS_URL(34-34)
🪛 Biome (2.1.2)
src/components/Claim/Claim.tsx
[error] 10-10: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (1)
🔇 Additional comments (20)
src/data/constants.ts (1)
34-35: Good: centralize profiles URLsUsing PROFILES_SETTINGS_URL and PROFILES_SIGN_IN_URL avoids string duplication and reduces drift across environments.
src/components/DiscoverWorksAlert/DiscoverWorksAlert.tsx (2)
10-10: Nice cleanup: import the specific settings URLSwitching to PROFILES_SETTINGS_URL improves readability and avoids recomputing it locally.
25-25: LGTM: constant for settings linkDirectly using PROFILES_SETTINGS_URL is the right approach.
src/components/Header/ServerButtons.tsx (1)
14-14: Consolidated constants import looks goodImporting PROFILES_SETTINGS_URL and PROFILES_SIGN_IN_URL reduces manual string construction.
src/styles.css (7)
316-357: Badge color tweaks look consistentThe unified selectors and clear borders/readable colors are good. No issues spotted.
573-573: Mobile font-size tweak for cookie contentThe 12px adjustment improves small-screen readability and layout. Looks good.
626-632: Bootstrap tooltip arrow adjustmentsSelector consolidation and arrow styling are fine; no regressions anticipated.
639-646: Nav tabs member tweaksThese overrides are minimal and scoped. LGTM.
653-656: work-identifiers link colorExplicit link coloring is fine and matches the palette.
657-660: download-list heading spacingHeavier font-weight and tighter spacing read well.
680-682: Utility class .text-justifyGood addition for long guidance text. No conflicts detected.
src/components/DataCiteButton/DataCiteButton.module.scss (1)
33-37: Responsive width for buttonsKeeping full-width buttons on small screens is a good UX choice.
src/app/doi.org/[...doi]/Content.tsx (2)
55-56: LGTM: Move DownloadMetadata above ClaimSurfacing Download first makes sense and the modal wiring remains unchanged.
58-59: Claim spacing tweakmt-3 mb-3 reads better visually against the new layout. Looks good.
src/components/ClaimStatus/ClaimStatus.tsx (2)
8-9: LGTM! Clean migration to DataCiteButton.The imports properly support the transition from Bootstrap Button to the custom DataCiteButton component with centralized color constants.
45-53: LGTM! Well-structured DataCiteButton implementation.The DataCiteButton properly displays claim status with appropriate icon, dynamic color based on state, and disabled styling to indicate non-interactive status display.
src/components/Claim/Claim.tsx (4)
38-42: LGTM! Good error state management.The claimError state properly captures and preserves error messages from claim mutations, improving the user experience by maintaining error visibility.
127-136: LGTM! Clear loading state indication.The loading state properly uses DataCiteButton with appropriate disabled state and clear messaging.
147-169: LGTM! Well-designed action buttons.The action buttons properly differentiate between add (green) and remove (orange) operations, with appropriate access control via the disabled state.
173-175: LGTM! Contextual help messaging.The help text provides appropriate contextual guidance based on user authentication state and error conditions, improving the overall user experience.
| import { session, User } from 'src/utils/session' | ||
| import { Claim as ClaimType } from 'src/data/types' | ||
| import Error from 'src/components/Error/Error' | ||
| import ClaimStatus from 'src/components/ClaimStatus/ClaimStatus' | ||
| import styles from './Claim.module.scss' | ||
| import { useGetClaimQuery, GET_CLAIM_GQL, CREATE_CLAIM_GQL, DELETE_CLAIM_GQL, QueryData, QueryVar } from 'src/data/queries/claimQuery'; | ||
| import { ACCENT_GREEN, PROFILES_SETTINGS_URL, PROFILES_SIGN_IN_URL, WARNING } from 'src/data/constants'; | ||
| import DataCiteButton from '../DataCiteButton/DataCiteButton'; |
There was a problem hiding this comment.
Avoid shadowing the global Error constructor.
The import of the Error component on line 10 shadows JavaScript's global Error constructor, which could lead to confusion.
Consider using an alias:
-import Error from 'src/components/Error/Error'
+import ErrorComponent from 'src/components/Error/Error'Then update line 140 accordingly:
-return <Error title="Error fetching claim." message={error.message} />
+return <ErrorComponent title="Error fetching claim." message={error.message} />📝 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.
| import { session, User } from 'src/utils/session' | |
| import { Claim as ClaimType } from 'src/data/types' | |
| import Error from 'src/components/Error/Error' | |
| import ClaimStatus from 'src/components/ClaimStatus/ClaimStatus' | |
| import styles from './Claim.module.scss' | |
| import { useGetClaimQuery, GET_CLAIM_GQL, CREATE_CLAIM_GQL, DELETE_CLAIM_GQL, QueryData, QueryVar } from 'src/data/queries/claimQuery'; | |
| import { ACCENT_GREEN, PROFILES_SETTINGS_URL, PROFILES_SIGN_IN_URL, WARNING } from 'src/data/constants'; | |
| import DataCiteButton from '../DataCiteButton/DataCiteButton'; | |
| // src/components/Claim/Claim.tsx | |
| import { session, User } from 'src/utils/session' | |
| import { Claim as ClaimType } from 'src/data/types' | |
| -import Error from 'src/components/Error/Error' | |
| +import ErrorComponent from 'src/components/Error/Error' | |
| import ClaimStatus from 'src/components/ClaimStatus/ClaimStatus' | |
| import { useGetClaimQuery, GET_CLAIM_GQL, CREATE_CLAIM_GQL, DELETE_CLAIM_GQL, QueryData, QueryVar } from 'src/data/queries/claimQuery'; | |
| import { ACCENT_GREEN, PROFILES_SETTINGS_URL, PROFILES_SIGN_IN_URL, WARNING } from 'src/data/constants'; | |
| import DataCiteButton from '../DataCiteButton/DataCiteButton'; | |
| // … later in the file, where you render errors … | |
| -return <Error title="Error fetching claim." message={error.message} /> | |
| +return <ErrorComponent title="Error fetching claim." message={error.message} /> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 10-10: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In src/components/Claim/Claim.tsx around lines 8 to 14, the import "import Error
from 'src/components/Error/Error'" shadows the global Error constructor; change
the import to use an alias (for example "ErrorComponent" or "ClaimError") and
then update the component usage at line 140 to use that new alias; also search
and replace any other references in this file to the old name to the new alias
to avoid further shadowing.
| const stateColors = { | ||
| done: 'success', | ||
| failed: 'danger', | ||
| working: 'info', | ||
| waiting: 'info', | ||
| working: ACCENT_GREEN, | ||
| waiting: ACCENT_GREEN, | ||
| ready: 'default', | ||
| deleted: 'info' | ||
| deleted: ACCENT_GREEN | ||
| } |
There was a problem hiding this comment.
Type inconsistency in stateColors mapping.
The stateColors object now mixes Bootstrap color names ('success', 'danger', 'default') with hex color values (ACCENT_GREEN = '#1ABC9C'). This works for DataCiteButton which accepts color strings, but could cause issues with the Badge component on line 55 which expects Bootstrap color names for its bg prop.
Consider maintaining consistency:
const stateColors = {
done: 'success',
failed: 'danger',
- working: ACCENT_GREEN,
- waiting: ACCENT_GREEN,
+ working: 'info',
+ waiting: 'info',
ready: 'default',
- deleted: ACCENT_GREEN
+ deleted: 'info'
}
+
+const buttonColors = {
+ done: 'success',
+ failed: 'danger',
+ working: ACCENT_GREEN,
+ waiting: ACCENT_GREEN,
+ ready: 'default',
+ deleted: ACCENT_GREEN
+}Then use buttonColors[claim.state] for DataCiteButton and keep stateColors[claim.state] for Badge.
📝 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.
| const stateColors = { | |
| done: 'success', | |
| failed: 'danger', | |
| working: 'info', | |
| waiting: 'info', | |
| working: ACCENT_GREEN, | |
| waiting: ACCENT_GREEN, | |
| ready: 'default', | |
| deleted: 'info' | |
| deleted: ACCENT_GREEN | |
| } | |
| const stateColors = { | |
| done: 'success', | |
| failed: 'danger', | |
| working: 'info', | |
| waiting: 'info', | |
| ready: 'default', | |
| deleted: 'info' | |
| } | |
| const buttonColors = { | |
| done: 'success', | |
| failed: 'danger', | |
| working: ACCENT_GREEN, | |
| waiting: ACCENT_GREEN, | |
| ready: 'default', | |
| deleted: ACCENT_GREEN | |
| } |
🤖 Prompt for AI Agents
In src/components/ClaimStatus/ClaimStatus.tsx around lines 18 to 25, the
stateColors map mixes Bootstrap color names and hex ACCENT_GREEN which causes
type mismatch for Badge.bg (expects Bootstrap names) while DataCiteButton
accepts arbitrary color strings; split the mapping into two consistent objects
(e.g., stateColors with only Bootstrap color keys for Badge and buttonColors
with hex/ACCENT_GREEN for DataCiteButton), update the component to use
stateColors[claim.state] for the Badge bg prop and buttonColors[claim.state] for
the DataCiteButton color prop, and add a safe fallback value for unknown states
in both maps.
| .button { | ||
| color: $color !important; | ||
| font-size: 1em !important; | ||
| padding: .5em 2em !important; | ||
| border-width: 1.5px !important; | ||
| border-style: solid !important; | ||
| border-radius: 100px !important; | ||
| } | ||
| $text-color: white; | ||
| $color: var(--button-color, white); | ||
|
|
||
| .primary { | ||
| background-color: $primary-bg-color !important; | ||
| border-color: $primary-bg-color !important; | ||
| } | ||
| color: $text-color !important; | ||
| background-color: $color !important; | ||
| border-color: $color !important; | ||
|
|
||
| .secondary { | ||
| background-color: $secondary-bg-color !important; | ||
| border-color: $secondary-bg-color !important; | ||
| } | ||
|
|
||
| .outline { | ||
| color: $primary-bg-color !important; | ||
| background-color: transparent !important; | ||
| } | ||
| font-size: 1em !important; | ||
| padding: .5em 2em !important; | ||
|
|
||
| .secondary.outline { | ||
| color: $secondary-bg-color !important; | ||
| } | ||
| border-width: 1.5px !important; | ||
| border-style: solid !important; | ||
| border-radius: 100px !important; | ||
|
|
||
| .button:hover { | ||
| &.outline { | ||
| color: $color !important; | ||
| background-color: $hover-bg-color !important; | ||
| border-color: $hover-bg-color !important; | ||
| background-color: transparent !important; | ||
| } | ||
|
|
||
| &:hover { | ||
| color: $text-color !important; | ||
| background-color: $color !important; | ||
| border-color: $color !important; | ||
| filter: brightness(0.9) !important; | ||
| } | ||
|
|
||
| &.outline:hover { | ||
| filter: unset !important; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Outline hover currently fills with color; likely unintended
&:hover applies to all buttons and sets background to $color; &.outline:hover only unsets filter, so outline buttons fill on hover. If you want outline to remain transparent on hover, gate the fill to non-outline.
- &:hover {
+ &:not(.outline):hover {
color: $text-color !important;
background-color: $color !important;
border-color: $color !important;
filter: brightness(0.9) !important;
}
- &.outline:hover {
- filter: unset !important;
- }
+ &.outline:hover {
+ color: $color !important;
+ background-color: transparent !important;
+ border-color: $color !important;
+ filter: unset !important;
+ }Also, consider moving $text-color and $color to the module scope if you plan to reuse them in other variants.
Restrict hover styles to non-outline buttons and refine outline hover behavior
To keep the outline variant transparent on hover, update your selectors as follows:
- Change the general hover rule to only apply to non-outline buttons (
&:not(.outline):hover). - In the outline hover rule, explicitly reset filter and preserve transparency.
- Optionally, promote
$text-colorand$colorto module scope if you’ll reuse them elsewhere.
.button {
// …
- &:hover {
+ &:not(.outline):hover {
color: $text-color !important;
background-color: $color !important;
border-color: $color !important;
filter: brightness(0.9) !important;
}
- &.outline:hover {
- filter: unset !important;
- }
+ &.outline:hover {
+ color: $color !important;
+ background-color: transparent !important;
+ border-color: $color !important;
+ filter: unset !important;
+ }
}📝 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.
| .button { | |
| color: $color !important; | |
| font-size: 1em !important; | |
| padding: .5em 2em !important; | |
| border-width: 1.5px !important; | |
| border-style: solid !important; | |
| border-radius: 100px !important; | |
| } | |
| $text-color: white; | |
| $color: var(--button-color, white); | |
| .primary { | |
| background-color: $primary-bg-color !important; | |
| border-color: $primary-bg-color !important; | |
| } | |
| color: $text-color !important; | |
| background-color: $color !important; | |
| border-color: $color !important; | |
| .secondary { | |
| background-color: $secondary-bg-color !important; | |
| border-color: $secondary-bg-color !important; | |
| } | |
| .outline { | |
| color: $primary-bg-color !important; | |
| background-color: transparent !important; | |
| } | |
| font-size: 1em !important; | |
| padding: .5em 2em !important; | |
| .secondary.outline { | |
| color: $secondary-bg-color !important; | |
| } | |
| border-width: 1.5px !important; | |
| border-style: solid !important; | |
| border-radius: 100px !important; | |
| .button:hover { | |
| &.outline { | |
| color: $color !important; | |
| background-color: $hover-bg-color !important; | |
| border-color: $hover-bg-color !important; | |
| background-color: transparent !important; | |
| } | |
| &:hover { | |
| color: $text-color !important; | |
| background-color: $color !important; | |
| border-color: $color !important; | |
| filter: brightness(0.9) !important; | |
| } | |
| &.outline:hover { | |
| filter: unset !important; | |
| } | |
| } | |
| .button { | |
| $text-color: white; | |
| $color: var(--button-color, white); | |
| color: $text-color !important; | |
| background-color: $color !important; | |
| border-color: $color !important; | |
| font-size: 1em !important; | |
| padding: .5em 2em !important; | |
| border-width: 1.5px !important; | |
| border-style: solid !important; | |
| border-radius: 100px !important; | |
| &.outline { | |
| color: $color !important; | |
| background-color: transparent !important; | |
| } | |
| &:not(.outline):hover { | |
| color: $text-color !important; | |
| background-color: $color !important; | |
| border-color: $color !important; | |
| filter: brightness(0.9) !important; | |
| } | |
| &.outline:hover { | |
| color: $color !important; | |
| background-color: transparent !important; | |
| border-color: $color !important; | |
| filter: unset !important; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/components/DataCiteButton/DataCiteButton.module.scss around lines 1 to
31, the hover rule currently applies to all buttons including the .outline
variant which unintentionally applies a background and filter; change the
general hover selector to target non-outline buttons only (use
&:not(.outline):hover) and update the outline hover selector to explicitly reset
filter and keep background transparent so outline stays transparent on hover;
optionally move $text-color and $color to the top-level of the module if they
will be reused elsewhere.
| const color = props.color || (props.variant === 'secondary' ? COLORS.secondary : COLORS.primary) | ||
| // const variantStyle = props.variant === 'secondary' ? styles.secondary : styles.primary | ||
| const outlineStyle = props.outline ? styles.outline : '' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t forward custom props to DOM and merge style objects
- color is not a valid DOM attribute; avoid spreading it to Button.
- You overwrite consumer-provided style with your CSS variable; merge styles instead.
- For backward compatibility, treat variant values starting with "outline" as outline.
-export default function DataCiteButton(props: Props) {
- const color = props.color || (props.variant === 'secondary' ? COLORS.secondary : COLORS.primary)
- // const variantStyle = props.variant === 'secondary' ? styles.secondary : styles.primary
- const outlineStyle = props.outline ? styles.outline : ''
+export default function DataCiteButton(allProps: Props) {
+ // Avoid forwarding custom props to DOM
+ const { color: customColor, outline, className, style, variant, ...rest } = allProps
+ const color = customColor || (variant === 'secondary' ? COLORS.secondary : COLORS.primary)
+ const isOutline = outline || (typeof variant === 'string' && variant.startsWith('outline'))
+ const outlineStyle = isOutline ? styles.outline : ''
@@
- return <WrapLink {...props}>
- <Button
- {...props}
- className={`${props.className ?? ''} ${styles.button} ${outlineStyle}`}
- // @ts-expect-error CSS variable not in type definition
- style={{ '--button-color': color }}
- >
- {props.icon && <FontAwesomeIcon icon={props.icon} className="me-2" />}
- {props.children}
+ return <WrapLink {...(rest as Props)}>
+ <Button
+ {...rest}
+ variant={variant}
+ className={`${className ?? ''} ${styles.button} ${outlineStyle}`}
+ style={{
+ ...(style ?? {}),
+ // @ts-expect-error CSS variable not in type definition
+ ['--button-color']: color
+ }}
+ >
+ {allProps.icon && <FontAwesomeIcon icon={allProps.icon} className="me-2" />}
+ {allProps.children}
</Button>
</WrapLink>🤖 Prompt for AI Agents
In src/components/DataCiteButton/DataCiteButton.tsx around lines 23-25, avoid
forwarding the custom props and merge style objects: destructure props to pull
out color, variant, outline and style (e.g. const { color, variant, outline,
style, ...rest } = props) so color and outline are not spread to the DOM;
compute isOutline = outline || (variant &&
variant.toString().startsWith('outline')) and derive variantStyle based on
variant (use secondary vs primary) without overwriting consumer style; build a
mergedStyle that composes the consumer style with your CSS variable (e.g.
mergedStyle = { ...style, ...(color ? { ['--btn-color']: color } : {}) ,
...variantStyle, ...(isOutline ? styles.outline : {}) }) and pass mergedStyle
and rest to Button.
| const hrefWorksById = `/doi.org?query=creators_and_contributors.nameIdentifiers.nameIdentifier:(${user.uid} OR "https://orcid.org/${user.uid}")®istration-agency=datacite` | ||
| const hrefWorksByName = `/doi.org?query=${user.name}®istration-agency=datacite` | ||
| const hrefAccountSettings = `${PROFILES_URL}/settings/me` | ||
| const hrefAccountSettings = PROFILES_SETTINGS_URL | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encode query parameters to avoid breaking characters
user.uid and especially user.name may include spaces or special characters; encode them to keep links valid.
- const hrefWorksById = `/doi.org?query=creators_and_contributors.nameIdentifiers.nameIdentifier:(${user.uid} OR "https://orcid.org/${user.uid}")®istration-agency=datacite`
- const hrefWorksByName = `/doi.org?query=${user.name}®istration-agency=datacite`
+ const uid = encodeURIComponent(user.uid)
+ const name = encodeURIComponent(user.name)
+ const hrefWorksById = `/doi.org?query=creators_and_contributors.nameIdentifiers.nameIdentifier:(${uid}%20OR%20%22https://orcid.org/${uid}%22)®istration-agency=datacite`
+ const hrefWorksByName = `/doi.org?query=${name}®istration-agency=datacite`📝 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.
| const hrefWorksById = `/doi.org?query=creators_and_contributors.nameIdentifiers.nameIdentifier:(${user.uid} OR "https://orcid.org/${user.uid}")®istration-agency=datacite` | |
| const hrefWorksByName = `/doi.org?query=${user.name}®istration-agency=datacite` | |
| const hrefAccountSettings = `${PROFILES_URL}/settings/me` | |
| const hrefAccountSettings = PROFILES_SETTINGS_URL | |
| const uid = encodeURIComponent(user.uid) | |
| const name = encodeURIComponent(user.name) | |
| const hrefWorksById = `/doi.org?query=creators_and_contributors.nameIdentifiers.nameIdentifier:(${uid}%20OR%20%22https://orcid.org/${uid}%22)®istration-agency=datacite` | |
| const hrefWorksByName = `/doi.org?query=${name}®istration-agency=datacite` | |
| const hrefAccountSettings = PROFILES_SETTINGS_URL |
🤖 Prompt for AI Agents
In src/components/DiscoverWorksAlert/DiscoverWorksAlert.tsx around lines 23 to
26, the hrefs interpolate user.uid and user.name directly which can break URLs
when those values contain spaces or special characters; fix by URL-encoding the
dynamic parts—use encodeURIComponent for user.uid and for the two OR variants
(or encode the whole query string value) and use encodeURIComponent(user.name)
for the name-based query, then insert those encoded values into the template
literals so parentheses/quotes and other special chars are properly escaped in
the final hrefs.
| export type User = { | ||
| uid: string, | ||
| name: string | ||
| } | null |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don't conflate User with null in the type alias
Defining User to include null makes downstream types noisier. Prefer a non-nullable User and a separate Session type for clarity.
-export type User = {
- uid: string,
- name: string
-} | null
+export type User = {
+ uid: string
+ name: string
+}
+
+export type Session = User | null📝 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.
| export type User = { | |
| uid: string, | |
| name: string | |
| } | null | |
| export type User = { | |
| uid: string | |
| name: string | |
| } | |
| export type Session = User | null |
🤖 Prompt for AI Agents
In src/utils/session.ts around lines 5 to 8, the User type currently includes
null which conflates a user object with absence; change User to be a
non-nullable object type (uid: string, name: string) and add a separate exported
Session type (e.g., type Session = User | null) to represent presence/absence of
a signed-in user; update any exports accordingly so downstream code uses Session
when nullability is needed and User when a real user object is required.
| JsonWebToken.verify(token, JWT_KEY, { algorithms: ['RS256'] }, setUser) | ||
|
|
||
| return user as { uid: string, name: string } | ||
| return user as User |
There was a problem hiding this comment.
🛠️ Refactor suggestion
session() returns before JWT verify callback — user stays null
JsonWebToken.verify with a callback is async; you return before user is set. The cast masks the issue. Use the sync form (no callback) with try/catch, and give session() an explicit return type.
-export const session = () => {
+export const session = (): Session => {
@@
- let user: any = null
- function setUser(error: any, payload: any) {
- if (error) {
- console.log('JWT verification error: ' + error.message)
- return
- }
-
- user = payload
- }
-
- // verify asymmetric token, using RSA with SHA-256 hash algorithm
- JsonWebToken.verify(token, JWT_KEY, { algorithms: ['RS256'] }, setUser)
-
- return user as User
+ try {
+ // verify asymmetric token, using RSA with SHA-256 hash algorithm
+ const payload = JsonWebToken.verify(token, JWT_KEY, { algorithms: ['RS256'] }) as {
+ uid?: string
+ name?: string
+ }
+ if (!payload?.uid || !payload?.name) return null
+ return { uid: payload.uid, name: payload.name }
+ } catch (e: any) {
+ console.warn('JWT verification error:', e?.message ?? e)
+ return null
+ }📝 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.
| return user as User | |
| export const session = (): Session => { | |
| try { | |
| // verify asymmetric token, using RSA with SHA-256 hash algorithm | |
| const payload = JsonWebToken.verify(token, JWT_KEY, { algorithms: ['RS256'] }) as { | |
| uid?: string | |
| name?: string | |
| } | |
| if (!payload?.uid || !payload?.name) return null | |
| return { uid: payload.uid, name: payload.name } | |
| } catch (e: any) { | |
| console.warn('JWT verification error:', e?.message ?? e) | |
| return null | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/utils/session.ts around line 31, session() currently uses
JsonWebToken.verify with a callback and returns user as User before the callback
sets it; change to the synchronous form jwt.verify(token, secret) inside a
try/catch, parse the returned payload to a User, and return that value directly
(remove the async callback and the unsafe cast). Also add an explicit function
return type (e.g., Promise<User | null> or User | null depending on sync/async
design) so callers know when null is returned.
There was a problem hiding this comment.
@bklaing2 is this something we should look at? I'm not worried about the other coderabbitai suggestions.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@wendelfabianchinsamy I only added some simple typing to this file, I didn't touch the logic. I don't think this is something we need to worry about (at the moment) as it's been working just fine so far.
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
master
|
| Run status |
|
| Run duration | 01m 31s |
| Commit |
|
| Committer | Bryceson Laing |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
3
|
|
Skipped
|
0
|
|
Passing
|
46
|
| View all changes introduced in this branch ↗︎ | |
3 participants
Purpose
Update the UX for the claim button on work pages
closes: https://github.com/datacite/product-backlog/issues/268
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit
New Features
Improvements
Style