fix(extmgr): support Pi 0.70 session startup#16
Conversation
📝 WalkthroughWalkthroughThe extensions manager is updated to work with Pi API v0.70.6, which deprecated the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/auto-update.test.ts (1)
76-151:⚠️ Potential issue | 🟡 MinorAdd
session_shutdowncall to test the replacement session lifecycle.The test title states "replacement session" but doesn't exercise the actual replacement flow. Per Pi's session lifecycle, when a session is replaced,
session_shutdownis fired on the old session first, thensession_starton the new one. Your code registers asession_shutdownhandler that stops the timer (insrc/index.ts), but the test never invokes it, leaving that cleanup path untested. Additionally, whensession_startreceivesreason: "resume", Pi includespreviousSessionFilein the payload, which the test omits.🧪 Suggested test adjustment
- await handlers.session_start?.({}, enabledCtx); + await handlers.session_start?.({ reason: "startup" }, enabledCtx); assert.equal(isAutoUpdateRunning(), true); - await handlers.session_start?.({ reason: "resume" }, disabledCtx); + await handlers.session_shutdown?.({ reason: "switch" }, enabledCtx); + await handlers.session_start?.( + { reason: "resume", previousSessionFile: "/tmp/previous-session.pi" }, + disabledCtx, + ); assert.equal(isAutoUpdateRunning(), false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/auto-update.test.ts` around lines 76 - 151, The test should simulate the full replacement lifecycle by invoking the registered session_shutdown on the old session before starting the new one and include previousSessionFile in the resume payload; specifically, call the stored handlers.session_shutdown with the enabledCtx (to trigger the timer-stop cleanup path in your extension's session_shutdown handler) and then call handlers.session_start with { reason: "resume", previousSessionFile: "<some path>" } and disabledCtx so the replacement path in session_start is exercised and the auto-update timer is correctly stopped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/auto-update.test.ts`:
- Around line 76-151: The test should simulate the full replacement lifecycle by
invoking the registered session_shutdown on the old session before starting the
new one and include previousSessionFile in the resume payload; specifically,
call the stored handlers.session_shutdown with the enabledCtx (to trigger the
timer-stop cleanup path in your extension's session_shutdown handler) and then
call handlers.session_start with { reason: "resume", previousSessionFile: "<some
path>" } and disabledCtx so the replacement path in session_start is exercised
and the auto-update timer is correctly stopped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31127b59-3ba7-4f03-a79a-9f559d05dac9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonsrc/index.tstest/auto-update.test.ts
💤 Files with no reviewable changes (1)
- src/index.ts
|
lgtm |
Summary
session_switchextension hook and rely onsession_startfor session bootstrapping.session_start.Closes #15.
Verification
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores