Styling connect #406
Merged
Styling connect #406
Conversation
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
…onnection and simple item tiles
…ith additional guidelines for component creation and usage.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| .passthrough() | ||
| .openapi({ref: 'core.integration', title: 'Integration'}), | ||
| customer: coreBase | ||
| .extend({ | ||
| id: z.string(), |
There was a problem hiding this comment.
Duplicate field definition: 'id' is already defined in coreBase which Customer extends. This will cause a runtime error as Zod will detect the duplicate field.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| {id: '3', name: 'Item 3', color: 'bg-green-500', description: 'Third item'}, | ||
| ] | ||
|
|
||
| const simpleColumns: ColumnDef<SimpleItem>[] = [ |
There was a problem hiding this comment.
ColumnDef type is used but not imported. This will cause a TypeScript compilation error. Need to import ColumnDef from the appropriate library (likely '@tanstack/react-table' or similar table library).
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
😱 Found 2 issues. Time to roll up your sleeves! 😱 🗒️ View all ignored comments in this repo
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b8b312e in 1 minute and 21 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages/api-v1/trpc/openapi-to-trpc.spec.ts:51
- Draft comment:
Skipped test without an explanation. Add a comment explaining why it’s skipped and a plan to reinstate it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
Skipped tests without explanation can lead to technical debt and confusion. However, the comment is asking the author to explain their intention and add documentation, which violates our rule about not asking authors to explain or confirm their intentions. The comment is also somewhat speculative - we don't know if there's a ticket or if the author plans to reinstate it.
The comment could be helping prevent technical debt. Skipped tests without explanation often become permanent and lead to reduced test coverage.
While preventing technical debt is important, our rules explicitly state not to ask authors to explain their intentions or add documentation. We should trust that the author has a good reason for skipping the tests.
Delete the comment as it violates our rule about not asking authors to explain their intentions or add documentation.
2. packages/api-v1/trpc/openapi-to-trpc.spec.ts:103
- Draft comment:
Skipped test without an explanation. Provide a rationale for skipping this test to maintain test coverage clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for an explanation for a skipped test, which is a request for clarification. This violates the rule against asking the PR author to explain or confirm their intention. Therefore, this comment should be removed.
3. packages/api-v1/trpc/openapi-to-trpc.spec.ts:103
- Draft comment:
Avoid skipping tests without documenting the reason. Include a comment or TODO to clarify why this test is disabled. - Reason this comment was not posted:
Marked as duplicate.
4. packages/api-v1/trpc/openapi-to-trpc.spec.ts:78
- Draft comment:
There appears to be a double slash in the logo_url string ('.../public//_assets/logo-google.svg'). Consider removing the extra slash to ensure the URL is formatted correctly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_8PbkDhjU2FS7h8Zb
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -48,7 +48,7 @@ describeEachDatabase({drivers: ['pglite'], migrate: true}, (db) => { | |||
| app = createApp({db}) | |||
| }) | |||
|
|
|||
| test('GET /connector-config with expand options', async () => { | |||
| test.skip('GET /connector-config with expand options', async () => { | |||
There was a problem hiding this comment.
Avoid leaving tests skipped without an explanation. If this test is temporarily disabled, please add a comment why it's skipped and/or a TODO to re-enable it.
Suggested change
| test.skip('GET /connector-config with expand options', async () => { | |
| test.skip('GET /connector-config with expand options', async () => { // TODO: Add reason for skipping this test |
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to d755bd9 in 2 minutes and 34 seconds
More details
- Looked at
2483lines of code in24files - Skipped
0files when reviewing. - Skipped posting
20drafted comments based on config settings.
1. apps/web/app/(v1)/connect-v1/client.tsx:65
- Draft comment:
Usage of 'React.use' for unwrapping a promise is experimental. Confirm that this pattern is intended for client components. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to confirm their intention of using 'React.use' for unwrapping a promise, which is experimental. This falls under asking the author to confirm their intention, which is against the rules.
2. apps/web/app/(v1)/connect-v1/client.tsx:34
- Draft comment:
Remove or disable debugging console.log statements before production deployment. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/api-v1/routers/connection.ts:238
- Draft comment:
Avoid 'any' casts in formatConnection; improve type-safety rather than using 'connection as any as ...'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/ui-v1/domain-components/__stories__/fixtures.ts:127
- Draft comment:
Consider using fixed dates or constants in test fixtures to avoid non‐determinism in tests. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
- This is a fixtures file for Storybook stories, not unit tests. 2. The exact date values don't matter for UI stories. 3. Non-determinism in dates won't affect story functionality or visual testing. 4. The comment would require changing many instances throughout the file, not just one line. 5. The benefit of fixed dates here is minimal.
The reviewer has a valid point about non-determinism in test fixtures generally being bad practice. In some cases, varying dates could affect snapshot tests or visual regression tests.
However, this is specifically a Storybook fixtures file where exact dates don't impact functionality or testing. The maintenance cost of keeping fixed dates would outweigh the minimal benefits.
The comment should be deleted as it suggests a change that would add maintenance overhead without providing meaningful benefits in this specific context.
5. apps/web/app/(v1)/connect-v1/client.tsx:67
- Draft comment:
Good refactor: replacing the old extractId with direct use of connectorConfig.connector_name makes the code clearer. Ensure that the connectorConfig object always includes a valid connector_name. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/web/app/(v1)/connect-v1/client.tsx:51
- Draft comment:
Dynamic import via wrapConnectorClientModule is well structured. Consider adding a comment on why memoization is used, to help future maintainers. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%
None
7. apps/web/app/(v1)/connect-v1/page.tsx:73
- Draft comment:
The theming logic converting searchParams to CSS custom properties is neat. Consider removing commented-out debug code once confirmed stable. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
8. packages/api-v1/models/index.ts:135
- Draft comment:
The updated ConnectorConfig type using intersection & Partial is well defined. Consider explicitly documenting which fields are optional. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. packages/api-v1/routers/connection.ts:71
- Draft comment:
The commented-out block for settingsToInclude is left in the code. Either implement the secret stripping logic or remove it to reduce confusion. - Reason this comment was not posted:
Marked as duplicate.
10. packages/api-v1/routers/connector.spec.ts:23
- Draft comment:
Tests for 'list connectors with integrations' are skipped. Please document the reason for skipping and consider un-skipping once the integration support is stable. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While better documentation of skipped tests is generally good practice, this comment is essentially asking for confirmation/explanation from the PR author. The existing "TODO: fix this" comment already indicates this is temporary and needs fixing. The suggested comment doesn't add meaningful technical value beyond what's already implied.
Better documentation of skipped tests could help future developers understand the codebase better. The current TODO comment is quite vague.
However, asking for explanations falls under the rule of "do NOT ask the PR author to confirm their intention or explain things." The existing TODO already serves the essential purpose.
Delete the comment as it violates the rule about not asking authors for explanations or confirmations, and doesn't suggest any concrete code changes.
11. packages/api-v1/routers/connectorConfig.ts:223
- Draft comment:
The section handling 'enabled_integrations' expansion is commented out with a TODO. It would be best to address or remove such dead code for clarity. - Reason this comment was not posted:
Marked as duplicate.
12. packages/ui-v1/components/DataTileView.tsx:30
- Draft comment:
DataTileView component correctly leverages tanstack/react-table. The use of the 'satisfies' operator for TableOptions enhances type safety. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
13. packages/ui-v1/components/command/command-components.tsx:52
- Draft comment:
The renamed CommandPopover component is well implemented. For clarity, consider updating documentation or inline comments to reflect its new intended usage as a menu/popover button. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. packages/ui-v1/domain-components/ConnectionCard.tsx:56
- Draft comment:
ConnectionCard handles hover state and rendering fallback well. Consider adding ARIA attributes to improve accessibility, particularly for interactive elements triggered on hover. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
15. packages/ui-v1/domain-components/ConnectorTableCell.tsx:26
- Draft comment:
Debug logging in ConnectorTableCell (DebugData component) is useful during development but should be removed or disabled in production builds to avoid cluttering the console. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. packages/ui-v1/domain-components/__stories__/fixtures.ts:337
- Draft comment:
The fixtures file is very comprehensive. Ensure these mocks remain consistent with any future changes to the Core models to avoid type mismatches in components or tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
17. packages/api-v1/routers/connector.spec.ts:31
- Draft comment:
Typographical error in the test description on line 31: "get connector by with invalid name returns error". Consider revising it to remove the extra "by" or rephrase it for clarity (e.g., "get connector with invalid name returns error"). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. packages/api-v1/routers/connectorConfig.spec.ts:37
- Draft comment:
Typographical error: The comment on line 37 uses 'simplicty' instead of 'simplicity'. Please correct the typo. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. packages/ui-v1/domain-components/ConnectionCard.tsx:48
- Draft comment:
Typo Alert: In the className of the div on line 48, the utility class 'size-full' does not appear to be a standard Tailwind CSS class. Did you mean to use 'w-full' or a similar intended utility class? - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is purely a UI styling concern. The rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic. Assume that if the author changed the UI, they did it correctly." Additionally, 'size-full' could be a valid custom Tailwind class defined in their configuration.
What if 'size-full' is actually a typo that could cause layout issues? What if this is a standardization issue that affects maintainability?
Even if it is a typo, it's still a pure UI styling concern. The rules are clear that we should trust the author on UI changes and not comment on styling.
The comment should be removed as it violates the rule about not commenting on UI and styling changes.
20. packages/ui-v1/domain-components/__stories__/fixtures.ts:26
- Draft comment:
Minor typographical note: The display name for the HubSpot connector is spelled 'HubSpot' in the connectors object (line 26) but appears as 'Hubspot' in the integrations object (line 102). For clarity and consistency, please choose one consistent spelling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_rTgVu2606ZGI0U7y
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| } else if (include_secrets === 'all') { | ||
| settingsToInclude = {settings: connection.settings} | ||
| } | ||
| // let settingsToInclude = {settings: {}} |
There was a problem hiding this comment.
Clean up commented-out secret-handling code before finalizing the implementation.
| if (filteredIntegrations) { | ||
| result.integrations = filteredIntegrations | ||
| } | ||
| // const filteredIntegrations = expandIntegrations(ccfg) |
There was a problem hiding this comment.
Expansion for 'enabled_integrations' is commented out. Either remove the commented code or finish implementation with clear documentation.
| @@ -138,7 +138,7 @@ describeEachDatabase({drivers: ['pglite'], migrate: true, logger}, (db) => { | |||
| }) | |||
| }) | |||
|
|
|||
| test('list connector config with expand enabled_integrations', async () => { | |||
| test.skip('list connector config with expand enabled_integrations', async () => { | |||
There was a problem hiding this comment.
Skipped tests (e.g. 'list connector config with expand enabled_integrations') should include a clear TODO or comment explaining the issue to track resolution.
.cursorrules
Outdated
|
|
||
| - Classname should be combined using the `cn` util from `@openint/shadcn/lib/utils` whenever we create a component | ||
| - Always use with `React.` prefix for things like React.useState and React.useEffect, or whenever referencing built-in hooks. | ||
| - Use Components should reference Core models whwnever possible rather than repeating types. Use consistent mocks from fixtures file |
There was a problem hiding this comment.
Typo detected: 'whwnever' should be corrected to 'whenever' for clarity and correctness.
Suggested change
| - Use Components should reference Core models whwnever possible rather than repeating types. Use consistent mocks from fixtures file | |
| - Use Components should reference Core models whenever possible rather than repeating types. Use consistent mocks from fixtures file |
| }, | ||
| 'google-drive-basic': { | ||
| id: 'conn_gdrive_123', | ||
| connector_config_id: 'ccfg_google-drive-beta_123', |
There was a problem hiding this comment.
Typographical inconsistency: The connector_config_id for the google-drive connections is written as 'ccfg_google-drive-beta_123', whereas in the connectorConfigs object the id is 'ccfg_google-drive_123'. Please correct the config id so it is consistent.
pellicceama
pushed a commit
that referenced
this pull request
Jul 16, 2025
* Scaffold connnector card proper * Add some fixture data for connections card and start of .cursorrules * Further update fixtures for connect * Bettering story * Add grid for connection * Add DataTileView component and its associated stories for rendering connection and simple item tiles * Remove SimpleDataTable and related story files; update .cursorrules with additional guidelines for component creation and usage. * Use ConnectionCard in Connect * expand on connector when retrieving connection * Removing irrelevant stuff * Scaffold ConnectorConfigCard * use the connector config card in connect * Fix type errors * fix more tests * Fix more tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Important
Enhance UI components and API models for connectors and connections, adding new components and expanding existing functionalities.
DataTileViewcomponent inDataTileView.tsxfor rendering data in a tile view.ConnectionCardandConnectorConfigCardcomponents for displaying connection and connector configuration details.CommandPopoverincommand-components.tsxto include aclassNameprop.ConnectorConfigandConnectionExpandedtypes inmodels/index.tsto include additional fields.expandIntegrationsandexpandConnectorfunctions inconnectorConfig.tsfor expanding connector details.connectionRouterinconnection.tsto handle expanded fields and include secrets.connectorConfigRouterinconnectorConfig.tsto support expanded fields and handle connector configurations.connector.spec.ts,connectorConfig.spec.ts, andopenapi-to-trpc.spec.tsto cover new and modified functionalities..cursorrulesfile for code style and structure guidelines.SimpleDataTablecomponents and related stories.This description was created by
for b8b312e. It will automatically update as commits are pushed.