refactor(desktop): move dev state to per-worktree dev-config/ directory#1475
Closed
Kitenite wants to merge 3 commits into
Closed
refactor(desktop): move dev state to per-worktree dev-config/ directory#1475Kitenite wants to merge 3 commits into
Kitenite wants to merge 3 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes Superset home directory resolution in Changes
Sequence Diagram(s)sequenceDiagram
participant ElectronApp as Electron App
participant Env as app-environment
participant FS as Filesystem
participant Child as Child Process
ElectronApp->>Env: initialize (packaged/dev/env present)
Env->>FS: resolveSupersetHomeDir() -> check/create dir
FS-->>Env: dir path / ensure result
Env->>ElectronApp: export SUPERSET_HOME_DIR, APP_STATE_PATH, WINDOW_STATE_PATH
ElectronApp->>Child: spawn with env SUPERSET_HOME_DIR
Child->>FS: read/write sockets, tokens, pid under SUPERSET_HOME_DIR
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.superset/setup.sh:
- Around line 515-526: The script silently skips WAL checkpointing and migration
re-application when sqlite3 is missing; add an else branch to the existing if
command -v sqlite3 check so that when sqlite3 is not found it emits a clear
warning referencing the variables dest_db and migrations_dir (e.g., "warning:
sqlite3 not installed; cannot checkpoint $dest_db or apply migrations in
$migrations_dir"), so callers know the DB may have stale WAL or an out-of-date
schema; keep current behavior otherwise (do not change the existing sqlite3
commands or their || true handling).
Comment on lines
+515
to
+526
| if command -v sqlite3 &> /dev/null; then | ||
| sqlite3 "$dest_db" "PRAGMA wal_checkpoint(TRUNCATE);" &> /dev/null || true | ||
|
|
||
| # Source DB may be from a different branch with divergent migrations. | ||
| # Re-apply all migration files so the schema matches this branch. | ||
| local migrations_dir="packages/local-db/drizzle" | ||
| if [ -d "$migrations_dir" ]; then | ||
| for migration in "$migrations_dir"/0*.sql; do | ||
| sqlite3 "$dest_db" < "$migration" 2>/dev/null || true | ||
| done | ||
| fi | ||
| fi |
Contributor
There was a problem hiding this comment.
No feedback when sqlite3 is unavailable — DB may be left with stale WAL and wrong schema.
When sqlite3 is not found, both the WAL checkpoint and migration re-application are silently skipped. The seeded DB could contain uncommitted WAL data and a schema that doesn't match the current branch, which is exactly the scenario the migration loop is meant to fix. A warn here would save debugging time.
Proposed fix
# Checkpoint the copy's WAL (no lock contention since nothing else has it open)
if command -v sqlite3 &> /dev/null; then
sqlite3 "$dest_db" "PRAGMA wal_checkpoint(TRUNCATE);" &> /dev/null || true
# Source DB may be from a different branch with divergent migrations.
# Re-apply all migration files so the schema matches this branch.
local migrations_dir="packages/local-db/drizzle"
if [ -d "$migrations_dir" ]; then
for migration in "$migrations_dir"/0*.sql; do
sqlite3 "$dest_db" < "$migration" 2>/dev/null || true
done
fi
+ else
+ warn "sqlite3 not found — skipping WAL checkpoint and migration replay on seeded DB"
fi🤖 Prompt for AI Agents
In @.superset/setup.sh around lines 515 - 526, The script silently skips WAL
checkpointing and migration re-application when sqlite3 is missing; add an else
branch to the existing if command -v sqlite3 check so that when sqlite3 is not
found it emits a clear warning referencing the variables dest_db and
migrations_dir (e.g., "warning: sqlite3 not installed; cannot checkpoint
$dest_db or apply migrations in $migrations_dir"), so callers know the DB may
have stale WAL or an out-of-date schema; keep current behavior otherwise (do not
change the existing sqlite3 commands or their || true handling).
Kitenite
force-pushed
the
kitenite/side
branch
4 times, most recently
from
bfe6aec to
a91cadf
Compare
Kitenite
force-pushed
the
kitenite/side
branch
from
a91cadf to
4522318
Compare
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
{worktree}/dev-config/instead of~/.superset-{workspace}/gh~/.superset)Changes
app-environment.ts:SUPERSET_HOME_DIRresolves to{worktree}/dev-config/when!app.isPackaged. Propagated to child processes viaprocess.env.SUPERSET_HOME_DIRterminal-host/index.ts(daemon): ReadsSUPERSET_HOME_DIRfrom inherited env var, falling back to~/.supersetfor productionterminal-host/client.ts: ImportsSUPERSET_HOME_DIRfromapp-environmentinstead of constructing inlineterminal-history.ts:getTerminalHistoryRootDir()uses importedSUPERSET_HOME_DIRsession-store.ts: Chat session store path uses importedSUPERSET_HOME_DIRsetup.ts: User config path uses importedSUPERSET_HOME_DIRshell-wrappers.ts: Shell scripts bake in absoluteSUPERSET_HOME_DIR/binpath instead of$HOME/.superset/bin.gitignore: Addeddev-config/Test Plan
bun run typecheckpassesbun run lint:fixreports no issuesdev-config/is created at worktree root with expected files (local.db, app-state.json, terminal-host.sock, etc.)dev-config/terminal-host.sock)Summary by CodeRabbit
New Features
Chores