Conversation
🦋 Changeset detectedLatest commit: ef22a4a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes remove stop path validation and related error handling from the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant findUp
participant tryFileStats
Caller->>findUp: Call with entry, stop, and options
loop Upward Search
findUp->>tryFileStats: Check if file exists at current path
tryFileStats-->>findUp: Return stats or undefined (no error thrown)
alt File found
findUp-->>Caller: Return found file path
end
alt Reached stop or root
findUp-->>Caller: Return undefined
end
end
Possibly related PRs
Suggested labels
Poem
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
packages/core/src/helpers.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #416 +/- ##
=========================================
+ Coverage 9.17% 9.72% +0.55%
=========================================
Files 12 12
Lines 458 432 -26
Branches 205 185 -20
=========================================
Hits 42 42
+ Misses 416 390 -26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@pkgr/browser
@pkgr/core
@pkgr/es-modules
@pkgr/imagemin
@pkgr/rollup
@pkgr/umd-globals
@pkgr/utils
commit: |
|
Deploy preview for pkgr ready! ✅ Preview Built with commit ef22a4a. |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 662edc3 in 1 minute and 46 seconds. Click for details.
- Reviewed
86lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/core/src/helpers.ts:43
- Draft comment:
Swallowing fs.statSync errors here may hide unexpected issues. Consider checking for expected error codes (e.g. ENOENT) in the catch block. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code is using throwIfNoEntry: false which is the proper way to handle non-existent files. Any other errors would be unexpected filesystem errors like permissions. The function returns undefined on failure which is a clear failure mode. The comment suggests a more complex solution without clear benefit. I could be wrong about error handling best practices. Maybe there are important error cases that should be handled differently. The code's current error handling is appropriate for its purpose - it's a helper that safely checks if a file exists and returns undefined if anything goes wrong. More granular error handling would add complexity without clear benefit. Delete this comment. The current error handling is appropriate and the suggested change would add unnecessary complexity.
2. packages/core/src/helpers.ts:95
- Draft comment:
Removal of stop validation logic means the 'stop' option is no longer normalized. Without converting it to an absolute path, comparing it with 'entry' may lead to inconsistencies. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_yFzZah3wTr2me1jn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that findUp no longer throws by catching file system errors and removing throw-based options and stop-path validation.
- Wrapped
fs.statSyncintryFileStatsinside atry/catchto swallow unexpected errors. - Removed
throwOnStopNotFoundandthrowOnInvalidStopoptions and related stop-path checks infindUp. - Simplified
findUpsignature by removing throw options and the initial entry resolution viatryFileStats.
Comments suppressed due to low confidence (1)
packages/core/src/helpers.ts:99
- Initial resolution of
entryviatryFileStatswas removed, soentrymay not be normalized to a directory or absolute path. This can cause the upward search to start from an unintended location.
do {
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/giant-trainers-swim.md(1 hunks)packages/core/src/helpers.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/src/helpers.ts (1)
packages/core/src/constants.ts (1)
CWD(5-5)
🪛 GitHub Check: codecov/patch
packages/core/src/helpers.ts
[warning] 44-45: packages/core/src/helpers.ts#L44-L45
Added lines #L44 - L45 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
🔇 Additional comments (2)
packages/core/src/helpers.ts (1)
95-95:Details
✅ Verification successful
Simplified destructuring aligns with no-throw objective.
The removal of the
throwOnStopNotFoundandthrowOnInvalidStopoptions simplifies the function and eliminates potential error paths, which perfectly aligns with makingfindUpnever throw.Verify that all existing usage of
findUpis compatible with the removed options:
🏁 Script executed:
#!/bin/bash # Description: Search for usage of removed options to ensure compatibility # Search for any usage of the removed options rg -A 3 -B 3 "throwOnStopNotFound|throwOnInvalidStop" # Search for findUp usage patterns to ensure compatibility rg -A 5 -B 2 "findUp\("Length of output: 81
To ensure no remaining references to the old options or the
findUpAPI, let’s broaden our search:
🏁 Script executed:
#!/bin/bash # Re-verify that no call sites or type references remain # Search for any runtime calls to findUp rg -n "findUp\(" --color never # Search for any mention of FindUpOptions in types or docs rg -n "FindUpOptions" --color never # Double-check for the removed options rg -n "throwOnStopNotFound|throwOnInvalidStop" --color neverLength of output: 330
Let’s inspect the
FindUpOptionsinterface to confirm it no longer includes the removed properties:
🏁 Script executed:
#!/bin/bash # Show the FindUpOptions interface definition rg -n "export interface FindUpOptions" -A15 -B0 packages/core/src/helpers.tsLength of output: 457
All outdated options removed and no lingering usage detected.
Verification steps confirmed that:
- The
FindUpOptionsinterface no longer includesthrowOnStopNotFoundorthrowOnInvalidStop.- No calls to
findUp(exist outside its own definition.- No references to
FindUpOptionsremain elsewhere in the repo.This cleanly aligns with the goal of making
findUpnon-throwing. Feel free to merge..changeset/giant-trainers-swim.md (1)
1-6: Well-documented changeset for the patch update.The changeset correctly follows the standard format and accurately describes the fix. The patch-level version bump is appropriate for this bug fix that ensures
findUpnever throws.
Important
findUpinhelpers.tsnow handles errors gracefully by removing throw options and wrappingfs.statSyncin a try-catch.findUpfunction inhelpers.tsno longer throws errors for invalidstoppaths or whenstopis not found.throwOnStopNotFoundandthrowOnInvalidStopoptions fromFindUpOptions.fs.statSynccall intryFileStatswith atry-catchblock to prevent exceptions from propagating.This description was created by
for 662edc3. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit