fix: improve sync stability, auth handling, and mobile layoutFix/mobile sync stability#55
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds backend full-sync pruning for repositories/releases, responsive frontend layout updates, session-scoped backend secret handling and verifyAuth, local-change tracking with visual sync signaling (and forceSyncToBackend), and a safer AI message extraction fallback. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant RepoCard as RepositoryCard (UI)
participant AutoSync as AutoSync Service
participant Store as App Store
participant Backend as Backend API
User->>RepoCard: Unstar repository
RepoCard->>AutoSync: forceSyncToBackend()
activate AutoSync
AutoSync->>AutoSync: clear debounce timer
AutoSync->>Store: mark pending local changes
AutoSync->>RepoCard: emit gsm:repository-sync-visual-state (isSyncing=true)
RepoCard->>RepoCard: disable card animations
AutoSync->>Backend: PATCH /api/repositories (push)
activate Backend
Backend-->>AutoSync: success/failure
deactivate Backend
AutoSync->>Store: update last-known hashes / drain pending pushes
AutoSync->>Store: clear pending flag (on success)
AutoSync->>RepoCard: emit gsm:repository-sync-visual-state (isSyncing=false)
RepoCard->>RepoCard: re-enable animations
deactivate AutoSync
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![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
server/src/routes/repositories.ts
Outdated
| 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.
|
已推送修复到 PR 分支（fix/mobile-sync-stability）。 修改原因：
修复说明：
|
2 participants
Summary
Details
Testing
Summary by CodeRabbit
New Features
Improvements
Bug Fixes