Report progress while a plugin installs - #1015
Conversation
|
🚨 SLOP COP 🚨 · Performance review complete. I found one low-severity render cost in #1015. I added an inline comment. The Linux, application, package, server, and integration checks passed. The macOS package smoke check is still pending. |
|
|
||
| @keyframes plugin-install-progress { | ||
| 0% { | ||
| margin-inline-start: -35%; |
There was a problem hiding this comment.
🚨 slopcop/review — This animation changes a layout property on every frame. A cold install can show it for about 17 seconds, and a git install can take longer. Use a transform animation so the browser can move the bar without repeated layout work.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Performance phase complete.
Low: the progress sweep animates margin-inline-start. This property causes layout and paint work on each frame.
Animate a transform on the inner bar. Keep the current static fill for reduced motion.
|
🚨 SLOP COP 🚨 · Security review complete. I found no security issue in #1015. The new callbacks expose only fixed package versions and elapsed time. |
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
End-to-end phase complete on the top PR.
I started the development app and used a browser to install a real local plugin.
The pending state showed Installing plugin…, set aria-busy=true, displayed one progress bar, and removed the trust warning.
The server installed the plugin. The dialog closed, and the plugin detail page showed version 0.1.0.
I found no end-to-end defect.
|
🚨 SLOP COP 🚨 · Code quality review complete. I found one additional low-severity wording issue in #1015. The performance issue already has a separate inline comment. A shared indeterminate Progress mode could replace the plugin-specific progress CSS. This refactor is optional. All focused Turbo typechecks and tests passed. |
| const pins = Object.entries(PLUGIN_TOOLCHAIN_PINS) | ||
| .map(([name, version]) => `${name}@${version}`) | ||
| .join(", "); | ||
| console.log("Downloading the plugin build toolchain (one time)…"); |
There was a problem hiding this comment.
🚨 slopcop/review — “One time” is incorrect after a toolchain pin changes. Say “for this toolchain version” or remove the frequency claim.
|
|
||
| @keyframes plugin-install-progress { | ||
| 0% { | ||
| margin-inline-start: -35%; |
There was a problem hiding this comment.
🚨 slopcop/review — Use a compositor transform for the progress sweep
margin-inline-start causes layout work on each animation frame. The animation can run during a long network install. Animate transform: translateX(...) instead. Keep the static reduced-motion state.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Result: the progress workflow works. I found one low performance issue.
The browser test used the real top-stack dev app. It confirmed these states:
- The request reached
POST /api/v1/plugins/install. - The progress bar became visible.
- The button showed
Installing plugin…, became disabled, and setaria-busy=true. - The full-trust warning disappeared after the user started the install.
- The progress animation was active.
The changed dialog test passed all six tests. The only finding asks the animation to use a compositor transform.
|
|
||
| @keyframes plugin-install-progress { | ||
| 0% { | ||
| margin-inline-start: -35%; |
There was a problem hiding this comment.
🚨 slopcop/review — The progress bar animates a layout property.
margin-inline-start causes layout and paint work on every frame. The cold install can keep this animation active for many seconds.
Animate a transform on the inner bar. Keep the current static fill for reduced motion.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Final review summary for PR #1015.
I found one low performance issue. The progress bar animates a layout property instead of a transform.
The end-to-end test passed on the stack tip. The dialog showed its pending state and installed a real local plugin.
The focused app test passed all six cases. I found no security or correctness defect in this PR.
I submitted only comments. I did not approve or request changes.
|
🚨 SLOP COP 🚨 · End-to-end review complete on top pull request #1015. I started the application from commit 949f994. I opened Settings, Plugins, and Add plugin. I held the install request open. The dialog showed the progress role, “Installing plugin…” text, and the 1.5-second animation. The full-trust warning left the dialog during the request. The progress state cleared after the request failed. With reduced motion enabled, the animation stopped and the bar used a static 50-percent width. The browser test passed. |
macOS QA measured a first install sitting silent for 17.22s against 1.64s once cached, because the first build on a machine downloads the toolchain. The dialog now says "Installing plugin…" and shows an indeterminate bar in place of the full-trust warning, which has served its purpose by then. It deliberately does not name the phase: the server does the work behind one blocking request, so the client cannot know whether it is downloading tools, resolving dependencies, or bundling. Saying so would mean either guessing or adding a realtime phase contract maintained forever for a window that happens once per machine. resolvePluginBuildToolchain gains onFetchDone(elapsedMs). bb plugin build and bb plugin dev resolve the toolchain in-process, so they now print the pinned versions and the elapsed time. bb plugin install cannot — the server fetches on its behalf — so it is unchanged; the server logs the duration instead. Reduced motion gets a static half-width fill rather than a frozen sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Final review: two low findings remain in #1015.
- Low performance: the progress animation changes
margin-inline-starton every frame.
Use a transform animation to avoid repeated layout work. Keep the existing reduced-motion state.
- Low quality: “one time” is incorrect after a toolchain pin changes.
Use “for this pinned toolchain set” or remove the frequency statement.
The progress CSS also duplicates shared Progress behavior. A shared indeterminate mode could centralize this optional refactor.
All GitHub checks passed. The focused Turbo typechecks and tests also passed.
The browser test passed on the top commit. It verified the pending state, error cleanup, and reduced-motion state.
I used a comment-only review. This rule forbids approval and request-changes actions.
949f994 to
a1c3a82
Compare
Layer 4 of 4. Depends on #1006. Addresses the one P2 from macOS QA.
A first install sat silent for 17.22 s (vs 1.64 s cached), because the first plugin build on a machine downloads the toolchain. Measured on macOS by
thr_3wmkakaaix.Dialog
Installing plugin…plus an indeterminate bar, replacing the full-trust warning — which has served its purpose by the time you've clicked install.It deliberately does not name the phase. The server does the work behind one blocking
POST, so the client cannot know whether it's downloading tools, resolving dependencies, or bundling. Saying so would mean either guessing, or adding a realtime phase contract maintained forever for a window that happens once per machine.Reduced motion gets a static half-width fill rather than a frozen sweep.
CLI
resolvePluginBuildToolchaingainsonFetchDone(elapsedMs).bb plugin buildandbb plugin devresolve the toolchain in-process, so they now print the pinned versions on start and the elapsed time on completion.bb plugin installis unchanged — the server fetches on its behalf, so the CLI has no more visibility than the browser does. The server logs the duration instead.Verification
AddPluginDialogtest asserts the label, the bar, and the warning being replaced. Confirmed to fail with the label change reverted.onFetchStartandonFetchDonefire on a real download.role="progressbar", and no trust warning.turbo run typecheck --force: 57/57, 0 errors.🤖 Generated with Claude Code