fix(dashboard): stop the expanded table row remounting on every poll - #558
Merged
Conversation
DataTable positions its inline detail panel in a portal host <tr> inserted next to the target row. The layout effect that does the insertion lists `rows` in its deps and its cleanup always removed the host, so every re-run built a fresh <tr>/<td> and re-pointed the portal at it. React answers that by unmounting the panel and mounting it into the new node, which replays the 180ms reveal animation and throws away the panel's state, including the routing-plan lookup. The activity page polls for in-flight requests every two seconds and hands the table a rebuilt rows array each time, so an operator who expanded a row while a request was running watched it flash and slide open again on a timer. Build the host once and keep it in a ref, so the portal target survives the effect re-running. The effect now only repositions it, and only when it is not already in place: re-inserting an attached node detaches and re-attaches its subtree, which cancels and restarts the animation running inside it. Detaching moves out of the effect cleanup (which runs on every dependency change) into the no-target branch plus a mount-scoped effect, so a vanished target still leaves nothing behind. Regression tests at both levels: a unit test that a no-op rows rebuild does not remount the panel, and a page-level test that expands a row with a request in flight and advances past two polls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
missing-template
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDataTable now reuses its detail-row portal host across effect reruns. Portal content remains mounted for the same detail key and remounts when the detail key changes. Regression tests cover copied rows, row changes, and ActivityPage polling. ChangesDetail panel persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
github-actions
Bot
removed
the
missing-template
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@web/src/components/DataTable.tsx`:
- Around line 183-205: Rebuild the dashboard after the DataTable changes and
commit the generated output under src/gateway/static/dashboard/. Ensure the
bundled dashboard includes the updated hostRef and ensureHost behavior from
DataTable.tsx.
🪄 Autofix
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 Plus
Run ID: ac6f0032-e099-4a53-a2e9-336892fec8ca
📒 Files selected for processing (3)
web/src/components/DataTable.test.tsxweb/src/components/DataTable.tsxweb/src/pages/ActivityPage.test.tsx
Stabilizing the portal host so a poll cannot remount the panel also stopped it remounting on a genuine row change: with the container node fixed, React reconciles the panel across a detailKey jump from one row to the next. That breaks the Routing page's Examples panel. RouterReadiness seeds its user picker with `useState(scopedUserId)`, so it only reads the prop at mount, and the Examples button toggles `expanded` straight from one policy's key to another with no null in between. The panel then renders the new policy's header and copy while the warmth numbers still come from the previous policy's user, and no status is fetched for the user it claims to be showing. ActivityPage escaped it: RequestDetail holds no state and RoutingPlan already filters its group lookup by request_group_id. Key the portal's wrapper by detailKey. A poll leaves detailKey alone, so the host stays put and the panel stays mounted; only a different row remounts, which is also what replays the reveal animation under the newly opened row. Found by independent review of the parent commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
14 tasks
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.
Description
Expanding a row in the Activity pane while a request is running made that row flash and slide open again every couple of seconds.
DataTableputs its inline detail panel in a portal host<tr>inserted next to the target row. The layout effect that does the insertion listsrowsin its deps and its cleanup always removed the host, so every re-run built a fresh<tr>/<td>and re-pointed the portal at it. React answers that by unmounting the panel and mounting it into the new node, which replays the 180ms reveal animation and drops the panel's state (including the routing-plan lookup). The activity page polls for in-flight requests every 2s and hands the table a rebuilt rows array each time, so this fired on a timer for as long as a request was running.The host is now built once and held in a ref, so the portal target survives the effect re-running. The effect only repositions it, and only when it is not already in place: re-inserting an attached node detaches and re-attaches its subtree, which cancels and restarts the animation inside it. Detaching moved out of the effect cleanup into the no-target branch plus a mount-scoped effect, so a vanished target (filtered out, page flipped) still leaves nothing behind.
No change to the in-flight polling itself. I checked whether the 2s rows-array churn was worth stabilizing and it is not: react-aria caches rendered rows on the row object, so a new-but-equal array costs zero cell re-renders and the same wall time as a stable one.
PR Type
Relevant issues
None filed; reported directly.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py).Notes on the checklist: this is a dashboard-only change, so the relevant checks are
npm --prefix web run typecheck(clean) andnpm --prefix web test(577 passing). Two regression tests were added, both of which fail onmain: a unit test inDataTable.test.tsxthat a no-op rows rebuild must not remount the panel, and a page-level test inActivityPage.test.tsxthat expands a row with a request in flight and advances past two polls. No user-facing behavior is documented for this, no API contract changed, so no docs or OpenAPI regeneration were needed.AI Usage
AI Model/Tool used:
Claude Opus 5 (1M context), via the Claude Code CLI.
AI Model/Tool details:
The root cause analysis, the fix, and both regression tests were produced by the model through back-and-forth with @njbrake. The prose here is the model's; the direction and the decisions are his. One suggested extra change (memoizing the in-flight rows to stop the 2s array churn) was measured, found to buy nothing, and dropped at his call, so the diff is only the actual fix.
NOTE:
When responding to reviewer questions, please respond yourself rather than copy/pasting reviewer comments into an AI and pasting back its answer. We want to discuss with you, not your AI :)
Summary
Benefit
Expanded activity panels now keep their state and DOM node during polling.