Skip to content

feat: add onSessionStart/onBeforeSessionEnd/onAfterSessionEnd plugin hooks#275

Merged
jagadeshjai merged 1 commit intomainfrom
jagaesh/feat__plugin_session_lifecycle_hooks
Apr 2, 2026
Merged

feat: add onSessionStart/onBeforeSessionEnd/onAfterSessionEnd plugin hooks#275
jagadeshjai merged 1 commit intomainfrom
jagaesh/feat__plugin_session_lifecycle_hooks

Conversation

@jagadeshjai
Copy link
Copy Markdown
Contributor

Description

Introduces three new lifecycle hooks to BasePlugin/PluginManager/CDPService that fire at precise points around a session boundary:

  • onSessionStart: fires before any launch/reuse work begins
  • onBeforeSessionEnd: fires before shutdown starts
  • onAfterSessionEnd: fires after all teardown is complete

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Performance improvement
  • Test addition/update

Related Issues

Closes #(issue number)
Related to #(issue number)

Changes Made

  • List specific changes made
  • Include any new files or major modifications
  • Mention any removed functionality

Testing

  • I have tested this locally
  • I have added/updated unit tests
  • I have added/updated integration tests
  • I have tested with Docker
  • All existing tests pass

Documentation

  • I have updated relevant documentation
  • I have added JSDoc comments for new public APIs
  • I have updated the README if needed
  • I have updated the CHANGELOG if needed

Code Quality

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings or errors

Breaking Changes

If this is a breaking change, please describe:

  1. What breaks:
  2. How users should migrate:
  3. Why this change is necessary:

Screenshots (if applicable)

Include screenshots or GIFs for UI changes.

Additional Notes

Any additional information, concerns, or context for reviewers.


Reviewer Checklist

  • Code follows project conventions and style
  • Changes are well-tested
  • Documentation is updated appropriately
  • No security concerns
  • Performance impact is acceptable
  • Breaking changes are properly documented

this.logger,
);

await this.pluginManager.onAfterSessionEnd(sessionConfig);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be in a finally block incase teardown/relaunch throws

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I've handled the teardown block throws, the idle browser relaunch feels like an independent operational failure unrelated to whether the session ended cleanly or to session itself, so I kept it outside the finally block. WDYT?

@jagadeshjai jagadeshjai force-pushed the jagaesh/feat__plugin_session_lifecycle_hooks branch from 2d86162 to f764c1a Compare April 1, 2026 09:51
@jagadeshjai jagadeshjai requested a review from danew April 2, 2026 06:18
Comment on lines +1280 to +1286
} catch (error) {
await this.pluginManager
.onAfterSessionEnd(sessionConfig)
.catch((e) =>
this.logger.error(`[CDPService] onAfterSessionEnd cleanup after failed launch: ${e}`),
);
throw error;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a finally here works, no need to re-throw. Also the catch shouldn't be needed here either

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With finally it would incorrectly call the onAfterSessionEnd on successful launch when the session is still active right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol valid point, ignore

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need to await the this.launch(sessionConfig);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@jagadeshjai jagadeshjai force-pushed the jagaesh/feat__plugin_session_lifecycle_hooks branch 2 times, most recently from 960d8f8 to 362b302 Compare April 2, 2026 14:27
…hooks

Introduces three new lifecycle hooks to BasePlugin/PluginManager/CDPService
that fire at precise points around a session boundary:

- onSessionStart: fires before any launch/reuse work begins
- onBeforeSessionEnd: fires before shutdown starts
- onAfterSessionEnd: fires after all teardown is complete
@jagadeshjai jagadeshjai force-pushed the jagaesh/feat__plugin_session_lifecycle_hooks branch from 362b302 to 9be34c8 Compare April 2, 2026 17:05
@jagadeshjai jagadeshjai merged commit 8ccf2d4 into main Apr 2, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants