Conversation
mattermost-build
added
the
E2E/Run
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe change validates response content, preserves response details, retries 406 pings using sanitized base URLs, returns effective server URLs, propagates redirected URLs through login, and adds unit and Detox coverage. ChangesServer connection flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServerScreen
participant doPing
participant NetworkManager
participant RedirectedServer
ServerScreen->>doPing: ping original server URL
doPing->>NetworkManager: create client for original URL
NetworkManager->>RedirectedServer: send ping
RedirectedServer-->>doPing: HTTP 406 with base_url
doPing->>NetworkManager: create client for sanitized base_url
NetworkManager->>RedirectedServer: retry ping
RedirectedServer-->>doPing: successful ping
doPing-->>ServerScreen: return serverUrl
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/screens/server/index.tsx (1)
331-335: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
??for theserverUrlfallback.
result.serverUrlisstring | undefined; using??is more precise than||for this fallback (avoids accidentally treating a falsy-but-valid string as absent).As per coding guidelines: "In TypeScript, use ?? instead of || for fallbacks when appropriate."
♻️ Proposed fix
- const serverUrlToUse = result.serverUrl || headRequest.url; + const serverUrlToUse = result.serverUrl ?? headRequest.url;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/screens/server/index.tsx` around lines 331 - 335, Replace the `||` fallback in the `serverUrlToUse` assignment with `??`, preserving `result.serverUrl` when it is defined and falling back to `headRequest.url` only when it is nullish.Source: Path instructions
detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts (1)
42-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
ServerScreen.dismissIosNotificationsAlert()instead of a bespoke inline dismissal.This block reimplements a simplified version of the alert-dismissal logic already handled robustly (multiple title/label variants, retry strategies) by
dismissIosNotificationsAlert()indetox/e2e/support/ui/screen/server.ts. Reusing it would avoid duplicated, less resilient logic in this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts` around lines 42 - 49, The inline alert dismissal logic in the if block (checking isIos() and !process.env.CI) duplicates functionality already provided by ServerScreen.dismissIosNotificationsAlert() from detox/e2e/support/ui/screen/server.ts. Replace the entire try-catch block that waits for and taps Alert.okayButton with a single call to ServerScreen.dismissIosNotificationsAlert(). This will use the existing, more robust implementation that handles multiple title and label variants and includes proper retry strategies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/actions/remote/general.ts`:
- Around line 91-140: Normalize the extracted base URL before both 406 retry
calls in doPing: after getBaseUrlFromResponseData returns a value, remove any
trailing slash while preserving the rest of the URL, then pass the normalized
value to doPing. Apply this consistently in both the response-handling branch
and the caught ClientError branch so NetworkManager.createClient and the
returned serverUrl receive the normalized URL.
In `@detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts`:
- Line 36: Replace the placeholder test identifier in the skipped test name
within the `base_url_redirect` suite with the assigned tracking ticket using the
required `MM-TXXXX_N` format, preserving the test description.
---
Nitpick comments:
In `@app/screens/server/index.tsx`:
- Around line 331-335: Replace the `||` fallback in the `serverUrlToUse`
assignment with `??`, preserving `result.serverUrl` when it is defined and
falling back to `headRequest.url` only when it is nullish.
In `@detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts`:
- Around line 42-49: The inline alert dismissal logic in the if block (checking
isIos() and !process.env.CI) duplicates functionality already provided by
ServerScreen.dismissIosNotificationsAlert() from
detox/e2e/support/ui/screen/server.ts. Replace the entire try-catch block that
waits for and taps Alert.okayButton with a single call to
ServerScreen.dismissIosNotificationsAlert(). This will use the existing, more
robust implementation that handles multiple title and label variants and
includes proper retry strategies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71487aa3-f4c7-4a82-9996-05b368c6ec42
📒 Files selected for processing (7)
app/actions/remote/general.test.tsapp/actions/remote/general.tsapp/client/rest/tracking.test.tsapp/client/rest/tracking.tsapp/screens/server/index.tsxdetox/e2e/support/ui/screen/server.tsdetox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts
| }); | ||
|
|
||
| // Skipping until the server is updated to return the base URL in the 406 response | ||
| it.skip('MM-TBD_1 - should rewrite server URL from 406 base_url response and connect', async () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Placeholder ticket number in test name.
MM-TBD_1 doesn't match the required MM-TXXXX_N format. Update once the tracking ticket exists.
As per coding guidelines: "Name test cases with MM-TXXXX_N where XXXX is the Jira/Zephyr ticket number."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts` at
line 36, Replace the placeholder test identifier in the skipped test name within
the `base_url_redirect` suite with the assigned tracking ticket using the
required `MM-TXXXX_N` format, preserving the test description.
Source: Path instructions
mattermost-build
removed
the
E2E/Run
mattermost-build
added
the
E2E/Run
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/utils/url/index.ts (1)
55-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant
else ifbranch — collapse the protocol selection.With the new logic, the middle branch is dead code. When
keepProtocolis false and!useHttp, the finalelsealready yields'https:'— the exact value theelse if (preUrl.protocol === 'http:' && !useHttp)branch returns. WhenuseHttpis true, that middle condition never fires. So for every input theelseproduces the same result, making the branch a duplicate conditional.As per coding guidelines: "Avoid redundant checks, impossible states, duplicate conditional branches that return the same result, and other over-engineered control flow."
♻️ Proposed simplification
let protocol; if (keepProtocol) { protocol = preUrl.protocol; - } else if (preUrl.protocol === 'http:' && !useHttp) { - protocol = 'https:'; } else { protocol = useHttp ? 'http:' : 'https:'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/utils/url/index.ts` around lines 55 - 62, Remove the redundant `else if (preUrl.protocol === 'http:' && !useHttp)` branch in the protocol selection logic, retaining the `keepProtocol` check and using the final fallback to assign `useHttp ? 'http:' : 'https:'`.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/utils/url/index.ts`:
- Around line 55-62: Remove the redundant `else if (preUrl.protocol === 'http:'
&& !useHttp)` branch in the protocol selection logic, retaining the
`keepProtocol` check and using the final fallback to assign `useHttp ? 'http:' :
'https:'`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e853d506-2a0d-4399-9808-4fcc70644b0c
📒 Files selected for processing (3)
app/actions/remote/general.tsapp/utils/url/index.tsdetox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts
✅ Files skipped from review due to trivial changes (1)
- detox/e2e/test/products/channels/server_login/base_url_redirect.e2e.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/actions/remote/general.ts
Coverage Comparison Report |
mattermost-build
removed
the
E2E/Run
mattermost-build
added
the
E2E/Run
mattermost-build
removed
the
E2E/Run
|
|
||
| function getBaseUrlFromResponseData(data: unknown): string | undefined { | ||
| if (data && typeof data === 'object' && 'base_url' in data) { | ||
| const baseUrl = data.base_url; |
There was a problem hiding this comment.
Are you sure about adding this base_url to the server response? I wouldn't expect the server to return anything like that for a 406 error, so I'd expect something like we trim the path from serverUrl. If we want to fully support subpaths with that, we could even retry multiple times, each time with one more path segment removed
There was a problem hiding this comment.
See https://hub.mattermost.com/private-core/pl/z4rgearu6ignuqrr1szpaxqf6e for more context on this solution.
There was a problem hiding this comment.
Thanks for the context. I'm still not sure if I'd expect that since there's other reasons someone might accidentally request JSON from another endpoint, but I guess that makes sense when you compare to how we return the web app for any non-API URL
| if (response.code === 406 && !hasRetriedBaseUrl) { | ||
| const baseUrl = getBaseUrlFromResponseData(response.data); | ||
| if (baseUrl) { | ||
| return doPing(sanitizeUrl(baseUrl, false, true), verifyPushProxy, timeoutInterval, preauthSecret, undefined, true); |
There was a problem hiding this comment.
Is there any chance that this throws an error that's caught by the outer try/catch?
If it can and client is undefined, it looks like we might call NetworkManager.invalidateClient twice if that matters
There was a problem hiding this comment.
In reality, the normal path is going to be the try catch. We have this as defensive coding.
Invalidating the same client twice is not an issue.
| const errorObj = error as ClientError; | ||
|
|
||
| // Check if this is a 406 with base_url in the response data | ||
| if (errorObj.status_code === 406 && !hasRetriedBaseUrl) { |
There was a problem hiding this comment.
Why are we handling this in both the try and catch blocks? Should we perhaps take these out of the try/catch?
There was a problem hiding this comment.
It is the current pattern. Not great, but I didn't want to deviate from it.
| await wait(timeouts.ONE_SEC); | ||
| }; | ||
|
|
||
| waitForServerUrl = async (expectedUrl: string, timeout = timeouts.TEN_SEC) => { |
There was a problem hiding this comment.
Are these branches supposed to be the same?
There was a problem hiding this comment.
Not sure what do you mean here.
There was a problem hiding this comment.
Oh, I thought the await lines were both identical, but I missed that one is toHaveValue and the other is toHaveText
|
|
||
| if (response.ok) { | ||
| const contentType = getResponseHeader(headers, 'Content-Type'); | ||
| if (contentType?.toLowerCase().includes('text/html')) { |
There was a problem hiding this comment.
Why test for text/html instead of asserting application/json?
There was a problem hiding this comment.
Just in case in any API we are sending a empty content type, or anything similar. But you are probably right, and we should go with the application/json one.
5 participants
Summary
When we try to get something from the API, an HTML response is considered a success. This then fails down the line, since we try to access the data of the response, and it happens to be something unexpected.
This was specially confusing when adding a server using the full route to, for example, a channel. In that scenario, the ping will succeed, the fetch of config and license would also succeed, but when trying to access to the diagnostic id in the license, it would fail.
So this PR handles two things here:
Ticket Link
Fix https://mattermost.atlassian.net/browse/MM-67557
Related PRs
Server: TBD
Release Note