fix(mapping): stop discarding stale auto-load responses, always clear loading state, keep column-role menu open - #153
Conversation
… loading state, keep column-role menu open Three related fixes in the entity/relationship mapping panels: The saved SQL query auto-loads in the background when a panel opens. Its response is now matched against the generation captured when it was scheduled (not re-read at response time), so it is no longer silently discarded if the panel finished mounting before the response arrived. Both the entity and relationship panels also stop discarding a response just because the user switched panel/generation in the meantime — the Refresh button and loading spinner are now always cleared in the completion path regardless, so a slow query can no longer leave the button stuck on "Refreshing..." or the spinner stuck visible. If the previously saved ID column is no longer present in a refreshed result set, the first available column is now auto-selected instead of being left unset. The per-column role menu (assign ID / Label / an attribute to a result column) now stays open across clicks and reopens on the same column after each assignment, instead of closing after every single click — multiple roles can be assigned to different columns in one pass. The existing "Clear" menu item (already present in the markup) now actually clears a column's roles, which it previously did not do at all: the click handler had no branch for it, so clicking it silently did nothing. Rebuilt from scratch against the current develop, which has meanwhile gained a schema-drift feature (auto-load, badges, panel close) in this same area — this PR only touches the pre-existing race-condition and menu-interaction issues above, none of it overlaps with schema-drift. A companion, unrelated bug — a literal DOM id shared between the entity and relationship panels' save buttons — no longer applies: develop's redesign replaced the old manual Apply/Save button with auto-save on panel dismiss, and removed that button from the markup entirely.
|
|
benoitcayladbx
left a comment
There was a problem hiding this comment.
Relevant, but the stale-response change as written would regress develop. Please keep the UX pieces (Clear handler, keep menu open, always restore Refresh / spinner); please do not drop the generation / panel-type guards.
develop already has:
EntityPanelState._generationincremented ininitEntityPanel()so in-flight fetches from a previous entity are discarded;if (currentPanelType !== 'entity' || capturedGeneration !== EntityPanelState._generation) return;before applying rows;- panel-host claiming (
claimMappingPanel/ issue #145) so Manual Mapping setscurrentPanelTypeand the auto-load is not discarded for the right reason.
Must fix — do not apply stale query results
Removing
if (currentPanelType !== 'entity' || capturedGeneration !== EntityPanelState._generation) return;means a slow preview for entity A can write EntityPanelState.columns / idColumn (and auto-pick columns[0]) after the user has opened entity B. Same for the relationship panel (currentPanelType !== 'relationship'). That is a data-corruption race, not a loading-state fix.
Intended split:
| Concern | Keep / change |
|---|---|
Apply result.columns / mutate idColumn / render grid |
Keep the generation + currentPanelType guard. Do not apply if stale. |
| Refresh button stuck on “Refreshing…” | In finally, always restore btn.disabled / label (you got this right). The stuck button is because finally currently restores only when generation still matches — after an entity→entity switch it is the same DOM node. |
Spinner stuck (epMappingLoading) |
If stale, only hide the spinner; do not render the old grid into the new panel. |
| Auto-load discarded because generation changed during the 100ms timer | Capture generation at schedule time if you want, but still compare at response time to the current generation. If they differ, skip apply. Passing the scheduled generation and then not comparing to current state is what makes the race come back. |
Passing generation: scheduledGeneration into runEntityPanelQuery is optional; capturing at call time (when the timer fires) is already correct on develop as long as initEntityPanel bumped _generation first (it does).
Other product concerns
-
Auto-select
columns[0]as ID when the saved ID is gone. Convenient, but it silently maps the wrong column after a schema change. Prefer: leave ID unset, surface the existing drift badge, and let the user pick. If you keep a fallback, only do it when there is no savedid_column(first-time mapping), not when a previously saved column disappeared. -
epSummary.style.display = ''vs'none'. Please confirm this matches the current Designer markup (summary vs grid). Un-hiding the summary on successful load can fight the grid you just showed. -
Dead
getElementById('savePanelBtn'). Agreed it is unused after auto-save-on-dismiss. Either remove those two helpers in this PR (tiny, related cleanup) or file a follow-up — leaving known dead code without a ticket tends to rot (.cursorrulesdead-code step).
Rules
- Changelog —
changelogs/v0.8.0/<github-user>_YYYY-MM-DD.log(English,.cursorrules). - Tests:
tests/units/front/already uses source-contract tests for this file (panel host / stale-response guards). Please add assertions that the stalereturn(or equivalent) is still present, plus thatfinallyrestores the button unconditionally. That will lock the split above. - CLA still pending.
The column-role menu (keep open, toggle, implement clear) is a solid UX fix — happy to take that once the race guard is restored.
What
Three related fixes in the entity/relationship mapping panels (
mapping-design.js), rebuilt from scratch against the currentdevelop:Auto-load race condition. The saved SQL query auto-loads in the background when a panel opens. Its response is now matched against the generation captured when the auto-load was scheduled, not re-read at response time — so it's no longer silently discarded if the panel finishes mounting before the response arrives. Both the entity and relationship panels also stop discarding a response just because the user switched panel/generation in the meantime; the Refresh button and loading spinner are now unconditionally cleared in the completion path, so a slow query can no longer leave the button stuck on "Refreshing..." or the spinner stuck visible. If a previously saved ID column is no longer present in a refreshed result set, the first available column is now auto-selected instead of left unset.
Column-role menu. The per-column menu (assign ID / Label / an attribute to a result column) now stays open across clicks and reopens on the same column after each assignment, instead of closing after every single click — so multiple roles can be assigned across columns in one pass without reopening the menu each time. The existing "Clear" menu item (already present in the markup, added along with the rest of this menu's current design) now actually clears a column's roles — previously the click handler had no branch for
data-action="clear"at all, so clicking it silently did nothing.Why
All three are UX papercuts that make the mapping panels tedious or misleading to use in practice: a stuck "Refreshing..." button after switching panels, a saved ID column silently reset to nothing after a schema change, and a "Clear" button that visibly exists but doesn't work.
Note on scope — rebuilt against current develop
This PR replaces a fix we originally wrote against 0.7.1 and held back, because
develophad since reworked this same file substantially (a schema-drift feature: auto-load of live-schema warnings, drift badges,guardedCloseMappingPanel). We rebuilt the fix from scratch directly against the current file rather than porting the old patch, dropping one piece of it that no longer applies: our original fix also renamed a literal DOM id (savePanelBtn→manualSavePanelBtn) shared between the entity and relationship panels' save buttons. That whole button is gone ondevelop— replaced by auto-save on panel dismiss — so there's nothing left to rename; the two leftovergetElementById('savePanelBtn')calls inupdateEntityPanelSaveBtn/updateRelPanelSaveBtnare harmless dead code (alwaysnull, guarded byif (saveBtn)) and we left them alone rather than touching code unrelated to what this PR fixes.How to test