Skip to content

Abstract UAT keywords feedback#814

Open
shinyichen wants to merge 1 commit intoadsabs:masterfrom
shinyichen:SCIX-531-abs-uat-keywords-feedback
Open

Abstract UAT keywords feedback#814
shinyichen wants to merge 1 commit intoadsabs:masterfrom
shinyichen:SCIX-531-abs-uat-keywords-feedback

Conversation

@shinyichen
Copy link
Member

@shinyichen shinyichen commented Feb 26, 2026

Add a feedback button link in abstract UAT keywords. When clicked, link to general feedback prefilled with subject and record ID

Screen.Recording.2026-02-27.at.2.25.46.PM.mov

@shinyichen
Copy link
Member Author

shinyichen commented Feb 26, 2026

@kelockhart Would you see if the video demo looks good before I include Tim for the code review?

@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.0%. Comparing base (ea6f244) to head (95df1c0).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #814     +/-   ##
========================================
- Coverage    62.0%   62.0%   -0.0%     
========================================
  Files         317     317             
  Lines       36536   36536             
  Branches     1642    1642             
========================================
- Hits        22621   22620      -1     
- Misses      13878   13879      +1     
  Partials       37      37             

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kelockhart
Copy link
Member

@shinyichen the functionality looks good! But I wonder if the little icon will be noticed. Can you try adding "feedback" or something next to the icon?

@shinyichen shinyichen force-pushed the SCIX-531-abs-uat-keywords-feedback branch from 0f546cd to 95df1c0 Compare February 27, 2026 22:24
@shinyichen
Copy link
Member Author

@kelockhart Please see updated screen capture.
Screenshot 2026-02-27 at 2 26 45 PM

@kelockhart
Copy link
Member

@shinyichen yes! Looks great, no more feedback on my end.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a “Feedback” entry point from the Abstract UAT Keywords section that routes users to the General Feedback form with a prefilled subject and record identifier.

Changes:

  • Add a “Feedback” button to the UAT Keywords block on abstract details and navigate to the general feedback route with query params.
  • Update the General Feedback page to prefill the comment body and email subject from router.query.
  • Expand feedback _subject typing to include the new UAT-specific subject.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/pages/feedback/general.tsx Reads subject/id from query to prefill comments and _subject during submission.
src/components/AbstractDetails/AbstractDetails.tsx Adds UAT “Feedback” button that routes to general feedback with record id + subject.
src/api/feedback/types.ts Updates _subject typing to allow the new UAT feedback subject (and currently any string).
Comments suppressed due to low confidence (2)

src/pages/feedback/general.tsx:125

  • _subject: !!subject ? subject : 'Nectar Feedback' can pass a string[] through to the API if the query contains repeated subject params. Normalize subject to a single string before using it, and consider whitelisting allowed subjects so this endpoint isn't accepting arbitrary query-controlled subject lines.
          {
            name,
            _replyto: email,
            _subject: !!subject ? subject : 'Nectar Feedback',
            'feedback-type': 'feedback',
            'user-agent-string': globalThis?.navigator?.userAgent,
            origin: 'bbb_feedback', // indicate general feedback
            'g-recaptcha-response': recaptchaToken,

src/pages/feedback/general.tsx:88

  • useForm({ defaultValues }) only applies defaultValues on the first mount. On client-side navigation, router.query may be empty until router.isReady is true, so the comments prefill can be skipped. Consider calling reset(...) in an effect when router.isReady and the normalized subject/id are available (or pass values to useForm if you intend it to be reactive).
  const initialFormValues: FormValues = {
    name: '',
    email: userEmail ?? '',
    comments: !!subject && subject === 'UAT keywords feedback' ? `UAT Keywords feedback on record: ${id}\n\n` : '',
  };

  const formMethods = useForm<FormValues>({
    defaultValues: initialFormValues,
    resolver: zodResolver(validationSchema),
    mode: 'onSubmit',
    reValidateMode: 'onBlur',
    shouldFocusError: true,
  });

Comment on lines +68 to +75
const router = useRouter();

const handleUATFeedback = () => {
void router.push({
pathname: feedbackItems.general.path,
query: { id: doc.bibcode ? doc.bibcode : doc.identifier?.[0], subject: 'UAT keywords feedback' },
});
};
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleUATFeedback is re-created on every render and passed into a memo(..., equals) component. Since equals compares function props by reference, this defeats memoization and can cause unnecessary rerenders of the (potentially large) UAT keyword list. Wrap handleUATFeedback in useCallback (with the needed deps) or pass a stable callback/URL into UATKeywords.

Copilot uses AI. Check for mistakes.
Comment on lines +11 to +12
| 'UAT keywords feedback'
| string;
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding | string to FeedbackBaseParams._subject makes the literal union effectively untyped and removes most compile-time protection against invalid subjects across the codebase. If you only need to support the new UAT flow, keep _subject as a strict union (add 'UAT keywords feedback' only) and validate/coerce the query param on the general feedback page before assigning it.

Suggested change
| 'UAT keywords feedback'
| string;
| 'UAT keywords feedback';

Copilot uses AI. Check for mistakes.
Comment on lines +74 to 80
const { subject, id } = router.query;

const initialFormValues: FormValues = {
name: '',
email: userEmail ?? '',
comments: '',
comments: !!subject && subject === 'UAT keywords feedback' ? `UAT Keywords feedback on record: ${id}\n\n` : '',
};
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

router.query values are string | string[] | undefined. Here subject === 'UAT keywords feedback' and template-interpolating ${id} will behave unexpectedly for string[] (and can produce undefined in the prefill). Normalize subject/id to strings (e.g., take first element) before comparing/using them, and avoid prefilling when id is missing.

Copilot uses AI. Check for mistakes.
Copy link
Member

@thostetler thostetler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes look generally pretty good. If we end up doing this sort of then again, we'll want to generalize it more so that we don't have the logic for this one case in the main email config.

I just have one small thing I mentioned

name,
_replyto: email,
_subject: 'Nectar Feedback',
_subject: !!subject ? subject : 'Nectar Feedback',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't allow the subject get passed in here directly.

Let's use an enum and force the subject to be uat-keywords that maps to "UAT Keywords..." or something like that, otherwise it defaults back to "Nectar Feedback"

Just to make sure that we can't have arbitrary strings getting into the subject directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants