QA: homepage, nav, footer by bryceerobertson · Pull Request #305 · StampyAI/AISafety.com

bryceerobertson · 2026-03-11T08:58:04Z

QA pass on the homepage, navigation bar, and footer.

from the commit messages:

QA fixes: consolidate last-updated routes, server-side homepage dates, serialized counts, error handling

Replace 11 individual last-updated API routes with single dynamic [resource] route

Move homepage date fetching from client-side to server-side (async component)

Serialize Airtable requests in counts endpoint to avoid rate limiting

Add shared format-date utility (formatDate, formatRelativeDate)

Fix stacking context for bookmark icons (add isolation: isolate)

Return null instead of fake dates on error

Fix events sort field to match actual Airtable field name

Improve error handling in Navigation counts fetch

Remove unused useEffect import in donation-guide page

Show nothing while loading LastUpdated instead of "Loading..." text

QA fixes: dynamic nav counts, dynamic last-updated dates, mobile nav and homepage styling

Replace hardcoded nav counts with dynamic Airtable view counts via /api/counts endpoint

Replace hardcoded homepage "Last Updated" dates with dynamic LastUpdated components using UTC date comparison

Add Last Modified field support for 6 Airtable tables (self-study, media-channels, communities, advisors, projects, funding)

Add /api/last-updated/donation-guide endpoint with shared date constant

Fix Jobs last-updated to use view instead of missing Publish? field

Fix mobile nav: smaller items (40px height), slide-down animation for menu items

Fix homepage card styling: isolation/z-index for bookmarks, responsive padding, card heights

Fix footer: use opacity-80 instead of color-teal-300 for link text

Hide up-button on homepage

Update text shadow, optimized images (faces-desktop.png, sff.png), TAIS event image

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

…and homepage styling - Replace hardcoded nav counts with dynamic Airtable view counts via /api/counts endpoint - Replace hardcoded homepage "Last Updated" dates with dynamic LastUpdated components using UTC date comparison - Add Last Modified field support for 6 Airtable tables (self-study, media-channels, communities, advisors, projects, funding) - Add /api/last-updated/donation-guide endpoint with shared date constant - Fix Jobs last-updated to use view instead of missing Publish? field - Fix mobile nav: smaller items (40px height), slide-down animation for menu items - Fix homepage card styling: isolation/z-index for bookmarks, responsive padding, card heights - Fix footer: use opacity-80 instead of color-teal-300 for link text - Hide up-button on homepage - Update text shadow, optimized images (faces-desktop.png, sff.png), TAIS event image Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…, serialized counts, error handling - Replace 11 individual last-updated API routes with single dynamic [resource] route - Move homepage date fetching from client-side to server-side (async component) - Serialize Airtable requests in counts endpoint to avoid rate limiting - Add shared format-date utility (formatDate, formatRelativeDate) - Fix stacking context for bookmark icons (add isolation: isolate) - Return null instead of fake dates on error - Fix events sort field to match actual Airtable field name - Improve error handling in Navigation counts fetch - Remove unused useEffect import in donation-guide page - Show nothing while loading LastUpdated instead of "Loading..." text Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

vercel · 2026-03-11T08:58:10Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
ai-safety-com	Ready	Preview, Comment	Mar 11, 2026 8:58am

jakkdl · 2026-03-12T16:08:06Z

I added the commit messages to your PR description - when viewing github PR's you're not immediately shown the commit messages, so you usually want to have that info in the description to make it easier for reviewers. In that description it can also be nice to have motivation, failed attempts, etc. But you can probably ultimately ask Claude for a nice summarizing PR and then read it through to see if it covers your intentions & process properly.
And while it's not as huge a PR as previous ones, it's conceptually sprawling and you could have tried splitting it into conceptually separate ones to ease review and make them easier to process in parallel. The workflow with an LLM doesn't necessarily make it easy, but it can lead to stalling and taking days for a PR to get merged when it requires several back-and-forths.

Have you delved into why almost all updated <x> differ between aisafety.com and this PR? A lot of them differ only by a day, and it differs in both directions, which makes me quite confused when I feel like they should be using the same source of data.

optimized images (faces-desktop.png, sff.png)

were these images in particular badly optimized? faces-desktop.png was brought down from 300k to 47k, but the change to sff.png only saved 53 bytes? Meanwhile there's other images that are even bigger, three of them at a megabyte. Don't love the haphazardness of the change. Can undo this and create a standalone issue/PR tackling this where we can focus on comparing image quality and if we try-hard even compare load times.
Also this added TAIS.webp but didn't remove amsterdam.png.

The background splash is still of a different size to aisafety.com, I don't think this was ever intentionally changed? And there's some minor padding that differs, but not sure if sufficiently different to matter.

Claude review:

  Critical

  1. /api/counts rate limiting approach is excessive (src/app/api/counts/route.ts:81-88)
    - The counts endpoint serializes 10 Airtable requests, taking ~2+ seconds per request.
  This creates a ~20+ second cold start for the counts endpoint.
    - The comment says "cold-start latency is acceptable" but that's quite long.
    - Consider: Could use a single Airtable request with formula-based counting, or batch
  the requests differently.
  2. fetchAllLastUpdated does parallel requests (src/lib/data/last-updated.ts:177-186)
    - The homepage calls fetchAllLastUpdated() which does Promise.all on 11 resources -
  this will hit rate limits just like the original parallel counts approach.
    - This contradicts the commit 2 claim about serializing requests to avoid rate limits.

this seems like a very technical thing, and tricky to get it fully right. It should include motivation & documentation for why a specific strategy was taken, and ideally also some tests to see if it works in practice. It's also not obvious to me that it's an issue. I think you should pull this out from this PR that's otherwise mostly focused on visual consistency.
EDIT: after further reflection the correct approach here would be to statically fetch these just like we statically generate all the other pages. The nav bar has to be a dynamic component with client-side behavior, but the counts can be baked into a static server component that the nav bar gets its data from. This also goes for last-updated.
But, once again, that deserves a separate PR - revert the changes in this one that tries to optimize rate limiting.

  Medium

  3. Mysterious adjust values in counts (src/app/api/counts/route.ts:19,33)
    - /map has adjust: -4 and /self-study has adjust: 1
    - No explanation for these magic numbers. They appear to be manual corrections but
  should be documented.

Being familiar with the layout of the airtable I know why they're there, but they should indeed be documented in the code. The funding item also differ by 2 from the live site, don't know which one of them is correct.

  4. Inconsistent field name casing in configs (src/lib/data/last-updated.ts)
    - Line 31: 'Last modified' (lowercase m)
    - Line 43: 'Last Modified' (uppercase M)
    - Line 86: 'Last modified' (lowercase m again)
    - Airtable field names are case-sensitive. This works if the Airtable columns have
  different casing, but it's suspicious.

I suspect this is an inconsistency on the airtable end. We could fix that later, but should be documented in the code so an LLM (or human) doesn't accidentally "fix" it.

  5. Invalid field name '!Title' (src/app/api/counts/route.ts:38)
    - Field is '!Title' - the ! prefix is unusual. Is this intentional or a typo?

why does the field names in airtable have leading !'s? Not terribly important though

  6. Navigation counts fetch silently fails (src/components/Navigation.tsx:115-122)
    - Catches error and does .catch(err => console.warn(...)) - if counts fail, nav shows
  no counts at all. Might want a fallback to cached/hardcoded values.

not showing any value is probably fine, but we should ideally test how that would look. Though if we statically fetch them on the backend it's much less of an issue to have error handling here, we would ideally make the failure bubble up into a build error so the website doesn't get updated and sticks to an old version if it fails to get all the values needed.

  Minor
  <...>
  9. Duplicate mobile-menu-items CSS rule (src/components/Navigation.module.css:210-216 and
   :230-236)
    - The same class is defined twice in the mobile media query - first for animation, then
   again with layout properties. This works but is confusing.

this sounds like it should be fixed

melissasamworth

Let's talk elsewhere about process for doing QA on styling. For now could you let me know your thinking or fix the stuff I commented on? The other stuff is over my head so I'm okay to trust Claude and John but yeah make sure to go to that whole process. Additionally this was again a super long PR, I would prefer shorter ones!

melissasamworth · 2026-03-12T16:36:37Z

src/app/globals.css

 /* Text effects */
 .shadow-text {
-  text-shadow: 0 2px 4px rgba(0, 0, 0, 0.1);
+  text-shadow: 0 0 12px rgba(0, 25, 27, 0.8);


melissasamworth · 2026-03-12T16:37:31Z

src/app/globals.css

  }

  /* Padding Bottom - responsive */
+  .padding-bottom-8px {


Sorry but can you remove all these padding differences? I had the system I had before for a reason. If there was something specific you were going for let me know and I can reconsider.

melissasamworth · 2026-03-12T16:37:59Z

src/app/page.module.css

  background-color: rgba(5, 24, 26, 0.5);
  backdrop-filter: blur(16px);
  border-radius: 8px;
+  isolation: isolate;


What's this?

melissasamworth · 2026-03-12T16:38:27Z

src/app/page.module.css

  position: absolute;
  top: 0;
  right: 24px;
+  z-index: -1;


Why do you need to assign z-index to this?

melissasamworth · 2026-03-12T16:39:00Z

src/app/page.module.css

+  z-index: -1;
 }

 .date {


Ouch. I know you didn't write this, and I'm not sure how it got there, but let's remove it. We specifically have gap classes for one-off gap properties.

melissasamworth · 2026-03-12T16:46:27Z

src/app/page.tsx


      <div className={styles['aisafety-info']}>
-        <div className="width-6-col">
+        <div className={styles['aisafety-info-text']}>


Same comment, why is this needed?

melissasamworth · 2026-03-12T16:46:54Z

src/components/Footer.tsx

            </h4>
            <div
-              className={`color-teal-300 paragraph-small flex flex-col gap-8px ${styles.links}`}
+              className={`paragraph-small flex flex-col gap-8px opacity-80 ${styles.links}`}


Nooo, this was the thing I kept trying to not change here lol.

melissasamworth · 2026-03-12T16:47:30Z

src/components/Footer.tsx

            </h4>
            <div
-              className={`color-teal-300 paragraph-small flex flex-col gap-8px ${styles.links}`}
+              className={`paragraph-small flex flex-col gap-8px opacity-80 ${styles.links}`}


Same comment. Never use opacity as a way to lighten text on a solid bg. Use a predefined color

melissasamworth · 2026-03-12T16:48:18Z

src/components/Navigation.module.css

-    opacity: 0;
    visibility: hidden;
-    transition:
-      opacity 0.2s ease,


Why are you taking away the transition effects?

melissasamworth · 2026-03-12T16:49:15Z

src/components/Navigation.module.css

+    opacity: 1;
+  }
+
+  .mobile-menu-items {


What were you going for here?

bryceerobertson and others added 2 commits March 11, 2026 21:05

bryceerobertson requested review from jakkdl and melissasamworth March 11, 2026 08:58

melissasamworth requested changes Mar 12, 2026

View reviewed changes

Conversation

bryceerobertson commented Mar 11, 2026 • edited by jakkdl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Mar 11, 2026

Uh oh!

jakkdl commented Mar 12, 2026

Uh oh!

melissasamworth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bryceerobertson commented Mar 11, 2026 •

edited by jakkdl

Loading