perf: tighten issue sync pre-filter — drop hasAssignee, add updateDate gate (Issue #158)#159
perf: tighten issue sync pre-filter — drop hasAssignee, add updateDate gate (Issue #158)#159okorach-sonar wants to merge 2 commits into
Conversation
SummaryTightens the issue sync pre-filter in two ways:
On the reference project (sonar-tools, ~1200 issues, ~200 needing sync), this should cut issue sync time from ~2:30 to near the lower bound implied by actual candidates. What reviewers should knowStart here: Read the Key behavioral changes for reviewers:
Testing coverage:
Watch for: The gate check is strict. If you're reviewing performance impact, note that this will filter more aggressively on projects where many issues have creationDate == updateDate (e.g., fresh scans with few manual edits). This is intentional—the author expects it to improve sync time significantly.
|
| * @returns {boolean} | ||
| */ | ||
| export function hasManualChanges(issue, changelog) { | ||
| if (!wasUpdatedAfterCreation(issue)) return false; |
There was a problem hiding this comment.
PR description claims safety-net behavior that code removes
Medium Severity
The updateDate != creationDate check was changed from a positive signal (catch-all safety net that qualified an issue on its own) to a mandatory gate. Issues where updateDate differs from creationDate but whose changes aren't detected by the three explicit checks (human changelog, manual comments, custom tags) will now be silently dropped. This could miss issues modified through admin tools, batch operations, or changelog entries with empty user fields.
Reviewed by Cursor Bugbot for commit 6ffecbc. Configure here.
The hasManualChanges pre-filter was keeping every source issue that had an assignee, even when nothing else about the issue had been touched manually. Source assignees do not map onto the target SonarQube Cloud user base, so an assignee on the source side is not a reason to keep an issue in the sync candidate set — once filtered through, it would just be a no-op (or a mapped-away skip) at sync time. On a reference project (sonar-tools, ~1200 issues, ~200 actually requiring sync), removing this check is expected to drop the candidate set dramatically and cut sync time, which currently dominates total migration time at ~88%. Other criteria are preserved: - Human-authored changelog entries - Non-migration comments - Custom tags - updateDate ≠ creationDate (catch-all safety net) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Two tests in the sqClient section of migrators.test.js have been silently broken by the gate change. They still pass, but only because the gate filters the test fixture issue before the scenario under test ever executes — both assert stats.transitioned === 0, which is now trivially true for the wrong reason. The author fixed the first test in that section (correctly adding dates to its fixture) but missed the other two.
| creationDate: '2024-01-01T00:00:00+0000', updateDate: '2024-06-15T10:30:00+0000' } | ||
| ]; | ||
|
|
||
| const stats = await syncIssues('proj', sqIssues, client, { concurrency: 1, sqClient }); |
There was a problem hiding this comment.
The fix here is correct, but two tests that follow immediately also pass sqClient and have the same problem — their sqIssues fixtures have no creationDate/updateDate:
syncIssues with sqClient returns not transitioned when transition fails(≈line 2269): the fixture issue at line 2282 has no dates. The gate filters it before the transition is attempted, sotransitionIssueis never called andstats.transitioned === 0passes vacuously. The error-handling path is no longer exercised.syncIssues skips transition when SC and SQ statuses already match(≈line 2290): same story — the fixture at line 2302 has no dates, the issue is filtered, andtransitionIssue.callCount === 0passes for the wrong reason.
Both fixtures need the same creationDate/updateDate treatment as the issue at line 2258–2259.
- Mark as noise
| if (hasHumanChangelog(changelog)) return true; | ||
| if (hasManualComments(issue)) return true; | ||
| if (hasCustomTags(issue)) return true; |
There was a problem hiding this comment.
Moving wasUpdatedAfterCreation to a gate (first check, hard-return false) changes the contract beyond what the PR description's "What's preserved" section implies. Previously an issue with a human-authored changelog entry would qualify regardless of whether updateDate was present or equal to creationDate. Now, missing or equal dates block every other signal.
In practice a real SQ issue with changelog entries will always have updateDate != creationDate, so the risk is low. But the edge case is worth calling out: if the SQ API ever returns an issue with genuine manual signals but without date fields (data quality issue, partial API response), it will be silently skipped. The old code erred on the side of syncing; the new code errs on the side of filtering.
- Mark as noise
The hasManualChanges pre-filter that decides whether a source issue is a sync candidate now requires the gate `updateDate !== creationDate` BEFORE checking human changelog entries, non-migration comments, or custom tags. A brand-new issue that just came out of analysis has matching dates and is now skipped regardless of any other signal — eliminating stragglers where there is nothing real to propagate. Old semantics: an issue qualified if ANY of (changelog | comments | tags | updateDate-changed) was true. New semantics: gate on updateDate-changed; if it passes, qualify on (changelog | comments | tags). The catch-all `updateDate != creationDate` alone no longer keeps an issue in the candidate set — combined with dropping the assignee check (the prior commit), this brings the pre-filter close to "issues that actually have something the user touched and that maps onto SonarQube Cloud". Tests updated to match: existing assertions get explicit creationDate/updateDate where the issue should pass the gate, and a new case verifies the gate blocks even strong signals when the dates match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6ffecbc to
7d6818d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7d6818d. Configure here.
| * @returns {boolean} | ||
| */ | ||
| export function hasManualChanges(issue, changelog) { | ||
| if (!wasUpdatedAfterCreation(issue)) return false; |
There was a problem hiding this comment.
Changelogs fetched for issues that immediately fail gate
Low Severity
fetchSqChangelogs is called for ALL issues before the hasManualChanges filter is applied. Since the new date gate in hasManualChanges only needs issue.updateDate and issue.creationDate (already present on issue objects), issues failing the gate could be excluded before the expensive batch changelog fetch, avoiding unnecessary API calls for every issue where dates match or are missing.
Reviewed by Cursor Bugbot for commit 7d6818d. Configure here.
| * @returns {boolean} | ||
| */ | ||
| export function hasManualChanges(issue, changelog) { | ||
| if (!wasUpdatedAfterCreation(issue)) return false; |
There was a problem hiding this comment.
Assignee-only issues silently skip assignment sync
Medium Severity
The downstream syncSingleIssue calls syncIssueAssignment, which uses userMappings to actively sync assignees to the target platform. Removing hasAssignee from the pre-filter means issues whose only manual change is an assignment (e.g., auto-assigned at creation or assigned via API with an empty changelog user) will now be filtered out before reaching syncIssueAssignment. This silently breaks the userMappings feature for those issues, contradicting the PR's claim that "source assignees don't map onto the target platform's user base" — the codebase clearly supports mapped assignment syncing.
Reviewed by Cursor Bugbot for commit 7d6818d. Configure here.


Summary
Tighten the
hasManualChangespre-filter that decides whether a source issue enters the matched-pair sync set. Two independent reductions of the candidate set, both targeted at Issue #158:Drop
hasAssignee(5e7da4b). A source-side assignee alone is no longer a sync trigger — assignees don't map onto SonarQube Cloud's user base, so an issue with only an assignee and no other signal would just be a no-op (or a mapped-away skip) at sync time.Add an
updateDategate (7d6818d). The pre-filter now requiresupdateDate !== creationDatebefore checking human changelog / non-migration comments / custom tags. A brand-new issue from analysis has matching dates and is now skipped regardless of any other signal — there is nothing manual to propagate.Old semantics:
New semantics:
On a reference project (sonar-tools, ~1200 source issues, ~200 actually require sync), the issue/hotspot sync currently dominates total migration time at ~88%. Tightening the candidate set this way is expected to bring the work close to the actual ~200 and cut sync time accordingly.
Files changed (3)
src/shared/utils/issue-sync/has-manual-changes.js— new gate semantics,hasAssigneeremoved, JSDoc updated.test/utils/issue-sync.test.js— existing assertions get explicitcreationDate/updateDatewhere the issue should pass the gate; assignee tests rewritten to assert the new behaviour; new test verifies the gate blocks even strong signals when dates match.test/sonarcloud/migrators/migrators.test.js—#140regression-test fixture gains explicit dates so it still passes the gate.Test plan
test/utils/issue-sync.test.jspass.main, 70 with this branch. Sorted-diff confirms identical failure set — zero regressions.🤖 Generated with Claude Code