release: promote main to production (77 commits) - #500
Merged
Conversation
Fix Xero fixture crash during instance provisioning
Suppression comments had accumulated as a way to make checks pass rather
than as deliberate, justified overrides. Audit found 61; this removes 52,
leaving 9 that are genuinely unavoidable.
Config, not inline comments:
- Delete .flake8, which competed with pyproject.toml's [tool.flake8] and
disagreed with it. Every `# noqa: E402` in scripts/ (15) was dead under
both configs. Drop the stale test_gzip.py per-file-ignore.
- Add an eslint override for the vendored shadcn-vue components instead of
repeating `eslint-disable vue/multi-word-component-names` in 23 files.
- Configure no-unused-vars with ignoreRestSiblings/^_ patterns rather than
suppressing deliberate destructuring-omits case by case.
- Enable reportUnusedDisableDirectives so dead directives can't reaccumulate.
Delete the xero/sync.py re-export shim: the autogenerated api/xero/__init__.py
already re-exports every name from its defining module, so the `# noqa: F401`
block was redundant. Consumers now import from push/seed/transforms directly
(ADR 0017 - no aliases kept for safety).
Replace `no-explicit-any` suppressions with real types. Two were hiding live
unsoundness: updateField/updateTaskField took `keyof T` but assigned a string,
so `updateField('tasks', 'oops')` type-checked. archived-jobs.vue discarded a
fully-typed generated response for hand-copied annotations that had already
drifted from the schema; restoring the real type proved several guards dead.
quote_spreadsheet.py's blanket `# flake8: noqa` + `# pylint: skip-file`
suppressed nothing - the file is clean under both configs.
Also fixes pre-existing gate failures on this branch: check_mypy.sh was
already red at HEAD with 12 unbaselined errors from the demo-seeding commits.
Types them properly rather than baselining, shrinking mypy-baseline.txt by 37.
Removes shadow defaults in the xero command, where `options.get(key, default)`
duplicated defaults argparse already guarantees.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RrwgHpia3Cbs4YBWPd7ggJ
fix: repair demo instance seeding
`apps/workflow/apps.py:63` carried `# noqa: F401` that suppressed nothing. Both provider imports are `import apps.workflow.accounting.xero.<mod>`, so both bind the same top-level name `apps`; pyflakes reports only the last binding as unused. Verified with `flake8 --disable-noqa`, which flags line 64 alone. Ruff's RUF100 agrees. The remaining directive on line 64 is live and stays. The pair now reads asymmetrically, which is accurate rather than an oversight. Note for future readers: RUF100 is the Python analogue of the eslint reportUnusedDisableDirectives enabled in 8af74f2, but it cannot be adopted as a gate while flake8 is the authority. Ruff does not emit F401 for `from x import *`, so it misreports settings_test.py's `# noqa: F401, F403` as half-redundant when flake8 needs both codes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RrwgHpia3Cbs4YBWPd7ggJ
Clears six high-severity Dependabot alerts on the mcp Python SDK (WebSocket Host/Origin validation, HTTP transport principal verification, experimental task handler access) and one on adm-zip (crafted ZIP triggering a 4GB allocation). Neither was reachable here: no MCP transport is served (mcp_server is in INSTALLED_APPS but no URLs are mounted; the SDK is used only for in-process toolset definitions), and adm-zip only reads Playwright trace zips this repo generates itself. Bumped to clear the noise. pyproject.toml needed no change - mcp = "^1.9.4" already permitted 1.28.1. requirements.txt regenerated from the lock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XQL2tdpSfL4ezmMReMiEg6
`DeltaValidationError` subclasses `PreconditionFailed`, but the broader handler was ordered first, so the `DeltaValidationError` arm in `JobRestService.update_job` was unreachable. The impact was silent data loss, not just a dead branch. Validation runs at line 1033; `soft_fail_context` is not assigned until line 1050. So a hard checksum or before-state failure hit `except PreconditionFailed` with `soft_fail_context` still None, the `if soft_fail_context:` guard was False, and nothing was recorded — the JobDeltaRejection carrying the reason, detail, envelope, change_id, checksum and request ETag was never written. Operators lost the only record of why a client's edit was refused. Reorder so the subclass is caught first. Adds a test that drives update_job with a stale checksum and asserts the rejection is persisted; it fails on the old ordering with DoesNotExist. Found by pylint E0701 (bad-except-order), which has never run in this repo — full pylint crashes on startup because pylint_django's foreign_key_strings checker calls django.setup() without DJANGO_SETTINGS_MODULE set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RrwgHpia3Cbs4YBWPd7ggJ
Adds E0701, E1123, W0101, W0612, W0707 and R1710 to the pre-commit pylint allowlist and fixes all 136 resulting findings. Deliberately an allowlist rather than a baseline. Full pylint reports ~4300 findings here and ~72% are noise or actively fight this codebase: line-too-long duplicates Black (which is why flake8 already ignores E501), abstract-method is a DRF false positive, duplicate-code is cross-module so it cannot baseline stably, and broad-exception-caught fires on the very handler pattern ADR 0019 mandates. Baselining that would have enshrined 4300 lines of debt nobody could burn down. The error category is no better: of 19 E-level findings, 14 were false positives on SDK property monkeypatching in xero/payroll.py, plus E0203 misreading Django FK _id descriptors and E0606 unable to prove two paired `if query:` branches. E1111, E1121, E0203, E0606, E1120 and C0325 were tested and rejected. E0701 and E1123 stayed — each found a live bug. The single highest-value fix is a contract correction, not a lint tweak: `persist_and_raise` was annotated `-> None` but always raises. That one wrong annotation produced 37 of the 41 inconsistent-return findings. Annotating it `NoReturn` fixed all 37 at once and makes callers' unreachable code visible to mypy — which immediately exposed a dead `raise AssertionError` guard in workshop_pdf_service.py, now removed. apps/accounting/services/core.py carries a near-verbatim duplicate of that helper with the same wrong annotation; it is also corrected, and should probably be deleted in favour of the shared one. Also deletes xero.get_projects: it passed `if_modified_since` to ProjectApi.get_projects, which has no such parameter, so the `case datetime()` branch raised TypeError at runtime. It had zero callers and the parameter is unimplementable against the SDK, so per ADR 0017 it goes rather than being patched. Found by E1123. mypy-baseline.txt shrinks by 37, deletions only, via `mypy-baseline sync`. Note for anyone running full pylint: it crashes without DJANGO_SETTINGS_MODULE set, because pylint_django's foreign_key_strings checker calls django.setup(). The hook survives only because --disable=all stops that checker loading. Also note `pylint apps/` discovers fewer modules than passing explicit file paths; three findings were only visible the way pre-commit actually invokes it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RrwgHpia3Cbs4YBWPd7ggJ
Full pylint has never run in this repo. pylint_django's foreign_key_strings checker calls django.setup() when it opens; without DJANGO_SETTINGS_MODULE in the environment that raises ImproperlyConfigured, and the plugin's fallback (settings.configure(Settings(...))) then dies with "Settings already configured". Pylint crashed before checking anything. The existing hook only survived because --disable=all stops that checker loading, so the Django config below it was never exercised. Set the variable via init-hook so `poetry run pylint <path>` just works — for the pre-commit hook, CI, and anyone running it by hand. Not .env: .env is gitignored, so CI and fresh clones would still crash, and it is only read by load_dotenv() inside settings.py — which Django cannot reach until DJANGO_SETTINGS_MODULE is already known. The value is invariant for the repo, so it belongs in committed config. Also drops the [tool.pylint.MASTER] block, which duplicated load-plugins and django-settings-module verbatim from [tool.pylint]. Re-tested the six rules rejected in d5597fd now that Django is properly configured: they are still false positives (20 findings, up from 19 — too-many-function-args went 7 to 8). The rejection was not a config artifact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RrwgHpia3Cbs4YBWPd7ggJ
`synchronise_xero_data` and `deep_sync_xero_data` were untyped, so every call site tripped no-untyped-call. Annotate both as `Iterator[XeroSyncEvent]`, matching the already-typed `sync_all_xero_data`/`one_way_sync_all_xero_data`. Two contract corrections fall out of this: - `synchronise_xero_data(delay_between_requests=1)` never read the parameter and no caller passed it. Dropped (ADR 0017). - `XeroSyncEvent` was missing `status`, which the orchestrator and `sync_all_xero_data` both emit and `frontend/src/pages/xero.vue` renders as the per-entity status. Added to the TypedDict rather than suppressed. Baseline shrinks by 8; gate reports 0 new. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1w9HHLiRq52gzaVAsFqkA
`--import-staff` crashed on any instance whose Xero tenant contains a
terminated employee:
TypeError: can't compare datetime.datetime to datetime.date
The Xero Payroll NZ SDK deserializes employee `end_date` as a datetime,
and the active-employee filter compared it directly against
`date.today()`. Every other Xero date comparison in the codebase already
routes through the shared normalizer; this was the one place that did
not, and the one place that crashed.
Rename `_coerce_xero_date` to `coerce_xero_date` so it can be used from
apps.timesheet, updating all call sites, and use it in the filter.
The import path had no test coverage, and the seeded demo tenant has no
terminated employees, so neither dev nor E2E reached the branch. Adds a
regression test whose stub employees carry datetime end_dates, which
reproduces the original traceback without the fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019c1zitTHbPmsFaiEubgpVn
CodeRabbit flagged ~50 handlers on this PR for missing persist_app_error. Most were false positives: apps/workflow/exception_handlers.py is a global DRF boundary that persists every exception not already wrapped in AlreadyLoggedException, so DRF-served sites already produce exactly one AppError row. Persisting at those inner sites would either double-log, or break handle_service_error's match on type(error).__name__ and turn 400s into 500s. Management commands have no such boundary. They run under cron/systemd, so a CommandError raised from a caught exception was invisible in AppError. Persist the caught exception before converting it, keeping the operator- facing CommandError message unchanged. test_gemini_chat's broad handler gains the AlreadyLoggedException pass-through arm so upstream-persisted failures don't get a second row. Also narrows job_rest_views.py:1235 from `except Exception` to `except Http404` — it wrapped get_object_or_404 and reported every failure, including DB and programming errors, as "Job with id X not found". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1w9HHLiRq52gzaVAsFqkA
…ption
AlreadyLoggedException conflated metadata about an exception ("already
persisted") with the exception's identity. Carrying the metadata destroyed
the type — and the type is what a boundary needs to choose 404 vs 412 vs
503. That caused three live defects:
1. Double-persisted errors. 167 of 236 persist_app_error call sites
persisted and re-raised without wrapping, so the exception reached
custom_exception_handler looking un-persisted and earned a second
AppError row. AppError has no uniqueness constraint to collapse them.
2. Correctly-handled failures became 500s. handle_service_error matched on
type(error).__name__, so an AlreadyLoggedException from a service that
followed ADR 0001 matched nothing and fell to the default arm.
3. ValueError subclasses became 500s. Name-string matching sent
AllocationDeletionError, DeliveryReceiptValidationError and
InvoiceCalculationError to 500 instead of 400.
Mark, don't wrap. persist_app_error now records the AppError on the
exception and returns the existing row on any later call, so one failure
is one row by construction rather than by 936 handlers each remembering a
two-arm ritual. Lookup follows the __cause__ chain, so a handler that
converts (raise ValueError(...) from exc) still resolves to one row — which
is what makes the pylint W0707 chaining mandatory rather than cosmetic.
AlreadyLoggedException and persist_and_raise are deleted (ADR 0017): 114
pass-through arms collapse to plain re-raises, 69 persist_and_raise calls
become persist_app_error + raise, and ~35 response builders read the id via
the new app_error_for accessor.
handle_service_error now dispatches by isinstance, specific-before-general,
so subclasses resolve to their base's status. Response bodies are unchanged
throughout — frontend/schema.yml has an empty diff, so the generated client
does not move. job_quote_chat_views.handle_error gains persistence; it
returns a Response instead of re-raising, so its failures never reached the
DRF boundary that would have recorded them.
Also narrows job_rest_views.py's delta-rejection lookup from except
Exception to except Http404 (it reported DB and programming errors as "Job
not found"), and drops scripts/persist_error_audit.py, whose only signal
was the presence of the now-deleted class.
Removing persist_and_raise's **context: Any restored real type checking at
its call sites, shrinking mypy-baseline.txt by 32 entries.
Full suite: 1039 tests, all passing. check_mypy.sh exit 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J1w9HHLiRq52gzaVAsFqkA
Commit 066a1df retired AlreadyLoggedException and made persist_app_error idempotent, but the standing guidance still taught the deleted two-arm wrapper pattern. Those guidelines are what CodeRabbit cited as its source, so left stale they regenerate the same review and lead the next agent to reintroduce the wrapper. - ADR 0001 rewritten in place (number stable per the ADR README): the decision is now idempotent persist_app_error — it marks the exception and returns the existing row on any later call, follows the __cause__ chain so conversions stay one row, and the boundary maps status from the real type. The retired marker wrapper is recorded under Alternatives with why it was dropped. - ADR 0019: the two "two-arm dedup pattern" references reworded to idempotency; the persist-every-exception mandate is unchanged. - CLAUDE.md and .github/copilot-instructions.md: the mandatory-error-persistence block now shows the one-arm handler, the from-exc requirement on conversions (pylint W0707), and app_error_for at the boundary. - ADR index retitled to match 0001's new heading. No standing guideline references AlreadyLoggedException or persist_and_raise now; the single remaining mention is ADR 0001's Alternatives section recording the superseded design. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1w9HHLiRq52gzaVAsFqkA
`--import-staff` still crashed after the end_date fix, one call later in `get_employee_working_patterns`. The function read `response.working_patterns` and `pattern.working_weeks`, but the Xero SDK exposes neither: the list call returns `payee_working_patterns` (identifiers and effective dates only), and the weekly hours live behind a second per-pattern fetch. It had never returned data since it was written — no test covered it, and the seeded demo tenant never reached the branch. Rewrite as the documented sequence: list patterns, select the latest whose `effective_from` (normalised via `coerce_xero_date`, since the SDK hands back datetimes here too) is already in effect, then fetch that pattern's working weeks. Return an empty list — which the caller already handles by seeding CompanyDefaults hours — for no pattern, none-yet-effective, or an alternating multi-week roster that has no single-week representation in Staff.hours_*. These hours are a Docketworks roster expectation (timesheet visibility and workshop capacity), seeded once from Xero at onboarding; they are not synced back and an admin can correct them. Adds direct coverage for the function with stubs mirroring the real SDK models, and replaces the previous `return_value=[]` mock — which let the import tests pass over a function that could only raise — with a realistic pattern that asserts the created Staff's weekly hours. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019c1zitTHbPmsFaiEubgpVn
…o-instance-seeding Consolidates PR #483 (fix/KAN-295-adjustment-continuous-entry) into #480 — one branch, one PR. #483's draftSession/usePhantomRow model lets a typed adjustment promote to a draft immediately, freeing the blank row for continuous entry. Conflict was confined to SmartCostLinesTable.vue. #483 rewrote the emit path (removing the create-line event entirely in favour of the draft model), which collided with our branch's loggedEmit removal (8af74f2). Resolved by taking #483's version and re-applying the loggedEmit/(emit as any) removal on top — the file is free of the banned `as any` and no longer references the deleted create-line event. Also fixed a stale assertion #483 left behind: it refactored the cleared-unit_cost handlers from `null` to `undefined` (schema-correct — CostLine.unit_cost is a non-nullable number, so a cleared field is absent, not null) but did not update SmartCostLinesTable.test.ts, whose assertion still expected null. The test now asserts toBeUndefined, matching the type-safe behaviour. Our branch's previous `null` only compiled through Object.assign's looseness; #483's typed updateLine correctly rejects it. Frontend green: type-check, eslint, SmartCostLinesTable (7) and useCostLineDrafts (3) unit tests all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1w9HHLiRq52gzaVAsFqkA
…etRowId) The #483 merge combined our branch's getRowId (which commit 8af74f2 had changed from `||` to `??`) with #483's phantom row `{ id: '', __localId: ... }`. `'' ?? x` returns '' (empty string is not nullish), so the phantom rendered `data-row-id=""` and the E2E add-row helper rejected it ("Could not find phantom row", test 9 "add Labour entry"). Two textually non-conflicting changes that were semantically incompatible, so git auto-merged them and only E2E caught it. Revert the operator to `||`: an empty-string id means "no persisted id yet" and must fall through to the draft's __localId. Keeps 8af74f2's typed signature (no `any`); type-neutral. Adds DataTable.getRowId.test.ts guarding that a row with id:'' renders a non-empty data-row-id — fails against `??`, passes with `||`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1w9HHLiRq52gzaVAsFqkA
The timesheet entry phantom row carried no __localId, so DataTable keyed it
by array index. When its index shifted during a save (the pushed saved row
plus the still-set pending row briefly make three rows), its :key changed and
the TimesheetJobPicker remounted — tearing down the popover and dropping
keyboard focus. Under full-suite CPU load the async focus-handoff lost this
race ~25% of the time (72 pass / 19 fail / 6 perf-fail in the ledger),
failing the keyboard-only Tab-entry E2E.
- usePhantomRow now stamps a fresh, stable __localId on every phantom by
construction, so the identity invariant can't be forgotten per-table. Uses
a shared createLocalRowId() util. This also fixes the latent PurchaseOrder
line phantom; SmartCostLinesTable moves onto the same helper.
- DataTable.getRowId drops the local-${index} fallback and fails-early when a
row has neither id nor __localId — a row with no stable identity is a bug to
surface, not hide behind an index key (ADR 0015).
- TimesheetJobPicker focuses its search input via PopoverContent's
@open-auto-focus (fires after content mount) instead of a fire-and-forget
nextTick that silently no-opped under load.
- Breadcrumb left on the focusPhantomToken watcher: if the row-to-row handoff
flakes again, collapse the multi-hop chain rather than adding more hops.
keyboard-nav.spec.ts is unchanged — it is the behavioural guard for this fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The auth-guard test mocks vue-router/auto-routes with component-less records. vue-router v5 warns on any leaf record lacking a component/children, spamming stderr on every unit run. The test drives the real production guard and asserts navigation outcomes (route name, redirect query) that don't depend on a component, so it stays correct — the records just need to be valid v5 records. Attach a shared no-op render-null stub (defined inside the vi.mock factory to avoid the hoisting trap). No change to what is tested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix: enforce Xero sync gate, make pylint runnable, and retire the KAN-278 cutover path
…t-config-typescript, rrweb-player, adm-zip) Batched frontend Dependabot updates on chore/dependabot-batch: - js-cookie 3.0.7 -> 3.0.8 (#473) - @vitejs/plugin-vue 6.0.7 -> 6.0.8 (#475) - @vue/eslint-config-typescript 14.7.0 -> 14.9.0 (#471) - rrweb-player 1.0.0-alpha.4 -> 2.1.0 (#472, major — type-check + build clean) - adm-zip 0.5.17 -> 0.6.0 (#479, lockfile) Dropped from this batch: - typescript 6.0.3 -> 7.0.2 (#474): upstream-blocked. typescript-eslint has no TS7 release (latest 8.65.0 still pins typescript <6.1.0) and vue-tsc's line calls TS's removed ./lib/tsc export. Adopting TS7 would mean removing type-aware ESLint, not migrating it. Keep #474 open until the eslint stack ships TS7 support (mirrors the astroid/pylint block on #444). Verified: npm run type-check and npm run build both pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
KAN-294: ops docs → Drive/NotebookLM (+ staff offboarding fix, gemini-flash-latest)
Address CodeRabbit review on PR #490. Root cause of two findings was modelling a required setting as nullable. Xero quote terms are mandatory (quote creation 400s without them) and both fixtures already seed a value, so the null state and its once-only auto-derive branch were dead. Make the field non-null with a default: - The request schema no longer advertises a nullable it rejects (finding 3). - Saving any Xero setting can no longer 400 the whole PATCH by round-tripping a null terms value through AdminCompanySectionView.save(). Remove the try/except/finally from inspect_quote_pdf entirely (finding 1): a failed unlink in finally masked the real PDF error. Unlink only on the success path; let errors propagate as a traceback from the ops command. Declined finding 2 (URL-component terms builder): its fix drops the URL path, turning https://sites.google.com/view/acme into https://sites.google.com/terms-of-trade. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UTKr8tHRW5iam4YriPSzc
CodeRabbit incremental review: extract_text() returns "" for blank or image-only pages, so an all-blank PDF produced a non-empty page_text list, skipped the no-extractable-text guard, and reported the marker absent while deleting the diagnostic file. Keep only pages with real text so the guard fires. Add a blank-page regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UTKr8tHRW5iam4YriPSzc
KAN-299 Send terms on Xero quotes and verify native PDFs
Replace the homegrown src/utils/debug.ts (debugLog) with the `debug` npm
library as the one logging gate for both the Vue app and the Playwright
E2E suite. Each module owns a <domain>:<feature> namespace (job:autosave,
kanban:core, ...); enable via localStorage.debug='job:*' in the browser or
DEBUG=e2e:kanban for node E2E. Silent by default, loud on demand — feature
logs are preserved-but-quiet instead of kept-as-noise or deleted-and-lost.
- Migrate every debugLog call site across 92 source files to namespaced
debug() loggers; delete the homegrown module (atomic, no shim, ADR 0017).
- Fix 19 vitest files that mocked the old module; kanban log-assertions
replaced by the behavioural assertions they duplicated.
- Add tests/fixtures/debug-forwarder.ts: a gated, namespaced browser->test
console forwarder wired into the auth fixture (DEBUG=e2e:<area>),
replacing ad-hoc per-test page.on('console') handlers.
- Whole-suite E2E cleanup: tests console.log ~148 -> 12 (bad-state keeps
only); redundant-with-assertion logs deleted, feature narration gated.
- Record the discipline: ADR 0031 (single logging gate) and ADR 0032
(prefer libraries over homegrown), plus frontend/CLAUDE.md rule 31 and
new frontend/tests/CLAUDE.md.
Verified: type-check, vitest (361), lint, build all green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UTKr8tHRW5iam4YriPSzc
…slowdown
E2E ran ~2.5x slow (1.2h vs ~25min). Root cause (single-variable A/B):
ngrok-free's tunnel is throughput-capped (~1 MB/s; loopback is 67-247 MB/s),
and Vite dev serves a 21 MB uncompressed module payload (lucide 4 MB, vue
1.7 MB, rrweb, generated api.ts ...). 21 MB / 1 MB/s = the 20-30s page loads.
Not the laptop, not the logging change, not prod (prod ships a built,
tree-shaken, gzipped ~1.5 MB bundle).
Fix: serve the production build for E2E, kept on the ngrok public URL (Xero
OAuth callback + real round-trip preserved) and exposed on the LAN. Fast
because the payload is ~14x smaller, not because the network is removed --
so it also matches what real users load, instead of masking the cost via
loopback.
- vite.config.ts: add a preview{} block. `vite preview` does NOT inherit
`server.proxy`, so mirror the /api + /media proxy and allowedHosts (or the
whole suite 404s), plus host 0.0.0.0 for LAN exposure.
- package.json: preview:e2e = "vite build && vite preview".
- .vscode/tasks.json: "Frontend Preview (build)" task + "Start E2E
Environment" compound (mirror of Start Dev Environment, frontend swapped).
The two modes both bind :5173, so you pick one.
Verified on loopback: build serves, /api proxies (401 from Django), login
2.3s, LAN-exposed. Suite-level "<30min through ngrok" confirmation is still
pending a run via the E2E environment -- merge gate until then.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UTKr8tHRW5iam4YriPSzc
Start E2E Environment started frontend-preview + ngrok + celery but not Django, so every /api call would fail. Add a hidden Django (runserver) task (plain `runserver --noreload`, no debugpy -- faster per request and no Run>Debug dependency) and include it in the compound. The E2E env is now self-contained; still only two visible tasks (Dev / E2E). Run>Debug stays for actual debugging. Binds :8000, so stop the debugpy session first (same conflict pattern as :5173). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UTKr8tHRW5iam4YriPSzc
…ble via ngrok readBackendAppDomain() returned "" under NODE_ENV=production -- the mode that `vite build && vite preview` runs in -- so appDomain was empty, preview allowedHosts collapsed to ["localhost"], and the ngrok host got a 403 (the built app never loaded). Remove the NODE_ENV fork: read APP_DOMAIN whenever the backend .env is present (all local use), "" only if absent (CI). Dev and preview now resolve allowedHosts identically. Verified on the REAL path (not localhost): ngrok/login -> 200, and login through the actual ngrok URL against the build = ~3.0s (vs ~20s against the dev server). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UTKr8tHRW5iam4YriPSzc
Preflight kept aborting on leftover [TEST] companies. Teardown was not at fault: it restores the DB and its post-restore checkSafeToTest passed. The rows came back afterwards, from Xero. E2E creates real Contacts/Invoices/Quotes in the dev Xero org (XERO_READONLY=False by design), and xero_regular_sync_task polls hourly with if_modified_since. The run always sits inside that window, so the next poll re-imported the run's artifacts into the freshly restored database: teardown clean at 20:15, poll at 20:19:44, 12 [TEST] companies by 20:53. The surviving row had xero_last_modified 20:14 but django_created_at 21:19. Not the webhook - none fired. No pause shorter than the poll interval can help, so the existing 90s settle covers the wrong channel. Record each run's wall-clock window, and skip an inbound Xero object when both its timestamp falls inside a closed window and it belongs to a test company. Neither condition works alone: the window alone would discard genuine Xero edits landing inside it, and the name alone is equally true mid-run, which would blind the round trip the run exists to exercise. Windows stay open while a run executes, so a poll firing mid-run - or after a pause of a day - behaves exactly as production. Development only, via the existing PRODUCTION_LIKE setting plus the windows file being absent anywhere E2E has never run. No new env var, so no deployment, .env.example or instance template changes. State lives in a temp file rather than the database because teardown restores from a backup taken before the run, so database state cannot describe a run that has just finished. Filtering happens on the fetched batch before sync_function: the invoice, quote and PO importers resolve their contact through resolve_company_from_xero_contact, which raises rather than skips when the company is gone, so a contact and its documents must drop together. The sync cursor deliberately advances over the unfiltered batch so suppressed objects are not refetched every hour. Also fixes the login flake that surfaced this: the E2E /me waiter resolved on the expected pre-auth 401 session check instead of the authenticated response. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZwrpicKhDWgYN7hHAitTm
Strict mypy (no-untyped-def) requires return + parameter annotations; add them (test methods -> None, helper updated_at: datetime). No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Creating a staff member always returned 400, and editing any current
employee did too. The three staff endpoints accepted only multipart, and
zodios serialises a body with formData.append(), which turns null into
the literal string "null". A blank Date Left therefore arrived as "null"
and DRF's DateField rejected it. Every current employee has a blank
date_left, so the only staff you could edit were ones already offboarded.
The encoding was the defect, not the field. Multipart cannot express
null, numbers, booleans or arrays, so GenericStaffMethodsMixin existed
purely to rebuild the types it destroyed ("" -> [], "" -> "0.00"). That
exception list never covered dates. The endpoints were multipart only to
carry the optional profile icon -- and PUT/PATCH never even sent one.
Give the staff resource JSONParser and move the icon to its own
multipart endpoint, mirroring the company-logo and job-file split. The
coercion mixin is deleted rather than extended. As a side effect the
icon now uploads on edit as well as create; previously the file was
silently discarded on edit, with no error shown.
Two latent bugs surfaced once a staff member could actually have a
photo. Neither was reachable before, because no staff row has ever had
one:
- icon_url was built with request.build_absolute_uri, but
USE_X_FORWARDED_HOST is set only under PRODUCTION_LIKE. Behind a
proxy that yields the internal host, and the browser blocks the
image as a cross-origin loopback request. It now returns a
site-root-relative path, matching what workshop_view,
daily_timesheet_service and kanban_service already emit and what
StaffAvatar's startsWith('/') branch was written to consume.
- KanbanJobPersonSerializer declared icon_url as URLField, which
reaches the generated client as z.string().url() and rejects a
relative path. kanban_service already returned relative URLs, so
this was armed for the first upload. Now CharField.
Tests: staff create/edit had no backend coverage and the edit path had
no E2E coverage at all, which is how the regression reached main. Adds
backend tests for the JSON contract (blank date_left leaves staff
active, clearing it reinstates, groups round-trip as arrays) and for the
icon endpoint, plus E2E covering create, edit, and a photo upload that
must actually serve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The get_enum_choices view at /api/enums/<enum_name>/ had no consumers: the frontend reads enums from the generated OpenAPI zod client (ADR 0021), and no backend code or test invoked it. Its defensive machinery never did useful work — the import/hasattr/fallback cascade resolved no real case, and the "helpful 404" branch imported bare job.enums/workflow.enums against an apps.-rooted package, so it silently returned 500 since inception. Removes the view, the URL route, the autogenerated re-export, the stale schema-coverage exemption, the docs category override, and regenerates the URL docs. Per ADR 0032 (less code) and ADR 0017 (no "for safety" retention). Also folds in an unrelated in-progress StaffAvatar.vue edit that was in the worktree (uses staff.icon_url directly instead of the origin-prefixing computed), committed together per the all-or-nothing worktree policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ing to mediafiles Follow-ups to the staff API split. The icon endpoint gains DELETE, so a profile picture can be removed and not just replaced. This also lets the E2E clean up after itself: teardown restores the database but not MEDIA_ROOT, so a run that uploaded a photo left the file behind for good. _build_logo_url had the same defect already fixed for staff icons — it built an absolute URL from the request, but USE_X_FORWARDED_HOST is only set under PRODUCTION_LIKE, so behind a proxy it emits the internal host and the browser blocks the image. Now site-root-relative. Safe: PDF rendering reads company.logo.path from the filesystem, not this URL. The icon tests were writing real files into the developer's own mediafiles/ tree, because settings_test.py does not override MEDIA_ROOT. That leaked an image per test run, worse than the E2E leak above. MEDIA_ROOT is now redirected to a temporary directory for that test class. Both try/except blocks in the icon view are gone. Neither reshaped the failure nor added business context — a storage error there means nothing the request path doesn't already say — so they only claimed the AppError row without contributing to it. The `if staff.icon:` guard went too: Django's FieldFile.delete() already opens with `if not self: return` and nulls the field, so the branch was duplicated logic wrapped around a no-op else. ADR 0019 gains the rule those removals follow, which it never stated: a `try` needs a strong reason, because `try` is where you handle the failure — reshape it, or persist it from the layer that understands it well enough to add business context. It scopes the existing "every except block persists" rather than contradicting it. ADR 0001's and CLAUDE.md's canonical snippets now pass job_id, so the example shows a handler with a reason to exist; the conversion example stays bare, since conversion is its own justification. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_quote_creation_stops_when_terms_are_unconfigured set xero_quote_terms to None to simulate "unconfigured", but the column is NOT NULL with a default, so the raw UPDATE hit a NotNullViolation before the guard ran. Production represents unconfigured terms as a blank string (get_xero_quote_terms strips and returns None), so use "" to exercise the configuration_error path. ADR 0015: match the data model rather than injecting a state it forbids. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The staff form narrated its own progress with console.log, and three of those lines printed user-typed secrets: form.value carries password and password_confirmation, the zod safeParse result carries both, and apiData carries password. Moving them behind the debug gate would not have fixed that — enabling the namespace would still print the secrets — so the success-path narration is deleted outright. That also satisfies frontend rule 31 by removal rather than ceremony. The three console.error calls stay: each sets error.value or raises a toast, which is what rule 30 asks for. Separately, closeSyncWindow() raised when a run had no recorded window. That is the wrong reaction to a benign state: the requested end state, no window left open for this run, already holds. Since it runs inside E2E teardown, the throw skipped the post-restore safety check, the backup and token cleanup, and the caller's lock removal — wedging the next run behind a stale lock. It now warns and returns, and the warning says the run's Xero artifacts were never covered so the poll may replay them. Deliberately not hardened further. The remaining ways that call can throw are disk-level, where the surrounding unlinkSync calls would fail anyway, and replayed test data is already caught by checkSafeToTest at the start of every run with a documented one-command fix. Both from the Copilot review on PR #498. The other two comments are answered in their threads and need no change: readBackendAppDomain() must keep returning '' because the frontend is built from a release checkout with no backend .env, and datetime.fromisoformat has accepted a trailing Z since Python 3.11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KAN-307: staff API JSON refactor + E2E logging gate, perf/reliability fixes
Cell renderers capture `line` at render time, but drafts are replaced immutably by useCostLineDrafts.updateDraft. Writes still landed (they are keyed by __localId); reads off the captured object saw pre-update values. - Stock selection now threads the replacement draft through readiness, persistence, and actual-tab consumption, and branches on the selected kind instead of the render-time kind. Description-first entry on the actual tab previously consumed no stock and left a draft stranded with id: '' that reached no server record. - Consumption is the create for actual material lines, so the local row is now discarded once the parent holds the server row. Without this the now-firing consume would leave the operator seeing the row twice. - Creates serialize per draft session, so rows append in the order the operator entered them rather than the order the server answers in. This also removes the create-vs-refresh clobber by construction: only one create, and therefore one parent refresh, is ever outstanding. - Ctrl/Cmd+Backspace routes owned drafts through deleteDraft, matching the Delete button. A draft with a committed create stays protected. - Enter falls back to the next enabled cell when the same-column target in the row below is disabled, instead of blurring and focusing nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bwLHUKBG5PNnWov73CUZE
Cost line descriptions render in a textarea, so the value is a DOM property rather than text content and a hasText filter never matches it. The new description-first assertion silently found zero rows regardless of what the app did. Count rows by textarea value, as findRowIndexByDescription does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bwLHUKBG5PNnWov73CUZE
The calls page rendered an <audio preload="metadata"> per row, so the browser
downloaded every recording on load, and the download endpoint returned no
validator, so each play re-downloaded in full. One run of the CRM E2E test
fetched 19 recordings 39 times for 2,034 KB -- 91% of the test's total bytes,
for audio nobody played.
- preload="none": nothing is fetched until the operator presses play. No UI
change; duration already comes from duration_seconds in its own column.
- ETag from the recording's stored sha256, and 304 on a matching If-None-Match,
so replaying a recording revalidates instead of re-downloading. sha256 is
cleared by delete_local_recording, so the header is only set when present --
such a recording has no file and 404s regardless.
Measured on the CRM spec: recording downloads 39 -> 0, total bytes 2,237 KB ->
202 KB. The 'linked job persists after reload' step went from a 5.1s median
(3.5s budget, 290s worst case) to 2.4s, and 'open CRM calls page' from 5.0s to
2.5s. That step waited on every audio fetch via waitForLoadState('networkidle'),
which is why this test was the least reliable in the suite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013bwLHUKBG5PNnWov73CUZE
Converting an existing time line to a stock item had no test anywhere: timeGuard covers material to time, kindInference covers adjust to material. That path is easy to get wrong because the row still holds the staff wage in unit_cost when the kind flips, so revenue derived before the stock cost lands charges markup on a labour rate that no longer applies -- 40 * 1.2 = 48 instead of 25 * 1.2 = 30, and both look plausible on screen. Verified as a real guard: restoring the old render-time `kind !== 'time'` condition makes the new test fail with unit_rev 48. Also updates the PhoneCallTable recording assertion to preload="none", which the previous commit changed without running that suite. Its intent -- a relative, same-origin src -- is unchanged; the preload line now states why eager preloading is wrong rather than recording whatever the attribute was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bwLHUKBG5PNnWov73CUZE
… paths
Three CodeRabbit findings, each verified against the code before acting:
- The 304 was returned before the file was opened, so a row whose digest
survived but whose bytes did not answered 304 to any client holding a cached
copy -- hiding a data problem from exactly the clients affected. The file is
now opened first, and a vanished file 404s even on a conditional request.
- The 304 dropped the validator it was matched on. RFC 9110 expects a
conditional response to carry its ETag; it now does.
- The item-replacement patch sent `ext_refs: { stock_id }` while the local
update merged. CostLineCreateUpdateSerializer patches a DictField, which
replaces wholesale, so re-picking an item on a line carrying delivery-receipt
or PO references dropped them server-side while the row still showed them.
The patch now merges, matching the local update.
Each is covered: the 304 asserts its ETag, a deleted-on-disk recording asserts
404 through revalidation, and the merge is verified to fail (dropping po_number)
when reverted.
Skipped: making useGridKeyboardNav test teardown failure-safe. Every test in
that file uses the same inline cleanup, so fixing one is inconsistent and
fixing all is unrelated to this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013bwLHUKBG5PNnWov73CUZE
Two further review findings. deleteDraft guards on inFlight, which becomes true synchronously, but isDraftLocked reads __status, which only flips when the draft's queued turn arrives. A draft sitting behind another create therefore rendered as deletable and the delete silently did nothing. Fixed by exposing isPersisting from the session and driving the delete control off it. Deliberately not applied to isDraftLocked as the review suggested: that gates description, quantity and rate editing, and KAN-296 requires queued drafts to stay editable. The row stays editable; only the delete is withheld, and visibly so. Also asserts the CRM page renders at least one player before asserting no recording was fetched. The restored database does carry recordings -- the pre-fix run measured 39 downloads on that test -- but without the precondition the assertion would pass vacuously on a page with none, which is worse than no test at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bwLHUKBG5PNnWov73CUZE
Several comments described what the code used to do, which reads as noise to anyone who never saw the earlier version: - The download ETag comment still pointed at "the 404 below" as the answer for a cleared digest, which stopped being true when the file open moved ahead of the conditional. - The stock revenue comment justified the absence of a guard that no longer exists, raising a question the code does not pose. - isDraftPersisting carried the same rationale as the composable's isPersisting, and again inline at the call site -- three statements of one idea. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bwLHUKBG5PNnWov73CUZE
fix(cost-entry): close draft lifecycle data-loss regressions (KAN-296)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release candidate: promotes
maintoproduction. 77 commits since the last promotion.Verification
Full E2E suite run against this exact tree (
aebe88f5, identical tomain's tip): 107 passed, 0 failed, 0 perf-fail, 16.8 min. Teardown completed normally — DB restored, Xero token reinjected, integrity and safety checks clean.origin/production's one divergent commit (the #482 merge) is already fully contained inmain, so nothing on production is lost by this merge.What ships
Cost entry (KAN-296, #499) — closes draft-lifecycle data-loss regressions: stock selection and actual-tab consumption now operate on the current draft instance rather than a stale render-time object, draft creates are serialized to preserve entry order, and queued creates are reflected in the delete control. Enter-key navigation falls back to the next enabled cell.
CRM call recordings — per-row
<audio>no longer eagerly downloads every recording on page load, and the download endpoint supports ETag revalidation (304 with the validator retained, 404 preserved for a file that outlived its digest).Staff API (KAN-307, #498) — staff API is JSON-only with icon upload separated; stops the staff form logging passwords to the console; icon removal and relative logo URLs fixed.
E2E infrastructure — production build served through ngrok (fixes a ~2.5x slowdown), E2E gets its own plain Django backend, uniform
APP_DOMAINresolution, and Xero no longer replays E2E data into the restored dev DB.Xero quotes (KAN-299, #490) — terms are sent on Xero quotes;
xero_quote_termsis non-null; an all-blank quote PDF is treated as unreadable rather than terms-absent.Ops manual (KAN-294, #489) — per-client NotebookLM training links as a managed table; Google Docs tooling salvaged to
scripts/; VitePress training manual retired.Demo/UAT seeding (KAN-297, #486) — demo
company_defaultsfixture passes theinstance.shvalidator; pandoc runs in a writable cwd sorecreate_jobfilesworks in the read-only release; guards against accidental upgrades.Error persistence —
persist_app_erroris now idempotent andAlreadyLoggedExceptionis retired; exceptions raised asCommandErrorin management commands are persisted.Housekeeping —
debuglibrary adopted as the single frontend logging gate (ADR 0031); pylint gate widened to high-signal rules; staff offboarding viadate_leftreplaces the broken hard-delete; dead/api/enums/endpoint removed; Dependabot batches (#484).🤖 Generated with Claude Code
https://claude.ai/code/session_01Qo7UkADwmj73int333bEnV