Conversation
feat: ✨ Detect COPYING & COPYING.LESSER license files
|
Thank you for submitting this pull request! We appreciate your contribution to the project. Before we can merge it, we need to review the changes you've made to ensure they align with our code standards and meet the requirements of the project. We'll get back to you as soon as we can with feedback. Thanks again! |
Reviewer's GuideRefactors license detection to rely primarily on GitHub's licensee engine (via detectLicense) while persisting and surfacing the detected license file path and content throughout the compliance, persistence, and dashboard layers, with a small schema migration to store the license path and an expanded fallback search for license-like files. Sequence diagram for updated license detection using GitHub licensee enginesequenceDiagram
participant Caller
participant ComplianceLicense as checkForLicense
participant Provider as GitHubRepositoryProvider
Caller->>ComplianceLicense: checkForLicense(owner, repo)
ComplianceLicense->>Provider: detectLicense(owner, repo)
alt provider returns detected.path
Provider-->>ComplianceLicense: DetectedLicense{name, content, path, spdxId}
ComplianceLicense-->>Caller: LicenseResult{status:true, path:detected.path, content:detected.content, spdx_id:detected.spdxId}
else no detected.path
Provider-->>ComplianceLicense: DetectedLicense{name:null, content:null, path:null, spdxId:null}
loop LICENSE_PATHS
ComplianceLicense->>Provider: getFileContent(owner, repo, filePath)
alt file found
Provider-->>ComplianceLicense: {content}
ComplianceLicense-->>Caller: LicenseResult{status:true, path:filePath, content, spdx_id:null}
end
end
ComplianceLicense-->>Caller: LicenseResult{status:false, path:"No LICENSE file found", content:"", spdx_id:null}
end
Entity relationship diagram for LicenseRequest with persisted license_patherDiagram
Repository ||--o{ LicenseRequest : has
Repository {
int id
}
LicenseRequest {
int id
string license_id
string license_path
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for making updates to your pull request. Our team will take a look and provide feedback as soon as possible. Please wait for any GitHub Actions to complete before editing your pull request. If you have any additional questions or concerns, feel free to let us know. Thank you for your contributions! |
PR Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
checkForLicense, thedetectLicensecall swallows any error and downgrades it to a "no license" result; consider narrowing this catch (e.g., only handling 404/known API failures) so unexpected errors still surface rather than silently hiding provider issues. - The
renderLicensemessages now interpolatelicense.pathbut still claim the file is "at the root level"; since the GitHub API may return non-root paths, consider updating this wording to reflect the actual path location or omitting the root-level assumption.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `checkForLicense`, the `detectLicense` call swallows any error and downgrades it to a "no license" result; consider narrowing this catch (e.g., only handling 404/known API failures) so unexpected errors still surface rather than silently hiding provider issues.
- The `renderLicense` messages now interpolate `license.path` but still claim the file is "at the root level"; since the GitHub API may return non-root paths, consider updating this wording to reflect the actual path location or omitting the root-level assumption.
## Individual Comments
### Comment 1
<location path="ui/server/services/dashboard/manager.ts" line_range="501" />
<code_context>
license: {
content: licenseDb?.license_content ?? "",
- path: "",
+ path: licenseDb?.license_path ?? "",
spdx_id: licenseDb?.license_id ?? null,
status: licenseDb?.contains_license ?? false,
</code_context>
<issue_to_address>
**suggestion:** Handle empty `license_path` more gracefully when rendering the dashboard
For existing `LicenseRequest` rows, `license_path` will be `""` until they’re backfilled, so this change will render `A `` file is found...` for those cases. Consider normalizing the value here (e.g., use `null` or a default like `"LICENSE"` when the DB value is empty) and update `renderLicense` to treat that case explicitly.
Suggested implementation:
```typescript
cwl: null,
license: {
content: licenseDb?.license_content ?? "",
// Normalize empty/whitespace-only license_path to null so rendering can treat it explicitly.
path: licenseDb?.license_path?.trim() ? licenseDb.license_path.trim() : null,
spdx_id: licenseDb?.license_id ?? null,
status: licenseDb?.contains_license ?? false,
},
```
To fully implement your comment, the UI code that renders license information (likely a `renderLicense` function or component) should be updated to:
1. Treat `license.path === null` (or `!license.path`) as a special case and avoid rendering backticks with an empty path (e.g., show “A license file is found...” or “A license file is present but its path is not yet available” instead of `A `` file is found...`).
2. Optionally, if you’d rather display a default like `"LICENSE"`, you could instead normalize with `path: licenseDb?.license_path?.trim() || "LICENSE"`, and adjust the renderer text accordingly to explain it’s an assumed/default path.
3. Ensure any logic that tests for existence of a license file uses `license.status` (or similar boolean) rather than assuming a non-empty `path` is the only indicator.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thank you for getting this pull request merged. We appreciate your contribution and look forward to your next one! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Delegate license detection to GitHub’s licensee engine while persisting and surfacing the detected license file path across backend, storage, and dashboard rendering.
New Features:
Enhancements:
Chores: