Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrate events atoms to reatom3 #856

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .cursorignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Add directories or file patterns to ignore during indexing (e.g. foo/ or *.csv)
__fixtures__
__mocks__
__test__
./**/*.fixture.tsx
./**/fixture/*.*
Comment on lines +5 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Fixture-related ignore patterns are comprehensive.

The ignore patterns for fixture files are well-defined, covering both TypeScript and TypeScript React files. However, there's a small optimization opportunity.

Consider combining lines 5 and 9 into a single pattern:

*.fixture.{ts,tsx}

This would cover both cases without the need for two separate entries.

Also applies to: 9-9

.github/workflows/run_e2e_tests.yml
.helpers
*.fixture.{ts,tsx}
**/*.test.tsx
appconfig.js
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Configuration file ignore patterns are appropriate.

The ignore patterns for various configuration files are well-defined and cover a wide range of tools and frameworks used in the project.

Consider using a more generic pattern to catch all configuration files:

*.config.{js,ts,cjs}

This would cover most of the configuration files and reduce the number of individual entries. You may still need to keep specific entries for files that don't follow this naming convention.

Also applies to: 15-16, 18-18, 22-25

configs/*
cosmos**
e2e/**
jest.config.js
jest.setup.js
playwright**
postcss.config.ts
public
scripts
server
template.config.js
vite.config.ts
vite.proxy.ts
i18next-scanner.config.cjs
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@
"@reatom/core-v2": "^3.1.4",
"@reatom/hooks": "^3.5.5",
"@reatom/logger": "^3.8.4",
"@reatom/npm-react": "^3.8.10",
"@reatom/npm-react": "^3.9.0",
"@reatom/primitives": "^3.7.3",
Comment on lines +98 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Overall, the package.json changes look appropriate for the PR objective.

The updates to @reatom/npm-react and the addition of @reatom/primitives are in line with migrating events atoms to reatom3. However, to ensure a smooth transition, consider the following follow-up actions:

  1. Update the project's documentation to reflect the use of these new/updated packages.
  2. Review and update any affected components or modules that rely on the previous version of reatom.
  3. Run the project's test suite to catch any potential issues introduced by these changes.
  4. Consider adding or updating integration tests specifically for the migrated events atoms.

To facilitate a smooth migration process, consider creating a migration guide or updating the existing one if available. This guide should outline the steps to update components using the old reatom version to the new one, highlighting any API changes or new features that developers should be aware of.

"@reatom/react-v2": "^3.1.2",
"@sentry/react": "^8.18.0",
"@slack/web-api": "^7.3.1",
Expand Down
21 changes: 12 additions & 9 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 25 additions & 0 deletions src/core/api/events.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { configRepo } from '~core/config';
import { apiClient } from '~core/apiClientInstance';
import type { Event } from '~core/types';

export interface EventListParams {
feed?: string;
bbox?: string;
}
Comment on lines +5 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider adding type constraints or documentation for EventListParams.

The interface looks good, but consider the following suggestions:

  1. Add JSDoc comments to explain the purpose and expected format of each property.
  2. If possible, use more specific types for feed and bbox instead of broad string types.

Example:

export interface EventListParams {
  /** The feed identifier to filter events. */
  feed?: string; // Consider using a union type if there are specific feed options
  /** Bounding box coordinates for geographic filtering. Format: "min_lon,min_lat,max_lon,max_lat" */
  bbox?: string; // Consider using a tuple type [number, number, number, number] if appropriate
}


export function getEventsList(params: EventListParams, abortController: AbortController) {
return apiClient
.get<Event[]>(
'/events',
{
appId: configRepo.get().id,
...params,
},
true,
{
signal: abortController.signal,
errorsConfig: { hideErrors: true },
},
)
.then((response) => response ?? []); // Ensure we always return an array
}
Comment on lines +10 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and config access in getEventsList function.

