Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR refactors server port handling by decoupling it from the settings model. The port is now managed via AppStateService with dedicated API endpoints, a new v4 migration, and conditional UI components that hide port settings in Docker environments. Environment variable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alex-Arc
force-pushed
the
remove-serverport
branch
from
27c4520 to
d2c2e8e
Compare
alex-Arc
force-pushed
the
remove-serverport
branch
2 times, most recently
from
c0c8e19 to
322c73a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/server/src/api-data/settings/settings.router.ts`:
- Around line 69-72: Replace the backtick character in the error message sent
when isDocker is true with a proper apostrophe; update the string passed to
res.status(403).json(...) (the message used in the isDocker branch) from "Can`t
change port when running inside docker" to use "Can't change port when running
inside docker" so the punctuation is correct.
🧹 Nitpick comments (6)
apps/server/src/classes/data-provider/__tests__/DataProvider.utils.test.ts (1)
1-1: Resolve or track the FIXME before merge.Leaving a “FIXME: AI generated and not reviewed” in tests suggests unresolved work. Please remove it once reviewed or replace with a tracked ticket reference so it’s not forgotten.
apps/server/src/setup/environment.ts (1)
10-14: Consider simplifying the nested ternary for readability.The logic is correct, but the nested ternary can be harder to follow. A minor refactor could improve clarity:
♻️ Optional: Simplified port parsing
-export const startUpPort = process.env.PORT - ? !isNaN(Number(process.env.PORT)) - ? Number(process.env.PORT) - : null - : null; +const portEnv = process.env.PORT; +export const startUpPort = portEnv && !isNaN(Number(portEnv)) ? Number(portEnv) : null;apps/server/src/api-data/settings/settings.validation.ts (1)
33-35: Minor validation inconsistency with client-side.The server uses
isPort()which validates ports 1-65535, while the client-sideServerPortSettings.tsxrestricts input to 1024-65535 (excluding privileged ports). This means the API would accept ports < 1024, but the UI wouldn't allow submitting them.This is likely intentional (API is more permissive, UI enforces best practice), but worth confirming. If you want strict alignment, consider adding a custom validator:
♻️ Optional: Align with client-side validation
export const validateServerPort = [ - body('serverPort').isPort().withMessage('Invalid value found for server port').toInt(), + body('serverPort') + .isPort() + .withMessage('Invalid value found for server port') + .toInt() + .custom((value) => { + if (value < 1024) { + throw new Error('Port must be >= 1024 to avoid privileged ports'); + } + return true; + }), requestValidationFunction, ];apps/server/src/api-data/db/migration/db.migration.v4.ts (1)
13-26: Consider logging or migrating the removedserverPortvalue.The migration extracts
serverPort(line 15) but discards it. Per the PR description, this is intentional to avoid port-change-on-restart issues. However, consider whether the value should be logged for debugging purposes or migrated to the app-state file for users upgrading from older versions.apps/client/src/features/app-settings/panel/settings-panel/ServerPortSettings.tsx (2)
34-38: Redundant validation inonSubmit.The range validation (lines 35-38) duplicates the
min/maxrules inregister(lines 100-101). Since the submit button is disabled when!isValid(line 76), this check is unreachable during normal form submission. Consider removing the redundant check to reduce code duplication.♻️ Proposed simplification
const onSubmit = async (formData: ServerPortForm) => { - if (formData.serverPort < 1024 || formData.serverPort > 65535) { - setError('serverPort', { message: 'Port must be within range 1024 - 65535' }); - return; - } try { await postServerPort(formData.serverPort);
93-107:maxLengthattribute is ineffective ontype="number"inputs.The
maxLength={5}attribute (line 96) has no effect on number-type inputs in most browsers. Themaxvalidation rule already constrains the value to 65535, which provides the intended limit.♻️ Proposed fix
<Input id='serverPort' type='number' - maxLength={5} style={{ width: '75px' }}
alex-Arc
force-pushed
the
remove-serverport
branch
2 times, most recently
from
66bb825 to
edb3f2c
Compare
cpvalente
left a comment
cpvalente
left a comment
There was a problem hiding this comment.
When I tried to test this feature I was surprised to see that my server started in a random port. At first I thought that I had something on port 4001 but that was not the case.
We need to ensure that the migration is not changing perceived behaviour, ie: the app should prefer port 4001
I also wonder if we should have a small utility resolveServerPort which collects all the data sources and has logic for how to chose one. as of now things come from different places and it can be difficult to trace
alex-Arc
force-pushed
the
remove-serverport
branch
3 times, most recently
from
0068f84 to
fea1c18
Compare
alex-Arc
force-pushed
the
remove-serverport
branch
from
fea1c18 to
61c0430
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/server/src/api-data/db/db.parser.ts`:
- Line 47: Update the FIXME comment in db.parser.ts to correct the typo: change
"dent" to "don't" so the comment reads e.g. "// FIXME: we should keep the port
if it is loaded from an initial file so people don't lose connection"; locate
the comment near the parser logic referencing initial file port handling and
update the text only.
In `@apps/server/src/classes/port-manager/PortManager.ts`:
- Around line 116-127: The forceCloudPort(Server) promise currently only
resolves on successful listen and has no error handler, so attach an 'error'
listener to the provided server to reject the promise if bind fails (e.g.,
EADDRINUSE), and ensure you remove the error listener after success to avoid
leaks; mirror the behavior from tryServerPort by adding server.once('error', err
=> { cleanup listeners; reject(err); }) and remove the handler inside the listen
callback before calling resolve(address.port).
- Around line 73-113: Invert the desktop/docker branch in tryServerPort: when
canChangePort() is true (desktop) allow falling back to an ephemeral port on
EADDRINUSE and only reject immediately for non-EADDRINUSE errors; when
canChangePort() is false (cloud/docker) reject any error immediately.
Concretely, negate the existing canChangePort() check and the
isPortInUseError(error) condition so the server.listen(0, ...) ephemeral
fallback runs for desktop when isPortInUseError(error) is true. Also add
symmetric error handling to forceCloudPort: attach server.once('error', ...) to
reject the Promise on errors (and, if you want parity with tryServerPort, treat
EADDRINUSE explicitly), so server.listen(...) cannot hang without resolving or
rejecting. Reference: PortManager.tryServerPort and PortManager.forceCloudPort.
🧹 Nitpick comments (2)
apps/server/src/classes/port-manager/PortManager.ts (1)
10-13: Consider using instance properties instead of static properties.Using static properties (
PortManager.port,PortManager.pendingRestart,PortManager.newPort) in a class that's instantiated as a singleton is unconventional. If the intent is to have a single instance, consider using instance properties withthis.port, etc. The current pattern makes the class harder to test (can't reset state between tests) and mixes singleton patterns.♻️ Alternative using instance properties
class PortManager { - private static port: number; - private static pendingRestart = false; - private static newPort: MaybeNumber = null; + private port: number; + private pendingRestart = false; + private newPort: MaybeNumber = null; public getPort() { return { - port: PortManager.port, - pendingRestart: PortManager.pendingRestart, - newPort: PortManager.newPort, + port: this.port, + pendingRestart: this.pendingRestart, + newPort: this.newPort, }; } // ... update other methods similarly }apps/server/src/api-data/settings/settings.router.ts (1)
66-82: Consider using "Cannot" for consistency.The error message "Can not change port" is grammatically correct but "Cannot" is more conventional in error messages.
📝 Proposed fix
router.post('/serverport', validateServerPort, async (req: Request, res: Response<PortInfo | ErrorResponse>) => { if (!canChangePort()) { - res.status(403).json({ message: 'Can not change port when running inside docker' }); + res.status(403).json({ message: 'Cannot change port when running inside Docker' }); return; }
alex-Arc
force-pushed
the
remove-serverport
branch
2 times, most recently
from
a0ceb14 to
7232a40
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/server/src/classes/port-manager/PortManager.ts (1)
54-71: Consider validating env port range and integer-ness.
parsePortcurrently accepts any numeric value (including decimals or out-of-range ports), which will later fail duringlisten.♻️ Suggested refinement
private parsePort(port: string | undefined) { if (typeof port !== 'string') return null; if (port === '') return null; const maybePort = Number(port); - if (isNaN(maybePort)) return null; - return maybePort; + if (!Number.isInteger(maybePort)) return null; + if (maybePort < 1 || maybePort > 65535) return null; + return maybePort; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/classes/port-manager/PortManager.ts` around lines 54 - 71, parsePort currently converts any numeric string to a Number and returns it, allowing floats and out-of-range values that will break listen; update parsePort to validate that envPort is an integer (no decimals) and within the valid port range (0–65535, or 1–65535 if you disallow 0), returning null for invalid values so attachServer/PortManager.port falls back correctly; keep the function name parsePort and ensure callers in attachServer/PortManager.port and any uses of envPort continue to expect null for invalid input.apps/server/src/classes/port-manager/PortManager.utils.ts (1)
14-15: Strengthen the AddressInfo type guard.
typeof address === 'object'accepts any object; checking for a numericportreduces false positives.♻️ Suggested refinement
export function isAddressInfo(address: string | AddressInfo | null): address is AddressInfo { - return typeof address === 'object' && address !== null; + return ( + typeof address === 'object' && + address !== null && + 'port' in address && + typeof (address as AddressInfo).port === 'number' + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/classes/port-manager/PortManager.utils.ts` around lines 14 - 15, The isAddressInfo type guard (isAddressInfo) is too broad because typeof address === 'object' matches any object; tighten it by first confirming address !== null and then verifying it has a numeric port property (e.g., 'port' in address && typeof (address as any).port === "number"); update the function to check the port field (and optionally other AddressInfo keys like family or address) so the guard reliably narrows to AddressInfo.apps/client/src/features/app-settings/panel/settings-panel/ServerPortSettings.tsx (2)
32-35: AddresettouseCallbackdeps to avoid stale closure.React hooks best practice;
resetis captured but not listed.♻️ Suggested update
- const setPort = useCallback((info: PortInfo) => { + const setPort = useCallback((info: PortInfo) => { reset({ serverPort: info.port }); setPendingRestart(info.pendingRestart); - }, []); + }, [reset]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/features/app-settings/panel/settings-panel/ServerPortSettings.tsx` around lines 32 - 35, The useCallback for setPort captures reset and setPendingRestart but currently has an empty deps array; update the dependency array of setPort to include reset (and keep setPendingRestart if not stable) so the closure doesn't hold a stale reference — locate setPort defined via useCallback and add reset to its dependency list.
49-50: Avoid the extra GET by using the POST response.
postServerPortalready returnsPortInfo, so you can update state without a second round-trip (and avoid a failure window after a successful POST).♻️ Suggested update
- await postServerPort(formData.serverPort); - setPort(await getServerPort()); + const res = await postServerPort(formData.serverPort); + setPort(res.data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/features/app-settings/panel/settings-panel/ServerPortSettings.tsx` around lines 49 - 50, postServerPort already returns the PortInfo, so avoid the extra getServerPort round-trip: capture the result of postServerPort(formData.serverPort) and pass that value directly to setPort instead of calling getServerPort again; update the code around postServerPort / setPort to use the returned PortInfo (and remove the redundant getServerPort call) so the UI updates immediately with the POST response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/client/src/features/app-settings/panel/settings-panel/ServerPortSettings.tsx`:
- Around line 106-114: The register call for serverPort currently validates
range and pattern but does not coerce the input type, so React Hook Form will
keep the value as a string; update the register('serverPort', ...) options to
include valueAsNumber: true so the field is converted to a number before
validation/submission (modify the register invocation where serverPort is
registered to add valueAsNumber: true alongside required, min, max, pattern).
In `@apps/server/src/api-data/db/db.parser.ts`:
- Around line 47-57: The code sets migrated = true before calling
v4.migrateServerPort, so if migrateServerPort throws the file will be
incorrectly marked migrated; move the assignment of migrated (and any logging
that assumes success) to after v4.migrateServerPort returns successfully: call
v4.shouldMigrateServerPort, then try to run const { db, serverPort } =
v4.migrateServerPort(migratedData); on success set migrated = true, update
migratedData = db, and call portManager.migratePortFromProjectFile(serverPort)
when initialLoad; keep the catch to logger.error(LogOrigin.Server, 'Failed to
migrate serverPort') without changing migrated.
In `@apps/server/src/classes/port-manager/PortManager.ts`:
- Around line 73-103: The fallback call to server.listen(0, '0.0.0.0', ...) in
tryServerPort can itself emit an 'error' and currently has no handler; attach a
temporary error listener before calling listen(0) that rejects the Promise with
that error and cleans itself up (and the success listener) if invoked, and
ensure you remove that listener on successful bind (after isAddressInfo check)
to avoid leaking handlers; reference tryServerPort, the existing
server.once('error') flow, the fallback listen(0) block, and the resolve/reject
paths when adding this error handling.
---
Nitpick comments:
In
`@apps/client/src/features/app-settings/panel/settings-panel/ServerPortSettings.tsx`:
- Around line 32-35: The useCallback for setPort captures reset and
setPendingRestart but currently has an empty deps array; update the dependency
array of setPort to include reset (and keep setPendingRestart if not stable) so
the closure doesn't hold a stale reference — locate setPort defined via
useCallback and add reset to its dependency list.
- Around line 49-50: postServerPort already returns the PortInfo, so avoid the
extra getServerPort round-trip: capture the result of
postServerPort(formData.serverPort) and pass that value directly to setPort
instead of calling getServerPort again; update the code around postServerPort /
setPort to use the returned PortInfo (and remove the redundant getServerPort
call) so the UI updates immediately with the POST response.
In `@apps/server/src/classes/port-manager/PortManager.ts`:
- Around line 54-71: parsePort currently converts any numeric string to a Number
and returns it, allowing floats and out-of-range values that will break listen;
update parsePort to validate that envPort is an integer (no decimals) and within
the valid port range (0–65535, or 1–65535 if you disallow 0), returning null for
invalid values so attachServer/PortManager.port falls back correctly; keep the
function name parsePort and ensure callers in attachServer/PortManager.port and
any uses of envPort continue to expect null for invalid input.
In `@apps/server/src/classes/port-manager/PortManager.utils.ts`:
- Around line 14-15: The isAddressInfo type guard (isAddressInfo) is too broad
because typeof address === 'object' matches any object; tighten it by first
confirming address !== null and then verifying it has a numeric port property
(e.g., 'port' in address && typeof (address as any).port === "number"); update
the function to check the port field (and optionally other AddressInfo keys like
family or address) so the guard reliably narrows to AddressInfo.
01a556b to
e514d99
Compare
optional startup port from env will always override function for parsing port from env populate default port in app state add test for migration
alex-Arc
force-pushed
the
remove-serverport
branch
from
e514d99 to
5bea134
Compare
2 participants
serverPortvalue is dropped from the project file (nothing is moved over to avoid issues where the port moves on the next restart)PORTis now available and will overwire the port on start-up