-
Notifications
You must be signed in to change notification settings - Fork 579
Port Change Bug Fix #1691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Port Change Bug Fix #1691
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 5c9d85a in 2 minutes and 25 seconds
More details
- Looked at
39
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. apps/studio/src/components/Modals/Settings/Project/index.tsx:40
- Draft comment:
Ensure that url.split(':').pop() reliably retrieves a valid port. Validate the URL format to avoid incorrect commands. - 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 concern - the code assumes the URL will have a port number after a colon and blindly uses split(':').pop(). This could fail if the URL is malformed or doesn't contain a port. However, the comment starts with "Ensure that..." which violates our rules. Additionally, URL validation would likely be handled elsewhere in the application.
I might be underestimating the severity of potential runtime errors this could cause. The code could break if given an invalid URL format.
While URL validation is important, this kind of "ensure that..." comment doesn't provide actionable feedback and asks the author to verify something rather than suggesting a specific fix.
Delete this comment since it starts with "Ensure that..." and asks for verification rather than providing a specific actionable suggestion for improvement.
2. apps/studio/src/components/Modals/Settings/Project/index.tsx:78
- Draft comment:
Restarting the runner on blur may introduce a lag. Confirm if this behavior is acceptable. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. apps/studio/src/components/Modals/Settings/Project/index.tsx:78
- Draft comment:
Restarting the runner on blur may trigger unnecessary restarts. Consider restarting only if the port value has changed. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. apps/studio/src/lib/projects/create.ts:147
- Draft comment:
Default run/build commands hard-code port 3000. Consider deriving the port from the projectUrl to ensure consistency if the URL changes. - 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 has merit since there's potential for inconsistency if projectUrl changes but commands don't. However, looking at the code, projectUrl is also hardcoded to 'http://localhost:3000'. There's no evidence that projectUrl is meant to be configurable. The comment is somewhat speculative - "if the URL changes" - but we don't see evidence that it's meant to change.
I might be missing configuration that happens elsewhere in the codebase that could change projectUrl. The hardcoding might be intentional as a default that gets overridden later.
Without seeing evidence that projectUrl is meant to be configurable, suggesting to make the port dynamic is premature optimization. The current implementation maintains consistency by using 3000 in both places.
Delete the comment as it's speculative and assumes projectUrl needs to be configurable, which we don't have evidence for.
5. apps/studio/src/components/Modals/Settings/Project/index.tsx:9
- Draft comment:
Typographical error: The import path './ReinstallButon' appears to be misspelled. It should likely be './ReinstallButton' to match the actual component name. - 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_ECEeI3eZp3cWUpf8
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.
@@ -145,8 +145,8 @@ export class CreateManager { | |||
const projectUrl = 'http://localhost:3000'; | |||
const projectCommands = { | |||
install: 'npm install', | |||
run: 'npm run dev', | |||
build: 'npm run build', | |||
run: 'npx next dev -p 3000', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default commands now use explicit npx
commands with -p 3000
; consider extracting the port to a variable for maintainability.
@@ -37,6 +37,11 @@ const ProjectTab = observer(() => { | |||
const handleUpdateUrl = (url: string) => { | |||
projectsManager.updatePartialProject({ | |||
url, | |||
commands: { | |||
...project?.commands, | |||
run: 'npx next dev -p ' + url.split(':').pop(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using url.split(':').pop()
to extract port is fragile. Consider using URL parsing (e.g., new URL(url).port
) for robust port extraction.
@Kitenite can you please check it out |
Description
This PR is an attempt to fix several bugs related to port change
Related Issues and Commits
#1165
#1673
#1571
#1690
Type of Change
Testing
No Tests Added
Screenshots (if applicable)
demo2.mp4
Additional Notes
There is a slight lag when changes are being applied due to restart of runner.
Important
Fixes port change bugs by updating commands and ensuring correct port usage in
index.tsx
andcreate.ts
.Settings
did not update the window port inindex.tsx
.run
andbuild
commands inindex.tsx
andcreate.ts
.onBlur
event to restart runner inindex.tsx
.run
andbuild
commands to usenpx next dev -p
andnpx next build -p
with the specified port inindex.tsx
andcreate.ts
.npx
for command execution increate.ts
.This description was created by
for 5c9d85a. It will automatically update as commits are pushed.