Skip to content

Dockview UI redesign + default @internal filesystem provider#920

Merged
PythonFZ merged 121 commits intomainfrom
spec/dockview-ui-redesign
Apr 21, 2026
Merged

Dockview UI redesign + default @internal filesystem provider#920
PythonFZ merged 121 commits intomainfrom
spec/dockview-ui-redesign

Conversation

@PythonFZ
Copy link
Copy Markdown
Member

@PythonFZ PythonFZ commented Apr 17, 2026

Summary

Two bundled changes that landed on the same branch:

  1. Dockview UI redesign — replaces the legacy sidebar/window-manager + react-rnd with dockview-react: activity bars, side/bottom zones, panel registry, drag-to-bar with hover state, rooms panel rewrite, plot browser, filesystem panel, chat panel, etc. (55 commits)
  2. Default @internal filesystem provider — new symmetric @internal Provider path (mirroring @internal extensions). Ships a FilesystemRead provider registered on startup at @internal:filesystem:FilesystemRead, configurable via ZNDRAW_SERVER_FILEBROWSER_PATH (default ".", "none" disables). Adds register_internal_providers + ensure_internal_providers helpers, InternalProviderExecutor, taskiq dispatch fork in read_provider, visibility widening, and a useHasFilesystemProviders hook that hides the filesystem activity-bar icon when no providers are registered.

Plan: docs/superpowers/plans/2026-04-17-internal-providers.md.

Test plan

  • Full tests/zndraw_joblib suite (313 + 6 new tests) — green
  • tests/zndraw/test_providers_executor.py — green
  • tests/zndraw/test_config.py::TestFilebrowserPath — green
  • Frontend bun --bun tsc --noEmit — zero errors
  • uvx prek run --files <pr-files> — green on all PR-touched files
  • Manual UI check — default filesystem provider shows up in the activity bar, listing rooted at the server's cwd
  • Known: test_default_internal_filesystem_read + 7 pre-existing extension-e2e tests timeout locally due to a taskiq-receiver issue that does not reproduce on CI (confirmed on main locally; CI run 24327808136 green). Leaving the regression-investigation out of scope for this PR.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • VS Code–style activity bars (Left/Right/Bottom) with docked editor area, resizable sidebars and bottom zone
    • Docked plots: simplified Plots Browser, deterministic tab placement, popout/maximize controls, group actions
    • Rooms panel with search, create/duplicate/lock, and drag‑and‑drop upload; integrated Filesystem sidebar (auto-hidden when disabled)
    • Theme syncing between app and dock styling; improved chat panel and unread-badge behavior
  • Bug Fixes

    • Fixed bottom-panel shrink regression and improved plot/resize responsiveness
  • Tests

    • New E2E and unit tests covering dockview layout, providers, and internal provider dispatch and safety

PythonFZ and others added 30 commits April 16, 2026 18:10
Reframes the redesign around VS Code's two-tier model: tools (single-panel-per-bar
on Left/Right/Bottom activity bars) vs. views (splittable editor area). Adds a
Plots Browser tool panel and treats plots as views in the editor area. Drops VS
Code multi-panel-native integration as out-of-scope. Switches to feature-flagged
incremental rollout. Confirms zero backend changes are needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Revised spec based on brainstorming: drop feature-flag migration
in favor of big-bang replacement with final verification sweep;
add Rooms, Filesystem panels alongside Plots Browser; move chat
to right activity bar default (closed); make viewer closable with
F1 leave-room cascade (plots close, URL updates); replace ad-hoc
panel defs with a single PANELS registry as the frontend source
of truth; defer Pydantic panel control entirely (UI chrome, not a
visualization feature); add default-layout table covering every
panel and state.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
16-task plan for replacing fixed sidebar + react-rnd floating windows
with dockview-react editor area and three activity bars with
single-panel-per-bar zones.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Installs dockview-react@^5.2.0 in preparation for replacing the fixed
sidebar + react-rnd floating window layout with a dockview-managed
editor area and activity-bar-driven tool panels.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ?panel=filesystem effect called toggleActive unconditionally, which
flipped the panel on then off under React StrictMode's double-effect in
dev. Check the current active first and only toggle when the target
panel is not already active.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… react-rnd

Delete files fully replaced by dockview panels:
- SideBar, PrimaryDrawer, WindowManager, FigureWindow, AddPlotButton,
  ChatWindow, windowManagerStore, formStore, pages/filesystemBrowser.
