Skip to content

fix: show an image count for image-only commits in notifications - #407

Open
udaykiranchenna wants to merge 4 commits into
dev-null-sh:productionfrom
udaykiranchenna:fix/image-only-notification-excerpt
Open

udaykiranchenna wants to merge 4 commits into
dev-null-sh:productionfrom
udaykiranchenna:fix/image-only-notification-excerpt

Conversation

@udaykiranchenna

Copy link
Copy Markdown

Problem

A commit can be images alone, with no text. In the notifications inbox such a commit had nothing to show: a star, approval or rejection drew an empty bordered box, and an image-only reply, quote or mention showed nothing under the sentence. The notification payload didn't say the commit had images, so the page couldn't tell.

Fix

  • NotificationGroupsQuery loads each notification's commit with withCount('images'), both for the inbox and for an expanded group (members()). NotificationGroupController no longer loads the commit itself, since members() now does.
  • When a commit has no body segments, the notification shows an image icon and "Image" or "N images" where the excerpt would be, like chat reply snippets and scheduled commits already do. Commits with text are unchanged.
  • The Notifications section of DESIGN.md mentions it.

Notifications only cover approved commits (or your own, for moderation), and a commit only loses its images once it is deleted, so an empty body always comes with at least one image.

Before / After

An image-only reply (top) and a star on an image-only commit (bottom).

Before After
Light before-light after-light
Dark before-dark after-dark

Tests

  • NotificationControllerTest: an image-only commit comes with empty body_segments and its images_count, in the inbox and in an expanded group.
  • NotificationIndex.test.tsx: a starred image-only commit renders "Image" or "3 images" inside the bordered block. Both cases fail without the fix.
  • The fixtures in NotificationIndex.test.tsx and NotificationInbox.test.tsx gain images_count.

Lint, PHPStan, type coverage and the full suite (2459 tests, 100% coverage) pass locally. bun run test:unit only picks up .ts files, so I ran the two .tsx notification tests with node --test after bundling them. In a browser, light and dark: no layout shift, no horizontal overflow, no console errors.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@hbenabdallah hbenabdallah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Merged locally onto current production, the new tests pass (NotificationControllerTest, and NotificationIndex.test.tsx / NotificationInbox.test.tsx with bun test).

One thing about the new .tsx tests: CI doesn't run them. test:unit only picks up tests/Unit/*.test.ts, and they aren't listed in tests/draft-images.config.ts, so they only guard the change when run by hand. Adding them to that config would put them in CI. Also, the image-count test matches the exact markup (class="mt-2 rounded-lg border[^"]*"><p class="[^"]*"><svg…), so any styling change to the block will break it. Asserting on the label text inside the bordered block would be less brittle.

udaykiranchenna and others added 2 commits October 18, 2026 23:52
Moves NotificationIndex and NotificationInbox to vite-plus/test and lists them in tests/draft-images.config.ts, which SeriesStash.test.ts runs in CI. The image-count test now checks the label text inside the bordered block instead of its exact markup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@udaykiranchenna

Copy link
Copy Markdown
Author

@hbenabdallah Thanks for the review! Both done:

  • NotificationIndex.test.tsx and NotificationInbox.test.tsx now use vite-plus/test and are listed in tests/draft-images.config.ts, so SeriesStash.test.ts runs them in CI. vp test run --config tests/draft-images.config.ts passes (9 files, 48 tests).
  • The image-count test finds the bordered block and asserts its text is exactly "Image" or "3 images", so styling changes won't break it. It still fails against the empty box.

I also merged current production into the branch (no conflicts). vp fmt reformatted two lines in NotificationInbox.test.tsx that came in with #409: a wrapped assert.match and a doubled blank line. The full PHP suite passes (2618 tests), and the notification classes stay at 100% coverage.

…notification-excerpt

Resolves the conflicts with dev-null-sh#489, which moved the bordered block into OwnCommitExcerpt and shows the quoted commit under quotes. The quoted commit now loads its image count too, so an image-only one shows "Image" instead of an empty block.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@udaykiranchenna

Copy link
Copy Markdown
Author

Merged current production again to resolve the conflicts with #489.

#489 moved the bordered block into OwnCommitExcerpt and added the quoted commit under quote notifications. An image-only quoted commit had the same empty box, so withQuotedCommits() now loads its image count as well and the block shows "Image" there too, with tests in both suites. I've updated the description to match, and added a line to KNOWLEDGE_BASE.md.

The CI coverage command passes locally (3245 tests, 100%).

@jubianchi

Copy link
Copy Markdown
Contributor

@udaykiranchenna, #661 (merged Oct 19, the day of your last update) covers stars on image-only commits, with a different approach: a thumbnail of the first image on the right, "starred your image(s)", and no excerpt box (isImageOnly). Images are loaded by withStarredCommitImages(), for starred groups only.

After screenshot from #661

The other cases this PR handles are still empty on production: approvals and rejections get the empty bordered box (OwnCommitExcerpt), and image-only replies, quotes, mentions and quoted commits show nothing.

@xHeaven Would it make sense to extend #661's thumbnail to those, rather than the "N images" label, so image-only commits look the same across the inbox? That would mean loading commit.images for those groups too, instead of withCount('images').

#1078 and #1311 also change how NotificationGroupsQuery and NotificationGroupController load commit, so expect conflicts there too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants