Kyle/create monorepo #738
Merged
Kyle/create monorepo #738
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
|
|
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 46ed2d7 in 1 minute and 30 seconds. Click for details.
- Reviewed
1245lines of code in6files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/react/package.json:2
- Draft comment:
Package name updated to '@codelayer/react'. Ensure this change is consistent with the monorepo naming conventions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. apps/react/src/components/Editor.tsx:31
- Draft comment:
Commented-out awareness handler remains. Remove or re-enable it if necessary to keep the code clean. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. apps/react/src/components/Editor.tsx:47
- Draft comment:
The useEditor hook no longer uses a dependency array; however, wrapping ActualEditor with a key prop based on documentId forces remounts. Confirm this behavior is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. apps/react/src/lib/electric-proxy.ts:23
- Draft comment:
Consider removing commented-out console.log statements to reduce clutter. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. apps/react/src/y-electric/index.ts:70
- Draft comment:
The 'resume' property for offline support is added. Consider using the updateShapeState function (lines 290–297) to persist resume state, or remove it if not needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. apps/react/src/y-electric/index.ts:376
- Draft comment:
Inside the pushLocalChanges subscription, calling pushLocalChangesUnsubscribe() immediately unsubscribes. Verify that handling only one update is the intended behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. apps/react/src/y-electric/index.ts:357
- Draft comment:
Commented-out unsubscribeAwarenessHandler code is present. Consider removing it if awareness resume is not implemented to reduce dead code. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. apps/react/src/y-electric/index.ts:278
- Draft comment:
Typo: 'paserToTimestamptz' appears to be misspelled. It should likely be 'parseToTimestamptz' or a similar correct identifier. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_A4L4fARdxTcQ7i41
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to fe34e2d in 2 minutes and 57 seconds. Click for details.
- Reviewed
6170lines of code in84files - Skipped
9files when reviewing. - Skipped posting
16draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/daemon/src/index.ts:8
- Draft comment:
Consider adding more robust error handling in the onError interceptor beyond just console.error. - 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% While the suggestion isn't wrong, it's somewhat vague and doesn't provide specific guidance. The current error handling might be intentionally simple for initial setup. Without knowing the project's error handling requirements or seeing how errors are handled elsewhere, we can't be certain this needs to be more robust. The comment could be valid if this is production code that needs proper error tracking. Basic console.error might miss critical errors. However, the comment doesn't provide specific actionable guidance and assumes requirements we don't know about. It's more of a general suggestion than a concrete issue. The comment should be removed as it's too vague and makes assumptions about requirements we don't have evidence for.
2. apps/react/build.ts:36
- Draft comment:
Review the argument parsing logic to ensure it correctly handles edge cases such as decimal numbers and comma‐separated lists. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to review the argument parsing logic for edge cases. This falls under asking the author to ensure behavior is intended or to double-check things, which is against the rules.
3. apps/react/package.json:28
- Draft comment:
React version '^19' is unusual; confirm that this cutting‐edge version is intended for production. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding the use of an unusual React version. This violates the rule against asking for confirmation of intention. The comment does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
4. apps/react/src/y-electric/index.ts:335
- Draft comment:
The subscription for local changes uses a self-unsubscribing pattern; consider using a one-time subscription or clearer logic to avoid potential side effects. - 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 current implementation is actually using a self-unsubscribing pattern intentionally to handle the initial connection state and sync offline changes exactly once. This is a valid approach and is commonly used. The code is clear about its intention - it sets up connection state and syncs offline changes on first connection, then cleans itself up. A one-time subscription wouldn't necessarily be clearer. The comment suggests alternatives but doesn't identify any actual problems with the current implementation. The current pattern is actually quite standard for one-time event handling. The current implementation is already effectively a one-time subscription pattern, just implemented manually. There's no evidence of potential side effects. The comment should be deleted as it suggests changes without identifying actual problems, and the current implementation is already using a valid and common pattern for one-time event handling.
5. packages/contracts/src/daemon/openapi.ts:9
- Draft comment:
Top-level await is used to generate the OpenAPI spec; ensure that your build environment supports top-level await in ESM modules. - 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 their build environment supports a specific feature, which violates the rule against asking the author to ensure something. It doesn't provide a specific code suggestion or ask for a test to be written.
6. packages/database/drizzle/0001_new_dazzler.sql:8
- Draft comment:
Remove or adapt the 'statement-breakpoint' comments before production migrations. - 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% Statement-breakpoints are commonly used in migration tools like Drizzle to separate SQL statements. They are not harmful in production and are often part of the migration tool's functionality. The comment seems to be making assumptions about migration practices without strong evidence that these comments cause issues. This appears to be a speculative suggestion rather than pointing to a concrete problem. Maybe these statement-breakpoints could cause issues in certain deployment environments or with certain database tools? Maybe there's a specific reason why they should be removed that I'm not aware of? Without concrete evidence that these statement-breakpoints cause problems, and given that they're likely part of the migration tool's standard functionality, suggesting their removal seems more likely to cause problems than solve them. Delete this comment as it makes a speculative suggestion without strong evidence of an actual problem, and the statement-breakpoints are likely intentional and functional parts of the migration system.
7. packages/database/schema/thoughts.ts:20
- Draft comment:
Consider adding a foreign key constraint for 'organization_id' for ensuring data integrity in the thoughtsDocuments table. - 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's own comment explicitly acknowledges that a foreign key is missing and should be added later. This means the author is already aware of this issue. The PR comment is just repeating what's already documented in the code itself. Additionally, the code comment suggests this is intentionally omitted for testing purposes. The comment does point out an important data integrity concern that should eventually be addressed. Maybe the inline code comment isn't prominent enough? The code already has an explicit comment about this exact issue, making the PR comment redundant. The omission is intentional for testing purposes. Delete the comment since it's redundant with the code's own documentation and the omission is intentional for testing.
8. thoughts/searchable/shared/research/2025-10-04-vite-tauri-integration.md:4
- Draft comment:
The research document is comprehensive; ensure revisions and updates as architecture changes over time. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
9. apps/daemon/src/router/index.ts:4
- Draft comment:
There is a typographical error in the comment. It currently reads "basedthe imported server". Please update it to "based on the imported server". - 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 typo, it's an extremely minor issue that doesn't affect code functionality or readability significantly. Comments about pure typos, especially in comments themselves, are generally too trivial to warrant a PR comment. This falls under the "Do NOT make comments that are obvious or unimportant" rule. The typo could potentially indicate a lack of attention to detail that might extend to more important aspects of the code. Documentation quality is important. While documentation quality matters, this is such a minor typo that it could be fixed in a subsequent PR or ignored without any real impact. The meaning is still clear despite the typo. Delete this comment as it's too trivial and doesn't meet the bar for important PR feedback.
10. apps/react/src/components/ui/select.tsx:88
- Draft comment:
Typo: The utility classes*:[span]:last:flex,*:[span]:last:items-center, and*:[span]:last:gap-2appear to contain a typographical error. They might be intended as[&>span:last:flex],[&>span:last:items-center], and[&>span:last:gap-2]respectively. - 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 a new file, so all code is changed. The comment suggests changing the syntax, but I'm not 100% confident that the original syntax is wrong - Tailwind has complex selector syntax and without deep knowledge of the specific selectors being used here, I can't be certain the original isn't intentional. The component seems to be working as part of a UI library. I don't have definitive evidence that the original syntax is incorrect. The current implementation might be intentionally using a different selector pattern than what the comment suggests. Given that this is a UI component library and the code is new, we should trust that the authors tested these selectors and they work as intended. Without clear evidence of a problem, we shouldn't suggest changes. Delete the comment since we don't have strong evidence that the current selector syntax is incorrect, and the component likely works as intended.
11. apps/react/src/y-electric/utils.ts:38
- Draft comment:
Typo: It looks like 'paserToTimestamptz' is misspelled. Consider renaming it to 'parseToTimestamptz' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The typo is clear and obvious. The comment is about a real issue in the code. However, this kind of issue would likely be caught by linting or code review anyway. Variable rename suggestions are specifically mentioned in the rules as something not to comment on since they would be caught by the build. The typo could impact code searchability and maintainability. It's a real issue that should be fixed. While true, the rules explicitly state not to comment on variable renames as these would be caught by the build process. Following the rules strictly, we should delete this comment as variable rename suggestions are explicitly listed as something not to comment on.
12. packages/contracts/tsconfig.types.json:8
- Draft comment:
There is an extra trailing comma after "rootDir": "./src". In strict JSON, trailing commas are not allowed and may lead to parsing errors. Consider removing it. - 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% TypeScript config files (.tsconfig.json) actually support trailing commas and comments, unlike strict JSON. They use a more relaxed JSON5-like format. The comment is technically incorrect in assuming this would cause parsing errors. Even if it was strict JSON, this would be a trivial issue that would be caught immediately by any build or validation. Maybe there's a specific reason they want to enforce a no-trailing-comma style across all JSON files for consistency? No, TypeScript configs specifically allow trailing commas by design, and enforcing JSON style should be handled by linting rules, not PR comments. Delete this comment as it's incorrect about TypeScript configs and would be caught by linting/building if it was actually an issue.
13. packages/database/.gitignore:16
- Draft comment:
Typographical Concern: The pattern 'report.[0-9].[0-9].[0-9].[0-9].json' seems unusual. Verify if the underscores are intended here or if a wildcard (e.g., '*') was meant to be used. - 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 this is technically about a change (new file), .gitignore patterns are very project-specific and the current pattern might be intentionally matching specific report files with underscores. Without knowing the actual report file naming convention used in this project, suggesting a change could be wrong. The pattern works either way - it's just a matter of which files need to be ignored. The pattern with underscores does look unusual, and asterisks would be more conventional for version numbers. Maybe this is a real issue? Even if unconventional, the current pattern is valid and might be specifically crafted for their report file naming scheme. We shouldn't assume their file naming convention. Delete this comment as it's making assumptions about the project's file naming conventions without strong evidence that the current pattern is wrong.
14. packages/database/drizzle/0001_new_dazzler.sql:12
- Draft comment:
Typo: The data type for the column 'op' is enclosed in quotes as "bytea". For consistency with other type declarations, please remove the quotation marks (i.e., use bytea without quotes). - 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 suggests removing quotes for consistency, but examining the full file shows that quoted types are actually the consistent pattern. The comment is therefore incorrect in its reasoning. Additionally, both forms (quoted and unquoted) are valid in PostgreSQL, so this is purely a style choice with no functional impact. Could there be a broader project-wide style guide that prefers unquoted types that I'm not aware of? Is there a technical reason to prefer unquoted types in PostgreSQL? Even if there is a broader style guide, this file consistently uses quotes, and changing just one instance would create inconsistency. The technical validity is the same either way. The comment should be deleted because it suggests breaking the file's internal consistency and the change would have no functional impact.
15. packages/database/drizzle/0001_new_dazzler.sql:18
- Draft comment:
Typo: The data type for the column 'op' is unnecessarily quoted as "bytea". It would be clearer and more standard to declare it as bytea (without quotes). - 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 it's true that bytea doesn't strictly need quotes in PostgreSQL, having quotes around type names is also valid PostgreSQL syntax and is consistently used throughout this file for all column names and types. This appears to be a deliberate styling choice. The suggestion wouldn't change functionality and is purely stylistic. Given that it's consistent with the file's style, changing it would actually reduce consistency. The comment is technically correct that quotes aren't needed. Maybe there's a team style guide that prefers unquoted types? Without access to team style guides, we should respect the consistent style chosen in this file. The current style is valid PostgreSQL syntax. Delete the comment. While technically correct, it's a purely stylistic suggestion that would make the file less consistent internally.
16. scripts/README.md:12
- Draft comment:
There's a trailing whitespace afterbun run. Consider removing the extra space to keep the command clear. - 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 trailing space, this is an extremely minor formatting issue in a documentation file. It doesn't affect functionality, readability is barely impacted, and it's not the kind of issue that warrants a PR comment. This feels like unnecessary nitpicking that adds noise to the review process. The trailing space could potentially cause issues if someone copies and pastes the command directly from the README. Some terminals might interpret the trailing space differently. Even in a copy-paste scenario, most shells handle trailing spaces gracefully, and this is such a basic command that users are likely to type it manually anyway. This comment should be deleted as it's too minor of an issue to warrant attention in a code review. It violates the rule about not making obvious or unimportant comments.
Workflow ID: wflow_OP8WRurFQuBET850
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| return | ||
| } | ||
|
|
||
| const html = ` |
Contributor
There was a problem hiding this comment.
Extract the inline HTML template into a separate file for better maintainability.
| }), | ||
| }, | ||
| ) | ||
| if (response.ok) { |
Contributor
There was a problem hiding this comment.
Enhance error handling in createDocument; consider handling non-OK responses and providing user feedback.
apps/react/src/components/Editor.tsx
Outdated
| { | ||
| extensions: createTiptapExtensions(provider), | ||
| }, | ||
| [documentId], |
Contributor
There was a problem hiding this comment.
Review the dependency array for useEditor; consider if additional dependencies (e.g., electricUrl) should be included.
Suggested change
| [documentId], | |
| [documentId, electricUrl], |
| ## Notes | ||
|
|
||
| - **IndexedDB persistence is NOT included** - removed to keep implementation simple | ||
| - Each browser tab creates its own Y.js client with unique clientID |
Contributor
There was a problem hiding this comment.
Typo: The term "clientID" is used here, but elsewhere in the code it's referred to as "clientId". Please update for consistency.
Suggested change
| - Each browser tab creates its own Y.js client with unique clientID | |
| - Each browser tab creates its own Y.js client with unique clientId |
| <SelectPrimitive.Item | ||
| data-slot="select-item" | ||
| className={cn( | ||
| "focus:bg-accent focus:text-accent-foreground [&_svg:not([class*='text-'])]:text-muted-foreground relative flex w-full cursor-default items-center gap-2 rounded-sm py-1.5 pr-8 pl-2 text-sm outline-hidden select-none data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 *:[span]:last:flex *:[span]:last:items-center *:[span]:last:gap-2", |
Contributor
There was a problem hiding this comment.
Typo: In the SelectItem component's className, outline-hidden is not a standard Tailwind utility. Did you mean outline-none?
Suggested change
| "focus:bg-accent focus:text-accent-foreground [&_svg:not([class*='text-'])]:text-muted-foreground relative flex w-full cursor-default items-center gap-2 rounded-sm py-1.5 pr-8 pl-2 text-sm outline-hidden select-none data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 *:[span]:last:flex *:[span]:last:items-center *:[span]:last:gap-2", | |
| "focus:bg-accent focus:text-accent-foreground [&_svg:not([class*='text-'])]:text-muted-foreground relative flex w-full cursor-default items-center gap-2 rounded-sm py-1.5 pr-8 pl-2 text-sm outline-none select-none data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 *:[span]:last:flex *:[span]:last:items-center *:[span]:last:gap-2", |
| import * as awarenessProtocol from 'y-protocols/awareness' | ||
| import * as syncProtocol from 'y-protocols/sync' | ||
| import * as Y from 'yjs' | ||
| import { parseToDecoder, parseToDecoderLazy, paserToTimestamptz } from './utils' |
Contributor
There was a problem hiding this comment.
Typographical error: the import name 'paserToTimestamptz' appears to be misspelled. Consider renaming it to 'parseToTimestamptz' (or the intended name) to improve clarity.
|
|
||
| # logs | ||
| logs | ||
| _.log |
Contributor
There was a problem hiding this comment.
Typo Alert: The entry '_.log' on this line might be a mistake. Typically, log files are ignored using '*.log'. Please confirm if this was intended.
Suggested change
| _.log | |
| *.log |
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.
Create monorepo structure with example apps (does not affect existing build/make or applications); towards incremental adoption of monorepo and architecture
Important
Introduces a monorepo structure with example apps using Bun, focusing on collaborative editing and server operations with PostgreSQL and ElectricSQL.
daemonandreact.turbo.jsonfor task management with Turborepo.index.ts,swagger.ts).@orpc/openapiand@orpc/server.router/index.tsandsessions.ts.App.tsx,Editor.tsx).build.ts,bunfig.toml).APITester.tsx).thoughts.ts,y-schema.sql).database.ts,drizzle.config.ts).docker-compose.yml).biome.jsonc).This description was created by
for 46ed2d7. You can customize this summary. It will automatically update as commits are pushed.