[codex] Fix v1 import modal layout#4822
Conversation
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR enhances the V1 project import workflow by introducing a new progress-tracking component and refactoring the import page to support batch importing all projects sequentially. The modal UI is updated to display step progress, add explicit cancellation, and adjust layouts for the new action flow. ChangesV1 Project Import All with Progress Tracking
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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. Comment |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit b81bf3f on May 21, 2026 10:18pm UTC. |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR fixes the v1 import modal's row layout (flex → CSS grid to prevent row shrinkage), relocates the close affordance to the footer as a "Cancel" button, and adds a footer-border step-progress indicator. It also introduces an "Import all" batch operation that sequentially imports eligible projects, skipping already-linked, ambiguous, or cloud-error rows.
Confidence Score: 3/5The layout and UI restructuring are safe, but the new Import all flow continues making project-creation/linking mutations in the background even after the user dismisses the modal. The wizard can silently create or link projects after the user has explicitly dismissed it, with no way to stop it. The importAll async loop runs to completion regardless of modal close. ImportProjectsPage.tsx — specifically the importAll function and its interaction with the Cancel button in the modal footer.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx | Adds Import all batch operation and refactors importProject into a standalone async function; the importAll loop has no cancellation guard, so closing the modal mid-run continues making API mutations in the background. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/V1ImportModal.tsx | Restructures footer: replaces the top-right close icon with an always-visible Cancel button, adds StepProgress bar, and groups Back/Next/Done together. Changes are self-contained and look correct. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportRow/ImportRow.tsx | Converts row layout from flex to a two/three-column CSS grid with a responsive sm: breakpoint, fixing row shrinkage and action alignment. Styling-only change with no logic impact. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/StepProgress/StepProgress.tsx | New accessible step-progress bar component; clamps index/step values defensively and uses correct WAI-ARIA progressbar attributes. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/StepProgress/StepProgress.test.tsx | Covers getStepProgress logic and ARIA attributes correctly, but two assertions check for specific Tailwind class strings, coupling tests to implementation details. |
Sequence Diagram
sequenceDiagram
participant U as User
participant Modal as V1ImportModal
participant Page as ImportProjectsPage
participant QC as QueryClient
participant API as Host Service API
U->>Modal: Click Import all
Modal->>Page: importAll()
Page->>Page: "setImportAllProgress({current:0, total:N})"
loop for each project i
Page->>QC: fetchQuery(findByPath)
QC->>API: project.findByPath.query(...)
API-->>QC: candidates / cloudErrors
QC-->>Page: findByPathResult
alt already imported / ambiguous / cloud-error
Page->>Page: continue (skip)
else importable
Page->>API: project.setup.mutate OR project.create.mutate
API-->>Page: result
Page->>Page: finalizeSetup(activeHostUrl)
Page->>QC: invalidateProjectImportQueries
end
Page->>Page: "setImportAllProgress({current:i, total:N})"
end
Page->>Page: setImportAllProgress(null) [finally]
Note over U,Page: If user clicks Cancel mid-loop, modal unmounts but loop continues running
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx:66-114
**`importAll` loop is not cancellable after modal close**
Clicking "Cancel" while an import is in progress calls `close()`, which unmounts the modal, but the `importAll` async loop keeps running. Every subsequent iteration will still call `client.project.setup.mutate` / `client.project.create.mutate` and then `finalizeSetup`, creating or linking projects after the user has explicitly dismissed the wizard. The `setImportAllProgress(null)` call in `finally` also attempts to update state on an unmounted component.
A common fix is a cancellation ref: set it to `true` in a cleanup effect (or on the Cancel click), and check it at the top of each loop iteration before issuing any mutations.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/StepProgress/StepProgress.test.tsx:25-27
**Test couples to Tailwind utility class names**
The assertions `expect(markup).toContain("-top-px")` and `expect(markup).toContain("h-px")` check for specific Tailwind class strings in the rendered HTML. Any future restyling of the progress bar (e.g. changing `h-px` to `h-[1px]` for consistency) would fail this test without any actual regression in behaviour. Consider asserting on the `role`, ARIA attributes, and inline `style` (which already drive correctness), rather than on CSS class tokens.
Reviews (1): Last reviewed commit: "Add v1 import step progress" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const importAll = async () => { | ||
| if (isImportingAll) return; | ||
| const queue = projects; | ||
| setImportAllProgress({ current: 0, total: queue.length }); | ||
| try { | ||
| for (let i = 0; i < queue.length; i++) { | ||
| const project = queue[i]; | ||
| if (!project) continue; | ||
| setImportAllProgress({ current: i, total: queue.length }); | ||
| try { | ||
| const findByPathResult = await fetchProjectFindByPath( | ||
| queryClient, | ||
| project, | ||
| activeHostUrl, | ||
| ); | ||
| if (isProjectAlreadyImported(findByPathResult)) { | ||
| continue; | ||
| } | ||
| if (findByPathResult.candidates.length > 1) { | ||
| continue; | ||
| } | ||
| if ( | ||
| findByPathResult.candidates.length === 0 && | ||
| findByPathResult.cloudErrors.length > 0 | ||
| ) { | ||
| continue; | ||
| } | ||
| const result = await importProject({ | ||
| project, | ||
| activeHostUrl, | ||
| findByPathResult, | ||
| finalizeSetup, | ||
| }); | ||
| if (result.kind === "imported") { | ||
| await invalidateProjectImportQueries(queryClient, project); | ||
| } | ||
| } catch (err) { | ||
| console.error("[v1-import] project import all failed", { | ||
| v1ProjectId: project.id, | ||
| mainRepoPath: project.mainRepoPath, | ||
| organizationId, | ||
| err, | ||
| }); | ||
| } | ||
| } | ||
| } finally { | ||
| setImportAllProgress(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
importAll loop is not cancellable after modal close
Clicking "Cancel" while an import is in progress calls close(), which unmounts the modal, but the importAll async loop keeps running. Every subsequent iteration will still call client.project.setup.mutate / client.project.create.mutate and then finalizeSetup, creating or linking projects after the user has explicitly dismissed the wizard. The setImportAllProgress(null) call in finally also attempts to update state on an unmounted component.
A common fix is a cancellation ref: set it to true in a cleanup effect (or on the Cancel click), and check it at the top of each loop iteration before issuing any mutations.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportProjectsPage/ImportProjectsPage.tsx
Line: 66-114
Comment:
**`importAll` loop is not cancellable after modal close**
Clicking "Cancel" while an import is in progress calls `close()`, which unmounts the modal, but the `importAll` async loop keeps running. Every subsequent iteration will still call `client.project.setup.mutate` / `client.project.create.mutate` and then `finalizeSetup`, creating or linking projects after the user has explicitly dismissed the wizard. The `setImportAllProgress(null)` call in `finally` also attempts to update state on an unmounted component.
A common fix is a cancellation ref: set it to `true` in a cleanup effect (or on the Cancel click), and check it at the top of each loop iteration before issuing any mutations.
How can I resolve this? If you propose a fix, please make it concise.| expect(markup).toContain("-top-px"); | ||
| expect(markup).toContain("h-px"); | ||
| }); |
There was a problem hiding this comment.
The assertions expect(markup).toContain("-top-px") and expect(markup).toContain("h-px") check for specific Tailwind class strings in the rendered HTML. Any future restyling of the progress bar (e.g. changing h-px to h-[1px] for consistency) would fail this test without any actual regression in behaviour. Consider asserting on the role, ARIA attributes, and inline style (which already drive correctness), rather than on CSS class tokens.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/StepProgress/StepProgress.test.tsx
Line: 25-27
Comment:
**Test couples to Tailwind utility class names**
The assertions `expect(markup).toContain("-top-px")` and `expect(markup).toContain("h-px")` check for specific Tailwind class strings in the rendered HTML. Any future restyling of the progress bar (e.g. changing `h-px` to `h-[1px]` for consistency) would fail this test without any actual regression in behaviour. Consider asserting on the `role`, ARIA attributes, and inline `style` (which already drive correctness), rather than on CSS class tokens.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Non-applicable to current fork structure: superset-sh#3960 and superset-sh#4068 require linux-arm64/full CLI dist targets that this fork does not ship; superset-sh#4678 targets a relay deploy script intentionally absent from the fork; superset-sh#4694 requires DuckDB native packaging but the fork has no DuckDB runtime dependency; superset-sh#4822 targets removed v1 import modal paths; superset-sh#4826 assumes upstream release-cli.yml while this fork uses build-cli.yml with draft release semantics.
1 participant
Summary
Why
The importer list was visually compressed and did not scroll correctly because rows could shrink inside the flex scroll container. The close button placement also left unused header padding after moving the action into the footer.
Validation
bun test ./src/renderer/routes/_authenticated/components/V1ImportModal/components/StepProgress/StepProgress.test.tsxbun run --cwd apps/desktop typecheckbun run lintNotes
bun.lockhas an unrelated local modification and was intentionally excluded from this PR.Summary by cubic
Fixes the v1 import modal layout and adds “Import all” with a step progress indicator for a smoother import experience. Rows keep a stable height, actions align predictably, and the list scrolls correctly.
New Features
Bug Fixes
ImportRowuses a grid layout to keep heights consistent.Written for commit b81bf3f. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Improvements