feat(frontend): detect new deployment and prompt to reload#5849
feat(frontend): detect new deployment and prompt to reload#5849Ma77Ball wants to merge 17 commits into
Conversation
|
I feel this PR may be over-engineering a relatively small and infrequent issue. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5849 +/- ##
============================================
+ Coverage 54.89% 54.92% +0.02%
+ Complexity 2962 2957 -5
============================================
Files 1117 1120 +3
Lines 43133 43190 +57
Branches 4648 4651 +3
============================================
+ Hits 23680 23724 +44
- Misses 18064 18069 +5
- Partials 1389 1397 +8
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@aglinxinyuan Fair point, but the issue isn't cosmetic:
It's also lightweight: one small service that polls a static Happy to scale it back if you'd prefer (e.g. check only on navigation, or behind a config flag). |
Ma77Ball
force-pushed
the
feat/deployment-version-check
branch
from
60be3aa to
0317dce
Compare
…exera into feat/deployment-version-check
|
/request-review @aglinxinyuan |
|
@Ma77Ball please include a UI screenshot/gif for this feature. Thanks! |
There was a problem hiding this comment.
Pull request overview
Adds a frontend mechanism to detect when a newer deployment is available (via a generated assets/version.json) and prompts the user to reload, reducing the chance of running an outdated bundle after a deploy.
Changes:
- Generate a runtime-readable deployment manifest (
src/assets/version.json) alongsideversion.prod.tsduringyarn buildviabuild-version.js. - Add
DeploymentVersionServiceto pollassets/version.jsonand show a sticky notification once a new build is detected. - Wire polling startup in
AppComponent(guarded to skip"dev"builds) and add unit tests for the build artifact generator, the service, and the AppComponent wiring.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/build-version.spec.ts | Adds tests for renderVersionArtifacts (TS module + manifest JSON generation and escaping). |
| frontend/src/app/common/service/deployment-version/deployment-version.service.ts | New service that polls assets/version.json and prompts once on mismatch with compiled build number. |
| frontend/src/app/common/service/deployment-version/deployment-version.service.spec.ts | Unit tests covering update detection, malformed/failed fetches, cache-busting, and polling behavior. |
| frontend/src/app/app.component.ts | Starts deployment polling on app bootstrap when not in "dev" build. |
| frontend/src/app/app.component.spec.ts | Adds tests for config-loaded detection, polling guard, and retry reload behavior. |
| frontend/build-version.js | Extends build step to emit src/assets/version.json and exports a testable artifact renderer. |
| frontend/build-version.d.ts | Provides TypeScript typings for build-version.js exports used in tests. |
| frontend/.gitignore | Ignores generated src/assets/version.json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("reloads the page", () => { | ||
| const reload = vi.fn(); | ||
| // location.reload itself is non-configurable, so swap the whole | ||
| // location object for the duration of the test. | ||
| const original = window.location; | ||
| Object.defineProperty(window, "location", { | ||
| configurable: true, | ||
| value: { ...original, reload }, | ||
| }); | ||
| try { | ||
| create().componentInstance.retry(); | ||
| expect(reload).toHaveBeenCalledTimes(1); | ||
| } finally { | ||
| Object.defineProperty(window, "location", { configurable: true, value: original }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
In this jsdom build location.reload is non-writable/non-configurable and not on the prototype, so spying throws. window.location is configurable, so swapping the whole object is the only thing that works. Kept it with a comment.
|
To reply @aglinxinyuan, I somewhat think this might be a good feature to include, but would love to see the frontend experience to decide. @Ma77Ball it may be a good idea to have a config to turn off by default. We could try it with one specific deployment later. |
|
@Yicong-Huang Added |
|
@aglinxinyuan It's now behind a flag that's off by default, so no change unless a deployment opts in. |
Automated Reviewer SuggestionsBased on the
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 401 | 0.245 | 24,309/34,490/34,490 us | 🔴 +13.6% / 🔴 +121.4% |
| 🔴 | bs=100 sw=10 sl=64 | 795 | 0.485 | 128,091/147,297/147,297 us | 🟢 -22.4% / 🔴 +34.4% |
| ⚪ | bs=1000 sw=10 sl=64 | 905 | 0.552 | 1,116,759/1,144,436/1,144,436 us | ⚪ within ±5% / 🔴 +10.6% |
Baseline details
Latest main 7a38b6c from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 401 tuples/sec | 429 tuples/sec | 758.88 tuples/sec | -6.5% | -47.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.245 MB/s | 0.262 MB/s | 0.463 MB/s | -6.5% | -47.1% |
| bs=10 sw=10 sl=64 | p50 | 24,309 us | 21,391 us | 12,965 us | +13.6% | +87.5% |
| bs=10 sw=10 sl=64 | p95 | 34,490 us | 34,271 us | 15,578 us | +0.6% | +121.4% |
| bs=10 sw=10 sl=64 | p99 | 34,490 us | 34,271 us | 18,378 us | +0.6% | +87.7% |
| bs=100 sw=10 sl=64 | throughput | 795 tuples/sec | 801 tuples/sec | 968.9 tuples/sec | -0.7% | -17.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.485 MB/s | 0.489 MB/s | 0.591 MB/s | -0.8% | -18.0% |
| bs=100 sw=10 sl=64 | p50 | 128,091 us | 118,461 us | 102,767 us | +8.1% | +24.6% |
| bs=100 sw=10 sl=64 | p95 | 147,297 us | 189,912 us | 109,629 us | -22.4% | +34.4% |
| bs=100 sw=10 sl=64 | p99 | 147,297 us | 189,912 us | 118,129 us | -22.4% | +24.7% |
| bs=1000 sw=10 sl=64 | throughput | 905 tuples/sec | 927 tuples/sec | 997.01 tuples/sec | -2.4% | -9.2% |
| bs=1000 sw=10 sl=64 | MB/s | 0.552 MB/s | 0.566 MB/s | 0.609 MB/s | -2.5% | -9.3% |
| bs=1000 sw=10 sl=64 | p50 | 1,116,759 us | 1,079,573 us | 1,009,306 us | +3.4% | +10.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,144,436 us | 1,115,875 us | 1,051,088 us | +2.6% | +8.9% |
| bs=1000 sw=10 sl=64 | p99 | 1,144,436 us | 1,115,875 us | 1,082,535 us | +2.6% | +5.7% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,498.44,200,128000,401,0.245,24308.58,34489.80,34489.80
1,100,10,64,20,2516.37,2000,1280000,795,0.485,128090.65,147296.71,147296.71
2,1000,10,64,20,22094.49,20000,12800000,905,0.552,1116759.42,1144436.14,1144436.14
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
Thanks, left comments inline
The idea is not bad, many services (e.g., google doc) has this force refresh alert.
The only big downside is what if a user is in the middle of something where he/she can lose unsaved progress. For workflow edits/execution etc probably be fine, because we do fine grained millisecond level auto save. But some cases where we have not taken care of, could have issues. For instance, when I am uploading very large files, can this interrupt?
Or do we have the assumption that frontend deployment happens only with backend service restart? then all operations are disconnected, anyway.
| // Records blank() calls so the prompt is observable without a spy framework. | ||
| class FakeNotificationService { | ||
| public blankCalls: { title: string; content: string; options: NzNotificationDataOptions }[] = []; | ||
| blank(title: string, content: string, options: NzNotificationDataOptions = {}): void { | ||
| this.blankCalls.push({ title, content, options }); | ||
| } | ||
| } |
There was a problem hiding this comment.
let's call it a mock, not "fake"
There was a problem hiding this comment.
why don't we test against the real notification service?
There was a problem hiding this comment.
Done. Now injects the real NotificationService (stubbing only ng-zorro's NzNotificationService/NzMessageService) and spies on blank().
| promptReload(): void { | ||
| this.notification.blank( | ||
| "New version available", | ||
| "A new version of Texera is available. Refresh the page to update.", | ||
| { nzDuration: 0 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
not sure if notification can add this:
have a button to trigger page refresh?
just for convenience?
There was a problem hiding this comment.
Done. blank() returns the NzNotificationRef, so clicking the notification now reloads the page (guarded with take(1)). Added a test for it.
| } | ||
|
|
||
| // Records start() invocations without a spy framework. | ||
| class FakeDeploymentVersionService { |
There was a problem hiding this comment.
create a spy around the real service?
vitest should have framework to spy a function?
There was a problem hiding this comment.
Done. Uses the real service with vi.spyOn(service, "startPollingForUpdates") instead of a fake.
| private config: GuiConfigService, | ||
| private deploymentVersion: DeploymentVersionService |
There was a problem hiding this comment.
configService and deploymentVersionService
There was a problem hiding this comment.
Done. Renamed to configService and deploymentVersionService.
| // config actually loaded, and this isn't the "dev" placeholder build where | ||
| // no deployments occur. | ||
| if (this.configLoaded && this.config.env.deploymentVersionCheckEnabled && Version.buildNumber !== "dev") { | ||
| this.deploymentVersion.start(); |
There was a problem hiding this comment.
you cannot start a version. let's make the naming better
There was a problem hiding this comment.
Done. Renamed start() to startPollingForUpdates().
| function main() { | ||
| const { version } = require("./package.json"); | ||
| const { buildNumber, prodTs, manifestJson } = renderVersionArtifacts(version); | ||
| writeFileSync(resolve(__dirname, "src", "environments", "version.prod.ts"), prodTs); | ||
| writeFileSync(resolve(__dirname, "src", "assets", "version.json"), manifestJson); | ||
| console.log(`build-version: ${buildNumber}`); | ||
| } |
There was a problem hiding this comment.
is it the only way to write a file? how about use an env variable?
either way we have to take care of the file life cycle: who is going to delete this file/env variable?
There was a problem hiding this comment.
An env var won't work: the browser fetches version.json at runtime to read the deployed version, and it can't read a server-side env var. Both generated files are gitignored and rewritten in place every build, so there's nothing to clean up.
refactor(frontend): address review feedback on
deployment version check
- Test the real
NotificationService/DeploymentVersionService with
vi.spyOn
instead of hand-rolled fakes, stubbing only
ng-zorro's lower-level services
- Make the reload notification actionable: blank()
returns the NzNotificationRef
and promptReload() reloads the page on click
(guarded with take(1))
- Rename start() -> startPollingForUpdates() for
clearer intent
- Rename AppComponent constructor params to
configService/deploymentVersionService
Ma77Ball
force-pushed
the
feat/deployment-version-check
branch
from
49b77f0 to
7502d56
Compare
What changes were proposed in this PR?
Any related issues, documentation, discussions?
Closes: #5836
How was this PR tested?
yarn test --include='**/deployment-version.service.spec.ts'from frontend/, expect 15 passing cases: update detected (differing build), no update (same build), malformed manifests (missing/empty/non-string buildNumber, null body), transport failures (network error, 404, 500), the request carries a cache-busting query param, promptReload shows exactly one sticky notification, and the polling path prompts once on update, never prompts while unchanged, and stops after the first update.Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.8 in compliance with ASF