ronantakizawa/codereview-bench huggingface dataset:
Repository: novuhq/novu
File: apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.ts (TypeScript)

Code before:

    if (actor) {
      createNotificationJobsCommand.actor = actor;
    }

    const notificationJobs = await this.createNotificationJobs.execute(
      CreateNotificationJobsCommand.create(createNotificationJobsCommand)
    );

    await this.storeSubscriberJobs.execute(
      StoreSubscriberJobsCommand.create({
        environmentId: command.environmentId,
        jobs: notificationJobs,
        organizationId: command.organizationId,
      })
    );
  }

  private async mapBridgeWorkflow(command: SubscriberJobBoundCommand): Promise<NotificationTemplateEntity | null> {
    const bridgeWorkflow = command.bridge?.workflow;

    if (!bridgeWorkflow) {
      return null;
    }

    const storedWorkflowId = (
      await this.notificationTemplateRepository.findByTriggerIdentifier(
        command.environmentId,
        bridgeWorkflow.workflowId
      )
    )?._id;

    /*
     * Cast used to convert data type for further processing.
     * todo Needs review for potential data corruption.
     */
    return {
      ...bridgeWorkflow,
      type: WorkflowTypeEnum.BRIDGE,
      _id: storedWorkflowId,
      steps: bridgeWorkflow.steps.map((step) => {
        const stepControlVariables = command.controls?.steps?.[step.stepId];

        return {
          ...step,
          bridgeUrl: command.bridge?.url,
          controlVariables: stepControlVariables,
          active: true,
          template: {
            type: step.type,
          },

Review comment (suggestion):
```suggestion
    const syncedWorkflowId = (
```

Code after:

    if (actor) {
      createNotificationJobsCommand.actor = actor;
    }

    const notificationJobs = await this.createNotificationJobs.execute(
      CreateNotificationJobsCommand.create(createNotificationJobsCommand)
    );

    await this.storeSubscriberJobs.execute(
      StoreSubscriberJobsCommand.create({
        environmentId: command.environmentId,
        jobs: notificationJobs,
        organizationId: command.organizationId,
      })
    );
  }

  private async getCodeFirstWorkflow(command: SubscriberJobBoundCommand): Promise<NotificationTemplateEntity | null> {
    const bridgeWorkflow = command.bridge?.workflow;

    if (!bridgeWorkflow) {
      return null;
    }

    const syncedWorkflowId = (
      await this.notificationTemplateRepository.findByTriggerIdentifier(
        command.environmentId,
        bridgeWorkflow.workflowId
      )
    )?._id;

    /*
     * Cast used to convert data type for further processing.
     * todo Needs review for potential data corruption.
     */
    return {
      ...bridgeWorkflow,
      type: WorkflowTypeEnum.BRIDGE,
      _id: syncedWorkflowId,
      steps: bridgeWorkflow.steps.map((step) => {
        const stepControlVariables = command.controls?.steps?.[step.stepId];

        return {
          ...step,
          bridgeUrl: command.bridge?.url,
          controlVariables: stepControlVariables,
          active: true,
          template: {
            type: step.type,
          },
Repository: novuhq/novu
File: apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.ts (TypeScript)

Code before:

  @InstrumentUsecase()
  async execute(command: SubscriberJobBoundCommand) {
    this.logger.assign({
      transactionId: command.transactionId,
      environmentId: command.environmentId,
      organizationId: command.organizationId,
    });

    const {
      subscriber,
      templateId,
      environmentId,
      organizationId,
      userId,
      actor,
      tenant,
      identifier,
      _subscriberSource,
      requestCategory,
      environmentName,
    } = command;

    const template = command.bridge?.workflow
      ? await this.mapBridgeWorkflow(command)
      : await this.getNotificationTemplate({
          _id: templateId,
          environmentId,
        });

    if (!template) {
      throw new BadRequestException(`Workflow id ${templateId} was not found`);
    }

    const templateProviderIds = await this.getProviderIdsForTemplate(environmentId, template);

    await this.validateSubscriberIdProperty(subscriber);

    /**
     * Due to Mixpanel HotSharding, we don't want to pass userId for production volume
     */
    const segmentUserId = ['test-workflow', 'digest-playground', 'dashboard', 'inbox-onboarding'].includes(
      command.payload.__source
    )
      ? userId
      : '';

    this.analyticsService.mixpanelTrack('Notification event trigger - [Triggers]', segmentUserId, {
      name: template.name,
      type: template?.type || WorkflowTypeEnum.REGULAR,
      origin: template?.origin,

Review comment (suggestion):
```suggestion
      : await this.getNotificationTemplateFromDB({
```

Code after:

  @InstrumentUsecase()
  async execute(command: SubscriberJobBoundCommand) {
    this.logger.assign({
      transactionId: command.transactionId,
      environmentId: command.environmentId,
      organizationId: command.organizationId,
    });

    const {
      subscriber,
      templateId,
      environmentId,
      organizationId,
      userId,
      actor,
      tenant,
      identifier,
      _subscriberSource,
      requestCategory,
      environmentName,
    } = command;

    const template = command.bridge?.workflow
      ? await this.getCodeFirstWorkflow(command)
      : await this.getWorkflow({
          _id: templateId,
          environmentId,
        });

    if (!template) {
      throw new BadRequestException(`Workflow id ${templateId} was not found`);
    }

    const templateProviderIds = await this.getProviderIdsForTemplate(environmentId, template);

    await this.validateSubscriberIdProperty(subscriber);

    /**
     * Due to Mixpanel HotSharding, we don't want to pass userId for production volume
     */
    const segmentUserId = ['test-workflow', 'digest-playground', 'dashboard', 'inbox-onboarding'].includes(
      command.payload.__source
    )
      ? userId
      : '';

    this.analyticsService.mixpanelTrack('Notification event trigger - [Triggers]', segmentUserId, {
      name: template.name,
      type: template?.type || WorkflowTypeEnum.REGULAR,
      origin: template?.origin,
Repository: novuhq/novu
File: apps/dashboard/src/utils/liquid-autocomplete.ts (TypeScript)

Code before:

    const { from, to } = options;

    return {
      from,
      to,
      options: options.options.map((option) => ({
        ...option,
        apply: (view: EditorView, completion: Completion, from: number, to: number) => {
          const selectedValue = completion.label;

          const content = view.state.doc.toString();
          const beforeCursor = content.slice(0, from);
          const afterCursor = content.slice(to);

          // Ensure proper {{ }} wrapping
          const needsOpening = !beforeCursor.endsWith('{{');
          const needsClosing = !afterCursor.startsWith('}}');

          const wrappedValue = `${needsOpening ? '{{' : ''}${selectedValue}${needsClosing ? '}}' : ''}`;

          // Calculate the final cursor position
          // Add 2 if we need to account for closing brackets
          const finalCursorPos = from + wrappedValue.length + (needsClosing ? 0 : 2);

          if (completion.type === 'digest') {
            const parts = completion.displayLabel?.split('.');
            const lastElement = parts?.[parts.length - 1];

            if (lastElement && lastElement in DIGEST_VARIABLES_FILTER_MAP) {
              track(TelemetryEvent.DIGEST_VARIABLE_SELECTED, {
                variable: lastElement,
              });
            }
          }

          view.dispatch({
            changes: { from, to, insert: wrappedValue },
            selection: { anchor: finalCursorPos },
          });

          return true;
        },
      })),
    };
  };
}


Review comment (suggestion):
This code couples autocompletion with the tracking functionality for a specific control-input use case. Since this code is a critical piece of functionality without any testing yet, I suggest passing a callback `onSelect` function and then provide in the createAutocomplete function.

Code after:

    return {
      from,
      to,
      options: options.options.map((option) => ({
        ...option,
        apply: (view: EditorView, completion: Completion, from: number, to: number) => {
          const selectedValue = completion.label;

          const content = view.state.doc.toString();
          const beforeCursor = content.slice(0, from);
          const afterCursor = content.slice(to);

          // Ensure proper {{ }} wrapping
          const needsOpening = !beforeCursor.endsWith('{{');
          const needsClosing = !afterCursor.startsWith('}}');

          const wrappedValue = `${needsOpening ? '{{' : ''}${selectedValue}${needsClosing ? '}}' : ''}`;

          // Calculate the final cursor position
          // Add 2 if we need to account for closing brackets
          const finalCursorPos = from + wrappedValue.length + (needsClosing ? 0 : 2);

          onVariableSelect?.(completion);

          view.dispatch({
            changes: { from, to, insert: wrappedValue },
            selection: { anchor: finalCursorPos },
          });

          return true;
        },
      })),
    };
  };
}
Repository: novuhq/novu
File: packages/js/src/notifications/helpers.ts (TypeScript)

Code before:

    const response = await apiService.snooze(notificationId, args.snoozeUntil);

    const updatedNotification = new Notification(response, emitter, apiService);
    emitter.emit('notification.snooze.resolved', { args, data: updatedNotification });

    return { data: updatedNotification };
  } catch (error) {
    emitter.emit('notification.snooze.resolved', { args, error });

    return { error: new NovuError('Failed to snooze notification', error) };
  }
};

export const unsnooze = async ({
  emitter,
  apiService,
  args,
}: {
  emitter: NovuEventEmitter;
  apiService: InboxService;
  args: UnsnoozeArgs;
}): Result<Notification> => {
  const { notificationId, optimisticValue } = getNotificationDetails(
    args,
    {
      isSnoozed: false,
      snoozedUntil: null,
    },
    {
      emitter,
      apiService,
    }
  );

  try {
    emitter.emit('notification.unsnooze.pending', {
      args,
      data: optimisticValue,
    });

    const response = await apiService.unsnooze(notificationId);

    const updatedNotification = new Notification(response, emitter, apiService);
    emitter.emit('notification.unsnooze.resolved', { args, data: updatedNotification });

    return { data: updatedNotification };
  } catch (error) {
    emitter.emit('notification.unsnooze.resolved', { args, error });

    return { error: new NovuError('Failed to unsnooze notification', error) };

Review comment (suggestion):
according to the API changes we have to update these as well:
```
createdAt: now,
read: false,
lastReadDate: null,
```

Code after:

    const updatedNotification = new Notification(response, emitter, apiService);
    emitter.emit('notification.snooze.resolved', { args, data: updatedNotification });

    return { data: updatedNotification };
  } catch (error) {
    emitter.emit('notification.snooze.resolved', { args, error });

    return { error: new NovuError('Failed to snooze notification', error) };
  }
};

export const unsnooze = async ({
  emitter,
  apiService,
  args,
}: {
  emitter: NovuEventEmitter;
  apiService: InboxService;
  args: UnsnoozeArgs;
}): Result<Notification> => {
  const { notificationId, optimisticValue } = getNotificationDetails(
    args,
    {
      isSnoozed: false,
      snoozedUntil: null,
    },
    {
      emitter,
      apiService,
    }
  );

  try {
    emitter.emit('notification.unsnooze.pending', {
      args,
      data: optimisticValue,
    });

    const response = await apiService.unsnooze(notificationId);

    const updatedNotification = new Notification(response, emitter, apiService);
    emitter.emit('notification.unsnooze.resolved', { args, data: updatedNotification });

    return { data: updatedNotification };
  } catch (error) {
    emitter.emit('notification.unsnooze.resolved', { args, error });

    return { error: new NovuError('Failed to unsnooze notification', error) };
  }
};
Repository: novuhq/novu
File: apps/dashboard/src/utils/liquid-autocomplete.ts (TypeScript)

Code before:
import { getFilters } from '@/components/variable/constants';
import { LiquidVariable } from '@/utils/parseStepVariables';
import { Completion, CompletionContext, CompletionResult } from '@codemirror/autocomplete';
import { EditorView } from '@uiw/react-codemirror';

interface CompletionOption {
  label: string;
  type: string;
  boost?: number;
}

// Novu JIT namespaces
const PAYLOAD_NAMESPACE = 'payload';
const SUBSCRIBER_DATA_NAMESPACE = 'subscriber.data';
const STEP_PAYLOAD_REGEX = /^steps\.[^.]+\.events\[\d+\]\.payload/;

/**
 * Liquid variable autocomplete for the following patterns:
 *
 * 1. Payload Variables:
 *    Valid:
 *    - payload.
 *    - payload.user
 *    - payload.anyNewField (allows any new field)
 *    - payload.deeply.nested.field
 *    Invalid:
 *    - pay (shows suggestions but won't validate)
 *    - payload (shows suggestions but won't validate)
 *
 * 2. Subscriber Variables:
 *    Valid:
 *    - subscriber.data.
 *    - subscriber.data.anyNewField (allows any new field)
 *    - subscriber.data.custom.nested.field
 *    - subscriber (shows suggestions but won't validate)
 *    - subscriber.email
 *    - subscriber.firstName
 *    Invalid:
 *    - subscriber.someOtherField (must use valid subscriber field)
 *

Review comment (suggestion):
for the email step, we will also have `current`; but when iterating over the `events` we should probably also do `current.payload`

Code after:
import { getFilters } from '@/components/variable/constants';
import { LiquidVariable } from '@/utils/parseStepVariables';
import { Completion, CompletionContext, CompletionResult } from '@codemirror/autocomplete';
import { EditorView } from '@uiw/react-codemirror';

interface CompletionOption {
  label: string;
  type: string;
  boost?: number;
}

// Novu JIT namespaces
const PAYLOAD_NAMESPACE = 'payload';
const SUBSCRIBER_DATA_NAMESPACE = 'subscriber.data';
const STEP_PAYLOAD_REGEX = /^steps\.[a-zA-Z0-9_-]+\.events/;

/**
 * Liquid variable autocomplete for the following patterns:
 *
 * 1. Payload Variables:
 *    Valid:
 *    - payload.
 *    - payload.user
 *    - payload.anyNewField (allows any new field)
 *    - payload.deeply.nested.field
 *    Invalid:
 *    - pay (shows suggestions but won't validate)
 *    - payload (shows suggestions but won't validate)
 *
 * 2. Subscriber Variables:
 *    Valid:
 *    - subscriber.data.
 *    - subscriber.data.anyNewField (allows any new field)
 *    - subscriber.data.custom.nested.field
 *    - subscriber (shows suggestions but won't validate)
 *    - subscriber.email
 *    - subscriber.firstName
 *    Invalid:
 *    - subscriber.someOtherField (must use valid subscriber field)
 *
Repository: novuhq/novu
File: packages/js/src/ui/components/primitives/Popover/PopoverContent.tsx (TypeScript)

Code before:
    const container = containerElement();

    if (
      active() !== floating() ||
      floating()?.contains(e.target as Node) ||
      (container && (e.target as Element).shadowRoot === container)
    ) {
      return;
    }

    onClose();
  };

  const handleEscapeKey: EventListener = (e) => {
    if (active() !== floating()) {
      return;
    }

    if (e instanceof KeyboardEvent && e.key === 'Escape') {
      onClose();
    }
  };

  onMount(() => {
    document.body.addEventListener('click', handleClickOutside);
    containerElement()?.addEventListener('click', handleClickOutside);
    document.body.addEventListener('keydown', handleEscapeKey);
  });

  onCleanup(() => {
    document.body.removeEventListener('click', handleClickOutside);
    containerElement()?.removeEventListener('click', handleClickOutside);
    document.body.removeEventListener('keydown', handleEscapeKey);
  });

  return (
    <Show when={open()}>
      <Portal>
        <PopoverContentBody {...props} />
      </Portal>
    </Show>
  );
};


Review comment (suggestion):
listen to the `containerElement` internal clicks, for example, outside clicks to close the dropdowns

Code after:
    const containerElement = container();

    if (
      active() !== floating() ||
      floating()?.contains(e.target as Node) ||
      (containerElement && (e.target as Element).shadowRoot === containerElement)
    ) {
      return;
    }

    onClose();
  };

  const handleEscapeKey: EventListener = (e) => {
    if (active() !== floating()) {
      return;
    }

    if (e instanceof KeyboardEvent && e.key === 'Escape') {
      onClose();
    }
  };

  onMount(() => {
    document.body.addEventListener('click', handleClickOutside);
    container()?.addEventListener('click', handleClickOutside);
    document.body.addEventListener('keydown', handleEscapeKey);
  });

  onCleanup(() => {
    document.body.removeEventListener('click', handleClickOutside);
    container()?.removeEventListener('click', handleClickOutside);
    document.body.removeEventListener('keydown', handleEscapeKey);
  });

  return (
    <Show when={open()}>
      <Portal>
        <PopoverContentBody {...props} />
      </Portal>
    </Show>
  );
};
Repository: novuhq/novu
File: packages/js/src/ui/components/primitives/Tabs/useKeyboardNavigation.ts (TypeScript)

Code before:
import { Accessor, createEffect, createSignal, onCleanup, Setter } from 'solid-js';
import { useAppearance } from '../../../context';

export const useKeyboardNavigation = ({
  activeTab,
  setActiveTab,
  tabsContainer,
}: {
  activeTab: Accessor<string>;
  setActiveTab: Setter<string>;
  tabsContainer: Accessor<HTMLDivElement | undefined>;
}) => {
  const { containerElement } = useAppearance();
  const [keyboardNavigation, setKeyboardNavigation] = createSignal(false);

  const getRoot = () => {
    const container = containerElement();

    return container instanceof ShadowRoot ? container : document;
  };

  createEffect(() => {
    const root = getRoot();

    const handleTabKey: EventListener = (event) => {
      if (!(event instanceof KeyboardEvent) || event.key !== 'Tab') {
        return;
      }

      const tabs = tabsContainer()?.querySelectorAll('[role="tab"]');
      if (!tabs || !root.activeElement) {
        return;
      }

      setKeyboardNavigation(Array.from(tabs).includes(root.activeElement));
    };

    root.addEventListener('keyup', handleTabKey);

    return onCleanup(() => root.removeEventListener('keyup', handleTabKey));
  });

  createEffect(() => {
    const root = getRoot();


Review comment (suggestion):
handle tab and keyboard navigation on components, for example `tabs`

Code after:
import { Accessor, createEffect, createSignal, onCleanup, Setter } from 'solid-js';
import { useAppearance } from '../../../context';

export const useKeyboardNavigation = ({
  activeTab,
  setActiveTab,
  tabsContainer,
}: {
  activeTab: Accessor<string>;
  setActiveTab: Setter<string>;
  tabsContainer: Accessor<HTMLDivElement | undefined>;
}) => {
  const { container } = useAppearance();
  const [keyboardNavigation, setKeyboardNavigation] = createSignal(false);

  const getRoot = () => {
    const containerElement = container();

    return containerElement instanceof ShadowRoot ? containerElement : document;
  };

  createEffect(() => {
    const root = getRoot();

    const handleTabKey: EventListener = (event) => {
      if (!(event instanceof KeyboardEvent) || event.key !== 'Tab') {
        return;
      }

      const tabs = tabsContainer()?.querySelectorAll('[role="tab"]');
      if (!tabs || !root.activeElement) {
        return;
      }

      setKeyboardNavigation(Array.from(tabs).includes(root.activeElement));
    };

    root.addEventListener('keyup', handleTabKey);

    return onCleanup(() => root.removeEventListener('keyup', handleTabKey));
  });

  createEffect(() => {
    const root = getRoot();
Repository: novuhq/novu
File: packages/react/src/hooks/NovuProvider.tsx (TypeScript)

Code before:

  const novu = useMemo(
    () =>
      new Novu({
        applicationIdentifier,
        subscriberHash,
        backendUrl,
        apiUrl,
        socketUrl,
        useCache,
        __userAgent: `${baseUserAgent} ${userAgentType}`,
        subscriber: subscriberObj,
      }),
    [applicationIdentifier, subscriberHash, backendUrl, apiUrl, socketUrl, useCache, userAgentType]
  );

  useEffect(() => {
    const currentSubscriberId = subscriberObj.subscriberId;
    const prevSubscriberId = prevSubscriberIdRef.current;

    if (prevSubscriberId !== undefined && prevSubscriberId !== currentSubscriberId) {
      novu.changeSubscriber({
        subscriber: subscriberObj,
        subscriberHash: props.subscriberHash,
      });
    }

    prevSubscriberIdRef.current = currentSubscriberId;
  }, [subscriberObj.subscriberId, props.subscriberHash, novu]);

  return <NovuContext.Provider value={novu}>{children}</NovuContext.Provider>;
};

export const useNovu = () => {
  const context = useContext(NovuContext);
  if (!context) {
    throw new Error('useNovu must be used within a <NovuProvider />');
  }

  return context;
};

export const useUnsafeNovu = () => {
  const context = useContext(NovuContext);

  return context;
};

function buildSubscriber(subscriberId: string | undefined, subscriber: Subscriber | string | undefined): Subscriber {
  // subscriber object
  if (subscriber) {

Review comment (suggestion):
this if statement seems to me should be an internal logic of the `changeSubscriber`, so it should initialize the session only when the subscriberId changes, then this `useEffect` will be simpler and we won't need to have previous refs

Code after:
  const novu = useMemo(
    () =>
      new Novu({
        applicationIdentifier,
        subscriberHash,
        backendUrl,
        apiUrl,
        socketUrl,
        useCache,
        __userAgent: `${baseUserAgent} ${userAgentType}`,
        subscriber: subscriberObj,
      }),
    [applicationIdentifier, subscriberHash, backendUrl, apiUrl, socketUrl, useCache, userAgentType]
  );

  useEffect(() => {
    novu.changeSubscriber({
      subscriber: subscriberObj,
      subscriberHash: props.subscriberHash,
    });
  }, [subscriberObj.subscriberId, props.subscriberHash, novu]);

  return <NovuContext.Provider value={novu}>{children}</NovuContext.Provider>;
};

export const useNovu = () => {
  const context = useContext(NovuContext);
  if (!context) {
    throw new Error('useNovu must be used within a <NovuProvider />');
  }

  return context;
};

export const useUnsafeNovu = () => {
  const context = useContext(NovuContext);

  return context;
};

function buildSubscriber(subscriberId: string | undefined, subscriber: Subscriber | string | undefined): Subscriber {
  // subscriber object
  if (subscriber) {
    return typeof subscriber === 'string' ? { subscriberId: subscriber } : subscriber;
  }

  // subscriberId
  if (subscriberId) {
    return { subscriberId: subscriberId as string };
  }