fix: default destination page to match source on import by tonypiper · Pull Request #4084 · bitfocus/companion

tonypiper · 2026-04-11T07:33:47Z

Summary

When importing, the destination page now defaults to match the source page number rather than always defaulting to page 1
For single-page imports: uses snapshot.oldPageNumber if valid, otherwise defaults to "new page" (-1)
For full imports: destination syncs to the selected source page, updating as the user changes source page
Renamed importPageNumber/setImportPageNumber/changeImportPage to sourcePageNumber/setSourcePageNumber/changeSourcePage for clarity

Why

The previous default of page 1 was almost always wrong — if you export page 21 and re-import it, you want to restore it to page 21, not overwrite page 1. Defaulting to "new page" when the source page doesn't exist in the destination is safer than silently overwriting page 1. The same logic applies to full imports where the destination should follow the source page selection.

Reviewer feedback addressed

Per @arikorn's review:

Extended destination-follows-source to full imports via useEffect
Renamed ambiguous importPageNumber vars to sourcePageNumber
Removed unnecessary useMemo and !isSinglePage guard from defaultDestinationPage

Test plan

Export a page that is not page 1 (e.g. page 21)
Import the file → verify destination defaults to page 21 (or "new page" if page 21 doesn't exist)
Verify the destination page can still be changed freely
Import a full config → verify destination matches source page 1
Change source page on full import → verify destination follows
Import a full config, select a source page beyond the destination page count → verify destination defaults to "new page"

🤖 Generated with Claude Code

Previously the destination page always defaulted to page 1, meaning users importing a page export (e.g. page 21) would need to manually change the destination, risking accidentally overwriting the wrong page. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

CLAassistant · 2026-04-11T07:33:55Z

All committers have signed the CLA.

coderabbitai · 2026-04-11T07:34:10Z

📝 Walkthrough

Walkthrough

The import flow now distinguishes source vs destination pages: state renamed to sourcePageNumber, destination default is computed from snapshot.oldPageNumber (if valid) or validated sourcePageNumber/-1, a useEffect syncs destination to source for multi-page imports, and doImport's first parameter was renamed to sourcePageNumber.

Changes

Cohort / File(s)	Summary
Import page flow & API `webui/src/ImportExport/Import/Page.tsx`	Renamed `importPageNumber` → `sourcePageNumber`; updated `doImport` signature to `(sourcePageNumber, pageNumber, connectionRemap, ...)`; bound source PageNumberPicker and source grid to `sourcePageNumber`; destination picker now uses a computed `defaultDestinationPage` (uses `snapshot.oldPageNumber` when integer in range, else falls back to validated `sourcePageNumber` or `-1`); added useEffect to set destination to source for multi-page imports.

Poem

Two pages now know which is which,
Source named clear, destination stitched.
Defaults that check memory's sign,
Sync on change to keep things fine.
Thanks for the tidy, thoughtful switch!

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fixing the default destination page behavior to match the source page during imports, which is the core objective addressed in the PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

Updates the Import/Export single-page import wizard so the destination page defaults to the original exported page number (via snapshot.oldPageNumber) instead of always defaulting to page 1, improving the “export page N, re-import page N” workflow.

Changes:

Compute a defaultDestinationPage for single-page imports using snapshot.oldPageNumber ?? 1.
Initialize the destination page picker with that computed default (instead of always 1).
Keep existing behavior for full config imports (Buttons tab) by continuing to initialize at page 1.

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

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0b1623e8-deae-40cd-9c62-32b47ee856b6

📥 Commits

Reviewing files that changed from the base of the PR and between 9d843b3 and 3aec109.

📒 Files selected for processing (1)

webui/src/ImportExport/Import/Page.tsx

If snapshot.oldPageNumber is out of range for the destination system, default to new page (-1) rather than an invalid page number. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

arikorn · 2026-04-11T18:33:45Z

+		if (typeof oldPage === 'number' && Number.isInteger(oldPage) && oldPage >= 1 && oldPage <= pages.data.length) {
+			return oldPage
+		}
+		return -1


Nice suggestion @tonypiper ! Some comments

This will default to a new page (-1) rather than the first page (1), as was the pre-existing behavior. Is that your intention? This actually makes sense when oldPage is beyond pages.data.length, but otherwise it should probably default to 1.

useMemo here seems a bit of overkill: you're just doing a couple of logical tests and all it's doing is setting a default, so it's a no-op if any of the dependents change, anyway. Suggestion: either it should be done only on mount or just take it out of a useMemo.

Technically the !isSinglePage test isn't necessary, since full won't have an oldPageNumber, and really, if there is an oldPageNumber it should probably be used.

Thank you for the quick and helpful feedback. Defaulting to a new page was intentional - original motivation was that I have important buttons on page 1 and they kept getting overwritten. It seems to me that it's safer to create a new page (and then move it manually if need be) than overwrite an existing page if we can't be sure that it's the right page to overwrite.

arikorn · 2026-04-11T18:53:23Z

@tonypiper two general comments:

It seems to me that your suggestion would be just as useful on "full" imports as well (either way it only affects the "Page" tab). You just need to add a call to setPageNumber to set it either to importPageNumber or -1 immediately after your usePagePicker line. So if the user chooses page 3 for the source page, it would change destination page to the same (or -1 if beyond the end).
As long as you're touching this file, I'd recommend changing importPageNumber/setImportPageNumber/changeImportPage to use "sourcePage" for clarity, since "import" is ambiguous (is it the page you're importing from or importing to?!). In the source code just refactor using F2 (if using VS Code) on lines 71-73 of the original file. (maybe 70-72 now?). Or ask Claude.

Address PR review feedback: - Rename importPageNumber/setImportPageNumber/changeImportPage to sourcePageNumber/setSourcePageNumber/changeSourcePage for clarity - Remove useMemo and isSinglePage guard from defaultDestinationPage - Sync destination page to follow source page changes on full imports - Default to new page (-1) when source page doesn't exist in destination Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 05f63e74-23d3-4ae5-ad7f-dd22b01f5701

📥 Commits

Reviewing files that changed from the base of the PR and between 874e6f9 and c01c9e5.

📒 Files selected for processing (1)

webui/src/ImportExport/Import/Page.tsx

arikorn · 2026-04-13T19:49:22Z

+
+	useEffect(() => {
+		if (isSinglePage) return
+		setPageNumber(pageOrNew(sourcePageNumber))


It's looking much better now, @tonypiper. One comment here: useEffect here is in inefficient since it adds a render cycle and it somewhat hides the intent, which is that it's specifically in response to a user action. Better to wrap setSourcePageNumber and changeSourcePage in useCallbacks for use by lines 136-137, which set both source and dest. (yeah, sorry my original suggestion wasn't quite right) Also, again, you don't need to protect for isSinglePage.

Another thing, which is mainly for clarity: instead of usePagePicker(pageCount, 1) online 73, determine validOldPage first and set the default page to Math.abs(validOldPage) or if that horrifies you validOldPage === -1 ? 1 : validOldPage, then you can get rid of defaultDestinationPage and just use sourcePageNumber in the second usePagePicker, which makes both of the calls clearer (to me).

Anyway, think about that second suggestion and question it, if you disagree!

Julusian · 2026-04-15T21:27:10Z

I'm not 100% convinced by some of the behaviour here. While the auto page changing will be useful in some cases, in others it will be very frustrating.

eg, if you want to import 1,2,3 to 15,16,17, then it requires jumping all the way through the list of pages after each one instead of simply hitting next on both.

sometimes I pick the destination page first, before the source page too.

Maybe this is solvable by having the destination page picker track that the user has changed it manually, and stop auto-adjusting when that happens? might need a way to indicate that and put it back to auto, not sure where that should go though

Copilot AI review requested due to automatic review settings April 11, 2026 07:33

Copilot started reviewing on behalf of tonypiper April 11, 2026 07:34 View session

Copilot AI reviewed Apr 11, 2026

View reviewed changes

Comment thread webui/src/ImportExport/Import/Page.tsx Outdated

coderabbitai Bot reviewed Apr 11, 2026

View reviewed changes

Comment thread webui/src/ImportExport/Import/Page.tsx Outdated

fix: clamp default destination page to valid range

874e6f9

If snapshot.oldPageNumber is out of range for the destination system, default to new page (-1) rather than an invalid page number. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

arikorn reviewed Apr 11, 2026

View reviewed changes

coderabbitai Bot reviewed Apr 13, 2026

View reviewed changes

Comment thread webui/src/ImportExport/Import/Page.tsx

tonypiper changed the title ~~fix: default destination page to match source on single page import~~ fix: default destination page to match source on import Apr 13, 2026

arikorn reviewed Apr 13, 2026

View reviewed changes

Uh oh!

Conversation

tonypiper commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Reviewer feedback addressed

Test plan

Uh oh!

CLAassistant commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Poem

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arikorn Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

tonypiper Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

arikorn commented Apr 11, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arikorn Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Julusian commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tonypiper commented Apr 11, 2026 •

edited

Loading

CLAassistant commented Apr 11, 2026 •

edited

Loading

coderabbitai Bot commented Apr 11, 2026 •

edited

Loading