feat(v1): API V1 methods #288
Merged
Conversation
This file contains 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
|
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}
|
😱 Found 1 issue. Time to roll up your sleeves! 😱 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 9497dc3 in 2 minutes and 37 seconds
More details
- Looked at
121lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connection.ts:32
- Draft comment:
Avoid using console.warn in production code. Consider using a proper logging mechanism (or remove the warning) especially in areas marked TODO. - Reason this comment was not posted:
Marked as duplicate.
2. packages-v1/api-v1/trpc/routers/connection.ts:81
- Draft comment:
Similar to getConnection, a console.warn is used for connections requiring refresh. A proper logging strategy or a refresh-handling implementation should be provided instead of just logging. - 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 comment identifies a real pattern of using console.warn as a temporary solution in two places. The TODO comments suggest this is known technical debt. However, the comment is more observational than actionable - it doesn't specify what logging strategy to use or how to implement refresh handling. It's stating something the developers are already aware of via TODOs.
The comment does point out a legitimate concern about proper error handling and logging. Maybe having this feedback could help prioritize implementing the proper solution.
While valid, the comment doesn't add new information beyond what's already acknowledged in the TODOs. The developers are clearly aware this is temporary.
Delete the comment since it's not providing actionable feedback beyond what's already acknowledged in the code's TODO comments.
3. packages-v1/api-v1/trpc/routers/connection.ts:24
- Draft comment:
Ambiguous operator precedence in the credentialsRequiresRefresh calculation. Wrap the OR condition in parentheses to ensure it evaluates as intended (e.g., input.force_refresh || (connection.settings.oauth?.credentials?.expires_at && new Date(connection.settings.oauth.credentials.expires_at) < new Date())). - Reason this comment was not posted:
Marked as duplicate.
4. packages-v1/api-v1/trpc/routers/connection.ts:75
- Draft comment:
The force_refresh logic in listConnections will mark all connections for refresh regardless of credential expiry. Confirm that this behavior is intentional or adjust the condition accordingly. - 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 comment violates the rule about not asking authors to confirm their intentions. The force_refresh parameter is clearly meant to force a refresh regardless of expiry - that's why it's called force_refresh. The logic is consistent between both endpoints. There's no evidence this is a bug rather than intended behavior.
Maybe there's a legitimate security or performance concern about allowing forced refreshes that I'm missing? Maybe this is actually a bug that could cause unnecessary API calls?
The code shows this is a deliberate feature with consistent implementation across endpoints. If there were security/performance concerns, they should be raised directly rather than asking if it's intentional.
Delete the comment. It merely asks for confirmation of clearly intended behavior rather than identifying an actual issue.
Workflow ID: wflow_sI7SzOElKkM8P4zy
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.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a8dc83d in 2 minutes and 11 seconds
More details
- Looked at
203lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connection.ts:96
- Draft comment:
Avoid using non-null assertion (!) for connector_config_id in listConnections mapping. Instead, ensure schema validation or proper handling when the field might be null. - 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 in getConnection shows that connections without connector_config_id are considered invalid and filtered out with a NOT_FOUND error. This suggests it's a required field in valid connections. The non-null assertion here isn't actually dangerous because invalid connections are filtered out by design. The suggestion to remove it would add unnecessary null checks for a condition that can't occur.
I could be wrong about the database schema - maybe connector_config_id is nullable in the actual database. Also, there could be race conditions where a connection becomes invalid between validation and usage.
Even if the field is nullable in the database, the application clearly treats null connector_config_id as an invalid state that should result in NOT_FOUND. The non-null assertion aligns with this design choice.
The comment should be deleted because the non-null assertion reflects the application's intentional handling of connections without connector_config_id as invalid entities.
2. packages-v1/api-v1/trpc/routers/connection.ts:11
- Draft comment:
Removal of the optional 'force_refresh' in getConnection input: ensure this change is intentional and that refresh behavior is only meant to be handled in checkConnection. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages-v1/api-v1/trpc/routers/connection.ts:130
- Draft comment:
Clarify the credentialsRequiresRefresh expression by adding parentheses; the current OR/ternary usage could be ambiguous regarding operator precedence. - 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 expression does look complex with the OR operator followed by a ternary. However, JavaScript has well-defined operator precedence where || has lower precedence than ?:, so there's no actual ambiguity. The code will work correctly as written. Adding parentheses would only be for readability.
The expression could be hard for some developers to parse at first glance. Parentheses could make the logic more immediately clear to readers.
While parentheses might help readability slightly, this is a relatively minor stylistic suggestion. The code is functionally correct and the logic is not overly complex once you understand it.
This comment is more of a style suggestion than a significant code quality issue. While not wrong, it doesn't meet the bar of being clearly required.
4. packages-v1/api-v1/trpc/routers/index.ts:26
- Draft comment:
Document the generic function zListResponse to clarify its intent and ensure consumers know the expected paginated response shape. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
Workflow ID: wflow_P3wxLt7AfGswcZWY
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.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on dfe4a7c in 2 minutes and 56 seconds
More details
- Looked at
74lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connection.ts:10
- Draft comment:
Avoid using 'any' for the connection parameter. Define and use an appropriate type for better type safety. - Reason this comment was not posted:
Marked as duplicate.
2. packages-v1/api-v1/trpc/routers/connection.ts:20
- Draft comment:
The logic for constructing ‘logoUrl’ uses nested conditionals and a hardcoded CDN URL. Consider refactoring for clarity and externalizing configuration. - 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 comment makes valid points - the nested ternary is somewhat complex and the hardcoded CDN URL could be better as a config. However, the code is still readable and follows a clear logic flow. The suggestion is more of a nice-to-have refactor than a critical issue. The comment is somewhat vague about exactly how to improve it.
The code works as is and the suggested improvements are subjective style preferences rather than clear problems. The current implementation might actually be the clearest way to express this logic.
While the suggestions are valid, they don't point to concrete problems or provide specific actionable improvements. The current code, while not perfect, is functional and reasonably clear.
Delete this comment as it suggests optional style improvements rather than identifying clear issues that need to be fixed.
3. packages-v1/api-v1/trpc/routers/connection.ts:157
- Draft comment:
checkConnection mutation returns connection without formatting (no logo_url added). If consistent API response is desired, consider using formatConnection here as well. - 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/connection.ts:9
- Draft comment:
Clarify the 'TODO: don't make any' comment. It's unclear what action is intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for clarification on a 'TODO' comment in the code. This is essentially asking the author to explain their intention, which violates the rule against asking for explanations or confirmations of intent.
5. packages-v1/api-v1/trpc/routers/connection.ts:20
- Draft comment:
Review the logic for constructing logoUrl: if connector.metadata.logoUrl is undefined or not a valid string, the fallback URL may include an 'undefined' segment. Consider adding an explicit check to ensure a valid value before concatenating. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_z8oWnUHJC479I81N
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.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 53e98c3 in 2 minutes and 25 seconds
More details
- Looked at
79lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connectorConfig.ts:33
- Draft comment:
Using reduce with an initial boolean (true) to build SQL conditions is hard to read and error-prone. - Reason this comment was not posted:
Marked as duplicate.
2. packages-v1/api-v1/trpc/routers/connectorConfig.ts:54
- Draft comment:
Relying on result[0]?.total fails when no rows are returned due to high offset; total count could be lost. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages-v1/api-v1/trpc/routers/connectorConfig.ts:14
- Draft comment:
Consider using a more specific schema (or enum) for connector_name instead of a generic string, as noted by the TODO. - 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 comment is pointing out something that the code author already knows (there's a TODO). The comment doesn't add any new information or specific guidance beyond what's already acknowledged. It's essentially just restating the TODO in different words. Per the rules, we should not make purely informative comments that don't require clear code changes.
The comment could be more useful if it provided specific guidance on what the schema should look like or suggested concrete enum values.
Even with more specific guidance, the author is already aware of the need to improve this via the TODO comment. The automated comment adds no value.
Delete the comment as it only restates what's already acknowledged in the code's TODO comment without providing actionable guidance.
4. packages-v1/api-v1/trpc/routers/connectorConfig.ts:54
- Draft comment:
If the offset is beyond available rows, the query returns an empty result and total defaults to 0 despite possibly having matching records. Consider a fallback or separate total count query to handle this edge case. - 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 comment raises a valid point about the total count logic. However, looking at the SQL query, it uses COUNT() without OVER(), which means it will return the total count of all matching rows regardless of LIMIT/OFFSET. The comment's premise appears to be incorrect - the total count should be accurate even with large offsets. The current implementation is actually correct.
I could be misunderstanding how COUNT() works in this specific database system. Different databases might handle COUNT() differently with LIMIT/OFFSET.
Even if different databases handle COUNT() differently, the code comment shows this is using @openint/db which likely has consistent behavior across systems. The COUNT() without OVER() clause is a standard way to get total counts regardless of pagination.
The comment should be deleted because it appears to be based on a misunderstanding of how COUNT() works in SQL. The current implementation will correctly return the total count regardless of offset.
Workflow ID: wflow_BIf8AbskzbfKcSdb
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.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 30dc9e5 in 1 minute and 39 seconds
More details
- Looked at
83lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connection.ts:129
- Draft comment:
Avoid force-casting in mapping function for connections. Use proper type handling rather than 'conn as any as z.infer'. - Reason this comment was not posted:
Marked as duplicate.
2. packages-v1/api-v1/trpc/routers/connection.ts:177
- Draft comment:
Force-casting for 'connection' in checkConnection is used again. Please ensure proper typing to maintain type safety. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%
The comment is asking the author to ensure proper typing, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure something. However, the comment does point out a potential issue with type safety, which could be useful if rephrased to suggest a specific action or improvement.
3. packages-v1/api-v1/trpc/routers/connection.ts:60
- Draft comment:
Avoid using double casting (e.g. 'connection as any as z.infer'). Instead, validate or transform the database result using your schema to maintain type safety. - Reason this comment was not posted:
Marked as duplicate.
4. packages-v1/api-v1/trpc/routers/connection.ts:130
- Draft comment:
Avoid using double casting (e.g. 'conn as any as z.infer'). Replace it by properly validating the data with your Zod schema. - Reason this comment was not posted:
Marked as duplicate.
5. packages-v1/api-v1/trpc/routers/connection.ts:177
- Draft comment:
Avoid double casting (e.g. 'connection as any as z.infer'). Use proper type validation instead of bypassing type checking. - Reason this comment was not posted:
Marked as duplicate.
6. packages-v1/api-v1/trpc/routers/connection.ts:107
- Draft comment:
The reduce initializer 'undefined as unknown as SQL' is unclear and error-prone. Consider handling the first condition separately or using a clearer approach to combine SQL conditions. - 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 is actually functional - it uses the index parameter to handle the first condition specially, making the initial value irrelevant. While the type casting looks awkward, it's serving a purpose to satisfy TypeScript's type system while allowing the SQL conditions to be combined. The alternative approaches would likely be more complex or require restructuring the logic significantly.
The comment doesn't provide a concrete alternative that would be clearly better. The current approach, while not beautiful, is working and type-safe.
While the code could potentially be cleaner, the current implementation is functional and type-safe. The suggested changes might introduce more complexity for minimal benefit.
Delete this comment. The code works as intended and the suggested changes would not provide significant benefits to justify the refactoring effort.
Workflow ID: wflow_rPMEevvOyrwY1XKa
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.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b61fe5d in 2 minutes and 35 seconds
More details
- Looked at
121lines of code in3files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connection.ts:69
- Draft comment:
Review the use of .optional() after extending zListParams. This may allow undefined input objects and bypass expected defaults. - 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 handles undefined input safely by using optional chaining and nullish coalescing throughout. The defaults for limit and offset are explicitly set. All other fields are optional anyway. Making the whole input optional doesn't introduce any actual issues since the code is properly defensive.
The comment raises a valid concern about type safety and default values. Could there be edge cases where an undefined input causes problems?
The code explicitly handles all cases of undefined input safely, and all the extended fields are optional anyway, so making the whole input optional doesn't introduce any actual problems.
The comment should be deleted because the code already handles undefined input correctly and there's no actual issue to fix.
2. packages-v1/api-v1/trpc/routers/connectorConfig.ts:36
- Draft comment:
Using 'true' as the reduce initial value for SQL conditions is non-standard. Consider initializing with the first condition to safely combine SQL clauses. - 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:130
- Draft comment:
Ensure consistent default handling for 'include_secrets'; getConnection defaults to false while listConnections may pass undefined. - 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 comment points out a real inconsistency. The formatConnection function has a default of false, but passing undefined directly could lead to different behavior than explicitly defaulting to false. However, looking at the function definition, the default parameter value means undefined will become false anyway, so this isn't actually causing any behavioral difference.
The comment identifies a real code style inconsistency, but does it actually matter given the function's default parameter?
While the code style is inconsistent, the behavior is identical due to the default parameter value in formatConnection. This makes the comment less valuable.
The comment points out a style inconsistency but not a functional issue. Given our rules about only keeping comments that clearly require code changes, this should be removed.
4. packages-v1/api-v1/trpc/routers/connection.ts:68
- Draft comment:
Making the input schema optional with.optional()means no input will fall back to defaults. Confirm this is intended and update docs/comments to explain how defaults (e.g. limit = 50, offset = 0) are assigned. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%
The comment is asking the author to confirm their intention regarding making the input schema optional and to update documentation to explain defaults. While the first part is a valid suggestion, the second part about updating docs/comments is not allowed according to the rules.
5. packages-v1/api-v1/trpc/routers/connectorConfig.ts:36
- Draft comment:
The reduction logic combining where conditions uses an initial value oftrue as boolean | SQL<unknown>, which differs from the approach in connection.ts. Consider extracting a helper or unifying the pattern to combine SQL conditions consistently. - 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.
6. packages-v1/api-v1/trpc/routers/event.ts:25
- Draft comment:
The input schema is now optional; ensure that clients are aware of the default values for limit and offset. Document this behavior to avoid surprises. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that clients are aware of default values and to document the behavior. This falls under asking the author to ensure something is done, which is against the rules. The comment does not provide a specific code suggestion or ask for a specific test to be written.
Workflow ID: wflow_0eNTs2anUJpaWhvS
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.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6667058 in 1 minute and 26 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connection.ts:36
- Draft comment:
Consider implementing or clarifying the 'display_name' field in the returned object. If this field is required, please add its handling; if not, remove the TODO. - 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 comment is essentially just repeating what the TODO already says. It doesn't provide any additional insight or guidance. TODOs are normal parts of code and don't necessarily need to be addressed immediately. The comment isn't pointing out a bug or suggesting a concrete improvement.
Maybe the TODO indicates a missing required field that should be implemented now rather than later? Maybe there's important context about display_name that makes this urgent?
Without more context about whether display_name is actually needed or what its purpose would be, we can't determine if this is urgent. The TODO can remain as a reminder for future work.
The comment should be deleted as it doesn't provide actionable feedback beyond what the TODO already communicates.
2. packages-v1/api-v1/trpc/routers/connection.ts:36
- Draft comment:
Ensure that the TODO for 'display_name' is resolved. If display_name is needed, add its implementation; otherwise, remove the comment. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_l3fs8A2scvff2M0y
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Skipped PR review on 7cf1381 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on afe36fe in 1 minute and 8 seconds
More details
- Looked at
136lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. packages-v1/api-v1/trpc/routers/connection.ts:10
- Draft comment:
Good update to import pagination utils from the dedicated file. Ensure tests cover pagination behavior in live scenarios. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages-v1/api-v1/trpc/routers/connectorConfig.ts:6
- Draft comment:
The reordering of imports and migration to the pagination util is clear. Verify that no unwanted dependencies were removed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. packages-v1/api-v1/trpc/routers/event.ts:4
- Draft comment:
The switch to import pagination utilities from the new location is properly done. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. packages-v1/api-v1/trpc/routers/index.ts:1
- Draft comment:
Removed pagination utility exports from the routers index. Ensure documentation is updated to reflect the new location of these helpers. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. packages-v1/api-v1/trpc/routers/connection.ts:11
- Draft comment:
Refactored pagination utilities import to '../utils/pagination'. Verify the new module provides all required functions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify that the new module provides all required functions. This falls under the rule of not asking the author to double-check things or ensure behavior is intended. Therefore, this comment should be removed.
6. packages-v1/api-v1/trpc/routers/connectorConfig.ts:6
- Draft comment:
Updated pagination utilities import path to '../utils/pagination' for consistency across routers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it only states what was changed without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
7. packages-v1/api-v1/trpc/routers/event.ts:9
- Draft comment:
Changed pagination utilities import to use '../utils/pagination'. Ensure the interface remains compatible. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure compatibility, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
8. packages-v1/api-v1/trpc/routers/index.ts:11
- Draft comment:
Removed pagination utility definitions in favor of the dedicated module. Confirm that all consumers now import these from '../utils/pagination'. - 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.
9. packages-v1/api-v1/trpc/routers/event.ts:17
- Draft comment:
In the commented-out section, the phrase 'Ref does not work for input params for now in zod-openapi. So will be inlined in the spec unfortunately' could benefit from clearer wording. Consider revising it to something like: 'Reference resolution does not currently work for input parameters in zod-openapi, so they are inlined in the specification.' This will improve clarity without changing functionality. - 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_Qtd8FFhANRsdCqe6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 7199b96 in 1 minute and 50 seconds
More details
- Looked at
64lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. packages-v1/api-v1/trpc/utils/pagination.ts:17
- Draft comment:
Consider using more specific types thanFunctionfororderBy,limit, andoffsetin the generic constraint. This can help catch errors earlier and improve 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 = 30% vs. threshold = 50%
The suggestion is technically correct - usingFunctionis very loose typing. More specific function signatures would catch type errors earlier. However, without knowing the exact types these functions should have (which would require knowledge of the query builder being used), this comment may not be actionable enough. The code works as is, even if not maximally type-safe.
I might be underestimating the value of type safety. Even if we don't know the exact types, suggesting to make them more specific could prevent future bugs.
While type safety is important, without providing specific type suggestions, this comment may lead to back-and-forth discussion rather than actionable improvement.
The comment should be deleted as it's not actionable enough without specific type suggestions, and the current code works despite being loosely typed.
2. packages-v1/api-v1/trpc/utils/pagination.ts:41
- Draft comment:
Avoid usinganyfor the query parameter in processPaginatedResponse. A more specific type would enhance type safety and clarity. - 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 'any' does reduce type safety. However, looking at how the query is used (just awaited and accessed for length/total), it's not clear what the exact type should be without more context about the query objects. The function seems intentionally generic to handle different query types. Making it more specific might limit its reusability.
I might be too lenient on type safety. The 'any' type could hide potential runtime errors and make refactoring harder.
While type safety is important, without knowing the exact query type interface or seeing usage examples, suggesting to remove 'any' without proposing a specific alternative type isn't actionable enough.
The comment identifies a real type safety issue but doesn't provide enough specific guidance to be actionable. Without more context about the query types, it's not clear what type should be used instead.
3. packages-v1/api-v1/trpc/utils/pagination.ts:50
- Draft comment:
Usingresult[0]?.totalto determine the total count may be error-prone if the query doesn’t consistently return atotalfield in every row. Consider adding error handling or validation. - 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 already has reasonable safeguards: length check, optional chaining, null coalescing, and type conversion. The comment doesn't provide specific scenarios where this would fail or concrete suggestions for improvement. The existing comment in the code acknowledges future error handling needs. This seems like a speculative "what-if" type comment rather than pointing out a clear issue.
The comment might have a point about validation - we don't know if the query is supposed to always return a total field. Maybe there are edge cases we're not considering.
The code already handles the case where total is missing or invalid through null coalescing and type conversion. Without specific knowledge of the query contract, this is as robust as we can reasonably make it.
Delete the comment. It's speculative, doesn't provide concrete improvements, and the code already has reasonable error handling in place.
4. packages-v1/api-v1/trpc/utils/pagination.ts:22
- Draft comment:
Default orderByColumn (schema.connection.created_at) may not suit all queries. Consider requiring an explicit column when needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. packages-v1/api-v1/trpc/utils/pagination.ts:50
- Draft comment:
Relying on result[0]?.total for the total count may be fragile if the query response structure varies. Consider a more robust count mechanism. - 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 comment points out a potential issue but doesn't provide a clear alternative. The code is part of a database query processing system where the query structure is likely controlled by the application. Without seeing the actual queries being used, we can't determine if this is actually problematic. The comment is speculative - "may be fragile if..."
I might be overlooking a genuine architectural concern about query result structures. Perhaps there's a better standard practice for count queries.
While there might be other ways to handle counts, without seeing the query construction or knowing the database system, we can't determine if this is actually an issue or if there's a better approach.
Delete the comment as it's speculative ("may be fragile if...") and doesn't provide clear, actionable guidance. It would require more context to determine if this is actually a problem.
6. packages-v1/api-v1/trpc/utils/pagination.ts:52
- Draft comment:
Mapping items with r[entityKey] using 'any' reduces type safety. Consider enforcing stricter types or additional checks. - 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%
While usinganydoes reduce type safety, the function is already using advanced TypeScript features for type inference in the return type. Theanyhere might be necessary due to the database query result being dynamically typed. Without seeing the schema types and database layer, we can't be certain if there's a better way to type this.
I might be too lenient on type safety. The use ofanycould mask real runtime errors that TypeScript could catch.
However, given this is a database query result and we're using a generic type parameter with proper return type inference, theanyusage here may be unavoidable or require significant architectural changes to fix.
The comment should be deleted as it's not clearly actionable without major architectural changes, and the code already shows careful attention to types in the return signature.
7. packages-v1/api-v1/trpc/utils/pagination.ts:45
- Draft comment:
The conditional type using '$inferSelect' is complex. Adding a comment to document its purpose would improve maintainability. - 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%
While the type is indeed complex, it appears to be a standard TypeScript pattern for type inference from a schema. The type itself is clear in its purpose - it extracts the select type from the schema for a given entity. Adding a comment would not add significant value since the type's purpose is evident from its usage context and naming.
The type's complexity might genuinely make it harder for junior developers to understand. A comment could help explain the type inference pattern.
The type follows standard TypeScript patterns for type inference and is self-documenting through its clear usage context. Adding a comment would be redundant and could become outdated.
The comment should be deleted as it suggests documentation for a type that is already clear in its purpose and follows standard TypeScript patterns.
Workflow ID: wflow_R78Z6q6JzkmeRzCA
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.
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
Introduces new API methods for connections, connector configurations, and events, with pagination utilities and updated database schema.
getConnection,listConnections, andcheckConnectionmethods inconnection.ts.listConnectorConfigsmethod inconnectorConfig.ts.listEventsmethod inevent.ts.expandConnectorfunction inconnectorUtils.tsfor expanding connector details.applyPaginationAndOrderandprocessPaginatedResponseinpagination.ts.connection,pipeline,integration,connector_config, andeventtables inschema.tswith new policies and constraints.QueryBuilderexport inindex.ts.This description was created by
for 7199b96. It will automatically update as commits are pushed.