Skip to content

feat(paths): centralize data/template/plugin dirs under one Documents app root - #29

Merged
Xander-git merged 3 commits into
mainfrom
refactor/centralize-data-dir
Jun 3, 2026
Merged

feat(paths): centralize data/template/plugin dirs under one Documents app root#29
Xander-git merged 3 commits into
mainfrom
refactor/centralize-data-dir

Conversation

@Xander-git

Copy link
Copy Markdown
Collaborator

Summary

Collapses the three independently-configured roots (paths.templates_dir / paths.plugin_dir / paths.local_root) into a single paths.app_root that defaults under the OS Documents folder (~/Documents/ExLabWizard). templates/, plugins/, and the experiment data/ root are now derived read-only properties, and each equipment's data lives at <app_root>/data/<EQUIPMENT_ID>/….

This also removes the per-equipment EquipmentConfig.local_root, which fixes a latent data-loss-class bug: run creation used the global root while the auto-sync quiescence poller (and the validator's audit roots) used the per-equipment one — if they diverged, runs were written to one tree and watched in another. All three now derive config.paths.local_root (<app_root>/data/<id>).

Design: docs/superpowers/specs/2026-06-01-centralize-data-dir-design.md.

Key changes

  • paths.py: os_documents_path() (macOS ~/Documents, Windows Known-Folder via SHGetKnownFolderPath with %USERPROFILE%/home fallbacks, Linux $XDG_DOCUMENTS_DIR), default_app_root(), ensure_app_dirs(), app_root_writable(); DISPLAY_NAME constant + test-mode (-test) suffix.
  • PathsConfig: single stored app_root; derived data_root/local_root/templates_dir/plugin_dir (only app_root serializes).
  • Setup gate: INCOMPLETE_MISSING_PATHSINCOMPLETE_PATHS_UNWRITABLE, driven by an injected paths_writable flag (callers compute app_root_writable).
  • UI: Settings shows one "Data folder" input + derived read-only labels; the Add-Equipment wizard drops its local-root step; ensure_app_dirs runs on save and at tray bring-up.
  • Clean break (pre-release): no migration shims; legacy configs carrying the retired keys fail validation and route to the setup wizard.

Test Plan

  • ruff check . + ruff format --check . — clean
  • mypy src/exlab_wizard — 0 issues (167 files)
  • pytest tests/unit tests/integration --cov-fail-under=912607 passed, 22 skipped, coverage 91.17%
  • New regression tests/unit/test_data_root_agreement.py pins creation/poller/validator data-dir agreement
  • pytest tests/e2e — flows/page-objects updated for the single-input UI and are lint-clean, but not executed in this environment (needs Playwright + chromium + a live server); please run before merge.

Notes / follow-ups

  • Pre-existing, out-of-scope bug spotted during review: Validator.from_config gates staging_root on getattr(orch, "enabled", …), but OrchestratorConfig has no enabled field — so that branch is always dead. Unrelated to this refactor (the production _build_validator path is correct); flagged for a separate change.

🤖 Generated with Claude Code

Xander-git and others added 2 commits June 1, 2026 15:19
…app root

Single configurable app_root (default ~/Documents/ExLabWizard) with
templates/plugins/data derived; removes per-equipment local_root and the
creation-vs-poller divergence; repurposes the paths setup gate to a
writability check. Clean break, no migration.

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

Collapse PathsConfig's three roots (templates_dir/plugin_dir/local_root) into a
single configurable `app_root` (default ~/Documents/ExLabWizard) with derived
read-only properties; remove the per-equipment EquipmentConfig.local_root.

- paths.py: os_documents_path / default_app_root / ensure_app_dirs /
  app_root_writable (Windows Known Folder + USERPROFILE/home fallbacks);
  add DISPLAY_NAME constant + _display_name() test-mode suffix.
- Unify equipment data-dir derivation across run creation, the quiescence
  poller, and the validator audit roots on config.paths.local_root
  (== <app_root>/data/<id>), eliminating two divergence sources.
- Repurpose the setup gate: INCOMPLETE_MISSING_PATHS -> INCOMPLETE_PATHS_UNWRITABLE,
  driven by an injected paths_writable flag (callers compute app_root_writable).
- Settings: single "Data folder" input + derived read-only labels; the
  equipment wizard drops its local-root step; ensure_app_dirs wired at save
  and tray bring-up.
- Clean break (pre-release): no migration shims; legacy configs that still
  carry the retired keys fail validation and route to the setup wizard.

Tests: full unit+integration green (2607 passed); new regression pins
creation/poller/validator data-dir agreement; e2e flows + page objects updated
for the single-input UI. ruff + mypy clean; coverage 91.17%.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Xander-git

Copy link
Copy Markdown
Collaborator Author

Code review

Found 1 issue:

  1. Saving over a legacy config preserves the retired paths.* keys, so the setup wizard cannot repair the config. PathsConfig now serializes only app_root, but save_config still deep-merges model_dump() into original_text; omitted keys in the existing paths: mapping (templates_dir, plugin_dir, local_root) stay on disk. Since this PR also makes those keys extra="forbid", a user with an old config is routed to setup, saves the new data folder, and then reloads the same invalid YAML again.

app_root: str = Field(default_factory=lambda: str(default_app_root()))
@property
def data_root(self) -> str:
"""The experiment data root, ``<app_root>/data``."""
return str(Path(self.app_root) / "data")
@property
def local_root(self) -> str:
"""Alias of :attr:`data_root` (kept for existing consumers)."""
return self.data_root
@property
def templates_dir(self) -> str:
"""The global Copier template library, ``<app_root>/templates``."""
return str(Path(self.app_root) / "templates")
@property
def plugin_dir(self) -> str:
"""The lab plugin directory, ``<app_root>/plugins``."""
return str(Path(self.app_root) / "plugins")

new_dict = config.model_dump(mode="python", exclude_none=False)
if original_text is not None:
# Round-trip merge: load original, mutate values in place, dump.
original = yaml.load(original_text) or {}
_deep_merge(original, new_dict)
out = original
else:
out = new_dict
text_buf = io.StringIO()
yaml.dump(out, text_buf)
encoded = text_buf.getvalue().encode("utf-8")
path.parent.mkdir(parents=True, exist_ok=True)
atomic_write_bytes(path, encoded)
_log.info("saved config.yaml [path=%s] [keys=%d]", str(path), len(out))
def _deep_merge(target: Any, source: dict[str, Any]) -> None:
"""In-place deep-merge source into target.
Used by save_config to overlay new values onto a ruamel-loaded
document so comments/key order survive. Lists are replaced wholesale
(the Settings UI hands us the full list to write).
"""
for key, value in source.items():
if isinstance(value, dict) and isinstance(target.get(key), dict):
_deep_merge(target[key], value)
else:
target[key] = value

🤖 Generated with Codex

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@Xander-git

Copy link
Copy Markdown
Collaborator Author

Code review update

Per maintainer guidance, legacy-schema compatibility is intentionally out of scope here; disregard my earlier legacy-config comment. I found 4 current-shape issues:

  1. paths.app_root accepts an empty string. Settings binds the Data folder input directly to app_root, and the model revalidation still accepts ""; then the derived roots become relative paths like data, templates, and plugins. The runtime writability check calls ensure_app_dirs() before returning false, so API/setup-status paths can create those relative directories under the process working directory instead of rejecting the config at validation time.

model_config = ConfigDict(extra="forbid", str_strip_whitespace=True)
app_root: str = Field(default_factory=lambda: str(default_app_root()))
@property
def data_root(self) -> str:
"""The experiment data root, ``<app_root>/data``."""
return str(Path(self.app_root) / "data")

ui.input(label="Data folder", value=draft.paths.app_root).props(
'data-testid="settings-paths-app-root"'
).bind_value(draft.paths, "app_root")

paths = config.paths
for directory in (paths.app_root, paths.data_root, paths.templates_dir, paths.plugin_dir):
ensure_dir(Path(directory))
def app_root_writable(config: Config) -> bool:
"""Return True when the app root and ``data/`` are creatable and writable.
Drives the §4.9.1 paths gate (see :func:`evaluate_setup_state`): the app
root is always populated (it defaults under Documents), so the gate no
longer asks "is it blank" but "can we actually create and write runs
here". Attempts :func:`ensure_app_dirs`, then probes ``os.access(W_OK)``
on the app root and the data root. Returns False on any ``OSError`` (bad
drive, permission denied) rather than raising.
"""
try:
ensure_app_dirs(config)
except OSError:
return False
return os.access(config.paths.app_root, os.W_OK) and os.access(config.paths.data_root, os.W_OK)

  1. The Settings section picker never returns paths for an existing config, even when the setup state is INCOMPLETE_PATHS_UNWRITABLE. If the app root is unusable and another setup section is also incomplete, Settings auto-selects that other section and does not mark the Data folder section, despite next_action being set_paths.

# The single ``paths.app_root`` always carries a sensible Documents-based
# default, so there is no "unset paths" section to auto-select for a
# config that exists. The rare unwritable-app-root case is surfaced by the
# setup banner (``compute_setup_state`` -> ``INCOMPLETE_PATHS_UNWRITABLE``),
# not by this Settings section picker.

  1. The updated full-lifecycle e2e still asserts the derived data_root string is serialized into config.yaml, but the new contract serializes only paths.app_root. With app_root = tmp_path, this assertion will fail when the e2e suite runs.

assert config_path.exists(), "Save must persist config.yaml"
config_text = config_path.read_text(encoding="utf-8")
assert "MICROSCOPE1" in config_text
assert "SPECTROMETER1" in config_text
assert str(data_root) in config_text

  1. Generated UX docs are stale after the ux_catalog.py test-id changes. tests/e2e/test_ux_documentation.py regenerates docs/UX_INTERACTIONS.md and fails if it differs, and the committed doc still lists removed test IDs like settings-paths-local-root and settings-equipment-local-root.

| Route | Test ID | Element | Action | Outcome |
|---|---|---|---|---|
| `/settings` | `settings-nav-paths` | nav row | Click the 'Paths' sidebar row | Shows the Paths section (client-side; edits are preserved). |
| `/settings` | `settings-paths-templates` | input | Type the templates directory | Binds config.paths.templates_dir on the draft. |
| `/settings` | `settings-paths-plugin` | input | Type the plugin directory | Binds config.paths.plugin_dir on the draft. |
| `/settings` | `settings-paths-local-root` | input | Type the local data root | Binds config.paths.local_root on the draft. |
| `/settings` | `settings-nav-lims` | nav row | Click the 'LIMS' sidebar row | Shows the LIMS section. |
| `/settings` | `settings-lims-endpoint` | input | Type the LIMS endpoint URL | Binds config.lims.endpoint on the draft. |
| `/settings` | `settings-lims-email` | input | Type the operator email | Binds config.lims.email on the draft. |
| `/settings` | `settings-save` | button | Click 'Save all' | Persists config.yaml, applies it to the running components in-process, and shows a 'Settings saved' toast (no relaunch). |
| `/settings` | `settings-discard` | button | Click 'Discard all' | Drops the in-memory draft edits. |
## Equipment
| Route | Test ID | Element | Action | Outcome |
|---|---|---|---|---|
| `/settings` | `settings-nav-equipment` | nav row | Click the 'Equipment List' sidebar row | Shows the equipment list and the add-equipment sub-form. |
| `/settings` | `settings-equipment-id` | input | Type the equipment ID (^[A-Z][A-Z0-9_]*$) | Provides the EquipmentConfig.id for the new entry. |
| `/settings` | `settings-equipment-label` | input | Type the equipment label | Provides the EquipmentConfig.label for the new entry. |
| `/settings` | `settings-equipment-local-root` | input | Type the equipment local root | Provides the EquipmentConfig.local_root for the new entry. |
| `/settings` | `settings-equipment-nas-root` | input | Type the equipment NAS root | Provides the EquipmentConfig.nas_root for the new entry. |

def test_ux_interactions_doc_is_current() -> None:
"""``docs/UX_INTERACTIONS.md`` is regenerated from the catalog.
Regenerates the doc and fails if the committed copy was stale -- so
a catalog change without a doc refresh is caught in CI. The fresh
content is written either way, so re-running the suite makes the
test pass.
"""
expected = _render_doc(UX_INTERACTIONS)
current = _DOC_PATH.read_text(encoding="utf-8") if _DOC_PATH.exists() else None
if current != expected:
_DOC_PATH.parent.mkdir(parents=True, exist_ok=True)
_DOC_PATH.write_text(expected, encoding="utf-8")
msg = (
f"{_DOC_PATH.relative_to(_REPO_ROOT)} was stale and has been "
"regenerated from tests/e2e/ux_catalog.py -- commit the update."
)
raise AssertionError(msg)

- test_flow_00_full_lifecycle asserted the derived data root appears in the
  saved config.yaml, but the config now persists only `app_root` (data/ is
  derived) -- assert `app_root` instead. Run-location assertions keep using the
  derived `data_root`.
- Regenerate docs/UX_INTERACTIONS.md from the updated ux_catalog.py (single
  "Data folder" app-root input; per-dir + per-equipment local-root rows gone).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Xander-git
Xander-git merged commit 63011ef into main Jun 3, 2026
8 checks passed
@Xander-git
Xander-git deleted the refactor/centralize-data-dir branch June 3, 2026 22:34
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.

1 participant