The function looks good overall, but consider the following improvements:

  1. Error handling:

    • Instead of suppressing all errors, consider handling specific error types or allowing certain errors to propagate.
    • You might want to log errors even if they're not shown to the user.
  2. Config access:

    • The use of configRepo.get().id assumes the config is always available. Consider adding a null check or using a default value to prevent potential runtime errors.
  3. Return type:

    • Explicitly declare the return type of the function for better type safety.

Example implementation:

export function getEventsList(params: EventListParams, abortController: AbortController): Promise<Event[]> {
  const appId = configRepo.get()?.id; // Add null check
  if (!appId) {
    console.error('Application ID not found in config');
    return Promise.resolve([]); // or throw an error
  }

  return apiClient
    .get<Event[]>(
      '/events',
      {
        appId,
        ...params,
      },
      true,
      {
        signal: abortController.signal,
        errorsConfig: { 
          hideErrors: true,
          onError: (error) => console.error('Error fetching events:', error), // Log errors
        },
      },
    )
    .then((response) => response ?? [])
    .catch((error) => {
      if (error.name === 'AbortError') {
        console.log('Request was aborted');
      } else {
        console.error('Unexpected error:', error);
      }
      return [];
    });
}

These changes improve error handling, add safety checks, and provide more information about potential issues during execution.

2 changes: 1 addition & 1 deletion src/core/resources/bivariateStatisticsResource/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const bivariateStatisticsDependencyAtom = v3toV2(
atom((ctx) => {
const focusedGeometry = ctx.spy(focusedGeometryAtom.v3atom);
return { focusedGeometry };
}),
}, 'bivariateStatisticsDependencyAtom'),
);

let worldStatsCache: Stat;
Expand Down
89 changes: 57 additions & 32 deletions src/core/shared_state/currentEvent.ts
Original file line number Diff line number Diff line change
@@ -1,38 +1,63 @@
import { createAtom, createBooleanAtom } from '~utils/atoms';
import { atom, action } from '@reatom/core';
import { v3toV2 } from '~utils/atoms/v3tov2';
import { focusedGeometryAtom } from '../focused_geometry/model';

// * CurrentEventAtomState *
// null represents the initial state of event - we need that state for cases of autoselecting event
// { id: null } represents event reset was caused by user actions
export type CurrentEventAtomState = {
id: string | null;
} | null;

