feat(frontend): detect new deployment and prompt to reload#5849
feat(frontend): detect new deployment and prompt to reload#5849Ma77Ball wants to merge 18 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 55.25% 55.23% -0.02%
+ Complexity 2993 2986 -7
============================================
Files 1117 1120 +3
Lines 43195 43252 +57
Branches 4657 4660 +3
============================================
+ Hits 23866 23892 +26
- Misses 17930 17957 +27
- Partials 1399 1403 +4
*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.
|
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 | 406 | 0.248 | 24,243/33,673/33,673 us | 🔴 +7.9% / 🔴 +117.8% |
| 🟢 | bs=100 sw=10 sl=64 | 793 | 0.484 | 125,962/137,936/137,936 us | 🟢 -9.9% / 🔴 +25.3% |
| ⚪ | bs=1000 sw=10 sl=64 | 932 | 0.569 | 1,075,477/1,134,053/1,134,053 us | ⚪ within ±5% / 🔴 +7.4% |
Baseline details
Latest main 7944c09 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 406 tuples/sec | 413 tuples/sec | 756.6 tuples/sec | -1.7% | -46.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.248 MB/s | 0.252 MB/s | 0.462 MB/s | -1.6% | -46.3% |
| bs=10 sw=10 sl=64 | p50 | 24,243 us | 25,090 us | 13,009 us | -3.4% | +86.4% |
| bs=10 sw=10 sl=64 | p95 | 33,673 us | 31,218 us | 15,463 us | +7.9% | +117.8% |
| bs=10 sw=10 sl=64 | p99 | 33,673 us | 31,218 us | 18,561 us | +7.9% | +81.4% |
| bs=100 sw=10 sl=64 | throughput | 793 tuples/sec | 816 tuples/sec | 963.83 tuples/sec | -2.8% | -17.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.484 MB/s | 0.498 MB/s | 0.588 MB/s | -2.8% | -17.7% |
| bs=100 sw=10 sl=64 | p50 | 125,962 us | 121,969 us | 103,320 us | +3.3% | +21.9% |
| bs=100 sw=10 sl=64 | p95 | 137,936 us | 153,116 us | 110,058 us | -9.9% | +25.3% |
| bs=100 sw=10 sl=64 | p99 | 137,936 us | 153,116 us | 118,543 us | -9.9% | +16.4% |
| bs=1000 sw=10 sl=64 | throughput | 932 tuples/sec | 918 tuples/sec | 989.07 tuples/sec | +1.5% | -5.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.569 MB/s | 0.56 MB/s | 0.604 MB/s | +1.6% | -5.7% |
| bs=1000 sw=10 sl=64 | p50 | 1,075,477 us | 1,091,442 us | 1,015,599 us | -1.5% | +5.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,134,053 us | 1,118,482 us | 1,055,944 us | +1.4% | +7.4% |
| bs=1000 sw=10 sl=64 | p99 | 1,134,053 us | 1,118,482 us | 1,086,834 us | +1.4% | +4.3% |
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,492.69,200,128000,406,0.248,24242.73,33672.58,33672.58
1,100,10,64,20,2522.73,2000,1280000,793,0.484,125962.24,137935.69,137935.69
2,1000,10,64,20,21457.71,20000,12800000,932,0.569,1075476.88,1134053.12,1134053.12
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.
| 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.
| 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.
There was a problem hiding this comment.
they are replaced but they are never removed. it is not that clean of a solution.
can this be stored in the memory instead? It can be written once at service deployment. the frontend/browser go ahead fetches it whenever connects. And we don't have to leave file behind on disk, and we won't need to deal with its life cycle (removal, git ignore, etc).
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