-
Notifications
You must be signed in to change notification settings - Fork 298
feat(server): create client-version-check route [WPB-23299] #20307
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
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.
Pull request overview
This PR introduces a new server endpoint /client-version-check as the first step in implementing a force-reload feature. The endpoint is a simple stub that returns HTTP 200 OK.
Changes:
- Created a new route handler for client version checking at
/client-version-check
| import {Router} from 'express'; | ||
| import {StatusCodes as HTTP_STATUS} from 'http-status-codes'; | ||
|
|
||
| export const ClientVersionCheck = () => { |
Copilot
AI
Feb 4, 2026
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.
The exported function name should be ClientVersionCheckRoute instead of ClientVersionCheck to follow the established naming convention in the codebase. All other route files follow the pattern of appending "Route" to the function name (e.g., HealthCheckRoute, ConfigRoute, GoogleWebmasterRoute, AppleAssociationRoute).
| export const ClientVersionCheck = () => { | |
| export const ClientVersionCheckRoute = () => { |
| export const ClientVersionCheck = () => { | ||
| return Router().get('/client-version-check', (request, response) => { | ||
| return response.sendStatus(HTTP_STATUS.OK); | ||
| }); | ||
| }; |
Copilot
AI
Feb 4, 2026
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.
This route is defined but not registered in Server.ts, which means it will not be accessible. The route needs to be imported and registered in the Server.ts file's init() method, similar to how other routes like HealthCheckRoute and ConfigRoute are registered (see apps/server/Server.ts lines 33 and 73-76). Without this registration, this endpoint will return 404.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
|
f6764b7 to
b6dc392
Compare
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.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
| export const ClientVersionCheck = () => { | ||
| return Router().get('/client-version-check', (request, response) => { | ||
| return response.sendStatus(HTTP_STATUS.OK); | ||
| }); | ||
| }; |
Copilot
AI
Feb 5, 2026
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.
[Important] - Missing test coverage
The codebase has test coverage for utility functions (BrowserUtil.test.ts, TimeUtil.test.ts), and this new route should also have test coverage. While some simple routes like HealthCheckRoute may not have tests, this is the beginning of a force-reload feature which is likely to grow in complexity.
Consider adding a test file at apps/server/routes/client-version-check/ClientVersionCheck.test.ts to verify the endpoint responds correctly with HTTP 200 status.
| export const ClientVersionCheck = () => { | ||
| return Router().get('/client-version-check', (request, response) => { | ||
| return response.sendStatus(HTTP_STATUS.OK); |
Copilot
AI
Feb 5, 2026
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.
[Important] - Route implementation is incomplete for force-reload feature
According to the PR description, this is meant to "implement a force-reload feature." However, the current implementation only returns HTTP 200 without any functionality to:
- Check the client version (the route is named "client-version-check" but doesn't check anything)
- Provide version information to clients
- Signal when a reload is necessary
The route needs to either:
- Accept client version information and compare it against current/minimum versions
- Return version information that clients can use to determine if they need to reload
- Include logic that indicates whether a force reload is required
Consider what data structure this endpoint should return. For example, it might return JSON like:
{
"currentVersion": "2024.12.14",
"minimumVersion": "2024.12.01",
"forceReload": false
}| export const ClientVersionCheck = () => { | |
| return Router().get('/client-version-check', (request, response) => { | |
| return response.sendStatus(HTTP_STATUS.OK); | |
| const CURRENT_VERSION = process.env.CLIENT_CURRENT_VERSION ?? '0.0.0'; | |
| const MINIMUM_VERSION = process.env.CLIENT_MINIMUM_VERSION ?? CURRENT_VERSION; | |
| const VERSION_REGEX = /^[0-9]+(\.[0-9]+){0,2}$/; | |
| const MAX_VERSION_LENGTH = 32; | |
| const compareVersions = (a: string, b: string): number => { | |
| const aParts = a.split('.').map(part => parseInt(part, 10) || 0); | |
| const bParts = b.split('.').map(part => parseInt(part, 10) || 0); | |
| const length = Math.max(aParts.length, bParts.length); | |
| for (let i = 0; i < length; i += 1) { | |
| const aVal = aParts[i] ?? 0; | |
| const bVal = bParts[i] ?? 0; | |
| if (aVal < bVal) { | |
| return -1; | |
| } | |
| if (aVal > bVal) { | |
| return 1; | |
| } | |
| } | |
| return 0; | |
| }; | |
| export const ClientVersionCheck = () => { | |
| return Router().get('/client-version-check', (request, response) => { | |
| const clientVersionRaw = | |
| (typeof request.query.version === 'string' ? request.query.version : undefined) || | |
| (typeof request.headers['x-client-version'] === 'string' | |
| ? request.headers['x-client-version'] | |
| : undefined); | |
| let clientVersion: string | undefined; | |
| if ( | |
| typeof clientVersionRaw === 'string' && | |
| clientVersionRaw.length > 0 && | |
| clientVersionRaw.length <= MAX_VERSION_LENGTH && | |
| VERSION_REGEX.test(clientVersionRaw) | |
| ) { | |
| clientVersion = clientVersionRaw; | |
| } | |
| const forceReload = | |
| typeof clientVersion === 'string' && | |
| compareVersions(clientVersion, MINIMUM_VERSION) < 0; | |
| return response.status(HTTP_STATUS.OK).json({ | |
| currentVersion: CURRENT_VERSION, | |
| minimumVersion: MINIMUM_VERSION, | |
| forceReload, | |
| }); |
| import {StatusCodes as HTTP_STATUS} from 'http-status-codes'; | ||
|
|
||
| export const ClientVersionCheck = () => { | ||
| return Router().get('/client-version-check', (request, response) => { |
Copilot
AI
Feb 5, 2026
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.
[Suggestion] - Consider route placement in initLatestBrowserRequired
The route /client-version-check may need to be added to the ignored paths in the initLatestBrowserRequired middleware (apps/server/Server.ts:183-195) if it should be accessible without browser checks. Review whether this endpoint should bypass the browser requirement checks similar to how /_health, /commit, /test, etc. are handled.
b6dc392 to
7b9dffc
Compare
Previously there were no single unit tests for this route so we should verify that the route is behaving as intended.
7b9dffc to
d610ecb
Compare
|
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| this.app.use(NotFoundRoute()); | ||
| this.app.use(InternalErrorRoute()); | ||
| this.app.use(createClientVersionCheckRoute({router: Router()})); |
Copilot
AI
Feb 11, 2026
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.
[Blocker] - /client-version-check is registered after NotFoundRoute() and InternalErrorRoute(), so it will never be reached.
NotFoundRoute() registers a GET * handler (apps/server/routes/error/ErrorRoutes.ts:53-68) which will match /client-version-check first and return a 404 error page. Move createClientVersionCheckRoute(...) above NotFoundRoute() (and keep not-found / error handlers last).
| this.app.use(NotFoundRoute()); | |
| this.app.use(InternalErrorRoute()); | |
| this.app.use(createClientVersionCheckRoute({router: Router()})); | |
| this.app.use(createClientVersionCheckRoute({router: Router()})); | |
| this.app.use(NotFoundRoute()); | |
| this.app.use(InternalErrorRoute()); |
| return router.get('/client-version-check', (request, response) => { | ||
| return response.sendStatus(HTTP_STATUS.OK); |
Copilot
AI
Feb 11, 2026
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.
[Important] - Unused request parameter will trigger @typescript-eslint/no-unused-vars warnings.
Other routes use _req/_request for unused parameters (e.g. AppleAssociationRoute, RedirectRoutes). Consider renaming request to _request (or _req) here to match the repo’s lint expectations.
| type ClientVersionCheckRouteDependencies = { | ||
| readonly router: ReturnType<typeof Router>; | ||
| }; | ||
|
|
||
| export function createClientVersionCheckRoute(dependencies: ClientVersionCheckRouteDependencies) { | ||
| const {router} = dependencies; |
Copilot
AI
Feb 11, 2026
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.
[Suggestion] - Route factory signature is inconsistent with the surrounding route style.
Most existing routes export XRoute = (...) => Router().get(...) and construct their own router (e.g. HealthCheckRoute, AppleAssociationRoute, ConfigRoute). Consider following the same pattern here (instead of injecting router), which would simplify usage in Server.ts and keep route registration consistent.



Pull Request
Summary
Start to implement a force-reload feature.
Security Checklist (required)
Accessibility (required)
Standards Acknowledgement (required)
Screenshots or demo (if the user interface changed)
Notes for reviewers