Commit fb3a879e authored by vertighel's avatar vertighel
Browse files

Merge branch 'refactor/v0.9-cleanup'

# Conflicts:
#	noctua/devices/atik.py
parents c4e04ca8 f01ed980
Loading
Loading
Loading
Loading
Loading
+148 −0
Original line number Diff line number Diff line
# HTML conventions

Target style for `noctua/web/pages/**/*.html`. Written during the v0.9
cleanup (2026-07), grounded in an audit of every template/macro file.

## Layer boundary — the rule that protects everything else

Jinja markup and JS talk to each other through element ids, `data-*`
attributes, and (in one case) tag names. Any change to *how* an
element is found by JS (its id, its tag, a `data-*` name) must land in
the same commit as the JS change that depends on it. Pure visual/class
changes that don't touch those hooks are safe to do independently.

## Semantic tags already established — keep using them

- `<aside>` for a telemetry block.
- `<var data-status="...">` inside a `<small>` for a telemetry value —
  **being replaced by `<data value="...">`, see below.**
- `<figure>`/`<figcaption>` for Webcam and Viewer panels.
- `<section>`/`<article>` for page structure.

## `<var>` → `<data>` — approved, NOT purely mechanical

Confirmed correct semantic upgrade (`<data value="...">` is built for
"content with a machine-readable value"; `<var>` is technically for "a
variable in a mathematical/programming expression", a stretch for
telemetry). Audit found 18 live `<var data-status=...>` occurrences,
all simple `<var>text</var>` with no nested markup — the *markup*
rename is mechanical. **The JS side is not**:
`noctua/web/static/js/status-view.js` selects/creates by tag name in
three places (not just by attribute):
- `status-view.js:34``document.createElement('var')`
- `status-view.js:159`, `:209``el.tagName.toLowerCase() === 'var'`

`status-stream.js`, `status-view.js:112`, `synoptic.js:206` select
purely by `[data-status]`/`[data-status^=...]` — those are already
safe. `macros/widgets.html:230` defines a bare `<var>` with no
`data-status` at all (populated dynamically by JS) — the rename must
cover that template too.

**Do this as one coordinated commit**: rename the tag in every macro
*and* update the three `status-view.js` tag-name checks to `'data'` in
the same commit — not two separate "HTML phase" / "JS phase" changes,
this is exactly the cross-layer-contract case the boundary rule above
exists for.

## Semantic tags considered and rejected (2026-07-17)

Proposed, user said no — **do not revisit** unless asked again:
`<fieldset>`+`<legend>` pairing, `<details>`/`<summary>` (both for the
error-details widget and for the raw/badge toggle), `<time>` for
timestamps, `<meter>` for gain/cooler level, `<progress>` for exposure
progress.

## Bootstrap classes — Sass, tag selectors, not Jinja helper macros

**Decision**: compile a project Sass bundle from Bootstrap 5.3.8
source (`dart-sass`, standalone, no Node/npm needed) replacing
`css/vendor/bootstrap.min.css`, using `@extend` on **tag selectors**
where — and only where — the audit confirms the class combo is truly
universal for that tag. Chosen over Jinja helper macros because the
goal is a clean *rendered* DOM (one class or none, inspectable in
DevTools), not just a clean template source.

```scss
@import "bootstrap/scss/bootstrap";

input[type="checkbox"] { @extend .form-check-input; }        // 11/11, fully universal
select                  { @extend .form-select; @extend .form-select-sm; @extend .bg-black; }  // 10/11
input[type="number"],
input[type="text"]     { @extend .form-control; @extend .bg-black; } // majority, NOT universal — see outliers below
```

### What the audit actually found (2026-07-17) — scope precisely, don't assume uniformity

- **`input[type="checkbox"]` (11/11)**: fully universal, always
  `form-check-input` (plus `role="switch"` or a hook class on some) —
  safe as a bare tag+attribute selector, no outliers.
- **`<select>` (10/11)**: `form-select form-select-sm bg-black` is
  dominant. One outlier: `macros/sequencer_elements.html:154` adds
  `text-light border-secondary`. Extend the dominant combo via tag
  selector; leave that one file's extra classes explicit in HTML.
- **`input[type="number"]` (16/18)**: `form-control bg-black` (plus
  optional trailing utility/hook classes) is dominant. Two real
  outliers with a *different base*: `macros/viewer_panel.html:64` (no
  `bg-black`), `macros/sequencer_elements.html:194` (`bg-dark
  text-light border-secondary` instead of `bg-black`). A third
  recurring shape, `col-md form-control bg-black` (2 occurrences,
  `webcam_panel.html:42`, `widgets.html:43`), is a layout variant, not
  a style outlier — the `col-md` part stays explicit (responsive grid
  class, never fold into a tag selector), the rest still extends.
- **`input[type="text"]` (4 total)**: **no dominant combo** — all four
  differ (`control_panel.html:417`, `sequencer_elements.html:116`,
  `sequencer_elements.html:179`, plus one in a dead backup file). Do
  **not** write a tag selector for this one until/unless a real pattern
  emerges — leave classes explicit.
- **`<fieldset>` (18 live)**: `row mt-1 align-items-center` is dominant
  (12/18) but genuinely fragmented — 5 distinct combos total (`row
  mt-2` alone ×3, `row mt-1 widget-universal align-items-center` ×3,
  `widget-universal` alone ×1, one fully unique one). Extend only the
  dominant combo via tag selector; the other 6 elements keep explicit
  classes.
- **`<small>` near telemetry `<var>`/`<data>` (16)**: all share
  `text-success`, split only on whether `mt-1` is also present (6 vs
  9) — safe to extend `text-success` alone via tag selector, leave
  `mt-1`/`col-4` explicit (layout, not decoration).
- **`<button>` (54) — confirmed NOT universal, do not attempt a tag
  selector.** Real semantic split by role: primary action
  (`btn btn-primary`), success/confirm (`btn btn-success`),
  danger/stop (6+ distinct variants), outline/secondary (4+ shapes),
  nav-tabs (`nav-link`, unrelated `.btn` family entirely). If this
  needs consolidating, it's per-role Sass placeholders (`%btn-danger-x`
  style), not one blanket rule — a separate, smaller decision from the
  input/select/fieldset case.

### Mechanics

Sorgente Bootstrap 5.3.8 → `custom.scss` importing it + the tag rules
above → compiled once (or whenever `.scss` changes) with standalone
`dart-sass` into a single `.css` file replacing
`css/vendor/bootstrap.min.css` in place. Zero new runtime/client-side
tooling — same static-file deployment shape as today.

**Blast-radius warning**: a class-scoped mistake breaks only elements
using that class; a tag-selector mistake breaks *every* element of
that type across the whole app instantly. Treat this as its own
sub-phase of the HTML pass, with a full visual walk-through (every
page, every panel state) after each new tag rule, not bundled with
unrelated HTML changes.

## Repo cruft found by the audit — clean up regardless of the rest

- Stray editor backup/swap files, untracked but present, duplicating
  live content with minor drift: `sequencer.html~`, `webcam.html~`,
  `synoptic.html~`, `subsystem.html~`, `status-stream.js~`,
  `macros/#widgets.html#`. Delete.
- `macros/widgets.html:234``widget_telemetry_card` macro, zero
  callers anywhere. Dead, remove.
- `macros/control_panel.html:328` — a commented-out `<select>`, dead
  markup.
- `macros/control_panel.html:467` — placeholder value `asd` left over
  from debugging (should be `—`/`N/A` like everywhere else).

## Raw HTML injection — none found, keep it that way

No `| safe` filter, no `Markup(...)` call anywhere in `pages/` or
`pages/macros/` today (confirmed by audit). Don't introduce one without
a specific, discussed reason.
+177 −0
Original line number Diff line number Diff line
# JavaScript conventions

Target style for `noctua/web/static/js/**/*.js`. Written during the
v0.9 cleanup (2026-07), grounded in a full-file audit.

## Paradigm — ES modules, not Web Components (with one confirmed exception)

Standard: small, focused ES modules exporting named functions — the
`wireXxx(el, getters..., onChange)` shape already established in
`loop-toggle.js`/`control.js` is the model, generalize it, don't
replace it. This app is server-rendered (Jinja) with progressive
enhancement, not a SPA — Custom Elements would add lifecycle/shadow-DOM
complexity with no real benefit for that shape.

**`components/NoctuaWidget.js` is a legitimate, narrow exception, not
an inconsistency to fix**: confirmed a real registered Custom Element
(`customElements.define('noctua-widget', ...)`), but it solves a
genuinely different problem from `widget_standard()` (the Jinja
macro): auto-generating a control widget for *any* arbitrary API route
from the live `/api/` schema, used only by the API-catalog exploration
page (`snippet_viewer.html`) — not the hand-built control pages.
`widget_standard()` stays a Jinja macro (fixed markup, known at
render-time); `NoctuaWidget.js` stays a Custom Element (markup only
knowable at runtime from the schema). Keep both, don't merge — just
make sure this distinction is documented so nobody "fixes" it into one
or the other by mistake. It already reuses the same
`.btn-universal`/`data-url`/`data-inputs` conventions as everything
else, so its output is wired by the same `actions.js` handler.

**Known non-module scripts to convert**: `actions.js` and `webcam.js`
are the only two plain `<script src>` page scripts (excluding vendor
bundles). Both already use modern syntax and both only stay
non-modules because they read the global `window.showToast` instead of
`import { showToast } from './ui.js'` — converting to `type="module"`
removes the `typeof showToast === 'function'` guards
(`actions.js:77,88,140`) and the load-order dependency on `ui.js`'s
window shim.

## No injected HTML except genuinely dynamic content

Audited every `createElement`/`innerHTML =` in the codebase. **Keep
dynamic** (confirmed genuinely unbounded/data-driven, not a fixed set
of states): `ws-client.js:24,27` (arbitrary ANSI log lines),
`status-view.js:34,100,104,165,191` (telemetry rows/badges, set
unknown ahead of time), `sequencer.js:82` (template dropdown from
server data), `ui.js:17,38` (toast notifications), `synoptic.js:193`
(whole external SVG document).

**Convert to toggling a pre-existing hidden element** (confirmed fixed
set of states — the actual point of the "no injected HTML" rule):
- `control.js:318``.tab-new-badge` dot created/removed per tab;
  should be one pre-existing `<span class="tab-new-badge d-none">`
  per tab, toggled via `classList`.
- `sequencer.js:169` — static "no steps" alert string re-injected on
  every render; should be a pre-existing hidden element.

## No injected CSS except genuinely runtime-computed values

**Synoptic panel**: `synoptic.js` fetches and injects an Inkscape SVG
(`web/static/img/synoptic.svg`) whose elements it drives purely through
`data-status` attributes (matched against telemetry subsystem names by
`parseStatusKey()` in `ui-core.js`), not hardcoded element IDs.

**Keep inline** (confirmed live-data-driven, no static class could
express it): `synoptic.js:47,79,111,127,139-140,154-157`
(`transform`/`transformOrigin` from dome azimuth, mirror offset, cover
position telemetry), `viewer/fits-viewer.js:652` (aspect ratio from
FITS shape), `:1408-1409` (pan/zoom transform), `:1450-1452`
(crosshair overlay position from live pixel coords).

`synoptic.svg`'s `camera`/`camera2`/`camera3` ids and `data-status`
values (leftover from before the scicam/teccam split) have been renamed
to `scicam1`/`scicam2`/`scicam3` to match the real telemetry subsystem
names (`/scicam1/...`, `/scicam2/...`, ... in `api.ini`) — before this
they could never match a broadcast subsystem, so those SVG fields were
silently dead.

**Custom class names** should start with `noctua-`.

**Convert to a class**:
- `control.js:320``dot.style.cssText` (badge sizing) → a CSS class.
- `components/NoctuaWidget.js:129``refreshBtn.style.display =
  'inline-block'`, paired with an inline `style="display:none"` already
  in the blueprint template (`macros/widget_blueprints.html:14`) — both
  sides should become `.d-none` toggling instead of inline style both
  ways.
- `viewer/fits-viewer.js:1041,1138``cursor` grabbing/crosshair,
  low-value inline case, could be `.noctua-cursor-grabbing`/`.noctua-cursor-crosshair`
  classes (minor, not urgent).

## Click safety — disable during in-flight requests

