I added some basic instructions on how to use ReportIncident locally (https://www.mediawiki.org/wiki/Extension:ReportIncident#Usage), you can also use it on a user talk page on beta wikis (e.g. https://en.wikipedia.beta.wmflabs.org/w/index.php?title=User_talk%3ABluedot) or via Patch Demo (enable ReportIncident when setting up the patch demo instance).

In T350260#9316524, @CCiufo-WMF wrote:

Thanks for filing @kostajh! Could you confirm the urgency of this review? DST is planning to take this on in our next sprint cycle (starting the week of Nov 18).

Lowest urgency, we'd just appreciate a cursory scan and review that we haven't missed anything obvious. We're doing a next round of work starting in January, and we aim to have in production sometime in March.

In T350260#9316789, @kostajh wrote:

Lowest urgency, we'd just appreciate a cursory scan and review that we haven't missed anything obvious. We're doing a next round of work starting in January, and we aim to have in production sometime in March.

Gotcha, we'll still aim for next sprint but keep you updated.

Overall, the ReportIncident code looks very good. I'm very pleased to see Codex used this way, including advanced usage in some places, like the useModelWrapper composable. The code mostly follows best practices for use of Vue, Codex and Pinia. There are only two things I found where the code didn't follow best practices (although these practices weren't well documented), explained in detail below, plus some minor things here and there.

Computed properties should not have side effects
At a high level, formErrorMessages in stores/Form.js follows best practices (a computed property that returns form validation messages). However, the function that computes its value has side effects, which is a bad practice. Specifically, it modifies other refs, like displayBehaviorsRequiredError, displayReportedUserRequiredError, etc. This makes the code difficult to reason about, because you have to think about how the first execution of the function changes the behavior of the second execution of the function. I recommend finding a different way to update these refs, like using a watcher. For example:

watch ( inputBehaviors, ( newBehaviors ) => {
    if  ( newBehaviors.length > 0 ) {
        displayBehaviorsRequiredError.value = true;
    }
} );

Alternatively, you may be able to simplify this logic substantially by only doing emptiness validation when the user attempts to submit the form. Overall, this question of how to do emptiness validation without it being too noisy is an interesting one that we haven't documented a good solution to, as far as I can tell. The documentation for Field validation alludes to the possibility of validating on blur, but doesn't show an example of how you would do that.

MW setup and DOM interaction code should live in init.js, not App.vue
In the setup function in App.vue, you have:

// Open the dialog if the link is clicked on.
$( '.ext-reportincident-link' ).on( 'click', reportLinkInToolsMenuHandler );

Ideally, code that interacts with the DOM outside of the component should not live inside the component, but should instead live in init.js. This should be relatively straightforward to change, since the App component already contains a nicely encapsulated method that you can call:

// In init.js, capture the return value of Vue.createMwApp():
const reportIncidentApp = Vue.createMwApp( App )
	.use( pinia )
	.mount( '#ext-reportincident-app' );

// Then set up the event listener here instead of App.vue:
$( '.ext-reportincident-link' ).on( 'click', ( event ) => {
	event.preventDefault();
	// And call the method exposed by App.vue:
	reportIncidentApp.reportLinkInToolsMenuHandler(); // no longer takes an event param, because preventDefault is handled here now
} );

You couldn't really have known this, because there's nothing in the Vue documentation (as far as I know) that explains that you can call methods of the root component on the app instance, and there's no documentation on our end recommending this practice (we should write some).

Similarly, I recommend moving the mw.hook setup step that points to discussionToolsOverflowMenuOnChooseHandler from App.vue to init.js as well.

Minor notes

• package.json:
  • npm seems unhappy with the version of Vue that is used, which is now out of date with what MW core uses (we upgraded a few weeks ago). I've submitted a patch to upgrade the Vue version in ReportIncident to match MW core.
• App.vue:
  • The other components are brought in with components: { 'email-alert-dialog': EmailAlertDialog, 'report-incident-dialog': ReportIncidentDialog, ...etc... }. But you don't need to provide a name for the component if it's just the kebab-case version of the PascalCase variable name. In this case, that means you can do components: { EmailAlertDialog, ReportIncidentDialog, ...etc... } and it'll still work.
  • There's an emailAlertOpen ref and an open ref. I suggest renaming open to something like reportIncidentOpen for clarity.
  • You can avoid having to disable the vue/no-unused-properties rule for the methods that are exposed for external use (currently for tests, in my recommendation also for init.js) by explicitly exposing them: expose: [ 'checkUsernameExists', ...etc... ]. (This also applies to similar code in ReportIncidentDialog.vue and ReportIncidentDialogStep2.vue
• ReportIncidentDialogStep2.vue:
  • The dynamic visibleItemLimit based on the window height is a clever way to prevent the menu from overflowing! As of Codex v1.1.1 (released on December 6th), Codex menus now do this automatically, so you should need this workaround anymore.

Internally within DST, we should think about:

• How do we document these best practices?
• How should people do Field validation on blur? Can we provide an example in the docs?

In T350260#9419877, @Catrope wrote:

• How should people do Field validation on blur? Can we provide an example in the docs?

I've filed T353846 for this.

In T350260#9419873, @Catrope wrote:

Overall, this question of how to do emptiness validation without it being too noisy is an interesting one that we haven't documented a good solution to, as far as I can tell. The documentation for Field validation alludes to the possibility of validating on blur, but doesn't show an example of how you would do that.

I was wrong, we do have an example of how to validate on blur, it's the "Complex field with two inputs" example on that same page. As part of my patch to T353846 I'm making this clearer by adding a link to that example to the sentence that talks about validating on blur.

In T350260#9419873, @Catrope wrote:

Thanks so much for these notes! We will come back to this when we pick up the next round of work for this project.

In T350260#9534562, @CCiufo-WMF wrote:

I had moved this to Following while we were waiting for someone from T&S Product to ack. I'm resolving it now because the work described in the task description is complete.

Sorry for the delay in acknowledging! I had been keeping it open until we worked on implementing the recommendations, but filed T357424: ReportIncident: Implement recommendations from DST review of codebase for doing that. Thanks again for the review and suggestions.