- Drop react-rnd dependency (dockview provides native floating panels).
- Port StaticInfoBox off react-rnd to a plain absolutely-positioned Box.
- Prune now-unused openWindow plumbing from HandlerContext and
  useSocketManager.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e cascade

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Scopes the resolution of the 7 ISSUES.md items plus a bottom-drawer
shrink bug found during brainstorming into one coherent design.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second-pass spec review resolved a blocker where the §8 tab-hide rule
would have hidden the rightHeaderActionsComponent container, making the
popout/maximize buttons and the viewer close affordance unreachable on
single-panel groups. Dropped §8 entirely; updated goals, space reclaim
math, and E2E coverage (added theme sync, plotly resize, plots browser
simplification, and plot positioning assertions). Fixed registry.tsx
extension, tightened sliver drag scope phrasing, concretized the
dockview CSS variable list.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Third-pass spec review surfaced two ambiguities: §3/§4 did not make it
explicit that rightHeaderActionsComponent is rendered per dockview
group (so popout/maximize appear on every group, not just the root),
and §7 step 1 conflated "oldest panel" (temporal) with "sorts first
lexicographically" (alphabetical). Tightened both.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Maps the 8 issues in 2026-04-16-dockview-followup-design.md to 10
bite-sized TDD tasks. Each task contains a failing E2E assertion,
the implementation, and a commit step.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Part of the dockview follow-up: empty right bar is a prerequisite for
the sliver drop-zone change.
…draw

uv build --reinstall is not a valid flag combination (verified via
uv build --help). The hatch build hook handles frontend packaging
on any wheel build, so uv sync --reinstall-package zndraw rebuilds
frontend and reinstalls in one step.
Wrap DockviewReact in an absolutely-positioned box so its ResizeObserver
sees the flex-parent's computed size. Before this, bottom-drawer opens
did not shrink the viewer because DockviewReact rendered inline and
its height was tied to content, not the parent's current size.
Tab chrome now reads MUI palette tokens so the dockview surface visually
belongs to the app. Works for both light and dark themes because the
MUI tokens themselves flip on color scheme change.
Three interacting bugs made the earlier Task 3 commit (f65b6dd) a no-op:

1. createTheme did not emit --mui-palette-* variables. ThemeProvider
   alone only wires the React context. Added cssVariables with
   colorSchemeSelector='data' so dark mode is togglable via the
   [data-dark] attribute MUI generates.

2. DockviewReact fell back to its internal themeAbyss default because
   no `theme` prop was passed — our className="dockview-theme-light"
   was additive, not a replacement. Switched to theme={themeLight}
   so the abyss class is no longer applied.

