Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(v1): API remaining methods and docs improvements #292

Merged
merged 5 commits into from
Mar 5, 2025

Conversation

pellicceama
Copy link
Collaborator

@pellicceama pellicceama commented Mar 4, 2025

Important

Enhance API with new endpoints for connection management, connector configurations, and event listing, along with magic link and token generation.

  • API Endpoints:
    • Add getConnection, listConnections, and checkConnection endpoints in connection.ts for managing connections.
    • Add listConnectorConfigs endpoint in connectorConfig.ts for listing connector configurations.
    • Add listEvents endpoint in event.ts for listing events.
    • Add createMagicLink and createToken endpoints in connect.ts for generating magic links and tokens.
  • Types and Schemas:
    • Update openapi.types.ts to include new endpoints and their parameters.
    • Add core.connector_config schema in openapi.json for connector configurations.
  • Utilities:
    • Add zConnectorName in connection.ts for validating connector names.
    • Use asCustomer function in connect.ts for customer context in token generation.
  • Miscellaneous:
    • Update appRouter in index.ts to include new routers.
    • Add error handling for NOT_FOUND in connection.ts.

This description was created by Ellipsis for 2c2ba07. It will automatically update as commits are pushed.

Copy link

vercel bot commented Mar 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
openint ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 5, 2025 0:28am

@pellicceama pellicceama marked this pull request as draft March 4, 2025 23:54
@pellicceama pellicceama changed the title adding connect router feat(v1): API remaining methods and docs improvements Mar 4, 2025
Copy link

recurseml bot commented Mar 4, 2025

✨ No issues found! Your code is sparkling clean! ✨

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 9a13af1 in 2 minutes and 56 seconds

More details
  • Looked at 189 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/index.ts:27
  • Draft comment:
    Stylistic: The trailing comma in the mergeRouters argument list (after connectRouter) may be non-standard in some setups. Consider removing it if not intended.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. packages-v1/api-v1/trpc/routers/connect.ts:65
  • Draft comment:
    Avoid using the non-null assertion (!) on process.env['JWT_SECRET']. Validate its existence or provide a clear fallback to prevent runtime errors.
  • Reason this comment was not posted:
    Marked as duplicate.
3. packages-v1/api-v1/trpc/routers/connect.ts:86
  • Draft comment:
    When splitting 'integration_ids', filter out any empty strings to avoid unintended empty values in the resulting list.
  • 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%
    The code already trims each ID which handles spaces, but consecutive commas could create empty strings. However, this is likely an edge case and the empty strings would just be ignored by the receiving code. The current code is simple and clear. Adding filter() would add complexity for minimal benefit.
    Empty strings in URL parameters could potentially cause issues in some systems. The code would be more robust with the suggested change.
    The impact of empty strings here is speculative - we don't see evidence of actual problems. The current code is clean and handles the common cases well.
    Delete the comment as it suggests a defensive coding practice without clear evidence of need, making the code more complex for an edge case.
4. packages-v1/api-v1/trpc/routers/connect.ts:87
  • Draft comment:
    Review the logic adding the 'int_' prefix for integration IDs. Ensure that IDs containing more than one underscore are handled as intended.
  • 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%
    The code explicitly checks for length === 2 after split, which means it deliberately handles IDs with multiple underscores by not modifying them. This appears to be intentional behavior, not a bug. The comment is asking for verification without providing evidence of an actual issue.
    The code could potentially be improved with a comment explaining why only IDs with exactly one underscore get the prefix, but that's a different issue.
    The current logic appears intentional and the comment is just asking for verification without demonstrating any actual problem.
    Delete the comment as it's asking for verification without providing evidence of an actual issue, and the current logic appears intentional.
5. packages-v1/api-v1/trpc/routers/connect.ts:74
  • Draft comment:
    Sanitize the 'redirect_url' input to mitigate potential open redirect vulnerabilities before setting it as a query parameter.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
    Open redirect vulnerabilities are serious security issues. The code does accept any string as redirect_url without validation. However, we don't know if this is an internal URL that's already validated elsewhere, or if sanitizeUrl() function exists. The comment suggests a specific fix but doesn't provide enough context about the security implications or the suggested function.
    I might be overly cautious about a real security issue. Open redirects are a common attack vector and the code shows no obvious URL validation.
    While the security concern may be valid, the comment doesn't provide enough context about the sanitizeUrl function or the specific security requirements. We don't know if URL validation happens at a different layer.
    While the security concern is valid, the comment suggests using an undefined function and lacks context about the security requirements. It's not actionable enough as written.
6. packages-v1/api-v1/trpc/routers/index.ts:27
  • Draft comment:
    Ensure that 'connectRouter' is merged in the intended order to avoid potential route conflicts with other routers.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the author to ensure something, which violates the rule against asking the author to ensure behavior is intended. It doesn't provide a specific suggestion or ask for a specific test to be written.
7. packages-v1/api-v1/trpc/routers/connect.ts:38
  • Draft comment:
    In the description for connector_names (line 36-38), consider hyphenating 'comma separated' to 'comma-separated' for improved clarity.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
    The comment suggests a minor grammatical improvement, which is not strictly necessary for code functionality. It doesn't violate any rules, but it doesn't provide significant value either.
8. packages-v1/api-v1/trpc/routers/connect.ts:43
  • Draft comment:
    In the description for integration_ids (line 40-43), consider hyphenating 'comma separated' to 'comma-separated' for improved clarity.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
    The comment suggests a minor grammatical improvement, which is not strictly necessary for code functionality. It doesn't violate any rules, but it doesn't provide significant value either. It's more of a stylistic suggestion.

Workflow ID: wflow_zBTUqShC4IB3kI0K


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.

@pellicceama pellicceama marked this pull request as ready for review March 5, 2025 00:26
@pellicceama pellicceama merged commit 3ca38d1 into main Mar 5, 2025
2 of 4 checks passed
@pellicceama pellicceama deleted the api-docs-improvs branch March 5, 2025 00:26
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 2c2ba07 in 2 minutes and 45 seconds

More details
  • Looked at 5518 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 14 drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connect.ts:20
  • Draft comment:
    Avoid casting zCustomerId to any. Consider using the proper zod type to retain type safety.
  • 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%
    The repeated use of 'as any' in multiple places suggests this might be intentionally working around type constraints. Without seeing the definition of zCustomerId and understanding why the casting is needed, removing it could break things. The describe() method might not be directly available on the zCustomerId type without the cast.
    I might be too cautious - type safety is important and removing 'any' casts is generally good practice. The repeated pattern could be a bad practice that should be fixed.
    However, without seeing the zCustomerId definition and understanding why the casts were added, suggesting removal could cause issues. The consistent pattern suggests intentional design.
    While improving type safety is good, we lack sufficient context about zCustomerId to be confident this change would be safe and correct.
2. packages-v1/api-v1/trpc/routers/connection.ts:148
  • Draft comment:
    Avoid using any for casting connections. Use proper inferred types from core.connection.
  • 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-v1/api-v1/trpc/routers/connection.ts:282
  • Draft comment:
    Consider handling errors in checkConnection more granularly instead of always returning 'unknown_external_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.
4. packages-v1/api-v1/trpc/routers/event.ts:12
  • Draft comment:
    Remove unused commented-out createEvent API if not planned for publishing.
  • 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.
5. packages-v1/api-v1/trpc/routers/index.ts:22
  • Draft comment:
    Ensure routers are merged in a logical order; consider placing general endpoints (health, viewer) at the top.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
6. packages/engine-backend/router/customerRouter.ts:130
  • Draft comment:
    Consider minimizing sensitive logging in customer token creation to avoid exposing customer IDs.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
7. packages-v1/api-v1/trpc/routers/connect.ts:20
  • Draft comment:
    Avoid using 'as any' with zCustomerId. Use the proper, fully typed Zod schema instead.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
    The use of 'as any' is generally a TypeScript code smell as it bypasses type checking. However, without seeing the definition of zCustomerId from @openint/cdk, I can't be 100% certain that removing the cast won't cause type errors. The presence of similar casts elsewhere in the file (zId) suggests there might be a reason for these casts.
    I don't have visibility into the zCustomerId definition or why the type cast was added in the first place. The cast might be working around a legitimate type compatibility issue.
    While type casts should generally be avoided, without more context about zCustomerId's type definition, removing it could potentially break type checking.
    Given the lack of context about why the type cast exists and what zCustomerId's type definition is, we should err on the side of caution and not keep this comment.
8. packages-v1/api-v1/trpc/routers/connect.ts:37
  • Draft comment:
    Consider using an array for connector_names instead of a comma-separated string to improve clarity and type safety.
  • 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 reasonable suggestion for code quality as arrays are more type-safe than comma-separated strings. The current implementation requires string splitting/joining operations and could be error-prone. However, this may be an intentional API design choice to match REST conventions or existing patterns. The change would be breaking for API consumers.
    The comment doesn't acknowledge that this could be a breaking change for API consumers. Also, URL parameters traditionally use comma-separated values, so this might be intentionally following that convention.
    While the suggestion has merit from a code quality perspective, changing the API parameter type would be a breaking change and the current approach follows common URL parameter conventions.
    The comment should be deleted as it suggests a breaking API change and the current implementation follows standard URL parameter patterns.
9. packages-v1/api-v1/trpc/routers/connection.ts:112
  • Draft comment:
    Refine the type for the id field instead of using 'as any' with zId('conn').
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
    The presence of the TODO comment suggests this is a known issue that needs proper fixing. The 'as any' appears multiple times in the file for zId usage. Simply removing 'as any' without fixing the underlying type issue would likely cause TypeScript errors. The comment doesn't provide enough context about how to properly fix the type issue.
    The comment might be oversimplifying the solution - if it was as simple as removing 'as any', why is there a TODO comment? There may be underlying type compatibility issues that need to be resolved first.
    However, suggesting to remove type assertions is generally good practice as they can hide real type issues, and the comment does point to a real code quality issue.
    While the comment identifies a real issue, it doesn't provide enough guidance on how to properly fix the underlying type problem. The TODO comment suggests this needs a more comprehensive solution.
10. packages-v1/api-v1/trpc/routers/connection.ts:278
  • Draft comment:
    Remove 'QQ' comments and improve error logging in checkConnection for better traceability.
  • 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.
11. packages-v1/api-v1/trpc/routers/connectorConfig.ts:61
  • Draft comment:
    Ensure robust error handling around expandConnector so that failures in expanding fields are caught appropriately.
  • 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.
12. packages/engine-backend/router/customerRouter.ts:205
  • Draft comment:
    Refactor the mapping logic for connectorNames and integrationIds to accept arrays rather than comma-separated strings for better type safety.
  • 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.
13. packages-v1/api-v1/__generated__/openapi.types.ts:2310
  • Draft comment:
    There appears to be a duplicated word in the title of the 500 error block. It currently reads 'Internal server error error (500)'; consider changing it to 'Internal server error (500)'.
  • 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.
14. packages/engine-backend/router/customerRouter.ts:24
  • Draft comment:
    Typographical error: In the comment at line 24, "Optional because when creating magic link as current customer we dont' need it..." should be corrected to "don't".
  • 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_WCRDZoTBlalAay53


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant