Add test for connection router #299
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}
| viewer: currentViewer, | ||
| db, | ||
| }: CreateRouterContextOptions): RouterContext { | ||
| function createViewerContext(viewer: Viewer): ViewerContext { | ||
| }: CreateRouterContextOptions<T>): RouterContext<T> { |
There was a problem hiding this comment.
The function declares it returns RouterContext but the actual return value type is inconsistent because createViewerContext returns ViewerContext instead of ViewerContext. This creates a type mismatch that could lead to runtime type errors.
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 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 bc2d075 in 1 minute and 28 seconds
More details
- Looked at
143lines of code in3files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. packages/api-v1/trpc/context.ts:51
- Draft comment:
Return type of createViewerContext should preserve the viewer generic, e.g. ViewerContext instead of ViewerContext. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/api-v1/trpc/_base.ts:13
- Draft comment:
Consider parameterizing the 'as' method to preserve the generic type (e.g. use T instead of Viewer) for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
3. packages/api-v1/trpc/routers/connection.spec.ts:34
- Draft comment:
Dependent tests can be brittle; consider isolating tests if possible for improved robustness. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. packages/api-v1/trpc/context.ts:18
- Draft comment:
Consider revising the comment on line 18: 'JWT always include a dot. Without a dot we assume it's an API key' to 'JWT always includes a dot. Without a dot, we assume it's an API key' for correct subject-verb agreement and clarity. - 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/api-v1/trpc/routers/connection.spec.ts:34
- Draft comment:
Typographical error: Consider changing 'simplicty' to 'simplicity' for clarity. - 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 the typo correction is technically accurate, it's in a comment rather than actual code. Comments are documentation and typos in them don't affect functionality. The rules state we should not make purely informative comments or comments that are obvious/unimportant. A typo in a comment falls into this category.
The typo could potentially confuse future readers of the code. Documentation quality is important for maintainability.
While documentation quality matters, this particular typo is minor and obvious enough that readers would still understand the meaning. The cost of the review comment outweighs the benefit.
Delete this comment as it's too minor and doesn't affect code functionality. It violates the rule about not making purely informative or unimportant comments.
6. packages/api-v1/trpc/routers/connection.spec.ts:67
- Draft comment:
Typographical/lexical issue: The test title 'find own connection connection' seems to duplicate the word 'connection'. Consider renaming it to a clearer title (e.g., 'find own connection') or ensure the duplication is intentional. - 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 the comment is technically correct about the duplication, test names are primarily for developer readability and documentation. The meaning is still clear despite the typo. According to the rules, we should not make comments that are obvious or unimportant. This feels like a very minor issue that doesn't impact code functionality or maintainability.
The duplicated word could make the test slightly harder to read at first glance. It's a valid observation about code quality.
However, this is an extremely minor issue in a test file. The meaning is still clear, and fixing it provides minimal value.
This comment should be deleted as it violates the rule about not making comments that are obvious or unimportant. The typo is too minor to warrant a review comment.
Workflow ID: wflow_jx2yuneUYpRrzETa
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.
| db: AnyDrizzle | ||
| } | ||
|
|
||
| export interface RouterContext extends ViewerContext { | ||
| export interface RouterContext<T extends Viewer = Viewer> | ||
| extends ViewerContext<T> { | ||
| as: (viewer: Viewer) => ViewerContext |
There was a problem hiding this comment.
Consider making the as function generic. Currently its signature is (viewer: Viewer) => ViewerContext but it might be more type-safe as <U extends Viewer>(viewer: U) => ViewerContext<U>.
Suggested change
| as: (viewer: Viewer) => ViewerContext | |
| as: <U extends Viewer>(viewer: U) => ViewerContext<U> |
packages/api-v1/trpc/context.ts
Outdated
| }: CreateRouterContextOptions): RouterContext { | ||
| function createViewerContext(viewer: Viewer): ViewerContext { | ||
| }: CreateRouterContextOptions<T>): RouterContext<T> { | ||
| function createViewerContext<T2 extends Viewer>(viewer: T2): ViewerContext { |
There was a problem hiding this comment.
Update createViewerContext return type to ViewerContext<T2> to propagate the generic viewer type.
Suggested change
| function createViewerContext<T2 extends Viewer>(viewer: T2): ViewerContext { | |
| function createViewerContext<T2 extends Viewer>(viewer: T2): ViewerContext<T2> { |
| connId = res[0]!.id | ||
| }) | ||
|
|
||
| test('find own connection connection', async () => { |
There was a problem hiding this comment.
Test name contains duplicate 'connection'; consider renaming to 'find own connection'.
Suggested change
| test('find own connection connection', async () => { | |
| test('find own connection', async () => { |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 530c21e in 1 minute and 12 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. packages/api-v1/trpc/routers/connection.spec.ts:70
- Draft comment:
Test name simplified; ensure this renaming is reflected in any documentation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/api-v1/trpc/routers/connection.spec.ts:78
- Draft comment:
Commented-out code with a GROUP BY error note: consider linking to an issue or removing if not required. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. packages/api-v1/trpc/routers/connection.spec.ts:78
- Draft comment:
Avoid leaving commented-out test code. If 'listConnections' is expected to work, file a proper bug/ticket and either fix or remove the commented code. - 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%
Commented-out code with error messages can clutter the codebase and hide real issues. However, this appears to be actively being worked on - it includes a specific error message and tags a specific developer (@pellicceama). This looks more like a work-in-progress note than abandoned code. The comment may be premature since this could be mid-development.
The comment may be too aggressive - this could be an active development discussion between team members rather than abandoned code that needs cleanup.
While keeping code clean is important, we should allow for work-in-progress discussions in test files, especially when they include specific error messages and tag specific developers.
The comment should be removed as it's too aggressive for what appears to be an active development discussion in a test file.
4. packages/api-v1/trpc/routers/connection.spec.ts:78
- Draft comment:
Replace developer handle in TODO comments with a reference to a ticket or more descriptive note. - 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 commented code appears to be a work in progress documenting a failing test case. While having ticket references is generally good practice, this seems like temporary debugging code that will likely be fixed or removed entirely. The comment is more about style than substance, and doesn't address any actual code issues.
The comment could be valid if this was production code or if there was a policy about TODO comments, but this appears to be temporary debugging code.
Given this is likely temporary debugging code that will either be fixed or removed, nitpicking the TODO comment format adds unnecessary noise to the review.
The comment should be deleted as it's focusing on style in what appears to be temporary debugging code, rather than addressing actual code issues.
5. packages/api-v1/trpc/routers/connection.spec.ts:37
- Draft comment:
Typo: 'simplicty' should be corrected to 'simplicity'. - 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_lSLid4NtHGtVrcgY
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 3bd4763 in 1 minute and 33 seconds
More details
- Looked at
71lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages/api-v1/trpc/routers/connection.spec.ts:92
- Draft comment:
The window function query test assumes only one row is returned. If multiple rows exist, the count may be higher. Consider filtering the query or updating the expectation. - 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 test is well-controlled - it runs with a clean database and has explicit setup creating exactly one connection. The window function COUNT(*) OVER () is intentionally counting all rows to verify the query works correctly. The test is asserting both the count and the specific row details match expectations. The comment's suggestion to use res.length would actually test something different than the window function behavior.
The comment raises a valid point about window functions in general - they can behave differently with multiple rows. But am I missing something about test isolation?
The test environment is explicitly controlled with a clean database and specific setup. The test is specifically verifying window function behavior works as expected in this controlled case.
Delete this comment. The test is intentionally verifying window function behavior in a controlled environment, and the suggested change would test something different than intended.
2. packages/api-v1/trpc/routers/connection.spec.ts:115
- Draft comment:
Test name mismatch: the title 'does not find other customer connection' contradicts the expectation (length of 1). Update the test name to reflect that it retrieves the other customer's connection. - Reason this comment was not posted:
Marked as duplicate.
3. packages/api-v1/trpc/routers/connection.spec.ts:70
- Draft comment:
The use of interdependent tests (with $test saving state) can lead to brittle behavior. Consider isolating test data using beforeAll/afterAll hooks. - 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/api-v1/trpc/routers/connection.spec.ts:37
- Draft comment:
Typo: 'simplicty' should be corrected to 'simplicity' in the comment on line 37. - 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_2zf1Mk9uf6aqqVEe
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 b2352d4 in 1 minute and 37 seconds
More details
- Looked at
108lines of code in3files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. packages/api-v1/trpc/routers/connection.spec.ts:62
- Draft comment:
The TODO comment for validating connection id format is noted. Consider adding explicit assertions for the id format in the test to ensure consistency. - 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 redundant with the existing TODO comment. The code already shows awareness of ID format validation being needed. The review comment doesn't add any new information or specific actionable guidance beyond what's already acknowledged in the code.
The comment could be seen as helpful reinforcement of an important validation concern. Maybe having both a TODO and review comment would emphasize the importance.
While validation is important, duplicate reminders don't add value. The TODO is already tracking this issue and is more permanently visible in the codebase.
Delete the comment since it's redundant with the existing TODO comment and doesn't provide additional actionable guidance.
2. packages/api-v1/trpc/routers/connection.spec.ts:115
- Draft comment:
Tests using TRPC calls look good. It would be beneficial to include additional assertions for secret inclusions or edge cases in future tests. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. packages/api-v1/trpc/routers/connection.ts:146
- Draft comment:
Avoid the any casting for connection here. Instead, refine the type so that it matches core.connection 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.
4. packages/api-v1/models.ts:26
- Draft comment:
Reintroduce datetime validation instead of simply commenting it out. Consider using zod's .datetime() or a regex/custom refinement to enforce ISO date formats as noted in 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 = 20% vs. threshold = 50%
The change explicitly removed datetime validation and added a TODO, suggesting this was an intentional change. The author seems aware of the implications as evidenced by the TODO comment. The comment doesn't provide new information beyond what the TODO already states. While datetime validation would be good, the author has acknowledged this with the TODO.
The removal of datetime validation could lead to invalid data being accepted, making this a legitimate concern. The comment is technically correct about what should be done.
However, the author is clearly aware of this as shown by the TODO comment. The automated comment doesn't add any new information or insight beyond what's already acknowledged in the code.
Delete the comment since it merely restates what the author has already acknowledged in their TODO comment. It doesn't provide additional value.
5. packages/api-v1/trpc/routers/connection.spec.ts:43
- Draft comment:
Connector name updated from 'qbo' to 'greenhouse'. Verify that this change is consistently reflected across all tests and fixtures. - 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 consistency across tests and fixtures, which is against the rules. It does not provide a specific suggestion or point out a specific issue.
6. packages/api-v1/trpc/routers/connection.ts:178
- Draft comment:
Switched total count computation to use a window function (sqlcount(*) over ()). Ensure that this approach scales well and that client-side handling of the count per row is as expected. - 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 the approach scales well and that client-side handling is as expected. This falls under asking the author to ensure behavior is intended or tested, which violates the rules.
7. packages/api-v1/trpc/routers/connection.spec.ts:37
- Draft comment:
Typo found: In the comment at line 37, 'simplicty' should be corrected to 'simplicity' for clarity. - 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_bVnoRXI0EzvMqS7D
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 35c4f53 in 39 seconds
More details
- Looked at
28lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. packages/api-v1/trpc/context.ts:51
- Draft comment:
Improved type specificity by returning ViewerContext instead of generic ViewerContext. This ensures better type-checking. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/api-v1/trpc/routers/connection.spec.ts:124
- Draft comment:
Updated test description to match expected behavior: 'other customer finds own connection'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. packages/api-v1/trpc/context.ts:51
- Draft comment:
Improved generic typing: Using ViewerContext refines type safety for the viewer. Ensure that downstream usage correctly leverages this strengthened type constraint. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. packages/api-v1/trpc/routers/connection.spec.ts:124
- Draft comment:
Test description update: 'other customer also finds own connection' now clarifies that each customer retrieves only their own connection. Verify that this naming matches the intended behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. packages/api-v1/trpc/routers/connection.spec.ts:37
- Draft comment:
Typographical error found: In the comment on line 37, 'simplicty' should be corrected to 'simplicity'. - 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_HaL6HCfDhY9zjVwA
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 bee9202 in 39 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. packages/api-v1/trpc/routers/connection.ts:5
- Draft comment:
Removed unused import 'count'. Ensure it's not needed elsewhere. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that the removed import is not needed elsewhere, which violates the rule against asking the author to double-check things. The comment is not making a specific code suggestion or pointing out a specific issue.
2. packages/api-v1/trpc/routers/connection.ts:5
- Draft comment:
Removed unused 'count' import improves clarity. Confirm it isn't needed elsewhere. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%
The comment suggests removing an unused import, which is a valid code suggestion. However, it also asks the author to confirm that the import isn't needed elsewhere, which violates the rule against asking the author to confirm or double-check things. Therefore, this comment is not fully compliant with the rules.
Workflow ID: wflow_N4AUS3Bz8Wmne1IZ
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
Add test for connection router and enhance type support in context handling.
connection.spec.tsto test connection router functionality, including creating connections and verifying access for different roles.$testutility intest-utils.tsfor managing test references.ViewerContextandRouterContextin_base.tsandcontext.ts.coreBaseinmodels.tsto remove.datetime()fromupdated_atandcreated_atfields, with a TODO to ensure date format.This description was created by
for bee9202. It will automatically update as commits are pushed.