fix: Fixed issue with repositories appearing in the carousel when indexing fails on first sync #305
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes update backend logic to ensure that the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Backend
participant Database
User->>Backend: Trigger repo indexing or connection sync
Backend->>Database: Start job, set status to IN_PROGRESS
alt On Success
Backend->>Database: Update status to SUCCESS
Backend->>Database: Update indexedAt/syncedAt to now
else On Failure
Backend->>Database: Update status to FAILED
Note right of Database: Do NOT update indexedAt/syncedAt
end
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
10-11: Ensure consistent verb tense in changelog entries
Other "Fixed" sections often use present-tense verbs (e.g., "- Fixes bug..."). Consider changing- Fixed issue with repos appearing in the carousel when they fail indexing for the first time.to
- Fix issue with repos appearing in the carousel when they fail indexing for the first time.for stylistic consistency.
🧰 Tools
🪛 LanguageTool
[duplication] ~10-~10: Possible typo: you repeated a word.
Context: ...pec/v2.0.0.html). ## [Unreleased] ### Fixed - Fixed issue with repos appearing in the carou...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)packages/backend/src/connectionManager.ts(0 hunks)packages/backend/src/repoManager.ts(0 hunks)packages/db/prisma/schema.prisma(2 hunks)
💤 Files with no reviewable changes (2)
- packages/backend/src/repoManager.ts
- packages/backend/src/connectionManager.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[duplication] ~10-~10: Possible typo: you repeated a word.
Context: ...pec/v2.0.0.html). ## [Unreleased] ### Fixed - Fixed issue with repos appearing in the carou...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
packages/db/prisma/schema.prisma (2)
44-45: ClarifyindexedAtsemantics with Prisma doc-comment
The added/// When the repo was last indexed successfully.comment correctly documents thatindexedAtshould only reflect successful indexing events. This aligns with the updated backend logic.
90-91: ClarifysyncedAtsemantics with Prisma doc-comment
The added/// When the connection was last synced successfully.comment accurately describes thatsyncedAtnow only updates on successful syncs, matching the change inconnectionManager.
brendan-kellam
force-pushed
the
bkellam/fix_indexedAt
branch
from
de1d849 to
eab7b23
Compare
|
/review |
![sourcebot-review-agent-dev[bot]](https://avatars.githubusercontent.com/u/135396723?s=60&v=4){kind=small}
| ### 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) | ||
|
|
There was a problem hiding this comment.
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'.
| ### 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) | ||
|
|
There was a problem hiding this comment.
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.
| syncStatus: ConnectionSyncStatus.FAILED, | ||
| syncedAt: new Date(), | ||
| syncStatusMetadata: syncStatusMetadata as Prisma.InputJsonValue, |
There was a problem hiding this comment.
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.
| @@ -334,7 +334,6 @@ export class ConnectionManager implements IConnectionManager { | |||
| }, | |||
| data: { | |||
| syncStatus: ConnectionSyncStatus.FAILED, | |||
| syncedAt: new Date(), | |||
| syncStatusMetadata: syncStatusMetadata as Prisma.InputJsonValue, | |||
There was a problem hiding this comment.
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.
| repoIndexingStatus: RepoIndexingStatus.FAILED, | ||
| indexedAt: new Date(), | ||
| } |
There was a problem hiding this comment.
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.
| data: { | ||
| repoIndexingStatus: RepoIndexingStatus.FAILED, | ||
| indexedAt: new Date(), | ||
| } |
There was a problem hiding this comment.
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.
| @@ -41,6 +41,7 @@ model Repo { | |||
| displayName String? | |||
| createdAt DateTime @default(now()) | |||
| updatedAt DateTime @updatedAt | |||
| /// When the repo was last indexed successfully. | |||
There was a problem hiding this comment.
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.
| @@ -41,6 +41,7 @@ model Repo { | |||
| displayName String? | |||
| createdAt DateTime @default(now()) | |||
| updatedAt DateTime @updatedAt | |||
| /// When the repo was last indexed successfully. | |||
There was a problem hiding this comment.
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.
| @@ -86,6 +87,7 @@ model Connection { | |||
| isDeclarative Boolean @default(false) | |||
| createdAt DateTime @default(now()) | |||
| updatedAt DateTime @updatedAt | |||
| /// When the connection was last synced successfully. | |||
There was a problem hiding this comment.
Add a newline between the comment and the field 'syncedAt' to improve readability and maintain consistent formatting with other fields that have comments.
| @@ -86,6 +87,7 @@ model Connection { | |||
| isDeclarative Boolean @default(false) | |||
| createdAt DateTime @default(now()) | |||
| updatedAt DateTime @updatedAt | |||
| /// When the connection was last synced successfully. | |||
There was a problem hiding this comment.
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.
Problem: If a repository fails to index on the first sync, it will appear in the repository carousel, even if the repo doesn't have any shards associated with it:
Solution: I've changed the behaviour of repoManager to not set the
indexedAtfield on index job failure. Functionally, this changes the behaviour from:To:
IMO the new behaviour is more expected for the name we are using. To keep things consistent, I've also changed the
syncedAtfield in connectionManager to have the same behaviour.Summary by CodeRabbit
Bug Fixes
Documentation