fix(components): AbortController lifecycle + focus traps for interactive controls#118
Merged
Conversation
…ive controls DepthControl, ControlsBentoCell, GovernancePreview, and ShibuiOnboarding re-bound listeners on every astro:page-load/after-swap with no AbortController, accumulating handlers on persistent elements (e.g. the header DepthControl button) and leaking a document keydown in the onboarding modal. - All four now scope listeners to an AbortController that is aborted and recreated on each page-load (DepthControl/ControlsBentoCell/Governance) or torn down on before-swap (Onboarding), matching the project's established lifecycle pattern enforced by listener-lifecycle.test.ts. - ShibuiOnboarding + GovernancePreview: added/retained Escape + Tab focus traps with initial focus and focus restore on close. - GovernancePreview: reset document.body.style.overflow on before-swap so navigating away with the modal open can't leave the next page scroll-locked. Verified: lint, typecheck:strict (0 hints), build, component tests 72/72 (incl. listener-lifecycle). Focus-trap behavior is implemented to the standard pattern but not browser-verified here. https://claude.ai/code/session_01PW9DnyVijUNmUJ4qMfgpHn
Reviewer's GuideImplements AbortController-scoped event listener lifecycles for four interactive controls, adds robust focus-trap and focus-restore behavior to onboarding and governance modals, and ensures scroll-lock is properly cleaned up across Astro page transitions. Sequence diagram for ShibuiOnboarding focus-trap and lifecyclesequenceDiagram
title ShibuiOnboarding focus-trap and lifecycle
actor User
participant Document
participant ShibuiOnboarding as ShibuiOnboardingPanel
participant OnboardingController as AbortController
Document->>ShibuiOnboarding: initOnboarding
ShibuiOnboarding->>OnboardingController: new AbortController
ShibuiOnboarding->>Document: addEventListener keydown { signal }
ShibuiOnboarding->>ShibuiOnboarding: store previouslyFocused
ShibuiOnboarding->>ShibuiOnboarding: collect focusables
ShibuiOnboarding->>ShibuiOnboarding: panel.classList.add visible
ShibuiOnboarding->>ShibuiOnboarding: focus first focusable
rect rgb(230,230,250)
User->>Document: keydown Escape
Document->>ShibuiOnboarding: dismiss
ShibuiOnboarding->>OnboardingController: abort
ShibuiOnboarding->>ShibuiOnboarding: panel.classList.add hiding
ShibuiOnboarding->>ShibuiOnboarding: panel.remove
ShibuiOnboarding->>User: previouslyFocused.focus
end
rect rgb(230,250,230)
User->>Document: keydown Tab
alt [focus outside panel]
Document->>ShibuiOnboarding: focus first focusable
else [Shift+Tab on first]
Document->>ShibuiOnboarding: focus last focusable
else [Tab on last]
Document->>ShibuiOnboarding: focus first focusable
end
end
rect rgb(250,230,230)
Document->>ShibuiOnboarding: teardownOnboarding (astro:before-swap)
ShibuiOnboarding->>OnboardingController: abort
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/components/shibui/ShibuiOnboarding.astro" line_range="144-147" />
<code_context>
+ { signal },
+ );
requestAnimationFrame(() => {
panel.classList.add('shibui-onboarding--visible');
+ focusables[0]?.focus();
});
}
</code_context>
<issue_to_address>
**suggestion:** Guard the rAF callback against the panel being dismissed before it runs.
If the onboarding is dismissed or the route changes before the `requestAnimationFrame` callback runs, this code may call `classList.add`/`focus()` on detached elements. Check `signal.aborted` or `panel.isConnected` inside the callback and return early before mutating or focusing, e.g.:
```ts
requestAnimationFrame(() => {
if (signal.aborted || !panel.isConnected) return;
panel.classList.add('shibui-onboarding--visible');
focusables[0]?.focus();
});
```
```suggestion
requestAnimationFrame(() => {
if (signal.aborted || !panel.isConnected) return;
panel.classList.add('shibui-onboarding--visible');
focusables[0]?.focus();
});
```
</issue_to_address>
### Comment 2
<location path="src/components/resume/GovernancePreview.astro" line_range="89-90" />
<code_context>
+ document.addEventListener('astro:before-swap', () => {
+ govController?.abort();
+ govController = null;
+ // Reset the scroll-lock in case we navigate away while the modal is open.
+ document.body.style.overflow = '';
+ });
</script>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Restoring `body.style.overflow` unconditionally may clobber a pre-existing overflow style.
This handler assumes the modal is the only code modifying `body.style.overflow`. If any other feature sets a non-empty overflow on `<body>`, this will be cleared on navigation. Consider capturing the prior value when you first apply the scroll lock and restoring that value here, instead of always assigning an empty string.
Suggested implementation:
```
// Track the body's previous overflow so we can restore it when removing scroll-lock.
let previousBodyOverflow: string | null = null;
document.addEventListener('astro:page-load', () => {
```
```
document.addEventListener('astro:before-swap', () => {
govController?.abort();
govController = null;
// Reset the scroll-lock in case we navigate away while the modal is open.
if (previousBodyOverflow !== null) {
document.body.style.overflow = previousBodyOverflow;
previousBodyOverflow = null;
}
});
```
`).
Here are the concrete edits:
<file_operations>
<file_operation operation="edit" file_path="src/components/resume/GovernancePreview.astro">
<<<<<<< SEARCH
document.addEventListener('astro:page-load', () => {
=======
// Track the body's previous overflow so we can restore it when removing scroll-lock.
let previousBodyOverflow: string | null = null;
document.addEventListener('astro:page-load', () => {
>>>>>>> REPLACE
<<<<<<< SEARCH
document.addEventListener('astro:before-swap', () => {
govController?.abort();
govController = null;
// Reset the scroll-lock in case we navigate away while the modal is open.
document.body.style.overflow = '';
});
=======
document.addEventListener('astro:before-swap', () => {
govController?.abort();
govController = null;
// Reset the scroll-lock in case we navigate away while the modal is open.
if (previousBodyOverflow !== null) {
document.body.style.overflow = previousBodyOverflow;
previousBodyOverflow = null;
}
});
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully implement the behavior you described, you should also:
1. Locate the code in this component (or its helpers such as `setupGovPreview`) that applies the scroll lock, e.g. any `document.body.style.overflow = 'hidden'` or similar.
2. Before changing `document.body.style.overflow`, capture the existing value if it hasn't been captured yet:
- `if (previousBodyOverflow === null) previousBodyOverflow = document.body.style.overflow;`
- Then apply the scroll lock: `document.body.style.overflow = 'hidden';`
3. When the modal is closed (not just on navigation), restore `previousBodyOverflow` in the same way as in the `astro:before-swap` handler and reset it to `null` so the next lock cycle correctly recaptures the current value.
This ensures you never clobber a pre-existing overflow style and that the value is properly restored on both modal close and page navigation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
144
to
147
| requestAnimationFrame(() => { | ||
| panel.classList.add('shibui-onboarding--visible'); | ||
| focusables[0]?.focus(); | ||
| }); |
There was a problem hiding this comment.
suggestion: Guard the rAF callback against the panel being dismissed before it runs.
If the onboarding is dismissed or the route changes before the requestAnimationFrame callback runs, this code may call classList.add/focus() on detached elements. Check signal.aborted or panel.isConnected inside the callback and return early before mutating or focusing, e.g.:
requestAnimationFrame(() => {
if (signal.aborted || !panel.isConnected) return;
panel.classList.add('shibui-onboarding--visible');
focusables[0]?.focus();
});
Suggested change
| requestAnimationFrame(() => { | |
| panel.classList.add('shibui-onboarding--visible'); | |
| focusables[0]?.focus(); | |
| }); | |
| requestAnimationFrame(() => { | |
| if (signal.aborted || !panel.isConnected) return; | |
| panel.classList.add('shibui-onboarding--visible'); | |
| focusables[0]?.focus(); | |
| }); |
Comment on lines
+89
to
+90
| // Reset the scroll-lock in case we navigate away while the modal is open. | ||
| document.body.style.overflow = ''; |
There was a problem hiding this comment.
suggestion (bug_risk): Restoring body.style.overflow unconditionally may clobber a pre-existing overflow style.
This handler assumes the modal is the only code modifying body.style.overflow. If any other feature sets a non-empty overflow on <body>, this will be cleared on navigation. Consider capturing the prior value when you first apply the scroll lock and restoring that value here, instead of always assigning an empty string.
Suggested implementation:
// Track the body's previous overflow so we can restore it when removing scroll-lock.
let previousBodyOverflow: string | null = null;
document.addEventListener('astro:page-load', () => {
document.addEventListener('astro:before-swap', () => {
govController?.abort();
govController = null;
// Reset the scroll-lock in case we navigate away while the modal is open.
if (previousBodyOverflow !== null) {
document.body.style.overflow = previousBodyOverflow;
previousBodyOverflow = null;
}
});
`).
Here are the concrete edits:
<file_operations>
<file_operation operation="edit" file_path="src/components/resume/GovernancePreview.astro">
<<<<<<< SEARCH
document.addEventListener('astro:page-load', () => {
// Track the body's previous overflow so we can restore it when removing scroll-lock.
let previousBodyOverflow: string | null = null;
document.addEventListener('astro:page-load', () => {
REPLACE
<<<<<<< SEARCH
document.addEventListener('astro:before-swap', () => {
govController?.abort();
govController = null;
// Reset the scroll-lock in case we navigate away while the modal is open.
document.body.style.overflow = '';
});
document.addEventListener('astro:before-swap', () => {
govController?.abort();
govController = null;
// Reset the scroll-lock in case we navigate away while the modal is open.
if (previousBodyOverflow !== null) {
document.body.style.overflow = previousBodyOverflow;
previousBodyOverflow = null;
}
});
REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully implement the behavior you described, you should also:
- Locate the code in this component (or its helpers such as
setupGovPreview) that applies the scroll lock, e.g. anydocument.body.style.overflow = 'hidden'or similar. - Before changing
document.body.style.overflow, capture the existing value if it hasn't been captured yet:if (previousBodyOverflow === null) previousBodyOverflow = document.body.style.overflow;- Then apply the scroll lock:
document.body.style.overflow = 'hidden';
- When the modal is closed (not just on navigation), restore
previousBodyOverflowin the same way as in theastro:before-swaphandler and reset it tonullso the next lock cycle correctly recaptures the current value.
This ensures you never clobber a pre-existing overflow style and that the value is properly restored on both modal close and page navigation.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces AbortController logic across multiple interactive components—ControlsBentoCell, GovernancePreview, DepthControl, and ShibuiOnboarding—to properly manage event listeners during page transitions. It also enhances the onboarding modal with focus trapping and better lifecycle management. The review feedback highlights the need for consistent adherence to the project's style guide regarding Astro lifecycle events, specifically recommending the use of astro:page-load for initialization and ensuring astro:before-swap is used for explicit teardown in all components to prevent memory leaks or stale event handlers.
Comment on lines
156
to
+157
| document.addEventListener('astro:after-swap', initOnboarding); | ||
| document.addEventListener('astro:before-swap', teardownOnboarding); |
There was a problem hiding this comment.
For consistency with the other interactive components in this PR and to adhere to the project's preferred pattern (as noted in the Repository Style Guide, line 65), consider using astro:page-load instead of separate initOnboarding and astro:after-swap calls. This centralizes initialization and ensures it runs correctly both on initial load and after view transitions.
document.addEventListener('astro:page-load', initOnboarding);
document.addEventListener('astro:before-swap', teardownOnboarding);
References
- Uses Astro's . Use astro:page-load for initialization and astro:before-swap for teardown. (link)
Comment on lines
+124
to
+128
| document.addEventListener('astro:page-load', () => { | ||
| bentoController?.abort(); | ||
| bentoController = new AbortController(); | ||
| initBentoViewToggle(bentoController.signal); | ||
| }); |
There was a problem hiding this comment.
While astro:page-load correctly handles re-initialization and prevents handler accumulation, it is best practice to also provide an explicit teardown on astro:before-swap to ensure total cleanup when navigating away from the page, especially since this component uses a module-level AbortController.
document.addEventListener('astro:page-load', () => {
bentoController?.abort();
bentoController = new AbortController();
initBentoViewToggle(bentoController.signal);
});
document.addEventListener('astro:before-swap', () => {
bentoController?.abort();
bentoController = null;
});
References
- Use astro:page-load for initialization and astro:before-swap for teardown. (link)
Comment on lines
186
to
191
| document.addEventListener('astro:page-load', () => { | ||
| initDepthControl(); | ||
| initBridgeExpanders(); | ||
| depthController?.abort(); | ||
| depthController = new AbortController(); | ||
| initDepthControl(depthController.signal); | ||
| initBridgeExpanders(depthController.signal); | ||
| }); |
There was a problem hiding this comment.
Similar to ControlsBentoCell, this component should implement an astro:before-swap listener to explicitly abort the depthController when the page is swapped. This ensures that all listeners (including those on the global document object, if any are added in the future) are properly cleaned up.
document.addEventListener('astro:page-load', () => {
depthController?.abort();
depthController = new AbortController();
initDepthControl(depthController.signal);
initBridgeExpanders(depthController.signal);
});
document.addEventListener('astro:before-swap', () => {
depthController?.abort();
depthController = null;
});
References
- Use astro:page-load for initialization and astro:before-swap for teardown. (link)
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
2 participants
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.
Addresses the lifecycle/focus-trap findings in four interactive controls.
All four re-bound listeners on every
astro:page-load/after-swapwith no AbortController, accumulating handlers on persistent elements (notably the header's DepthControl button) and leaking adocumentkeydown in the onboarding modal.page-load(matches the project's enforced pattern).before-swap; added a guard against double-wiring.document.body.style.overflowonbefore-swap, so navigating away with the modal open can't leave the next page scroll-locked.Test plan
npm run lint/typecheck:strict— clean / 0 hintsnpm run buildlistener-lifecycle.test.ts)https://claude.ai/code/session_01PW9DnyVijUNmUJ4qMfgpHn
Generated by Claude Code
Summary by Sourcery
Ensure interactive controls clean up event listeners correctly across Astro page transitions and improve modal focus handling.
Bug Fixes:
Enhancements: