Commit 70aeebcd authored by vertighel's avatar vertighel
Browse files

Fase 0 refactor v0.9: convenzioni e piano; fix indentazione atik.py



dev/conventions/{python,html,javascript}.md: stile obiettivo per i tre
strati, con le decisioni prese in fase di pianificazione (var->data
coordinato con status-view.js, Sass @extend su selettori di tag con lo
scope esatto trovato dall'audit, ES module come paradigma JS, pattern
click-safety withBusyButton, PEP8 standard + regole aggiuntive).

dev/refactor/PLAN.md: checklist per fase (devices/templates/api/jinja/
javascript/UX), con i bug reali trovati dall'audit a sola lettura sui 5
strati (non solo problemi di stile) — es. observation.py/snapshot.py
divergenti, focus2.py orfano, domotics.py/meteo.py/ipcam.py con bug,
environment.py/common.py/test.py morti nell'api.

atik.py: fix indentazione (log.debug fuori posto dopo hdu.writeto).

Co-Authored-By: default avatarClaude Sonnet 5 <noreply@anthropic.com>
parent cee66ed7
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.
+163 −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

**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).

**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 `.cursor-grabbing`/`.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.
+163 −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`). Deferred
imports inside a method only when strictly necessary (circular
imports, optional dependency).

## Naming

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

## 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. Scope is comments/docstrings only — log
  and user-facing error strings are a separate, not-yet-decided
  question (today's codebase mixes Italian/English there, e.g.
  `mako.py`: `"Mako: binning {b} non nel range [{lo}, {hi}]"` vs
  `"Mako connection failed"" — leave as-is until asked).

## 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).

## 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.

**Known violation** (audit, 2026-07): `atik.py`'s `get(key)`/`put(key,
value)` (atik.py:129-135) is a generic Python `getattr`/`setattr`
proxy, not hardware I/O — every real SDK call in the file
(`_start`/`download`/`set_window`/binning/cooler/temperature, see
atik.py:204-215, 261-324, 417-480, 532-561) bypasses it and calls
`self._lib.Artemis...` directly. Fix during the devices phase: give
atik.py a real get/put wrapping the Artemis SDK, and route the
bypassing methods through it.

## 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).

**Known violations** (audit, 2026-07): `atik.py` (no error handling at
all around its bare `get`/`put` proxy calls — the SDK calls that
bypass it have no protection either), `mako.py` (`get()`,
mako.py:101-104, has *no* error handling; `put()`, mako.py:106-114,
catches generic `Exception` inline), `stl.py` (uses a fundamentally
different, non-exception paradigm — `get`/`put` return a `PAR_ERROR`
code, every call site checks it manually; this is a deliberate,
documented choice per the module docstring, not a bug — a decorator
here would need to match that shape, not force exceptions onto it).
Fix: design new decorators for the ctypes/SDK family (one for
Atik/STL-style ctypes calls, one for VmbPy) instead of reusing the
existing HTTP/telnet-specific ones, which don't fit.

**Open question, not decided**: several `check.py` fallbacks do `raise
SystemExit(e)` on a fully unhandled exception — this kills the entire
`noctua-api` process for an unexpected error on a single device.
Decide during the devices phase whether new decorators should replicate
this or contain the failure to just that device/subsystem.

**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
+244 −0

File added.

Preview size limit exceeded, changes collapsed.

+2 −1
Original line number Diff line number Diff line
@@ -323,6 +323,7 @@ class Camera(BaseDevice):
        log.debug(f"Writing on disk")
        hdu.writeto(filepath, overwrite=True)
        log.debug(f"Written")
        
        return filepath

    @property