Skip to content

fix: dim labels and reposition note for webhook-driven readonly fields#148

Open
R-Hart80 wants to merge 1 commit into
CatholicOS:devfrom
R-Hart80:fix/webhook-driven-readonly-fields
Open

fix: dim labels and reposition note for webhook-driven readonly fields#148
R-Hart80 wants to merge 1 commit into
CatholicOS:devfrom
R-Hart80:fix/webhook-driven-readonly-fields

Conversation

@R-Hart80

@R-Hart80 R-Hart80 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Refs #5

Problem

When isWebhookDriven is true, the four GitHub-integration input fields (repo_owner, repo_name, branch, git_ontology_path) were already rendered with readOnly and cursor-not-allowed opacity-60 on the inputs themselves. However, their labels remained at full opacity, which was visually inconsistent — the fields looked disabled but the labels did not.

Additionally, the explanatory note "Repository fields are managed by the GitHub integration." was placed between the first two-column grid (repo owner + name) and the second grid (branch + file path), making it ambiguous whether it applied to all four fields or only the bottom two.

Changes

  • app/projects/[id]/settings/page.tsx
    • Added isWebhookDriven && "opacity-60" to the className of all four field labels so they dim in sync with their inputs.
    • Moved the explanatory note to appear after both field grids so it clearly covers all four read-only fields as a group.

Test plan

  • Open a project that has a GitHub webhook configured (isWebhookDriven === true).
  • Navigate to Settings → GitHub Integration.
  • Verify the four labels ("Repository owner", "Repository name", "Branch", "File path") are visually dimmed (opacity-60) alongside their inputs.
  • Verify the note "Repository fields are managed by the GitHub integration." appears below all four fields, not between them.
  • Open a project without GitHub integration — verify labels are full opacity and inputs are editable.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Apr 13, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@R-Hart80 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 44 minutes and 33 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6a5a1c2d-2c08-49e4-a1da-0636d9938d76

📥 Commits

Reviewing files that changed from the base of the PR and between 0233a83 and 1e51ffb.

📒 Files selected for processing (1)
  • app/projects/[id]/settings/page.tsx
✨ 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 and usage tips.

@JohnRDOrazio JohnRDOrazio added the bug Something isn't working label Apr 13, 2026
@damienriehl damienriehl force-pushed the fix/webhook-driven-readonly-fields branch from 207119a to c9d1614 Compare May 2, 2026 13:13
@codecov

codecov Bot commented May 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

When a project is webhook-driven, the four GitHub integration input
fields (repo owner, repo name, branch, file path) were already marked
readOnly and visually styled as disabled on the inputs themselves.
This commit extends the visual treatment to their labels by applying
opacity-60 when isWebhookDriven is true, and moves the explanatory
note ("Repository fields are managed by the GitHub integration.")
to appear after all four fields so it clearly covers the whole group.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@R-Hart80 R-Hart80 force-pushed the fix/webhook-driven-readonly-fields branch from c9d1614 to 1e51ffb Compare May 11, 2026 13:55
@JohnRDOrazio

Copy link
Copy Markdown
Member

Scope note: this PR is a UI polish, not the verification #5 asks for

I've changed Closes #5Refs #5 in the description, and here's why.

Issue #5 is a verification task. The webhook-driven upstream-sync feature already shipped (frontend 57435cb + backend 3800304); #5 exists to confirm it works end-to-end via an 8-item checklist that spans both backend and frontend.

This PR changes one file (app/projects/[id]/settings/page.tsx, +22/−10) in the edit form only:

  • dims the four field labels (previously only the <input>s were dimmed), and
  • moves the "Repository fields are managed by the GitHub integration" note below both field grids.

Mapping it against #5's checklist:

#5 checklist item Covered here?
Enabling webhooks auto-creates config with frequency="webhook" ❌ backend — untouched
Configured view shows "Automatically triggered by GitHub webhooks" ❌ that note isn't in this diff
Edit form: repo fields read-only + dimmed ⚠️ inputs already dimmed; this only adds label dimming
Explanatory note present ⚠️ note already existed; this only repositions it
Frequency + update mode still editable ❌ not exercised
Disabling webhooks downgrades to "manual" ❌ backend — untouched
Existing config updated, not duplicated ❌ backend — untouched
New integration with webhooks creates config on save ❌ backend — untouched

This is a legitimate standalone UI fix and good to merge on its own merits — but it shouldn't auto-close the verification issue. #5 stays open until the 8-item checklist is actually run (backend behavior in particular). Running that checklist now.

@JohnRDOrazio JohnRDOrazio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for spotting the inconsistency here — but I'd like to split this PR's two changes, because they don't pull in the same direction.

✅ Keep: moving the note below both grids

Repositioning "Repository fields are managed by the GitHub integration." so it sits after all four fields (instead of between the two grids) is a clear win — it now unambiguously reads as a caption for the whole group rather than just repo owner/name. No objection.

⚠️ Please drop: dimming the labels to opacity-60

These four fields are read-only but informational — they show the real owner / name / branch / path the user is meant to read and confirm, not disabled-empty inputs. Labels are never interactive, so making them "look disabled" doesn't communicate anything functional; it just makes the content harder to read. The note already explains why they can't be edited.

It's also a measurable accessibility regression. The labels are text-slate-700 (#334155), text-sm / font-mediumnormal text under WCAG, so the 4.5:1 AA threshold applies:

State Effective color on white Contrast AA (normal text, 4.5:1)
Full opacity (current) #334155 ~9.3:1 ✅ pass
opacity-60 (this PR) #858d99 ~3.35:1 fail (only clears the 3:1 large-text bar, which doesn't apply)

Dark mode (slate-300 over slate-800) takes a comparable hit.

Suggested change

Keep the note move; revert the four isWebhookDriven && "opacity-60" additions on the <label>s so they stay at full contrast.

Longer term, the cleaner way to signal a read-only field is to keep label and value fully legible and instead use a muted background + no focus ring + maybe a small lock icon — i.e. the opposite of fading the content. (The pre-existing opacity-60 on the inputs themselves is a mild version of the same issue, but that's out of scope for this PR.)

Happy to merge as soon as the label changes are dropped. 🙏

@JohnRDOrazio

Copy link
Copy Markdown
Member

Tracked the longer-term read-only field treatment (muted bg + lock instead of fading content) separately in #301 — explicitly out of scope here. This PR just needs the label opacity-60 reverted (note reposition stays) and it's good to merge.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants