[morozov-av]

1. Render navigation elements as real links instead of buttons
2. Allow Ctrl/Cmd-click, Shift-click, and middle-click to use native browser behavior
3. Added tests

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[bnmnetp]

Thanks for this. Making the header items real links so Ctrl/Cmd-click and middle-click work is the right fix. Below are the issues I found, most important first. Paths are under bases/rsptx/assignment_server_api/assignment_builder/, and line numbers are approximate.

Navigation behaviour

1. Hrefs don't match the router outside production (src/navUtils.ts ~L115). The hrefs hardcode /assignment/instructor/..., but the router's basename comes from VITE_BASE_URL, which is / under npm run dev. In dev, a Ctrl-click on Grader / Assignment Builder / Accommodations opens a route that doesn't exist, while a plain click works. Could the href be built from the same base the router uses?
2. Disabled items can still be clicked (src/components/shell/AppNavBar.tsx ~L38). The disabled attribute does nothing on an <a>. A disabled item with an href still runs onClick and still navigates on middle-click or Ctrl-click, which the old disabled <button> never did. handleNavClick should return early when item.disabled is set, and the anchor should also get aria-disabled and no href.
3. Each URL is written twice (src/navUtils.ts ~L102). Every entry has its URL in href and again inside its onClick, and the two already differ for SPA routes (/assignment/instructor/grader and "grader"). If someone updates one copy and not the other, a click and a Ctrl-click go to different places. A single target per entry, with AppNavBar doing the navigation (plus a separate route field for SPA routes), would prevent that.
4. "Back to course" link can be invalid (src/navUtils.ts ~L102). Its href is /ns/books/published/${course}/index.html with no check, while the label falls back to course || "Course". If course is empty or undefined, the link becomes /ns/books/published//index.html or .../undefined/..., and now that it's a real link, someone can open it.
5. External links are opened from script (src/components/shell/AppNavBar.tsx ~L17). A plain click on "Join our Discord" or the other Help links calls preventDefault and then window.open(_blank) without noopener. The opened page keeps a window.opener reference to the Runestone tab, and popup blockers may stop it. A native target="_blank" rel="noopener" anchor for external links would avoid both problems.

Unrelated dependency changes

6. TipTap code-block extension downgraded (package.json ~L71). tiptap-extension-code-block-prism changes from ^0.0.2-e to exactly 0.0.2-e. In semver, 0.0.2-e sorts before 0.0.2, so the lockfile drops from the stable release to the older pre-release.
7. The vite.config.ts workaround only applies in dev (~L59). The new optimizeDeps include / needsInterop entries only affect the dev server; the production build ignores them. The dev server and the production bundle could load the extension differently, so CodeBlockPrism might be undefined only in production.
8. Playwright upgrade (package-lock.json). Playwright goes from 1.60 to 1.64.1, which requires Node 20 or later, and the lockfile drops the darwin fsevents optional dependency.

Could #6–8 go in a separate PR, or be reverted here? Bundled with this change, they can't be rolled back separately if the editor or CI breaks.

Tests

9. Modified-click tests don't check the main goal (src/components/shell/AppNavBar.spec.tsx ~L225). They only assert that onClick wasn't called. They don't check that preventDefault wasn't called, which is what keeps native Ctrl/Cmd-click working. The altKey case and the event.button !== 0 case aren't tested.

Suggested before merging: #1, #2 and #4, plus splitting out #6–8.

🤖 Generated with Claude Code

[bnmnetp]

The main test I've been trying is using right click to see what I can open up in a new window. I'm not sure what @ascholerChemeketa all had in mind with the request, or how complex this might be to implement. When I right click on something in the navbar I'm given the option to open that in a new window. But lets say I'm in the assignment building, I cannot click on the name of an assignment and open it in a new tab/window.
I'm on a mac in Firefox (Zen) Same in Firefox.

I'm not even sure that this is a high priority unless it is some kind of a11y requirement?? Probably rendering links is important for a11y navigation.

[morozov-av]

Thanks for this. Making the header items real links so Ctrl/Cmd-click and middle-click work is the right fix. Below are the issues I found, most important first. Paths are under bases/rsptx/assignment_server_api/assignment_builder/, and line numbers are approximate.

Navigation behaviour

