Skip to content

Remove three stale lint suppressions; say which DB the migrator manages - #300

Merged
Sbussiso merged 1 commit into
masterfrom
chore/stale-suppressions
Sep 13, 2026
Merged

Sbussiso merged 1 commit into
masterfrom
chore/stale-suppressions

Conversation

@Sbussiso

Copy link
Copy Markdown
Contributor

Two accuracy fixes found during the second system review. Neither changes
behaviour.

1. Three lint suppressions that suppress nothing

eslint reports all three as unused directives — the rule no longer fires
on those lines, because the dependency arrays were completed and the
eslint-disable comments were left behind.

AdminPage.jsx was a plain duplicate:

    // eslint-disable-next-line react-hooks/exhaustive-deps
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [organization?.id, hasAdminFeature])

The first applied to the second comment rather than to any code.

Why this matters more than "3 warnings": a stale exhaustive-deps
suppression silently swallows the next genuine missing dependency on that
line. That's a real bug class in effect hooks — stale closures reading old
state — and it's precisely what the rule exists to catch. The explanatory
comments above two of them ("orgId: tear down + reconnect the stream under
the new org's token") are kept; only the dead directives go.

Lint: 47 → 44 problems, still 0 errors.

2. The migrator says SQLite; it manages production Postgres

app/core/migrations.py opened with "Lightweight schema sync for SQLite",
and every caveat in it is written in SQLite terms — which reads as though it
were self-host-only machinery.

It isn't. main.py calls sync_schema(engine, Base.metadata) unconditionally
on every boot against whatever engine is configured, and there is no Alembic
in this repo
, so on Fly this module plus create_all() is the production
Postgres schema management. The header now says so, and notes where the SQLite
caveats read differently on Postgres (an ADD COLUMN with a non-volatile
default is metadata-only on PG11+, where SQLite rewrites).

Verified

  • ruff check → All checks passed
  • pytest864 passed
  • npm run lint → exit 0, 44 warnings

🤖 Generated with Claude Code

Two small accuracy fixes found while reviewing, neither changing behaviour.

1. Three `eslint-disable-next-line react-hooks/exhaustive-deps` comments
   suppress nothing. eslint reports them as unused directives, which means
   the rule no longer fires on those lines — the dependency arrays were
   completed and the suppressions left behind.

   AdminPage.jsx was a plain duplicate: two identical directives on
   consecutive lines, so the first applied to the second comment rather
   than to any code.

   These matter more than the count suggests. A stale
   exhaustive-deps suppression silently swallows the next genuine missing
   dependency on that line, which is a real bug class in effect hooks —
   exactly the kind of thing the rule exists to catch. Lint drops 47 -> 44
   problems, still 0 errors.

2. app/core/migrations.py opened with "Lightweight schema sync for
   SQLite", and all of its caveats are written in SQLite terms, which
   reads as though it were self-host-only machinery. It is not: main.py
   calls sync_schema() unconditionally on every boot against whatever
   engine is configured, so on Fly this module plus create_all() IS the
   production Postgres schema management — there is no Alembic in this
   repo. Says so now, and notes where the SQLite caveats read differently
   on Postgres.

ruff clean, 864 backend tests pass, frontend lint exits 0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Sbussiso
Sbussiso merged commit b44f7da into master Sep 13, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant