Add web app #1702
Merged
Add web app #1702
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
![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 318c1f4 in 3 minutes and 28 seconds
More details
- Looked at
3097lines of code in36files - Skipped
2files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. package.json:19
- Draft comment:
Script 'build:cli' filters using 'onlook' (e.g. bun --filter onlook typecheck), but all packages seem to be namespaced (e.g. '@onlook/foundation' or '@onlook/web'). Verify that the filter pattern is correct. - 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.
2. package.json:12
- Draft comment:
Workspaces are defined correctly covering packages, apps, and tooling. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. package.json:12
- Draft comment:
The workspaces array now uses 'apps/*' instead of just 'apps/studio'. Confirm that this change intentionally brings in all app packages (e.g. web, backend, studio) without pulling in unintended directories. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. tooling/typescript/next-react.json:22
- Draft comment:
The TSConfig settings align with Next.js best practices. Consider verifying if 'checkJs: true' is required (if JS files aren’t extensively used) and ensure 'moduleResolution' set to 'Bundler' is intentional for your build setup. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/web/.env.example:3
- Draft comment:
There's no newline at the end of the file. While this is a minor stylistic issue, it's good practice to end files with a newline to meet common formatting conventions. - 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 having a newline at end of file is indeed a common convention, this is: 1) A very minor formatting issue 2) Something that should be handled by .editorconfig or similar tooling 3) Not related to the actual functionality or purpose of this example env file. The comment doesn't suggest any meaningful code improvements.
The missing newline could potentially cause issues with some tools or shell scripts that expect files to end with newlines.
While true, this is still just a formatting issue that should be handled by automated tooling rather than manual review comments. The primary purpose of code review is to catch logical issues and improve code quality.
Delete this comment as it's too minor of a formatting issue and doesn't suggest any meaningful code improvements.
6. apps/web/src/app/api/trpc/[trpc]/route.ts:10
- Draft comment:
Typographical suggestion: In the comment on line 10, consider changing 'a HTTP request' to 'an HTTP request' for improved grammatical correctness. - 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 technically correct (we use "an" before words that sound like they start with a vowel, and "HTTP" is pronounced "aitch-tee-tee-pee"), this is an extremely minor documentation issue. The current wording is understandable and doesn't impact code quality or functionality. The rules state not to make purely informative comments or obvious/unimportant changes.
The comment is technically correct and would improve the documentation's grammar. Poor documentation can be considered a code quality issue.
While correct, this is an extremely minor documentation issue that doesn't impact understanding or code quality significantly. The rules specifically state not to make purely informative comments or unimportant changes.
This comment should be deleted as it's too minor and doesn't require a code change that would meaningfully improve the codebase.
7. apps/web/src/app/auth/signout/route.ts:8
- Draft comment:
Consider updating the informal contraction in the comment on line 8. Instead of "Check if a user's logged in", it could be changed to "Check if a user is logged in" for improved clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%
The comment is suggesting a change in the wording of a comment in the code for clarity. This is a stylistic suggestion and does not impact the functionality of the code. The rules do not explicitly prohibit stylistic suggestions, but they do emphasize avoiding purely informative comments. This suggestion is not about code functionality or testing, so it might not be considered useful in the context of the rules.
8. apps/web/src/server/api/trpc.ts:35
- Draft comment:
Minor suggestion: Consider changing 'typesafety' to 'type safety' (two words) to improve readability. - 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 technically correct that 'type safety' is the more common spelling, this is a very minor documentation change that doesn't affect functionality. The meaning is perfectly clear either way. This feels like unnecessary nitpicking that doesn't improve code quality in any meaningful way.
The suggestion is technically correct and could make the documentation slightly more professional. Some style guides might require consistent terminology.
While technically correct, this level of nitpicking on documentation wording doesn't provide enough value to justify the comment. The current spelling is perfectly understandable.
Delete this comment as it's too minor and doesn't meaningfully improve code quality. It's just nitpicking documentation formatting.
9. apps/web/src/utils/supabase/middleware.ts:34
- Draft comment:
The file is missing a newline at the end. Adding a newline can improve compatibility with many tools and adhere to best practices. - 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 missing newlines at end of file can cause minor issues with some tools, this is a very minor issue that would typically be handled automatically by IDE settings or linting rules. It's not a significant enough issue to warrant a PR comment, as it doesn't affect functionality and is more of a formatting preference.
The comment is technically correct and follows a common best practice. Some version control systems and tools do work better with newlines at end of file.
While technically correct, this is too minor of an issue to warrant a PR comment. This should be handled by automated tooling rather than manual review comments.
Delete this comment as it's too minor of an issue and would be better handled by automated tooling or IDE settings.
10. tooling/typescript/package.json:6
- Draft comment:
The file is missing a newline at the end. Adding a newline improves compatibility with various tools and adheres to common conventions. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_31I2lcA4Q1XWr1Ap
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.
|
|
||
| We try to keep this project as simple as possible, so you can start with just the scaffolding we set up for you, and add additional things later when they become necessary. | ||
|
|
||
| If you are not familiar with the different technologies used in this project, please refer to the respective docs. If you still are in the wind, please join our [Discord](https://t3.gg/discord) and ask for help. |
There was a problem hiding this comment.
The phrase "if you still are in the wind" on line 9 is non-standard and may confuse readers. Consider rephrasing it to a more common expression such as "if you are still lost" or "if you still need help".
Suggested change
| If you are not familiar with the different technologies used in this project, please refer to the respective docs. If you still are in the wind, please join our [Discord](https://t3.gg/discord) and ask for help. | |
| If you are not familiar with the different technologies used in this project, please refer to the respective docs. If you are still lost, please join our [Discord](https://t3.gg/discord) and ask for help. |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on c440be4 in 3 minutes and 3 seconds
More details
- Looked at
441lines of code in12files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/web/src/app/login/actions.tsx:9
- Draft comment:
Check that the 'origin' header exists before using it in the redirect URL. - 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 raises a valid security concern - if origin is null, the redirectTo URL would be malformed. However, this is using Next.js's headers() API which is a server-side API. In Next.js server components/actions, the origin header should always be present as it's a standard HTTP header. The comment seems overly cautious for a standard Next.js pattern.
I might be wrong about the origin header always being present - there could be edge cases where it's missing. Also, defensive programming is generally good practice.
While defensive programming is good, this is using Next.js's standard headers API in a server action, where the origin header is guaranteed by the HTTP spec. Adding a check would be unnecessary defensive programming.
The comment should be deleted as it suggests adding unnecessary checks for a standard Next.js pattern using a guaranteed HTTP header.
2. apps/web/src/app/login/page.tsx:8
- Draft comment:
Using a server action directly in a client component via onClick may not be supported; ensure compatibility with Next.js Server Actions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is suggesting to ensure compatibility with Next.js Server Actions, which is a form of asking the author to double-check something. This violates the rule against asking the author to ensure behavior is intended or tested.
3. apps/web/src/app/auth/callback/route.ts:12
- Draft comment:
Consider handling the error from exchangeCodeForSession more robustly instead of only logging it. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. apps/web/src/app/login/actions.tsx:9
- Draft comment:
Ensure the 'origin' header is defined to avoid potential null values when constructing the redirect URL. - 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 origin header is a standard header that should always be present in web requests. Next.js's headers() function is used in a server component context, which guarantees request headers. If origin was undefined, it would indicate a serious server configuration issue that should fail fast rather than use a default value. Using a default origin for OAuth callback could create security issues.
Maybe there are edge cases where the origin header could be missing, like in certain test environments or unusual deployment configurations.
Even if there are edge cases, using a default origin for OAuth would be dangerous as it could enable redirect attacks. Better to fail fast if the header is missing.
The comment should be deleted. Adding a default origin value could mask configuration issues and potentially create security vulnerabilities.
5. apps/web/src/app/login/actions.tsx:27
- Draft comment:
Remove or conditionally disable the console.log for data to avoid leaking sensitive information in production. - Reason this comment was not posted:
Marked as duplicate.
6. apps/web/src/middleware.ts:18
- Draft comment:
Including '/' in the matcher may cause the middleware to run on all requests; verify that this is the intended behavior considering the performance impact. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
7. apps/web/src/utils/supabase/middleware.ts:17
- Draft comment:
Consider refactoring the cookies.setAll logic to avoid repeatedly reinitializing NextResponse; this may simplify cookie management. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. apps/web/src/app/projects/page.tsx:3
- Draft comment:
It appears that the file lacks a trailing newline at the end. Adding a newline at the end of the file would improve consistency with many coding style guidelines. - 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 missing trailing newlines are a common style guideline, this is exactly the kind of minor formatting issue that should be handled by automated tooling like ESLint or Prettier. It's not a logic issue, and it's too minor to warrant manual intervention. Most IDEs and formatting tools will handle this automatically.
The comment is technically correct - there is no trailing newline. And some tools do work better with trailing newlines.
However, this is exactly the kind of minor formatting issue that should be handled by automated tooling, not manual code review comments. It adds noise without providing significant value.
Delete this comment as it's too minor and should be handled by automated formatting tools rather than manual review.
9. apps/web/src/utils/supabase/middleware.ts:33
- Draft comment:
There is no newline at the end of the file. While this is a trivial style issue, many style guidelines recommend ending files with a newline for consistency. Please consider adding a newline at the end of the file. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_27O84ETjlS9FpWDx
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.
| return NextResponse.redirect(`${origin}${next}`) | ||
| } | ||
| } | ||
| console.log(error) |
There was a problem hiding this comment.
Avoid logging auth errors to the console; use proper error handling to avoid exposing sensitive details.
| redirect('/error') | ||
| } | ||
|
|
||
| console.log(data) |
There was a problem hiding this comment.
Remove or disable debug logging (console.log(data)) before production deployment.
Suggested change
| console.log(data) |
apps/web/src/app/login/page.tsx
Outdated
| export default function LoginPage() { | ||
| return ( | ||
| <> | ||
| <button onClick={() => login('github')}>GitHub</button> |
There was a problem hiding this comment.
Invoking a server action via an onClick handler may not work as expected; consider using a form action for server actions.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f07c9a9 in 3 minutes and 50 seconds
More details
- Looked at
1828lines of code in15files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. apps/web/next.config.ts:1
- Draft comment:
Ensure that the next-intl plugin configuration is working as expected with a static locale. If dynamic locale detection is needed, update accordingly. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. apps/web/src/app/layout.tsx:8
- Draft comment:
NextIntlClientProvider is used without props. Typically, it should be supplied with locale and messages. Confirm if this 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 = 20% vs. threshold = 50%
The comment is asking for confirmation rather than pointing out a definite issue. It's speculative and starts with "Confirm if..." which violates our rules. Without deep knowledge of next-intl's internals, we can't be certain this is actually a problem. The author is already handling locale at the HTML level.
Maybe there are serious implications of not passing these props that could cause runtime issues. The library documentation might require these props.
Even if the props might be useful, the comment is phrased as a question asking for confirmation rather than stating a definite issue. We should trust that the author tested their implementation.
Delete this comment because it's speculative and asks for confirmation rather than pointing out a definite issue.
3. apps/web/src/app/login/page.tsx:4
- Draft comment:
The login page uses localStorage to track the last sign-in method. Consider adding error handling if localStorage is unavailable. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. apps/web/src/app/projects/layout.tsx:14
- Draft comment:
Good use of the Routes constant for redirection. Ensure consistency across the app for route strings. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. General:1
- Draft comment:
The issue description specifies adding onboarding documentation for Next.js, Remix, Webpack, and Babel, but this PR does not appear to include any such documentation. Please confirm if documentation files are missing or if the issue description needs updating. - 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. apps/web/messages/ja.json:22
- Draft comment:
Inconsistent interpolation syntax: double curly braces (e.g. "{{total}}のうち{{current}}") are used here, whereas the English file uses single braces. Please verify and standardize the placeholder format across locales. - 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%
- This is a new file being added, so any changes would be relevant. 2. The comment is about a potential cross-file issue comparing to the English file. 3. Per the rules, we should "Ignore cross-file issues. Only think about the file you are reviewing." 4. Without seeing the English file, we can't verify if there's actually an inconsistency.
The interpolation format could be a project-wide standard that's important for the application to function correctly. Maybe this isn't just a style issue.
Even if it is important, we can't verify the claim without seeing the English file. The rules explicitly say to ignore cross-file issues and to only keep comments we have strong evidence for.
Delete the comment because it requires comparing against another file we can't see, and the rules say to ignore cross-file issues.
7. apps/web/src/i18n/request.ts:3
- Draft comment:
The locale is hard-coded to 'en'. If multi-language support is intended, consider detecting the locale dynamically (e.g. from headers or cookies). - 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's own comments at L4-5 already acknowledge that this could be made dynamic. The comment is essentially restating what the code's comments already say. Additionally, this is a new file, so the author clearly knows about i18n and made an intentional choice to start with English only. The comment isn't pointing out a bug or issue, just suggesting a potential future enhancement that the author is already aware of.
The comment could be valuable if this is meant to be production code and English-only would be a serious limitation. The author's intention for internationalization scope isn't clear from just this file.
The code's own comments show the author is aware of the options. If multi-language support becomes needed, they can easily change this later following their own comment's guidance.
Delete the comment as it's not pointing out anything the author isn't already aware of, and the current implementation may be intentionally English-only for now.
8. apps/web/next.config.ts:1
- Draft comment:
The issue description mentions adding framework documentation (Next.js, Remix, Webpack, Babel) for better onboarding, but no documentation files or content are visible. Please confirm that the required onboarding docs are included. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm whether the documentation files are included, which violates the rule against asking for confirmation or ensuring behavior. It does not provide a specific code suggestion or point out a specific issue with the code itself.
9. apps/web/messages/ja.json:266
- Draft comment:
Consider revising the label for 'imageUpload'. The current text '画像参照をアップロード' might be misinterpreted. Perhaps changing it to '画像をアップロード' would be clearer. - 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 suggested translation is arguably more concise, the current translation "画像参照をアップロード" (upload image reference) is not incorrect - it's actually more specific about the purpose of the upload being for reference images. The difference is subtle and both versions would be understood by Japanese users. This seems like a matter of preference rather than a clear error.
The comment could be right that the simpler version is better for UI clarity. Japanese UI text often favors brevity.
However, the current translation provides more context about the purpose of the upload feature, which could be helpful for users. Neither version is wrong.
This is a subjective translation preference rather than a clear error or improvement. The existing translation is functional and clear enough.
10. apps/web/messages/ja.json:267
- Draft comment:
Check if 'ファイル参照' should be reworded to 'ファイルを参照' (or similar) for clarity, aligning with typical Japanese UI terminology. - 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 pure translation/UI suggestion. The current translation "ファイル参照" is actually a valid and commonly used Japanese UI term. While "ファイルを参照" is also valid, this is more of a stylistic preference than a clear error. According to our rules, we should not comment on pure UI changes unless there's a clear error.
Could this translation choice impact user understanding? Could "ファイル参照" be too ambiguous for some users?
"ファイル参照" is a standard term in Japanese UIs and would be readily understood by users. This is not a case where the current translation would cause confusion.
Delete this comment as it's suggesting a UI translation change that's purely stylistic, and the current translation is already valid and commonly used.
11. apps/web/src/app/layout.tsx:43
- Draft comment:
There's an extra space in the closing tag on line 43. It should be written as to follow standard conventions. - 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 technically correct, this is an extremely minor formatting issue that doesn't affect functionality. It's the kind of thing that would typically be handled by a code formatter or linter. The comment feels overly pedantic and doesn't improve code quality in any meaningful way.
The space in the HTML tag could be a symptom of inconsistent code formatting practices that might be worth addressing systematically.
Even if there are formatting inconsistencies, addressing them one character at a time via PR comments is not productive. This should be handled by automated tooling.
Delete this comment as it's too minor and doesn't meaningfully improve code quality. This kind of formatting issue should be handled by automated tooling rather than manual review comments.
Workflow ID: wflow_mO8ZITpd4NJmpw6s
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.
|
|
||
| const handleSignOut = async () => { | ||
| await supabase.auth.signOut(); | ||
| redirect(Routes.LOGIN); |
There was a problem hiding this comment.
Avoid using next/navigation's redirect() in a client component. Switch to useRouter() and router.push() for client-side navigation.
| <html lang={locale} className={`${geist.variable}`}> | ||
| <body> | ||
| <TRPCReactProvider> | ||
| <NextIntlClientProvider> |
There was a problem hiding this comment.
NextIntlClientProvider is used without passing locale and messages. To ensure proper localization, consider providing these props or integrating with the getRequestConfig setup.
|
Generated with ❤️ by ellipsis.dev |
|
Generated with ❤️ by ellipsis.dev |
|
Generated with ❤️ by ellipsis.dev |
zongkelong
pushed a commit
to zongkelong/coolook
that referenced
this pull request
Apr 3, 2025
…slation_zh * 'main' of https://github.com/onlook-dev/onlook: fix: add validation when create new color group (onlook-dev#1703) Disable web when running dev (onlook-dev#1718) feat: font panel config (onlook-dev#1666) Add web app (onlook-dev#1702)
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* Add web folder * Integrate with monorepo * Implement auth * Redirect auth * Working redirect * Working auth * Add intl * Add shadcn * Upgrade to tailwind v4 * Restore old ui * Move variables
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* Add web folder * Integrate with monorepo * Implement auth * Redirect auth * Working redirect * Working auth * Add intl * Add shadcn * Upgrade to tailwind v4 * Restore old ui * Move variables
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.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Add a new web app with components, localization, authentication, routing, and TRPC API setup.
apps/webwith initial setup and configuration.package.jsonfor dependencies and scripts.postcss.config.jsfor PostCSS setup.Maincomponent inmain.tsxfor the main layout with user authentication handling.LatestPostcomponent inpost.tsxfor displaying and creating posts.ThemeProviderintheme.tsxfor theme management.en.json,ja.json,ko.json,zh.jsoninmessagesfor multi-language support.auth/callback/route.tsandauth/confirm/route.ts.login/actions.tsxand error handling inlogin/error.tsx.layout.tsxandprojects/layout.tsx.middleware.tsfor session updates.api/trpc/[trpc]/route.tsandserver/api/root.ts.postRouterinserver/api/routers/post.tsfor post-related operations.eslint.config.js.next-react.json.bun.lockandpackage.jsonfor new dependencies and scripts.This description was created by
for f07c9a9. It will automatically update as commits are pushed.