Skip to content

Conversation

@jasukej
Copy link
Contributor

@jasukej jasukej commented May 18, 2025

PR Type

Enhancement


Description

  • Introduce regex-based pattern validation for strings

  • Extend StringValidation with optional pattern field

  • Add new CONFIRMED_RSVP application status

  • Normalize enum string values to lowercase variants


Changes walkthrough 📝

Relevant files
Enhancement
application_controller.ts
Add pattern validation to strings                                               

functions/src/controllers/application_controller.ts

  • Introduced regex-based pattern validation
  • Added error push on pattern mismatch
  • +12/-2   
    application_types.ts
    Enhance application type definitions                                         

    functions/src/types/application_types.ts

  • Added CONFIRMED_RSVP status
  • Lowercased APPLICATION_STATES values
  • Extended StringValidation with pattern
  • Made Question.id required and added placeholder
  • +9/-7     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @jasukej jasukej merged commit f514bee into main May 18, 2025
    1 check passed
    @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Regex instantiation

    The new pattern validation creates a new RegExp on every call and does not handle invalid patterns. Consider validating or compiling regexes once and adding error handling for malformed patterns.

    if (validation.pattern) {
      const regex = new RegExp(validation.pattern);
      if (!regex.test(fieldValue)) {
        errors.push({
          field_id: `${question.id}`,
          message: `Not a valid ${question.text}.`,
        });
      }
    Empty input handling

    Pattern validation runs even when fieldValue is empty or null. You may want to skip regex checks when the value is not provided or when required is false.

    // validate pattern
    if (validation.pattern) {
      const regex = new RegExp(validation.pattern);
      if (!regex.test(fieldValue)) {
        errors.push({
          field_id: `${question.id}`,
          message: `Not a valid ${question.text}.`,
        });
      }
    }
    Interface breaking change

    Question.id was changed from optional to required and state/placeholder were added. Ensure all existing consumers provide these fields to avoid compilation or runtime errors.

    id: string;
    order: number;
    text: string;
    type: QUESTION_TYPE;
    validation: ValidationTypeMap[Question["type"]];
    placeholder?: string;
    options?: string[]; // for dropdown only
    state: APPLICATION_STATES;

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Catch invalid regex patterns

    Wrap the regex construction and test inside a try-catch to prevent invalid patterns
    from throwing runtime errors and crashing the validation.

    functions/src/controllers/application_controller.ts [467-469]

     if (validation.pattern) {
    -  const regex = new RegExp(validation.pattern);
    -  if (!regex.test(fieldValue)) {
    +  try {
    +    const regex = new RegExp(validation.pattern);
    +    if (!regex.test(fieldValue)) {
    +      errors.push({
    +        field_id: `${question.id}`,
    +        message: `Not a valid ${question.text}.`,
    +      });
    +    }
    +  } catch {
    +    // invalid pattern, skip this check
    +  }
    +}
    Suggestion importance[1-10]: 8

    __

    Why: Wrapping the regex construction and test in a try-catch prevents invalid patterns from throwing errors at runtime and crashing the validation logic.

    Medium
    Guard regex test by type

    Ensure fieldValue is actually a string before running the regex test to avoid type
    errors on null or non-string inputs.

    functions/src/controllers/application_controller.ts [469]

    -if (!regex.test(fieldValue)) {
    +if (typeof fieldValue === 'string' && !regex.test(fieldValue)) {
    Suggestion importance[1-10]: 6

    __

    Why: Adding a typeof check ensures fieldValue is a string before running regex.test, avoiding potential type errors on null or non-string inputs.

    Low
    General
    Anchor regex to full string

    Anchor the pattern with ^ and $ to enforce a full-string match rather than a
    substring match.

    functions/src/controllers/application_controller.ts [468]

    -const regex = new RegExp(validation.pattern);
    +const regex = new RegExp(`^${validation.pattern}$`);
    Suggestion importance[1-10]: 5

    __

    Why: Anchoring the pattern with ^ and $ enforces a full-string match instead of a substring match, improving the accuracy of the validation.

    Low

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

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants