Connections refactor, phase C (12/13): rename Configuration atoms to Connection by mjuarros · Pull Request #2361 · aws/graph-explorer

mjuarros · 2026-10-05T22:27:52Z

Part of the Phase C work on #2299 (consolidate Connection logic, epic #2296). Follows #2357 (PR 11).

What this does

Renames the four legacy Configuration* Jotai atoms to Connection* names. Identifier renames only — no behavior changes. The storage-key string literals passed to the atoms ("configuration", "active-configuration") are byte-identical; only the TypeScript names change.

Atom renames:

configurationAtom → savedConnectionsAtom
activeConfigurationAtom → activeConnectionIdAtom
activeConfigSelector → activeSavedConnectionSelector
createActiveConfigurationAtom → createActiveConnectionIdAtom

Review follow-ups from #2357 folded in

@kmcginnes left a nit list on #2299 after approving PR 11. The mechanical ones are addressed here (no behavior change), so there's no need to hand over a separate commit for them:

saveConnectionToFile converted from an arrow to a named export default function
docs/agents/testing.md reworded ("stored configuration" → saved-connection wording) and its preload-helper return list corrected to include the renamed active-connection atom
Leftover config-style names renamed to connection forms: the PreloadedConfigurationAtoms interface, DbState.#inactiveConfigs, and the reviewer's named locals/params/handlers/props/test-helpers/comments across legacyConnection.ts, useDeleteConnection.ts, CreateConnection.tsx, typeConfigs.ts, connectionLink.ts, connectionFormModel.ts, ConnectionRow.tsx, ConnectionDetail/*, and the named connection test files

Deliberately deferred to PR 13

The "brand an existing id" creator function and removal of the as ConnectionId casts (defaultConnection.ts, parseConnectionFile.ts, activeConnectionStorage.ts) — pairs with the interface-enforcement work.
The oxlint no-restricted-imports rule itself.
A handful of further config-named locals that sit outside the reviewer's explicit list and are more ambiguous (activeConfigId in query code / UserPrefixes.tsx / edgeConnectionsQuery.ts, and configId/setActiveConfig in AppStatusLoader.tsx / useActivateConnection.ts). Left for a later sweep to keep this PR bounded. Note: AppStatusLoader.tsx's "Reading configuration..." is user-facing copy, not a code name.

Kept by design (unchanged): ConfigurationContextProps, MergedConfiguration, useConfiguration, ConnectionWithId, ConnectionConfig, and the existingConfig prop (typed ConfigurationContextProps).

Testing

pnpm checks — green
pnpm test — green (3290 tests)
Phase A pinning/golden tests (storedConnectionShapes, connectionFileGoldenFiles, persistence) pass unchanged; the "active-configuration" storage-key pin was verified by mutation (break the literal → pin goes red → restore). No __fixtures__ touched.

kmcginnes

Renames look complete and the storage keys are untouched. One thing to fix before merge, plus a few nits inline.

Fix: GLOSSARY.md:81 (Exported Connection File) still says "the connection lands in configurationAtom". That atom no longer exists, so it should say savedConnectionsAtom.

Nits:

GLOSSARY.md:23: the Active Connection Avoid note cites activeConfigurationAtom as a "legacy code term", but it's gone from the code now. Either drop it or call it the former name.
useActivateConnection.ts (configId) and edgeConnectionsQuery.ts (activeConfigId) are already edited in this diff, so finishing those renames here would be cheap rather than deferring them.
Optional: about 40 added test lines still use a local named config (e.g. new Map([[config.id, config]])).

kmcginnes · 2026-10-06T01:23:42Z


 import { normalizeConnection } from "./normalizeConnection";

 /** Gets the currently active config. */


This JSDoc still says "config" and doesn't match the new name. Maybe "Gets the active saved connection, or null."

Done in f7d8ed7 — reworded to /** Gets the active saved connection, or null. */, matching your suggestion.

kmcginnes · 2026-10-06T01:23:42Z

+        set(savedConnectionsAtom, prevConfig => {
          const updatedConfig = new Map(prevConfig);


prevConfig / updatedConfig were missed. The same pattern in useDeleteConnection.ts and CreateConnection.tsx became prevConnections / updatedConnections.

Fixed in f7d8ed7 — renamed to prevConnections / updatedConnections, matching the useDeleteConnection.ts and CreateConnection.tsx pattern.

kmcginnes · 2026-10-06T01:23:42Z

+      const allConfigs = useAtomValue(savedConnectionsAtom);
+      const activeConfig = useAtomValue(activeConnectionIdAtom);


allConfigs / activeConfig are still here, even though the PR body lists this file as cleaned up.

Good catch — the PR body overclaimed. Fixed in f7d8ed7: allConfigs → allConnections and activeConfig → activeConnectionId (it holds the active connection id), with all references updated. The "cleaned up" claim is now accurate.

kmcginnes · 2026-10-06T01:23:42Z

+    [...connections].map(([id, connection]) => [
      id,
-      config.connection
+      connection.connection


Renaming to connection gives us connection.connection. Naming the outer SavedConnection value savedConnection would read better. Same in connectionLink.ts.

Fixed in f7d8ed7 — the outer SavedConnection value is now savedConnection, so it reads savedConnection.connection with no stutter.

kmcginnes · 2026-10-06T01:23:42Z

+      connection.connection != null &&
+      identitiesMatch(identityOf(connection.connection), proposedIdentity),


Same connection.connection stutter as in legacyConnection.ts. savedConnection => would read better.

Fixed in f7d8ed7 — the .filter callback that dereferenced .connection is now savedConnection. Followed up in d84ccab by renaming the two .find callbacks below (activeMatch / nameMatch) to savedConnection as well, so findMatchingConnection uses one consistent name throughout.

…nfig→connection renames Resolves kmcginnes's review comments on PR aws#2361 (phase C part 12). Pure refactor: identifier, comment, and doc edits only — no behavior change, storage-key literals byte-identical, no __fixtures__ edits. - GLOSSARY: Exported Connection File entry referenced the removed configurationAtom; now points at savedConnectionsAtom. Active Connection Avoid note reworded to mark activeConfigurationAtom as the former name (now activeConnectionIdAtom). - activeConnection.ts: JSDoc on activeSavedConnectionSelector no longer says "config". - useActivateConnection.ts: callback param configId → connectionId. - edgeConnectionsQuery.ts: local activeConfigId → activeConnectionId. - useImportConnectionFile.ts: prevConfig/updatedConfig → prevConnections/ updatedConnections, matching useDeleteConnection and CreateConnection. - useDeleteConnection.test.ts: locals config1 → connection1, allConfigs → allConnections, activeConfig → activeConnectionId (+ assertions). - legacyConnection.ts and connectionLink.ts: resolved the connection.connection stutter by renaming the outer SavedConnection value to savedConnection. - Test-local config sweep: useImportConnectionFile.test.tsx file-shaped locals (valid/direct/invalid/legacyConfig → *ConnectionFile, importedConfig → importedConnection), parseConnectionFile.test.ts validConfig → validConnectionFile, CreateConnection.test.tsx configId → connectionId. Verified: pnpm check:types, pnpm checks (lint+format+types) and pnpm test all green — 3290 tests pass across all 248 test files. Kept-by-design symbols (ConfigurationContextProps, mergeConfiguration, existingConfig prop) and PR-13 deferred items (brand-existing-id creator, as ConnectionId casts, oxlint rule) left untouched.

mjuarros · 2026-10-06T16:15:33Z

Thanks for the review. All of it is addressed — replies are inline on each thread. Summary of the two GLOSSARY.md items from the review body:

Must-fix — GLOSSARY.md:81: the Exported Connection File entry now says the connection lands in savedConnectionsAtom (was the removed configurationAtom). Fixed in f7d8ed77.
Nit — GLOSSARY.md:23: the Active Connection Avoid note now reads "formerly the code atom activeConfigurationAtom, now activeConnectionIdAtom" rather than citing it as a current legacy term. Fixed in f7d8ed77.

I grepped the tree to confirm zero remaining live code references to either configurationAtom or activeConfigurationAtom; the only mentions left are the "formerly" prose in GLOSSARY.md:23.

I also took the optional test-local sweep — the ~40 config-named locals (config → connection, configMap → connectionMap, etc.) are renamed too, in f7d8ed77, with a scope guard that left the inner .connection (ConnectionConfig) field, storage-key literals, and the kept-by-design symbols untouched.

Follow-up commits: f7d8ed77 (all review items) and d84ccab6 (the findMatchingConnection naming-consistency follow-up). Storage keys stay byte-identical and the Phase A golden/pinning tests pass unchanged. The brand-an-existing-id creator + as ConnectionId cast removal remain deferred to PR 13 as planned.

kmcginnes

Thanks, everything from my review is addressed. I'll rebase onto main and fix the conflict before merging.

…Connection and clean up leftover config names Renames the four persisted-connection Jotai atom identifiers tree-wide (production + tests), leaving every storage-key string literal unchanged: - configurationAtom -> savedConnectionsAtom - activeConfigurationAtom -> activeConnectionIdAtom - activeConfigSelector -> activeSavedConnectionSelector - createActiveConfigurationAtom -> createActiveConnectionIdAtom The "configuration" and "active-configuration" localForage/session keys passed into the atom creators are byte-identical, so stored data and the Phase A pinning/golden tests are unaffected. Also folds in the mechanical config->connection follow-ups from the PR 11 review (no behavior change): - saveConnectionToFile: arrow default export -> named default function. - docs/agents/testing.md: reworded the preload-helper lines to the current terms and corrected the return list to include activeConnectionIdAtom. - Renamed leftover config-style locals/params/fields/props/handlers/ test-helpers/comments that refer to a SavedConnection or ConnectionId: PreloadedConfigurationAtoms -> PreloadedConnectionAtoms; DbState #inactiveConfigs -> #inactiveConnections; locals in legacyConnection.ts, useDeleteConnection.ts, CreateConnection.tsx, typeConfigs.ts, connectionLink.ts, connectionFormModel.ts; the setActiveConfig handler in ConnectionRow.tsx; deleteActiveConfig/onConfigExport in ConnectionDetail; configMap/configWithLegacyConnection test helpers and config locals in the named connection test files; config comments in randomData.ts, types.ts, normalizeConnection.ts. Left out of scope per the issue: the "brand an existing id" creator and the as ConnectionId casts (PR 13), the no-restricted-imports rule (PR 13), and the genuinely-bundled Configuration symbols (MergedConfiguration, ConfigurationContextProps, useConfiguration, existingConfig prop). Verification (all from repo root, prefixed COREPACK_ENABLE_DOWNLOAD_PROMPT=0): - pnpm check:types -> Done, no errors (all 4 workspace projects) - pnpm checks -> lint clean, format clean (all matched files correct), types Done - pnpm test -> 248 files, 3290 tests passed - pnpm test storedConnectionShapes connectionFileGoldenFiles persistence.test -> 3 files, 28 tests passed (Phase A pinning/golden tests unchanged)

…its renamed sibling field

…nfig→connection renames Resolves kmcginnes's review comments on PR aws#2361 (phase C part 12). Pure refactor: identifier, comment, and doc edits only — no behavior change, storage-key literals byte-identical, no __fixtures__ edits. - GLOSSARY: Exported Connection File entry referenced the removed configurationAtom; now points at savedConnectionsAtom. Active Connection Avoid note reworded to mark activeConfigurationAtom as the former name (now activeConnectionIdAtom). - activeConnection.ts: JSDoc on activeSavedConnectionSelector no longer says "config". - useActivateConnection.ts: callback param configId → connectionId. - edgeConnectionsQuery.ts: local activeConfigId → activeConnectionId. - useImportConnectionFile.ts: prevConfig/updatedConfig → prevConnections/ updatedConnections, matching useDeleteConnection and CreateConnection. - useDeleteConnection.test.ts: locals config1 → connection1, allConfigs → allConnections, activeConfig → activeConnectionId (+ assertions). - legacyConnection.ts and connectionLink.ts: resolved the connection.connection stutter by renaming the outer SavedConnection value to savedConnection. - Test-local config sweep: useImportConnectionFile.test.tsx file-shaped locals (valid/direct/invalid/legacyConfig → *ConnectionFile, importedConfig → importedConnection), parseConnectionFile.test.ts validConfig → validConnectionFile, CreateConnection.test.tsx configId → connectionId. Verified: pnpm check:types, pnpm checks (lint+format+types) and pnpm test all green — 3290 tests pass across all 248 test files. Kept-by-design symbols (ConfigurationContextProps, mergeConfiguration, existingConfig prop) and PR-13 deferred items (brand-existing-id creator, as ConnectionId casts, oxlint rule) left untouched.

…allbacks

kmcginnes

Rebased onto main and resolved the conflict in CreateConnection.test.tsx with the new renderEditing helper. Checks and tests pass locally.

mjuarros marked this pull request as ready for review October 5, 2026 22:39

kmcginnes reviewed Oct 6, 2026

View reviewed changes

kmcginnes previously approved these changes Oct 6, 2026

View reviewed changes

mjuarros added 4 commits October 6, 2026 16:04

Rename DbState.activeConfig to activeConnection for consistency with …

cf4c8c8

…its renamed sibling field

Use consistent savedConnection naming across findMatchingConnection c…

c79bda2

…allbacks

kmcginnes dismissed their stale review via c79bda2 October 6, 2026 21:14

kmcginnes force-pushed the connections-phase-c-rename-atoms branch from d84ccab to c79bda2 Compare October 6, 2026 21:14

kmcginnes approved these changes Oct 6, 2026

View reviewed changes

kmcginnes merged commit fccdf84 into aws:main Oct 6, 2026
3 checks passed

mjuarros mentioned this pull request Oct 7, 2026

Connections refactor, phase C (13/13): enforce the module interface #2383

Open


		import { normalizeConnection } from "./normalizeConnection";

		/** Gets the currently active config. */

		set(savedConnectionsAtom, prevConfig => {
		const updatedConfig = new Map(prevConfig);

		const allConfigs = useAtomValue(savedConnectionsAtom);
		const activeConfig = useAtomValue(activeConnectionIdAtom);

		connection.connection != null &&
		identitiesMatch(identityOf(connection.connection), proposedIdentity),

Conversation

mjuarros commented Oct 5, 2026

What this does

Review follow-ups from #2357 folded in

Deliberately deferred to PR 13

Testing

Uh oh!

kmcginnes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjuarros commented Oct 6, 2026

Uh oh!

kmcginnes left a comment

Choose a reason for hiding this comment

Uh oh!

kmcginnes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants