Skip to content

refactor: Switch from localStorage to sessionStorage and clean up on navigation#5

Open
thejuggernaut01 wants to merge 1 commit intoJC-Coder:mainfrom
thejuggernaut01:fix/data-store-change
Open

refactor: Switch from localStorage to sessionStorage and clean up on navigation#5
thejuggernaut01 wants to merge 1 commit intoJC-Coder:mainfrom
thejuggernaut01:fix/data-store-change

Conversation

@thejuggernaut01
Copy link

Problem

Migration configurations (migration_${jobId}) were stored in localStorage, meaning they persisted across browser sessions indefinitely — even after the tab or browser was closed. This is unnecessary since migration configs are only needed during the active session. Additionally, draft connection strings (db_mover_draft_${dbType}) and migration configs were never cleaned up when users navigated away from the page, leaving stale sensitive data in storage.

Changes

client/src/pages/ConfigPage.tsx

  • Switched localStorage.setItemsessionStorage.setItem for storing migration configs (migration_${jobId}), so data is automatically cleared when the tab is closed.
  • Added a useEffect cleanup that removes db_mover_draft_${dbType} from session storage when the user navigates away from the config page. This handles the case where a user enters connection strings (triggering draft auto-save) but navigates elsewhere without starting a migration or manually clearing the draft.

client/src/pages/MigrationPage.tsx

  • Switched localStorage.getItem / localStorage.setItemsessionStorage for reading and storing migration configs during retry.
  • Added session storage cleanup to the existing SSE useEffect cleanup function. When a user navigates away from the migration page:
    • Reads the dbType from the stored migration_${jobId} config
    • Removes the corresponding db_mover_draft_${dbType} key
    • Removes the migration_${jobId} key itself

Why sessionStorage over localStorage

  • Automatic browser cleanup: Session storage is cleared when the tab closes, so users don't need to worry about stale connection strings lingering.
  • Scoped to the session: Migration configs are only relevant for the current session — there's no need to persist them across browser restarts.
  • Combined with navigation cleanup: The useEffect cleanups handle the remaining case where data persists during in-app navigation within the same tab.

…nfigurations and implement cleanup on component unmount.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the client-side migration flow to store migration configuration and draft connection strings in sessionStorage (instead of localStorage) and adds cleanup on route transitions to reduce persistence of sensitive data.

Changes:

  • Switch migration config persistence (migration_${jobId}) from localStorage to sessionStorage for start + retry flows.
  • Add navigation/unmount cleanup to remove db_mover_draft_${dbType} (Config page) and both draft + migration config (Migration page).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
client/src/pages/ConfigPage.tsx Writes migration config to sessionStorage and removes draft data on unmount/navigation.
client/src/pages/MigrationPage.tsx Reads/writes retry config via sessionStorage and removes draft + migration config on unmount/navigation.
Comments suppressed due to low confidence (1)

client/src/pages/ConfigPage.tsx:44

  • sessionStorage.setItem() can throw (quota exceeded / storage blocked). Because this is inside the same try as the API call, a storage write failure would route into the catch path and report “Migration failed” even though the migration job may already have been started successfully. Consider guarding the storage write with its own try/catch and treating failures as a non-fatal loss of retry capability (e.g. log + toast warning, but still navigate).
      // Store migration config for retry functionality
      sessionStorage.setItem(
        `migration_${jobId}`,
        JSON.stringify({
          type: "copy",
          sourceUri: config.sourceUri,
          targetUri: config.targetUri,
          dbType: dbType,
        }),
      );

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +90 to +95
const storedConfig = sessionStorage.getItem(`migration_${jobId}`);
if (storedConfig) {
const { dbType: storedDbType } = JSON.parse(storedConfig);
sessionStorage.removeItem(`db_mover_draft_${storedDbType}`);
}
sessionStorage.removeItem(`migration_${jobId}`);
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup unconditionally JSON.parse()s the stored config. If the storage entry is missing/corrupted (or sessionStorage is unavailable and throws), this can throw during effect cleanup and interrupt navigation/unmount. Wrap the storage access + JSON.parse in a try/catch, and only remove the draft key when the parsed dbType is a non-empty string.

Suggested change
const storedConfig = sessionStorage.getItem(`migration_${jobId}`);
if (storedConfig) {
const { dbType: storedDbType } = JSON.parse(storedConfig);
sessionStorage.removeItem(`db_mover_draft_${storedDbType}`);
}
sessionStorage.removeItem(`migration_${jobId}`);
try {
const storedConfig = sessionStorage.getItem(`migration_${jobId}`);
if (storedConfig) {
const parsedConfig = JSON.parse(storedConfig);
const storedDbType =
typeof parsedConfig?.dbType === "string"
? parsedConfig.dbType.trim()
: "";
if (storedDbType) {
sessionStorage.removeItem(
`db_mover_draft_${storedDbType}`,
);
}
}
sessionStorage.removeItem(`migration_${jobId}`);
} catch (storageError) {
console.error("Error cleaning up migration storage", storageError);
}

Copilot uses AI. Check for mistakes.
useEffect(() => {
return () => {
if (dbType) {
sessionStorage.removeItem(`db_mover_draft_${dbType}`);
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sessionStorage.removeItem() can throw in some environments (e.g. storage blocked), and elsewhere in the codebase storage access is guarded with try/catch. Consider wrapping this cleanup in try/catch to avoid errors during unmount/navigation; also consider centralizing the draft key format to avoid divergence from DatabaseConfigForm’s DRAFT_KEY helper.

Suggested change
sessionStorage.removeItem(`db_mover_draft_${dbType}`);
try {
sessionStorage.removeItem(`db_mover_draft_${dbType}`);
} catch {
// Ignore storage access errors (e.g. when storage is blocked)
}

Copilot uses AI. Check for mistakes.
Comment on lines 38 to 44
// Start new migration with same config
const res = await api.post("/migrate/start", config);
const newJobId = res.data.jobId;

// Store config for new job
localStorage.setItem(`migration_${newJobId}`, JSON.stringify(config));
sessionStorage.setItem(`migration_${newJobId}`, JSON.stringify(config));

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sessionStorage.setItem() can throw (quota exceeded / storage blocked). If that happens here, the catch will run and show a retry failure even though the new migration job may already have been started successfully. Consider isolating the storage write in its own try/catch and continuing navigation even if persisting retry config fails (optionally toast a warning about retry being unavailable).

Copilot uses AI. Check for mistakes.
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.

2 participants