feat: cli sync databases #1222
feat: cli sync databases #1222
Conversation
|
Warning Rate limit exceeded@ChiragAgg5k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request updates the push command template to add three exports from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/cli/lib/commands/push.js.twig(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: cli (console)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: apple (client)
- GitHub Check: build (8.3, CLINode16)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: swift (server)
- GitHub Check: node (server)
- GitHub Check: android (client)
- GitHub Check: web (client)
- GitHub Check: react-native (client)
🔇 Additional comments (2)
templates/cli/lib/commands/push.js.twig (2)
58-62: LGTM - New imports support database synchronization.The added imports (
tablesDBList,tablesDBDelete,tablesDBListTables) are necessary for the new synchronization functionality.
1828-1845: Configuration resync logic is correct and properly persists changes.The
localConfig.set()method updates the configuration data and callsthis.write()to persist changes to the file, confirming that the integration at lines 1828-1845 will correctly:
- Filter out tables and tablesDBs that were deleted remotely
- Persist both updates to the local configuration file
No issues found.
ChiragAgg5k
force-pushed
the
sync-databases
branch
from
075c00c to
16620e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
templates/cli/lib/commands/push.js.twig (3)
1747-1763: Avoid duplicate updates when both name and enabled change.Current else-if chain can push the same DB twice to
toUpdate. Compute both differences first, then push once.Apply:
- } else if (remoteDB.name !== localDB.name) { - toUpdate.push(localDB); - changes.push({ - id: localDB.$id, - key: 'Name', - remote: chalk.red(remoteDB.name), - local: chalk.green(localDB.name) - }); - } else if (remoteDB.enabled !== localDB.enabled) { - toUpdate.push(localDB); - changes.push({ - id: localDB.$id, - key: 'Enabled?', - remote: chalk.red(remoteDB.enabled), - local: chalk.green(localDB.enabled) - }); - } + } else { + let hasChanges = false; + if (remoteDB.name !== localDB.name) { + hasChanges = true; + changes.push({ + id: localDB.$id, + key: 'Name', + remote: chalk.red(remoteDB.name), + local: chalk.green(localDB.name) + }); + } + if (remoteDB.enabled !== localDB.enabled) { + hasChanges = true; + changes.push({ + id: localDB.$id, + key: 'Enabled?', + remote: chalk.red(remoteDB.enabled), + local: chalk.green(localDB.enabled) + }); + } + if (hasChanges) { + toUpdate.push(localDB); + } + }
1797-1804: Creation should honor enabled flag.
tablesDBCreateomitsenabled, so newly created DBs won’t match local config.Apply:
await tablesDBCreate({ databaseId: db.$id, name: db.name, + enabled: db.enabled, parseOutput: false });
1783-1816: Add failure isolation around delete/create/update loops.A mid-run failure can leave state inconsistent. Wrap each op with try/catch, log context, and abort with a clear error.
Example:
- for (const db of toDelete) { - log(`Deleting database ${db.name} ( ${db.$id} ) ...`); - await tablesDBDelete({ databaseId: db.$id, parseOutput: false }); - success(`Deleted ${db.name} ( ${db.$id} )`); - needsResync = true; - } + for (const db of toDelete) { + try { + log(`Deleting database ${db.name} ( ${db.$id} ) ...`); + await tablesDBDelete({ databaseId: db.$id, parseOutput: false }); + success(`Deleted ${db.name} ( ${db.$id} )`); + needsResync = true; + } catch (e) { + error(`Failed to delete database ${db.name} ( ${db.$id} ): ${e.message}`); + throw new Error(`Database sync failed during deletion of ${db.$id}. Some changes may have been applied.`); + } + } @@ - for (const db of toCreate) { - log(`Creating database ${db.name} ( ${db.$id} ) ...`); - await tablesDBCreate({ - databaseId: db.$id, - name: db.name, - parseOutput: false - }); - success(`Created ${db.name} ( ${db.$id} )`); - } + for (const db of toCreate) { + try { + log(`Creating database ${db.name} ( ${db.$id} ) ...`); + await tablesDBCreate({ + databaseId: db.$id, + name: db.name, + enabled: db.enabled, + parseOutput: false + }); + success(`Created ${db.name} ( ${db.$id} )`); + } catch (e) { + error(`Failed to create database ${db.name} ( ${db.$id} ): ${e.message}`); + throw new Error(`Database sync failed during creation of ${db.$id}. Some changes may have been applied.`); + } + } @@ - for (const db of toUpdate) { - log(`Updating database ${db.name} ( ${db.$id} ) ...`); - await tablesDBUpdate({ - databaseId: db.$id, - name: db.name, - enabled: db.enabled, - parseOutput: false - }); - success(`Updated ${db.name} ( ${db.$id} )`); - } + for (const db of toUpdate) { + try { + log(`Updating database ${db.name} ( ${db.$id} ) ...`); + await tablesDBUpdate({ + databaseId: db.$id, + name: db.name, + enabled: db.enabled, + parseOutput: false + }); + success(`Updated ${db.name} ( ${db.$id} )`); + } catch (e) { + error(`Failed to update database ${db.name} ( ${db.$id} ): ${e.message}`); + throw new Error(`Database sync failed during update of ${db.$id}. Some changes may have been applied.`); + } + }
🧹 Nitpick comments (2)
templates/cli/lib/commands/push.js.twig (2)
1716-1764: Reduce O(n²) scans with maps.Building a map of remote DBs avoids repeated array.find and scales better.
Apply:
- const changes = []; + const changes = []; const toCreate = []; const toUpdate = []; const toDelete = []; - // Check for deletions - remote DBs that aren't in local config + // Index remote by id + const remoteById = new Map(remoteTablesDBs.map(db => [db.$id, db])); + + // Check for deletions - remote DBs that aren't in local config for (const remoteDB of remoteTablesDBs) { - const localDB = localTablesDBs.find(db => db.$id === remoteDB.$id); + const localDB = localTablesDBs.find(db => db.$id === remoteDB.$id); if (!localDB) { toDelete.push(remoteDB); changes.push({ @@ - for (const localDB of localTablesDBs) { - const remoteDB = remoteTablesDBs.find(db => db.$id === localDB.$id); + for (const localDB of localTablesDBs) { + const remoteDB = remoteById.get(localDB.$id);
1892-1893: Differentiate 404 (expected) from real errors in tablesDB name sync.Only log-and-continue on 404; surface other errors.
Apply:
- } catch (err) { - log(`There was an error when pushing tables: ${err.message}`); + } catch (err) { + if (Number(err.code) === 404) { + log(`Tables DB ${databaseId} not found. Skipping name sync (possibly declined earlier).`); + } else { + error(`Failed to sync tables DB ${databaseId} name: ${err.message}`); + throw err; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/cli/lib/commands/push.js.twig(3 hunks)
🔇 Additional comments (2)
templates/cli/lib/commands/push.js.twig (2)
1828-1845: Resync flow after deletions looks good.Fetching fresh remote DBs and pruning local tables/tablesDB to match is correct. Nice.
58-62: Verify that imported modules exist or will be provided during build/runtime.After exhaustive repository search, the following modules referenced in lines 58–62 could not be located:
./tables-db(imported functions:tablesDBList,tablesDBDelete,tablesDBListTables,tablesDBUpdateTable)- Also missing:
./databases,./storage,./messaging,./teams,./projectsThese modules do not appear as
.js,.twig, or any other file in the repository. Note thatpull.js.twig(line 12) imports the same./tables-dbmodule, suggesting either: (1) modules are generated/injected at build time, (2) they are dynamically loaded, or (3) they require separate implementation.Confirm these modules exist or will be provided before merge to prevent runtime errors.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes