Conversation
Fixes #168
Contributor
Reviewer's GuideUpgrades the monorepo to Vitest 4 and Vite 8, centralizes Vitest/Storybook/browser test configuration (including shared dirname utility and coverage settings), tightens TypeScript typings and mocking patterns in tests, fixes Mongo optional-field persistence semantics, and adjusts Storybook/Docusaurus configs to align with ESM and the new toolchain. Sequence diagram for Storybook browser tests with Vitest 4 and Playwright providersequenceDiagram
participant DevVitestConfig
participant VitestRunner
participant StorybookPlugin as StorybookVitestPlugin
participant BrowserApi as VitestBrowserApi
participant Playwright as PlaywrightProvider
participant StorybookServer
participant ReactComponent
DevVitestConfig->>VitestRunner: defineConfig(createStorybookVitestConfig(pkgDirname, opts))
VitestRunner->>StorybookPlugin: load project name storybook with browser.enabled true
StorybookPlugin->>BrowserApi: start api host 127.0.0.1 port getBrowserApiPort(pkgDirname)
BrowserApi->>Playwright: initialize provider playwright() with instances
Playwright->>StorybookServer: launch headless browser and open Storybook url
StorybookServer->>ReactComponent: render component story
VitestRunner->>ReactComponent: execute tests against rendered story
VitestRunner->>VitestRunner: collect coverage for src/**/*.{ts,tsx}
Class diagram for shared Vitest and Storybook configurationclassDiagram
class ConfigVitestPackage {
<<module>>
}
class baseConfig {
<<config>>
+test_typecheck_enabled : boolean
+test_typecheck_checker : string
+test_typecheck_tsconfig : string
+test_typecheck_include : string[]
+test_typecheck_exclude : string[]
+test_typecheck_ignoreSourceErrors : boolean
+test_coverage_provider : string
+test_coverage_reporter : string[]
+test_coverage_reportsDirectory : string
}
class StorybookVitestConfigOptions {
<<type>>
+storybookDirRelativeToPackage : string
+setupFiles : string[]
+browsers : object[]
+additionalCoverageExclude : string[]
}
class createStorybookVitestConfig {
<<function>>
+createStorybookVitestConfig(pkgDirname : string, opts : StorybookVitestConfigOptions) ViteUserConfig
}
class getBrowserApiPort {
<<function>>
+getBrowserApiPort(pkgDirname : string) number
}
class getDirnameFromImportMetaUrl {
<<function>>
+getDirnameFromImportMetaUrl(importMetaUrl : string) string
}
class StorybookBrowserProject {
<<config>>
+name : string
+configFile : string
+setupFiles : string[]
+browser_enabled : boolean
+browser_api_host : string
+browser_api_port : number
+browser_headless : boolean
+browser_provider : PlaywrightProvider
+browser_instances : object[]
+coverage_include : string[]
+coverage_exclude : string[]
}
class PlaywrightProvider {
<<external>>
+playwright() PlaywrightProvider
}
ConfigVitestPackage --> baseConfig : exports
ConfigVitestPackage --> createStorybookVitestConfig : exports
ConfigVitestPackage --> getDirnameFromImportMetaUrl : exports
createStorybookVitestConfig --> StorybookVitestConfigOptions : uses
createStorybookVitestConfig --> baseConfig : merges
createStorybookVitestConfig --> getBrowserApiPort : calls
createStorybookVitestConfig --> StorybookBrowserProject : builds
StorybookBrowserProject --> PlaywrightProvider : uses
Flow diagram for Storybook logged-in user stories using typed argsflowchart TD
StoryRootDefault[RootDefault Story]
StoryRootLoading[RootLoading Story]
StoryRootError[RootError Story]
StoryCommunityDefault[CommunityDefault Story]
ArgsRoot[args satisfies LoggedInUserContainerProps]
ArgsRootRootContainer[args.autoLogin passed to LoggedInUserRootContainer]
ComponentLoggedInContainer[LoggedInUserContainer]
ComponentLoggedInRootContainer[LoggedInUserRootContainer]
RouterRoutes[React Router Routes]
StoryRootDefault --> ArgsRoot
StoryRootLoading --> ArgsRoot
StoryRootError --> ArgsRoot
StoryCommunityDefault --> ArgsRoot
ArgsRoot --> RouterRoutes
RouterRoutes --> ComponentLoggedInContainer
StoryRootDefault --> RouterRoutes
StoryRootLoading --> RouterRoutes
StoryRootError --> RouterRoutes
StoryCommunityDefault --> RouterRoutes
ComponentLoggedInRootContainer --> ComponentLoggedInContainer
ArgsRoot --> ComponentLoggedInRootContainer
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
apps/ui-community/package.jsonandpackages/ocom/ui-components/package.json, thetest:coveragescripts now just runpnpm run testwithout coverage flags, which is confusing given the script name—consider either reintroducing the--coverageoptions or renaming these scripts to reflect their behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `apps/ui-community/package.json` and `packages/ocom/ui-components/package.json`, the `test:coverage` scripts now just run `pnpm run test` without coverage flags, which is confusing given the script name—consider either reintroducing the `--coverage` options or renaming these scripts to reflect their behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Mar 31, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
packages/cellix/ui-core/vitest.config.tsyou explicitly settest.coverage.provider = 'istanbul', but this is already defined centrally inbaseConfig; consider relying on the shared config to avoid redundant per-package overrides that can drift over time. - Adding
"types": ["vitest/globals", "@testing-library/jest-dom"]directly topackages/cellix/ui-core/tsconfig.jsonmixes test-only types into the library’s main TS config; it may be cleaner to move these to a dedicated test tsconfig (ortsconfig.vitest.json) that extends the base one so consumers of the library don’t implicitly inherit test globals.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `packages/cellix/ui-core/vitest.config.ts` you explicitly set `test.coverage.provider = 'istanbul'`, but this is already defined centrally in `baseConfig`; consider relying on the shared config to avoid redundant per-package overrides that can drift over time.
- Adding `"types": ["vitest/globals", "@testing-library/jest-dom"]` directly to `packages/cellix/ui-core/tsconfig.json` mixes test-only types into the library’s main TS config; it may be cleaner to move these to a dedicated test tsconfig (or `tsconfig.vitest.json`) that extends the base one so consumers of the library don’t implicitly inherit test globals.
## Individual Comments
### Comment 1
<location path="packages/cellix/ui-core/src/components/molecules/require-auth/index.test.tsx" line_range="145-154" />
<code_context>
+ expect(window.sessionStorage.getItem('redirectTo')).toBe('/private?tab=1');
+ });
+
+ it('does not auto-login when auth params are present', async () => {
+ const signinRedirect = vi.fn().mockResolvedValue(undefined);
+
+ mockHasAuthParams.mockReturnValue(true);
+ mockUseAuth.mockReturnValue({
+ isLoading: false,
+ activeNavigator: undefined,
+ isAuthenticated: false,
+ error: undefined,
+ signinRedirect,
+ });
+
+ render(
+ <MemoryRouter>
+ <RequireAuth forceLogin>
+ <div>Private content</div>
+ </RequireAuth>
+ </MemoryRouter>,
+ );
+
+ await waitFor(() => {
+ expect(signinRedirect).toHaveBeenCalledTimes(1);
+ });
+
+ expect(window.sessionStorage.getItem('redirectTo')).toBeNull();
+ });
+});
</code_context>
<issue_to_address>
**issue (testing):** Test description and expectations contradict each other regarding auto-login behavior.
The test name says it "does not auto-login" when auth params are present, yet it asserts `signinRedirect` is called once. If the behavior should skip `signinRedirect` when `hasAuthParams` is true, this assertion should expect it not to be called. If calling `signinRedirect` once is correct, please update the test name/description to reflect that behavior more clearly.
</issue_to_address>Hi @nnoce14! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
packages/cellix/ui-core/src/components/molecules/require-auth/index.test.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
packages/cellix/ui-core/vitest.config.ts, the explicittest.coverage.provider: 'istanbul'override appears redundant now thatbaseConfigalready sets Istanbul globally, so you could simplify by relying on the shared config unless you intend to diverge locally. - The header story files in
packages/ocom/ui-componentsnow typeMetaandStoryObjwith*Propsinstead oftypeof Component; this drops the link to the actual component type and may reduce Storybook/TS inference (e.g. for args and controls), so consider keepingtypeof Componentin the meta and story types for stronger type safety. - In
apps/ui-community/package.json, the"test:coverage"script uses different indentation than the surrounding scripts, which could be normalized to keep the JSON formatting consistent with the rest of the repo.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `packages/cellix/ui-core/vitest.config.ts`, the explicit `test.coverage.provider: 'istanbul'` override appears redundant now that `baseConfig` already sets Istanbul globally, so you could simplify by relying on the shared config unless you intend to diverge locally.
- The header story files in `packages/ocom/ui-components` now type `Meta` and `StoryObj` with `*Props` instead of `typeof Component`; this drops the link to the actual component type and may reduce Storybook/TS inference (e.g. for args and controls), so consider keeping `typeof Component` in the meta and story types for stronger type safety.
- In `apps/ui-community/package.json`, the `"test:coverage"` script uses different indentation than the surrounding scripts, which could be normalized to keep the JSON formatting consistent with the rest of the repo.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…t.config.ts and shared config, update test description in RequireAuth tests
Member
Author
|
@sourcery-ai review |
…h tests and adjust biome version in lockfile to fix pipeline
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
packages/cellix/ui-core/tsconfig.json, addingvitest/globalsand@testing-library/jest-domto the top-leveltypesfor the package will leak test-only types into the library’s public type surface; consider moving these to a dedicated test tsconfig or usingtsconfig.test.jsonthat extends the main config instead. - The new
packages/cellix/ui-core/vitest.setup.tsfile is introduced but doesn’t appear to be referenced by the ui-core Vitest configuration (which still defaults to.storybook/vitest.setup.ts); either wire this file intocreateStorybookVitestConfigor remove it to avoid confusion. - For the updated Storybook stories where
Meta/StoryObjgenerics now use...Propstypes, consider also setting thecomponentfield explicitly in themetaobject to preserve strong linkage to the React component for Storybook tooling and typings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `packages/cellix/ui-core/tsconfig.json`, adding `vitest/globals` and `@testing-library/jest-dom` to the top-level `types` for the package will leak test-only types into the library’s public type surface; consider moving these to a dedicated test tsconfig or using `tsconfig.test.json` that extends the main config instead.
- The new `packages/cellix/ui-core/vitest.setup.ts` file is introduced but doesn’t appear to be referenced by the ui-core Vitest configuration (which still defaults to `.storybook/vitest.setup.ts`); either wire this file into `createStorybookVitestConfig` or remove it to avoid confusion.
- For the updated Storybook stories where `Meta`/`StoryObj` generics now use `...Props` types, consider also setting the `component` field explicitly in the `meta` object to preserve strong linkage to the React component for Storybook tooling and typings.
## Individual Comments
### Comment 1
<location path="packages/cellix/ui-core/vitest.config.ts" line_range="8-17" />
<code_context>
+ ? path.dirname(fileURLToPath(import.meta.url))
+ : __dirname;
export default defineConfig(
- createStorybookVitestConfig(dirname, {
- additionalCoverageExclude: ['eslint.config.js'],
</code_context>
<issue_to_address>
**issue (bug_risk):** Unbalanced parentheses around `defineConfig` and `createStorybookVitestConfig` make this file syntactically invalid.
In the previous version, both `createStorybookVitestConfig(` and `defineConfig(` were closed with `}),` and `);`. In the new version there are still two opening parens but only one closing `)` at the end of the file, so the file won’t parse. You need to close both calls, e.g.:
```ts
export default defineConfig(
createStorybookVitestConfig(dirname, {
additionalCoverageExclude: [
'src/index.ts',
'src/components/index.ts',
'src/components/molecules/index.tsx',
'src/components/organisms/index.tsx',
],
}),
);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…and refactor render methods to utilize args
…fig to simplify includes
Member
Author
|
@sourcery-ai dismiss |
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
mongo-unit-of-work.integration.test.tsyou addedvi.restoreAllMocks()inbeforeEach, which will also clear any spies/mocks set up atdescribescope—if there are (or will be) top-level mocks that should persist across scenarios, consider movingrestoreAllMockstoafterEachor scoping it more narrowly to avoid unintentionally resetting shared mocks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `mongo-unit-of-work.integration.test.ts` you added `vi.restoreAllMocks()` in `beforeEach`, which will also clear any spies/mocks set up at `describe` scope—if there are (or will be) top-level mocks that should persist across scenarios, consider moving `restoreAllMocks` to `afterEach` or scoping it more narrowly to avoid unintentionally resetting shared mocks.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The logged-in user Storybook stories now repeat the same
args: { autoLogin: false }and(args) =>render pattern across multiple variants; consider settingmeta.argsor a shared base story to reduce duplication and make changes to default args easier. - The
dirnamefallback logic usingtypeof __dirname === 'undefined' ? path.dirname(fileURLToPath(import.meta.url)) : __dirnameis now duplicated across several Vitest configs; you could extract this into a small shared helper to keep the configuration DRY and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logged-in user Storybook stories now repeat the same `args: { autoLogin: false }` and `(args) =>` render pattern across multiple variants; consider setting `meta.args` or a shared base story to reduce duplication and make changes to default args easier.
- The `dirname` fallback logic using `typeof __dirname === 'undefined' ? path.dirname(fileURLToPath(import.meta.url)) : __dirname` is now duplicated across several Vitest configs; you could extract this into a small shared helper to keep the configuration DRY and easier to maintain.
## Individual Comments
### Comment 1
<location path="packages/cellix/config-vitest/tsconfig.json" line_range="3-6" />
<code_context>
{
"extends": "@cellix/config-typescript/node",
"compilerOptions": {
+ "lib": ["ES2023", "DOM"],
"outDir": "dist",
"rootDir": "src",
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Including the DOM lib in a config package may hide type issues for non-DOM consumers.
Since this helper is used by both Node and browser projects, enabling `DOM` here encourages accidental use of browser globals in code that may run in Node. If this config is meant for Node only, consider restricting `lib` to ES libs and letting downstream browser apps add `DOM` themselves so typings match the actual runtime.
```suggestion
"compilerOptions": {
"lib": ["ES2023"],
"outDir": "dist",
"rootDir": "src",
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… vitest type checking for test files without polluting production output with test files and dev dependencies - Updated sidebars.ts to use ES module syntax for exporting sidebars. - Modified vitest.config.ts files across multiple packages to disable type checking. - Added tsconfig.vitest.json files to various packages and apps to extend base TypeScript configurations for Vitest. - Enhanced test files to use Mock types for better type safety and clarity. - Adjusted test implementations to ensure proper async handling and error propagation. - Updated turbo.json to streamline build dependencies and inputs.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
packages/cellix/mongoose-seedwork/tests/integration/mongo-unit-of-work.integration.test.tsthe imports from../../src/...were changed to include.tsextensions; unlessallowImportingTsExtensionsis enabled everywhere, this can break TypeScript module resolution and bundling, so consider reverting to extension-less imports for internal TS modules. - The change in
apps/docs/sidebars.tsfrommodule.exports = sidebars satisfies SidebarsConfigtoexport = sidebarsloses thesatisfiesconstraint and mixes TSexport =with the existing CommonJS expectation; consider keeping thesatisfiescheck (e.g. via a separate typed constant) and double‑check that the emitted JS still matches what Docusaurus expects (plainmodule.exports = ...).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `packages/cellix/mongoose-seedwork/tests/integration/mongo-unit-of-work.integration.test.ts` the imports from `../../src/...` were changed to include `.ts` extensions; unless `allowImportingTsExtensions` is enabled everywhere, this can break TypeScript module resolution and bundling, so consider reverting to extension-less imports for internal TS modules.
- The change in `apps/docs/sidebars.ts` from `module.exports = sidebars satisfies SidebarsConfig` to `export = sidebars` loses the `satisfies` constraint and mixes TS `export =` with the existing CommonJS expectation; consider keeping the `satisfies` check (e.g. via a separate typed constant) and double‑check that the emitted JS still matches what Docusaurus expects (plain `module.exports = ...`).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new tsconfig.vitest.json files are duplicated across many apps and packages; consider centralizing the shared config in @cellix/config-typescript and having each package extend it to reduce configuration drift.
- In mongo-unit-of-work.integration.test.ts you switched to importing source files with explicit .ts extensions (e.g. '../../src/mongoose-seedwork/mongo-unit-of-work.ts'); it might be safer and more consistent to use extensionless imports like the rest of the codebase, depending on your moduleResolution settings.
- Several tests now replace mockResolvedValue/mockRejectedValue with explicit Promise.resolve/Promise.reject wrappers around vi.fn; you might be able to keep the more concise mock helpers by tightening the Mock typing instead, which would make the tests shorter and easier to read.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new tsconfig.vitest.json files are duplicated across many apps and packages; consider centralizing the shared config in @cellix/config-typescript and having each package extend it to reduce configuration drift.
- In mongo-unit-of-work.integration.test.ts you switched to importing source files with explicit .ts extensions (e.g. '../../src/mongoose-seedwork/mongo-unit-of-work.ts'); it might be safer and more consistent to use extensionless imports like the rest of the codebase, depending on your moduleResolution settings.
- Several tests now replace mockResolvedValue/mockRejectedValue with explicit Promise.resolve/Promise.reject wrappers around vi.fn; you might be able to keep the more concise mock helpers by tightening the Mock<T> typing instead, which would make the tests shorter and easier to read.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SourceryAI
reviewed
Mar 31, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- You introduce several local
Mock<...>type aliases for handlers (e.g.TestEventHandler,EventAHandler, etc.); consider extracting a small shared helper/util type for async and sync handlers to avoid repeating similar alias definitions across tests and keep handler signatures consistent. - The
dirnameresolution pattern for ESM (typeof __dirname === 'undefined' ? path.dirname(fileURLToPath(import.meta.url)) : __dirname) is duplicated across multiple Vitest configs; factoring this into a small shared utility would reduce repetition and make future changes (e.g. if the pattern needs to change) easier. - In
apps/ui-community/package.json, thetest:coveragescript line is indented with spaces instead of tabs like the surrounding entries, which makes the scripts block visually inconsistent; aligning the indentation would improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You introduce several local `Mock<...>` type aliases for handlers (e.g. `TestEventHandler`, `EventAHandler`, etc.); consider extracting a small shared helper/util type for async and sync handlers to avoid repeating similar alias definitions across tests and keep handler signatures consistent.
- The `dirname` resolution pattern for ESM (`typeof __dirname === 'undefined' ? path.dirname(fileURLToPath(import.meta.url)) : __dirname`) is duplicated across multiple Vitest configs; factoring this into a small shared utility would reduce repetition and make future changes (e.g. if the pattern needs to change) easier.
- In `apps/ui-community/package.json`, the `test:coverage` script line is indented with spaces instead of tabs like the surrounding entries, which makes the scripts block visually inconsistent; aligning the indentation would improve readability.
## Individual Comments
### Comment 1
<location path="packages/cellix/config-vitest/src/configs/storybook.config.ts" line_range="35" />
<code_context>
enabled: true,
headless: true,
- provider: 'playwright',
+ provider: playwright(),
instances,
},
</code_context>
<issue_to_address>
**issue (bug_risk):** The `browser.provider` option likely expects a string identifier (e.g. `'playwright'`) rather than `playwright()`.
In Vitest 4, `browser.provider` is typically a string identifier like `'playwright'`, while `playwright()` is meant to be used in the top-level `plugins` array. Using `playwright()` here likely violates the expected type and could break browser tests. Please keep `provider: 'playwright'` and configure `playwright()` via plugins, or confirm in the latest Vitest browser docs that passing the plugin instance here is supported before proceeding.
</issue_to_address>
### Comment 2
<location path="apps/ui-community/vitest.config.ts" line_range="12" />
<code_context>
+ export default defineConfig(
</code_context>
<issue_to_address>
**issue (bug_risk):** `createStorybookVitestConfig` is called without the `opts` argument even though the function signature requires it.
Here `createStorybookVitestConfig` is called as `createStorybookVitestConfig(dirname)` but its `opts: StorybookVitestConfigOptions` parameter is required. This will fail type checking and may break assumptions about `opts`. Please either make `opts` optional with a default (`opts: StorybookVitestConfigOptions = {}`) or pass `{}`/explicit options here.
</issue_to_address>Hi @nnoce14! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.…omImportMetaUrl utility and updating related imports according to sourcery suggestions
Member
Author
|
@sourcery-ai dismiss |
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Several vitest configs now import
getDirnameFromImportMetaUrlfrom@cellix/config-vitest, but the package’s publicsrc/index.tsdoes not export this helper, so consumers will currently fail to build/run; consider re-exporting it fromindex.ts(and the package’s exports field if needed). - With
typecheckenabled in the shared Vitest base config andtsconfig.vitest.jsonwired as the tsconfig, double-check that every workspace that runs Vitest either has an appropriatetsconfig.vitest.jsoncommitted or overridestest.typechecklocally to avoid unexpected typecheck failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several vitest configs now import `getDirnameFromImportMetaUrl` from `@cellix/config-vitest`, but the package’s public `src/index.ts` does not export this helper, so consumers will currently fail to build/run; consider re-exporting it from `index.ts` (and the package’s exports field if needed).
- With `typecheck` enabled in the shared Vitest base config and `tsconfig.vitest.json` wired as the tsconfig, double-check that every workspace that runs Vitest either has an appropriate `tsconfig.vitest.json` committed or overrides `test.typecheck` locally to avoid unexpected typecheck failures.
## Individual Comments
### Comment 1
<location path="packages/cellix/config-vitest/src/index.ts" line_range="1-3" />
<code_context>
export { baseConfig } from "./configs/base.config.ts";
export { nodeConfig } from "./configs/node.config.ts";
-export { createStorybookVitestConfig, type StorybookVitestConfigOptions } from "./configs/storybook.config.ts";
\ No newline at end of file
+export { createStorybookVitestConfig, type StorybookVitestConfigOptions } from "./configs/storybook.config.ts";
</code_context>
<issue_to_address>
**issue (bug_risk):** Re-export `getDirnameFromImportMetaUrl` so consumers can import it from `@cellix/config-vitest`.
Several configs now import `getDirnameFromImportMetaUrl` from `@cellix/config-vitest`, but this symbol is only defined in `src/utils/dirname.ts` and not exported from the package entry. Those imports will fail at runtime. Please re-export it here (e.g. `export { getDirnameFromImportMetaUrl } from "./utils/dirname.ts";` or via a barrel) so it’s available to consumers.
</issue_to_address>
### Comment 2
<location path="packages/cellix/mongoose-seedwork/tests/integration/mongo-unit-of-work.integration.test.ts" line_range="123-132" />
<code_context>
return this.doc.bar;
}
set bar(value: string | undefined) {
+ if (value === undefined) {
+ delete this.doc.bar;
+ return;
+ }
this.doc.bar = value;
}
get baz(): string | undefined {
return this.doc.baz;
}
set baz(value: string | undefined) {
+ if (value === undefined) {
+ delete this.doc.baz;
+ return;
+ }
this.doc.baz = value;
}
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add an integration assertion that `bar`/`baz` are actually removed from the Mongo document when set to `undefined`
Since the setters now `delete` these fields when set to `undefined`, add an integration test that:
- creates a model with `bar`/`baz` set
- updates it with `bar` and/or `baz` set to `undefined`
- commits via `MongoUnitOfWork`
- asserts the persisted `TestModel` document no longer has those fields (e.g., `expect(doc.bar).toBeUndefined()` or using `"bar" in doc` checks).
This ensures we don’t regress to storing `null` or otherwise retaining the fields instead of removing them.
Suggested implementation:
```typescript
beforeEach(async () => {
vi.restoreAllMocks();
await TestModel.deleteMany({});
// biome-ignore lint:useLiteralKeys
eventBus['eventSubscribers'] = {};
});
it('removes bar/baz fields from the persisted document when set to undefined', async () => {
// Arrange: create and persist initial document with bar and baz set
const initial = await TestModel.create({
foo: 'foo-value',
bar: 'bar-value',
baz: 'baz-value',
});
// Act: use MongoUnitOfWork to load, update, and commit the aggregate
const uow = new MongoUnitOfWork(connection);
await uow.start();
const repo = new TestModelRepository(uow);
const aggregate = await repo.getById(initial.id);
if (!aggregate) {
throw new Error('Expected aggregate to be loaded');
}
aggregate.bar = undefined;
aggregate.baz = undefined;
await repo.save(aggregate);
await uow.commit();
// Assert: reload raw document and ensure bar/baz are removed
const persisted = await TestModel.findById(initial.id).lean().exec();
expect(persisted).not.toBeNull();
expect('bar' in persisted!).toBe(false);
expect('baz' in persisted!).toBe(false);
expect(persisted!.bar).toBeUndefined();
expect(persisted!.baz).toBeUndefined();
});
```
Depending on the existing test setup in this file, you may need to:
1. Ensure `MongoUnitOfWork`, `TestModelRepository`, and `connection` are imported at the top of the file (or use the same repository/connection objects as the other tests in this file).
2. If the repository or unit-of-work instantiation is done via helpers (e.g., `makeTestRepository(uow)` or similar), adjust the `new MongoUnitOfWork(connection)` and `new TestModelRepository(uow)` lines to match the conventions already used in the other integration tests.
3. If `foo` is not a required field in your schema (or another required field exists), update the `{ foo: 'foo-value', ... }` initializer to satisfy the actual `TestModel` schema requirements.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
packages/cellix/mongoose-seedwork/tests/integration/mongo-unit-of-work.integration.test.ts
Show resolved
Hide resolved
… for optional fields handling according to sourcery suggestion
…uration with dynamic API port
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consumers now import
getDirnameFromImportMetaUrlfrom@cellix/config-vitest, butsrc/index.tsonly re-exportsbaseConfig,nodeConfig, andcreateStorybookVitestConfig, so you should re-exportgetDirnameFromImportMetaUrlthere to avoid runtime/TS resolution errors. createStorybookVitestConfigno longer uses theadditionalCoverageExcludeoption when building thecoverage.excludearray, which is a behavior change from the previous version; if that option is still intended, merge it into the exclude list so existing per-package overrides continue to work.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consumers now import `getDirnameFromImportMetaUrl` from `@cellix/config-vitest`, but `src/index.ts` only re-exports `baseConfig`, `nodeConfig`, and `createStorybookVitestConfig`, so you should re-export `getDirnameFromImportMetaUrl` there to avoid runtime/TS resolution errors.
- `createStorybookVitestConfig` no longer uses the `additionalCoverageExclude` option when building the `coverage.exclude` array, which is a behavior change from the previous version; if that option is still intended, merge it into the exclude list so existing per-package overrides continue to work.
## Individual Comments
### Comment 1
<location path="packages/cellix/config-vitest/src/index.ts" line_range="1-3" />
<code_context>
export { baseConfig } from "./configs/base.config.ts";
export { nodeConfig } from "./configs/node.config.ts";
-export { createStorybookVitestConfig, type StorybookVitestConfigOptions } from "./configs/storybook.config.ts";
\ No newline at end of file
+export { createStorybookVitestConfig, type StorybookVitestConfigOptions } from "./configs/storybook.config.ts";
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `getDirnameFromImportMetaUrl` helper is not exported from the package entrypoint, causing imports from `@cellix/config-vitest` to fail.
Several Vitest configs (e.g. `apps/ui-community/vitest.config.ts`, `packages/cellix/ui-core/vitest.config.ts`, `packages/ocom/ui-components/vitest.config.ts`) import `getDirnameFromImportMetaUrl` from `@cellix/config-vitest`, but `src/index.ts` doesn’t re-export it, so those imports will be `undefined` at runtime.
Please add a re-export, for example:
```ts
export { getDirnameFromImportMetaUrl } from "./utils/dirname.ts";
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new getDirnameFromImportMetaUrl helper is used via
import { createStorybookVitestConfig, getDirnameFromImportMetaUrl } from '@cellix/config-vitest', but it isn’t exported frompackages/cellix/config-vitest/src/index.ts; add it to the index exports so consumers can import it without relying on deep paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new getDirnameFromImportMetaUrl helper is used via `import { createStorybookVitestConfig, getDirnameFromImportMetaUrl } from '@cellix/config-vitest'`, but it isn’t exported from `packages/cellix/config-vitest/src/index.ts`; add it to the index exports so consumers can import it without relying on deep paths.
## Individual Comments
### Comment 1
<location path="packages/cellix/config-vitest/src/index.ts" line_range="1-3" />
<code_context>
export { baseConfig } from "./configs/base.config.ts";
export { nodeConfig } from "./configs/node.config.ts";
-export { createStorybookVitestConfig, type StorybookVitestConfigOptions } from "./configs/storybook.config.ts";
\ No newline at end of file
+export { createStorybookVitestConfig, type StorybookVitestConfigOptions } from "./configs/storybook.config.ts";
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `getDirnameFromImportMetaUrl` helper is not exported but is used by consumers, which will cause build/runtime failures.
Several Vitest config files (e.g. `apps/ui-community/vitest.config.ts`, `packages/cellix/ui-core/vitest.config.ts`, `packages/ocom/ui-components/vitest.config.ts`) import `getDirnameFromImportMetaUrl` from `@cellix/config-vitest`, but `src/index.ts` does not export it. Please re-export it here, e.g.
```ts
export { getDirnameFromImportMetaUrl } from "./utils/dirname.ts";
```
(or via a `utils` barrel) so those configs continue to work after compilation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
getDirnameFromImportMetaUrlhelper is used from@cellix/config-vitestin multiple vitest configs but isn’t exported fromsrc/index.ts, so consumers importing it from the package entrypoint will fail to build—consider re-exporting it alongsidebaseConfig/nodeConfig/createStorybookVitestConfig.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `getDirnameFromImportMetaUrl` helper is used from `@cellix/config-vitest` in multiple vitest configs but isn’t exported from `src/index.ts`, so consumers importing it from the package entrypoint will fail to build—consider re-exporting it alongside `baseConfig`/`nodeConfig`/`createStorybookVitestConfig`.
## Individual Comments
### Comment 1
<location path="packages/cellix/config-vitest/src/index.ts" line_range="1-3" />
<code_context>
export { baseConfig } from "./configs/base.config.ts";
export { nodeConfig } from "./configs/node.config.ts";
-export { createStorybookVitestConfig, type StorybookVitestConfigOptions } from "./configs/storybook.config.ts";
\ No newline at end of file
+export { createStorybookVitestConfig, type StorybookVitestConfigOptions } from "./configs/storybook.config.ts";
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `getDirnameFromImportMetaUrl` helper isn’t re-exported from the package entrypoint, but is imported from `@cellix/config-vitest` elsewhere.
These vitest configs (`apps/ui-community/vitest.config.ts`, `packages/cellix/ui-core/vitest.config.ts`, `packages/ocom/ui-components/vitest.config.ts`) already import `getDirnameFromImportMetaUrl` from `@cellix/config-vitest`, so without re-exporting it from `src/index.ts` those imports will fail. Please add:
```ts
export { getDirnameFromImportMetaUrl } from './utils/dirname.ts';
```
to align the public API with existing usage.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai summary |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Upgrade the monorepo to Vite 8 and Vitest 4 with Istanbul coverage, updating shared Vitest configs, package tooling, and tests to align with the new ecosystem.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: