Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMainContent's first-response detection and subsequent navigation were changed to use a group-prefixed path from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🤖 Fix all issues with AI agents
In `@web/src/components/layout/collapsibleSidebar.tsx`:
- Around line 69-74: Update the onClick handler in collapsibleSidebar.tsx to
allow modifier-key and middle-click behavior and to normalize paths before
comparing: first guard out clicks that should open in a new tab/window (if
e.metaKey || e.ctrlKey || e.shiftKey || e.altKey || e.button !== 0) and return
without preventDefault, then compare a normalized form of
window.location.pathname and newChatUrl (e.g., strip trailing slashes with a
small helper like normalize(path) => path.replace(/\/+$/, '') || '/') and only
call e.preventDefault() and window.location.assign(newChatUrl) when the
normalized paths match.
| onClick={(e) => { | ||
| if (window.location.pathname === newChatUrl) { | ||
| e.preventDefault(); | ||
| window.location.assign(newChatUrl); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Next.js Link onClick preventDefault modifier keys new tab behavior
💡 Result:
If you call e.preventDefault() in a next/link onClick handler unconditionally, you also cancel the browser’s default “open in new tab/window” behaviors (Cmd/Ctrl-click, middle-click, Shift-click). onClick fires for those clicks too, so you must only prevent default on a plain left click. Next.js’ docs also note that modifier-key clicks won’t run onNavigate because they’re treated as “new tab” navigation. [1]
Use this pattern:
import Link from "next/link";
import { useRouter } from "next/navigation";
function isPlainLeftClick(e: React.MouseEvent<HTMLAnchorElement>) {
return (
e.button === 0 && // left click
!e.defaultPrevented &&
!e.metaKey && !e.ctrlKey && !e.shiftKey && !e.altKey
);
}
export function MyLink() {
const router = useRouter();
return (
<Link
href="/dashboard"
onClick={(e) => {
// Let the browser handle new-tab/window gestures
if (!isPlainLeftClick(e)) return;
// Also don’t hijack if the link targets another browsing context
const target = (e.currentTarget as HTMLAnchorElement).target;
if (target && target !== "_self") return;
e.preventDefault();
router.push("/dashboard");
}}
>
Dashboard
</Link>
);
}Alternative (App Router): prefer onNavigate when your goal is “optionally cancel SPA navigation”, because it’s specifically for client-side navigation and supports e.preventDefault()—but it won’t run for Ctrl/Cmd-click new-tab gestures anyway. [1]
Sources: Next.js Link docs (onNavigate, modifier keys behavior, preventDefault). [1]
[1] https://nextjs.org/docs/app/api-reference/components/link
🏁 Script executed:
# Find and inspect the collapsibleSidebar.tsx file to understand context
find . -name "collapsibleSidebar.tsx" -type fRepository: ucdavis/PolicyWonk
Length of output: 112
🏁 Script executed:
# Read the collapsibleSidebar.tsx file to understand the context
cat -n ./web/src/components/layout/collapsibleSidebar.tsx | head -100Repository: ucdavis/PolicyWonk
Length of output: 3877
Guard modifier-key clicks and normalize the path comparison.
Right now a same-path click always calls preventDefault, which blocks Cmd/Ctrl‑click (new tab) and middle‑click behavior per Next.js best practices. Also, with trailingSlash enabled in next.config.js, /chat/new/ won't equal /chat/new, so the forced reload won't run. A small guard + normalization keeps UX intact and makes the equality check resilient.
Suggested fix
- onClick={(e) => {
- if (window.location.pathname === newChatUrl) {
- e.preventDefault();
- window.location.assign(newChatUrl);
- }
- }}
+ onClick={(e) => {
+ if (
+ e.metaKey ||
+ e.ctrlKey ||
+ e.shiftKey ||
+ e.altKey ||
+ e.button !== 0
+ ) {
+ return;
+ }
+ const currentPath = window.location.pathname.replace(/\/$/, '');
+ const targetPath = newChatUrl.replace(/\/$/, '');
+ if (currentPath === targetPath) {
+ e.preventDefault();
+ window.location.assign(newChatUrl);
+ }
+ }}🤖 Prompt for AI Agents
In `@web/src/components/layout/collapsibleSidebar.tsx` around lines 69 - 74,
Update the onClick handler in collapsibleSidebar.tsx to allow modifier-key and
middle-click behavior and to normalize paths before comparing: first guard out
clicks that should open in a new tab/window (if e.metaKey || e.ctrlKey ||
e.shiftKey || e.altKey || e.button !== 0) and return without preventDefault,
then compare a normalized form of window.location.pathname and newChatUrl (e.g.,
strip trailing slashes with a small helper like normalize(path) =>
path.replace(/\/+$/, '') || '/') and only call e.preventDefault() and
window.location.assign(newChatUrl) when the normalized paths match.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web/src/components/chat/main.tsx`:
- Around line 28-35: The effect uses aiState.group in its condition but
hardcodes "/ucdavis" in window.history.pushState and omits aiState.group from
the dependency array; update the pushState target to use the dynamic group (use
`${aiState.group}`) and add aiState.group to the effect dependencies so the
effect reacts to group changes; keep the existing router.refresh() and
aiState.id checks in the effect (refer to aiState.group, aiState.id,
aiState.messages, router.refresh, and window.history.pushState).
| pathname === `/${aiState.group}/chat/new` && | ||
| aiState.id !== '' // id is set only once the chat has been saved to the db | ||
| ) { | ||
| // reloads the sidebar, which repulls from the db now that the chat has been saved | ||
| router.refresh(); | ||
| window.history.pushState({}, '', `/chat/${aiState.id}`); | ||
| window.history.pushState({}, '', `/ucdavis/chat/${aiState.id}`); // change to group | ||
| } | ||
| }, [aiState.messages, router, aiState.id, pathname]); |
There was a problem hiding this comment.
Hardcoded group value and missing dependency will break navigation for non-ucdavis groups.
Two issues in this effect:
-
Inconsistent group handling: Line 28 dynamically checks
aiState.group, but line 33 hardcodes/ucdavis/instead of using${aiState.group}. This breaks navigation for any group other than "ucdavis". -
Missing dependency:
aiState.groupis used in the condition but not included in the dependency array, violating React's exhaustive-deps rule and potentially causing stale closure bugs.
🐛 Proposed fix
if (
// on first response from AI
pathname === `/${aiState.group}/chat/new` &&
aiState.id !== '' // id is set only once the chat has been saved to the db
) {
// reloads the sidebar, which repulls from the db now that the chat has been saved
router.refresh();
- window.history.pushState({}, '', `/ucdavis/chat/${aiState.id}`); // change to group
+ window.history.pushState({}, '', `/${aiState.group}/chat/${aiState.id}`);
}
- }, [aiState.messages, router, aiState.id, pathname]);
+ }, [aiState.messages, router, aiState.id, aiState.group, pathname]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pathname === `/${aiState.group}/chat/new` && | |
| aiState.id !== '' // id is set only once the chat has been saved to the db | |
| ) { | |
| // reloads the sidebar, which repulls from the db now that the chat has been saved | |
| router.refresh(); | |
| window.history.pushState({}, '', `/chat/${aiState.id}`); | |
| window.history.pushState({}, '', `/ucdavis/chat/${aiState.id}`); // change to group | |
| } | |
| }, [aiState.messages, router, aiState.id, pathname]); | |
| pathname === `/${aiState.group}/chat/new` && | |
| aiState.id !== '' // id is set only once the chat has been saved to the db | |
| ) { | |
| // reloads the sidebar, which repulls from the db now that the chat has been saved | |
| router.refresh(); | |
| window.history.pushState({}, '', `/${aiState.group}/chat/${aiState.id}`); | |
| } | |
| }, [aiState.messages, router, aiState.id, aiState.group, pathname]); |
🤖 Prompt for AI Agents
In `@web/src/components/chat/main.tsx` around lines 28 - 35, The effect uses
aiState.group in its condition but hardcodes "/ucdavis" in
window.history.pushState and omits aiState.group from the dependency array;
update the pushState target to use the dynamic group (use `${aiState.group}`)
and add aiState.group to the effect dependencies so the effect reacts to group
changes; keep the existing router.refresh() and aiState.id checks in the effect
(refer to aiState.group, aiState.id, aiState.messages, router.refresh, and
window.history.pushState).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/chat/main.tsx`:
- Around line 32-33: The navigation uses router.push which adds a history entry
and causes a back-navigation redirect loop; change the call to router.replace
when canonicalizing the URL (the code that currently calls
router.push(`/${aiState.group}/chat/${aiState.id}`) inside the effect that
checks aiState.group and aiState.id) so the navigation doesn't create a new
history entry, preserving router.refresh() behavior if still needed.
---
Duplicate comments:
In `@web/src/components/chat/main.tsx`:
- Line 35: The useEffect callback reads aiState.group but the dependency array
currently lacks it (only has aiState.messages, router, aiState.id, pathname),
creating a stale-closure and violating exhaustive-deps; update the dependency
array for the effect (the useEffect that references aiState.messages, router,
aiState.id, pathname) to include aiState.group so the effect re-runs when the
group changes.
web/src/components/chat/main.tsx
Outdated
| router.push(`/${aiState.group}/chat/${aiState.id}`); | ||
| router.refresh(); |
There was a problem hiding this comment.
Use router.replace instead of router.push to avoid a redirect loop on back-navigation.
router.push adds a new entry to the browser's history stack, so pressing Back returns the user to /${aiState.group}/chat/new. The effect condition will match again (since aiState.id is still set), triggering another router.push — creating an infinite redirect cycle between /chat/new and /chat/{id}.
router.replace performs the same navigation but without a new history entry, which is the correct semantic for this URL-canonicalization use case.
🐛 Proposed fix
- router.push(`/${aiState.group}/chat/${aiState.id}`);
+ router.replace(`/${aiState.group}/chat/${aiState.id}`);
router.refresh();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.push(`/${aiState.group}/chat/${aiState.id}`); | |
| router.refresh(); | |
| router.replace(`/${aiState.group}/chat/${aiState.id}`); | |
| router.refresh(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/chat/main.tsx` around lines 32 - 33, The navigation uses
router.push which adds a history entry and causes a back-navigation redirect
loop; change the call to router.replace when canonicalizing the URL (the code
that currently calls router.push(`/${aiState.group}/chat/${aiState.id}`) inside
the effect that checks aiState.group and aiState.id) so the navigation doesn't
create a new history entry, preserving router.refresh() behavior if still
needed.
2 participants
Summary by CodeRabbit