Skip to content

fix: Fixed issue with repositories appearing in the carousel when indexing fails on first sync #305

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.

Sign up for GitHub

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 2 commits into from
May 12, 2025
Merged

fix: Fixed issue with repositories appearing in the carousel when indexing fails on first sync #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09d7f23
fix
brendan-kellam May 12, 2025
eab7b23
changelog
brendan-kellam May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed
- Fixed issue with repos appearing in the carousel when they fail indexing for the first time. [#305](https://github.com/sourcebot-dev/sourcebot/pull/305)

Comment on lines +10 to +12
Copy link

Choose a reason for hiding this comment

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

The new entry added under the 'Unreleased' section duplicates the same fix already present under the '3.2.0' release. To avoid redundancy, please remove this duplicate changelog entry from 'Unreleased'.

Comment on lines +10 to +12
Copy link

Choose a reason for hiding this comment

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

The new changelog entry for the 'Fixed' section should be added under '## [Unreleased]' not above '## [3.1.4] - 2025-05-10'. Please insert the fix description after line 9 and before the '3.1.4' release heading to maintain changelog format consistency.

## [3.1.4] - 2025-05-10

### Fixed
Expand Down
1 change: 0 additions & 1 deletion packages/backend/src/connectionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,6 @@ export class ConnectionManager implements IConnectionManager {
},
data: {
syncStatus: ConnectionSyncStatus.FAILED,
syncedAt: new Date(),
syncStatusMetadata: syncStatusMetadata as Prisma.InputJsonValue,
Comment on lines 336 to 337
Copy link

Choose a reason for hiding this comment

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

The previous code was updating the "syncedAt" field when the syncStatus was set to FAILED, but the new code removes this. However, in the onSyncJobFailed method (lines 353-380), after a failure, "syncedAt" is not explicitly updated. Given the PR context where "syncedAt" should reflect the last successful sync, it is correct to remove the update on failure. Just ensure that elsewhere the "syncedAt" field is only updated on success (as onSyncJobCompleted does). No change is required here, just confirm consistency.

Copy link

Choose a reason for hiding this comment

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

The removal of setting 'syncedAt: new Date()' on sync failure means the syncedAt field will now only reflect the time of last successful sync, which is intended. However, verify if there are any downstream consumers or UI components relying on syncedAt being updated even on failure, as this change could lead to stale syncedAt values if sync repeatedly fails.

}
});
Expand Down
1 change: 0 additions & 1 deletion packages/backend/src/repoManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,6 @@ export class RepoManager implements IRepoManager {
},
data: {
repoIndexingStatus: RepoIndexingStatus.FAILED,
indexedAt: new Date(),
}
Comment on lines 394 to 395
Copy link

Choose a reason for hiding this comment

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

Removing the update to the 'indexedAt' field on indexing failure changes the timestamp semantics. Ensure that all code depending on 'indexedAt' properly reflects this new meaning (last successful index time) and handle cases where 'indexedAt' may be null. Also verify that this change does not cause unintended consequences where the timestamp is expected to be updated regardless of failure or success.

Comment on lines 393 to 395
Copy link

Choose a reason for hiding this comment

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

The removal of setting indexedAt on failure can affect downstream logic that relies on this timestamp. Ensure that all consumers of indexedAt can handle it being null or unset in failure cases to avoid unexpected errors or behavior.

})
}
Expand Down
2 changes: 2 additions & 0 deletions packages/db/prisma/schema.prisma
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ model Repo {
displayName String?
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
/// When the repo was last indexed successfully.
Copy link

Choose a reason for hiding this comment

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

The added comment 'When the repo was last indexed successfully.' could be improved for clarity by specifying this corresponds to the last successful indexing time, differentiating it explicitly from previous semantics that could include failed indexing attempts.

Copy link

Choose a reason for hiding this comment

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

The added comment '/// When the repo was last indexed successfully.' for 'indexedAt' is good for clarity. Please ensure the comment for 'syncedAt' in the Connection model is updated similarly to reflect it tracks last successful syncing only, to maintain consistency.

indexedAt DateTime?
isFork Boolean
isArchived Boolean
Expand Down Expand Up @@ -86,6 +87,7 @@ model Connection {
isDeclarative Boolean @default(false)
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
/// When the connection was last synced successfully.
Copy link

Choose a reason for hiding this comment

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

Add a newline between the comment and the field 'syncedAt' to improve readability and maintain consistent formatting with other fields that have comments.

Copy link

Choose a reason for hiding this comment

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

The added comment "/// When the connection was last synced successfully." duplicates the style of other field comments but to maintain clarity and completeness, please ensure that the comments for similar date tracking fields (e.g., indexedAt on Repo) follow a consistent style and format, including specifying that this timestamp reflects the last successful sync rather than any attempt. Consider updating related documentation to reflect this behavior change.

syncedAt DateTime?
repos RepoToConnection[]
syncStatus ConnectionSyncStatus @default(SYNC_NEEDED)
Expand Down