Fix build: restore wa-automate SessionManager + Windows rimraf globs#3329
Open
t0g3pii wants to merge 5 commits into
Open
Fix build: restore wa-automate SessionManager + Windows rimraf globs#3329t0g3pii wants to merge 5 commits into
t0g3pii wants to merge 5 commits into
Conversation
Re-add the wa-automate SessionManager source (previously ignored via a broad .gitignore rule) and make node-red clean scripts work on Windows by enabling rimraf globbing. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds exported SessionManager (local compression + optional S3 backup/restore), anchors /session/ and /newsession/ in .gitignore and ignores *.mdx, updates Node-RED clean scripts to use rimraf --glob, and removes legacy exclusions from root turbo scripts. ChangesSession Management System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Author
|
Just a quick hint here, i have NOT removed the filters for this pr, its ur choice if u remove them since theyre doing trouble currently |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Line 72: The .gitignore entry "*.mdx" is unclear since no .mdx files exist;
either remove the "*.mdx" pattern from .gitignore if it's unnecessary or retain
it but explicitly document its purpose in the PR description (e.g., "preventive
rule for future MDX build artifacts or editor-generated files") so reviewers
know it's intentional—refer to the "*.mdx" pattern when making the change and
update the PR description accordingly.
In `@packages/wa-automate/src/session/SessionManager.ts`:
- Line 11: SessionManagerConfig currently declares an unused compressionOptions
field; either remove compressionOptions from the SessionManagerConfig type (and
any callers relying on it) or, if you intended it to control compression, add
compressionOptions to the CompressionOptions interface and thread it through
createFromConfig so it is passed into LocalSessionCompression when constructing
compression (update the createFromConfig call site that currently hardcodes '-1
-T0' to use the new config value and ensure LocalSessionCompression's
constructor accepts and uses that option).
- Around line 109-121: startPeriodicSync currently discards the setInterval
handle causing timers to leak and preventing clearInterval in stop; modify
startPeriodicSync to store the timer id on the instance (e.g., this.syncTimer)
and ensure stop clears it (clearInterval/clearTimeout) and nulls the reference
when this.s3Sync is disabled; also prevent overlapping backups by replacing
setInterval with a recursive setTimeout or add a running flag inside
startPeriodicSync to skip if backupSession is still in-flight (use the same
this.backupSession() reference) so concurrent writes to S3 cannot occur and
captured references can be released for GC.
- Around line 123-134: Replace the unsafe double-cast and the naive
enableS3Backup gate in SessionManager.createFromConfig: instead of doing
clientConfig.s3Sync as unknown as S3Config, validate/parse clientConfig.s3Sync
against the expected S3Config shape (or a Zod schema) and only construct an
S3Config object when required credentials (bucket, region, accessKeyId,
secretAccessKey) are present; set enableS3Backup to true only if the
parsed/validated S3Config is non-null and contains those required fields, and
pass that validated S3Config into the SessionManager constructor so
S3SyncManager receives a correctly-shaped config (use createFromConfig,
SessionManager, S3Config, S3SyncManager, and enableS3Backup as anchors).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7337363-3ba5-46a1-bd12-d1a06da53565
📒 Files selected for processing (3)
.gitignoreintegrations/node-red/package.jsonpackages/wa-automate/src/session/SessionManager.ts
Parse and validate s3Sync into an S3Config only when required credentials are present; enableS3Backup is now gated on validated config. Co-authored-by: Cursor <cursoragent@cursor.com>
Store the periodic S3 sync timer handle so stop() can clear it, and avoid overlapping backupSession calls by scheduling via setTimeout with an in-flight guard. Also drop unused compressionOptions config field. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace single-quoted turbo filters (breaks on Windows) and remove filters for non-existent legacy packages; keep docs excluded via filter on the actual docs workspace package. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/wa-automate/src/session/SessionManager.ts`:
- Around line 39-43: SessionManager currently starts S3SyncManager and periodic
uploads when config.enableS3Backup is true even if local compression is
disabled, but backupSession() always uploads the .data.zst file so periodic sync
will target a non-existent file; update the S3 startup guard in the constructor
(the branch that assigns this.s3Sync and calls this.startPeriodicSync) to
require both config.enableS3Backup and config.enableLocalCompression (or check
that LocalSessionCompression was initialized) before creating S3SyncManager and
calling startPeriodicSync, and ensure any code that references this.s3Sync
(e.g., startPeriodicSync, backupSession) handles the null/undefined s3Sync case
safely.
- Around line 46-60: The stop() method should perform cleanup on a best-effort
basis so one failing operation doesn't prevent others from running: wrap the
await this.localCompression.stop() call in its own try/catch (or use
Promise.resolve(...).catch(...)) so any rejection is logged/ignored and then
continue to clear this.syncTimer, set this.syncInFlight = false, and null out
this.localCompression and this.s3Sync; ensure each cleanup step
(localCompression.stop, clearTimeout(this.syncTimer), nulling this.s3Sync) runs
independently so the timeout and s3Sync aren't left alive if
LocalSessionCompression.stop() throws.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37e7d821-b99b-484e-b356-5bb3de6558bb
📒 Files selected for processing (1)
packages/wa-automate/src/session/SessionManager.ts
Make stop() best-effort so one failure doesn't prevent timer/S3 cleanup, and only enable S3 periodic uploads when local compression is enabled (since backups upload the .data.zst artifact). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
30-30: 💤 Low valueMinor inconsistency:
turbo run buildvsturbo build.All other task scripts use the shorthand
turbo build, butpublish-packagesstill saysturbo run build. Both are equivalent in Turbo v2, but consistency reduces confusion.♻️ Proposed fix
- "publish-packages": "turbo run build --filter=\"!docs\" && changeset publish", + "publish-packages": "turbo build --filter=\"!docs\" && changeset publish",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 30, The package.json "publish-packages" script is inconsistent with other scripts: it uses "turbo run build" instead of the shorthand "turbo build"; update the "publish-packages" script's command string (the value for the "publish-packages" key) to use "turbo build --filter=\"!docs\" && changeset publish" so it matches the rest of the scripts' style and tooling conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@package.json`:
- Line 30: The package.json "publish-packages" script is inconsistent with other
scripts: it uses "turbo run build" instead of the shorthand "turbo build";
update the "publish-packages" script's command string (the value for the
"publish-packages" key) to use "turbo build --filter=\"!docs\" && changeset
publish" so it matches the rest of the scripts' style and tooling conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9aea1f67-8827-4240-ab97-523471b6efd0
📒 Files selected for processing (2)
package.jsonpackages/wa-automate/src/session/SessionManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wa-automate/src/session/SessionManager.ts
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
@open-wa/wa-automatebuilds again.@open-wa/node-redclean scripts Windows-safe by enabling rimraf globbing.s3Syncinto a realS3Configonly when required credentials exist.stop()to be best-effort so timer/S3 cleanup still runs if local compression stop fails.docsworkspace package.Test plan
pnpm -C packages/wa-automate run buildpnpm -w run buildSummary by CodeRabbit
New Features
Chores