3. dockview-mui.css was loaded before dockview-react's runtime CSS
   injection, so dockview's base theme rule won on the cascade.
   Doubled the class selector to out-specificity the base rule, and
   replaced two phantom variables (--dv-tab-background-color and
   --dv-tab-header-foreground-color do not exist in dockview's CSS)
   with the real per-state tab-color variants.

New E2E assertions lock in the behavior in both light and dark mode:
each dockview CSS variable must resolve to the same value as the
corresponding MUI palette token, and the MUI background-default must
be light (#fff) or dark (#121212) depending on which mode is active.

Root cause verified by reading dockview-core/theme.js (internal
themeAbyss default on line 2236 of dockviewComponent.js) and
inspecting the generated MUI stylesheet (only :root+[data-light]
rule was emitted before the cssVariables config).
Read MUI's resolved color mode via useColorScheme() and pass
themeLight or themeDark to DockviewReact. Combined with the MUI
palette overrides from Task 3, dockview chrome now fully tracks
the app's light/dark mode.
PythonFZ and others added 12 commits April 20, 2026 12:51
Eliminate env_parse_none_str='none' sentinel. Two explicit fields:
  filebrowser_enabled: bool = True
  filebrowser_path: str = '.'

Removes the model-wide footgun where any string field accepting
'none' (e.g., guest_password) was silently coerced to None.

Migrate all callsites: bootstrap.py and database.py None-checks
now test filebrowser_enabled directly. conftest.py default changes
from FILEBROWSER_PATH=none to FILEBROWSER_ENABLED=false; every test
that opts in to the filebrowser now passes FILEBROWSER_ENABLED=true
explicitly.

Delete the weak tautological test; rename remaining two filesystem
tests to match the new env-var names.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit d65d919 added TestFilebrowserConfig.test_default_filebrowser_path_is_cwd
and a module-level test_filebrowser_path_default_is_cwd asserting the same
invariant. Keep the class method.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dockview-layout.spec.ts and pr920-fixes.spec.ts referenced selectors
(data-sliver-state, text 'Drop to dock right') that do not exist in
the implementation. Frontend e2e is not in CI; the files were
misleading-docs masquerading as coverage.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vite.config.ts uses @vitejs/plugin-react-swc; the non-SWC variant was
installed alongside but never imported. Dropping removes ~7 transitive
Babel packages.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the 'let sharedApi' module singleton + setInterval polling with
a Zustand store. Consumers:
  - Components subscribe via useDockviewApi((s) => s.api) and re-render
    when the api binds — no more polling.
  - Non-component callers (socket handlers, effect callbacks) use
    .getState().api.

Also closes the ViewerView.onLeaveRoom stale-reference race: the
onDidRemovePanel subscription now lives in a useEffect([dockApi, api.id])
that re-runs whenever the store api binds, so the handler is always
current.

Additional consumers migrated beyond the spec: RoomsPanel, FilesystemPanel,
and landingPage (viewer re-add + reset-layout handler) which also held
getDockviewApi references.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three paths previously re-declared the viewer panel config (id,
component, title): DockviewLayout.onReady, roomId useEffect, and the
Reset-layout MenuItem. Extract two helpers colocated with addViewerPanel;
consumers import and call.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two consumers (useHasFilesystemProviders, FilesystemPanel) used the
same queryKey with different staleTime. Per-observer staleTime means
the Panel's default-0 defeated the Hook's 5_000 dedup whenever the
Panel was open.

Extract one hook owning both query + socket invalidation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extract ~90 LOC of duplicated drag-depth/setTimeout(0)/onDragEnter
logic into useDragHover(position). Shared keyframes shimmer in
dragStyles.ts (three copies removed).

Empty bars now render as a 4px sliver instead of returning null, so
dragging an icon off a bar still leaves a drop target to drag one
back.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Covers moveIconToBar (with/without index), dropIconOnPanel,
resetLayout, and toggleActive (open/close/switch). Also updates
test-setup.ts mock to provide real default panel icons for slice tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace useAppStore.getState() inside the effect with explicit
selectors in the dep array. Effect now re-runs correctly when the
active bar state changes out-of-band.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Safe autofix pass + hand fixes for noNonNullAssertion (FilesystemPanel
early-returns on null roomId) and useExhaustiveDependencies (ChatPanel).

- FilesystemPanel.tsx: replaced roomId!/selectedProvider! non-null assertions
  with early guard inside queryFn; fixed useTemplate string concatenation
- ChatPanel.tsx: wrapped scrollToBottom in useCallback; added biome-ignore
  for intentional tick-trigger deps (data) in both effects
- PlotsBrowserPanel.tsx: moved biome-ignore to useMemo declaration line
  (was orphaned on the closing brace); updated reason text
- RoomsPanel.tsx: useOptionalChain applied to r.description check
- PlotView.tsx: suppressed pre-existing noDoubleEquals (intentional loose
  equality for plotly string/number coercion) with targeted biome-ignore

PlotView.tsx 'any' types and noNonNullAssertion are pre-existing legacy
debt and explicitly out of scope per spec §3.9.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Member Author

@PythonFZ PythonFZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • style sometimes hard coded, sometimes using theme provider
  • inconsistent testing / tests are missing in CI
  • LoadFile no appears as modifier in the UI
  • lots of defensive imports on the python side!
  • lots of small util functions not necessarily needed! Evaluate each one of them if they should remain or be removed
  • getattr(app.state, "settings", None) patterns throughout the code, whilst the settings in this case always exist! Unecessary defensive programming!

Comment on lines +30 to +31
const api = useDockviewApi.getState().api;
if (api) closePlotTab(api, data.key);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the intended REACT way of using the useHook?

Comment on lines +1 to +2
import { act, renderHook } from "@testing-library/react";
import type { DragEvent } from "react";
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there seems to be some inconsistency with the test, some are in __test__ and some are directly along the other files

Comment on lines +13 to +17
const BAR_SX = {
left: { flexDirection: "column", width: 48, borderRight: 1 },
right: { flexDirection: "column", width: 48, borderLeft: 1 },
bottom: { flexDirection: "row", height: 40, borderTop: 1, width: "100%" },
} as const;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a style provider somewhere?

import { type BarPosition, PANELS, type PanelId } from "./registry";
import { useDragHover } from "./useDragHover";

const DRAG_MIME = "application/x-zndraw-panel-id";
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const DRAG_MIME = "application/x-zndraw-panel-id"; duplicated throughout the code

Comment on lines +3 to +6
export const shimmer = keyframes`
0%, 100% { background-color: rgba(25, 118, 210, 0.12); }
50% { background-color: rgba(25, 118, 210, 0.28); }
`;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we have a style provider somehwer?

Comment on lines +139 to +145
async def _execute(
request_id: str,
provider_id: str,
params_json: str,
token: str,
) -> None:
await ex(cls, params_json, provider_id, request_id, token)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functools partial?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushback: taskiq rejects functools.partial. Verified with a minimal repro — broker.register_task(partial(executor, prov_cls), ...) raises:

TypeError: functools.partial(...) is not a module, class, method, or function.

from typing.get_type_hints(func) inside register_task. taskiq needs a real function object for type-hint introspection. Keeping the factory-closure pattern with default-arg binding.

# Hide @internal:filesystem:* from non-superusers when the gate is on.
if settings.filebrowser_require_superuser and not _current_user.is_superuser:
gate_filter = ~(
(ProviderRecord.room_id == "@internal")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check that provider registration prohibits @internal prefix! TDD if not tested!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 403 gate in register_provider (router.py:1062) mirroring the existing DELETE-side policy (@internal providers cannot be deleted). Two new TDD tests in test_providers.py: test_register_internal_provider_forbidden_normal_user and test_register_internal_provider_forbidden_superuser — both RED before the fix, GREEN after.

Comment on lines +35 to +37
atoms = ase.Atoms("H2O", positions=[[0, 0, 0], [0, 0, 1], [1, 0, 0]])
xyz_path = tmp_path / "water.xyz"
ase.io.write(xyz_path, atoms)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use a fixture. check conftest, otherwise add simple water file!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a water_xyz fixture to tests/zndraw/worker/conftest.py that writes a 3-atom H2O xyz to tmp_path and returns the path. The e2e test now consumes the fixture directly — no more inline ase.Atoms(...) + ase.io.write(...).

Comment thread tests/zndraw/test_config.py Outdated
Comment on lines +259 to +266
def test_filebrowser_enabled_from_env(self) -> None:
"""Configurable via ZNDRAW_SERVER_FILEBROWSER_ENABLED env var."""
os.environ["ZNDRAW_SERVER_FILEBROWSER_ENABLED"] = "false"
try:
settings = Settings()
assert settings.filebrowser_enabled is False
finally:
os.environ.pop("ZNDRAW_SERVER_FILEBROWSER_ENABLED", None)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just testting that pydantic-settings work. We can trust the package and need no double testing here! Remove this and similiar non-tests! Keep tests for testing defaults!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped 12 *_from_env tests that just exercised pydantic-settings' own env-var plumbing (storage, media_path, host/port, simgen, worker, internal_url, filebrowser_enabled/path, task_queue_name, plus a generic "picks up env changes" smoke). Kept: all test_default_*, the env_parse_none_str-disabled tests (ZnDraw-specific behavior), customise_sources/pyproject.toml loader tests (ZnDraw-specific), and the /v1/config/global-settings endpoint tests. 32 → 20 tests, all green.

Comment thread tests/zndraw_joblib/test_providers.py Outdated
Comment on lines +147 to +148
from zndraw_auth import User
from zndraw_joblib.models import ProviderRecord
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defensive imports ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoisted from zndraw_auth import User and from zndraw_joblib.models import ProviderRecord to the top of test_providers.py. Removed 10 duplicate function-local imports.

PythonFZ and others added 10 commits April 20, 2026 14:52
- docs: re-declare/re-declared -> redeclare/redeclared (codespell)
- tests: split compound isinstance+len assert (PT018)
- tests: extract error_body literal to keep line <=88 (E501)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@internal is a server-bootstrap namespace seeded at startup by
register_internal_providers. Previously, any authenticated user
could PUT /v1/joblib/rooms/@internal/providers and inject a row
into the reserved namespace (201 Created).

Gate in register_provider mirrors the existing DELETE-side policy
(@internal providers cannot be deleted). 403 Forbidden for all
callers, superuser or not.

TDD: new tests RED before the fix, GREEN after.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…agic

Three dependency sites used getattr(app.state, ..., None) as a defensive
fallback: get_internal_provider_registry (deps:32),
mint_internal_worker_token (deps:170, :175 for settings).

Per reviewer: app.state.* attrs consumed by request-time deps must be
assigned unconditionally in lifespan (to a value or to None). No
getattr fallbacks, no half-populated-state shims.

- database.py lifespan pre-inits internal_worker_user,
  internal_provider_registry, internal_registry to None up front so
  request-time access never AttributeErrors.
- dependencies.py switches to direct app.state.X access.
- conftest fixtures (_build_app, unguarded_client_factory) set
  internal_provider_registry + settings to satisfy the new contract.
- test_mint_internal_worker_token_raises_runtime_error_on_missing_cache
  (specifically guarded the getattr fallback) is obsolete — the
  substantive user-is-None case is covered by
  test_read_remote_provider_works_with_no_internal_worker_cache.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four lazy-import sites were flagged as defensive without a cycle or
performance justification:

- bootstrap.py:51,79 — from zndraw.database import _collect_providers
  Traced the import graph: zndraw.database only imports bootstrap lazily
  (lifespan, not module-top), so bootstrap can safely top-import
  zndraw.database. Same for zndraw_joblib.registry.
- executor.py:80 — from zndraw_joblib.exceptions import
  ProviderExecutionFailed
  Called inside an except block; zndraw_joblib.exceptions has no back-
  import on the providers package.
- executor.py:134-135 — import fsspec / DirFileSystem
  fsspec is the core dep of the filesystem provider; ~87 ms at startup
  is the correct place to pay it vs first-call-time.
- test_providers.py:148 (and 9 duplicate sites) — User, ProviderRecord
  Consolidated to module-top imports.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
InternalProviderExecutor._resolve_handler was a one-line method called
exactly once (from _run). Inlined the call to resolve_internal_provider_handler
and dropped the wrapper method.

Reviewer asked whether functools.partial fits here. It does not — the
simpler fix is to remove the needless indirection entirely.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer flagged executor.py:91 — the error-upload headers duplicated
"application/problem+json" which is a property of the error class, not
custom code at each call site.

- Add ProblemDetail.MEDIA_TYPE ClassVar to both zndraw and zndraw_joblib
  copies of the ProblemDetail model.
- Replace four literal "application/problem+json" sites
  (problem_exception_handler x2, router.py Response x2, executor.py upload
  headers) with ProblemDetail.MEDIA_TYPE.

Auth/X-Request-Hash/X-Result-Status headers remain at the call site —
those are upload-context, not error-context.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four separate review comments consolidated:

- database.py: drop _collect_providers() wrapper; call sites inline
  list(BUNDLED_PROVIDERS) directly. Hoist BUNDLED_PROVIDERS import to
  module top (no cycle — zndraw.providers does not import database).
- result_backends.py: rename the cryptic _k() helper to
  _namespaced_key() across both RedisResultBackend and
  StorageResultBackend (and the one cross-backend access in
  CompositeResultBackend).
- config.py: drop the two Pydantic "comment-docstring under the field"
  annotations on filebrowser_enabled / filebrowser_path; move the
  explanatory text to the Settings class docstring to match the rest
  of the file (which uses inline # comments).
- executor.py: remove _extension_accepts_providers_kwarg and its
  runtime inspect.signature check. All subclass .run() overrides in
  selections.py (9 sites) and analysis.py (4 sites) now accept
  **_kwargs: t.Any — matching the base Extension.run signature. Caller
  can always pass providers= without a TypeError risk.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s packages

Reviewer flagged: the zndraw.exceptions and zndraw_joblib.exceptions
packages both defined bit-identical ProblemDetail and ProblemError
classes (same fields, same MEDIA_TYPE, same init signature).

Fold the zndraw copies into re-exports from zndraw_joblib.exceptions.
zndraw-specific extensions stay (ProblemType subclass with openapi_response
and raise_for_client, plus problem_exception_handler auto-filling
problem.instance from request.url.path — a real behaviour difference).

Verified runtime identity: zndraw.exceptions.ProblemDetail is
zndraw_joblib.exceptions.ProblemDetail -> True. Full test suite
(1433 passed, 1 skipped) unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two separate review comments:

tests/zndraw/test_config.py:266
  Reviewer: "just testing that pydantic-settings work. We can trust the
  package and need no double testing here! Remove this and similar
  non-tests!"

  Dropped 12 "*_from_env" tests that only exercised pydantic-settings'
  own env-var/type-coercion plumbing (storage, media_path, host, port,
  simgen, worker, internal_url, filebrowser_*, task_queue_name, plus a
  generic "picks up env changes" smoke). Kept:
    * all test_default_*
    * tests that exercise ZnDraw-specific behaviour (env_parse_none_str
      disabled — "none" as literal string, customise_sources ordering,
      pyproject.toml loader, /v1/config/global-settings endpoint).
  32 → 20 tests, all still green.

tests/zndraw/worker/test_internal_loadfile_e2e.py:37
  Reviewer: "why not use a fixture. check conftest, otherwise add
  simple water file!"

  Added `water_xyz` pytest fixture to tests/zndraw/worker/conftest.py
  — writes a 3-atom H2O xyz to tmp_path and returns the path. Test
  drops the inline ase.Atoms + ase.io.write call and uses the fixture.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously ``ensure_internal_providers`` only ran inside
``init_database`` (gated by ``settings.init_db_on_startup``). Existing
production deployments — where the DB was bootstrapped before the
@internal providers feature landed and ``init_db_on_startup=False`` so
``init_database`` never re-runs — started against DBs without any
``@internal:*`` ProviderRecord rows. Broker tasks registered fine
in-process but the rows did not exist, so the frontend could not
discover providers and lookup routes returned 404.

Lift the seed step out into the lifespan after caching
``internal_worker_user``. Guarded by ``filebrowser_enabled`` (feature
flag) and the cached user being present (init_database seeds both).
Idempotent — the function already catches ``IntegrityError`` on the
``unique_provider`` constraint, safe under concurrent replica startup.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@PythonFZ
Copy link
Copy Markdown
Member Author

Triage of the latest review batch:

Critical — ProblemDetail dedup: Already done in 7730352d. src/zndraw/exceptions.py:674-681 is now a comment block; ProblemDetail / ProblemError are re-exported from zndraw_joblib.exceptions. Runtime identity verified: zndraw.exceptions.ProblemDetail is zndraw_joblib.exceptions.ProblemDetailTrue. One canonical class, no divergence risk.

Important 1 — register_filebrowser_providers gated by init_db_on_startup: Looks like this was read off an older revision. In current database.py:360-368 the call sits outside the if settings.init_db_on_startup: branch — it runs unconditionally at lifespan startup, gated only by settings.filebrowser_enabled inside the function. Docker/production paths do register the broker tasks. Same symmetric call in broker.py:42 for external workers.

Important 2 — deleted flag for providers: ProviderRecord (models.py:106-127) has no deleted column, unlike Job (models.py:59). The asymmetry is intentional — @internal providers are server-bootstrap-only and the DELETE endpoint (router.py:1337) rejects deletion outright. There is no soft-delete state to reactivate, so the ensure-reactivation pattern does not apply.

Important 3 — Migration for existing DBs: Real issue, fixed in 0ec6b256. ensure_internal_providers now runs at every server startup (after caching internal_worker_user), not only inside init_database. Idempotent (IntegrityError-handled under concurrent replica startup), gated by filebrowser_enabled and the cached user being present. Existing deployments with init_db_on_startup=False will backfill @internal provider rows on restart, no zndraw-db CLI run required.

Minors — acknowledged, not blocking this PR:

  • filebrowser_require_superuser=True default — doc/release-note item; deployment operators who want non-superuser access set ZNDRAW_JOBLIB_FILEBROWSER_REQUIRE_SUPERUSER=false.
  • useFilesystemProviders 5s staleTime — frontend caching tune, separate follow-up.
  • RoomsPanel drag-drop partial-failure rollback — UX follow-up.
  • Hidden-filesystem-icon tooltip — UX follow-up.

@PythonFZ PythonFZ merged commit acfdea9 into main Apr 21, 2026
6 checks passed
@PythonFZ PythonFZ deleted the spec/dockview-ui-redesign branch April 21, 2026 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants