Skip to content

fix(desktop): stop folder import on cloud lookup errors#4547

Merged
Kitenite merged 4 commits into
mainfrom
qa/v2-repo-onboarding
May 17, 2026
Merged

fix(desktop): stop folder import on cloud lookup errors#4547
Kitenite merged 4 commits into
mainfrom
qa/v2-repo-onboarding

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented May 14, 2026

Summary

  • stop folder-first import when cloud lookup returns errors and there are no local candidates
  • surface the first cloud lookup error instead of creating a new local import while lookup state is indeterminate
  • keep candidate import flowing when candidates exist, even if the response also includes cloud errors

Note

Duplicate GitHub remotes are still allowed. This only blocks the no-candidate + cloud-error fallback that could create an unintended new project when existing-project discovery failed.

QA

  • bun --cwd apps/desktop test src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.test.ts
  • bun --cwd apps/desktop typecheck
  • bun run lint

Summary by CodeRabbit

  • Bug Fixes

    • Folder-first imports now stop and surface a clear cloud-specific error message when a cloud lookup returns no candidates but reports cloud errors, preventing unintended setup.
  • Tests

    • Added a test verifying folder-first import is blocked and reports cloud errors when a cloud lookup returns errors.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 14, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e1710031-0a47-4e7f-97b5-e9918c49d4eb

📥 Commits

Reviewing files that changed from the base of the PR and between abec228 and 3a8c837.

📒 Files selected for processing (1)
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.test.ts

📝 Walkthrough

Walkthrough

Adds handling in useFolderFirstImport for findByPath responses with zero candidates but non-empty cloudErrors: the hook reports the first cloud error via onError with a formatted message and returns null, skipping setup/create/finalize. Includes a test for this path.

Changes

Cloud Error Handling

Layer / File(s) Summary
Cloud error detection and reporting
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.ts, apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.test.ts
useFolderFirstImport checks for cloud errors when candidates is empty, calls onError with a "Couldn't reach cloud: — " message derived from the first cloud error, and returns null. Tests verify cloud-only error responses skip create/setup/finalizeSetup and report errors.
sequenceDiagram
  participant Hook
  participant Query
  participant ErrorCb
  participant SetupSrv
  participant CreateSrv
  participant FinalizeSrv

  Hook->>Query: findByPath.query(path)
  Query-->>Hook: response (candidates[], cloudErrors[])
  alt No candidates + cloudErrors
    Hook->>ErrorCb: onError("Couldn't reach cloud: <url> — <message>")
    Hook-->>Hook: return null
  else Candidate found
    Hook->>SetupSrv: setup(...)
    Hook->>CreateSrv: create(...)
    Hook->>FinalizeSrv: finalizeSetup(...)
  end
Loading

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 A cloud whisper on the wire,

When candidates fail to appear,
I call the error, bow out with care,
No setup now, the path stays clear. 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description clearly explains what changed and why, but does not follow the provided template structure with required sections like 'Description', 'Related Issues', 'Type of Change', 'Testing', etc. Restructure the PR description to follow the template: add 'Description' section, link related issues, check the 'Bug fix' box under 'Type of Change', and organize testing steps under 'Testing' section.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: stopping folder import when cloud lookup errors occur with no local candidates.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch qa/v2-repo-onboarding

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

@Kitenite Kitenite marked this pull request as ready for review May 15, 2026 17:16
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 15, 2026

Greptile Summary

This PR adds an early-exit guard to useFolderFirstImport so that when the cloud lookup returns errors and yields no local candidates, the hook surfaces the first error to the caller and aborts instead of silently falling through to create a new local import. Two focused tests are added to cover the new stop-on-error path and the existing continue-with-candidates path.

  • useFolderFirstImport.ts: Inserts a candidates.length === 0 && cloudErrors.length > 0 check immediately after findByPath resolves; calls onError with the first cloud error and returns null, preventing an unintended local project creation.
  • useFolderFirstImport.test.ts: New test file with two cases — stop on cloud errors with no candidates, and continue the candidate import flow when candidates exist alongside cloud errors.

Confidence Score: 4/5

The fix correctly stops the folder-import flow on cloud errors and is safe to merge.

The only issue is the first ? ... : ... ternary inside the cloudErrors.length > 0 check — since the array length is already confirmed non-zero, the fallback string is unreachable, making the code slightly misleading but not harmful.

The implementation file's new guard block is the only area worth a second look, specifically the redundant ternary on first.

Important Files Changed

Filename Overview
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.ts Adds an early-exit guard when cloud lookup returns errors and no local candidates; logic is correct but the inner ternary on first is dead code since the array length is already checked.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.test.ts New test file covering the two new cloud-error paths; baseline happy paths (no errors, single or multiple candidates) are not tested, leaving the pre-existing flow uncovered.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.ts:52-60
The `first` ternary guard is unreachable dead code. Because `response.cloudErrors.length > 0` is already asserted on the enclosing `if`, `response.cloudErrors[0]` is guaranteed to be a defined object — the fallback string `"Couldn't reach cloud"` can never execute.

Reviews (1): Last reviewed commit: "test(desktop): cover folder import cloud..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 2 files

Re-trigger cubic

@stage-review
Copy link
Copy Markdown

stage-review Bot commented May 15, 2026

Ready to review this PR? Stage has broken it down into 2 individual chapters for you:

Title
1 Block folder import on cloud lookup errors
2 Verify error handling in folder-first imports
Open in Stage

Chapters generated by Stage for commit 3a8c837 on May 17, 2026 10:48pm UTC.

@Kitenite Kitenite force-pushed the qa/v2-repo-onboarding branch from 22e8ed6 to a0477c4 Compare May 16, 2026 23:07
@capy-ai
Copy link
Copy Markdown

capy-ai Bot commented May 16, 2026

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.

@Kitenite Kitenite merged commit 0820af1 into main May 17, 2026
9 of 10 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch

Thank you for your contribution! 🎉

sazabi Bot pushed a commit that referenced this pull request May 20, 2026
* fix(desktop): stop folder import on cloud lookup errors

* test(desktop): cover folder import cloud lookup errors

* fix(desktop): remove unreachable cloud error fallback

* test(desktop): trim folder import cloud error coverage
MocA-Love pushed a commit to MocA-Love/superset that referenced this pull request May 25, 2026
…4547)

* fix(desktop): stop folder import on cloud lookup errors

* test(desktop): cover folder import cloud lookup errors

* fix(desktop): remove unreachable cloud error fallback

* test(desktop): trim folder import cloud error coverage
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.

1 participant