Two categories of `disabled`, both needed, don't conflate them:
1. **State-derived** (already used): reflects a real precondition
   (Expose disabled while Loop is on, `subsystem-offline`). Stays
   until the condition changes.
2. **Request-in-flight** (this section): self-clearing, only for the
   duration of *this* click's own round-trip. Prevents
   double-submission/races.

**Already done right, use as the template** — but duplicated 3x nearly
identically, extract into one shared `withBusyButton(btn, fn)` helper:
`actions.js:17-19,92-93` (`.btn-universal`), `control.js:98-100,117-118`
(`btn-check-target`), `guider-panel.js:137-139,154-155` (`postGuider`).

**Confirmed unprotected — close these when the helper lands**:
- Expose buttons (`postExpose`, `control.js:179-209`)
- Loop toggle switches (`wireLoopToggle`, `loop-toggle.js:60-83`) —
  checkbox equivalent: `el.disabled` for the duration, not a spinner.
- Stage relative +/- buttons (`moveStageRelative`, `control.js:141-159`)
  — also a genuine two-step read-then-write race (GET position, then
  PUT computed from it), not just a missing disable.
- `select.select-universal` change handler (`actions.js:103-142`) — no
  disable during its own fetch, unlike the `.btn-universal` click path
  in the same file.
- `webcam.js:18-71``.btn-webcam-move`/`.btn-webcam-preset`, zero
  `disabled` in the whole file; same GET-then-PUT race shape as stage
  relative.
- `sequencer.js:98-127` `saveOB()`, `:132-137` `deleteOB()`, `:317`
  template-browser refresh — none disable their triggering button.

**Required addition, not present anywhere today**: pair the
disable-until-response with a client-side timeout (`AbortController`,
~10-15s). No `fetch()` call in this codebase has one. Without it,
"re-enable when the response comes back" becomes "re-enable never" if
the backend hangs — the same failure class already seen with the Mako
connection lockup that needed a process restart to clear.

**Not needed**: idempotent periodic polling (e.g. `wireLoopToggle`'s
5s poll while a loop is on) — harmless to overlap.

## Dedupe found by the audit

- **`applyTransform` logic** duplicated near-verbatim:
  `status-stream.js:58-72` and `status-view.js:144-155` (same
  `data-map`/`data-transform` application, same `window.
  noctuaTransforms` lookup, same try/catch shape, differ only in the
  warn-message prefix). Hoist into one shared helper in `ui-core.js`.
- **Error-toast extraction** done four slightly different ways:
  `control.js:187-193`, `guider-panel.js:147-152`, `actions.js:70-72,
  135-138`, `sequencer.js:117-120` — same server error shape, four
  idioms. `webcam.js` (44-50,60-69) is the outlier that only
  `console.error`s with no `showToast` at all, unlike every sibling.
  Unify into one helper.
- **Ad-hoc error-alert HTML**: `components/NoctuaWidget.js:39,55`
  build their own `<div class="alert alert-danger p-2 small">` instead
  of calling `showToast()` like every other module — switch to
  `showToast()`.

## Dead code to remove

- `ui.js:58,95``noctuaUpdateFromRest` (+ its `window.
  noctuaUpdateFromRest` shim): zero callers anywhere, module or global.
- `ui-core.js:28``formatValue`: exported, never imported elsewhere.
- `ui-core.js:105``noctuaMaps`: exported but only ever used
  internally in the same file — drop the export.
- `ui-core.js:15``getStatusType`: only used internally too, same
  treatment as above (verify no external caller before removing the
  export, not the function itself).

**Not dead, but worth tidying**: `status-stream.js`/`status-view.js`
already `import` from `ui-core.js` yet reach for `window.
noctuaTransforms` instead of importing `noctuaTransforms` directly
(unlike `synoptic.js`, which does import it directly) — avoidable
global dependency, fix while touching these files for the
`applyTransform` dedupe above.

## JSDoc

Present with real docblocks: `ui.js`, `ui-core.js`, `loop-toggle.js`,
`guider-target-coords.js`, the whole `viewer/` subfolder,
`guider-panel.js`. Everything else (`control.js`, `sequencer.js`,
`synoptic.js`, `status-view.js`, `status-stream.js`,
`dependency-guard.js`, `toggle.js`, `ws-client.js`, `actions.js`,
`webcam.js`) has none — add JSDoc to exported functions as each file
comes up for review, following the `viewer/` subfolder's style as the
model.
+198 −0
Original line number Diff line number Diff line
# Python conventions

