Feature Data sources #4786
Feature Data sources #4786
Conversation
📝 WalkthroughWalkthroughAdds comprehensive Data Sources functionality to the Feature Store: new routes and navigation entries; screen components for Data Sources list, table, rows, filters, details page, details view, tabs (Details, Feature views, Schema), and supporting utilities. Introduces typed API endpoints (getDataSources, getDataSourceByName), updated getFeatureViews signature to accept a data_source filter, new hooks (useFeatureStoreDataSources, useFeatureStoreDataSourceByName, useFeatureViews updates), expanded types (DataSource, DataSourceList, related types), mocks, unit tests, and extensive Cypress page objects and tests. Minor unrelated UI change in PopularTags skeleton layout. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
openshift-ci
bot
added
the
area/feature-store-modular-ui
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Nitpick comments (36)
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.ts (1)
29-36: Good addition: visitDataSources() mirrors established visit patternsMatches cy.visitWithLogin usage and devFeatureFlags convention.
Consider adding a navigateToDataSources() method (like Feature views/Entities) for symmetry in nav-based flows. If you want, I can draft it.
packages/feature-store/src/screens/metrics/PopularTags.tsx (2)
85-119: Avoid nested Gallery layouts in skeletonPopularTagsSkeleton returns a Gallery+GalleryItem, but renderContent also wraps it in a Gallery+GalleryItem, yielding nested galleries and extra spacing. Return just a Card from PopularTagsSkeleton.
Apply:
const PopularTagsSkeleton = () => { const fontSize = 'var(--pf-t--global--font--size--body--sm)'; return ( - <Gallery hasGutter> - <GalleryItem> - <Card> + <Card> <CardHeader> <CardTitle> <Flex gap={{ default: 'gapSm' }} alignItems={{ default: 'alignItemsCenter' }}> <TagIcon /> <Skeleton width="88%" height="1.5rem" /> </Flex> </CardTitle> </CardHeader> <CardBody> <List> <ListItem> <Skeleton width="50%" style={{ fontSize }} /> </ListItem> <ListItem> <Skeleton width="70%" style={{ fontSize }} /> </ListItem> <ListItem> <Skeleton width="70%" style={{ fontSize }} /> </ListItem> </List> </CardBody> <CardFooter> <Skeleton width="50%" style={{ fontSize }} /> </CardFooter> - </Card> - </GalleryItem> - </Gallery> + </Card> ); };
149-155: Drive skeleton count from the limit propKeeps loading layout consistent with the configured limit instead of hardcoding 4.
- <Gallery hasGutter> - {[1, 2, 3, 4].map((item: number) => ( - <GalleryItem key={item}> + <Gallery hasGutter> + {Array.from({ length: limit }).map((_, idx) => ( + <GalleryItem key={idx}> <PopularTagsSkeleton /> </GalleryItem> ))} </Gallery>packages/feature-store/src/types/featureView.ts (1)
164-171: Use camelCase in TS API surface; translate to snake_case only at request-build timeKeeping TS props/params camelCase avoids global lint disables elsewhere.
export type GetFeatureViews = ( opts: K8sAPIOptions, project?: string, entity?: string, featureService?: string, feature?: string, - data_source?: string, + dataSource?: string, ) => Promise<FeatureViewsList>;After renaming, ensure callers and the API layer (custom.ts) map dataSource -> data_source in the outgoing query.
packages/feature-store/src/apiHooks/useFeatureViews.tsx (2)
1-1: Remove file-wide camelcase disablePrefer camelCase in TS and translate at the API boundary.
-/* eslint-disable camelcase */
10-16: Rename prop to dataSource and pass through accordinglyKeeps lint happy and API surface consistent.
type UseFeatureViewsProps = { project?: string; entity?: string; featureService?: string; feature?: string; - data_source?: string; + dataSource?: string; }; const useFeatureViews = ({ project, entity, featureService, feature, - data_source, + dataSource, }: UseFeatureViewsProps): FetchStateObject<FeatureViewsList> => { const { api, apiAvailable } = useFeatureStoreAPI(); const call = React.useCallback<FetchStateCallbackPromise<FeatureViewsList>>( (opts) => { if (!apiAvailable) { return Promise.reject(new Error('API not yet available')); } - return api.getFeatureViews(opts, project, entity, featureService, feature, data_source); + return api.getFeatureViews(opts, project, entity, featureService, feature, dataSource); }, - [api, apiAvailable, project, entity, featureService, feature, data_source], + [api, apiAvailable, project, entity, featureService, feature, dataSource], );Ensure packages/feature-store/src/api/custom.ts accepts dataSource and converts it to data_source in the query string.
Also applies to: 18-24, 33-36
packages/feature-store/src/apiHooks/__tests__/useFeatureViews.test.tsx (1)
197-224: Add coverage for the new data_source filter and refetch behavior.Current tests only pad args; they don’t verify forwarding or dependency changes when data_source is provided/updated. Add a spec mirroring project/entity cases.
Apply this addition near the end of the file:
+ it('should refetch when data_source parameter changes', async () => { + useFeatureStoreAPIMock.mockReturnValue({ + api: { getFeatureViews: mockGetFeatureViews }, + apiAvailable: true, + } as unknown as ReturnType<typeof useFeatureStoreAPI>); + + mockGetFeatureViews.mockResolvedValue(mockFeatureViewsList); + + const renderResult = testHook(useFeatureViews)({ + project: 'project-1', + entity: 'entity-1', + data_source: 'source-1', + }); + + await renderResult.waitForNextUpdate(); + expect(mockGetFeatureViews).toHaveBeenCalledTimes(1); + expect(mockGetFeatureViews).toHaveBeenLastCalledWith( + expect.objectContaining({ signal: expect.any(AbortSignal) }), + 'project-1', + 'entity-1', + undefined, + undefined, + 'source-1', + ); + + renderResult.rerender({ project: 'project-1', entity: 'entity-1', data_source: 'source-2' }); + await renderResult.waitForNextUpdate(); + expect(mockGetFeatureViews).toHaveBeenCalledTimes(2); + expect(mockGetFeatureViews).toHaveBeenLastCalledWith( + expect.objectContaining({ signal: expect.any(AbortSignal) }), + 'project-1', + 'entity-1', + undefined, + undefined, + 'source-2', + ); + });Also applies to: 225-261, 263-300
frontend/src/__tests__/cypress/cypress/support/commands/odh.ts (1)
904-923: Typed intercepts for data_sources endpoints: good addition; consider optional query typing.Overloads are consistent and avoid hardcoded namespaces. If the API supports query filters/pagination (e.g., project, page, limit), expose them for stronger typing.
Apply this diff to both new overloads:
- options: { - path: { namespace: string; serviceName: string; apiVersion: string }; - }, + options: { + path: { namespace: string; serviceName: string; apiVersion: string }; + query?: { project?: string; page?: string; limit?: string }; + },packages/feature-store/src/apiHooks/useFeatureStoreDataSourceByName.ts (2)
22-28: Avoid error flicker when inputs aren’t readyRejecting when
projectordataSourceNameare undefined can surface transient errors during initial render. Prefer short‑circuiting to the same initial object you pass touseFetchso the UI stays in a loading state until inputs are available.- if (!project) { - return Promise.reject(new Error('Project is required')); - } - - if (!dataSourceName) { - return Promise.reject(new Error('Data source name is required')); - } + if (!project || !dataSourceName) { + // Defer fetch until required params are available + return Promise.resolve(initialData); + }Note: see the next diff to introduce
initialDataabove.
35-53: Hoist initial data and reuse it in the short‑circuit pathDefine the initial
DataSourceonce and reuse it both inuseFetchand the early return above.- return useFetch( - call, - { - type: 'BATCH_FILE', - timestampField: '', - createdTimestampColumn: '', - fileOptions: { - uri: '', - }, - name: '', - meta: { - createdTimestamp: '', - lastUpdatedTimestamp: '', - }, - featureDefinition: '', - project: '', - }, - { initialPromisePurity: true }, - ); + const initialData: DataSource = { + type: 'BATCH_FILE', + timestampField: '', + createdTimestampColumn: '', + fileOptions: { uri: '' }, + name: '', + meta: { createdTimestamp: '', lastUpdatedTimestamp: '' }, + featureDefinition: '', + project: '', + }; + + return useFetch(call, initialData, { initialPromisePurity: true });packages/feature-store/src/apiHooks/useFeastureStoreDataSources.ts (2)
1-1: Rename file: “Feasture” → “Feature”The filename is misspelled. Please rename the file to
useFeatureStoreDataSources.tsand update imports to match. This prevents future confusion and aligns with other hooks.
24-37: Expose pagination controls (optional)Hardcoding
limit: 50works, but consider acceptingpage/limitas hook params (with sensible defaults) so the table can drive server‑side pagination later.packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTable.tsx (1)
35-41: Stabilize row keys to avoid potential collisionsTo be safe across contexts and future changes, consider always including project in the key.
- key={currentProject ? cr.name : `${cr.name}-${cr.project ?? ''}`} + key={`${cr.project ?? ''}:${cr.name}`}packages/feature-store/src/screens/dataSources/DataSources.tsx (3)
9-9: Update import after hook file renameAfter renaming the hook file, update this import path.
-import useFeatureStoreDataSources from '../../apiHooks/useFeastureStoreDataSources'; +import useFeatureStoreDataSources from '../../apiHooks/useFeatureStoreDataSources';
11-14: Terminology consistencyThe rest of the UI uses “project.” Consider replacing “workspace” with “project” to keep terminology consistent.
36-44: Prevent empty-state flicker while loadingGate the
emptyprop byloadedso users don’t see a brief “No data sources” before data arrives.- empty={dataSources.dataSources.length === 0} + empty={dataSourcesLoaded && dataSources.dataSources.length === 0}packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsView.tsx (1)
70-75: Test ID naming nit: "join-key" reads as a different concept"Data source connector" stored under
data-source-join-keytest id can be confusing for future readers/tests. Consider renaming todata-source-connectorfor clarity. If you change this, remember to update the page object accordingly.packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsPage.tsx (1)
29-41: Empty state copy fitMinor: On a details page, "No data sources have been found in this project." could be confusing when a single resource is missing. Consider "This data source was not found in the current project." for the loading-empty state as well, or align with your pattern elsewhere.
packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceListView.tsx (1)
29-43: Redundant dynamic label for "type"
dataSourceTableFilterOptionsalready labelstypeas "Data source connector". Re-assigning it conditionally is redundant. Safe to keep, but you can remove the conditional block for simplicity.frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts (1)
4-7: Avoid text-based heading selectorsPrefer stable test ids over visible text to reduce brittleness; you already expose
findPageTitle(). Consider removingfindHeading().Apply this diff:
- findHeading() { - return cy.findByText('Data Source Details'); - }packages/feature-store/src/screens/dataSources/const.ts (2)
9-9: Ensure proper type annotation for sortable parameter.The
sortablecallback parameters should be properly typed for clarity.Consider adding explicit parameter types:
- sortable: (a: DataSource, b: DataSource): number => a.name.localeCompare(b.name), + sortable: (a: DataSource, b: DataSource): number => a.name.localeCompare(b.name),
84-88: Consider using an enum for tab constants.While the current object approach works, using a TypeScript enum would provide better type safety and prevent accidental mutations.
Consider refactoring to use an enum:
-export const DataSourceDetailsTab = { - DETAILS: 'Details', - FEATURE_VIEWS: 'Feature views', - SCHEMA: 'Schema', -}; +export enum DataSourceDetailsTab { + DETAILS = 'Details', + FEATURE_VIEWS = 'Feature views', + SCHEMA = 'Schema', +}packages/feature-store/src/api/custom.ts (1)
47-48: Remove eslint-disable comment in favor of proper naming.Using
camelCaseis the standard convention in JavaScript/TypeScript. Consider accepting thedataSourceparameter name instead ofdata_source.Consider using camelCase consistently:
- // eslint-disable-next-line camelcase - data_source?: string, + dataSource?: string,And update the query parameter construction:
- // eslint-disable-next-line camelcase - if (data_source) { - queryParams.push(`data_source=${encodeURIComponent(data_source)}`); + if (dataSource) { + queryParams.push(`data_source=${encodeURIComponent(dataSource)}`);frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts (1)
1-1: Remove unnecessary eslint-disable comment.The eslint-disable for camelcase shouldn't be needed if the code follows proper naming conventions.
-/* eslint-disable camelcase */ -packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTableRow.tsx (1)
95-95: Show “-” when project is unknownAvoid rendering undefined in the Project column.
- {renderTableCell('Project', dataSource.project)} + {renderTableCell('Project', dataSource.project ?? '-')}packages/feature-store/src/__mocks__/mockDataSources.ts (1)
88-99: Use slug-safe names in mocks to avoid brittle URL assertions“Loan table” contains a space and may complicate route assertions. Prefer slugified names unless tests explicitly cover encoding.
- name: 'Loan table', + name: 'loan_table',packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceTabs.tsx (2)
43-51: Tighten ARIA labels and test IDsThis table lists feature views, not data sources. Update labels for clarity and accessibility.
- <Table - aria-label="Data sources table" - data-testid="feature-view-data-sources-table" + <Table + aria-label="Feature views table" + data-testid="feature-views-table" variant="compact" >
86-90: Schema table labelsAlign ARIA label and test ID to the schema content.
- <Table - aria-label="Data sources table" - data-testid="feature-view-data-sources-table" + <Table + aria-label="Request schema table" + data-testid="data-source-schema-table" variant="compact" >packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSourceByName.spec.ts (3)
14-38: Test factory is helpful; consider adding STREAM_KAFKA and PUSH_SOURCE variants.Broaden mockDataSource coverage to all discriminants to catch shape regressions early.
const mockDataSource = (partial?: Partial<DataSource>): DataSource => ({ type: 'BATCH_FILE', @@ featureDefinition: 'test-feature-def', ...partial, }); + + // Example additional fixtures: + const kafkaDataSource = () => + mockDataSource({ + type: 'STREAM_KAFKA', + kafkaOptions: { + kafkaBootstrapServers: 'kafka:9092', + topic: 'events', + messageFormat: { jsonFormat: { schemaJson: '{}' } }, + }, + watermarkDelayThreshold: '5m', // if modeled on the server payload + }); + + const pushDataSource = () => mockDataSource({ type: 'PUSH_SOURCE' });
81-98: Great: asserting AbortSignal is threaded; add an abort-on-change/unmount test.Verify in-flight requests are aborted on parameter change and component unmount to avoid leaks.
@@ describe('useFeatureStoreDataSourceByName', () => { it('should refetch when project parameter changes', async () => { @@ }); + + it('aborts in-flight request on parameter change', async () => { + useFeatureStoreAPIMock.mockReturnValue({ + api: { getDataSourceByName: mockGetDataSourceByName }, + apiAvailable: true, + } as unknown as ReturnType<typeof useFeatureStoreAPI>); + const aborts: AbortSignal[] = []; + mockGetDataSourceByName.mockImplementation(async (opts: { signal: AbortSignal }) => { + aborts.push(opts.signal); + // Simulate never-resolving call to test abort + return new Promise<DataSource>(() => {}); + }); + const renderResult = testHook(useFeatureStoreDataSourceByName)('p1', 'ds1'); + // Trigger param change to cause previous fetch to be aborted + renderResult.rerender('p2', 'ds1'); + // Give the hook a tick + await Promise.resolve(); + expect(aborts.length).toBe(2); + // The first should be aborted + expect(aborts[0].aborted).toBe(true); + }); + + it('aborts in-flight request on unmount', async () => { + useFeatureStoreAPIMock.mockReturnValue({ + api: { getDataSourceByName: mockGetDataSourceByName }, + apiAvailable: true, + } as unknown as ReturnType<typeof useFeatureStoreAPI>); + let capturedSignal: AbortSignal | undefined; + mockGetDataSourceByName.mockImplementation(async (opts: { signal: AbortSignal }) => { + capturedSignal = opts.signal; + return new Promise<DataSource>(() => {}); + }); + const renderResult = testHook(useFeatureStoreDataSourceByName)('p1', 'ds1'); + renderResult.unmount(); + await Promise.resolve(); + expect(capturedSignal?.aborted).toBe(true); + });Also applies to: 268-288, 311-320, 341-353
120-124: Nit: compare error messages, not Error instances.Safer across wrapped/rethrown errors.
- expect(renderResult.result.current.error).toEqual(new Error('API not yet available')); + expect(renderResult.result.current.error?.message).toBe('API not yet available');(Apply similarly for “Project is required”, “Data source name is required”, etc.)
Also applies to: 146-149, 168-173, 192-197
packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSources.spec.ts (2)
112-132: Good: asserts AbortSignal propagation; add abort-on-change/unmount tests.Mirror the by-name tests to ensure list fetches are also cancelled correctly.
@@ describe('useFeatureStoreDataSources', () => { it('should refetch when project parameter changes', async () => { @@ }); + + it('aborts in-flight list request on project change', async () => { + useFeatureStoreAPIMock.mockReturnValue({ + api: { getDataSources: mockGetDataSources }, + apiAvailable: true, + } as unknown as ReturnType<typeof useFeatureStoreAPI>); + const signals: AbortSignal[] = []; + mockGetDataSources.mockImplementation(async (opts: { signal: AbortSignal }) => { + signals.push(opts.signal); + return new Promise<DataSourceList>(() => {}); + }); + const r = testHook(useFeatureStoreDataSources)('p1'); + r.rerender('p2'); + await Promise.resolve(); + expect(signals[0].aborted).toBe(true); + }); + + it('aborts in-flight list request on unmount', async () => { + useFeatureStoreAPIMock.mockReturnValue({ + api: { getDataSources: mockGetDataSources }, + apiAvailable: true, + } as unknown as ReturnType<typeof useFeatureStoreAPI>); + let s: AbortSignal | undefined; + mockGetDataSources.mockImplementation(async (opts: { signal: AbortSignal }) => { + s = opts.signal; + return new Promise<DataSourceList>(() => {}); + }); + const r = testHook(useFeatureStoreDataSources)(); + r.unmount(); + await Promise.resolve(); + expect(s?.aborted).toBe(true); + });Also applies to: 144-164, 255-273, 318-323
218-244: Optional: test empty-string project treated as undefined.Matches behavior in the by-name tests; prevents accidental double-fetch with ''.
+ it('treats empty string project as undefined', async () => { + useFeatureStoreAPIMock.mockReturnValue({ + api: { getDataSources: mockGetDataSources }, + apiAvailable: true, + } as unknown as ReturnType<typeof useFeatureStoreAPI>); + mockGetDataSources.mockResolvedValue(mockDataSourceListData); + const r = testHook(useFeatureStoreDataSources)(''); + await r.waitForNextUpdate(); + expect(mockGetDataSources).toHaveBeenCalledWith( + expect.objectContaining({ signal: expect.any(AbortSignal) }), + undefined, + ); + });packages/feature-store/src/types/dataSources.ts (3)
51-69: Consider a strict discriminated union for DataSource.Encoding per-type required fields at the type level improves DX and prevents partial objects.
-export type DataSource = { - type: 'BATCH_FILE' | 'REQUEST_SOURCE' | 'STREAM_KAFKA' | 'PUSH_SOURCE'; - timestampField?: string; - createdTimestampColumn?: string; - fileOptions?: FileOptions; - kafkaOptions?: KafkaOptions; - requestDataOptions?: RequestDataOptions; - batchSource?: BatchSource; - name: string; - description?: string; - tags?: Record<string, string>; - owner?: string; - meta: { createdTimestamp: string; lastUpdatedTimestamp: string }; - project?: string; - featureDefinition?: string; -}; +export type DataSource = + | ({ + type: 'BATCH_FILE'; + fileOptions: FileOptions; + timestampField?: string; + createdTimestampColumn?: string; + } & CommonDataSourceFields) + | ({ + type: 'REQUEST_SOURCE'; + requestDataOptions: RequestDataOptions; + } & CommonDataSourceFields) + | ({ type: 'STREAM_KAFKA'; kafkaOptions: KafkaOptions } & CommonDataSourceFields) + | ({ type: 'PUSH_SOURCE' } & CommonDataSourceFields); + +type CommonDataSourceFields = { + name: string; + description?: string; + tags?: Record<string, string>; + owner?: string; + meta: { createdTimestamp: string; lastUpdatedTimestamp: string }; + project?: string; + featureDefinition?: string; +};
4-9: Schema fields: ensure parity with backend.jsonFormat.schemaJson optional here (FileFormat) but required in KafkaOptions.messageFormat; verify server responses align.
- If schemaJson is always present, consider making it required for both.
- Otherwise document optionality with comments.
Also applies to: 16-25
11-14: Optional: make FileOptions.uri required (if API guarantees it).If uri is always present for BATCH_FILE, tightening improves safety.
-export type FileOptions = { - fileFormat?: FileFormat; - uri?: string; -}; +export type FileOptions = { + fileFormat?: FileFormat; + uri: string; +};Also applies to: 51-56
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (30)
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts(1 hunks)frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts(1 hunks)frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.ts(2 hunks)frontend/src/__tests__/cypress/cypress/support/commands/odh.ts(2 hunks)frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts(1 hunks)packages/feature-store/extensions.ts(1 hunks)packages/feature-store/src/FeatureStoreRoutes.tsx(2 hunks)packages/feature-store/src/__mocks__/mockDataSources.ts(3 hunks)packages/feature-store/src/api/custom.ts(4 hunks)packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSourceByName.spec.ts(1 hunks)packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSources.spec.ts(1 hunks)packages/feature-store/src/apiHooks/__tests__/useFeatureViews.test.tsx(7 hunks)packages/feature-store/src/apiHooks/useFeastureStoreDataSources.ts(1 hunks)packages/feature-store/src/apiHooks/useFeatureStoreAPIState.tsx(2 hunks)packages/feature-store/src/apiHooks/useFeatureStoreDataSourceByName.ts(1 hunks)packages/feature-store/src/apiHooks/useFeatureViews.tsx(3 hunks)packages/feature-store/src/screens/dataSources/DataSources.tsx(1 hunks)packages/feature-store/src/screens/dataSources/__tests__/utils.spec.ts(1 hunks)packages/feature-store/src/screens/dataSources/const.ts(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsPage.tsx(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsView.tsx(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceTabs.tsx(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceListView.tsx(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTable.tsx(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTableRow.tsx(1 hunks)packages/feature-store/src/screens/dataSources/utils.ts(1 hunks)packages/feature-store/src/screens/metrics/PopularTags.tsx(2 hunks)packages/feature-store/src/types/dataSources.ts(1 hunks)packages/feature-store/src/types/featureView.ts(1 hunks)packages/feature-store/src/types/global.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/cypress-e2e.mdc)
frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx}: Don't create TypeScript interfaces for test data; use direct object access patterns.
All linting errors must be fixed: use object destructuring, proper formatting, no unused variables/imports, no any types unless necessary.
Never hardcode namespaces in tests or utilities; always derive namespaces from test variables or environment variables.
Files:
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.tsfrontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.tsfrontend/src/__tests__/cypress/cypress/support/commands/odh.ts
frontend/src/__tests__/cypress/cypress/pages/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/cypress-e2e.mdc)
frontend/src/__tests__/cypress/cypress/pages/**/*.ts: All selectors must be encapsulated in page object methods; page objects should return Cypress chainables or other page objects.
All page object methods named find... must only return Cypress chainables for elements, never perform actions.
All actions (e.g., .click(), .type()) must be performed in the test itself, not inside the page object method.
Always add data-testid attributes to the UI and corresponding find... methods in page objects whenever a test needs to interact with or validate an element.
Files:
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts
🧠 Learnings (14)
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : Use .navigate() methods in page objects for navigation instead of cy.visit() in tests.
Applied to files:
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/pages/**/*.ts : Always add data-testid attributes to the UI and corresponding find... methods in page objects whenever a test needs to interact with or validate an element.
Applied to files:
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.tsfrontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts
📚 Learning: 2025-07-24T07:46:43.192Z
Learnt from: manosnoam
PR: opendatahub-io/odh-dashboard#4511
File: frontend/src/__tests__/cypress/cypress/tests/e2e/lmEval/testLMEvalStatic.cy.ts:56-56
Timestamp: 2025-07-24T07:46:43.192Z
Learning: In ODH Dashboard Cypress tests, cy.visitWithLogin() is the established and consistent pattern used across all tests for navigation with authentication. This is different from the general guideline about using .navigate() methods in page objects, and cy.visitWithLogin() should be considered an accepted pattern in this codebase.
Applied to files:
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : MANDATORY: Use page objects for ALL UI interactions; NEVER use cy.findByTestId(), cy.findByRole(), or cy.get() directly in test files.
Applied to files:
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.tsfrontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts
📚 Learning: 2025-08-07T16:03:53.439Z
Learnt from: hardengl
PR: opendatahub-io/odh-dashboard#4642
File: frontend/src/__tests__/cypress/cypress/tests/e2e/nim/testEnableNIM.cy.ts:91-93
Timestamp: 2025-08-07T16:03:53.439Z
Learning: In ODH Dashboard Cypress tests, cy.wait() with fixed delays can be appropriate within retry/polling loops where you need to give time for resources to become available or pages to fully render between attempts, as demonstrated in the NIM enablement test's card detection loop.
Applied to files:
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : Use descriptive file names matching feature area, e.g., testFeatureName.cy.ts for main functionality.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/fixtures/e2e/*.yaml : Store test-specific data in YAML fixture files with descriptive names like testFeatureName.yaml.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx} : Don't create TypeScript interfaces for test data; use direct object access patterns.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.tsfrontend/src/__tests__/cypress/cypress/support/commands/odh.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/ : Group related tests in feature directories under tests/e2e.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx} : Never hardcode namespaces in tests or utilities; always derive namespaces from test variables or environment variables.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts
📚 Learning: 2025-06-19T20:38:32.485Z
Learnt from: christianvogt
PR: opendatahub-io/odh-dashboard#4381
File: frontend/src/__tests__/cypress/tsconfig.json:9-9
Timestamp: 2025-06-19T20:38:32.485Z
Learning: In the ODH Dashboard project, the `frontend/src/__tests__/cypress/tsconfig.json` file intentionally has an empty `files` array to disable type checking for Cypress test files. This is part of the monorepo structure where Cypress was separated into its own package but type checking is deliberately disabled for it.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : Delete test projects, PVCs, and other resources after tests using after() hooks.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx} : All linting errors must be fixed: use object destructuring, proper formatting, no unused variables/imports, no any types unless necessary.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/pages/**/*.ts : All selectors must be encapsulated in page object methods; page objects should return Cypress chainables or other page objects.
Applied to files:
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.tsfrontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts
🧬 Code graph analysis (23)
packages/feature-store/src/screens/dataSources/__tests__/utils.spec.ts (1)
packages/feature-store/src/screens/dataSources/utils.ts (1)
getDataSourceConnectorType(51-64)
packages/feature-store/src/apiHooks/useFeastureStoreDataSources.ts (3)
frontend/src/utilities/useFetch.ts (1)
FetchStateObject(37-43)packages/feature-store/src/types/dataSources.ts (1)
DataSourceList(71-75)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreAPI(83-90)
packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSources.spec.ts (3)
packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreAPI(83-90)packages/feature-store/src/types/dataSources.ts (1)
DataSourceList(71-75)frontend/packages/llama-stack-modular-ui/frontend/src/__tests__/unit/testUtils/hooks.ts (1)
testHook(106-127)
packages/feature-store/src/FeatureStoreRoutes.tsx (1)
packages/feature-store/src/types/featureView.ts (1)
DataSources(29-31)
packages/feature-store/src/types/global.ts (1)
packages/feature-store/src/types/dataSources.ts (2)
GetDataSources(77-77)GetDataSourceByName(78-82)
packages/feature-store/src/apiHooks/useFeatureStoreDataSourceByName.ts (2)
frontend/src/utilities/useFetch.ts (1)
FetchStateObject(37-43)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreAPI(83-90)
packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTable.tsx (2)
packages/feature-store/src/types/global.ts (1)
FeatureStoreRelationship(62-71)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreProject(92-111)
frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts (13)
frontend/src/__mocks__/mockDscStatus.ts (1)
mockDscStatus(16-155)frontend/src/__mocks__/mockDashboardConfig.ts (1)
mockDashboardConfig(50-267)frontend/src/__mocks__/mockProjectK8sResource.ts (1)
mockProjectK8sResource(18-56)packages/feature-store/src/__mocks__/mockFeatureStore.ts (1)
mockFeatureStore(3-169)frontend/src/api/models/k8s.ts (1)
ServiceModel(81-85)packages/feature-store/src/__mocks__/mockFeatureStoreService.ts (1)
mockFeatureStoreService(12-75)packages/feature-store/src/__mocks__/mockFeatureStoreProject.ts (1)
mockFeatureStoreProject(3-14)packages/feature-store/src/__mocks__/mockDataSources.ts (1)
mockDataSource(4-16)packages/feature-store/src/__mocks__/mockFeatureViews.ts (1)
mockFeatureView(4-124)frontend/src/__tests__/cypress/cypress/utils/mockUsers.ts (1)
asClusterAdminUser(24-26)frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.ts (1)
featureStoreGlobal(208-208)frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts (2)
featureDataSourcesTable(191-193)dataSourceDetailsPage(195-195)frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts (1)
featureDataSourceDetails(174-176)
packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsView.tsx (3)
packages/feature-store/src/const.ts (1)
hasContent(22-22)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreProject(92-111)packages/feature-store/src/routes.ts (1)
featureDataSourceRoute(28-31)
packages/feature-store/src/screens/dataSources/utils.ts (3)
packages/feature-store/src/utils/filterUtils.ts (1)
createFeatureStoreFilterUtils(307-315)packages/feature-store/src/types/global.ts (1)
FeatureStoreRelationship(62-71)packages/feature-store/src/screens/dataSources/const.ts (1)
dataSourceTableFilterKeyMapping(65-73)
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts (1)
frontend/src/__tests__/cypress/cypress/pages/components/table.ts (1)
TableRow(3-26)
packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsPage.tsx (1)
packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreProject(92-111)
packages/feature-store/src/screens/dataSources/DataSources.tsx (2)
packages/feature-store/src/types/featureView.ts (1)
DataSources(29-31)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreProject(92-111)
packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSourceByName.spec.ts (2)
packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreAPI(83-90)frontend/packages/llama-stack-modular-ui/frontend/src/__tests__/unit/testUtils/hooks.ts (1)
testHook(106-127)
packages/feature-store/src/screens/dataSources/const.ts (1)
frontend/src/components/table/types.ts (1)
SortableData(5-30)
packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTableRow.tsx (4)
packages/feature-store/src/types/global.ts (1)
FeatureStoreRelationship(62-71)packages/feature-store/src/routes.ts (2)
featureDataSourceRoute(28-31)featureViewRoute(13-14)packages/feature-store/src/screens/dataSources/utils.ts (1)
getDataSourceConnectorType(51-64)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreProject(92-111)
packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceListView.tsx (5)
packages/feature-store/src/types/dataSources.ts (1)
DataSourceList(71-75)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreProject(92-111)packages/feature-store/src/screens/dataSources/utils.ts (2)
getDataSourceConnectorType(51-64)applyDataSourceFilters(12-49)packages/feature-store/src/screens/dataSources/const.ts (1)
dataSourceTableFilterOptions(74-82)packages/feature-store/src/components/FeatureStoreToolbar.tsx (1)
FeatureStoreToolbar(104-155)
packages/feature-store/src/types/dataSources.ts (3)
packages/feature-store/src/types/global.ts (4)
FileOptions(37-42)BatchSource(50-60)FeatureStorePagination(11-18)FeatureStoreRelationship(62-71)packages/feature-store/src/types/featureView.ts (1)
DataSource(11-27)frontend/src/k8sTypes.ts (1)
K8sAPIOptions(292-296)
packages/feature-store/src/__mocks__/mockDataSources.ts (1)
packages/feature-store/src/types/dataSources.ts (1)
DataSourceList(71-75)
frontend/src/__tests__/cypress/cypress/support/commands/odh.ts (1)
packages/feature-store/src/types/dataSources.ts (1)
DataSourceList(71-75)
packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceTabs.tsx (3)
packages/feature-store/src/routes.ts (2)
featureViewRoute(13-14)featureServiceRoute(19-23)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreProject(92-111)packages/feature-store/src/screens/dataSources/const.ts (1)
DataSourceDetailsTab(84-88)
packages/feature-store/src/api/custom.ts (5)
frontend/src/k8sTypes.ts (1)
K8sAPIOptions(292-296)packages/feature-store/src/types/dataSources.ts (1)
DataSourceList(71-75)packages/feature-store/src/const.ts (1)
FEATURE_STORE_API_VERSION(1-1)packages/feature-store/src/api/errorUtils.ts (1)
handleFeatureStoreFailures(7-30)packages/feature-store/src/types/featureView.ts (1)
DataSource(11-27)
packages/feature-store/src/apiHooks/useFeatureStoreAPIState.tsx (1)
packages/feature-store/src/api/custom.ts (2)
getDataSources(265-275)getDataSourceByName(277-285)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress-Setup
- GitHub Check: Lint
🔇 Additional comments (40)
packages/feature-store/extensions.ts (1)
73-85: Nav item for “Data sources” looks correctIDs, href/path, and flag gating align with existing entries. No issues found.
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureStoreGlobal.ts (1)
158-161: Verified title casing; no changes needed
The page title is defined asconst title = 'Data Sources'in packages/feature-store/src/screens/dataSources/DataSources.tsx (line 11), so the test’s assertion is correct.packages/feature-store/src/apiHooks/__tests__/useFeatureViews.test.tsx (1)
62-69: API arity update reflected correctly in assertions.The added trailing undefined accommodates the new data_source arg; mocks and expectations line up.
Also applies to: 98-105, 135-142, 241-247, 254-260, 279-285, 291-298
frontend/src/__tests__/cypress/cypress/support/commands/odh.ts (1)
16-16: Type imports for DataSource endpoints look correct.Matches packages/feature-store/src/types/dataSources exports.
packages/feature-store/src/FeatureStoreRoutes.tsx (2)
16-17: Routes for Data Sources wired correctly.List and details routes align with the new pages; path patterns are consistent with other FS resources.
Also applies to: 60-64
62-64: Confirm tab routing needs; add wildcard if nested routes are used.If DataSourceDetailsPage renders nested routes for tabs (e.g., /schema), append /* to the details path.
- path="dataSources/:fsProjectName/:dataSourceName" + path="dataSources/:fsProjectName/:dataSourceName/*"packages/feature-store/src/types/global.ts (2)
9-9: FeatureStoreAPIs extended with data source methods — looks good.Types match the dataSources.ts signatures.
Also applies to: 89-91
73-91: getDataSources/getDataSourceByName are correctly wired in useFeatureStoreAPIState Both APIs have corresponding type exports, hook implementations, and are injected in the returned object of useFeatureStoreAPIState.packages/feature-store/src/screens/dataSources/__tests__/utils.spec.ts (1)
1-33: Solid mapping tests; covers known, unknown, and undefined types.Test suite is concise and aligned with utils implementation.
packages/feature-store/src/apiHooks/useFeatureStoreDataSourceByName.ts (1)
16-33: LGTM overallHook wiring, dependencies, and API usage look correct.
packages/feature-store/src/apiHooks/useFeastureStoreDataSources.ts (1)
13-21: LGTMClean
useFetchintegration with proper API gating and memoized call.packages/feature-store/src/apiHooks/useFeatureStoreAPIState.tsx (1)
20-22: LGTM: API surface extension is consistentNew endpoints are correctly imported and exposed via
createAPI. Dependency list for the factory is appropriate.Also applies to: 47-49
packages/feature-store/src/screens/dataSources/DataSources.tsx (1)
53-54: LGTMPage wiring, error/loaded handling, and header navigator are all set up correctly.
packages/feature-store/src/screens/dataSources/utils.ts (1)
51-64: Connector-type mapping looks goodMapping is clear and matches UI expectations.
packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceListView.tsx (1)
19-27: Project backfill LGTMBackfilling
projectfrom context when missing is a sensible default and localized here.frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts (1)
16-39: General: page-object structure is solidSelectors are encapsulated via
find...methods returning chainables; good alignment with the page-object pattern and test guidelines.Also applies to: 60-87, 120-172
packages/feature-store/src/screens/dataSources/const.ts (2)
1-3: LGTM! Import statements are appropriate.The imports are correctly structured and use the expected internal paths for the
SortableDatatype andDataSourcetype.
65-83: LGTM! Filter configuration exports are well-structured.The filter key mapping and options provide a clean interface for data source filtering with appropriate human-readable labels.
packages/feature-store/src/api/custom.ts (3)
17-17: LGTM! Import properly added for data source types.The import for
DataSourceandDataSourceListtypes is correctly added.
265-275: LGTM! Data sources endpoint implementation is consistent.The
getDataSourcesfunction follows the established pattern with proper endpoint construction, query parameter handling, and error handling throughhandleFeatureStoreFailures.
277-285: LGTM! Data source by name endpoint is properly implemented.The
getDataSourceByNamefunction correctly encodes parameters and includes relationships in the response.frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts (7)
84-140: LGTM! Mock intercept for all data sources is comprehensive.The mock data includes proper pagination, relationships, and variety of data source types for testing.
186-203: LGTM! Data source details intercept is well-structured.The mock properly returns detailed data source information including metadata timestamps.
255-322: Test suite for all projects view is comprehensive.The tests properly validate table display, project column visibility, navigation to feature views, and breadcrumb navigation. Good coverage of the all-projects context.
390-406: Good error handling test for 404 scenario.The test properly validates the error message display when a data source is not found.
425-499: Excellent test coverage for Feature Views tab.The test suite thoroughly covers:
- Display of feature views
- Empty state handling
- Lazy loading behavior (API called only when tab is clicked)
- Navigation to feature view details
579-648: LGTM! Schema tab tests validate conditional display correctly.The tests properly verify that the Schema tab only appears for REQUEST_SOURCE data sources and displays the schema correctly when present.
21-25: Ignore hardcoding concern—definingk8sNamespace,fsName, and project names as top-of-file constants is the established Cypress test pattern across all mocked featureStore tests.Likely an incorrect or invalid review comment.
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts (3)
5-7: No action needed:data-testidexists Verified thatdata-testid="feature-store-data-sources-table"is present in packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTable.tsx (line 27).
191-195: Page object instantiation doesn't match constructor pattern.The instantiation passes a selector to the constructor, but it should match the actual table element.
Ensure the instantiation matches the actual implementation:
export const featureDataSourcesTable = new FeatureDataSourcesTable(() => - cy.findByTestId('feature-store-data-sources-table'), + cy.findByTestId('feature-store-data-sources-page'), );Or update the class to properly use the passed context.
⛔ Skipped due to learnings
Learnt from: CR PR: opendatahub-io/odh-dashboard#0 File: .cursor/rules/cypress-e2e.mdc:0-0 Timestamp: 2025-07-21T11:49:57.416Z Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : MANDATORY: Use page objects for ALL UI interactions; NEVER use cy.findByTestId(), cy.findByRole(), or cy.get() directly in test files.Learnt from: CR PR: opendatahub-io/odh-dashboard#0 File: .cursor/rules/cypress-e2e.mdc:0-0 Timestamp: 2025-07-21T11:49:57.416Z Learning: Applies to frontend/src/__tests__/cypress/cypress/pages/**/*.ts : All selectors must be encapsulated in page object methods; page objects should return Cypress chainables or other page objects.Learnt from: CR PR: opendatahub-io/odh-dashboard#0 File: .cursor/rules/cypress-e2e.mdc:0-0 Timestamp: 2025-07-21T11:49:57.416Z Learning: Applies to frontend/src/__tests__/cypress/cypress/pages/**/*.ts : Always add data-testid attributes to the UI and corresponding find... methods in page objects whenever a test needs to interact with or validate an element.
33-34: Assert toolbar existence via data-testid in Cypress test
Add a.should('exist')assertion on the toolbar returned byFeatureDataSourceToolbarinfrontend/src/__tests__/cypress/pages/featureStore/featureDataSource.ts.⛔ Skipped due to learnings
Learnt from: CR PR: opendatahub-io/odh-dashboard#0 File: .cursor/rules/cypress-e2e.mdc:0-0 Timestamp: 2025-07-21T11:49:57.416Z Learning: Applies to frontend/src/__tests__/cypress/cypress/pages/**/*.ts : Always add data-testid attributes to the UI and corresponding find... methods in page objects whenever a test needs to interact with or validate an element.Learnt from: CR PR: opendatahub-io/odh-dashboard#0 File: .cursor/rules/cypress-e2e.mdc:0-0 Timestamp: 2025-07-21T11:49:57.416Z Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : MANDATORY: Use page objects for ALL UI interactions; NEVER use cy.findByTestId(), cy.findByRole(), or cy.get() directly in test files.packages/feature-store/src/__mocks__/mockDataSources.ts (2)
4-16: Nice factory helperThe mockDataSource factory with partial overrides is clean and test-friendly.
110-117: No changes needed for pagination key casing
The mock pagination object uses the same snake_case keys as FeatureStorePagination (total_count, total_pages, has_next, has_previous), so the shape already aligns.packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSourceByName.spec.ts (2)
7-12: Mocking strategy looks solid.Good isolation of useFeatureStoreAPI and clear separation of the API function under test.
230-256: Stability assertion is nice.Good signal that rerender with identical params is pure and memoized where expected.
packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSources.spec.ts (2)
135-164: Nice parameterized test.Ensures project is forwarded; shape assertions look good.
358-388: Empty list case covered well.Pagination defaults validated; good guard against UI assumptions.
packages/feature-store/src/types/dataSources.ts (3)
71-83: API surface types LGTM.Signatures match test expectations; opts-first calling convention is consistent.
11-14: Ignore global type reuse – definitions diverge
TheFileOptionsandBatchSourcein dataSources.ts have different shapes and optionality than those in global.ts, so they aren’t duplicates and shouldn’t be aliased or re-exported.Likely an incorrect or invalid review comment.
1-1: Import path is correct:@odh-dashboard/internal/k8sTypesis the intended shared module forK8sAPIOptionsand is consistently used across all packages.
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts
Outdated
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts
Outdated
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts
Outdated
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts
Outdated
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts
Outdated
Show resolved
Hide resolved
packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTable.tsx
Show resolved
Hide resolved
...ges/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTableRow.tsx
Show resolved
Hide resolved
...ges/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTableRow.tsx
Show resolved
Hide resolved
...ges/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTableRow.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/feature-store/src/utils/lineageDataConverter.ts (2)
184-191: Include stream data source in objectRef→nodeId mapping.If relationships start emitting a distinct 'streamDataSource' type, edges will fail to resolve.
Apply:
case 'dataSource': case 'batchDataSource': + case 'streamDataSource': case 'pushDataSource': case 'requestDataSource': return `datasource-${objectRef.name}`;
33-46: Explicitly map all DataSource types to labels and entityType values
- STREAM_KAFKA is currently falling through to “Request” and must be labeled “Stream” with entityType ‘stream_data_source’.
- REQUEST_SOURCE should map to “Request” with entityType ‘request_data_source’.
- BATCH_FILE and PUSH_SOURCE remain “Batch”/‘batch_data_source’ and “Push”/‘push_data_source’ respectively.
Ensure these mappings align with any existing display-label or styling logic elsewhere.
🧹 Nitpick comments (1)
packages/feature-store/src/utils/lineageDataConverter.ts (1)
108-111: Avoid Array.prototype.toSorted unless you’re polyfilling ES2023.Some environments/TS lib targets may not support
toSorted. If not guaranteed, use.sorton a cloned array.Apply:
- const sortedLayers = Object.keys(layerGroups) - .map(Number) - .toSorted((a, b) => a - b); + const sortedLayers = Object.keys(layerGroups) + .map(Number) + .sort((a, b) => a - b);Please confirm tsconfig lib and browserslist/polyfills cover
toSorted; otherwise adopt the change above.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/feature-store/src/utils/lineageDataConverter.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T13:29:21.949Z
Learnt from: claudialphonse78
PR: opendatahub-io/odh-dashboard#4759
File: packages/feature-store/src/screens/metrics/utils.ts:94-115
Timestamp: 2025-08-26T13:29:21.949Z
Learning: In the Feature Store metrics utils.ts, the getResourceRoute function expects mapped display labels (from resourceTypeMap values) as input for the resourceType parameter, not raw API resource type strings. The switch cases should compare against resourceTypeMap values, not raw type strings like 'feature_views'.
Applied to files:
packages/feature-store/src/utils/lineageDataConverter.ts
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsView.tsx (1)
48-52: Broaden helpers to handle numeric timestamps (type-safety and correct styling).
getDisabledClassNameandgetContentValueare typed for strings, but you pass numeric timestamps at Lines 107 and 119. Broaden types and logic to avoid TS errors and misclassification.-const getDisabledClassName = (value: string | undefined) => - value && hasContent(value) ? '' : text.textColorDisabled; +const getDisabledClassName = (value?: string | number | null) => + value !== undefined && + value !== null && + (typeof value === 'number' || (typeof value === 'string' && hasContent(value))) + ? '' + : text.textColorDisabled; -const getContentValue = (value: string | undefined, fallback: string) => - value && hasContent(value) ? value : fallback; +const getContentValue = (value: string | number | undefined | null, fallback: string) => + value !== undefined && + value !== null && + (typeof value === 'number' || (typeof value === 'string' && hasContent(value))) + ? String(value) + : fallback;
🧹 Nitpick comments (7)
packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsView.tsx (2)
103-116: Consistent fallback phrasing for timestamps.Minor copy nit: consider using “No … date” for both fallbacks to keep UX consistent.
- 'No last modified' + 'No last modified date'
77-96: Redundant disabled class on File URL block.This item only renders when URI is truthy, so the disabled class will never apply. You can drop it.
- testId="data-source-file-url" - className={getDisabledClassName(dataSource.fileOptions.uri)} + testId="data-source-file-url"packages/feature-store/src/apiHooks/useFeatureStoreDataSources.ts (5)
13-22: Prefer returning INITIAL_DATA over rejecting when API is unavailableAvoids transient error states and aligns with initialPromisePurity; also de-dupes the initial shape.
import { DataSourceList } from '../types/dataSources'; +const INITIAL_DATA: DataSourceList = { + dataSources: [], + pagination: { + page: 1, + limit: 50, + total_count: 0, + total_pages: 0, + has_next: false, + has_previous: false, + }, + relationships: {}, +}; + const useFeatureStoreDataSources = (project?: string): FetchStateObject<DataSourceList> => { const { api, apiAvailable } = useFeatureStoreAPI(); const call = React.useCallback<FetchStateCallbackPromise<DataSourceList>>( (opts) => { if (!apiAvailable) { - return Promise.reject(new Error('API not yet available')); + return Promise.resolve(INITIAL_DATA); } return api.getDataSources(opts, project); }, [api, apiAvailable, project], ); return useFetch( call, - { - dataSources: [], - pagination: { - page: 1, - limit: 50, - total_count: 0, - total_pages: 0, - has_next: false, - has_previous: false, - }, - relationships: {}, - }, + INITIAL_DATA, { initialPromisePurity: true }, ); };Also applies to: 24-39
13-22: Type the opts parameter for end-to-end safetyAnnotate opts with the list/query options type used across FS APIs (pagination, filters, etc.) to prevent accidental misuse.
If there’s a shared type (e.g., FeatureStoreListOptions), apply it here and in api.getDataSources signature for consistency.
24-39: Centralize pagination defaults50-limit and initial pagination appear in several hooks. Consider a shared constant to keep UX consistent and avoid drift.
I can extract a FeatureStore.DEFAULT_PAGINATION and update related hooks if you want.
1-1: Narrow the eslint-disable scopePrefer a scoped disable or rule comment explaining snake_case from backend, rather than file-wide disable.
Example:
- Keep the file-level disable but add reason: /* eslint-disable camelcase -- API returns snake_case fields */
- Or disable next-line only around pagination literals.
42-42: Also export a named symbol for tree-shaking/import consistencyNon-breaking and helps align with other hooks.
export default useFeatureStoreDataSources; +export { useFeatureStoreDataSources };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSource.ts(1 hunks)frontend/src/__tests__/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts(1 hunks)frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts(1 hunks)packages/feature-store/src/apiHooks/__tests__/useFeatureStoreDataSources.spec.ts(1 hunks)packages/feature-store/src/apiHooks/useFeatureStoreDataSources.ts(1 hunks)packages/feature-store/src/screens/dataSources/DataSources.tsx(1 hunks)packages/feature-store/src/screens/dataSources/const.ts(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsPage.tsx(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsView.tsx(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceTabs.tsx(1 hunks)packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTableRow.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceTabs.tsx
- packages/feature-store/src/screens/dataSources/const.ts
- packages/feature-store/src/screens/dataSources/DataSources.tsx
- frontend/src/tests/cypress/cypress/pages/featureStore/featureDataSourceDetails.ts
- packages/feature-store/src/screens/dataSources/dataSourceTable/FeatureStoreDataSourceTableRow.tsx
- frontend/src/tests/cypress/cypress/pages/featureStore/featureDataSource.ts
- packages/feature-store/src/apiHooks/tests/useFeatureStoreDataSources.spec.ts
- packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsPage.tsx
- frontend/src/tests/cypress/cypress/tests/mocked/featureStore/featureDataSources.cy.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/feature-store/src/apiHooks/useFeatureStoreDataSources.ts (3)
frontend/src/utilities/useFetch.ts (1)
FetchStateObject(37-43)packages/feature-store/src/types/dataSources.ts (1)
DataSourceList(71-75)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreAPI(83-90)
packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsView.tsx (3)
packages/feature-store/src/const.ts (1)
hasContent(22-22)packages/feature-store/src/FeatureStoreContext.tsx (1)
useFeatureStoreProject(92-111)packages/feature-store/src/routes.ts (1)
featureDataSourceRoute(28-31)
🔇 Additional comments (6)
packages/feature-store/src/screens/dataSources/dataSourceDetails/DataSourceDetailsView.tsx (3)
97-120: Nice fix: guarded timestamps and correct “Created” className.Avoiding
new Date(undefined)and basing disabled styling on the matching field are both correct.
127-142: Good guard: link only when both batch name and project exist.Prevents empty href/undefined label. Looks good.
71-76: Your script is running; I’ll review the DetailsItem implementation and its testId handling before updating the comment.packages/feature-store/src/apiHooks/useFeatureStoreDataSources.ts (3)
2-6: Cross-package import path sanity checkVerify that '@odh-dashboard/internal/utilities/useFetch' is resolvable from packages/feature-store (tsconfig paths/build). Avoid hidden coupling to frontend-only internals.
If coupling is unintended, mirror useFetch in a shared package or move the hook into a common workspace.
10-12: LGTM overallHook shape matches other FS patterns; dependencies look correct.
3-6: No action needed—useFetch already refetches on callback change
TheReact.useEffectin useFetch.ts (depends on[call, refreshRate]) invokescallAndSave()whenever thecallfunction identity changes, so a refetch is already triggered.
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/feature-store/src/__mocks__/mockDataSources.ts (1)
106-119: Compute pagination dynamically and allow overrides.
total_pagesis hardcoded to 1 and flags are always false, which can mislead tests with larger datasets. Derive values fromdataSources.lengthand acceptpagination/relationshipsoverrides.Apply:
-export const mockDataSources = ({ - dataSources = [mockDataSource_REQUEST_SOURCE({}), mockDataSource_BATCH_FILE({})], -}: Partial<DataSourceList>): DataSourceList => ({ - dataSources, - pagination: { - page: 1, - limit: 50, - total_count: dataSources.length, - total_pages: 1, - has_next: false, - has_previous: false, - }, - relationships: {}, -}) +export const mockDataSources = ( + { + dataSources = [mockDataSource_REQUEST_SOURCE({}), mockDataSource_BATCH_FILE({})], + pagination, + relationships, + }: Partial<DataSourceList> = {}, +): DataSourceList => { + const page = pagination?.page ?? 1 + const limit = pagination?.limit ?? 50 + const totalCount = pagination?.total_count ?? dataSources.length + const totalPages = pagination?.total_pages ?? Math.max(1, Math.ceil(totalCount / limit)) + const hasNext = pagination?.has_next ?? page < totalPages + const hasPrevious = pagination?.has_previous ?? page > 1 + return { + dataSources, + pagination: { page, limit, total_count: totalCount, total_pages: totalPages, has_next: hasNext, has_previous: hasPrevious }, + relationships: relationships ?? {}, + } +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/feature-store/src/__mocks__/mockDataSources.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/feature-store/src/__mocks__/mockDataSources.ts (1)
packages/feature-store/src/types/dataSources.ts (1)
DataSourceList(71-75)
🔇 Additional comments (3)
packages/feature-store/src/__mocks__/mockDataSources.ts (3)
2-2: Correct import move to dataSources types.Type source consolidation looks right.
70-73: Meta added for REQUEST_SOURCE looks good.Consistent timestamps across mocks aid UI rendering and sorting.
99-102: Meta added for BATCH_FILE looks good.Keeps parity with other mocks.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: manaswinidas The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Closes
Figma Link with content for data sources
Description
This PR contains the Data sources page for feature store
Data.sources.table.list.and.details.page.mp4
How Has This Been Tested?
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
New Features
Style
Tests