feat(plugin-stack-persistence): navigation context persistence plugin#727
Merged
Conversation
🦋 Changeset detectedLatest commit: 97e4a61 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds the new ChangesStack Persistence Plugin
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StackflowCore
participant stackPersistencePlugin
participant StackSnapshotStorage
participant StackSnapshotStrategy
StackflowCore->>stackPersistencePlugin: provideSnapshot(initialContext)
stackPersistencePlugin->>StackSnapshotStorage: load()
StackSnapshotStorage-->>stackPersistencePlugin: snapshot record or null
stackPersistencePlugin->>StackSnapshotStrategy: shouldReuse(record, initialContext)
StackSnapshotStrategy-->>stackPersistencePlugin: reuse decision
stackPersistencePlugin-->>StackflowCore: snapshot or null
StackflowCore->>stackPersistencePlugin: idle initialization or change
stackPersistencePlugin->>StackSnapshotStrategy: createMetadata(snapshot)
stackPersistencePlugin->>StackSnapshotStorage: save(record)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
Deploying stackflow-demo with
|
| Latest commit: |
97e4a61
|
| Status: | ⚡️ Build in progress... |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | 97e4a61 | Commit Preview URL | Jul 16 2026, 03:48 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
extensions/plugin-stack-persistence/src/__fixtures__/isolatedProcess.ts (1)
10-11: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueShared static
OUT_DIR+ whole-directory cleanup is a latent race if invoked concurrently.
outfileis keyed only bychildFileNameunder a fixedOUT_DIR, andfinallyremoves the entire directory recursively. Currently the two call sites run sequentially in one spec file, so it's inert, but any futuretest.concurrent/parallel invocation ofrunIsolatedChild(same or different fixtures) could have one call'srmSyncdelete another's in-flight build/output. Consider a per-invocation unique subdirectory (e.g.fs.mkdtempSync) instead of a shared static path.Also applies to: 28-31, 83-85
🤖 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 `@extensions/plugin-stack-persistence/src/__fixtures__/isolatedProcess.ts` around lines 10 - 11, Update runIsolatedChild to create a unique temporary output directory for each invocation, using the existing filesystem utilities such as fs.mkdtempSync under PACKAGE_ROOT or the harness output root. Build outfile paths from that per-invocation directory and keep cleanup scoped to only the directory created by that invocation, removing reliance on the shared static OUT_DIR.
🤖 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 `@extensions/plugin-stack-persistence/package.json`:
- Line 30: Update the package’s dev script to start the JavaScript build
watcher, declaration build watcher, and test watcher concurrently instead of
chaining them with &&. Ensure the test command invokes Vitest’s watch mode
directly rather than appending --watch to a vitest run command, using the
existing script symbols where possible.
In `@extensions/plugin-stack-persistence/src/__fixtures__/assertions.ts`:
- Around line 110-139: Update installBrowserGlobalTraps to validate every name
in BANNED_BROWSER_GLOBALS for pre-existing globalThis properties in a complete
first pass, throwing before any defineProperty calls when validation fails; then
perform the existing trap-installation pass and return the uninstall handler.
In `@extensions/plugin-stack-persistence/src/__fixtures__/isolatedProcess.ts`:
- Around line 52-60: Update the env configuration in the spawnSync call to
preserve any inherited process.env.NODE_OPTIONS while appending the required
.pnp.cjs --require option. Keep the existing PnP requirement and ensure the
resulting value remains valid when NODE_OPTIONS is unset.
In `@extensions/plugin-stack-persistence/src/preservationScope.spec.ts`:
- Around line 194-197: Update the assertion in preservationScope.spec.ts to
locate the Home activity by its contractual identity rather than assuming it is
at activities[0]. Preserve the existing checks for the activity name and
restored params, while avoiding reliance on raw activities-array ordering.
In `@extensions/plugin-stack-persistence/src/stackPersistencePlugin.ts`:
- Around line 136-140: Update the save flow around storage.save in the stack
persistence plugin so synchronous exceptions are caught and routed through the
same save-error policy as rejected promises, producing StackPersistenceSaveError
without synchronously unwinding initialization or navigation. Preserve the
existing asynchronous rejection handling and policy behavior for both failure
paths.
---
Nitpick comments:
In `@extensions/plugin-stack-persistence/src/__fixtures__/isolatedProcess.ts`:
- Around line 10-11: Update runIsolatedChild to create a unique temporary output
directory for each invocation, using the existing filesystem utilities such as
fs.mkdtempSync under PACKAGE_ROOT or the harness output root. Build outfile
paths from that per-invocation directory and keep cleanup scoped to only the
directory created by that invocation, removing reliance on the shared static
OUT_DIR.
🪄 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: 264afffb-23fa-4d23-847f-81a406d255a1
⛔ Files ignored due to path filters (55)
.yarn/cache/@esbuild-darwin-arm64-npm-0.28.1-b71e3b7721-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@esbuild-darwin-x64-npm-0.28.1-4318a1bdc6-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@esbuild-linux-arm64-npm-0.28.1-33ca543ef9-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@esbuild-linux-ia32-npm-0.28.1-1ab1ff7c72-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@esbuild-linux-x64-npm-0.28.1-f7b93afd3d-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@jridgewell-sourcemap-codec-npm-1.5.5-5189d9fc79-5d9d207b46.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@rollup-rollup-darwin-arm64-npm-4.62.2-23ccb466e4-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@rollup-rollup-darwin-x64-npm-4.62.2-02ad04466d-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@rollup-rollup-linux-arm64-gnu-npm-4.62.2-22a54d9321-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@rollup-rollup-linux-arm64-musl-npm-4.62.2-123c27c34b-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@rollup-rollup-linux-x64-gnu-npm-4.62.2-1774ccb0dc-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@rollup-rollup-linux-x64-musl-npm-4.62.2-bf1e2b555e-10.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@types-chai-npm-5.2.3-5f61dbddda-e79947307d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@types-deep-eql-npm-4.0.2-e6bc68cc92-249a27b0bb.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@types-estree-npm-1.0.9-63428f58ff-16aabfa703.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@vitest-expect-npm-3.2.7-287b64f32d-2621cf1e90.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@vitest-mocker-npm-3.2.7-6daa3361b9-fccb77857a.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@vitest-pretty-format-npm-3.2.7-fb6e627f7c-81286f667f.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@vitest-runner-npm-3.2.7-101792a6a3-ff7c14726c.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@vitest-snapshot-npm-3.2.7-59081b8193-8d1bc49c1d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@vitest-spy-npm-3.2.7-407a36bb46-a125bff0ec.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@vitest-utils-npm-3.2.7-c519404dea-d17fda64b6.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/assertion-error-npm-2.0.1-8169d136f2-a0789dd882.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/chai-npm-5.3.3-ebef71cdac-0d0ef63106.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/check-error-npm-2.1.3-e17bcf3ed8-f1868d3db6.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/debug-npm-4.4.3-0105c6123a-9ada3434ea.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/deep-eql-npm-5.0.2-3bce58289f-a529b81e2e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/es-module-lexer-npm-1.7.0-456b47a08a-b6f3e576a3.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/esbuild-npm-0.28.1-23da8d1b66-aaa4a92264.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/estree-walker-npm-3.0.3-0372979673-a65728d572.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/expect-type-npm-1.4.0-aeee227f9a-bad91f4b7e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/fdir-npm-6.5.0-8814a0dec7-14ca1c9f0a.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/js-tokens-npm-9.0.1-3ed793c0c1-3288ba73bb.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/loupe-npm-3.2.1-a8f491982f-a4d78ec758.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/magic-string-npm-0.30.21-9a226cb21e-57d5691f41.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/nanoid-npm-3.3.15-2658de05f8-13c74a5208.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/pathval-npm-2.0.1-7fb9ae82ba-f5e8b82f6b.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/picomatch-npm-4.0.5-bb8e0de0f7-8bad770af9.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/postcss-npm-8.5.16-086b209555-b39408900a.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/rollup-npm-4.62.2-b7a75098ad-40a8551c1b.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/siginfo-npm-2.0.0-9bbac931f8-e93ff66c65.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/stackback-npm-0.0.2-73273dc92e-2d4dc4e64e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/std-env-npm-3.10.0-30d3e2646f-19c9cda4f3.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/strip-literal-npm-3.1.0-b0340463b3-6eb00906a1.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/tinybench-npm-2.9.0-2861a048db-cfa1e1418e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/tinyexec-npm-0.3.2-381b1e349c-b9d5fed316.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/tinyglobby-npm-0.2.17-f2c3ddb917-f85e8a217d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/tinypool-npm-1.1.1-6772421283-0d54139e9d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/tinyrainbow-npm-2.0.0-b4ba575b93-94d4e16246.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/tinyspy-npm-4.0.4-94a3f61e82-858a99e3de.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/vite-node-npm-3.2.4-cb1d79df3b-343244ecab.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/vite-npm-7.3.6-7e42869881-6fcbadb1a4.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/vitest-npm-3.2.7-232be925e6-b54cba8bbb.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/why-is-node-running-npm-2.3.0-011cf61a18-0de6e6cd8f.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (57)
.changeset/calm-stacks-persist.md.pnp.cjsCONTEXT.mddocs/adr/0001-recover-from-unusable-navigation-snapshots.mddocs/adr/0002-require-a-ready-snapshot-at-stack-creation.mddocs/adr/0003-use-one-asynchronous-snapshot-save-contract.mddocs/adr/0004-capture-only-idle-stacks.mddocs/adr/0005-inject-snapshot-metadata-and-a-reuse-gate.mddocs/design/fep-2546-navigation-context-persistence.mdextensions/plugin-stack-persistence/.gitignoreextensions/plugin-stack-persistence/README.mdextensions/plugin-stack-persistence/boundary-tests/consumer/consumer.tsextensions/plugin-stack-persistence/boundary-tests/consumer/consumerRuntime.cjsextensions/plugin-stack-persistence/boundary-tests/consumer/tsconfig.jsonextensions/plugin-stack-persistence/boundary-tests/packageBoundary.spec.tsextensions/plugin-stack-persistence/boundary-tests/tsconfig.jsonextensions/plugin-stack-persistence/esbuild.config.jsextensions/plugin-stack-persistence/package.jsonextensions/plugin-stack-persistence/src/StackSnapshotRecord.tsextensions/plugin-stack-persistence/src/StackSnapshotStorage.tsextensions/plugin-stack-persistence/src/StackSnapshotStrategy.tsextensions/plugin-stack-persistence/src/__fixtures__/assertions.tsextensions/plugin-stack-persistence/src/__fixtures__/clock.tsextensions/plugin-stack-persistence/src/__fixtures__/controlledStorage.tsextensions/plugin-stack-persistence/src/__fixtures__/isolated/childReport.tsextensions/plugin-stack-persistence/src/__fixtures__/isolated/handledSaveRejection.child.tsextensions/plugin-stack-persistence/src/__fixtures__/isolated/unhandledSaveRejection.child.tsextensions/plugin-stack-persistence/src/__fixtures__/isolatedProcess.tsextensions/plugin-stack-persistence/src/__fixtures__/observerPlugin.tsextensions/plugin-stack-persistence/src/__fixtures__/stackFixtures.tsextensions/plugin-stack-persistence/src/__fixtures__/strategySpy.tsextensions/plugin-stack-persistence/src/autoSave.spec.tsextensions/plugin-stack-persistence/src/errors.tsextensions/plugin-stack-persistence/src/index.tsextensions/plugin-stack-persistence/src/loadErrors.spec.tsextensions/plugin-stack-persistence/src/pluginObservability.spec.tsextensions/plugin-stack-persistence/src/preservationScope.spec.tsextensions/plugin-stack-persistence/src/responsibilityBoundaries.spec.tsextensions/plugin-stack-persistence/src/restoreOnStart.spec.tsextensions/plugin-stack-persistence/src/reuseStrategy.spec.tsextensions/plugin-stack-persistence/src/saveErrorPropagation.spec.tsextensions/plugin-stack-persistence/src/saveErrors.spec.tsextensions/plugin-stack-persistence/src/saveMetadata.spec.tsextensions/plugin-stack-persistence/src/ssr.spec.tsextensions/plugin-stack-persistence/src/stackPersistencePlugin.tsextensions/plugin-stack-persistence/tsconfig.jsonextensions/plugin-stack-persistence/type-tests/fixtures/errorContract.tsextensions/plugin-stack-persistence/type-tests/fixtures/helpers.tsextensions/plugin-stack-persistence/type-tests/fixtures/optionsMetadataInference.tsextensions/plugin-stack-persistence/type-tests/fixtures/optionsWithoutStrategy.tsextensions/plugin-stack-persistence/type-tests/fixtures/publicSurface.tsextensions/plugin-stack-persistence/type-tests/fixtures/recordAndStorage.tsextensions/plugin-stack-persistence/type-tests/fixtures/singleInjectionSurface.tsextensions/plugin-stack-persistence/type-tests/fixtures/strategyContract.tsextensions/plugin-stack-persistence/type-tests/tsconfig.jsonextensions/plugin-stack-persistence/type-tests/typeContracts.spec.tsextensions/plugin-stack-persistence/vitest.config.mts
ENvironmentSet
commented
Jul 13, 2026
ENvironmentSet
commented
Jul 13, 2026
ENvironmentSet
changed the title
…-surface stub (FEP-2546) Scaffold @stackflow/plugin-stack-persistence and pin its confirmed public contract as an executable harness ahead of the implementation: - Public surface stub: stackPersistencePlugin, StackSnapshotStorage/Record/ Strategy, StackPersistenceLoadError/SaveError with complete signatures; the plugin body throws until implemented, so the runtime suites fail red at runtime while everything compiles. - Runtime suite (vitest, node): restore-on-start, reuse strategy, load error policies, idle auto-save, save error handling, plugin/analytics observation, SSR neutrality, and responsibility boundaries — driven only through the package entrypoint and @stackflow/core public API, with deterministic fake-timer clocks and per-call deferred storage doubles. Unhandled-rejection contracts run in isolated node subprocesses. - Type suite: strict tsc fixtures for the public names, record/storage shape, metadata inference, strategy and error contracts, plus a control run that strips every @ts-expect-error and asserts the compile failures land exactly on the flagged lines. - Package-boundary suite: manifest/dist audit and a PnP subprocess that requires the built package by name in a React-free, browser-global-free process and connects it to a core store. The options type keeps the strategy branch as the sole Metadata inference site (NoInfer-equivalent alias) so mismatched storage/strategy metadata fails compilation instead of widening to a union. Current state: type suite 14/14 green, typecheck clean; runtime suite 68/68 red and boundary connection red via the unimplemented stub, which is the intended baseline for the implementation to turn green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…stack fields (FEP-2546) Three harness-layer corrections, none weakening the contracts under test: - Activity order: core sorts Stack.activities by activity id, so raw array order carries no navigation meaning. Assertions now compare navigation depth through zIndex (logicalStackView sorts by it and carries zIndex and enteredBy; multi-activity assertions use the navigation-order helpers). While correcting this, the round-trip case also pins the real params semantics: an activity's params are its current step's params, and the entry params stay distinguishable on the entry step and enteredBy event. - Paused fixture: current core replays only queued events on resume — an empty pause resumes to an unchanged stack. The paused snapshot now carries a navigation event recorded after Paused, so the restored stack has queued work and pause → resume → idle → save is observable: the queued entry stays unapplied while paused (no save), and resume applies it before the single unpaused-idle save. - Reuse call order: the load-phase exclusivity case now shares its call log with the strategy spy as intended; the log previously recorded only storage calls, failing even a correct implementation. Verified both ways: 68/68 pass against the implementation, and 68/68 fail via the unimplemented-stub throw when the plugin body is swapped back to the pre-implementation stub. Type suite 14/14, boundary suite 4/4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… its own fixture (FEP-2546) The queued-pause fixture introduced for the resume scenario had replaced the terminal-Paused snapshot everywhere, so no test fed the plugin a record whose last event is Paused — an implementation rejecting exactly those records passed the whole suite while violating the "no plugin-added Idle/tail-event load precondition" contract. Split the two semantic cases: pausedSnapshot() is terminal-Paused again and drives the envelope/paused pass-through case (with a fixture self-check pinning the Paused tail), while the queued variant lives on as resumablePausedSnapshot() for the pause → resume → idle → save case. Verified with cleared transform caches: 68/68 green on the implementation, 68/68 red on the pre-implementation stub, and a terminal-Paused-rejection mutant now fails exactly the pass-through test (67/68). Type suite 14/14, boundary suite 4/4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The run plan is a process document for the implementation run, not a shipped artifact. Repo discipline keeps process docs out of the tree (rationale lives in code comments and commits, transient process notes in the PR / issue tracker), so it should not land on main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ENvironmentSet
force-pushed
the
feature/fep-2546
branch
from
d7cdc31 to
97e4a61
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
extensions/plugin-stack-persistence/src/stackPersistencePlugin.ts (1)
46-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary optional chaining.
The
strategyobject is a required property inStackPersistencePluginOptions, so the optional chaining?.is unnecessary here.♻️ Proposed refactor
- if (strategy?.shouldReuse({ record, initialContext }) === false) + if (strategy.shouldReuse({ record, initialContext }) === false) return null;🤖 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 `@extensions/plugin-stack-persistence/src/stackPersistencePlugin.ts` around lines 46 - 47, Update the strategy check in the stack persistence plugin to call the required strategy object directly instead of using optional chaining, while preserving the existing shouldReuse comparison and null return behavior.
🤖 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 `@extensions/plugin-stack-persistence/package.json`:
- Line 29: Update the package.json dev script so the watch processes for
build:js and build:dts start concurrently instead of chaining with &&. Use
background execution with & as requested, or the existing concurrently utility
if the project already provides one.
In `@extensions/plugin-stack-persistence/src/stackPersistencePlugin.ts`:
- Around line 21-36: Update saveIfIdle to wrap snapshot capture,
strategy.createMetadata, and storage.save in a try/catch so synchronous failures
enter the same save-error handling path. Convert caught errors to
StackSnapshotRecordSaveError before invoking onRecordSaveError, and preserve the
rejected-Promise behavior when no handler is configured.
---
Nitpick comments:
In `@extensions/plugin-stack-persistence/src/stackPersistencePlugin.ts`:
- Around line 46-47: Update the strategy check in the stack persistence plugin
to call the required strategy object directly instead of using optional
chaining, while preserving the existing shouldReuse comparison and null return
behavior.
🪄 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: d5f4aca2-709f-4826-b162-93aebadd846a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.changeset/calm-stacks-persist.md.pnp.cjsextensions/plugin-stack-persistence/esbuild.config.jsextensions/plugin-stack-persistence/package.jsonextensions/plugin-stack-persistence/src/StackSnapshotRecord.tsextensions/plugin-stack-persistence/src/StackSnapshotStorage.tsextensions/plugin-stack-persistence/src/StackSnapshotStrategy.tsextensions/plugin-stack-persistence/src/errors.tsextensions/plugin-stack-persistence/src/index.tsextensions/plugin-stack-persistence/src/stackPersistencePlugin.tsextensions/plugin-stack-persistence/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/plugin-stack-persistence/esbuild.config.js
| "build:dts": "tsc --emitDeclarationOnly", | ||
| "build:js": "node ./esbuild.config.js", | ||
| "clean": "rimraf dist", | ||
| "dev": "yarn build:js --watch && yarn build:dts --watch", |
There was a problem hiding this comment.
dev script blocks on first --watch command.
The && operator runs commands sequentially. Since yarn build:js --watch is a long-running process, yarn build:dts --watch will never execute. Please run them concurrently in the background.
🔧 Proposed fix: use background `&` for parallel watch processes
- "dev": "yarn build:js --watch && yarn build:dts --watch",
+ "dev": "yarn build:js --watch & yarn build:dts --watch",If cross-platform support is needed, consider using concurrently (if installed) instead.
🤖 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 `@extensions/plugin-stack-persistence/package.json` at line 29, Update the
package.json dev script so the watch processes for build:js and build:dts start
concurrently instead of chaining with &&. Use background execution with & as
requested, or the existing concurrently utility if the project already provides
one.
Comment on lines
+21
to
+36
| const saveIfIdle = (actions: StackflowActions) => { | ||
| const stack = actions.getStack(); | ||
|
|
||
| if (stack.globalTransitionState !== "idle") return; | ||
|
|
||
| const snapshot = actions.captureSnapshot(); | ||
| const metadata = strategy.createMetadata({ snapshot }); | ||
|
|
||
| storage.save({ | ||
| snapshot, | ||
| metadata | ||
| }).catch(error => { | ||
| if (onRecordSaveError) return onRecordSaveError(error); | ||
| else throw error; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Fix type error and route synchronous exceptions through the save-error policy.
onRecordSaveError expects a StackSnapshotRecordSaveError but is currently receiving the raw error from the .catch block.
Additionally, strategy.createMetadata() and storage.save() can throw synchronously. Because they are not wrapped in a try/catch, any synchronous exceptions will bypass the .catch block and synchronously unwind onInit or onChanged, potentially crashing the navigation lifecycle.
Wrap the synchronous execution in a try/catch that converts errors to a rejected Promise, and ensure the error is properly wrapped in StackSnapshotRecordSaveError.
🔧 Proposed fix
- const saveIfIdle = (actions: StackflowActions) => {
- const stack = actions.getStack();
-
- if (stack.globalTransitionState !== "idle") return;
-
- const snapshot = actions.captureSnapshot();
- const metadata = strategy.createMetadata({ snapshot });
-
- storage.save({
- snapshot,
- metadata
- }).catch(error => {
- if (onRecordSaveError) return onRecordSaveError(error);
- else throw error;
- });
- }
+ const saveIfIdle = (actions: StackflowActions) => {
+ const stack = actions.getStack();
+
+ if (stack.globalTransitionState !== "idle") return;
+
+ let savePromise: Promise<void>;
+ try {
+ const snapshot = actions.captureSnapshot();
+ const metadata = strategy.createMetadata({ snapshot });
+
+ savePromise = storage.save({
+ snapshot,
+ metadata
+ });
+ } catch (error) {
+ savePromise = Promise.reject(error);
+ }
+
+ savePromise.catch(error => {
+ if (onRecordSaveError) {
+ onRecordSaveError(new StackSnapshotRecordSaveError(error));
+ } else {
+ throw error;
+ }
+ });
+ }🤖 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 `@extensions/plugin-stack-persistence/src/stackPersistencePlugin.ts` around
lines 21 - 36, Update saveIfIdle to wrap snapshot capture,
strategy.createMetadata, and storage.save in a try/catch so synchronous failures
enter the same save-error handling path. Convert caught errors to
StackSnapshotRecordSaveError before invoking onRecordSaveError, and preserve the
rejected-Promise behavior when no handler is configured.
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.
Adds
@stackflow/plugin-stack-persistence— a framework-neutral core plugin that persists complete Stackflow navigation snapshots and restores them on startup, so an app can reopen exactly where the user left off.