Minor: commits in draft PRs #218
Merged
Conversation
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
Signed-off-by: Vedansh Saini <[email protected]>
Reviewer's GuideThis PR enables displaying commit details for existing draft pull requests under the “Show commits” toggle by injecting a nested commits list, updates the UI label and all translations to reflect that drafts are included, and removes an obsolete comment from the PR date-range logic. Sequence diagram for displaying commits in draft PRssequenceDiagram
actor User
participant UI
participant scrumHelper
participant GitHubAPI
User->>UI: Toggle 'Show commits' option
UI->>scrumHelper: Request PR list with commits
scrumHelper->>GitHubAPI: Fetch PRs and their commits
GitHubAPI-->>scrumHelper: Return PRs (open and draft) with commits
scrumHelper-->>UI: Render PR list
UI-->>User: Display PRs with nested commits for open and draft PRs
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Signed-off-by: Vedansh Saini <[email protected]>
github-actions
bot
added
javascript
Now the commits are being shown in the draft PRs.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vedansh-5 - I've reviewed your changes and found some issues that need to be addressed.
- Rendering commit items as direct
- children of another
- breaks HTML semantics—consider wrapping them in a nested
- or similar container inside the parent list item.
- The inline style attributes on each commit entry could be replaced with CSS classes for better maintainability and theming.
- Rather than unconditionally calling log(), guard the debug logging behind a log level check or remove it in production to avoid console noise.
Prompt for AI Agents
Please address the comments from this code review: ## Overall Comments - Rendering commit items as direct <li> children of another <li> breaks HTML semantics—consider wrapping them in a nested <ul> or similar container inside the parent list item. - The inline style attributes on each commit entry could be replaced with CSS classes for better maintainability and theming. - Rather than unconditionally calling log(), guard the debug logging behind a log level check or remove it in production to avoid console noise.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Vedansh Saini <[email protected]>
|
@hpdang Ready for review, thankyou. |
|
LGTM! @Saksham-Sirohi could you please test this if it works for you? @Preeti9764 please review |
|
@hpdang tested works! |
|
Can the commits be more indented next to an open pr? This feels like very little indentation for commits inside |
@Saksham-Sirohi I have added more indent for commits on drafts
|
Signed-off-by: Vedansh Saini <[email protected]>
There was a problem hiding this comment.
Hey @vedansh-5 - I've reviewed your changes - here's some feedback:
- Extract the commit-list markup into a helper (or use Array.map + join) to simplify string concatenation and improve readability.
- Replace inline styles on the commit list items with CSS classes in your stylesheet for better maintainability.
- Remove or guard the debug console.log to avoid polluting production logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the commit-list markup into a helper (or use Array.map + join) to simplify string concatenation and improve readability.
- Replace inline styles on the commit list items with CSS classes in your stylesheet for better maintainability.
- Remove or guard the debug console.log to avoid polluting production logs.
## Individual Comments
### Comment 1
<location> `src/scripts/scrumHelper.js:1281` </location>
<code_context>
- li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a>${showOpenLabel ? ' ' + pr_draft_button : ''}</li>`;
+ li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a>${showOpenLabel ? ' ' + pr_draft_button : ''}`;
+ if (showCommits && item._allCommits && item._allCommits.length && !isNewPR) {
+ log(`[PR DEBUG] Rendering commits for existing draft PR #${number}:`, item._allCommits);
+ li += '<ul>';
+ item._allCommits.forEach(commit => {
</code_context>
<issue_to_address>
Debug logging is present in production code.
Debug logs in production can clutter output and expose sensitive data. Remove this log or restrict it to debug mode if not essential for users.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
log(`[PR DEBUG] Rendering commits for existing draft PR #${number}:`, item._allCommits);
li += '<ul>';
=======
li += '<ul>';
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@vedansh-5 The preview report is not getting generated without the token .Please check .Thanks! |
|
@vedansh-5, the draft commits now have good indentation also, I am seeing the same problem as @Preeti9764, without github token, I receive error 422 |
|
works properly now!
|
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
There was a problem hiding this comment.
Hey @vedansh-5 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/scripts/scrumHelper.js:1281` </location>
<code_context>
- li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a>${showOpenLabel ? ' ' + pr_draft_button : ''}</li>`;
+ li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a>${showOpenLabel ? ' ' + pr_draft_button : ''}`;
+ if (showCommits && item._allCommits && item._allCommits.length && !isNewPR && githubToken) {
+ log(`[PR DEBUG] Rendering commits for existing draft PR #${number}:`, item._allCommits);
+ li += '<ul>';
+ item._allCommits.forEach(commit => {
</code_context>
<issue_to_address>
Debug logging is present in production code.
Debug logs in production can clutter output and expose sensitive data. Please remove or conditionally enable this log with a debug flag.
Suggested implementation:
```javascript
if (showCommits && item._allCommits && item._allCommits.length && !isNewPR && githubToken) {
if (typeof DEBUG !== 'undefined' && DEBUG) {
log(`[PR DEBUG] Rendering commits for existing draft PR #${number}:`, item._allCommits);
}
li += '<ul>';
item._allCommits.forEach(commit => {
```
If a `DEBUG` flag is not already defined in your codebase, you should define it at the top of your script or in your configuration. For example:
```js
const DEBUG = false; // Set to true to enable debug logging
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@Preeti9764 please review this again |
|
The closed is visible in red it should be in purple as in master branch .Please correct that !Thanks. |
Were these changes made in the last merged PR by you? If so, this would be fine in master because there are no conflicts and this would not affect the master branch's behavior. Thankyou! |
Edit - I did not touch any code concerning the labels. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Adding commits in draft PRs
Fixes #213
Summary by Sourcery
Enable viewing commit details for draft PRs alongside open PRs when the showCommits option is enabled.
New Features:
Enhancements:
Chores: