Ops 4720/status change validation

[josbell]

What changed

When requesting a bli status change, now the validation of the agreement/blis will be postponed until user selects the blis that need to change. Furthermore, instead of validating all blis, only the selected blis will be validated.

Described what changes in this PR, at a high level

Previously the agreement/blis were validated using vest as a whole suite and done on page rendering. This change decouples the agreement and blis suites. They are not triggered not on page rendering but on bli selection.

Additionally there is also an unrelated fix to a warning emerging from Agreement table row where a div ended up being rendered as a direct child of a table when loading.

Issue

4720

Add link to issue here
#4720

How to test

1. Navigate to Agreement
2. Request a Status Change
3. No agreement or bli validation error messages should be rendered
4. Select the type of status change (draft to planned or planned to executing)
5. Only the blis with the correct status should be able to be selected
6. Select one or more blis
7. Only now should the agreement be validated and only the selected blis and error messages (if any) rendered

Write out steps for how someone could test this PR against the acceptance criteria

Screenshots

If relevant, e.g. for a front-end feature

Definition of Done Checklist

• OESA: Code refactored for clarity
• OESA: Dependency rules followed
• Automated unit tests updated and passed
• Automated integration tests updated and passed
• Automated quality tests updated and passed
• Automated load tests updated and passed
• Automated a11y tests updated and passed
• Automated security tests updated and passed
• 90%+ Code coverage achieved
• Form validations updated

Links

If relevant, e.g. for a link to a piece of markdown documentation

[rajohnson90]

What I've seen here all makes sense. Just respond to my one question and I'll be ready to approve.

[rajohnson90]

Given that Wei has dynamic fixes for the things I'm concerned with, I am fine with approving this name.

[Copilot]

Pull request overview

This PR decouples agreement and budget line item (BLI) validation, postponing validation until budget lines are selected for status changes. Previously, validation was triggered on page render for all items; now it only runs when BLIs are selected, validating only those selected items.

Key changes:

• Split validation logic into separate agreement and BLI suites with new exported utility functions
• Updated validation timing to occur on BLI selection rather than page load
• Fixed rendering issue in AgreementTableRow where loading state rendered a div as direct child of table

Reviewed changes

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

Show a summary per file

File	Description
frontend/src/pages/agreements/review/suite.js	Refactored validation suite into separate agreement and BLI validators; added validateBudgetLineItem and validateBudgetLineItems utility functions
frontend/src/pages/agreements/review/ReviewAgreement.hooks.js	Updated validation timing to trigger on BLI selection; refactored error aggregation logic; replaced res with agreementValidationResults
frontend/src/pages/agreements/review/ReviewAgreement.jsx	Updated to use agreementValidationResults prop; removed validation-based disabling of action options
frontend/src/components/BudgetLineItems/BLIReviewTable/BLIReviewRow.jsx	Modified date error classes to only apply when budget line is selected
frontend/src/components/Agreements/AgreementsTable/AgreementTableRow.jsx	Fixed loading state to render table row with td instead of bare div
frontend/src/components/Agreements/AgreementMetaAccordion/AgreementMetaAccordion.jsx	Updated prop name from res to agreementValidationResults
frontend/test_validation.js	Updated date and formatting (quote style changes)
frontend/cypress/e2e/createAgreementWithValidations.cy.js	Enhanced test robustness with unique suffixes, better error handling, and removed validation checks that no longer apply
frontend/cypress/e2e/agreementList.cy.js	Improved test stability with better selectors and retry logic

Comments suppressed due to low confidence (1)

frontend/src/components/BudgetLineItems/BLIReviewTable/BLIReviewRow.jsx:144

• Inconsistent error display logic: The dateNeeded field only shows errors when the budget line is selected (line 139), but the canNumber and amount fields (lines 142, 144) show errors based on isReviewMode regardless of selection status. This inconsistency may confuse users - either all fields should respect the selection status, or none should. Consider updating these lines to match the pattern: budgetLine.selected ? addErrorClassIfNotFound(...) : ''

        const canNumberClasses = `${addErrorClassIfNotFound(canNumber, isReviewMode)} ${borderExpandedStyles}`;
        const amount = budgetLine?.amount ?? 0;
        const amountClasses = `${addErrorClassIfNotFound(amount, isReviewMode)} ${borderExpandedStyles}`;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The newly exported functions validateBudgetLineItem and validateBudgetLineItems lack unit test coverage. Given that the repository has comprehensive test coverage for helper functions (as seen in the helpers directory), these validation utility functions should have corresponding unit tests to verify their behavior with valid and invalid budget line data, edge cases like null/undefined values, and array handling.

[josbell]

I added specs. Not sure why this comment is not being flagged as outdated. See suite.test.js

[fpigeonjr]

🤖 Claude's review

---

PR Review: #4809 - Status Change Validation Timing

Overview

This PR improves the user experience for budget line item status changes by deferring validation until BLIs are selected, rather than showing validation errors immediately on page load. All CI checks are passing ✅

