fix(dashboard): path boundary guards, title sync safety, AGENTS docs#263
fix(dashboard): path boundary guards, title sync safety, AGENTS docs#263BillChirico merged 1 commit intomainfrom
Conversation
- page-titles.ts: leaf matchers now use exact equality OR subtree startsWith to prevent false-positive matches (e.g. /dashboard/ai != /dashboard/airline) - dashboard-title-sync.tsx: guard against overwriting more-specific page-level metadata titles set by Next.js — only update if title is missing or generic - page-titles.test.ts: add path-boundary and subtree match test cases - AGENTS.md: document dashboardTitleMatchers pattern for new routes
BillChirico
added
priority: low
BillChirico
added
priority: low
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-263
|
🚅 Deployed to the volvox-bot-pr-263 environment in volvox-bot
|
|
Claude finished @BillChirico's task in 6m 17s —— View job Code Review — PR #263
Issues Found1. Guard in
|
| Step | pathname |
computed |
current (before effect) |
Guard fires? | Result |
|---|---|---|---|---|---|
| 1. Initial load | /dashboard |
Overview - APP_TITLE |
'' |
No (endsWith = false) |
Sets "Overview" ✓ |
| 2. Nav to members | /dashboard/members |
Members - APP_TITLE |
Overview - APP_TITLE |
Yes (all 3 conditions true) | Stays "Overview" ✗ |
After step 1, every subsequent navigation will trigger all three guard conditions: current.endsWith(APP_TITLE) ✓, current !== computed ✓, current !== APP_TITLE ✓ — and the title never updates again.
The existing test 'updates the title when the route changes' (dashboard-title-sync.test.tsx:29) should be broken by this guard: it navigates from /dashboard → /dashboard/tickets/42 after the first render sets the title, hitting exactly the broken scenario above. This is only undetected because web tests are not run by any CI job (the Test (Vitest Coverage) job runs root Vitest only; the Web — Lint, Typecheck & Build job does typecheck/lint/build but no pnpm --prefix web test step).
Fix — track what the component last set via a useRef to distinguish its own title from a server-set one: Fix this →
// Correct approach
const lastSetTitle = useRef<string | null>(null);
useEffect(() => {
const computed = getDashboardDocumentTitle(pathname);
const current = document.title;
if (
current !== lastSetTitle.current && // not set by us
current.endsWith(APP_TITLE) &&
current !== computed &&
current !== APP_TITLE
) {
return;
}
lastSetTitle.current = computed;
document.title = computed;
}, [pathname]);2. No tests for the guard's intended behavior
web/tests/components/layout/dashboard-title-sync.test.tsx was not modified. There is no test verifying that a server-set metadata title (e.g. 'Member Profile - APP_TITLE' set before the component mounts) is correctly preserved, nor any test that cross-section navigation still works after an initial component-driven nav. Without these, the guard has no regression coverage regardless of correctness.
Pre-existing CI Blocker (not caused by this PR)
The Web — Lint, Typecheck & Build job is failing due to a Biome formatting error in web/src/components/dashboard/performance-dashboard.tsx (introduced in a6e76d0 / f0ba92a on main). The <Tooltip> elements on lines 363–365 and 405–407 need to be inlined. This is unblocked from this PR but will need resolving before merge.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 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 |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (current.endsWith(APP_TITLE) && current !== computed && current !== APP_TITLE) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guard fires on every client-side navigation, stalling the title
The condition current.endsWith(APP_TITLE) will match titles that DashboardTitleSync itself previously set (e.g. "Bot Config - Volvox.Bot - AI Powered Discord Bot"). Because the component runs in the persistent dashboard shell and sets the title on every navigation, the next navigation will always see a current that ends with APP_TITLE (set by the previous run) but differs from the new computed. All three guard conditions will be true, the early return fires, and the title is never updated after the first page visit.
Concrete trace:
/dashboard/config→DashboardTitleSyncsets title to"Bot Config - Volvox.Bot - AI Powered Discord Bot"- Navigate to
/dashboard/ai→DashboardTitleSyncfires:computed = "AI Chat - Volvox.Bot - AI Powered Discord Bot"current = "Bot Config - Volvox.Bot - AI Powered Discord Bot"← set by step 1current.endsWith(APP_TITLE)→ truecurrent !== computed→ truecurrent !== APP_TITLE→ true- Guard returns early → title stuck as
"Bot Config - ..."❌
Furthermore, the guard is designed to protect titles set by a page's metadata export. However, createPageMetadata() returns bare titles (e.g. { title: 'Bot Config' }) without the APP_TITLE suffix, so current.endsWith(APP_TITLE) would be false for those titles anyway — meaning the guard never fires for its intended use case.
A reliable fix is to track what DashboardTitleSync last set via a ref so the guard can distinguish "title set externally by page metadata" from "stale title set by this component":
import { useRef } from 'react';
…
const prevComputedRef = useRef<string | null>(null);
useEffect(() => {
const computed = getDashboardDocumentTitle(pathname);
const current = document.title;
// Only respect a page-level title if it wasn't set by this component on the
// previous navigation.
if (
current.endsWith(APP_TITLE) &&
current !== computed &&
current !== APP_TITLE &&
current !== prevComputedRef.current
) {
prevComputedRef.current = null;
return;
}
document.title = computed;
prevComputedRef.current = computed;
}, [pathname]);Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/components/layout/dashboard-title-sync.tsx
Line: 24-26
Comment:
Guard fires on every client-side navigation, stalling the title
The condition `current.endsWith(APP_TITLE)` will match titles that `DashboardTitleSync` itself previously set (e.g. `"Bot Config - Volvox.Bot - AI Powered Discord Bot"`). Because the component runs in the persistent dashboard shell and sets the title on every navigation, the *next* navigation will always see a `current` that ends with `APP_TITLE` (set by the previous run) but differs from the new `computed`. All three guard conditions will be true, the early return fires, and the title is **never updated** after the first page visit.
Concrete trace:
1. `/dashboard/config` → `DashboardTitleSync` sets title to `"Bot Config - Volvox.Bot - AI Powered Discord Bot"`
2. Navigate to `/dashboard/ai` → `DashboardTitleSync` fires:
- `computed = "AI Chat - Volvox.Bot - AI Powered Discord Bot"`
- `current = "Bot Config - Volvox.Bot - AI Powered Discord Bot"` ← set by step 1
- `current.endsWith(APP_TITLE)` → **true**
- `current !== computed` → **true**
- `current !== APP_TITLE` → **true**
- Guard returns early → title stuck as `"Bot Config - ..."` ❌
Furthermore, the guard is designed to protect titles set by a page's `metadata` export. However, `createPageMetadata()` returns bare titles (e.g. `{ title: 'Bot Config' }`) without the `APP_TITLE` suffix, so `current.endsWith(APP_TITLE)` would be **false** for those titles anyway — meaning the guard never fires for its intended use case.
A reliable fix is to track what `DashboardTitleSync` last set via a ref so the guard can distinguish "title set externally by page metadata" from "stale title set by this component":
```javascript
import { useRef } from 'react';
…
const prevComputedRef = useRef<string | null>(null);
useEffect(() => {
const computed = getDashboardDocumentTitle(pathname);
const current = document.title;
// Only respect a page-level title if it wasn't set by this component on the
// previous navigation.
if (
current.endsWith(APP_TITLE) &&
current !== computed &&
current !== APP_TITLE &&
current !== prevComputedRef.current
) {
prevComputedRef.current = null;
return;
}
document.title = computed;
prevComputedRef.current = computed;
}, [pathname]);
```
How can I resolve this? If you propose a fix, please make it concise.
🧹 Preview Environment Cleaned UpThe Railway preview environment for this PR has been removed. Environment: |
Summary
Follow-up to #262 based on Claude's code review.
Changes
1. Leaf-route path boundary fix (
page-titles.ts)All leaf matchers now use
pathname === '/dashboard/x' || pathname.startsWith('/dashboard/x/')instead of barestartsWith. Prevents false-positive matches on future sibling routes (e.g./dashboard/aiwould have matched/dashboard/airline).2.
DashboardTitleSynctitle overwrite guard (dashboard-title-sync.tsx)Added a guard to avoid overwriting more-specific page-level
metadatatitles set by Next.js. If the current title already ends withAPP_TITLEand differs from what the component would set, it leaves the page-level title intact.3. AGENTS.md docs (
AGENTS.md)Documented the
dashboardTitleMatcherspattern — new routes must add an entry, with instructions on the correct matcher format.4. Tests (
page-titles.test.ts)Added path-boundary and subtree match test cases.