Skip to content

Show regions in previews - #174

Merged
superg merged 1 commit into
superg:mainfrom
eagleflo:show-regions-in-preview
Aug 3, 2026
Merged

Show regions in previews#174
superg merged 1 commit into
superg:mainfrom
eagleflo:show-regions-in-preview

Conversation

@eagleflo

@eagleflo eagleflo commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

region_names was already defined on Rust side.

Fixes #171.

Summary by CodeRabbit

  • Bug Fixes
    • Updated disc page titles to display the correct base ROM name for improved clarity and consistency.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The disc view now passes base_rom_name to the template. The page title renders this value instead of title. The test fixture initializes the new field.

Changes

Disc preview title

Layer / File(s) Summary
ROM base name title wiring
src/routes/disc_view.rs, templates/disc_view.html
DiscViewTemplate includes base_rom_name. The route supplies the computed ROM base name. The template uses it for the page title. The test fixture initializes the field.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Suggested reviewers: superg, deterous, whatever-industries

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: adding region information to previews.
Linked Issues check ✅ Passed The changes use the computed base ROM name for preview titles, which addresses region visibility in link previews [#171].
Out of Scope Changes check ✅ Passed All changes support the linked issue by updating preview data and the page title; no unrelated changes are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@superg

superg commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Couple things:

  • This manually adds comma separated regions list. What we want to see in link previews is a datname and there is already a function for that: build_rom_base_name().
  • How this change was tested? I currently don't have any staging area for testing pre-prod, I test my things locally, did you deploy the whole website locally?

@eagleflo
eagleflo force-pushed the show-regions-in-preview branch from dafd4ba to 5ac60d2 Compare August 1, 2026 02:55
@eagleflo

eagleflo commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

I was following the suggestion in issue #171, adding regions to og:description. Using datname in og:title could work equally well to tell discs apart in link previews. I rewrote the PR to do that instead.

I just worked backwards from where og:title and og:description were being rendered in the template and ran cargo test afterwards -- I thought this was such a simple template change that it didn't warrant its own test case.

For more complex changes deploying locally would be required, but I didn't have a good way to populate the database at hand.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
templates/disc_view.html (1)

3-3: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a regression test for the preview-title contract.

The test fixture initializes base_rom_name, but no test asserts that the rendered title uses it. Add a region-bearing case and verify that the visible <h2> still uses title. Also confirm that the shared title block feeds og:title, because that is the stated PR objective.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@templates/disc_view.html` at line 3, Add a regression test for the template’s
preview-title contract using a region-bearing fixture with `base_rom_name`
initialized; assert the rendered visible `<h2>` uses `title`, and verify the
shared `{% block title %}` value is also propagated to `og:title`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@templates/disc_view.html`:
- Line 3: Add a regression test for the template’s preview-title contract using
a region-bearing fixture with `base_rom_name` initialized; assert the rendered
visible `<h2>` uses `title`, and verify the shared `{% block title %}` value is
also propagated to `og:title`.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1cba7dfb-b4a7-4f0a-b4d3-a2b95cf1a778

📥 Commits

Reviewing files that changed from the base of the PR and between faec2ae and 5ac60d2.

📒 Files selected for processing (2)
  • src/routes/disc_view.rs
  • templates/disc_view.html

@superg
superg merged commit 1067b41 into superg:main Aug 3, 2026
3 checks passed
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.

Add regions to link previews

2 participants