Target style for `noctua/**/*.py`. Written during the v0.9 cleanup
(2026-07). Not enforced yet — this is the reference the refactor works
towards, one file/module at a time, tested on real hardware before and
after.

## File header

Every `.py` file starts with:

```python
#!/usr/bin/env python3
# -*- coding: utf-8 -*-
```

followed by a one-line module docstring.

## Imports — three sections, always at the top

```python
# System modules
import asyncio
import time

# Third-party modules
import numpy as np
from astropy.io import fits

# Custom modules
from noctua import devices
from noctua.utils.logger import log
```

No underscore aliases unless there's a genuine name conflict (`import
time`, not `import time as _time`; `devices`, not `_dev`). 
No imports without a method.
No third-party modules except the ones listed in `pyproject.toml`

## Naming

`snake_case` everywhere (PEP 8) — methods, attributes, variables,
functions. No camelCase, even for names that mirror a JS-side concept.
Use declarative names, maximize auto-documentable variable names.

## Private names — minimize

Default to public names. Reach for a `_` prefix only when the name
genuinely must not be accessed from outside (internal thread/lock
bookkeeping, an SDK handle) — not as a default habit. This is an
explicit project preference, not just an observation.

## Docstrings

- Module: one-line, right after the header.
- Class: multi-line for non-trivial behavior, one-liner if simple.
- Public methods with a non-obvious signature: NumPy style
  (`Parameters`/`Returns`).
- Simple public methods: one-liner (`"""Aborts the current
  exposure."""`).
- Properties: prefixed with the type (`"""float : Current CCD
  temperature in degrees Celsius."""`).
- Private methods: no docstring unless the logic is non-obvious.
- **Always in English** — comments too. This is new as of this
  cleanup; older Italian comments/docstrings get translated as their
  file comes up for review. Translate also eventual italian log
  and user-facing error strings, e.g.
  `mako.py`: `"Mako: binning {b} non nel range [{lo}, {hi}]"`.

## Blank lines

- 1 between methods within a class; 2 between top-level
  classes/functions (PEP 8 standard — confirmed explicitly, not
  raising method spacing to 2).
- 1 blank line after a docstring, before the method body.
- 1 blank line before the **final** `return` of a function — not
  before an early one-line guard clause (`if self._looping: return`
  stays compact, very common in this codebase).

## Columns

- Fit into maximum 85 columns.

## Alignment

Vertically align `=` in `__init__` assignment blocks and dict
literals:

```python
self.active  = False
self.camera  = "teccam"
self.exptime = 1.0
```

## No type hints

Deliberate project choice — don't add them.

## `self.get()`/`self.put()` as the standard hardware I/O entry point

Every device should route generic hardware reads/writes through its
own `get()`/`put()`, the way `stx.py` (CGI HTTP), `mako.py` (VmbPy
feature), and `stl.py` (`SBIGUnivDrvCommand`) already do — not call the
underlying SDK/protocol directly from other methods.

**Fixed** (devices phase): `atik.py`'s `get(key)`/`put(key, value)` used
to be a generic Python `getattr`/`setattr` proxy, not hardware I/O.
`get(method, *args)`/`put(method, *args)` now call the named Artemis SDK
function directly (`getattr(self._lib, method)(h, *args)`), and every
method in the file routes through them instead of calling
`self._lib.Artemis...` itself — the only exception is `_check_connection()`'s
own bootstrap (establishing the handle can't go through get/put, which
need a handle to already exist; same precedent as mako.py's/stl.py's own
connection setup). The lock serializing SDK access (added earlier while
chasing the atik.py hardware bugs on `main`, see PLAN.md) now lives
inside get()/put() themselves, the single choke point, instead of being
sprinkled across every property.

## Error handling — `utils/check.py` decorators

Every device's `get()`/`put()` should be wrapped by a protocol-specific
decorator from `utils/check.py`, not an inline `try/except`. Already
consistent: `stx.py`/`astelco.py`/`alpaca.py`/`netio.py`/`ipcam.py`/
`siemens.py`/`meteo.py` use `@check.request_errors` or
`@check.telnet_errors` (`ascom_errors`/`meteo_errors`/`socket_errors`
are defined but currently unused anywhere).

**Fixed** (devices phase): two new decorators added — `check.ctypes_errors`
(`atik.py`, `stl.py`) and `check.vmbpy_errors` (`mako.py`), replacing
`mako.py put()`'s inline `try/except` and covering `atik.py`/`mako.py
get()`, which had no error handling at all before. Both catch
Python-level failures around the call (bad function/feature name,
connection lost, driver/OS failure) — neither interprets a SDK's own
numeric return/error code (still each call site's job: `stl.py`'s
`PAR_ERROR` meaning isn't uniform across every wrapped command, same for
Atik's return values, so a decorator can't generically judge
success/failure from them). Unlike the HTTP/telnet decorators, **neither
resets `this.error` at the start of the call** — get()/put() here are
the low-level primitive, called many times per higher-level operation
(e.g. once per row in `stl.py`'s readout loop), and both devices already
manage `self.error` themselves, clearing it only after a successful
reconnect. Resetting it per get()/put() call would wipe out an error
appended by an earlier call in the same operation.

**Decided**: several `check.py` fallbacks do `raise SystemExit(e)` on a
fully unhandled exception — this kills the entire `noctua-app` process
for an unexpected error on a single device. `pyproject.toml` defines
three separate entry points (`noctua-app`, `noctua-sequencer`,
`noctua-guider`), plus ad hoc usage (ipython, scripts) — only
`noctua-app` is the long-lived process where recovering from a single
device's error is worth it; everywhere else (ipython, the other two
entry points) should keep failing loudly and immediately, as today.

Don't infer this from the environment (`sys.argv[0]`, env vars — both
fragile across systemd/tmux/`python -m`/pip-wrapper invocations).
Instead, an explicit opt-in flag in `check.py`, default `False` (today's
hard-exit behavior, unchanged everywhere):

```python
_RECOVERABLE = False

