fix: improve sync stability, auth handling, and mobile layoutFix/mobile sync stability#55
Open
HuberttFox wants to merge 3 commits intoAmintaCCCP:mainfrom
Open
fix: improve sync stability, auth handling, and mobile layoutFix/mobile sync stability#55HuberttFox wants to merge 3 commits intoAmintaCCCP:mainfrom
HuberttFox wants to merge 3 commits intoAmintaCCCP:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces a responsive UI redesign for mobile/tablet devices across multiple frontend components, adds bulk repository cleanup logic in the backend, and implements local change tracking with visual sync state signaling to prevent overwriting user edits during synchronization. Changes
Sequence DiagramsequenceDiagram
participant User as User Action
participant UI as UI Component<br/>(RepositoryCard)
participant AutoSync as AutoSync Service
participant Backend as Backend API
participant Store as App Store
User->>UI: Unstar repository
UI->>AutoSync: forceSyncToBackend()
activate AutoSync
AutoSync->>AutoSync: Clear debounce timer
AutoSync->>AutoSync: Mark pending local changes
AutoSync->>UI: Emit sync-visual-state (isSyncing=true)
UI->>UI: Disable card animations
AutoSync->>Backend: PATCH /api/repositories
activate Backend
Backend-->>AutoSync: Success response
deactivate Backend
AutoSync->>AutoSync: Update last-known hashes
AutoSync->>AutoSync: Clear pending changes flag
AutoSync->>UI: Emit sync-visual-state (isSyncing=false)
deactivate AutoSync
UI->>UI: Re-enable card animations
UI-->>User: Visual feedback complete
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/RepositoryList.tsx (1)
483-493:⚠️ Potential issue | 🟠 MajorThe
repository-list-syncingclass is applied to the wrong element.The class is applied to a
<div>within the statistics section, but the repository cards are rendered in a separate grid at line 516. The CSS selector.repository-list-syncing .repository-cardrequires the class to be on an ancestor of the cards.The class should be applied to a container that wraps the repository grid, or directly on the outer
<div className="space-y-6">at line 351.🐛 Proposed fix
return ( - <div className="space-y-6"> + <div className={`space-y-6 ${disableCardAnimations ? 'repository-list-syncing' : ''}`}> {/* AI Analysis Controls */} <div className="flex items-center justify-between bg-white dark:bg-gray-800 rounded-xl border border-gray-200 dark:border-gray-700 p-4"> ... {/* Statistics */} <div className="text-sm text-gray-500 dark:text-gray-400"> <div className="flex items-center justify-between"> - <div className={disableCardAnimations ? 'repository-list-syncing' : undefined}> + <div> {t(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RepositoryList.tsx` around lines 483 - 493, The CSS class "repository-list-syncing" is currently applied to the statistics <div> (the t(...) string) but must be on the ancestor that contains the repository cards so the selector ".repository-list-syncing .repository-card" matches; remove the className={disableCardAnimations ? 'repository-list-syncing' : undefined} from that stats <div> and add it instead to the higher-level container that wraps the repository grid (the element with className "space-y-6" or the element that renders the repository map in the RepositoryList component) so the repository-card elements become children of the element with "repository-list-syncing".
🧹 Nitpick comments (2)
src/services/autoSync.ts (1)
52-60: Consider potential starvation scenario with_debounceTimerguard.The guard skips
syncFromBackendif_debounceTimeris active. While this prevents overwriting pending local changes, if the user makes frequent edits, the debounce timer keeps resetting, and backend data may never be pulled. This could lead to stale data on multi-device scenarios where other devices push updates.Consider whether a maximum "staleness" threshold might be appropriate for future enhancement, or document this trade-off as intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/autoSync.ts` around lines 52 - 60, The guard that returns early when _debounceTimer is active can starve syncs during frequent local edits; update syncFromBackend to track when debouncing started (e.g., add _debounceStartTimestamp or reuse _lastSuccessfulSyncAt) and introduce a MAX_STALENESS_MS constant so that if Date.now() - _debounceStartTimestamp (or _lastSuccessfulSyncAt) exceeds that threshold the function ignores _debounceTimer and proceeds with the sync; change the conditional that currently includes _debounceTimer to also check the staleness threshold (while preserving the other guards like _isSyncingFromBackendActive and _isPushingToBackend) so multi-device updates aren’t permanently delayed.server/src/routes/repositories.ts (1)
103-108: Add a foreign key constraint to enforce release cleanup at the schema level.The manual delete statements keep
releasesconsistent only in this handler.server/src/db/schema.tsdoes not declare a foreign key fromreleases.repo_idtorepositories.id, so any other repository-delete path can orphan release rows despite foreign-key enforcement being enabled inconnection.ts. AddingON DELETE CASCADEto the schema would centralize the invariant and eliminate the need for manual cleanup here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/routes/repositories.ts` around lines 103 - 108, The releases table lacks a foreign key to repositories so orphaned releases can occur; update the schema in server/src/db/schema.ts to declare releases.repo_id as a FOREIGN KEY referencing repositories.id with ON DELETE CASCADE, then remove or simplify the manual cleanup logic in this route (remove usages of deleteAllReleases, deleteReleasesNotIn and related manual deletes in server/src/routes/repositories.ts) so the database enforces cascading deletions centrally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/routes/repositories.ts`:
- Around line 111-123: The current logic treats a filtered-empty repoIds as an
intent to purge; instead, validate inputs and make full-delete explicit: before
the transaction, check the original repositories array and ensure every item
yields a numeric id (compare repositories.length vs repoIds.length) and reject
(throw/return 400) if any id is missing/malformed; only allow the
deleteAllReleases.run() / deleteAllRepositories.run() path when the caller
explicitly requested a purge (e.g., an explicit boolean flag or an empty
original repositories payload known-intent), and otherwise skip deletion when
repoIds.length === 0; keep the deleteReleasesNotIn(placeholders).run(...repoIds)
and deleteRepositoriesNotIn(placeholders).run(...repoIds) usage but only call
them when repoIds.length > 0.
---
Outside diff comments:
In `@src/components/RepositoryList.tsx`:
- Around line 483-493: The CSS class "repository-list-syncing" is currently
applied to the statistics <div> (the t(...) string) but must be on the ancestor
that contains the repository cards so the selector ".repository-list-syncing
.repository-card" matches; remove the className={disableCardAnimations ?
'repository-list-syncing' : undefined} from that stats <div> and add it instead
to the higher-level container that wraps the repository grid (the element with
className "space-y-6" or the element that renders the repository map in the
RepositoryList component) so the repository-card elements become children of the
element with "repository-list-syncing".
---
Nitpick comments:
In `@server/src/routes/repositories.ts`:
- Around line 103-108: The releases table lacks a foreign key to repositories so
orphaned releases can occur; update the schema in server/src/db/schema.ts to
declare releases.repo_id as a FOREIGN KEY referencing repositories.id with ON
DELETE CASCADE, then remove or simplify the manual cleanup logic in this route
(remove usages of deleteAllReleases, deleteReleasesNotIn and related manual
deletes in server/src/routes/repositories.ts) so the database enforces cascading
deletions centrally.
In `@src/services/autoSync.ts`:
- Around line 52-60: The guard that returns early when _debounceTimer is active
can starve syncs during frequent local edits; update syncFromBackend to track
when debouncing started (e.g., add _debounceStartTimestamp or reuse
_lastSuccessfulSyncAt) and introduce a MAX_STALENESS_MS constant so that if
Date.now() - _debounceStartTimestamp (or _lastSuccessfulSyncAt) exceeds that
threshold the function ignores _debounceTimer and proceeds with the sync; change
the conditional that currently includes _debounceTimer to also check the
staleness threshold (while preserving the other guards like
_isSyncingFromBackendActive and _isPushingToBackend) so multi-device updates
aren’t permanently delayed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e263125-ce01-4201-9e79-6486344d2804
📒 Files selected for processing (14)
server/src/routes/repositories.tssrc/App.tsxsrc/components/CategorySidebar.tsxsrc/components/Header.tsxsrc/components/ReleaseTimeline.tsxsrc/components/RepositoryCard.tsxsrc/components/RepositoryList.tsxsrc/components/SearchBar.tsxsrc/components/SettingsPanel.tsxsrc/index.csssrc/services/aiService.tssrc/services/autoSync.tssrc/services/backendAdapter.tssrc/store/useAppStore.ts
Comment on lines
+111
to
+123
| const repoIds = repositories | ||
| .map((repo) => repo.id) | ||
| .filter((id): id is number => typeof id === 'number'); | ||
|
|
||
| if (repoIds.length === 0) { | ||
| deleteAllReleases.run(); | ||
| deleteAllRepositories.run(); | ||
| return 0; | ||
| } | ||
|
|
||
| const placeholders = repoIds.map(() => '?').join(', '); | ||
| deleteReleasesNotIn(placeholders).run(...repoIds); | ||
| deleteRepositoriesNotIn(placeholders).run(...repoIds); |
There was a problem hiding this comment.
Don't let an empty or malformed sync purge the whole dataset.
Line 115 deletes both tables whenever repoIds.length === 0. Because that array is built by filtering non-numeric IDs, a malformed batch is treated the same as an intentional empty sync. Unlike server/src/routes/releases.ts:78-93, this route never rejects missing IDs, and src/services/backendAdapter.ts:201-210 can also send an empty in-memory state. That turns transient client-state or payload bugs into full data loss. Validate every repository ID before entering the transaction, and make the delete-all path explicit instead of the default for [].
Minimum validation guard
if (!Array.isArray(repositories)) {
res.status(400).json({ error: 'repositories array required', code: 'REPOSITORIES_ARRAY_REQUIRED' });
return;
}
+ for (const repo of repositories) {
+ const id = repo.id;
+ if (typeof id !== 'number' || !Number.isInteger(id) || id <= 0) {
+ res.status(400).json({ error: 'Each repository must have a numeric id', code: 'REPOSITORY_ID_REQUIRED' });
+ return;
+ }
+ }
const upsert = db.transaction(() => {
- const repoIds = repositories
- .map((repo) => repo.id)
- .filter((id): id is number => typeof id === 'number');
+ const repoIds = repositories.map((repo) => repo.id as number);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/routes/repositories.ts` around lines 111 - 123, The current logic
treats a filtered-empty repoIds as an intent to purge; instead, validate inputs
and make full-delete explicit: before the transaction, check the original
repositories array and ensure every item yields a numeric id (compare
repositories.length vs repoIds.length) and reject (throw/return 400) if any id
is missing/malformed; only allow the deleteAllReleases.run() /
deleteAllRepositories.run() path when the caller explicitly requested a purge
(e.g., an explicit boolean flag or an empty original repositories payload
known-intent), and otherwise skip deletion when repoIds.length === 0; keep the
deleteReleasesNotIn(placeholders).run(...repoIds) and
deleteRepositoriesNotIn(placeholders).run(...repoIds) usage but only call them
when repoIds.length > 0.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Details
Testing
Summary by CodeRabbit
New Features
Improvements