1. Hrefs don't match the router outside production (src/navUtils.ts ~L115). The hrefs hardcode /assignment/instructor/..., but the router's basename comes from VITE_BASE_URL, which is / under npm run dev. In dev, a Ctrl-click on Grader / Assignment Builder / Accommodations opens a route that doesn't exist, while a plain click works. Could the href be built from the same base the router uses?
2. Disabled items can still be clicked (src/components/shell/AppNavBar.tsx ~L38). The disabled attribute does nothing on an <a>. A disabled item with an href still runs onClick and still navigates on middle-click or Ctrl-click, which the old disabled <button> never did. handleNavClick should return early when item.disabled is set, and the anchor should also get aria-disabled and no href.
3. Each URL is written twice (src/navUtils.ts ~L102). Every entry has its URL in href and again inside its onClick, and the two already differ for SPA routes (/assignment/instructor/grader and "grader"). If someone updates one copy and not the other, a click and a Ctrl-click go to different places. A single target per entry, with AppNavBar doing the navigation (plus a separate route field for SPA routes), would prevent that.
4. "Back to course" link can be invalid (src/navUtils.ts ~L102). Its href is /ns/books/published/${course}/index.html with no check, while the label falls back to course || "Course". If course is empty or undefined, the link becomes /ns/books/published//index.html or .../undefined/..., and now that it's a real link, someone can open it.
5. External links are opened from script (src/components/shell/AppNavBar.tsx ~L17). A plain click on "Join our Discord" or the other Help links calls preventDefault and then window.open(_blank) without noopener. The opened page keeps a window.opener reference to the Runestone tab, and popup blockers may stop it. A native target="_blank" rel="noopener" anchor for external links would avoid both problems.

Unrelated dependency changes

6. TipTap code-block extension downgraded (package.json ~L71). tiptap-extension-code-block-prism changes from ^0.0.2-e to exactly 0.0.2-e. In semver, 0.0.2-e sorts before 0.0.2, so the lockfile drops from the stable release to the older pre-release.
7. The vite.config.ts workaround only applies in dev (~L59). The new optimizeDeps include / needsInterop entries only affect the dev server; the production build ignores them. The dev server and the production bundle could load the extension differently, so CodeBlockPrism might be undefined only in production.
8. Playwright upgrade (package-lock.json). Playwright goes from 1.60 to 1.64.1, which requires Node 20 or later, and the lockfile drops the darwin fsevents optional dependency.

Could #6–8 go in a separate PR, or be reverted here? Bundled with this change, they can't be rolled back separately if the editor or CI breaks.

Tests

9. Modified-click tests don't check the main goal (src/components/shell/AppNavBar.spec.tsx ~L225). They only assert that onClick wasn't called. They don't check that preventDefault wasn't called, which is what keeps native Ctrl/Cmd-click working. The altKey case and the event.button !== 0 case aren't tested.

Suggested before merging: #1, #2 and #4, plus splitting out #6–8.

🤖 Generated with Claude Code

Thanks for review. I’ve fixed issues:

• SPA destinations now use React Router links
• Disabled items no longer receive an href or route, and they expose aria-disabled
• “Back to course” is disabled when the course is unavailable
• External destinations are native links with target="_blank" and rel="noopener".
  I also I think it's ok to kept dependency changes in this PR:
• tiptap-extension-code-block-prism now uses the stable 0.0.2 release
• Playwright was upgraded to 1.64.1 for Ubuntu 26.04 support

[morozov-av]

The main test I've been trying is using right click to see what I can open up in a new window. I'm not sure what @ascholerChemeketa all had in mind with the request, or how complex this might be to implement. When I right click on something in the navbar I'm given the option to open that in a new window. But lets say I'm in the assignment building, I cannot click on the name of an assignment and open it in a new tab/window. I'm on a mac in Firefox (Zen) Same in Firefox.

I'm not even sure that this is a high priority unless it is some kind of a11y requirement?? Probably rendering links is important for a11y navigation.

Good point! I agree that links within main content should support the same browser behavior as navbar.
I've updated assignment names in Assignment Builder to use React Router links, so you can now right-click or Cmd/Ctrl-click to open an assignment in a new tab or window.
I kept other table cells as actions since they don't all represent navigation to separate pages. Yes, using actual links also helps with accessibility.