Skip to content

fix: don't error remote database status command if database was not enabled yet to provide more meaningful output#8218

Open
pieh wants to merge 4 commits intomainfrom
fix/database-status-branch-not-yet-created
Open

fix: don't error remote database status command if database was not enabled yet to provide more meaningful output#8218
pieh wants to merge 4 commits intomainfrom
fix/database-status-branch-not-yet-created

Conversation

@pieh
Copy link
Copy Markdown
Contributor

@pieh pieh commented Apr 28, 2026

🎉 Thanks for submitting a pull request! 🎉

Summary


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f313c6b9-9794-4261-924b-f37d70de1e1f

📥 Commits

Reviewing files that changed from the base of the PR and between ae8c1fc and 4ea63a0.

📒 Files selected for processing (1)
  • tests/unit/commands/database/db-status.test.ts

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Database status checks now skip remote operations when the database feature is disabled, preventing unnecessary requests and avoided branch connection attempts; status reporting proceeds and shows zero applied/pending migrations.
  • Tests
    • Added a unit test to verify the disabled-database path (no remote fetches and appropriate log output).

Walkthrough

The pull request updates the database status command's remote --branch logic to respect a computed enabled state: when enabled is false the code no longer fetches the branch connection string or remote applied migrations and instead uses an empty applied-migrations result so computeStatus can proceed without remote requests. A unit test is added that verifies, with Netlify Database disabled (siteDatabase: null), no remote branch or migration endpoints are called, no DB connection is established, and logs report the database is not enabled with zero applied/pending migrations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing the remote database status command to not error when the database hasn't been enabled yet, matching the code changes that gate remote fetching on the enabled state.
Description check ✅ Passed The description is related to the changeset as it references the pull request context and includes a contributor checklist, though it is primarily a template rather than a detailed explanation of the specific changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 fix/database-status-branch-not-yet-created

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.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 28, 2026

📊 Benchmark results

Comparing with 3ececcd

  • Dependency count: 1,061 (no change)
  • Package size: 355 MB (no change)
  • Number of ts-expect-error directives: 356 (no change)

@pieh pieh marked this pull request as ready for review April 28, 2026 08:24
@pieh pieh requested a review from a team as a code owner April 28, 2026 08:24
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/database/db-status.ts`:
- Around line 418-421: Remove the explanatory inline comments and make the
intent explicit in code: replace the commented assignment with a clearly named
noop function (e.g., const noopFetchApplied = async () => []; ) and assign
fetchApplied = noopFetchApplied (or directly use fetchApplied = async () => []),
removing the comment lines entirely so the code reads self-documentingly; update
any nearby references to use the new symbol if introduced.

In `@tests/unit/commands/database/db-status.test.ts`:
- Line 773: The test uses an invalid matcher by expecting a Promise<void> to
"resolves.not.toThrow()"; update the assertion to check the resolved value
instead: call statusDb with createMockCommand() and replace the matcher with
"resolves.toBeUndefined()" so the promise resolution for statusDb({ branch:
'feature-x' }, createMockCommand()) is asserted correctly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 635136a6-44a3-4f75-8109-fbb7cacda462

📥 Commits

Reviewing files that changed from the base of the PR and between 3deb552 and ae8c1fc.

📒 Files selected for processing (2)
  • src/commands/database/db-status.ts
  • tests/unit/commands/database/db-status.test.ts

Comment on lines +418 to +421
// When the database isn't enabled, skip the remote fetch — the branch endpoint would 404 with
// a non-actionable error. The pending-migrations hint below directs users to install
// @netlify/database and deploy, which is the actionable next step.
fetchApplied = () => Promise.resolve([])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove explanatory inline comments and keep intent in code shape

This block introduces comments that explain behavior; please keep this self-explanatory via code and remove these comments.

Suggested cleanup
-    } else {
-      // When the database isn't enabled, skip the remote fetch — the branch endpoint would 404 with
-      // a non-actionable error. The pending-migrations hint below directs users to install
-      // `@netlify/database` and deploy, which is the actionable next step.
-      fetchApplied = () => Promise.resolve([])
-    }
+    } else {
+      fetchApplied = async () => []
+    }

As per coding guidelines, **/*.{ts,tsx,js,jsx}: Never write comments on what the code does; make the code clean and self-explanatory instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/database/db-status.ts` around lines 418 - 421, Remove the
explanatory inline comments and make the intent explicit in code: replace the
commented assignment with a clearly named noop function (e.g., const
noopFetchApplied = async () => []; ) and assign fetchApplied = noopFetchApplied
(or directly use fetchApplied = async () => []), removing the comment lines
entirely so the code reads self-documentingly; update any nearby references to
use the new symbol if introduced.

Comment thread tests/unit/commands/database/db-status.test.ts Outdated
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