Skip to content

fix: patch four correctness bugs found in state/plugin/API layer - #521

Closed
noahzuerii wants to merge 1 commit into
silvertakana:mainfrom
noahzuerii:claude/worldwideview-code-review-zvgkvx
Closed

noahzuerii wants to merge 1 commit into
silvertakana:mainfrom
noahzuerii:claude/worldwideview-code-review-zvgkvx

Conversation

@noahzuerii

@noahzuerii noahzuerii commented Sep 24, 2026 •

Copy link
Copy Markdown
  • PluginManager: fix a race where disabling a layer while enablePlugin() was still resolving its awaits could restart polling and re-emit layerToggled(true), leaking a background fetch loop and a stale websocket subscription for a layer the UI showed as disabled. Fixed with a per-plugin toggle epoch that lets an in-flight enablePlugin detect it was superseded by a later toggle.
  • data-query service / region API route: clamp limit with a lower bound of 1 (matching searchEntities' existing clamp), so a negative limit can no longer produce a negative Array.slice bound and return an incorrect entity set.
  • favoritesSlice: revert the optimistic add/remove on a failed POST/DELETE and surface an error toast, following the same optimistic-flip-with-rollback pattern already used in alertsSlice.
  • dataSlice.getAllEntities: filter by the layer's enabled flag to match its own docstring and PluginManager.getAllEntities' semantics.

Fix: chore/refactor+fix mix -> patch bump per project convention.

Type of Change

  • Bug fix
  • New feature
  • New plugin
  • Refactor / code quality
  • Documentation
  • Performance improvement

@codecov

codecov Bot commented Sep 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/core/plugins/PluginManager.ts 87.50% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@silvertakana

Copy link
Copy Markdown
Owner

Thanks for this - a careful read of the state and API layers, and two of these are bugs I am glad to have found.
Where it stands: the branch is ten commits behind main and conflicts, but only on package.json (main has since moved ioredis, size-limit and jsdom). The four code files have no semantic drift against current main, so the fixes apply as written - a rebase is all that is needed.
What I verified against current main:

  1. PluginManager enable/disable race - real, and the fix is right. src/core/plugins/PluginManager.ts:220 awaits the local engine manifest before setting enabled = true (L228) and starting polling (L245), while disablePlugin (L257) is synchronous. Toggling a layer off during a slow enable resurrects it: polling restarts and a stale layerToggled(true) is emitted for a layer the user disabled. The per-plugin epoch guard closes it, and because no await sits between the final check and the emit, no interleave survives.
  2. Negative limit clamp - real, and the fix is right. src/app/api/v1/entities/region/route.ts:65 passes a negative ?limit= straight through, and src/lib/data-query/service.ts:253 does the same for bounds.limit. A negative limit reaches allMatched.length > effectiveLimit at service.ts:291, where slice(0, -5) returns a size-dependent, arbitrary subset rather than an error. searchEntities already clamps at service.ts:198, so your precedent holds.
  3. dataSlice.getAllEntities ignoring the enabled flag - a real defect, but nothing calls it yet. The method contradicts its own docstring at dataSlice.ts:21-22 and its PluginManager twin at PluginManager.ts:388, which is worth fixing on its own terms. Being straight about the impact, though: git grep getAllEntities finds no caller under src/, and the two grids that actually render entities (searchEntities.tsx:23, GlobeView.tsx:97) read entitiesByPlugin directly and apply the enabled flag themselves. This one is latent drift rather than something a user hits today.
  4. Favourites optimistic rollback - real and user-visible, the strongest of the four. .catch() on a fetch only sees network failures; a 500 or a 401 resolves, so the optimistic favourite stays on screen while nothing was saved, and a failed delete looks like it worked. Checking res.ok, reverting the optimistic update, and surfacing a toast is the right shape, and the duplicate guard on the re-add is a nice touch.
    showErrorToast is already on the store (FETCH_HEAD:src/core/state/alertsSlice.ts:115: get().showErrorToast?.(result.error ?? "Failed to update alert rule");), so the toast call is real rather than a silent no-op.
    CI on this branch: FAILURE: 1 SKIPPED: 2 SUCCESS: 21 (I approved the workflow runs so they could execute on a fork pull request).
    So: I am happy to take all four. A rebase onto main to clear the package.json conflict is the only thing standing in the way.

…ased on main)

- PluginManager: fix a race where disabling a layer while enablePlugin()
  was still resolving its awaits could restart polling and re-emit
  layerToggled(true), leaking a background fetch loop and a stale
  websocket subscription for a layer the UI showed as disabled. Fixed
  with a per-plugin toggle epoch that lets an in-flight enablePlugin
  detect it was superseded by a later toggle.
- data-query service / region API route: clamp `limit` with a lower
  bound of 1 (matching searchEntities' existing clamp), so a negative
  limit can no longer produce a negative Array.slice bound and return
  an incorrect entity set.
- favoritesSlice: revert the optimistic add/remove on a failed
  POST/DELETE and surface an error toast, following the same
  optimistic-flip-with-rollback pattern already used in alertsSlice.
- dataSlice.getAllEntities: filter by the layer's enabled flag to match
  its own docstring and PluginManager.getAllEntities' semantics.

Fix: chore/refactor+fix mix -> patch bump per project convention.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FyQHTVUZs5PzRkYJa7Tq4h
@noahzuerii
noahzuerii force-pushed the claude/worldwideview-code-review-zvgkvx branch from f935148 to c6480b0 Compare September 26, 2026 11:41
@github-actions

Copy link
Copy Markdown
Contributor

Left for a person rather than auto-merged: not a Dependabot pull request. Patch and minor updates merge themselves once every check has completed green; this one is listed in the weekly dependency digest.

@noahzuerii noahzuerii closed this by deleting the head repository Sep 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants