Skip to content

feat(access-list): add optional note field to access list clients#5540

Open
TheMazeIsAmazing wants to merge 13 commits into
NginxProxyManager:developfrom
TheMazeIsAmazing:feat/notes
Open

feat(access-list): add optional note field to access list clients#5540
TheMazeIsAmazing wants to merge 13 commits into
NginxProxyManager:developfrom
TheMazeIsAmazing:feat/notes

Conversation

@TheMazeIsAmazing
Copy link
Copy Markdown

@TheMazeIsAmazing TheMazeIsAmazing commented May 14, 2026

Why

This PR introduces an optional “note” field for access list client rules to improve clarity when managing multiple IP entries. It allows users to annotate each rule (e.g. “Home” or “Office”) without affecting current behaviour.
(See feat-req: #5277)

Front-end change:
Screenshot 2026-05-14 at 14 03 49

Note added to the generated Nginx files (e.g. /data/nginx/proxy_host/X.conf):
image

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Code refactoring
  • API changes
  • Performance improvement
  • Test addition or update

AI Usage

  • AI was used to write this
  • AI was used to review this

Misc.

I'm sorry, this is a new version of #5333. Wanted to make sure it was up-to-date with current develop branch and resolve merge conflicts. Guess I did it wrong and accidentally removed my PR 🥲.

Mees Muller and others added 8 commits February 20, 2026 14:41
… widen modal

- Add optional note property to access list clients in backend model and schema
- Extend OpenAPI schemas (common + access-list endpoints) to support note
- Created necessary migration to add the note row to access_list_client table
- Persist note in access_list_client inserts/updates
- Render note in nginx config as inline comment for access rules (when present)
- Extend frontend AccessListClient model with optional note
- Add note input field to Access List modal (Rules tab)
- Only include note in payload when non-empty
- Add i18n placeholder for note field
- Increase Access List modal width for improved usability
@jc21
Copy link
Copy Markdown
Member

jc21 commented May 14, 2026

Code Review

Thanks for the PR and for keeping it up-to-date with develop. The feature is well-scoped and non-breaking — the nginx sanitization is correct and the localization coverage is thorough.

Blocking Issue

Global CSS override in frontend/src/App.css

.modal-dialog { 
    max-width: 700px !important; 
}

This widens every modal in the application, not just the Access List modal. Bootstrap's default is 500px. This needs to be scoped to only the Access List modal — e.g. via a dedicated CSS class applied through modalClassName or a wrapper class — to avoid a regression across all other dialogs.


Minor Issues

  1. Accessibility: The note <input> has no associated <label>, only a placeholder. Placeholders disappear on focus and are not a substitute for labels — a visually hidden label or aria-label would fix this.

  2. Note not visible in read view: The note only appears during edit. Users can't see annotations without opening the edit dialog or inspecting the nginx config file. Worth considering showing the note inline in the client list within the modal.

  3. Migration down is a no-op: The rollback function just logs a warning rather than dropping the column. This is an acceptable choice (dropping a column would destroy data), but it's worth a comment in the code marking it as intentional.

  4. Unrelated locale additions: Several locales (de, it, ja, pl, vi, zh) also add access-list.rule-source.placeholder (the IP address placeholder), which was previously missing from those files. Worth a quick sanity-check that those IP placeholder strings are correct since they weren't the original focus of the PR.


What's Good

  • client.note || null correctly coerces empty string to null before persisting
  • The nginx comment sanitization ([\r\n#;] → space) is correct — stripping newlines is the key safety measure
  • Frontend only includes note if non-empty after trimming (in AccessListModal.tsx)
  • Schema maxLength: 200 matches the DB column size

Fix the global CSS scope and this is good to go.

Comment thread frontend/src/modals/AccessListModal.tsx Outdated
@nginxproxymanagerci
Copy link
Copy Markdown

CI Error:

/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory
certbot-node: Pulling from nginxproxymanager/nginx-full
Digest: sha256:b258c49eb54f67430900b235e93a890605162a57f877c115c700a65783d9ede0
Status: Image is up to date for nginxproxymanager/nginx-full:certbot-node
docker.io/nginxproxymanager/nginx-full:certbot-node
�[1;34m❯ �[1;36mTesting backend ...�[0m
yarn install v1.22.22
[1/4] Resolving packages...
[2/4] Fetching packages...
warning lru.min@1.1.4: The engine "bun" appears to be invalid.
warning lru.min@1.1.4: The engine "deno" appears to be invalid.
warning sql-escaper@1.3.3: The engine "bun" appears to be invalid.
warning sql-escaper@1.3.3: The engine "deno" appears to be invalid.
[3/4] Linking dependencies...
warning " > @apidevtools/json-schema-ref-parser@15.3.5" has unmet peer dependency "@types/json-schema@^7.0.15".
warning " > mysql2@3.22.3" has unmet peer dependency "@types/node@>= 8".
warning " > @apidevtools/swagger-parser@12.1.0" has unmet peer dependency "openapi-types@>=7".
[4/4] Building fresh packages...
Done in 20.60s.
yarn run v1.22.22
$ biome lint .
Checked 89 files in 48ms. No fixes applied.
Done in 0.11s.
�[1;34m❯ �[1;32mTesting Complete�[0m
�[1;34m❯ �[1;36mBuilding ...�[0m
#0 building with "default" instance using docker driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 2.20kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/nginxproxymanager/nginx-full:certbot-node
#2 DONE 0.6s

#3 [internal] load metadata for docker.io/nginxproxymanager/testca:latest
#3 DONE 0.6s

#4 [internal] load .dockerignore
#4 transferring context: 2B done
#4 DONE 0.0s

#5 [testca 1/1] FROM docker.io/nginxproxymanager/testca:latest@sha256:14d7c3250135d3ebc058b1e8bb41b171f622b7cb5fc2b4b2bcc8f555cf926140
#5 resolve docker.io/nginxproxymanager/testca:latest@sha256:14d7c3250135d3ebc058b1e8bb41b171f622b7cb5fc2b4b2bcc8f555cf926140 0.0s done
#5 ...

#6 [internal] load build context
#6 transferring context: 3.92MB 0.0s done
#6 DONE 0.1s

#5 [testca 1/1] FROM docker.io/nginxproxymanager/testca:latest@sha256:14d7c3250135d3ebc058b1e8bb41b171f622b7cb5fc2b4b2bcc8f555cf926140
#5 sha256:9ab966394e7386f8e212deb17c9369ed98d5eb3a4df9070c28d96bae1362e810 116B / 116B 0.0s done
#5 sha256:29a151a3ba18b782725510791d49c081fb88a4f0633331fd0f826d36655fb78e 58.56kB / 58.56kB 0.0s done
#5 sha256:c99c8ac6013d99408e670c2d0ebeac923d390d90c09a55b4e0b266d0cc061e65 2.86kB / 2.86kB 0.0s done
#5 sha256:987ab97de7c8e28cebb6b45a2546298654c7d25b24720c371d04838152bb3e95 1.36kB / 1.36kB 0.0s done
#5 sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1 0B / 32B 0.0s
#5 sha256:59043673e5af8c5511ac545b23eb3e1d018bd25a54698cc89c668e6be7cd82e5 0B / 14.58MB
#5 sha256:f19dea81e0af6255cb52700a24ca2dec7ff42c12c20486f1bd6e7fac5c735db3 0B / 16.68MB 0.0s
#5 sha256:3882371c5807b6189df26b2c3139baab81e7ceee303ec493ac1c9f8445fb7171 0B / 9.38MB 0.0s
#5 sha256:b275842c598b60e8357d6da9be429887df83c85f3266eb8f6368291a03727348 0B / 3.91MB 0.0s
#5 ...

#7 [stage-1  4/11] RUN /tmp/install-s6 "linux/amd64" && rm -f /tmp/install-s6
#7 CACHED

#8 [stage-1  6/11] COPY frontend/dist /app/frontend
#8 CACHED

#9 [stage-1  7/11] WORKDIR /app
#9 CACHED

#10 [stage-1  8/11] RUN yarn install 	&& yarn cache clean
#10 CACHED

#11 [stage-1  3/11] COPY docker/scripts/install-s6 /tmp/install-s6
#11 CACHED

#12 [stage-1  5/11] COPY backend       /app
#12 CACHED

#13 [stage-1  9/11] COPY docker/rootfs /
#13 CACHED

#14 [stage-1  2/11] RUN echo "fs.file-max = 65535" > /etc/sysctl.conf 	&& apt-get update 	&& apt-get install -y --no-install-recommends jq logrotate 	&& apt-get clean 	&& rm -rf /var/lib/apt/lists/*
#14 CACHED

#15 [stage-1 10/11] COPY --from=testca /home/step/certs/root_ca.crt /etc/ssl/certs/NginxProxyManager.crt
#15 ERROR: short read: expected 32 bytes but got 0: unexpected EOF

#16 [stage-1  1/11] FROM docker.io/nginxproxymanager/nginx-full:certbot-node@sha256:b258c49eb54f67430900b235e93a890605162a57f877c115c700a65783d9ede0
#16 resolve docker.io/nginxproxymanager/nginx-full:certbot-node@sha256:b258c49eb54f67430900b235e93a890605162a57f877c115c700a65783d9ede0 0.0s done
#16 CANCELED
------
 > [stage-1 10/11] COPY --from=testca /home/step/certs/root_ca.crt /etc/ssl/certs/NginxProxyManager.crt:
------
Dockerfile:48
--------------------
  46 |     # add late to limit cache-busting by modifications
  47 |     COPY docker/rootfs /
  48 | >>> COPY --from=testca /home/step/certs/root_ca.crt /etc/ssl/certs/NginxProxyManager.crt
  49 |     
  50 |     # Remove frontend service not required for prod, dev nginx config as well
--------------------
ERROR: failed to build: failed to solve: failed to compute cache key: short read: expected 32 bytes but got 0: unexpected EOF

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants