Preserve scroll position when adding actions to Focus#42
Merged
Conversation
When adding a focus item from the SphereView, saving the focus file triggered a metadata cache change that caused a full view refresh, resetting scroll to top. Instead, focus file changes now trigger a lightweight CSS-only update that toggles sphere-action-in-focus classes on existing DOM elements without re-rendering. Fixes #41
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSphereView adds workspace-level event tracking and a suppressFocusRefresh flag, replaces simple focus-file handling with refreshFocusHighlighting, emits/handles flow:action-completed to prune DOM, and annotates action elements with per-item focus data attributes; tests and mocks updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant MC as MetadataCache
participant SV as SphereView
participant FP as FocusPersistence
participant DOM as ActionItemsDOM
participant WS as Workspace
MC->>SV: metadataChanged(filePath)
activate SV
SV->>FP: read FOCUS_FILE_PATH / load focus items
alt filePath == FP::FOCUS_FILE_PATH and not SV.suppressFocusRefresh
SV->>SV: schedule full refresh
else if SV.isRelevantFile(filePath)
SV->>SV: schedule auto-refresh
else
SV-->>MC: ignore
end
SV->>SV: save container.scrollTop
SV->>DOM: renderActionItems (attach data-focus-file / data-focus-action)
SV->>FP: loadFocusItems()
FP-->>SV: currentFocusItems
SV->>DOM: apply/remove sphere-action-in-focus per item (refreshFocusHighlighting)
SV->>SV: restore container.scrollTop
deactivate SV
Note over WS,SV: When an item is completed in FocusView
WS->>SV: flow:action-completed {file, action}
SV->>SV: removeActionFromDom(file, action)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/sphere-view.ts (1)
602-609:loadFocusItemsis called per action item — O(n) file reads.Each call to
renderActionItemindividually loads focus items from disk (line 603). With many actions this means redundant I/O. The newrefreshFocusHighlightingalready demonstrates the better pattern: load once, iterate. Consider passing the pre-loaded focus items intorenderActionItem, or skipping the initial focus check entirely and relying on a singlerefreshFocusHighlighting()call after the view is rendered.Sketch: load once, pass in
In
renderProjectsSection/renderGeneralNextActionsSection, load once before the loop:+ const focusItems = await loadFocusItems(this.app.vault); project.nextActions.forEach((action, index) => { - void this.renderActionItem(list, action, project.file, this.sphere, false); + void this.renderActionItem(list, action, project.file, this.sphere, false, focusItems); });Then in
renderActionItem, accept and use the pre-loaded list instead of callingloadFocusItemsagain.Alternatively, remove the in-focus check from
renderActionItementirely and callthis.refreshFocusHighlighting()once at the end ofrenderContent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 602 - 609, The per-item call to loadFocusItems inside renderActionItem causes O(n) disk reads; instead load focus items once and pass them into renderActionItem (or remove the check there and call this.refreshFocusHighlighting() once after renderContent). Concretely: update renderProjectsSection and renderGeneralNextActionsSection to call const focusItems = await loadFocusItems(this.app.vault) before looping, change renderActionItem to accept a focusItems parameter and use that list for the inFocus check (replace the await loadFocusItems(...) call), or alternatively delete the in-focus logic from renderActionItem and invoke this.refreshFocusHighlighting() once after rendering completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sphere-view.ts`:
- Around line 602-609: The per-item call to loadFocusItems inside
renderActionItem causes O(n) disk reads; instead load focus items once and pass
them into renderActionItem (or remove the check there and call
this.refreshFocusHighlighting() once after renderContent). Concretely: update
renderProjectsSection and renderGeneralNextActionsSection to call const
focusItems = await loadFocusItems(this.app.vault) before looping, change
renderActionItem to accept a focusItems parameter and use that list for the
inFocus check (replace the await loadFocusItems(...) call), or alternatively
delete the in-focus logic from renderActionItem and invoke
this.refreshFocusHighlighting() once after rendering completes.
The CSS-only approach for focus file changes broke sphere view updates when items were completed from the Focus view, because the focus file change was the only reliable trigger for a full refresh (Obsidian's metadata cache doesn't fire for checkbox body changes). Instead, preserve scroll position by loading sphere data before emptying the container, so the empty-and-rebuild happens in one synchronous block with no async gap (no visible flash). Scroll position is saved and restored around the refresh.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sphere-view.ts (1)
600-607: 🛠️ Refactor suggestion | 🟠 Major
loadFocusItemsis called once per action item — O(N) file reads.Each call to
renderActionItemindependently callsloadFocusItems, which reads the focus file from the vault. For a sphere with many actions, this results in redundant I/O. Load the focus items once in the calling context (e.g.,renderProjectsSection/renderGeneralNextActionsSection) and pass the array intorenderActionItem.Sketch of the fix
private async renderActionItem( list: HTMLElement, action: string, file: string, sphere: string, - isGeneral: boolean + isGeneral: boolean, + focusItems?: FocusItem[] ): Promise<void> { // ... - const focusItems = await loadFocusItems(this.app.vault); + const resolvedFocusItems = focusItems ?? await loadFocusItems(this.app.vault); const inFocus = resolvedFocusItems.some( (focusItem) => focusItem.file === file && focusItem.text === action );Then in the callers, load once and pass:
+ const focusItems = await loadFocusItems(this.app.vault); project.nextActions.forEach((action, index) => { - void this.renderActionItem(list, action, project.file, this.sphere, false); + void this.renderActionItem(list, action, project.file, this.sphere, false, focusItems); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 600 - 607, renderActionItem currently calls loadFocusItems for every action, causing O(N) vault reads; instead, change renderActionItem to accept a preloaded focusItems array parameter (from loadFocusItems) and use that to determine inFocus, then remove the internal loadFocusItems call. Update callers such as renderProjectsSection and renderGeneralNextActionsSection to call loadFocusItems(this.app.vault) once, store the result, and pass it into each renderActionItem invocation. Ensure function signatures (renderActionItem) and all call sites are updated accordingly and types/signatures are adjusted where needed.
🧹 Nitpick comments (3)
src/sphere-view.ts (2)
160-168: Double-refresh after focus toggle: consider guarding.When a user clicks an action,
addToFocus/removeFromFocusupdates the CSS class locally, but also writes the focus file. The metadata cache listener on line 165 will then firescheduleAutoRefresh, causing a redundant full re-render 500ms later. The scroll preservation mitigates the UX impact, but you could avoid the extra work by setting a short "suppress auto-refresh" flag around focus writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 160 - 168, The metadata change listener (registered in registerMetadataCacheListener via this.metadataCacheEventRef) triggers scheduleAutoRefresh even when focus toggles originate locally (addToFocus/removeFromFocus) causing a redundant full re-render; introduce a short-lived guard (e.g., this.suppressAutoRefresh boolean) that addToFocus/removeFromFocus sets before writing the FOCUS_FILE_PATH and clears after the write completes or after a short timeout, and update the "changed" handler to return early when this.suppressAutoRefresh is true so scheduleAutoRefresh is not called for locally-triggered metadata updates.
1-9: Missing required ABOUTME comment lines.Source files must start with two ABOUTME comment lines describing file purpose. As per coding guidelines:
src/**/*.{ts,tsx}: "All source files must start with two ABOUTME comment lines describing file purpose."Proposed fix
+// ABOUTME: SphereView renders the GTD sphere view showing projects and next actions. +// ABOUTME: Handles focus toggling, scroll preservation, filtering, and project hierarchy display. import { ItemView, WorkspaceLeaf,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 1 - 9, This file is missing the mandated two ABOUTME header comment lines; add two top-of-file comment lines starting with ABOUTME that succinctly describe the file's purpose (e.g., what SphereView provides/does) and its relation to the Obsidian plugin, placing them before the import block in src/sphere-view.ts; reference the primary symbol implemented here (e.g., the SphereView class or any exported functions) in those comments so the file meets the `src/**/*.{ts,tsx}` guideline.tests/sphere-view.test.ts (1)
743-744: Reliance onsetTimeout(resolve, 10)for async rendering is fragile.These timeouts may flake in slow CI environments. Consider a polling helper (e.g., wait until
container.querySelectorAll("li[data-focus-file]").length === expectedCount) for more deterministic assertions. This is a minor concern — the tests likely pass reliably in practice.Also applies to: 766-767
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sphere-view.test.ts` around lines 743 - 744, The tests use fragile fixed delays (await new Promise(resolve => setTimeout(resolve, 10))) after calling view.onOpen(); instead replace them with a deterministic polling helper that waits until the DOM reaches the expected state (for example, a helper like waitForCondition(() => container.querySelectorAll('li[data-focus-file]').length === expectedCount, { timeout: 2000, interval: 50 }) or use your test framework's waitFor/awaitFor utilities) so assertions only run once the expected elements exist; update the two occurrences after view.onOpen() to call that helper (referencing view.onOpen() and container.querySelectorAll('li[data-focus-file]') to locate the changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/sphere-view.ts`:
- Around line 600-607: renderActionItem currently calls loadFocusItems for every
action, causing O(N) vault reads; instead, change renderActionItem to accept a
preloaded focusItems array parameter (from loadFocusItems) and use that to
determine inFocus, then remove the internal loadFocusItems call. Update callers
such as renderProjectsSection and renderGeneralNextActionsSection to call
loadFocusItems(this.app.vault) once, store the result, and pass it into each
renderActionItem invocation. Ensure function signatures (renderActionItem) and
all call sites are updated accordingly and types/signatures are adjusted where
needed.
---
Nitpick comments:
In `@src/sphere-view.ts`:
- Around line 160-168: The metadata change listener (registered in
registerMetadataCacheListener via this.metadataCacheEventRef) triggers
scheduleAutoRefresh even when focus toggles originate locally
(addToFocus/removeFromFocus) causing a redundant full re-render; introduce a
short-lived guard (e.g., this.suppressAutoRefresh boolean) that
addToFocus/removeFromFocus sets before writing the FOCUS_FILE_PATH and clears
after the write completes or after a short timeout, and update the "changed"
handler to return early when this.suppressAutoRefresh is true so
scheduleAutoRefresh is not called for locally-triggered metadata updates.
- Around line 1-9: This file is missing the mandated two ABOUTME header comment
lines; add two top-of-file comment lines starting with ABOUTME that succinctly
describe the file's purpose (e.g., what SphereView provides/does) and its
relation to the Obsidian plugin, placing them before the import block in
src/sphere-view.ts; reference the primary symbol implemented here (e.g., the
SphereView class or any exported functions) in those comments so the file meets
the `src/**/*.{ts,tsx}` guideline.
In `@tests/sphere-view.test.ts`:
- Around line 743-744: The tests use fragile fixed delays (await new
Promise(resolve => setTimeout(resolve, 10))) after calling view.onOpen();
instead replace them with a deterministic polling helper that waits until the
DOM reaches the expected state (for example, a helper like waitForCondition(()
=> container.querySelectorAll('li[data-focus-file]').length === expectedCount, {
timeout: 2000, interval: 50 }) or use your test framework's waitFor/awaitFor
utilities) so assertions only run once the expected elements exist; update the
two occurrences after view.onOpen() to call that helper (referencing
view.onOpen() and container.querySelectorAll('li[data-focus-file]') to locate
the changes).
The fire-and-forget renderActionItem calls mean the DOM isn't fully built when scrollTop is set, causing the browser to clamp the value. Use MutationObserver to wait for 50ms of DOM stability before restoring the scroll position.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/sphere-view.ts (4)
624-631:loadFocusItemsis called once per action item — O(n) file reads per refresh.Each
renderActionItemcall independently reads and parsesfocus.mdfrom disk (line 625). For a sphere with many actions, this results in redundant I/O. Consider loading focus items once per refresh cycle and passing the array intorenderActionItem.Sketch
- private async renderActionItem( - list: HTMLElement, - action: string, - file: string, - sphere: string, - isGeneral: boolean - ): Promise<void> { + private async renderActionItem( + list: HTMLElement, + action: string, + file: string, + sphere: string, + isGeneral: boolean, + focusItems?: FocusItem[] + ): Promise<void> { ... - const focusItems = await loadFocusItems(this.app.vault); + const items = focusItems ?? await loadFocusItems(this.app.vault); - const inFocus = focusItems.some( + const inFocus = items.some( (focusItem) => focusItem.file === file && focusItem.text === action );Then in the callers (
renderProjectsSection,renderGeneralNextActionsSection), load once and pass through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 624 - 631, The current renderActionItem calls loadFocusItems() inside each item, causing O(n) file reads; instead, load focusItems once per refresh and pass that array into renderActionItem. Change renderActionItem to accept a focusItems parameter (or optional cache) and remove the internal await loadFocusItems call, then update callers renderProjectsSection and renderGeneralNextActionsSection to call loadFocusItems(this.app.vault) once, store the result in a local variable (focusItems), and pass it into each renderActionItem invocation so the per-item file read is eliminated.
1-9: Missing required ABOUTME comment lines at the top of the file.As per coding guidelines, all source files under
src/must start with two ABOUTME comment lines describing the file's purpose.Proposed fix
+// ABOUTME: SphereView renders the GTD sphere view showing projects, actions, and focus state. +// ABOUTME: Handles scroll preservation, metadata-driven refresh, and focus toggling. import { ItemView,As per coding guidelines:
src/**/*.{ts,tsx}: All source files must start with two ABOUTME comment lines describing file purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 1 - 9, This file is missing the required two ABOUTME comment lines at the top; add two single-line comments as the first two lines of src/sphere-view.ts that briefly describe the file's purpose (e.g., what the ItemView subclass in this file does), so the ABOUTME header precedes any imports such as ItemView, WorkspaceLeaf, or other symbols; ensure both comment lines follow the project's ABOUTME format and are placed before the existing import block.
302-310: Redundantcontainer.empty()—renderContentalready empties the container.Line 306 calls
container.empty(), thenrenderContentat line 381 does the same. The explicit empty on line 306 is redundant.Proposed fix
private async refresh(): Promise<void> { const container = this.contentEl; const scrollTop = container.scrollTop; const data = await this.loadSphereData(); - container.empty(); this.renderContent(container, data); await this.waitForRenderSettled(container); container.scrollTop = scrollTop; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 302 - 310, Redundant container.empty() in refresh(): remove the explicit call to container.empty() inside the refresh() method and rely on renderContent(container, data) to clear and repopulate the container; keep preserving scrollTop, calling waitForRenderSettled(container), and restoring container.scrollTop to preserve behavior. Update the refresh() method (the container.empty() call between loadSphereData() and renderContent()) accordingly so there is no double-emptying.
160-169: Focus file changes now trigger a full refresh — acceptable with scroll preservation, but worth noting the interaction.When
addToFocus/removeFromFocussave focus items, this listener will fire ~500ms later and trigger a fullrefresh(). The CSS class was already toggled immediately for responsiveness, so the subsequent full refresh is redundant but ensures consistency for external modifications. The scroll preservation inrefresh()keeps this from being disruptive.If performance becomes a concern with many projects, consider short-circuiting the full refresh when the focus file changes (e.g., only updating
sphere-action-in-focusclasses on existing DOM elements using thedata-focus-*attributes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 160 - 169, Listener registerMetadataCacheListener currently treats changes to FOCUS_FILE_PATH the same as other files and calls scheduleAutoRefresh (leading to a full refresh); instead, short-circuit inside the metadataCache "changed" handler: if file.path === FOCUS_FILE_PATH then update the DOM-only state by toggling the sphere-action-in-focus class on existing elements using the data-focus-* attributes (find elements by data-focus-id or equivalent) and return early without calling scheduleAutoRefresh; otherwise fall through to the existing isRelevantFile check and call scheduleAutoRefresh as before. Ensure you still update metadataCacheEventRef via app.metadataCache.on and preserve scroll logic inside refresh unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sphere-view.ts`:
- Around line 312-333: The waitForRenderSettled function can hang because the
50ms debounce timeout is continually reset by mutations; add a hard upper-bound
timeout (e.g., MAX_WAIT_MS = 2000) so the returned Promise always resolves even
if mutations keep occurring. Modify waitForRenderSettled to start both the
existing debounce timeout (variable timeout) and a separate maxTimeout; if
maxTimeout fires, disconnect the MutationObserver, clear the debounce timeout,
and resolve; likewise, when the debounce resolves first, clear the maxTimeout
and disconnect. Reference the function name waitForRenderSettled, the observer
variable, timeout local, and ensure both timers are cleared on settle to avoid
leaks.
---
Nitpick comments:
In `@src/sphere-view.ts`:
- Around line 624-631: The current renderActionItem calls loadFocusItems()
inside each item, causing O(n) file reads; instead, load focusItems once per
refresh and pass that array into renderActionItem. Change renderActionItem to
accept a focusItems parameter (or optional cache) and remove the internal await
loadFocusItems call, then update callers renderProjectsSection and
renderGeneralNextActionsSection to call loadFocusItems(this.app.vault) once,
store the result in a local variable (focusItems), and pass it into each
renderActionItem invocation so the per-item file read is eliminated.
- Around line 1-9: This file is missing the required two ABOUTME comment lines
at the top; add two single-line comments as the first two lines of
src/sphere-view.ts that briefly describe the file's purpose (e.g., what the
ItemView subclass in this file does), so the ABOUTME header precedes any imports
such as ItemView, WorkspaceLeaf, or other symbols; ensure both comment lines
follow the project's ABOUTME format and are placed before the existing import
block.
- Around line 302-310: Redundant container.empty() in refresh(): remove the
explicit call to container.empty() inside the refresh() method and rely on
renderContent(container, data) to clear and repopulate the container; keep
preserving scrollTop, calling waitForRenderSettled(container), and restoring
container.scrollTop to preserve behavior. Update the refresh() method (the
container.empty() call between loadSphereData() and renderContent()) accordingly
so there is no double-emptying.
- Around line 160-169: Listener registerMetadataCacheListener currently treats
changes to FOCUS_FILE_PATH the same as other files and calls scheduleAutoRefresh
(leading to a full refresh); instead, short-circuit inside the metadataCache
"changed" handler: if file.path === FOCUS_FILE_PATH then update the DOM-only
state by toggling the sphere-action-in-focus class on existing elements using
the data-focus-* attributes (find elements by data-focus-id or equivalent) and
return early without calling scheduleAutoRefresh; otherwise fall through to the
existing isRelevantFile check and call scheduleAutoRefresh as before. Ensure you
still update metadataCacheEventRef via app.metadataCache.on and preserve scroll
logic inside refresh unchanged.
Comment on lines
312
to
333
| private waitForRenderSettled(container: HTMLElement): Promise<void> { | ||
| if (typeof MutationObserver === "undefined") { | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| return new Promise((resolve) => { | ||
| const observer = new MutationObserver(() => { | ||
| clearTimeout(timeout); | ||
| timeout = setTimeout(() => { | ||
| observer.disconnect(); | ||
| resolve(); | ||
| }, 50); | ||
| }); | ||
|
|
||
| let timeout = setTimeout(() => { | ||
| observer.disconnect(); | ||
| resolve(); | ||
| }, 50); | ||
|
|
||
| observer.observe(container, { childList: true, subtree: true, characterData: true }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
waitForRenderSettled has no upper-bound timeout — can hang indefinitely.
If DOM mutations keep firing (e.g., animated elements, recursive renders, or third-party plugins modifying the container), the 50ms timer resets on every mutation and the promise never resolves. This silently blocks refresh() and scroll restoration.
Add a maximum timeout as a safety net:
Proposed fix
private waitForRenderSettled(container: HTMLElement): Promise<void> {
if (typeof MutationObserver === "undefined") {
return Promise.resolve();
}
+ const MAX_WAIT = 2000;
return new Promise((resolve) => {
+ const done = () => {
+ clearTimeout(timeout);
+ clearTimeout(maxTimeout);
+ observer.disconnect();
+ resolve();
+ };
+
const observer = new MutationObserver(() => {
clearTimeout(timeout);
timeout = setTimeout(() => {
- observer.disconnect();
- resolve();
+ done();
}, 50);
});
let timeout = setTimeout(() => {
- observer.disconnect();
- resolve();
+ done();
}, 50);
+ const maxTimeout = setTimeout(() => {
+ done();
+ }, MAX_WAIT);
+
observer.observe(container, { childList: true, subtree: true, characterData: true });
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sphere-view.ts` around lines 312 - 333, The waitForRenderSettled function
can hang because the 50ms debounce timeout is continually reset by mutations;
add a hard upper-bound timeout (e.g., MAX_WAIT_MS = 2000) so the returned
Promise always resolves even if mutations keep occurring. Modify
waitForRenderSettled to start both the existing debounce timeout (variable
timeout) and a separate maxTimeout; if maxTimeout fires, disconnect the
MutationObserver, clear the debounce timeout, and resolve; likewise, when the
debounce resolves first, clear the maxTimeout and disconnect. Reference the
function name waitForRenderSettled, the observer variable, timeout local, and
ensure both timers are cleared on settle to avoid leaks.
Instead of re-rendering and restoring scroll (which causes a flash), set a flag before saving focus items so the metadata cache handler skips the auto-refresh. The CSS class update already happens locally. External focus changes (e.g. completing items in FocusView) still trigger a full refresh as before.
Focus file changes now trigger CSS-only updates in the sphere view (toggling sphere-action-in-focus classes) instead of a full re-render. When an item is completed in FocusView, a flow:action-completed workspace event removes the specific action from the sphere view DOM. This eliminates the jarring flash when interacting with the focus view while the sphere view is visible alongside it.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/sphere-view.ts (2)
650-663:loadFocusItemscalled per action item — N reads for N actions.Each call to
renderActionItemindependently callsloadFocusItems(line 657), resulting in one vault read per action. For spheres with many actions, this multiplies I/O on every render.Consider loading the focus items once in the caller (
renderProjectsSection/renderGeneralNextActionsSection) and passing the array intorenderActionItem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 650 - 663, renderActionItem currently calls loadFocusItems for each action causing N vault reads; instead, remove the await loadFocusItems(...) call from renderActionItem and add a new parameter (e.g., focusItems: FocusItem[]) to renderActionItem so it checks inFocus against the passed array; update the callers renderProjectsSection and renderGeneralNextActionsSection to call loadFocusItems(this.app.vault) once before iterating actions and pass that array into each renderActionItem invocation; keep the same equality check (focusItem.file === file && focusItem.text === action) and ensure callers await the single loadFocusItems promise once.
1-16: Missing ABOUTME comment lines.As per coding guidelines, all source files in
src/must start with two ABOUTME comment lines describing the file's purpose.Proposed fix
+// ABOUTME: Sphere view displaying projects and actions grouped by a single sphere. +// ABOUTME: Supports focus toggling, priority editing, and CSS-only focus highlighting. + import { ItemView, WorkspaceLeaf,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sphere-view.ts` around lines 1 - 16, This file is missing the required two ABOUTME comment lines at the top; add two comment lines at the very beginning of src/sphere-view.ts that start with ABOUTME and briefly describe the file's purpose (e.g., "ABOUTME: UI view for Sphere projects and summaries" and a second line with author/intent or short description), placing them before the import block so tools/readers pick them up; ensure they follow the project's comment style (//) and include a concise description referencing that this file defines the Sphere view and coordinates SphereDataLoader, SphereProjectSummary, and related view components like ItemView/WorkspaceLeaf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sphere-view.ts`:
- Around line 184-192: registerWorkspaceEvents currently appends a new
completedRef to workspaceEventRefs every time it's called, leaking EventRefs;
before registering the new handler inside registerWorkspaceEvents, unregister
any existing refs by iterating this.workspaceEventRefs and calling
this.app.workspace.off(ref) (or the appropriate unregister method used
elsewhere), then clear this.workspaceEventRefs, then create the new completedRef
for the "flow:action-completed" listener and push it; this mirrors the cleanup
behavior used in registerMetadataCacheListener and prevents accumulating
handlers.
In `@tests/sphere-view.test.ts`:
- Around line 945-963: The test for SphereView's cleanup is weak because it only
asserts app.workspace.offref is defined instead of verifying it was called;
update the test after creating the view and calling view.onOpen() and
view.onClose() to assert that app.workspace.offref (the mock function) was
called (e.g., using toHaveBeenCalled or toHaveBeenCalledTimes) and optionally
verify it was called with the same event refs registered via app.workspace.on —
locate the test referencing SphereView, onOpen, onClose, and
app.workspace.offref and replace the toBeDefined assertion with a call assertion
against the mock.
---
Nitpick comments:
In `@src/sphere-view.ts`:
- Around line 650-663: renderActionItem currently calls loadFocusItems for each
action causing N vault reads; instead, remove the await loadFocusItems(...) call
from renderActionItem and add a new parameter (e.g., focusItems: FocusItem[]) to
renderActionItem so it checks inFocus against the passed array; update the
callers renderProjectsSection and renderGeneralNextActionsSection to call
loadFocusItems(this.app.vault) once before iterating actions and pass that array
into each renderActionItem invocation; keep the same equality check
(focusItem.file === file && focusItem.text === action) and ensure callers await
the single loadFocusItems promise once.
- Around line 1-16: This file is missing the required two ABOUTME comment lines
at the top; add two comment lines at the very beginning of src/sphere-view.ts
that start with ABOUTME and briefly describe the file's purpose (e.g., "ABOUTME:
UI view for Sphere projects and summaries" and a second line with author/intent
or short description), placing them before the import block so tools/readers
pick them up; ensure they follow the project's comment style (//) and include a
concise description referencing that this file defines the Sphere view and
coordinates SphereDataLoader, SphereProjectSummary, and related view components
like ItemView/WorkspaceLeaf.
Comment on lines
+184
to
+192
| private registerWorkspaceEvents(): void { | ||
| const completedRef = (this.app.workspace as any).on( | ||
| "flow:action-completed", | ||
| (detail: { file: string; action: string }) => { | ||
| this.removeActionFromDom(detail.file, detail.action); | ||
| } | ||
| ); | ||
| this.workspaceEventRefs.push(completedRef); | ||
| } |
There was a problem hiding this comment.
Workspace event handlers accumulate on repeated onOpen calls.
registerWorkspaceEvents pushes new refs without clearing previous ones. Since onOpen is re-invoked by setState and setSphere, each call registers an additional flow:action-completed handler. While the duplicate handlers are functionally harmless (the DOM element is already removed by the first), this leaks EventRefs until onClose.
Compare with registerMetadataCacheListener (line 168–170) which correctly unregisters the old ref before re-registering.
Proposed fix
private registerWorkspaceEvents(): void {
+ for (const ref of this.workspaceEventRefs) {
+ this.app.workspace.offref(ref);
+ }
+ this.workspaceEventRefs = [];
+
const completedRef = (this.app.workspace as any).on(
"flow:action-completed",
(detail: { file: string; action: string }) => {
this.removeActionFromDom(detail.file, detail.action);
}
);
this.workspaceEventRefs.push(completedRef);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private registerWorkspaceEvents(): void { | |
| const completedRef = (this.app.workspace as any).on( | |
| "flow:action-completed", | |
| (detail: { file: string; action: string }) => { | |
| this.removeActionFromDom(detail.file, detail.action); | |
| } | |
| ); | |
| this.workspaceEventRefs.push(completedRef); | |
| } | |
| private registerWorkspaceEvents(): void { | |
| for (const ref of this.workspaceEventRefs) { | |
| this.app.workspace.offref(ref); | |
| } | |
| this.workspaceEventRefs = []; | |
| const completedRef = (this.app.workspace as any).on( | |
| "flow:action-completed", | |
| (detail: { file: string; action: string }) => { | |
| this.removeActionFromDom(detail.file, detail.action); | |
| } | |
| ); | |
| this.workspaceEventRefs.push(completedRef); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sphere-view.ts` around lines 184 - 192, registerWorkspaceEvents currently
appends a new completedRef to workspaceEventRefs every time it's called, leaking
EventRefs; before registering the new handler inside registerWorkspaceEvents,
unregister any existing refs by iterating this.workspaceEventRefs and calling
this.app.workspace.off(ref) (or the appropriate unregister method used
elsewhere), then clear this.workspaceEventRefs, then create the new completedRef
for the "flow:action-completed" listener and push it; this mirrors the cleanup
behavior used in registerMetadataCacheListener and prevents accumulating
handlers.
Comment on lines
+945
to
+963
| it("should clean up workspace event listeners on close", async () => { | ||
| const view = new SphereView(leaf, "work", settings, mockSaveSettings); | ||
| view.app = app; | ||
|
|
||
| await view.onOpen(); | ||
|
|
||
| // Verify workspace events were registered | ||
| const workspaceOnCalls = (app.workspace.on as jest.Mock).mock.calls; | ||
| const completedHandler = workspaceOnCalls.find( | ||
| (call: any[]) => call[0] === "flow:action-completed" | ||
| ); | ||
| expect(completedHandler).toBeTruthy(); | ||
|
|
||
| await view.onClose(); | ||
|
|
||
| // offref should have been called for workspace event refs | ||
| expect(app.workspace.offref).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Weak assertion — only checks offref is defined, not that it was called.
The test title says "should clean up workspace event listeners on close" but the assertion at line 961 only verifies the method exists (toBeDefined), not that it was actually invoked. This test would pass even if onClose didn't call offref at all.
Proposed fix
await view.onClose();
- // offref should have been called for workspace event refs
- expect(app.workspace.offref).toBeDefined();
+ // offref should have been called for each workspace event ref
+ expect(app.workspace.offref).toHaveBeenCalled();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("should clean up workspace event listeners on close", async () => { | |
| const view = new SphereView(leaf, "work", settings, mockSaveSettings); | |
| view.app = app; | |
| await view.onOpen(); | |
| // Verify workspace events were registered | |
| const workspaceOnCalls = (app.workspace.on as jest.Mock).mock.calls; | |
| const completedHandler = workspaceOnCalls.find( | |
| (call: any[]) => call[0] === "flow:action-completed" | |
| ); | |
| expect(completedHandler).toBeTruthy(); | |
| await view.onClose(); | |
| // offref should have been called for workspace event refs | |
| expect(app.workspace.offref).toBeDefined(); | |
| }); | |
| }); | |
| it("should clean up workspace event listeners on close", async () => { | |
| const view = new SphereView(leaf, "work", settings, mockSaveSettings); | |
| view.app = app; | |
| await view.onOpen(); | |
| // Verify workspace events were registered | |
| const workspaceOnCalls = (app.workspace.on as jest.Mock).mock.calls; | |
| const completedHandler = workspaceOnCalls.find( | |
| (call: any[]) => call[0] === "flow:action-completed" | |
| ); | |
| expect(completedHandler).toBeTruthy(); | |
| await view.onClose(); | |
| // offref should have been called for each workspace event ref | |
| expect(app.workspace.offref).toHaveBeenCalled(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sphere-view.test.ts` around lines 945 - 963, The test for SphereView's
cleanup is weak because it only asserts app.workspace.offref is defined instead
of verifying it was called; update the test after creating the view and calling
view.onOpen() and view.onClose() to assert that app.workspace.offref (the mock
function) was called (e.g., using toHaveBeenCalled or toHaveBeenCalledTimes) and
optionally verify it was called with the same event refs registered via
app.workspace.on — locate the test referencing SphereView, onOpen, onClose, and
app.workspace.offref and replace the toBeDefined assertion with a call assertion
against the mock.
FocusView now triggers a flow:action-waiting workspace event when converting an action to waiting-for. SphereView listens and re-renders the specific action's content with the handshake emoji, avoiding a full view refresh.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/sphere-view.ts`:
- Around line 184-200: registerWorkspaceEvents is re-registering handlers on
each onOpen (called via setState/setSphere) and simply pushes new EventRef
objects into workspaceEventRefs, leaking refs; before adding the new refs,
unregister any existing ones (like registerMetadataCacheListener does) by
iterating this.workspaceEventRefs and calling the workspace/unregister/off
method with each stored EventRef, then clear the array, and only then attach the
two listeners (the handlers removeActionFromDom and markActionWaitingInDom) and
push their new refs into workspaceEventRefs.
In `@tests/sphere-view.test.ts`:
- Around line 1005-1008: The test asserts that app.workspace.offref exists but
doesn't verify it was invoked; update the assertion to spy on or mock
app.workspace.offref before calling await view.onClose() (e.g.,
jest.spyOn(app.workspace, 'offref') or set app.workspace.offref = jest.fn()) and
then assert it was called (e.g., expect(app.workspace.offref).toHaveBeenCalled()
or toHaveBeenCalledTimes(1) and/or with the expected args) so the test for
view.onClose actually verifies cleanup occurred.
Unregister existing workspace event handlers before registering new ones in registerWorkspaceEvents, matching the pattern used by registerMetadataCacheListener. Also strengthen the cleanup test assertion from toBeDefined to toHaveBeenCalled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
data-focus-file,data-focus-action) so focus highlighting can be toggled on existing DOM elements without rebuilding the viewFixes #41
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests