Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughNavPartial was centralized into the app root; pages were refactored to consume typed fixture content via a new content module; a new WorkWithUs page and route were added; SEO constants now derive from siteContent; social link resolution and project response typings were updated; multiple fixtures and tests were adapted. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant App as App.vue
participant Nav as NavPartial
participant Router
participant Page as WorkWithUsPage
participant Content as content.ts
participant SEO as seo.ts
Browser->>App: initial load
App->>Nav: mount NavPartial (siteContent.nav.links, social fallbacks)
App->>Router: render <router-view/>
Router->>Page: lazy-load /work-with-us component
Page->>Content: import workWithUsPageContent
Page->>SEO: call useSeo(resolveJsonLdArray(seo.jsonLd, siteUrlFor) + ORGANIZATION_JSON_LD)
Page->>App: render page content + FooterPartial
App->>Browser: deliver rendered DOM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the application's navigation structure by centralizing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the application to use a single, centralized navigation component, which is a great improvement for maintainability. However, the implementation has introduced a couple of significant issues. Firstly, the mobile navigation UI has been compromised, leading to a cluttered top bar. Secondly, the new logic for social links uses hardcoded personal URLs as fallbacks, which is a critical risk if the links aren't configured in the backend. My review includes detailed comments on these two points.
I am having trouble creating individual review comments. Click here to see my feedback.
src/support/links.ts (19-23)
Hardcoding specific personal URLs as fallbacks is highly risky. If the API fails to provide links, or if this code is used in a different context, the website will display incorrect social media profiles. This could mislead users and damage the site's credibility. It's better to not render a link if it's not configured.
A safer approach would be to have empty strings as fallbacks and use v-if in the template to conditionally render the links. If you change the fallbacks to empty strings, you will need to re-introduce the v-if checks in NavPartial.vue.
src/partials/NavPartial.vue (59-73)
Removing the nav-sheet-footer from the mobile menu SheetContent causes the social links and theme toggle to no longer appear in the slide-out menu on mobile devices. This, combined with other changes in this file, makes them permanently visible in the top navigation bar. This can create a cluttered user interface on smaller screens.
To maintain a clean mobile layout, please consider restoring these controls within the mobile sheet. If the goal was to remove code duplication, extracting the social links into a separate component to be used in both desktop and mobile views would be a good approach.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d667acb45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/WritingPage.vue (1)
45-68:⚠️ Potential issue | 🟡 MinorFinish the rename in the remaining metadata fields.
This block now describes the page as “field notes”, but
imageAlt,jsonLd[0].name, andjsonLd[0].descriptionstill describe a “writing archive”. Social cards and structured data will advertise two different versions of the same page.📝 Suggested metadata alignment
useSeo({ title: 'Writing', image: SEO_IMAGE, url: siteUrlFor('/writing'), - imageAlt: `${SITE_NAME} writing archive preview`, + imageAlt: `${SITE_NAME} field notes preview`, description: `Field notes from real systems. Use cases, case studies, and technical essays on AI architecture, production systems, and engineering judgment — problems, decisions, and outcomes from 20+ years of building software that has to keep working.`, keywords: buildKeywords( @@ jsonLd: [ { - name: 'Writing Archive', + name: 'Writing', '@type': 'CollectionPage', url: siteUrlFor('/writing'), '@context': 'https://schema.org', - description: `${SITE_NAME}'s archive of essays, notes, and articles on software engineering, AI, and architecture.`, + description: + 'Field notes from real systems. Use cases, case studies, and technical essays on AI architecture, production systems, and engineering judgment.', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/WritingPage.vue` around lines 45 - 68, The metadata currently mixes "Field notes" (description) with "writing archive" in other fields; update the useSeo call so imageAlt and the JSON-LD entries match the "Field notes" phrasing — specifically change imageAlt, jsonLd[0].name, and jsonLd[0].description in the useSeo(...) call to describe the page as "Field notes" (or the exact SITE_NAME Field notes wording used in the description) so social cards and structured data are consistent with the title/description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/css/theme.css`:
- Around line 1721-1724: The CSS rule adding white-space: nowrap to
.page-section-label and .page-meta-list can cause horizontal overflow on narrow
viewports; update the rule (targeting .page-section-label and .page-meta-list at
the 768px breakpoint) to include overflow: hidden and text-overflow: ellipsis
and ensure the elements are constrained (e.g., set display and a max-width or
allow them to be inline-block/block) so long labels truncate cleanly on mobile
rather than causing layout overflow.
In `@src/pages/HomePage.vue`:
- Around line 18-58: Extract the hardcoded AI ERA content in HomePage.vue (the
<section id="ai-era"> block including the leftTag, headline, body copy, rightTag
and the engagements list rendered inside .work-list/.work-item) into a new JSON
fixture (e.g., storage/fixtures/ai-era.json) modeled like the existing fixtures
(principles/about/cta), then import that fixture in HomePage.vue and render the
left content and map the engagements array to generate .work-item elements (so
the rendering uses the fixture instead of hardcoded text). Remove the
<div> </div> spacer and rely on CSS spacing (apply existing classes like
mt-8 on the RouterLink or add appropriate margin/padding) to keep markup clean;
ensure RouterLink and class names (btn-ghost, mt-8, work-list, work-item) are
preserved so styling remains consistent.
In `@src/pages/WorkWithUsPage.vue`:
- Around line 54-55: The page uses plain spans and paragraphs for section titles
(e.g., the element with class "page-section-label" and the paragraph with class
"page-panel-copy") so assistive tech can't navigate the offer titles, engagement
items, FAQ prompts, and "Start here" heading; update those label elements in
WorkWithUsPage.vue to semantic headings (promote the main offer/engagement
titles to h3/h4 and the FAQ prompts/"Start here" to appropriate h3/h4) while
preserving the existing CSS classes (page-section-label, page-panel-copy) and
text content so styling stays the same but the elements become real headings for
accessibility.
In `@src/support/links.ts`:
- Around line 19-23: ContactPage.vue is using its own separate all-or-nothing
fallback map causing drift; import and use the shared NAV_SOCIAL_FALLBACKS or
the resolveNavSocialLinks(...) helper instead of the local default map, and
replace the all-or-nothing logic with the same merge/resolve logic used by the
header so partial API responses are merged with NAV_SOCIAL_FALLBACKS; update the
ContactPage component to call resolveNavSocialLinks(apiSocials) (or perform the
same merge using NAV_SOCIAL_FALLBACKS) and remove the duplicated local fallback
map so both header and contact page produce the same resolved social links.
- Around line 25-35: The resolveNavSocialLinks helper currently promotes raw
match.url values into the nav; update resolveNavSocialLinks to validate that
match.url is an absolute http or https URL before assigning it to resolvedLinks
(use a safe check such as constructing a URL in a try/catch and confirming
url.protocol is 'http:' or 'https:' or a strict regex like ^https?://). If
validation fails, do not overwrite the NAV_SOCIAL_FALLBACKS value for that
platform; keep the existing fallback. Reference resolveNavSocialLinks and
NAV_SOCIAL_FALLBACKS when making the change so only trusted http(s) links are
used in NavPartial.vue.
---
Outside diff comments:
In `@src/pages/WritingPage.vue`:
- Around line 45-68: The metadata currently mixes "Field notes" (description)
with "writing archive" in other fields; update the useSeo call so imageAlt and
the JSON-LD entries match the "Field notes" phrasing — specifically change
imageAlt, jsonLd[0].name, and jsonLd[0].description in the useSeo(...) call to
describe the page as "Field notes" (or the exact SITE_NAME Field notes wording
used in the description) so social cards and structured data are consistent with
the title/description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d55806c6-e501-4371-add6-db2caf9c06c3
📒 Files selected for processing (27)
src/App.vuesrc/css/theme.csssrc/pages/AboutPage.vuesrc/pages/ContactPage.vuesrc/pages/HomePage.vuesrc/pages/PostPage.vuesrc/pages/ProjectsPage.vuesrc/pages/TagPostsPage.vuesrc/pages/TermsAndPoliciesPage.vuesrc/pages/WorkWithUsPage.vuesrc/pages/WritingPage.vuesrc/partials/NavPartial.vuesrc/partials/ProjectCardPartial.vuesrc/router.tssrc/stores/api/response/projects-response.tssrc/support/links.tssrc/support/seo.tsstorage/fixtures/about.jsonstorage/fixtures/hero.jsonstorage/fixtures/principles.jsontests/pages/AboutPage.test.tstests/pages/ContactPage.test.tstests/pages/ProjectsPage.test.tstests/pages/TermsAndPoliciesPage.test.tstests/pages/WritingPage.test.tstests/partials/HeroPartial.test.tstests/partials/ProjectCardPartial.test.ts
💤 Files with no reviewable changes (4)
- src/pages/PostPage.vue
- tests/pages/TermsAndPoliciesPage.test.ts
- src/partials/ProjectCardPartial.vue
- src/pages/TermsAndPoliciesPage.vue
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b34cfc49dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/pages/HomePage.test.ts (1)
36-53: 🧹 Nitpick | 🔵 TrivialConsider making remaining hardcoded strings data-driven.
Lines 49 and 61 still use hardcoded string literals (
'20+ years across software'and'COMPLEX') while surrounding assertions usehomePageContent. For consistency and maintainability, consider sourcing these from the content fixture as well, if they exist there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/pages/HomePage.test.ts` around lines 36 - 53, The test in HomePage.test.ts hardcodes strings in assertions instead of using the fixture; update the two assertions that check '20+ years across software' and 'COMPLEX' to read the corresponding values from the homePageContent fixture (e.g., use fields under homePageContent.about.body or homePageContent.about.work items) so assertions use the same data source as the other checks; locate these assertions in the test that mounts HomePage and replace the literals with the appropriate homePageContent properties.
♻️ Duplicate comments (2)
src/support/links.ts (1)
30-33:⚠️ Potential issue | 🟡 MinorMatch on the first valid URL inside
find().
find()currently stops on the first non-empty record for a platform. If that record is invalid and a later record is valid, the helper keeps the fallback instead of promoting the valid URL.♻️ Proposed fix
- const match = links.find((item) => item.name === platform && item.url); - if (match?.url && isHttpUrl(match.url)) { + const match = links.find((item) => item.name === platform && isHttpUrl(item.url)); + if (match?.url) { resolvedLinks[platform] = match.url; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/support/links.ts` around lines 30 - 33, The current loop uses links.find(item => item.name === platform && item.url) which returns the first non-empty URL even if it's invalid; change the predicate to only match items whose name equals platform and whose url passes isHttpUrl (e.g., find(item => item.name === platform && item.url && isHttpUrl(item.url))) so resolvedLinks[platform] is set to the first valid HTTP(S) URL; update the search logic inside the for (const platform of Object.keys(NAV_SOCIAL_FALLBACKS) as NavSocialPlatform[]) loop to use isHttpUrl when selecting from links.src/pages/ContactPage.vue (1)
109-119:⚠️ Potential issue | 🔴 CriticalBuild
visibleLinksfrom the shared resolver, not the raw API array.As soon as one API link exists, this computed bypasses both the fallback merge and the HTTP(S) filtering added in
src/support/links.ts. That means partial responses can drop platforms, and an unsafelink.urlcan still flow straight into the anchor on Line 69.🔒 Proposed fix
-import { NAV_SOCIAL_FALLBACKS } from '@support/links.ts'; +import { resolveNavSocialLinks } from '@support/links.ts'; ... -const fallbackLinks: LinksResponse[] = Object.entries(NAV_SOCIAL_FALLBACKS).map(([name, url]) => ({ - uuid: `social-${name}`, - name, - handle: '', - url, - description: '', -})); - const visibleLinks = computed<LinksResponse[]>(() => { - return links.value.length > 0 ? links.value : fallbackLinks; + const resolved = resolveNavSocialLinks(links.value); + return (['linkedin', 'x', 'github'] as const).map((name) => ({ + uuid: `social-${name}`, + name, + handle: '', + url: resolved[name], + description: '', + })); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ContactPage.vue` around lines 109 - 119, visibleLinks currently returns the raw API array when any item exists, bypassing the fallback merge and HTTP(S) filtering implemented in the shared resolver in src/support/links.ts; change visibleLinks to call the shared resolver (e.g., the exported function such as resolveLinks/buildLinks from src/support/links.ts) and pass links.value and NAV_SOCIAL_FALLBACKS to it so the result includes merged fallbacks and URL sanitization instead of using fallbackLinks or links.value directly; remove the manual fallbackLinks usage inside visibleLinks and rely on the resolver's output for rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/HomePage.vue`:
- Line 105: The jsonLd array construction spreads resolveJsonLd(seo.jsonLd,
siteUrlFor) which can be either a single JsonLdEntry or an array, causing
runtime errors when a single object is spread; update each affected page
(HomePage, WritingPage, WorkWithUsPage, ProjectsPage, ContactPage, AboutPage) to
normalize the result into an array first (use Array.isArray(resolveJsonLd(...))
? resolveJsonLd(...) : [resolveJsonLd(...)]) and then spread that normalized
array into the jsonLd assignment (the symbol to change is the jsonLd property
where resolveJsonLd is currently cast and spread).
In `@src/pages/ProjectsPage.vue`:
- Around line 20-23: ProjectsPage.vue currently binds raw HTML via v-html on
sidebar.focus.items (the span rendering loop) which is only safe because the
values come from a static fixture; update the code/comments and add a defensive
check to guarantee this data path remains trusted: ensure sidebar.focus.items is
populated exclusively from the static fixture loader (or add an explicit
isTrusted flag on the fixture and assert it before rendering), and add an inline
comment next to the v-html usage (and/or a small runtime guard in the component
setup that throws or sanitizes if sidebar.focus.items originates from external
input) so future changes cannot accidentally feed user/external data into
v-html.
In `@src/pages/TermsAndPoliciesPage.vue`:
- Around line 9-10: Replace the fragile hardcoded indices usage of hero.copy[0]
and hero.copy[1] with an iterative render: loop over the hero.copy array (e.g.,
using Vue's v-for) to render each paragraph, supply a unique key, and guard
against non-array or undefined values (e.g., check Array.isArray(hero.copy) or
provide a fallback) so TermsAndPoliciesPage.vue reliably renders any number of
copy items.
- Around line 32-41: The template in TermsAndPoliciesPage.vue currently uses a
v-if/v-else so section.paragraphs are skipped whenever section.contactLink
exists (section.contactLink, section.paragraphs, RouterLink); fix by removing
the v-else and rendering the paragraphs always (or render paragraphs first then
render the contact link) so the fixture text is not silently ignored, or
alternatively update the fixture to remove the redundant paragraphs; if the
paragraph should contain an embedded link, parse/render the paragraph text to
include a RouterLink (e.g., replace a placeholder token in section.paragraphs
with a RouterLink component) instead of using the current exclusive v-if/v-else
logic.
In `@src/support/content.ts`:
- Around line 68-75: Add explicit TypeScript types for each exported page
content to ensure compile-time validation: define interfaces (or reuse a generic
PageContent/PageWithSeo type) that include at least a seo: PageSeoContent
property and other section shapes, then update each export (homePageContent,
aboutPageContent, workWithUsPageContent, contactPageContent,
projectsPageContent, writingPageContent, termsAndPoliciesPageContent) to use
type assertions (e.g., homePage as HomePageContent) similar to siteContent so
consumers get strong typing and drift is caught at compile time.
In `@storage/fixtures/writing-page.json`:
- Around line 21-30: The keywords array contains a compound entry "software
architect/engineering"; update the "keywords" array to remove that
slash-combined entry and replace it with two separate entries "software
architect" and "software engineering" so search engines index them as distinct
phrases—locate the "keywords" array in the JSON and modify the string list
accordingly.
In `@tests/partials/NavPartial.test.ts`:
- Around line 43-45: The test assumes the contact nav item is the last entry by
using lastNavLink, which is brittle; instead locate the expected nav entry by
its route or semantic key and then find the corresponding anchor element to
assert its to attribute. Replace the lastNavLink lookup with a search like
siteContent.nav.links.find(l => l.to === '<expected-route>' || l.key ===
'contact') and then locate the anchor via wrapper.findAll('a').find(link =>
link.attributes('to') === foundLink.to) and assert that attributes('to') equals
foundLink.to; update references to lastNavLink and contactLink accordingly.
---
Outside diff comments:
In `@tests/pages/HomePage.test.ts`:
- Around line 36-53: The test in HomePage.test.ts hardcodes strings in
assertions instead of using the fixture; update the two assertions that check
'20+ years across software' and 'COMPLEX' to read the corresponding values from
the homePageContent fixture (e.g., use fields under homePageContent.about.body
or homePageContent.about.work items) so assertions use the same data source as
the other checks; locate these assertions in the test that mounts HomePage and
replace the literals with the appropriate homePageContent properties.
---
Duplicate comments:
In `@src/pages/ContactPage.vue`:
- Around line 109-119: visibleLinks currently returns the raw API array when any
item exists, bypassing the fallback merge and HTTP(S) filtering implemented in
the shared resolver in src/support/links.ts; change visibleLinks to call the
shared resolver (e.g., the exported function such as resolveLinks/buildLinks
from src/support/links.ts) and pass links.value and NAV_SOCIAL_FALLBACKS to it
so the result includes merged fallbacks and URL sanitization instead of using
fallbackLinks or links.value directly; remove the manual fallbackLinks usage
inside visibleLinks and rely on the resolver's output for rendering.
In `@src/support/links.ts`:
- Around line 30-33: The current loop uses links.find(item => item.name ===
platform && item.url) which returns the first non-empty URL even if it's
invalid; change the predicate to only match items whose name equals platform and
whose url passes isHttpUrl (e.g., find(item => item.name === platform &&
item.url && isHttpUrl(item.url))) so resolvedLinks[platform] is set to the first
valid HTTP(S) URL; update the search logic inside the for (const platform of
Object.keys(NAV_SOCIAL_FALLBACKS) as NavSocialPlatform[]) loop to use isHttpUrl
when selecting from links.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb1928b1-cbbf-41ed-b7a2-59fbe4282ed7
⛔ Files ignored due to path filters (3)
public/images/profile/about-seo.jpgis excluded by!**/*.jpgpublic/images/profile/about-seo.pngis excluded by!**/*.pngpublic/images/profile/about.jpgis excluded by!**/*.jpg
📒 Files selected for processing (38)
src/pages/AboutPage.vuesrc/pages/ContactPage.vuesrc/pages/HomePage.vuesrc/pages/ProjectsPage.vuesrc/pages/TermsAndPoliciesPage.vuesrc/pages/WorkWithUsPage.vuesrc/pages/WritingPage.vuesrc/partials/FooterPartial.vuesrc/partials/HeroPartial.vuesrc/partials/NavPartial.vuesrc/partials/RecommendationPartial.vuesrc/support/content.tssrc/support/links.tssrc/support/seo.tsstorage/fixtures/about-page.jsonstorage/fixtures/about.jsonstorage/fixtures/contact-page.jsonstorage/fixtures/cta.jsonstorage/fixtures/hero.jsonstorage/fixtures/home-page.jsonstorage/fixtures/marquee.jsonstorage/fixtures/principles.jsonstorage/fixtures/projects-page.jsonstorage/fixtures/site.jsonstorage/fixtures/terms-and-policies-page.jsonstorage/fixtures/work-with-us-page.jsonstorage/fixtures/writing-page.jsontests/pages/AboutPage.test.tstests/pages/ContactPage.test.tstests/pages/HomePage.test.tstests/pages/ProjectsPage.test.tstests/pages/TermsAndPoliciesPage.test.tstests/pages/WorkWithUsPage.test.tstests/pages/WritingPage.test.tstests/partials/FooterPartial.test.tstests/partials/HeroPartial.test.tstests/partials/NavPartial.test.tstests/partials/RecommendationPartial.test.ts
💤 Files with no reviewable changes (5)
- storage/fixtures/cta.json
- storage/fixtures/principles.json
- storage/fixtures/hero.json
- storage/fixtures/about.json
- storage/fixtures/marquee.json
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/pages/AboutPage.test.ts (1)
76-76:⚠️ Potential issue | 🟡 MinorAvoid pinning the proof assertion to
items[1].This makes the test depend on an arbitrary second element and fails outright if the fixture ever shrinks to one proof item. Iterating
aboutPageContent.sidebar.proof.itemswould be safer and cover the whole block.🔧 Suggested test hardening
- expect(wrapper.text()).toContain(aboutPageContent.sidebar.proof.items[1]); + aboutPageContent.sidebar.proof.items.forEach((item) => { + expect(wrapper.text()).toContain(item); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/pages/AboutPage.test.ts` at line 76, The test currently pins the proof assertion to a single index (expect(wrapper.text()).toContain(aboutPageContent.sidebar.proof.items[1])) which is fragile; change the assertion to iterate over aboutPageContent.sidebar.proof.items and assert that wrapper.text() contains each item (e.g., forEach over aboutPageContent.sidebar.proof.items and run expect(wrapper.text()).toContain(item)) so the test covers all proof entries and won’t break if the fixture shrinks or ordering changes.src/pages/ProjectsPage.vue (1)
20-21: 🛠️ Refactor suggestion | 🟠 MajorThe
v-htmltrust boundary is still documentation-only.The new comment helps, but Line 21 still renders arbitrary strings as raw HTML with no runtime assertion or sanitization. If this data ever stops being fixture-only, this becomes an XSS sink immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ProjectsPage.vue` around lines 20 - 21, The span uses v-html to render sidebar.focus.items (the v-html on the <span> rendering each item) which is an XSS sink; replace the raw HTML rendering with a safe alternative: either render as text (use text binding / v-text or interpolation) or sanitize each item at runtime before binding (e.g., run sidebar.focus.items through a sanitizer like DOMPurify in a computed property or method and bind the sanitized result), and ensure the change is applied to the component logic that produces sidebar.focus.items so only sanitized HTML is ever passed to the template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/ContactPage.vue`:
- Around line 15-20: The template in ContactPage.vue hardcodes the fallback
sentence instead of using the sidebar data; change the template to use a
fallback field on the sidebar primaryChannel (e.g. replace the literal 'Contact
details are loading or temporarily unavailable.' with
sidebar.primaryChannel.fallbackCopy) and add that string to the
contactPageContent fixture under sidebar.primaryChannel (or reuse an existing
fallbackTitle field if preferred), so all copy lives in the fixture and the
template reads profile ? sidebar.primaryChannel.copy :
sidebar.primaryChannel.fallbackCopy.
In `@src/pages/HomePage.vue`:
- Around line 99-106: The useSeo call is missing the page-specific title, so
update the useSeo(...) invocation to pass title: seo.title (optionally with a
sensible fallback) alongside the existing fields; specifically add title:
seo.title to the options object passed to useSeo so the HomePage uses the
fixture's title instead of the default document title.
In `@tests/pages/ContactPage.test.ts`:
- Around line 80-81: The test is asserting against siteContent.fallbackLinks but
ContactPage.vue uses NAV_SOCIAL_FALLBACKS from src/support/links.ts for its
fallback socials; update the test (tests/pages/ContactPage.test.ts) to import
NAV_SOCIAL_FALLBACKS and replace references to
siteContent.fallbackLinks[0].name/1.name with NAV_SOCIAL_FALLBACKS[index].name
(uppercased) in the expectations that check wrapper.text(), and make the
analogous change for the assertions around lines 92-93 so the test matches the
component's actual fallback source.
In `@tests/pages/WorkWithUsPage.test.ts`:
- Around line 21-23: The test currently only asserts visible labels; update it
to also assert the CTA targets by locating the rendered anchor/link nodes and
checking their `to` props against the stubbed data: use `wrapper.find` (or the
component-specific selector used in this test) to locate the engagement CTA and
main CTA button and add expectations that `prop('to')` (or `.props().to`) equals
`workWithUsPageContent.engagements[0].cta.to` and
`workWithUsPageContent.cta.button.to` respectively so the spec fails if those
targets regress.
---
Duplicate comments:
In `@src/pages/ProjectsPage.vue`:
- Around line 20-21: The span uses v-html to render sidebar.focus.items (the
v-html on the <span> rendering each item) which is an XSS sink; replace the raw
HTML rendering with a safe alternative: either render as text (use text binding
/ v-text or interpolation) or sanitize each item at runtime before binding
(e.g., run sidebar.focus.items through a sanitizer like DOMPurify in a computed
property or method and bind the sanitized result), and ensure the change is
applied to the component logic that produces sidebar.focus.items so only
sanitized HTML is ever passed to the template.
In `@tests/pages/AboutPage.test.ts`:
- Line 76: The test currently pins the proof assertion to a single index
(expect(wrapper.text()).toContain(aboutPageContent.sidebar.proof.items[1]))
which is fragile; change the assertion to iterate over
aboutPageContent.sidebar.proof.items and assert that wrapper.text() contains
each item (e.g., forEach over aboutPageContent.sidebar.proof.items and run
expect(wrapper.text()).toContain(item)) so the test covers all proof entries and
won’t break if the fixture shrinks or ordering changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99bb47e2-9f6e-4bb5-b480-d8c6efdba71b
⛔ Files ignored due to path filters (3)
public/images/profile/about-seo.pngis excluded by!**/*.pngpublic/images/profile/about.jpgis excluded by!**/*.jpgpublic/images/profile/seo.pngis excluded by!**/*.png
📒 Files selected for processing (16)
src/pages/AboutPage.vuesrc/pages/ContactPage.vuesrc/pages/HomePage.vuesrc/pages/ProjectsPage.vuesrc/pages/TermsAndPoliciesPage.vuesrc/pages/WorkWithUsPage.vuesrc/pages/WritingPage.vuesrc/support/content.tsstorage/fixtures/writing-page.jsontests/pages/AboutPage.test.tstests/pages/ContactPage.test.tstests/pages/ProjectsPage.test.tstests/pages/TermsAndPoliciesPage.test.tstests/pages/WorkWithUsPage.test.tstests/pages/WritingPage.test.tstests/partials/NavPartial.test.ts
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
1 participant
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.
Summary by CodeRabbit
New Features
Content Updates
Design & Layout
Tests