[Enhancement] - Refactor Page Layout

[leekahung]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Infrastructure
• Maintenance

Description

This is a minor PR to refactor common styles for pages into a single layout component. It's mainly to make the UI more maintainable; it should have no visual changes. Architecture.md has been updated to include the new layout component.

The package clsx has also been added to help with conditional Tailwind classes in components.

Related Tickets & Documents

• Related Issue #
• Closes #

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, a note on the devices and browsers this has been tested on, as well as any relevant images for UI changes.

Added/updated tests?

• Yes
• No, and this is why: please replace this line with details on why tests have not been included
• I need help with writing tests

Documentation

• If this PR changes the system architecture, Architecture.md has been updated

[optional] Are there any post deployment tasks we need to perform?

[github-actions]

💡 Minor suggestion: Consider more descriptive naming

The generic name Props could be more descriptive:

interface PageLayoutProps {
  children: React.ReactNode;
}

export default function PageLayout({ children }: PageLayoutProps) {

This follows common React conventions and improves code searchability. Not blocking, just a style preference.

[github-actions]

⚠️ Missing test coverage

Great start on testing! Consider adding these additional test cases for more comprehensive coverage:

1. Static /letter route (no params):

it("returns h-screen when path is /letter (no params)", async () => {
  const pageLayout = await pageSetup("/letter");
  expect(pageLayout).toHaveClass("h-screen");
});

2. Other static pages (/disclaimer, /privacy-policy):

it("returns sm:pt-32 sm:pb-16 for /disclaimer", async () => {
  const pageLayout = await pageSetup("/disclaimer");
  expect(pageLayout).toHaveClass("sm:pt-32", "sm:pb-16");
});

3. Children rendering:

it("renders children correctly", () => {
  const { container } = render(
    <MemoryRouter initialEntries={["/"]}>
      <PageLayout>
        <div>Test</div>
      </PageLayout>
    </MemoryRouter>
  );
  expect(screen.getByTestId("test-child")).toBeInTheDocument();
});

These additional tests would bring the coverage in line with the thoroughness of other component tests in the codebase (like About.test.tsx).

[leekahung]

Need adjustment with recent merges, revert to draft

[github-actions]

Claude finished @leekahung's task in 3m 19s —— View job

---

Code Review Complete ✅

• Gather context and understand the request
• Read changed files
• Check CI status
• Review code quality, security, performance, testing, documentation
• Post inline comments for specific issues
• Final summary

CI Status: PR Checks ✅ | CodeQL in progress

---

Summary

Solid refactoring that achieves its stated goal cleanly. PageLayout is a focused 23-line component with a clear responsibility, and the integration across all page components is consistent.

Code Quality

The core change is straightforward and well-executed. clsx is used consistently across the modified components, replacing the previous mix of template literals. The isChatPages conditional correctly covers /, /letter, and all dynamic letter routes.

One observation in PageLayout.tsx: the flex justify-center class is applied unconditionally, but items-center is only applied for static pages. This means chat pages are centered horizontally but not vertically (intentional, since h-dvh fills the full height and the chat content manages its own layout). Worth documenting this asymmetry with a comment for future maintainers since it's not immediately obvious why items-center is conditional while justify-center is not.

Security

No concerns. DOMPurify.sanitize in MessageContent.tsx is preserved, dangerouslySetInnerHTML in MessageContainer.tsx continues to use sanitized content, and PageLayout itself processes no user input.

Performance

useLocation() inside PageLayout triggers a re-render on route change, which is the expected and minimal overhead for this pattern. No regressions to lazy loading or Suspense structure. The clsx package is ~400 bytes.

Testing

Core conditional logic is covered: index path → h-dvh, dynamic letter route → h-dvh, static page → sm:pt-32 sm:pb-16. See inline comment about the missing base /letter path test and the beforeAll scope style issue. Neither is blocking.

Documentation

Architecture.md is correctly updated with the new layouts/ directory and PageLayout.test.tsx entry. The placement in the directory tree is accurate and follows existing conventions.

Issues Found

Must Fix: None

Non-blocking (inline comments posted):

• DisclaimerLayout.tsx:12 — empty string in clsx ternary; prefer !isOngoing && "max-w-[600px]"
• PageLayout.test.tsx:7 — beforeAll outside describe (style)
• PageLayout.test.tsx:46 — no test for base /letter route (nice-to-have)
• MessageWindow.tsx:66 — pre-existing overflow-y-none non-standard class (not introduced by this PR)

---

| Branch: refactor-page-layout

[github-actions]

Claude finished @leekahung's task in 1m 51s —— View job

---

Code Review Complete ✅

• Gather context and understand the request
• Check what changed since last review
• Read changed files
• Check CI status (PR Checks in progress, CodeQL in progress)
• Review code quality, security, performance, testing, documentation
• Final summary

---

What Changed Since Last Review

The two previous non-blocking suggestions were implemented in commit 6396655:

• ✅ DisclaimerLayout.tsx — empty string in clsx ternary replaced with !isOngoing && "max-w-[600px]"
• ✅ MessageWindow.tsx — overflow-y-none (non-standard class) replaced with isOngoing && "overflow-y-scroll"

Both are clean fixes.

---

Summary

The PR is in good shape. PageLayout correctly uses pathname.startsWith("/letter") to cover dynamic letter routes, clsx is applied consistently throughout, and the test descriptions now accurately reflect what's being tested. All previous must-fix items were absent from the start; both non-blocking suggestions from the last round have been addressed.

Code Quality

PageLayout.tsx is clean. pt-(--navbar-height) is valid Tailwind v4 CSS variable shorthand (project is on ^4.1.6). The isChatPages conditional correctly gates items-center for non-chat pages — chat pages don't need vertical centering since h-dvh fills the viewport and content manages its own layout.

One minor nit in Navbar.tsx: "no-underline" appears twice in the clsx call — once unconditionally (line 38) and again in the inactive branch of the isActive ternary (line 39). Harmless, but it would be cleaner to remove the one in the ternary.

Security

No concerns. DOMPurify.sanitize in MessageContent.tsx is intact. dangerouslySetInnerHTML in both MessageContent.tsx and MessageContainer.tsx remains unchanged from main. PageLayout processes no user input.

Performance

No regressions. useLocation() re-renders on route change as expected. clsx is ~400 bytes. Lazy loading and Suspense structure are untouched.

Testing

The three existing tests cover the key branching logic. Two items from last review remain as nice-to-haves:

• PageLayout.test.tsx:7 — beforeAll is outside the describe block. Functionally fine with a single describe, but convention in this codebase is to nest it inside.
• No test for the base /letter route (without params). startsWith("/letter") covers it logically, but an explicit test would document the intent.

Issues Found

Must Fix: None

Non-blocking:

• Navbar.tsx:38-39 — redundant no-underline in inactive ternary branch (harmless)
• PageLayout.test.tsx:7 — beforeAll outside describe (style, pre-existing from last push)
• PageLayout.test.tsx — no test for base /letter (nice-to-have, pre-existing from last push)

---

| Branch: refactor-page-layout

[github-actions]

Claude finished @leekahung's task in 2m 39s —— View job

---

Code Review Complete ✅

• Gather context and understand the request
• Check what changed since last review
• Read changed files
• Check CI status (PR Checks ✅ | CodeQL ✅)
• Review code quality, security, performance, testing, documentation
• Post inline comments for specific issues
• Final summary

---

What Changed Since Last Review

The branch was rebased onto main (870ebfc). No substantive logic changes — the previous suggestions from the Feb 19 review were already in place. This review catches one new formatting issue and restates the two outstanding carry-overs.

---

Summary

The core refactor is solid. PageLayout correctly gates items-center behind !isChatPages, clsx is used consistently throughout, and pt-(--navbar-height) is valid Tailwind v4 CSS variable shorthand. No regressions in lazy loading or Suspense structure.

Code Quality

The clsx migration across MessageContainer, MessageWindow, NavbarMenuButton, Sidebar, and Navbar is clean and consistent. Chat.tsx correctly sheds its old wrapper div since PageLayout now handles centering.

One new issue in App.tsx: </Routes> at line 35 has drifted to 6-space indentation while the opening <Routes> is at 8 spaces — likely a merge artifact. As-is, the <footer> lands inside PageLayout, which is fine visually (it's fixed), but the indentation makes the JSX structure ambiguous. Inline comment posted.

Security

No concerns. DOMPurify.sanitize in MessageContent.tsx is intact. dangerouslySetInnerHTML usage is unchanged. PageLayout processes no user input.

Performance

No regressions. useLocation() re-renders on route change as expected. Lazy loading and Suspense are untouched. clsx is ~400 bytes.

Testing

Three tests cover the branching logic correctly. Two carry-over nits remain (see inline comments).

Documentation

Architecture.md was already updated in a prior commit. No further changes needed.

---

Issues Found

Must Fix: None

Non-blocking (inline comments posted):

• App.tsx:35 — </Routes> indentation mismatch; footer ends up inside PageLayout (ambiguous intent, harmless visually)
• Navbar.tsx:39 — redundant "no-underline" in inactive ternary branch (carry-over from last review)
• PageLayout.test.tsx:7 — beforeAll outside describe (carry-over from last review)
• PageLayout.test.tsx — no test for base /letter route (nice-to-have, carry-over)

---

| Branch: refactor-page-layout

[github-actions]

Carry-over from last review: "no-underline" appears twice — once unconditionally on line 38 and again in the inactive branch here. The inactive branch one is redundant.

Suggested change

	isActive ? "bg-green-medium rounded" : "no-underline",
	isActive ? "bg-green-medium rounded" : "",

[github-actions]

Claude finished @leekahung's task in 1m 29s —— View job

---

Code Review Complete ✅

• Gather context and understand the request
• Check what changed since last review
• Read changed files
• Check CI status (PR Checks in progress, CodeQL in progress)
• Review code quality, security, performance, testing, documentation
• Final summary

---

What Changed Since Last Review

The two non-blocking suggestions from the Feb 23 review were addressed in commit 7f94ab0:

• ✅ App.tsx — </Routes> indentation corrected; <footer> now cleanly nested inside PageLayout at the right depth
• ✅ Navbar.tsx — isActive ? "bg-green-medium rounded" : "no-underline" replaced with isActive && "bg-green-medium rounded", removing the redundant "no-underline" in the inactive branch (unconditional "no-underline" on the preceding line is correct and unchanged)

---

Summary

This PR is ready. All must-fix items were absent from the start, and all non-blocking suggestions across four review rounds have now been resolved. The PageLayout component is clean and focused, clsx is applied consistently throughout, and the isChatPages conditional correctly covers all chat routes.

Code Quality

App.tsx structure is now unambiguous — <Routes>, <footer>, and the closing </PageLayout> are all at the correct indentation level. Navbar.tsx clsx usage is clean. PageLayout.tsx is a focused 23-line component. No issues.

Security

No concerns. DOMPurify.sanitize in MessageContent.tsx is intact. dangerouslySetInnerHTML usage in MessageContainer.tsx is unchanged from main. PageLayout processes no user input.

Performance

No regressions. useLocation() re-renders on route change as expected. Lazy loading and Suspense structure are untouched. clsx is ~400 bytes.

Testing

Three tests cover the branching logic. Two carry-over nits persist but are non-blocking:

• PageLayout.test.tsx:7 — beforeAll is at file scope; convention in this codebase is to nest it inside describe
• PageLayout.test.tsx — no explicit test for the base /letter route (without params); startsWith("/letter") covers it logically but a test would document the intent

Documentation

Architecture.md was updated in a prior commit. No further changes needed.

---

Issues Found

Must Fix: None

Non-blocking (carry-overs, both pre-existing from prior reviews):

• PageLayout.test.tsx:7 — beforeAll outside describe (style)
• PageLayout.test.tsx — no test for base /letter route (nice-to-have)

---

| Branch: refactor-page-layout