GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Improve UX of rapidly changing transaction IDs and commit progress by Karakatiza666 · Pull Request #7742 · feldera/feldera Conversation
|
| not, without either row stretching to fill a line it has to itself. --> | ||
| <div | ||
| class="items-top flex max-w-150 basis-150 flex-wrap gap-x-4 gap-y-2 {_class}" | ||
| class="items-top flex max-w-150 basis-150 flex-wrap gap-x-4 gap-y-2 h-16 {_class}" |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
bug (medium): the fixed h-16 breaks the layout below sm. There, the label column is w-full and the bar column wraps under it. I rendered a long commit in a 390px viewport: the row's clientHeight was 64px and its scrollHeight was 112px, so the content overflows into whatever sits below the row. Use sm:h-16, or a min-h-16, to keep the constant height on desktop only. The old mobile handling (useIsScreenSm) was removed, but its import is still there.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| async function push() { | ||
| const workDir = mkdtempSync(path.join(tmpdir(), 'playwright-snapshots-')) | ||
| await $`git clone -q --depth 1 [email protected]:${REPO}.git ${workDir}` | ||
| await $`git -C ${workDir} rm -r -q --ignore-unmatch .` |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
pitfall (high): push replaces the whole tree of the snapshots repo with the local copy and pushes it to main. Suppose two PRs update screenshots at the same time. Each pins a commit that lacks the other PR's new screenshots, so whichever PR merges second breaks the other's screenshot tests on main. A fix could rebase onto the snapshots repo's current main and copy only the changed files (the manifest already lists them via listChangedFiles). At least, document this in the README.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| "test-prepare": "bun scripts/playwright-snapshots.ts pull", | ||
| "test-snapshots-push": "bun scripts/playwright-snapshots.ts push", | ||
| "test-update-snapshots": "bun run deps && bun run test-prepare && vitest --run --project client --update && DISPLAY= bun playwright test --update-snapshots", | ||
| "test-unit": "bun run deps && bun run test-prepare && vitest --run --project client --project server", |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
pitfall (medium): test-unit (and test-e2e) now has to download from codeload.github.com. If that download fails, the run fails. The old git clone … || true did not fail. A fresh CI checkout downloads on every run. Also, the commit message only covers the transaction UX, but this PR also changes the snapshot workflow (pinned commit, pull/push script, viewport, ui: false). Please split that into its own commit or PR, and explain why in the commit message.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /** Waits until the bars have moved to the latest poll. */ | ||
| const settleBars = () => sleep(2 * PERIOD_MS + 100) | ||
| // The browser is slow while the other test files run. Thus, the screenshot can take longer. | ||
| const screenshotOptions = { timeout: 15_000 } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
flaky (medium): the screenshot tests rely on wall-clock time: settleBars() sleeps, and the comment says the browser is slow under load. I ran bun run test-unit with the default worker count. shows an idle transaction row timed out (locator.screenshot: Timeout 5000ms exceeded), and three unrelated screenshot tests failed too. With VITEST_MAX_WORKERS=4, as CI uses, all tests passed. The sleep(600) waits in the height test and the real-timer ID count test are similar. A scheduler prop (ManualScheduler) could drive the animations deterministically instead of sleeping.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Review summary. The rules for the counter, the long-commit detector and the bar animator are clean, and their unit tests are good. What I ran
Gates
Missing (medium): +++ b/js-packages/web-console/src/lib/compositions/counterAnimation.spec.ts +import { describe, expect, it } from 'vitest' +import { CounterAnimator, MAX_COUNTED_STEP } from './counterAnimation' +import { ManualScheduler } from './manualAnimationScheduler' + +const PERIOD = 2000 +const setup = () => { + const scheduler = new ManualScheduler() + const shown: number[] = [] + const counter = new CounterAnimator(PERIOD, (v) => shown.push(v), { scheduler }) + return { scheduler, shown, counter, last: () => shown.at(-1) } +} + +describe('CounterAnimator', () => { + it('counts up to the new value in one period while the activity runs', () => { + const { counter, scheduler, last } = setup() + counter.update({ value: 7, active: true }) + counter.update({ value: 11, active: true }) + scheduler.advance(PERIOD / 2) + expect(last()).toBe(9) + scheduler.advance(PERIOD / 2) + expect(last()).toBe(11) + expect(scheduler.pendingCount).toBe(0) + }) + it('jumps after an idle poll, on a decrease, and on a step above MAX_COUNTED_STEP', () => { + const { counter, last } = setup() + counter.update({ value: 7, active: false }) + counter.update({ value: 9, active: true }) + expect(last()).toBe(9) + counter.update({ value: 3, active: true }) + expect(last()).toBe(3) + counter.update({ value: 4 + MAX_COUNTED_STEP, active: true }) + expect(last()).toBe(4 + MAX_COUNTED_STEP) + }) + it('continues from the shown value when a new poll interrupts a count', () => { + const { counter, scheduler, last } = setup() + counter.update({ value: 0, active: true }) + counter.update({ value: 4, active: true }) + scheduler.advance(PERIOD / 2) + expect(last()).toBe(2) + counter.update({ value: 6, active: true }) + scheduler.advance(PERIOD) + expect(last()).toBe(6) + }) + it('cancels a running count on dispose', () => { + const { counter, scheduler } = setup() + counter.update({ value: 0, active: true }) + counter.update({ value: 4, active: true }) + counter.dispose() + expect(scheduler.pendingCount).toBe(0) + }) +}) Missing (low-medium): no test renders the indicator below the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| scheduler?: AnimationScheduler | ||
| } | ||
|
|
||
| type Phase = 'follow' | 'finish' | 'restart' |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
you don't like enums? they should be more efficient
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Typescript introduced enums to JS.. and they are considered an anti-pattern.
https://dev.to/ivanzm123/dont-use-enums-in-typescript-they-are-very-dangerous-57bh
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@lalithsuresh let me know what you'd like to change / don't like about the current design |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
If the ID only shows up for active transactions, and there are modes where transactions happen all the time, I think the wheel isn't helpful -- let's remove. I'm less sure about the ID showing up and disappearing but let's keep that behavior for now. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@lalithsuresh I removed the spinner: Screencast.From.2026-11-03.00-55-27.webm |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
- Remove the spinner next to the transaction ID. Transactions can run all the time, so the spinner told nothing. - Color "Transaction Started" in the page header blue, as "Started" in the performance tab. - Keep the row height constant only from `sm` up. Below `sm`, the label and the bar take separate lines, and a fixed height made them overflow. - Replace the transaction ID counter with `useInterpolatedValue`, a Svelte `Tween` that jumps when a key changes. - Drive the animations in tests with Vitest fake timers instead of sleeps and a custom scheduler. Under load, the sleeps made the tests flaky. - Push only the changed screenshots on top of the latest commit of the snapshots repository. A full-tree push deleted the screenshots that another branch had pushed. Retry a failed download. - Reword the docs of the progress bar animation, and update the screenshots. Signed-off-by: Karakatiza666 <[email protected]>
5fc851e to
4bcfe7d
Compare
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.