Skip to content

Reviewed Prs Bug fixed #225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 4, 2025
Merged

Reviewed Prs Bug fixed #225

merged 5 commits into from
Aug 4, 2025

Conversation

Preeti9764
Copy link
Contributor

@Preeti9764 Preeti9764 commented Aug 2, 2025

📌 Fixes

Fixes #224


📝 Summary of Changes

  • Correctly showing the reviewed prs of selected date range
  • not showing unnecessary ones.

Summary by Sourcery

Fix the reviewed PR bug by standardizing date handling and strengthening PR filtering to only include items within the selected date range.

Bug Fixes:

  • Ensure reviewed PRs are correctly filtered by the active date range and exclude out-of-range items

Enhancements:

  • Replace custom date formatting with ISO strings in getToday/getYesterday
  • Centralize date range calculation for cache keys, API queries, commit fetching, PR filtering, and days-range logic
  • Add stricter date and state checks when including commits and PRs
  • Introduce detailed debug logging for commit and PR processing

@github-actions github-actions bot added javascript Pull requests that update javascript code core extension labels Aug 2, 2025
Copy link
Contributor

sourcery-ai bot commented Aug 2, 2025

Reviewer's Guide

This PR standardizes date handling across the app by replacing custom padding logic with ISO date strings, centralizes date‐range computation for caching and API queries, tightens commit and PR filtering using unified start/end dates with detailed logging, and cleans up date utilities in the UI scripts.

Class diagram for updated date handling and PR filtering

classDiagram
    class ScrumHelper {
        +getYesterday()
        +getToday()
        +allIncluded(outputTarget)
        +fetchCommitsForOpenPRs(prs, githubToken, startDate, endDate)
        +filterReviewedPRs(items, dateRange)
    }
    class DateUtils {
        +getYesterday()
        +getToday()
    }
    class PRItem {
        +number
        +state
        +created_at
        +updated_at
        +pull_request
        +_allCommits
    }
    ScrumHelper --> DateUtils : uses
    ScrumHelper --> PRItem : filters

    class MainUI {
        +getYesterday()
        +getToday()
    }
    class PopupUI {
        +getYesterday()
        +getToday()
    }
    MainUI ..> DateUtils : similar logic
    PopupUI ..> DateUtils : similar logic
Loading

File-Level Changes

Change Details Files
Standardize date string generation in utility functions
  • Replace manual YYYY-MM-DD padding with toISOString().split('T')[0] in getYesterday/getToday
  • Apply same simplification in main.js and popup.js date helpers
src/scripts/scrumHelper.js
src/scripts/main.js
src/scripts/popup.js
Centralize and normalize date-range logic for caching and API URL construction
  • Compute startDateForCache/endDateForCache based on yesterdayContribution, provided range, or default to last 7 days
  • Update cacheKey and GitHub issue/pr query URLs to use normalized date variables
src/scripts/scrumHelper.js
Refine commit fetching parameters and add detailed commit logs
  • Calculate startDateForCommits/endDateForCommits with same date-range logic
  • Pass new range into fetchCommitsForOpenPRs and adjust since/until formatting with 'Z'
  • Log commit date checks and filtering outcomes
src/scripts/scrumHelper.js
Enhance PR review filtering with comprehensive date checks
  • Derive startDateTime/endDateTime and filter out items updated, created, or merged outside the range
  • Add extra conservative rules for PRs created before range and special yesterdayContribution handling
  • Embed detailed log statements for each skip decision
src/scripts/scrumHelper.js
Refactor new vs existing PR classification and rendering logic
  • Determine isNewPR, isUpdatedInRange, hasCommitsInRange and skip or include accordingly
  • Update rendering to wrap commit lists in UL tags and tweak styling
  • Add debug logs around PR action classification
src/scripts/scrumHelper.js
Minor UI code cleanup
  • Remove unnecessary trailing space in userReasonElement declaration
src/scripts/main.js

Assessment against linked issues

Issue Objective Addressed Explanation
#224 Filter reviewed PRs according to the selected date range so that only PRs reviewed within the specified range are shown.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-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.

Hey @Preeti9764 - I've reviewed your changes - here's some feedback:

  • The repeated date‐range computation logic is duplicated in several places—extract it into a single helper to DRY up the code and ensure consistency.
  • Subtracting 24h via getTime() - 246060*1000 can misbehave around DST transitions; consider using setDate(getDate() - 1) or handling dates in UTC consistently.
  • The added verbose logging may flood production output—consider gating these debug statements behind a log level or feature flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated date‐range computation logic is duplicated in several places—extract it into a single helper to DRY up the code and ensure consistency.
- Subtracting 24h via getTime() - 24*60*60*1000 can misbehave around DST transitions; consider using setDate(getDate() - 1) or handling dates in UTC consistently.
- The added verbose logging may flood production output—consider gating these debug statements behind a log level or feature flag.

## Individual Comments

### Comment 1
<location> `src/scripts/scrumHelper.js:341` </location>
<code_context>
-        '-' +
-        ('00' + yesterdayDay.toString()).slice(-2);
-    return yesterdayPadded;
+    let yesterday = new Date(today.getTime() - 24 * 60 * 60 * 1000);
+    return yesterday.toISOString().split('T')[0];
 }
</code_context>

<issue_to_address>
Subtracting 24 hours may not always yield the previous calendar day due to DST changes.

This approach can produce incorrect results around DST changes. Use a date library or `setDate(today.getDate() - 1)` for reliable calculation of the previous calendar day.
</issue_to_address>

### Comment 2
<location> `src/scripts/popup.js:16` </location>
<code_context>
-        '-' +
-        ('00' + yesterdayDay.toString()).slice(-2);
-    return yesterdayPadded;
+    let yesterday = new Date(today.getTime() - 24 * 60 * 60 * 1000);
+    return yesterday.toISOString().split('T')[0];
 }

</code_context>

<issue_to_address>
Date calculation and formatting for 'yesterday' may not be robust across timezones and DST.

This method may produce incorrect dates near DST transitions or for users outside UTC. Use a timezone-aware approach for accurate results.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@Preeti9764
Copy link
Contributor Author

@hpdang Please review.

@Preeti9764 Preeti9764 self-assigned this Aug 2, 2025
@hpdang hpdang requested a review from vedansh-5 August 3, 2025 21:01
Copy link
Contributor

@vedansh-5 vedansh-5 left a comment

Choose a reason for hiding this comment

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

LGTM!

@hpdang hpdang merged commit bcb16c8 into fossasia:master Aug 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core extension javascript Pull requests that update javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Showing out of range reviewed prs
3 participants