fix(dashboard): settle failed refresh requests - #2688
Conversation
3175f5a to
aee5e48
Compare
aee5e48 to
e8ac916
Compare
openai0229
left a comment
There was a problem hiding this comment.
Blocking regression: runDashboardRefresh now converts a rejected load into the fulfilled value false, but both callers ignore the boolean. In particular, DingChartModal uses refreshCurrentDashboard().then(() => staticMessage.success(...)), so a failed refresh now displays "pin to dashboard success". Previously that callback did not run on rejection.
Please update callers to distinguish success from failure and use finally for loading cleanup, or preserve rejection while ensuring the refresh button always settles. Add a caller-level test for the failed refresh path, not only the helper test.
2be02a1 to
30eb49f
Compare
|
Addressed the requested caller-level regression: pin success now depends on the boolean refresh result, both refresh-button and pin-submit loading clear in finally, and caller tests cover false/rejected refreshes plus failed pin requests. The branch is also rebased to current main and Playwright-verified with a delayed 500 refresh. |
|
Addressed on the current head: callers now distinguish the boolean result and use inally for loading cleanup; caller-level pin/refresh tests cover failure without a success toast. The combined production build passed, and Playwright verified that a forced failed refresh settles and a second refresh succeeds. Please re-review when convenient. |
openai0229
left a comment
There was a problem hiding this comment.
Re-reviewed the minimized implementation at 2da56d2. Refresh rejection now propagates, DashboardContent clears loading in finally, existing pin-success semantics remain unchanged, and the change is limited to five frontend files. Focused tests, full lint/build, Playwright failure-and-retry coverage, and JavaScript security checks passed.
openai0229
left a comment
There was a problem hiding this comment.
Re-reviewed final head 63a6e09. Both refresh callers now consume request rejections after global error handling, so loading settles without unhandled promise rejections and pin success is not shown after a failed refresh. Focused tests, full Community build, Playwright forced-500 verification, and CI passed.
Related issue
Fixes #2682
Summary
Dashboard refresh failures previously left the refresh action pending forever because the store wrapped the detail request without propagating rejection. The refresh request now resolves to
trueon success, resolves tofalsewhen no dashboard is selected, and preserves request rejection on failure.DashboardContentclears its loading state infinally, so the refresh control always settles while existing callers keep their original success semantics.Affected surfaces
Verification
yarn test:dashboard-store: passed success, rejection, missing-dashboard, and zero-ID cases.yarn lint: passed.yarn build:web:community --app_version=0.0.0: all Community prebuild tests, production build, and bundle verification passed.127.0.0.1:3306: datasourcepre_connectpassed in the isolated Community runtime; temporary database and account were removed after verification.Risk and compatibility
No API or persisted-data format changes. Failed detail requests reject as they did before the wrapper was introduced, but the UI now clears loading through
finally. The change is limited to six frontend files.Reviewer map
runDashboardRefresh,refreshCurrentDashboard, andDashboardContent.refreshChartList.Contributor declaration
AI assistance: OpenAI Codex assisted with conflict resolution, implementation reduction, tests, Playwright verification, and adversarial review.