Repository navigation
Conversation
…ith no-restricted-imports and add a ConnectionId brander Final PR of phase C. Pure refactor, no behavior change: storage-key string literals are byte-identical, no __fixtures__ edits, golden/pinning tests pass unchanged. Part A — ConnectionId brander + remove `as ConnectionId` casts - `createConnectionId(id: string)` now brands an existing string, mirroring `createVertexId(id)`/`createEdgeId(id)` in core/entities/entityIdType.ts (the established convention: a function that takes the raw value and returns the branded type, with the lone internal cast living inside the creator). - `createNewConnectionId()` generates a fresh UUID, mirroring the existing `createNewConnectionForm` "New" naming for value generation. - Removed the 5 known `as ConnectionId` casts on existing strings (defaultConnection.ts x2, parseConnectionFile.ts x1, activeConnectionStorage.ts x2) plus 3 in tests (connectionFileGoldenFiles.test.ts x2, AppStatusLoader.test.tsx x1), all now via the brander. The only remaining cast is inside the brander itself. - Updated generation call sites to `createNewConnectionId()`. Part B — oxlint no-restricted-imports interface enforcement (issue aws#2299 item 13) Two scoped overrides in .oxlintrc.json: 1. Outside src/connections/: block deep imports `@/connections/*` (barrel `@/connections` stays allowed). storageAtoms.ts is exempted via `excludeFiles` so its sanctioned `@/connections/legacyConnection` deep import keeps working. 2. Inside src/connections/: block the bare `@/core` barrel; specific core paths (e.g. `@/core/StateProvider`) stay allowed. Probe results (throwaway files, all reverted; final tree has only the rule): (a) outside file importing `@/connections/defaultConnection` -> ERROR (fires) (b) storageAtoms.ts deep import of `@/connections/legacyConnection` -> allowed (c) outside file importing barrel `@/connections` -> allowed (d) inside file importing bare `@/core` -> ERROR (fires); inside file importing `@/core/StateProvider` -> allowed The rule surfaced one real violation: AppStatusLoader.test.tsx deep-imported `@/connections/defaultConnection` to spy on `fetchDefaultConnection`. Fixed to import the barrel namespace `* as connections` and spy on `connections.fetchDefaultConnection` (6 AppStatusLoader tests pass). Part C — final sweep of deferred config-named locals Renamed locals/params that hold a SavedConnection or ConnectionId: - UserPrefixes.tsx: `activeConfigId`->`activeConnectionId`, `configId`->`connectionId` - AppStatusLoader.tsx: `setActiveConfig`->`setActiveConnectionId` - connector/queries/schemaSyncQuery.test.ts: `activeConfigId`->`activeConnectionId` - Connect.test.tsx: `inactiveConfig`->`inactiveConnection` Left untouched per scope guard: user-facing copy ("Reading configuration...", etc.), the inner `.connection` field, storage-key literals, and kept-by-design symbols (useConfiguration/MergedConfiguration, type-config locals). Verification (run from repo root): - `pnpm check:types` -> green - `pnpm check:lint` -> clean on this tree (verified via oxlint with nested config disabled; the bare `pnpm check:lint` currently errors only on an unrelated stray `.worktrees/pr-2145-review/.oxlintrc.json` left by another task, which is untracked and not part of this commit) - `pnpm check:format` -> clean - `pnpm test` -> 251 files, 3312 tests passed
mjuarros
marked this pull request as ready for review
October 8, 2026 00:21
This branch has not been deployed
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
1 participant
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.
Final PR of Phase C on #2299, completing the Connections consolidation epic #2296. Follows #2357 (PR 11) and #2361 (PR 12).
What this does
Three parts, all pure refactor — no behavior change, storage-key literals byte-identical, Phase A golden/pinning tests pass unchanged.
Enforce the module interface (the headline)
Adds two scoped oxlint
no-restricted-importsrules in.oxlintrc.json:src/connections/: deep imports (@/connections/*) are blocked; the barrel@/connectionsstays allowed.core/StateProvider/storageAtoms.tsis exempt — it keeps its one sanctioned deep import of@/connections/legacyConnection(the module's top-level-await atom creation requires it; documented at the import site).src/connections/: importing the@/corebarrel is blocked; specific core paths (@/core/StateProvider, etc.) stay allowed.Both directions were verified to actually fire and allow:
@/connections/*import outside the module → errorsstorageAtoms.tsdeep import → allowed (exemption works)@/connectionsoutside the module → allowed@/coreinside the module → errors;@/core/StateProviderinside the module → allowedAdd a
ConnectionIdbrander + remove castscreateConnectionId()only generates new ids, so code receiving an existing id string was casting withas ConnectionId. Added a brander matching the repo'screateVertexId/createEdgeIdconvention (the only cast now lives inside the creator) and removed the 5as ConnectionIdcasts indefaultConnection.ts,parseConnectionFile.ts, andactiveConnectionStorage.ts. Noas ConnectionIdcast on a plain string remains.Final
config-local sweepRenamed the remaining deferred
config-named locals that hold aSavedConnection/ConnectionId(activeConfigId,configId, etc. inedgeConnectionsQuery/schemaSyncQuery.test/UserPrefixes/AppStatusLoader/CreateConnection/Connect.test). Scope guard held: the inner.connection(ConnectionConfig) field, storage keys, kept-by-design symbols, and the user-facing "Reading configuration..." copy are untouched.Testing
pnpm checks— green (lint/format/types)pnpm test— green (3312 tests)__fixtures__touched.Epic status
With this merged, epic #2296 is complete: all Connection code lives in
src/connections/, the legacyConfiguration*names are gone, and the module interface is enforced by lint.