def set_recoverable(value=True):
    global _RECOVERABLE
    _RECOVERABLE = value
```

New decorators check `_RECOVERABLE` in the unhandled-exception fallback
(`if _RECOVERABLE: log.error(...); return` instead of `raise
SystemExit(e)`). Only `noctua.app:run()` calls `set_recoverable(True)`,
before starting the server — `noctua.sequencer:cli()`,
`noctua.guider:cli()`, and ipython never call it, so they keep the
current hard-exit behavior unchanged. Implementation is devices-phase
work, not done yet.

**Also found by audit**: `domotics.py`'s `get`/`put` (lines 25-26,
58-59) start with a bare `return` before any body — dead code, every
property built on them is non-functional as written. `meteo.py`'s
`summary()` (99-109) reads keys (`TempIn`/`TempOut`/...) that `data()`
(46-97) has already renamed to `Temperature_first_floor`/etc — would
`KeyError` if ever called. `ipcam.py`'s `DlinkDCSCamera.__init__`
(20-26) duplicates `BaseDevice.__init__`'s body instead of calling
`super().__init__(url)`; its `save_image` (173-174) is a `@property`
that takes a `filename` parameter, which a property can never receive
through attribute access — likely a leftover-from-method bug. Fix
these as real bugs when their device comes up in the devices phase,
not just style.

## Formatting/linting tools — confirmed

- **`ruff format`** for mechanical whitespace (Black-compatible,
  faster than `autopep8`). Does not enforce the two custom rules above
  (blank line after docstring, blank line before final return) — those
  stay manual.
- **`ruff`** as a linter, **advisory only, not a blocking gate**
  chosen over `pylint` for speed and a single config file. Needs a
  project-tailored config given deliberate choices (no type hints,
  dynamic ctypes attribute access, `check.py`'s intentionally repeated
  decorator boilerplate) that default rules would flag as problems.

dev/refactor/PLAN.md

0 → 100644
+1051 −0

File added.

Preview size limit exceeded, changes collapsed.

+25.7 KiB

File added.

No diff preview for this file type.

Loading