export const currentEventAtom = createAtom(
{
setCurrentEventId: (eventId: string | null) => eventId,
focusedGeometryAtom,
},
({ onAction, onChange }, state: CurrentEventAtomState = null) => {
onChange('focusedGeometryAtom', (focusedGeometry) => {
const currentGeometrySource = focusedGeometry?.source;
if (
currentGeometrySource &&
currentGeometrySource.type !== 'event' &&
currentGeometrySource.type !== 'episode'
) {
// if focused geometry is no longer represents event, user stopped work with events
// following state specifies that
state = { id: null };
// reatom v2 imports mapped to reatom v3
const __v3_imports = {
focusedGeometryAtom: focusedGeometryAtom.v3atom,
};

function __create_v3() {
const { focusedGeometryAtom } = __v3_imports;
// v3 definitions section

// * CurrentEventAtomState *
// null represents the initial state of event - we need that state for cases of autoselecting event
// { id: null } represents event reset was caused by user actions
type CurrentEventAtomState = { id: string | null } | null;

const currentEventAtom = atom<CurrentEventAtomState>(null, 'currentEventAtom');
const scheduledAutoSelect = atom(false, 'scheduledAutoSelect');
const scheduledAutoFocus = atom(false, 'scheduledAutoFocus');

const setCurrentEventId = action((ctx, eventId: string | null) => {
currentEventAtom(ctx, { id: eventId });
}, 'setCurrentEventId');

// Stateless computed atom that updates currentEventAtom based on focusedGeometryAtom
const computedCurrentEventAtom = atom((ctx) => {
const currentEvent = ctx.spy(currentEventAtom);
const focusedGeometry = ctx.spy(focusedGeometryAtom);
const currentGeometrySource = focusedGeometry?.source;

if (
currentGeometrySource &&
currentGeometrySource.type !== 'event' &&
currentGeometrySource.type !== 'episode'
) {
if (currentEvent?.id !== null) {
currentEventAtom(ctx, { id: null });
}
});
}
}, 'computedCurrentEventAtom');

Comment on lines +28 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Avoid updating state within computed atoms; use actions or effects instead

Mutating atoms within computed atoms can lead to unintended side effects and violates Reatom's reactive programming principles. Instead of updating currentEventAtom directly within computedCurrentEventAtom, consider refactoring the code to use an effect or an action to handle the state update when the specified conditions are met.

Here's a suggested refactoring using an effect:

import { onUpdate } from '@reatom/hooks';

onUpdate(focusedGeometryAtom, (ctx, focusedGeometry) => {
  const currentEvent = ctx.get(currentEventAtom);
  const currentGeometrySource = focusedGeometry?.source;

  if (
    currentGeometrySource &&
    currentGeometrySource.type !== 'event' &&
    currentGeometrySource.type !== 'episode' &&
    currentEvent?.id !== null
  ) {
    currentEventAtom(ctx, { id: null });
  }
});

By using onUpdate, you can react to changes in focusedGeometryAtom and update currentEventAtom accordingly, adhering to Reatom's best practices and avoiding direct state mutations within computed atoms.

onAction('setCurrentEventId', (eventId) => (state = { id: eventId }));
// v3 exports object
return {
currentEventAtom,
scheduledAutoSelect,
scheduledAutoFocus,
setCurrentEventId,
computedCurrentEventAtom,
};
}

return state;
},
'[Shared state] currentEventAtom',
);
const v3 = __create_v3();
// v3 exports as default
export default v3;

export const scheduledAutoSelect = createBooleanAtom(false, 'scheduledAutoSelect');
export const scheduledAutoFocus = createBooleanAtom(false, 'scheduledAutoFocus');
// v2 compatible exports keeping the same names
export const currentEventAtom = v3toV2(v3.currentEventAtom, {
setCurrentEventId: v3.setCurrentEventId,
});
export const scheduledAutoSelect = v3toV2(v3.scheduledAutoSelect);
export const scheduledAutoFocus = v3toV2(v3.scheduledAutoFocus);
122 changes: 76 additions & 46 deletions src/core/shared_state/currentEventFeed.ts
Original file line number Diff line number Diff line change
@@ -1,55 +1,85 @@
import { createAtom } from '~utils/atoms';
import { atom, action, type Ctx } from '@reatom/core';
import { v3toV2 } from '~utils/atoms/v3tov2';
import { configRepo } from '~core/config';
import { currentEventAtom, scheduledAutoSelect } from './currentEvent';
import type { EventFeedConfig } from '~core/config/types';

type CurrentEventFeedAtomState = {
id: string;
} | null;

export const currentEventFeedAtom = createAtom(
{
setCurrentFeed: (feedId: string) => feedId,
setFeedForExistingEvent: (feedId: string) => feedId,
resetCurrentFeed: () => null,
syncFeed: (eventFeeds) => eventFeeds,
},
(
{ onAction, onChange, schedule, getUnlistedState },
state: CurrentEventFeedAtomState = { id: configRepo.get().defaultFeed },
) => {
onAction('setCurrentFeed', (feedId) => {
if (state?.id !== feedId) {
state = { id: feedId };
}
schedule((dispatch) => dispatch(currentEventAtom.setCurrentEventId(null)));
});
// reatom v2 imports mapped to reatom v3
const __v3_imports = {
currentEventAtom: currentEventAtom.v3atom,
scheduledAutoSelect: scheduledAutoSelect.v3atom,
};

onAction('resetCurrentFeed', () => {
if (state) {
state = null;
}
});

onAction('syncFeed', (eventFeeds) => {
if (eventFeeds && eventFeeds.data.length && !eventFeeds.loading) {
const newFeed = checkFeed(eventFeeds.data, state?.id);
if (newFeed !== undefined && newFeed !== state?.id) {
state = { id: newFeed };
const currentEvent = getUnlistedState(currentEventAtom);
if (currentEvent !== null)
schedule((dispatch) => dispatch(scheduledAutoSelect.setTrue()));
}
function __create_v3() {
const { currentEventAtom, scheduledAutoSelect } = __v3_imports;
// v3 definitions section

type CurrentEventFeedAtomState = {
id: string;
} | null;

const currentEventFeedAtom = atom<CurrentEventFeedAtomState>(
{ id: configRepo.get().defaultFeed },
'currentEventFeedAtom',
);

const setCurrentFeed = action((ctx, feedId: string) => {
updateFeed(ctx, feedId);
}, 'setCurrentFeed');

const setFeedForExistingEvent = action((ctx, feedId: string) => {
updateFeed(ctx, feedId);
}, 'setFeedForExistingEvent');

const resetCurrentFeed = action((ctx) => {
updateFeed(ctx, null);
}, 'resetCurrentFeed');

const syncFeed = action(
(ctx, eventFeeds: { data: EventFeedConfig[]; loading: boolean }) => {
if (eventFeeds?.data?.length && !eventFeeds.loading) {
const currentFeed = ctx.get(currentEventFeedAtom);
const newFeed = checkFeed(eventFeeds.data, currentFeed?.id);
updateFeed(ctx, newFeed);
}
});
},
'syncFeed',
);

function updateFeed(ctx: Ctx, newFeedId: string | null) {
const currentFeed = ctx.get(currentEventFeedAtom);
if (currentFeed?.id !== newFeedId) {
currentEventFeedAtom(ctx, newFeedId ? { id: newFeedId } : null);
// deselect current event
currentEventAtom(ctx, { id: null });
scheduledAutoSelect(ctx, false);
}
}
Comment on lines +52 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect atom state updates using function call syntax

In Reatom v3, atoms are no longer functions and should not be called directly to update their state. Instead, use ctx.set to update the atom's value.

Please apply the following changes:

-      currentEventFeedAtom(ctx, newFeedId ? { id: newFeedId } : null);
+      ctx.set(currentEventFeedAtom, newFeedId ? { id: newFeedId } : null);
-      currentEventAtom(ctx, { id: null });
+      ctx.set(currentEventAtom, { id: null });
-      scheduledAutoSelect(ctx, false);
+      ctx.set(scheduledAutoSelect, false);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
currentEventFeedAtom(ctx, newFeedId ? { id: newFeedId } : null);
// deselect current event
currentEventAtom(ctx, { id: null });
scheduledAutoSelect(ctx, false);
}
ctx.set(currentEventFeedAtom, newFeedId ? { id: newFeedId } : null);
// deselect current event
ctx.set(currentEventAtom, { id: null });
ctx.set(scheduledAutoSelect, false);
}


return state;
},
'[Shared state] currentEventFeedAtom',
);
function checkFeed(eventFeeds: EventFeedConfig[], feedId?: string) {
if (!feedId) return configRepo.get().defaultFeed;
const feed = eventFeeds?.find((fd) => fd.feed === feedId);
return feed ? feed.feed : configRepo.get().defaultFeed;
}

function checkFeed(eventFeeds: EventFeedConfig[], feedId?: string) {
if (!feedId) return configRepo.get().defaultFeed;
const feed = eventFeeds?.find((fd) => fd.feed === feedId);
return feed ? feed.feed : configRepo.get().defaultFeed;
// v3 exports object
return {
currentEventFeedAtom,
setCurrentFeed,
setFeedForExistingEvent,
resetCurrentFeed,
syncFeed,
};
}

const v3 = __create_v3();
// v3 exports as default
export default v3;

// v2 compatible exports keeping the same names
export const currentEventFeedAtom = v3toV2(v3.currentEventFeedAtom, {
setCurrentFeed: v3.setCurrentFeed,
setFeedForExistingEvent: v3.setFeedForExistingEvent,
resetCurrentFeed: v3.resetCurrentFeed,
syncFeed: v3.syncFeed,
});
Loading
Loading