feat: closeable info alerts on Sequence Properties (session-only)#13
feat: closeable info alerts on Sequence Properties (session-only)#13PaulNewling wants to merge 7 commits into
Conversation
PlAlert on the Main tab now renders a close button. Dismissals live in a local ref<Set<string>> — not persisted; resets when the block UI unmounts (project close, app reload, multi-client independent). Hairpin- safe: the only write is from PlAlert's close-button emit (user gesture). Minimal-change variant of the persisted-dismissal approach (paulnewling/fix/dismissable-info-messages). Useful when you want "acknowledged for this session, fresh on reopen" semantics and don't want to extend BlockData or add a migration step.
There was a problem hiding this comment.
Code Review
This pull request introduces a session-only dismissal mechanism for info-message alerts in MainPage.vue by using a ref and computed property. A review comment correctly identifies that defining the state within <script setup> causes it to reset upon component unmounting; it is recommended to move this state to a module-level scope to ensure persistence across tab navigation.
| const dismissedInfoMessages = ref(new Set<string>()); | ||
| const visibleInfoMessages = computed(() => | ||
| (app.model.outputs.info?.messages ?? []).filter((m) => !dismissedInfoMessages.value.has(m)), | ||
| ); |
There was a problem hiding this comment.
In Vue, variables declared inside <script setup> are instance-level state, not module-level state. When the user navigates to another tab (e.g., /scatter or /histogram), the MainPage component is unmounted, and its state is destroyed. When they navigate back to the Main tab, MainPage is remounted, and dismissedInfoMessages is re-initialized to an empty Set, causing the dismissed alerts to reappear.
To persist the dismissed state across tab navigation within the same session, you should move dismissedInfoMessages to a module-level scope (e.g., in ui/src/app.ts or in a separate <script> block in MainPage.vue).
Since MainPage.vue already imports from ../app, defining and exporting dismissedInfoMessages from ui/src/app.ts is a clean and robust solution.
In ui/src/app.ts:
export const dismissedInfoMessages = ref(new Set<string>());And in ui/src/pages/MainPage.vue:
import { useApp, dismissedInfoMessages } from "../app";const visibleInfoMessages = computed(() =>
(app.model.outputs.info?.messages ?? []).filter((m) => !dismissedInfoMessages.value.has(m)),
);
There was a problem hiding this comment.
Applied in d99e41e. Hoisted dismissedInfoMessages to module scope in app.ts; MainPage.vue imports it. Dismissals now survive Main ↔ Property Relationships ↔ Property Distribution tab switches; still reset on project close / app reload / switching to another block.
There was a problem hiding this comment.
Correction to my earlier reply: I was wrong that switching to another block resets dismissals. Block UIs are LRU-cached (limit 4) in the desktop app, so the JS context survives brief navigation away. Actual reset triggers: project close, block reload, LRU eviction once you've opened ~5 other blocks, or app restart. Code comment + PR description updated in 183be03.
CI flags the 2.5.29 pin as outdated. Build is clean against the new major version; no workflow code touched on this branch so the bump is catalog-only.
In Vue, refs declared inside <script setup> are component-instance state. Switching between block sections (Main, Property Relationships, Property Distribution) unmounts MainPage and resets the dismissals. Hoisting the ref to a module-level singleton in app.ts makes dismissals survive in-block route changes; still resets on project close / app reload / switching to another block. Addresses gemini-code-assist review comment.
Earlier comment said dismissals reset on "switching to another block and back", which is wrong. Block UIs are LRU-cached (limit 4) in the desktop app's WebContentsMap, so the JS context survives brief navigation away. Actual reset triggers: project close (cleanCachedBlockViews on deleteProjectOverview), block reload (LoadBlockFrontend with recreate), LRU eviction once the cache exceeds 4 entries, or app restart.
Dismissals are now tied to the currently-firing advisory set. When the workflow stops emitting a previously-dismissed string (re-run with a different input, advisory naturally goes away), it drops from the set. A future re-fire of the same string shows fresh — no carry-over. Implemented as watch(outputs.info.messages → local ref). Output → local Vue ref is the sanctioned pattern in hairpin.md (state lives in the JS context, not BlockData). No multi-client interleave risk. Closes the stickiness gap on this branch — dismissals follow the firing rather than persisting indefinitely.
| type="info" | ||
| closeable | ||
| :model-value="true" | ||
| @update:model-value="() => dismissInfoMessage(message)" |
There was a problem hiding this comment.
The
@update:model-value handler ignores the emitted boolean and calls dismissInfoMessage unconditionally. With :model-value="true" hardcoded, a close click will emit false — but if PlAlert ever emits update:model-value with true (e.g., during internal re-sync or after an animation), every message would be dismissed the moment it mounts. Guarding on !v makes the intent explicit and is safe against any emit direction.
| @update:model-value="() => dismissInfoMessage(message)" | |
| @update:model-value="(v) => !v && dismissInfoMessage(message)" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/pages/MainPage.vue
Line: 81
Comment:
The `@update:model-value` handler ignores the emitted boolean and calls `dismissInfoMessage` unconditionally. With `:model-value="true"` hardcoded, a close click will emit `false` — but if `PlAlert` ever emits `update:model-value` with `true` (e.g., during internal re-sync or after an animation), every message would be dismissed the moment it mounts. Guarding on `!v` makes the intent explicit and is safe against any emit direction.
```suggestion
@update:model-value="(v) => !v && dismissInfoMessage(message)"
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
Closeable info alerts on the Main tab. Dismissals are tied to currently-firing advisories: a dismissal applies while the message string is in the workflow output, and is automatically pruned when the message stops firing — so a re-fire later shows the advisory fresh.
State lives in a module-scope
ref<Set<string>>inapp.ts. Persists across in-block tab switches and switching away to other blocks (desktop LRU-caches block UIs, limit 4). Resets on project close, block reload, LRU eviction, or app restart. NoBlockDatachange. No migration. No Settings UI change.PR #12 prototyped a server-persisted alternative —
BlockData.dismissedInfoMessages+ a migration step — and was closed in favor of this approach. The persisted design gave "sticky" semantics: dismissed once stayed dismissed across all future workflow runs that re-emitted the same string. The "follow the firing" semantics here fit the UX better.Implementation
Two files (
ui/src/app.ts+ui/src/pages/MainPage.vue):dismissedInfoMessages = ref(new Set<string>())exported fromapp.ts. Module scope is what gives cross-tab and cross-block persistence.watch(() => app.model.outputs.info?.messages, ...)inapp.tsprunes the dismissed set to the intersection with currently-emitted messages whenever the output changes. Output → local Vue ref, sanctioned byharnesses/block-dev/hairpin.md.visibleInfoMessagescomputed inMainPage.vuefiltersoutputs.info?.messagesby exclusion from the dismissed set.dismissInfoMessage(m)writes to the ref on PlAlert's@update:model-valueclose emit.:key="message"(string content) replaces:key="idx"so Vue diffs correctly across dismissals.:model-value="true"keeps PlAlert rendered; the v-for filter governs visibility, not PlAlert's internal toggle.Hairpin-safe: the only writes are user-gesture dismissals and output → local-ref prune. Both shapes are explicitly sanctioned by the harness. No
watchEffect/watchwrites toBlockData.Comparison vs the persisted alternative (#12, closed)
BlockDataBlockDataschema changedismissedInfoMessages: string[]+ migrationWhy follow-firing
A dismissal acknowledges the advisory in its current context — it isn't a permanent silencing rule. Sticky semantics would have let a user dismiss "VHH detected" on one dataset and never see the advisory again, even on a fresh dataset where the heuristic legitimately re-fires under different conditions. Follow-firing matches the actual UX: acknowledge while relevant, surface again when conditions change.
Test plan
pnpm build:dev)