From 4bc81b59e785f4fb94a57c79a67bd30536ac5903 Mon Sep 17 00:00:00 2001 From: James Griffin Date: Fri, 28 Aug 2026 20:21:19 -0300 Subject: [PATCH] Keep the sidebar's open rows across refreshes Expanding a workflow run and waiting through one poll closed it again. Two causes. Nova applies whatever collapsible state getTreeItem returns, and it returned Collapsed for every row that was not built default-open, so each reload closed everything the user had opened. On top of that, a reload fired on every poll whether or not anything had changed, so an idle tree was rebuilt every 15 to 60 seconds for no reason. The provider now tracks expansion by node identifier, fed from the tree view's expand and collapse events. A node built default-open seeds that set the first time its identifier is seen, so such a section can still be closed by hand and stay closed. Node identifiers consequently have to be stable across rebuilds. The fallback for nodes without an explicit one was a counter, which changed on every build; it is now derived from the node's kind and name. Step rows were keyed by step number alone, which collided between jobs, and are now qualified by the job. Reloads are also skipped entirely when store.signature() is unchanged. The signature covers everything drawn, including the branch filter, which changes the tree without changing any data. A live run contributes a coarse time bucket so its elapsed clock still ticks, and the explicit Refresh command always redraws. Adds twelve checks covering expansion surviving a rebuild, a default-open section staying closed once closed, and the signature responding to run state and the branch filter but not to a no-op poll. 143 checks total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MQuusXgZC2dzwpJJ1qhtti --- CHANGELOG.md | 2 + CLAUDE.md | 17 +++++++- Scripts/main.js | 47 +++++++++++++++++++-- Scripts/store.js | 67 ++++++++++++++++++++++++++++++ Scripts/views/builders.js | 8 ++-- Scripts/views/nodes.js | 7 ++-- Scripts/views/provider.js | 41 +++++++++++++++--- Tests/integration.test.js | 87 +++++++++++++++++++++++++++++++++++++++ Tests/nova-stub.js | 10 ++++- 9 files changed, 267 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f46092..370682d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,3 +29,5 @@ Initial release. - Adaptive polling and a notification when a workflow run fails. - Gitea Actions CI running the test suites, script syntax checks, manifest validation, and a generated-image check. +- The sidebar keeps its open rows and scroll position across refreshes, and a + poll that changes nothing no longer redraws it at all. diff --git a/CLAUDE.md b/CLAUDE.md index a73c4c5..20aa5d7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,8 +70,12 @@ Controllers mutate through it. It owns: - `repos`, `unmatchedHosts`, `instanceStatus` - per-run `jobs`/`artifacts`, per-PR `pullRequestDetail`, `secrets`, `variables` -- `onDidChange` → `main.js` debounces 150ms → every provider's `invalidate()` + - `reload()` +- `onDidChange` → `main.js` debounces 150ms, compares `store.signature()`, and + reloads only when it changed. An idle poll therefore leaves the tree, its + scroll position, and its open rows completely alone. Anything that changes + what is drawn has to appear in that signature — the branch filter is in there + for exactly that reason, and a live run contributes a coarse time bucket so + its elapsed clock keeps moving. - adaptive polling: `runningInterval` when anything is queued or running, else `idleInterval`. `refresh()` coalesces — a second call while one is in flight returns the same promise rather than stacking. @@ -81,6 +85,15 @@ only `roots()`. Nodes are plain objects (`views/nodes.js` `node()`), turned into `TreeItem`s centrally. A node with `load` fetches when expanded; the result is cached onto `element.children`. +It also owns **expansion state**, which is not optional bookkeeping. Nova +applies whatever `collapsibleState` `getTreeItem` returns, so a reload closes +every row the provider does not report as open. `main.js` feeds +`onDidExpandElement`/`onDidCollapseElement` into `setExpanded`, keyed by node +identifier. A node built with `expanded: true` seeds that set the first time its +identifier is seen, so a default-open section can still be closed and stay +closed. **Node identifiers must therefore be stable across rebuilds** — never +derive one from a counter or from anything that changes between refreshes. + **`controllers/selection.js`** — a command can arrive from a sidebar (Nova passes the `TreeView`), from a node directly, or from a menu with no argument at all. `selectedNode()` normalises all three; `resolveRepo()` falls back to the diff --git a/Scripts/main.js b/Scripts/main.js index 59ea9bb..7d02ae4 100644 --- a/Scripts/main.js +++ b/Scripts/main.js @@ -52,7 +52,10 @@ function buildSections(store) { return definitions.map((definition) => { const view = new TreeView(definition.id, { dataProvider: definition.provider }); nova.subscriptions.add(view); - return Object.assign(definition, { view: view }); + + const section = Object.assign(definition, { view: view }); + trackExpansion(section); + return section; }); } @@ -64,7 +67,25 @@ function reloadSections(sections) { } } -function registerCommands(store, refresh, sections, reviewComments) { +/** + * Mirrors expansion back into the provider. Nova applies whatever collapsible + * state getTreeItem returns, so a reload closes anything the provider does not + * know is open. + */ +function trackExpansion(section) { + const expand = section.view.onDidExpandElement((element) => + section.provider.setExpanded(element, true), + ); + const collapse = section.view.onDidCollapseElement((element) => + section.provider.setExpanded(element, false), + ); + + for (const subscription of [expand, collapse]) { + if (subscription) nova.subscriptions.add(subscription); + } +} + +function registerCommands(store, refresh, reviewComments, redraw) { auth.register(store, refresh); logs.register(store); artifacts.register(store); @@ -75,6 +96,9 @@ function registerCommands(store, refresh, sections, reviewComments) { browser.register(); nova.commands.register("gitea.refresh", async () => { + // An explicit refresh always redraws, even if nothing came back + // different, so the command visibly does something. + redraw(); await refresh({ rediscover: true }); await reviewComments.reload({ force: true }); }); @@ -129,10 +153,27 @@ exports.activate = function () { ); let timer = null; + let lastSignature = null; + + /** Drops the memo so the next change redraws unconditionally. */ + const redraw = () => { + lastSignature = null; + }; + const scheduleReload = () => { if (timer) clearTimeout(timer); timer = setTimeout(() => { timer = null; + + // Reloading rebuilds every row, which is disruptive even with + // expansion preserved, so a poll that changed nothing does nothing. + const signature = store.signature(); + if (signature === lastSignature) { + log.debug("nothing changed; leaving the sidebar alone"); + return; + } + lastSignature = signature; + reloadSections(sections); }, RELOAD_DEBOUNCE_MS); }; @@ -144,7 +185,7 @@ exports.activate = function () { log.error("refresh failed", String(error)); }); - registerCommands(store, refresh, sections, reviewComments); + registerCommands(store, refresh, reviewComments, redraw); watchPreferences(refresh); state = { diff --git a/Scripts/store.js b/Scripts/store.js index 22a4215..d25d8d1 100644 --- a/Scripts/store.js +++ b/Scripts/store.js @@ -328,6 +328,73 @@ class Store { this.notify(); } + /** + * Fingerprint of everything the sidebar draws. `main.js` compares it before + * reloading, so an idle poll that changes nothing leaves the tree — and the + * user's scroll position and open rows — completely alone. + */ + signature() { + const parts = []; + + // The loading flag only reaches the screen while there is nothing else + // to show; including it always would defeat the whole comparison. + if (!this.repos.length) parts.push(`loading:${this.loading}`); + parts.push(`error:${this.lastError || ""}`); + + for (const entry of this.unmatchedHosts) parts.push(`unmatched:${entry.host}`); + + // The branch filter changes what is drawn without changing any data. + const filter = config.branchFilter(); + parts.push(`filter:${filter.mode}:${filter.branch || ""}`); + + for (const [baseUrl, status] of this.instanceStatus) { + parts.push( + `instance:${baseUrl}:${status.ok}:${status.version || ""}:${status.user || ""}:${status.error || ""}`, + ); + } + + for (const repo of this.repos) { + parts.push(`repo:${repo.key}:${repo.branch || ""}`); + + const runs = this.runsFor(repo.key); + parts.push(`runs:${runs.error || ""}:${runs.legacy}`); + for (const run of runs.runs) { + parts.push(`run:${run.id}:${run.status}:${run.conclusion}:${run.updated_at || ""}`); + } + + const pulls = this.pullRequestsFor(repo.key); + parts.push(`pulls:${pulls.error || ""}`); + for (const pull of pulls.items) { + parts.push(`pull:${pull.number}:${pull.state}:${pull.draft}:${pull.updated_at}`); + } + + const secrets = this.secrets.get(repo.key); + if (secrets) { + parts.push( + `secrets:${secrets.error || ""}:${secrets.items.map((x) => x.name).join(",")}`, + ); + } + + const variables = this.variables.get(repo.key); + if (variables) { + parts.push( + `variables:${variables.error || ""}:${variables.items + .map((x) => `${x.name}=${x.data || x.value || ""}`) + .join(",")}`, + ); + } + } + + // While something is running its elapsed time is on screen and has to + // keep moving, so let the signature change once per active interval. + if (this.hasActiveRuns) { + const seconds = Math.max(5, config.runningInterval()); + parts.push(`tick:${Math.floor(Date.now() / 1000 / seconds)}`); + } + + return parts.join("|"); + } + // -- Housekeeping ------------------------------------------------------ _pruneCaches() { diff --git a/Scripts/views/builders.js b/Scripts/views/builders.js index 8473dea..ab44126 100644 --- a/Scripts/views/builders.js +++ b/Scripts/views/builders.js @@ -10,10 +10,10 @@ const { isActiveRun } = require("../store.js"); const { node, message } = nodes; -function stepNode(step, index) { +function stepNode(job, step, index) { const state = nodes.runState(step); return node("step", `${nodes.glyphFor(state)} ${step.name || `Step ${index + 1}`}`, { - identifier: `step-${step.number || index}`, + identifier: `step-${job.id}-${step.number || index}`, description: time.duration(step.started_at, step.completed_at), tooltip: `${step.name || ""}\nStatus: ${state}`, contextValue: "step", @@ -39,7 +39,9 @@ function jobNode(store, repo, run, job) { command: "gitea.viewJobLogs", repo: repo, data: { job: job, run: run, repoKey: repo.key }, - children: steps.length ? steps.map(stepNode) : null, + children: steps.length + ? steps.map((step, index) => stepNode(job, step, index)) + : null, }); } diff --git a/Scripts/views/nodes.js b/Scripts/views/nodes.js index d35469b..486172c 100644 --- a/Scripts/views/nodes.js +++ b/Scripts/views/nodes.js @@ -50,18 +50,17 @@ function reviewGlyph(state) { return "◆"; } -let counter = 0; - /** * Builds a tree node. `children` is a resolved array; `load` is an async * producer used for rows that fetch on expand. */ function node(kind, name, options = {}) { - counter += 1; const value = { kind: kind, name: name, - identifier: options.identifier || `${kind}-${counter}`, + // Identifiers must be stable across rebuilds: Nova matches them to + // work out which rows stay open when the tree reloads. + identifier: options.identifier || `${kind}:${name}`, descriptiveText: options.description || "", tooltip: options.tooltip || "", image: options.image || null, diff --git a/Scripts/views/provider.js b/Scripts/views/provider.js index aa8017b..d3f8b32 100644 --- a/Scripts/views/provider.js +++ b/Scripts/views/provider.js @@ -10,6 +10,35 @@ class NodeProvider { constructor(store) { this.store = store; this._roots = null; + + // Expansion has to be tracked here. A reload rebuilds every node, and + // the collapsible state we hand back is what Nova applies, so without + // this every refresh would close whatever the user had opened. + this._expanded = new Set(); + this._known = new Set(); + } + + /** Records what the user opened or closed, keyed by node identifier. */ + setExpanded(element, expanded) { + if (!element || !element.identifier) return; + if (expanded) this._expanded.add(element.identifier); + else this._expanded.delete(element.identifier); + } + + /** + * Whether a row should render open. A node marked `expanded` seeds the set + * the first time it is seen, so a default-open section can still be closed + * by hand and stay closed. + */ + isExpanded(element) { + const id = element.identifier; + if (!id) return element.expanded === true; + + if (!this._known.has(id)) { + this._known.add(id); + if (element.expanded) this._expanded.add(id); + } + return this._expanded.has(id); } /** Subclasses return an array of nodes (or a Promise of one). */ @@ -55,12 +84,12 @@ class NodeProvider { } getTreeItem(element) { - const collapsible = - element.children || element.load - ? element.expanded - ? TreeItemCollapsibleState.Expanded - : TreeItemCollapsibleState.Collapsed - : TreeItemCollapsibleState.None; + const expandable = Boolean(element.children || element.load); + const collapsible = !expandable + ? TreeItemCollapsibleState.None + : this.isExpanded(element) + ? TreeItemCollapsibleState.Expanded + : TreeItemCollapsibleState.Collapsed; const item = new TreeItem(element.name, collapsible); item.identifier = element.identifier; diff --git a/Tests/integration.test.js b/Tests/integration.test.js index 24207e4..5b3530f 100644 --- a/Tests/integration.test.js +++ b/Tests/integration.test.js @@ -242,6 +242,93 @@ function declaredCommands(manifest) { JSON.stringify(global.__messages), ); + t.section("the sidebar keeps its shape across refreshes"); + // A poll that rebuilds every row used to close whatever the user had open. + const workflowsView = views["gitea.workflows"]; + const workflowsProvider = workflowsView.dataProvider; + + const group = (await workflowsProvider.getChildren(null)).find( + (n) => n.kind === "workflow" && n.name === "ci.yml", + ); + const openMe = (await workflowsProvider.getChildren(group)).find((n) => n.data.run.id === 101); + + t.equals( + "a run starts collapsed", + workflowsProvider.getTreeItem(openMe).collapsibleState, + TreeItemCollapsibleState.Collapsed, + ); + + workflowsView.expand(group); + workflowsView.expand(openMe); + + // What a refresh does: throw the built tree away and rebuild it. + workflowsProvider.invalidate(); + const rebuiltGroup = (await workflowsProvider.getChildren(null)).find( + (n) => n.kind === "workflow" && n.name === "ci.yml", + ); + const rebuiltRun = (await workflowsProvider.getChildren(rebuiltGroup)).find( + (n) => n.data.run.id === 101, + ); + + t.check("the rebuilt run is a different object", rebuiltRun !== openMe); + t.equals("but carries the same identifier", rebuiltRun.identifier, openMe.identifier); + t.equals( + "and is still open", + workflowsProvider.getTreeItem(rebuiltRun).collapsibleState, + TreeItemCollapsibleState.Expanded, + ); + t.equals( + "its parent too", + workflowsProvider.getTreeItem(rebuiltGroup).collapsibleState, + TreeItemCollapsibleState.Expanded, + ); + + workflowsView.collapse(rebuiltRun); + workflowsProvider.invalidate(); + const closedAgain = (await workflowsProvider.getChildren( + (await workflowsProvider.getChildren(null)).find((n) => n.name === "ci.yml"), + )).find((n) => n.data.run.id === 101); + t.equals( + "closing it sticks too", + workflowsProvider.getTreeItem(closedAgain).collapsibleState, + TreeItemCollapsibleState.Collapsed, + ); + + // A section that opens by default must still be closable. + const branchProvider2 = views["gitea.branch"].dataProvider; + const folder = (await branchProvider2.getChildren(null)).find((n) => n.kind === "prFolder"); + t.equals( + "a default-open folder starts open", + branchProvider2.getTreeItem(folder).collapsibleState, + TreeItemCollapsibleState.Expanded, + ); + views["gitea.branch"].collapse(folder); + branchProvider2.invalidate(); + const folderAgain = (await branchProvider2.getChildren(null)).find((n) => n.kind === "prFolder"); + t.equals( + "and stays closed once the user closes it", + branchProvider2.getTreeItem(folderAgain).collapsibleState, + TreeItemCollapsibleState.Collapsed, + ); + + t.section("idle polls do not redraw"); + const first = store.signature(); + t.equals("an unchanged store yields an unchanged signature", store.signature(), first); + + const trackedRuns = store.runsFor(store.repos[0].key).runs; + const finished = trackedRuns.find((run) => run.id === 103); + const previousConclusion = finished.conclusion; + finished.conclusion = "success"; + t.check("a changed run conclusion changes it", store.signature() !== first); + finished.conclusion = previousConclusion; + + t.equals("and restoring it restores the signature", store.signature(), first); + + const branchFilterBefore = store.signature(); + nova.workspace.config.set("gitea.branchFilter", "all"); + t.check("changing the branch filter changes it", store.signature() !== branchFilterBefore); + nova.workspace.config.set("gitea.branchFilter", "current"); + t.section("api paths exercised"); console.log([...new Set(fake.calls.map((call) => call.split("?")[0]))].sort().join("\n")); diff --git a/Tests/nova-stub.js b/Tests/nova-stub.js index 62872e6..6c0e6c6 100644 --- a/Tests/nova-stub.js +++ b/Tests/nova-stub.js @@ -64,9 +64,17 @@ global.TreeView = class TreeView { this.dataProvider = options.dataProvider; this.visible = true; this.selection = []; + this.reloads = 0; + this._onExpand = []; + this._onCollapse = []; treeViews.push(this); } - reload() { return Promise.resolve(); } + reload() { this.reloads += 1; return Promise.resolve(); } + onDidExpandElement(cb) { this._onExpand.push(cb); return { dispose() {} }; } + onDidCollapseElement(cb) { this._onCollapse.push(cb); return { dispose() {} }; } + // Test helpers: pretend the user opened or closed a row. + expand(element) { for (const cb of this._onExpand) cb(element); } + collapse(element) { for (const cb of this._onCollapse) cb(element); } dispose() {} };