---

Strengths

1. Better UX Pattern

The change from eager validation (on render) to lazy validation (on selection) is a solid UX improvement. Users aren't confronted with errors until they've actually indicated intent to make changes.

2. Performance Optimization

Only validating selected BLIs rather than all BLIs is more efficient, especially for agreements with many budget lines.

3. Cleaner Architecture

The separation of agreement and BLI validation suites (agreementSuite and budgetLineSuite) is a good refactor that improves maintainability.

4. Comprehensive Testing

The E2E test updates show attention to test stability, particularly around flakiness in CI environments (using GITHUB_RUN_ID, better selectors, proper waiting).

---

Issues & Concerns

1. Potential Validation Bypass ⚠️

Location: frontend/src/pages/agreements/review/ReviewAgreement.hooks.js:81-83

const agreementValidationResults = React.useMemo(() => {
    if (selectedBudgetLines.length === 0) {
        return null;  // ⚠️ Returns null, not a validation result
    }
    return agreementSuite.get();
}, [selectedBudgetLines.length]);

Issue: When agreementValidationResults is null, this line in ReviewAgreement.jsx:310 could fail:
{!isSubmissionReady || !agreementValidationResults.isValid() ? (

Risk: If agreementValidationResults is null, calling .isValid() will throw a TypeError. The conditional rendering pattern in line 76 uses optional chaining (agreementValidationResults ? classnames(...) : undefined), but line 310 doesn't.

Recommendation:
// Option 1: Return empty validation result instead of null
const agreementValidationResults = React.useMemo(() => {
    const result = agreementSuite.get();
    if (selectedBudgetLines.length === 0) {
        return { isValid: () => true, getErrors: () => ({}) };
    }
    return result;
}, [selectedBudgetLines.length]);

// Option 2: Add null check in ReviewAgreement.jsx:310
{!isSubmissionReady || !(agreementValidationResults?.isValid() ?? true) ? (

2. Inconsistent Validation State Management

Location: frontend/src/pages/agreements/review/ReviewAgreement.hooks.js:146-171

The useEffect that manages pageErrors has complex conditional logic:

React.useEffect(() => {
    if (!isSuccess || selectedBudgetLines.length === 0) {
        setPageErrors((prev) => {
            if (Object.keys(prev).length === 0) {
                return prev;  // Why this check?
            }
            return {};
        });
        // ...
    }
    // ...
}, [agreementValidationResults, isSuccess, agreement, hasBLIError, bliValidationResults, selectedBudgetLines]);

Issue: The early return with setPageErrors((prev) => prev ? ... : prev) is confusing. It seems to be optimizing to avoid unnecessary re-renders, but the logic is hard to follow.

Recommendation: Add a comment explaining the optimization, or simplify:
if (!isSuccess || selectedBudgetLines.length === 0) {
    // Clear errors when no BLIs selected to avoid showing stale validation
    setPageErrors({});
    setIsAlertActive(false);
    return;
}

3. Formatting Issue in BLIReviewRow 🐛

Location: frontend/src/components/BudgetLineItems/BLIReviewTable/BLIReviewRow.jsx:138-139

const dateErrorClasses = `${futureDateErrorClass(dateNeededErrorValue, isReviewMode)} ${addErrorClassIfNotFound(dateNeededErrorValue, isReviewMode)}`
const dateNeededClasses =  `${budgetLine.selected ? dateErrorClasses: ''} ${borderExpandedStyles}`;

Issues:
- Missing space after : in ternary operator
- Extra space before = assignment
- Missing semicolon on line 138

Recommendation: Auto-format with Prettier to catch these:
const dateErrorClasses = `${futureDateErrorClass(dateNeededErrorValue, isReviewMode)} ${addErrorClassIfNotFound(dateNeededErrorValue, isReviewMode)}`;
const dateNeededClasses = `${budgetLine.selected ? dateErrorClasses : ''} ${borderExpandedStyles}`;

4. E2E Test Resilience

Location: frontend/cypress/e2e/agreementList.cy.js:189-197

The new approach using data-testid and filtering is much better, but the retry logic could be more explicit:

cy.get("tbody tr[data-testid^='agreement-table-row-'] [data-cy='agreement-type']", { timeout: 30000 }).should(
    ($cells) => {
        const texts = [...$cells].map((cell) => cell.textContent.trim()).filter(Boolean);
        expect(texts.length).to.be.greaterThan(0);
        expect(texts.every((text) => text === "Contract")).to.eq(true);
    }
);

Recommendation: Consider adding a custom Cypress command for this pattern since it's repeated:
Cypress.Commands.add('verifyTableColumnValues', (selector, expectedValue) => {
    cy.get(selector, { timeout: 30000 }).should(($cells) => {
        const texts = [...$cells].map((cell) => cell.textContent.trim()).filter(Boolean);
        expect(texts.length).to.be.greaterThan(0);
        expect(texts.every((text) => text === expectedValue)).to.eq(true);
    });
});

5. Test Data Management Complexity

Location: frontend/cypress/e2e/createAgreementWithValidations.cy.js:5-7

const runId = Cypress.env("GITHUB_RUN_ID") || Cypress.env("CI_PIPELINE_ID") || "local";
const runAttempt = Cypress.env("GITHUB_RUN_ATTEMPT") || "1";
const buildUniqueSuffix = () => `${runId}-${runAttempt}-${Date.now()}-${Cypress._.random(0, 1_000_000_000)}`;

Observation: This is a good fix for test isolation, but it indicates potential database cleanup issues between test runs.

Recommendation: Consider adding a test cleanup hook or using transaction rollback strategies to avoid relying on unique names. This would make tests more predictable and faster.

---
Questions for Author

1. Validation on Submit: If a user never selects any BLIs but tries to submit, what happens? Is there validation to catch this edge case?
2. Error Message Persistence: When a user deselects all BLIs after seeing errors, the errors are cleared. Is this the intended UX, or should we show a message like "Please select at least one budget line to submit"?
3. Backwards Compatibility: Are there any existing workflows or bookmarked URLs that might break with this validation timing change?
4. Performance: For agreements with 100+ BLIs, have you tested the performance of the validation memoization? The dependencies array includes selectedBudgetLines.length but not the actual content - is this intentional?

---
Testing Recommendations

Unit Tests to Add:

1. Test agreementValidationResults when selectedBudgetLines.length === 0
2. Test bliValidationResults with partially valid selection
3. Test the error aggregation logic with both agreement and BLI errors

E2E Tests to Add:

1. Select BLIs → see errors → deselect BLIs → verify errors cleared
2. Select only invalid BLIs → verify only those errors show
3. Submit without selecting any BLIs → verify appropriate error

---
Minor Suggestions

Code Quality:

1. Consistent naming: agreementValidationResults vs bliValidationResults - consider budgetLineValidationResults for clarity
2. Magic numbers: Line 138 in BLIReviewRow could extract the conditional logic to a named function: shouldShowDateErrors(budgetLine, isReviewMode)
3. Comments: Add JSDoc comments to the new validateBudgetLineItems function in suite.js

Documentation:

Consider updating CLAUDE.md with this validation pattern since it's a significant architectural change that affects how forms work in the application.

---
Verdict

Recommendation: Approve with Minor Changes ✅

This is a well-thought-out improvement to the UX and architecture. The primary concern is the potential null reference issue with agreementValidationResults.isValid(). Once that's addressed (either via null check or returning a valid result object), this is ready to merge.

The test improvements alone make this PR valuable - the E2E test fixes will reduce CI flakiness significantly.

---
Suggested Changes

// frontend/src/pages/agreements/review/ReviewAgreement.hooks.js

// Fix 1: Ensure agreementValidationResults is never null
const agreementValidationResults = React.useMemo(() => {
    const result = agreementSuite.get();
    if (selectedBudgetLines.length === 0) {
        // Return a valid result object that passes validation when no BLIs selected
        return {
            isValid: () => true,
            getErrors: () => ({}),
            hasErrors: () => false
        };
    }
    return result;
}, [selectedBudgetLines.length]);

// Fix 2: Simplify the pageErrors useEffect early return
if (!isSuccess || selectedBudgetLines.length === 0) {
    setPageErrors({});
    setIsAlertActive(false);
    return;
}

// frontend/src/components/BudgetLineItems/BLIReviewTable/BLIReviewRow.jsx

// Fix 3: Format the date classes properly
const dateErrorClasses = `${futureDateErrorClass(dateNeededErrorValue, isReviewMode)} ${addErrorClassIfNotFound(dateNeededErrorValue, isReviewMode)}`;
const dateNeededClasses = `${budgetLine.selected ? dateErrorClasses : ''} ${borderExpandedStyles}`;

---
Great work on improving the validation UX and test stability! 🎉

[fpigeonjr]

found a bug with manually testing a minimal agreement with a minimal BLI and trying to send to review.

Screen.Recording.2026-01-08.at.10.25.11.AM.mov

[josbell]

found a bug with manually testing a minimal agreement with a minimal BLI and trying to send to review.

Screen.Recording.2026-01-08.at.10.25.11.AM.mov

Bug resolved

[josbell]

@fpigeonjr For Claude concerns

1. Null deref risk: not valid. In ReviewAgreement.jsx the render already guards agreementValidationResults with agreementValidationResults && agreementValidationResults.isValid(), so no TypeError when it’s null

2. Inconsistent validation state management: The early‑return block is an optimization to avoid unnecessary state updates. It’s not a bug. I added comment to clarify further

3. BLIReviewRow formatting issue: fixed

4. E2E / test‑data suggestions: Added command as suggested

[github-actions]

🎉 This PR is included in version 1.262.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